-
Notifications
You must be signed in to change notification settings - Fork 964
Fix incorrect renaming of arguments in non-reference positions in async downlevel
#3910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Copilot
wants to merge
4
commits into
main
Choose a base branch
from
copilot/fix-tsgo-arguments-rename
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
66666f2
Initial plan
Copilot 64f45fd
fix: prevent arguments rename in destructuring property name position
Copilot 0f09058
Also handle JsxAttribute name in arguments substitution guard
Copilot 08ed2ea
Refactor: use ast.IsIdentifierName instead of custom isNameOfProperty…
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,9 +30,6 @@ type asyncTransformer struct { | |
| enclosingFunctionParameterNames *collections.Set[string] | ||
| lexicalArguments lexicalArgumentsInfo | ||
|
|
||
| parentNode *ast.Node | ||
| currentNode *ast.Node | ||
|
|
||
| asyncBodyVisitor *ast.NodeVisitor | ||
| fallbackNodeVisitor *ast.NodeVisitor | ||
| } | ||
|
|
@@ -111,31 +108,22 @@ func (tx *asyncTransformer) fallbackVisitor(node *ast.Node) *ast.Node { | |
| ast.KindVariableDeclaration: | ||
| // fall through to visitEachChild | ||
| case ast.KindIdentifier: | ||
| if tx.lexicalArguments.binding != nil && node.Text() == "arguments" && !isNameOfPropertyAccessOrAssignment(tx.parentNode, node) { | ||
| if tx.lexicalArguments.binding != nil && | ||
| node.Text() == "arguments" && | ||
| !ast.IsIdentifierName(node) && | ||
| !ast.IsLabelName(node) { | ||
| tx.lexicalArguments.used = true | ||
| return tx.lexicalArguments.binding | ||
| } | ||
| } | ||
| return tx.fallbackNodeVisitor.VisitEachChild(node) | ||
| } | ||
|
|
||
| func (tx *asyncTransformer) descendInto(node *ast.Node) func() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this removed?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the latest commit, this infra is entirely unreferenced |
||
| savedParent := tx.parentNode | ||
| tx.parentNode = tx.currentNode | ||
| tx.currentNode = node | ||
| return func() { tx.currentNode = tx.parentNode; tx.parentNode = savedParent } | ||
| } | ||
|
|
||
| func (tx *asyncTransformer) visitFallback(node *ast.Node) *ast.Node { | ||
| cleanup := tx.descendInto(node) | ||
| defer cleanup() | ||
| return tx.fallbackVisitor(node) | ||
| } | ||
|
|
||
| func (tx *asyncTransformer) visit(node *ast.Node) *ast.Node { | ||
| cleanup := tx.descendInto(node) | ||
| defer cleanup() | ||
|
|
||
| if node.SubtreeFacts()&(ast.SubtreeContainsAnyAwait|ast.SubtreeContainsAwait) == 0 { | ||
| return tx.fallbackVisitor(node) | ||
| } | ||
|
|
@@ -965,12 +953,6 @@ func (tx *asyncTransformer) getOriginalIfFunctionLike(node *ast.Node) *ast.Node | |
| return node | ||
| } | ||
|
|
||
| func isNameOfPropertyAccessOrAssignment(parent *ast.Node, node *ast.Node) bool { | ||
| return parent != nil && | ||
| (ast.IsPropertyAccessExpression(parent) || ast.IsPropertyAssignment(parent)) && | ||
| parent.Name() == node | ||
| } | ||
|
|
||
| // isSimpleParameterList checks if every parameter has no initializer and an Identifier name. | ||
| func isSimpleParameterList(params []*ast.Node) bool { | ||
| for _, param := range params { | ||
|
|
||
26 changes: 26 additions & 0 deletions
26
testdata/baselines/reference/compiler/asyncDestructuringArgumentsProperty.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| //// [tests/cases/compiler/asyncDestructuringArgumentsProperty.ts] //// | ||
|
|
||
| //// [asyncDestructuringArgumentsProperty.ts] | ||
| async function f() { | ||
| const { arguments: args } = await { arguments: 42 }; | ||
| return args; | ||
| } | ||
|
|
||
|
|
||
| //// [asyncDestructuringArgumentsProperty.js] | ||
| "use strict"; | ||
| var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) { | ||
| function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); } | ||
| return new (P || (P = Promise))(function (resolve, reject) { | ||
| function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } } | ||
| function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } } | ||
| function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); } | ||
| step((generator = generator.apply(thisArg, _arguments || [])).next()); | ||
| }); | ||
| }; | ||
| function f() { | ||
| return __awaiter(this, void 0, void 0, function* () { | ||
| const { arguments: args } = yield { arguments: 42 }; | ||
| return args; | ||
| }); | ||
| } |
15 changes: 15 additions & 0 deletions
15
testdata/baselines/reference/compiler/asyncDestructuringArgumentsProperty.symbols
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| //// [tests/cases/compiler/asyncDestructuringArgumentsProperty.ts] //// | ||
|
|
||
| === asyncDestructuringArgumentsProperty.ts === | ||
| async function f() { | ||
| >f : Symbol(f, Decl(asyncDestructuringArgumentsProperty.ts, 0, 0)) | ||
|
|
||
| const { arguments: args } = await { arguments: 42 }; | ||
| >arguments : Symbol(arguments, Decl(asyncDestructuringArgumentsProperty.ts, 1, 37)) | ||
| >args : Symbol(args, Decl(asyncDestructuringArgumentsProperty.ts, 1, 9)) | ||
| >arguments : Symbol(arguments, Decl(asyncDestructuringArgumentsProperty.ts, 1, 37)) | ||
|
|
||
| return args; | ||
| >args : Symbol(args, Decl(asyncDestructuringArgumentsProperty.ts, 1, 9)) | ||
| } | ||
|
|
18 changes: 18 additions & 0 deletions
18
testdata/baselines/reference/compiler/asyncDestructuringArgumentsProperty.types
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| //// [tests/cases/compiler/asyncDestructuringArgumentsProperty.ts] //// | ||
|
|
||
| === asyncDestructuringArgumentsProperty.ts === | ||
| async function f() { | ||
| >f : () => Promise<number> | ||
|
|
||
| const { arguments: args } = await { arguments: 42 }; | ||
| >arguments : any | ||
| >args : number | ||
| >await { arguments: 42 } : { arguments: number; } | ||
| >{ arguments: 42 } : { arguments: number; } | ||
| >arguments : number | ||
| >42 : 42 | ||
|
|
||
| return args; | ||
| >args : number | ||
| } | ||
|
|
28 changes: 28 additions & 0 deletions
28
testdata/baselines/reference/compiler/asyncJsxArgumentsAttributeName.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| //// [tests/cases/compiler/asyncJsxArgumentsAttributeName.ts] //// | ||
|
|
||
| //// [test.tsx] | ||
| declare namespace JSX { | ||
| interface IntrinsicElements { div: any; } | ||
| } | ||
|
|
||
| async function f() { | ||
| return <div arguments={42} />; | ||
| } | ||
|
|
||
|
|
||
| //// [test.jsx] | ||
| "use strict"; | ||
| var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, generator) { | ||
| function adopt(value) { return value instanceof P ? value : new P(function (resolve) { resolve(value); }); } | ||
| return new (P || (P = Promise))(function (resolve, reject) { | ||
| function fulfilled(value) { try { step(generator.next(value)); } catch (e) { reject(e); } } | ||
| function rejected(value) { try { step(generator["throw"](value)); } catch (e) { reject(e); } } | ||
| function step(result) { result.done ? resolve(result.value) : adopt(result.value).then(fulfilled, rejected); } | ||
| step((generator = generator.apply(thisArg, _arguments || [])).next()); | ||
| }); | ||
| }; | ||
| function f() { | ||
| return __awaiter(this, void 0, void 0, function* () { | ||
| return <div arguments={42}/>; | ||
| }); | ||
| } |
19 changes: 19 additions & 0 deletions
19
testdata/baselines/reference/compiler/asyncJsxArgumentsAttributeName.symbols
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| //// [tests/cases/compiler/asyncJsxArgumentsAttributeName.ts] //// | ||
|
|
||
| === test.tsx === | ||
| declare namespace JSX { | ||
| >JSX : Symbol(JSX, Decl(test.tsx, 0, 0)) | ||
|
|
||
| interface IntrinsicElements { div: any; } | ||
| >IntrinsicElements : Symbol(IntrinsicElements, Decl(test.tsx, 0, 23)) | ||
| >div : Symbol(IntrinsicElements.div, Decl(test.tsx, 1, 31)) | ||
| } | ||
|
|
||
| async function f() { | ||
| >f : Symbol(f, Decl(test.tsx, 2, 1)) | ||
|
|
||
| return <div arguments={42} />; | ||
| >div : Symbol(JSX.IntrinsicElements.div, Decl(test.tsx, 1, 31)) | ||
| >arguments : Symbol(arguments, Decl(test.tsx, 5, 13)) | ||
| } | ||
|
|
18 changes: 18 additions & 0 deletions
18
testdata/baselines/reference/compiler/asyncJsxArgumentsAttributeName.types
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| //// [tests/cases/compiler/asyncJsxArgumentsAttributeName.ts] //// | ||
|
|
||
| === test.tsx === | ||
| declare namespace JSX { | ||
| interface IntrinsicElements { div: any; } | ||
| >div : any | ||
| } | ||
|
|
||
| async function f() { | ||
| >f : () => Promise<any> | ||
|
|
||
| return <div arguments={42} />; | ||
| ><div arguments={42} /> : error | ||
| >div : any | ||
| >arguments : number | ||
| >42 : 42 | ||
| } | ||
|
|
6 changes: 6 additions & 0 deletions
6
testdata/tests/cases/compiler/asyncDestructuringArgumentsProperty.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| // @target: es2015 | ||
|
|
||
| async function f() { | ||
| const { arguments: args } = await { arguments: 42 }; | ||
| return args; | ||
| } |
11 changes: 11 additions & 0 deletions
11
testdata/tests/cases/compiler/asyncJsxArgumentsAttributeName.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // @target: es2015 | ||
| // @jsx: preserve | ||
|
|
||
| // @filename: test.tsx | ||
| declare namespace JSX { | ||
| interface IntrinsicElements { div: any; } | ||
| } | ||
|
|
||
| async function f() { | ||
| return <div arguments={42} />; | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared with the old TS approach. The old code in
checker.ts:50594-50605:The old code has a narrow syntactic guard (only PropertyAccess + PropertyAssignment) but is saved by
getReferencedValueSymbol(node) === argumentsSymbolwhich 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:
{ arguments }): Not covered byIsIdentifierName, so we substitute. This is correct—it's a value reference. Same behavior as old code (the old comment even acknowledges this gap).const arguments): Not covered byIsIdentifierName, but unreachable—async functions are strict mode, whereargumentscan't be a binding name.IsIdentifierNamereturns true, but these are module-top-level only, unreachable inside async function bodies.I believe we're covering all reachable cases correctly.