Bug 144955 - [ES6] Implement ES6 arrow function syntax. Parser of arrow function with execution as common function
Summary: [ES6] Implement ES6 arrow function syntax. Parser of arrow function with exec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 144954 145108 146344
Blocks: 140855 144956 145132 145638
  Show dependency treegraph
 
Reported: 2015-05-13 10:42 PDT by GSkachkov
Modified: 2015-06-26 05:13 PDT (History)
12 users (show)

See Also:


Attachments
Patch (93.51 KB, patch)
2015-05-13 11:44 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (91.95 KB, patch)
2015-05-13 15:53 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (92.14 KB, patch)
2015-05-15 06:24 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (57.94 KB, patch)
2015-05-17 14:15 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (57.93 KB, patch)
2015-05-18 23:16 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (57.84 KB, patch)
2015-05-19 04:15 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (74.60 KB, patch)
2015-06-02 12:58 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (74.61 KB, patch)
2015-06-02 13:16 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (74.60 KB, patch)
2015-06-02 14:02 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (1.03 MB, application/zip)
2015-06-02 15:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-mavericks (775.77 KB, application/zip)
2015-06-02 15:42 PDT, Build Bot
no flags Details
Patch (71.36 KB, patch)
2015-06-03 04:30 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (71.36 KB, patch)
2015-06-03 07:42 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (71.34 KB, patch)
2015-06-03 14:29 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (71.22 KB, patch)
2015-06-03 21:24 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (68.70 KB, patch)
2015-06-07 09:58 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (76.63 KB, patch)
2015-06-07 11:29 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (80.38 KB, patch)
2015-06-11 15:41 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (90.84 KB, patch)
2015-06-19 07:09 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (90.92 KB, patch)
2015-06-19 07:52 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (88.82 KB, patch)
2015-06-23 00:24 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (88.82 KB, patch)
2015-06-23 02:40 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (88.62 KB, patch)
2015-06-23 03:00 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (88.83 KB, patch)
2015-06-23 13:20 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (87.88 KB, patch)
2015-06-24 13:02 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (87.34 KB, patch)
2015-06-25 16:00 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (87.07 KB, patch)
2015-06-25 16:16 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (87.19 KB, patch)
2015-06-25 16:31 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (87.13 KB, patch)
2015-06-25 22:26 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (87.16 KB, patch)
2015-06-25 22:35 PDT, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2015-05-13 10:42:48 PDT
Implement of the parser of arrow function with execution as common function. Part of the 140855 task.
Comment 1 GSkachkov 2015-05-13 11:44:39 PDT
Created attachment 253039 [details]
Patch
Comment 2 WebKit Commit Bot 2015-05-13 11:47:46 PDT
Attachment 253039 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 GSkachkov 2015-05-13 15:53:20 PDT
Created attachment 253069 [details]
Patch
Comment 4 WebKit Commit Bot 2015-05-13 15:55:38 PDT
Attachment 253069 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 GSkachkov 2015-05-15 06:24:48 PDT
Created attachment 253195 [details]
Patch
Comment 6 WebKit Commit Bot 2015-05-15 07:07:42 PDT
Attachment 253195 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Ryosuke Niwa 2015-05-15 14:33:12 PDT
Comment on attachment 253195 [details]
Patch

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

> Source/JavaScriptCore/parser/ASTBuilder.h:372
> +        ? m_sourceCode->subArrowExpression(info.arrowFunctionOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn)
> +        : m_sourceCode->subExpression(info.openBraceOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn);

Wrong indentation. ? and : should be exactly 4 spaces to the right from the beginning of "SourceCode source".

> Source/JavaScriptCore/parser/ASTBuilder.h:375
> +        info.body->setLoc(info.bodyStartLine, info.bodyEndLine, location.startOffset, location.lineStartOffset);

Ditto. Indent the second line by exactly 4 spaces further to the right (12 total spaces).

> Source/JavaScriptCore/parser/Lexer.cpp:1824
> +            tokenData->line = savedlineNumber;
> +            tokenData->offset = savedOffset;
> +            tokenData->lineStartOffset = savedLineStartOffset;
> +
> +            ASSERT(tokenData->offset >= tokenData->lineStartOffset);

I don't think we want to shift() and then set offset back manually like this.
We should use peek() instead.

> Source/JavaScriptCore/parser/Parser.cpp:605
> +    setEndOfStatement();

Why do we need this?

> Source/JavaScriptCore/parser/Parser.cpp:1203
> +            context.setEndOffset(result, m_lastTokenEndPosition.offset);

What is this setEndOffset for?

> Source/JavaScriptCore/parser/Parser.cpp:1260
> +        // Consume the end of arrow function ,;]) and prevent
> +        // parse this symbols by parseStatement as separate statement

I don't understand this comment.
In general, we prefer adding assertions to document conditions rather than adding comments.
Is there a way to express this as an assertion instead?

> Source/JavaScriptCore/parser/Parser.cpp:1341
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +    if (parseType == StandardFunctionParseType) {
> +        next();
> +        if (match(CLOSEBRACE)) {
> +            unsigned endColumn = tokenColumn();
> +            return context.createFunctionBody(startLocation, tokenLocation(), startColumn, endColumn, functionKeywordStart, functionNameStart, parametersStart, strictMode(), constructorKind);
> +        }
> +    }
> +#else

