21

I'm not sure we already have something similar, but I'm working on more code inspections for the IntelliJ plugin and it's always a good idea to ask the community. Since it doesn't really fit on main, I'm posting it here on Meta.

Linting is an excellent way to point the developer to probable errors that he might have overlooked. With a dynamic language like the one of Mathematica, we are a bit restricted with what we can do, since we cannot evaluate code and since most things require evaluation to be sure if they are a bug or not. Nevertheless, there are checks we can do. For instance If[a=b, ..] is most likely a bug and even if the developer knew what he did, it is a bad style.

There are trickier examples like If[a<5,...]. This looks okay but knowing that a<5 stays unevaluated if the comparison cannot be done, it is a source of error because you end up with the unevaluated If expression in your wrong result and debugging might be complicated.

In both examples, wrapping TrueQ around the condition resolves the issue and although there might still be a bug, at least you can be sure your If expression is evaluated to some branch. Other common sources of error are, e.g. x_?testFunc[#]& or implicit multiplication through linebreaks.

Question: What are common bugs in your code and could they have been pointed out by a linter? If you like to share your thoughts, please provide one issue per answer, so that others can vote. I'm looking forward to your suggestions and see if I can implement some of them in IntelliJ.


Example issue: With the alternative layout for packages which was pointed out by Leonid, we can use directives for a static code analyzer to easily export symbols or declare them as package symbols. As Leonid pointed out, the directives need to be on their own source-line with nothing else on it. So for the directives

PackageScope["myFunc"]
PackageExport["MyExportedFunc"]

I implemented the following rules

  1. They need to be on their own source line with nothing else on it
  2. Their string argument must be a valid identifier

enter image description here

  • 5
    I can't really think of any off the top of my head, but one thing worth contemplating is whether it is necessary to only include bugs or whether it's also worth adding best-practices suggestions (like making sure each a ::usage message starts with the name of the symbol it's being defined for or something) – b3m2a1 Feb 24 at 3:05
  • 2
    @b3m2a1 I actually have tests in my test suite to verify that each usage message starts with the name of the symbol that it belongs to. It would indeed be useful for the IDE to check this. – Szabolcs Feb 25 at 8:41
  • 1
    @Szabolcs yeah I thought of it because I never do it and am always like like "wow I should really be doing this..." – b3m2a1 Feb 25 at 8:42
  • Are lower case Symbol names for trig functions and sqrt detected? These seem to slip in out of habit for even non-novice Mathematica users from what I have observed. – Mr.Wizard Feb 26 at 17:27
  • How about this? posVec_List ? AllTrue[Positive] The intention is to write a pattern for a vector containing all positive numbers. But ? actually binds even tighter than [ ... ]! It must be written as posVec_List ? (AllTrue[Positive]). I do not fall for x_?testFunc[#]& anymore but I did fall for this one a few times. – Szabolcs Feb 27 at 9:15
  • 1
    @Mr.Wizard The plugin already does that because it tries to resolve all used symbols (i.imgur.com/SlGvVRC.png). So if you haven't defined sqrt yourself, then this will always trigger a warning. – halirutan Feb 27 at 9:19
  • @Szabolcs Should be a small fix on the existing inspection. – halirutan Feb 27 at 9:19
  • Another small idea: warn about nested pure functions with Slot-arguments, like #^2& /@ Join[#,#]&. Many people would prefer to turn such an inspection off, and even those of us who strive to never write such code might rarely benefit from it (as it's relatively easy to avoid doing this). It might be useful when refactoring others' code (I think @Kuba does that a lot). I'd suggest low priority if you consider it at all. – Szabolcs Feb 27 at 9:29
  • I need to leave now but we might find good tips in Fateman's original criticism paper: people.eecs.berkeley.edu/~fateman/papers/mma.review.pdf – Szabolcs Feb 27 at 9:32

11 Answers 11

13

1. Invalid number of arguments for some of built-in functions e.g.

  • Which called with an odd number of arguments

  • Switch called with an even number of arguments

  • Module/With/... called with more than two arguments


    I don't think it needs to work for every built-in function but will probably be useful for scoping and conditional expressions.

2. Backtick in Begin and friends

  • Begin called with "Private`" instead of "`Private`"

    etc.


Implemented

img


Oh, I almost forgot my personal number one problem:

3. ClearAll @ foo

when it is called after options/attributes or any definition for foo was defined.

4. Need for explicit Optional.

This gives a syntax error, it is debatable but certainly can be unexpected:

    {_} : {1}
  • 1
    Excellent suggestions. I already thought about 1. more than once and although I have the information for most built-in functions, I believe it would be too annoying to the experienced developer to see such warnings when arguments are regularly zipped together with Sequence, ##, With, and whatnot.. For a selected set of scoping functions, this makes sense though. The backtick bug is also nice, but when someone wants to begin a context without leading backtick, he cannot get rid of the warning except turning the whole inspection off. I have to think about that. – halirutan Feb 25 at 1:23
  • The last example is nice, but it requires to resolve all definitions for foo before the ClearAll which might be too slow for a fast local inspection. On the other hand, there are usually not so many ClearAll in package code and this might be worth a shot. – halirutan Feb 25 at 1:25
  • @halirutan 1. If you can detect and ignore cases with ## then I think it should be done. Dealing with With injecting sequences is probably too tough. At the end MMA allows to much freedom with respect to syntax to care about such cases. User can disable this feature or it can be disabled by default if needed. 2. Only for hardcoded "Private`" then, 95% chance it was a mistake. 3. I'd say ClearAll should not be present inside implementation part, it happens for me because I deal a lot with others code so I sometimes forget to remove it. ... – Kuba Feb 25 at 7:47
  • @halirutan ... It should be a best practice to only ClearAll["`*", "`*`*] on top and every other case should be flagged. Again, user can disable this. – Kuba Feb 25 at 7:47
10

Status Completed

It would be neat if IntelliJ could detect this precedence issue:

Plot3D[Sin[x y], {x, 0, 3}, {y, 0, 3}, ColorFunction -> Hue[#3] &]

it should be

Plot3D[Sin[x y], {x, 0, 3}, {y, 0, 3}, ColorFunction -> (Hue[#3] &)]

This may not be as much of a problem in an IDE as in a notebook, but nevertheless:

{1, 2, 3}/.2 -> 3

There is a missing space between /. and 2.

Comment halirutan:

Excellent suggestions and I implemented both. Anonymous function in IntelliJ are italic and it is easier to see the scope, but I included this inspection anyway

function

The second inspection has some corner-cases, but a basic version is also available now

replace

5

These are really common mistakes that go unnoticed because they are valid syntax:

  • Which statement using _ as the final predicate
  • Switch statement using True as the final pattern
  • A list with a random dot product in the middle, usually means someone typed a period instead of a comma in a long multi-line list
  • The Which one screws me over daily... – b3m2a1 Mar 6 at 4:07
4

I agree with @CE that precedence issues would be great to point out, and maybe suggest to the programmer to use more parentheses. The relative precedences of @, @@, @@@, /@, /., //., etc. and the scopes of any associated slot operators #, ##, etc. are sometimes difficult to track; can't expect anyone to remember the full Operator Precedence Table. I tend to use the CTRL-. selection trick to group-select according to precedence (that would also be great if you don't yet have it in IntelliJ); but complex cases still exceed my mental capacity.

For example, quick, without looking it up, does

f /@ {1, 2, 3} /. i_ -> i^2

give {f[1]^2, f[2]^2, f[3]^2} or {f[1], f[4], f[9]}? In my opinion such edge cases deserve a lint warning.

Comment halirutan: As said in the comment, structure-based selection already worked for a long time and you can hit Ctrl+W to expand or Ctrl+Shift+W to shrink the selection

img

For highly complex expressions, rather than making a warning message which probably would annoy experienced users, maybe a "Can you parenthesize this expression for me"-action would be more convenient. With this, the user can quickly check how an expression is parsed

img2

The API for these "Inlay Markups" is still experimental, but I like it very much because it doesn't change any code. It simply lets you add info that is rendered in the editor and can be disposed at any time.

  • Yes, IntelliJ also has structure based selection of expression. The algorithm for computing some kind of operator complexity measure is too time-consuming for a "local inspection", but something like that can be implemented as global inspection which is not run after each key-stroke. I believe it should not be too hard to implement, but I need to think about this some more. However, you gave me another great idea and I need to read some IntelliJ code to see if I can make this work. – halirutan Feb 25 at 2:28
  • @halirutan - if you could make that work it would be fantastic. That is one of the only reasons I still use Eclipse, that I can keep clicking an expression and the selection expands based on scope. – Jason B. Feb 27 at 18:21
  • @JasonB. The structure-based selection worked almost from the beginning of the development years ago (i.stack.imgur.com/P0bcU.gif). The difference is that it is bound to Ctrl+W for increasing and Ctrl+Shift+W for decreasing the selection. The repeated click has a standard meaning in idea: First select token unter the cursor, then select the whole line. – halirutan Feb 27 at 19:02
4

Status Completed

I often accidentally include a ; at the end of a line in the variable declaration for Module/Block/DynamicModule/etc or screw them up somehow. It'd be nice if IntelliJ could catch fundamental, silly errors like that.

E.g. I do this all the time:

Module[{a, b, c; d=10, f, 12},  
  ...
  ]

And presumably both of those would be pretty easy to do for the IDE

Comment halirutan: I have implemented this. Here you see 3 warnings

img

The first warning on a is because it is not used inside the Module body. The other two warnings are because it is not a valid variable declaration or assignment.

This will be published in version 2019.1.2

2

I'm not sure if this qualifies as a separate claim related to operator precedence. I include it as an answer simply to facilitate the discussion format, even though I believe it should be a comment, instead.

I consistently have had a hard time with expr//f/*g when g is an anonymous function (ie defined 'then and there' like eg f/*#^2&). The issue becomes more complicated when more functions are involved or when the complexity of the functions involved, increases.

A close second, which is probably too idiosyncratic or personal, has to do with confusing Options with Attributes and using SetOptions (like one would have used SetAttributes) when defining new functions. Obviously, using SetOptions instead of Options on a symbol that has no default Options causes a SetOptions::optnf message.

  • Can you elaborate on what exactly you mean by "I consistently have had a hard time"? Do you mean that the anonymous function also captures the f/* part, right? – halirutan Feb 27 at 19:15
  • yes, I keep consistently forgetting that when I intend eg. x//f/*#^2& to evaluate to f[x]^2 I need to use parentheses as in x//f/*(#^2&) because & binds tighter than /*. – user42582 Feb 27 at 20:07
2

Status Completed

Invalid attributes

The Workbench has a feature where it will warn about invalid attributes in constructs similar to

Attributes[symbol] = {attr1, attr2, ...};

enter image description here

It does not at this point warn about wrong attributes in SetAttributes though.

I think this would be a useful inspection because it is not an uncommon mistake to try to use Hold instead of HoldAll, or HoldComplete instead of HoldAllComplete.

This should not be of high priority because:

  • Attributes[foo] = ... will typically appear at the top level and get evaluated at package load time.

  • Mathematica will show an error message when trying to set a wrong attribute.

Thus this error won't go unnoticed for long even if the IDE doesn't warn about it.


Comment halirutan: I have implemented this

img

It will be available in the WL Plugin version 2019.1.2

2

Status Completed

I'll convert my comment as this came up again:

This is already handled:

_?foo[#]&

But with the many operator forms, now this is a common mistake:

_?foo[12]

For example,

f[x_?GreaterEqualThan[5]] := Sqrt[x-5]

IDEA should warn about this.

Comment halirutan: Very good catch! I fixed this and it should now also report such operator forms

img

Will be available in WL Plugin version 2019.1.2

2

Status Partially Completed

Unused variables

fun[x_, y_] := x^2

        ^ y is unused
Module[{x, y},  <-- y is unused
  x+1
]
y=1;  <-- this instance of y is effectively unused
y=2;

The last one is quite involved and may be difficult to implement, therefore I won't give more examples of this type unless you ask.

Generally, the idea is that if a localized variable (Module, Block, With or function arg) is unreferenced within the local scope, there should be a warning.

Cases to exclude:

  • Rule, as it is typically used with explicit pattern matching, where naming subpatterns often aids readability even if their value isn't used.
  • Patterns like fun[list : {x_, x_}] := ... where a subpattern needed to be named to indicate repetition.

UPDATE:

It may be best to add this for Module and With only for the time being. Block has too many reasonable uses where the localized variables do not need to appear explicitly, e.g. Block[{$RecursionLimit = ...}, ...] or setting other flags which are user-defined, but affect the Block body in a similar way.


UPDATE 2:

The Workbench has this feature:

enter image description here

Comment halirutan: This inspection has been implemented and is available since WL Plugin version 2019.1

1

Non-ASCII characters

Add an inspection to warn about non-ASCII characters that appear anywhere else except in comments.

The encoding that Mathematica will assume when loading a file using Get is not predictable and typically differs between operating systems. E.g., on macOS/Linux it might assume UTF-8 while on Windows it may assume Windows-1252. If the source file contains anything but ASCII, it might be interpreted differently on different platforms.

Such characters will typically appear in strings such as usage messages. I personally encountered this:

https://github.com/szhorvat/IGraphM/issues/44

I used non-ASCII characters in my usage messages when I edited the source files on macOS and on Windows they appeared as Chinese characters.

Note: I would exempt comments from this inspection because misinterpretation won't usually cause problems there. In principle it could, in particular if it affects the closing *) (note that in my example above an extra ASCII letter was corrupted), but usually it won't. I expect many people will want to write comments in their native language.

Comment halirutan: I've implemented this and created a mapping to all known named characters so that I can provide a QuickFix which is able to replace the Unicode character automatically.

enter image description here

1

Things like

f[a_Association]:=...

Should ask if you want to convert to

f[a_Association?AssociationQ]:=...

And similarly

f[a_?AssociationQ]:=...

Should ask if you want to do the same

The longer pattern is the fastest, most robust one, for most pattern types:

f[a_Association?AssociationQ] := True;
g[a_?AssociationQ] := True;
h[a_Association] := True;

a = Association[1];
b = <||>;
c = Dogs;

RepeatedTiming[f[a];, 1] // First
RepeatedTiming[f[b];, 1] // First
RepeatedTiming[f[c];, 1] // First

1.2*10^-6

1.2*10^-6

4.7*10^-7

RepeatedTiming[g[a];, 1] // First
RepeatedTiming[g[b];, 1] // First
RepeatedTiming[g[c];, 1] // First

1.2*10^-6

1.2*10^-6

9.3*10^-7

RepeatedTiming[h[a];, 1] // First
RepeatedTiming[h[b];, 1] // First
RepeatedTiming[h[c];, 1] // First

7.2*10^-7

6.9*10^-7

4.3*10^-7

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .