Repository navigation
fix(python): keep missing relative-import targets out of the checkout path - #4157
rohit-jsfreaky wants to merge 1 commit into
Conversation
… path A relative import of a module with no file behind it (`from .absent import x`) built its target from the attempted absolute path, so the id became e.g. `c_users_<user>_..._pkg_absent_py`. The root-relative id remap never rewrites it (there is no node to anchor on), so the checkout location and the OS username ended up in graph.json and every clone got a different id. Use the dotted module name relative to the scan root instead (`pkg_absent`), the id an unresolved absolute import of the same module already gets, so `from .absent` and `from pkg.absent` now meet on one node. An import that climbs above the scan root falls back to a `ref` id. Imports that resolve, and attempted paths that exist, are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B8gwEpjajiWHVW3c9smuKf
|
Thanks for the pull request, @rohit-jsfreaky. 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.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. PR-changed functions: 0/1 verified (0 proven, 0 may-equivalent, 0 distinguished) · 1 not verified (1 vacuous).
Not verified on this run: \_import\_python (vacuous: never exercised).
Graphify review — findings
Fixes node ids for unresolved relative Python imports: when no file exists behind the import, the target id now comes from the dotted module name relative to the scan root (e.g. pkg_absent), matching what an unresolved absolute import gets. Previously the id came from the attempted absolute path, which baked the checkout location and OS username into the graph. Imports that climb above the scan root fall back to ref: plus the raw specifier.
No blocking issues surfaced. 3 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 2431 functions depend on the 300 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 779 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()— 102 callers, 3 callees - new:
dispatch_command()— 2 callers, 128 callees - new:
_get_extractor()— 27 callers, 6 callees - new:
collect_files()— 21 callers, 7 callees - …and 38 more — each is listed as a finding
Verification — 2431 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: 2241 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
147 of 334 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_cache.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— impacttests/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— impact- … and 97 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 \_import\_python.
The verifier did not have enough to check \_import\_python, 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 264 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly AttributeError — names the real obstacle, not a sampling gap)
· 1 grounded finding(s) anchored inline below; 45 more finding(s) on lines outside this diff (see the check run).
| return ".".join(parts) if parts else f"ref:{raw}" | ||
|
|
||
|
|
||
| def _import_python( |
There was a problem hiding this comment.
_import_python()
fans out to 7 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
|
Landed in v0.9.78 via an authorship-preserving cherry-pick, so your commit is on |
What does this PR do?
Fixes #4156.
A relative import whose module has no file behind it (
from .absent import x) built its target from the attempted path and minted the id from it, e.g.c_users_<user>_..._pkg_absent_py. There is no node for that path, so the root-relative id remap never rewrites it: the checkout location and the OS username ended up in graph.json, and every clone got a different id for the same import.For that case only, the id is now the dotted module name relative to the scan root (
pkg_absent), which is exactly the id an unresolved absolute import of the same module already gets. Sofrom .absentandfrom pkg.absentin one file now meet on one target instead of two. An import that climbs above the scan root falls back to aref:<specifier>id, so the id never carries an absolute path.Unchanged: relative imports that resolve, attempted paths that exist on disk, absolute imports, and the existence-gated
target_filestamp below it (a missing sibling still stays dangling, as the comment there intends; only its id changes).Type of change
Verification & Invariants
Invariant: node ids do not depend on where the repo is checked out (#2457, #1789, #1899).
The [Bug]: Python relative import of a missing module mints its target id from the absolute checkout path #4156 repro (same package in two checkouts), v8
5c7b847:c_users_<user>_..._clone_one_pkg_missing_rel_pyexists only in the first checkout. With this PR no id is unique to either checkout, and the target ispkg_missing_rel, next to the absolute import'spkg_missing_pkg_mod.graphify's own
worked/mixed-corpus/raw/build.py(from .validate import ..., novalidate.py) no longer contributes a checkout-specific id.Existing relative-import tests keep passing, including
test_python_relative_import_out_of_root_target_id_is_portable(its target exists, so this branch is not taken) andtest_overdeep_relative_import_is_unresolved_not_fatal(climbs above the root:reffallback).Persisted state: only ids that contained the checkout path change, once.
Read the CONTRIBUTING.md guide.
Reproduced the issue and identified the invariant.
Made the smallest fix necessary.
Added a regression test (if bug fix) or isolated boundary test.
Kept the PR description synchronized with the final implementation.
Documented any limitations / unsupported cases explicitly.
Limitation: two different missing relative imports that climb above the scan root with the same specifier share one
refid. That case has no meaningful root-relative name, and before this change it was a per-machine absolute id.How was this tested?
test_missing_relative_import_target_id_does_not_depend_on_the_checkout(tests/test_python_import_resolution.py) builds the same package in two checkouts withfrom .absent import helperandfrom pkg.absent import other, and requires theimports_fromtargets to be{"pkg_absent"}in both. On v8 it fails withc_users_<user>_..._clone_one_pkg_absent_py.Windows 11, Python 3.12,
uv sync --all-extras --frozen.Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments. (not applicable)🤖 Generated with Claude Code