-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Fixes var declaration shadowing in async functions #21215
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
Conversation
@@ -83,6 +85,103 @@ namespace ts { | |||
} | |||
} | |||
|
|||
function asyncBodyVisitor(node: Node): VisitResult<Node> { |
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.
Is there a canonical source we can reference in a comment here for the list-of-things-which-may-contain-or-whose-children-may-contain-hoisted-declarations? It seems nonobvious which elements of the statement and declaration list need to be explicitly visited here.
Also: Does LabeledStatement
need to be handled? eg
async function fn4(x) {
lbl: {
var x = y;
}
}
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.
Good catch
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.
I've added isNodeWithPossibleVarDeclaration
in 2ba29d8 as a way to verify this list.
|
||
function visitCatchClauseInAsyncBody(node: CatchClause) { | ||
const catchClauseNames = createUnderscoreEscapedMap<true>(); | ||
recordDeclarationName(node.variableDeclaration, catchClauseNames); |
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.
variableDeclaration may be undefined
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.
Actually, by the time we get to the es2017
transform a CatchClause
must have a catch variable, otherwise it's not valid ES2017. Assigning the catch variable is handled in the esnext
transform before the es2017
transform is run.
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.
I've added a test case in e655446 to verify this case.
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.
Small suggestion to improve the flow of the one function, but seems good.
src/compiler/transformers/es2017.ts
Outdated
case SyntaxKind.LabeledStatement: | ||
return visitEachChild(node, asyncBodyVisitor, context); | ||
} | ||
Debug.assert(!isNodeWithPossibleVarDeclaration(node), "Unhandled node."); |
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.
Why not make isNodeWithPossibleVarDeclaration
into a type guard, start the function with it, then end with an assertNever
to get the type checker to help out here?:
if(!isNodeWithPossibleVarDeclaration(node)) {
return visitor(node);
}
// ... switch block
Debug.assertNever(node, "Unhandled node.");
This fixes an issue where the downlevel emit for async functions to ES2015 can result in incorrect runtime semantics when the async function body contains a
var
declaration that shadows a parameter name.Fixes #20461