Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fixes var declaration shadowing in async functions#21215

Merged
rbuckton merged 6 commits into
mastermicrosoft/TypeScript:masterfrom
fix20461microsoft/TypeScript:fix20461Copy head branch name to clipboard
Jan 17, 2018
Merged

Fixes var declaration shadowing in async functions#21215
rbuckton merged 6 commits into
mastermicrosoft/TypeScript:masterfrom
fix20461microsoft/TypeScript:fix20461Copy head branch name to clipboard

Conversation

@rbuckton

@rbuckton rbuckton commented Jan 16, 2018

Copy link
Copy Markdown
Contributor

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

}
}

function asyncBodyVisitor(node: Node): VisitResult<Node> {

@weswigham weswigham Jan 17, 2018

Copy link
Copy Markdown
Member

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;
  }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

Copy link
Copy Markdown
Contributor Author

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variableDeclaration may be undefined

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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.

@weswigham weswigham left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/compiler/transformers/es2017.ts Outdated
case SyntaxKind.LabeledStatement:
return visitEachChild(node, asyncBodyVisitor, context);
}
Debug.assert(!isNodeWithPossibleVarDeclaration(node), "Unhandled node.");

@weswigham weswigham Jan 17, 2018

Copy link
Copy Markdown
Member

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.");

@rbuckton rbuckton merged commit 3c988e8 into master Jan 17, 2018
@rbuckton rbuckton deleted the fix20461 branch January 17, 2018 21:08
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Async function __awaiter allows var declarations to clobber function parameters

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.