Bug 167963 - [ESnext] Implement Object Spread
Summary: [ESnext] Implement Object Spread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords:
Depends on: 167962
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-07 15:44 PST by Caio Lima
Modified: 2017-07-13 00:11 PDT (History)
10 users (show)

See Also:


Attachments
Patch (28.58 KB, patch)
2017-03-12 19:17 PDT, Caio Lima
utatane.tea: review+
Details | Formatted Diff | Diff
Proposed Patch for Landing (32.59 KB, patch)
2017-03-14 17:59 PDT, Caio Lima
utatane.tea: review+
Details | Formatted Diff | Diff
Patch for landing (32.88 KB, patch)
2017-03-15 19:07 PDT, Caio Lima
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (32.88 KB, patch)
2017-03-16 04:02 PDT, Caio Lima
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch rebased for landing (33.03 KB, patch)
2017-03-16 05:23 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (24.99 KB, patch)
2017-06-28 20:04 PDT, Caio Lima
utatane.tea: review+
Details | Formatted Diff | Diff
Patch For landing (24.57 KB, patch)
2017-06-30 19:43 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.39 MB, application/zip)
2017-06-30 21:10 PDT, Build Bot
no flags Details
Patch for landing (24.59 KB, patch)
2017-07-01 09:22 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (29.54 KB, patch)
2017-07-01 20:44 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.44 MB, application/zip)
2017-07-01 22:37 PDT, Build Bot
no flags Details
Patch (28.47 KB, patch)
2017-07-06 18:40 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (30.46 KB, patch)
2017-07-07 19:12 PDT, Caio Lima
sbarati: review+
Details | Formatted Diff | Diff
Patch for landing (31.34 KB, patch)
2017-07-12 20:29 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (953.88 KB, application/zip)
2017-07-12 21:36 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2017-02-07 15:44:47 PST
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 }
```
Comment 1 Caio Lima 2017-03-12 19:17:18 PDT
Created attachment 304214 [details]
Patch
Comment 2 Caio Lima 2017-03-12 19:19:23 PDT
First proposed Patch. Let's see what EWS thinks about it.
Comment 3 Yusuke Suzuki 2017-03-14 11:30:19 PDT
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.
Comment 4 Caio Lima 2017-03-14 16:33:15 PDT
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.
Comment 5 Caio Lima 2017-03-14 17:59:44 PDT
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.
Comment 6 Yusuke Suzuki 2017-03-14 22:09:05 PDT
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.
Comment 7 Caio Lima 2017-03-15 19:07:36 PDT
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-.
Comment 8 Saam Barati 2017-03-15 20:48:48 PDT
(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>>
Comment 9 Yusuke Suzuki 2017-03-15 22:23:16 PDT
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>>.
Comment 10 WebKit Commit Bot 2017-03-15 22:24:18 PDT
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
Comment 11 Yusuke Suzuki 2017-03-15 22:27:15 PDT
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
Comment 12 Caio Lima 2017-03-16 04:02:50 PDT
Created attachment 304629 [details]
Patch for landing

Fixing ChangeLog Typos.
Comment 13 WebKit Commit Bot 2017-03-16 04:59:32 PDT
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
Comment 14 Caio Lima 2017-03-16 05:23:27 PDT
Created attachment 304634 [details]
Patch rebased for landing

Rebased with master.
Comment 15 WebKit Commit Bot 2017-03-16 05:58:17 PDT
Comment on attachment 304634 [details]
Patch rebased for landing

Clearing flags on attachment: 304634

Committed r214038: <http://trac.webkit.org/changeset/214038>
Comment 16 WebKit Commit Bot 2017-03-16 05:58:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Mark Lam 2017-05-15 16:25:05 PDT
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.
Comment 18 Mark Lam 2017-05-15 17:02:12 PDT
I'm going to roll this out.  Let's re-land after we have a proper fix all the issues.
Comment 19 Mark Lam 2017-05-15 17:23:41 PDT
This has been rolled out in r216891: <http://trac.webkit.org/r216891>.
Comment 20 Caio Lima 2017-06-28 20:04:26 PDT
Created attachment 314099 [details]
Patch

I rebased the last reverted Patch version with master.
Comment 21 Yusuke Suzuki 2017-06-30 18:06:17 PDT
Comment on attachment 314099 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/ChangeLog:14
> +        It's also fixing CopyDataProperties that was using
> +        Object.getOwnPropertyNames to list all keys to be copied, and now is
> +        using Relect.ownKeys.

Let's drop this.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1959
> +    for (const auto& entry : m_codeBlock->constantIdentifierSets()) {

OK, reference is better here.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4326
> +    return 0;

Should return dst.

> Source/JavaScriptCore/parser/Parser.cpp:3880
> -        if (m_useObjectRestSpread) {
> -            classifyExpressionError(ErrorIndicatesPattern);
> -            return 0;
> -        }
> -        FALLTHROUGH;
> +        auto spreadLocation = m_token.m_location;
> +        auto start = m_token.m_startPosition;
> +        auto divot = m_token.m_endPosition;
> +        next();
> +        TreeExpression elem = parseAssignmentExpressionOrPropagateErrorClass(context);
> +        failIfFalse(elem, "Cannot parse subject of a spread operation");
> +        auto node = context.createObjectSpreadExpression(spreadLocation, elem, start, divot, m_lastTokenEndPosition);
> +        return context.createProperty(node, PropertyNode::Spread, PropertyNode::Unknown, complete, SuperBinding::NotNeeded, isClassProperty);

I think we need to use `Options::useObjectRestSpread()` here to guard against using it. Correct?
Comment 22 Caio Lima 2017-06-30 18:43:32 PDT
Comment on attachment 314099 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:14
>> +        using Relect.ownKeys.
> 
> Let's drop this.

Oops. Nice Catch.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4326
>> +    return 0;
> 
> Should return dst.

ok

>> Source/JavaScriptCore/parser/Parser.cpp:3880
>> +        return context.createProperty(node, PropertyNode::Spread, PropertyNode::Unknown, complete, SuperBinding::NotNeeded, isClassProperty);
> 
> I think we need to use `Options::useObjectRestSpread()` here to guard against using it. Correct?

You are right. I didn't remember about the flag. Updating it.
Comment 23 Caio Lima 2017-06-30 19:43:49 PDT
Created attachment 314354 [details]
Patch For landing

Fixed comments
Comment 24 Build Bot 2017-06-30 21:10:52 PDT
Comment on attachment 314354 [details]
Patch For landing

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

New failing tests:
fast/workers/worker-exception-during-navigation.html
Comment 25 Build Bot 2017-06-30 21:10:55 PDT
Created attachment 314367 [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
Comment 26 Caio Lima 2017-07-01 09:22:08 PDT
Created attachment 314384 [details]
Patch for landing

Rebased with master.
Comment 27 Saam Barati 2017-07-01 14:47:47 PDT
Comment on attachment 314384 [details]
Patch for landing

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4283
> +    IdentifierSet excludedSet;

This seems wasteful. Is it specified to just copy all properties excluding nothing? If so, you should call a function that doesn’t do hashmap lookups
Comment 28 Caio Lima 2017-07-01 18:24:05 PDT
Comment on attachment 314384 [details]
Patch for landing

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4283
>> +    IdentifierSet excludedSet;
> 
> This seems wasteful. Is it specified to just copy all properties excluding nothing? If so, you should call a function that doesn’t do hashmap lookups

That's a fair concern. I can change the copyDataProperties to avoid that behavior when the set is null.
Comment 29 Saam Barati 2017-07-01 18:54:06 PDT
(In reply to Caio Lima from comment #28)
> Comment on attachment 314384 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314384&action=review
> 
> >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4283
> >> +    IdentifierSet excludedSet;
> > 
> > This seems wasteful. Is it specified to just copy all properties excluding nothing? If so, you should call a function that doesn’t do hashmap lookups
> 
> That's a fair concern. I can change the copyDataProperties to avoid that
> behavior when the set is null.
Is this the specified behavior? Also, I’d just write a second builtin that doesn’t have a null check
Comment 30 Saam Barati 2017-07-01 18:55:13 PDT
Comment on attachment 314384 [details]
Patch for landing

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

> JSTests/stress/object-spread.js:297
> +

Also, please add a test using Proxy that validates spec methods are called and in the proper order.
Comment 31 Caio Lima 2017-07-01 19:08:39 PDT
(In reply to Saam Barati from comment #29)
> (In reply to Caio Lima from comment #28)
> > Comment on attachment 314384 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=314384&action=review
> > 
> > >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4283
> > >> +    IdentifierSet excludedSet;
> > > 
> > > This seems wasteful. Is it specified to just copy all properties excluding nothing? If so, you should call a function that doesn’t do hashmap lookups
> > 
> > That's a fair concern. I can change the copyDataProperties to avoid that
> > behavior when the set is null.
> Is this the specified behavior? Also, I’d just write a second builtin that
> doesn’t have a null check

Well, The behavior in the spec is pass an empty list to CopyDataProperties. See https://tc39.github.io/proposal-object-rest-spread/#Spread-RuntimeSemantics-PropertyDefinitionEvaluation
However, I can't see how it can harm our implementation. The problem of creating a new builtin is that if CopyDataProperties changes, we have 2 places to change as well.
Comment 32 Caio Lima 2017-07-01 20:44:31 PDT
Created attachment 314399 [details]
Patch
Comment 33 Build Bot 2017-07-01 22:37:26 PDT
Comment on attachment 314399 [details]
Patch

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

New failing tests:
fast/events/before-unload-returnValue.html
Comment 34 Build Bot 2017-07-01 22:37:27 PDT
Created attachment 314406 [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
Comment 35 Caio Lima 2017-07-06 18:40:34 PDT
Created attachment 314784 [details]
Patch

Rebased with upstream/trunk.
Comment 36 Saam Barati 2017-07-07 09:46:30 PDT
Comment on attachment 314784 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:517
> +            if (node->m_type & PropertyNode::Constant || node->m_type & PropertyNode::Spread)

this looks like the wrong logic here. The order of when this happens is observable. I think that means you need to immediately execute the loop below
Comment 37 Caio Lima 2017-07-07 19:11:03 PDT
Comment on attachment 314784 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:517
>> +            if (node->m_type & PropertyNode::Constant || node->m_type & PropertyNode::Spread)
> 
> this looks like the wrong logic here. The order of when this happens is observable. I think that means you need to immediately execute the loop below

You are right. I'm also adding test case for that.
Comment 38 Caio Lima 2017-07-07 19:12:08 PDT
Created attachment 314907 [details]
Patch

Fixed bug pointed by Saam.
Comment 39 Saam Barati 2017-07-11 16:28:36 PDT
Comment on attachment 314907 [details]
Patch

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

r=me

> Source/JavaScriptCore/parser/Parser.cpp:3878
> +            TreeExpression elem = parseAssignmentExpressionOrPropagateErrorClass(context);

Is this the grammar that's specified? If so, please add some more syntax tests for what's inside an assignment expression.
Comment 40 Caio Lima 2017-07-12 20:29:12 PDT
Created attachment 315331 [details]
Patch for landing

Adding syntax tests
Comment 41 Build Bot 2017-07-12 21:36:26 PDT
Comment on attachment 315331 [details]
Patch for landing

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

New failing tests:
storage/websql/execute-sql-rowsAffected.html
Comment 42 Build Bot 2017-07-12 21:36:28 PDT
Created attachment 315335 [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.12.5
Comment 43 Caio Lima 2017-07-12 21:45:43 PDT
(In reply to Saam Barati from comment #39)
> Comment on attachment 314907 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314907&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/parser/Parser.cpp:3878
> > +            TreeExpression elem = parseAssignmentExpressionOrPropagateErrorClass(context);
> 
> Is this the grammar that's specified? If so, please add some more syntax
> tests for what's inside an assignment expression.

Yes. You can check it in https://tc39.github.io/proposal-object-rest-spread/#Spread
Comment 44 WebKit Commit Bot 2017-07-13 00:11:48 PDT
Comment on attachment 315331 [details]
Patch for landing

Clearing flags on attachment: 315331

Committed r219443: <http://trac.webkit.org/changeset/219443>
Comment 45 WebKit Commit Bot 2017-07-13 00:11:50 PDT
All reviewed patches have been landed.  Closing bug.