Bug 140855 – [ES6] Implement ES6 arrow function syntax

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

[GSkachkov]

Description of the syntax of the Arrow Function. 
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrow-function-definitions

[GSkachkov]

Created attachment 247543 [details]
Patch

[GSkachkov]

Created attachment 247579 [details]
Patch

[GSkachkov]

Created attachment 247598 [details]
Patch

[GSkachkov]

Created attachment 247599 [details]
Patch

[GSkachkov]

Created attachment 247600 [details]
Patch

[WebKit Commit Bot]

Attachment 247600 [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:708:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:709:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 25 files


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

[GSkachkov]

Will not do the fixing this 3 style check errors, possible was chosen some wrong approach. Will wait until review result.

[Build Bot]

Comment on attachment 247600 [details]
Patch

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

New failing tests:
fast/css/object-fit/object-fit-canvas.html

[Build Bot]

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

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

[GSkachkov]

(In reply to comment #9)
> Created attachment 247601 [details]
> Archive of layout-test-results from ews105 for mac-mavericks-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

That strange. My patch doesn't touch the failed tests.

[GSkachkov]

(In reply to comment #5)
> Created attachment 247600 [details]
> Patch

Supported cases:
 () => 2*2
 x => x*x
 (x, y) => x*y

[GSkachkov]

Created attachment 247886 [details]
Patch

[WebKit Commit Bot]

Attachment 247886 [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:708:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:709:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 27 files


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

[GSkachkov]

Created attachment 247894 [details]
Patch

[WebKit Commit Bot]

Attachment 247894 [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:708:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:709:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 27 files


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

[Build Bot]

Comment on attachment 247894 [details]
Patch

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

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.2_T2.html
http/tests/workers/worker-importScripts.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T9.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T7.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.3_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.1_T3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.2_T3.html
js/dom/parse-error-external-script-in-eval.html
js/kde/garbage-n.html
js/dom/parse-error-external-script-in-new-Function.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.1_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.4_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T8.html
sputnik/Conformance/12_Statement/12.11_The_switch_Statement/S12.11_A3_T5.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 247894 [details]
Patch

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

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.2_T2.html
http/tests/workers/worker-importScripts.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T9.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T7.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.3_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.1_T3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.2_T3.html
js/dom/parse-error-external-script-in-eval.html
js/kde/garbage-n.html
js/dom/parse-error-external-script-in-new-Function.html
sputnik/Conformance/12_Statement/12.6_Iteration_Statements/12.6.3_The_for_Statement/S12.6.3_A8.1_T3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.1_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A3.4_T2.html
sputnik/Conformance/07_Lexical_Conventions/7.8_Literals/7.8.5_Regular_Expression_Literals/S7.8.5_A3.1_T8.html
sputnik/Conformance/12_Statement/12.11_The_switch_Statement/S12.11_A3_T5.html

[Build Bot]

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

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

[GSkachkov]

Created attachment 247934 [details]
Patch

[WebKit Commit Bot]

Attachment 247934 [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:708:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:709:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 27 files


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

[Yusuke Suzuki]

Good start!
Do you have a plan to implement |this| binding semantics of arrow functions?
Are you planning to implement it in the subsequent patches? :D

[GSkachkov]

(In reply to comment #22)
> Good start!
> Do you have a plan to implement |this| binding semantics of arrow functions?
> Are you planning to implement it in the subsequent patches? :D

Thanks for comment. 

I'm goint to implement full spec of the arrow function. But for now I'm tring to follow the webkit contribution rule - deliver the smollest path/feature. Also I'm quite new in webkit project and expect that I will redo a lot in current patch to reach webkit standards in quality, performance and etc :-)

[Yusuke Suzuki]

One tip: we can write tests related to JavaScriptCore under 'tests/stress' instead of layout-tests. fpizlo told me this very nice test infrastructure :)

[Yusuke Suzuki]

(In reply to comment #24)
> One tip: we can write tests related to JavaScriptCore under 'tests/stress'
> instead of layout-tests. fpizlo told me this very nice test infrastructure :)

And execute it with `Tools/Scripts/run-javascriptcore-tests` :D

[GSkachkov]

(In reply to comment #24)
> One tip: we can write tests related to JavaScriptCore under 'tests/stress'
> instead of layout-tests. fpizlo told me this very nice test infrastructure :)

Thanks for the tip. I'll do this.
Yusuke Suzuki, What do you think, do I need add/change something inside of the tests, for instance cover some special cases or cover some special exception and etc?

[GSkachkov]

Created attachment 248363 [details]
Patch

[Yusuke Suzuki]

(In reply to comment #26)
> (In reply to comment #24)
> > One tip: we can write tests related to JavaScriptCore under 'tests/stress'
> > instead of layout-tests. fpizlo told me this very nice test infrastructure :)
> 
> Thanks for the tip. I'll do this.
> Yusuke Suzuki, What do you think, do I need add/change something inside of
> the tests, for instance cover some special cases or cover some special
> exception and etc?

Ideally, I think there's 3 edge cases for arrow functions in the ES6 spec.
If I missed / misunderstood something, please point it :)

1. Arrow Function's environment doesn't has |this| binding.

So, |this| should be lookuped from outer side.
And Function.prototype.bind for ArrowFunction doesn't work well according to the spec.
And since `class syntax` is now landed (like https://trac.webkit.org/changeset/181293), be careful for treating |this| and TDZ. (at some point, |this| is not accessable)

2. Arrow Function doesn't has [[Construct]]

So, arrow function with `new` call should be raise TypeError.

3. Don't create "arguments" for arrow function

arrow function doesn't have "arguments" in its environment.

[Yusuke Suzuki]

(In reply to comment #26)
> Thanks for the tip. I'll do this.
> Yusuke Suzuki, What do you think, do I need add/change something inside of
> the tests, for instance cover some special cases or cover some special
> exception and etc?

And I think tests for syntax errors are needed.
Let's write syntax error tests with `eval` and `SyntaxError` :D

[GSkachkov]

Created attachment 248414 [details]
Patch

[WebKit Commit Bot]

Attachment 248414 [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:716:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:717:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 24 files


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

[GSkachkov]

(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #24)
> > > One tip: we can write tests related to JavaScriptCore under 'tests/stress'
> > > instead of layout-tests. fpizlo told me this very nice test infrastructure :)
> > 
> > Thanks for the tip. I'll do this.
> > Yusuke Suzuki, What do you think, do I need add/change something inside of
> > the tests, for instance cover some special cases or cover some special
> > exception and etc?
> 
> Ideally, I think there's 3 edge cases for arrow functions in the ES6 spec.
> If I missed / misunderstood something, please point it :)
> 
> 1. Arrow Function's environment doesn't has |this| binding.
> 
> So, |this| should be lookuped from outer side.
> And Function.prototype.bind for ArrowFunction doesn't work well according to
> the spec.
> And since `class syntax` is now landed (like
> https://trac.webkit.org/changeset/181293), be careful for treating |this|
> and TDZ. (at some point, |this| is not accessable)
> 
> 2. Arrow Function doesn't has [[Construct]]
> 
> So, arrow function with `new` call should be raise TypeError.
> 
> 3. Don't create "arguments" for arrow function
> 
> arrow function doesn't have "arguments" in its environment.

Thanks for feedback. 
I'll try to cover this 3 edge cases soon.

[Saam Barati]

Comment on attachment 248414 [details]
Patch

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

Patch looks good overall. I think you can touch it up a bit before landing.

> Source/JavaScriptCore/ChangeLog:16
> +

It's worth going through some of this to check for spelling/grammatical errors. There are many 
of these such errors.

Also, it's nice to have some annotations of the functions you changed below,
but stay away from restating what the changelog already states. Your comments
should give insight as to why you changed something or how something works.
It's not worth saying something like: "Added extra parameter". The diff below
will tell us this.

> Source/JavaScriptCore/parser/ASTBuilder.h:-314
> -    

You should revert this whitespace and all other white space changes below.

> Source/JavaScriptCore/parser/Parser.cpp:584
> +    semanticFailIfFalse(currentScope()->isFunction(), "Return statements are only valid inside arrow functions");

I'm confused by this error message here. Is it actually supposed to say that?

> Source/JavaScriptCore/parser/Parser.cpp:1502
> +template <class TreeBuilder> bool Parser<LexerType>::parseArrowFunctionInfo(TreeBuilder& context, FunctionParseMode mode, ConstructorKind constructorKind, ParserFunctionInfo<TreeBuilder>& info)

This function looks like it has a ton of duplicated code with parseFunctionInfo(.).
It's worth creating an abstraction that handles both function info's so we don't
end up with two divergent code paths in the future.

> Source/JavaScriptCore/parser/Parser.cpp:1524
> +            info.parameters = parseFormalParameters(context);

I don't think this will do what you want. This will parse:
x,y => {}
I think the syntax only allows for single parameter arrow functions to not have parens around
their formal parameter list.
You will probably want to failIfFalse if there isn't only a single parameter here

> Source/JavaScriptCore/parser/Parser.cpp:1642
> +        bool isEndOfArrowFunction = match(SEMICOLON) || match(COMMA) || match(CLOSEPAREN) || match(CLOSEBRACE) || match(CLOSEBRACKET);

Nit: It may be worth writing a function to do this since
you have almost this same expression above.

> Source/JavaScriptCore/parser/Parser.h:84
> +enum FunctionParseType { StandardFunctionParseType, ArrowFunctionParseType };

I think we should use a better name for this because parseSourceElements(.) is used to parse more than functions.
Maybe something as simple as SourceElementsParseType

> Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> +var result;

Is there another bug open for the correct implementation of the arrow function to have
lexical "this" scoping?

I think it's probably okay to land this in two different bugs, but you'll probably end up changing some 
of the code in this patch.

We should also have a lot of parsing tests for this patch on top of these tests.
For example, I think your patch as it stands will incorrectly parse:
x,y => x + y;

[GSkachkov]

(In reply to comment #33)
> Comment on attachment 248414 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248414&action=review
> 
> Patch looks good overall. I think you can touch it up a bit before landing.
> 
> > Source/JavaScriptCore/ChangeLog:16
> > +
> 
> It's worth going through some of this to check for spelling/grammatical
> errors. There are many 
> of these such errors.
> 
> Also, it's nice to have some annotations of the functions you changed below,
> but stay away from restating what the changelog already states. Your comments
> should give insight as to why you changed something or how something works.
> It's not worth saying something like: "Added extra parameter". The diff below
> will tell us this.
> 
> > Source/JavaScriptCore/parser/ASTBuilder.h:-314
> > -    
> 
> You should revert this whitespace and all other white space changes below.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:584
> > +    semanticFailIfFalse(currentScope()->isFunction(), "Return statements are only valid inside arrow functions");
> 
> I'm confused by this error message here. Is it actually supposed to say that?
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1502
> > +template <class TreeBuilder> bool Parser<LexerType>::parseArrowFunctionInfo(TreeBuilder& context, FunctionParseMode mode, ConstructorKind constructorKind, ParserFunctionInfo<TreeBuilder>& info)
> 
> This function looks like it has a ton of duplicated code with
> parseFunctionInfo(.).
> It's worth creating an abstraction that handles both function info's so we
> don't
> end up with two divergent code paths in the future.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1524
> > +            info.parameters = parseFormalParameters(context);
> 
> I don't think this will do what you want. This will parse:
> x,y => {}
> I think the syntax only allows for single parameter arrow functions to not
> have parens around
> their formal parameter list.
> You will probably want to failIfFalse if there isn't only a single parameter
> here
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1642
> > +        bool isEndOfArrowFunction = match(SEMICOLON) || match(COMMA) || match(CLOSEPAREN) || match(CLOSEBRACE) || match(CLOSEBRACKET);
> 
> Nit: It may be worth writing a function to do this since
> you have almost this same expression above.
> 
> > Source/JavaScriptCore/parser/Parser.h:84
> > +enum FunctionParseType { StandardFunctionParseType, ArrowFunctionParseType };
> 
> I think we should use a better name for this because parseSourceElements(.)
> is used to parse more than functions.
> Maybe something as simple as SourceElementsParseType
> 
> > Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> > +var result;
> 
> Is there another bug open for the correct implementation of the arrow
> function to have
> lexical "this" scoping?
> 
> I think it's probably okay to land this in two different bugs, but you'll
> probably end up changing some 
> of the code in this patch.
> 
> We should also have a lot of parsing tests for this patch on top of these
> tests.
> For example, I think your patch as it stands will incorrectly parse:
> x,y => x + y;

Thank you for this deep review. 
I'm finishing fixing comments from Yusuke Suzuki, after that I'll switch to the fixing of your comments.

[GSkachkov]

Created attachment 249479 [details]
Patch

[GSkachkov]

Created attachment 249529 [details]
Patch

[WebKit Commit Bot]

Attachment 249529 [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/WebKit/mac/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:729:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:730:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 49 files


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

[GSkachkov]

(In reply to comment #36)
> Created attachment 249529 [details]
> Patch

Ohhh, It was deep & long dive into core of the JSC.
I've implemented 1&2 second comments from Yusuke Suzuki.
Now I'm working on adding the description of all changes into Changlog and increasing the numbers of test cases. Hope after that can start work on comments from Saam Barati.

[GSkachkov]

Created attachment 249563 [details]
Patch

[WebKit Commit Bot]

Attachment 249563 [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/WebKit/mac/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:729:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:730:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 49 files


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

[GSkachkov]

Created attachment 249565 [details]
Patch

[WebKit Commit Bot]

Attachment 249565 [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/WebKit/mac/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:729:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:730:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 51 files


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

[GSkachkov]

Created attachment 249587 [details]
Patch

[WebKit Commit Bot]

Attachment 249587 [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/WebKit/mac/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:729:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:730:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 51 files


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

[GSkachkov]

Created attachment 249664 [details]
Patch

[WebKit Commit Bot]

Attachment 249664 [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:729:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:730:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 51 files


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

[Saam Barati]

Comment on attachment 249664 [details]
Patch

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

Looking pretty good. Some comments below:

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1752
> +    RegisterID* value = emitThisNodeBytecode(r0, n->position());

I don't think this is doing exactly what you want here.
r0 here is the destination of this function expression, and you will 
move lexical 'this' into this destination, then, you will override that
with the function expression. I don't think this is the intention. I'm
not exactly sure of the consequences of this are, but it may be easier to
get rid of the ::emitThisNodeBytecode function, and just do this here:
```RegiserID* value = emitMove(newTemporary(), thisRegister()),``
or, you may even just be able to do:
```instructions().append(thisRegister()->index())```

> Source/JavaScriptCore/jit/JITOpcodes.cpp:985
> +    emitGetVirtualRegister(currentInstruction[4].u.operand, regT1);

I don't think you want to load 'this' after the above branch because
you need it at the branch destination. (same for 32-bit version below).

> Source/JavaScriptCore/jit/JITOpcodes.cpp:995
> +    FunctionExecutable* funcExpr = m_codeBlock->functionExpr(currentInstruction[3].u.operand);

Nit: I would call this 'executable' or 'function' or something other than 'funcExpr'.

> Source/JavaScriptCore/jit/JITOperations.cpp:951
> +    return JSValue::encode(JSBoundFunction::create(vm, globalObject, func, JSValue::decode(thisValue), boundArgs, 0, ""));

Maybe we should give this a name here instead of ""?
We should investigate this.

Also, is 0 the correct thing to pass here?
I'm not really sure what that does.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1013
> +    LLINT_RETURN(JSBoundFunction::create(vm, globalObject, func, thisValue, boundArgs, 0, ""));

ditto.

Also, maybe it's worth having some kind of helper function here 
because this function is so similar to the above.

> Source/JavaScriptCore/parser/Parser.cpp:1538
> +{

I still think this function has too much code in common with ::parseFunctionInfo,
we should abstract away the common bits.

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

Nit: I think that can just be one contiguous region, not need for #endif then #if

> Source/JavaScriptCore/parser/Parser.h:560
> +    const SourceProviderCacheItem* findCachedFunctionInfo(int openBracePos)

Nit: whitespace

> Source/JavaScriptCore/parser/Parser.h:568
> +    void didFinishParsing(SourceElements*, DeclarationStacks::VarStack&,

ditto

> Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> +var result;

I think we need some tests for lexical 'this'.

[GSkachkov]

Created attachment 249862 [details]
Patch

[WebKit Commit Bot]

Attachment 249862 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 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 249664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249664&action=review
> 
> Looking pretty good. Some comments below:
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1752
> > +    RegisterID* value = emitThisNodeBytecode(r0, n->position());
> 
> I don't think this is doing exactly what you want here.
> r0 here is the destination of this function expression, and you will 
> move lexical 'this' into this destination, then, you will override that
> with the function expression. I don't think this is the intention. I'm
> not exactly sure of the consequences of this are, but it may be easier to
> get rid of the ::emitThisNodeBytecode function, and just do this here:
> ```RegiserID* value = emitMove(newTemporary(), thisRegister()),``
> or, you may even just be able to do:
> ```instructions().append(thisRegister()->index())```
> 
> > Source/JavaScriptCore/jit/JITOpcodes.cpp:985
> > +    emitGetVirtualRegister(currentInstruction[4].u.operand, regT1);
> 
> I don't think you want to load 'this' after the above branch because
> you need it at the branch destination. (same for 32-bit version below).
> 
> > Source/JavaScriptCore/jit/JITOpcodes.cpp:995
> > +    FunctionExecutable* funcExpr = m_codeBlock->functionExpr(currentInstruction[3].u.operand);
> 
> Nit: I would call this 'executable' or 'function' or something other than
> 'funcExpr'.
> 
> > Source/JavaScriptCore/jit/JITOperations.cpp:951
> > +    return JSValue::encode(JSBoundFunction::create(vm, globalObject, func, JSValue::decode(thisValue), boundArgs, 0, ""));
> 
> Maybe we should give this a name here instead of ""?
> We should investigate this.
> 
> Also, is 0 the correct thing to pass here?
> I'm not really sure what that does.
> 
> > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1013
> > +    LLINT_RETURN(JSBoundFunction::create(vm, globalObject, func, thisValue, boundArgs, 0, ""));
> 
> ditto.
> 
> Also, maybe it's worth having some kind of helper function here 
> because this function is so similar to the above.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:1538
> > +{
> 
> I still think this function has too much code in common with
> ::parseFunctionInfo,
> we should abstract away the common bits.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:2080
> > +#endif
> 
> Nit: I think that can just be one contiguous region, not need for #endif
> then #if
> 
> > Source/JavaScriptCore/parser/Parser.h:560
> > +    const SourceProviderCacheItem* findCachedFunctionInfo(int openBracePos)
> 
> Nit: whitespace
> 
> > Source/JavaScriptCore/parser/Parser.h:568
> > +    void didFinishParsing(SourceElements*, DeclarationStacks::VarStack&,
> 
> ditto
> 
> > Source/JavaScriptCore/tests/stress/arrowfunction.js:1
> > +var result;
> 
> I think we need some tests for lexical 'this'.

I really appreciate your feedback. It allow to fix several failing tests. 
I've tried to fix most of the comments with new patch. I'll provide more detail for each your comments after checking the build by CI.

[GSkachkov]

Created attachment 249940 [details]
Patch

[WebKit Commit Bot]

Attachment 249940 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


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

[GSkachkov]

Comment on attachment 249664 [details]
Patch

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

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:985
>> +    emitGetVirtualRegister(currentInstruction[4].u.operand, regT1);
> 
> I don't think you want to load 'this' after the above branch because
> you need it at the branch destination. (same for 32-bit version below).

I've changed position of the loading 'this'. I'm hope it fixed

>> Source/JavaScriptCore/jit/JITOpcodes.cpp:995
>> +    FunctionExecutable* funcExpr = m_codeBlock->functionExpr(currentInstruction[3].u.operand);
> 
> Nit: I would call this 'executable' or 'function' or something other than 'funcExpr'.

done

>> Source/JavaScriptCore/jit/JITOperations.cpp:951
>> +    return JSValue::encode(JSBoundFunction::create(vm, globalObject, func, JSValue::decode(thisValue), boundArgs, 0, ""));
> 
> Maybe we should give this a name here instead of ""?
> We should investigate this.
> 
> Also, is 0 the correct thing to pass here?
> I'm not really sure what that does.

Not sure that we should add the name of the function. I've checked this parameters with on following code 
var f = function (x) { return x + 1}.bind(this); and name had value "" and number of additional parameters  were 0.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1013
>> +    LLINT_RETURN(JSBoundFunction::create(vm, globalObject, func, thisValue, boundArgs, 0, ""));
> 
> ditto.
> 
> Also, maybe it's worth having some kind of helper function here 
> because this function is so similar to the above.

I've tried to eliminate of code duplication by using DEFINE. Is it correct to use this approach in such cases?

>> Source/JavaScriptCore/parser/Parser.cpp:1538
>> +{
> 
> I still think this function has too much code in common with ::parseFunctionInfo,
> we should abstract away the common bits.

I've merged parseArrowFunctionInfo with parseFunctionInfo and parseArrowFunctioBody with  parseFunctionBody. Hope they are still readable.

>> Source/JavaScriptCore/parser/Parser.cpp:2080
>> +#endif
> 
> Nit: I think that can just be one contiguous region, not need for #endif then #if

Done

>> Source/JavaScriptCore/tests/stress/arrowfunction.js:1
>> +var result;
> 
> I think we need some tests for lexical 'this'.

I've added one more Source/JavaScriptCore/tests/stress/arrowfunction-lexicalthis.js file to  test lexical 'this'.

[GSkachkov]

Created attachment 249942 [details]
Patch

[WebKit Commit Bot]

Attachment 249942 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


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

[GSkachkov]

Created attachment 249946 [details]
Patch

[WebKit Commit Bot]

Attachment 249946 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


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

[GSkachkov]

Created attachment 249965 [details]
Patch

[WebKit Commit Bot]

Attachment 249965 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


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

[GSkachkov]

Created attachment 249966 [details]
Patch

[WebKit Commit Bot]

Attachment 249966 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


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

[GSkachkov]

Created attachment 249967 [details]
Patch

[WebKit Commit Bot]

Attachment 249967 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


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

[GSkachkov]

Created attachment 249968 [details]
Patch

[WebKit Commit Bot]

Attachment 249968 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


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

[Ryosuke Niwa]

Comment on attachment 249968 [details]
Patch

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

I ran out of the time to review mid way but this should be a plenty of feedback to work on :)

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1768
> +    instructions().append(thisRegister()->index());

Are we emitting a TDZ check if an arrow function is used inside a derived class' constructor before super() is called?
e.g. new (class extends (class {}) { () => this; super(); }) should throw a ReferenceError instead of crashing.

Please add a stress test too.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1772
> +RegisterID* BytecodeGenerator::emitThisNodeBytecode(RegisterID* dst, JSTextPosition position)

How is this code different at all from what's in ThisNode::emitBytecode?
I don't think we should duplicate this much code in two places.

> Source/JavaScriptCore/jit/JITOperations.cpp:47
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +#include "JSArrowFunction.h"
> +#endif

We should probably be checking this inside JSArrowFunction.h

> Source/JavaScriptCore/jit/JITOperations.h:121
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +typedef EncodedJSValue JIT_OPERATION (*J_JITOperation_EJscCJ)(ExecState*, JSScope*, JSCell*, EncodedJSValue);
> +#endif

I don't think we need to if-def op codes like this.  e.g. op_check_tdz is used only when class syntax is enabled but we don't if-def it.

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

Typo: correc*t*.  How is this condition different from autoSemiColon()?
It seems that we need to document the difference one way or another.

> Source/JavaScriptCore/parser/Parser.cpp:1198
> +    bool handled = false;

This variable is unnecessary.

> Source/JavaScriptCore/parser/Parser.cpp:1206
> +            handled = true;

We should just call context.setEndOffset and return here instead.

> Source/JavaScriptCore/parser/Parser.cpp:1457
> +    if (parseType == ArrowFunctionParseType) {

We should either use switch here or assert that parseType == ArrowFunctionParseType if the previous if evaluated to false.
Also, can we do a refactoring to put the original code into a separate helper function in a separate patch so that the diff here is readable?
As is, it's hard to review...

> Source/JavaScriptCore/parser/Parser.cpp:1523
> +        if (parseType == ArrowFunctionParseType) {
> +            info.arrowFunctionBodyType = cachedInfo->isBodyArrowFunctionBlock ? ArrowFunctionBodyBlock : ArrowFunctionBodyExpression;

Why do we need to condition this based on parseType?
Since arrowFunctionBodyType is never used, it should be fine to always set info.arrowFunctionBodyType.

> Source/JavaScriptCore/parser/Parser.cpp:1528
> +            m_token = info.arrowFunctionBodyType == ArrowFunctionBodyBlock ? cachedInfo->closeBraceToken() : cachedInfo->endArrowFunctionToken();
> +        } else
> +            m_token = cachedInfo->closeBraceToken();
> +#else   
>          m_token = cachedInfo->closeBraceToken();

Can't we just add endFunctionToken to cacheInfo instead?

> Source/JavaScriptCore/parser/Parser.cpp:1544
> +        if (parseType == ArrowFunctionParseType) {
> +            if (info.arrowFunctionBodyType == ArrowFunctionBodyBlock)
> +                next();
> +        } else
> +            next();

!?  Why not (parseType != ArrowFunctionParseType || info.arrowFunctionBodyType == ArrowFunctionBodyBlock)?

> Source/JavaScriptCore/parser/Parser.cpp:1629
> +    if (parseType == ArrowFunctionParseType) {
> +        if (info.arrowFunctionBodyType == ArrowFunctionBodyBlock) {
> +            matchOrFail(CLOSEBRACE, "Expected a closing '}' after a ", stringForFunctionMode(mode), " body");
> +            next();
> +        } else

It looks like these two conditions can be combined into parseType == ArrowFunctionParseType && info.arrowFunctionBodyType != ArrowFunctionBodyBlock
to run the code in else since the statement in the inner if and the outer else are identical.

[GSkachkov]

Ryosuke Niwa  Thanks for the review!
I'll try to fix soon.

[GSkachkov]

Created attachment 250421 [details]
Patch

[WebKit Commit Bot]

Attachment 250421 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 53 files


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

[GSkachkov]

Comment on attachment 249968 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1768
>> +    instructions().append(thisRegister()->index());
> 
> Are we emitting a TDZ check if an arrow function is used inside a derived class' constructor before super() is called?
> e.g. new (class extends (class {}) { () => this; super(); }) should throw a ReferenceError instead of crashing.
> 
> Please add a stress test too.

I've added test Source/JavaScriptCore/tests/stress/arrowfunction-tdz.js.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1772
>> +RegisterID* BytecodeGenerator::emitThisNodeBytecode(RegisterID* dst, JSTextPosition position)
> 
> How is this code different at all from what's in ThisNode::emitBytecode?
> I don't think we should duplicate this much code in two places.

Removed duplication.

>> Source/JavaScriptCore/jit/JITOperations.cpp:47
>> +#endif
> 
> We should probably be checking this inside JSArrowFunction.h

Done.

>> Source/JavaScriptCore/jit/JITOperations.h:121
>> +#endif
> 
> I don't think we need to if-def op codes like this.  e.g. op_check_tdz is used only when class syntax is enabled but we don't if-def it.

removed if-def

>> Source/JavaScriptCore/parser/Parser.cpp:598
>> +    bool correcArrowFinish = isEndOfArrowFunction() || match(EOFTOK);
> 
> Typo: correc*t*.  How is this condition different from autoSemiColon()?
> It seems that we need to document the difference one way or another.

I'll take a look deeper in autoSemiColon(), possible this functions both do the same.

>> Source/JavaScriptCore/parser/Parser.cpp:1198
>> +    bool handled = false;
> 
> This variable is unnecessary.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1206
>> +            handled = true;
> 
> We should just call context.setEndOffset and return here instead.

Done

>> Source/JavaScriptCore/parser/Parser.cpp:1457
>> +    if (parseType == ArrowFunctionParseType) {
> 
> We should either use switch here or assert that parseType == ArrowFunctionParseType if the previous if evaluated to false.
> Also, can we do a refactoring to put the original code into a separate helper function in a separate patch so that the diff here is readable?
> As is, it's hard to review...

I'll try to refactor to increase patch readability this weekend.

>> Source/JavaScriptCore/parser/Parser.cpp:1528
>>          m_token = cachedInfo->closeBraceToken();
> 
> Can't we just add endFunctionToken to cacheInfo instead?

Introduced new function with name endFunctionToken instead of closeBraceToken() & endArrowFunctionToken

>> Source/JavaScriptCore/parser/Parser.cpp:1544
>> +            next();
> 
> !?  Why not (parseType != ArrowFunctionParseType || info.arrowFunctionBodyType == ArrowFunctionBodyBlock)?

Fixed

>> Source/JavaScriptCore/parser/Parser.cpp:1629
>> +        } else
> 
> It looks like these two conditions can be combined into parseType == ArrowFunctionParseType && info.arrowFunctionBodyType != ArrowFunctionBodyBlock
> to run the code in else since the statement in the inner if and the outer else are identical.

Done

[GSkachkov]

Created attachment 250425 [details]
Patch

[WebKit Commit Bot]

Attachment 250425 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 54 files


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

[GSkachkov]

Created attachment 250430 [details]
Patch

[WebKit Commit Bot]

Attachment 250430 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 54 files


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

[GSkachkov]

Created attachment 250437 [details]
Patch

[WebKit Commit Bot]

Attachment 250437 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 54 files


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

[GSkachkov]

Created attachment 250438 [details]
Patch

[WebKit Commit Bot]

Attachment 250438 [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:736:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:737:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:781:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:801:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 55 files


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

[GSkachkov]

Created attachment 250618 [details]
Patch

[GSkachkov]

Created attachment 250626 [details]
Patch

[GSkachkov]

Created attachment 250628 [details]
Patch

[GSkachkov]

Created attachment 250629 [details]
Patch

[GSkachkov]

Created attachment 250633 [details]
Patch

[GSkachkov]

Created attachment 250643 [details]
Patch

[GSkachkov]

Created attachment 250647 [details]
Patch

[WebKit Commit Bot]

Attachment 250647 [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 "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:796:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 55 files


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

[GSkachkov]

Created attachment 250649 [details]
Patch

[GSkachkov]

Created attachment 250651 [details]
Patch

[GSkachkov]

Created attachment 250656 [details]
Patch

[GSkachkov]

Created attachment 250911 [details]
Patch

[GSkachkov]

Created attachment 250912 [details]
Patch

[Darin Adler]

Comment on attachment 250912 [details]
Patch

Please rebase the patch again so it applies and the EWS can run the tests.

[GSkachkov]

Created attachment 251029 [details]
Patch

[GSkachkov]

Created attachment 251037 [details]
Patch

[WebKit Commit Bot]

Attachment 251037 [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 "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:796:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 64 files


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

[GSkachkov]

Created attachment 251050 [details]
Patch

[GSkachkov]

Created attachment 251055 [details]
Patch

[GSkachkov]

Created attachment 251056 [details]
Patch

[GSkachkov]

Created attachment 251057 [details]
Patch

[WebKit Commit Bot]

Attachment 251057 [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 "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:796:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 64 files


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

[GSkachkov]

Created attachment 251059 [details]
Patch.1(Init)

[GSkachkov]

Created attachment 251061 [details]
Patch

[GSkachkov]

Created attachment 251063 [details]
Patch

[WebKit Commit Bot]

Attachment 251063 [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 "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:796:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 64 files


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

[GSkachkov]

(In reply to comment #92)
> Comment on attachment 250912 [details]
> Patch
> 
> Please rebase the patch again so it applies and the EWS can run the tests.

Done!

[GSkachkov]

Created attachment 251129 [details]
Patch

[WebKit Commit Bot]

Attachment 251129 [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 "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:796:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 68 files


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

[GSkachkov]

(In reply to comment #105)
> (In reply to comment #92)
> > Comment on attachment 250912 [details]
> > Patch
> > 
> > Please rebase the patch again so it applies and the EWS can run the tests.
> 
> Done!

I've made small refactoring and added new test. It would be great to have new review :)

[Filip Pizlo]

Comment on attachment 251129 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3755
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> +        case op_new_arrow_func_exp: {
> +            FunctionExecutable* expr = m_inlineStackTop->m_profiledBlock->functionExpr(currentInstruction[3].u.operand);
> +            FrozenValue* frozen = m_graph.freezeStrong(expr);
> +            set(VirtualRegister(currentInstruction[1].u.operand),
> +                addToGraph(NewFunction, OpInfo(frozen), get(VirtualRegister(currentInstruction[2].u.operand))));
> +            NEXT_OPCODE(op_new_arrow_func_exp);
> +        }
> +#endif

This looks like it has to be wrong since you're not using operand 4.

[GSkachkov]

Created attachment 252149 [details]
Patch

[GSkachkov]

Created attachment 252164 [details]
Patch

[GSkachkov]

Created attachment 252174 [details]
Patch

[GSkachkov]

Created attachment 252175 [details]
Patch

[GSkachkov]

Created attachment 252184 [details]
Patch

[GSkachkov]

Created attachment 252189 [details]
Patch

[GSkachkov]

Created attachment 252191 [details]
Patch

[GSkachkov]

Created attachment 252199 [details]
Patch

[GSkachkov]

Created attachment 252201 [details]
Patch

[GSkachkov]

Created attachment 252204 [details]
Patch

[GSkachkov]

Created attachment 252205 [details]
Patch

[WebKit Commit Bot]

Attachment 252205 [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 "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:777:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/parser/Parser.h:802:  The parameter name "functionParseType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 87 files


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

[GSkachkov]

Comment on attachment 252205 [details]
Patch

It strange that gtk-wk2/elf-wk2 build were failed. I didn't do anything gtk-wk2/elf-wk2 related in my source code

[GSkachkov]

(In reply to comment #109)
> Comment on attachment 251129 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251129&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3755
> > +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> > +        case op_new_arrow_func_exp: {
> > +            FunctionExecutable* expr = m_inlineStackTop->m_profiledBlock->functionExpr(currentInstruction[3].u.operand);
> > +            FrozenValue* frozen = m_graph.freezeStrong(expr);
> > +            set(VirtualRegister(currentInstruction[1].u.operand),
> > +                addToGraph(NewFunction, OpInfo(frozen), get(VirtualRegister(currentInstruction[2].u.operand))));
> > +            NEXT_OPCODE(op_new_arrow_func_exp);
> > +        }
> > +#endif
> 
> This looks like it has to be wrong since you're not using operand 4.

Thanks for review. Comment is fixed in new patch.

[GSkachkov]

Created attachment 252411 [details]
Patch

[WebKit Commit Bot]

Attachment 252411 [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 87 files


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

[GSkachkov]

Created attachment 252412 [details]
Patch

[WebKit Commit Bot]

Attachment 252412 [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 87 files


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

[GSkachkov]

Created attachment 252416 [details]
Patch.2(Main)

[WebKit Commit Bot]

Attachment 252416 [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 88 files


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

[Filip Pizlo]

Comment on attachment 252416 [details]
Patch.2(Main)

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1541
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)

Why do you need this guard?  I recommend adding the arraowFunctionStructure() to JSGlobalObject unconditionally.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:234
> +    case op_new_arrow_func_exp: // Fixme: Arrow function cann't be inline as the common function because it has lexicaly bind this/arguments and etc. and  arrow function has to have its own compile and inline approach.

I don't buy this comment at all.  Sounds like either there is a horrible bug, or you could just allow it.

> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:134
> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)

Ditto to my previous.  Get rid of this guard.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2992
> +    void compileNewArrowFunction()

If you're going to implement it in the FTL, then also implement it in DFG.

[GSkachkov]

Created attachment 252606 [details]
Patch.3(Fix review Comments)

[GSkachkov]

Comment on attachment 252416 [details]
Patch.2(Main)

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

I've added separate patch with fixes.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1541
>> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> 
> Why do you need this guard?  I recommend adding the arraowFunctionStructure() to JSGlobalObject unconditionally.

Done

>> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:234
>> +    case op_new_arrow_func_exp: // Fixme: Arrow function cann't be inline as the common function because it has lexicaly bind this/arguments and etc. and  arrow function has to have its own compile and inline approach.
> 
> I don't buy this comment at all.  Sounds like either there is a horrible bug, or you could just allow it.

Implemented.

>> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:134
>> +#if ENABLE(ES6_ARROWFUNCTION_SYNTAX)
> 
> Ditto to my previous.  Get rid of this guard.

Done.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2992
>> +    void compileNewArrowFunction()
> 
> If you're going to implement it in the FTL, then also implement it in DFG.

I've implemented it in DFG.

[GSkachkov]

Created attachment 252633 [details]
Patch

[WebKit Commit Bot]

Attachment 252633 [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 93 files


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

[GSkachkov]

Created attachment 252637 [details]
Full patch. Used for test build

[WebKit Commit Bot]

Attachment 252637 [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 93 files


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

[GSkachkov]

Comment on attachment 251059 [details]
Patch.1(Init)

Patch moved in subtask. https://bugs.webkit.org/show_bug.cgi?id=144954

[Csaba Osztrogonác]

Comment on attachment 251059 [details]
Patch.1(Init)

Cleared Filip Pizlo's review+ from obsolete attachment 251059 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Joseph Pecoraro]

Can this bug be closed now? We've had support for Arrow functions for a while. Or are the remaining bugs this depends on still relevant?

[Saam Barati]

I think this should be closed. All things left are optimizations and miscellaneous things, which aren't blocking the actual feature.