Closed zachallaun closed 2 months ago
Few TODOs on this:
*_test.exs
files should get this treatment. For simplicity, I think they should. The approach outlined above should not be a performance issue because we entirely skip over AST wrapped in a defmodule
, which is almost the entirety of every test file.Is use
really safe? It's probably uncommon but the underlying __using__
macro could very well have side effects.
Is
use
really safe? It's probably uncommon but the underlying__using__
macro could very well have side effects.
I considered that. I think it's extremely uncommon for a top-level use
to have anything resembling dangerous side effects. Much more common, and the case I'd like to support if possible, is "bag of aliases/imports" where use Something
is a sort of short-hand for importing Something.Foo
, Something.Bar
, aliasing Something.Baz
, etc.
In the same vein, defmodule
isn't really safe, since top-level forms inside it are also evaluated. So if you put silly stuff in the top level of your modules, silly stuff will happen when you compile it.
I went ahead and implemented the approach I suggested in the PR to fix incorrect unused/undefined variable warnings. Unfortunately, it makes the code rather more complicated, but I've added some unit tests for the stickier bits that will hopefully also help the next person who works on this understand what's going on.
Still very open to other approaches, but I couldn't think of any.
Re: issue 2, I think the use case that won't work is exceedingly rare, and I'm fine not supporting it.
Re: issue 2, I think the use case that won't work is exceedingly rare, and I'm fine not supporting it.
@scohen 👍 I'll cross that off the list.
The only thing to decide on now is whether to exclude tests.
My opinion is that we shouldn't. I think the best argument for excluding them from the ".exs treatment" is that the treatment might be buggy, but:
Agree, let's give it a shot. This makes working with scripts possible. It's so funny that I was literally working on benchmarks and becoming frustrated with lexical being "slow", when it was just running the benchmark.
@zachallaun is this ready to merge?
Lexical's as-you-type compilation and error reporting works by parsing the AST after a small period of quiescence and then compiling it with
Code.compile_quoted/2
. This runs top-level forms, however, so a.exs
script that performs side-effects when run, e.g. creating or deleting a file, would have them run repeatedly unless you wrap them in adef
inside a module.The goal, then, becomes reporting on as many errors/warnings as possible without running top-level forms.
The current approach here is to chunk all top-level forms into "safe" and "unsafe", where safe forms are only
defmodule/use/import/require/alias
. Safe forms stay at the top level, while unsafe forms are wrapped to prevent their execution. Concretely, this script:would be transformed to this after parsing, but prior to compilation:
Issue 1: Incorrect unused/undefined var warnings
This isn't quite good enough, though. It means that if you, for instance, set some constants in your script at the top, you'd get warnings for any usage below if they're separated by a "safe" form. For instance:
This would result in a warning where
dir
is assigned (unused variable) and an error wheredir
is used (undefined variable).Potential solution: collect vars and references in each chunk of top-level code, replacing unused vars with
_var
. Then, for any later chunks, use those vars to build "stub parameters" in the wrapper def to avoid undefined variable errors.Issue 2: Using top-level var as a module name
The approach above will break this code:
I'm personally okay with this as a "known defect" that perhaps could be tackled later on, though I'm not sure a perfect solution exists.
Things this explicitly doesn't address
Mix.install
, which will require wider changes in order to support. Any usage of deps coming from theMix.install
will show "undefined" warnings. To actually support it, we'd need to consider that file its own project, because install doesn't work inside an existing mix project. I think this will be easier to support when we support multiple workspaces.