[ANN] ShowLint

rikh · December 20, 2020, 7:14pm

Hi all,

TLDR: I made a Julia linter which shows rewrite suggestions online. At the time of writing it analyses 128 Julia repositories (about 1,012,728 lines of code) and shows the diffs.

I was recently watching a video of SourceGraph Campaigns and found it super cool. Basically, it allows you to apply a transformation to many repositories automatically. It would be especially nice if you could rewrite, for example, all occurrences of findfirst(a, b) === nothing to !occursin(a, b).

Anyway, Sourcegraph pointed me to an awesome rewrite tool, namely Comby. For a brief moment, I considered building a replacement for Lint.jl. However, then everyone has to install that tool and keep it updated. So, now I created some rewrite rules, run them over many repositories and show the diffs online. This, for example, can be useful for spotting coding mistakes, make code more readable by simplifying it, and can help in the transition after the release of the new Julia LTS.

You can see the source code at https://github.com/rikhuijzer/ShowLint and the replacement patterns and diffs at https://lint.huijzer.xyz.

PRs for extra repositories or rewrite rules (patterns) are welcome!

tlienart · December 20, 2020, 8:30pm

Actually this is pretty nice, I wonder whether you could also automatically

Link to relevant docs for your suggestion
Open a PR automatically against the base repo (I guess maybe first you’d have to ask people if they’re happy to receive such PRs)

But having a centralised place for “poor patterns that could be improved” can be quite nice for devs to learn new stuff too

Mason · December 20, 2020, 8:32pm

Sorry if this is off topic, but why suggest turning x === missing to ismissing(x)? Especially when it seems to be fine with x === nothing?

rikh · December 20, 2020, 9:02pm

@tlienart Thanks! I’ve added an issue for your first suggestion and am going to improve on that. For your second suggestion, I think that’s not possible yet. The site shows too many false positives. So far, I did find some interesting problems in code and created PRs manually:

@Mason that’s not off topic. Are you sure that x === missing is the same as x === nothing? I’m not an expert, so it could definitely be true. Would you mind elaborating a bit more?

Mason · December 20, 2020, 9:12pm

Both missing and nothing are defined as Missing() and Nothing() respectively, and ismissing and isnothing are both defined as

ismissing(::Any) = false
ismissing(::Missing) = true

and

isnothing(::Any) = false
isnothing(::Nothing) = true

respectively, so as far as I can tell, they’re exactly analogous.

I believe it’s usually recommended to use x === nothing and x === missing when possible because the compiler is able to optimize it in more circumstances. From what I understand, ismissing and isnothing are mostly useful for higher order functions, e.g.

any(ismissing, x)

rikh · December 20, 2020, 10:20pm

I do expect that ismissing(x) is easier to read for new Julia users, and performance is not really the issue for most practical problems („premature optimization is the root of all evil“), but you might very well be right that its not an useful pattern to check for. I‘m gonna look into it a bit more.

greg_plowman · December 21, 2020, 12:46am

Maybe these are relevant:

rikh · December 21, 2020, 1:30pm

I’ve opened a topic at Ismissing(x) versus x === missing.

Wikunia · December 21, 2020, 2:27pm

That’s pretty dope and I’m happy that you analysed two of my repos
How long does it take to produces the results? Is it out of reach to do this for all registered packages?

rikh · December 21, 2020, 2:33pm

Haha, you’re welcome

Currently, it takes about 20 minutes (https://github.com/rikhuijzer/ShowLint/actions), so it’s not too bad. Good idea. I’ve put it in my task list (https://github.com/rikhuijzer/ShowLint/issues/11).

rikh · December 21, 2020, 2:44pm

@joa-quim In response to https://discourse.julialang.org/t/ismissing-x-versus-x-missing/52171/3: That’s why I’m working ShowLint with the possibility to disable lint patterns in bulk. Instead of adding a lint configuration to each project/repository, I could just disable the check for this pattern and similar checks on all “high-performance” repositories. Basically, the idea is that all the configuration complexity is moved away from the user and is managed at one place. Also, the idea is that its easier for the Julia community to help busy package creators out. For example, some packages are created one or more years ago and the creator has moved on to other things. With something like ShowLint, the community can more easily assist these creators in keeping their packages working and up-to-date.

Wikunia · December 21, 2020, 2:50pm

Oh and maybe only lint the src folder as something like a == a can be helpful in testing to check if the function for == is correctly implemented.

rikh · December 21, 2020, 3:00pm

You’re quick. It took me until this morning to realize that https://github.com/rikhuijzer/ShowLint/issues/9

GunnarFarneback · December 21, 2020, 3:23pm

Comparison with Boolean constants are only redundant if you can prove that the value itself is a Bool. And b == trues(n) is not equivalent to bs(n).

Wikunia · December 21, 2020, 3:27pm

Haha I didn’t look at the issues though I saw that you changed some a == a in the latest run. Have you tackled the false positives in JuMP.jl for example? There you have @constraint(m, x-1 == 1) and form it into @constraint(m, x-true)

rikh · December 21, 2020, 3:40pm

@GunnarFarneback Yes, I’m still tweaking the rules and am note sure what to do with it. I do think that https://github.com/JuliaData/YAML.jl/pull/103 shows that there are situations where that check can be useful but, indeed, its a part of Julia since x == true will give missing if x is a missing.

@Wikunia I got some tips from Rijnard van Tonder (Comby’s creator) and expect to have that fixed later today

jakobnissen · December 21, 2020, 4:08pm

This is great. I’d love to see more linting in Julia. A few points:

a == a appears to also trigger on 2 * a == a * 2 and even x/a == a, which should not be intended.
x -> f(x) triggers on x -> 2gx(x), suggesting 2gx, which is not corret

rikh · December 21, 2020, 4:27pm

Thanks for both. Good catch on x/a == a. Should be fixed somewhere today.

mcabbott · December 21, 2020, 4:37pm

Looks like a neat tool!

My few points:

The replacement for findall(x -> x==false, [true, false]) should probably be findall(!, [true, false]) rather than broadcasting.
But replacing cond == false with cond only seems safe if you are sure that cond::Bool. I don’t see examples where this is clearly violated, but I do have functions which expect either nothing or false. And findall(!, [true, false, nothing]) obviously won’t work.

rikh · December 21, 2020, 6:07pm

Thanks for your suggestions!

I’m afraid that I don’t understand what would be the difference between findall(!, A) and findall(.!A), and why the former would be preferred. Could you explain the difference?

So, this is now the second pattern which doesn’t handle missings well. Thanks for pointing that out