Repository navigation
fix(ts): a call to a parameter of any type links nothing elsewhere - #2732
Merged
Merged
Conversation
A JS/TS name the calling function binds as a parameter is that parameter, never a same-named function in another file, as the file's own were made in #2226. The parameter reader took a type or default to be anything up to a comma or a parenthesis, so a parameter typed as a function, defaulted to one, or after one was never seen: bitwarden's isAvailable(f) on a callback typed (f: string) => boolean linked to the napi isAvailable, and vscode's merge editor's compare(a, b) on a comparator parameter to strings.ts's. A type or default may now hold balanced parentheses, braces, brackets and generics, a parameter property's modifiers are read, and an earlier destructured parameter is passed over; each alternative still has one parse. A name ends where no identifier character follows, so a const syncState$ no longer declares syncState. The parameter also shadows the file's own import of the name, which the import resolver answered before the check ran, but only where it is in scope: in the body of the function, arrow or catch clause that binds it, or the block or for statement of a const, let, var, function or class, read from the function's text up to the reference. A lone arrow parameter counts, and an object type a function returns is no body. A Promise executor's resolve is the calls inside it, never a resolve(file) after it, and a const X = require('./x').default is the import itself. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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
A name the calling JS/TS function binds itself is that binding. #2226 holds a call to a parameter or local to that rule, so it never reaches a same-named function elsewhere. Three gaps let such calls through:
pick(formats, isAvailable: (f: string) => boolean)callingisAvailable(f)linked to the napiisAvailable;compare(a, b)on a comparator parameter linked tostrings.ts'scompare;setTimeout(callback, delay)insideuseCallback((callback: () => void, delay: number) => …)linked to thedelaycallback declared below it.new Promise((resolve) => { … resolve(x) })in a file importingpath'sresolvelinked topath.resolve, and atelemetry: TelemetryDataparameter passed on linked to the importedtelemetryfunction.$was read as the name without it.\bstops before a$. So legend-state'sconst syncState$ = syncState(obs$)counted as declaringsyncState, andconst $elcounted as declaring$, whilefunction $()did not.Follow-up 3 of 3 from #2718.
Fix
All of it is in
src/resolution/name-matcher.ts, plus one gate inindex.ts.Parameter reader (
localBindingPatterns'param):PARAM_PART). A piece is a character other than a comma, or a balanced(…),{…},[…]or<…>group up to two levels deep, which may hold commas.=>is a function type's arrow,=not followed by>starts a default, and a<or>spaced like an operator is no bracket. A failing search therefore stays linear. On lists of 12 to 30 parameters typed(a: A, b: B) => RorMap<K, (v: V) => W> = cond > 1 ? a : b, it takes microseconds.private readonly store: Store) and an earlier destructured parameter are passed over.bindsAtSitesfinds the(that opens a list by reading back over balanced groups (enclosingOpenParen). It no longer looks for "the last(with no)between", which no list holding a function type passes.Names end at
(?![\w$])in every binding pattern, never at\b.Import shadow (
isImportShadowedByJsBinding, a gate inresolveOneafter resolution):jsBindingEnclosesReferencereads the function's text from its first line to the reference's column. Comments, strings, and template and regex literals are blanked first.catchclause's block, or an arrow's block or expression. An expression ends at a comma, a semicolon, the bracket around it, or a line that starts the next statement where no semicolon ends the last.resolve => …), only where an expression starts. The name a return type ends with ((): Bounds =>) doesn't count.const/let/var,functionorclassdeclaration's block, or aforhead's statement.delta(…): { removed: T[]; added: T[] } {, the body starts at the second{.const X = require('./x').default: it is the import.isLocallyBoundJsName, read once per file and name, answers before the read. So the read only runs where the file binds the name at all.Decisions (made without asking)
function resolve() {}nested whereresolveis imported, isn't linked in its place. Main linked the import there.({ compare }) => compare()). fix(js): a name the calling function binds is its local #2226 left destructured declarations out the same way:const { t } = useI18n()usually re-binds the same function.const $ = dom.$; $('div')is one (2 edges). Every other name already behaved this way; it only linked before because\bhid the$local. Following such an alias to its target is a separate change.vardeclared in a closed block, a comma inside an arrow expression's generics, and a function longer than 100,000 characters up to the reference.Tests
__tests__/js-function-local-binding.test.tshas 12 tests: the 2 existing lodash ones and 10 in a new describe, built from the shapes the A/B found.resolve, in both the(resolve) =>andresolve =>forms;for (const resolve of …)body;function resolve;resolve(file)after an executor or aforstatement;const syncState$ = syncState(obs$);const X = require('./x').default; new X();(): Widget => new Widget();Sizetype annotation next to aSizeparameter.Each mutation below makes at least one test fail:
[^,()])(, no)between)\bsyncState$testrequirelocal hides the importrequiretestforhead's declaration scoped like a block'sfortestresolve =>testfunction/classdeclaration(): Widget =>testSizetestA/B
Arms. Dist and kernel snapshots of main
009e4377and of this change on it. The head here is the same change rebased onto5994edb3; #2730 and #2731 touchedname-matcher.tsonly away from these hunks. Each repo gotcodegraph init -yper arm, with two resolver workers for vscode and frontend. Edges were diffed by natural key (an SQLEXCEPTfor vscode).fnparameter that linked toTTDDialog'sfn;newElementas an arrow parameter or aconst newElement = this.state.newElement(3);line/curveparameters passed on as values (3)generator,target,configTypeandt(new t(…)on anew (r: any) => Tparameter) parameters;IconComponent(…)on a function-typed parameterequal,setValue,getEntityIdandlocalizeparameters typed as functionsresolveparameters typed as functions (2), a Promise executor'sresolve,toggleBlock,build,replace,diffget/setparameters that linked toObservableObject's;syncStateparameters passed onpluginparameter; aComponent: Crendered as<Component>;useTimeout'sdelayvscode:
resolve→path.resolve;basename?: string→path.basename;telemetry: TelemetryData→ the importedtelemetryfunction;transaction?: ITransaction→ the observabletransaction;const dispose = () => …→ lifecycle'sdispose;const status = …→ aria'sstatus;size: IViewSize→ dom'ssize;debounce(event, merge, …)'smergecalled in a nested callback, or are a destructured option, like{ now = Date.now }, in a file that also has anow: () => numberparameter.$alias calls, as described under Decisions.(tree: WorkbenchAsyncDataTree<ITreeItem, ITreeItem, FuzzyScore>),ITreeItemread as a second parameter, so the references were dropped as shadowed.$helper keep their target, at 0.9 instead of 0.7. The file'sfunction $()orconst $ = …now counts, so other files'$drop out of the pool.Earlier runs, all fixed here before this head:
new OsBiometricsServiceMac(…)after aconst OsBiometricsServiceMac = require(…).default, and legend-state'ssyncState(obs$), which is the$bug.resolve()insidenew Promise<void>(resolve => …), and the parameters ofstatWithLink(fsPath: string): Promise<{ … }>anddelta(…, compare: …): { … }.(): CipherView =>cut 32 bitwarden constructions likenew CipherView(), and 36 type annotations in bitwarden, excalidraw and frontend.Cost.
The gate's own time, from the resolver's stage profiler (
CODEGRAPH_RESOLVE_PROFILE=2, summed over workers): 0.6 s of 34.9 s of resolver CPU on outline, and 2.2 s of 141 s on bitwarden. That is under 2%: about 20 µs per call and 45 µs per reference.Check order:
Reordering the checks changed no edge; both repos diff empty against the A/B's indexes.
Whole-index CPU, interleaved, median of 3: outline 48.6 s → 50.7 s and bitwarden 143.2 s → 144.6 s. Paired runs on this machine spread ±7%, so that's within noise.
🤖 Generated with Claude Code