A scope utility for JavaScript LSTs, and the import recipes use it

[knutwannheden]

maybeAddImport deconflicts the local name it returns against what the file already binds, and read reach from the frame a declaration sits in rather than from the declaration itself. Two shapes came out wrong, and both emit a reference that silently resolves to a local instead of the import.

A var or function declaration binds its whole function, so one in a block that does not enclose the anchor is still in scope there — and the walk only looked at frames that do enclose it:

function f() {
    if (x) { var merge = 1; }   // binds `merge` across all of f
    anchor();                   // emitting `merge` here reaches the var, not an import
}

Asking about the anchor at all is the deeper problem. An import binds at module scope, and the afterVisit queue answers every later request for that module with the name the first one chose, so one name serves reference sites that are not known when it is picked:

function free()          { return foo('x'); }
function shadowed(merge) { return foo('y'); }

A rule firing at both sites takes plain merge from the first, where nothing shadows it, and the reference in the second then reads the parameter. Reproduced with two maybeAddImport calls for one module; it predates this branch, since the pool used to scan compilation-unit statements only and never saw the parameter either.

The utility

scope.ts answers both questions a file asks, off one declaration reader:

scopeOf(cursor).declares('merge')   // in scope at a point: shadowing checks, renaming
namesDeclaredIn(cu)                 // every name the file binds anywhere

scopeOf resolves by declaration kind. Each frame contributes what it binds — a block its declarations, a function its parameters, a catch its parameter, a loop its control bindings, a class its own name — and every function frame also contributes what the blocks under it hoist out. That hoisting walk is generic and bounded by the function, so a shape it has not been taught still yields its declarations; only a let, const or using keyword takes a name out of it. Where reach cannot be read the name is counted rather than missed, because counting one in error costs a suffix while missing one emits a reference that binds to the wrong thing.

Why maybeAddImport uses the file-wide answer

Not as a blunt over-approximation of per-site analysis, but because it is the same set: every name in scope at some point is declared somewhere in the file, and every name declared somewhere is in scope at least within its own scope, so the union of the in-scope sets over all program points is the set of declared names, modulo globals that deconfliction never considered. It is exact for the question as posed, and over-approximate only relative to the sites that turn out to fire.

It also removes the conditions for the bug rather than patching it: with one answer per file, the queue's file-wide caching becomes correct. A distinct binding per site was rejected because it means several imports of one module under different aliases; re-resolving per site was rejected because reconciling already-emitted references is what the queue exists to avoid.

Class member names are left out. A method named merge shadows nothing at any site — bare merge inside it resolves to module scope — so counting it would suffix imports against get, render and merge throughout class-heavy code. What a member's own code declares is still counted.

Tests

17 tests exercise the utility directly, one per decision, plus two through maybeAddImport: the reported two-site defect, and a var in a sibling block. Every arm was mutation-checked by disabling it and confirming a test fails. That caught three tests that pinned nothing — the class-name case passed because a top-level class K is bound by the compilation unit too, and the catch-parameter arm was dead because the hoisting walk was wrongly treating a catch parameter as hoisting, which would also have marked e taken across a whole function. Both are fixed and now covered.

Performance

Measured before caching rather than presumed. scopeOf at 500 call sites in a 200-function module cost 1323ms against a 33ms traversal baseline, and namesDeclaredIn for 500 imports into that file cost 3761ms; each walk is now remembered against the subtree it reads, giving 143ms and 11ms. Keyed by node identity in a WeakMap, so a replaced subtree is a miss rather than a stale hit.

A second consumer

maybeRemoveImport had the same class of defect, found by surveying for hand-rolled scope reasoning once the utility existed. A require destructuring a member under another name kept it, whatever the request asked for:

maybeRemoveImport(v, 'fs', 'readFile');
const {readFile: rf, writeFile} = require('fs');   // unchanged

The pattern element yielded one string — the local name — and that was matched against the member being removed. The two coincide only for shorthand, so shorthand worked and everything else silently did nothing; a nested pattern yielded '' and never matched either. bindingNames reports both, so the element now states the member it reads and the name it binds. A name a nested pattern binds reads a property of a property, so it reads no member of the module at all and is kept.

That distinction is why the member parameter has no default: an argument passed explicitly as undefined takes the default value, which is exactly what a nested pattern passes to say it reads nothing. Mutation-checking caught that — the fix looked right and the test still passed for the wrong reason.

Scope

patternNames moves out of add-import.ts into scope.ts as bindingNames, extended to array patterns and rest names, so one reader of a binding pattern now serves add-import.ts, remove-import.ts and the scope walk — that file has been bitten before by two walkers over one shape drifting apart. ChangeImport renames a specifier onto a name the file may already bind, producing a duplicate declaration; that needs alias-form and reference-rewriting decisions beyond this change and is left for a follow-up. Follows #8680. Full suite: 2031 passing.