[ES6] Implement ES6 arrow function syntax. Parser of arrow function with execution as common function

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[GSkachkov]

Implement of the parser of arrow function with execution as common function. Part of the 140855 task.

[GSkachkov]

Created attachment 253039 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

Created attachment 253069 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

Created attachment 253195 [details]
Patch

[WebKit Commit Bot]

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.

[Ryosuke Niwa]

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.

[Saam Barati]

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

[Saam Barati]

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?

[GSkachkov]

Created attachment 253296 [details]
Patch

[GSkachkov]

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.

[GSkachkov]

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.

[GSkachkov]

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

[WebKit Commit Bot]

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.

[GSkachkov]

Created attachment 253369 [details]
Patch

[WebKit Commit Bot]

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.

[Saam Barati]

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.

[GSkachkov]

Created attachment 253378 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

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

[GSkachkov]

Comment on attachment 253378 [details]
Patch

Found broken test. Cancel review until fix.

[GSkachkov]

Comment on attachment 253378 [details]
Patch

Test are green now.

[Yusuke Suzuki]

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.

[Saam Barati]

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.

[GSkachkov]

Created attachment 254083 [details]
Patch

[GSkachkov]

Created attachment 254087 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

Created attachment 254098 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[GSkachkov]

Created attachment 254166 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

Created attachment 254174 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

(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!

[GSkachkov]

(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!!!!)

[Darin Adler]

(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)

[GSkachkov]

Created attachment 254209 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

Created attachment 254239 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

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

[Darin Adler]

I don’t think I’m the right reviewer for this new JavaScript engine feature. Geoff, who should review?

[Yusuke Suzuki]

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.

[Saam Barati]

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

[GSkachkov]

Created attachment 254438 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

Created attachment 254445 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

(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?

[GSkachkov]

(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?

[Yusuke Suzuki]

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?

[GSkachkov]

Created attachment 254763 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

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

[Saam Barati]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[GSkachkov]

Created attachment 255189 [details]
Patch

[GSkachkov]

Created attachment 255191 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

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

[GSkachkov]

(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

[Yusuke Suzuki]

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 };

[GSkachkov]

Created attachment 255398 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

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

[Yusuke Suzuki]

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.

[GSkachkov]

Created attachment 255400 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

Created attachment 255401 [details]
Patch

[GSkachkov]

Created attachment 255426 [details]
Patch

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

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?

[GSkachkov]

Created attachment 255506 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

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

[Yusuke Suzuki]

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

[Yusuke Suzuki]

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

[GSkachkov]

Created attachment 255586 [details]
Patch

[GSkachkov]

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

[WebKit Commit Bot]

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.

[GSkachkov]

Created attachment 255589 [details]
Patch

[GSkachkov]

Created attachment 255593 [details]
Patch

[GSkachkov]

(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

[WebKit Commit Bot]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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

[GSkachkov]

Created attachment 255616 [details]
Patch

[GSkachkov]

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.

[WebKit Commit Bot]

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.

[GSkachkov]

Created attachment 255617 [details]
Patch

[WebKit Commit Bot]

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.

[GSkachkov]

(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

[Yusuke Suzuki]

Comment on attachment 255617 [details]
Patch

r=me. Thank you!

[WebKit Commit Bot]

Comment on attachment 255617 [details]
Patch

Clearing flags on attachment: 255617

Committed r185989: <http://trac.webkit.org/changeset/185989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.