Rework computed property name + decorator emit by weswigham · Pull Request #18350 · microsoft/TypeScript

weswigham · 2017-09-08T23:02:37Z

Computed property names for properties were being elided if uninitialized, or, if initialized, run within the constructor of the class. This is incorrect - for reference, computed property names for methods/getters/setters execute outside the class, and only once. This change makes a computed property name be hoisted to outside of the class declaration if it is not "simple" (and could have observable side effects from being invoked multiple times on each construction of the class). Then, if hoisted, the hoisted identifier is used in the decorator emit if need be. If not hoisted, but still decorated (if the expression is simple), then the simple expression is duplicated into the decorator emit. As an added bonus, we also no longer omit a computed property name expression whose execution may have observable side effects if there is no initializer.

We issue errors on the usage of most complex expressions in computed property names today, but this brings the property computed name emit (when it could matter) in-line with the method/getter/setter computed name runtime behavior. This shouldn't be considered a breaking change, since we issue an error on computed names on properties which are not directly a string/number/symbol; but should now have correct emit for if/when we allow it as part of allowing local symbol types as computed property names or other proposals around expanding what we allow for computed property names.

As of now, a "simple" computed property is defined as a numeric literal, a string literal, a no substitution template literal, a keyword, or a syntactically well-known symbol - it is possible that there is room to expand this if anyone has further suggestions.

mhegazy · 2017-09-14T23:22:57Z

@rbuckton can you please review

rbuckton · 2017-10-12T21:42:31Z

    }

