Fix incorrect renaming of arguments in non-reference positions in async downlevel

[Copilot]

When targeting ES2015, the async transformer incorrectly substitutes arguments identifiers in non-reference positions with the lexical capture binding:

async function f() {
  const { arguments: args } = await { arguments: 42 };
  return args;
}

Emitted { arguments_1: args } instead of { arguments: args }, causing the destructured value to be undefined.

Similarly, with jsx: preserve, a JSX attribute named arguments (e.g., <div arguments={42} />) would be incorrectly substituted since JSX nodes survive to the async transformer when the JSX transform is skipped.

Root cause: The custom isNameOfPropertyAccessOrAssignment helper only guarded against substituting arguments in PropertyAccessExpression and PropertyAssignment name positions, missing other non-reference identifier positions like BindingElement.PropertyName, JsxAttribute names, and labels.

Fix: Replace the custom helper with the existing ast.IsIdentifierName(node) utility (which covers all non-reference identifier positions: property access, property assignment, binding element property name, JSX attributes, qualified names, import/export specifiers, etc.) plus ast.IsLabelName(node) for label positions. This also removes the now-unnecessary parentNode/currentNode tracking and descendInto method, since IsIdentifierName uses the node's original parse-tree parent directly.

[DanielRosenwasser]

This is pretty similar to IsIdentifierName so I checked what else we're missing.

Can we end up with a JSX attribute named arguments at this point if we have JSX preserved?

[jakebailey]

@copilot investigate

[Copilot]

Yes — with jsx: preserve, JSX nodes survive to the async transformer since the JSX transform is skipped. A <div arguments={42} /> would have the attribute name visited through fallbackVisitor and incorrectly substituted. Added the JsxAttribute check in 0f09058.

[jakebailey]

I think this code should probably be flipped, so that the caller can do:

case ast.KindIdentifier:
    if tx.lexicalArguments.binding != nil &&
        node.Text() == "arguments" &&
        !ast.IsIdentifierName(node) /* and not a label */ {
        ...
    }

And only substitute when the identifier is in a reference position?

[jakebailey]

@copilot address

[Copilot]

Refactored in 08ed2ea — replaced the custom isNameOfPropertyAccessOrAssignment with !ast.IsIdentifierName(node) && !ast.IsLabelName(node), which covers all the non-reference identifier positions. Also removed the now-dead parentNode/currentNode/descendInto tracking since IsIdentifierName uses the node's parse-tree parent directly.

[jakebailey]

Basically the old codebase used to call resolver.isArgumentsLocalBinding(node) instead of this

[jakebailey]

@copilot Compare this to the way the old compiler worked in its transform; make sure you haven't forgotten any edge cases here, we might not have this 100% tested

[Copilot]

Compared with the old TS approach. The old code in checker.ts:50594-50605:

function isArgumentsLocalBinding(nodeIn: Identifier): boolean {
    // Note: does not handle isShorthandPropertyAssignment (and probably a few more)
    if (isGeneratedIdentifier(nodeIn)) return false;
    const node = getParseTreeNode(nodeIn, isIdentifier);
    if (!node) return false;
    const parent = node.parent;
    if (!parent) return false;
    const isPropertyName = (isPropertyAccessExpression(parent)
        || isPropertyAssignment(parent))
        && parent.name === node;
    return !isPropertyName && getReferencedValueSymbol(node) === argumentsSymbol;
}

The old code has a narrow syntactic guard (only PropertyAccess + PropertyAssignment) but is saved by getReferencedValueSymbol(node) === argumentsSymbol which does full name resolution—identifiers in non-reference positions won't resolve to the arguments symbol.

Our !ast.IsIdentifierName(node) && !ast.IsLabelName(node) is a broader syntactic guard covering PropertyAccess, PropertyAssignment, BindingElement.PropertyName, JsxAttribute, QualifiedName, ImportSpecifier.PropertyName, ExportSpecifier, JSX element tags, etc.—all positions where an identifier isn't a value reference.

Edge cases considered:

• ShorthandPropertyAssignment ({ arguments }): Not covered by IsIdentifierName, so we substitute. This is correct—it's a value reference. Same behavior as old code (the old comment even acknowledges this gap).
• Declaration names (const arguments): Not covered by IsIdentifierName, but unreachable—async functions are strict mode, where arguments can't be a binding name.
• ExportSpecifier/ImportSpecifier: IsIdentifierName returns true, but these are module-top-level only, unreachable inside async function bodies.

I believe we're covering all reachable cases correctly.

[Copilot]

Pull request overview

Fixes an async downlevel (ES2015 target) bug where arguments was incorrectly renamed to the lexical capture binding in non-reference identifier positions (e.g. destructuring property names and JSX attribute names when jsx: preserve leaves JSX nodes for the async transformer).

Changes:

• Replace the custom isNameOfPropertyAccessOrAssignment guard with ast.IsIdentifierName(node) plus ast.IsLabelName(node) when deciding whether to substitute arguments.
• Simplify traversal by removing the async transformer’s manual parentNode/currentNode tracking and descendInto helper.
• Add compiler regression tests (with baselines) covering destructuring { arguments: ... } and JSX <div arguments={...} /> cases.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
testdata/tests/cases/compiler/asyncJsxArgumentsAttributeName.ts	Adds a regression test ensuring JSX attribute name arguments is not renamed during async downlevel.
testdata/tests/cases/compiler/asyncDestructuringArgumentsProperty.ts	Adds a regression test ensuring destructuring property name arguments is preserved under async downlevel.
testdata/baselines/reference/compiler/asyncJsxArgumentsAttributeName.types	Baseline for type output of the new JSX regression test.
testdata/baselines/reference/compiler/asyncJsxArgumentsAttributeName.symbols	Baseline for symbol output of the new JSX regression test.
testdata/baselines/reference/compiler/asyncJsxArgumentsAttributeName.js	Baseline for emit output of the new JSX regression test.
testdata/baselines/reference/compiler/asyncDestructuringArgumentsProperty.types	Baseline for type output of the destructuring regression test.
testdata/baselines/reference/compiler/asyncDestructuringArgumentsProperty.symbols	Baseline for symbol output of the destructuring regression test.
testdata/baselines/reference/compiler/asyncDestructuringArgumentsProperty.js	Baseline for emit output of the destructuring regression test.
internal/transformers/estransforms/async.go	Fixes arguments substitution logic to avoid renaming in non-reference identifier positions using shared AST utilities.

[DanielRosenwasser]

Why was this removed?

[jakebailey]

With the latest commit, this infra is entirely unreferenced