Summary: | [ESnext] Implement Object Spread | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Lima <ticaiolima> | ||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Caio Lima <ticaiolima> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, chi187, commit-queue, keith_miller, mark.lam, msaboff, rniwa, saam, ticaiolima, ysuzuki | ||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||
Bug Depends on: | 167962 | ||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Caio Lima
2017-02-07 15:44:47 PST
Created attachment 304214 [details]
Patch
First proposed Patch. Let's see what EWS thinks about it. 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 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. 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 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. 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-.
(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 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 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 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 Created attachment 304629 [details]
Patch for landing
Fixing ChangeLog Typos.
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 Created attachment 304634 [details]
Patch rebased for landing
Rebased with master.
Comment on attachment 304634 [details] Patch rebased for landing Clearing flags on attachment: 304634 Committed r214038: <http://trac.webkit.org/changeset/214038> All reviewed patches have been landed. Closing bug. 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. I'm going to roll this out. Let's re-land after we have a proper fix all the issues. This has been rolled out in r216891: <http://trac.webkit.org/r216891>. Created attachment 314099 [details]
Patch
I rebased the last reverted Patch version with master.
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 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. Created attachment 314354 [details]
Patch For landing
Fixed comments
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 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
Created attachment 314384 [details]
Patch for landing
Rebased with master.
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 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. (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 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. (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. Created attachment 314399 [details]
Patch
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 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
Created attachment 314784 [details]
Patch
Rebased with upstream/trunk.
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 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. Created attachment 314907 [details]
Patch
Fixed bug pointed by Saam.
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. Created attachment 315331 [details]
Patch for landing
Adding syntax tests
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 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
(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 on attachment 315331 [details] Patch for landing Clearing flags on attachment: 315331 Committed r219443: <http://trac.webkit.org/changeset/219443> All reviewed patches have been landed. Closing bug. |