1339395 - Implement Object Rest/Spread Properties proposal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Object Rest/Spread Properties proposal is now Stage 3.
http://sebmarkbage.github.io/ecmascript-rest-spread/
https://github.com/sebmarkbage/ecmascript-rest-spread

Comment 1

[Rémy Hubscher (:natim)]

Refs https://github.com/Kinto/kinto.js/pull/652#issuecomment-279994961

Comment 2

[André Bargull [:anba]]

Attachment: bug1339395-part1-change-obj-lit-parser.patch

This only changes Parser::objectBindingPattern() and Parser::objectLiteral() to match the parsing strategy used for Parser::arrayBindingPattern() and Parser::arrayInitializer(). (This makes part 2 easier to implement.)

Comment 3

[André Bargull [:anba]]

Attachment: bug1339395-part1-ignore-whitespace.patch (not for checkin)

Part 1 with --ignore-whitespace.

Comment 4

[André Bargull [:anba]]

Attachment: bug1339395-part2-parse-object-rest-spread.patch

Adds parser support for object rest and spread properties. (I still need to write Reflect.parse tests!)

Comment 5

[André Bargull [:anba]]

Attachment: bug1339395-part3-bytecode-emitter.patch

Do you think this is the correct approach for an initial implementation of this feature?

Comment 6

[André Bargull [:anba]]

Attachment: bug1339395-part4-enable-test262.patch

Enables the test262 tests.

Comment 7

[André Bargull [:anba]]

Attachment: bug1339395-part5-reflect-parse.patch

Basic tests for Reflect.parse.

Comment 8

[André Bargull [:anba]]

Attachment: bug1339395-part6-update-jit-test.patch

The error column numbers need to be update for one jit-test.

Comment 9

[André Bargull [:anba]]

Try (without part 6): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c357d7f289125e5c3938507e02fd84f53c64b8f

Comment 10

[André Bargull [:anba]]

I should probably file an issue at the proposal repository to forbid array patterns in rest-property positions (var {...[]} = o), because that's not useful to have at all.

Comment 11

[Shu-yu Guo [:shu]]

Oh wow, I was just thinking of getting around to this. Thanks for the patches.

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8860905 [details] [diff] [review]
bug1339395-part1-change-obj-lit-parser.patch

Review of attachment 8860905 [details] [diff] [review]:
-----------------------------------------------------------------

Double checking -- all this does is change the obj literal parse methods to

1) Use peekToken instead of getToken
2) Break out of loop on COMMA
3) Require RC
4) Extra indentation

Didn't read the now-indented parts line-by-line.

Comment 13

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #12)
> Comment on attachment 8860905 [details] [diff] [review]
> bug1339395-part1-change-obj-lit-parser.patch
> 
> Review of attachment 8860905 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Double checking -- all this does is change the obj literal parse methods to
> 
> 1) Use peekToken instead of getToken
> 2) Break out of loop on COMMA
> 3) Require RC
> 4) Extra indentation
> 
> Didn't read the now-indented parts line-by-line.

Oh derp, you even uploaded an ignore-whitespace version. Looks good.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8860908 [details] [diff] [review]
bug1339395-part2-parse-object-rest-spread.patch

Review of attachment 8860908 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +9981,5 @@
>              return null();
>          if (!matched)
>              break;
> +        if (tt == TOK_TRIPLEDOT && possibleError)
> +            possibleError->setPendingDestructuringErrorAt(pos(), JSMSG_REST_WITH_COMMA);

Isn't this an unconditional error both in destructuring and in literals?

The spec text is actually missing extensions to the _ObjectLiteral_ production, even though it uses it in an object literal in one of the motivation examples. I filed https://github.com/tc39/proposal-object-rest-spread/issues/41

Comment 15

[André Bargull [:anba]]

