Object rest #12028

sandersn · 2016-11-03T18:36:39Z

Leaves #10727 still open
Fixes #2103

Add object rest syntax as specified in the stage 3 proposal.

Note:

This does not include a rest type or a difference type. Rest syntax does not work if the rest target is not an object type. For example, type parameters do not work. We expect rest types or something similar in future versions of TypeScript.
The PR is relative to object-spread, not master. Mostly this is so I don't have to duplicate transformers/esnext.ts and the build file updates. I expect object-spread to go in first so it shouldn't make a difference.

Missed previously, just got the baselines

sandersn · 2016-11-03T22:24:06Z

@ahejlsberg @mhegazy do you want to take a look at this after the object-spread PR?

mhegazy · 2016-11-03T22:41:58Z

src/compiler/checker.ts

+            }
+            for (const prop of getPropertiesOfType(source)) {
+                const inNamesToRemove = prop.name in names;
+                const isPrivate = getDeclarationModifierFlagsFromSymbol(prop) & (ModifierFlags.Private | ModifierFlags.Protected);


why are we removing private? it will exist at runtime, we do not create it as enumerable: false, and it not a parent property.

well.. this is actually different from what we do with keyof where we filter privates as well.

Spread also removes privates. It's the right thing to do there, and I think it is here too:

class P { private p: number } let p: P; var { ...exposed } = p;

I don't think exposed should be an easy way to get all of P's private members.

ahejlsberg

Make sure you have @rbuckton look at the emitter changes.

ahejlsberg · 2016-11-03T23:16:09Z

src/compiler/binder.ts

+                    while (root && root.kind !== SyntaxKind.BinaryExpression) {
+                        root = root.parent;
+                    }
+                    emitFlags |= root && isDestructuringAssignment(root) ? NodeFlags.HasRestAttribute : NodeFlags.HasSpreadAttribute;


I'm not 100% sure this is right. You might be walking up from the right side of a destructuring assignment, in which case the spread is actually a spread.

I improved it to check that the relation between root and parent is that the object literal is the lefthand side.

ahejlsberg · 2016-11-03T23:34:03Z

src/compiler/emitter.ts

+        const restHelper = `
+var __rest = (this && this.__rest) || function (s, e) {
+    var t = {};
+


No blank lines in the helpers.

ahejlsberg · 2016-11-03T23:38:52Z

src/compiler/transformers/esnext.ts

+                    )
+                );
+            }
+        }


Wow, that's an awful lot of code. I'm surprised it takes this much, but I'm not familiar enough with the new emitter to truly evaluate this. Make sure you have @rbuckton take a look.

It's copied from ES2015's function and for-of transforms, then simplified. I'll go back and see whether the two can share the code better.

I moved a lot of shared code from ES2015 and ESNext to factory.ts. ESNext is a lot shorter now.

mhegazy · 2016-11-03T23:44:57Z

src/compiler/emitter.ts

+        const restHelper = `
+var __rest = (this && this.__rest) || function (s, e) {
+    var t = {};
+


nit. remove empty line

1. Remove extra line in __rest shim. 2. Improve __rest vs __assign check for destructuring assignment.

Saves a lot of duplicated code

It is shared by es2015 and esNext transformers. This commit just adds a convertObjectRest flag to be passed on to flattenDestructuring functions, as well as adding necessary parameters to use the code outside a transformer.

sandersn · 2016-11-04T21:00:15Z

src/compiler/factory.ts

+     * Transforms the body of a function-like node.
+     *
+     * @param node A function-like node.
+     */


the only changes here are to add extra parameters needed to use transformFunctionBody and friends outside of a transformer, and to pass along convertObjectRest to flattenParameterDestructuring.

sandersn · 2016-11-04T21:02:17Z

src/compiler/factory.ts

+        //
+        //     for (let v of arr) { }
+        //
+        // we don't want to emit a temporary variable for the RHS, just use it directly.


the only changes to convertForOf are

in creating counter and elementAccess

calling flattenParameterDestructuring with convertObjectRest

calling createForOf vs createFor depending on whether convertObjectRest is true.

Otherwise it's just moved from es2015

sandersn · 2016-11-04T21:08:40Z

@dotnet-bot test this please

Previously array destructuring inside an object destructuring with an object rest would downlevel the array destructuring to ES5. This breaks if the code that targets ES2015 is using iterators instead of arrays since iterators don't support [0] or .slice that the ES5 emit uses.

Now with object destructuring inside array destructuring inside object destructuring! Each with their own array/object rest! Also updates baselines.

rbuckton

The emit looks good, based on our offline discussion of the runtime semantics.

mhegazy · 2016-11-10T17:14:06Z

src/compiler/emitter.ts

@@ -42,6 +42,14 @@ var __assign = (this && this.__assign) || Object.assign || function(t) {
    return t;
 };`;

+        const restHelper = `
+var __rest = (this && this.__rest) || function (s, e) {


we will need to add this to https://github.com/Microsoft/tslib

also you will need a check in checker.ts for __rest and __assign, and add some tests for these.

sveisvei · 2016-11-10T19:04:37Z

Great success!

lgleim · 2016-11-10T19:07:03Z

Wohoo! Awesome :)

damianobarbati · 2016-11-14T11:00:25Z

How is the object spread operator going to deal with nested objects?
Is it going to clone them or is it going to pass the reference?

Babel-kindaof-bugged example:

'use strict';

let a = { first: 1, next: { two: 2 } };
let b = { ...a };
b.first = 11;
b.next.two = 22;

//here we get a.next.two = 22 instead of 2!
console.log('a =', a);
console.log('b =', b);

alitaheri · 2016-11-14T11:05:08Z

@damianobarbati This is the correct behavior. That's how it works. { ...a } will create a shallow clone, not a deep clone. If you want a deep clone you can use lodash. { ...a } is equivalent to Object.assign({}, a) as per spec.

damianobarbati · 2016-11-14T11:06:34Z

Ok, just like current array spread operator then. Thanks @alitaheri

sandersn added 6 commits November 2, 2016 14:55

Parse, bind and check rest elements

a7c1836

Downlevel emit of rest elements

e3a08ed

Add rest tests

ac20b46

Update baselines

9d4ddb6

Merge branch 'object-spread' into object-rest-syntax

7ff8876

Update objectRestAssignment test and baselines

7ed5204

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

sandersn added 2 commits November 3, 2016 13:33

Update objectRestAssignment test

14f8b99

Missed previously, just got the baselines

Improve readability of ES next destructuring emit

c9c5f49

mhegazy reviewed Nov 3, 2016

View reviewed changes

ahejlsberg requested changes Nov 3, 2016

View reviewed changes

mhegazy reviewed Nov 3, 2016

View reviewed changes

sandersn added 4 commits November 4, 2016 09:03

Address PR comments

71f3157

1. Remove extra line in __rest shim. 2. Improve __rest vs __assign check for destructuring assignment.

Move convertForOf to factory for esnext and es2015

cc342d1

Saves a lot of duplicated code

Update improved baselines

4337369

Move transformFunctionBody to factory

4c365bd

It is shared by es2015 and esNext transformers. This commit just adds a convertObjectRest flag to be passed on to flattenDestructuring functions, as well as adding necessary parameters to use the code outside a transformer.

sandersn commented Nov 4, 2016

View reviewed changes

dsifford mentioned this pull request Nov 5, 2016

Support proposed ES Rest/Spread properties #2103

Closed

sandersn added 4 commits November 7, 2016 11:10

Merge branch 'object-spread' into object-rest

609cd00

Do not emit __rest for --target esnext

9977936

Improve nested destructuring test for ESNext emit

0196947

Now with object destructuring inside array destructuring inside object destructuring! Each with their own array/object rest! Also updates baselines.

rbuckton approved these changes Nov 9, 2016

View reviewed changes

mhegazy reviewed Nov 10, 2016

View reviewed changes

sandersn merged commit 25462c9 into object-spread Nov 10, 2016

sandersn deleted the object-rest branch November 10, 2016 17:18

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Object rest #12028

Object rest #12028

sandersn commented Nov 3, 2016 •

edited

Loading

sandersn commented Nov 3, 2016

mhegazy Nov 3, 2016

mhegazy Nov 3, 2016

sandersn Nov 3, 2016

ahejlsberg left a comment

ahejlsberg Nov 3, 2016

sandersn Nov 4, 2016

ahejlsberg Nov 3, 2016

sandersn Nov 4, 2016

ahejlsberg Nov 3, 2016

sandersn Nov 4, 2016

sandersn Nov 4, 2016

mhegazy Nov 3, 2016

sandersn Nov 4, 2016

sandersn Nov 4, 2016

sandersn Nov 4, 2016

sandersn commented Nov 4, 2016

rbuckton left a comment

mhegazy Nov 10, 2016

mhegazy Nov 10, 2016

sveisvei commented Nov 10, 2016

lgleim commented Nov 10, 2016

damianobarbati commented Nov 14, 2016 •

edited

Loading

alitaheri commented Nov 14, 2016

damianobarbati commented Nov 14, 2016

Object rest #12028

Object rest #12028

Conversation

sandersn commented Nov 3, 2016 • edited Loading

sandersn commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahejlsberg 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

sandersn commented Nov 4, 2016

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

sveisvei commented Nov 10, 2016

lgleim commented Nov 10, 2016

damianobarbati commented Nov 14, 2016 • edited Loading

alitaheri commented Nov 14, 2016

damianobarbati commented Nov 14, 2016

sandersn commented Nov 3, 2016 •

edited

Loading

damianobarbati commented Nov 14, 2016 •

edited

Loading