Fix property name bindings for class expr in loops

Author: joeywatts

scripts/request-pr-review.ts

@@ -42,7 +42,7 @@ main().catch(console.error);
 
 async function main() {
     const gh = new Octokit({ auth: options.token });
-    const response = await gh.pulls.requestReviewers({
+    const response = await (gh.pulls as any).requestReviewers({
         owner: options.owner,
         repo: options.repo,
         pull_number,

src/compiler/checker.ts

@@ -22262,6 +22262,10 @@ namespace ts {
             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 ||
@@ -22277,18 +22281,9 @@ namespace ts {
 
             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;
-            }
 
-            if (containedInIterationStatement) {
+            const enclosingIterationStatement = getEnclosingIterationStatement(container);
+            if (enclosingIterationStatement) {
                 if (usedInFunction) {
                     // mark iteration statement as containing block-scoped binding captured in some function
                     let capturesBlockScopeBindingInLoopBody = true;
@@ -22310,7 +22305,7 @@ namespace ts {
                         }
                     }
                     if (capturesBlockScopeBindingInLoopBody) {
-                        getNodeLinks(current).flags |= NodeCheckFlags.LoopWithCapturedBlockScopedBinding;
+                        getNodeLinks(enclosingIterationStatement).flags |= NodeCheckFlags.LoopWithCapturedBlockScopedBinding;
                     }
                 }
 
@@ -23882,6 +23877,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 ||
@@ -30714,6 +30723,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;
+                    }
+                }
             }
         }
 

src/compiler/transformer.ts

@@ -156,6 +156,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;
@@ -179,6 +182,9 @@ namespace ts {
             hoistVariableDeclaration,
             hoistFunctionDeclaration,
             addInitializationStatement,
+            startBlockScope,
+            endBlockScope,
+            addBlockScopedVariable,
             requestEmitHelper,
             readEmitHelpers,
             enableSubstitution,
@@ -465,6 +471,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.");
@@ -531,5 +577,8 @@ namespace ts {
         startLexicalEnvironment: noop,
         suspendLexicalEnvironment: noop,
         addDiagnostic: noop,
+        startBlockScope: noop,
+        endBlockScope: returnUndefined,
+        addBlockScopedVariable: noop
     };
 }

src/compiler/transformers/classFields.ts

@@ -36,7 +36,8 @@ namespace ts {
             factory,
             hoistVariableDeclaration,
             endLexicalEnvironment,
-            resumeLexicalEnvironment
+            resumeLexicalEnvironment,
+            addBlockScopedVariable
         } = context;
         const resolver = context.getEmitResolver();
         const compilerOptions = context.getCompilerOptions();
@@ -312,7 +313,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);
@@ -533,8 +534,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();
@@ -859,7 +862,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
@@ -873,7 +875,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;
@@ -900,7 +907,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(

src/compiler/transformers/es2017.ts

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

src/compiler/transformers/es2018.ts

@@ -542,7 +542,7 @@ namespace ts {
                 visitNode(node.initializer, visitorNoDestructuringValue, isForInitializer),
                 visitNode(node.condition, visitor, isExpression),
                 visitNode(node.incrementor, visitor, isExpression),
-                visitNode(node.statement, visitor, isStatement)
+                visitIterationBody(node.statement, visitor, context)
             );
         }
 
@@ -615,7 +615,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;

src/compiler/transformers/generators.ts

@@ -1477,7 +1477,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 {

src/compiler/transformers/module/system.ts

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

src/compiler/types.ts

@@ -7311,6 +7311,12 @@ namespace ts {
         /** Hoists a variable declaration to the containing scope. */
         hoistVariableDeclaration(node: Identifier): void;
 
+        /*@internal*/ startBlockScope(): void;
+
+        /*@internal*/ endBlockScope(): Statement[] | undefined;
+
+        /*@internal*/ addBlockScopedVariable(node: Identifier): void;
+
         /** Adds an initialization statement to the top of the lexical environment. */
         /* @internal */
         addInitializationStatement(node: Statement): void;