fix(resolution): a name bound to a bare import exact-matches no other file's symbol - #1715
fix(resolution): a name bound to a bare import exact-matches no other file's symbol#1715danusha2345 wants to merge 8 commits into
Conversation
8c71e12 to
95f07e8
Compare
|
Ran the vite arm you said was not re-run on the final head. It supports the patch: 2,487 edge rows removed, 0 added, and every one of the 2,487 is attributable to a bare import. I went in expecting the opposite. #1713 tried guarding Method
Removed by kind: Every removal is a true positiveFor each of the 2,487 removed rows I read the source file and found the specifier that binds that name:
100.00%. Nothing was removed from a project-internal import, which is the failure mode that would matter. Concretely, the top targets:
The unresolved count rising by 2,509 is the intended cost: a declined reference is counted as unresolved, which is the honest state for an import whose target is not in the graph. One gap, and it is not a fault in this patchIt removes 0 edges targeting a node named The guard is behaving as designed. Nit
LimitsGraph effects only. I did not reproduce your suite run — your 4,258 passing on Linux/Node 24.15.0 stands unchecked by me. My classifier takes the first import statement binding each name, which is sound in JS/TS since one name cannot have two import bindings in a file, but it is a regex over source rather than the resolver's own view. |
…ymchenry#1715 supersedes the bare-import row Two things a reader cannot check from the table. The millisecond figures depend on a machine-local Windows Defender exclusion. The corpus sat under an excluded directory and the interpreter in Program Files; on this host an unexcluded directory read by an untrusted binary costs ~12.6s for one 1.2MB file. All three arms shared the configuration so the comparison holds, but the absolute totals do not transfer to a stock host. And the bare-import row is no longer the best available fix. colbymchenry#1715 moves the guard into matchByExactName, where these references actually resolve, and measured on the same corpus against the same merge base it removes 2,487 edge rows to this change's 4, adding none. All 2,487 have a source file importing that name from a bare npm package or node builtin, so the removals are precision gains rather than a regression.
…0 of them The paragraph described the 60 as the class. They are the fork's share of it. Measured on main: 157 cross-file `imports` rows resolve onto a single target, and it is not a file-level symbol as written but a `const` at module scope in `playground/ssr-html/test-stacktrace.js`, a file with no exports at all. None of the six changes fix the cause. Neither does colbymchenry#1715, which removes 0 of the 157 while removing 2,487 other wrong edges. A non-exported top-level binding is admitted as a cross-file exact-match candidate, filed as colbymchenry#1719. Table figures unchanged; only the explanation and the markdown-index cell move.
|
Thanks for running the arm — 2,487 removed / 0 added with every row read back to a bare package or a builtin is the number this PR needed and did not have on its final head. I've put it in the description, and rewritten the description in English (the last revision had slipped into Russian). Nit fixed in On the |
|
Already filed — #1719. Don't open a second one. Two things in it are sharper than what I wrote above, because I went looking for the target after posting and it is not what I described. The class is 157 rows, not 60. On The target is not the member's entry being missed — it is a non-exported local. It is const vite = await createServer({ /* ... */ })at module scope in a file with zero exports. So the framing we agreed on — workspace name should resolve to the member's entry rather than a same-named symbol elsewhere — is right about the symptom, but the cause is more general than the workspace branch: I put both framings in #1719 and did not pick one, since the export-visibility fix would subsume the workspace one and has the wider blast radius. That is the maintainer's call, not mine. Thanks for |
…mport The consumer bound every name from 'some-external-pkg'. A bare specifier names a package that is not in the graph, so no project node is the right target for such a reference and colbymchenry#1715 declines it -- which made four of the five positive assertions depend on a resolution that should not happen, and they failed the moment this branch was stacked on colbymchenry#1715. Free references reach the same exact-match path without asserting that. `strayVar` was not testable at all: a bare identifier read emits no edge, so that assertion only ever passed through the bare-import binding. The `declare global` coverage moves to an interface reached through a type annotation, paired with an identical file whose interface is not in a `declare global` -- so the assertion turns on that clause rather than passing whichever way the guard goes.
…cross-file candidate (#1719) (#1746) * fix(resolution): a binding in a module that exports nothing is not a cross-file candidate On vitejs/vite, 157 cross-file `imports` refs — every `import { defineConfig } from 'vite'` in the playground and the create-vite templates — resolved onto `playground/ssr-html/test-stacktrace.js::vite`, which is `const vite = await createServer(...)` at module scope in a file with zero exports. Neither existing guard can see it. `isLexicallyReachable` returns early for any candidate that is not a `function`, and the bare-import guard correctly declines because `vite` IS a workspace member, so the specifier really is project-local. What is wrong is only which node the name lands on. A JS/TS file that contains an `import` statement and no export of any form offers nothing to any other file, so none of its bindings is a candidate for a cross-file name match. Applied in both name-based strategies: declining in matchByExactName alone just hands the same target to matchFuzzy, which resolves a unique candidate on its own. Narrow on three axes, each a class this would otherwise get wrong in the opposite direction: a classic script is exempt (a top-level binding really is a reachable global), CommonJS is exempt (`module.exports` and `exports.x` count as exports), and every non-JS/TS language is exempt. The export test reads source rather than the node's `isExported` flag, because that flag is set only from an `export_statement` ancestor and so reads false for `const x = ...; export { x }`. * fix(resolution): count bracket CommonJS exports and `declare global` as exports A file writing `exports["x"] = …` exports x, and a file with a `declare global` block contributes every name in it to every other file whether or not it exports anything of its own — the extractor emits nodes for the ambient `var` and `interface` members, so sealing such a file would hide names that really are reachable everywhere. Neither shape occurs on the vite corpus, so this changes no measured count; both are now covered by the test. * test(resolution): bind the #1719 fixture without a bare import The consumer bound every name from 'some-external-pkg'. A bare specifier names a package that is not in the graph, so no project node is the right target for such a reference and #1715 declines it -- which made four of the five positive assertions depend on a resolution that should not happen, and they failed the moment this branch was stacked on #1715. Free references reach the same exact-match path without asserting that. `strayVar` was not testable at all: a bare identifier read emits no edge, so that assertion only ever passed through the bare-import binding. The `declare global` coverage moves to an interface reached through a type annotation, paired with an identical file whose interface is not in a `declare global` -- so the assertion turns on that clause rather than passing whichever way the guard goes. * docs(changelog): record the sealed-module guard under Unreleased * fix(resolution): the sealed test rejects fuzzy's survivor, never filters its set matchFuzzy declines an ambiguous name outright, so filtering sealed candidates out of its set can leave a lone survivor and manufacture a 0.5 edge from an ambiguity that would have been declined. Testing the single survivor instead closes that path; matchByExactName keeps the filter, because it ranks a crowd rather than declining one. No instance on vitejs/vite either way (row-identical, LOST 0 / GAINED 0 per #1720 review). It also declines one shape the filter form resolved: a sealed same-language survivor no longer yields to a cross-language candidate at 0.3. * fix(resolution): reject invalid fallback targets without retargeting --------- Co-authored-by: Aaron Queen <bompus@users.noreply.github.com> Co-authored-by: Colby McHenry <colbymchenry@users.noreply.github.com>
d0efd27 to
905cadf
Compare
`serialize(this.raw)` inside `Record.serialize`, with a module-scope `function serialize` in the same file, resolved onto the method itself: both were exact-name candidates, both same-file, and findBestMatch's line-proximity term always prefers the enclosing method (colbymchenry#1714). In JS/TS a call written without a receiver cannot reach a method at all — methods need `this.`, an object, or a bound reference. The extractor emits `this.m()` and `super.m()` under the bare method name, so the receiver is read back from the call site's own line (the ref's column is the start of the call expression): when the text there begins with the name itself and nothing but whitespace, an operator or an opener precedes it, the call is bare, and `method` nodes leave the candidate set before ranking. matchFuzzy declines a lone `method` survivor for the same ref. `this.serialize()` (recursion) and `other.serialize()` are unchanged. Standalone on vite this removes 566 method-bound bare calls (`log(…)` onto a spec file's `log` method, `import(…)` onto a runner method, `resolve(…)` onto PluginContainer.resolve) and lets 274 previously out-ranked candidates through — `resolve` bound by `import { resolve } from 'node:path'` and Promise-callback `resolve` parameters — which colbymchenry#1715 and a local-binding rule are for; measured in the stack it is purely subtractive. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Rebased onto current |
905cadf to
99c96cd
Compare
… 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.
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.
… file's symbol The exact-name strategy has the single-survivor trap the fuzzy one had (a320ed1): `import { test } from 'vitest'` linked every `test(...)` in a spec to the one project function called `test` — on vite, a fixture, 1,747 times — and `import { resolve } from 'node:path'` linked to a plugin container's `resolve` method. matchByExactName now drops the candidates from other files when isBoundToBareImport says the binding is a builtin or an npm package; a same-file definition stays, since a local declaration shadows the import. This is what the exact-match attempt 6d36f3f's parent reverted, and the reason it lost 4,048 is the prefix rule 6d36f3f fixed: with `~utils` classed as bare, its 1,395 real edges on vite went with the wrong ones. vitejs/vite@8492422, indexed at a320ed1 and at this change, edge sets joined back to symbols: 2,536 lost (1,757 calls, 642 imports, 129 references, 6 instantiates), every one bound through vitest, node:path, node:fs, node:http, rolldown, picocolors, kill-port, escape-html and the like; 33 gained, of which 29 are the SAME wrong `PluginContext` targets re-resolved by a framework resolver once exact-match stepped aside, 2 are a `declare module 'rolldown'` augmentation in the importing file, and 2 are a playground dedupe fixture. fuzzy stays at 9. The `~utils` edges are all present. The predicate is exported, and optional-called on getImportMappings: a minimal context (the older resolution.test.ts mocks) carries none, and without mappings nothing is known to be bare. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…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.
The one-line comment had drifted above readLinkDepNames's block in 3bea4a3, leaving readPackageName undocumented and readLinkDepNames with two. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
99c96cd to
d45cefa
Compare
|
Rebased onto current |
…external-binding # Conflicts: # src/resolution/name-matcher.ts
|
Merged current |
2 participants
Stacked on #1713 — its commits come first here (
a320ed1,6d36f3f,3bea4a3,d146c37, merged as7429ca2); this PR's own change is the rest, and it rebases to a single commit once #1713 lands. It is the exact-match half that #1713's description sets aside ("I also tried the same guard inmatchByExactName… 4,048 edges lost — so I reverted it"), with the measurement and with the reason the first attempt lost real edges.What
matchByExactNamehas the same single-survivor commitmatchFuzzyhad: one candidate with the right name, and the reference is bound to it regardless of what the call site's own name is bound to. On vite:The change: when
isBoundToBareImport(from #1713) says the binding is a builtin or an npm package,matchByExactNamedrops the candidates from other files before the single-survivor / best-match logic runs. A same-file definition stays — a local declaration shadows the file-level import, and that is what the reference then means.What the predicate has to keep
Exact-match is where a name imported through a specifier the resolver cannot follow actually resolves, so a false "external" here is a real edge lost. That is what cost the first attempt 4,048 on vite, and each of these is now covered:
~utils,#types/x,$lib/…— alias-looking prefixes no npm name can start with (fix(resolution): a name bound to a bare import resolves to no project node #1713's6d36f3f; vite's~utilsalone carried 1,395 real edges).link:/file:dependencies outside every workspace glob (fix(resolution): a name bound to a bare import resolves to no project node #1713's3bea4a3, found by @bompus on vitest's@vitest/bundled-lib).import { x } from 'lib/utils'under abaseUrldeclared in a nested tsconfig the alias loader never reads (eb9fddf, memoised per context, cleared with the other name-matcher memos). A Node builtin stays external even when a same-named directory exists (path/).getImportMappings: the olderresolution.test.tsmocks carry none, and without mappings nothing is known to be bare.Measurement
vitejs/vite at
8492422, edge sets joined back to symbol names and call-site lines (two indexes from one build differ by 0 edges).Against #1713's first commit (
a320ed1, the earlier head of this branch): LOST 2,536, GAINED 33. Every lost edge grouped by the import that bound the name at the call site:vitest1,747,node:path261,rolldown126,node:fs65,picocolors44,node:http33,kill-port22,escape-html12, … — not one through~utils,#types/…or a relative path. Of the 33 gained, 29 are the same wrongPluginContexttargets coming back through a framework resolver at 0.8 once exact-match stepped aside (unchanged in the graph, onlyresolvedBymoved; that resolver does not consult the binding either and is a separate fix), 2 areplugin.ts's owndeclare module 'rolldown'augmentation, which the same-file rule keeps, 2 a playground dedupe fixture.Against
main(b9ca4b7) on the current head, @bompus's independent run (below): −2,487 edges, 0 gained, and every one of the 2,487 reads back to a bare npm package (2,088) or a Node builtin (399) — relative, alias and unbound: 0.Tests
__tests__/exact-match-bare-import-binding.test.tsdrives the whole pipeline over source fixtures, because a bare-import binding with exactly one same-named project definition is precisely the shape that routes throughmatchByExactName: five bindings that must not bind cross-file (node:path, barepath, an aliased named import, a default import from an npm name, a scoped deep path — ablating the guard fails exactly these five); a same-file definition that still shadows the import; six bindings the resolver cannot follow that must keep their name match (~utils,#lib/utils,@/lib/utils,$lib/utils,src/lib/utils, an unresolvable relative path), plus a name bound by no import at all. Apath/directory in the fixture pins that a builtin stays external.__tests__/local-import-bindings.test.ts:link:andfile:dependencies in a workspace member and at the root without workspaces, including a subpath import, and a root-directory import under a nestedbaseUrl. The nested-baseUrlcase fails beforeeb9fddfand passes after; the four link/file cases fail against the guard without3bea4a3.Full suite on Linux, Node 24.15.0, native kernel built from this checkout and confirmed loaded with
CODEGRAPH_KERNEL_DEBUG=1: 239 files, 4,258 passed, 11 skipped, 0 failed.npm run buildincluding the viewer assets: clean.Known residual, not in scope
import { defineConfig } from 'vite'inside vite's own playground still lands onplayground/ssr-html/test-stacktrace.js::vite(60 edges): the workspace map rightly saysviteis project-local, and the name match then picks any node calledviterather than the member's entry. That is the workspace branch resolving a package name to a same-named node, separate from this change — tracked as #1719, fix in #1720 (@bompus).🤖 Generated with Claude Code