maybeAddImport returns the name it bound, and templates resolve the modules they name

[knutwannheden]

maybeAddImport returned void. It 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, and there was no shadowing check anywhere in add-import.ts. A recipe that wanted to reference an import it was adding had to guess the name, and guessed wrong whenever the file already used it:

maybeAddImport(this, {module: 'fs', member: 'readFile', onlyIfReferenced: false});

// before                      // after
const readFile = 1;            import {readFile} from 'fs';
                               const readFile = 1;

That output is a SyntaxError — readFile is bound twice.

What it returns

maybeAddImport now reaches the compilation unit from visitor.cursor and returns the local name the module is bound to. Imports, and everything that can shadow one, are top-level statements, so this stays synchronous: existence, the deconfliction pool and the queue reservations are all flat scans of cu.statements.

override async visitJsCompilationUnit(cu: JS.CompilationUnit, p: any): Promise<J | undefined> {
    const readFile = maybeAddImport(this, {module: 'fs', member: 'readFile', onlyIfReferenced: false});
    // readFile === 'readFile_1', and the emitted import binds that name
    return super.visitJsCompilationUnit(cu, p);
}

// before                      // after
const readFile = 1;            import {readFile as readFile_1} from 'fs';
                               const readFile = 1;

Resolution runs in three steps. An import already binding this module and member answers with its own name and queues nothing. An alias the caller pinned is honoured verbatim. Otherwise the derived member ?? module is deconflicted with a _1 suffix — a bare digit reads wrong on a name that already ends in one, so base64 deconflicts to base64_1.

The first step is conditional, and the condition is what the caller asked for. A preferredName or an alias says it will take whatever name comes back; a bare {module, member} says it is assuming the name is member. So a binding under some other name answers only the first kind:

maybeAddImport(this, {module: 'react', member: 'useState', onlyIfReferenced: false});

// before                                  // after
import {useState as useS} from 'react';    import {useState as useS, useState} from 'react';

Answering useS there would leave every useState(...) the caller emits unbound. rewrite-react, rewrite-nodejs, javascript-recipe-starter and recipes-testing-frameworks all call it this way and all ignore the return value, so the import it emits is the whole contract for them.

The pool is module-scope declarations — including namespaces and type aliases, which shadow an import just as a const does — existing import and require() bindings, and names claimed by AddImports already on the queue. A queued RemoveImport deliberately frees nothing: it removes only what the file leaves unused, so binding a name it keeps is an error, where an unnecessary suffix merely reads oddly.

Pinning is the opt-out, and ChangeImport uses it. It moves a binding rather than introducing one, and the import it is replacing is still present in the tree maybeAddImport reads:

maybeAddImport(this, {module: newModule, member: newMember, alias: aliasToUse ?? newMember, onlyIfReferenced: false});

An alias equal to the member prints as a plain specifier, so pinning a name does not disfigure the output. Its two named-member branches collapse into one as a result.

AddImport carries the resolved name as bindingName, which is both what it emits and what onlyIfReferenced searches for — 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. The third is load-bearing: call sites passing an AddImportOptions-typed variable rather than a literal match neither of the first two.

export function maybeAddImport(v: JavaScriptVisitor<any>, o: AddImportOptions & {sideEffectOnly: true}): undefined;
export function maybeAddImport(v: JavaScriptVisitor<any>, o: AddImportOptions & {sideEffectOnly?: false}): string;
export function maybeAddImport(v: JavaScriptVisitor<any>, o: AddImportOptions): string | undefined;

Templates declare the modules they need

A returned name only helps if the caller respects it, and a caller writing a template has to thread it into generated code by hand. A template now declares its modules, keyed by the identifier its own source uses:

const tmpl = template`Theming.setTheme(${theme})`.configure({
    bindings: {Theming: {module: 'sap/ui/core/Theming', member: 'default'}}
});
const rule = rewrite(() => ({before: pattern`applyTheme(${theme})`, after: tmpl}));

override async visitMethodInvocation(m: J.MethodInvocation, p: any): Promise<J | undefined> {
    m = await super.visitMethodInvocation(m, p) as J.MethodInvocation;
    return await rule.tryOn(this.cursor, m, {visitor: this}) || m;
}

// before                      // after
const Theming = 1;             import Theming_1 from 'sap/ui/core/Theming';
applyTheme('dark');            const Theming = 1;
                               Theming_1.setTheme('dark');