It's not great to duplicate the code like this.
Since FunctionParseType is always define, we should just use that without wrapping in if-def.
Comment 8 Saam Barati 2015-05-15 19:48:28 PDT
(In reply to comment #7)
> Comment on attachment 253195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253195&action=review
> 
> > Source/JavaScriptCore/parser/ASTBuilder.h:372
> > +        ? m_sourceCode->subArrowExpression(info.arrowFunctionOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn)
> > +        : m_sourceCode->subExpression(info.openBraceOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn);
> 
> Wrong indentation. ? and : should be exactly 4 spaces to the right from the
> beginning of "SourceCode source".
> 
> > Source/JavaScriptCore/parser/ASTBuilder.h:375
> > +        info.body->setLoc(info.bodyStartLine, info.bodyEndLine, location.startOffset, location.lineStartOffset);
> 
> Ditto. Indent the second line by exactly 4 spaces further to the right (12
> total spaces).
> 
> > Source/JavaScriptCore/parser/Lexer.cpp:1824
> > +            tokenData->line = savedlineNumber;
> > +            tokenData->offset = savedOffset;
> > +            tokenData->lineStartOffset = savedLineStartOffset;
> > +
> > +            ASSERT(tokenData->offset >= tokenData->lineStartOffset);
> 
> I don't think we want to shift() and then set offset back manually like this.
> We should use peek() instead.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:605
> > +    setEndOfStatement();
> 
> Why do we need this?
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1203
> > +            context.setEndOffset(result, m_lastTokenEndPosition.offset);
> 
> What is this setEndOffset for?
I believe this is for the controlFlowProfiler

> 
> > Source/JavaScriptCore/parser/Parser.cpp:1260
> > +        // Consume the end of arrow function ,;]) and prevent
> > +        // parse this symbols by parseStatement as separate statement
> 
> I don't understand this comment.
> In general, we prefer adding assertions to document conditions rather than
> adding comments.
> Is there a way to express this as an assertion instead?
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1341
> > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> > +    if (parseType == StandardFunctionParseType) {
> > +        next();
> > +        if (match(CLOSEBRACE)) {
> > +            unsigned endColumn = tokenColumn();
> > +            return context.createFunctionBody(startLocation, tokenLocation(), startColumn, endColumn, functionKeywordStart, functionNameStart, parametersStart, strictMode(), constructorKind);
> > +        }
> > +    }
> > +#else
> 
> It's not great to duplicate the code like this.
> Since FunctionParseType is always define, we should just use that without
> wrapping in if-def.
Comment 9 Saam Barati 2015-05-16 00:58:40 PDT
Comment on attachment 253195 [details]
Patch

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

This looks pretty good to me. 
It may be worth opening up some follow up bugs that
will be aimed at getting this to be spec compliant (i.e lexical this, etc)
and adding this bug as blocking those bugs. It will make it explicit
what this patch does and doesn't have implemented.

>> Source/JavaScriptCore/parser/Parser.cpp:1203
>> +            context.setEndOffset(result, m_lastTokenEndPosition.offset);
> 
> What is this setEndOffset for?

Actually, this is wrong. How does this code not segfault?
setEndOffset is meant to be called with non-null AST nodes

> Source/JavaScriptCore/parser/Parser.cpp:1427
> +            consume(OPENPAREN);

Nit: you can use next() here. We already know it's an open paren

> Source/JavaScriptCore/parser/Parser.cpp:2852
> +    while (newCount--)

This should be tested.

> Source/JavaScriptCore/parser/Parser.cpp:2854
> +    return base;

Do we currently throw when trying to 'new' an arrow function?
Comment 10 GSkachkov 2015-05-17 14:15:17 PDT
Created attachment 253296 [details]
Patch
Comment 11 GSkachkov 2015-05-17 14:31:39 PDT
I've extracted all changes that related to the adding of the arrow function feature flag to the project in another task https://bugs.webkit.org/show_bug.cgi?id=145108, current task is depends on 145108, and can be landed only after 145108.
Comment 12 GSkachkov 2015-05-18 09:36:34 PDT
Comment on attachment 253195 [details]
Patch

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

>>> Source/JavaScriptCore/parser/ASTBuilder.h:372
>>> +        : m_sourceCode->subExpression(info.openBraceOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn);
>> 
>> Wrong indentation. ? and : should be exactly 4 spaces to the right from the beginning of "SourceCode source".
> 
> I believe this is for the controlFlowProfiler

Fixed. Please check if correct.

>> Source/JavaScriptCore/parser/ASTBuilder.h:375
>> +        info.body->setLoc(info.bodyStartLine, info.bodyEndLine, location.startOffset, location.lineStartOffset);
> 
> Ditto. Indent the second line by exactly 4 spaces further to the right (12 total spaces).

Why do we need indent for 'info.body->setLoc()' statement?

>> Source/JavaScriptCore/parser/Lexer.cpp:1824
>> +            ASSERT(tokenData->offset >= tokenData->lineStartOffset);
> 
> I don't think we want to shift() and then set offset back manually like this.
> We should use peek() instead.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:605
>> +    setEndOfStatement();
> 
> Why do we need this?

Ohh, it will be difficult to explain with my English, but I'll try. 
I need invoke this function to set correctly position inside of the lexer. It is added by me in Source/JavaScriptCore/parser/Lexer.cpp line 1723 and repeat the same statement as do for '}'  in lexer https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Lexer.cpp#L1990. 
It is necessary because incase of arrow function with expression (var f = x=> x+1;) symbols ';', ']', '}', ')', and ',' has two meaning first it is end of statement and  second end of arrow function body. I haven't found way move this logic to lexer because only parser knowing context when it is only end of statement or end of arrow function body as well.

>>> Source/JavaScriptCore/parser/Parser.cpp:1203
>>> +            context.setEndOffset(result, m_lastTokenEndPosition.offset);
>> 
>> What is this setEndOffset for?
> 
> Actually, this is wrong. How does this code not segfault?
> setEndOffset is meant to be called with non-null AST nodes

Hmm, I run this method to set correct ending of the block for arrow function with expression for instance var f = x => x + 10; 
Where ';' is used as end of the statement var f...; and end of arrow function x=>x+10;
For the same purpose as in this method https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Parser.cpp#L1426

>> Source/JavaScriptCore/parser/Parser.cpp:2852
>> +    while (newCount--)
> 
> This should be tested.

I've removed this code.
Comment 13 GSkachkov 2015-05-18 11:18:36 PDT
(In reply to comment #9)
> Comment on attachment 253195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253195&action=review
> 
> This looks pretty good to me. 
> It may be worth opening up some follow up bugs that
> will be aimed at getting this to be spec compliant (i.e lexical this, etc)
> and adding this bug as blocking those bugs. It will make it explicit
> what this patch does and doesn't have implemented.
> 
> >> Source/JavaScriptCore/parser/Parser.cpp:1203
> >> +            context.setEndOffset(result, m_lastTokenEndPosition.offset);
> > 
> > What is this setEndOffset for?
> 
> Actually, this is wrong. How does this code not segfault?
> setEndOffset is meant to be called with non-null AST nodes
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1427
> > +            consume(OPENPAREN);
> 
> Nit: you can use next() here. We already know it's an open paren
> 
> > Source/JavaScriptCore/parser/Parser.cpp:2852
> > +    while (newCount--)
> 
> This should be tested.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:2854
> > +    return base;
> 
> Do we currently throw when trying to 'new' an arrow function?

As for now I've created issue where is implemented lexical bind of 'this' and raising exception in case of trying use arrow function with new https://bugs.webkit.org/show_bug.cgi?id=144956, 
also I've add new one	https://bugs.webkit.org/show_bug.cgi?id=145132 and will add one more later.
Comment 14 WebKit Commit Bot 2015-05-18 14:15:53 PDT
Attachment 253296 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:2836:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 3 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 GSkachkov 2015-05-18 23:16:09 PDT
Created attachment 253369 [details]
Patch
Comment 16 WebKit Commit Bot 2015-05-18 23:20:22 PDT
Attachment 253369 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Saam Barati 2015-05-19 00:44:14 PDT
Comment on attachment 253195 [details]
Patch

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

>>>>> Source/JavaScriptCore/parser/Parser.cpp:1203
>>>>> +            context.setEndOffset(result, m_lastTokenEndPosition.offset);
>>>> 
>>>> What is this setEndOffset for?
>>> 
>>> Actually, this is wrong. How does this code not segfault?
>>> setEndOffset is meant to be called with non-null AST nodes
>> 
>> Hmm, I run this method to set correct ending of the block for arrow function with expression for instance var f = x => x + 10; 
>> Where ';' is used as end of the statement var f...; and end of arrow function x=>x+10;
>> For the same purpose as in this method https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Parser.cpp#L1426
> 
> As for now I've created issue where is implemented lexical bind of 'this' and raising exception in case of trying use arrow function with new https://bugs.webkit.org/show_bug.cgi?id=144956, 
> also I've add new one	https://bugs.webkit.org/show_bug.cgi?id=145132 and will add one more later.

Right. But, if you look at the code for setEndOffset, it will deference the AST node you pass it (inside ASTBuilder, not SyntaxChecker).
This will be bad because we're passing in null for that AST node.

On another note, this code path seems a little bit weird to me. Is there an alternate
way to check the end of an arrow function that isn't inside parseStatement? 
To me, this seems like a weird place for this code to live.
Comment 18 GSkachkov 2015-05-19 04:15:36 PDT
Created attachment 253378 [details]
Patch
Comment 19 WebKit Commit Bot 2015-05-19 04:18:34 PDT
Attachment 253378 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.h:738:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 GSkachkov 2015-05-19 04:23:37 PDT
(In reply to comment #17)
> Comment on attachment 253195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253195&action=review
> 
> >>>>> Source/JavaScriptCore/parser/Parser.cpp:1203
> >>>>> +            context.setEndOffset(result, m_lastTokenEndPosition.offset);
> >>>> 
> >>>> What is this setEndOffset for?
> >>> 
> >>> Actually, this is wrong. How does this code not segfault?
> >>> setEndOffset is meant to be called with non-null AST nodes
> >> 
> >> Hmm, I run this method to set correct ending of the block for arrow function with expression for instance var f = x => x + 10; 
> >> Where ';' is used as end of the statement var f...; and end of arrow function x=>x+10;
> >> For the same purpose as in this method https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/parser/Parser.cpp#L1426
> > 
> > As for now I've created issue where is implemented lexical bind of 'this' and raising exception in case of trying use arrow function with new https://bugs.webkit.org/show_bug.cgi?id=144956, 
> > also I've add new one	https://bugs.webkit.org/show_bug.cgi?id=145132 and will add one more later.
> 
> Right. But, if you look at the code for setEndOffset, it will deference the
> AST node you pass it (inside ASTBuilder, not SyntaxChecker).
> This will be bad because we're passing in null for that AST node.
> 
> On another note, this code path seems a little bit weird to me. Is there an
> alternate
> way to check the end of an arrow function that isn't inside parseStatement? 
> To me, this seems like a weird place for this code to live.

I've checked it seems that this statement is unnecessary. So I've removed it. Tests are still green.
Comment 21 GSkachkov 2015-05-20 05:18:18 PDT
Comment on attachment 253378 [details]
Patch

Found broken test. Cancel review until fix.
Comment 22 GSkachkov 2015-05-20 08:20:03 PDT
Comment on attachment 253378 [details]
Patch

Test are green now.
Comment 23 Yusuke Suzuki 2015-05-25 10:45:46 PDT
Comment on attachment 253378 [details]
Patch

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

Quick comment.
Maybe I need to learn the control flow profiler context. Saam is good reviewer for the control flow profiler part.
I'll review it again. great work!

> Source/JavaScriptCore/parser/Parser.cpp:1202
> +            return result;

Is there any reason not using `isEndOfArrowFunction`?

> Source/JavaScriptCore/parser/Parser.cpp:1259
> +        break;

This code makes that `=>{}` can be parsed.
But it should be SyntaxError.
And when passing `=>{}`, assertion failure occurs.

> Source/JavaScriptCore/parser/Parser.cpp:1613
> +        failIfFalse(isEndOfArrowFunction(), "Expected the closing ';' ',' ']' ')' '}' after arrow function");

Here, it doesn't allow EOF. So, it seems that `()=>e<EOF>` becomes SyntaxError.
Is there any reason not adding EOF to isEndOfArrowFunction?

> Source/JavaScriptCore/parser/Parser.cpp:2832
> +        functionKeywordStart, info, ArrowFunctionParseType)), "Cannot parse arrow function expression");

Nits: Line breaking is not needed.
Comment 24 Saam Barati 2015-05-25 23:21:59 PDT
Comment on attachment 253378 [details]
Patch

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

Patch looks good to me. Great work.
Some suggestions below.

> Source/JavaScriptCore/ChangeLog:9
> +        In patch were implemented the simplest cases of arrow function declaration:

"In patch were implemented" => "This patch implements"

> Source/JavaScriptCore/ChangeLog:17
> +          2 raising exception in case of trying to use ânewâ with arrow function

Nit: Looks like some weird characters around the "new" here.

