Object spread rest fixes #12248

sandersn · 2016-11-15T00:26:43Z

Fixes #12211
Fixes #12200
Fixes #12227
Fixes #12228

New:
Fixes #12252

Rename finds identifiers in spread assignment expressions.
Spreads with computed properties of type number or any no longer crash the compiler.
Rest emit uses indexOf === -1 instead of !indexOf to filter properties.
Rest emit correctly refers to computed properties' generated temps.

1. Rename finds identifiers in spread assignment expressions. 2. Spreads with computed properties of type number or any no longer crash the compiler. 3. Rest emit uses indexOf === -1 instead of !indexOf to filter properties. 4. Rest emit correctly refers to computed properties' generated temps.

rbuckton

A few minor comments, but generally this looks fine.

rbuckton · 2016-11-15T01:18:38Z

src/compiler/checker.ts

@@ -11193,7 +11193,10 @@ namespace ts {
            let hasComputedStringProperty = false;
            let hasComputedNumberProperty = false;

-            for (const memberDecl of node.properties) {
+            let i = 0;


This looks like it can probably be part of the following for declaration rather than on its own.

Fixed, left over from an earlier single-variable attempt.

rbuckton · 2016-11-15T01:19:56Z

src/compiler/emitter.ts

@@ -45,7 +45,7 @@ var __assign = (this && this.__assign) || Object.assign || function(t) {
        const restHelper = `
 var __rest = (this && this.__rest) || function (s, e) {
    var t = {};
-    for (var p in s) if (Object.prototype.hasOwnProperty.call(s, p) && !e.indexOf(p))
+    for (var p in s) if (Object.prototype.hasOwnProperty.call(s, p) && e.indexOf(p) === -1)


Wouldn't >= 0 be a few less chars?

Even less chars would be !~e.indexOf(p)

The test is for not found. But I guess i could use < 0.

@timsuchanek Typescript's emit tries to balance readability, speed and brevity. Brevity is the least important of the three, though, and I think !~e.indexOf(p) is less readable than e.indexOf(p) < 0.

Switched to < 0.

rbuckton · 2016-11-15T01:22:03Z

src/compiler/transformers/destructuring.ts

-                propertyNames.push(str);
+                if (isComputedPropertyName(getPropertyName(element))) {
+                    // get the temp name and put that in there instead, like `_tmp + ""`
+                    const stringifiedTemp = <StringLiteral>createSynthesizedNode(SyntaxKind.StringLiteral);


Do we need the location for the literal? Also, why not just use createLiteral("", location)?

Weird, last time I looked I thought createLiteral said that it returns NumericLiteral. You're right, that's much better.

I don't know what location is used for -- source maps, right? I'll leave it out of the createLiteral("") call for now and leave it in elsewhere.

rbuckton · 2016-11-15T01:25:00Z

src/compiler/transformers/destructuring.ts

+                    propertyNames.push(createBinary(computedTempVariables.shift(), SyntaxKind.PlusToken, stringifiedTemp));
+                }
+                else {
+                    const str = <StringLiteral>createSynthesizedNode(SyntaxKind.StringLiteral);


For StringLiteral and Identifier you can just call createLiteral(element), which creates a StringLiteral that reads its literal text from the original identifier. That way you can preserve unicode escape sequences. We could probably add support for NumericLiteral to createLiteral as well.

Ah, that's how I got confused. Tide for emacs doesn't display overloads and I got shown the number overload of createLiteral with no indication that there are others.

rbuckton · 2016-11-15T01:26:41Z

src/compiler/transformers/destructuring.ts

@@ -548,8 +569,13 @@ namespace ts {
                        bindingElements = [];
                    }
                    // Rewrite element to a declaration with an initializer that fetches property
+                    // TODO: Probably save the result of createDestructuringPropertyAccess if propName is a computed property


Is this TODO something you still have yet to do for this PR or is it for future work?

It's already done. I delete the TODO.

rbuckton · 2016-11-15T01:27:36Z

src/compiler/transformers/destructuring.ts

-                    emitBindingElement(element, createDestructuringPropertyAccess(value, propName));
+                    const propAccess = createDestructuringPropertyAccess(value, propName);
+                    if (isComputedPropertyName(propName)) {
+                        (computedTempVariables = computedTempVariables || []).push((propAccess as ElementAccessExpression).argumentExpression);


See append in core.ts:

computedTempVariables = append(computedTempVariables, (propAccess as ElementAccessExpression).argumentExpression);

Thanks for the reminder. I keep forgetting about append.

rbuckton · 2016-11-15T01:28:38Z

tests/baselines/reference/objectRest.symbols

@@ -1,42 +1,42 @@
 === tests/cases/conformance/types/rest/objectRest.ts ===
-let o = { a: 1, b: 'no' }
->o : Symbol(o, Decl(objectRest.ts, 0, 3))
+var o = { a: 1, b: 'no' }


Why are these suddenly var and not let?

I changed the source declaration (line 2) so I could reuse o in a destructuring assignment below.

Jessidhia · 2016-11-15T01:53:38Z

src/compiler/transformers/destructuring.ts

+                    stringifiedTemp.pos = location.pos;
+                    stringifiedTemp.end = location.end;
+                    stringifiedTemp.text = "";
+                    propertyNames.push(createBinary(computedTempVariables.shift(), SyntaxKind.PlusToken, stringifiedTemp));


This will crash at runtime if the typeof of the key is "symbol".

Do you suggest toString instead?

That would crash with "number" / "undefined" / null instead :(

A workaround could be to use String() instead, as that will work with anything (and hopefully nobody actually has a string key that looks like "Symbol()"), but it would prevent easily improving __rest into being able to preserve (and filter) Symbol...

The most correct option would be typeof x == "symbol" ? x : String(x).

Update baselines with smaller emit as part of that.

Previously, symbols were not handled.

It used the index of the symbol property as the name, not the symbol itself.

sandersn added 3 commits November 14, 2016 16:20

Add spread tests and update rest tests

fe66d9e

Update baselines

7c72f3d

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

This was referenced Nov 15, 2016

Object spread rename issue #12211

Closed

Object spread with rest arguments causes compile error #12200

Closed

Object rest issue with computed property names #12228

Closed

rbuckton requested changes Nov 15, 2016

View reviewed changes

Jessidhia mentioned this pull request Nov 15, 2016

Object rest does not preserve enumerable Symbol keys #12252

Closed

Jessidhia reviewed Nov 15, 2016

View reviewed changes

sandersn added 7 commits November 15, 2016 09:10

Address PR commennts

b9fa06f

Update baselines with smaller emit as part of that.

Improve rest/spread emit for symbols

47f3319

Previously, symbols were not handled.

Update baselines

0690bf4

Merge branch 'master' into object-spread-rest-fixes

2496756

Update baseline of newly merged test

c011b36

Fix spread/rest emit for symbols.

bb46e78

It used the index of the symbol property as the name, not the symbol itself.

Update baselines with fixed emit

6c1f733

rbuckton approved these changes Nov 15, 2016

View reviewed changes

sandersn merged commit d96a0ad into master Nov 15, 2016

sandersn deleted the object-spread-rest-fixes branch November 15, 2016 20:32

weswigham mentioned this pull request Nov 16, 2016

Snapshot nightlies during the night #12283

Closed

alitaheri mentioned this pull request Nov 16, 2016

Add __rest helper microsoft/tslib#6

Merged

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Object spread rest fixes #12248

Object spread rest fixes #12248

sandersn commented Nov 15, 2016 •

edited

Loading

rbuckton left a comment

rbuckton Nov 15, 2016

sandersn Nov 15, 2016

rbuckton Nov 15, 2016

timsuchanek Nov 15, 2016

sandersn Nov 15, 2016

sandersn Nov 15, 2016

rbuckton Nov 15, 2016

sandersn Nov 15, 2016

sandersn Nov 15, 2016

rbuckton Nov 15, 2016

sandersn Nov 15, 2016

rbuckton Nov 15, 2016

sandersn Nov 15, 2016

rbuckton Nov 15, 2016

sandersn Nov 15, 2016

rbuckton Nov 15, 2016

sandersn Nov 15, 2016

Jessidhia Nov 15, 2016 •

edited

Loading

sandersn Nov 15, 2016

Jessidhia Nov 15, 2016 •

edited

Loading

Object spread rest fixes #12248

Object spread rest fixes #12248

Conversation

sandersn commented Nov 15, 2016 • edited Loading

rbuckton left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jessidhia Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jessidhia Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

sandersn commented Nov 15, 2016 •

edited

Loading

Jessidhia Nov 15, 2016 •

edited

Loading

Jessidhia Nov 15, 2016 •

edited

Loading