Add expand_imports to resolve bare and except imports#5
Conversation
9c4903e to
be71b0b
Compare
621eeb6 to
6d0bda7
Compare
dannote
left a comment
There was a problem hiding this comment.
Thanks — this is a useful direction, and keeping it opt-in is the right default. I found a few correctness issues around import scope and only: :functions / only: :macros semantics that I think need changes before merge.
| end) | ||
|
|
||
| aliases | ||
| if Keyword.get(opts, :expand_imports, false) do |
There was a problem hiding this comment.
This currently makes expand_imports file-global: imports are collected once for the whole AST and then used while matching every node in the file.
I validated this false positive:
defmodule A do
import Enum
def run(list), do: map(list, & &1)
end
defmodule B do
def run(list), do: map(list, & &1)
endWith expand_imports: true, Enum.map(_, _) matches both calls. The import from A leaks into sibling module B. I think this needs lexical/module scoping before the option is safe.
| imports = Map.get(aliases, {__MODULE__, :imports}, []) | ||
|
|
||
| if Enum.any?(imports, &resolvable?/1) do | ||
| local = local_definitions(ast) |
There was a problem hiding this comment.
local_definitions(ast) is also file-global, so local shadowing can leak across sibling modules.
I validated this false negative:
defmodule A do
def map(a, b), do: {a, b}
end
defmodule B do
import Enum
def run(list), do: map(list, & &1)
endWith expand_imports: true, Enum.map(_, _) returns no matches because A.map/2 suppresses expansion in B. Shadowing should be scoped to the module/function context where the call appears.
| case opt_value(opts, :only) do | ||
| nil -> :all | ||
| nil -> import_except(opts) | ||
| only_ast -> parse_only_list(only_ast) |
There was a problem hiding this comment.
only: :functions and only: :macros collapse to :all, and expand_imports then resolves :all to functions + macros. That changes Elixir import semantics.
I validated both directions:
import ExAST.Query, only: :functions
where(query, expr)incorrectly matches ExAST.Query.where(_, _), even though where/2 is a macro.
import ExAST.Query, only: :macros
from(pattern)incorrectly matches ExAST.Query.from(_), even though from/1 is a function. We probably need to preserve whether the unresolved import is functions-only, macros-only, or both.
The bare-import fix left `import Enum` matching nothing: with no `:only`
list there's no way to tell which local calls came from the import. This
adds an opt-in `expand_imports: true` that loads the module and uses its
real exports as the membership list, so a bare `import Enum` makes
`map(a, b)` match `Enum.map(_, _)`.
find_all(source, "Enum.map(_, _)") # 0
find_all(source, "Enum.map(_, _)", expand_imports: true) # 1
It also covers `except:`, which has the same problem — `import Enum,
except: [map: 2]` only makes sense once we know the full export list, so
it resolves to the exports minus the excluded names.
Functions the module defines itself shadow the import and are dropped, so
a local `def map/2` is never rewritten to `Enum.map`. When the module
isn't loadable we fall back to no expansion.
Off by default; the extra export walk only runs for files that have a bare
or `except:` import, so the common path is unaffected.
expand_imports collected imports and locals once for the whole file, so
an import in one module leaked into its siblings and a local def in one
module shadowed the import everywhere.
defmodule A do
import Enum
def run(l), do: map(l, & &1)
end
defmodule B do
def run(l), do: map(l, & &1)
end
find_all(src, "Enum.map(_, _)", expand_imports: true) # was 2, now 1
Imports now carry the module they were declared in, and matching scopes
them to the node's module path (a nested module still inherits an outer
import). Local shadowing moved to match time, keyed by the call site's
module, so a `def map/2` in one module no longer suppresses the import in
another — and a local in a nested module shadows an outer import only
within that module.
only: :functions / :macros also collapsed to :all, which then resolved to
functions + macros. They now keep their kind, so only: :functions never
matches a macro and vice versa.
6d0bda7 to
8ca804f
Compare
|
@dannote thanks for the review! Brought the edge cases into the tests, and made a few changes.
|
|
Thanks, the behavior issues look fixed now. Remaining blocker is CI: Credo flags |
Expands #4 to enable bare import resolution, if you think this path makes sense for ex_ast.
The bare-import fix left
import Enummatching nothing: with no:onlylist there's no way to tell which local calls came from the import. This adds an opt-inexpand_imports: truethat loads the module and uses its real exports as the membership list, so a bareimport Enummakesmap(a, b)matchEnum.map(_, _).It also covers
except:, which has the same problem —import Enum, except: [map: 2]only makes sense once we know the full export list, so it resolves to the exports minus the excluded names.Functions the module defines itself shadow the import and are dropped, so a local
def map/2is never rewritten toEnum.map. When the module isn't loadable we fall back to no expansion.Off by default; the extra export walk only runs for files that have a bare or
except:import, so the common path is unaffected.