From 7c8a59937ae12e31b53c573bdd10a26bc63a102e Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 12:38:42 +0200 Subject: [PATCH 01/11] maybeAddImport returns the name it bound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `maybeAddImport` was a pure queue push: it appended an `AddImport` to `visitor.afterVisit` and decided nothing at call time, so it could not tell a caller what local name the module ended up bound to. Every decision ran later in `AddImport.visitJsCompilationUnit`. It now resolves the name at call time by reaching the compilation unit from `visitor.cursor` and returns it — the name of an import that already binds the module, or the name the new import will use. Imports and everything that can shadow one are top-level statements, so this stays synchronous: existence, the deconfliction pool and the reservation scan are all flat scans of `cu.statements`. Where no compilation unit is reachable — `maybeAddImport` called from a visitor constructor, as several tests do — the derived name is returned without a lookup. There was no shadowing check anywhere in add-import.ts, so `maybeAddImport({module: 'fs', member: 'readFile'})` into a file already declaring a local `readFile` emitted a colliding binding, which is a SyntaxError. A name derived from `member` or `module` is now deconflicted with a `_1` suffix against module-scope declarations, existing import bindings, and names claimed by `AddImport`s already on the queue, less those a queued `RemoveImport` will free. A bare digit would be ambiguous on names already ending in one, so `base64` deconflicts to `base64_1`. An alias the caller pinned explicitly is honoured verbatim, even into a collision: the caller may already have emitted code naming it, so it owns the consequence. `ChangeImport` uses that to opt out — it moves a binding rather than introducing one, and the import it is replacing is still in the tree `maybeAddImport` reads. Its two named-member branches collapse into one now that an alias equal to the member prints as a plain specifier. `AddImport` carries the resolved name as `bindingName`, which is both what it emits and what `onlyIfReferenced` searches for; previously that search used `alias || member`, which would have looked for the un-deconflicted name. Overloads keep the `undefined` return confined to the side-effect path, where there is no binding to name. --- .../rewrite/src/javascript/add-import.ts | 227 +++++++++++++++++- .../src/javascript/recipes/change-import.ts | 11 +- .../test/javascript/add-import.test.ts | 173 +++++++++++++ 3 files changed, 394 insertions(+), 17 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/add-import.ts b/rewrite-javascript/rewrite/src/javascript/add-import.ts index 07918d4f45f..ce291d59f1f 100644 --- a/rewrite-javascript/rewrite/src/javascript/add-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/add-import.ts @@ -4,6 +4,8 @@ import {JS, JSX} from "./tree"; import {randomId} from "../uuid"; import {emptyMarkers, markers} from "../markers"; import {getStyle, PrettierStyle, SpacesStyle, StyleKind} from "./style"; +import {Cursor} from "../tree"; +import {RemoveImport} from "./remove-import"; export type QuoteChar = "'" | '"'; @@ -32,6 +34,8 @@ export interface AddImportOptions { /** Optional alias for the imported member. * Required when member is 'default' or '*'. + * Taken verbatim, never deconflicted: a caller that names an alias may already have + * emitted code using it. * Cannot be combined with `sideEffectOnly`. */ alias?: string; @@ -60,6 +64,10 @@ export interface AddImportOptions { * Register an AddImport visitor to add an import statement to a JavaScript/TypeScript file * @param visitor The visitor to add the import addition to * @param options Configuration options for the import to add + * @returns The local name the module is bound to: an existing import's, or the one the new import + * will use, suffixed if the file already binds that name. `onlyIfReferenced` defaults to true, so + * the import may never appear, and the name is then what it would have gone by. A side-effect + * import binds no name and returns `undefined`. * * @example * // Add a named import @@ -81,22 +89,217 @@ export interface AddImportOptions { * // Add a side-effect import * maybeAddImport(visitor, { module: 'core-js/stable', sideEffectOnly: true }); */ +export function maybeAddImport( + visitor: JavaScriptVisitor, + options: AddImportOptions & { sideEffectOnly: true } +): undefined; +export function maybeAddImport( + visitor: JavaScriptVisitor, + options: AddImportOptions & { sideEffectOnly?: false } +): string; export function maybeAddImport( visitor: JavaScriptVisitor, options: AddImportOptions -) { +): string | undefined; +export function maybeAddImport( + visitor: JavaScriptVisitor, + options: AddImportOptions +): string | undefined { + const module = moduleNameOf(options.module); for (const v of visitor.afterVisit || []) { if (v instanceof AddImport && - v.module === moduleNameOf(options.module) && + v.module === module && v.quoteStyle === options.quoteStyle && v.member === options.member && v.alias === options.alias && v.sideEffectOnly === (options.sideEffectOnly ?? false) && v.typeOnly === (options.typeOnly ?? false)) { - return; + return v.bindingName; + } + } + + if (options.sideEffectOnly) { + visitor.afterVisit.push(new AddImport(options)); + return undefined; + } + + const derived = options.alias ?? options.member ?? module; + const cu = compilationUnitOf(visitor); + if (!cu) { + visitor.afterVisit.push(new AddImport(options, derived)); + return derived; + } + + const bindings = moduleScopeBindings(cu); + const typeOnly = options.typeOnly ?? false; + + // 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. + if (!options.alias) { + for (const binding of bindings) { + if (binding.module === module && binding.member === options.member && binding.typeOnly === typeOnly) { + return binding.name; + } } } - visitor.afterVisit.push(new AddImport(options)); + + const name = options.alias ?? deconflict(derived, takenNames(bindings, visitor)); + visitor.afterVisit.push(new AddImport(options, name)); + return name; +} + +/** A name introduced into the file's module scope, and where it came from when that is an import. */ +interface ModuleScopeBinding { + name: string; + module?: string; + /** Carries the {@link AddImportOptions.member} spelling, so `undefined` means a default import. */ + member?: string; + typeOnly?: boolean; +} + +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. + const cursor = (visitor as unknown as { cursor?: Cursor }).cursor; + return cursor?.firstEnclosing((v): v is JS.CompilationUnit => v?.kind === JS.Kind.CompilationUnit); +} + +/** Imports are top-level statements and so is anything that can shadow one, so a flat scan sees every name. */ +function moduleScopeBindings(cu: JS.CompilationUnit): ModuleScopeBinding[] { + const bindings: ModuleScopeBinding[] = []; + + const declaredBy = (name: J | undefined): void => { + if (name?.kind === J.Kind.Identifier) { + bindings.push({name: (name as J.Identifier).simpleName}); + } else if (name?.kind === JS.Kind.ObjectBindingPattern) { + for (const elem of (name as JS.ObjectBindingPattern).bindings.elements) { + if (elem.element?.kind === JS.Kind.BindingElement) { + declaredBy((elem.element as JS.BindingElement).name); + } + } + } + }; + + const declaredByVariables = (varDecl: J.VariableDeclarations): void => { + for (const variable of varDecl.variables) { + declaredBy(variable.element?.name); + } + }; + + for (const stmt of cu.statements) { + const statement = stmt.element; + switch (statement?.kind) { + case JS.Kind.Import: + bindings.push(...importBindings(statement as JS.Import)); + break; + case J.Kind.VariableDeclarations: + declaredByVariables(statement as J.VariableDeclarations); + break; + case JS.Kind.ScopedVariableDeclarations: + for (const variable of (statement as JS.ScopedVariableDeclarations).variables) { + if (variable.element?.kind === J.Kind.VariableDeclarations) { + declaredByVariables(variable.element as J.VariableDeclarations); + } + } + break; + case J.Kind.MethodDeclaration: + declaredBy((statement as J.MethodDeclaration).name); + break; + case J.Kind.ClassDeclaration: + declaredBy((statement as J.ClassDeclaration).name); + break; + } + } + + return bindings; +} + +function importBindings(jsImport: JS.Import): ModuleScopeBinding[] { + const moduleSpecifier = jsImport.moduleSpecifier?.element; + const module = moduleSpecifier?.kind === J.Kind.Literal + ? moduleNameOf(moduleSpecifier as J.Literal) + : undefined; + const importClause = jsImport.importClause; + if (!importClause) { + return []; + } + + const typeOnly = importClause.typeOnly; + const bindings: ModuleScopeBinding[] = []; + + if (importClause.name?.element?.kind === J.Kind.Identifier) { + bindings.push({name: (importClause.name.element as J.Identifier).simpleName, module, member: undefined, typeOnly}); + } + + const namedBindings = importClause.namedBindings; + if (namedBindings?.kind === J.Kind.Identifier) { + bindings.push({name: (namedBindings as J.Identifier).simpleName, module, member: '*', typeOnly}); + } else if (namedBindings?.kind === JS.Kind.Alias) { + const alias = (namedBindings as JS.Alias).alias; + if (alias?.kind === J.Kind.Identifier) { + bindings.push({name: (alias as J.Identifier).simpleName, module, member: '*', typeOnly}); + } + } else if (namedBindings?.kind === JS.Kind.NamedImports) { + for (const elem of (namedBindings as JS.NamedImports).elements.elements) { + if (elem.element?.kind !== JS.Kind.ImportSpecifier) { + continue; + } + const specifier = elem.element as JS.ImportSpecifier; + if (specifier.specifier?.kind === J.Kind.Identifier) { + const name = (specifier.specifier as J.Identifier).simpleName; + bindings.push({name, module, member: name, typeOnly}); + } else if (specifier.specifier?.kind === JS.Kind.Alias) { + const alias = specifier.specifier as JS.Alias; + if (alias.propertyName?.element?.kind === J.Kind.Identifier && alias.alias?.kind === J.Kind.Identifier) { + bindings.push({ + name: (alias.alias as J.Identifier).simpleName, + module, + member: (alias.propertyName.element as J.Identifier).simpleName, + typeOnly + }); + } + } + } + } + + return bindings; +} + +/** On the `afterVisit` queue, a pending `AddImport` has claimed a name and a pending `RemoveImport` frees one. */ +function takenNames(bindings: ModuleScopeBinding[], visitor: JavaScriptVisitor): Set { + const taken = new Set(); + const removals: RemoveImport[] = []; + + for (const v of visitor.afterVisit || []) { + if (v instanceof AddImport) { + if (v.bindingName) { + taken.add(v.bindingName); + } + } else if (v instanceof RemoveImport) { + removals.push(v); + } + } + + for (const binding of bindings) { + const removed = binding.module !== undefined && removals.some(r => + r.module === binding.module && (r.member === undefined || r.member === binding.member)); + if (!removed) { + taken.add(binding.name); + } + } + + return taken; +} + +function deconflict(derived: string, taken: Set): string { + if (!taken.has(derived)) { + return derived; + } + let suffix = 1; + while (taken.has(`${derived}_${suffix}`)) { + suffix++; + } + return `${derived}_${suffix}`; } /** @@ -227,8 +430,10 @@ export class AddImport

extends JavaScriptVisitor

{ readonly typeOnly: boolean; readonly style?: ImportStyle; readonly quoteStyle?: QuoteChar; + /** The local name this import binds; `undefined` for a side-effect import. */ + readonly bindingName?: string; - constructor(options: AddImportOptions) { + constructor(options: AddImportOptions, bindingName?: string) { super(); // Validate that alias is provided when member is 'default' @@ -267,6 +472,7 @@ export class AddImport

extends JavaScriptVisitor

{ this.typeOnly = options.typeOnly ?? false; this.style = options.style; this.quoteStyle = options.quoteStyle; + this.bindingName = this.sideEffectOnly ? undefined : bindingName ?? this.alias ?? this.member ?? this.module; } /** @@ -1031,7 +1237,7 @@ export class AddImport

extends JavaScriptVisitor

{ } // Step 2: Look for references that match - const targetName = this.alias || this.member; + const targetName = this.bindingName; const targetModule = this.module; let found = false; const self = this; @@ -1206,7 +1412,7 @@ export class AddImport

extends JavaScriptVisitor

{ prefix: singleSpace, markers: emptyMarkers, annotations: [], - simpleName: this.alias!, + simpleName: this.bindingName!, type: undefined, fieldType: undefined }; @@ -1238,7 +1444,7 @@ export class AddImport

extends JavaScriptVisitor

{ prefix: singleSpace, markers: emptyMarkers, annotations: [], - simpleName: this.alias || this.module, + simpleName: this.bindingName!, type: undefined, fieldType: undefined }; @@ -1313,7 +1519,8 @@ export class AddImport

extends JavaScriptVisitor

{ private createImportSpecifier(): JS.ImportSpecifier { let specifier: J.Identifier | JS.Alias; - if (this.alias) { + // An alias equal to the member says nothing `{member}` alone does not. + if (this.bindingName !== this.member) { // Aliased import: import { member as alias } from 'module' const propertyName: J.Identifier = { id: randomId(), @@ -1332,7 +1539,7 @@ export class AddImport

extends JavaScriptVisitor

{ prefix: singleSpace, markers: emptyMarkers, annotations: [], - simpleName: this.alias, + simpleName: this.bindingName!, type: undefined, fieldType: undefined }; diff --git a/rewrite-javascript/rewrite/src/javascript/recipes/change-import.ts b/rewrite-javascript/rewrite/src/javascript/recipes/change-import.ts index d7f87c733cc..22b255c4b7d 100644 --- a/rewrite-javascript/rewrite/src/javascript/recipes/change-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/recipes/change-import.ts @@ -164,17 +164,14 @@ export class ChangeImport extends Recipe { alias: aliasToUse, onlyIfReferenced: false }); - } else if (aliasToUse && aliasToUse !== newMember) { - maybeAddImport(this, { - module: newModule, - member: newMember, - alias: aliasToUse, - onlyIfReferenced: false - }); } else { maybeAddImport(this, { module: newModule, member: newMember, + // A moved binding keeps the local name it had. Pinning it also tells + // `maybeAddImport` not to deconflict against the import being replaced, + // which is still present in the tree it reads. + alias: aliasToUse ?? newMember, onlyIfReferenced: false }); } diff --git a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts index 21b609c423a..7278819735c 100644 --- a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts @@ -2625,4 +2625,177 @@ describe('AddImport visitor', () => { }); }); + + describe('bound name', () => { + /** + * Calls from `visitJsCompilationUnit` rather than the constructor, so that the cursor + * reaches the compilation unit and the name can be resolved against it. + */ + function captureBoundName(options: AddImportOptions, bound: { name?: string }): JavaScriptVisitor { + return new class extends JavaScriptVisitor { + override async visitJsCompilationUnit(cu: JS.CompilationUnit, p: any): Promise { + bound.name = maybeAddImport(this, options); + return super.visitJsCompilationUnit(cu, p); + } + }; + } + + test('an import that already binds the member is the answer, not a conflict', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName( + {module: 'fs', member: 'readFile', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + import {readFile} from 'fs'; + + readFile('x'); + ` + ) + ); + + expect(bound.name).toBe('readFile'); + }); + + test('a module-scope declaration of the same name pushes the binding aside', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName( + {module: 'fs', member: 'readFile', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + const readFile = 1; + `, + ` + import {readFile as readFile_1} from 'fs'; + + const readFile = 1; + ` + ) + ); + + expect(bound.name).toBe('readFile_1'); + }); + + test('an import binding of the same name from another module pushes the binding aside', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName( + {module: 'fs', member: 'readFile', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + import {readFile} from 'node:fs'; + + readFile('x'); + `, + ` + import {readFile} from 'node:fs'; + import {readFile as readFile_1} from 'fs'; + + readFile('x'); + ` + ) + ); + + expect(bound.name).toBe('readFile_1'); + }); + + test('an explicitly pinned alias is honoured even into a collision', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName( + {module: 'fs', member: 'readFile', alias: 'readFile', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + const readFile = 1; + `, + ` + import {readFile} from 'fs'; + + const readFile = 1; + ` + ) + ); + + expect(bound.name).toBe('readFile'); + }); + + test('two calls in one visit resolve against each other', async () => { + const spec = new RecipeSpec(); + const first: { name?: string } = {}; + const second: { name?: string } = {}; + spec.recipe = fromVisitor(new class extends JavaScriptVisitor { + override async visitJsCompilationUnit(cu: JS.CompilationUnit, p: any): Promise { + first.name = maybeAddImport(this, {module: 'fs', member: 'readFile', onlyIfReferenced: false}); + second.name = maybeAddImport(this, {module: 'fs/promises', member: 'readFile', onlyIfReferenced: false}); + return super.visitJsCompilationUnit(cu, p); + } + }); + + await spec.rewriteRun( + typescript( + ` + const x = 1; + `, + ` + import {readFile} from 'fs'; + import {readFile as readFile_1} from 'fs/promises'; + + const x = 1; + ` + ) + ); + + expect(first.name).toBe('readFile'); + expect(second.name).toBe('readFile_1'); + }); + + test('onlyIfReferenced looks for the deconflicted name, so a shadowed member is not imported', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName({module: 'fs', member: 'readFile'}, bound)); + + await spec.rewriteRun( + typescript( + ` + const readFile = 1; + + readFile; + ` + ) + ); + + expect(bound.name).toBe('readFile_1'); + }); + + test('a side-effect import binds no name', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {name: 'unset'}; + spec.recipe = fromVisitor(captureBoundName({module: 'core-js/stable', sideEffectOnly: true}, bound)); + + await spec.rewriteRun( + typescript( + ` + const x = 1; + `, + ` + import 'core-js/stable'; + + const x = 1; + ` + ) + ); + + expect(bound.name).toBeUndefined(); + }); + }); + }); From 2b85fbfeb04ca98d84dc01a1fb31ca29a64059ed Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 14:28:03 +0200 Subject: [PATCH 02/11] Templates resolve the modules their code names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A returned binding name only helps if the caller respects it, and a caller generating code from a template has to thread it in by hand. A template now declares the modules its own source names: template`Theming.setTheme(${theme})`.configure({ bindings: {Theming: {module: 'sap/ui/core/Theming', member: 'default'}} }) `Template.resolveBindings(visitor)` binds each one in the file being visited and returns the local names, which the caller hands back through `ApplyOptions.bindings` or the new `TryOnOptions`. Applying the template rewrites its references to whatever the file settled on, so a file already importing the module under another name gets that name, and one that shadows it gets a suffixed one. The caller resolves and the engine substitutes, rather than `apply` taking a visitor: a Template's parsed AST is shared across call sites through `globalAstCache`, and a cached object that mutates visitor state as a side effect of being applied is the wrong lifetime. A declared key with no entry in the map throws, so forgetting is loud rather than a name nothing bound. The declaration also generates the type-attribution context imports, which `getTemplateTree` leaves out of the output by taking only the last statement. The rename runs before parameter substitution, so only the template's own code is in scope and a caller's captured code is untouched by it. Attribution decides where the context import resolved; where it did not, position does, and an identifier in its parent's `name` slot is being named rather than referencing the binding. `resolveBindings` is safe to call for a node the template turns out not to apply to, because the import lands in `afterVisit`, by which point the file references the name only where the template did. `AddImportOptions` gains `preferredName`: a name that loses to an existing import and deconflicts against a shadow, where `alias` does neither. That made `member: 'default'` reachable without an alias, which exposed the reuse lookup comparing `member` literally — `'default'` and an absent member both name a default import, and `memberName` normalizes them. Four comparisons treated a field describing how an import prints, or what name would be nice, as part of which binding was requested. Each was harmless while the function returned void and each returned a name no import bound once it did not: `quoteStyle` and `preferredName` split one request into two that both survive the queue dedup, while `isMatchingImport` and `isMatchingRequire` stayed on `alias` after emission moved to `bindingName`, merging a second `readFile` specifier into an import that already had one. `TemplateOptions.bindings` names modules and members only, and resolution is a separate call, so nothing in the rename pass is ESM-specific. --- .../rewrite/src/javascript/add-import.ts | 61 +++++---- .../src/javascript/templating/bindings.ts | 108 +++++++++++++++ .../src/javascript/templating/engine.ts | 13 +- .../src/javascript/templating/index.ts | 2 + .../src/javascript/templating/rewrite.ts | 22 +-- .../src/javascript/templating/template.ts | 39 +++++- .../src/javascript/templating/types.ts | 35 ++++- .../test/javascript/add-import.test.ts | 119 ++++++++++++++++ .../templating/template-bindings.test.ts | 128 ++++++++++++++++++ 9 files changed, 487 insertions(+), 40 deletions(-) create mode 100644 rewrite-javascript/rewrite/src/javascript/templating/bindings.ts create mode 100644 rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts diff --git a/rewrite-javascript/rewrite/src/javascript/add-import.ts b/rewrite-javascript/rewrite/src/javascript/add-import.ts index ce291d59f1f..d65177335e5 100644 --- a/rewrite-javascript/rewrite/src/javascript/add-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/add-import.ts @@ -5,7 +5,6 @@ import {randomId} from "../uuid"; import {emptyMarkers, markers} from "../markers"; import {getStyle, PrettierStyle, SpacesStyle, StyleKind} from "./style"; import {Cursor} from "../tree"; -import {RemoveImport} from "./remove-import"; export type QuoteChar = "'" | '"'; @@ -39,6 +38,11 @@ export interface AddImportOptions { * Cannot be combined with `sideEffectOnly`. */ alias?: string; + /** Preferred local name, deconflicted if the file already binds it. Stands in for `alias` where + * `member` is 'default' or '*' and the caller wants a name it does not insist on. + * Cannot be combined with `sideEffectOnly`. */ + preferredName?: string; + /** If true, only add the import if the member is actually used in the file. Default: true * Cannot be combined with `sideEffectOnly`. */ onlyIfReferenced?: boolean; @@ -106,10 +110,12 @@ export function maybeAddImport( options: AddImportOptions ): string | undefined { const module = moduleNameOf(options.module); + // Neither the quote nor a preferred name says which binding is wanted, so a request differing + // only in those is the same request, and answering with the name already settled on keeps a + // caller from emitting a reference to an import that never appears. for (const v of visitor.afterVisit || []) { if (v instanceof AddImport && v.module === module && - v.quoteStyle === options.quoteStyle && v.member === options.member && v.alias === options.alias && v.sideEffectOnly === (options.sideEffectOnly ?? false) && @@ -123,7 +129,7 @@ export function maybeAddImport( return undefined; } - const derived = options.alias ?? options.member ?? module; + const derived = options.alias ?? options.preferredName ?? memberName(options.member) ?? module; const cu = compilationUnitOf(visitor); if (!cu) { visitor.afterVisit.push(new AddImport(options, derived)); @@ -137,7 +143,7 @@ export function maybeAddImport( // derive a suffixed name from the binding this call just added. if (!options.alias) { for (const binding of bindings) { - if (binding.module === module && binding.member === options.member && binding.typeOnly === typeOnly) { + if (binding.module === module && binding.member === memberName(options.member) && binding.typeOnly === typeOnly) { return binding.name; } } @@ -148,6 +154,11 @@ export function maybeAddImport( return name; } +/** `'default'` and an absent member both name a default import, which binds no member name of its own. */ +function memberName(member: string | undefined): string | undefined { + return member === 'default' ? undefined : member; +} + /** A name introduced into the file's module scope, and where it came from when that is an import. */ interface ModuleScopeBinding { name: string; @@ -265,27 +276,22 @@ function importBindings(jsImport: JS.Import): ModuleScopeBinding[] { return bindings; } -/** On the `afterVisit` queue, a pending `AddImport` has claimed a name and a pending `RemoveImport` frees one. */ +/** + * 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. + */ function takenNames(bindings: ModuleScopeBinding[], visitor: JavaScriptVisitor): Set { const taken = new Set(); - const removals: RemoveImport[] = []; for (const v of visitor.afterVisit || []) { - if (v instanceof AddImport) { - if (v.bindingName) { - taken.add(v.bindingName); - } - } else if (v instanceof RemoveImport) { - removals.push(v); + if (v instanceof AddImport && v.bindingName) { + taken.add(v.bindingName); } } for (const binding of bindings) { - const removed = binding.module !== undefined && removals.some(r => - r.module === binding.module && (r.member === undefined || r.member === binding.member)); - if (!removed) { - taken.add(binding.name); - } + taken.add(binding.name); } return taken; @@ -425,6 +431,7 @@ export class AddImport

extends JavaScriptVisitor

{ readonly moduleUnicodeEscapes?: J.LiteralUnicodeEscape[]; readonly member?: string; readonly alias?: string; + readonly preferredName?: string; readonly onlyIfReferenced: boolean; readonly sideEffectOnly: boolean; readonly typeOnly: boolean; @@ -436,13 +443,13 @@ export class AddImport

extends JavaScriptVisitor

{ constructor(options: AddImportOptions, bindingName?: string) { super(); - // Validate that alias is provided when member is 'default' - if (options.member === 'default' && !options.alias) { + // Validate that a name is provided when member is 'default' + if (options.member === 'default' && !options.alias && !options.preferredName) { throw new Error("When member is 'default', the alias parameter is required"); } - // Validate that alias is provided when member is '*' (namespace import) - if (options.member === '*' && !options.alias) { + // Validate that a name is provided when member is '*' (namespace import) + if (options.member === '*' && !options.alias && !options.preferredName) { throw new Error("When member is '*', the alias parameter is required"); } @@ -454,6 +461,9 @@ export class AddImport

extends JavaScriptVisitor

{ if (options.alias !== undefined) { throw new Error("Cannot combine sideEffectOnly with alias"); } + if (options.preferredName !== undefined) { + throw new Error("Cannot combine sideEffectOnly with preferredName"); + } if (options.onlyIfReferenced !== undefined) { throw new Error("Cannot combine sideEffectOnly with onlyIfReferenced"); } @@ -467,12 +477,13 @@ export class AddImport

extends JavaScriptVisitor

{ this.moduleUnicodeEscapes = typeof options.module === 'string' ? undefined : options.module.unicodeEscapes; this.member = options.member; this.alias = options.alias; + this.preferredName = options.preferredName; this.onlyIfReferenced = options.onlyIfReferenced ?? true; this.sideEffectOnly = options.sideEffectOnly ?? false; this.typeOnly = options.typeOnly ?? false; this.style = options.style; this.quoteStyle = options.quoteStyle; - this.bindingName = this.sideEffectOnly ? undefined : bindingName ?? this.alias ?? this.member ?? this.module; + this.bindingName = this.sideEffectOnly ? undefined : bindingName ?? this.alias ?? this.preferredName ?? memberName(this.member) ?? this.module; } /** @@ -1048,7 +1059,9 @@ export class AddImport

extends JavaScriptVisitor

{ const importName = this.getImportName(specifier); const aliasName = this.getImportAlias(specifier); - if (importName === this.member && aliasName === this.alias) { + // A specifier binds its alias, or its own name where it has none; that is + // what this request has to match, however it came by the name it binds. + if (importName === this.member && (aliasName ?? importName) === this.bindingName) { return true; } } @@ -1104,7 +1117,7 @@ export class AddImport

extends JavaScriptVisitor

{ if (elem.element?.kind === JS.Kind.BindingElement) { const bindingElem = elem.element as JS.BindingElement; const name = (bindingElem.name as J.Identifier)?.simpleName; - if (name === (this.alias || this.member)) { + if (name === this.bindingName) { return true; } } diff --git a/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts b/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts new file mode 100644 index 00000000000..1593b94c1ae --- /dev/null +++ b/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts @@ -0,0 +1,108 @@ +/* + * 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 {J, Type} from '../../java'; +import {JavaScriptVisitor} from '../visitor'; +import {Cursor} from '../../tree'; +import {ModuleBinding} from './types'; + +/** + * The import that gives a declared binding's name its type attribution. Parsed ahead of the + * template as context, so it does not reach the output. + */ +export function bindingContextStatement(name: string, binding: ModuleBinding): string { + const type = binding.typeOnly ? 'type ' : ''; + if (binding.member === '*') { + return `import ${type}* as ${name} from '${binding.module}';`; + } + if (binding.member === undefined || binding.member === 'default') { + return `import ${type}${name} from '${binding.module}';`; + } + const specifier = binding.member === name ? name : `${binding.member} as ${name}`; + return `import ${type}{${specifier}} from '${binding.module}';`; +} + +/** + * Renames the identifiers a template uses for its declared bindings to the names the file + * actually binds. Runs before parameter substitution, so only the template's own code is in + * scope and a caller's captured code is never rewritten. + */ +export async function renameBindings(tree: T, renames: Record, modules: Record): Promise { + return new RenameBindingsVisitor(renames, modules).visit(tree, undefined) as Promise; +} + +class RenameBindingsVisitor extends JavaScriptVisitor { + constructor(private readonly renames: Record, + private readonly modules: Record) { + super(); + } + + override async visitIdentifier(identifier: J.Identifier, p: undefined): Promise { + const renamed = this.renames[identifier.simpleName]; + if (renamed === undefined || renamed === identifier.simpleName) { + return identifier; + } + + // Attribution settles it where the context import resolved. It is absent for a module the + // parse could not reach, and the identifier's position decides instead. + const resolved = resolvedModule(identifier); + const refersToBinding = resolved !== undefined + ? resolved === this.modules[identifier.simpleName] + : !namesItsParent(this.cursor, identifier); + + return refersToBinding ? {...identifier, simpleName: renamed} as J.Identifier : identifier; + } +} + +/** An identifier in its parent's `name` slot is being named — a property, a method, a declaration. */ +function namesItsParent(cursor: Cursor, identifier: J.Identifier): boolean { + let c: Cursor | undefined = cursor.parent; + while (c && isPadding(c.value)) { + c = c.parent; + } + const name = (c?.value as { name?: unknown } | undefined)?.name; + return name === identifier || (name as { element?: unknown } | undefined)?.element === identifier; +} + +function isPadding(value: unknown): boolean { + const kind = (value as { kind?: string } | undefined)?.kind; + return kind === J.Kind.RightPadded || kind === J.Kind.LeftPadded || kind === J.Kind.Container; +} + +/** The module an identifier's attribution traces back to, following the owning-class chain to its root. */ +function resolvedModule(identifier: J.Identifier): string | undefined { + const fieldType = identifier.fieldType; + if (fieldType?.kind === Type.Kind.Variable) { + const owner = (fieldType as Type.Variable).owner; + return owner && Type.isClass(owner) ? rootName(owner as Type.Class) : undefined; + } + const type = identifier.type; + if (type && Type.isMethod(type)) { + const declaring = (type as Type.Method).declaringType; + return declaring ? rootName(declaring as Type.Class) : undefined; + } + if (type && Type.isClass(type)) { + return rootName(type as Type.Class); + } + return undefined; +} + +function rootName(classType: Type.Class): string { + let current: Type.Class = classType; + while (current.owningClass && Type.isClass(current.owningClass)) { + current = current.owningClass as Type.Class; + } + return Type.FullyQualified.getFullyQualifiedName(current); +} diff --git a/rewrite-javascript/rewrite/src/javascript/templating/engine.ts b/rewrite-javascript/rewrite/src/javascript/templating/engine.ts index 12aedc3a94f..e5c32379cb8 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/engine.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/engine.ts @@ -23,6 +23,7 @@ import {PlaceholderReplacementVisitor} from './placeholder-replacement'; import {maybeParenthesize, parenthesize, requiredPrecedence, startsWithDeclarationToken} from './precedence'; import {JavaCoordinates} from './template'; import {maybeAutoFormat} from '../format'; +import {renameBindings} from './bindings'; import {isExpression, isStatement} from '../parser-utils'; import {randomId} from '../../uuid'; import ts from "typescript"; @@ -245,6 +246,8 @@ export class TemplateEngine { * @param values Map of capture names to values to replace the parameters with * @param wrappersMap Map of capture names to J.RightPadded wrappers (for preserving markers) * @param format Whether to fit the result to where it lands + * @param renames Local names for the template's declared bindings, keyed as declared + * @param modules The module each declared binding names, keyed as declared * @returns A Promise resolving to the generated AST node */ static async applyTemplateFromAst( @@ -254,7 +257,9 @@ export class TemplateEngine { coordinates: JavaCoordinates, values: Pick, 'get'> = new Map(), wrappersMap: Pick | J.RightPadded[]>, 'get'> = new Map(), - format: boolean = true + format: boolean = true, + renames: Record = {}, + modules: Record = {} ): Promise { // Create substitutions map for placeholders const substitutions = new Map(); @@ -266,9 +271,13 @@ export class TemplateEngine { // Before substitution, so that ids carried over from the source tree survive this pass const fresh = await randomizeIds(ast); + const bound = Object.keys(renames).length > 0 + ? await renameBindings(fresh.tree as J, renames, modules) + : fresh.tree; + // Unsubstitute placeholders with actual parameter values and match results const visitor = new PlaceholderReplacementVisitor(substitutions, values, wrappersMap); - const unsubstitutedAst = (await visitor.visit(fresh.tree, null))!; + const unsubstitutedAst = (await visitor.visit(bound, null))!; // An id may only be kept where the node answering to it is leaving the tree, which is the // subtree this application replaces. A parameter named twice, or spliced in from somewhere diff --git a/rewrite-javascript/rewrite/src/javascript/templating/index.ts b/rewrite-javascript/rewrite/src/javascript/templating/index.ts index e038fc2b5ad..376c751d2ab 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/index.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/index.ts @@ -25,7 +25,9 @@ export type { MatchOptions, TemplateParameter, TemplateOptions, + ModuleBinding, RewriteRule, + TryOnOptions, RewriteConfig, DebugOptions, DebugLogEntry, diff --git a/rewrite-javascript/rewrite/src/javascript/templating/rewrite.ts b/rewrite-javascript/rewrite/src/javascript/templating/rewrite.ts index 533cca9943c..0e97b49d343 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/rewrite.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/rewrite.ts @@ -15,7 +15,7 @@ */ import {Cursor, ExecutionContext, Recipe, TreeVisitor} from '../..'; import {J, Statement} from '../../java'; -import {PostMatchContext, PreMatchContext, RewriteConfig, RewriteRule} from './types'; +import {PostMatchContext, PreMatchContext, RewriteConfig, RewriteRule, TryOnOptions} from './types'; import {MatchResult, Pattern} from './pattern'; import {Template} from './template'; import {JavaScriptVisitor} from '../visitor'; @@ -33,7 +33,7 @@ class RewriteRuleImpl implements RewriteRule { ) { } - async tryOn(cursor: Cursor, node: J): Promise { + async tryOn(cursor: Cursor, node: J, options?: TryOnOptions): Promise { // Evaluate preMatch before attempting any pattern matching if (this.preMatch) { const preMatchResult = await this.preMatch(node, { cursor }); @@ -57,11 +57,11 @@ class RewriteRuleImpl implements RewriteRule { // Apply transformation let result: J | undefined; - const options = { values: match, format: this.format }; + const applyOptions = { values: match, format: this.format, bindings: options?.bindings }; if (typeof this.after === 'function') { - result = await this.after(match).apply(node, cursor, options); + result = await this.after(match).apply(node, cursor, applyOptions); } else { - result = await this.after.apply(node, cursor, options); + result = await this.after.apply(node, cursor, applyOptions); } if (result) { @@ -83,10 +83,10 @@ class RewriteRuleImpl implements RewriteRule { super([], () => undefined as unknown as Template); } - async tryOn(cursor: Cursor, node: J): Promise { - const firstResult = await first.tryOn(cursor, node); + async tryOn(cursor: Cursor, node: J, options?: TryOnOptions): Promise { + const firstResult = await first.tryOn(cursor, node, options); if (firstResult !== undefined) { - const secondResult = await next.tryOn(cursor, firstResult); + const secondResult = await next.tryOn(cursor, firstResult, options); return secondResult ?? firstResult; } return undefined; @@ -103,12 +103,12 @@ class RewriteRuleImpl implements RewriteRule { super([], () => undefined as unknown as Template); } - async tryOn(cursor: Cursor, node: J): Promise { - const firstResult = await first.tryOn(cursor, node); + async tryOn(cursor: Cursor, node: J, options?: TryOnOptions): Promise { + const firstResult = await first.tryOn(cursor, node, options); if (firstResult !== undefined) { return firstResult; } - return await alternative.tryOn(cursor, node); + return await alternative.tryOn(cursor, node, options); } })(); } diff --git a/rewrite-javascript/rewrite/src/javascript/templating/template.ts b/rewrite-javascript/rewrite/src/javascript/templating/template.ts index 9da068f0e73..7b1d1191aee 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/template.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/template.ts @@ -16,6 +16,9 @@ import {Cursor, Tree} from '../..'; import {J} from '../../java'; import {ApplyOptions, Parameter, TemplateOptions, TemplateParameter} from './types'; +import {bindingContextStatement} from './bindings'; +import {maybeAddImport} from '../add-import'; +import {JavaScriptVisitor} from '../visitor'; import {MatchResult} from './pattern'; import {generateCacheKey, globalAstCache, WRAPPERS_MAP_SYMBOL} from './utils'; import {CAPTURE_NAME_SYMBOL, RAW_CODE_SYMBOL} from './capture'; @@ -246,7 +249,10 @@ export class Template { // Generate cache key for global lookup // For raw() parameters, we need to include their code values in the key // since they're spliced at construction time, not application time - const contextStatements = this.options.context || this.options.imports || []; + const contextStatements = [ + ...(this.options.context || this.options.imports || []), + ...Object.entries(this.options.bindings ?? {}).map(([name, b]) => bindingContextStatement(name, b)) + ]; const parametersKey = this.parameters.map((p, i) => { const value = p.value; // Include raw code values in the cache key using the symbol @@ -307,6 +313,20 @@ export class Template { * }); * ``` */ + /** + * Binds every module this template declares in the file `visitor` is traversing, and returns + * the local names to hand back through {@link ApplyOptions.bindings}. Safe to call for a node + * the template turns out not to apply to: the import lands in `afterVisit`, by which point the + * file references the name only where the template did land. + */ + resolveBindings(visitor: JavaScriptVisitor): Record { + const resolved: Record = {}; + for (const [name, binding] of Object.entries(this.options.bindings ?? {})) { + resolved[name] = maybeAddImport(visitor, {...binding, preferredName: name}); + } + return resolved; + } + async apply(tree: J, cursor: Cursor, options?: ApplyOptions): Promise { // Extract values from options const values = options?.values; @@ -346,6 +366,19 @@ export class Template { } } + const declared = this.options.bindings ?? {}; + const renames: Record = {}; + const modules: Record = {}; + for (const [name, binding] of Object.entries(declared)) { + const bound = options?.bindings?.[name]; + if (bound === undefined) { + throw new Error(`Template declares a binding for '${name}' but was applied without a local name for it. ` + + `Pass bindings: template.resolveBindings(visitor) to apply().`); + } + renames[name] = bound; + modules[name] = binding.module; + } + // Use instance-level cache to get the template tree const ast = await this.getTemplateTree(); @@ -360,7 +393,9 @@ export class Template { }, normalizedValues, wrappersMap, - options?.format ?? true + options?.format ?? true, + renames, + modules ); } } diff --git a/rewrite-javascript/rewrite/src/javascript/templating/types.ts b/rewrite-javascript/rewrite/src/javascript/templating/types.ts index 04ccdc4a834..11a9b901aa9 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/types.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/types.ts @@ -18,6 +18,7 @@ import {J, Type} from '../../java'; import type {Pattern} from "./pattern"; import type {Template} from "./template"; import type {CaptureValue, RawCode} from "./capture"; +import type {AddImportOptions} from "../add-import"; /** * Options for variadic captures that match zero or more nodes in a sequence. @@ -481,8 +482,28 @@ export interface TemplateOptions { * The template engine will create a package.json with these dependencies. */ dependencies?: Record; + + /** + * Modules the template's code refers to, keyed by the identifier its source uses for each. + * The key is a preferred name: {@link Template.resolveBindings} deconflicts it against the + * file, and applying the template rewrites the template's references to whatever it settled on. + * + * @example + * ```typescript + * template`Theming.setTheme(${capture('theme')})` + * .configure({bindings: {Theming: {module: 'sap/ui/core/Theming', member: 'default'}}}) + * ``` + */ + bindings?: Record; } +/** + * A module a template's code depends on. The local name comes from the key it is declared under, + * and {@link Template.resolveBindings} settles the rest. + */ +export type ModuleBinding = Omit & { module: string }; + /** * Options for template application. */ @@ -516,6 +537,18 @@ export interface ApplyOptions { * The anchor supplies the result's prefix either way. */ format?: boolean; + + /** + * Local names for the bindings the template declares, keyed as they are declared — normally + * {@link Template.resolveBindings}. Every declared binding needs one. + */ + bindings?: Record; +} + +/** Options a caller supplies per node, as against the ones {@link RewriteConfig} fixes for the rule. */ +export interface TryOnOptions { + /** As {@link ApplyOptions.bindings}, for the template the rule applies. */ + bindings?: Record; } /** @@ -531,7 +564,7 @@ export interface RewriteRule { * When using in a visitor, always use the `|| node` pattern to return the original * node when there's no match: `return await rule.tryOn(this.cursor, node) || node;` */ - tryOn(cursor: Cursor, node: J): Promise; + tryOn(cursor: Cursor, node: J, options?: TryOnOptions): Promise; /** * Chains this rule with another rule, creating a composite rule that applies both transformations sequentially. diff --git a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts index 7278819735c..18bb883eed9 100644 --- a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts @@ -2776,6 +2776,125 @@ describe('AddImport visitor', () => { expect(bound.name).toBe('readFile_1'); }); + test('a preferred name is deconflicted, unlike a pinned alias', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName( + {module: 'sap/ui/core/Theming', member: 'default', preferredName: 'Theming', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + const Theming = 1; + `, + ` + import Theming_1 from 'sap/ui/core/Theming'; + + const Theming = 1; + ` + ) + ); + + expect(bound.name).toBe('Theming_1'); + }); + + test("a default import is reused whether the request spells the member 'default' or omits it", async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName( + {module: 'sap/ui/core/Theming', member: 'default', preferredName: 'Theming', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + import Th from 'sap/ui/core/Theming'; + + Th.setTheme('dark'); + ` + ) + ); + + expect(bound.name).toBe('Th'); + }); + + test('a pinned alias that names the member matches the plain specifier already there', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName( + {module: 'fs', member: 'readFile', alias: 'readFile', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + import {readFile} from 'fs'; + + readFile('x'); + ` + ) + ); + + expect(bound.name).toBe('readFile'); + }); + + test('a require binding the plain name does not answer a request that had to be renamed', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName( + {module: 'fs', member: 'readFile', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + const {readFile} = require('fs'); + + readFile('x'); + `, + ` + import {readFile as readFile_1} from 'fs'; + + const {readFile} = require('fs'); + + readFile('x'); + ` + ) + ); + + expect(bound.name).toBe('readFile_1'); + }); + + test('a second request for the same binding gets the name the first settled on', async () => { + const spec = new RecipeSpec(); + const quoted: string[] = []; + const preferred: string[] = []; + spec.recipe = fromVisitor(new class extends JavaScriptVisitor { + override async visitJsCompilationUnit(cu: JS.CompilationUnit, p: any): Promise { + quoted.push(maybeAddImport(this, {module: 'fs', member: 'readFile', onlyIfReferenced: false})); + quoted.push(maybeAddImport(this, {module: 'fs', member: 'readFile', quoteStyle: '"', onlyIfReferenced: false})); + + preferred.push(maybeAddImport(this, {module: 'theme', member: 'default', preferredName: 'Theming', onlyIfReferenced: false})); + preferred.push(maybeAddImport(this, {module: 'theme', member: 'default', preferredName: 'Th', onlyIfReferenced: false})); + return super.visitJsCompilationUnit(cu, p); + } + }); + + await spec.rewriteRun( + typescript( + ` + const x = 1; + `, + ` + import {readFile} from 'fs'; + import Theming from 'theme'; + + const x = 1; + ` + ) + ); + + expect(quoted).toEqual(['readFile', 'readFile']); + expect(preferred).toEqual(['Theming', 'Theming']); + }); + test('a side-effect import binds no name', async () => { const spec = new RecipeSpec(); const bound: { name?: string } = {name: 'unset'}; diff --git a/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts b/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts new file mode 100644 index 00000000000..28dcd9b2a4d --- /dev/null +++ b/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts @@ -0,0 +1,128 @@ +/* + * 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 {fromVisitor, RecipeSpec} from "../../../src/test"; +import {capture, JavaScriptVisitor, pattern, rewrite, template, typescript} from "../../../src/javascript"; +import {J} from "../../../src/java"; + +describe('templates that declare module bindings', () => { + const spec = new RecipeSpec(); + + /** Rewrites every `applyTheme(...)` through `tmpl`, resolving the template's bindings per file. */ + function recipeApplying(tmpl: ReturnType, arg: ReturnType) { + const rule = rewrite(() => ({before: pattern`applyTheme(${arg})`, after: tmpl})); + return fromVisitor(new class extends JavaScriptVisitor { + override async visitMethodInvocation(method: J.MethodInvocation, p: any): Promise { + method = await super.visitMethodInvocation(method, p) as J.MethodInvocation; + return await rule.tryOn(this.cursor, method, {bindings: tmpl.resolveBindings(this)}) || method; + } + }); + } + + test('the bound name is deconflicted and the template follows it', async () => { + const arg = capture('arg'); + spec.recipe = recipeApplying(template`Theming.setTheme(${arg})`.configure({ + bindings: {Theming: {module: 'sap/ui/core/Theming', member: 'default'}} + }), arg); + + await spec.rewriteRun( + //language=typescript + typescript( + `const Theming = 1;\napplyTheme('dark');`, + `import Theming_1 from 'sap/ui/core/Theming';\n\nconst Theming = 1;\nTheming_1.setTheme('dark');` + ) + ); + }); + + test('an import already binding the module is reused, under the name it already has', async () => { + const arg = capture('arg'); + spec.recipe = recipeApplying(template`Theming.setTheme(${arg})`.configure({ + bindings: {Theming: {module: 'sap/ui/core/Theming', member: 'default'}} + }), arg); + + await spec.rewriteRun( + //language=typescript + typescript( + `import Th from 'sap/ui/core/Theming';\n\napplyTheme('dark');`, + `import Th from 'sap/ui/core/Theming';\n\nTh.setTheme('dark');` + ) + ); + }); + + test('a template resolves every binding it declares', async () => { + const arg = capture('arg'); + spec.recipe = recipeApplying(template`new Locale(Localization.getLanguageTag(${arg}))`.configure({ + bindings: { + Locale: {module: 'sap/ui/core/Locale', member: 'default'}, + Localization: {module: 'sap/base/i18n/Localization', member: 'default'} + } + }), arg); + + await spec.rewriteRun( + //language=typescript + typescript( + `applyTheme('dark');`, + `import Locale from 'sap/ui/core/Locale';\nimport Localization from 'sap/base/i18n/Localization';\n\nnew Locale(Localization.getLanguageTag('dark'));` + ) + ); + }); + + test('a name in a naming position is not a reference to the binding', async () => { + const arg = capture('arg'); + spec.recipe = recipeApplying(template`Theming.setTheme(${arg}.Theming)`.configure({ + bindings: {Theming: {module: 'sap/ui/core/Theming', member: 'default'}} + }), arg); + + await spec.rewriteRun( + //language=typescript + typescript( + `const Theming = 1;\napplyTheme(props);`, + `import Theming_1 from 'sap/ui/core/Theming';\n\nconst Theming = 1;\nTheming_1.setTheme(props.Theming);` + ) + ); + }); + + test('a file the rule never rewrites gets no import', async () => { + const arg = capture('arg'); + spec.recipe = recipeApplying(template`Theming.setTheme(${arg})`.configure({ + bindings: {Theming: {module: 'sap/ui/core/Theming', member: 'default'}} + }), arg); + + await spec.rewriteRun( + //language=typescript + typescript(`somethingElse('dark');`) + ); + }); + + test('a declared binding left unresolved at apply is an error, not a silent wrong name', async () => { + const arg = capture('arg'); + const tmpl = template`Theming.setTheme(${arg})`.configure({ + bindings: {Theming: {module: 'sap/ui/core/Theming', member: 'default'}} + }); + const rule = rewrite(() => ({before: pattern`applyTheme(${arg})`, after: tmpl})); + + spec.recipe = fromVisitor(new class extends JavaScriptVisitor { + override async visitMethodInvocation(method: J.MethodInvocation, p: any): Promise { + method = await super.visitMethodInvocation(method, p) as J.MethodInvocation; + return await rule.tryOn(this.cursor, method) || method; + } + }); + + await expect(spec.rewriteRun( + //language=typescript + typescript(`applyTheme('dark');`) + )).rejects.toThrow(/Theming/); + }); +}); From f5e4b935a2306e729546ed00b166b4bd3b03ec95 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 14:49:03 +0200 Subject: [PATCH 03/11] Bind the modules a require names, and resolve a rule's bindings after it matches `moduleScopeBindings` recorded a `require()` binding without the module it comes from, so the reuse lookup could not match one. A file holding `const {join} = require('path')` therefore had `join` counted as an occupied name rather than as the very binding being asked for: maybeAddImport(v, {module: 'path', member: 'join'}) // import {join as join_1} from 'path'; alongside the require Recording the module and member for both `const F = require('m')` and `const {k: kk} = require('n')` lets a require answer a request the way an import does, under whatever name it destructured. The same gap made a default binding with a `preferredName` return a name that `isMatchingRequire` then declined to emit. Namespace and type declarations join the pool. They shadow an import exactly as a `const` does, and an import that collides with one is TS2440. The `afterVisit` scan now mirrors the file-side rule: a queued import binds the module as much as one already present, so it answers a later request for the same module and member. Comparing `member` literally missed that `'default'` and an absent member name the same import, which left a second request holding a name the merged one never emits. `TryOnOptions.visitor` lets a rule resolve its own template's declared modules, since it holds that template. It resolves once a pattern has matched, so a rule that does not fire leaves the file's imports alone rather than reserving a name that pushes an unrelated binding aside. A caller that resolves some other way still passes `bindings`. `Template.apply` keeps its own documentation, and `resolveBindings` sits above it with its own. --- .../rewrite/src/javascript/add-import.ts | 80 ++++++++++++++++--- .../src/javascript/templating/rewrite.ts | 10 +-- .../src/javascript/templating/template.ts | 28 +++---- .../src/javascript/templating/types.ts | 10 ++- .../test/javascript/add-import.test.ts | 48 ++++++++--- .../templating/template-bindings.test.ts | 6 +- 6 files changed, 137 insertions(+), 45 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/add-import.ts b/rewrite-javascript/rewrite/src/javascript/add-import.ts index d65177335e5..21f8a134508 100644 --- a/rewrite-javascript/rewrite/src/javascript/add-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/add-import.ts @@ -110,21 +110,27 @@ export function maybeAddImport( options: AddImportOptions ): string | undefined { const module = moduleNameOf(options.module); - // Neither the quote nor a preferred name says which binding is wanted, so a request differing - // only in those is the same request, and answering with the name already settled on keeps a - // caller from emitting a reference to an import that never appears. + const sideEffectOnly = options.sideEffectOnly ?? false; + const typeOnly = options.typeOnly ?? false; + + // A queued import binds the module as much as one already in the file, so it answers a later + // request the same way; a name a merged request never emits would be referenced but not bound. for (const v of visitor.afterVisit || []) { - if (v instanceof AddImport && - v.module === module && - v.member === options.member && - v.alias === options.alias && - v.sideEffectOnly === (options.sideEffectOnly ?? false) && - v.typeOnly === (options.typeOnly ?? false)) { + if (!(v instanceof AddImport) || v.module !== module || + v.sideEffectOnly !== sideEffectOnly || v.typeOnly !== typeOnly) { + continue; + } + // How a specifier prints, or what name would be nice, does not say which binding is wanted. + // A pinned alias does: it asks for a binding of its own, which only the same request answers. + const answers = options.alias + ? v.alias === options.alias && v.member === options.member + : memberName(v.member) === memberName(options.member); + if (answers) { return v.bindingName; } } - if (options.sideEffectOnly) { + if (sideEffectOnly) { visitor.afterVisit.push(new AddImport(options)); return undefined; } @@ -137,7 +143,6 @@ export function maybeAddImport( } const bindings = moduleScopeBindings(cu); - const typeOnly = options.typeOnly ?? false; // 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. @@ -193,7 +198,12 @@ function moduleScopeBindings(cu: JS.CompilationUnit): ModuleScopeBinding[] { const declaredByVariables = (varDecl: J.VariableDeclarations): void => { for (const variable of varDecl.variables) { - declaredBy(variable.element?.name); + const required = requiredModule(variable.element?.initializer?.element); + if (required !== undefined) { + bindings.push(...requireBindings(variable.element?.name, required)); + } else { + declaredBy(variable.element?.name); + } } }; @@ -219,12 +229,58 @@ function moduleScopeBindings(cu: JS.CompilationUnit): ModuleScopeBinding[] { 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; } } return bindings; } +/** The module a `require('...')` initializer names, `undefined` for an initializer that is anything else. */ +function requiredModule(initializer: J | undefined): string | undefined { + if (initializer?.kind !== J.Kind.MethodInvocation) { + return undefined; + } + const methodInv = initializer as J.MethodInvocation; + if (methodInv.select || methodInv.name?.kind !== J.Kind.Identifier || + (methodInv.name as J.Identifier).simpleName !== 'require') { + return undefined; + } + const argument = methodInv.arguments?.elements[0]?.element; + return argument?.kind === J.Kind.Literal ? moduleNameOf(argument as J.Literal) : undefined; +} + +/** 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[] { + if (pattern?.kind === J.Kind.Identifier) { + return [{name: (pattern as J.Identifier).simpleName, module, member: undefined, typeOnly: false}]; + } + if (pattern?.kind !== JS.Kind.ObjectBindingPattern) { + return []; + } + const bindings: ModuleScopeBinding[] = []; + 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; + const member = propertyName?.kind === J.Kind.Identifier + ? (propertyName as J.Identifier).simpleName + : name; + bindings.push({name, module, member, typeOnly: false}); + } + } + return bindings; +} + function importBindings(jsImport: JS.Import): ModuleScopeBinding[] { const moduleSpecifier = jsImport.moduleSpecifier?.element; const module = moduleSpecifier?.kind === J.Kind.Literal diff --git a/rewrite-javascript/rewrite/src/javascript/templating/rewrite.ts b/rewrite-javascript/rewrite/src/javascript/templating/rewrite.ts index 0e97b49d343..56ed9f6fe8f 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/rewrite.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/rewrite.ts @@ -57,12 +57,10 @@ class RewriteRuleImpl implements RewriteRule { // Apply transformation let result: J | undefined; - const applyOptions = { values: match, format: this.format, bindings: options?.bindings }; - if (typeof this.after === 'function') { - result = await this.after(match).apply(node, cursor, applyOptions); - } else { - result = await this.after.apply(node, cursor, applyOptions); - } + const template = typeof this.after === 'function' ? this.after(match) : this.after; + const bindings = options?.bindings ?? (options?.visitor && template.resolveBindings(options.visitor)); + result = await template.apply(node, cursor, + { values: match, format: this.format, bindings: bindings || undefined }); if (result) { return result; diff --git a/rewrite-javascript/rewrite/src/javascript/templating/template.ts b/rewrite-javascript/rewrite/src/javascript/templating/template.ts index a59e0485041..5e322872d33 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/template.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/template.ts @@ -291,6 +291,20 @@ export class Template { return result; } + /** + * Binds every module this template declares in the file `visitor` is traversing, and returns + * the local names to hand back through {@link ApplyOptions.bindings}. Safe to call for a node + * the template turns out not to apply to: the import lands in `afterVisit`, by which point the + * file references the name only where the template did land. + */ + resolveBindings(visitor: JavaScriptVisitor): Record { + const resolved: Record = {}; + for (const [name, binding] of Object.entries(this.options.bindings ?? {})) { + resolved[name] = maybeAddImport(visitor, {...binding, preferredName: name}); + } + return resolved; + } + /** * Applies this template and returns the resulting tree. * @@ -314,20 +328,6 @@ export class Template { * }); * ``` */ - /** - * Binds every module this template declares in the file `visitor` is traversing, and returns - * the local names to hand back through {@link ApplyOptions.bindings}. Safe to call for a node - * the template turns out not to apply to: the import lands in `afterVisit`, by which point the - * file references the name only where the template did land. - */ - resolveBindings(visitor: JavaScriptVisitor): Record { - const resolved: Record = {}; - for (const [name, binding] of Object.entries(this.options.bindings ?? {})) { - resolved[name] = maybeAddImport(visitor, {...binding, preferredName: name}); - } - return resolved; - } - async apply(tree: J, cursor: Cursor, options?: ApplyOptions): Promise { // Extract values from options const values = options?.values; diff --git a/rewrite-javascript/rewrite/src/javascript/templating/types.ts b/rewrite-javascript/rewrite/src/javascript/templating/types.ts index 11a9b901aa9..6b6f85d3bde 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/types.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/types.ts @@ -19,6 +19,7 @@ import type {Pattern} from "./pattern"; import type {Template} from "./template"; import type {CaptureValue, RawCode} from "./capture"; import type {AddImportOptions} from "../add-import"; +import type {JavaScriptVisitor} from "../visitor"; /** * Options for variadic captures that match zero or more nodes in a sequence. @@ -547,7 +548,14 @@ export interface ApplyOptions { /** Options a caller supplies per node, as against the ones {@link RewriteConfig} fixes for the rule. */ export interface TryOnOptions { - /** As {@link ApplyOptions.bindings}, for the template the rule applies. */ + /** + * The visitor to bind the applied template's declared modules in. The rule holds that template, + * so it resolves them itself, and only once a pattern has matched — a rule that does not fire + * leaves the file's imports alone. {@link TryOnOptions.bindings} overrides this. + */ + visitor?: JavaScriptVisitor; + + /** As {@link ApplyOptions.bindings}, for a caller that resolves the modules some other way. */ bindings?: Record; } diff --git a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts index 18bb883eed9..d300c1e0e7f 100644 --- a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts @@ -2836,30 +2836,60 @@ describe('AddImport visitor', () => { expect(bound.name).toBe('readFile'); }); - test('a require binding the plain name does not answer a request that had to be renamed', async () => { + test('a require of the same member answers the request, under the name it destructured', async () => { const spec = new RecipeSpec(); - const bound: { name?: string } = {}; - spec.recipe = fromVisitor(captureBoundName( - {module: 'fs', member: 'readFile', onlyIfReferenced: false}, bound)); + const bound: string[] = []; + spec.recipe = fromVisitor(new class extends JavaScriptVisitor { + override async visitJsCompilationUnit(cu: JS.CompilationUnit, p: any): Promise { + bound.push(maybeAddImport(this, {module: 'fs', member: 'readFile', onlyIfReferenced: false})); + bound.push(maybeAddImport(this, {module: 'fs/promises', member: 'readFile', onlyIfReferenced: false})); + return super.visitJsCompilationUnit(cu, p); + } + }); await spec.rewriteRun( typescript( ` const {readFile} = require('fs'); + const {readFile: rf} = require('fs/promises'); readFile('x'); - `, + rf('y'); ` - import {readFile as readFile_1} from 'fs'; + ) + ); - const {readFile} = require('fs'); + expect(bound).toEqual(['readFile', 'rf']); + }); - readFile('x'); + test('a namespace or type declaration shadows an import just as a value declaration does', async () => { + const spec = new RecipeSpec(); + const bound: string[] = []; + spec.recipe = fromVisitor(new class extends JavaScriptVisitor { + override async visitJsCompilationUnit(cu: JS.CompilationUnit, p: any): Promise { + bound.push(maybeAddImport(this, {module: 'm', member: 'join', onlyIfReferenced: false})); + bound.push(maybeAddImport(this, {module: 'n', member: 'shape', onlyIfReferenced: false})); + return super.visitJsCompilationUnit(cu, p); + } + }); + + await spec.rewriteRun( + typescript( + ` + namespace join { export const a = 1; } + type shape = string; + `, + ` + import {join as join_1} from 'm'; + import {shape as shape_1} from 'n'; + + namespace join { export const a = 1; } + type shape = string; ` ) ); - expect(bound.name).toBe('readFile_1'); + expect(bound).toEqual(['join_1', 'shape_1']); }); test('a second request for the same binding gets the name the first settled on', async () => { diff --git a/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts b/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts index 28dcd9b2a4d..aafc865cf5d 100644 --- a/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts @@ -26,7 +26,7 @@ describe('templates that declare module bindings', () => { return fromVisitor(new class extends JavaScriptVisitor { override async visitMethodInvocation(method: J.MethodInvocation, p: any): Promise { method = await super.visitMethodInvocation(method, p) as J.MethodInvocation; - return await rule.tryOn(this.cursor, method, {bindings: tmpl.resolveBindings(this)}) || method; + return await rule.tryOn(this.cursor, method, {visitor: this}) || method; } }); } @@ -94,7 +94,7 @@ describe('templates that declare module bindings', () => { ); }); - test('a file the rule never rewrites gets no import', async () => { + test('a rule that does not fire leaves the file\'s imports alone', async () => { const arg = capture('arg'); spec.recipe = recipeApplying(template`Theming.setTheme(${arg})`.configure({ bindings: {Theming: {module: 'sap/ui/core/Theming', member: 'default'}} @@ -102,7 +102,7 @@ describe('templates that declare module bindings', () => { await spec.rewriteRun( //language=typescript - typescript(`somethingElse('dark');`) + typescript(`const x = Theming;\nsomethingElse('dark');`) ); }); From 3fa0a9133de141fc78b2680adc7f61872602595f Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 15:13:20 +0200 Subject: [PATCH 04/11] Reuse a differently named binding only where the caller asked for a name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reuse answered a request from any binding of the same module and member, whatever local name it carried. A caller that named no preference is assuming the name it derived, so handing back another one left the references it emits unbound: maybeAddImport(this, {module: 'react', member: 'useState', onlyIfReferenced: false}); // on: import {useState as useS} from 'react'; // returned 'useS' and added nothing, so a recipe emitting useState(...) bound nothing `rewrite-react`, `rewrite-nodejs`, `javascript-recipe-starter` and `recipes-testing-frameworks` all call `maybeAddImport` this way and all ignore the return value, so the emitted import is the whole contract for them. A `preferredName` or an `alias` is the caller saying it will take whatever name comes back. Without one, only a binding under the very name the request derived answers it, and anything else falls through to the ordinary path — which adds the member as a second specifier under the name the caller expects. Reuse across a different name still holds where it was designed to: `bindModule` and `Template.resolveBindings` both name a preference. --- .../rewrite/src/javascript/add-import.ts | 12 ++++-- .../test/javascript/add-import.test.ts | 43 +++++++++++++++---- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/add-import.ts b/rewrite-javascript/rewrite/src/javascript/add-import.ts index 21f8a134508..acaa91b7ad5 100644 --- a/rewrite-javascript/rewrite/src/javascript/add-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/add-import.ts @@ -68,8 +68,8 @@ export interface AddImportOptions { * Register an AddImport visitor to add an import statement to a JavaScript/TypeScript file * @param visitor The visitor to add the import addition to * @param options Configuration options for the import to add - * @returns The local name the module is bound to: an existing import's, or the one the new import - * will use, suffixed if the file already binds that name. `onlyIfReferenced` defaults to true, so + * @returns The local name the module is bound to: an existing binding's where one answers this + * request, otherwise the name the new import will use, suffixed if the file already binds it. `onlyIfReferenced` defaults to true, so * the import may never appear, and the name is then what it would have gone by. A side-effect * import binds no name and returns `undefined`. * @@ -145,10 +145,14 @@ export function maybeAddImport( const bindings = moduleScopeBindings(cu); // 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. + // 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. if (!options.alias) { for (const binding of bindings) { - if (binding.module === module && binding.member === memberName(options.member) && binding.typeOnly === typeOnly) { + if (binding.module === module && binding.member === memberName(options.member) && + binding.typeOnly === typeOnly && + (options.preferredName !== undefined || binding.name === derived)) { return binding.name; } } diff --git a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts index d300c1e0e7f..4ae488ae31f 100644 --- a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts @@ -2836,13 +2836,32 @@ describe('AddImport visitor', () => { expect(bound.name).toBe('readFile'); }); - test('a require of the same member answers the request, under the name it destructured', async () => { + test('a require of the same member answers the request', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName( + {module: 'fs', member: 'readFile', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + const {readFile} = require('fs'); + + readFile('x'); + ` + ) + ); + + expect(bound.name).toBe('readFile'); + }); + + test('a binding renamed where it is declared does not answer a request that named no preference', async () => { const spec = new RecipeSpec(); const bound: string[] = []; spec.recipe = fromVisitor(new class extends JavaScriptVisitor { override async visitJsCompilationUnit(cu: JS.CompilationUnit, p: any): Promise { - bound.push(maybeAddImport(this, {module: 'fs', member: 'readFile', onlyIfReferenced: false})); - bound.push(maybeAddImport(this, {module: 'fs/promises', member: 'readFile', onlyIfReferenced: false})); + bound.push(maybeAddImport(this, {module: 'react', member: 'useState', onlyIfReferenced: false})); + bound.push(maybeAddImport(this, {module: 'fs/promises', member: 'writeFile', onlyIfReferenced: false})); return super.visitJsCompilationUnit(cu, p); } }); @@ -2850,16 +2869,24 @@ describe('AddImport visitor', () => { await spec.rewriteRun( typescript( ` - const {readFile} = require('fs'); - const {readFile: rf} = require('fs/promises'); + import {useState as useS} from 'react'; + const {writeFile: wf} = require('fs/promises'); - readFile('x'); - rf('y'); + useS(0); + wf('x'); + `, + ` + import {useState as useS, useState} from 'react'; + import {writeFile} from 'fs/promises'; + const {writeFile: wf} = require('fs/promises'); + + useS(0); + wf('x'); ` ) ); - expect(bound).toEqual(['readFile', 'rf']); + expect(bound).toEqual(['useState', 'writeFile']); }); test('a namespace or type declaration shadows an import just as a value declaration does', async () => { From 885768a8789d1bf69d832619985ab922daa2cec5 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 15:22:25 +0200 Subject: [PATCH 05/11] Parse a declared binding against an import only where it can resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bindingContextStatement` prepended `import X from '';` to every template that declared a binding. The import is what carries attribution, and it costs a module resolution to get it, measured over 12 distinct templates on a warm parser: no bindings 49 ms/template bindings, plain import 112 ms/template For a module no `dependencies` entry covers there is no workspace to resolve it against, so the resolution is paid and nothing comes back. Those bindings now parse against `declare const X: any;` — or `type X = any;` where the binding is type-only — which brings the cost back to 31 ms/template. `renameBindings` reads attribution first and falls to the identifier's position without it, so the rename is unaffected. `onlyIfReferenced` is not: recognising the reference a template splices in is precisely an attribution question, and searching for a name that can carry none finds nothing and drops the import. So `resolveBindings` binds an unresolvable module unconditionally, which is sound where the template is known to apply — `tryOn` resolves only once its pattern has matched, and its own test covers a rule that never fires. `ModuleBinding.member` and `typeOnly` now say they shape the import `resolveBindings` creates, since a caller supplying its own `bindings` reads neither. --- .../src/javascript/templating/bindings.ts | 19 ++++++++++++++++--- .../src/javascript/templating/template.ts | 17 +++++++++++------ .../src/javascript/templating/types.ts | 3 ++- .../templating/template-bindings.test.ts | 13 +++++++++++++ 4 files changed, 42 insertions(+), 10 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts b/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts index 1593b94c1ae..ff2452c03bd 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts @@ -19,10 +19,23 @@ import {Cursor} from '../../tree'; import {ModuleBinding} from './types'; /** - * The import that gives a declared binding's name its type attribution. Parsed ahead of the - * template as context, so it does not reach the output. + * Whether the template's dependencies bring a workspace that could resolve `module`. */ -export function bindingContextStatement(name: string, binding: ModuleBinding): string { +export function isResolvable(module: string, dependencies: Record): boolean { + const segments = module.split('/'); + const pkg = module.startsWith('@') ? segments.slice(0, 2).join('/') : segments[0]; + return Object.prototype.hasOwnProperty.call(dependencies, pkg); +} + +/** + * What a declared binding's name is parsed against, ahead of the template and so out of its output. + * An import is what carries attribution, and costs a module resolution to get it; a declaration + * names the binding for a module no workspace could have resolved anyway. + */ +export function bindingContextStatement(name: string, binding: ModuleBinding, dependencies: Record): string { + if (!isResolvable(binding.module, dependencies)) { + return binding.typeOnly ? `type ${name} = any;` : `declare const ${name}: any;`; + } const type = binding.typeOnly ? 'type ' : ''; if (binding.member === '*') { return `import ${type}* as ${name} from '${binding.module}';`; diff --git a/rewrite-javascript/rewrite/src/javascript/templating/template.ts b/rewrite-javascript/rewrite/src/javascript/templating/template.ts index 5e322872d33..fdc0a9c69e9 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/template.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/template.ts @@ -16,7 +16,7 @@ import {Cursor, Tree} from '../..'; import {J} from '../../java'; import {ApplyOptions, Parameter, TemplateOptions, TemplateParameter} from './types'; -import {bindingContextStatement} from './bindings'; +import {bindingContextStatement, isResolvable} from './bindings'; import {maybeAddImport} from '../add-import'; import {JavaScriptVisitor} from '../visitor'; import {MatchResult} from './pattern'; @@ -251,7 +251,8 @@ export class Template { // since they're spliced at construction time, not application time const contextStatements = [ ...(this.options.context || this.options.imports || []), - ...Object.entries(this.options.bindings ?? {}).map(([name, b]) => bindingContextStatement(name, b)) + ...Object.entries(this.options.bindings ?? {}) + .map(([name, b]) => bindingContextStatement(name, b, this.options.dependencies ?? {})) ]; const parametersKey = this.parameters.map((p, i) => { const value = p.value; @@ -293,14 +294,18 @@ export class Template { /** * Binds every module this template declares in the file `visitor` is traversing, and returns - * the local names to hand back through {@link ApplyOptions.bindings}. Safe to call for a node - * the template turns out not to apply to: the import lands in `afterVisit`, by which point the - * file references the name only where the template did land. + * the local names to hand back through {@link ApplyOptions.bindings}. A module the dependencies + * cannot resolve is bound whether or not the template goes on to reference it, so call this + * where the template is known to apply — {@link RewriteRule.tryOn} does, once a pattern matched. */ resolveBindings(visitor: JavaScriptVisitor): Record { const resolved: Record = {}; + const dependencies = this.options.dependencies ?? {}; for (const [name, binding] of Object.entries(this.options.bindings ?? {})) { - resolved[name] = maybeAddImport(visitor, {...binding, preferredName: name}); + // Recognising the reference the template splices in takes attribution, which only the + // import form of a context statement carries. Without one there is nothing to look for. + const onlyIfReferenced = isResolvable(binding.module, dependencies); + resolved[name] = maybeAddImport(visitor, {...binding, preferredName: name, onlyIfReferenced}); } return resolved; } diff --git a/rewrite-javascript/rewrite/src/javascript/templating/types.ts b/rewrite-javascript/rewrite/src/javascript/templating/types.ts index 6b6f85d3bde..b0e3729c1b6 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/types.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/types.ts @@ -500,7 +500,8 @@ export interface TemplateOptions { /** * A module a template's code depends on. The local name comes from the key it is declared under, - * and {@link Template.resolveBindings} settles the rest. + * and {@link Template.resolveBindings} settles the rest. `member` and `typeOnly` shape the import + * it creates, so a caller passing {@link ApplyOptions.bindings} of its own reads neither. */ export type ModuleBinding = Omit & { module: string }; diff --git a/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts b/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts index aafc865cf5d..1d86f2ed884 100644 --- a/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts @@ -16,6 +16,7 @@ import {fromVisitor, RecipeSpec} from "../../../src/test"; import {capture, JavaScriptVisitor, pattern, rewrite, template, typescript} from "../../../src/javascript"; import {J} from "../../../src/java"; +import {bindingContextStatement} from "../../../src/javascript/templating/bindings"; describe('templates that declare module bindings', () => { const spec = new RecipeSpec(); @@ -106,6 +107,18 @@ describe('templates that declare module bindings', () => { ); }); + test('a binding is parsed against an import only where the dependencies could resolve it', () => { + expect(bindingContextStatement('Theming', {module: 'sap/ui/core/Theming', member: 'default'}, {})) + .toBe('declare const Theming: any;'); + expect(bindingContextStatement('Theming', {module: 'sap/ui/core/Theming', member: 'default'}, {sap: '^1.0.0'})) + .toBe("import Theming from 'sap/ui/core/Theming';"); + + expect(bindingContextStatement('Props', {module: '@scope/pkg/props', member: 'Props', typeOnly: true}, {})) + .toBe('type Props = any;'); + expect(bindingContextStatement('Props', {module: '@scope/pkg/props', member: 'Props', typeOnly: true}, {'@scope/pkg': '^1.0.0'})) + .toBe("import type {Props} from '@scope/pkg/props';"); + }); + test('a declared binding left unresolved at apply is an error, not a silent wrong name', async () => { const arg = capture('arg'); const tmpl = template`Theming.setTheme(${arg})`.configure({ From 2338a1238dac332ff7a6ab3218b8f856fe8f69f9 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 15:29:51 +0200 Subject: [PATCH 06/11] A call with nothing selected references its name rather than naming a member MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `renameBindings` judges an unattributed identifier by position, and read every identifier in its parent's `name` slot as being named rather than referenced. A call selects a member from something, so its `name` is that member — but a call with nothing to select from names no member, and `merge(a, b)` puts the function being called in exactly that slot. Bare call callees were therefore left alone. The template rendered the name it declared while the import bound another, which is silent where a file already binds the declared name to something else: template`merge(${a}, ${b})`.configure({bindings: {merge: {module: 'sap/base/util/merge'}}}) // const merge = 1; import merge_1 from 'sap/base/util/merge'; merge('dark', {}); // calls the local, not the module Attribution had been covering it: a context import gave the identifier an owner tracing to the module, and the rename took that branch without consulting position. Parsing an unresolvable module against a declaration instead left position to decide alone, where it was wrong. Found by the UI5 port, which renders `jQuery.sap.extend` through `merge($2, $3)` into blocks that bind `merge` to something else. --- .../rewrite/src/javascript/templating/bindings.ts | 12 ++++++++++-- .../templating/template-bindings.test.ts | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts b/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts index ff2452c03bd..56f396bd803 100644 --- a/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts +++ b/rewrite-javascript/rewrite/src/javascript/templating/bindings.ts @@ -79,13 +79,21 @@ class RenameBindingsVisitor extends JavaScriptVisitor { } } -/** An identifier in its parent's `name` slot is being named — a property, a method, a declaration. */ +/** Whether the parent is naming this identifier — as a property, a method, a declaration — rather than referencing it. */ function namesItsParent(cursor: Cursor, identifier: J.Identifier): boolean { let c: Cursor | undefined = cursor.parent; while (c && isPadding(c.value)) { c = c.parent; } - const name = (c?.value as { name?: unknown } | undefined)?.name; + const parent = c?.value as { kind?: string; name?: unknown; select?: unknown } | undefined; + + // A call names a member of whatever it selects from. With nothing selected there is no member, + // and its `name` is a reference to the function being called. + if (parent?.kind === J.Kind.MethodInvocation && !parent.select) { + return false; + } + + const name = parent?.name; return name === identifier || (name as { element?: unknown } | undefined)?.element === identifier; } diff --git a/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts b/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts index 1d86f2ed884..a5feea88c98 100644 --- a/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts @@ -95,6 +95,21 @@ describe('templates that declare module bindings', () => { ); }); + test('a binding called on its own is a reference to it, not a name it gives something', async () => { + const arg = capture('arg'); + spec.recipe = recipeApplying(template`merge(${arg}, {})`.configure({ + bindings: {merge: {module: 'sap/base/util/merge', member: 'default'}} + }), arg); + + await spec.rewriteRun( + //language=typescript + typescript( + `const merge = 1;\napplyTheme('dark');`, + `import merge_1 from 'sap/base/util/merge';\n\nconst merge = 1;\nmerge_1('dark', {});` + ) + ); + }); + test('a rule that does not fire leaves the file\'s imports alone', async () => { const arg = capture('arg'); spec.recipe = recipeApplying(template`Theming.setTheme(${arg})`.configure({ From 68ac5eb0241d28a28b948eadc0b170dc365d920d Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 15:54:24 +0200 Subject: [PATCH 07/11] Read a binding pattern once, and a require call one way MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two walks of an object binding pattern had drifted apart. The one behind plain declarations recursed; the one behind requires read only a flat identifier, so `const {a: {b}} = require('m')` put nothing in the pool and an import was free to bind `b` a second time: maybeAddImport(v, {module: 'm', member: 'b'}) import {b} from 'm'; // alongside the require, binding `b` twice `patternNames` is now the single walk. A name bound directly by the pattern reads a member of the module; anything deeper reads a property of a property, so it occupies its name without the module answering for it. Three predicates decided what a `require` call is, and each had tightened a different clause: one demanded no `select`, another a string specifier, the third neither. `obj.require('fs')` was a require to one reader and not another. `requiredModuleOf` decides once, demanding both. A merged specifier sorted by `alias || member` while its neighbours sorted by the name they bind, so a request naming a `preferredName` — which every template binding does — sorted by the wrong key. A pinned alias is settled by the queue scan alone, so the pool it used to build and discard is gone, along with the three separate places that encoded "an alias skips this step". `ChangeImport` pins on every call site and paid that walk each time. `derivedName` is the one place the fallback order is written, rather than two four hundred lines apart, and `AddImport.preferredName` is gone — its only reader was its own constructor. --- .../rewrite/src/javascript/add-import.ts | 113 ++++++++++-------- .../test/javascript/add-import.test.ts | 56 +++++++++ .../templating/template-bindings.test.ts | 4 +- 3 files changed, 123 insertions(+), 50 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/add-import.ts b/rewrite-javascript/rewrite/src/javascript/add-import.ts index acaa91b7ad5..0c192d7803a 100644 --- a/rewrite-javascript/rewrite/src/javascript/add-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/add-import.ts @@ -135,7 +135,13 @@ export function maybeAddImport( return undefined; } - const derived = options.alias ?? options.preferredName ?? memberName(options.member) ?? module; + // A pinned alias is the whole answer, so the file has no say and nothing below needs to read it. + if (options.alias) { + visitor.afterVisit.push(new AddImport(options, options.alias)); + return options.alias; + } + + const derived = derivedName(options); const cu = compilationUnitOf(visitor); if (!cu) { visitor.afterVisit.push(new AddImport(options, derived)); @@ -148,21 +154,24 @@ export function maybeAddImport( // 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. - if (!options.alias) { - for (const binding of bindings) { - if (binding.module === module && binding.member === memberName(options.member) && - binding.typeOnly === typeOnly && - (options.preferredName !== undefined || binding.name === derived)) { - return binding.name; - } + for (const binding of bindings) { + if (binding.module === module && binding.member === memberName(options.member) && + binding.typeOnly === typeOnly && + (options.preferredName !== undefined || binding.name === derived)) { + return binding.name; } } - const name = options.alias ?? deconflict(derived, takenNames(bindings, visitor)); + const name = deconflict(derived, takenNames(bindings, visitor)); visitor.afterVisit.push(new AddImport(options, name)); return name; } +/** The local name a request asks for, before the file has a say in it. */ +function derivedName(options: AddImportOptions): string { + return options.alias ?? options.preferredName ?? memberName(options.member) ?? moduleNameOf(options.module); +} + /** `'default'` and an absent member both name a default import, which binds no member name of its own. */ function memberName(member: string | undefined): string | undefined { return member === 'default' ? undefined : member; @@ -189,14 +198,8 @@ function moduleScopeBindings(cu: JS.CompilationUnit): ModuleScopeBinding[] { const bindings: ModuleScopeBinding[] = []; const declaredBy = (name: J | undefined): void => { - if (name?.kind === J.Kind.Identifier) { - bindings.push({name: (name as J.Identifier).simpleName}); - } else if (name?.kind === JS.Kind.ObjectBindingPattern) { - for (const elem of (name as JS.ObjectBindingPattern).bindings.elements) { - if (elem.element?.kind === JS.Kind.BindingElement) { - declaredBy((elem.element as JS.BindingElement).name); - } - } + for (const bound of patternNames(name)) { + bindings.push({name: bound.name}); } }; @@ -247,27 +250,39 @@ function moduleScopeBindings(cu: JS.CompilationUnit): ModuleScopeBinding[] { /** The module a `require('...')` initializer names, `undefined` for an initializer that is anything else. */ function requiredModule(initializer: J | undefined): string | undefined { - if (initializer?.kind !== J.Kind.MethodInvocation) { - return undefined; - } - const methodInv = initializer as J.MethodInvocation; + return initializer?.kind === J.Kind.MethodInvocation + ? requiredModuleOf(initializer as J.MethodInvocation) + : undefined; +} + +/** + * The module a `require(...)` call loads. `obj.require('x')` selects a method rather than loading a + * module, and a specifier that is not a string literal names none that can be read here. + */ +function requiredModuleOf(methodInv: J.MethodInvocation): string | undefined { if (methodInv.select || methodInv.name?.kind !== J.Kind.Identifier || (methodInv.name as J.Identifier).simpleName !== 'require') { return undefined; } const argument = methodInv.arguments?.elements[0]?.element; - return argument?.kind === J.Kind.Literal ? moduleNameOf(argument as J.Literal) : undefined; + return argument?.kind === J.Kind.Literal && typeof (argument as J.Literal).value === 'string' + ? moduleNameOf(argument as J.Literal) + : undefined; } -/** 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[] { +/** + * 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, module, member: undefined, typeOnly: false}]; + return [{name: (pattern as J.Identifier).simpleName}]; } if (pattern?.kind !== JS.Kind.ObjectBindingPattern) { return []; } - const bindings: ModuleScopeBinding[] = []; + const names: { name: string; member?: string }[] = []; for (const elem of (pattern as JS.ObjectBindingPattern).bindings.elements) { if (elem.element?.kind !== JS.Kind.BindingElement) { continue; @@ -276,13 +291,28 @@ function requireBindings(pattern: J | undefined, module: string): ModuleScopeBin if (bindingElem.name?.kind === J.Kind.Identifier) { const name = (bindingElem.name as J.Identifier).simpleName; const propertyName = bindingElem.propertyName?.element; - const member = propertyName?.kind === J.Kind.Identifier - ? (propertyName as J.Identifier).simpleName - : name; - bindings.push({name, module, member, typeOnly: false}); + 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 bindings; + 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 + ? {name: bound.name} + : {name: bound.name, module, member: bound.member, typeOnly: false}); } function importBindings(jsImport: JS.Import): ModuleScopeBinding[] { @@ -491,7 +521,6 @@ export class AddImport

extends JavaScriptVisitor

{ readonly moduleUnicodeEscapes?: J.LiteralUnicodeEscape[]; readonly member?: string; readonly alias?: string; - readonly preferredName?: string; readonly onlyIfReferenced: boolean; readonly sideEffectOnly: boolean; readonly typeOnly: boolean; @@ -537,13 +566,12 @@ export class AddImport

extends JavaScriptVisitor

{ this.moduleUnicodeEscapes = typeof options.module === 'string' ? undefined : options.module.unicodeEscapes; this.member = options.member; this.alias = options.alias; - this.preferredName = options.preferredName; this.onlyIfReferenced = options.onlyIfReferenced ?? true; this.sideEffectOnly = options.sideEffectOnly ?? false; this.typeOnly = options.typeOnly ?? false; this.style = options.style; this.quoteStyle = options.quoteStyle; - this.bindingName = this.sideEffectOnly ? undefined : bindingName ?? this.alias ?? this.preferredName ?? memberName(this.member) ?? this.module; + this.bindingName = this.sideEffectOnly ? undefined : bindingName ?? derivedName(options); } /** @@ -560,8 +588,7 @@ export class AddImport

extends JavaScriptVisitor

{ * Check if a method invocation is a require() call */ private isRequireCall(methodInv: J.MethodInvocation): boolean { - return methodInv.name?.kind === J.Kind.Identifier && - (methodInv.name as J.Identifier).simpleName === 'require'; + return requiredModuleOf(methodInv) !== undefined; } /** @@ -882,7 +909,7 @@ export class AddImport

extends JavaScriptVisitor

{ const existingElements = namedImports.elements.elements; // Find the correct insertion position (alphabetical, case-insensitive) - const newName = (this.alias || this.member!).toLowerCase(); + const newName = this.bindingName!.toLowerCase(); let insertIndex = existingElements.findIndex(elem => { if (elem.element?.kind === JS.Kind.ImportSpecifier) { const name = this.getImportAlias(elem.element) || this.getImportName(elem.element); @@ -1707,17 +1734,7 @@ export class AddImport

extends JavaScriptVisitor

{ * Get the module name from a require() call */ private getModuleNameFromRequire(methodInv: J.MethodInvocation): string | undefined { - const args = methodInv.arguments?.elements; - if (!args || args.length === 0) { - return undefined; - } - - const firstArg = args[0].element; - if (!firstArg || firstArg.kind !== J.Kind.Literal || typeof (firstArg as J.Literal).value !== 'string') { - return undefined; - } - - return moduleNameOf(firstArg as J.Literal); + return requiredModuleOf(methodInv); } /** diff --git a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts index 4ae488ae31f..1a7a2b0f9b7 100644 --- a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts @@ -2952,6 +2952,62 @@ describe('AddImport visitor', () => { expect(preferred).toEqual(['Theming', 'Theming']); }); + test('a name bound through a nested pattern is occupied without answering for the module', async () => { + const spec = new RecipeSpec(); + const bound: string[] = []; + spec.recipe = fromVisitor(new class extends JavaScriptVisitor { + override async visitJsCompilationUnit(cu: JS.CompilationUnit, p: any): Promise { + bound.push(maybeAddImport(this, {module: 'm', member: 'b', onlyIfReferenced: false})); + bound.push(maybeAddImport(this, {module: 'other', preferredName: 'z', onlyIfReferenced: false})); + return super.visitJsCompilationUnit(cu, p); + } + }); + + await spec.rewriteRun( + typescript( + ` + const {a: {b}} = require('other'); + + b(); + `, + ` + import {b as b_1} from 'm'; + import z from 'other'; + + const {a: {b}} = require('other'); + + b(); + ` + ) + ); + + expect(bound).toEqual(['b_1', 'z']); + }); + + test('a merged specifier sorts by the name it binds, as its neighbours do', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName( + {module: 'm', member: 'zzz', preferredName: 'aaa', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + import {mmm} from 'm'; + + mmm(); + `, + ` + import {zzz as aaa, mmm} from 'm'; + + mmm(); + ` + ) + ); + + expect(bound.name).toBe('aaa'); + }); + test('a side-effect import binds no name', async () => { const spec = new RecipeSpec(); const bound: { name?: string } = {name: 'unset'}; diff --git a/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts b/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts index a5feea88c98..6b3e0fcd993 100644 --- a/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/templating/template-bindings.test.ts @@ -97,7 +97,7 @@ describe('templates that declare module bindings', () => { test('a binding called on its own is a reference to it, not a name it gives something', async () => { const arg = capture('arg'); - spec.recipe = recipeApplying(template`merge(${arg}, {})`.configure({ + spec.recipe = recipeApplying(template`merge(${arg}, {}).merge()`.configure({ bindings: {merge: {module: 'sap/base/util/merge', member: 'default'}} }), arg); @@ -105,7 +105,7 @@ describe('templates that declare module bindings', () => { //language=typescript typescript( `const merge = 1;\napplyTheme('dark');`, - `import merge_1 from 'sap/base/util/merge';\n\nconst merge = 1;\nmerge_1('dark', {});` + `import merge_1 from 'sap/base/util/merge';\n\nconst merge = 1;\nmerge_1('dark', {}).merge();` ) ); }); From 01fcb0675820f266473858f530c4c6a88f7a127e Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 16:30:46 +0200 Subject: [PATCH 08/11] Ask the file what it binds in one place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two readers enumerated a compilation unit's bindings. `moduleScopeBindings` and its `importBindings`/`requireBindings` answer at call time, for the name a request will be bound to; `isMatchingImport`/`isMatchingRequire` answered at apply time, for whether the import is already there. Both walked the same nodes for the same fields, and they drifted three times over this branch — a specifier's bound name computed as `aliasName ?? importName` in one and as `name` in the other, three disagreeing spellings of what a `require` call is, and a nested binding pattern one recursed into and the other did not. The matchers now query the enumeration rather than repeating it. What stays separate is the question, because the two really are different: the reuse lookup asks what a request will be called, `answeredBy` asks whether a binding already serves it. What they can no longer disagree about is the file. The asymmetry that made the matchers look special was a default import the request named nothing for, which any default import of the module satisfied through a bare `return true`. That is `anyNameAnswers`, said once. Merge ordering read a specifier's bound name through its own pair of helpers; `specifierBinding` is the reader the pool already used. With the last callers gone, `getImportName`, `getImportAlias`, `isRequireCall` and `getModuleNameFromRequire` go with them. Two shapes tighten as a result, both toward what the file actually binds: `const {foo: readFile} = require('fs')` no longer answers a request for member `readFile`, since it binds `fs.foo`, and a name reached through a nested pattern answers for no member at all. --- .../rewrite/src/javascript/add-import.ts | 236 ++++-------------- 1 file changed, 47 insertions(+), 189 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/add-import.ts b/rewrite-javascript/rewrite/src/javascript/add-import.ts index 0c192d7803a..eb626b6cb16 100644 --- a/rewrite-javascript/rewrite/src/javascript/add-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/add-import.ts @@ -315,6 +315,21 @@ function requireBindings(pattern: J | undefined, module: string): ModuleScopeBin : {name: bound.name, module, member: bound.member, typeOnly: false}); } +/** The member a specifier imports and the name it binds it under, which are the same absent an alias. */ +function specifierBinding(specifier: JS.ImportSpecifier): { name: string; member: string } | undefined { + if (specifier.specifier?.kind === J.Kind.Identifier) { + const name = (specifier.specifier as J.Identifier).simpleName; + return {name, member: name}; + } + if (specifier.specifier?.kind !== JS.Kind.Alias) { + return undefined; + } + const alias = specifier.specifier as JS.Alias; + return alias.propertyName?.element?.kind === J.Kind.Identifier && alias.alias?.kind === J.Kind.Identifier + ? {name: (alias.alias as J.Identifier).simpleName, member: (alias.propertyName.element as J.Identifier).simpleName} + : undefined; +} + function importBindings(jsImport: JS.Import): ModuleScopeBinding[] { const moduleSpecifier = jsImport.moduleSpecifier?.element; const module = moduleSpecifier?.kind === J.Kind.Literal @@ -342,23 +357,11 @@ function importBindings(jsImport: JS.Import): ModuleScopeBinding[] { } } else if (namedBindings?.kind === JS.Kind.NamedImports) { for (const elem of (namedBindings as JS.NamedImports).elements.elements) { - if (elem.element?.kind !== JS.Kind.ImportSpecifier) { - continue; - } - const specifier = elem.element as JS.ImportSpecifier; - if (specifier.specifier?.kind === J.Kind.Identifier) { - const name = (specifier.specifier as J.Identifier).simpleName; - bindings.push({name, module, member: name, typeOnly}); - } else if (specifier.specifier?.kind === JS.Kind.Alias) { - const alias = specifier.specifier as JS.Alias; - if (alias.propertyName?.element?.kind === J.Kind.Identifier && alias.alias?.kind === J.Kind.Identifier) { - bindings.push({ - name: (alias.alias as J.Identifier).simpleName, - module, - member: (alias.propertyName.element as J.Identifier).simpleName, - typeOnly - }); - } + const bound = elem.element?.kind === JS.Kind.ImportSpecifier + ? specifierBinding(elem.element as JS.ImportSpecifier) + : undefined; + if (bound) { + bindings.push({...bound, module, typeOnly}); } } } @@ -528,6 +531,11 @@ export class AddImport

extends JavaScriptVisitor

{ readonly quoteStyle?: QuoteChar; /** The local name this import binds; `undefined` for a side-effect import. */ readonly bindingName?: string; + /** + * A default import the request named no local name for, so whatever the file already calls + * that module answers it. + */ + readonly anyNameAnswers: boolean; constructor(options: AddImportOptions, bindingName?: string) { super(); @@ -572,6 +580,8 @@ export class AddImport

extends JavaScriptVisitor

{ this.style = options.style; this.quoteStyle = options.quoteStyle; this.bindingName = this.sideEffectOnly ? undefined : bindingName ?? derivedName(options); + this.anyNameAnswers = !this.sideEffectOnly && memberName(options.member) === undefined && + options.alias === undefined && options.preferredName === undefined; } /** @@ -584,12 +594,6 @@ export class AddImport

extends JavaScriptVisitor

{ return moduleNameOf(moduleSpecifier as J.Literal); } - /** - * Check if a method invocation is a require() call - */ - private isRequireCall(methodInv: J.MethodInvocation): boolean { - return requiredModuleOf(methodInv) !== undefined; - } /** * Determine the appropriate import style based on file type and existing imports @@ -668,8 +672,7 @@ export class AddImport

extends JavaScriptVisitor

{ if (varDecl.variables.length === 1) { const namedVar = varDecl.variables[0].element; const initializer = namedVar?.initializer?.element; - if (initializer?.kind === J.Kind.MethodInvocation && - this.isRequireCall(initializer as J.MethodInvocation)) { + if (requiredModule(initializer) !== undefined) { hasCommonJSRequires = true; } } @@ -746,12 +749,8 @@ export class AddImport

extends JavaScriptVisitor

{ const namedVar = varDecl.variables[0].element; const initializer = namedVar?.initializer?.element; - if (initializer?.kind === J.Kind.MethodInvocation && - this.isRequireCall(initializer as J.MethodInvocation)) { - const moduleName = this.getModuleNameFromRequire(initializer as J.MethodInvocation); - if (moduleName === this.module) { - return ImportStyle.CommonJS; - } + if (requiredModule(initializer) === this.module) { + return ImportStyle.CommonJS; } } } @@ -912,7 +911,7 @@ export class AddImport

extends JavaScriptVisitor

{ const newName = this.bindingName!.toLowerCase(); let insertIndex = existingElements.findIndex(elem => { if (elem.element?.kind === JS.Kind.ImportSpecifier) { - const name = this.getImportAlias(elem.element) || this.getImportName(elem.element); + const name = specifierBinding(elem.element as JS.ImportSpecifier)?.name ?? ''; return newName.localeCompare(name.toLowerCase()) < 0; } return false; @@ -1070,93 +1069,27 @@ export class AddImport

extends JavaScriptVisitor

{ * Check if the import matches what we're trying to add */ private isMatchingImport(jsImport: JS.Import): boolean { - // Check module specifier const moduleSpecifier = jsImport.moduleSpecifier?.element; - if (!moduleSpecifier) { - return false; - } - - const moduleName = this.getModuleName(moduleSpecifier); - if (moduleName !== this.module) { + if (!moduleSpecifier || this.getModuleName(moduleSpecifier) !== this.module) { return false; } + // A side-effect import is the clause-less one, and no import carrying bindings stands in + // for it — nor it for them. const importClause = jsImport.importClause; - - // Handle side-effect imports (no import clause) - if (!importClause) { - // If we're trying to add a side-effect import and one already exists, it's a match - return this.sideEffectOnly; + if (!importClause || this.sideEffectOnly) { + return !importClause && this.sideEffectOnly; } - // If we're adding a side-effect import but there's an existing import with bindings, - // it's not a match (side-effect import should be separate) - if (this.sideEffectOnly) { - return false; - } - - // Check if the typeOnly flag matches - type-only and value imports are separate - if (importClause.typeOnly !== this.typeOnly) { - return false; - } - - // Check if the specific member or default import already exists - if (this.member === '*') { - // We're adding a namespace import, check if one exists - const namedBindings = importClause.namedBindings; - if (!namedBindings) { - return false; - } - - // Namespace imports can be represented as J.Identifier or JS.Alias - if (namedBindings.kind === J.Kind.Identifier) { - const identifier = namedBindings as J.Identifier; - return identifier.simpleName === this.alias; - } else if (namedBindings.kind === JS.Kind.Alias) { - const alias = namedBindings as JS.Alias; - if (alias.alias?.kind === J.Kind.Identifier) { - return (alias.alias as J.Identifier).simpleName === this.alias; - } - } - return false; - } else if (this.member === undefined || this.member === 'default') { - // We're adding a default import, check if one exists - // For member === 'default', also verify the alias matches if specified - if (importClause.name === undefined) { - return false; - } - // If we have an alias, check that it matches - if (this.alias && importClause.name.element?.kind === J.Kind.Identifier) { - const existingName = (importClause.name.element as J.Identifier).simpleName; - return existingName === this.alias; - } - return true; - } else { - // We're adding a named import, check if it exists - const namedBindings = importClause.namedBindings; - if (!namedBindings) { - return false; - } - - if (namedBindings.kind === JS.Kind.NamedImports) { - const namedImports = namedBindings as JS.NamedImports; - for (const elem of namedImports.elements.elements) { - if (elem.element?.kind === JS.Kind.ImportSpecifier) { - const specifier = elem.element as JS.ImportSpecifier; - const importName = this.getImportName(specifier); - const aliasName = this.getImportAlias(specifier); - - // A specifier binds its alias, or its own name where it has none; that is - // what this request has to match, however it came by the name it binds. - if (importName === this.member && (aliasName ?? importName) === this.bindingName) { - return true; - } - } - } - } - } + return importBindings(jsImport).some(binding => this.answeredBy(binding)); + } - return false; + /** Whether a binding the file already has serves this request. */ + private answeredBy(binding: ModuleScopeBinding): boolean { + return binding.module === this.module && + binding.member === memberName(this.member) && + binding.typeOnly === this.typeOnly && + (this.anyNameAnswers || binding.name === this.bindingName); } /** @@ -1166,52 +1099,12 @@ export class AddImport

extends JavaScriptVisitor

{ if (varDecl.variables.length !== 1) { return false; } - const namedVar = varDecl.variables[0].element; - if (!namedVar) { - return false; - } - - const initializer = namedVar.initializer?.element; - if (!initializer || initializer.kind !== J.Kind.MethodInvocation) { - return false; - } - - const methodInv = initializer as J.MethodInvocation; - if (!this.isRequireCall(methodInv)) { - return false; - } - - const moduleName = this.getModuleNameFromRequire(methodInv); - if (moduleName !== this.module) { + const module = requiredModule(namedVar?.initializer?.element); + if (module !== this.module) { return false; } - - // Check if the variable name matches what we're trying to add - const pattern = namedVar.name; - if ((this.member === undefined || this.member === 'default') && pattern?.kind === J.Kind.Identifier) { - // Default import style: const fs = require('fs') - // For member === 'default', also check the alias matches if specified - if (this.alias) { - const varName = (pattern as J.Identifier).simpleName; - return varName === this.alias; - } - return true; - } else if (this.member !== undefined && this.member !== 'default' && pattern?.kind === JS.Kind.ObjectBindingPattern) { - // Destructured import: const { member } = require('module') - const objectPattern = pattern as JS.ObjectBindingPattern; - for (const elem of objectPattern.bindings.elements) { - if (elem.element?.kind === JS.Kind.BindingElement) { - const bindingElem = elem.element as JS.BindingElement; - const name = (bindingElem.name as J.Identifier)?.simpleName; - if (name === this.bindingName) { - return true; - } - } - } - } - - return false; + return requireBindings(namedVar!.name, module).some(binding => this.answeredBy(binding)); } /** @@ -1730,41 +1623,6 @@ export class AddImport

extends JavaScriptVisitor

{ return 0; } - /** - * Get the module name from a require() call - */ - private getModuleNameFromRequire(methodInv: J.MethodInvocation): string | undefined { - return requiredModuleOf(methodInv); - } - /** - * Get the import name from an import specifier - */ - private getImportName(specifier: JS.ImportSpecifier): string { - const spec = specifier.specifier; - if (spec?.kind === JS.Kind.Alias) { - const alias = spec as JS.Alias; - const propertyName = alias.propertyName.element; - if (propertyName?.kind === J.Kind.Identifier) { - return (propertyName as J.Identifier).simpleName; - } - } else if (spec?.kind === J.Kind.Identifier) { - return (spec as J.Identifier).simpleName; - } - return ''; - } - /** - * Get the import alias from an import specifier - */ - private getImportAlias(specifier: JS.ImportSpecifier): string | undefined { - const spec = specifier.specifier; - if (spec?.kind === JS.Kind.Alias) { - const alias = spec as JS.Alias; - if (alias.alias?.kind === J.Kind.Identifier) { - return (alias.alias as J.Identifier).simpleName; - } - } - return undefined; - } } From e9c48ac197609f96f37e40c20329d5c8bda8dcfa Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 17:01:48 +0200 Subject: [PATCH 09/11] Count the names an enclosing scope declares as taken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pool held what the file binds at module scope. A template lands somewhere inside that file, though, and a name declared between there and the top is the one its code will reach: import merge from 'sap/base/util/merge'; function handler(merge) { return merge('x', {}); // the parameter, not the import } `bindingsInScope` walks the cursor out to the compilation unit and counts what each frame declares — a function's parameters, a lambda's, a block's declarations — as occupying the name without the module answering for it. The statement scan the compilation unit already used serves a block unchanged. This reaches what encloses the anchor, which is not yet all of it: `var` and function declarations bind across their whole function, so one inside a sibling block is in scope at the anchor and is not seen here. A name that reaches the anchor from a scope this misses still shadows, so that stays wrong until a scope utility answers reach per declaration kind rather than per frame. --- .../rewrite/src/javascript/add-import.ts | 49 ++++++++++++++++-- .../test/javascript/add-import.test.ts | 51 +++++++++++++++++++ 2 files changed, 95 insertions(+), 5 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/add-import.ts b/rewrite-javascript/rewrite/src/javascript/add-import.ts index eb626b6cb16..936c9463b0a 100644 --- a/rewrite-javascript/rewrite/src/javascript/add-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/add-import.ts @@ -148,7 +148,7 @@ export function maybeAddImport( return derived; } - const bindings = moduleScopeBindings(cu); + 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 @@ -186,15 +186,54 @@ interface ModuleScopeBinding { typeOnly?: boolean; } +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. - const cursor = (visitor as unknown as { cursor?: Cursor }).cursor; - return cursor?.firstEnclosing((v): v is JS.CompilationUnit => v?.kind === JS.Kind.CompilationUnit); + return cursorOf(visitor)?.firstEnclosing((v): v is JS.CompilationUnit => v?.kind === JS.Kind.CompilationUnit); +} + +/** 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; +} + +/** 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 moduleScopeBindings(cu: JS.CompilationUnit): ModuleScopeBinding[] { +function statementBindings(statements: J.RightPadded[]): ModuleScopeBinding[] { const bindings: ModuleScopeBinding[] = []; const declaredBy = (name: J | undefined): void => { @@ -214,7 +253,7 @@ function moduleScopeBindings(cu: JS.CompilationUnit): ModuleScopeBinding[] { } }; - for (const stmt of cu.statements) { + for (const stmt of statements) { const statement = stmt.element; switch (statement?.kind) { case JS.Kind.Import: diff --git a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts index 1a7a2b0f9b7..4ee34d90fa1 100644 --- a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts @@ -3008,6 +3008,57 @@ describe('AddImport visitor', () => { expect(bound.name).toBe('aaa'); }); + test('a name an enclosing scope declares is taken where the import would be used', async () => { + 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. + override async visitMethodInvocation(method: J.MethodInvocation, p: any): Promise { + if ((method.name as J.Identifier)?.simpleName === 'anchor') { + bound.push(maybeAddImport(this, + {module: `m${bound.length}`, member: 'merge', onlyIfReferenced: false})); + } + return super.visitMethodInvocation(method, p); + } + }); + + await spec.rewriteRun( + typescript( + ` + function byParameter(merge) { + anchor(); + } + + function byDeclaration() { + const merge = 1; + anchor(); + } + + const byLambda = (merge) => anchor(); + `, + ` + import {merge as merge_1} from 'm0'; + import {merge as merge_2} from 'm1'; + import {merge as merge_3} from 'm2'; + + function byParameter(merge) { + anchor(); + } + + function byDeclaration() { + const merge = 1; + anchor(); + } + + const byLambda = (merge) => anchor(); + ` + ) + ); + + expect(bound).toEqual(['merge_1', 'merge_2', 'merge_3']); + }); + test('a side-effect import binds no name', async () => { const spec = new RecipeSpec(); const bound: { name?: string } = {name: 'unset'}; From 5ba5298ad6b2a94ca8de018c91a0d103929a2cc3 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 18:04:05 +0200 Subject: [PATCH 10/11] Let both sides agree on what answers a request that named nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `{module: 'lodash'}` names no local name and asks for no member, so whatever the file already calls that module answers it. `AddImport` read it that way and skipped emitting; the reuse lookup did not, and handed back a name nothing binds: maybeAddImport(v, {module: 'lodash', onlyIfReferenced: false}) // on `import Foo from 'lodash';` returned "lodash", emitted nothing The two were separate spellings of one question, so `anyNameAnswers` is now the function both read. A caller that named a preference is still answered only by an exact name at emission, which is deliberate: by then the name has been resolved and a fresh binding is what is being added. Whether a call is a `require` is a question about its shape, and which module it loads is a further question about its argument. Folding the first into the second narrowed style detection, which asks only the first — a file whose sole require passes a variable stopped counting as CommonJS. `isRequireCall` answers the shape and `requiredModuleOf` builds on it. No output moves today, since the CommonJS style falls back to ES6 until require creation exists. Option validation ran after the queue scan, so a request the queue could answer was never checked. It is a property of the request, so `validate` runs first. --- .../rewrite/src/javascript/add-import.ts | 89 ++++++++++++------- .../test/javascript/add-import.test.ts | 18 ++++ 2 files changed, 73 insertions(+), 34 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/add-import.ts b/rewrite-javascript/rewrite/src/javascript/add-import.ts index 936c9463b0a..76336a9f2d5 100644 --- a/rewrite-javascript/rewrite/src/javascript/add-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/add-import.ts @@ -109,6 +109,7 @@ export function maybeAddImport( visitor: JavaScriptVisitor, options: AddImportOptions ): string | undefined { + validate(options); const module = moduleNameOf(options.module); const sideEffectOnly = options.sideEffectOnly ?? false; const typeOnly = options.typeOnly ?? false; @@ -157,7 +158,8 @@ export function maybeAddImport( for (const binding of bindings) { if (binding.module === module && binding.member === memberName(options.member) && binding.typeOnly === typeOnly && - (options.preferredName !== undefined || binding.name === derived)) { + (options.preferredName !== undefined || anyNameAnswers(options) || + binding.name === derived)) { return binding.name; } } @@ -167,6 +169,47 @@ export function maybeAddImport( return name; } +/** Rejects a request on its own terms, whatever the queue already holds. */ +function validate(options: AddImportOptions): void { + // Validate that a name is provided when member is 'default' + if (options.member === 'default' && !options.alias && !options.preferredName) { + throw new Error("When member is 'default', the alias parameter is required"); + } + + // Validate that a name is provided when member is '*' (namespace import) + if (options.member === '*' && !options.alias && !options.preferredName) { + throw new Error("When member is '*', the alias parameter is required"); + } + + // Validate that sideEffectOnly is not combined with incompatible options + if (options.sideEffectOnly) { + if (options.member !== undefined) { + throw new Error("Cannot combine sideEffectOnly with member"); + } + if (options.alias !== undefined) { + throw new Error("Cannot combine sideEffectOnly with alias"); + } + if (options.preferredName !== undefined) { + throw new Error("Cannot combine sideEffectOnly with preferredName"); + } + if (options.onlyIfReferenced !== undefined) { + throw new Error("Cannot combine sideEffectOnly with onlyIfReferenced"); + } + if (options.typeOnly) { + throw new Error("Cannot combine sideEffectOnly with typeOnly"); + } + } +} + +/** + * A request that named no local name and asks for no member of the module. Having expressed no + * preference it has none to disappoint, so whatever the file already calls that module answers it. + */ +function anyNameAnswers(options: AddImportOptions): boolean { + return !options.sideEffectOnly && memberName(options.member) === undefined && + options.alias === undefined && options.preferredName === undefined; +} + /** The local name a request asks for, before the file has a say in it. */ function derivedName(options: AddImportOptions): string { return options.alias ?? options.preferredName ?? memberName(options.member) ?? moduleNameOf(options.module); @@ -294,13 +337,18 @@ function requiredModule(initializer: J | undefined): string | undefined { : undefined; } +/** A `require(...)` call, whatever it is passed. `obj.require('x')` selects a method rather than loading a module. */ +function isRequireCall(methodInv: J.MethodInvocation): boolean { + return !methodInv.select && methodInv.name?.kind === J.Kind.Identifier && + (methodInv.name as J.Identifier).simpleName === 'require'; +} + /** * The module a `require(...)` call loads. `obj.require('x')` selects a method rather than loading a * module, and a specifier that is not a string literal names none that can be read here. */ function requiredModuleOf(methodInv: J.MethodInvocation): string | undefined { - if (methodInv.select || methodInv.name?.kind !== J.Kind.Identifier || - (methodInv.name as J.Identifier).simpleName !== 'require') { + if (!isRequireCall(methodInv)) { return undefined; } const argument = methodInv.arguments?.elements[0]?.element; @@ -579,34 +627,7 @@ export class AddImport

extends JavaScriptVisitor

{ constructor(options: AddImportOptions, bindingName?: string) { super(); - // Validate that a name is provided when member is 'default' - if (options.member === 'default' && !options.alias && !options.preferredName) { - throw new Error("When member is 'default', the alias parameter is required"); - } - - // Validate that a name is provided when member is '*' (namespace import) - if (options.member === '*' && !options.alias && !options.preferredName) { - throw new Error("When member is '*', the alias parameter is required"); - } - - // Validate that sideEffectOnly is not combined with incompatible options - if (options.sideEffectOnly) { - if (options.member !== undefined) { - throw new Error("Cannot combine sideEffectOnly with member"); - } - if (options.alias !== undefined) { - throw new Error("Cannot combine sideEffectOnly with alias"); - } - if (options.preferredName !== undefined) { - throw new Error("Cannot combine sideEffectOnly with preferredName"); - } - if (options.onlyIfReferenced !== undefined) { - throw new Error("Cannot combine sideEffectOnly with onlyIfReferenced"); - } - if (options.typeOnly) { - throw new Error("Cannot combine sideEffectOnly with typeOnly"); - } - } + validate(options); this.module = moduleNameOf(options.module); this.moduleValueSource = typeof options.module === 'string' ? undefined : options.module.valueSource; @@ -619,8 +640,7 @@ export class AddImport

extends JavaScriptVisitor

{ this.style = options.style; this.quoteStyle = options.quoteStyle; this.bindingName = this.sideEffectOnly ? undefined : bindingName ?? derivedName(options); - this.anyNameAnswers = !this.sideEffectOnly && memberName(options.member) === undefined && - options.alias === undefined && options.preferredName === undefined; + this.anyNameAnswers = anyNameAnswers(options); } /** @@ -711,7 +731,8 @@ export class AddImport

extends JavaScriptVisitor

{ if (varDecl.variables.length === 1) { const namedVar = varDecl.variables[0].element; const initializer = namedVar?.initializer?.element; - if (requiredModule(initializer) !== undefined) { + if (initializer?.kind === J.Kind.MethodInvocation && + isRequireCall(initializer as J.MethodInvocation)) { hasCommonJSRequires = true; } } diff --git a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts index 4ee34d90fa1..b2f50d00431 100644 --- a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts @@ -3059,6 +3059,24 @@ describe('AddImport visitor', () => { expect(bound).toEqual(['merge_1', 'merge_2', 'merge_3']); }); + test('a request that named nothing takes the name the file already gives the module', async () => { + const spec = new RecipeSpec(); + const bound: { name?: string } = {}; + spec.recipe = fromVisitor(captureBoundName({module: 'lodash', onlyIfReferenced: false}, bound)); + + await spec.rewriteRun( + typescript( + ` + import Foo from 'lodash'; + + Foo(); + ` + ) + ); + + expect(bound.name).toBe('Foo'); + }); + test('a side-effect import binds no name', async () => { const spec = new RecipeSpec(); const bound: { name?: string } = {name: 'unset'}; From c9250aa6d0f3ae43df7ffd260e1dbde650de91f1 Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Thu, 27 Aug 2026 18:08:56 +0200 Subject: [PATCH 11/11] Count what an array pattern binds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `patternNames` read object binding patterns and stopped there, so the names an array pattern introduces were absent from the pool and an import was free to bind one of them again: // const [Element] = window; maybeAddImport(v, {module: 'sap/ui/core/Element', member: 'default', preferredName: 'Element'}) import Element from 'sap/ui/core/Element'; // Identifier 'Element' has already been declared An array pattern binds by position, so unlike an object pattern its elements name no member of whatever they destructure — they occupy their names and nothing answers for them. Nesting works in either direction, so `const [{Deep}] = window` is counted too. --- rewrite-javascript/rewrite/src/javascript/add-import.ts | 8 ++++++++ .../rewrite/test/javascript/add-import.test.ts | 8 ++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/rewrite-javascript/rewrite/src/javascript/add-import.ts b/rewrite-javascript/rewrite/src/javascript/add-import.ts index 76336a9f2d5..05d641a9362 100644 --- a/rewrite-javascript/rewrite/src/javascript/add-import.ts +++ b/rewrite-javascript/rewrite/src/javascript/add-import.ts @@ -366,6 +366,14 @@ 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 []; } diff --git a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts index b2f50d00431..94b74485781 100644 --- a/rewrite-javascript/rewrite/test/javascript/add-import.test.ts +++ b/rewrite-javascript/rewrite/test/javascript/add-import.test.ts @@ -2952,13 +2952,14 @@ describe('AddImport visitor', () => { expect(preferred).toEqual(['Theming', 'Theming']); }); - test('a name bound through a nested pattern is occupied without answering for the module', async () => { + test('a name bound through any binding pattern is occupied without answering for the module', async () => { const spec = new RecipeSpec(); const bound: string[] = []; spec.recipe = fromVisitor(new class extends JavaScriptVisitor { override async visitJsCompilationUnit(cu: JS.CompilationUnit, p: any): Promise { bound.push(maybeAddImport(this, {module: 'm', member: 'b', onlyIfReferenced: false})); bound.push(maybeAddImport(this, {module: 'other', preferredName: 'z', onlyIfReferenced: false})); + bound.push(maybeAddImport(this, {module: 'p', member: 'default', preferredName: 'e', onlyIfReferenced: false})); return super.visitJsCompilationUnit(cu, p); } }); @@ -2967,21 +2968,24 @@ describe('AddImport visitor', () => { typescript( ` const {a: {b}} = require('other'); + const [e] = window; b(); `, ` import {b as b_1} from 'm'; import z from 'other'; + import e_1 from 'p'; const {a: {b}} = require('other'); + const [e] = window; b(); ` ) ); - expect(bound).toEqual(['b_1', 'z']); + expect(bound).toEqual(['b_1', 'z', 'e_1']); }); test('a merged specifier sorts by the name it binds, as its neighbours do', async () => {