fix(resolution): fuzzy reachability rejects a unique guess, never manufactures one - #1718
Conversation
…'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>
|
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 That makes your GAINED 0 stronger than you claimed it. You reported it as measured on vite; it's structural. A guard on the
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). 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 Thanks for the authorship credit on the fixture. |
|
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 ( When I reviewed this I said the two guards were complementary and gave 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 Same lesson as the daemon probe, one turn later: I argued the interaction from reading rather than running the arms together. Details on #1713. |
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).
matchByExactNamealready declined such candidates;matchFuzzydid 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
resolvedefinitions, most nested, so the filter left exactly one reachableresolvemethod and the strategy committed everyimport { 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
matchFuzzywould commit to, not on the set: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):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 59pluginContainer.ts:resolveedges 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:function text()in one file,settled.value.text()in another; nocallsedge onto the closure, the in-container call still resolves.matchFuzzycases 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,
tscclean.🤖 Generated with Claude Code