Bug 167962 – [ESnext] Implement Object Rest - Implementing Object Rest Destructuring

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

Object Rest/Spread Properties for ECMAScript proposal now is in stage 3 and we can implement a prototype of it.

Official proposal: https://github.com/sebmarkbage/ecmascript-rest-spread

This new feature enable the following source code to be parsed and executed:
```
let { x, y, ...z } = { x: 1, y: 2, a: 3, b: 4 };
x; // 1
y; // 2
z; // { a: 3, b: 4 }
```

[Caio Lima]

Created attachment 300994 [details]
WIP - Patch

It just starts the implementation. A lot of optimizations need to be done yet and it is not following full specification. I'm going to improve excludedList verification performance using set instead of array.

[WebKit Commit Bot]

Attachment 300994 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4043:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 300994 [details]
WIP - Patch

Attachment 300994 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3029145

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 300998 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300994 [details]
WIP - Patch

Attachment 300994 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3029332

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 301001 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300994 [details]
WIP - Patch

Attachment 300994 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3029322

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 301002 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 300994 [details]
WIP - Patch

Attachment 300994 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3029633

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 301003 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Caio Lima]

Created attachment 301324 [details]
Patch

[WebKit Commit Bot]

Attachment 301324 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

(In reply to comment #11)
> Created attachment 301324 [details]
> Patch

This is the proposal for this implementation. I'm thinking in put it behind a flag. Performance comparison is coming soon.

[Caio Lima]

Created attachment 302011 [details]
Benchmark results

These changes are neutral.

[Keith Miller]

Comment on attachment 301324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301324&action=review

I think it looks pretty good. I have a few comments, however.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4051
> +            if (UNLIKELY(m_containsRestElement)) {
> +                RefPtr<RegisterID> propertyName = generator.emitLoad(nullptr, target.propertyName);
> +                generator.emitDirectPutByVal(excludedList.get(), excludedIndex.get(), propertyName.get());
> +                generator.emitInc(excludedIndex.get());

Don't we statically know what we are going to put in the excludedList at this point? Can we just add this array to the constant pool? If we do, we should probably freeze so the array can't be modified.

> Source/JavaScriptCore/parser/Parser.cpp:1019
> +                if (kind == DestructuringKind::DestructureToExpressions && !innerPattern)
> +                    return 0;
> +                failIfFalse(innerPattern, "Cannot parse this destructuring pattern");

I think you just want to do propageError(); here

> Source/JavaScriptCore/runtime/JSGlobalObject.h:117
> +    macro(Set, set, set, JSSet, Set, object) \

Why did you move this?

[Caio Lima]

Comment on attachment 301324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301324&action=review

>> Source/JavaScriptCore/parser/Parser.cpp:1019
>> +                failIfFalse(innerPattern, "Cannot parse this destructuring pattern");
> 
> I think you just want to do propageError(); here

Sure. This way we keep the error message more clear.

>> Source/JavaScriptCore/runtime/JSGlobalObject.h:117
>> +    macro(Set, set, set, JSSet, Set, object) \
> 
> Why did you move this?

I'm using it in copyPropertiesData (Source/JavaScriptCore/builtins/GlobalOperations.js) because it makes the algorithm O(n + m) where n = Object.getOwnPropertyNames(source).length and m = excludedList.length. Does it make sense?

[Caio Lima]

Comment on attachment 301324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301324&action=review

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4051
>> +                generator.emitInc(excludedIndex.get());
> 
> Don't we statically know what we are going to put in the excludedList at this point? Can we just add this array to the constant pool? If we do, we should probably freeze so the array can't be modified.

Yes. I agree. However how can we proceed with that? Is it possible to create an JSArray without ExecState? I also have the same question for JSSet. If there is a way to create them here, please let me know. I already know how to get identifiers as constant, but should be good how could I allocate constant Array (better if it is a JSSet).

[Caio Lima]

Created attachment 302774 [details]
Patch

[WebKit Commit Bot]

Attachment 302774 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

(In reply to comment #18)
> Created attachment 302774 [details]
> Patch

Updated implementation using constant pool to HashSets. Let's see what EWS thinks about it.

[Keith Miller]

Comment on attachment 302774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302774&action=review

I think it's looking pretty good, I have a couple of style comments, however.

> Source/JavaScriptCore/builtins/GlobalOperations.js:95
> +        if (!excludedSet.has(nextKey)) {

I think you need to make private symbols for these properties. If a user deletes the has property from Set.prototype your code won't work.

> Source/JavaScriptCore/builtins/GlobalOperations.js:99
> +                @Object.defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});

ditto.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1944
> +    if (!setConstantDeconstructionSetRegisters(vm, unlinkedCodeBlock->constantDestructSets()))

I would call this setConstantIdentifierSetRegisters.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:71
> +typedef std::pair<HashSet<UniquedStringImpl*>, unsigned> ConstantDeconstructSetEntry;

I would call this ConstantIndentifierSetEntry

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:230
> +    const Vector<ConstantDeconstructSetEntry>& constantDestructSets() { return m_constantObjDeconstructSets; }

Style: We normally don't abbreviate names like this.

but I would also change this to constantIdentifierSets().

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:461
> +    Vector<ConstantDeconstructSetEntry> m_constantObjDeconstructSets;

ditto.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1927
> +        if (!(entry.first == set))

I would do if (entry.first != set), although you might need to add an operator != to HashSet.

> Source/JavaScriptCore/runtime/ScriptExecutable.cpp:193
> +            // At this point, the CodeBlock creation can fail trying to
> +            // allocate JSSet constants in CodeBlock::setConstantDeconstructionSetRegister.
> +            // Since the only exception from it is OOM, we throw this kind of exception.

I don't think you need this comment.

[Caio Lima]

Created attachment 303408 [details]
Patch

[WebKit Commit Bot]

Attachment 303408 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

(In reply to comment #22)
> Created attachment 303408 [details]
> Patch

Thank you very much for the review Keith. Updated based in you comments. Let's see what EWS thinks about it.

[Build Bot]

Comment on attachment 303408 [details]
Patch

Attachment 303408 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3242966

New failing tests:
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-dfg-eager-no-cjit
stress/object-rest-deconstruct.js.dfg-maximal-flush-validate-no-cjit
stress/object-rest-deconstruct.js.ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-no-cjit
stress/object-rest-deconstruct.js.no-ftl
stress/object-rest-deconstruct.js.dfg-eager-no-cjit-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-ftl
stress/object-rest-deconstruct.js.ftl-no-cjit-small-pool
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-no-cjit-no-inline-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-llint
stress/object-rest-deconstruct.js.ftl-no-cjit-validate-sampling-profiler
stress/object-rest-deconstruct.js.ftl-no-cjit-no-put-stack-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout
stress/object-rest-deconstruct.js.default
stress/object-rest-deconstruct.js.no-cjit-collect-continuously
stress/object-rest-deconstruct.js.no-cjit-validate-phases
stress/object-rest-deconstruct.js.no-llint
stress/object-rest-deconstruct.js.dfg-eager

[Build Bot]

Comment on attachment 303408 [details]
Patch

Attachment 303408 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3243039

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 303411 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303408 [details]
Patch

Attachment 303408 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3243050

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 303412 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303408 [details]
Patch

Attachment 303408 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3243052

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 303413 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303408 [details]
Patch

Attachment 303408 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3243053

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 303415 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Caio Lima]

Created attachment 303438 [details]
Patch

[WebKit Commit Bot]

Attachment 303438 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Ping review

[Keith Miller]

Comment on attachment 303438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303438&action=review

One more, small set of changes then the patch should be gtg. Thanks for following through on this!

> Source/JavaScriptCore/builtins/GlobalOperations.js:94
> +    for (let nextKey of keys) {

I don't think this can be a for of loop since it uses the iterator protocol, which is, sadly, observable. Instead you should do an indexed loop. Getting the length property of keys should be fine since it's an array.

For some reason I thought I made this comment last time but I guess I forgot :(.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2410
> +    for (size_t i = 0; i < count; i++) {

I think you could do:
for (const auto& entry : constants)

Your current code will use the copy constructor of HashSet, which we can avoid.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2418
> +        HashSet<UniquedStringImpl*> set = entry.first;

you can make this:
 const HashSet<UniquedStringImpl*>& set = entry.first; 

for the same reason as above.

[Caio Lima]

Comment on attachment 303438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303438&action=review

Thank you very much for the reviews

>> Source/JavaScriptCore/builtins/GlobalOperations.js:94
>> +    for (let nextKey of keys) {
> 
> I don't think this can be a for of loop since it uses the iterator protocol, which is, sadly, observable. Instead you should do an indexed loop. Getting the length property of keys should be fine since it's an array.
> 
> For some reason I thought I made this comment last time but I guess I forgot :(.

You are right. Thanks catching it. Changed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2410
>> +    for (size_t i = 0; i < count; i++) {
> 
> I think you could do:
> for (const auto& entry : constants)
> 
> Your current code will use the copy constructor of HashSet, which we can avoid.

Agreed and changed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2418
>> +        HashSet<UniquedStringImpl*> set = entry.first;
> 
> you can make this:
>  const HashSet<UniquedStringImpl*>& set = entry.first; 
> 
> for the same reason as above.

Ditto.

[Caio Lima]

Created attachment 303896 [details]
Patch

[WebKit Commit Bot]

Attachment 303896 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2143:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 303896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303896&action=review

r=me.

> Source/JavaScriptCore/builtins/GlobalOperations.js:92
> +    let from = @toObject(source);

Nit: You could just do:
@Object(source)
here but this is probably fine.

[Caio Lima]

Comment on attachment 303896 [details]
Patch

Thank you very much for the review Keith!

[WebKit Commit Bot]

Comment on attachment 303896 [details]
Patch

Clearing flags on attachment: 303896

Committed r213697: <http://trac.webkit.org/changeset/213697>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 303896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303896&action=review

> Source/JavaScriptCore/parser/Parser.cpp:1017
> +                if (kind == DestructuringKind::DestructureToExpressions && !innerPattern)

This line of code looks wrong. Why do we only care about DestructureToExpressions?

[Caio Lima]

Comment on attachment 303896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303896&action=review

>> Source/JavaScriptCore/parser/Parser.cpp:1017
>> +                if (kind == DestructuringKind::DestructureToExpressions && !innerPattern)
> 
> This line of code looks wrong. Why do we only care about DestructureToExpressions?

I think you are right. We should care about other cases as well IMHO. I'm going to handle that in another bug.

Also I don't remember any test to check the condition when innerPatter != null. We are missing this piece of code.

[Mark Lam]

I'm going to roll this out.  Let's re-land after we have a proper fix all the issues.

[Mark Lam]

This has been rolled out in r216891: <http://trac.webkit.org/r216891>.

[Caio Lima]

Created attachment 310219 [details]
WIP - Patch RFC

The problem of last implementation is that it was considering that excludedList was always statically defined, but the example below shows that it's not true:

```
let a = "foo";
let {[a]: b, ...r} = {foo: 2, bar 3};
// b == 2;
// r.bar == 3;
```

This way, I decided change my approach and emit bytecode to fill excludedList, and then now we can carry the case above properly.
I added the test was in our suite but I would like to check EWS and get feedback as well here.

The previous implementations was faster and I'm thinking in a way to allow static usage that probably will the common case for this feature, but I think it's worth it bring it in another Patch. 

Also, I think it's a good idea put this feature behind a flag before commit.

[Build Bot]

Attachment 310219 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 310219 [details]
WIP - Patch RFC

Attachment 310219 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3748187

New failing tests:
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout
stress/object-rest-deconstruct.js.dfg-eager-no-cjit-validate
stress/object-rest-deconstruct.js.no-ftl
stress/object-rest-deconstruct.js.no-llint
stress/object-rest-deconstruct.js.ftl-eager-no-cjit-b3o1
stress/object-rest-deconstruct.js.ftl-eager
stress/object-rest-deconstruct.js.ftl-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-ftl
stress/object-rest-deconstruct.js.ftl-no-cjit-small-pool
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-no-cjit-no-inline-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-dfg-eager-no-cjit
stress/object-rest-deconstruct.js.default
stress/object-rest-deconstruct.js.dfg-maximal-flush-validate-no-cjit
stress/object-rest-deconstruct.js.dfg-eager
stress/object-rest-deconstruct.js.ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-no-cjit-validate-sampling-profiler
stress/object-rest-deconstruct.js.ftl-no-cjit-no-put-stack-validate
stress/object-rest-deconstruct.js.no-cjit-collect-continuously
stress/object-rest-deconstruct.js.no-cjit-validate-phases

[Caio Lima]

Created attachment 310256 [details]
WIP - Patch RFC

Fixing failures in EWS.

[Build Bot]

Attachment 310256 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 310256 [details]
WIP - Patch RFC

View in context: https://bugs.webkit.org/attachment.cgi?id=310256&action=review

> Source/JavaScriptCore/parser/Nodes.h:2133
> -            m_targetPatterns.append(Entry { Identifier(), propertyExpression, false, pattern, defaultValue });
> +            m_targetPatterns.append(Entry{ Identifier(), propertyExpression, false, pattern, defaultValue, bindingType });

Don't pass a stack instance of Identifier here.  Instead, you should have appendEntry() take a VM&, and pass vm.propertyNames->nullIdentifier here instead.

[Saam Barati]

(In reply to Caio Lima from comment #49)
> Created attachment 310219 [details]
> WIP - Patch RFC
> 
> The problem of last implementation is that it was considering that
> excludedList was always statically defined, but the example below shows that
> it's not true:
> 
> ```
> let a = "foo";
> let {[a]: b, ...r} = {foo: 2, bar 3};
> // b == 2;
> // r.bar == 3;
> ```
> 
> This way, I decided change my approach and emit bytecode to fill
> excludedList, and then now we can carry the case above properly.
> I added the test was in our suite but I would like to check EWS and get
> feedback as well here.
This sounds right to me when we have computed properties. However, I think it's a good optimization to build the list at compile time when we know all the properties statically. So I think we should have a combination of the old code and the new code depending on some simple static analysis.

> 
> The previous implementations was faster and I'm thinking in a way to allow
> static usage that probably will the common case for this feature, but I
> think it's worth it bring it in another Patch. 
> 
> Also, I think it's a good idea put this feature behind a flag before commit.
Yeah this sounds good. You could probably just put the feature check in the parser.

[Caio Lima]

Created attachment 310484 [details]
Patch

Fixed Mark's and Saam's comments. I also created the build flags. Let's see EWS results and it's also ready for another review round.

[Build Bot]

Attachment 310484 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/mac/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig). Use sync-feature-defines if possible.  [featuredefines/equality] [5]
ERROR: Source/WebCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig). Use sync-feature-defines if possible.  [featuredefines/equality] [5]
ERROR: Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig). Use sync-feature-defines if possible.  [featuredefines/equality] [5]
ERROR: Tools/TestWebKitAPI/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig). Use sync-feature-defines if possible.  [featuredefines/equality] [5]
ERROR: Source/JavaScriptCore/parser/ASTBuilder.h:953:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebKit2/Configurations/FeatureDefines.xcconfig:0:  Any changes made to FeatureDefines should be made to all of them (changed file does not match Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig). Use sync-feature-defines if possible.  [featuredefines/equality] [5]
ERROR: Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4101:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 9 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 310484 [details]
Patch

Attachment 310484 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3766894

New failing tests:
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout
stress/object-rest-deconstruct.js.dfg-eager-no-cjit-validate
stress/object-rest-deconstruct.js.no-ftl
stress/object-rest-deconstruct.js.no-llint
stress/object-rest-deconstruct.js.ftl-eager-no-cjit-b3o1
stress/object-rest-deconstruct.js.ftl-eager
stress/object-rest-deconstruct.js.ftl-no-cjit-b3o1
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-ftl
stress/object-rest-deconstruct.js.ftl-no-cjit-small-pool
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-no-cjit-no-inline-validate
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-dfg-eager-no-cjit
stress/object-rest-deconstruct.js.default
stress/object-rest-deconstruct.js.dfg-maximal-flush-validate-no-cjit
stress/object-rest-deconstruct.js.dfg-eager
stress/object-rest-deconstruct.js.ftl-eager-no-cjit
stress/object-rest-deconstruct.js.ftl-no-cjit-validate-sampling-profiler
stress/object-rest-deconstruct.js.ftl-no-cjit-no-put-stack-validate
stress/object-rest-deconstruct.js.no-cjit-collect-continuously
stress/object-rest-deconstruct.js.no-cjit-validate-phases

[Build Bot]

Comment on attachment 310484 [details]
Patch

Attachment 310484 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3767437

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 310495 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310484 [details]
Patch

Attachment 310484 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3767439

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 310496 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310484 [details]
Patch

Attachment 310484 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3767443

New failing tests:
js/parser-syntax-check.html

[Build Bot]

Created attachment 310498 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Caio Lima]

Created attachment 310510 [details]
Patch

Fixed flag configuration.

[Build Bot]

Attachment 310510 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/ASTBuilder.h:953:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4101:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 4 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Created attachment 310597 [details]
Patch

This version is disabling the recursive destructuring and in this implementation the following code results in syntax error:

```
let {a, ...{b}} = {a: 1, b: 2};
```

[Build Bot]

Attachment 310597 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 310597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310597&action=review

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:78
> +ENABLE_ESNEXT_OBJ_REST_SPREAD = ENABLE_ESNEXT_OBJ_REST_SPREAD;

Lets make it a runtime option, not a compile time one.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:890
> +        m_constantRegisters[entry.second].set(vm, this, JSValue(jsSet));

I don't think there is a need for the JSValue(jsSet) here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1967
> +    unsigned index = m_nextConstantOffset;
> +    m_constantPoolRegisters.append(FirstConstantRegisterIndex + m_nextConstantOffset);
> +    ++m_nextConstantOffset;
> +    m_codeBlock->addSetConstant(set);
> +    RegisterID* m_setRegister = &m_constantPoolRegisters[index];

It looks like we do this work in a few places, perhaps we can write a function that abstracts it, like:
```
unsigned addConstantIndex()
{
    unsigned index = m_nextConstantOffset;
    m_constantPoolRegisters.append(FirstConstantRegisterIndex + m_nextConstantOffset);
    ++m_nextConstantOffset;
    return index;
}
or even make the function return std::pair<unsigned, RegisterID*>
```
and then you can just be like:
``` unsigned index = addConstantIndex();```

Maybe it's not worth it, but just a suggestion since I'm not a big fan of repeating nearly identical code.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4071
> +        excludedList = generator.emitNewArray(generator.newTemporary(), 0, 0);
> +        excludedIndex = generator.newTemporary();
> +        generator.emitLoad(excludedIndex.get(), jsNumber(0));

Please don't do this. You should just build the Set in bytecode. There is no need to first build an array, and then have your builtin convert the array to a set.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4079
> +                // Should not emit get_by_id for indexed ones.

No need for this comment. We have code like this all over the place.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:-4076
> -            RefPtr<RegisterID> propertyName = generator.emitNode(target.propertyExpression);

Can you add an assertion here of what the bindingType is? Also, I think it's worth adding an assertion that this is the last thing in the list, otherwise this algorithm is broken.

> Source/JavaScriptCore/parser/Parser.cpp:980
> +        if (kind == DestructuringKind::DestructureToExpressions)
> +        return 0;

bad indentation

> Source/JavaScriptCore/parser/Parser.cpp:1070
> +            if (UNLIKELY(match(DOTDOTDOT))) {

Is this syntactically required to be the last element in the destrucruting clause?
Also, no need for UNLIKELY, we're just going to negatively impact parsing code that uses this.

[Caio Lima]

Comment on attachment 310597 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310597&action=review

>> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:78
>> +ENABLE_ESNEXT_OBJ_REST_SPREAD = ENABLE_ESNEXT_OBJ_REST_SPREAD;
> 
> Lets make it a runtime option, not a compile time one.

ok

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4071
>> +        generator.emitLoad(excludedIndex.get(), jsNumber(0));
> 
> Please don't do this. You should just build the Set in bytecode. There is no need to first build an array, and then have your builtin convert the array to a set.

I also think it's better use JSSet, but there is a thing that I'm worried:

If our implementation uses JSSet directly and then user overrides the Set.prototype.add, the rest semantics will be broken.

like:

```
Set.prototype.add = () => false;

let {a, b, ...r} = {a: 1, b: 2, c: 3, d: 4};

// here "r" will contain "a" and "b", and it will be wrong.

```

Do you think it is right?

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:-4076
>> -            RefPtr<RegisterID> propertyName = generator.emitNode(target.propertyExpression);
> 
> Can you add an assertion here of what the bindingType is? Also, I think it's worth adding an assertion that this is the last thing in the list, otherwise this algorithm is broken.

ok

>> Source/JavaScriptCore/parser/Parser.cpp:980
>> +        return 0;
> 
> bad indentation

Oops.

>> Source/JavaScriptCore/parser/Parser.cpp:1070
>> +            if (UNLIKELY(match(DOTDOTDOT))) {
> 
> Is this syntactically required to be the last element in the destrucruting clause?
> Also, no need for UNLIKELY, we're just going to negatively impact parsing code that uses this.

Yes. I will remove the UNLIKELY here.

[Saam Barati]

(In reply to Caio Lima from comment #70)
> Comment on attachment 310597 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310597&action=review
> 
> >> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:78
> >> +ENABLE_ESNEXT_OBJ_REST_SPREAD = ENABLE_ESNEXT_OBJ_REST_SPREAD;
> > 
> > Lets make it a runtime option, not a compile time one.
> 
> ok
> 
> >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4071
> >> +        generator.emitLoad(excludedIndex.get(), jsNumber(0));
> > 
> > Please don't do this. You should just build the Set in bytecode. There is no need to first build an array, and then have your builtin convert the array to a set.
> 
> I also think it's better use JSSet, but there is a thing that I'm worried:
> 
> If our implementation uses JSSet directly and then user overrides the
> Set.prototype.add, the rest semantics will be broken.
> 
This is an issue in all our builtins. They take care yo use private identifiers. You’ll need to do the same here, and make sure SetPrototye sets up your private identifier to the function you want. 
> like:
> 
> ```
> Set.prototype.add = () => false;
> 
> let {a, b, ...r} = {a: 1, b: 2, c: 3, d: 4};
> 
> // here "r" will contain "a" and "b", and it will be wrong.
> 
> ```
> 
> Do you think it is right?
> 
> >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:-4076
> >> -            RefPtr<RegisterID> propertyName = generator.emitNode(target.propertyExpression);
> > 
> > Can you add an assertion here of what the bindingType is? Also, I think it's worth adding an assertion that this is the last thing in the list, otherwise this algorithm is broken.
> 
> ok
> 
> >> Source/JavaScriptCore/parser/Parser.cpp:980
> >> +        return 0;
> > 
> > bad indentation
> 
> Oops.
> 
> >> Source/JavaScriptCore/parser/Parser.cpp:1070
> >> +            if (UNLIKELY(match(DOTDOTDOT))) {
> > 
> > Is this syntactically required to be the last element in the destrucruting clause?
> > Also, no need for UNLIKELY, we're just going to negatively impact parsing code that uses this.
> 
> Yes. I will remove the UNLIKELY here.

[Saam Barati]

You also need to set up an identifier to retrieve the Set constructor from global scope.

[Caio Lima]

(In reply to Saam Barati from comment #72)
> You also need to set up an identifier to retrieve the Set constructor from
> global scope.
You are right. I didn't update it before because I was thinking it wasn't possible, but I just realized that ```copyDataProperties``` is called this way. I'm going to fix it ASAP.

[Caio Lima]

Created attachment 310831 [details]
Patch

Addressed Saam's review

[Build Bot]

Attachment 310831 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 310831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310831&action=review

I haven't read all the code yet, quick comments.

> Source/JavaScriptCore/builtins/GlobalOperations.js:99
> +            let desc = @Object.@getOwnPropertyDescriptor(from, nextKey);
> +            if (desc.enumerable && desc !== @undefined) {

You should use `@propertyIsEnumerable(from, nextKey)` instead (See Object.assign implementation). This is more efficient.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:71
> +typedef std::pair<IdentifierSet, unsigned> ConstantIndentifierSetEntry;

For newly added code, we should use `using ConstantIndentifierSetEntry = std::pair<IdentifierSet, unsigned>;` instead.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:107
> +    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().getOwnPropertyDescriptorPrivateName(), objectConstructorGetOwnPropertyDescriptor, DontEnum, 1);

It is not necessary.

> Source/JavaScriptCore/runtime/SetPrototype.cpp:71
> +    JSC_NATIVE_INTRINSIC_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().addPrivateName(), setProtoFuncAdd, DontEnum, 1, JSSetHasIntrinsic);

This is not correct. Set#add should not have JSSetHasIntrinsic.

> Source/WTF/wtf/FeatureDefines.h:64
> +

This is not necessary.

> Source/cmake/WebKitFeatures.cmake:111
> +    WEBKIT_OPTION_DEFINE(ENABLE_ESNEXT_OBJ_REST_SPREAD "Toggle ESNext Object Rest/Spread support" PRIVATE ON)

This is not necessary.

[Caio Lima]

Comment on attachment 310831 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310831&action=review

>> Source/JavaScriptCore/builtins/GlobalOperations.js:99
>> +            if (desc.enumerable && desc !== @undefined) {
> 
> You should use `@propertyIsEnumerable(from, nextKey)` instead (See Object.assign implementation). This is more efficient.

Thanks for that!

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:71
>> +typedef std::pair<IdentifierSet, unsigned> ConstantIndentifierSetEntry;
> 
> For newly added code, we should use `using ConstantIndentifierSetEntry = std::pair<IdentifierSet, unsigned>;` instead.

Done.

>> Source/JavaScriptCore/runtime/SetPrototype.cpp:71
>> +    JSC_NATIVE_INTRINSIC_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().addPrivateName(), setProtoFuncAdd, DontEnum, 1, JSSetHasIntrinsic);
> 
> This is not correct. Set#add should not have JSSetHasIntrinsic.

oops. Fixed.

>> Source/WTF/wtf/FeatureDefines.h:64
>> +
> 
> This is not necessary.

Removed.

[Caio Lima]

(In reply to Yusuke Suzuki from comment #76)
> Comment on attachment 310831 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310831&action=review
> 
Thanks for the Review!

[Caio Lima]

Created attachment 311428 [details]
Patch

Addressing Yusuki's comments.

[Caio Lima]

Created attachment 311431 [details]
Patch

Rebased with upstream code.

[Build Bot]

Attachment 311431 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 311431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311431&action=review

> JSTests/ChangeLog:3
> +        [ESnext] Implement Object Rest - Implementing Object Rest Destructuring

Does this patch do spread?

> Source/JavaScriptCore/ChangeLog:13
> +        array identifiers of already destructed properties, following spec draft

your implementation uses a Set, not an array

> Source/JavaScriptCore/builtins/GlobalOperations.js:100
> +                @Object.@defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});

Don't we have a fast version of this?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:887
> +            jsSet->add(exec, JSValue(jsString));

style nit: I don't think there is a need for the JSValue(jsString) here, I think you can just do jsString.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4103
> +                    RefPtr<RegisterID> addMethod = generator.emitGetById(generator.newTemporary(), excludedList.get(), generator.propertyNames().builtinNames().addPrivateName());
> +                    CallArguments args(generator, nullptr, 1);
> +                    generator.emitMove(args.thisRegister(), excludedList.get());
> +                    generator.emitMove(args.argumentRegister(0), propertyName.get());
> +                    generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);

It's unfortunate to repeat this getById on every property. I think you can hoist this.
Also, it's interesting we're relying on on Set.prototype being non-writable and non-configurable. Can you add a test perhaps that just asserts this is true (if we don't already have one).
Perhaps such a test that would fail if it's not the case.

> Source/JavaScriptCore/parser/Parser.cpp:914
> +    auto element = parseMemberExpression(context);

why is this a member expression? Member expression allows "new"

> Source/JavaScriptCore/parser/Parser.cpp:1069
> +            if (Options::useObjectRestSpread() && match(DOTDOTDOT)) {

Nit: I think there is a chance it's more performant to swap the lhs and rhs of this &&.

> Source/JavaScriptCore/parser/Parser.cpp:3859
> +        if (Options::useObjectRestSpread())

the worlds tiniest nit suggestion: maybe it's worth caching this option in a parser field and perhaps it'll be hotter in cache.

[Caio Lima]

Comment on attachment 311431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311431&action=review

>> JSTests/ChangeLog:3
>> +        [ESnext] Implement Object Rest - Implementing Object Rest Destructuring
> 
> Does this patch do spread?

No. Spread is another Patch that will come after this one.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4103
>> +                    generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
> 
> It's unfortunate to repeat this getById on every property. I think you can hoist this.
> Also, it's interesting we're relying on on Set.prototype being non-writable and non-configurable. Can you add a test perhaps that just asserts this is true (if we don't already have one).
> Perhaps such a test that would fail if it's not the case.

Yes, you are right here. It's totally not efficient.

I can test if Set.prototype is non-configurable or non-writable, but by spec, it should be: https://tc39.github.io/ecma262/#sec-set.prototype.

>> Source/JavaScriptCore/parser/Parser.cpp:914
>> +    auto element = parseMemberExpression(context);
> 
> why is this a member expression? Member expression allows "new"

I actually just removed the recursive approach, i.e, not allowing "{" nor "[". I'm not sure if the prop spec changed, but it's unclear to me what should we allow here. Do you have any suggestion?

>> Source/JavaScriptCore/parser/Parser.cpp:1069
>> +            if (Options::useObjectRestSpread() && match(DOTDOTDOT)) {
> 
> Nit: I think there is a chance it's more performant to swap the lhs and rhs of this &&.

Changed.

>> Source/JavaScriptCore/parser/Parser.cpp:3859
>> +        if (Options::useObjectRestSpread())
> 
> the worlds tiniest nit suggestion: maybe it's worth caching this option in a parser field and perhaps it'll be hotter in cache.

I think it's a good idea.

[Caio Lima]

View in context: https://bugs.webkit.org/attachment.cgi?id=311431&action=review

>> JSTests/ChangeLog:3
>> +        [ESnext] Implement Object Rest - Implementing Object Rest Destructuring
> 
> Does this patch do spread?

No. Spread is another Patch that will come after this one.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4103
>> +                    generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
> 
> It's unfortunate to repeat this getById on every property. I think you can hoist this.
> Also, it's interesting we're relying on on Set.prototype being non-writable and non-configurable. Can you add a test perhaps that just asserts this is true (if we don't already have one).
> Perhaps such a test that would fail if it's not the case.

Yes, you are right here. It's totally not efficient.

I can test if Set.prototype is non-configurable or non-writable, but by spec, it should be: https://tc39.github.io/ecma262/#sec-set.prototype.

>> Source/JavaScriptCore/parser/Parser.cpp:914
>> +    auto element = parseMemberExpression(context);
> 
> why is this a member expression? Member expression allows "new"

I actually just removed the recursive approach, i.e, not allowing "{" nor "[". I'm not sure if the prop spec changed, but it's unclear to me what should we allow here. Do you have any suggestion?

>> Source/JavaScriptCore/parser/Parser.cpp:1069
>> +            if (Options::useObjectRestSpread() && match(DOTDOTDOT)) {
> 
> Nit: I think there is a chance it's more performant to swap the lhs and rhs of this &&.

Changed.

>> Source/JavaScriptCore/parser/Parser.cpp:3859
>> +        if (Options::useObjectRestSpread())
> 
> the worlds tiniest nit suggestion: maybe it's worth caching this option in a parser field and perhaps it'll be hotter in cache.

I think it's a good idea.

[Caio Lima]

View in context: https://bugs.webkit.org/attachment.cgi?id=311431&action=review

>> JSTests/ChangeLog:3
>> +        [ESnext] Implement Object Rest - Implementing Object Rest Destructuring
> 
> Does this patch do spread?

No. Spread is another Patch that will come after this one.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4103
>> +                    generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
> 
> It's unfortunate to repeat this getById on every property. I think you can hoist this.
> Also, it's interesting we're relying on on Set.prototype being non-writable and non-configurable. Can you add a test perhaps that just asserts this is true (if we don't already have one).
> Perhaps such a test that would fail if it's not the case.

Yes, you are right here. It's totally not efficient.

I can test if Set.prototype is non-configurable or non-writable, but by spec, it should be: https://tc39.github.io/ecma262/#sec-set.prototype.

>> Source/JavaScriptCore/parser/Parser.cpp:914
>> +    auto element = parseMemberExpression(context);
> 
> why is this a member expression? Member expression allows "new"

I actually just removed the recursive approach, i.e, not allowing "{" nor "[". I'm not sure if the prop spec changed, but it's unclear to me what should we allow here. Do you have any suggestion?

>> Source/JavaScriptCore/parser/Parser.cpp:1069
>> +            if (Options::useObjectRestSpread() && match(DOTDOTDOT)) {
> 
> Nit: I think there is a chance it's more performant to swap the lhs and rhs of this &&.

Changed.

>> Source/JavaScriptCore/parser/Parser.cpp:3859
>> +        if (Options::useObjectRestSpread())
> 
> the worlds tiniest nit suggestion: maybe it's worth caching this option in a parser field and perhaps it'll be hotter in cache.

I think it's a good idea.

[Caio Lima]

View in context: https://bugs.webkit.org/attachment.cgi?id=311431&action=review

Thank you very much for the review!

>> JSTests/ChangeLog:3
>> +        [ESnext] Implement Object Rest - Implementing Object Rest Destructuring
> 
> Does this patch do spread?

No. Spread is another Patch that will come after this one.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4103
>> +                    generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
> 
> It's unfortunate to repeat this getById on every property. I think you can hoist this.
> Also, it's interesting we're relying on on Set.prototype being non-writable and non-configurable. Can you add a test perhaps that just asserts this is true (if we don't already have one).
> Perhaps such a test that would fail if it's not the case.

Yes, you are right here. It's totally not efficient.

I can test if Set.prototype is non-configurable or non-writable, but by spec, it should be: https://tc39.github.io/ecma262/#sec-set.prototype.

>> Source/JavaScriptCore/parser/Parser.cpp:914
>> +    auto element = parseMemberExpression(context);
> 
> why is this a member expression? Member expression allows "new"

I actually just removed the recursive approach, i.e, not allowing "{" nor "[". I'm not sure if the prop spec changed, but it's unclear to me what should we allow here. Do you have any suggestion?

>> Source/JavaScriptCore/parser/Parser.cpp:1069
>> +            if (Options::useObjectRestSpread() && match(DOTDOTDOT)) {
> 
> Nit: I think there is a chance it's more performant to swap the lhs and rhs of this &&.

Changed.

>> Source/JavaScriptCore/parser/Parser.cpp:3859
>> +        if (Options::useObjectRestSpread())
> 
> the worlds tiniest nit suggestion: maybe it's worth caching this option in a parser field and perhaps it'll be hotter in cache.

I think it's a good idea.

[Caio Lima]

Created attachment 311679 [details]
Patch

Changed Patch based in Saam's review

[Build Bot]

Attachment 311679 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

Created attachment 311933 [details]
Patch

[Build Bot]

Attachment 311933 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Caio Lima]

(In reply to Caio Lima from comment #89)
> Created attachment 311933 [details]
> Patch

About memberExpression usage in "parseObjectRestAssignmentElement", it isn't allowing "new" expressions because we test "element && context.isAssignmentLocation(element)" just after memberExpressions parsing and fail if it's false.

[Caio Lima]

Ping review

[Saam Barati]

Comment on attachment 311933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311933&action=review

Patch looks good to me overall. I have one main suggestion for your builtin, and also some tests we should add before landing.

> Source/JavaScriptCore/builtins/GlobalOperations.js:100
> +                @Object.@defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});

I thought we added a bytecode to define property using an integer bit set. If so, we should use that here, so we don't allocate objects on every loop iteration.
Yusuke, does this bytecode exist?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:881
> +        JSSet* jsSet = JSSet::create(exec, vm, setStructure);

Nit: It might make sense to add a constructor to JSSet indicating how many items it'll have in it. This can be done in a follow up. That way, we don't end up doing any reallocation of its backing store when adding items to it.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4062
> +

Nice. This function looks good to me

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4089
> +                    RefPtr<RegisterID> pIndex = generator.emitLoad(nullptr, jsNumber(optionalIndex.value()));

Style nit: We typically don't names webkit. I'd name this either "index" or "propertyIndex"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4135
> +            generator.emitMove(args.argumentRegister(argumentCount++), newObject.get());
> +            generator.emitMove(args.argumentRegister(argumentCount++), rhs);
> +            if (m_containsComputedProperty)
> +                generator.emitMove(args.argumentRegister(argumentCount++), excludedList.get());
> +            else {
> +                RefPtr<RegisterID> excludedSetReg = generator.emitLoad(generator.newTemporary(), excludedSet);
> +                generator.emitMove(args.argumentRegister(argumentCount++), excludedSetReg.get());
> +            }

Why not just 0, 1, 2 for these as constants instead of using the argumentCount variable?

> Source/JavaScriptCore/parser/Parser.cpp:922
> +    if (strictMode() && m_parserState.lastIdentifier && context.isResolve(element)) {
> +        bool isEvalOrArguments = m_vm->propertyNames->eval == *m_parserState.lastIdentifier || m_vm->propertyNames->arguments == *m_parserState.lastIdentifier;
> +        failIfTrueIfStrict(isEvalOrArguments, "Cannot modify '", m_parserState.lastIdentifier->impl(), "' in strict mode");
> +    }

Please add a test for this. Also, please add tests for object rest being used in "catch", along with the expected syntax errors

> Source/JavaScriptCore/parser/Parser.cpp:981
> +        if (kind == DestructuringKind::DestructureToExpressions)
> +            return 0;

This branch is unreachable. I'd assert at the top of this function that this isn't the case.

> Source/JavaScriptCore/parser/Parser.cpp:986
> +    failIfTrue(match(LET) && (kind == DestructuringKind::DestructureToLet || kind == DestructuringKind::DestructureToConst), "Cannot use 'let' as an identifier name for a LexicalDeclaration");
> +    semanticFailIfTrue(isDisallowedIdentifierAwait(m_token), "Cannot use 'await' as a ", destructuringKindToVariableKindName(kind), " ", disallowedIdentifierAwaitReason());

Please add tests for these if you haven't already.

> Source/JavaScriptCore/parser/Parser.cpp:989
> +    m_parserState.nonLHSCount = nonLHSCount;

What's the point of this? It doesn't look like this gets changed.

> Source/JavaScriptCore/parser/Parser.cpp:998
> +    return parseObjectRestElement(context, kind, exportType, duplicateIdentifier, bindingContext);

Are we expected to get a syntax error here:
``` catch({...foo.bar})```? If so, please add a test to ensure we have this behavior.

[Caio Lima]

Comment on attachment 311933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311933&action=review

Thank you very much for the review. I've made the changes and I'm now running JSC tests before send the Patch.

>> Source/JavaScriptCore/builtins/GlobalOperations.js:100
>> +                @Object.@defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});
> 
> I thought we added a bytecode to define property using an integer bit set. If so, we should use that here, so we don't allocate objects on every loop iteration.
> Yusuke, does this bytecode exist?

IRC, I will work in the fast path here in https://bugs.webkit.org/show_bug.cgi?id=172888. But should be good Yusuke confirms that anyways =)

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:881
>> +        JSSet* jsSet = JSSet::create(exec, vm, setStructure);
> 
> Nit: It might make sense to add a constructor to JSSet indicating how many items it'll have in it. This can be done in a follow up. That way, we don't end up doing any reallocation of its backing store when adding items to it.

I agree. Created bug for that https://bugs.webkit.org/show_bug.cgi?id=173297

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4089
>> +                    RefPtr<RegisterID> pIndex = generator.emitLoad(nullptr, jsNumber(optionalIndex.value()));
> 
> Style nit: We typically don't names webkit. I'd name this either "index" or "propertyIndex"

Fixed

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4135
>> +            }
> 
> Why not just 0, 1, 2 for these as constants instead of using the argumentCount variable?

I was just following previous code pattern. Fixed.

>> Source/JavaScriptCore/parser/Parser.cpp:922
>> +    }
> 
> Please add a test for this. Also, please add tests for object rest being used in "catch", along with the expected syntax errors

Done.

>> Source/JavaScriptCore/parser/Parser.cpp:986
>> +    semanticFailIfTrue(isDisallowedIdentifierAwait(m_token), "Cannot use 'await' as a ", destructuringKindToVariableKindName(kind), " ", disallowedIdentifierAwaitReason());
> 
> Please add tests for these if you haven't already.

Done.

>> Source/JavaScriptCore/parser/Parser.cpp:989
>> +    m_parserState.nonLHSCount = nonLHSCount;
> 
> What's the point of this? It doesn't look like this gets changed.

I think you are right. Removed.

>> Source/JavaScriptCore/parser/Parser.cpp:998
>> +    return parseObjectRestElement(context, kind, exportType, duplicateIdentifier, bindingContext);
> 
> Are we expected to get a syntax error here:
> ``` catch({...foo.bar})```? If so, please add a test to ensure we have this behavior.

No. added the test.

[Caio Lima]

Created attachment 312769 [details]
Patch

Fixed Nits and added tests following Saam's review

[Build Bot]

Attachment 312769 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2128:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2133:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 311933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311933&action=review

>>> Source/JavaScriptCore/builtins/GlobalOperations.js:100
>>> +                @Object.@defineProperty(target, nextKey, {value: propValue, enumerable: true, writable: true, configurable: true});
>> 
>> I thought we added a bytecode to define property using an integer bit set. If so, we should use that here, so we don't allocate objects on every loop iteration.
>> Yusuke, does this bytecode exist?
> 
> IRC, I will work in the fast path here in https://bugs.webkit.org/show_bug.cgi?id=172888. But should be good Yusuke confirms that anyways =)

You can use op_define_data_property bytecode operation here. It can avoids costly object allocation for property descriptors. (Currently, Object.defineProperty is implemented in C++. Thus we cannot avoid this object allocation by using Object Allocation Sinking.
https://bugs.webkit.org/show_bug.cgi?id=172888 will solve it. But this can be leveraged only in FTL.

Thus, if we can use op_define_data_property, using it is the best way.
You can emit it by introducing some bytecode intrinsic.

[Caio Lima]

Created attachment 313398 [details]
Patch

Using intrinsic byte code to create data property.

[Build Bot]

Attachment 313398 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2153:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 313398 [details]
Patch

Attachment 313398 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3966284

New failing tests:
webrtc/video-replace-muted-track.html

[Build Bot]

Created attachment 313400 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Caio Lima]

Created attachment 313802 [details]
Patch

I'm adding intrinsic use in defineProperty to avoid Descriptor Object allocation.

[Build Bot]

Attachment 313802 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2148:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2153:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 313802 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313802&action=review

r=me

> JSTests/stress/object-rest-deconstruct.js:204
> +

Please add tests for object rest in catch.

[Caio Lima]

Created attachment 313959 [details]
Patch

Updating tests and ChangeLog

[Build Bot]

Attachment 313959 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2147:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2152:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 313959 [details]
Patch

Clearing flags on attachment: 313959

Committed r218861: <http://trac.webkit.org/changeset/218861>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.