-    export function nodeCanBeDecorated(node: Node): boolean {
+    export function nodeCanBeDecorated(node: Node, parent: Node = node.parent, grandparent: Node = parent.parent): boolean {


Rather than add parent and grandparent as optional arguments, I'd rather make them required. We can just modify the checker to pass in node.parent and node.parent.parent to the call. That way the function won't be possibly misused in other places later.

rbuckton · 2017-10-12T21:43:28Z

    }

-    export function childIsDecorated(node: Node): boolean {
+    export function childIsDecorated(node: Node, parent?: Node): boolean {


Can we remove the optionality?

rbuckton · 2017-10-12T21:44:55Z

    }

-    export function nodeIsDecorated(node: Node): boolean {
+    export function nodeIsDecorated(node: Node, parent?: Node, grandparent?: Node): boolean {


Can we remove the optionality, or possibly add overloads for cases where we don't need to know parent or grandparent?

rbuckton · 2017-10-12T21:45:01Z


-    export function nodeOrChildIsDecorated(node: Node): boolean {
-        return nodeIsDecorated(node) || childIsDecorated(node);
+    export function nodeOrChildIsDecorated(node: Node, parent?: Node, grandparent?: Node): boolean {


Can we remove the optionality, or possibly add overloads for cases where we don't need to know parent or grandparent?

rbuckton · 2017-10-12T21:49:57Z

         * @param isStatic A value indicating whether the member should be a static or instance member.
         */
-        function isInitializedProperty(member: ClassElement, isStatic: boolean) {
+        function isInitializedProperty(member: ClassElement, isStatic: boolean): member is PropertyDeclaration {


This type predicate is somewhat inaccurate, since it digs further into a PropertyDeclaration. This results in narrowing at the call site such that PropertyDeclaration is possibly removed from the list of constituents of member even though it still could contain other PropertyDeclaration nodes.

rbuckton · 2017-10-12T21:50:17Z

                && (<PropertyDeclaration>member).initializer !== undefined;
        }

+        function isComputedNameWhichRequiresHoisting(member: ClassElement, isStatic: boolean): member is PropertyDeclaration {


Same as above, the type predicate is inaccurate.

Judging from the one place I see this called, isStatic is only ever false.

rbuckton · 2017-10-16T20:40:02Z

                statements = [varStatement];
            }
+            if (hoistedNameAssignments.length) {
+                statements.unshift(createVariableStatement(/*modifiers*/ undefined, map(hoistedNameAssignments, a => createVariableDeclaration(a.left as Identifier, /*type*/ undefined, a.right))));


Seems a bit wasteful to create a BinaryExpression just to drop it. In destructuring.ts we have to do something similar, so we pass a callback that creates the correct assignment in each context.

rbuckton · 2017-10-16T20:43:24Z

+            if (!node.members) return;
+            const result: PropertyDeclaration[] = [];
+            for (let member of node.members) {
+                if (!isStatic && hasComputedNameWhichRequiresHoisting(member)) {


It feels like this function now has too many responsibilities. I'd much rather keep the hoistedComputedNames and decoratedMembers parts separate.

rbuckton · 2017-10-16T20:46:49Z

            // If the class does not contain nodes that require a synthesized constructor,
            // accept the current constructor if it exists.
            if (!hasInstancePropertyWithInitializer && !hasParameterPropertyAssignments) {
+                getInitializedProperties(node, /*isStatic*/ false, hoistedComputedNames, decoratedMembers);


The fact we don't use the return value here indicates getInitializedProperties now has too many responsibilities.

If I rename it to collectPropertyTransformationResults and make it output all three things through output array arguments (or does it make more sense to return an object with three array members?), does it become acceptable? Because it certainly doesn't only apply for initialized properties anymore, and all the work still needs to be done.

Possibly, if you are able to pass the results around rather than recalculate them. In general though I'd rather see smaller and more focused methods, e.g.:

getInitializedProperties(node, isStatic)

getHoistedComputedNames(properties)

getDecoratedMembers(properties)

rbuckton · 2017-10-16T20:47:47Z

         * @param receiver The object receiving the property assignment.
         */
        function transformInitializedProperty(property: PropertyDeclaration, receiver: LeftHandSideExpression) {
-            const propertyName = visitPropertyNameOfClassElement(property);


We don't need to do this for fields anymore?

Correct. The work is done inside the hoisting of computed names/decorated names. If you still do it, you get redundant output like so:

this[_a = _c] = null; this[_b = _f] = null; var _a, _b;

We know we don't need to transform computed property names on initialized properties here because we've already dealt with them in the section where we hoist them.

weswigham · 2017-10-16T22:30:30Z

@rbuckton I think I've refactored away all your concerns.

rbuckton · 2017-10-16T23:14:15Z

I was thinking something more like this:

// at the top of visitClassDeclaration/visitClassExpression
const allInitializedProperties = filter(node.members, isInitializedProperty);
const staticProperties = filter(allInitializedProperties, isStaticMember);
const nonStaticProperties = filter(allInitializedProperties, isNonStaticMember);
const hoistedMembers = filter(nonStaticProperties, hasComputedNameWhichRequiresHoisting);
const decoratedMembers = filter(node.members, isDecoratedClassElement);


// in visitClassDeclaration
const hoistingDeclarations = hoistComputedExpressions(hoistedMembers, createVariableDeclaration);
if (some(hoistingDeclarations)) {
    statements.push(createVariableStatement(hoistingDeclarations));
}

// in visitClassExpression
const hoistingAssignments = hoistComputedExpressions(hoistedMembers, hoistAssignment);
// prefix these before classExpression. hoistAssignment takes care of the temporary variables.

// in visitPropertyNameOfClassElement
if (hasComputedNameWhichRequiresHoisting(member)) {
    return updateComputedPropertyName(member.name, getGeneratedNameForNode(member.name));
}

// elsewhere in ts.ts
function isStaticMember(member: ClassElement) { return hasModifier(member, ModifierFlags.Static); }
function isNonStaticMember(member: ClassElement) { return !hasModifier(member, ModifierFlags.Static); }

function hoistComputedExpressions<T extends Expression | VariableDeclaration>(hoistedMembers: PropertyDeclaration[], createAssignmentCallback: (name: Identifier, expression: Expression) => T): T {
    let result: T[];
    for (const member of hoistedMembers) {
        const tempId = getGeneratedNameForNode(member.name);
        const assignment = createAssignmentCallback(tempId, visitNode((<ComputedPropertyName>member.name).expression, visitor, isExpression));
        result = append(result, assignment);
    }
    return result;
}

function hoistAssignment(name: Identifier, expression: Expression) {
    hoistVariableDeclaration(name);
    return createAssignment(name, expression);
}

weswigham · 2017-10-17T21:50:27Z

@rbuckton The only issue with what you have there is that we have to hoist all computed names which could have sideeffects, not just initialized ones. So you have this matrix where:

You can have a property which is both computed and initialized - must hoist and transform the computed name and transform the initializer.
You have a property with a computed name - must hoist and transform the computed name (to ensure the expression still gets executed) and elide the property.
You have a property with an initializer - must transform the initializer (static and instance are transformed differently, one stays a member, the other becomes a statement in the constructor body).
You have a property with neither a computed name, an initializer - it is just elided.

Right now I'm just doing this by applying two (well, three) small transforms in succession - one to hoist and transform computed names, then one to handle all initialized properties (then one to write decorators, but that's mostly independent now except that it needs the output of the last transform to get its property name references right).

Thinking on it, transformDecoratedProperties is clearly the wrong name for that function - I'm going to rename it to transformComputedPropertyNames. Additionally, if the array allocations are an issue, I can modify it to take a callback to use to hoist the expressions, which should eliminate them.

… array allocations using callback

weswigham · 2017-10-17T22:04:25Z

@rbuckton I think this is much cleaner now.

weswigham · 2017-10-17T22:43:57Z

After conversing with @rbuckton, we're going to change this to respect execution order in the event of an exetends clause which could depend on something executed within a computed property name.

weswigham · 2017-10-23T21:09:35Z

Superseded by #19430.

weswigham assigned rbuckton, mhegazy and ghost Sep 8, 2017

msftclas added the cla-already-signed label Sep 8, 2017

weswigham unassigned rbuckton, mhegazy and ghost Sep 8, 2017

weswigham requested review from mhegazy, rbuckton and sandersn September 8, 2017 23:03

msftgits removed the cla-already-signed label Sep 26, 2017

weswigham force-pushed the computed-property-decorator-emit branch 2 times, most recently from f2ad6c8 to 6b1d89b Compare October 12, 2017 17:39

rbuckton suggested changes Oct 12, 2017

View reviewed changes

weswigham force-pushed the computed-property-decorator-emit branch from aa68d8a to 44d3d8e Compare October 12, 2017 22:35

rbuckton suggested changes Oct 16, 2017

View reviewed changes

weswigham added 4 commits October 16, 2017 14:18

Correctly hoist computed property names of class elements

278dc72

Accept baseline updates from new computed property emit

e9b3c4c

Remove type guards, unneeded parameter

567d405

Overloads

f7c935d

weswigham force-pushed the computed-property-decorator-emit branch from 44d3d8e to f7c935d Compare October 16, 2017 21:21

Refactor to not use output arrays

7033e4e

Do away with specialized but overgeneral composition function, reduce…

7b81776

… array allocations using callback

Wrap up this style at a working state

9485c65

weswigham mentioned this pull request Oct 23, 2017

Revised emit for computed property names, including with decorators #19430

Merged

weswigham closed this Oct 23, 2017

microsoft locked and limited conversation to collaborators Jun 14, 2018

Conversation

weswigham commented Sep 8, 2017

Uh oh!

mhegazy commented Sep 14, 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 Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Oct 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weswigham commented Oct 16, 2017

Uh oh!

rbuckton commented Oct 16, 2017

Uh oh!

weswigham commented Oct 17, 2017

Uh oh!

weswigham commented Oct 17, 2017

Uh oh!

weswigham commented Oct 17, 2017

Uh oh!

weswigham commented Oct 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

weswigham Oct 16, 2017 •

edited

Loading

weswigham Oct 16, 2017 •

edited

Loading