Streamline destructuring by rbuckton · Pull Request #12250 · microsoft/TypeScript

rbuckton · 2016-11-15T01:16:09Z

This PR cleans up some of our destructuring transform to be more consistent, along with a few additional changes:

Converges flattening of binding pattern and assignment patterns to follow the same code paths.
Adds to NodeArray a transformFlags property which consists of the transform flags of each node in the array without exclusions. This allows us to query the transform flags of each node in a node array without the need to traverse the array.
Consolidates a few TransformFlags enum members to free up some space
Added ContainsObjectRest/ContainsObjectSpread to allow us to more accurately detect esnext features.
Changes the type check for Object rest assignment to allow any valid lref via a call to checkReferenceExpression to align with the spec proposal.
Fixes a few cases where we unnecessarily emit the rval for a DestructuringAssignment that goes unused.
Cleans up esnext.ts to remove dependencies on es2015 transformations.
Moves es2015 transformations added to factory.ts back into es2015.ts

rbuckton · 2016-11-15T01:16:21Z

…ctory

I missed these before, so emit was incorrect for object rest in a method or accessor parameter.

sandersn

The code change looks good although I still need to look at the tests. Glancing over them, though, it looks like it's just improvements.

sandersn · 2016-11-15T22:27:13Z

src/compiler/factory.ts

-                    /*recordTempVariable*/ undefined,
+                    /*skipInitializer*/ false,
+                    /*recordTempVariablesInLine*/ true,
                    convertObjectRest


This is not an issue now that I've merged in the work from isolateObjectSpread.

sandersn · 2016-11-15T22:27:39Z

src/compiler/factory.ts

+                                temp,
+                                /*skipInitializer*/ convertObjectRest,
+                                /*recordTempVariablesInLine*/ true,
+                                convertObjectRest


why not make the enum a parameter to this function instead of handling the boolean?

This is not an issue now that I've merged in the work from isolateObjectSpread.

sandersn · 2016-11-15T22:29:06Z

src/compiler/factory.ts

-                            context.hoistVariableDeclaration,
-                            visitor,
                            convertObjectRest
+                                ? FlattenLevel.ObjectRest


sandersn · 2016-11-15T23:29:55Z

src/compiler/transformers/destructuring.ts


 /*@internal*/
 namespace ts {
+    interface FlattenHost {


probably FlattenContext is a better name

Works for me.

sandersn · 2016-11-15T23:43:17Z

src/compiler/transformers/destructuring.ts

+            const propertyName = getPropertyNameOfBindingOrAssignmentElement(elements[i]);
+            if (propertyName) {
+                if (isComputedPropertyName(propertyName)) {
+                    // get the temp name and put that in there instead, like `_tmp + ""`


can you update this to just give the emit: typeof _tmp === "symbol" ? _tmp : _tmp + "" ? The comment I wrote is wordy and out of date to boot.

…accessor-parameters' into streamlineDestructuring

sandersn

Looks good except for some typos.

sandersn · 2016-11-16T15:58:56Z

src/compiler/transformer.ts

         */
        function hoistVariableDeclaration(name: Identifier): void {
-            Debug.assert(!lexicalEnvironmentDisabled, "Cannot modify the lexical environment during the print phase.");
+            Debug.assert(!scopeModificationDisabled, "Cannot modify the lexical environment during the print phase.");


Probably should update the assert message.

Or maybe this is still correct. I can't tell.

sandersn · 2016-11-16T16:04:56Z

src/compiler/transformers/destructuring.ts

        context: TransformationContext;
        level: FlattenLevel;
-        recordTempVariablesInLine: boolean;
+        doNotRecordTempVariablesInLine: boolean;


Minor, but is there a positive way to express doNotRecordInline? Hoist? Extrapose? Prepend?

sandersn · 2016-11-16T16:07:11Z

src/compiler/transformers/destructuring.ts

     * @param boundValue The value bound to the declaration.
-     * @param recordTempVariablesInLine Indicates whether temporary variables should be recored in-line.
+     * @param skipInitializer A value indicating whether to ignore the initializer of `node`.
+     * @param doNotRecordTempVariablesInLine Indicates whether temporary variables should not be recored in-line.


Typo:recorded

sandersn · 2016-11-16T16:10:36Z

src/compiler/transformers/destructuring.ts

-        boundValue: Expression | undefined,
+        flattenContext: FlattenContext,
+        element: BindingOrAssignmentElement,
+        value: Expression | undefined,


I like the shorter names

sandersn · 2016-11-16T16:14:32Z

src/compiler/transformers/destructuring.ts

        }
    }

+    /**


These 3-line jsdoc are low-value in my opinion. They just restate in English what the signature already says.

rbuckton added 6 commits November 11, 2016 17:23

WIP Clean up destructuring

8babde0

Consolidate destructuring code paths

d5b9263

Clean up destructuring

08f467b

Merge branch 'master' into isolateObjectSpread

4577227

Update baselines, fix assignment check for object rest.

60d5195

Moved the parenthesizing functions to make the diff easier to read

cbdde42

msftclas added the cla-not-required label Nov 15, 2016

rbuckton and others added 7 commits November 14, 2016 18:30

Removed esnext dependency on convertForOf/transformFunctionBody in fa…

ddcd22e

…ctory

Make multiLine optional in convertToFunctionBody

dff9849

Object rest emit for method and accessor parameters

b4d3bca

I missed these before, so emit was incorrect for object rest in a method or accessor parameter.

Test object rest parameter emit: methods/accessors

498d978

Move transformFunctionBody back into es2015

98e192f

Merge branch 'master' into streamlineDestructuring

0dd96f5

Merge branch 'streamlineDestructuring' into isolateObjectSpread

8733973

rbuckton mentioned this pull request Nov 15, 2016

Object rest emit for method and accessor parameters #12272

Merged

sandersn reviewed Nov 15, 2016

View reviewed changes

rbuckton added 3 commits November 15, 2016 17:09

Merge remote-tracking branch 'origin/object-rest-emit-for-method-and-…

ba4f52c

…accessor-parameters' into streamlineDestructuring

Added missing places for visitParameterList

a0da47f

Merge branch 'master' into streamlineDestructuring

3a67174

sandersn approved these changes Nov 16, 2016

View reviewed changes

PR Feedback

cd023b2

rbuckton merged commit 3110f40 into master Nov 16, 2016

rbuckton deleted the streamlineDestructuring branch November 16, 2016 18:10

rbuckton mentioned this pull request Nov 16, 2016

Improve spread helper emit #12304

Merged

microsoft locked and limited conversation to collaborators Jun 19, 2018

                       }
                   }
+                  /**

Conversation

rbuckton commented Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Nov 15, 2016

Uh oh!

sandersn left a comment

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!

sandersn left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rbuckton commented Nov 15, 2016 •

edited

Loading