From f8f4c44b285806993314cb382fe6c879a12b1197 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 18:28:39 +0200 Subject: [PATCH 1/3] A scope utility answers what a cursor can name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reach is a property of a declaration kind, not of the frame holding it, so walking out from a cursor and collecting what each frame declares directly answers only part of the question. `var` and function declarations reach the whole function they sit in, which puts one in a sibling block in scope at the cursor without ever enclosing it: function f() { if (x) { var merge = 1; } // binds `merge` across all of f anchor(); // `merge` here is the var } `scopeOf(cursor)` answers by 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 a generic one 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. A shape whose reach cannot be read is counted rather than missed, because a name counted in error costs a suffix while a name missed emits a reference that binds to the wrong thing. `namesDeclaredIn(cu)` answers the question a file-scoped binding asks instead: not what is in scope at one point, but every name the file binds anywhere. Such a binding is referenced from sites that are not known when it is named, and the union of what is in scope across all of them is what the file declares. A class member is left out, being reached through an instance rather than by name. `bindingNames` reads a binding pattern once for every caller, and covers array patterns and rest names alongside the object patterns. Both walks are remembered against the subtree they read. Asking a 500-call-site function what it hoists at every site took 1323ms of an otherwise 33ms traversal, and asking a 200-function file what it declares 500 times took 3761ms; kept, those are 143ms and 11ms. --- .../rewrite/src/javascript/index.ts | 1 + .../rewrite/src/javascript/scope.ts | 290 ++++++++++++++++++ .../rewrite/test/javascript/scope.test.ts | 224 ++++++++++++++ 3 files changed, 515 insertions(+) create mode 100644 rewrite-javascript/rewrite/src/javascript/scope.ts create mode 100644 rewrite-javascript/rewrite/test/javascript/scope.test.ts diff --git a/rewrite-javascript/rewrite/src/javascript/index.ts b/rewrite-javascript/rewrite/src/javascript/index.ts index a4bc48e4f12..7e0ddc986fd 100644 --- a/rewrite-javascript/rewrite/src/javascript/index.ts +++ b/rewrite-javascript/rewrite/src/javascript/index.ts @@ -31,6 +31,7 @@ export * from "./autodetect"; export * from "./tree-debug"; export * from "./project-parser"; +export * from "./scope"; export * from "./add-import"; export * from "./remove-import"; export * from "./cleanup/index"; diff --git a/rewrite-javascript/rewrite/src/javascript/scope.ts b/rewrite-javascript/rewrite/src/javascript/scope.ts new file mode 100644 index 00000000000..5c95ea0acbe --- /dev/null +++ b/rewrite-javascript/rewrite/src/javascript/scope.ts @@ -0,0 +1,290 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import {Cursor, isTree} from "../tree"; +import {J} from "../java"; +import {JS} from "./tree"; + +/** The names code at some position can reach unqualified. */ +export interface Scope { + /** Whether this scope or one enclosing it binds `name`. */ + declares(name: string): boolean; + + names(): ReadonlySet; +} + +/** + * What the code at `cursor` can name: what every enclosing scope binds, plus the declarations that + * hoist into those scopes from blocks that do not enclose it. Where a declaration's shape leaves its + * reach unreadable the answer counts it rather than miss it, so a name reported here may not truly + * reach the cursor. + */ +export function scopeOf(cursor: Cursor): Scope { + let resolved: Set | undefined; + const names = () => resolved ??= namesInScope(cursor); + return {declares: name => names().has(name), names}; +} + +/** + * Every name the file declares, wherever it sits. A binding the whole file shares is referenced from + * sites that are not known when it is named, so it has to steer clear of every name that could + * shadow it at one of them. + */ +export function namesDeclaredIn(cu: JS.CompilationUnit): ReadonlySet { + const cached = declared.get(cu); + if (cached) { + return cached; + } + + const names = new Set(); + const collect = (node: any): boolean => { + declarationNames(node).forEach(name => names.add(name)); + if (node.kind !== J.Kind.ClassDeclaration) { + return true; + } + // A member's name is not one the file binds, though the code inside one still declares names. + for (const member of (node as J.ClassDeclaration).body?.statements ?? []) { + const element = unwrap(member); + walk(element, node => node === element || collect(node)); + } + return false; + }; + walk(cu.statements, collect); + + declared.set(cu, names); + return names; +} + +/** + * Every name a binding pattern introduces. `member` is the property a name takes its value from, + * which only a name an object pattern binds directly has: anything deeper reads a property of a + * property, an array element is chosen by position, and a rest name gathers what nothing claimed. + */ +export function bindingNames(pattern: J | undefined): { name: string; member?: string }[] { + switch (pattern?.kind) { + case J.Kind.Identifier: { + const simpleName = (pattern as J.Identifier).simpleName; + return simpleName ? [{name: simpleName}] : []; + } + case JS.Kind.Spread: + return bindingNames((pattern as JS.Spread).expression); + case JS.Kind.ArrayBindingPattern: + return unnamedMembers((pattern as JS.ArrayBindingPattern).elements.elements + .flatMap(element => bindingNames(unwrap(element)))); + case JS.Kind.ObjectBindingPattern: + return (pattern as JS.ObjectBindingPattern).bindings.elements + .flatMap(element => bindingNames(unwrap(element))); + case JS.Kind.BindingElement: { + const element = pattern as JS.BindingElement; + if (element.name?.kind !== J.Kind.Identifier) { + return unnamedMembers(bindingNames(element.name as J)); + } + const name = (element.name as J.Identifier).simpleName; + const propertyName = unwrap(element.propertyName); + return [{ + name, + member: propertyName?.kind === J.Kind.Identifier ? (propertyName as J.Identifier).simpleName : name + }]; + } + default: + return []; + } +} + +function unnamedMembers(bound: { name: string }[]): { name: string }[] { + return bound.map(({name}) => ({name})); +} + +function namesInScope(cursor: Cursor): Set { + const names = new Set(); + for (let c: Cursor | undefined = cursor; c; c = c.parent) { + for (const name of frameBindings(c.value, c.parent?.value)) { + names.add(name); + } + } + return names; +} + +/** What one node on the cursor path binds, `parent` being the node it hangs from. */ +function frameBindings(node: any, parent: any): string[] { + switch (node?.kind) { + case JS.Kind.CompilationUnit: { + const statements = (node as JS.CompilationUnit).statements; + return [...declaredNames(statements), ...hoistedNames(statements)]; + } + case J.Kind.Block: + // A class body holds members, which are reached through an instance rather than by name. + return parent?.kind === J.Kind.ClassDeclaration ? [] : declaredNames((node as J.Block).statements); + case J.Kind.MethodDeclaration: { + const method = node as J.MethodDeclaration; + // A function expression is the only function whose own name its body reaches: a + // declaration's name belongs to the enclosing block, a method's to an instance. + const self = parent?.kind === JS.Kind.StatementExpression ? bindingNames(method.name) : []; + return [ + ...self.map(bound => bound.name), + ...declaredNames(method.parameters.elements), + ...hoistedNames(method.body) + ]; + } + case J.Kind.Lambda: { + const lambda = node as J.Lambda; + return [...declaredNames(lambda.parameters.parameters), ...hoistedNames(lambda.body)]; + } + case J.Kind.ClassDeclaration: + return bindingNames((node as J.ClassDeclaration).name).map(bound => bound.name); + case J.Kind.TryCatch: + return declaredNames([(node as J.Try.Catch).parameter.tree]); + case J.Kind.ForLoop: + return declaredNames((node as J.ForLoop).control.init); + case J.Kind.ForEachLoop: + return declaredNames([(node as J.ForEachLoop).control.variable]); + case JS.Kind.ForInLoop: + return declaredNames([(node as JS.ForInLoop).control.variable]); + default: + return []; + } +} + +/** The names statements declare directly in the scope holding them. */ +function declaredNames(statements: any[]): string[] { + return statements.flatMap(statement => declarationNames(unwrap(statement))); +} + +function declarationNames(statement: any): string[] { + switch (statement?.kind) { + case JS.Kind.Import: + return importNames(statement as JS.Import); + case J.Kind.VariableDeclarations: + return (statement as J.VariableDeclarations).variables + .flatMap(variable => bindingNames(unwrap(variable)?.name)) + .map(bound => bound.name); + case JS.Kind.ScopedVariableDeclarations: + return declaredNames((statement as JS.ScopedVariableDeclarations).variables); + case J.Kind.MethodDeclaration: + return bindingNames((statement as J.MethodDeclaration).name).map(bound => bound.name); + case J.Kind.ClassDeclaration: + return bindingNames((statement as J.ClassDeclaration).name).map(bound => bound.name); + case JS.Kind.NamespaceDeclaration: + return bindingNames(unwrap((statement as JS.NamespaceDeclaration).name)).map(bound => bound.name); + case JS.Kind.TypeDeclaration: + return bindingNames(unwrap((statement as JS.TypeDeclaration).name)).map(bound => bound.name); + case J.Kind.Case: + // The cases of a switch share the block it opens, so each one's declarations bind in all. + return declaredNames((statement as J.Case).statements.elements); + default: + return []; + } +} + +/** The names an import binds, which for an aliased or namespace specifier is the alias. */ +function importNames(jsImport: JS.Import): string[] { + const importClause = jsImport.importClause; + if (!importClause) { + return []; + } + const names = bindingNames(unwrap(importClause.name)).map(bound => bound.name); + const namedBindings = importClause.namedBindings; + if (namedBindings?.kind === JS.Kind.NamedImports) { + for (const element of (namedBindings as JS.NamedImports).elements.elements) { + const specifier = unwrap(element); + if (specifier?.kind === JS.Kind.ImportSpecifier) { + names.push(...aliasedName((specifier as JS.ImportSpecifier).specifier)); + } + } + } else { + names.push(...aliasedName(namedBindings)); + } + return names; +} + +function aliasedName(specifier: J | undefined): string[] { + const name = specifier?.kind === JS.Kind.Alias ? (specifier as JS.Alias).alias : specifier; + return bindingNames(name).map(bound => bound.name); +} + +const blockScoped = new Set(['let', 'const', 'using']); + +// A walk of an immutable subtree has one answer, so it runs once. Keyed on the subtree itself, a +// replaced one is walked afresh: every call site in a function asks what that body hoists, and every +// import added to a file asks what that file declares. +const hoisted = new WeakMap(); +const declared = new WeakMap>(); + +/** + * The names blocks under `scope` hoist out to it. A `var` or function declaration reaches the whole + * function it sits in, so one nested in a block is in scope outside that block; only a `let`, `const` + * or `using` keyword says otherwise. + */ +function hoistedNames(scope: any): string[] { + if (typeof scope !== 'object' || scope === null) { + return []; + } + const cached = hoisted.get(scope); + if (cached) { + return cached; + } + + const names: string[] = []; + const collect = (node: any): boolean => { + switch (node.kind) { + case J.Kind.MethodDeclaration: + names.push(...declarationNames(node)); + return false; + case J.Kind.VariableDeclarations: + case JS.Kind.ScopedVariableDeclarations: + if (!((node.modifiers ?? []) as J.Modifier[]).some(m => blockScoped.has(m.keyword!))) { + names.push(...declarationNames(node)); + } + return false; + case J.Kind.TryCatch: + // A catch parameter carries no keyword to read, and binds only in the catch block. + walk((node as J.Try.Catch).body, collect); + return false; + case JS.Kind.StatementExpression: + // A function or class used as an expression names itself for its own body alone. + return false; + case J.Kind.Lambda: + case J.Kind.ClassDeclaration: + return false; + default: + return true; + } + }; + walk(scope, collect); + hoisted.set(scope, names); + return names; +} + +/** Visits every LST node under `node`, leaving a subtree unvisited where `visit` returns false. */ +function walk(node: unknown, visit: (node: any) => boolean): void { + if (Array.isArray(node)) { + node.forEach(child => walk(child, visit)); + return; + } + const kind = (node as any)?.kind; + if (kind === J.Kind.RightPadded || kind === J.Kind.LeftPadded) { + walk((node as J.RightPadded).element, visit); + } else if (kind === J.Kind.Container) { + walk((node as J.Container).elements, visit); + } else if (isTree(node) && visit(node)) { + // Markers hang off a node rather than being part of the code it holds. + Object.entries(node).forEach(([key, value]) => key !== 'markers' && walk(value, visit)); + } +} + +/** The element a padding wrapper holds, or the node itself. */ +function unwrap(node: any): any { + return node?.kind === J.Kind.RightPadded || node?.kind === J.Kind.LeftPadded ? unwrap(node.element) : node; +} diff --git a/rewrite-javascript/rewrite/test/javascript/scope.test.ts b/rewrite-javascript/rewrite/test/javascript/scope.test.ts new file mode 100644 index 00000000000..c72de73b64c --- /dev/null +++ b/rewrite-javascript/rewrite/test/javascript/scope.test.ts @@ -0,0 +1,224 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { + bindingNames, + JavaScriptParser, + JavaScriptVisitor, + JS, + namesDeclaredIn, + Scope, + scopeOf, + sourceFileCache +} from "../../src/javascript"; +import {J} from "../../src/java"; + +const parser = new JavaScriptParser({sourceFileCache}); + +async function parse(source: string, sourcePath = 'test.ts'): Promise { + return (await parser.parse({text: source, sourcePath}).next()).value as JS.CompilationUnit; +} + +/** The scope where the source calls `anchor()`. */ +async function scopeAtAnchor(source: string, sourcePath?: string): Promise { + const found: Scope[] = []; + await new class extends JavaScriptVisitor { + override async visitMethodInvocation(method: J.MethodInvocation, p: undefined): Promise { + if ((method.name as J.Identifier)?.simpleName === 'anchor') { + found.push(scopeOf(this.cursor)); + } + return super.visitMethodInvocation(method, p); + } + }().visit(await parse(source, sourcePath), undefined); + expect(found).toHaveLength(1); + return found[0]; +} + +async function namesAtAnchor(source: string, sourcePath?: string): Promise { + return [...(await scopeAtAnchor(source, sourcePath)).names()].sort(); +} + +describe('scopeOf', () => { + test('a module binds every form of import', async () => { + expect(await namesAtAnchor(` + import def from 'a'; + import {named, other as renamed} from 'b'; + import * as ns from 'c'; + import 'd'; + anchor(); + `)).toEqual(['def', 'named', 'ns', 'renamed']); + }); + + test('a module binds what its declarations name', async () => { + expect(await namesAtAnchor(` + const c = 1; + let l = 2; + var v = 3, w = 4; + function f() {} + class K {} + namespace N {} + type T = string; + const {r} = require('m'); + anchor(); + `)).toEqual(['K', 'N', 'T', 'c', 'f', 'l', 'r', 'v', 'w']); + }); + + test('a var reaches the anchor from a sibling block, a let does not', async () => { + const names = await namesAtAnchor(` + function f() { + if (x) { + var hoisted = 1; + let confined = 2; + } + anchor(); + } + `); + expect(names).toContain('hoisted'); + expect(names).not.toContain('confined'); + }); + + test('a block binds its declarations wherever the anchor sits in it', async () => { + // `const` binds its whole block, so referencing it above its declaration is a temporal dead + // zone error rather than a reference to whatever the name means outside. + expect(await namesAtAnchor(` + function f() { + anchor(); + const later = 1; + } + `)).toContain('later'); + }); + + test("a switch's cases share one scope", async () => { + expect(await namesAtAnchor(` + switch (x) { + case 1: + let s = 1; + break; + case 2: + anchor(); + } + `)).toContain('s'); + }); + + test('a function binds its parameters, destructured and rest included', async () => { + expect(await namesAtAnchor(` + function f(plain, {prop, other: renamed, nested: {deep}}, [first], ...rest) { + anchor(); + } + `)).toEqual(['deep', 'f', 'first', 'plain', 'prop', 'renamed', 'rest']); + }); + + test('an arrow function binds its parameters', async () => { + expect(await namesAtAnchor(`const f = (a, {b}) => anchor();`)).toEqual(['a', 'b', 'f']); + }); + + test('a catch binds its parameter in its block alone', async () => { + expect(await namesAtAnchor(`try {} catch ({message}) { anchor(); }`)).toContain('message'); + + expect(await namesAtAnchor(`try {} catch (err) {} anchor();`)).not.toContain('err'); + }); + + test('a loop binds what its control declares', async () => { + expect(await namesAtAnchor(`for (let i = 0; i < 2; i++) { anchor(); }`)).toContain('i'); + + expect(await namesAtAnchor(`for (const [key, value] of pairs) { anchor(); }`)) + .toEqual(expect.arrayContaining(['key', 'value'])); + + expect(await namesAtAnchor(`for (const prop in obj) { anchor(); }`)).toContain('prop'); + }); + + test('a class binds its own name, not its members', async () => { + // Only a class expression's name is bound solely inside the class — the block holding a + // declaration binds its name too. + const names = await namesAtAnchor(` + const C = class K { + member() { anchor(); } + }; + `); + expect(names).toContain('K'); + expect(names).not.toContain('member'); + }); + + test('a nested function keeps its declarations to itself', async () => { + const names = await namesAtAnchor(` + function outer() { + function inner(param) { + var hidden = 1; + } + anchor(); + } + `); + expect(names).toEqual(['inner', 'outer']); + }); + + test('a function expression binds the name it gives itself, and only there', async () => { + expect(await namesAtAnchor(`const f = function named() { anchor(); };`)).toContain('named'); + + expect(await namesAtAnchor(`function f() { (function named() {})(); anchor(); }`)).not.toContain('named'); + }); + + test('a scope reaches through the JSX between it and the cursor', async () => { + // A handler prop is where a template most often lands in a component, and JSX is all the + // cursor crosses to get from the lambda binding `evt` out to the module. + expect(await namesAtAnchor( + `import R from 'r';\nfunction App() { return ; }`, + 'test.tsx' + )).toEqual(['App', 'R', 'evt']); + }); + + test('a binding pattern names the property each name reads', async () => { + const cu = await parse(`const {plain, other: renamed, nested: {deep}, ...rest} = o;`); + const declaration = cu.statements[0].element as J.VariableDeclarations; + expect(bindingNames(declaration.variables[0].element.name)).toEqual([ + {name: 'plain', member: 'plain'}, + {name: 'renamed', member: 'other'}, + // Neither reads a property of the object the pattern destructures. + {name: 'deep'}, + {name: 'rest'} + ]); + }); + + test('declares answers for a name any enclosing scope binds', async () => { + const scope = await scopeAtAnchor(`import {merge} from 'm';\nfunction f(param) { anchor(); }`); + expect(scope.declares('merge')).toBe(true); + expect(scope.declares('param')).toBe(true); + expect(scope.declares('absent')).toBe(false); + }); +}); + +describe('namesDeclaredIn', () => { + test('a name declared in any scope is a name the file has', async () => { + expect([...namesDeclaredIn(await parse(` + import imported from 'm'; + const top = 1; + function fn(param) { + if (c) { var nested = 2; } + try {} catch (caught) {} + for (const [element] of pairs) {} + return (inline) => inline; + } + `))].sort()).toEqual(['caught', 'element', 'fn', 'imported', 'inline', 'nested', 'param', 'top']); + }); + + test('a class member is reached through an instance, so its name is not the file\'s', async () => { + // What a member's own code declares is still a name the file has. + expect([...namesDeclaredIn(await parse(` + class K { + merge(param) { const local = 1; } + field = (bound) => bound; + } + `))].sort()).toEqual(['K', 'bound', 'local', 'param']); + }); +}); From da42d37e6c0f90dc2e2db36bb5fee15fe322093e Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 18:28:39 +0200 Subject: [PATCH 2/3] maybeAddImport clears every scope its binding is referenced from The pool counted what each frame between the anchor and the compilation unit declares directly, which left the gap that walk was named for: a `var` or function declaration binds its whole function, so one in a sibling block is in scope at the anchor and the walk never looked there. Asking about the anchor at all is the deeper problem. An import binds at module scope, and the queue answers every later request for that module with the name the first one chose, so a single 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. A name one binding shares has to clear every scope in the file, which is what `namesDeclaredIn` reports. That splits the two questions the binding list answered at once. Only an import or `require` at module scope answers for a name, so the reuse lookup reads module scope alone and `moduleScopeBindings` narrows to the imports and `require`s that carry a module. Every other declaration it used to scan is a name the file binds and nothing more. `patternNames` moved to `scope.ts` as `bindingNames`, leaving one reader of a binding pattern for the two callers that need one. --- .../rewrite/src/javascript/add-import.ts | 139 +++--------------- .../test/javascript/add-import.test.ts | 63 +++++++- 2 files changed, 80 insertions(+), 122 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/add-import.ts b/rewrite-javascript/rewrite/src/javascript/add-import.ts index 05d641a9362..3acfa297ebf 100644 --- a/rewrite-javascript/rewrite/src/javascript/add-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/add-import.ts @@ -5,6 +5,7 @@ import {randomId} from "../uuid"; import {emptyMarkers, markers} from "../markers"; import {getStyle, PrettierStyle, SpacesStyle, StyleKind} from "./style"; import {Cursor} from "../tree"; +import {bindingNames, namesDeclaredIn} from "./scope"; export type QuoteChar = "'" | '"'; @@ -143,19 +144,18 @@ export function maybeAddImport( } const derived = derivedName(options); - const cu = compilationUnitOf(visitor); + const cursor = cursorOf(visitor); + const cu = cursor && compilationUnitOf(cursor); if (!cu) { visitor.afterVisit.push(new AddImport(options, derived)); return derived; } - const bindings = bindingsInScope(cu, cursorOf(visitor)); - // An import already serving this request answers it; queuing one would, on the next cycle, // derive a suffixed name from the binding this call just added. A caller that named a preference // takes whatever comes back; one that did not assumes the name it derived, so a binding under // any other name would leave the references it emits unbound. - for (const binding of bindings) { + for (const binding of moduleScopeBindings(cu)) { if (binding.module === module && binding.member === memberName(options.member) && binding.typeOnly === typeOnly && (options.preferredName !== undefined || anyNameAnswers(options) || @@ -164,7 +164,10 @@ export function maybeAddImport( } } - const name = deconflict(derived, takenNames(bindings, visitor)); + // Only the module scope answers for a name, but any scope in the file occupies one. The queue + // gives every later request for this module the name chosen here, so it has to clear the scopes + // those references will sit in, which are not known yet. + const name = deconflict(derived, takenNames(namesDeclaredIn(cu), visitor)); visitor.afterVisit.push(new AddImport(options, name)); return name; } @@ -230,73 +233,29 @@ interface ModuleScopeBinding { } function cursorOf(visitor: JavaScriptVisitor): Cursor | undefined { - return (visitor as unknown as { cursor?: Cursor }).cursor; -} - -function compilationUnitOf(visitor: JavaScriptVisitor): JS.CompilationUnit | undefined { // `cursor` is protected on `TreeVisitor`, and the `maybeAddImport`/`maybeRemoveImport` // API is free functions, so reaching it takes a cast. - return cursorOf(visitor)?.firstEnclosing((v): v is JS.CompilationUnit => v?.kind === JS.Kind.CompilationUnit); + return (visitor as unknown as { cursor?: Cursor }).cursor; } -/** Every name in scope at `cursor`: what the file binds, and what each block enclosing it declares. */ -function bindingsInScope(cu: JS.CompilationUnit, cursor: Cursor | undefined): ModuleScopeBinding[] { - const bindings = statementBindings(cu.statements); - // A local shadows an import for the code the template lands in, so a name a block declares is - // taken there even though the module never answers for it. - for (let c = cursor; c && c.value !== cu; c = c.parent) { - for (const name of scopeNames(c.value)) { - bindings.push({name}); - } - } - return bindings; +function compilationUnitOf(cursor: Cursor): JS.CompilationUnit | undefined { + return cursor.firstEnclosing((v): v is JS.CompilationUnit => v?.kind === JS.Kind.CompilationUnit); } -/** The names a scope introduces directly: a block's declarations, or a function's parameters. */ -function scopeNames(scope: unknown): string[] { - const node = scope as J | undefined; - if (node?.kind === J.Kind.Block) { - return statementBindings((node as J.Block).statements).map(binding => binding.name); - } - if (node?.kind === J.Kind.MethodDeclaration) { - return (node as J.MethodDeclaration).parameters.elements - .flatMap(param => param.element?.kind === J.Kind.VariableDeclarations - ? (param.element as J.VariableDeclarations).variables.flatMap(v => patternNames(v.element?.name)) - : patternNames(param.element)) - .map(bound => bound.name); - } - if (node?.kind === J.Kind.Lambda) { - return (node as J.Lambda).parameters.parameters - .flatMap(param => param.element?.kind === J.Kind.VariableDeclarations - ? (param.element as J.VariableDeclarations).variables.flatMap(v => patternNames(v.element?.name)) - : patternNames(param.element)) - .map(bound => bound.name); - } - return []; -} - -/** Imports are top-level statements and so is anything that can shadow one, so a flat scan sees every name. */ -function statementBindings(statements: J.RightPadded[]): ModuleScopeBinding[] { +/** What the file's imports and `require`s bind at module scope, and the module each name comes from. */ +function moduleScopeBindings(cu: JS.CompilationUnit): ModuleScopeBinding[] { const bindings: ModuleScopeBinding[] = []; - const declaredBy = (name: J | undefined): void => { - for (const bound of patternNames(name)) { - bindings.push({name: bound.name}); - } - }; - const declaredByVariables = (varDecl: J.VariableDeclarations): void => { for (const variable of varDecl.variables) { const required = requiredModule(variable.element?.initializer?.element); if (required !== undefined) { bindings.push(...requireBindings(variable.element?.name, required)); - } else { - declaredBy(variable.element?.name); } } }; - for (const stmt of statements) { + for (const stmt of cu.statements) { const statement = stmt.element; switch (statement?.kind) { case JS.Kind.Import: @@ -312,18 +271,6 @@ function statementBindings(statements: J.RightPadded[]): ModuleScopeB } } break; - case J.Kind.MethodDeclaration: - declaredBy((statement as J.MethodDeclaration).name); - break; - case J.Kind.ClassDeclaration: - declaredBy((statement as J.ClassDeclaration).name); - break; - case JS.Kind.NamespaceDeclaration: - declaredBy((statement as JS.NamespaceDeclaration).name.element); - break; - case JS.Kind.TypeDeclaration: - declaredBy((statement as JS.TypeDeclaration).name.element); - break; } } @@ -357,55 +304,13 @@ function requiredModuleOf(methodInv: J.MethodInvocation): string | undefined { : undefined; } -/** - * Every name a binding pattern introduces. `member` is the property a name takes its value from, - * and only a name bound directly by the pattern has one — anything deeper reads a property of a - * property, so it occupies its name without binding a member of the module. - */ -function patternNames(pattern: J | undefined): { name: string; member?: string }[] { - if (pattern?.kind === J.Kind.Identifier) { - return [{name: (pattern as J.Identifier).simpleName}]; - } - // An array pattern binds by position, so its elements name no member of what they destructure. - if (pattern?.kind === JS.Kind.ArrayBindingPattern) { - return (pattern as JS.ArrayBindingPattern).elements.elements - .flatMap(elem => elem.element?.kind === JS.Kind.BindingElement - ? patternNames((elem.element as JS.BindingElement).name) - : patternNames(elem.element)) - .map(bound => ({name: bound.name})); - } - if (pattern?.kind !== JS.Kind.ObjectBindingPattern) { - return []; - } - const names: { name: string; member?: string }[] = []; - for (const elem of (pattern as JS.ObjectBindingPattern).bindings.elements) { - if (elem.element?.kind !== JS.Kind.BindingElement) { - continue; - } - const bindingElem = elem.element as JS.BindingElement; - if (bindingElem.name?.kind === J.Kind.Identifier) { - const name = (bindingElem.name as J.Identifier).simpleName; - const propertyName = bindingElem.propertyName?.element; - names.push({ - name, - member: propertyName?.kind === J.Kind.Identifier - ? (propertyName as J.Identifier).simpleName - : name - }); - } else { - names.push(...patternNames(bindingElem.name).map(bound => ({name: bound.name}))); - } - } - return names; -} - /** A `require` binds a module the way an import does, so the pool records it the same way. */ function requireBindings(pattern: J | undefined, module: string): ModuleScopeBinding[] { // A whole-module require binds no member, exactly as a default import does. if (pattern?.kind === J.Kind.Identifier) { return [{name: (pattern as J.Identifier).simpleName, module, member: undefined, typeOnly: false}]; } - return patternNames(pattern).map(bound => bound.member === undefined + return bindingNames(pattern).map(bound => bound.member === undefined ? {name: bound.name} : {name: bound.name, module, member: bound.member, typeOnly: false}); } @@ -465,12 +370,12 @@ function importBindings(jsImport: JS.Import): ModuleScopeBinding[] { } /** - * Names the file binds, plus those pending `AddImport`s on the `afterVisit` queue have claimed. A - * queued `RemoveImport` does not free one: it removes only what the file leaves unused, and binding - * a name it keeps is an error, where an unnecessary suffix merely reads oddly. + * Names in scope, plus those pending `AddImport`s on the `afterVisit` queue have claimed. A queued + * `RemoveImport` does not free one: it removes only what the file leaves unused, and binding a name + * it keeps is an error, where an unnecessary suffix merely reads oddly. */ -function takenNames(bindings: ModuleScopeBinding[], visitor: JavaScriptVisitor): Set { - const taken = new Set(); +function takenNames(inScope: ReadonlySet, visitor: JavaScriptVisitor): Set { + const taken = new Set(inScope); for (const v of visitor.afterVisit || []) { if (v instanceof AddImport && v.bindingName) { @@ -478,10 +383,6 @@ function takenNames(bindings: ModuleScopeBinding[], visitor: JavaScriptVisitor { const spec = new RecipeSpec(); const bound: string[] = []; spec.recipe = fromVisitor(new class extends JavaScriptVisitor { - // Each anchor asks for a different module, so the queue never answers one from - // another and every scope is resolved on its own. + // Each anchor asks for a different module, so the queue never answers one from another. override async visitMethodInvocation(method: J.MethodInvocation, p: any): Promise { if ((method.name as J.Identifier)?.simpleName === 'anchor') { bound.push(maybeAddImport(this, @@ -3040,11 +3039,19 @@ describe('AddImport visitor', () => { } const byLambda = (merge) => anchor(); + + function byHoistedVar() { + if (x) { + var merge = 1; + } + anchor(); + } `, ` import {merge as merge_1} from 'm0'; import {merge as merge_2} from 'm1'; import {merge as merge_3} from 'm2'; + import {merge as merge_4} from 'm3'; function byParameter(merge) { anchor(); @@ -3056,11 +3063,61 @@ describe('AddImport visitor', () => { } const byLambda = (merge) => anchor(); + + function byHoistedVar() { + if (x) { + var merge = 1; + } + anchor(); + } + ` + ) + ); + + expect(bound).toEqual(['merge_1', 'merge_2', 'merge_3', 'merge_4']); + }); + + test('one binding serves every site, so a name any of them shadows is taken', async () => { + const spec = new RecipeSpec(); + const bound: string[] = []; + spec.recipe = fromVisitor(new class extends JavaScriptVisitor { + override async visitMethodInvocation(method: J.MethodInvocation, p: any): Promise { + if ((method.name as J.Identifier)?.simpleName === 'anchor') { + bound.push(maybeAddImport(this, + {module: 'sap/base/util/merge', member: 'merge', onlyIfReferenced: false})); + } + return super.visitMethodInvocation(method, p); + } + }); + + await spec.rewriteRun( + typescript( + ` + function free() { + anchor(); + } + + function shadowed(merge) { + anchor(); + } + `, + ` + import {merge as merge_1} from 'sap/base/util/merge'; + + function free() { + anchor(); + } + + function shadowed(merge) { + anchor(); + } ` ) ); - expect(bound).toEqual(['merge_1', 'merge_2', 'merge_3']); + // `free` would take plain `merge`, but the one import both sites reference has to clear + // the parameter too, or the reference in `shadowed` binds to it. + expect(bound).toEqual(['merge_1', 'merge_1']); }); test('a request that named nothing takes the name the file already gives the module', async () => { From 2b240dfb75cf35190752efbbf4aa30399f6d1f26 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 21:55:41 +0200 Subject: [PATCH 3/3] maybeRemoveImport reads which member a destructured binding takes 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 string was matched against the member being removed. The two coincide only where the binding is shorthand, so the shorthand case worked and every other one silently did nothing. A nested pattern yielded the empty string and so never matched either. The named-import path already reads the two apart, matching the member and checking the local name for uses. `bindingNames` reports both, so the element states the member it reads and the name it binds, and `shouldRemoveImport` is given each. A name a nested pattern binds reads a property of a property and so reads no member of the module at all, which now says to keep it rather than saying its member is its own name. That distinction is what the member parameter carries, so it has no default: an argument passed explicitly as `undefined` takes a default value, which is precisely what a nested pattern passes to say it reads nothing. --- .../rewrite/src/javascript/remove-import.ts | 41 +++++++++---------- .../test/javascript/remove-import.test.ts | 34 +++++++++++++++ 2 files changed, 54 insertions(+), 21 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/remove-import.ts b/rewrite-javascript/rewrite/src/javascript/remove-import.ts index 94f72935957..370712600e9 100644 --- a/rewrite-javascript/rewrite/src/javascript/remove-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/remove-import.ts @@ -1,5 +1,6 @@ import {JavaScriptVisitor} from "./visitor"; import {J} from "../java"; +import {bindingNames} from "./scope"; import {JS, JSX} from "./tree"; import {mapAsync, updateIfChanged} from "../util"; import {ElementRemovalFormatter} from "../java"; @@ -268,7 +269,7 @@ export class RemoveImport

extends JavaScriptVisitor

{ shouldRemove = !usedIdentifiers.has(name) && !usedTypes.has(name); } else { // Regular case: check if the import name matches the removal criteria - shouldRemove = this.shouldRemoveImport(name, usedIdentifiers, usedTypes); + shouldRemove = this.shouldRemoveImport(name, usedIdentifiers, usedTypes, name); } if (shouldRemove) { @@ -325,7 +326,7 @@ export class RemoveImport

extends JavaScriptVisitor

{ }, p); } // Namespace is used, we can't remove individual members from it - } else if (this.shouldRemoveImport(name, usedIdentifiers, usedTypes)) { + } else if (this.shouldRemoveImport(name, usedIdentifiers, usedTypes, name)) { // If there's no default import, remove the entire import if (!importClause.name) { return undefined; @@ -355,7 +356,7 @@ export class RemoveImport

