fix(elixir): scope import/use to the declaring module, not the file - #4058
Ayushraj06-bit wants to merge 1 commit into
Conversation
The unqualified-call scope added for Graphify-Labs#4001 collected every `import` and `use` in a file into one list, so an import in one module leaked into a sibling module defined in the same file: `MyApp.Sibling.go/1` calling `fmt(x)` still bound to `MyApp.Helpers.fmt/1` because `MyApp.Importer` imported it. Elixir scopes `import`/`use` to the module body it appears in and to the modules nested in it. Record targets per enclosing module, with the file's top level as its own scope, and build each call's scope from its own module, the modules enclosing it, and the top level. Each raw call now gets its own list rather than sharing one mutable list with every other call in the file. A module nested in a same-named module shares its node id, so the walk up the enclosing chain stops at a module it has already visited. Follow-up to Graphify-Labs#4015. Refs Graphify-Labs#4001
|
Thanks for the pull request, @Ayushraj06-bit. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. PR-changed functions: 1/2 verified (0 proven, 1 may-equivalent, 0 distinguished) · 1 not verified (1 vacuous).
Not verified on this run: extract (vacuous: never exercised).
Graphify review — findings
Scopes Elixir import/use targets to the module that declares them and the modules nested inside it. extract_elixir now records targets per enclosing module and walks the parent chain for each caller, so an unqualified call in a sibling module in the same file no longer resolves through another module's import. File-level imports still apply to everything in the file.
Worth a look
- Module-body calls may miss their own module's import/use targets —
graphify/extractors/elixir.py:280· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2370 functions depend on the 288 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 753 callers, 50 callees - new:
_rebuild_code()— 149 callers, 56 callees - new:
extract_js()— 87 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
main()— 99 callers, 3 callees - new:
dispatch_command()— 2 callers, 127 callees - new:
_get_extractor()— 27 callers, 6 callees - new:
collect_files()— 19 callers, 6 callees - …and 37 more — each is listed as a finding
Verification — 2370 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 2185 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
142 of 326 test file(s) selected (44%) via static blast radius.
tests/test_astro_extraction.py— impacttests/test_astro_import_ids.py— impacttests/test_blade_extractor.py— impacttests/test_build.py— impacttests/test_builtin_global_type_refs.py— impacttests/test_case_sensitive_resolution.py— impacttests/test_cjs_module_extension.py— impacttests/test_cobol_extractor.py— impacttests/test_cpp_method_declarations.py— impacttests/test_cpp_nested_and_cli.py— impacttests/test_cpp_objc_cross_file_calls.py— impacttests/test_cross_extension_reexport_self_cycle.py— impacttests/test_cross_language_call_resolution.py— impacttests/test_cross_repo_external_call_guards.py— impacttests/test_cross_repo_member_calls.py— impacttests/test_csharp_call_site_generic_args.py— impacttests/test_csharp_enum_members.py— impacttests/test_csharp_field_generic_args.py— impacttests/test_csharp_generic_callsites.py— impacttests/test_csharp_interface_dispatch.py— impacttests/test_csharp_member_calls.py— impacttests/test_csharp_member_nodes.py— impacttests/test_csharp_object_creation.py— impacttests/test_csharp_partial_classes.py— impacttests/test_csharp_tuple_type_refs.py— impacttests/test_csharp_type_resolution.py— impacttests/test_definition_file_portability.py— impacttests/test_detect.py— impacttests/test_dotnet.py— impacttests/test_duplicate_annotation_edges.py— impacttests/test_elixir_import_resolution.py— impacttests/test_elixir_unqualified_call_scope.py— impact, changed-testtests/test_erlang_extractor.py— impacttests/test_extract.py— impacttests/test_extract_cache_location.py— impacttests/test_extract_php_closures.py— impacttests/test_file_label_disambiguation.py— impacttests/test_file_node_id_spec.py— impacttests/test_forwarding_review_findings.py— impacttests/test_go_builtin_call_targets.py— impacttests/test_go_import_repoint.py— impacttests/test_go_interface_methods.py— impacttests/test_go_qualified_resolution.py— impacttests/test_import_extension_resolution.py— impacttests/test_import_self_loops.py— impacttests/test_imported_export_forwarding.py— impacttests/test_incremental.py— impacttests/test_indirect_call_arrow_single_param_shadow.py— impacttests/test_indirect_call_block_scoped_shadow.py— impacttests/test_indirect_call_catch_binding_shadow.py— impact- … and 92 more
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify extract.
The verifier did not have enough to check extract, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 90 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly TypeError — names the real obstacle, not a sampling gap)
No difference found (not proven): No behavior difference found in extract\_elixir (not a proof).
The verifier ran both versions of extract\_elixir on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 45 more finding(s) on lines outside this diff (see the check run).
|
Shipped in v0.9.76 (live on PyPI as Closing as shipped. |
What does this PR do?
Follow-up to #4015 (shipped in 0.9.75). It fixes the finding the graphify-labs review raised on that PR: "One mutable call-scope list is shared by every module and call in a file".
The finding is real. #4015 collected every
import/usein a file into one list, so an import in one module leaked into a sibling module in the same file. Elixir scopesimport/useto the module body it appears in and to modules nested in it. Repro onv8(48d7c0e):The extractor now records
import/usetargets per enclosing module, with the file's top level as its own scope. Each call'selixir_call_scopeis built from its own module, the modules enclosing it, and the top level. Each raw call also gets its own list instead of sharing one mutable list with every other call in the file. Separately, a module nested in a same-named module (defmodule Foo do defmodule Foo do) gets the same node id, so the walk up the enclosing chain stops at a module it has already visited. Without that guard, extraction of such a file never terminates (checked: it hangs until killed).Nothing changes in the shared resolver beyond its comment. The candidate side, which keeps candidates whose file defines an in-scope top-level module, is the same as in #4015.
Type of change
Verification & Invariants
Invariant: an unqualified Elixir call resolves across files only to a module imported or used in its own lexical scope, never one imported by a sibling module in the same file.
How was this tested?
New test
test_import_is_scoped_to_the_module_that_declares_itintests/test_elixir_unqualified_call_scope.py. It uses the shape above and assertsshow → fmtandinner → fmtbind whilego → fmtdoes not.I also checked that each piece matters. Keying all imports to the file again, or not walking enclosing modules, each makes the new test fail. Dropping the visited-set guard makes the self-nested case hang.
Windows 11, Python 3.11,
uv sync --frozen:Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments. (n/a, no fragment changes)Refs #4001