Bug 146934 – [ES6] Arrow function syntax. Arrow function should support the destructuring parameters.

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

[GSkachkov]

Arrow function should support the destructuring parameters.
Example: 
1 ([a,b]) => a*b // OK
2 [a,b] => a*b   //Syntax error

[GSkachkov]

Created attachment 268657 [details]
Patch

Patch

[Saam Barati]

Comment on attachment 268657 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.h:1080
> +        else if (match(OPENPAREN)) {
> +            // Let's check if this destructuring paramters
> +            SavePoint saveArrowFunctionPoint = createSavePoint();
> +            next(); // Consume OPENPAREN;
> +            auto pattern = tryParseDestructuringPatternExpression(context, AssignmentContext::DeclarationStatement);
> +            isArrowFunction = pattern && consume(CLOSEPAREN) && match(ARROWFUNCTION);
> +            
> +            restoreSavePoint(saveArrowFunctionPoint);
> +        }
> +        

This code isn't correct.
It won't parse:
```
(a, b, {c}, [d], {e, f}) => ...;
```

I suggest an alternate approach here:
have this code check if we have the single parameter name with no parens case:
```
let x = a=>a;
``` 
and otherwise, we already know how to parse "(p1, p2, ...)" style parameter lists
in another function in the parser. We should use that same code here. Either we use
it directly as is (probably not possible), or we should abstract that code in such a way
that this code can use it. That way, we don't have to places in the parser where we parse parameters.

The reason we have this problem in the first place is that we have two places where we parse
parameters, lets not make that mistake again.

[GSkachkov]

Created attachment 268861 [details]
Patch

Patch with fixed comments

[GSkachkov]

Comment on attachment 268657 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.h:1080
>> +        
> 
> This code isn't correct.
> It won't parse:
> ```
> (a, b, {c}, [d], {e, f}) => ...;
> ```
> 
> I suggest an alternate approach here:
> have this code check if we have the single parameter name with no parens case:
> ```
> let x = a=>a;
> ``` 
> and otherwise, we already know how to parse "(p1, p2, ...)" style parameter lists
> in another function in the parser. We should use that same code here. Either we use
> it directly as is (probably not possible), or we should abstract that code in such a way
> that this code can use it. That way, we don't have to places in the parser where we parse parameters.
> 
> The reason we have this problem in the first place is that we have two places where we parse
> parameters, lets not make that mistake again.

I've tried to use function that parse parameters - parseFormalParameters, however parseFormalParameters adds parsed parameters to current scope, so I had to make face scope to avoid errors. 
Could you please check if it acceptable?

[Saam Barati]

