Warn on captures?

[chriselrod]

Capturing variables in closures is problematic because it introduces non-local behavior and means that modifying one piece of code can result in changes in the behavior of code elsewhere, possibly (for example) resulting in code no longer being thread-safe.

The fact that variables can be captured also means that we cannot reason about the behavior of code just by looking at the immediate surroundings, e.g.

let
  function foo(x)
    y = bar(x)
    # do stuff
  end

  # do a bunch of stuff
  # naming the below variable causes the `y = ` above to become a capture, rending foo not thread safe
  # renaming `y = ` or moving the `y =` code outside of this scope fixes that problem.
  # or maybe the capture was intentional, in which case that would break the code
  # this is a nonlocal behavior that could be separated by dozens or hundreds of lines
  # these sorts of changes (introducing or removing a `y = `) happen frequently when refactoring
  # and authors may not always double check all variable names used in assignments.
  y = ...

end

Being warned of this would make this apparent. Having diagnostics next to the y = makes the behavior and connection between those two assignments apparent, so that it no longer requires any non-local information for the developer.

Captures also do not require type information. However, many of the problematic cases I've faced that resulted in incorrect code did involve macros (i.e. the closures or even some of the assignments were generated by macro expansions).

I've been discouraging the use of closures because they're foot guns that cause unexpectedly bad performance or incorrect code that produces wrong results or errors sporadically, without having any actual upside. But the foot guns should be easy for the LanguageServer to find (at least, if it can expand macro) and are easily fixed, e.g. via just prefacing assignments with local inside the closures.

[davidanthoff]

Hm, I'm not sure. At most I think we would have this opt-in, right? After all, that is legal Julia code.

[pfitzseb]

We may need to start thinking about a eslintrc style config file for linting (although I guess having workspace-specific settings would also work for VS Code at least).

[davidanthoff]

I like the idea of a separate config file better, that integrates better with some potential future CI stuff, right?

[pfitzseb]

Yup. Also has the advantage of working with non-VS Code editors.

[chriselrod]

Hm, I'm not sure. At most I think we would have this opt-in, right? After all, that is legal Julia code.

This is also legal Julia code:

function foo(x)
    y = x + 1
    return x + 1
end

yet it warns that y is an unused variable.

I think the number one problem with capturing variables in a manner that they get Core.Boxed is that it isn't necessarily obvious when this is happening; that changing code hundreds of lines below can suddenly impact the correctness of the program above.

Being aware would let people decide if that is what they intended / if it is what they intended, make sure it is actually happening.

Defaults matter. The vast majority of people will use the default settings.

Personally, I'd also want it to warn about using non-const globals by default. These are things that are not obvious to new comers; having such things helpfully pointed out is much nicer than being oblivious.

I think helpful linting can go a long way toward reducing the discipline and expertise required to write good Julia code; a lot of the performance tips are things I think it could theoretically spot.

Although I do think there should be a distinction between warnings over invalid code vs helpful hints/providing users with more information/feedback as they write code.