Skip to content

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 17, 2018
Merged

Fixes var declaration shadowing in async functions #21215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
136c4d0
Fixes var declaration shadowing in async functions
rbuckton Jan 16, 2018
c4fddba
Remove duplicate implementations
rbuckton Jan 17, 2018
2ba29d8
Support labeled statement
rbuckton Jan 17, 2018
e655446
Add test for catch block without variable
rbuckton Jan 17, 2018
afaa139
Namespaces do not have the same 'var' scope
rbuckton Jan 17, 2018
5b45db7
PR Feedback
rbuckton Jan 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1341,7 +1341,8 @@ namespace ts {

export function cloneMap(map: SymbolTable): SymbolTable;
export function cloneMap<T>(map: ReadonlyMap<T>): Map<T>;
export function cloneMap<T>(map: ReadonlyMap<T> | SymbolTable): Map<T> | SymbolTable {
export function cloneMap<T>(map: ReadonlyUnderscoreEscapedMap<T>): UnderscoreEscapedMap<T>;
export function cloneMap<T>(map: ReadonlyMap<T> | ReadonlyUnderscoreEscapedMap<T> | SymbolTable): Map<T> | UnderscoreEscapedMap<T> | SymbolTable {
const clone = createMap<T>();
copyEntries(map as Map<T>, clone);
return clone;
Expand Down
216 changes: 202 additions & 14 deletions src/compiler/transformers/es2017.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ namespace ts {

export function transformES2017(context: TransformationContext) {
const {
startLexicalEnvironment,
resumeLexicalEnvironment,
endLexicalEnvironment
endLexicalEnvironment,
hoistVariableDeclaration
} = context;

const resolver = context.getEmitResolver();
Expand All @@ -33,6 +33,8 @@ namespace ts {
*/
let enclosingSuperContainerFlags: NodeCheckFlags = 0;

let enclosingFunctionParameterNames: UnderscoreEscapedMap<true>;

// Save the previous transformation hooks.
const previousOnEmitNode = context.onEmitNode;
const previousOnSubstituteNode = context.onSubstituteNode;
Expand Down Expand Up @@ -83,6 +85,108 @@ namespace ts {
}
}

function asyncBodyVisitor(node: Node): VisitResult<Node> {
Copy link
Member

@weswigham weswigham Jan 17, 2018 •
edited
Loading

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
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
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.

if (isNodeWithPossibleHoistedDeclaration(node)) {
switch (node.kind) {
case SyntaxKind.VariableStatement:
return visitVariableStatementInAsyncBody(node);
case SyntaxKind.ForStatement:
return visitForStatementInAsyncBody(node);
case SyntaxKind.ForInStatement:
return visitForInStatementInAsyncBody(node);
case SyntaxKind.ForOfStatement:
return visitForOfStatementInAsyncBody(node);
case SyntaxKind.CatchClause:
return visitCatchClauseInAsyncBody(node);
case SyntaxKind.Block:
case SyntaxKind.SwitchStatement:
case SyntaxKind.CaseBlock:
case SyntaxKind.CaseClause:
case SyntaxKind.DefaultClause:
case SyntaxKind.TryStatement:
case SyntaxKind.DoStatement:
case SyntaxKind.WhileStatement:
case SyntaxKind.IfStatement:
case SyntaxKind.WithStatement:
case SyntaxKind.LabeledStatement:
return visitEachChild(node, asyncBodyVisitor, context);
default:
return Debug.assertNever(node, "Unhandled node.");
}
}
return visitor(node);
}

function visitCatchClauseInAsyncBody(node: CatchClause) {
const catchClauseNames = createUnderscoreEscapedMap<true>();
recordDeclarationName(node.variableDeclaration, catchClauseNames);
Copy link
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
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
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.


// names declared in a catch variable are block scoped
let catchClauseUnshadowedNames: UnderscoreEscapedMap<true>;
catchClauseNames.forEach((_, escapedName) => {
if (enclosingFunctionParameterNames.has(escapedName)) {
if (!catchClauseUnshadowedNames) {
catchClauseUnshadowedNames = cloneMap(enclosingFunctionParameterNames);
}
catchClauseUnshadowedNames.delete(escapedName);
}
});

if (catchClauseUnshadowedNames) {
const savedEnclosingFunctionParameterNames = enclosingFunctionParameterNames;
enclosingFunctionParameterNames = catchClauseUnshadowedNames;
const result = visitEachChild(node, asyncBodyVisitor, context);
enclosingFunctionParameterNames = savedEnclosingFunctionParameterNames;
return result;
}
else {
return visitEachChild(node, asyncBodyVisitor, context);
}
}

function visitVariableStatementInAsyncBody(node: VariableStatement) {
if (isVariableDeclarationListWithCollidingName(node.declarationList)) {
const expression = visitVariableDeclarationListWithCollidingNames(node.declarationList, /*hasReceiver*/ false);
return expression ? createStatement(expression) : undefined;
}
return visitEachChild(node, visitor, context);
}

function visitForInStatementInAsyncBody(node: ForInStatement) {
return updateForIn(
node,
isVariableDeclarationListWithCollidingName(node.initializer)
? visitVariableDeclarationListWithCollidingNames(node.initializer, /*hasReceiver*/ true)
: visitNode(node.initializer, visitor, isForInitializer),
visitNode(node.expression, visitor, isExpression),
visitNode(node.statement, asyncBodyVisitor, isStatement, liftToBlock)
);
}

function visitForOfStatementInAsyncBody(node: ForOfStatement) {
return updateForOf(
node,
visitNode(node.awaitModifier, visitor, isToken),
isVariableDeclarationListWithCollidingName(node.initializer)
? visitVariableDeclarationListWithCollidingNames(node.initializer, /*hasReceiver*/ true)
: visitNode(node.initializer, visitor, isForInitializer),
visitNode(node.expression, visitor, isExpression),
visitNode(node.statement, asyncBodyVisitor, isStatement, liftToBlock)
);
}

function visitForStatementInAsyncBody(node: ForStatement) {
return updateFor(
node,
isVariableDeclarationListWithCollidingName(node.initializer)
? visitVariableDeclarationListWithCollidingNames(node.initializer, /*hasReceiver*/ false)
: visitNode(node.initializer, visitor, isForInitializer),
visitNode(node.condition, visitor, isExpression),
visitNode(node.incrementor, visitor, isExpression),
visitNode((<ForStatement>node).statement, asyncBodyVisitor, isStatement, liftToBlock)
);
}

/**
* Visits an AwaitExpression node.
*
Expand Down Expand Up @@ -197,6 +301,82 @@ namespace ts {
);
}

function recordDeclarationName({ name }: ParameterDeclaration | VariableDeclaration | BindingElement, names: UnderscoreEscapedMap<true>) {
if (isIdentifier(name)) {
names.set(name.escapedText, true);
}
else {
for (const element of name.elements) {
if (!isOmittedExpression(element)) {
recordDeclarationName(element, names);
}
}
}
}

function isVariableDeclarationListWithCollidingName(node: ForInitializer): node is VariableDeclarationList {
return node
&& isVariableDeclarationList(node)
&& !(node.flags & NodeFlags.BlockScoped)
&& forEach(node.declarations, collidesWithParameterName);
}

function visitVariableDeclarationListWithCollidingNames(node: VariableDeclarationList, hasReceiver: boolean) {
hoistVariableDeclarationList(node);

const variables = getInitializedVariables(node);
if (variables.length === 0) {
if (hasReceiver) {
return visitNode(convertToAssignmentElementTarget(node.declarations[0].name), visitor, isExpression);
}
return undefined;
}

return inlineExpressions(map(variables, transformInitializedVariable));
}

function hoistVariableDeclarationList(node: VariableDeclarationList) {
forEach(node.declarations, hoistVariable);
}

function hoistVariable({ name }: VariableDeclaration | BindingElement) {
if (isIdentifier(name)) {
hoistVariableDeclaration(name);
}
else {
for (const element of name.elements) {
if (!isOmittedExpression(element)) {
hoistVariable(element);
}
}
}
}

function transformInitializedVariable(node: VariableDeclaration) {
const converted = setSourceMapRange(
createAssignment(
convertToAssignmentElementTarget(node.name),
node.initializer
),
node
);
return visitNode(converted, visitor, isExpression);
}

function collidesWithParameterName({ name }: VariableDeclaration | BindingElement): boolean {
if (isIdentifier(name)) {
return enclosingFunctionParameterNames.has(name.escapedText);
}
else {
for (const element of name.elements) {
if (!isOmittedExpression(element) && collidesWithParameterName(element)) {
return true;
}
}
}
return false;
}

function transformAsyncFunctionBody(node: MethodDeclaration | AccessorDeclaration | FunctionDeclaration | FunctionExpression): FunctionBody;
function transformAsyncFunctionBody(node: ArrowFunction): ConciseBody;
function transformAsyncFunctionBody(node: FunctionLikeDeclaration): ConciseBody {
Expand All @@ -214,6 +394,13 @@ namespace ts {
// passed to `__awaiter` is executed inside of the callback to the
// promise constructor.

const savedEnclosingFunctionParameterNames = enclosingFunctionParameterNames;
enclosingFunctionParameterNames = createUnderscoreEscapedMap<true>();
for (const parameter of node.parameters) {
recordDeclarationName(parameter, enclosingFunctionParameterNames);
}

let result: ConciseBody;
if (!isArrowFunction) {
const statements: Statement[] = [];
const statementOffset = addPrologue(statements, (<Block>node.body).statements, /*ensureUseStrict*/ false, visitor);
Expand All @@ -223,7 +410,7 @@ namespace ts {
context,
hasLexicalArguments,
promiseConstructor,
transformFunctionBodyWorker(<Block>node.body, statementOffset)
transformAsyncFunctionBodyWorker(<Block>node.body, statementOffset)
)
)
);
Expand All @@ -246,35 +433,36 @@ namespace ts {
}
}

return block;
result = block;
}
else {
const expression = createAwaiterHelper(
context,
hasLexicalArguments,
promiseConstructor,
transformFunctionBodyWorker(node.body)
transformAsyncFunctionBodyWorker(node.body)
);

const declarations = endLexicalEnvironment();
if (some(declarations)) {
const block = convertToFunctionBody(expression);
return updateBlock(block, setTextRange(createNodeArray(concatenate(block.statements, declarations)), block.statements));
result = updateBlock(block, setTextRange(createNodeArray(concatenate(block.statements, declarations)), block.statements));
}
else {
result = expression;
}

return expression;
}

