Any sort of Property MemberExpression used in a destructuring assignment context results in a syntax error: ```js ({ x: this.x } = { x: true }); ({ x: (function() {}).x } = { x: true }); ({ x: ({}).x } = { x: true }); // etc ... ``` However, these are all legal (even if they don't make any sense), per http://tc39.github.io/ecma262/#prod-AssignmentElement
Created attachment 265428 [details] Accept any LHS for AssignmentElements (no tests yet)
Added a first-stab at a fix. This only supports the case of ObjectAssignmentPattern properties with identifier property names, so no integer/string/computed properties supported yet. Also, ArrayAssignmentPatterns aren't handled. A test case: ```js var foo = {}; ({ a: foo.a } = { a: true }); assert(foo.a === true); ``` I'm happy to add tests and clean it up (without cleanup, there will be a pile of duplicate code), but I'd like to make sure the codegen and new AST entry make sense first.
+CC Oliver, since he wrote a lot of this parser.
I'm curious: Is an expression like this ever useful, or at least observable to the programmer?
The most common observable use (as far as I can tell), is assigning to properties of "this", eg in a constructor. eg: ``` class FancyNode { constructor(nodePosition = null) { ({ x: this.x, y: this.y } = nodePosition); } } ``` or something similar. The other uses are legal and observable, but probably not very useful?
Wow. This destination-on-the-right JavaScript syntax is blowing my mind. In the bad way :(.
Comment on attachment 265428 [details] Accept any LHS for AssignmentElements (no tests yet) View in context: https://bugs.webkit.org/attachment.cgi?id=265428&action=review Thanks for the patch. This looks pretty good. Please add test cases. Also, a few comments below. > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3454 > +void AssignmentElementNode::toString(StringBuilder& builder) const > +{ > + if (m_assignmentTarget->isResolveNode()) > + builder.append(static_cast<ResolveNode*>(m_assignmentTarget)->identifier().string()); > +} Can you add support for isDotAccessorNode and isBracketAccessorNode here? > Source/JavaScriptCore/parser/Parser.cpp:864 > + if (!element || !context.isAssignmentLocation(element)) { > + // TODO: Early SyntaxError per http://tc39.github.io/ecma262/#sec-assignment-operators-static-semantics-early-errors > + return 0; You can use "failIfFalse(element && context.isAssignmentLocation(element), message)" here.
... In addition to adding tests, you should run the run-javascriptcore-tests script to see if you've changed any expected failures to passes in Source/JavaScriptCore/tests/es6/.
Left some comments on the diff about the `toString()` thing, and the approach for making the error message macro do something useful (propagate the correct message), which sort of depends on some hacks (the save point clears the original message, so it needs to be saved and reset manually later on in order to pass the right one in)
Created attachment 265470 [details] Patch
Comment on attachment 265470 [details] Patch r=me 👍
Comment on attachment 265470 [details] Patch Clearing flags on attachment: 265470 Committed r192436: <http://trac.webkit.org/changeset/192436>
All reviewed patches have been landed. Closing bug.
This seems to have caused a very long hang in Web Inspector: <https://webkit.org/b/151324> REGRESSION(r192436): Inspector Hanging under tryParseDestructuringPatternExpression
I'm going to roll out this patch for now since I don't have enough time to review review its fixes by the end of the day.
Re-opened since this is blocked by bug 151417
Created attachment 265822 [details] Original patch + perf fixes from bug151324 Original patch rebased on ToT + perf fixes from bug 151324
Created attachment 265825 [details] Original patch + perf fixes from bug151324 Original patch rebased on ToT + perf fixes from bug 151324 --- rebased again, my bad
Comment on attachment 265825 [details] Original patch + perf fixes from bug151324 Attachment 265825 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/448018 New failing tests: sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8_T3.html
Created attachment 265827 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 265825 [details] Original patch + perf fixes from bug151324 Attachment 265825 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/448026 New failing tests: sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8_T3.html
Created attachment 265830 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 265825 [details] Original patch + perf fixes from bug151324 Attachment 265825 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/448087 New failing tests: sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8_T3.html
Created attachment 265834 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 265835 [details] Original patch + perf fixes from bug151324 v2 Fix the new test failure
Comment on attachment 265835 [details] Original patch + perf fixes from bug151324 v2 Patch looks good to me. Do we have the test case that checks for the Web Inspector regression? I think that might have gotten lost.
There's a regression test, but the inspector issue was just a really long parse-time. I don't know if there's a non-flakey way to ensure that it parses within under 2ms or something.
oh you're right, it's gone from the patch, will re-add that...
If we can make the hang long enough (say, 5s), then we can probably test for that in a non-flaky way.
Created attachment 265897 [details] Patch
Created attachment 265899 [details] Patch
Should be good now, sorry about that.
Comment on attachment 265899 [details] Patch r=me
Comment on attachment 265899 [details] Patch Clearing flags on attachment: 265899 Committed r192661: <http://trac.webkit.org/changeset/192661>
*** Bug 151324 has been marked as a duplicate of this bug. ***