> Source/JavaScriptCore/parser/ASTBuilder.h:372
> +            : m_sourceCode->subExpression(info.openBraceOffset, info.closeBraceOffset, info.bodyStartLine, info.bodyStartColumn);

Nit: It may be worth renaming ParserFunctionInfo's "openBraceOffset" variable to "bodyStartOffset" or something like that and then this ternary expression becomes unnecessary.

> Source/JavaScriptCore/parser/Parser.cpp:1968
> +template <class TreeBuilder> TreeExpression Parser<LexerType>::parseArrowFunctionExpression(TreeBuilder& context)

In regards to the controlFlowProfiler, this code is ok. To be sure and prevent regressions, it's worth writing a few simple tests for the controlFlowProfiler.
If you look inside JavaScriptCore/tests/controlFlowProfiler/* there is a standardized way of doing this. You can do this in another patch or you can
open a bug and assign it to me and I'll write some tests for it. 

On another note, I don't think this code path is worthy of being its own function. I'd just inline this inside parseArrowFunctionStatement because
this looks like it's a simple wrapper around parseAssignmentExpression.

> Source/JavaScriptCore/parser/Parser.cpp:2026
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)

Nit: Would this whole sequence of checking if something is an arrow function be worthy of its own function?

> Source/JavaScriptCore/parser/Parser.cpp:2034
> +            if (match(ARROWFUNCTION))

This code seems unnecessarily convoluted. I'd move the below code inside the "if (match(ARROWFUNCTION))" into this branch.
If we reach this branch we're guaranteed to execute the code of parseArrowFunctionWrapper in the below branch.
That way we don't have to worry about the arrowFunctionEmptyList variable and we don't duplicate code inside the "#if/#else".

> Source/JavaScriptCore/parser/SourceCode.h:129
> +        ASSERT(provider()->source()[endArrowFunction - 1] == ',' || provider()->source()[endArrowFunction - 1] == ';' || provider()->source()[endArrowFunction - 1] == ')' || provider()->source()[endArrowFunction - 1] == ']' || provider()->source()[endArrowFunction - 1] == '}');

I'd just cache the character in a local variable and remove the Fixme and switch to ASSERT_UNUSED.

> Source/JavaScriptCore/tests/stress/arrowfunction-asparamter-1.js:1
> +// Inspired by mozilla tests

I think some of these tests don't make sense as "stress tests". 
Maybe they're better inside LayoutTests where  we have some similar tests.
Comment 25 GSkachkov 2015-06-02 12:58:31 PDT
Created attachment 254083 [details]
Patch
Comment 26 GSkachkov 2015-06-02 13:16:13 PDT
Created attachment 254087 [details]
Patch
Comment 27 WebKit Commit Bot 2015-06-02 13:20:03 PDT
Attachment 254087 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:752:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 GSkachkov 2015-06-02 14:02:55 PDT
Created attachment 254098 [details]
Patch
Comment 29 WebKit Commit Bot 2015-06-02 14:10:50 PDT
Attachment 254098 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:752:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Build Bot 2015-06-02 15:37:07 PDT
Comment on attachment 254098 [details]
Patch

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

New failing tests:
js/arrowfunction-syntax.html
js/arrowfunction-asparamter-2.html
js/arrowfunction-block-2.html
js/arrowfunction-associativity-2.html
js/arrowfunction-syntax-endings.html
js/arrowfunction-asparamter-1.html
js/arrowfunction-block-1.html
Comment 31 Build Bot 2015-06-02 15:37:13 PDT
Created attachment 254114 [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
Comment 32 Build Bot 2015-06-02 15:42:37 PDT
Comment on attachment 254098 [details]
Patch

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

New failing tests:
js/arrowfunction-syntax.html
js/arrowfunction-asparamter-2.html
js/arrowfunction-block-2.html
js/arrowfunction-associativity-2.html
js/arrowfunction-syntax-endings.html
js/arrowfunction-asparamter-1.html
js/arrowfunction-block-1.html
Comment 33 Build Bot 2015-06-02 15:42:42 PDT
Created attachment 254118 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 34 GSkachkov 2015-06-03 04:30:52 PDT
Created attachment 254166 [details]
Patch
Comment 35 WebKit Commit Bot 2015-06-03 04:45:31 PDT
Attachment 254166 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:752:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 36 GSkachkov 2015-06-03 07:42:05 PDT
Created attachment 254174 [details]
Patch
Comment 37 WebKit Commit Bot 2015-06-03 07:54:01 PDT
Attachment 254174 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:752:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 Darin Adler 2015-06-03 13:41:18 PDT
(In reply to comment #37)
> ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should
> use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> ERROR: Source/JavaScriptCore/parser/Parser.h:752:  The parameter name
> "functionParseType" adds no information, so it should be removed. 
> [readability/parameter_name] [5]

Please fix these!
Comment 39 GSkachkov 2015-06-03 14:03:30 PDT
(In reply to comment #38)
> (In reply to comment #37)
> > ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should
> > use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> > ERROR: Source/JavaScriptCore/parser/Parser.h:752:  The parameter name
> > "functionParseType" adds no information, so it should be removed. 
> > [readability/parameter_name] [5]
> 
> Please fix these!

Not sure that I know how. Could you please suggest how to fix them?

1. Source/JavaScriptCore/parser/ParserTokens.h:79
I've added new value ARROWFUNCTION and followed the same style as all enums in this file http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/parser/ParserTokens.h#L75

2. Source/JavaScriptCore/parser/Parser.h 
In this file I've added new parameter with default value in exist function. I've used default value to decrease number of changes, so to default value I had to add name of the parameter.

template <class TreeBuilder> TreeStatement parseStatement(TreeBuilder&, const Identifier*& directive, unsigned* directiveLiteralLength = 0, !!!!FunctionParseType functionParseType = StandardFunctionParseType!!!!)
Comment 40 Darin Adler 2015-06-03 14:12:22 PDT
(In reply to comment #39)
> 1. Source/JavaScriptCore/parser/ParserTokens.h:79
> I've added new value ARROWFUNCTION and followed the same style as all enums
> in this file
> http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/parser/
> ParserTokens.h#L75

This one is really strange. I’m not sure why we decided to use all caps for all these tokens. Worth discussing with someone, but for now I suppose you should match the style.

> 2. Source/JavaScriptCore/parser/Parser.h 
> In this file I've added new parameter with default value in exist function.
> I've used default value to decrease number of changes, so to default value I
> had to add name of the parameter.
> 
> template <class TreeBuilder> TreeStatement parseStatement(TreeBuilder&,
> const Identifier*& directive, unsigned* directiveLiteralLength = 0,
> !!!!FunctionParseType functionParseType = StandardFunctionParseType!!!!)

You don’t need am argument name to have a default value:

    [...] unsigned* directiveLiteralLength = nullptr, FunctionParseType = StandardFunctionParseType)
Comment 41 GSkachkov 2015-06-03 14:29:07 PDT
Created attachment 254209 [details]
Patch
Comment 42 WebKit Commit Bot 2015-06-03 14:44:07 PDT
Attachment 254209 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 GSkachkov 2015-06-03 21:24:40 PDT
Created attachment 254239 [details]
Patch
Comment 44 WebKit Commit Bot 2015-06-03 21:50:03 PDT
Attachment 254239 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 45 GSkachkov 2015-06-03 23:51:18 PDT
(In reply to comment #40)
> (In reply to comment #39)
> > 1. Source/JavaScriptCore/parser/ParserTokens.h:79
> > I've added new value ARROWFUNCTION and followed the same style as all enums
> > in this file
> > http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/parser/
> > ParserTokens.h#L75
> 
> This one is really strange. I’m not sure why we decided to use all caps for
> all these tokens. Worth discussing with someone, but for now I suppose you
> should match the style.
> 
> > 2. Source/JavaScriptCore/parser/Parser.h 
> > In this file I've added new parameter with default value in exist function.
> > I've used default value to decrease number of changes, so to default value I
> > had to add name of the parameter.
> > 
> > template <class TreeBuilder> TreeStatement parseStatement(TreeBuilder&,
> > const Identifier*& directive, unsigned* directiveLiteralLength = 0,
> > !!!!FunctionParseType functionParseType = StandardFunctionParseType!!!!)
> 
> You don’t need am argument name to have a default value:
> 
>     [...] unsigned* directiveLiteralLength = nullptr, FunctionParseType =
> StandardFunctionParseType)

Thanks for hint! I've loaded new version with fix. Could you please suggest what I need to fix also in this patch.
Comment 46 Darin Adler 2015-06-06 09:43:44 PDT
I don’t think I’m the right reviewer for this new JavaScript engine feature. Geoff, who should review?
Comment 47 Yusuke Suzuki 2015-06-06 09:49:24 PDT
Comment on attachment 254239 [details]
Patch

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

I can review (partially) this if it's limited to parser's change. In profiler part, I think I'm not a right reviewer.

> Source/JavaScriptCore/parser/Parser.cpp:2023
> +    bool doParseConditionalExpression = false;

I think `shouldParseConditionalExpression` is better.

> Source/JavaScriptCore/parser/Parser.cpp:2027
> +    doParseConditionalExpression = !isArrowFunctionEmptyParamterList();

In isArrowFunctionEmptyPararmeterList, we create SavePoint. So I think the above `createSavevPoint` and SavePoint are duplicate.
Could you change to the implementation that `isArrowFunctionEmptyParamterList` always restore the state?
With that implementation, the above SavePoint is not necessary.
Comment 48 Saam Barati 2015-06-06 22:56:31 PDT
Comment on attachment 254239 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:2027
>> +    doParseConditionalExpression = !isArrowFunctionEmptyParamterList();
> 
> In isArrowFunctionEmptyPararmeterList, we create SavePoint. So I think the above `createSavevPoint` and SavePoint are duplicate.
> Could you change to the implementation that `isArrowFunctionEmptyParamterList` always restore the state?
> With that implementation, the above SavePoint is not necessary.

I agree with Yusuke here, the implementation should always restore the save point. It's weird
for a function that tests this to sometimes restore and sometimes not depending on if it found 
a match. You can have that function always restore the save point and then ASSERT that
the assumption is held. Also, the structuring of these two if statements is weird to me, we know
for sure one or the other will execute. I'd structure it such that if we match an empty arrow function
then we early return and then remove the doParseConditionalExpression. Like so:
if (emptyArrow) {
   ASSERT (...)
    Blah
   return ...
}

TreeExpression lhs = parseConditionalExpression(...)
...

> Source/JavaScriptCore/tests/stress/arrowfunction-syntax-error.js:9
> +  var errorSyntaxError = false;

Should we be doing something with the "statement" parameter?

> Source/JavaScriptCore/tests/stress/arrowfunction-syntax-error.js:27
> +testCase(forceRaiseException('debug(=>x+1)'), true, "Error: No exception during parse wrong statement '=>{}'");

Also add a test for this syntax error: "x, y => ..."
(And maybe other cases, I'm not super familiar with the grammar on arrow functions.)
Comment 49 GSkachkov 2015-06-07 09:58:09 PDT
Created attachment 254438 [details]
Patch
Comment 50 WebKit Commit Bot 2015-06-07 10:03:07 PDT
Attachment 254438 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:638:  Missing space before ( in while(  [whitespace/parens] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:654:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 51 GSkachkov 2015-06-07 11:29:13 PDT
Created attachment 254445 [details]
Patch
Comment 52 WebKit Commit Bot 2015-06-07 11:35:39 PDT
Attachment 254445 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 53 GSkachkov 2015-06-07 12:17:44 PDT
(In reply to comment #47)
> Comment on attachment 254239 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254239&action=review
> 
> I can review (partially) this if it's limited to parser's change. In
> profiler part, I think I'm not a right reviewer.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:2023
> > +    bool doParseConditionalExpression = false;
> 
> I think `shouldParseConditionalExpression` is better.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:2027
> > +    doParseConditionalExpression = !isArrowFunctionEmptyParamterList();
> 
> In isArrowFunctionEmptyPararmeterList, we create SavePoint. So I think the
> above `createSavevPoint` and SavePoint are duplicate.
> Could you change to the implementation that
> `isArrowFunctionEmptyParamterList` always restore the state?
> With that implementation, the above SavePoint is not necessary.

I've fixed last comments. What do you think, who can help with profiler part?
Comment 54 GSkachkov 2015-06-08 06:30:35 PDT
(In reply to comment #24)
> Comment on attachment 253378 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253378&action=review
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1968
> > +template <class TreeBuilder> TreeExpression Parser<LexerType>::parseArrowFunctionExpression(TreeBuilder& context)
> 
> In regards to the controlFlowProfiler, this code is ok. To be sure and
> prevent regressions, it's worth writing a few simple tests for the
> controlFlowProfiler.
> If you look inside JavaScriptCore/tests/controlFlowProfiler/* there is a
> standardized way of doing this. You can do this in another patch or you can
> open a bug and assign it to me and I'll write some tests for it. 
> 
I've created the issue and upload patch with controlFlowProfiler
https://bugs.webkit.org/show_bug.cgi?id=145638. Could you please take a look if it make sense?
Comment 55 Yusuke Suzuki 2015-06-08 14:05:46 PDT
Comment on attachment 254445 [details]
Patch

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

Great.

> Source/JavaScriptCore/parser/Parser.cpp:607
> +    bool correctArrowFinish = isEndOfArrowFunction() || match(EOFTOK);

`match(EOFTOK)` is contained in `isEndOfArrowFunction()`. So I think `match(EOFTOK)` is not needed. Correct?
If so, we can change it to `failIfFalse(isEndOfArrowFunction(), "...");` by merging the following check.

> Source/JavaScriptCore/parser/Parser.cpp:608
> +    failIfFalse(correctArrowFinish, "Expected a ';', ']', '}', ')', or ',' following a arrow function statement");

it seems refering to EOF and Line terminator is needed.

> Source/JavaScriptCore/parser/Parser.cpp:1257
> +        result = parseArrowFunctionStatement(context, parseType);

This `parseArrowFunctionStatement` name confuses me.
This function is used when

(e) `=> expr;`

part, right? If so, I think clearer name is encouraged.

> Source/JavaScriptCore/parser/Parser.cpp:1407
> +template <typename LexerType> template <class TreeBuilder> int Parser<LexerType>::parseArrowFunctionParameters(TreeBuilder& context, FunctionParseMode mode, ParserFunctionInfo<TreeBuilder>& info)

Why parseArrowFunctionParameters is needed in addition to `parseFunctionParameters`?
Is it possible just using `parseFunctionParameters`?

> Source/JavaScriptCore/parser/Parser.cpp:1585
> +        position.offset++;

Is it handled when <CR><LF> comes? Hm, this part looks a little bit weird.

> Source/JavaScriptCore/parser/Parser.cpp:1621
> +        failIfFalse(isEndOfArrowFunction(), "Expected the closing ';' ',' ']' ')' '}' after arrow function");

it seems refering to EOF and Line terminator is needed.

> Source/JavaScriptCore/parser/Parser.h:632
> +        if (match(EOFTOK))
> +            return isArrowFunction;
> +        

Is the above match(EOFTOK) check needed?

> Source/JavaScriptCore/parser/SourceCode.h:130
> +            UChar symbol = provider()->source()[endArrowFunction - 1];

I suggest adding `ASSERT(endArrowFunction >= 1);` if it's guaranteed.

> Source/JavaScriptCore/parser/SourceCode.h:131
> +            ASSERT_UNUSED(symbol, symbol == 13 || symbol == 10 || symbol == ',' || symbol == ';' || symbol == ')' || symbol == ']' || symbol == '}');

What are 13 and 10? Is it possible to use Lexer<...>::isLineTerminator? So are 0x2028/0x2029 is handled? (They are also line terminators.)

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:73
>          token.m_location.endOffset = closeBraceOffset + 1;

Is this `+1` OK for non `CLOSEBRACE` token? For example, If the arrow function expression is splitted by previous line terminators, I think (almost) arbitrary tokens can come here. Correct?
Comment 56 GSkachkov 2015-06-11 15:41:45 PDT
Created attachment 254763 [details]
Patch
Comment 57 WebKit Commit Bot 2015-06-11 15:47:18 PDT
Attachment 254763 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 58 GSkachkov 2015-06-12 00:54:50 PDT
Comment on attachment 254445 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:607
>> +    bool correctArrowFinish = isEndOfArrowFunction() || match(EOFTOK);
> 
> `match(EOFTOK)` is contained in `isEndOfArrowFunction()`. So I think `match(EOFTOK)` is not needed. Correct?
> If so, we can change it to `failIfFalse(isEndOfArrowFunction(), "...");` by merging the following check.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:608
>> +    failIfFalse(correctArrowFinish, "Expected a ';', ']', '}', ')', or ',' following a arrow function statement");
> 
> it seems refering to EOF and Line terminator is needed.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1257
>> +        result = parseArrowFunctionStatement(context, parseType);
> 
> This `parseArrowFunctionStatement` name confuses me.
> This function is used when
> 
> (e) `=> expr;`
> 
> part, right? If so, I think clearer name is encouraged.

I've renamed it to parseArrowFunctionExpression.

>> Source/JavaScriptCore/parser/Parser.cpp:1407
>> +template <typename LexerType> template <class TreeBuilder> int Parser<LexerType>::parseArrowFunctionParameters(TreeBuilder& context, FunctionParseMode mode, ParserFunctionInfo<TreeBuilder>& info)
> 
> Why parseArrowFunctionParameters is needed in addition to `parseFunctionParameters`?
> Is it possible just using `parseFunctionParameters`?

Yep, I've refactored, please take a look if it looks better.

>> Source/JavaScriptCore/parser/Parser.cpp:1585
>> +        position.offset++;
> 
> Is it handled when <CR><LF> comes? Hm, this part looks a little bit weird.

It set position to <CR>, because there are no TOCKEN NextLine. So we back to last before new line and add move one position forward

>> Source/JavaScriptCore/parser/Parser.cpp:1621
>> +        failIfFalse(isEndOfArrowFunction(), "Expected the closing ';' ',' ']' ')' '}' after arrow function");
> 
> it seems refering to EOF and Line terminator is needed.

Done

>> Source/JavaScriptCore/parser/Parser.h:632
>> +        
> 
> Is the above match(EOFTOK) check needed?

Yes. 
It solve case when you at EOF and uses save point. If you do restoreSavePoint(saveArrowFunctionPoint) it will back to one step before EOF

>> Source/JavaScriptCore/parser/SourceCode.h:130
>> +            UChar symbol = provider()->source()[endArrowFunction - 1];
> 
> I suggest adding `ASSERT(endArrowFunction >= 1);` if it's guaranteed.

Done

>> Source/JavaScriptCore/parser/SourceCode.h:131
>> +            ASSERT_UNUSED(symbol, symbol == 13 || symbol == 10 || symbol == ',' || symbol == ';' || symbol == ')' || symbol == ']' || symbol == '}');
> 
> What are 13 and 10? Is it possible to use Lexer<...>::isLineTerminator? So are 0x2028/0x2029 is handled? (They are also line terminators.)

I've added new function isLineTerminator. I'm not sure that it will be good idea to add new dependany to this module. As for me Lexer is from another layer, and SourceCode should not interact with it. What do you think?

>> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:73
>>          token.m_location.endOffset = closeBraceOffset + 1;
> 
> Is this `+1` OK for non `CLOSEBRACE` token? For example, If the arrow function expression is splitted by previous line terminators, I think (almost) arbitrary tokens can come here. Correct?

Thanks for hint, I missed that case. I've added new property in parameters to store if line terminator was before token
Comment 59 Saam Barati 2015-06-12 06:52:59 PDT
(In reply to comment #58)
> Comment on attachment 254445 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254445&action=review
> 
> >> Source/JavaScriptCore/parser/Parser.cpp:607
> >> +    bool correctArrowFinish = isEndOfArrowFunction() || match(EOFTOK);
> > 
> > `match(EOFTOK)` is contained in `isEndOfArrowFunction()`. So I think `match(EOFTOK)` is not needed. Correct?
> > If so, we can change it to `failIfFalse(isEndOfArrowFunction(), "...");` by merging the following check.
> 
> Done
> 
> >> Source/JavaScriptCore/parser/Parser.cpp:608
> >> +    failIfFalse(correctArrowFinish, "Expected a ';', ']', '}', ')', or ',' following a arrow function statement");
> > 
> > it seems refering to EOF and Line terminator is needed.
> 
> Done
> 
> >> Source/JavaScriptCore/parser/Parser.cpp:1257
> >> +        result = parseArrowFunctionStatement(context, parseType);
> > 
> > This `parseArrowFunctionStatement` name confuses me.
> > This function is used when
> > 
> > (e) `=> expr;`
> > 
> > part, right? If so, I think clearer name is encouraged.
> 
> I've renamed it to parseArrowFunctionExpression.
Is this only used when parsing the sinle expression in the body syntax?
(If not, ignore the below.)
If so, I think this is a bad name. To me it sounds like the function
is named as if it's parsing the entire arrow function as an expression.
I'd be more verbose and go with a name like "parseArrowFunctionSingleExpressionBody"
or something else that indicates what the function is doing without any ambiguity.
Comment 60 Yusuke Suzuki 2015-06-12 09:12:58 PDT
Comment on attachment 254445 [details]
Patch

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

>>> Source/JavaScriptCore/parser/Parser.h:632
>>> +        
>> 
>> Is the above match(EOFTOK) check needed?
> 
> Yes. 
> It solve case when you at EOF and uses save point. If you do restoreSavePoint(saveArrowFunctionPoint) it will back to one step before EOF

Make sense.
Comment 61 Yusuke Suzuki 2015-06-12 09:13:39 PDT
Comment on attachment 254763 [details]
Patch

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

Thanks.

> Source/JavaScriptCore/ChangeLog:59
> +        (JSC::SourceProviderCacheItem::endArrowFunctionToken):

It seems that ChangeLog is not updated.
I suggest using `Tools/Scripts/webkit-patch upload --update-changelogs`.

> Source/JavaScriptCore/parser/Parser.cpp:-1326
> -    }

OK, the above sequence is related to the function's name. Splitting out from the `parseFunctionParameters` looks reasonable to me.

> Source/JavaScriptCore/parser/Parser.h:833
> +#endif

Need to drop it.

> Source/JavaScriptCore/parser/SourceCode.h:129
> +    }

Personally I think maintaining 2 same functions is error prone.
In fact, RegExpPrototype.cpp is already using isLineTerminator. So I think using Lexer<UChar>::isLineTerminator not so problematic.
Ideally, extracting these character related function into the different header from Lexer and this is nice.

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:74
>          token.m_location.endOffset = closeBraceOffset + 1;

Hmm, if m_type is not CLOSEBRACE, I think it's not correct because the token's endOffset is not closeBraceOffset + 1.
And I think now `closeBraceOffset` name looks misleading because m_type may not be close brace.
Comment 62 GSkachkov 2015-06-19 07:09:28 PDT
Created attachment 255189 [details]
Patch
Comment 63 GSkachkov 2015-06-19 07:52:08 PDT
Created attachment 255191 [details]
Patch
Comment 64 WebKit Commit Bot 2015-06-19 07:56:15 PDT
Attachment 255191 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 65 GSkachkov 2015-06-19 09:48:25 PDT
Comment on attachment 254763 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.h:833
>> +#endif
> 
> Need to drop it.

Done

>> Source/JavaScriptCore/parser/SourceCode.h:129
>> +    }
> 
> Personally I think maintaining 2 same functions is error prone.
> In fact, RegExpPrototype.cpp is already using isLineTerminator. So I think using Lexer<UChar>::isLineTerminator not so problematic.
> Ideally, extracting these character related function into the different header from Lexer and this is nice.

Lexer.h already is linked to SourceCode.h so we would be had circular dependancy. Anyway I've modified logic little bit and get rid of the LineTerminator check.

>> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:74
>>          token.m_location.endOffset = closeBraceOffset + 1;
> 
> Hmm, if m_type is not CLOSEBRACE, I think it's not correct because the token's endOffset is not closeBraceOffset + 1.
> And I think now `closeBraceOffset` name looks misleading because m_type may not be close brace.

I've added additional parameter to store end of the function
Comment 66 GSkachkov 2015-06-19 09:49:19 PDT
(In reply to comment #61)
> Comment on attachment 254763 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254763&action=review
> 
> Thanks.
> 
> > Source/JavaScriptCore/ChangeLog:59
> > +        (JSC::SourceProviderCacheItem::endArrowFunctionToken):
> 
> It seems that ChangeLog is not updated.
> I suggest using `Tools/Scripts/webkit-patch upload --update-changelogs`.
> 
 done
Comment 67 Yusuke Suzuki 2015-06-19 13:44:05 PDT
Comment on attachment 255191 [details]
Patch

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

Great work. Still a few nits remmain. But logic looks good to me :-)
It seems that there are some unitialized fields. Please ensure that all fields are initialized even in StandardFunction type.

> Source/JavaScriptCore/ChangeLog:60
> +        (JSC::SourceProviderCacheItem::endArrowFunctionToken):

The previous one remains here. Let's drop it :)

> Source/JavaScriptCore/ChangeLog:106
> +        (JSC::SyntaxChecker::setFunctionNameStart):

Ah, when using `Tools/Scripts/webkit-patch upload --update-changelogs` and the ChangeLog is significantly different from the previous version, it appends new logs.
So let's drop the previous one manually.

> Source/JavaScriptCore/parser/Parser.cpp:1270
> +        result = parseArrowFunctionExpression(context, parseType);

Saam's pointing is reasonable. Could you rename it to "parseArrowFunctionSingleExpressionBody"?

> Source/JavaScriptCore/parser/Parser.cpp:1447
> +    bool isClassConstructor;

Let's drop this line.

> Source/JavaScriptCore/parser/Parser.cpp:1472
> +        isClassConstructor = mode == MethodMode && info.name && *info.name == m_vm->propertyNames->constructor;

It seems this line is not necessary, right?

> Source/JavaScriptCore/parser/Parser.cpp:1478
> +        isClassConstructor = constructorKind != ConstructorKind::None;

This always override the previous one. And I think this line is not necessary.

> Source/JavaScriptCore/parser/Parser.cpp:1487
> +        isClassConstructor = false;

I think this line is not necessary. Instead, let's insert `ASSERT(constructorKind == ConstructorKind::None);` (correct?)

> Source/JavaScriptCore/parser/Parser.cpp:-1387
> -    bool isClassConstructor = constructorKind != ConstructorKind::None;

I think using this is enough because constructorKind for `ArrowFunctionParseType` is always ConstructorKind::None.

> Source/JavaScriptCore/parser/Parser.cpp:1495
> +    constructorKind = isClassConstructor ? constructorKind : ConstructorKind::None;

I think this line is not necessary.

> Source/JavaScriptCore/parser/Parser.cpp:1610
> +        info.isEndByTerminator = true;

OK, they are initialized in struct definition with C++11 initialization form.

> Source/JavaScriptCore/parser/Parser.cpp:1635
> +            parameters.isPrevTerminator = m_lexer->prevTerminator();

These fields are not initialized if parseType is not ArrowFunctionParseType.
Since they are primimtive types (unsigned int etc.), use of uninitialized values causes undefined behavior in C++.

Let's take the either way
1. initialize them in the struct definition with C++11 initialization form.
2. initialize them before this if-branch.

> Source/JavaScriptCore/parser/ParserFunctionInfo.h:41
> +    unsigned startFunctionOffset = 0;

Let's exchange this member's order.
    unsigned startFunctionOffset = 0;
    unsigned endFunctionOffset = 0;

> Source/JavaScriptCore/parser/ParserFunctionInfo.h:49
> +    FunctionBodyType functionBodyType = StandardFunctionBodyBlock;

They are initialized with C++ initialization form.

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:39
> +    unsigned endFunctionOffset;

Since now we have endFunctionEndOffset, endFunctionStartOffset sounds better.

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:50
> +    bool isPrevTerminator;

Since the above 3 fields not set if the function is standard function, it becomes undefined values in C++.
Let's use C++11 initialization.

    bool isBodyArrowExpression { false };
    JSTokenType tokenType { CLOSEBRACE };
    bool isPrevTerminator { false };
Comment 68 GSkachkov 2015-06-23 00:24:29 PDT
Created attachment 255398 [details]
Patch
Comment 69 WebKit Commit Bot 2015-06-23 00:30:14 PDT
Attachment 255398 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 70 GSkachkov 2015-06-23 00:49:28 PDT
Comment on attachment 255191 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:106
>> +        (JSC::SyntaxChecker::setFunctionNameStart):
> 
> Ah, when using `Tools/Scripts/webkit-patch upload --update-changelogs` and the ChangeLog is significantly different from the previous version, it appends new logs.
> So let's drop the previous one manually.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1270
>> +        result = parseArrowFunctionExpression(context, parseType);
> 
> Saam's pointing is reasonable. Could you rename it to "parseArrowFunctionSingleExpressionBody"?

Done.

>> Source/JavaScriptCore/parser/Parser.cpp:1447
>> +    bool isClassConstructor;
> 
> Let's drop this line.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1472
>> +        isClassConstructor = mode == MethodMode && info.name && *info.name == m_vm->propertyNames->constructor;
> 
> It seems this line is not necessary, right?

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1478
>> +        isClassConstructor = constructorKind != ConstructorKind::None;
> 
> This always override the previous one. And I think this line is not necessary.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1487
>> +        isClassConstructor = false;
> 
> I think this line is not necessary. Instead, let's insert `ASSERT(constructorKind == ConstructorKind::None);` (correct?)

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1495
>> +    constructorKind = isClassConstructor ? constructorKind : ConstructorKind::None;
> 
> I think this line is not necessary.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1610
>> +        info.isEndByTerminator = true;
> 
> OK, they are initialized in struct definition with C++11 initialization form.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1635
>> +            parameters.isPrevTerminator = m_lexer->prevTerminator();
> 
> These fields are not initialized if parseType is not ArrowFunctionParseType.
> Since they are primimtive types (unsigned int etc.), use of uninitialized values causes undefined behavior in C++.
> 
> Let's take the either way
> 1. initialize them in the struct definition with C++11 initialization form.
> 2. initialize them before this if-branch.

So I've added initializing of this paramters in the struct definition

>> Source/JavaScriptCore/parser/ParserFunctionInfo.h:41
>> +    unsigned startFunctionOffset = 0;
> 
> Let's exchange this member's order.
>     unsigned startFunctionOffset = 0;
>     unsigned endFunctionOffset = 0;

Done

>> Source/JavaScriptCore/parser/ParserFunctionInfo.h:49
>> +    FunctionBodyType functionBodyType = StandardFunctionBodyBlock;
> 
> They are initialized with C++ initialization form.

Done

>> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:39
>> +    unsigned endFunctionOffset;
> 
> Since now we have endFunctionEndOffset, endFunctionStartOffset sounds better.

Done

>> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:50
>> +    bool isPrevTerminator;
> 
> Since the above 3 fields not set if the function is standard function, it becomes undefined values in C++.
> Let's use C++11 initialization.
> 
>     bool isBodyArrowExpression { false };
>     JSTokenType tokenType { CLOSEBRACE };
>     bool isPrevTerminator { false };

Done
Comment 71 Yusuke Suzuki 2015-06-23 02:26:37 PDT
Comment on attachment 255398 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:2055
> +    failIfFalse(lhs, "Cannot parse expression");

Here, ^Y control sequence is inserted.
Comment 72 GSkachkov 2015-06-23 02:40:03 PDT
Created attachment 255400 [details]
Patch
Comment 73 WebKit Commit Bot 2015-06-23 02:45:04 PDT
Attachment 255400 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 74 GSkachkov 2015-06-23 03:00:53 PDT
Created attachment 255401 [details]
Patch
Comment 75 GSkachkov 2015-06-23 13:20:39 PDT
Created attachment 255426 [details]
Patch
Comment 76 WebKit Commit Bot 2015-06-23 13:29:05 PDT
Attachment 255426 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 77 Yusuke Suzuki 2015-06-23 23:57:53 PDT
Comment on attachment 255426 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:355
> +    while (TreeStatement statement = parseStatement(context, directive, &directiveLiteralLength, functionParseType)) {

I think using `parseStatement` for ArrowFunctionBodyExpression case is not good.
Now we have information about it as `functionParseType`.
so instead of using parseArrowFunctionSingleExpressionBody inside parseStatement, just separate path here and use parseArrowFunctionSingleExpressionBody directly.

> Source/JavaScriptCore/parser/Parser.cpp:593
> +        failDueToUnexpectedToken();

Why this is needed?

> Source/JavaScriptCore/parser/Parser.cpp:615
> +        next();

I think `parseType` is always ArrowFunctionParseType. So this is not necessary.

> Source/JavaScriptCore/parser/Parser.cpp:1226
> +#endif

Drop here.

> Source/JavaScriptCore/parser/Parser.cpp:1276
> +#endif

Move this into parseSourceElements (at that time, do not loop.)
So, parseStatement's modification is dropped.

> Source/JavaScriptCore/parser/Parser.cpp:1505
> +    if (const SourceProviderCacheItem* cachedInfo = TreeBuilder::CanUseFunctionCache ? findCachedFunctionInfo(info.startFunctionOffset) : 0) {

We lookup previously parsed function with info.startFunctionOffset key. But this value is later changed for ArrowFunctionBodyBlock case.
So when the type is ArrowFunctionBodyBlock, cachedInfo is never hit.
I've applied this patch to my workingcopy and log the searched `info.startFunctionOffset` and stored offset.

To test it, be carefull about the function's length. If the length is less than or equal to minimumFunctionLengthToCache, cachedInfo is not created.

> Source/JavaScriptCore/parser/Parser.cpp:1575
> +            info.startFunctionOffset = m_token.m_data.offset;

Because of this change, ArrowFunctionBodyBlock cache is never hit.

> Source/JavaScriptCore/parser/Parser.cpp:1620
> +    unsigned endFunctionOffset = location.startOffset;

Here, we just use location.startOffset. Is it OK for ArrowFunctionBodyExpression ended with line terminator? (we set info.endFunctionOffset = location.endOffset in the previous block).
I think dropping this is better to keep consistency with info.endFunctionOffset.

> Source/JavaScriptCore/parser/Parser.cpp:1634
> +        parameters.endFunctionStartOffset = endFunctionOffset;

Use info.endFunctionOffset.

> Source/JavaScriptCore/parser/Parser.cpp:2059
> +        return parseArrowFunctionWrapper(context);

Now, PrimaryExpression part is removed. I think `parseArrowFunctionWrapper` can be renamed to `parseArrowFunctionExpression`.

> Source/JavaScriptCore/parser/Parser.cpp:2846
> +    failIfFalse(match(IDENT) || match(OPENPAREN), "Expected an arrow function input parameter");

This condition is guaranteed by `isArrowFunctionParameters()`. So this can be ASSERT.

> Source/JavaScriptCore/parser/SourceCode.h:128
> +        ASSERT(endArrowFunction >= 1);

OK, now you updated `endArrowFunction`. So this assertion should be removed, correct?
Comment 78 GSkachkov 2015-06-24 13:02:57 PDT
Created attachment 255506 [details]
Patch
Comment 79 WebKit Commit Bot 2015-06-24 13:12:21 PDT
Attachment 255506 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 80 GSkachkov 2015-06-24 13:51:16 PDT
Comment on attachment 255426 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:355
>> +    while (TreeStatement statement = parseStatement(context, directive, &directiveLiteralLength, functionParseType)) {
> 
> I think using `parseStatement` for ArrowFunctionBodyExpression case is not good.
> Now we have information about it as `functionParseType`.
> so instead of using parseArrowFunctionSingleExpressionBody inside parseStatement, just separate path here and use parseArrowFunctionSingleExpressionBody directly.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:593
>> +        failDueToUnexpectedToken();
> 
> Why this is needed?

It is raise exception for following statement '=>{a}' 
This function can be run in several cases:
1)  from parseFunctionBody - correct path
2)  during reparsing  - correct path
3)  from parseSourceElements, when js has nor correct statement  '=>{ return a}'  - wrong path

>> Source/JavaScriptCore/parser/Parser.cpp:615
>> +        next();
> 
> I think `parseType` is always ArrowFunctionParseType. So this is not necessary.

In case of reparsing parseType there will be StandardFunctionParseType

>> Source/JavaScriptCore/parser/Parser.cpp:1226
>> +#endif
> 
> Drop here.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1276
>> +#endif
> 
> Move this into parseSourceElements (at that time, do not loop.)
> So, parseStatement's modification is dropped.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1505
>> +    if (const SourceProviderCacheItem* cachedInfo = TreeBuilder::CanUseFunctionCache ? findCachedFunctionInfo(info.startFunctionOffset) : 0) {
> 
> We lookup previously parsed function with info.startFunctionOffset key. But this value is later changed for ArrowFunctionBodyBlock case.
> So when the type is ArrowFunctionBodyBlock, cachedInfo is never hit.
> I've applied this patch to my workingcopy and log the searched `info.startFunctionOffset` and stored offset.
> 
> To test it, be carefull about the function's length. If the length is less than or equal to minimumFunctionLengthToCache, cachedInfo is not created.

I've moved up logic of the checking type of arrow function and setting the info.startFunctionOffset.

>> Source/JavaScriptCore/parser/Parser.cpp:1575
>> +            info.startFunctionOffset = m_token.m_data.offset;
> 
> Because of this change, ArrowFunctionBodyBlock cache is never hit.

Moved before looking up to cache

>> Source/JavaScriptCore/parser/Parser.cpp:1620
>> +    unsigned endFunctionOffset = location.startOffset;
> 
> Here, we just use location.startOffset. Is it OK for ArrowFunctionBodyExpression ended with line terminator? (we set info.endFunctionOffset = location.endOffset in the previous block).
> I think dropping this is better to keep consistency with info.endFunctionOffset.

There is no tocken line terminator. So I  end offset + 1 position of the tocken before line terminator.

>> Source/JavaScriptCore/parser/Parser.cpp:1634
>> +        parameters.endFunctionStartOffset = endFunctionOffset;
> 
> Use info.endFunctionOffset.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:2059
>> +        return parseArrowFunctionWrapper(context);
> 
> Now, PrimaryExpression part is removed. I think `parseArrowFunctionWrapper` can be renamed to `parseArrowFunctionExpression`.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:2846
>> +    failIfFalse(match(IDENT) || match(OPENPAREN), "Expected an arrow function input parameter");
> 
> This condition is guaranteed by `isArrowFunctionParameters()`. So this can be ASSERT.

Done.

>> Source/JavaScriptCore/parser/SourceCode.h:128
>> +        ASSERT(endArrowFunction >= 1);
> 
> OK, now you updated `endArrowFunction`. So this assertion should be removed, correct?

Correct. It always greater than startArrowFunction and 1. Done
Comment 81 Yusuke Suzuki 2015-06-25 04:50:04 PDT
Comment on attachment 255506 [details]
Patch

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

Great. I think the current token modification code has several issues. I suggest one solution. Please point it if there's an issue.

> Source/JavaScriptCore/parser/Parser.cpp:358
> +        TreeStatement arrowfunctionStatement = parseArrowFunctionSingleExpressionBody(context, functionParseType);

OK, I understand.
This inner branch is intended to be used for ArrowFunctionExpression + ExpressionBody case because in body case ((x) => { } case), ARROWFUNCTION token is already consumed.
However, in reparsing phase, functionParseType becomes StandardFunctionParseType (I think it should be fixed in the separated patch).

The problematic case is that,

{
    => x;
}

This is avoided because,
1. If the current phase is not reparsing phase, functionParseType is restricted to arrow function parse type.
2. If the current phase is reparsing phase, these code is once parsed in non reparsing phase. In reparsing phase, there's no syntax error. So above case is already avoided in the non reparsing phase.

> Source/JavaScriptCore/parser/Parser.cpp:365
> +        if ((functionParseType == ArrowFunctionParseType && isEndOfArrowFunction()) || !arrowfunctionStatement) {

Why this branch is needed?
Is it possible to write just the following here?

        propagateError();
        return sourceElements;

Since this branch is only used for body = expr case. So if we parse one assignment expression in parseArrowFunctionSingleExpressionBody, no further parsing should not be allowed.

> Source/JavaScriptCore/parser/Parser.cpp:368
> +        }

Is here's fall through correct?

> Source/JavaScriptCore/parser/Parser.cpp:610
> +        failDueToUnexpectedToken();

OK, please insert comment about reparsing phase and parseType relation like the following.

// When reparsing phase, parseType becomes StandardFunctionParseType even if the function is arrow function.
// This condition considers the following situations.
// (1): If we are in the reparsing phase, this arrow function is already parsed once, so there is no syntax error.
// (2): But if we are not in the reparsing phase, we should check this function is called in the context of the arrow function.

> Source/JavaScriptCore/parser/Parser.cpp:631
> +    if (parseType == StandardFunctionParseType)

Change it to `m_lexer->isReparsing()`. `parseType == StandardFunctionParseType` is not essential.

BTW, this code path is a little bit complicated. I recommend to insert comment here.

// Only when reparsing phase, our parsing target contains the EndOfArrowFunction token at the end of the code.
// So consuming the last token here is needed.

Correct?

> Source/JavaScriptCore/parser/Parser.cpp:1567
> +        }

Drop them.

> Source/JavaScriptCore/parser/Parser.cpp:1574
> +            next();

Drop this.

> Source/JavaScriptCore/parser/Parser.cpp:1590
> +            info.startFunctionOffset = m_token.m_data.offset;

This line is not needed since it's already set.
I think info.startFunctionOffset should be set before looking up the cache and should not be touched after that.

> Source/JavaScriptCore/parser/Parser.cpp:1625
> +            info.endFunctionOffset = location.endOffset + 1;

Modifying offset is dangerous I think. It easily leads heap overflow.
And as described below, it leads `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`.

I suggest always storing locationBeforeLastToken() in bodyexpression case. And drop prevTerminator information.
It ensures the following invariant.

1. the stored token is always the last token in the function's body. if it using braces ({ ... }), closed brace is stored. And if it takes expr style ((x) => x), the last token `x` is stored.

And when recovering from it, always consume the token.

> Source/JavaScriptCore/parser/Parser.cpp:1629
> +        info.isEndByTerminator = m_lexer->prevTerminator();

Drop isEndByTerminator information.

> Source/JavaScriptCore/parser/Parser.cpp:1647
> +        parameters.endFunctionEndOffset = endFunctionEndOffset;

In `m_lexer->prevTerminator()` case, `info.endFunctionOffset` = `location.endOffset + 1`. And `endFunctionEndOffset` is `location.endOffset`.
So now, `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`. Is it indended?

> Source/JavaScriptCore/parser/Parser.cpp:1663
> +    if ((parseType == ArrowFunctionParseType && info.functionBodyType == ArrowFunctionBodyBlock) || parseType != ArrowFunctionParseType) {

Use `parseType == StandardFunctionParseType` instead of `parseType != ArrowFunctionParseType`.
Comment 82 Yusuke Suzuki 2015-06-25 06:00:20 PDT
Hm, I think modifying index (like endArrowFunction + 1) is not good since it leads heap overrun easily.

So I suggest the following change, GSkachkov, what do you think about it?

1. info.endFunctionOffset is stored independently from the lastToken information. When cacheInfo is hit, just set info.endFunctionOffset = cachedInfo->endFunctionOffset. Not recalculate it.

2. cachedInfo (and param) has lastToken information. Change endFunctionStartOffset etc. to lastTokenStartOffset etc. And endFunctionToken is generated from this information.

3. Store the last token information to cacheInfo. Don't separate pathes for Arrow+Expr, Arrow+Block and Standard. In all cases, just store the last token. And info.endFunctionOffset is adjusted only in ArrowExpr case (lastToken.endOffset). In the other case, use lastToken.startOffset as info.endFunctionOffset. And when using cachedInfo, just set stored token and call `next()` in all cases.

4. Not containing the arrow function expression body's terminator token (like ';') in function code. `next()` in reparsing phase in SingleExpression parsing is removed.

This drops many if-defs, simplifies the code and avoids index modification (like endxxx + 1).
Comment 83 GSkachkov 2015-06-25 16:00:11 PDT
Created attachment 255586 [details]
Patch
Comment 84 GSkachkov 2015-06-25 16:01:24 PDT
Comment on attachment 255506 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:365
>> +        if ((functionParseType == ArrowFunctionParseType && isEndOfArrowFunction()) || !arrowfunctionStatement) {
> 
> Why this branch is needed?
> Is it possible to write just the following here?
> 
>         propagateError();
>         return sourceElements;
> 
> Since this branch is only used for body = expr case. So if we parse one assignment expression in parseArrowFunctionSingleExpressionBody, no further parsing should not be allowed.

Good shoot, before it was necessary. Done.

>> Source/JavaScriptCore/parser/Parser.cpp:368
>> +        }
> 
> Is here's fall through correct?

I've removed propagateError() statement

>> Source/JavaScriptCore/parser/Parser.cpp:610
>> +        failDueToUnexpectedToken();
> 
> OK, please insert comment about reparsing phase and parseType relation like the following.
> 
> // When reparsing phase, parseType becomes StandardFunctionParseType even if the function is arrow function.
> // This condition considers the following situations.
> // (1): If we are in the reparsing phase, this arrow function is already parsed once, so there is no syntax error.
> // (2): But if we are not in the reparsing phase, we should check this function is called in the context of the arrow function.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:631
>> +    if (parseType == StandardFunctionParseType)
> 
> Change it to `m_lexer->isReparsing()`. `parseType == StandardFunctionParseType` is not essential.
> 
> BTW, this code path is a little bit complicated. I recommend to insert comment here.
> 
> // Only when reparsing phase, our parsing target contains the EndOfArrowFunction token at the end of the code.
> // So consuming the last token here is needed.
> 
> Correct?

I'm not sure. It is possible when state of lexer is reparsing because of reparsing of wrapped function, but not arrow function. For instance test with  (function funcSelfExecAE1(value) { var f = x => x+1; return f(value);})(123); fails because parser is reparsing funcSelfExecAE1 function.

Anyway I get rid of this condition, after refactoring of the endFunctionOffset calculation

>> Source/JavaScriptCore/parser/Parser.cpp:1567
>> +        }
> 
> Drop them.

Not sure that I need to do this.
I've made small refactoring those line possible it will be more clear what is going on in this lines.

>> Source/JavaScriptCore/parser/Parser.cpp:1590
>> +            info.startFunctionOffset = m_token.m_data.offset;
> 
> This line is not needed since it's already set.
> I think info.startFunctionOffset should be set before looking up the cache and should not be touched after that.

The line above is moving to next token, so we need reset info.startFunctionOffset again.

I've made small refactoring of this conditions to avoid nested conditions, hope it will be more readable.

>> Source/JavaScriptCore/parser/Parser.cpp:1625
>> +            info.endFunctionOffset = location.endOffset + 1;
> 
> Modifying offset is dangerous I think. It easily leads heap overflow.
> And as described below, it leads `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`.
> 
> I suggest always storing locationBeforeLastToken() in bodyexpression case. And drop prevTerminator information.
> It ensures the following invariant.
> 
> 1. the stored token is always the last token in the function's body. if it using braces ({ ... }), closed brace is stored. And if it takes expr style ((x) => x), the last token `x` is stored.
> 
> And when recovering from it, always consume the token.

OK. Agree, done

>> Source/JavaScriptCore/parser/Parser.cpp:1629
>> +        info.isEndByTerminator = m_lexer->prevTerminator();
> 
> Drop isEndByTerminator information.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1647
>> +        parameters.endFunctionEndOffset = endFunctionEndOffset;
> 
> In `m_lexer->prevTerminator()` case, `info.endFunctionOffset` = `location.endOffset + 1`. And `endFunctionEndOffset` is `location.endOffset`.
> So now, `parameters.endFunctionStartOffset > parameters.endFunctionEndOffset`. Is it indended?

Fixed and used endFunctionEndOffset variables.

>> Source/JavaScriptCore/parser/Parser.cpp:1663
>> +    if ((parseType == ArrowFunctionParseType && info.functionBodyType == ArrowFunctionBodyBlock) || parseType != ArrowFunctionParseType) {
> 
> Use `parseType == StandardFunctionParseType` instead of `parseType != ArrowFunctionParseType`.

Done
Comment 85 WebKit Commit Bot 2015-06-25 16:04:40 PDT
Attachment 255586 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 86 GSkachkov 2015-06-25 16:16:49 PDT
Created attachment 255589 [details]
Patch
Comment 87 GSkachkov 2015-06-25 16:31:56 PDT
Created attachment 255593 [details]
Patch
Comment 88 GSkachkov 2015-06-25 16:33:00 PDT
(In reply to comment #82)
> Hm, I think modifying index (like endArrowFunction + 1) is not good since it
> leads heap overrun easily.
> 
> So I suggest the following change, GSkachkov, what do you think about it?
> 
> 1. info.endFunctionOffset is stored independently from the lastToken
> information. When cacheInfo is hit, just set info.endFunctionOffset =
> cachedInfo->endFunctionOffset. Not recalculate it.
> 
> 2. cachedInfo (and param) has lastToken information. Change
> endFunctionStartOffset etc. to lastTokenStartOffset etc. And
> endFunctionToken is generated from this information.
> 
> 3. Store the last token information to cacheInfo. Don't separate pathes for
> Arrow+Expr, Arrow+Block and Standard. In all cases, just store the last
> token. And info.endFunctionOffset is adjusted only in ArrowExpr case
> (lastToken.endOffset). In the other case, use lastToken.startOffset as
> info.endFunctionOffset. And when using cachedInfo, just set stored token and
> call `next()` in all cases.
> 
> 4. Not containing the arrow function expression body's terminator token
> (like ';') in function code. `next()` in reparsing phase in SingleExpression
> parsing is removed.
> 
> This drops many if-defs, simplifies the code and avoids index modification
> (like endxxx + 1).

I've tried to implement most of your suggestion in last patch. Please take a look if it code is better
Comment 89 WebKit Commit Bot 2015-06-25 16:37:20 PDT
Attachment 255593 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 90 Yusuke Suzuki 2015-06-25 19:21:59 PDT
Comment on attachment 255506 [details]
Patch

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

Comments.

>>> Source/JavaScriptCore/parser/Parser.cpp:368
>>> +        }
>> 
>> Is here's fall through correct?
> 
> I've removed propagateError() statement

Oh, why is it?
I think

propagateError();
return sourceElements;

is needed here. To fail with

{
    => expr;
}

correctly.
Comment 91 Yusuke Suzuki 2015-06-25 19:41:19 PDT
Comment on attachment 255593 [details]
Patch

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

I think only a few nits remains. Code becomes very simple, nice.
Thank you for your contributions. Sorry for troubling you by a long time review.

> Source/JavaScriptCore/parser/Parser.cpp:364
> +        

I think propagateError() is needed before `return sourceElements`.

> Source/JavaScriptCore/parser/Parser.cpp:1529
> +        endLocation.lineStartOffset = cachedInfo->lastTockenLineStartOffset;

Looks nice.

> Source/JavaScriptCore/parser/Parser.cpp:1570
> +        }

OK, looks nice.

> Source/JavaScriptCore/parser/Parser.cpp:1588
> +        info.startFunctionOffset = m_token.m_data.offset;

Still I don't understand why this line is needed.
`info.startFunctionOffset` is already adjusted in the
`info.startFunctionOffset = (info.functionBodyType == ArrowFunctionBodyBlock) ? m_token.m_data.offset : info.arrowFunctionOffset;`
part, right?

When setting info.startFunctionOffset previously, we already considered functionBodyType.
When block body functions come, we set `{`'s offset. And when expr body functions come, we set `=>`'s offset.
So this resetting is already done.

Anyway, if info.startFunctionOffset is modified after looking up the cacheInfo, cacheInfo's key becomes different.
So if this adjustment is needed, this need to be done before `if (const SourceProviderCacheItem* cachedInfo = TreeBuilder::CanUseFunctionCache ? findCachedFunctionInfo(info.startFunctionOffset) : 0) {` part. Reaching here, we already looked up the cache with the previous info.startFunctionOffset.

> Source/JavaScriptCore/parser/Parser.cpp:1628
> +#endif

Very nice :D
Comment 92 GSkachkov 2015-06-25 22:26:39 PDT
Created attachment 255616 [details]
Patch
Comment 93 GSkachkov 2015-06-25 22:29:18 PDT
Comment on attachment 255593 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.cpp:1588
>> +        info.startFunctionOffset = m_token.m_data.offset;
> 
> Still I don't understand why this line is needed.
> `info.startFunctionOffset` is already adjusted in the
> `info.startFunctionOffset = (info.functionBodyType == ArrowFunctionBodyBlock) ? m_token.m_data.offset : info.arrowFunctionOffset;`
> part, right?
> 
> When setting info.startFunctionOffset previously, we already considered functionBodyType.
> When block body functions come, we set `{`'s offset. And when expr body functions come, we set `=>`'s offset.
> So this resetting is already done.
> 
> Anyway, if info.startFunctionOffset is modified after looking up the cacheInfo, cacheInfo's key becomes different.
> So if this adjustment is needed, this need to be done before `if (const SourceProviderCacheItem* cachedInfo = TreeBuilder::CanUseFunctionCache ? findCachedFunctionInfo(info.startFunctionOffset) : 0) {` part. Reaching here, we already looked up the cache with the previous info.startFunctionOffset.

Now I got it :-) The same was done in line 1506. 
Fixed.
Comment 94 WebKit Commit Bot 2015-06-25 22:34:19 PDT
Attachment 255616 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 95 GSkachkov 2015-06-25 22:35:33 PDT
Created attachment 255617 [details]
Patch
Comment 96 WebKit Commit Bot 2015-06-25 22:37:57 PDT
Attachment 255617 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 97 GSkachkov 2015-06-25 22:38:33 PDT
(In reply to comment #91)
> Comment on attachment 255593 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255593&action=review
> 
> I think only a few nits remains. Code becomes very simple, nice.
> Thank you for your contributions. Sorry for troubling you by a long time
> review.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:364
> > +        
> 
> I think propagateError() is needed before `return sourceElements`.
> 
Returned back
Comment 98 Yusuke Suzuki 2015-06-25 23:00:14 PDT
Comment on attachment 255617 [details]
Patch

r=me. Thank you!
Comment 99 WebKit Commit Bot 2015-06-25 23:50:12 PDT
Comment on attachment 255617 [details]
Patch

Clearing flags on attachment: 255617

Committed r185989: <http://trac.webkit.org/changeset/185989>
Comment 100 WebKit Commit Bot 2015-06-25 23:50:22 PDT
All reviewed patches have been landed.  Closing bug.