[ESnext] Implement Object Spread

[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 z = { a: 3, b: 4 }
let n = { x, y, ...z };
n; // { x: 1, y: 2, a: 3, b: 4 }
```

[Caio Lima]

Created attachment 304214 [details]
Patch

[Caio Lima]

First proposed Patch. Let's see what EWS thinks about it.

[Yusuke Suzuki]

Comment on attachment 304214 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/builtins/BuiltinNames.h:73
> +    macro(getOwnPropertySymbols) \

I think it is not necessary. The reason will be described in the following comments :).

> Source/JavaScriptCore/builtins/GlobalOperations.js:90
>          return target;

How about just using `== null`?

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

I think we should use `@Object(source)` here. And we can drop `@toObject`.
`@Object` is nice because it is well handled in DFG :) (Like, if source is likely an object, we emit the fixup edge and drop @Object call!).

> Source/JavaScriptCore/builtins/GlobalOperations.js:94
> +    let propertyNames = @Object.@getOwnPropertyNames(from);
> +    let keys = propertyNames.@concat(@Object.@getOwnPropertySymbols(from));

You can use @Reflect.@ownKeys().

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4238
> +    HashSet<UniquedStringImpl*> excludedSet;

BTW, who guarantees this UniquedStringImpl*'s lifetime? Is HashSet<Ref<UniquedStringImpl>> correct?

> Source/JavaScriptCore/parser/NodeConstructors.h:239
> +        : m_name(0)

Use nullptr.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:93
> +    JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().concatPrivateName(), arrayPrototypeConcatCodeGenerator, DontEnum | DontDelete | ReadOnly);

So I think it is not necessary.

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:108
> +    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().getOwnPropertySymbolsPrivateName(), objectConstructorGetOwnPropertySymbols, DontEnum, 1);

Ditto.

[Caio Lima]

Comment on attachment 304214 [details]
Patch

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

Thank you for the review. Uploading a Patch soon.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4238
>> +    HashSet<UniquedStringImpl*> excludedSet;
> 
> BTW, who guarantees this UniquedStringImpl*'s lifetime? Is HashSet<Ref<UniquedStringImpl>> correct?

Actually, for this specific case, It is always going to be empty.

[Caio Lima]

Created attachment 304453 [details]
Proposed Patch for Landing

Asking for a new Batch of Review since I changed HashSet<UniquedStringImpl*> for HashSet<RefPtr<UniquedStringImpl>> in Object Rest code. I'm in doubt If I should use RefPtr here.

[Yusuke Suzuki]

Comment on attachment 304453 [details]
Proposed Patch for Landing

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

r=me with comments.

> Source/JavaScriptCore/builtins/GlobalOperations.js:89
> +    if (source === @undefined || source == null)

`source == null` includes `source === @undefined`. So drop `source === @undefined` part.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:878
> +        const HashSet<RefPtr<UniquedStringImpl>>& set = entry.first;

If HashSet does not include any nullptr, we should use Ref<>.
In this case, I think we shoud use Ref<> instead of RefPtr<>.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:879
>          for (auto setEntry : set) {

It becomes `auto& `

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:880
> +            JSString* jsString = jsOwnedString(&vm, setEntry.get());

And it becomes `setEntry.ptr().

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1925
>      for (ConstantIndentifierSetEntry entry : m_codeBlock->constantIdentifierSets()) {

This always copies HashSet. Let's use `const auto& entry` instead.

[Caio Lima]

Created attachment 304594 [details]
Patch for landing

In an IRC conversation with Saam, I decided to change HashMap<Ref<UniquedStringImpl>> usage to an already defined type called IdentiferSet. I noticed that HashMap<Ref<UniquedStringImpl>> doesn't compile, since there is no empty values for Ref<>. Thank you very much for the review Yusuki! If you doesn't agree with that approach, feel free to set cq-.

[Saam Barati]

(In reply to comment #7)
> Created attachment 304594 [details]
> Patch for landing
> 
> In an IRC conversation with Saam, I decided to change
> HashMap<Ref<UniquedStringImpl>> usage to an already defined type called
> IdentiferSet. I noticed that HashMap<Ref<UniquedStringImpl>> doesn't
> compile, since there is no empty values for Ref<>. Thank you very much for
> the review Yusuki! If you doesn't agree with that approach, feel free to set
> cq-.

I think we should open a bug to fix this in the future. We should be able to put Ref<T> in a HashMap, and we should probably make IdentifierSet be HashSet<Ref<UniquedStringImpl>>

[Yusuke Suzuki]

Comment on attachment 304594 [details]
Patch for landing

IIRC, I used HashSet<Ref<PendingScript>> in WebCore/dom/ScriptRunner.h.
Agreed with Saam, we should open the bug to convert IdentifierSet's definition from HashSet<RefPtr<UniquedStringImpl>> to HashSet<Ref<UniquedStringImpl>>.

[WebKit Commit Bot]

Comment on attachment 304594 [details]
Patch for landing

Rejecting attachment 304594 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 304594, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

NOBODY Yusuke Suzuki found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/3335899

[Yusuke Suzuki]

Comment on attachment 304594 [details]
Patch for landing

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

> LayoutTests/ChangeLog:6
> +        Reviewed by NOBODY Yusuke Suzuki.

This NOBODY should be removed :P

[Caio Lima]

Created attachment 304629 [details]
Patch for landing

Fixing ChangeLog Typos.

[WebKit Commit Bot]

Comment on attachment 304629 [details]
Patch for landing

Rejecting attachment 304629 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 304629, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
/git.webkit.org/WebKit
   a3ffd8d..64812ea  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 214035 = a3ffd8d0c8303a991dac3b1634695e9bfadf0a13
r214036 = 64812eab7acc61e63dbb848b176f646f49d117d3
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/3337753

[Caio Lima]

Created attachment 304634 [details]
Patch rebased for landing

Rebased with master.

[WebKit Commit Bot]

Comment on attachment 304634 [details]
Patch rebased for landing

Clearing flags on attachment: 304634

Committed r214038: <http://trac.webkit.org/changeset/214038>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

This patch did not handle the case of computed properties e.g.

var a = "x";
var { [a]:b, y, ...z } = { x: 1, y: 2, a: 3, b: 4 };

This results in a crash.

[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>.