Bug 142679 – [ES6] support default values in deconstruction parameter nodes

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

[Saam Barati]

var {x=20} = {}
should work.
see 13.2.3 of the spec
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-destructuring-binding-patterns

and this blog post:

http://www.2ality.com/2015/03/destructuring-algorithm.html

[Saam Barati]

Created attachment 254898 [details]
patch

It begins.

[Saam Barati]

Current implementation probably breaks for function parameters.
We should probably change the parser such that function parameters are parsed 
inside the same parser arena as the function itself.

[Saam Barati]

Created attachment 254906 [details]
patch

For now, remove support for function parameters and write some tests.

[Saam Barati]

Created attachment 254909 [details]
patch

[Saam Barati]

Comment on attachment 254909 [details]
patch

Forgot to generate test results.

[Saam Barati]

Created attachment 254913 [details]
patch

With test results.

[Darin Adler]

Comment on attachment 254913 [details]
patch

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

> Source/JavaScriptCore/parser/ASTBuilder.h:740
> +        node->appendIndex(location, 0, 0);

nullptr

> Source/JavaScriptCore/parser/Nodes.h:1797
> +            Entry(DeconstructionPatternNode* pattern, ExpressionNode* defaultValue)
> +                : pattern(pattern)
> +                , defaultValue(defaultValue)
> +            {
> +            }

No need for this. With C++11 a struct can be initialized with { } syntax anywhere we would use constructor syntax.

> Source/JavaScriptCore/parser/Nodes.h:1815
> +            m_targetPatterns.append(Entry(identifier, wasString, pattern, defaultValue));

Since this is a struct, I believe you can write:

    m_targetPatterns.append({ identifier, wasString, pattern, defaultValue });

It’s possible you might have to explicitly specify Entry, but I don’t think so.

> Source/JavaScriptCore/parser/Nodes.h:1830
> -            Entry(const Identifier& propertyName, bool wasString, DeconstructionPatternNode* pattern)
> +            Entry(const Identifier& propertyName, bool wasString, DeconstructionPatternNode* pattern, ExpressionNode* defaultValue)
>                  : propertyName(propertyName)
>                  , wasString(wasString)
>                  , pattern(pattern)
> +                , defaultValue(defaultValue)
>              {
>              }

No need for this. With C++11 a struct can be initialized with { } syntax anywhere we would use constructor syntax.

> Source/JavaScriptCore/parser/Parser.cpp:719
> +    TreeExpression defaultValue = 0;
> +    if (match(EQUAL)) {
> +        next(TreeBuilder::DontBuildStrings); // consume '='
> +        defaultValue = parseAssignmentExpression(context);
> +    }
> +    return defaultValue;

I think this reads better without a local variable. I would write:

    if (!match(EQUAL))
        return 0;
    next(TreeBuilder::DontBuildStrings); // consume '='
    return parseAssignmentExpression(context);

[Filip Pizlo]

Comment on attachment 254913 [details]
patch

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

>> Source/JavaScriptCore/parser/Nodes.h:1797
>> +            }
> 
> No need for this. With C++11 a struct can be initialized with { } syntax anywhere we would use constructor syntax.

Is this a matter of policy?  I believe that Entry(thing, thing) reads much better than {thing, thing} because we know what we are creating.

>> Source/JavaScriptCore/parser/Nodes.h:1815
>> +            m_targetPatterns.append(Entry(identifier, wasString, pattern, defaultValue));
> 
> Since this is a struct, I believe you can write:
> 
>     m_targetPatterns.append({ identifier, wasString, pattern, defaultValue });
> 
> It’s possible you might have to explicitly specify Entry, but I don’t think so.

I think it's a lot more clear to see Entry(identifier, wasString, pattern, defaultValue).  It's good to know that the struct being created is called Entry.  We have a lot of structs in the system, and it's great to know which one we are creating.

>> Source/JavaScriptCore/parser/Nodes.h:1830
>>              }
> 
> No need for this. With C++11 a struct can be initialized with { } syntax anywhere we would use constructor syntax.

Ditto.

[Saam Barati]

I agree with Fil on this one. I like the explicitness of Entry(.)

[Saam Barati]

Created attachment 255091 [details]
patch for landing

I went with the: "Entry{ . }" syntax which is both explicit and allows us to remove boilerplate constructors.

I'll let the CQ land this one for me.

[WebKit Commit Bot]

Attachment 255091 [details] did not pass style-queue:


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


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 255091 [details]
patch for landing

Attachment 255091 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4664861145432064

New failing tests:
js/destructuring-assignment-default-values.html

[Build Bot]

Created attachment 255093 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 255091 [details]
patch for landing

Attachment 255091 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5612020619542528

New failing tests:
js/destructuring-assignment-default-values.html

[Build Bot]

Created attachment 255095 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Saam Barati]

Created attachment 255108 [details]
patch for landing

Lets try that again.

[WebKit Commit Bot]

Attachment 255108 [details] did not pass style-queue:


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


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 255108 [details]
patch for landing

Clearing flags on attachment: 255108

Committed r185699: <http://trac.webkit.org/changeset/185699>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.