fix(resolution): a binding in a module that exports nothing is not a cross-file candidate (#1719) - #1746
Merged
Merged
Conversation
…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 }`.
…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.
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.
…ers 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.
…cross-file candidate (#1719) Merge bompus/fix/unexported-module-locals (#1720) onto main after #1745 (file-local visibility). Compose the sealed-module guard with isVisibleAcrossFiles so the post-pipeline gate also rejects JS/TS modules that export nothing — the vite playground shape where `import { defineConfig } from 'vite'` landed on a sealed `const vite`.
This was referenced Sep 8, 2026
colbymchenry
added a commit
that referenced
this pull request
Sep 8, 2026
…1748) A member call whose receiver is itself a call — `d.setdefault(k, []).append(v)`, `make().run()` — used to drop the receiver at extraction time, degrade to the bare method name, and exact-match any top-level project symbol of that name (Python and JavaScript/TypeScript). Keep the inner callee encoded as `<inner>().<method>` in the TS extractor and native kernel; the name-matcher refuses to guess for that shape (store-accessor exception only). Based on #1692, rebased onto main after #1746. Fixes #1683. Co-authored-by: Colby McHenry <colbymchenry@users.noreply.github.com>
This was referenced Sep 8, 2026
This was referenced Sep 8, 2026
inth3shadows
added a commit
to inth3shadows/codegraph
that referenced
this pull request
Sep 8, 2026
…ipper livePythonImportSources hand-rolled its own comment and string scanner. main now carries strip-comments.ts (colbymchenry#1746) with a python arm that does the same job, and this file already composes it one function above for the TS receiver-type read, so the bespoke scanner is the odd one out — and a bespoke one drifts from the extractor it exists to mirror. blankStringContents(stripCommentsForRegex(source, 'python')) blanks comments, triple-quoted docstrings and single-line string contents while preserving offsets, so the line anchoring the `^import` pattern relies on survives. The extractor's own two patterns are unchanged, which is the property that matters: a filter stricter than the thing it filters silently drops real bindings, and an earlier line-anchored version did exactly that. Checked against the cases this path exists for before swapping — a commented-out import, a docstring usage example, a single-line string holding import text, an inline comment after a real import, a triple quote opened and closed on one line, and two shapes that could trip the helper's JS regex- literal heuristic (`(a)/b`, an f-string with a slash). All eight give the same answer as the scanner they replace, including the two the hand-written version got wrong. The null-versus-empty distinction is kept: null means the file could not be read, empty means it was read and every import-looking line was comment or string — the case where every binding must be refused.
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
2 participants
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.
Summary
Lands bompus’s sealed-module guard from #1720 onto current
main(after #1745’s file-local visibility), composed so both gates apply.On
vitejs/vite, everyimport { defineConfig } from 'vite'across the playground resolved ontoplayground/ssr-html/test-stacktrace.js::vite— a module-scopeconst vite = await createServer(…)in a file that exports nothing.matchByExactNamecommitted as soon as one candidate survived; nothing asked whether an import could reach it. That one binding took 157 cross-fileimportsedges.#1745 (
isVisibleAcrossFiles) correctly declined C/static, Kotlin/private, Go unexported, Rust non-pub— and reported LOST 0 / GAINED 0 on vite JS/TS. It does not cover sealed ESM modules.#1720’s fix: a JS/TS file with an
importand no export of any kind (export, CommonJS includingexports["x"],declare global) is sealed — its locals are not cross-file candidates. Classic scripts, CJS, laterexport { … }, and ambient globals stay visible. Import ranking filters sealed candidates before ranking; calls/fuzzy reject the chosen survivor without promoting a runner-up.This PR merges that branch onto
mainand folds the sealed/markdown/JSON reachability predicate intoisVisibleAcrossFilesso the post-pipeline gate from #1745 also covers #1719.Fixes #1719.
Supersedes / lands #1720 (credit: @bompus).
Test plan
main(pre-fix):drops them as cross-file candidates…fails (widgetwrongly reached fromconsumer.js)Bindings in a module that exports nothing (#1719)— 8 passed__tests__/cross-file-visibility.test.ts(fix(resolution): a definition its language makes file-local is not a cross-file target (#1731) #1745) — 11 passed (composition intact)__tests__/frameworks-integration.test.ts— 24 passed