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

joeywatts · 2018-11-28T18:22:52Z

Fixes #27864.

For the following cases of class expressions/declarations in a loop:

ES private fields which are being downleveled generate a block-scoped binding for the WeakMap.
References to a block-scoped variable from within a property initializer are now marked as a capture in the checker, allowing that code to be properly downleveled to ES5 and lower
Computed instance properties which are downleveled generate block-scoped bindings for the name (which is referenced in the constructor).

msftclas · 2018-11-28T18:23:04Z

All CLA requirements met.

mheiber · 2018-11-28T18:42:49Z

This PR fixes #27864 (marked "Help Wanted") using the solution recommended by @rbuckton

rbuckton · 2018-11-29T00:45:30Z

src/compiler/types.ts

@@ -5241,6 +5241,11 @@ namespace ts {
        writeFile: WriteFileCallback;
    }

+    export const enum LexicalEnvironmentScoping {


I think LexicalEnvironmentKind would be more consistent with TypeScript's internal naming conventions.

rbuckton · 2018-11-29T00:48:51Z

src/compiler/transformer.ts

-                        createVariableDeclarationList(lexicalEnvironmentVariableDeclarations)
+                        createVariableDeclarationList(
+                            lexicalEnvironmentVariableDeclarations,
+                            lexicalEnvironmentScoping === LexicalEnvironmentScoping.Block ? NodeFlags.Let : undefined


Previously this was just a variable declaration environment, and not a block scope. I'm concerned that introducing let declarations here will be a problem for down-level transformations to ES5 as they won't have been visited by the type checker and won't have the necessary NodeCheckFlags to check for shadowing and per-iteration environments when transforming block-scoped variables down-level. This becomes even more complicated if this is part of the public API for TypeScript, as those things won't "just work" if someone builds a custom transformer.

Thanks for pointing this out - latest changes should rely on the checker flag being set to determine if it should be a block-hoisted variable - so there's no API to say "hoist this variable into the block scope."

rbuckton · 2018-11-29T00:52:24Z

src/compiler/types.ts

@@ -5249,7 +5254,7 @@ namespace ts {
        getCompilerOptions(): CompilerOptions;

        /** Starts a new lexical environment. */
-        startLexicalEnvironment(): void;
+        startLexicalEnvironment(scoping?: LexicalEnvironmentScoping): void;


I'm not sure this should be part of the public API. It presents the possibility of a custom transformer introducing a let-scoped variable we won't be able to properly down-level since it wasn't visited by the type checker.

The new code changes don't expose a public API for starting a block scope and rely on the checker flags being set to determine which scope it should hoist a variable into.

rbuckton · 2018-11-29T00:59:24Z

src/compiler/checker.ts

+                    const container = getEnclosingBlockScopeContainer(node);
+                    let current = container;
+                    let containedInIterationStatement = false;
+                    while (current && !nodeStartsNewLexicalEnvironment(current)) {


We're doing something similar already in checkNestedBlockScopedBinding. Is there overlap here that could be refactored into a separate method?

Refactored this into getEnclosingIterationStatement

joeywatts · 2019-02-25T16:34:48Z

pinging @rbuckton for another review pass

mheiber · 2019-03-06T13:35:18Z

@rbuckton would you be able to take another look? We're excited about this change, thanks for your time

rbuckton · 2019-03-08T23:10:19Z

@mheiber: I'll take another look this afternoon.
@joeywatts: There are some new conflicts with master. Could you address those please?

joeywatts · 2019-03-08T23:45:36Z

@rbuckton I've rebased and resolved the conflicts, should be good to go!

rbuckton · 2019-03-20T22:38:49Z

src/compiler/checker.ts

@@ -16726,6 +16726,11 @@ namespace ts {
                            if (container.kind === SyntaxKind.PropertyDeclaration && hasModifier(container, ModifierFlags.Static)) {
                                getNodeLinks(declaration).flags |= NodeCheckFlags.ClassWithConstructorReference;
                                getNodeLinks(node).flags |= NodeCheckFlags.ConstructorReferenceInClass;
+                                // If the class expression is in a loop and the name of the class is used,


This is something we would normally handle in checkNestedBlockScopedBinding, except it seems that function exits early if the target is es2015 or later. Perhaps we should instead add a parameter to checkNestedBlockScopedBinding to force it to execute when this condition is reached?

rbuckton · 2019-03-20T22:52:30Z

src/compiler/transformer.ts

+        /**
+         * Starts a block scope. Any existing block hoisted variables are pushed onto the stack and the related storage variables are reset.
+         */
+        function startBlockScope() {


If we are going to add these to TransformationContext, we should enforce them by default in visitEachChild (in visitor.ts), similar to how we enforce calls to startLexicalEnvironment and endLexicalEnvironment.

For example: in visitEachChild, we have the following switch case for a MethodDeclaration:

case SyntaxKind.MethodDeclaration: return updateMethod(<MethodDeclaration>node, ... visitParameterList((<MethodDeclaration>node).parameters, visitor, context, nodesVisitor), visitNode((<MethodDeclaration>node).type, visitor, isTypeNode), visitFunctionBody((<MethodDeclaration>node).body!, visitor, context));

If you look at visitParameterList, it contains the following at the top of the function:

export function visitParameterList(nodes: NodeArray<ParameterDeclaration> | undefined, visitor: Visitor, context: TransformationContext, nodesVisitor = visitNodes) { context.startLexicalEnvironment(); ...

Conversely, visitFunctionBody contains the following:

export function visitFunctionBody(node: ConciseBody | undefined, visitor: Visitor, context: TransformationContext): ConciseBody | undefined { ... const declarations = context.endLexicalEnvironment(); if (some(declarations)) { const block = convertToFunctionBody(updated); const statements = mergeLexicalEnvironment(block.statements, declarations); return updateBlock(block, statements); } return updated; }

We should so something similar for any Block.

rbuckton · 2019-03-20T22:55:42Z

src/compiler/transformer.ts

@@ -247,6 +252,11 @@ namespace ts {
        function hoistVariableDeclaration(name: Identifier): void {
            Debug.assert(state > TransformationState.Uninitialized, "Cannot modify the lexical environment during initialization.");
            Debug.assert(state < TransformationState.Completed, "Cannot modify the lexical environment after transformation has completed.");
+            // If the checker determined that this is a block scoped binding in a loop, we must emit a block-level variable declaration.


I would prefer if we had a separate function (i.e. addBlockVariableDeclaration) to add block-scoped variables rather than having hoistVariableDeclaration perform both. I'd rather avoid having to use the resolver for every temp variable we generate, and this would allow us to record net-new block-scoped temporary variables in other transformations.

NOTE: I suggest addBlockVariableDeclaration rather than hoistBlockVariableDeclaration above as "hoist" has a specific meaning that does not apply to block-scoped variables.

Also, for block-scoped variables we should probably always enforce GeneratedIdentifierFlags.ReserveInNestedScopes regardless of what was passed to createTempVariable.

How would you introduce a net-new block scoped variable correctly without the checker putting the appropriate flags on it? For example, the ES5 transformation relies on the checker flags to correctly determine whether there is a block scoped variable in a particular loop.

My primary concern is performance. I'd rather avoid lookups against the resolver for every generated identifier, if possible. The only case where this check seems like it is needed is here, so incurring the cost of the lookup for every hoisted variable is unnecessary overhead.

I would still recommend that we add addBlockVariableDeclaration for this case, but we can mark it as /* @internal */ for now and revisit the API for custom transformer consumption later.

rbuckton · 2019-03-20T22:57:10Z

src/compiler/transformers/ts.ts

@@ -549,6 +553,19 @@ namespace ts {
            }
        }

+        function visitBlock(node: Block): Block {


If you make the changes to visitEachChild I mentioned above, this function would not be necessary.

rbuckton · 2019-03-20T23:02:29Z

src/compiler/transformers/ts.ts

@@ -909,7 +926,13 @@ namespace ts {
            if (some(staticProperties) || some(pendingExpressions)) {
                const expressions: Expression[] = [];
                const isClassWithConstructorReference = resolver.getNodeCheckFlags(node) & NodeCheckFlags.ClassWithConstructorReference;
-                const temp = createTempVariable(hoistVariableDeclaration, !!isClassWithConstructorReference);
+                const temp = createTempVariable(
+                    name => {


If you make the changes to visitEachChild I mentioned above, you shouldn't need this closure as you can pass addBlockVariableDeclaration (or whatever we end up calling it) here and set the original node pointer after the fact.

sandersn · 2020-02-12T23:29:32Z

As best I can tell from reading through the comments, this PR has a few outstanding tasks before it's ready to merge. @joeywatts do you still have time to look at this? It seems like a worthwhile bug to fix.

joeywatts · 2020-02-12T23:38:07Z

@sandersn yes, I can try to fix this up in the next couple of days

rbuckton

Can you make a few small changes and update this to the new NodeFactory API? Once that's done I can merge.

src/compiler/visitor.ts

sandersn · 2021-02-16T23:30:55Z

@rbuckton can you check whether the commits since your June review address your comments and then merge this?

joeywatts · 2021-02-17T03:17:57Z

Rebased for you and got the tests all passing locally

sandersn · 2021-03-11T18:33:21Z

This is now merged in #43197

mheiber mentioned this pull request Nov 28, 2018

Computed Properties aren't bound correctly during Object/Class evaluation #27864

Open

rbuckton requested changes Nov 29, 2018

View reviewed changes

DanielRosenwasser assigned rbuckton Nov 29, 2018

DanielRosenwasser added this to the TypeScript 3.3.0 milestone Nov 29, 2018

This was referenced Dec 3, 2018

Fix Microsoft#27864 bloomberg/TypeScript#14

Closed

[blocked by discussion in Microsoft/TypeScript repo] Fix property initializers with block scoped bindings bloomberg/TypeScript#18

Closed

RyanCavanaugh modified the milestones: TypeScript 3.3, TypeScript 3.4.0 Feb 1, 2019

joeywatts force-pushed the computed-property-name-fix branch 2 times, most recently from 57a142f to 64a3d96 Compare March 8, 2019 23:28

DanielRosenwasser modified the milestones: TypeScript 3.4.0, TypeScript 3.5.0 Mar 20, 2019

rbuckton requested changes Mar 20, 2019

View reviewed changes

RyanCavanaugh modified the milestones: TypeScript 3.5.0, TypeScript 3.6.0 Jun 12, 2019

DanielRosenwasser modified the milestones: TypeScript 3.6.0, TypeScript 3.6.1 Jul 19, 2019

RyanCavanaugh modified the milestones: TypeScript 3.6.1, TypeScript 3.7.0, TypeScript 3.7.1 Sep 25, 2019

mheiber mentioned this pull request Dec 23, 2019

Private named instance fields #30829

Merged

7 tasks

sandersn added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 1, 2020

joeywatts force-pushed the computed-property-name-fix branch 3 times, most recently from b351223 to f48889f Compare February 21, 2020 06:50

joeywatts changed the title ~~Fix Computed Property Name Bindings~~ Fix Computed Property Name and Private Identifier Bindings inside Iteration Statements Feb 21, 2020

joeywatts changed the title ~~Fix Computed Property Name and Private Identifier Bindings inside Iteration Statements~~ Fix block-scoped capturing by class elements inside iteration statements Feb 21, 2020

joeywatts requested a review from rbuckton February 21, 2020 08:04

rbuckton requested changes Jun 27, 2020

View reviewed changes

src/compiler/visitor.ts Outdated Show resolved Hide resolved

src/compiler/visitor.ts Outdated Show resolved Hide resolved

joeywatts force-pushed the computed-property-name-fix branch from 2bc147e to 9c12240 Compare July 24, 2020 01:06

typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Milestone Bug PRs that fix a bug with a specific milestone labels Jul 24, 2020

joeywatts force-pushed the computed-property-name-fix branch 2 times, most recently from 1279c1e to 8230cef Compare July 24, 2020 19:09

joeywatts added 2 commits February 16, 2021 19:22

Fix property name bindings for class expr in loops

3839617

Fix block-scope capturing with prop initializers

6673dca

joeywatts force-pushed the computed-property-name-fix branch from 8230cef to 6673dca Compare February 17, 2021 03:16

sandersn mentioned this pull request Mar 11, 2021

Bloomberg computed property name fix #43197

Merged

sandersn closed this Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joeywatts commented Nov 28, 2018 •

edited

Loading

msftclas commented Nov 28, 2018 •

edited

Loading

mheiber commented Nov 28, 2018

rbuckton Nov 29, 2018

rbuckton Nov 29, 2018 •

edited

Loading

joeywatts Dec 3, 2018

rbuckton Nov 29, 2018

joeywatts Dec 3, 2018

rbuckton Nov 29, 2018

joeywatts Nov 29, 2018

joeywatts commented Feb 25, 2019

mheiber commented Mar 6, 2019

rbuckton commented Mar 8, 2019

joeywatts commented Mar 8, 2019

rbuckton Mar 20, 2019

rbuckton Mar 20, 2019

rbuckton Mar 20, 2019

rbuckton Mar 20, 2019

joeywatts Mar 27, 2019

rbuckton May 9, 2019

rbuckton Mar 20, 2019

rbuckton Mar 20, 2019

sandersn commented Feb 12, 2020

joeywatts commented Feb 12, 2020

rbuckton left a comment

sandersn commented Feb 16, 2021

joeywatts commented Feb 17, 2021

sandersn commented Mar 11, 2021

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

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

Conversation

joeywatts commented Nov 28, 2018 • edited Loading

msftclas commented Nov 28, 2018 • edited Loading

mheiber commented Nov 28, 2018

Choose a reason for hiding this comment

rbuckton Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeywatts commented Feb 25, 2019

mheiber commented Mar 6, 2019

rbuckton commented Mar 8, 2019

joeywatts commented Mar 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Feb 12, 2020

joeywatts commented Feb 12, 2020

rbuckton left a comment

Choose a reason for hiding this comment

sandersn commented Feb 16, 2021

joeywatts commented Feb 17, 2021

sandersn commented Mar 11, 2021

joeywatts commented Nov 28, 2018 •

edited

Loading

msftclas commented Nov 28, 2018 •

edited

Loading

rbuckton Nov 29, 2018 •

edited

Loading