Bug 151026 – Destructuring Assignment: Properties are valid AssignmentElements

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

Created attachment 265428 [details]
Accept any LHS for AssignmentElements (no tests yet)

[Caitlin Potter (:caitp)]

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.

[Geoffrey Garen]

+CC Oliver, since he wrote a lot of this parser.

[Geoffrey Garen]

I'm curious: Is an expression like this ever useful, or at least observable to the programmer?

[Caitlin Potter (:caitp)]

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?

[Geoffrey Garen]

Wow. This destination-on-the-right JavaScript syntax is blowing my mind. In the bad way :(.

[Geoffrey Garen]

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.

[Geoffrey Garen]

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

[Caitlin Potter (:caitp)]

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)

[Caitlin Potter (:caitp)]

Created attachment 265470 [details]
Patch

[Geoffrey Garen]

Comment on attachment 265470 [details]
Patch

r=me 👍

[WebKit Commit Bot]

Comment on attachment 265470 [details]
Patch

Clearing flags on attachment: 265470

Committed r192436: <http://trac.webkit.org/changeset/192436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

This seems to have caused a very long hang in Web Inspector:
<https://webkit.org/b/151324> REGRESSION(r192436): Inspector Hanging under tryParseDestructuringPatternExpression

[Geoffrey Garen]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 151417

[Caitlin Potter (:caitp)]

Created attachment 265822 [details]
Original patch + perf fixes from bug151324

Original patch rebased on ToT + perf fixes from bug 151324

[Caitlin Potter (:caitp)]

Created attachment 265825 [details]
Original patch + perf fixes from bug151324

Original patch rebased on ToT + perf fixes from bug 151324 --- rebased again, my bad

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Caitlin Potter (:caitp)]

Created attachment 265835 [details]
Original patch + perf fixes from bug151324 v2

Fix the new test failure

[Geoffrey Garen]

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.

[Caitlin Potter (:caitp)]

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.

[Caitlin Potter (:caitp)]

oh you're right, it's gone from the patch, will re-add that...

[Geoffrey Garen]

If we can make the hang long enough (say, 5s), then we can probably test for that in a non-flaky way.

[Caitlin Potter (:caitp)]

Created attachment 265897 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 265899 [details]
Patch

[Caitlin Potter (:caitp)]

Should be good now, sorry about that.

[Geoffrey Garen]

Comment on attachment 265899 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 265899 [details]
Patch

Clearing flags on attachment: 265899

Committed r192661: <http://trac.webkit.org/changeset/192661>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

*** Bug 151324 has been marked as a duplicate of this bug. ***