fix(resolution): a name bound to a bare import resolves to no project node - #1713
fix(resolution): a name bound to a bare import resolves to no project node#1713bompus wants to merge 4 commits into
Conversation
… node
Fuzzy matching commits to a lone surviving candidate. Filtering narrows a
crowd of same-named symbols; it does not establish that the true target was
ever in the crowd. `import { scan } from 'rolldown/experimental'` is the case
that matters: the real target is external and absent from the graph, so the
last project symbol standing inherits the reference -- in that instance the
importing file's own `scan`, a self-edge.
Decline fuzzy matching when the call site's own binding is a bare specifier.
Relative, alias and workspace imports point at project files and still fall
through, and only the JS/TS family is checked, since elsewhere a project's
own modules are imported by absolute name too and the same test would reject
the internal case along with the external one.
On vite this removes 4 wrong edges and adds none; no other resolver moves.
|
Reproduced on the same tree — vite at One thing in the predicate worth knowing before the exact-match half, since that is where it bites: I took the exact-match half as #1715, stacked on this: same predicate with |
isBoundToBareImport tested `startsWith('~/')`, so a slashless alias was classed
as an external package. vite's playground/tsconfig.json declares
`"paths": { "~utils": ["./test-utils.ts"] }` — a nested tsconfig the alias
loader never reads — and `#types/hmrPayload` is a package.json `imports`
subpath; both name project files.
None of `~`, `#` or `$` can begin an npm package name, so the prefix alone is
sufficient evidence of a local binding and no resolver lookup is needed.
In matchFuzzy this changes nothing measurable on vite, because those names
resolve by exact match before fuzzy is reached — which is exactly why the
defect survived a green measurement. It is load-bearing for any use of the
predicate in matchByExactName, where classing `~utils` as bare took 1,395 real
edges out with the wrong ones.
|
Thank you — both halves of that were more useful than a review usually is, and the second one caught a defect I had shipped. The What I want to flag about that, because it is the uncomfortable part: it would have merged on my own evidence. Those names resolve by exact match before On your independent reproduction — LOST 4, GAINED 0, fuzzy 13 → 9, same four edges, every other resolver at zero. That matters more than a second run of mine would. I have since rebuilt both arms on Node v24.16.0 (the version Your Python point is the one I was least sure of, so I am glad you tested it. I have not taken #1715 anywhere near this branch — it is yours, it is measured on its own, and stacking would only re-create the dependency problem I already had to cherry-pick my way out of once. On the survivor variant you posted on the other thread: I have read it and I think it is better than what I had, particularly that it clears the 15 this guard leaves without needing any knowledge of the binding. I am deliberately not commenting there, since that PR carries someone else's commit and it is not mine to speak for. |
|
Rebased #1715 onto On the survivor variant: understood on not speaking for #1709. Since #1713 is standalone off |
|
Checked the rebase rather than take it: #1715 carries the prefix rule verbatim — the comment and On the survivor variant: the shape you describe is the right one, and I'd take it for the same reason #1713 is standalone. An arm measured on its own base is the only kind whose numbers mean anything — stacking makes the delta a statement about two changes and you can never afterwards say which. Base → LOST 12, GAINED 0 is a claim a reviewer can check; the same number measured on top of But I can't tell you to move #1709's fixture, because it isn't mine. I'm not the author of #1709 and I'd be endorsing a repackaging of someone else's work on no authority at all. That's a question for them, in their thread — my honest read is they'll say yes, since a standalone PR that credits them gets their fixture reviewed sooner than a stalled stack does, but the difference between me predicting that and them saying it is the whole point. Concretely, what I'd do in your place: ask in #1709. If there's no reply in a reasonable window, open it anyway with the fixture credited and the PR linked in the description, and let the maintainer decide whether to take it that way. That avoids both failure modes — an indefinite block on an unresponsive thread, and a silent lift. What I'd avoid is treating my agreement about the shape as agreement about the fixture; I only have standing on the first. One thing worth stating plainly since it affects how you weigh my opinion here: I got a call wrong on this stack recently. I filed an issue against codegraph for a kernel gap that is entirely inside my own unmerged PR, because I ablated against a commit on my own branch instead of the merge base — a control that still contained the code under test. It's closed with a correction. Same class of error as measuring an arm on a stacked base, which is partly why I'm insistent about it above and why I'd rather you check the reachability numbers yourself than trust that I checked them. |
|
Agreed on both counts — shape yes, fixture not yours to grant. Asked in #1709. If that thread stays quiet I'll open it off |
|
I ran the two guards stacked, which I had not done when I argued they were complementary. The description is corrected; the short version: Indexed at
Stacked delta is LOST 0, GAINED 0 — the edge sets are identical. #1718's 12 already contain all 4 of this PR's. I told @danusha2345 in review that both vite instances were a candidate #1718 would pass, "uniquely surviving and lexically reachable". That was wrong: Both are nested functions — precisely the shape What survives is the predicate rather than an instance. With #1718 applied and this guard ablated, The merge is clean in either order: |
…kage vitest's test/browser declares "@vitest/bundled-lib": "link:./bundled-lib", a directory its test/* workspace globs do not reach, so the workspace map could not vouch for the name and the guard classed it external — removing two correct edges onto the linked package's own source. link: and file: are the protocols every package manager reads as "this is a directory in the project", so the name is local however much it is spelled like a scoped registry package.
|
Following the comment above: I widened the corpus from one repo to four, and it reversed the conclusion I had just posted. The description is rewritten around the new numbers; two commits are pushed. Stacked on #1718, all four indexed at
So the complementarity is real after all — 25 edges #1718 does not reach — but vite, the only repo I had measured, is the one where it is 0. My previous comment generalised from the single corpus that happens to have no instance. The reason #1718 cannot reach them is sharper than "different side": the match is case-insensitive, and the survivor is often a method.
The widened corpus also found a defect in this PR, of exactly the kind @danusha2345 found in the Standalone against the merge-base, same four repos: LOST 34 / 5 / 4 / 3 = 46, GAINED 0, every non-fuzzy resolver unchanged on every repo. The lesson I am taking from the pair of comments: one corpus was enough to find the bug and not enough to characterise the fix, and I stated a general conclusion twice from it — once too strong, once too weak. |
matchFuzzycommits to a candidate whenever filtering leaves exactly one. Filtering narrows a crowd of same-named symbols; it does not establish that the true target was ever in the crowd. When the call site's own name comes from a bare import the true target is external and absent from the graph, so the last project symbol standing inherits the reference.The match is case-insensitive, so the crowd is larger than the name suggests. Instances from three of the four repos measured below:
And on vite, two at the merge-base:
The first is a self-edge: a file importing a name from npm, resolved onto its own definition of that name, plus the
callsedge frombuildthat follows it.The change
Decline fuzzy matching when the call site's binding is a bare specifier — a builtin or an npm package. Relative, alias and workspace imports point at project files, so a fuzzy match is still a reasonable recovery there when the import resolver could not follow the path.
Only the JS/TS family is checked. There a project-internal import is distinguishable by shape — relative, aliased, or a workspace member — so "bare" really does mean external. In Java, Kotlin, Go and Python a project's own modules are imported by absolute name too, and the same test would reject the internal case along with the external one.
Measurement
Four repos, each indexed at this PR's merge-base (
b9ca4b7) and at its tip. Every non-fuzzy resolver is unchanged on every repo and GAINED is 0 everywhere — the change is purely subtractive:7c818155895c638492422e90af89The per-resolver breakdown on vite, as the shape all four have:
Edge-set delta on vite: LOST 4, GAINED 0 — the two instances above and the two
callsedges that follow them.Relationship to #1709 and #1718
The measurement above is against the merge-base and does not depend on either PR. Stacked on #1718, this guard removes a further 25 edges — measured below.
#1718 supersedes #1709 and is the better fix. #1709 filtered unreachable nested functions out of the fuzzy candidate set, which on vite left exactly one survivor for a crowd of
resolvedefinitions; the survivor then inherited everyresolvecall from the playground configs. #1718 instead checks reachability on the single candidatematchFuzzywould already commit to. That is structurally stronger, not merely better-measured: a guard on thefinalCandidates.length === 1branch can only decline an edge, never create a uniqueness, so GAINED 0 holds by construction. I checked that declining there is terminal rather than falling through — fuzzy is the last of four strategies atname-matcher.ts:2776, and the other call site at:1219returns null at:1220— so no reference can be routed into a later edge-producing path.That removes an argument this PR used to make. Against #1709's filtering approach, this guard removed 44 of the 59 edges that approach gained. Under #1718 those 59 are never manufactured, so those 44 are no longer a benefit this change provides, and I am not claiming them. The measurements are kept below because they are the evidence for why filtering the set was wrong, which is now the accepted conclusion — not because they argue for this PR.
Measured stacked. Both PRs branch from
b9ca4b7, so an arm merging the two differs from #1718 alone by this guard and nothing else:25 wrong edges that #1718 cannot reach, and two of them show why it cannot:
isLexicallyReachablereturnstrueon its first line for any candidate that is not afunction, so a method survivor is admitted by construction — it is not the shape that check is for. Nothing about reachability can declineEvaluatedModules; only the import binding shows it is wrong.A correction, since an earlier revision of this section argued the reverse of its own measurement. I claimed vite's
scanandgetEnvwere candidates #1718 would pass because they were top-level. They arescanImports::scanandimportAnalysisPlugin::getEnv— nested functions, exactly whatisLexicallyReachablerejects — so #1718 declines them unaided, and vite's row above is a legitimate 0. I had read vite's source rather than querying the node'squalifiedName, which is the only view of the symbol the guard has, and I had never run the two arms in one tree. Adding three repos is what turned the argument into a measurement, and it also caught a defect in this PR (below).The two guards check opposite sides and neither subsumes the other. #1718 guards the candidate it would commit to; this guards the reference, whose name is bound to an external specifier, so no project node is correct however reachable the survivors are. Conversely a
res.text()whose only candidate is a closure never trips this guard, and #1718 declines it. They touchmatchFuzzyat different points — this one at the top before candidate gathering, #1718 on the length-1 branch — and merging them confirms it:name-matcher.tsauto-merges, onlyCHANGELOG.mdconflicts.What this does not fix
Two mis-resolutions on vite survive this guard, and both are a different failure mode rather than bare imports. (They were 15 of the edges in the
#1709 + thisarm above; that arm is superseded, but the two shapes are still there and neither guard addresses them.)The first is a same-file
constarrow. Querying the graph, there is no node namedresolvein that file at all — extraction never emits the binding, so no resolver-layer rule can see it to shadow the cross-file candidate. The second is a method call on a rollup plugin context. Both are extraction-side and out of scope here.I also tried the same guard in
matchByExactName, which has an identical single-survivor commit atname-matcher.ts:418. On vite that moved exact-match 10,716 → 6,617 — 4,048 edges lost — so I reverted it.@danusha2345 found why, and it was a defect in my predicate rather than in the idea.
startsWith('~/')did not catch~utils, which vite'splayground/tsconfig.jsondeclares as apathsentry — a nested tsconfig the alias loader never reads — so a project alias was classed as an external package and took 1,395 real edges out with the wrong ones, plus 20 through#types/hmrPayload, apackage.jsonimportssubpath.The second commit here fixes that:
~,#and$are treated as local, since none of them can begin an npm package name, so the prefix alone is sufficient evidence without a resolver lookup. On vite this changes nothing measurable inmatchFuzzy— those names resolve by exact match before fuzzy is reached, which is exactly why the defect survived a green measurement — but it is load-bearing for any use of the predicate inmatchByExactName. The exact-match half is @danusha2345's #1715.Widening the corpus found a second one of the same kind, and the third commit fixes it. vitest's
test/browser/package.jsondeclares"@vitest/bundled-lib": "link:./bundled-lib", a directory itstest/*workspace globs do not reach — soresolveWorkspaceImportcould not vouch for the name, the predicate classed it external, and two correct edges onto the linked package's own source were removed.link:andfile:are the protocols npm, yarn, pnpm and bun all read as "this package is a directory in the project", so the name is local however much it is spelled like a scoped registry package.loadWorkspacePackagesnow collects those dependency names while reading the manifests it already opens, and the predicate treats a match as local. It is deliberately a set of names with no directory: resolving these imports is a real improvement and a separate change, and this PR stays subtractive. The stacked table above is post-fix — before it, vitest read 22 rather than 20, and those two extra were this PR being wrong.Tests
__tests__/fuzzy-bare-import-binding.test.tsdrivesmatchFuzzydirectly — 11 cases, all passing. Three must decline: anode:builtin, a bare npm specifier, and a scoped package that is not linked into the project. Eight must not change: a relative import the resolver could not follow, no import binding at all, a Python ref bound toos, the four local-prefix spellings~utils,~/utils,#types/hmrPayload,$lib/stores, and alink:dependency. Ablating the guard fails exactly the three declines.The guard is also ablated against #1718 rather than only against the merge-base, since that is the arm it has to earn its place in: with #1718 applied and this guard removed, the run is 3 failed / 8 passed — the same three declines. Their candidate is a method, which
isLexicallyReachableadmits at its first line, so no amount of reachability checking covers them.The direct-call shape is deliberate. Which strategy reaches a given ref depends on how many same-named symbols the tree holds and on what the earlier stages of
matchReferencemake of them, so a source fixture pins the pipeline rather than this guard — my first attempt at one resolved throughmatchByExactNameand passed with the guard removed.Verification notes
Runtime. Everything above was built, indexed and measured on Node v24.16.0 — the version
scripts/build-bundle.shvendors, insideengines.node(>=20.0.0 <25.0.0). Every arm is a freshnpm run build(exit 0) and a fresh index into a deleted.codegraph;tsc --noEmitexit 0.CODEGRAPH_KERNEL_DEBUG=1confirms the native kernel loaded for typescript/tsx/javascript/jsx on every indexing pass, so these are kernel-arm numbers rather than the wasm fallback.An earlier revision of this description reported the vite table measured under Bun and claimed that was the supported path. It is not — this project asks for Node and vendors its own — and I have replaced those numbers rather than annotate them. The vite arms were re-run tip-first, the reverse of the original ordering, and reproduce exactly: LOST 4, GAINED 0, fuzzy 13 → 9, total edges 27,778 → 27,774, the same four edges. @danusha2345 independently reproduced that table on their own tree. The three added repos were run in the opposite arm order again (#1718 before the stack), which is a formality here — every number in this description is a count, not a duration, and counts do not move with cache state.
Why four repos. vite alone was misleading in both directions: it is the only one of the four where this guard removes nothing on top of #1718, and it is the repo whose
link:dependencies are absent, so the defect fixed in the third commit could not surface there. One corpus was enough to find the bug and not enough to characterise the fix.Full suite on Node v24.16.0, Windows, at this PR's tip: 4,180 passed, 25 failed, 0 assertion failures. Every failure is an
EPERMonfs.rmSyncof a temp directory in teardown, or a test timeout — none is an assertion, and none is in the resolution path this PR touches.