(In reply to comment #4)
> Comment on attachment 268657 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268657&action=review
> 
> >> Source/JavaScriptCore/parser/Parser.h:1080
> >> +        
> > 
> > This code isn't correct.
> > It won't parse:
> > ```
> > (a, b, {c}, [d], {e, f}) => ...;
> > ```
> > 
> > I suggest an alternate approach here:
> > have this code check if we have the single parameter name with no parens case:
> > ```
> > let x = a=>a;
> > ``` 
> > and otherwise, we already know how to parse "(p1, p2, ...)" style parameter lists
> > in another function in the parser. We should use that same code here. Either we use
> > it directly as is (probably not possible), or we should abstract that code in such a way
> > that this code can use it. That way, we don't have to places in the parser where we parse parameters.
> > 
> > The reason we have this problem in the first place is that we have two places where we parse
> > parameters, lets not make that mistake again.
> 
> I've tried to use function that parse parameters - parseFormalParameters,
> however parseFormalParameters adds parsed parameters to current scope, so I
> had to make face scope to avoid errors. 
> Could you please check if it acceptable?

I think this approach is okay. It's unfortunate we have to create
a scope here, but this is probably the most reliable way to future-proof
this code.

[Saam Barati]

Comment on attachment 268861 [details]
Patch

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

this seems like the right approach, just a few comments to clean up the code.

> Source/JavaScriptCore/parser/Parser.h:1047
> +    ALWAYS_INLINE bool isArrowFunctionNoOrOneParamter()
>      {
>          bool isArrowFunction = false;
>          
> -        if (match(EOFTOK))
> +        if (match(EOFTOK) || (!match(IDENT) && !match(OPENPAREN)))
>              return isArrowFunction;
>          
>          SavePoint saveArrowFunctionPoint = createSavePoint();
> +        if (match(IDENT))
> +            isArrowFunction = consume(IDENT) && match(ARROWFUNCTION);
> +        else if (consume(OPENPAREN) && consume(CLOSEPAREN))
> +            isArrowFunction = match(ARROWFUNCTION);
>          
> -        if (consume(OPENPAREN)) {
> -            bool isArrowFunctionParamters = true;
> +        restoreSavePoint(saveArrowFunctionPoint);
> +        
> +        return isArrowFunction;
> +    }

This function has some redundancy with the below function, lets just combine
the necessary bits into the below function.

> Source/JavaScriptCore/parser/Parser.h:1049
> +    template <class TreeBuilder> bool isArrowFunctionParamters(TreeBuilder& context)

We don't need to templatize this once we move it to SyntaxChecker (see below).
Also, this function has been misspelled, lets fix the spelling in this patch:
"isArrowFunctionParamters" => "isArrowFunctionParameters"

> Source/JavaScriptCore/parser/Parser.h:1057
> +        if (isArrowFunctionNoOrOneParamter())
> +            isArrowFunction = true;

Instead of calling the above function, you
can just check:
```
if (match(IDENT)) {
   save point create;
   next();
   check if arrow function
   save point restore;
} else if ....
```

> Source/JavaScriptCore/parser/Parser.h:1066
> +            unsigned parametersCount = 1;

start this at zero.
Even though we ignore this number, it should start at zero.

> Source/JavaScriptCore/parser/Parser.h:1067
> +            isArrowFunction = parseFormalParameters(context, context.createFormalParameterList(), parametersCount) && consume(CLOSEPAREN) && match(ARROWFUNCTION);

Instead of passing in "context" here, we should create a SyntaxChecker and pass that in.
That way we're not actually allocating AST nodes that we throw away.

> LayoutTests/js/script-tests/arrowfunction-syntax.js:79
>  

add a test or a few tests for rest parameters:
```
(...rest) => rest;
```
etc.

[GSkachkov]

Created attachment 268951 [details]
Patch

Patch with fixed comments

[GSkachkov]

Comment on attachment 268861 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.h:1047
>> +    }
> 
> This function has some redundancy with the below function, lets just combine
> the necessary bits into the below function.

Merged into one function

>> Source/JavaScriptCore/parser/Parser.h:1049
>> +    template <class TreeBuilder> bool isArrowFunctionParamters(TreeBuilder& context)
> 
> We don't need to templatize this once we move it to SyntaxChecker (see below).
> Also, this function has been misspelled, lets fix the spelling in this patch:
> "isArrowFunctionParamters" => "isArrowFunctionParameters"

Fixed

>> Source/JavaScriptCore/parser/Parser.h:1057
>> +            isArrowFunction = true;
> 
> Instead of calling the above function, you
> can just check:
> ```
> if (match(IDENT)) {
>    save point create;
>    next();
>    check if arrow function
>    save point restore;
> } else if ....
> ```

Refactored

>> Source/JavaScriptCore/parser/Parser.h:1067
>> +            isArrowFunction = parseFormalParameters(context, context.createFormalParameterList(), parametersCount) && consume(CLOSEPAREN) && match(ARROWFUNCTION);
> 
> Instead of passing in "context" here, we should create a SyntaxChecker and pass that in.
> That way we're not actually allocating AST nodes that we throw away.

Done

[Saam Barati]

Comment on attachment 268951 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:377
> +bool Parser<LexerType>::isArrowFunctionParameters()

This looks good to me, just some really small nits on how to write the function:

> Source/JavaScriptCore/parser/Parser.cpp:382
> +        
> +    if (match(EOFTOK) || (!match(OPENPAREN) && !match(IDENT)))
> +        return isArrowFunction;

I would write this like:
```
if (match(EOFTOK))
    return false;
bool isOpenParen = match(OPENPAREN);
bool isIdent = match(IDENT);
if (!isOpenParen && !isIdent)
    return false;
```
Then, below, you can use these booleans instead of performing the match(...) twice.
I say this because I think that LLVM will CSE this, but lets take matters into our own hand.

> Source/JavaScriptCore/parser/Parser.cpp:386
> +    if (match(IDENT)) {

Then this could be: "if (isIdent) { next(); isArrowFunction = match(ARROWFUNCTION) }

> Source/JavaScriptCore/parser/Parser.cpp:388
> +    } else if (consume(OPENPAREN)) {

Then I would change this to a pure else clause.
Then, I would have the first two lines be:
RELEASE_ASSERT(isOpenParen); // your check at the top of the function guarantees it, so lets not pretend it isn't true.
next();

> Source/JavaScriptCore/parser/Parser.cpp:390
> +            isArrowFunction = consume(CLOSEPAREN) && match(ARROWFUNCTION);

I would write this as:
if {
    next();
    isArrowFunction = match(ARROWFUNCTION);
}

> Source/JavaScriptCore/parser/Parser.cpp:393
> +                // We make fake scope, otherwise parseFormalParameters will add variable to current scope that lead to errors

style: indentation.

[GSkachkov]

Created attachment 269172 [details]
Patch

Patch

[GSkachkov]

Comment on attachment 268951 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:382
>> +        return isArrowFunction;
> 
> I would write this like:
> ```
> if (match(EOFTOK))
>     return false;
> bool isOpenParen = match(OPENPAREN);
> bool isIdent = match(IDENT);
> if (!isOpenParen && !isIdent)
>     return false;
> ```
> Then, below, you can use these booleans instead of performing the match(...) twice.
> I say this because I think that LLVM will CSE this, but lets take matters into our own hand.

This condition is refactored

[WebKit Commit Bot]

Comment on attachment 269172 [details]
Patch

Clearing flags on attachment: 269172

Committed r195178: <http://trac.webkit.org/changeset/195178>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.