Revised emit for computed property names, including with decorators by weswigham · Pull Request #19430 · microsoft/TypeScript

weswigham · 2017-10-23T21:04:58Z

This is a more correct alternative to #18350 based on discussions we had around it.

Computed property names on elided declarations (namely property declarations when transpiling to es6 or below), if they could have sideffects, are now moved into the next expression execution position available for them to execute in. This could be in a computed method name, or after the end of the class declaration/expression. As a sideeffect, the ts transform now must visit methods to place any pending expressions in their computed names, if needed. This is important for compliance with the public fields proposal, as computed names for public fields are supposed to work the same as computed names for methods.

Fixes #17028.

weswigham · 2017-10-23T22:27:52Z

@rbuckton This PR implements the emit as you suggested in-person. Is anything missing?

rbuckton · 2017-10-23T23:21:45Z

            write("enum ");
            emit(node.name);
-            pushNameGenerationScope();
+            pushNameGenerationScope(node);


I suppose enums aren't really a place you can inject temporary variables either, so they probably shouldn't be a name-generation scope (even though that only really matters for javavascript output and enums are typescript only).

rbuckton · 2017-10-23T23:26:35Z

        Iterator = 1 << 23,                     // The expression to a `yield*` should be treated as an Iterator when down-leveling, not an Iterable.
        NoAsciiEscaping = 1 << 24,              // When synthesizing nodes that lack an original node or textSourceNode, we want to write the text on the node with ASCII escaping substitutions.
        /*@internal*/ TypeScriptClassWrapper = 1 << 25, // The node is an IIFE class wrapper created by the ts transform.
+        /*@internal*/ NoNameGenerationScope = 1 << 26, // Used to indicate when a function or other wrapper shouldn't be considered a name generation scope because its generated locals are merged with a higher scope


We already have ReuseTempVariableScope which is supposed to mean the same thing. I like your approach where you added node to pushNameGenerationScope and popNameGenerationScope. Perhaps we can combine the two approaches and deduplicate the behavior in the emitter?

rbuckton · 2017-10-23T23:29:36Z

            if (name.autoGenerateKind === GeneratedIdentifierKind.Node) {
                // Node names generate unique names based on their original node
                // and are cached based on that node's id.
+                let flags: TempFlags;


Rather than read twice, the approach we've taken in other places (such as with context flags in the parser) is to do something like this:

if (name.skipNameGenerationScope) { const savedTempFlags = tempFlags; popNameGenerationScope(/*node*/ undefined); const result = generateNameCached(getNodeForGeneratedName(name)); pushNameGenerationScope(/*node*/ undefined); tempFlags = savedTempFlags; } else { return generateNameCached(getNodeForGeneratedName(name)); }

rbuckton · 2017-10-23T23:31:45Z

            // transformation.
            if (getEmitFlags(node) & EmitFlags.Indented) {
-                setEmitFlags(classFunction, EmitFlags.Indented);
+                setEmitFlags(classFunction, EmitFlags.Indented | EmitFlags.NoNameGenerationScope);


Perhaps collapse/remove the if:

setEmitFlags(classFunction, (getEmitFlags(node) & EmitFlags.Indented) | EmitFlags.NoNameGenerationScope);

rbuckton · 2017-10-23T23:32:43Z


                case SyntaxKind.PropertyDeclaration:
-                    // TypeScript property declarations are elided.
+                    // TypeScript property declarations are elided. However their names are still visited, and can potentially be retained if they could have sideeffects


Can we do this in a visitPropertyDeclaration function instead?

rbuckton · 2017-10-23T23:48:47Z

    }
    return C;
 }());
+var _a;


We seem to now have an unnecessary variable here.

rbuckton · 2017-10-23T23:49:56Z

 //// [parserComputedPropertyName31.js]
 class C {
 }
+var _a, _b;


We seem to now have unnecessary variables here.

rbuckton · 2017-10-23T23:50:37Z

+        this[_a] = id++;
    }
 }
+_a = e, _b = e2;


_b isn't even used and e2 itself has no side effects.

rbuckton · 2017-10-23T23:51:37Z

 //// [parserComputedPropertyName36.js]
 class C {
 }
+var _a;


We seem to now have unnecessary variables here.

rbuckton · 2017-10-23T23:52:24Z

+        this[_a] = 0;
    }
-    [Symbol()]() { }
+    [_a = Symbol(), _b = Symbol(), Symbol()]() { }


_b is unused.

weswigham · 2017-10-24T00:52:56Z

@rbuckton Got 'em all. Made the consolidations and style changes you requested, and updated the transform to only create an assignment when the assignment result is used, and only retain the expression if there is no assignment use if the expression looks like it could be side-effecting.

Jessidhia · 2017-10-25T05:20:51Z

