Skip to content

fix(resolution): fuzzy reachability rejects a unique guess, never manufactures one - #1718

Open
danusha2345 wants to merge 3 commits into
colbymchenry:mainfrom
danusha2345:fix/1708-fuzzy-reachability-on-survivor
Open

fix(resolution): fuzzy reachability rejects a unique guess, never manufactures one#1718
danusha2345 wants to merge 3 commits into
colbymchenry:mainfrom
danusha2345:fix/1708-fuzzy-reachability-on-survivor

Conversation

@danusha2345

Copy link
Copy Markdown
Contributor

Fixes #1708. Standalone off main (b9ca4b7), two commits: #1709's two-file fixture, credited to @bompus as author, and the change.

What

A function nested inside another function is only callable from inside its container (#1230). matchByExactName already declined such candidates; matchFuzzy did not, so a builtin method call (settled.value.text(), items.push()) whose only same-named project symbol was some file's closure resolved onto that closure at 0.5.

#1709 fixed that by filtering the candidate set the way exact-match does. On vite that traded 12 correct removals for 59 wrong additions (its author's measurement, reproduced exactly on the same checkout): the repo has a dozen resolve definitions, most nested, so the filter left exactly one reachable resolve method and the strategy committed every import { resolve } from 'node:path' call in the playground configs to it. Filtering a crowd down to one survivor is not evidence the survivor was ever the target.

So the check sits on the one candidate matchFuzzy would commit to, not on the set:

if (finalCandidates.length === 1 && isLexicallyReachable(finalCandidates[0]!, ref, context)) {

A unique candidate the call cannot reach is declined; a crowd stays a crowd. Reachability may reject a unique guess; it never manufactures one.

Measurement

vitejs/vite at 8492422, indexed at this branch's base (b9ca4b7) and at its tip, edge sets joined back to symbol names and call-site lines (two indexes from one build differ by 0 edges):

resolvedBy base this delta
fuzzy 13 1 −12
exact-match 10715 10715 0
import 5361 5361 0
qualified-name / instance-method / function-ref / file-path / framework / (none) 0

Edge-set delta: LOST 12, GAINED 0. The 12 are all fuzzy edges onto nested functions — the same 12 #1709 removes (config.ts:resolveConfig → importAnalysis.ts:getEnv, scan.ts:build → scan.ts:scan, shared.js → client.ts:wait, main.ts → counter.ts:decrement, …). The 59 pluginContainer.ts:resolve edges never appear.

For the record, the same tree with #1709's filter is base → LOST 12, GAINED 59 (fuzzy 13 → 60); with #1709 + #1713 it is +3 net. This change alone is −12 / +0, and it composes with #1713 rather than overlapping it: #1713 answers "is the target even in the graph", this answers "did filtering create the uniqueness". The one fuzzy edge left on vite is preload/src/main.js → utils.ts:loader, a lone reachable candidate, which is the strategy working as designed.

Tests

__tests__/fuzzy-lexical-reach.test.ts:

  • fix(resolution): fuzzy matching skips a nested function the reference cannot reach #1709's fixture, unchanged: a nested function text() in one file, settled.value.text() in another; no calls edge onto the closure, the in-container call still resolves.
  • Four direct matchFuzzy cases pinning the shape: a lone unreachable closure declines; the same closure resolves from inside its container; closure + method is ambiguous and declines; a lone reachable method resolves as before.

Ablation: with the change removed, the fixture and the first direct case fail (2 of 5); with #1709's candidate-set filter in its place, exactly the "closure + method" case fails (1 of 5). Resolution suites 250/250, tsc clean.

🤖 Generated with Claude Code

bompus and others added 2 commits September 6, 2026 13:25
…'s closure

The two-file reachability fixture from colbymchenry#1709: a `function text()` nested in
one file, a `settled.value.text()` call in another. The caller must not get
a `calls` edge onto the closure; the in-container call still resolves.
…ufactures one

A function nested inside another function is only callable from inside its
container (colbymchenry#1230). matchByExactName already declined such candidates; the
fuzzy fallback did not, so a builtin method call (`res.text()`) whose only
same-named project symbol was some file's closure resolved onto that
closure at 0.5 (colbymchenry#1708).

colbymchenry#1709 fixed that by filtering the candidate set the way exact-match does,
and on vitejs/vite@8492422 that traded 12 correct removals for 59 wrong
additions: the repo has a dozen `resolve` definitions, most nested, so the
filter left exactly one reachable `resolve` method and the strategy
committed every `import { resolve } from 'node:path'` call in the
playground configs to it. Filtering a crowd down to one survivor is not
evidence the survivor was ever the target.

So the check sits on the ONE candidate matchFuzzy would commit to: a
unique candidate the call cannot reach is declined; a crowd stays a crowd.
Same tree, measured against this branch's own base b9ca4b7: 12 edges lost
(all fuzzy, all onto nested functions — the same 12 colbymchenry#1709 removes), 0
gained, fuzzy 13 -> 1, every other resolvedBy row at zero.

The two-file fixture is colbymchenry#1709's, credited in the previous commit; four
direct tests pin the shape: a lone unreachable closure declines, the same
closure resolves from inside its container, closure + method is ambiguous
and declines (the candidate-set filter fails exactly this one), a lone
reachable method resolves as before.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown

This is the right fix and mine was wrong. I read the diff rather than the description, and the distinction you're drawing is load-bearing.

Putting the check on the one candidate matchFuzzy would commit to, instead of filtering the candidate set, is not a smaller version of #1709 — it's the opposite operation. My filter changed which set the strategy saw, and a filter that thins a crowd to one survivor hands that survivor every call of the name. Yours changes only whether a uniqueness that already existed is trusted.

That makes your GAINED 0 stronger than you claimed it. You reported it as measured on vite; it's structural. A guard on the finalCandidates.length === 1 branch can only turn a return into a decline, so it can't emit an edge the old code didn't. The one thing that could break that argument is fallthrough — if declining there routed the reference into some later edge-producing strategy, the guard could reach a path it previously never did. It doesn't:

  • matchReference: fuzzy is step 4 of 4 — if (result) return result; then return null. Nothing runs after it.
  • the bare-name recovery in the factory/fluent path: matchByExactName(bareRef) ?? matchFuzzy(bareRef), then return bareMatch ? … : null. Also terminal, and exact-match runs before fuzzy, so a decline can't route back into it either.

So declining is terminal on both call sites and GAINED 0 holds by construction. That's a better guarantee than any measurement, because it doesn't depend on vite being representative.

On #1713 — worth saying plainly that it isn't redundant with this, and this isn't redundant with it. They guard opposite sides: #1713 guards the reference (the name is bound to an external import specifier, so no project node is correct no matter how few candidates survive or how reachable they are), this guards the candidate (the lone survivor is a closure the call site can't reach). import { scan } from 'rolldown/experimental' landing on the importing file's own top-level scan is a uniquely-surviving, lexically-reachable candidate — your guard passes it, only the import binding rejects it. res.text() onto a closure is the reverse. They also edit matchFuzzy at different points, so a textual conflict is unlikely.

I'll close #1709 pointing here. One correction I owe on #1713: its body currently argues it removes 44 of #1709's 59 false positives, which was an argument about #1709's filter-the-set behavior. Under your change those 59 are never manufactured, so that benefit doesn't exist any more and I'm taking the claim out. What stands on its own is #1713's standalone measurement, which never depended on #1709 — vite at 8492422, LOST 4, GAINED 0, purely subtractive. If this lands first I'll re-measure it stacked on this rather than on the merge-base, so the numbers describe the tree it would actually enter.

Thanks for the authorship credit on the fixture.

@bompus

bompus commented Sep 6, 2026

Copy link
Copy Markdown

A correction to my review, in your favour: this PR is stronger than I credited it.

I ran the stacked arms on vite at the merge-base of both PRs (b9ca4b7). #1718 alone takes fuzzy 13 → 1, LOST 12 GAINED 0, and that 12 includes all 4 edges my #1713 targets. Stacking #1713 on top removes nothing further — the edge sets are identical.

When I reviewed this I said the two guards were complementary and gave scan and getEnv as candidates yours would pass because they were top-level. The graph disagrees: both are scanImports::scan and importAnalysisPlugin::getEnv, nested functions, exactly the shape isLexicallyReachable rejects. I had read vite's source instead of querying the node's qualifiedName. On this corpus your guard covers the cases mine was written for.

They do still check different things — with this PR applied and mine ablated, my two must-decline tests fail, because their candidate is a method that passes isLexicallyReachable at its first line — but that is a predicate difference with no instance on vite, and I have said so on #1713 rather than claiming a reduction on top of yours.

Same lesson as the daemon probe, one turn later: I argued the interaction from reading rather than running the arms together. Details on #1713.

isLexicallyReachable trusted the graph's nesting for every language. C and
C++ have no nested named functions, so a function shown inside another is an
extraction artifact: tree-sitter-c cannot parse a macro call whose arguments
are designated initializers — betaflight's

    RESET_CONFIG(pidProfile_t, pidProfile, .pid = { … }, …);

— and its error recovery runs the enclosing function_definition (source lines
168–309) to line 1667, nesting the 45 functions after it. That tree has 310
such functions in 73 files. Before this commit exact-match already rejected
them as unreachable and the fuzzy fallback picked them up at 0.5; with the
survivor-side check alone, fuzzy rejected them too and 117 real calls into
pid.c disappeared (base → 4c8f165 on the 2,109-file betaflight fork: LOST
117, GAINED 0, all fuzzy, all pid.c).

With the gate the same tree is LOST 117 fuzzy / GAINED 117 exact-match — the
identical edges, now resolved by the strategy that should have had them, at
0.9. vite (no C) is unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@danusha2345

Copy link
Copy Markdown
Contributor Author

Pushed 5eb5750, a second commit: the survivor-side check had a regression on C that vite could not show, found by running this branch on a 2,109-file C tree (a betaflight fork).

What happened. base → 4c8f165 on that tree: LOST 117, GAINED 0, all fuzzy, all calls into src/main/flight/pid.ccore.c:processRx → pidSetItermReset, blackbox.c:loadMainState → pidGetPreviousSetpoint, and so on. Those are real calls. The graph shows every one of those callees nested inside resetPidProfile (qualifiedName resetPidProfile::pidSetItermReset), and the reason is extraction, not code: resetPidProfile is source lines 168–309, but tree-sitter-c cannot parse

RESET_CONFIG(pidProfile_t, pidProfile,
    .pid = { [PID_ROLL] = PID_ROLL_DEFAULT, … },
    …
);

— a macro call whose arguments are designated initializers — and its error recovery runs the function_definition to line 1667, so the 45 functions after it become its children. The tree has 310 such "nested" C functions in 73 files (STM32 HAL sources have the same shape). On main exact-match already rejected them as unreachable (#1230's check runs there too) and the fuzzy fallback picked them up at 0.5; with the check on fuzzy's survivor as well, nothing did.

The fix. C and C++ have no nested named functions, so a function the graph shows inside another can only be an artifact: isLexicallyReachable now returns true for c/cpp candidates before it looks at the nesting. Same tree, base → 5eb5750: LOST 117 fuzzy / GAINED 117 exact-match — the identical rows, now resolved by the strategy that should have had them, at 0.9 instead of 0.5. vite is row-identical to 4c8f165 (it has no C). One direct test pins it (a "nested" C candidate resolves from another file); 195 across fuzzy-lexical-reach + resolution.test.ts.

Kept the gate to the two languages the evidence is about. Python, Rust, Swift, Kotlin and JS/TS all have real nested functions, and #1230's fixture is Python.

Two things from the same run that are not this PR's, for the record: the same tree has 4,448 cross-file calls resolved by exact-match onto a static function in another file (the C analogue of #1719 — a static is file-local by definition), and the extraction extent bug itself is worth its own issue against the C extractor since it also mis-scopes callers/callees for those 310 functions. I'll file both separately rather than fold them in here.

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.

matchFuzzy resolves onto a nested function that matchByExactName had already rejected as lexically unreachable

2 participants