Bug 151494

Summary: [JSC] support Computed Property Names in destructuring Patterns
Product: WebKit Reporter: Caitlin Potter (:caitp) <caitp>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ggaren, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
more tests + changelog + less code duplication
none
Patch none

Description Caitlin Potter (:caitp) 2015-11-20 08:42:20 PST
[JSC] support Computed Property Names in destructuring Patterns
Comment 1 Caitlin Potter (:caitp) 2015-11-20 08:43:41 PST
Created attachment 265955 [details]
Patch

Support Computed Property names in destructuring patterns, eg
Comment 2 WebKit Commit Bot 2015-11-20 08:44:43 PST
Attachment 265955 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2020:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2025:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Caitlin Potter (:caitp) 2015-11-20 08:47:27 PST
Created attachment 265956 [details]
Patch

fixed changelog style --- check-webkit-style doesn't seem to approve of the style that was in the tree to begin with, I dunno.
Comment 4 WebKit Commit Bot 2015-11-20 08:49:15 PST
Attachment 265956 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2020:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2025:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Saam Barati 2015-11-20 09:23:30 PST
Comment on attachment 265956 [details]
Patch

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

r=me with some comments

> Source/JavaScriptCore/ChangeLog:8
> +        Add support for computed property names in destructuring BindingPatterns

It's worth adding a link to the spec here.

> Source/JavaScriptCore/parser/Parser.cpp:891
> +                    consumeOrFail(COLON, "Expected ':' after property name");

So this kind of expression is only allowed using the new feature you added recently that assigns to the rhs of the colon?

> Source/JavaScriptCore/parser/Parser.cpp:894
> +                    TreeExpression defaultValue = parseDefaultValueForDestructuringPattern(context);

Can we add a test for this case?

> Source/JavaScriptCore/tests/es6/destructuring_assignment_computed_properties.js:1
> +function test() {

I think this test is great. But, it's a bit tricky to follow. Can you also add just a couple really straight forward tests?
Comment 6 Caitlin Potter (:caitp) 2015-11-20 09:50:01 PST
> So this kind of expression is only allowed using the new feature you added recently that assigns to the rhs of the colon?

It was already possible to assign to the rhs of the colon, the consumeOrFail and other code duplication is here to keep it a bit more straight forward (instead of conditionally deciding how to behave depending on whether propertyExpression is null or not, further down). I don't have a strong opinion on it if people don't like the code duplication in the name of straightforwardness.

---

Adding some other tests
Comment 7 Caitlin Potter (:caitp) 2015-11-20 10:49:16 PST
Created attachment 265970 [details]
more tests + changelog + less code duplication
Comment 8 WebKit Commit Bot 2015-11-20 10:50:55 PST
Attachment 265970 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2020:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2025:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Caitlin Potter (:caitp) 2015-11-20 11:49:33 PST
Created attachment 265974 [details]
Patch

Fix email in ChangeLog entry
Comment 10 WebKit Commit Bot 2015-11-20 11:52:02 PST
Attachment 265974 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:2020:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:2025:  Missing space before {  [whitespace/braces] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Caitlin Potter (:caitp) 2015-11-24 10:44:51 PST
Comment on attachment 265974 [details]
Patch

Is there anything else needed here?
Comment 12 WebKit Commit Bot 2015-11-24 17:43:24 PST
Comment on attachment 265974 [details]
Patch

Clearing flags on attachment: 265974

Committed r192768: <http://trac.webkit.org/changeset/192768>
Comment 13 WebKit Commit Bot 2015-11-24 17:43:27 PST
All reviewed patches have been landed.  Closing bug.