Repository navigation
Conversation
_apply_symbol_resolution_facts looped over all edges per import fact, so graphify update never finished on mid-size TypeScript projects. Index references by file.
|
Thanks for the pull request, @bercedev. 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.
No issues found by static checks — no coupling regressions or blocking issues in the code graph. That is not the same as safe to merge: see what was not checked.
Not checked: tests were not run; formal verification proved 0 of 1 changed function(s) (1 not verified).
Formal verification. PR-changed functions: 0/1 verified (0 proven, 0 may-equivalent, 0 distinguished) · 1 not verified (1 unsupported).
Not verified on this run: \_apply\_symbol\_resolution\_facts (unsupported).
Graphify review — findings
Fixes quadratic work when rebinding JS/TS import stubs in _apply_symbol_resolution_facts: references edges are now grouped by source file once up front, so each import fact only looks at the edges from its own file instead of scanning every edge in the graph. Rebinding behavior is unchanged. Adds a regression test (#4905) that checks _js_source_path lookups grow roughly linearly when the project doubles in size, plus a check that an imported Shape stub still gets rebound to its declaration.
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 3356 functions depend on the 175 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 1082 callers, 63 callees
Verification — 3356 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: 1300 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
35 of 480 test file(s) selected (7%) via static blast radius.
tests/test_astro_extraction.py— impacttests/test_build.py— impacttests/test_cjs_module_extension.py— impacttests/test_ejs_extractor.py— impacttests/test_ext_tagged_id_clash.py— impacttests/test_extract.py— impacttests/test_forwarding_review_findings.py— impacttests/test_import_extension_resolution.py— impacttests/test_indirect_call_block_scoped_shadow.py— impacttests/test_js_exported_scalar_bindings.py— impacttests/test_js_import_facts_linear.py— impact, changed-testtests/test_js_member_collisions.py— impacttests/test_js_walk_order.py— impacttests/test_language_override_imports.py— impacttests/test_languages.py— impacttests/test_loose_sibling_import_resolution.py— impacttests/test_multilang.py— impacttests/test_package_json_subpath_imports.py— impacttests/test_pascal.py— impacttests/test_pascal_resolution.py— impacttests/test_phantom_external_import.py— impacttests/test_pr10_resolution_overrides.py— impacttests/test_python_import_resolution.py— impacttests/test_python_member_call_language_gate.py— impacttests/test_python_parse_memoization.py— impacttests/test_rationale.py— impacttests/test_resolve_memoization.py— impacttests/test_source_key_memoization.py— impacttests/test_src_layout_import_resolution.py— impacttests/test_svelte_extraction.py— impacttests/test_ts_new_expression_calls.py— impacttests/test_typescript_module_extensions.py— impacttests/test_unmapped_at_alias_resolution.py— impacttests/test_vue_extraction.py— impacttests/test_walk_python_tree_iterative.py— impact
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 \_apply\_symbol\_resolution\_facts.
The verifier did not have enough to check \_apply\_symbol\_resolution\_facts, 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: parameter `facts` is annotated `_SymbolResolutionFacts` — outside the synthesizable primitive/collection set
· 1 more finding(s) on lines outside this diff (see the check run).
What does this PR do?
Closes #4905
_apply_symbol_resolution_factswalked every edge for each JS/TS import, and built a path for each one, sographify updatenever finished on mid-size TypeScript projects. This indexes thereferencesedges by source file once before the loop, which is the fix from the issue. The loop only changesedge["target"], so the index stays valid while it runs.Type of change
Verification & Invariants
Invariant: same graph as before, only the lookup changes.
How was this tested?
Script from the issue, no cache:
The new test counts
_js_source_pathcalls at 20 and 40 files, it fails on v8 and passes here. A second test checks an imported interface still gets rebound to its declaration, it passes on both.Full suite,
--network=none, no.git: 18 failed, 9158 passed, 34 skipped, 1 xfailed. The 18 are thetest_security.pyDNS andtest_skillgen.pyorigin/v8ones that #4894 fixes, they fail on a clean v8 in that setup too.ruff checkis clean on the new test.I only compared the graph on the generated project, not on a real one.
Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments.Note: I used Claude Code to write this and ran the tests above locally.