(In reply to Shu-yu Guo [:shu] from comment #14)
> The spec text is actually missing extensions to the _ObjectLiteral_
> production, even though it uses it in an object literal in one of the
> motivation examples. I filed
> https://github.com/tc39/proposal-object-rest-spread/issues/41

The new spread syntax is part of the PropertyDefinition production https://tc39.github.io/proposal-object-rest-spread/#prod-PropertyDefinition.

Comment 16

[André Bargull [:anba]]

(In reply to André Bargull from comment #10)
> I should probably file an issue at the proposal repository to forbid array
> patterns in rest-property positions (var {...[]} = o), because that's not
> useful to have at all.

Filed https://github.com/tc39/proposal-object-rest-spread/issues/43.

Comment 17

[Shu-yu Guo [:shu]]

Comment on attachment 8860909 [details] [diff] [review]
bug1339395-part3-bytecode-emitter.patch

Review of attachment 8860909 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: js/src/builtin/Utilities.js
@@ +238,5 @@
>  
> +// Object Rest/Spread Properties proposal
> +// Abstract operation: CopyDataProperties (target, source, excluded)
> +function CopyDataProperties(target, source, excluded) {
> +    // Step 1.

All the step numbers are 1 off from the current revision of the spec.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5836,5 @@
>  
>      if (!emitRequireObjectCoercible())                            // ... RHS
>          return false;
>  
> +    bool needsExcludedSetForSpread = false;

In the case of ... on the LHS of assignment, the terminology seems to be "rest" instead of "spread".

@@ +5841,2 @@
>      for (ParseNode* member = pattern->pn_head; member; member = member->pn_next) {
> +        if (member->isKind(PNK_SPREAD) && member != pattern->pn_head)

Could you just check pattern->pn_tail here?

@@ +5901,5 @@
> +            // Destructure TARGET per this member's lhs.
> +            if (!emitSetOrInitializeDestructuring(lhs, flav))     // ... RHS
> +                return false;
> +
> +            continue;

The rest property should always be the last one, could assert that and break.

@@ +5979,5 @@
> +    MOZ_ASSERT(pattern->isKind(PNK_OBJECT));
> +    MOZ_ASSERT(pattern->isArity(PN_LIST));
> +
> +    // Try to construct the shape of the object as we go, so we can emit a
> +    // JSOP_NEWOBJECT with the final shape instead.

I like the optimization you do here. This high-level comment should be expanded:

In the case of computed property names and weird internal-to-TI indices (what are these?), we cannot fix the shape at bytecode compile time. When the shape cannot be determined, |obj| is nulled out.

@@ +5986,5 @@
> +    if (!emitNewInit(JSProto_Object))
> +        return false;
> +
> +    // No need to do any guessing for the object kind, since we know exactly
> +    // how many properties we plan to have.

The code is fine because it'll overestimate at worst, but since there can be duplicate property keys, we don't know the *exact* number. :)

@@ +6073,5 @@
> +        code[0] = JSOP_NEWOBJECT;
> +        code[1] = jsbytecode(index >> 24);
> +        code[2] = jsbytecode(index >> 16);
> +        code[3] = jsbytecode(index >> 8);
> +        code[4] = jsbytecode(index);

How scary.

@@ +6946,5 @@
>  bool
> +BytecodeEmitter::emitCopyDataProperties(CopyOption option)
> +{
> +#ifdef DEBUG
> +    auto depth = this->stackDepth;

Use mozilla::DebugOnly.

Comment 18

[André Bargull [:anba]]

(In reply to Shu-yu Guo [:shu] from comment #17)
> @@ +5979,5 @@
> > +    MOZ_ASSERT(pattern->isKind(PNK_OBJECT));
> > +    MOZ_ASSERT(pattern->isArity(PN_LIST));
> > +
> > +    // Try to construct the shape of the object as we go, so we can emit a
> > +    // JSOP_NEWOBJECT with the final shape instead.
> 
> I like the optimization you do here. This high-level comment should be
> expanded:
> 
> In the case of computed property names and weird internal-to-TI indices
> (what are these?), we cannot fix the shape at bytecode compile time. When
> the shape cannot be determined, |obj| is nulled out.

The check for internal-to-TI indices was added in bug 921902, because it seems like at that time it was possible to have string jsids which are indices. (That's what I assume based on the comments and patches in bug 1052491.) 
When I replaced this check with an assertion, I didn't get any errors from either jstests or jit-tests, so I guess we no longer need it and it should be removed.

Comment 19

[André Bargull [:anba]]

As a further improvement, we could reduce the byte code size resp. number of instructions.

With the current patch, we're emitting these byte codes for |({...r}=source)|.

00000:  dup                             # source source
00001:  newinit 1                       # source source OBJ
00006:  dup                             # source source OBJ OBJ
00007:  pick 2                          # source OBJ OBJ source
00009:  getintrinsic "CopyDataPropertiesUnfiltered" # source OBJ OBJ source CopyDataPropertiesUnfiltered
00014:  undefined                       # source OBJ OBJ source CopyDataPropertiesUnfiltered undefined
00015:  pick 3                          # source OBJ source CopyDataPropertiesUnfiltered undefined OBJ
00017:  pick 3                          # source OBJ CopyDataPropertiesUnfiltered undefined OBJ source
00019:  call-ignores-rv 2               # source OBJ CopyDataPropertiesUnfiltered(...)
00022:  pop                             # source OBJ


To reduce the byte code size by two:

00000:  dup                             # source source
00001:  newinit 1                       # source source OBJ
00006:  dup                             # source source OBJ OBJ
00007:  getintrinsic "CopyDataPropertiesUnfiltered" # source source OBJ OBJ CopyDataPropertiesUnfiltered
00012:  undefined                       # source source OBJ OBJ CopyDataPropertiesUnfiltered undefined
00014:  pick 2                          # source source OBJ CopyDataPropertiesUnfiltered undefined OBJ
00016:  pick 4                          # source OBJ CopyDataPropertiesUnfiltered undefined OBJ source
00019:  call-ignores-rv 2               # source OBJ CopyDataPropertiesUnfiltered(...)
00020:  pop                             # source OBJ


To reduce the byte code instructions by three:

00000:  newinit 1                       # source OBJ
00005:  getintrinsic "CopyDataPropertiesUnfiltered" # source OBJ CopyDataPropertiesUnfiltered
00010:  undefined                       # source OBJ CopyDataPropertiesUnfiltered undefined
00011:  dupat 2                         # source OBJ CopyDataPropertiesUnfiltered undefined OBJ
00015:  dupat 4                         # source OBJ CopyDataPropertiesUnfiltered undefined OBJ source
00019:  call-ignores-rv 2               # source OBJ CopyDataPropertiesUnfiltered(...)
00022:  pop                             # source OBJ

Comment 20

[André Bargull [:anba]]

Attachment: bug1339395-part3-bytecode-emitter.patch

Update part 3 per review comments, carrying r+.

Additionally I've moved the byte code rewriting from JSOP_NEWINIT to JSOP_NEWOBJECT into a new method, so it can be shared by BytecodeEmitter::emitObject() and BytecodeEmitter::emitDestructuringObjRestExclusionSet().

Comment 21

[André Bargull [:anba]]

Attachment: bug1339395-part7-remove-ti-index.patch

This removes the stale checks to ensure jsid strings aren't indices (they are no longer indices by construction, so we can safely remove the checks). The checks were originally added in bug 921902.

Comment 22

[André Bargull [:anba]]

(In reply to Shu-yu Guo [:shu] from comment #17)
> @@ +5841,2 @@
> >      for (ParseNode* member = pattern->pn_head; member; member = member->pn_next) {
> > +        if (member->isKind(PNK_SPREAD) && member != pattern->pn_head)
> 
> Could you just check pattern->pn_tail here?

Oh, definitely. 

> 
> @@ +5979,5 @@
> > +    MOZ_ASSERT(pattern->isKind(PNK_OBJECT));
> > +    MOZ_ASSERT(pattern->isArity(PN_LIST));
> > +
> > +    // Try to construct the shape of the object as we go, so we can emit a
> > +    // JSOP_NEWOBJECT with the final shape instead.
> 
> I like the optimization you do here.

It gave a noticeable performance improvement to have this optimization, so I copied it over from emitObject()/emitPropertyList(). 

> How scary.

Agreed! :-)

Comment 23

[André Bargull [:anba]]

Attachment: bug1339395-part3-bytecode-emitter.patch

Attached wrong patch, now with correct rooting.

Comment 24

[André Bargull [:anba]]

Attachment: bug1339395-part7-remove-ti-index.patch

Had to rebase the patch because I worked with the wrong version for part 3.

Comment 25

[Shu-yu Guo [:shu]]

Comment on attachment 8862509 [details] [diff] [review]
bug1339395-part7-remove-ti-index.patch

Review of attachment 8862509 [details] [diff] [review]:
-----------------------------------------------------------------

I mean I'll trust that the reason for this was symbols at some point? If nothing fails on the JS tests or on try, that's good enough for me to remove.

Comment 26

[André Bargull [:anba]]

(In reply to Shu-yu Guo [:shu] from comment #25)
> I mean I'll trust that the reason for this was symbols at some point? If
> nothing fails on the JS tests or on try, that's good enough for me to remove.

Try didn't show up anything and neither did local tests when I replaced the now removed branch with a MOZ_ASSERTION.

Comment 27

[André Bargull [:anba]]

Attachment: bug1339395-part2-parse-object-rest-spread.patch

Change objectBindindingPattern() to call addSpreadProperty() instead of addSpreadElement(), otherwise identical patch. Carrying r+.

Comment 28

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee31ecebe8180c70fc6f15b22d53f5353e2c8dcd

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/391306755ce6
Part 1: Align parse method for object literals to match array literals. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/c842e4b4dcb6
Part 2: Add parser support for rest and spread object properties. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b106f6bbb5
Part 3: Add BytecodeEmitter support for object rest and spread properties. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/69c6bbd9a508
Part 4: Enable test262 for object rest and spread properties. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/11af11887bad
Part 5: Add Reflect.parse tests for object rest and spread properties. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/09433b48aded
Part 6: Update jit-tests now that object rest/spread properties are a thing. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/410ac5a97d5e
Part 7: Remove no longer needed check for jsid strings which are indices from frontend. r=shu

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing Linux cgc's wasm-12.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/38f8077b0476919e367550d4a5522930adb319a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc1597126e09ab37d58dbd3ba0a35397aa456a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea538919eeca0d6e542b6e3a9168e50c407f009
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5fcb658f2bd570ecd6584406f06418e9072df60
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b934c38fc755269e5130ecbd0bc56fb18d0c77
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c99414f066b2dcba2350a6ae44a9c5240f8141
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e8d1d1ee323b8964269d0a319ada3f57c6cef54

Push showing the failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=de04eca510ed09012bd46b1bf3625fb5107d9dfe&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=95222007&repo=mozilla-inbound

[task 2017-04-28T15:04:27.918701Z] TEST-PASS | js/src/jit-test/tests/debug/wasm-11.js | Success (code 0, args "")
[task 2017-04-28T15:04:27.934944Z] TEST-PASS | js/src/jit-test/tests/debug/wasm-05.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off")
[task 2017-04-28T15:04:28.028864Z] TEST-PASS | js/src/jit-test/tests/debug/wasm-11.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off")
[task 2017-04-28T15:04:28.198327Z] TEST-PASS | js/src/jit-test/tests/debug/wasm-11.js | Success (code 0, args "--baseline-eager")
[task 2017-04-28T15:04:28.432819Z] TEST-PASS | js/src/jit-test/tests/debug/wasm-12.js | Success (code 0, args "")
[task 2017-04-28T15:04:28.704733Z] /home/worker/workspace/build/src/js/src/jit-test/tests/debug/wasm-12.js:23:1 Error: Assertion failed: got 1, expected 2
[task 2017-04-28T15:04:28.704800Z] Stack:
[task 2017-04-28T15:04:28.704855Z]   @/home/worker/workspace/build/src/js/src/jit-test/tests/debug/wasm-12.js:23:1
[task 2017-04-28T15:04:28.704880Z] Exit code: 3
[task 2017-04-28T15:04:28.704904Z] FAIL - debug/wasm-12.js
[task 2017-04-28T15:04:28.704984Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/wasm-12.js | /home/worker/workspace/build/src/js/src/jit-test/tests/debug/wasm-12.js:23:1 Error: Assertion failed: got 1, expected 2 (code 3, args "--baseline-eager")
[task 2017-04-28T15:04:28.705032Z] INFO exit-status     : 3
[task 2017-04-28T15:04:28.705067Z] INFO timed-out       : False
[task 2017-04-28T15:04:28.705134Z] INFO stderr         2> /home/worker/workspace/build/src/js/src/jit-test/tests/debug/wasm-12.js:23:1 Error: Assertion failed: got 1, expected 2
[task 2017-04-28T15:04:28.705169Z] INFO stderr         2> Stack:
[task 2017-04-28T15:04:28.705220Z] INFO stderr         2> @/home/worker/workspace/build/src/js/src/jit-test/tests/debug/wasm-12.js:23:1

Comment 31

[André Bargull [:anba]]

Looks like an issue with the test-file itself. It assigns both wasm modules to the same identifier, so if there's a GC the first wasm module may already be collected. This patch adds a new function to the self-hosted global, which can affect GC timings, which may explain why we now see this failure.

Comment 32

[Luke Wagner [:luke]]

Looks like Debugger.findScripts() is weak and therefore non-deterministically returns otherwise-unreachable objects.  I think the right fix here is to give those two modules a separate var (which is what the test should have originally done); otherwise other people will randomly hit this.

Comment 33

[André Bargull [:anba]]

Attachment: bug1339395-part8-wasm-debugger-test.patch

Create two different global variables for the wasm modules, so the GC won't remove the first one.

Comment 34

[Leo Balter]

I updated the skip list on the jstests.list, sending in a patch soon.

Comment 35

[Luke Wagner [:luke]]

Comment on attachment 8863919 [details] [diff] [review]
bug1339395-part8-wasm-debugger-test.patch

Review of attachment 8863919 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks and sorry for the hassle!

Comment 36

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a2c17c2cd7aede75165db935e2209f215e8e67

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ccc011304aa
Part 1: Align parse method for object literals to match array literals. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/66ad093f7e6a
Part 2: Add parser support for rest and spread object properties. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6c489432f11
Part 3: Add BytecodeEmitter support for object rest and spread properties. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d8c48c7e4f6
Part 4: Enable test262 for object rest and spread properties. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2dd7f08f77
Part 5: Add Reflect.parse tests for object rest and spread properties. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1b369f61b8
Part 6: Update jit-tests now that object rest/spread properties are a thing. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/81097689944a
Part 7: Remove no longer needed check for jsid strings which are indices from frontend. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/9af2b54dc7bd
Part 8: Add separate variables for wasm debugger test. r=luke

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ccc011304aa
https://hg.mozilla.org/mozilla-central/rev/66ad093f7e6a
https://hg.mozilla.org/mozilla-central/rev/b6c489432f11
https://hg.mozilla.org/mozilla-central/rev/4d8c48c7e4f6
https://hg.mozilla.org/mozilla-central/rev/fd2dd7f08f77
https://hg.mozilla.org/mozilla-central/rev/cf1b369f61b8
https://hg.mozilla.org/mozilla-central/rev/81097689944a
https://hg.mozilla.org/mozilla-central/rev/9af2b54dc7bd

Comment 39

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/55#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#Spread_properties
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment