fix(extract): resolve .js/.mjs/.cjs specifiers through TypeScript's full source order (#3486) - #3488
Conversation
…ull source order (Graphify-Labs#3486) `_resolve_js_import_path` mapped a `.js` specifier to `.ts` and stopped. TypeScript compiles `Button.tsx` to `Button.js`, so under NodeNext/Node16 the specifier the compiler requires is `./Button.js` while the source on disk is `.tsx` — and TypeScript resolves that specifier in the order `.ts` -> `.tsx` -> `.d.ts` -> `.js`. Only the first step was implemented, so every `.tsx` module imported that way dropped out of the graph and surfaced as a `ref_*` phantom edge instead of an `imports_from` edge to the real file node. Replace the `.js`-only branch with a suffix -> ordered-candidates table, so each emitted specifier is tried against every source suffix TypeScript accepts, implementation always ahead of a declaration. `.jsx -> .tsx` folds into the same table and behaves exactly as before; a real `.js`/`.jsx` file on disk still wins because the existence check short-circuits first. Nine of the eleven new tests fail on v8 and pass here; the other two lock behaviour that must not change.
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
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. No changes could be formally verified in this run.
Graphify review — findings
Extends _resolve_js_import_path so a .js/.jsx/.mjs/.cjs specifier resolves through TypeScript's full documented source-suffix order via the new _TS_SOURCE_SUFFIXES table (e.g. .js tries .ts, then .tsx, then .d.ts), rather than the old .js→.ts / .jsx→.tsx single-shot lookup that dropped .tsx-backed .js imports to phantom ref_ edges (#3486). A real file on disk still short-circuits and wins over any source sibling, and multi-dot stems keep their earlier dots since only the last suffix is replaced. Adds tests covering the precedence order, declaration-only fallbacks, the .mjs/.cjs variants, and the end-to-end .js→.tsx import edge.
No blocking issues surfaced. 1 lower-confidence candidate did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1894 functions depend on the 228 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 585 callers, 44 callees - new:
_rebuild_code()— 115 callers, 51 callees - new:
_extract_generic()— 18 callers, 26 callees - new:
_resolve_js_module_path()— 44 callers, 9 callees - new:
extract_js()— 86 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
extract_objc()— 27 callers, 9 callees - …and 35 more — each is listed as a finding
Verification — 1894 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: 919 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
29 of 268 test file(s) selected (11%) via static blast radius.
tests/test_astro_extraction.py— impacttests/test_build.py— impacttests/test_cjs_module_extension.py— impacttests/test_cpp_nested_and_cli.py— impacttests/test_dotnet.py— impacttests/test_extract.py— impacttests/test_forwarding_review_findings.py— impacttests/test_import_extension_resolution.py— impact, changed-testtests/test_indirect_dispatch.py— impacttests/test_indirect_dispatch_assign_return.py— impacttests/test_indirect_dispatch_getattr.py— impacttests/test_js_exported_scalar_bindings.py— impacttests/test_languages.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_python_import_resolution.py— impacttests/test_python_underscore_resolution.py— impacttests/test_rationale.py— impacttests/test_ruby_resolution.py— impacttests/test_scala_self_type.py— impacttests/test_src_layout_import_resolution.py— impacttests/test_swift_computed_properties.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— 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 \_resolve\_js\_import\_path.
The verifier did not have enough to check \_resolve\_js\_import\_path, 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: parameter `candidate` is annotated `Path` — outside the synthesizable primitive/collection set
· 43 more finding(s) on lines outside this diff (see the check run).
3789828 to
307504b
Compare
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. No changes could be formally verified in this run.
Graphify review — findings
Extends TypeScript emitted-module resolution so a .js/.jsx/.mjs/.cjs specifier resolves through TypeScript's full documented source order via _TS_SOURCE_SUFFIXES — e.g. ./Button.js now finds Button.tsx, and .js tries .ts then .tsx then .d.ts, preferring implementations over declarations. Previously the .js branch only tried .ts (and .jsx only .tsx), so imports of transpiled .tsx modules dropped to phantom ref_ edges (#3486). Real on-disk files still short-circuit ahead of any suffix substitution.
Worth a look
.jsspecifiers never resolve to.jsxsources —graphify/extractors/resolution.py:38· 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 — 1894 functions depend on the 228 functions this change touches.
Health — this change adds coupling hotspots:
- new:
extract()— 585 callers, 44 callees - new:
_rebuild_code()— 115 callers, 51 callees - new:
_extract_generic()— 18 callers, 26 callees - new:
_resolve_js_module_path()— 44 callers, 9 callees - new:
extract_js()— 86 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
dispatch_command()— 2 callers, 124 callees - new:
extract_objc()— 27 callers, 9 callees - …and 35 more — each is listed as a finding
Verification — 1894 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: 919 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
29 of 268 test file(s) selected (11%) via static blast radius.
tests/test_astro_extraction.py— impacttests/test_build.py— impacttests/test_cjs_module_extension.py— impacttests/test_cpp_nested_and_cli.py— impacttests/test_dotnet.py— impacttests/test_extract.py— impacttests/test_forwarding_review_findings.py— impacttests/test_import_extension_resolution.py— impact, changed-testtests/test_indirect_dispatch.py— impacttests/test_indirect_dispatch_assign_return.py— impacttests/test_indirect_dispatch_getattr.py— impacttests/test_js_exported_scalar_bindings.py— impacttests/test_languages.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_python_import_resolution.py— impacttests/test_python_underscore_resolution.py— impacttests/test_rationale.py— impacttests/test_ruby_resolution.py— impacttests/test_scala_self_type.py— impacttests/test_src_layout_import_resolution.py— impacttests/test_swift_computed_properties.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— 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 \_resolve\_js\_import\_path.
The verifier did not have enough to check \_resolve\_js\_import\_path, 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: parameter `candidate` is annotated `Path` — outside the synthesizable primitive/collection set
· 43 more finding(s) on lines outside this diff (see the check run).
|
Checked against the issue contract and TypeScript's emitted-module source order: a .js specifier resolves through .ts, .tsx, then .d.ts; .jsx is not a candidate for .js. The existing .jsx → .tsx behavior remains covered separately, so no code change is needed for this advisory. |
1 participant
Fixes #3486.
_resolve_js_import_pathmapped a.jsspecifier to.tsand stopped. TypeScript compilesButton.tsxtoButton.js, so under NodeNext/Node16 the specifier the compiler requires is./Button.jswhile the source on disk is.tsx— and TypeScript resolves that specifier in the order.ts→.tsx→.d.ts→.js. Only the first step was implemented, so every.tsxmodule imported that way dropped out of the graph.Before (v8, 0.9.58)
Resolver alone, one target on disk at a time:
Button.jsButton.tsxButton.js— unresolvedtypes.jstypes.d.tstypes.js— unresolvedmod.mjsmod.mtsmod.mjs— unresolvedmod.cjsmod.ctsmod.cjs— unresolvedhelper.jshelper.tshelper.ts— fineEnd to end on the issue's own fixture,
App.tsimporting./Button.jsemitted aref_button_jsphantom instead of an edge to theButton.tsxnode — the same asymmetry the issue measured as 171 lost edges on a 1,430-file monorepo.Change
graphify/extractors/resolution.py— replace the.js-only branch with a suffix → ordered-candidates table (_TS_SOURCE_SUFFIXES), so each emitted specifier is tried against every source suffix TypeScript accepts, implementation always ahead of a declaration. A real.js/.jsxfile on disk still wins, because the existence check short-circuits before any rewriting..jsx → .tsxfolds into the same table and behaves exactly as before.tests/test_import_extension_resolution.py— 11 regression locks: the four resolution cases, ordering (.tsbeats.tsxbeats.d.ts), multi-dot stems (tag-action.shared.js → tag-action.shared.tsx), two non-regression guards, and the issue's reproducer end to end. Nine of the eleven fail onv8and pass here; the other two pin behaviour that must not change.One correction to the issue's suggested fix
The proposal builds
.d.tsby concatenating ontowith_suffix(""), on the assumption thatPath.with_suffix(".d.ts")mangles a multi-dot stem. It does not —Path("Button.test.js").with_suffix(".d.ts")isButton.test.d.tson 3.10 and 3.13 alike — so this useswith_suffixdirectly. No behaviour difference; just less code.Validation
pytest tests/→ 5136 passed, 286 skipped, with the same 42 pre-existing failures asv8on Windows (FIFO/socket/symlink cases, tree-sitter-terraform, ollama deps). The failure set is byte-identical to thev8baseline; a re-run gives 5137 passed with the identical failure set. (One timing-ratio test,test_ts_normalizer_scales_linearly_on_large_files, flaked once under full-suite load on this machine and passes 5/5 in isolation and on re-run — it is unrelated to this path.)python -m tools.skillgen --check/--schema-singleton/--audit-coverage→ all OK.No new dependencies, no public API change.
I noticed @cbartens offered to upstream their fuller resolution logic in the issue. This PR deliberately implements only the file-level ordering from the issue's own table, and not the workspace
exports-map / platform-split work — that is the separate #3487 — so it stays small and does not collide with a larger contribution there.