Skip to content

fix(resolution): a name bound to a bare import resolves to no project node - #1713

Open
bompus wants to merge 4 commits into
colbymchenry:mainfrom
bompus:fix/fuzzy-bare-import-binding
Open

fix(resolution): a name bound to a bare import resolves to no project node#1713
bompus wants to merge 4 commits into
colbymchenry:mainfrom
bompus:fix/fuzzy-bare-import-binding

Conversation

@bompus

@bompus bompus commented Sep 6, 2026

Copy link
Copy Markdown

matchFuzzy commits 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:

vitest   import type { EvaluatedModules } from 'vite/module-runner'
           → [method] VitestMocker::evaluatedModules                   (17 refs)
vitest   import type { Result } from 'tinyexec'
           → [method] TestCase::result
svelte   import MagicString, { Bundle } from 'magic-string'
           → [function] bundle @ scripts/generate-browser-support.ts   (5 refs)

And on vite, two at the merge-base:

packages/vite/src/node/optimizer/scan.ts:7   import { scan } from 'rolldown/experimental'
  → packages/vite/src/node/optimizer/scan.ts:scan          (the importing file's OWN scan)

packages/vite/src/node/config.ts:14          import { getEnv } from '@vitejs/devtools/config'
  → packages/vite/src/node/plugins/importAnalysis.ts:getEnv

The first is a self-edge: a file importing a name from npm, resolved onto its own definition of that name, plus the calls edge from build that 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:

repo edges fuzzy LOST GAINED
vitest 7c81815 73,264 → 73,230 511 → 477 34 0
svelte 5895c63 65,427 → 65,422 131 → 126 5 0
vite 8492422 27,778 → 27,774 13 → 9 4 0
rollup e90af89 66,571 → 66,568 2,274 → 2,271 3 0

The per-resolver breakdown on vite, as the shape all four have:

resolvedBy base PR delta
fuzzy 13 9 -4
exact-match 10716 10716 0
import 5359 5359 0
qualified-name 1790 1790 0
instance-method 1035 1035 0
(none) 8684 8684 0
function-ref 99 99 0
file-path 63 63 0
framework 19 19 0

Edge-set delta on vite: LOST 4, GAINED 0 — the two instances above and the two calls edges 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 resolve definitions; the survivor then inherited every resolve call from the playground configs. #1718 instead checks reachability on the single candidate matchFuzzy would already commit to. That is structurally stronger, not merely better-measured: a guard on the finalCandidates.length === 1 branch 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 at name-matcher.ts:2776, and the other call site at :1219 returns 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.

LOST GAINED fuzzy
base → #1709 12 59 13 → 60
base → #1709 + this 12 15 13 → 16
#1709#1709 + this 44 0 60 → 16
base → this alone 4 0 13 → 9

Measured stacked. Both PRs branch from b9ca4b7, so an arm merging the two differs from #1718 alone by this guard and nothing else:

repo #1718 alone #1718 + this LOST GAINED
vitest 73,004 (fuzzy 251) 72,984 (fuzzy 231) 20 0
svelte 65,346 (fuzzy 50) 65,341 (fuzzy 45) 5 0
vite 27,766 (fuzzy 1) 27,766 (fuzzy 1) 0 0
rollup 64,604 (fuzzy 307) 64,604 (fuzzy 307) 0 0

25 wrong edges that #1718 cannot reach, and two of them show why it cannot:

17x  EvaluatedModules from 'vite/module-runner'  → [method] VitestMocker::evaluatedModules
 1x  Result           from 'tinyexec'            → [method] TestCase::result
 2x  Pattern          from 'estree'              → [function] pattern @ test/unit/test/pattern.test.ts
 5x  Bundle           from 'magic-string'        → [function] bundle @ scripts/generate-browser-support.ts

isLexicallyReachable returns true on its first line for any candidate that is not a function, so a method survivor is admitted by construction — it is not the shape that check is for. Nothing about reachability can decline EvaluatedModules; 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 scan and getEnv were candidates #1718 would pass because they were top-level. They are scanImports::scan and importAnalysisPlugin::getEnv — nested functions, exactly what isLexicallyReachable rejects — 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's qualifiedName, 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 touch matchFuzzy at different points — this one at the top before candidate gathering, #1718 on the length-1 branch — and merging them confirms it: name-matcher.ts auto-merges, only CHANGELOG.md conflicts.

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 + this arm above; that arm is superseded, but the two shapes are still there and neither guard addresses them.)