The file decides the name; the template follows it. Where the file already imports the module, the existing name wins and no import is added:

// before                                        // after
import Th from 'sap/ui/core/Theming';            import Th from 'sap/ui/core/Theming';
applyTheme('dark');                              Th.setTheme('dark');

The rule holds the template, so it resolves the declared modules itself — and does so only once a pattern has matched, which is why a rule that never fires leaves the file's imports alone. Template.apply still takes resolved names rather than a visitor and stays pure: 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 caller resolving some other way — keyed by module rather than by template name, say — passes bindings instead, and a declared key with no entry throws rather than emitting a name nothing bound. The declaration also decides what the template's own source is parsed against, which never reaches the output because getTemplateTree takes 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 never rewritten by it. Where dependencies covers the module, the binding is parsed against an import, which attributes its identifier — a Variable whose owner is the module — and the rename keys on that. Where nothing could resolve the module, it is parsed against declare const X: any; and position decides instead: an identifier in its parent's name slot is being named rather than referencing the binding.

template`Theming.setTheme(${props}.Theming)`   // →  Theming_1.setTheme(props.Theming)

Declaring a binding does not type the rest of the template's code: setTheme above is unresolved either way, and a template that needs it typed adds dependencies alongside, as it would for a hand-written context entry.

That split is a cost decision. An import costs a module resolution — 49 ms per template without bindings against 112 ms with, over 12 distinct templates on a warm parser — and where no dependencies entry covers the module it is paid for nothing. The declaration brings it back to 31 ms. It is not free either: recognising the reference a template splices in is itself an attribution question, so resolveBindings binds an unresolvable module whether or not the template goes on to reference it. It is sound because tryOn resolves only once its pattern has matched.

Template.resolveBindings(visitor) is public for those callers, and is 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.

TemplateOptions.bindings names modules and members only, and resolution is a separate call the caller makes, so nothing in the rename pass is ESM-specific — an AMD binding scheme substitutes its own resolver and hands apply the same shape of map.

One bug shape, four times

Returning a name turned every identity comparison in add-import.ts into a correctness surface. Each of these was harmless while the function returned void, and each produced a returned name that no import bound once it did not:

where	compared	with
queue dedup	quoteStyle	a field that decides how the specifier prints
queue dedup	preferredName	a field that decides what name would be nice
isMatchingImport	aliasName === this.alias	raw alias, after emission moved to bindingName
isMatchingRequire	name === (this.alias || this.member)	same, in the CommonJS path

The first two split one request in two. Both calls survive dedup, the second derives a fresh name, and only one import lands:

maybeAddImport(this, {module: 'm', member: 'default', preferredName: 'Theming'});  // 'Theming'
maybeAddImport(this, {module: 'm', member: 'default', preferredName: 'Th'});       // 'Th'  ← nothing binds this
// emitted: import Theming from 'm';

Reachable as soon as two templates in one recipe declare the same module under different keys. Neither field says which binding is wanted, so both are out of the comparison; alias stays, because two aliases for one member are two legal bindings.

The last two are the mirror image — matching left behind on a field emission had moved off. Pinning an alias equal to the member meets a plain specifier whose aliasName is undefined:

maybeAddImport(this, {module: 'fs', member: 'readFile', alias: 'readFile', onlyIfReferenced: false});

// before                            // after
import {readFile} from 'fs';         import {readFile, readFile} from 'fs';
readFile('x');                       readFile('x');

Both now compare the bound name: (aliasName ?? importName) === this.bindingName.

Tests

Fourteen under describe('bound name') in add-import.test.ts, seven in template-bindings.test.ts, each pinning a distinct line, one per bug above. The templating six were mutation-checked: breaking namesItsParent kills exactly the naming-position test, and making the rename a no-op kills the other four.

Three of the four bugs came out of review rather than from these tests, because each needed a caller shape the suite had never used. That is why the diff is larger than "add a return type".

Scope

The pool is read from the tree as it was when the visit began, so a binding the enclosing visitor removed inline in the same pass still looks occupied. That is not closeable at call time — ChangeImport opts out by pinning, and a caller that removes through maybeRemoveImport puts the freed name on the queue where it is visible. A module specifier that is not a valid identifier still has nothing to derive a default name from, so maybeAddImport({module: '@scope/pkg'}) emits an unusable import — unchanged here, and closed by passing alias or preferredName.