extends JavaScriptVisitor

{ }, p); } // Namespace is used, we can't remove individual members from it - } else if (this.shouldRemoveImport(aliasName, usedIdentifiers, usedTypes)) { + } else if (this.shouldRemoveImport(aliasName, usedIdentifiers, usedTypes, aliasName)) { // If there's no default import, remove the entire import if (!importClause.name) { return undefined; @@ -490,7 +491,7 @@ export class RemoveImport

extends JavaScriptVisitor

{ return true; // Keep imports that don't match the member } else { // We're removing based on the import name itself - return !this.shouldRemoveImport(importName, usedIdentifiers, usedTypes); + return !this.shouldRemoveImport(importName, usedIdentifiers, usedTypes, importName); } } return true; // Keep non-ImportSpecifier elements @@ -612,14 +613,10 @@ export class RemoveImport

extends JavaScriptVisitor

{ const {filtered, allRemoved} = await this.filterElementsWithPrefixPreservation( pattern.bindings.elements, (elem: J) => { - if (elem.kind === JS.Kind.BindingElement) { - const name = this.getBindingElementName(elem as JS.BindingElement); - return !this.shouldRemoveImport(name, usedIdentifiers, new Set()); - } else if (elem.kind === J.Kind.Identifier) { - const name = (elem as J.Identifier).simpleName; - return !this.shouldRemoveImport(name, usedIdentifiers, new Set()); - } - return true; // Keep other element types + const bound = this.boundByPatternElement(elem); + // An element whose names cannot be read is left alone. + return bound.length === 0 || + bound.some(b => !this.shouldRemoveImport(b.name, usedIdentifiers, new Set(), b.member)); }, async (elem: J, prefix: J.Space) => { if (elem.kind === J.Kind.Identifier) { @@ -687,23 +684,25 @@ export class RemoveImport

extends JavaScriptVisitor

{ return undefined; } - private getBindingElementName(bindingElement: JS.BindingElement): string { - const name = bindingElement.name; - if (name?.kind === J.Kind.Identifier) { - return (name as J.Identifier).simpleName; - } - return ''; + /** The names a pattern element binds, and for each the member of the module it reads. */ + private boundByPatternElement(elem: J): { name: string; member?: string }[] { + // Shorthand, so the name it binds is the member it reads. + return elem.kind === J.Kind.Identifier + ? [{name: (elem as J.Identifier).simpleName, member: (elem as J.Identifier).simpleName}] + : bindingNames(elem); } private shouldRemoveImport( name: string, usedIdentifiers: Set, - usedTypes: Set + usedTypes: Set, + member: string | undefined ): boolean { // If member is specified, we're removing a specific member from the module if (this.member !== undefined) { - // Only remove if this is the specific member we're looking for - if (this.member !== name) { + // A name bound under an alias reads one member and is referenced by another, and a name + // a nested pattern binds reads a property of a property, so it reads no member at all. + if (this.member !== member) { return false; } } diff --git a/rewrite-javascript/rewrite/test/javascript/remove-import.test.ts b/rewrite-javascript/rewrite/test/javascript/remove-import.test.ts index bf4a2b7aefd..b663100af88 100644 --- a/rewrite-javascript/rewrite/test/javascript/remove-import.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/remove-import.test.ts @@ -585,6 +585,40 @@ describe('RemoveImport visitor', () => { ); }); + test('should remove a destructured require member bound under another name', async () => { + const spec = new RecipeSpec(); + spec.recipe = fromVisitor(new RemoveImport("fs", "readFile")); + + //language=typescript + await spec.rewriteRun( + typescript( + ` + const {readFile: rf, writeFile} = require('fs'); + + function example() { + writeFile('test.txt', 'content', () => { + }); + } + `, + ` + const {writeFile} = require('fs'); + + function example() { + writeFile('test.txt', 'content', () => { + }); + } + ` + ), + // `readFile` here reads a property of `a`, not a member of the module, so nothing + // the module exports is bound and there is nothing to remove even though it is unused. + typescript( + ` + const {a: {readFile}} = require('fs'); + ` + ) + ); + }); + test('should remove destructured require', async () => { const spec = new RecipeSpec(); spec.recipe = fromVisitor(new RemoveImport("fs", "readFile"));