Skip to content

Fix block-scoped capturing by class elements inside iteration statements #28708

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

Closed
wants to merge 2 commits into from
Closed

Fix block-scoped capturing by class elements inside iteration statements #28708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3839617
Fix property name bindings for class expr in loops
joeywatts Feb 21, 2020
6673dca
Fix block-scope capturing with prop initializers
joeywatts Feb 21, 2020
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
55 changes: 38 additions & 17 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23485,14 +23485,20 @@ namespace ts {
return assignmentKind ? getBaseTypeOfLiteralType(flowType) : flowType;
}

function isInsideFunction(node: Node, threshold: Node): boolean {
return !!findAncestor(node, n => n === threshold ? "quit" : isFunctionLike(n));
function isInsideFunctionOrInstancePropertyInitializer(node: Node, threshold: Node): boolean {
return !!findAncestor(node, n => n === threshold ? "quit" : isFunctionLike(n) || (
n.parent && isPropertyDeclaration(n.parent) && !hasStaticModifier(n.parent) && n.parent.initializer === n
));
}

function getPartOfForStatementContainingNode(node: Node, container: ForStatement) {
return findAncestor(node, n => n === container ? "quit" : n === container.initializer || n === container.condition || n === container.incrementor || n === container.statement);
}

function getEnclosingIterationStatement(node: Node): Node | undefined {
return findAncestor(node, n => (!n || nodeStartsNewLexicalEnvironment(n)) ? "quit" : isIterationStatement(n, /*lookInLabeledStatements*/ false));
}

function checkNestedBlockScopedBinding(node: Identifier, symbol: Symbol): void {
if (languageVersion >= ScriptTarget.ES2015 ||
(symbol.flags & (SymbolFlags.BlockScopedVariable | SymbolFlags.Class)) === 0 ||
Expand All @@ -23507,20 +23513,11 @@ namespace ts {
// if there is an iteration statement in between declaration and boundary (is binding/class declared inside iteration statement)

const container = getEnclosingBlockScopeContainer(symbol.valueDeclaration);
const usedInFunction = isInsideFunction(node.parent, container);
let current = container;

let containedInIterationStatement = false;
while (current && !nodeStartsNewLexicalEnvironment(current)) {
if (isIterationStatement(current, /*lookInLabeledStatements*/ false)) {
containedInIterationStatement = true;
break;
}
current = current.parent;
}
const isCaptured = isInsideFunctionOrInstancePropertyInitializer(node, container);

if (containedInIterationStatement) {
if (usedInFunction) {
const enclosingIterationStatement = getEnclosingIterationStatement(container);
if (enclosingIterationStatement) {
if (isCaptured) {
// mark iteration statement as containing block-scoped binding captured in some function
let capturesBlockScopeBindingInLoopBody = true;
if (isForStatement(container)) {
Expand All @@ -23541,7 +23538,7 @@ namespace ts {
}
}
if (capturesBlockScopeBindingInLoopBody) {
getNodeLinks(current).flags |= NodeCheckFlags.LoopWithCapturedBlockScopedBinding;
getNodeLinks(enclosingIterationStatement).flags |= NodeCheckFlags.LoopWithCapturedBlockScopedBinding;
}
}

Expand All @@ -23558,7 +23555,7 @@ namespace ts {
getNodeLinks(symbol.valueDeclaration).flags |= NodeCheckFlags.BlockScopedBindingInLoop;
}

if (usedInFunction) {
if (isCaptured) {
getNodeLinks(symbol.valueDeclaration).flags |= NodeCheckFlags.CapturedBlockScopedBinding;
}
}
Expand Down Expand Up @@ -25151,6 +25148,20 @@ namespace ts {
const links = getNodeLinks(node.expression);
if (!links.resolvedType) {
links.resolvedType = checkExpression(node.expression);
// The computed property name of a non-static class field within a loop must be stored in a block-scoped binding.
// (It needs to be bound at class evaluation time.)
if (isPropertyDeclaration(node.parent) && !hasStaticModifier(node.parent) && isClassExpression(node.parent.parent)) {
const container = getEnclosingBlockScopeContainer(node.parent.parent);
const enclosingIterationStatement = getEnclosingIterationStatement(container);
if (enclosingIterationStatement) {
// The computed field name will use a block scoped binding which can be unique for each iteration of the loop.
getNodeLinks(enclosingIterationStatement).flags |= NodeCheckFlags.LoopWithCapturedBlockScopedBinding;
// The generated variable which stores the computed field name must be block-scoped.
getNodeLinks(node).flags |= NodeCheckFlags.BlockScopedBindingInLoop;
// The generated variable which stores the class must be block-scoped.
getNodeLinks(node.parent.parent).flags |= NodeCheckFlags.BlockScopedBindingInLoop;
}
}
// This will allow types number, string, symbol or any. It will also allow enums, the unknown
// type, and any union of these types (like string | number).
if (links.resolvedType.flags & TypeFlags.Nullable ||
Expand Down Expand Up @@ -32269,6 +32280,16 @@ namespace ts {
for (let lexicalScope = getEnclosingBlockScopeContainer(node); !!lexicalScope; lexicalScope = getEnclosingBlockScopeContainer(lexicalScope)) {
getNodeLinks(lexicalScope).flags |= NodeCheckFlags.ContainsClassWithPrivateIdentifiers;
}

// If this is a private field in a class expression inside the body of a loop,
// then we must use a block-scoped binding to store the WeakMap.
if (isClassExpression(node.parent)) {
const enclosingIterationStatement = getEnclosingIterationStatement(node.parent);
if (enclosingIterationStatement) {
getNodeLinks(node.name).flags |= NodeCheckFlags.BlockScopedBindingInLoop;
getNodeLinks(enclosingIterationStatement).flags |= NodeCheckFlags.LoopWithCapturedBlockScopedBinding;
}
}
}
}

Expand Down
49 changes: 49 additions & 0 deletions src/compiler/transformer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ namespace ts {
let lexicalEnvironmentFlagsStack: LexicalEnvironmentFlags[] = [];
let lexicalEnvironmentStackOffset = 0;
let lexicalEnvironmentSuspended = false;
let blockScopedVariableDeclarationsStack: Identifier[][] = [];
let blockScopeStackOffset = 0;
let blockScopedVariableDeclarations: Identifier[];
let emitHelpers: EmitHelper[] | undefined;
let onSubstituteNode: TransformationContext["onSubstituteNode"] = noEmitSubstitution;
let onEmitNode: TransformationContext["onEmitNode"] = noEmitNotification;
Expand All @@ -178,6 +181,9 @@ namespace ts {
hoistVariableDeclaration,
hoistFunctionDeclaration,
addInitializationStatement,
startBlockScope,
endBlockScope,
addBlockScopedVariable,
requestEmitHelper,
readEmitHelpers,
enableSubstitution,
Expand Down Expand Up @@ -469,6 +475,46 @@ namespace ts {
return lexicalEnvironmentFlags;
}

/**
* Starts a block scope. Any existing block hoisted variables are pushed onto the stack and the related storage variables are reset.
*/
function startBlockScope() {
Debug.assert(state > TransformationState.Uninitialized, "Cannot start a block scope during initialization.");
Debug.assert(state < TransformationState.Completed, "Cannot start a block scope after transformation has completed.");
blockScopedVariableDeclarationsStack[blockScopeStackOffset] = blockScopedVariableDeclarations;
blockScopeStackOffset++;
blockScopedVariableDeclarations = undefined!;
}

/**
* Ends a block scope. The previous set of block hoisted variables are restored. Any hoisted declarations are returned.
*/
function endBlockScope() {
Debug.assert(state > TransformationState.Uninitialized, "Cannot end a block scope during initialization.");
Debug.assert(state < TransformationState.Completed, "Cannot end a block scope after transformation has completed.");
const statements: Statement[] | undefined = some(blockScopedVariableDeclarations) ?
[
factory.createVariableStatement(
/*modifiers*/ undefined,
factory.createVariableDeclarationList(
blockScopedVariableDeclarations.map(identifier => factory.createVariableDeclaration(identifier)),
NodeFlags.Let
)
)
] : undefined;
blockScopeStackOffset--;
blockScopedVariableDeclarations = blockScopedVariableDeclarationsStack[blockScopeStackOffset];
if (blockScopeStackOffset === 0) {
blockScopedVariableDeclarationsStack = [];
}
return statements;
}

function addBlockScopedVariable(name: Identifier): void {
Debug.assert(blockScopeStackOffset > 0, "Cannot add a block scoped variable outside of an iteration body.");
(blockScopedVariableDeclarations || (blockScopedVariableDeclarations = [])).push(name);
}

function requestEmitHelper(helper: EmitHelper): void {
Debug.assert(state > TransformationState.Uninitialized, "Cannot modify the transformation context during initialization.");
Debug.assert(state < TransformationState.Completed, "Cannot modify the transformation context after transformation has completed.");
Expand Down Expand Up @@ -535,5 +581,8 @@ namespace ts {
startLexicalEnvironment: noop,
suspendLexicalEnvironment: noop,
addDiagnostic: noop,
startBlockScope: noop,
endBlockScope: returnUndefined,
addBlockScopedVariable: noop
};
}
26 changes: 19 additions & 7 deletions src/compiler/transformers/classFields.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ namespace ts {
factory,
hoistVariableDeclaration,
endLexicalEnvironment,
resumeLexicalEnvironment
resumeLexicalEnvironment,
addBlockScopedVariable
} = context;
const resolver = context.getEmitResolver();
const compilerOptions = context.getCompilerOptions();
Expand Down Expand Up @@ -310,7 +311,7 @@ namespace ts {
visitNode(node.initializer, visitor, isForInitializer),
visitNode(node.condition, visitor, isExpression),
visitPostfixUnaryExpression(node.incrementor, /*valueIsDiscarded*/ true),
visitNode(node.statement, visitor, isStatement)
visitIterationBody(node.statement, visitor, context)
);
}
return visitEachChild(node, visitor, context);
Expand Down Expand Up @@ -540,8 +541,10 @@ namespace ts {
}
else {
const expressions: Expression[] = [];
const isClassWithConstructorReference = resolver.getNodeCheckFlags(node) & NodeCheckFlags.ClassWithConstructorReference;
const temp = factory.createTempVariable(hoistVariableDeclaration, !!isClassWithConstructorReference);
const classCheckFlags = resolver.getNodeCheckFlags(node);
const isClassWithConstructorReference = classCheckFlags & NodeCheckFlags.ClassWithConstructorReference;
const requiresBlockScopedVar = classCheckFlags & NodeCheckFlags.BlockScopedBindingInLoop;
const temp = factory.createTempVariable(requiresBlockScopedVar ? addBlockScopedVariable : hoistVariableDeclaration, !!isClassWithConstructorReference);
if (isClassWithConstructorReference) {
// record an alias as the class name is not in scope for statics.
enableSubstitutionForClassAliases();
Expand Down Expand Up @@ -869,7 +872,6 @@ namespace ts {
return undefined;
}


/**
* If the name is a computed property, this function transforms it, then either returns an expression which caches the
* value of the result or the expression itself if the value is either unused or safe to inline into multiple locations
Expand All @@ -883,7 +885,12 @@ namespace ts {
const alreadyTransformed = isAssignmentExpression(innerExpression) && isGeneratedIdentifier(innerExpression.left);
if (!alreadyTransformed && !inlinable && shouldHoist) {
const generatedName = factory.getGeneratedNameForNode(name);
hoistVariableDeclaration(generatedName);
if (resolver.getNodeCheckFlags(name) & NodeCheckFlags.BlockScopedBindingInLoop) {
addBlockScopedVariable(generatedName);
}
else {
hoistVariableDeclaration(generatedName);
}
return factory.createAssignment(generatedName, expression);
}
return (inlinable || isIdentifier(innerExpression)) ? undefined : expression;
Expand All @@ -910,7 +917,12 @@ namespace ts {
function addPrivateIdentifierToEnvironment(name: PrivateIdentifier) {
const text = getTextOfPropertyName(name) as string;
const weakMapName = factory.createUniqueName("_" + text.substring(1), GeneratedIdentifierFlags.Optimistic | GeneratedIdentifierFlags.ReservedInNestedScopes);
hoistVariableDeclaration(weakMapName);
if (resolver.getNodeCheckFlags(name) & NodeCheckFlags.BlockScopedBindingInLoop) {
addBlockScopedVariable(weakMapName);
}
else {
hoistVariableDeclaration(weakMapName);
}
getPrivateIdentifierEnvironment().set(name.escapedText, { placement: PrivateIdentifierPlacement.InstanceField, weakMapName });
getPendingExpressions().push(
factory.createAssignment(
Expand Down
6 changes: 3 additions & 3 deletions src/compiler/transformers/es2017.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ namespace ts {
? visitVariableDeclarationListWithCollidingNames(node.initializer, /*hasReceiver*/ true)!
: visitNode(node.initializer, visitor, isForInitializer),
visitNode(node.expression, visitor, isExpression),
visitNode(node.statement, asyncBodyVisitor, isStatement, factory.liftToBlock)
visitIterationBody(node.statement, asyncBodyVisitor, context)
);
}

Expand All @@ -237,7 +237,7 @@ namespace ts {
? visitVariableDeclarationListWithCollidingNames(node.initializer, /*hasReceiver*/ true)!
: visitNode(node.initializer, visitor, isForInitializer),
visitNode(node.expression, visitor, isExpression),
visitNode(node.statement, asyncBodyVisitor, isStatement, factory.liftToBlock)
visitIterationBody(node.statement, asyncBodyVisitor, context)
);
}

Expand All @@ -250,7 +250,7 @@ namespace ts {
: visitNode(node.initializer, visitor, isForInitializer),
visitNode(node.condition, visitor, isExpression),
visitNode(node.incrementor, visitor, isExpression),
visitNode(node.statement, asyncBodyVisitor, isStatement, factory.liftToBlock)
visitIterationBody(node.statement, asyncBodyVisitor, context)
);
}

Expand Down
4 changes: 2 additions & 2 deletions src/compiler/transformers/es2018.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ namespace ts {
visitNode(node.initializer, visitorWithUnusedExpressionResult, isForInitializer),
visitNode(node.condition, visitor, isExpression),
visitNode(node.incrementor, visitorWithUnusedExpressionResult, isExpression),
visitNode(node.statement, visitor, isStatement)
visitIterationBody(node.statement, visitor, context)
);
}

Expand Down Expand Up @@ -648,7 +648,7 @@ namespace ts {
let bodyLocation: TextRange | undefined;
let statementsLocation: TextRange | undefined;
const statements: Statement[] = [visitNode(binding, visitor, isStatement)];
const statement = visitNode(node.statement, visitor, isStatement);
const statement = visitIterationBody(node.statement, visitor, context);
if (isBlock(statement)) {
addRange(statements, statement.statements);
bodyLocation = statement;
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/transformers/generators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1502,7 +1502,7 @@ namespace ts {
: undefined,
visitNode(node.condition, visitor, isExpression),
visitNode(node.incrementor, visitor, isExpression),
visitNode(node.statement, visitor, isStatement, factory.liftToBlock)
visitIterationBody(node.statement, visitor, context)
);
}
else {
Expand Down
10 changes: 5 additions & 5 deletions src/compiler/transformers/module/system.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1231,7 +1231,7 @@ namespace ts {
node.initializer && visitForInitializer(node.initializer),
visitNode(node.condition, destructuringAndImportCallVisitor, isExpression),
visitNode(node.incrementor, destructuringAndImportCallVisitor, isExpression),
visitNode(node.statement, nestedElementVisitor, isStatement)
visitIterationBody(node.statement, nestedElementVisitor, context)
);

enclosingBlockScopedContainer = savedEnclosingBlockScopedContainer;
Expand All @@ -1251,7 +1251,7 @@ namespace ts {
node,
visitForInitializer(node.initializer),
visitNode(node.expression, destructuringAndImportCallVisitor, isExpression),
visitNode(node.statement, nestedElementVisitor, isStatement, factory.liftToBlock)
visitIterationBody(node.statement, nestedElementVisitor, context)
);

enclosingBlockScopedContainer = savedEnclosingBlockScopedContainer;
Expand All @@ -1272,7 +1272,7 @@ namespace ts {
node.awaitModifier,
visitForInitializer(node.initializer),
visitNode(node.expression, destructuringAndImportCallVisitor, isExpression),
visitNode(node.statement, nestedElementVisitor, isStatement, factory.liftToBlock)
visitIterationBody(node.statement, nestedElementVisitor, context)
);

enclosingBlockScopedContainer = savedEnclosingBlockScopedContainer;
Expand Down Expand Up @@ -1320,7 +1320,7 @@ namespace ts {
function visitDoStatement(node: DoStatement): VisitResult<Statement> {
return factory.updateDoStatement(
node,
visitNode(node.statement, nestedElementVisitor, isStatement, factory.liftToBlock),
visitIterationBody(node.statement, nestedElementVisitor, context),
visitNode(node.expression, destructuringAndImportCallVisitor, isExpression)
);
}
Expand All @@ -1334,7 +1334,7 @@ namespace ts {
return factory.updateWhileStatement(
node,
visitNode(node.expression, destructuringAndImportCallVisitor, isExpression),
visitNode(node.statement, nestedElementVisitor, isStatement, factory.liftToBlock)
visitIterationBody(node.statement, nestedElementVisitor, context)
);
}

Expand Down
Loading