enclosingFunctionParameterNames = savedEnclosingFunctionParameterNames;
return result;
}

function transformFunctionBodyWorker(body: ConciseBody, start?: number) {
function transformAsyncFunctionBodyWorker(body: ConciseBody, start?: number) {
if (isBlock(body)) {
return updateBlock(body, visitLexicalEnvironment(body.statements, visitor, context, start));
return updateBlock(body, visitNodes(body.statements, asyncBodyVisitor, isStatement, start));
}
else {
startLexicalEnvironment();
const visited = convertToFunctionBody(visitNode(body, visitor, isConciseBody));
const declarations = endLexicalEnvironment();
return updateBlock(visited, setTextRange(createNodeArray(concatenate(visited.statements, declarations)), visited.statements));
return convertToFunctionBody(visitNode(body, asyncBodyVisitor, isConciseBody));
}
}

Expand Down
45 changes: 45 additions & 0 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1761,6 +1761,51 @@ namespace ts {
return getAssignmentTargetKind(node) !== AssignmentKind.None;
}

export type NodeWithPossibleHoistedDeclaration =
| Block
| VariableStatement
| WithStatement
| IfStatement
| SwitchStatement
| CaseBlock
| CaseClause
| DefaultClause
| LabeledStatement
| ForStatement
| ForInStatement
| ForOfStatement
| DoStatement
| WhileStatement
| TryStatement
| CatchClause;

/**
* Indicates whether a node could contain a `var` VariableDeclarationList that contributes to
* the same `var` declaration scope as the node's parent.
*/
export function isNodeWithPossibleHoistedDeclaration(node: Node): node is NodeWithPossibleHoistedDeclaration {
switch (node.kind) {
case SyntaxKind.Block:
case SyntaxKind.VariableStatement:
case SyntaxKind.WithStatement:
case SyntaxKind.IfStatement:
case SyntaxKind.SwitchStatement:
case SyntaxKind.CaseBlock:
case SyntaxKind.CaseClause:
case SyntaxKind.DefaultClause:
case SyntaxKind.LabeledStatement:
case SyntaxKind.ForStatement:
case SyntaxKind.ForInStatement:
case SyntaxKind.ForOfStatement:
case SyntaxKind.DoStatement:
case SyntaxKind.WhileStatement:
case SyntaxKind.TryStatement:
case SyntaxKind.CatchClause:
return true;
}
return false;
}

function walkUp(node: Node, kind: SyntaxKind) {
while (node && node.kind === kind) {
node = node.parent;
Expand Down
Loading