WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
167963
[ESnext] Implement Object Spread
https://bugs.webkit.org/show_bug.cgi?id=167963
Summary
[ESnext] Implement Object Spread
Caio Lima
Reported
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 } ```
Attachments
Patch
(28.58 KB, patch)
2017-03-12 19:17 PDT
,
Caio Lima
ysuzuki
: review+
Details
Formatted Diff
Diff
Proposed Patch for Landing
(32.59 KB, patch)
2017-03-14 17:59 PDT
,
Caio Lima
ysuzuki
: 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
ysuzuki
: 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
saam
: 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
Show Obsolete
(13)
View All
Add attachment
proposed patch, testcase, etc.
Caio Lima
Comment 1
2017-03-12 19:17:18 PDT
Created
attachment 304214
[details]
Patch
Caio Lima
Comment 2
2017-03-12 19:19:23 PDT
First proposed Patch. Let's see what EWS thinks about it.
Yusuke Suzuki
Comment 3
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.
Caio Lima
Comment 4
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.
Caio Lima
Comment 5
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.
Yusuke Suzuki
Comment 6
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.
Caio Lima
Comment 7
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-.
Saam Barati
Comment 8
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>>
Yusuke Suzuki
Comment 9
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>>.
WebKit Commit Bot
Comment 10
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
Yusuke Suzuki
Comment 11
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
Caio Lima
Comment 12
2017-03-16 04:02:50 PDT
Created
attachment 304629
[details]
Patch for landing Fixing ChangeLog Typos.
WebKit Commit Bot
Comment 13
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
Caio Lima
Comment 14
2017-03-16 05:23:27 PDT
Created
attachment 304634
[details]
Patch rebased for landing Rebased with master.
WebKit Commit Bot
Comment 15
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
>
WebKit Commit Bot
Comment 16
2017-03-16 05:58:22 PDT
All reviewed patches have been landed. Closing bug.
Mark Lam
Comment 17
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.
Mark Lam
Comment 18
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.
Mark Lam
Comment 19
2017-05-15 17:23:41 PDT
This has been rolled out in
r216891
: <
http://trac.webkit.org/r216891
>.
Caio Lima
Comment 20
2017-06-28 20:04:26 PDT
Created
attachment 314099
[details]
Patch I rebased the last reverted Patch version with master.
Yusuke Suzuki
Comment 21
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?
Caio Lima
Comment 22
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.
Caio Lima
Comment 23
2017-06-30 19:43:49 PDT
Created
attachment 314354
[details]
Patch For landing Fixed comments
Build Bot
Comment 24
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
Build Bot
Comment 25
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
Caio Lima
Comment 26
2017-07-01 09:22:08 PDT
Created
attachment 314384
[details]
Patch for landing Rebased with master.
Saam Barati
Comment 27
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
Caio Lima
Comment 28
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.
Saam Barati
Comment 29
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
Saam Barati
Comment 30
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.
Caio Lima
Comment 31
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.
Caio Lima
Comment 32
2017-07-01 20:44:31 PDT
Created
attachment 314399
[details]
Patch
Build Bot
Comment 33
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
Build Bot
Comment 34
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
Caio Lima
Comment 35
2017-07-06 18:40:34 PDT
Created
attachment 314784
[details]
Patch Rebased with upstream/trunk.
Saam Barati
Comment 36
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
Caio Lima
Comment 37
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.
Caio Lima
Comment 38
2017-07-07 19:12:08 PDT
Created
attachment 314907
[details]
Patch Fixed bug pointed by Saam.
Saam Barati
Comment 39
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.
Caio Lima
Comment 40
2017-07-12 20:29:12 PDT
Created
attachment 315331
[details]
Patch for landing Adding syntax tests
Build Bot
Comment 41
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
Build Bot
Comment 42
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
Caio Lima
Comment 43
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
WebKit Commit Bot
Comment 44
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
>
WebKit Commit Bot
Comment 45
2017-07-13 00:11:50 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug