Fix property name bindings for class expr in loops

Author: joeywatts

src/compiler/checker.ts

@@ -20722,6 +20722,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 ||
@@ -20737,18 +20741,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;
@@ -20770,7 +20765,7 @@ namespace ts {
                         }
                     }
                     if (capturesBlockScopeBindingInLoopBody) {
-                        getNodeLinks(current).flags |= NodeCheckFlags.LoopWithCapturedBlockScopedBinding;
+                        getNodeLinks(enclosingIterationStatement).flags |= NodeCheckFlags.LoopWithCapturedBlockScopedBinding;
                     }
                 }
 
@@ -22369,6 +22364,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 ||
@@ -28916,6 +28925,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/factory.ts

@@ -18,6 +18,9 @@ namespace ts {
         resumeLexicalEnvironment: noop,
         startLexicalEnvironment: noop,
         suspendLexicalEnvironment: noop,
+        startBlockScope: noop,
+        endBlockScope: returnUndefined,
+        addBlockScopedVariable: noop,
         addDiagnostic: noop,
     };
 

src/compiler/transformer.ts

@@ -152,6 +152,9 @@ namespace ts {
         let lexicalEnvironmentFunctionDeclarationsStack: FunctionDeclaration[][] = [];
         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;
@@ -170,6 +173,9 @@ namespace ts {
             endLexicalEnvironment,
             hoistVariableDeclaration,
             hoistFunctionDeclaration,
+            startBlockScope,
+            endBlockScope,
+            addBlockScopedVariable,
             requestEmitHelper,
             readEmitHelpers,
             enableSubstitution,
@@ -408,6 +414,46 @@ namespace ts {
             return statements;
         }
 
+        /**
+         * 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) ?
+                [
+                    createVariableStatement(
+                        /*modifiers*/ undefined,
+                        createVariableDeclarationList(
+                            blockScopedVariableDeclarations.map(identifier => 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.");

src/compiler/transformers/classFields.ts

@@ -35,7 +35,8 @@ namespace ts {
         const {
             hoistVariableDeclaration,
             endLexicalEnvironment,
-            resumeLexicalEnvironment
+            resumeLexicalEnvironment,
+            addBlockScopedVariable
         } = context;
         const resolver = context.getEmitResolver();
         const compilerOptions = context.getCompilerOptions();
@@ -210,9 +211,12 @@ namespace ts {
             // Create a temporary variable to store a computed property name (if necessary).
             // If it's not inlineable, then we emit an expression after the class which assigns
             // the property name to the temporary variable.
-            const expr = getPropertyNameExpressionIfNeeded(node.name, !!node.initializer || !!context.getCompilerOptions().useDefineForClassFields);
-            if (expr && !isSimpleInlineableExpression(expr)) {
-                (pendingExpressions || (pendingExpressions = [])).push(expr);
+            // No need to transform static computed property names since they are only evaluated once.
+            if (!hasStaticModifier(node)) {
+                const expr = getPropertyNameExpressionIfNeeded(node.name, !!node.initializer || !!context.getCompilerOptions().useDefineForClassFields);
+                if (expr && !isSimpleInlineableExpression(expr)) {
+                    (pendingExpressions || (pendingExpressions = [])).push(expr);
+                }
             }
             return undefined;
         }
@@ -532,8 +536,10 @@ namespace ts {
                 }
                 else {
                     const expressions: Expression[] = [];
-                    const isClassWithConstructorReference = resolver.getNodeCheckFlags(node) & NodeCheckFlags.ClassWithConstructorReference;
-                    const temp = createTempVariable(hoistVariableDeclaration, !!isClassWithConstructorReference);
+                    const classCheckFlags = resolver.getNodeCheckFlags(node);
+                    const isClassWithConstructorReference = classCheckFlags & NodeCheckFlags.ClassWithConstructorReference;
+                    const requiresBlockScopedVar = classCheckFlags & NodeCheckFlags.BlockScopedBindingInLoop;
+                    const temp = createTempVariable(requiresBlockScopedVar ? addBlockScopedVariable : hoistVariableDeclaration, !!isClassWithConstructorReference);
                     if (isClassWithConstructorReference) {
                         // record an alias as the class name is not in scope for statics.
                         enableSubstitutionForClassAliases();
@@ -748,7 +754,7 @@ namespace ts {
         function transformProperty(property: PropertyDeclaration, receiver: LeftHandSideExpression) {
             // We generate a name here in order to reuse the value cached by the relocated computed name expression (which uses the same generated name)
             const emitAssignment = !context.getCompilerOptions().useDefineForClassFields;
-            const propertyName = isComputedPropertyName(property.name) && !isSimpleInlineableExpression(property.name.expression)
+            const propertyName = !hasStaticModifier(property) && isComputedPropertyName(property.name) && !isSimpleInlineableExpression(property.name.expression)
                 ? updateComputedPropertyName(property.name, getGeneratedNameForNode(property.name))
                 : property.name;
 
@@ -858,7 +864,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
@@ -872,7 +877,12 @@ namespace ts {
                 const alreadyTransformed = isAssignmentExpression(innerExpression) && isGeneratedIdentifier(innerExpression.left);
                 if (!alreadyTransformed && !inlinable && shouldHoist) {
                     const generatedName = getGeneratedNameForNode(name);
-                    hoistVariableDeclaration(generatedName);
+                    if (resolver.getNodeCheckFlags(name) & NodeCheckFlags.BlockScopedBindingInLoop) {
+                        addBlockScopedVariable(generatedName);
+                    }
+                    else {
+                        hoistVariableDeclaration(generatedName);
+                    }
                     return createAssignment(generatedName, expression);
                 }
                 return (inlinable || isIdentifier(innerExpression)) ? undefined : expression;
@@ -892,7 +902,12 @@ namespace ts {
             const text = getTextOfPropertyName(name) as string;
             const weakMapName = createOptimisticUniqueName("_" + text.substring(1));
             weakMapName.autoGenerateFlags |= GeneratedIdentifierFlags.ReservedInNestedScopes;
-            hoistVariableDeclaration(weakMapName);
+            if (resolver.getNodeCheckFlags(name) & NodeCheckFlags.BlockScopedBindingInLoop) {
+                addBlockScopedVariable(weakMapName);
+            }
+            else {
+                hoistVariableDeclaration(weakMapName);
+            }
             (currentPrivateIdentifierEnvironment || (currentPrivateIdentifierEnvironment = createUnderscoreEscapedMap()))
                 .set(name.escapedText, { placement: PrivateIdentifierPlacement.InstanceField, weakMapName });
             (pendingExpressions || (pendingExpressions = [])).push(

src/compiler/types.ts

@@ -5929,6 +5929,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;
+
         /** Records a request for a non-scoped emit helper in the current context. */
         requestEmitHelper(helper: EmitHelper): void;
 

src/compiler/visitor.ts

@@ -549,6 +549,25 @@ namespace ts {
         return <Statement>singleOrUndefined(nodes) || createBlock(<NodeArray<Statement>>nodes);
     }
 
+    export function visitIterationBody(body: Statement, visitor: Visitor, context: TransformationContext): Statement {
+        context.startBlockScope();
+        const updated = visitNode(body, visitor, isStatement, liftToBlock);
+        const declarations = context.endBlockScope();
+        if (some(declarations)) {
+            if (isBlock(updated)) {
+                declarations.push(...updated.statements);
+            }
+            else {
+                declarations.push(updated);
+            }
+            return setTextRange(
+                setOriginalNode(createBlock(declarations), body),
+                body
+            );
+        }
+        return updated;
+    }
+
     /**
      * Aggregates the TransformFlags for a Node and its subtree.
      */

src/compiler/visitorPublic.ts

@@ -636,33 +636,33 @@ namespace ts {
 
             case SyntaxKind.DoStatement:
                 return updateDo(<DoStatement>node,
-                    visitNode((<DoStatement>node).statement, visitor, isStatement, liftToBlock),
+                    visitIterationBody((<DoStatement>node).statement, visitor, context),
                     visitNode((<DoStatement>node).expression, visitor, isExpression));
 
             case SyntaxKind.WhileStatement:
                 return updateWhile(<WhileStatement>node,
                     visitNode((<WhileStatement>node).expression, visitor, isExpression),
-                    visitNode((<WhileStatement>node).statement, visitor, isStatement, liftToBlock));
+                    visitIterationBody((<WhileStatement>node).statement, visitor, context));
 
             case SyntaxKind.ForStatement:
                 return updateFor(<ForStatement>node,
                     visitNode((<ForStatement>node).initializer, visitor, isForInitializer),
                     visitNode((<ForStatement>node).condition, visitor, isExpression),
                     visitNode((<ForStatement>node).incrementor, visitor, isExpression),
-                    visitNode((<ForStatement>node).statement, visitor, isStatement, liftToBlock));
+                    visitIterationBody((<ForStatement>node).statement, visitor, context));
 
             case SyntaxKind.ForInStatement:
                 return updateForIn(<ForInStatement>node,
                     visitNode((<ForInStatement>node).initializer, visitor, isForInitializer),
                     visitNode((<ForInStatement>node).expression, visitor, isExpression),
-                    visitNode((<ForInStatement>node).statement, visitor, isStatement, liftToBlock));
+                    visitIterationBody((<ForInStatement>node).statement, visitor, context));
 
             case SyntaxKind.ForOfStatement:
                 return updateForOf(<ForOfStatement>node,
                     visitNode((<ForOfStatement>node).awaitModifier, visitor, isToken),
                     visitNode((<ForOfStatement>node).initializer, visitor, isForInitializer),
                     visitNode((<ForOfStatement>node).expression, visitor, isExpression),
-                    visitNode((<ForOfStatement>node).statement, visitor, isStatement, liftToBlock));
+                    visitIterationBody((<ForOfStatement>node).statement, visitor, context));
 
             case SyntaxKind.ContinueStatement:
                 return updateContinue(<ContinueStatement>node,

tests/baselines/reference/computedPropertyNames52(target=es2015).js

@@ -0,0 +1,23 @@
+//// [computedPropertyNames52.js]
+const array = [];
+for (let i = 0; i < 10; ++i) {
+    array.push(class C {
+        [i] = () => C;
+        static [i] = 100;
+    })
+}
+
+
+//// [computedPropertyNames52-emit.js]
+const array = [];
+for (let i = 0; i < 10; ++i) {
+    let _a, _b;
+    array.push((_b = class C {
+            constructor() {
+                this[_a] = () => C;
+            }
+        },
+        _a = i,
+        _b[i] = 100,
+        _b));
+}

tests/baselines/reference/computedPropertyNames52(target=es2015).symbols

@@ -0,0 +1,27 @@
+=== tests/cases/conformance/es6/computedProperties/computedPropertyNames52.js ===
+const array = [];
+>array : Symbol(array, Decl(computedPropertyNames52.js, 0, 5))
+
+for (let i = 0; i < 10; ++i) {
+>i : Symbol(i, Decl(computedPropertyNames52.js, 1, 8))
+>i : Symbol(i, Decl(computedPropertyNames52.js, 1, 8))
+>i : Symbol(i, Decl(computedPropertyNames52.js, 1, 8))
+
+    array.push(class C {
+>array.push : Symbol(Array.push, Decl(lib.es5.d.ts, --, --))
+>array : Symbol(array, Decl(computedPropertyNames52.js, 0, 5))
+>push : Symbol(Array.push, Decl(lib.es5.d.ts, --, --))
+>C : Symbol(C, Decl(computedPropertyNames52.js, 2, 15))
+
+        [i] = () => C;
+>[i] : Symbol(C[i], Decl(computedPropertyNames52.js, 2, 24))
+>i : Symbol(i, Decl(computedPropertyNames52.js, 1, 8))
+>C : Symbol(C, Decl(computedPropertyNames52.js, 2, 15))
+
+        static [i] = 100;
+>[i] : Symbol(C[i], Decl(computedPropertyNames52.js, 3, 22))
+>i : Symbol(i, Decl(computedPropertyNames52.js, 1, 8))
+
+    })
+}
+