Implement Object Rest/Spread Properties proposal

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: arai, Assigned: anba)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

Trunk
mozilla55
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(9 attachments, 4 obsolete attachments)

22.30 KB, patch
shu
: review+
Details | Diff | Splinter Review
11.98 KB, patch
Details | Diff | Splinter Review
26.37 KB, patch
shu
: review+
Details | Diff | Splinter Review
4.20 KB, patch
shu
: review+
Details | Diff | Splinter Review
1.07 KB, patch
shu
: review+
Details | Diff | Splinter Review
26.25 KB, patch
anba
: review+
Details | Diff | Splinter Review
5.84 KB, patch
shu
: review+
Details | Diff | Splinter Review
10.18 KB, patch
anba
: review+
Details | Diff | Splinter Review
1.09 KB, patch
luke
: review+
Details | Diff | Splinter Review
Keywords: dev-doc-needed
Blocks: 652780
(Assignee)

Comment 2

a year ago
Created attachment 8860905 [details] [diff] [review]
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.)
Attachment #8860905 - Flags: review?(shu)
(Assignee)

Comment 3

a year ago
Created attachment 8860906 [details] [diff] [review]
bug1339395-part1-ignore-whitespace.patch (not for checkin)

Part 1 with --ignore-whitespace.
(Assignee)

Comment 4

a year ago
Created attachment 8860908 [details] [diff] [review]
bug1339395-part2-parse-object-rest-spread.patch

Adds parser support for object rest and spread properties. (I still need to write Reflect.parse tests!)
Attachment #8860908 - Flags: review?(shu)
(Assignee)

Comment 5

a year ago
Created attachment 8860909 [details] [diff] [review]
bug1339395-part3-bytecode-emitter.patch

Do you think this is the correct approach for an initial implementation of this feature?
Attachment #8860909 - Flags: review?(shu)
(Assignee)

Comment 6

a year ago
Created attachment 8860910 [details] [diff] [review]
bug1339395-part4-enable-test262.patch

Enables the test262 tests.
Attachment #8860910 - Flags: review?(shu)
(Assignee)

Comment 7

a year ago
Created attachment 8861038 [details] [diff] [review]
bug1339395-part5-reflect-parse.patch

Basic tests for Reflect.parse.
Attachment #8861038 - Flags: review?(shu)
(Assignee)

Comment 8

a year ago
Created attachment 8861055 [details] [diff] [review]
bug1339395-part6-update-jit-test.patch

The error column numbers need to be update for one jit-test.
Attachment #8861055 - Flags: review?(shu)
(Assignee)

Comment 10

a year ago
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

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

Comment 12

a year ago
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.
Attachment #8860905 - Flags: review?(shu) → review+

Comment 13

a year ago
(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

a year ago
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
Attachment #8860908 - Flags: review?(shu) → review+
(Assignee)

Comment 15

a year ago
(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.
(Assignee)

Comment 16

a year ago
(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

a year ago
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.
Attachment #8860909 - Flags: review?(shu) → review+

Updated

a year ago
Attachment #8860910 - Flags: review?(shu) → review+

Updated

a year ago
Attachment #8861038 - Flags: review?(shu) → review+

Updated

a year ago
Attachment #8861055 - Flags: review?(shu) → review+
(Assignee)

Comment 18

a year ago
(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.
(Assignee)

Comment 19

a year ago
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
(Assignee)

Comment 20

a year ago
Created attachment 8862490 [details] [diff] [review]
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().
Attachment #8860909 - Attachment is obsolete: true
Attachment #8862490 - Flags: review+
(Assignee)

Comment 21

a year ago
Created attachment 8862491 [details] [diff] [review]
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.
Attachment #8862491 - Flags: review?(shu)
(Assignee)

Comment 22

a year ago
(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! :-)
(Assignee)

Comment 23

a year ago
Created attachment 8862508 [details] [diff] [review]
bug1339395-part3-bytecode-emitter.patch

Attached wrong patch, now with correct rooting.
Attachment #8862490 - Attachment is obsolete: true
Attachment #8862508 - Flags: review+
(Assignee)

Comment 24

a year ago
Created attachment 8862509 [details] [diff] [review]
bug1339395-part7-remove-ti-index.patch

Had to rebase the patch because I worked with the wrong version for part 3.
Attachment #8862491 - Attachment is obsolete: true
Attachment #8862491 - Flags: review?(shu)
Attachment #8862509 - Flags: review?(shu)

Comment 25

a year ago
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.
Attachment #8862509 - Flags: review?(shu) → review+
(Assignee)

Comment 26

a year ago
(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.
(Assignee)

Comment 27

a year ago
Created attachment 8862769 [details] [diff] [review]
bug1339395-part2-parse-object-rest-spread.patch

Change objectBindindingPattern() to call addSpreadProperty() instead of addSpreadElement(), otherwise identical patch. Carrying r+.
Assignee: nobody → andrebargull
Attachment #8860908 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8862769 - Flags: review+

Comment 29

a year ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
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
Flags: needinfo?(andrebargull)
(Assignee)

Comment 31

a year ago
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.
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.
(Assignee)

Comment 33

a year ago
Created attachment 8863919 [details] [diff] [review]
bug1339395-part8-wasm-debugger-test.patch

Create two different global variables for the wasm modules, so the GC won't remove the first one.
Flags: needinfo?(andrebargull)
Attachment #8863919 - Flags: review?(luke)

Comment 34

a year ago
I updated the skip list on the jstests.list, sending in a patch soon.
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!
Attachment #8863919 - Flags: review?(luke) → review+

Comment 37

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
status-firefox54: affected → ---

Updated

11 months ago
Depends on: 1375689
Blocks: 1435799
You need to log in before you can comment on or make changes to this bug.