const resolve = (p) => path.resolve(import.meta.dirname, p)   // playground/ssr-conditions/server.js:13
await this.resolve(id, importer)                              // playground/css/vite.config.js:30

The first is a same-file const arrow. Querying the graph, there is no node named resolve in 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 at name-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's playground/tsconfig.json declares as a paths entry — 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, a package.json imports subpath.

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 in matchFuzzy — 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 in matchByExactName. 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.json declares "@vitest/bundled-lib": "link:./bundled-lib", a directory its test/* workspace globs do not reach — so resolveWorkspaceImport could not vouch for the name, the predicate classed it external, and two correct edges onto the linked package's own source were removed. link: and file: 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. loadWorkspacePackages now 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.ts drives matchFuzzy directly — 11 cases, all passing. Three must decline: a node: 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 to os, the four local-prefix spellings ~utils, ~/utils, #types/hmrPayload, $lib/stores, and a link: 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 isLexicallyReachable admits 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 matchReference make of them, so a source fixture pins the pipeline rather than this guard — my first attempt at one resolved through matchByExactName and 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.sh vendors, inside engines.node (>=20.0.0 <25.0.0). Every arm is a fresh npm run build (exit 0) and a fresh index into a deleted .codegraph; tsc --noEmit exit 0. CODEGRAPH_KERNEL_DEBUG=1 confirms 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 EPERM on fs.rmSync of a temp directory in teardown, or a test timeout — none is an assertion, and none is in the resolution path this PR touches.

… 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.
@danusha2345

Copy link
Copy Markdown
Contributor

Reproduced on the same tree — vite at 8492422, indexed at b9ca4b7 and at a320ed1, edge sets joined back to symbols: LOST 4, GAINED 0, fuzzy 13 → 9, every other resolvedBy row at exactly zero. The four are the two getEnv and the two scan edges from your description. The guard does what it says; the Python carve-out is right too, from os import path and from myapp.util import path really are the same shape.

One thing in the predicate worth knowing before the exact-match half, since that is where it bites: ~utils is not caught by startsWith('~/'). vite's playground/tsconfig.json declares "paths": { "~utils": ["./test-utils.ts"] } — a nested tsconfig, which the alias loader never reads — so import { getColor } from '~utils' reaches playground/test-utils.ts by name only. In matchFuzzy that costs nothing on vite (those names resolve by exact match first), but it is the reason the matchByExactName attempt lost 4,048: with ~utils classed as bare, its 1,395 real edges went out with the wrong ones, plus the 20 through #types/hmrPayload (a package.json imports subpath).

I took the exact-match half as #1715, stacked on this: same predicate with ~, # and $ prefixes treated as local, applied in matchByExactName by dropping the other-file candidates when the binding is bare (a same-file definition still shadows the import). On vite against your tip that is LOST 2,536, GAINED 33, and the lost list grouped by the binding import is vitest 1,747, node:path 261, rolldown 126, node:fs 65, picocolors 44, node:http 33, kill-port 22, … — not one through ~utils, #types or a relative path. Details and the gained-33 breakdown (29 are the same PluginContext targets re-resolved by a framework resolver at 0.8, so unchanged in the graph) are in the PR.

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.
@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

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 ~utils finding is fixed in the second commit here. You were right about the mechanism and right about why it stayed invisible: startsWith('~/') does not catch a slashless alias, and playground/tsconfig.json is a nested tsconfig the alias loader never reads, so ~utils was classed as an external package. The predicate now treats ~, # and $ as local. The argument I settled on is that none of those three can begin an npm package name, so the prefix alone is sufficient evidence of a local binding and no resolver lookup is needed — ~utils (tsconfig paths), #types/hmrPayload (a package.json imports subpath), $lib/… (SvelteKit). Four spellings are now pinned in the test file.

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 matchFuzzy is reached, so the predicate's wrongness is invisible in the one place I measured. My vite numbers were honest and the code was still wrong. That is a good argument for your #1715 being measured separately rather than folded in here, and I have updated this PR's description to say so and to credit the finding.

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 scripts/build-bundle.sh vendors, inside engines.node), with the arms run tip-first to reverse my original ordering, and it reproduces exactly — down to the total edge count, 27,778 → 27,774. An earlier revision of this description reported those numbers as measured under Bun and asserted Bun was what the project asks for. That was false, and I have replaced the numbers rather than annotate them.

Your Python point is the one I was least sure of, so I am glad you tested it. from os import path and from myapp.util import path really are the same shape, which is why the carve-out is by language family rather than by a cleverer specifier test. Java, Kotlin and Go have the same problem. In JS/TS "bare" genuinely means external once the local prefixes are excluded, and nowhere else — that asymmetry is the whole basis for the language gate, and it is worth stating plainly in case someone later wants to widen 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.

@danusha2345

Copy link
Copy Markdown
Contributor

Rebased #1715 onto 6d36f3f — it now carries your prefix rule verbatim, and its own diff is down to the matchByExactName change, the export plus optional-call of the predicate, and the source-fixture tests (211 across the three touched test files, tsc clean). Description updated to say the same.

On the survivor variant: understood on not speaking for #1709. Since #1713 is standalone off main, the natural shape for that one is the same — a standalone PR off main carrying #1709's two-file fixture, the reachability check moved onto the survivor, and the vite numbers (base → LOST 12, GAINED 0) — measured on its own rather than stacked on anything. I'll open it that way unless you'd rather it wait for #1709's author to pick the direction.

@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

Checked the rebase rather than take it: #1715 carries the prefix rule verbatim — the comment and source.startsWith('~') || source.startsWith('#') || source.startsWith('$') are identical to 6d36f3f, the predicate is exported and called optionally at both sites, and the diff is down to four files. That reads as a clean increment on top of this PR now rather than a second copy of it. Thanks for cutting it back.

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 ef2169e is not.

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.

@danusha2345

Copy link
Copy Markdown
Contributor

Agreed on both counts — shape yes, fixture not yours to grant. Asked in #1709. If that thread stays quiet I'll open it off main with the fixture credited and #1709 linked, and let the maintainer choose; and the reachability numbers there will be re-measured on that branch's own base, not carried over from the stacked run.

@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

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 b9ca4b7, the merge-base of both PRs.

arm edges fuzzy vs merge-base
merge-base 27,778 13
#1718 alone 27,766 1 LOST 12, GAINED 0
#1718 + this 27,766 1 LOST 12, GAINED 0

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:

scan   → [function] scanImports::scan             optimizer/scan.ts:131-204
getEnv → [function] importAnalysisPlugin::getEnv  plugins/importAnalysis.ts:233-255

Both are nested functions — precisely the shape isLexicallyReachable rejects. I read vite's source and called them top-level rather than querying the node's qualifiedName, which is the only view of the symbol the guard has.

What survives is the predicate rather than an instance. With #1718 applied and this guard ablated, __tests__/fuzzy-bare-import-binding.test.ts fails 2 of 9 — the two must-decline cases, whose candidate is a method and so passes isLexicallyReachable at its first line. Neither guard subsumes the other as a rule. But vite contains no instance of the shape only this one catches, so I am not claiming an edge reduction on top of #1718, and this PR should be judged on the standalone LOST 4 / GAINED 0 and on whether the shape is worth guarding before a corpus exhibits it.

The merge is clean in either order: name-matcher.ts auto-merges, only CHANGELOG.md conflicts.

…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.
@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown
Author

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 b9ca4b7:

repo #1718 alone #1718 + this LOST GAINED
vitest 73,004 (fuzzy 251) 72,984 (fuzzy 231) 20 0
svelte 65,346 (fuzzy 50) 65,341 (fuzzy 45) 5 0
vite 27,766 (fuzzy 1) 27,766 (fuzzy 1) 0 0
rollup 64,604 (fuzzy 307) 64,604 (fuzzy 307) 0 0

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.

17x  EvaluatedModules from 'vite/module-runner'  → [method] VitestMocker::evaluatedModules
 1x  Result           from 'tinyexec'            → [method] TestCase::result
 2x  Pattern          from 'estree'              → [function] pattern @ test/unit/test/pattern.test.ts
 5x  Bundle           from 'magic-string'        → [function] bundle @ scripts/generate-browser-support.ts

isLexicallyReachable returns true on its first line for any candidate that is not a function, so a method survivor is admitted by construction. Ablating this guard on top of #1718 fails 3 of 11 tests — the three declines — which is the same fact as a unit test.

The widened corpus also found a defect in this PR, of exactly the kind @danusha2345 found in the ~utils review. vitest's test/browser/package.json declares "@vitest/bundled-lib": "link:./bundled-lib", a directory its test/* workspace globs do not reach, so resolveWorkspaceImport could not vouch for the name and the predicate classed it external — removing two correct edges. link: and file: are the protocols every package manager reads as "this is a directory in the project". loadWorkspacePackages now collects those names while reading manifests it already opens, and the table above is post-fix: before it, vitest read 22, and the two extra were this PR being wrong.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants