fix(parser): resolve relative TS/JS imports by appending, not replacing, the extension - #831
Open
lavaxun wants to merge 1 commit into
Open
fix(parser): resolve relative TS/JS imports by appending, not replacing, the extension#831lavaxun wants to merge 1 commit into
lavaxun wants to merge 1 commit into
Conversation
…ng, the extension _do_resolve_module's relative-import branch probed extensionless imports with base.with_suffix(ext), which replaces the stem's final suffix instead of appending to it. A dotted stem like ./outlet.entity therefore probed outlet.ts rather than outlet.entity.ts, and dotted stems are the dominant NestJS naming convention (*.entity.ts, *.service.ts, *.controller.ts, *.module.ts) — measured in a large NestJS monorepo, this dropped 13,013 relative imports as unresolved (or silently misresolved onto a same-directory decoy). Switch the extension loop to append (Path(str(base) + ext)), mirroring _probe_path in tsconfig_resolver.py, which already gets this right for alias imports. Add a narrow fallback for ESM/NodeNext output, which writes ./foo.js for a module that is foo.ts on disk — the one case where replacing the suffix is actually correct. Adds regression coverage in test_commonjs_imports.py: a dotted stem resolving correctly in the presence of a same-directory decoy, a dotted stem with no decoy, the ESM .js-to-.ts fallback, and the plain single-word-stem case that must keep working unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
_do_resolve_module's relative-import branch for JS/TS/TSX/Vue (code_review_graph/parser.py, around line 13578 on main) resolves an extensionless relative import like./outlet.entityby tryingbase.with_suffix(ext)for each candidate extension.Path.with_suffixreplaces the final suffix rather than appending to it, andPath("outlet.entity").suffixis.entity— so the probe becomesoutlet.ts, notoutlet.entity.ts.Why this matters
Dotted stems are the dominant NestJS file-naming convention:
*.entity.ts,*.service.ts,*.controller.ts,*.module.ts,*.guard.ts, and so on. With the replace-based probe, almost every relative import between files following this convention either:outlet.tsnext tooutlet.entity.ts), orMeasured in a large real-world NestJS monorepo: 13,013 unresolved relative imports dropped to 13 after applying this fix. That's a large fraction of a typical NestJS codebase's internal import graph silently missing from
callers_of,get_impact_radius, and every other graph query that depends on relative-import edges resolving correctly._probe_pathintsconfig_resolver.pyalready gets this right for path-alias imports (it appends when the base already has a suffix); this fix mirrors that logic in the relative-import branch, which had drifted from it.The fix
target = base.with_suffix(ext)withtarget = Path(str(base) + ext), so the candidate extension is appended rather than replacing whatever the stem's own suffix looks like..js,.jsx,.mjs, or.cjs, retry withbase.with_suffix(".ts")/.with_suffix(".tsx"). This covers ESM/NodeNext output, which writes./foo.jsin source for a module that is actuallyfoo.tson disk — the one case where replacing the suffix is the correct behavior, since appending would probe the nonsensicalfoo.js.ts.Checked the rest of
parser.pyfor the same pattern: the Dart relative-import branch also callsbase.with_suffix(".dart"), but Dart stems essentially never contain dots, so it isn't exposed to this bug and was intentionally left alone._probe_pathintsconfig_resolver.pywas re-checked against current main and is already correct (append-not-replace) for path-alias resolution.Tests
Added four tests to
tests/test_commonjs_imports.py, following the file's existingtmp_path+CodeParser().parse_file+IMPORTS_FROM-edge style:test_dotted_stem_relative_import_resolves_to_the_dotted_file_not_a_decoy—./outlet.entityin a directory containing bothoutlet.entity.tsand a decoyoutlet.tsmust resolve to the dotted file. This is the case the old code got wrong (it resolved to the decoy).test_dotted_stem_relative_import_resolves_without_a_decoy_present— same dotted-stem import with no decoy present, plain regression.test_esm_js_extension_relative_import_still_resolves_to_the_ts_source—./helper.jsstill resolves tohelper.tson disk (the fallback added in step 2).test_plain_single_word_stem_relative_import_still_resolves— a non-dotted stem (./dependency) is unaffected.All four were run against the pre-fix source first. The two dotted-stem tests fail without the fix (one resolves to the decoy, the other fails to resolve at all); the ESM and plain-stem tests happen to already pass pre-fix, since
with_suffixincidentally does the right thing for single-dot stems — they're included as regression guards against the fix itself, not as bug demonstrations.Full targeted run after the fix:
tests/test_commonjs_imports.py,tests/test_tsconfig_resolver.py,tests/test_parser.py— 183 passed. Full suite: 2401 passed, 5 skipped, 2 xpassed, 1 pre-existing failure unrelated to this change (test_embedding_initialization.py::test_windows_server_still_prewarms_before_mcp_run, a Python 3.14asyncio.WindowsSelectorEventLoopPolicyremoval issue, reproduced identically on an unpatched checkout).