-    }()); }
+    if (y === void 0) { y = (_a = /** @class */ (function () {
+            function class_2() {
+                this[_b] = x;


I suppose this means that computed key names are evaluated only once, at declaration time, instead of at construction time?

Correct, just like computed names for methods already are. We were inconsistent here.

rbuckton · 2017-10-30T19:20:37Z

        }

+        function isSimpleInlineableExpression(expression: Expression) {
+            return !isIdentifier(expression) && isSimpleCopiableExpression(expression) ||


Would a GeneratedIdentifier be "inlineable"?

Only if such a Generatedidentifier has all of its assignments occur before any intended usages of the identifier. Not something we check for in a uniform way, I think, but it may happen to be true for all generated identifiers at present?

rbuckton · 2017-10-30T19:23:59Z

         */
        function transformInitializedProperty(property: PropertyDeclaration, receiver: LeftHandSideExpression) {
-            const propertyName = visitPropertyNameOfClassElement(property);
+            const propertyName = isComputedPropertyName(property.name) && !isSimpleInlineableExpression(property.name.expression)


Add comments to explain why we are generating the name here.

rbuckton · 2017-10-30T19:24:20Z

            );
        }

+        function isSimpleInlineableExpression(expression: Expression) {


Document what it means to be a "simple inlineable expression"

rbuckton · 2017-10-30T19:25:29Z


            let statements: Statement[] = [classStatement];

+            // Write any pending expressions


Add more context as to what these "pending expressions" are.

rbuckton · 2017-10-30T19:26:06Z

                // the body of a class with static initializers.
                setEmitFlags(classExpression, EmitFlags.Indented | getEmitFlags(classExpression));
                expressions.push(startOnNewLine(createAssignment(temp, classExpression)));
+                addRange(expressions, pendingExpressions);


As with visitClassDeclaration, add a comment to explain what the "pending expressions" are.

rbuckton · 2017-10-30T19:27:44Z

            }
        }

+        function getPropertyNameExpressionIfNeeded(name: PropertyName, shouldHoist: boolean, omitSimple: boolean): Expression {


Add documentation to explain this function's purpose. Also omitSimple is not very clear, either give it a better name or document it.

rbuckton · 2017-10-30T19:28:18Z

-                    expression = createAssignment(generatedName, expression);
+            let expr = getPropertyNameExpressionIfNeeded(name, some(member.decorators), /*omitSimple*/ false);
+            if (expr) { // expr only exists if `name` is a computed property name
+                if (some(pendingExpressions)) {


Add comments to explain why we are inlining the existing pending expressions.

rbuckton · 2017-10-30T19:29:28Z

+                this[_b] = x;
+            }
+            return class_2;
+        }()), _b = x,


Should _b = x, be on its own line?

I'm more befuddled as to why _a appears on the following line, actually (because they should all be inline). AFAIK the comma list doesn't specify multiline (nor should it), so I'm not sure where the break is coming from.

Then again statics appear on their own lines in the same context, apparently, so I suppose so?

rbuckton · 2017-10-30T19:35:30Z

 }
-var f2 = function _a() {
-    var _newTarget = this && this instanceof _a ? this.constructor : void 0;
+var f2 = function _b() {


What changed the names here?

_a was always used by the name for the static function expression on A - now there's no name generation scope for a class body, so it's the same scope as here, thereby bumping these names to the next character.

…sistency

weswigham · 2017-10-31T18:24:32Z

@rbuckton Done. As mentioned in person, I had to migrate startsOnNewLine to emitNode to handle it correctly for the pending expressions.

rbuckton · 2017-11-04T17:24:41Z

        constantValue?: string | number;         // The constant value of an expression
        externalHelpersModuleName?: Identifier;  // The local name for an imported helpers module
        helpers?: EmitHelper[];                  // Emit helpers for the node
+        startsOnNewLine?: boolean;               // If the ndoe should begin on a new line


weswigham mentioned this pull request Oct 23, 2017

Rework computed property name + decorator emit #18350

Closed

weswigham requested review from mhegazy and rbuckton October 23, 2017 21:10

Revised emit for computed property names

b3a1cf2

weswigham force-pushed the computed-property-rework-mark2 branch from 9ed0edb to b3a1cf2 Compare October 23, 2017 21:29

weswigham added 2 commits October 23, 2017 14:49

Fix downlevel name generation scopes

c2e2724

Accept slightly more conservative baseline

0c3357e

rbuckton suggested changes Oct 23, 2017

View reviewed changes

weswigham added 3 commits October 23, 2017 17:30

First feedback pass

02f532b

Reduce number of nonrequired variable declarations and assignments

3dd65cf

Remove side-effect-free identifier references

07146f3

skip partially emitted expressions

e2d362b

Jessidhia reviewed Oct 25, 2017

View reviewed changes

rbuckton suggested changes Oct 30, 2017

View reviewed changes

weswigham added 2 commits October 31, 2017 11:16

Comments, move starsOnNewLine to emitNode

bb00211

Put expressions on newlines when inlined in class expressions for con…

478c90e

…sistency

weswigham added 3 commits October 31, 2017 13:25

Merge branch 'master' into computed-property-rework-mark2

fb9ed85

Merge branch 'master' into computed-property-rework-mark2

3594148

Update new ref

f6c186c

rbuckton approved these changes Nov 4, 2017

View reviewed changes

Fix typo in comment

b86cbbb

weswigham merged commit 4f48bf8 into microsoft:master Nov 6, 2017

weswigham deleted the computed-property-rework-mark2 branch November 6, 2017 20:51

mattmccutchen mentioned this pull request Feb 1, 2018

Computed property names not emitted if containing class is not transformed #21544

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018


		let statements: Statement[] = [classStatement];

		// Write any pending expressions

Conversation

weswigham commented Oct 23, 2017

Uh oh!

weswigham commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham commented Oct 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants