Bug 153981 – [ES6] Arrow function syntax. Emit loading&putting this/super only if they are used in arrow function

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

[GSkachkov]

In current implementation of arrow function we emit loading/putting this/super alway nevertheless if this/super are used inside of the arrow function.

[GSkachkov]

Created attachment 271198 [details]
Patch

Patch

[GSkachkov]

Created attachment 271327 [details]
Patch

Tiny fixes

[Saam Barati]

Comment on attachment 271327 [details]
Patch

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

LGTM, just a few design suggestions and renaming suggestions.
Have you run perf numbers?

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:132
> +    bool isInnerArrowFunctionUseArguments() { return m_arrowFunctionCodeFeatures & ArgumentsArrowFunctionFeature; }
> +    bool isInnerArrowFunctionUseSuperCall() { return m_arrowFunctionCodeFeatures & SuperCallArrowFunctionFeature; }
> +    bool isInnerArrowFunctionUseSuperProperty() { return m_arrowFunctionCodeFeatures & SuperPropertyArrowFunctionFeature; }
> +    bool isInnerArrowFunctionUseEval() { return m_arrowFunctionCodeFeatures & EvalArrowFunctionFeature; }
> +    bool isInnerArrowFunctionUseThis() { return m_arrowFunctionCodeFeatures & ThisArrowFunctionFeature; }
> +    bool isInnerArrowFunctionUseNewTarget() { return m_arrowFunctionCodeFeatures & NewTargetArrowFunctionFeature; }

I would name these like "doAnyInnerArrowFunctionsX"
i.e:
isInnerArrowFunctionUseArguments => doAnyInnerArrowFunctionsUseArguments

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4051
> +bool BytecodeGenerator::isNewTargetUsedInArrowFunction()
> +{
> +    return m_codeBlock->isInnerArrowFunctionUseNewTarget() || m_codeBlock->isInnerArrowFunctionUseSuperCall();
> +}

Is "new.target" not allowed in eval?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:648
> +        void emitPutThisToArrowFunctionContextScope(bool = false);

Style: give this bool a name, like "shouldAlwaysPut" or something.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:724
> +        bool isThisUsedInArrowFunction();
> +        bool isNewTargetUsedInArrowFunction();
> +        bool isSuperUsedInArrowFunction();
> +        bool isArgumentsUsedInArrowFunction();

nit:  I would name these with "Inner" before "In"
like:
"isThisUsedInInnerArrowFunction"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:764
> +            generator.emitPutThisToArrowFunctionContextScope(true);

Style: We usually write code like this by assigning the boolean as a local variable and passing the local variable to the function:
```
bool forcePutToScope = true;
generator.emitPutThisToArrowFunctionContextScope(forcePutToScope);
```

> Source/JavaScriptCore/parser/Parser.cpp:2067
> +            if (functionBodyType != StandardFunctionBodyBlock)
> +                closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseSuperCall();

An alternative to repeatedly doing a scope search here is to just propagate this information
when we pop the scope. That's usually how we propagate information upwards. Look at popScopeInternal.
Also, instead of "functionBodyType != StandardFunctionBodyBlock", I think the code is easier to read
with affirmative conditions, like "bodyType == ArrowExpr || bodyType == ArrowBlock"

> Source/JavaScriptCore/parser/Parser.cpp:3644
> +            if (m_vm->propertyNames->eval == *ident)
> +                closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseEval();
> +            else if (m_vm->propertyNames->arguments == *ident)
> +                closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseArgumetns();

If you go with the popScopeInternalApproach, this can be determined then as well when we're already iterating over all used variables.

> Source/JavaScriptCore/parser/ParserModes.h:164
> +typedef uint16_t ArrowFunctionCodeFeatures;

this can be uint8_t

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:47
> +    unsigned innerArrowFunctionFeatures;

Why not ArrowFunctionCodeFeatures instead of unsigned?

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:92
> +    unsigned innerArrowFunctionFeatures : 31;

ditto.

[GSkachkov]

Created attachment 271677 [details]
Patch

Fix comments

[GSkachkov]

Comment on attachment 271327 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:132
>> +    bool isInnerArrowFunctionUseNewTarget() { return m_arrowFunctionCodeFeatures & NewTargetArrowFunctionFeature; }
> 
> I would name these like "doAnyInnerArrowFunctionsX"
> i.e:
> isInnerArrowFunctionUseArguments => doAnyInnerArrowFunctionsUseArguments

Renamed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4051
>> +}
> 
> Is "new.target" not allowed in eval?

Ups. I forget about eval. Fixed.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:648
>> +        void emitPutThisToArrowFunctionContextScope(bool = false);
> 
> Style: give this bool a name, like "shouldAlwaysPut" or something.

added

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:724
>> +        bool isArgumentsUsedInArrowFunction();
> 
> nit:  I would name these with "Inner" before "In"
> like:
> "isThisUsedInInnerArrowFunction"

Done

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:764
>> +            generator.emitPutThisToArrowFunctionContextScope(true);
> 
> Style: We usually write code like this by assigning the boolean as a local variable and passing the local variable to the function:
> ```
> bool forcePutToScope = true;
> generator.emitPutThisToArrowFunctionContextScope(forcePutToScope);
> ```

Done

>> Source/JavaScriptCore/parser/Parser.cpp:2067
>> +                closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseSuperCall();
> 
> An alternative to repeatedly doing a scope search here is to just propagate this information
> when we pop the scope. That's usually how we propagate information upwards. Look at popScopeInternal.
> Also, instead of "functionBodyType != StandardFunctionBodyBlock", I think the code is easier to read
> with affirmative conditions, like "bodyType == ArrowExpr || bodyType == ArrowBlock"

Refactored to use popScope and fixed condition

>> Source/JavaScriptCore/parser/Parser.cpp:3644
>> +                closestParentNonArrowFunctionNonLexicalScope()->setInnerArrowFunctionUseArgumetns();
> 
> If you go with the popScopeInternalApproach, this can be determined then as well when we're already iterating over all used variables.

Done

>> Source/JavaScriptCore/parser/ParserModes.h:164
>> +typedef uint16_t ArrowFunctionCodeFeatures;
> 
> this can be uint8_t

Changed

>> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:47
>> +    unsigned innerArrowFunctionFeatures;
> 
> Why not ArrowFunctionCodeFeatures instead of unsigned?

Fixed

>> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:92
>> +    unsigned innerArrowFunctionFeatures : 31;
> 
> ditto.

Done

[GSkachkov]

(In reply to comment #3)
> Comment on attachment 271327 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271327&action=review
> 
> LGTM, just a few design suggestions and renaming suggestions.
> Have you run perf numbers?
> 

Not yet, I'm going to run it tomorrow. I think we need to add several more benchmark tests with eval and classes

[GSkachkov]

Created attachment 271685 [details]
Patch

Fix Changelog & remove commented out code

[GSkachkov]

Comment on attachment 271685 [details]
Patch

Found bugs in using arrow function inside of the constructor

[GSkachkov]

Created attachment 271811 [details]
Patch

Fix bugs: putting 'this' to arrow function context

[Build Bot]

Comment on attachment 271811 [details]
Patch

Attachment 271811 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/856427

New failing tests:
js/regress/arrowfunction-call-in-class-constructor.html
js/regress/arrowfunction-call-in-class-method.html
js/regress/arrowfunction-call-in-function.html

[Build Bot]

Created attachment 271820 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 271811 [details]
Patch

Attachment 271811 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/856428

New failing tests:
js/regress/arrowfunction-call-in-class-constructor.html
js/regress/arrowfunction-call-in-class-method.html
js/regress/arrowfunction-call-in-function.html

[Build Bot]

Created attachment 271821 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 271811 [details]
Patch

Attachment 271811 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/856430

New failing tests:
js/regress/arrowfunction-call-in-class-constructor.html
js/regress/arrowfunction-call-in-class-method.html
js/regress/arrowfunction-call-in-function.html

[Build Bot]

Created attachment 271823 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[GSkachkov]

Created attachment 271852 [details]
Benchmark test result

See Benchmark test result in attach

[GSkachkov]

(In reply to comment #3)
> Comment on attachment 271327 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271327&action=review
> 
> LGTM, just a few design suggestions and renaming suggestions.
> Have you run perf numbers?
> 

I think it quite good figures for arrow function:
                                                      before_patch                 after_patch
   arrowfunction-call-in-class-constructor          300.1710+-13.5061    ^    191.4837+-4.5846        ^ definitely 1.5676x faster
   arrowfunction-call-in-class-method                91.2163+-7.7129     ^     29.3135+-10.2887       ^ definitely 3.1118x faster
   arrowfunction-call-in-function                    96.5411+-31.9533    ^     19.6813+-1.5141        ^ definitely 4.9052x faster
   arrowfunction-call                                33.9300+-3.3111     ^     27.0056+-0.9594        ^ definitely 1.2564x faster

[GSkachkov]

Created attachment 271854 [details]
Patch

Fix tests

[Saam Barati]

Comment on attachment 271854 [details]
Patch

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

r=me w/ comments.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4050
> +    return m_codeBlock->doAnyInnerArrowFunctionsUseNewTarget() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseEval();

I was wrong about this. new.target is prohibited inside eval. Sorry for stating the wrong thing in my previous review.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4055
> +    return m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperProperty() || m_codeBlock->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();

Also, 'super' isn't allowed inside an eval. It's a SyntaxError.

> Source/JavaScriptCore/parser/Parser.h:473
> +        if (m_isArrowFunction && m_usesEval)
> +            setInnerArrowFunctionUseEval();
> +        
> +        if (m_isArrowFunction && m_vm->propertyNames->arguments == *ident)
> +            setInnerArrowFunctionUseArgumetns();

An alternative to doing this at every useVariable(.) call
is to just do it once when popScope happens.

> Source/JavaScriptCore/parser/Parser.h:953
> +        // We do not up arrow function features for ordinary function

comment not needed.

[Saam Barati]

(In reply to comment #17)
> (In reply to comment #3)
> > Comment on attachment 271327 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=271327&action=review
> > 
> > LGTM, just a few design suggestions and renaming suggestions.
> > Have you run perf numbers?
> > 
> 
> I think it quite good figures for arrow function:
>                                                       before_patch          
> after_patch
>    arrowfunction-call-in-class-constructor          300.1710+-13.5061    ^  
> 191.4837+-4.5846        ^ definitely 1.5676x faster
>    arrowfunction-call-in-class-method                91.2163+-7.7129     ^  
> 29.3135+-10.2887       ^ definitely 3.1118x faster
>    arrowfunction-call-in-function                    96.5411+-31.9533    ^  
> 19.6813+-1.5141        ^ definitely 4.9052x faster
>    arrowfunction-call                                33.9300+-3.3111     ^  
> 27.0056+-0.9594        ^ definitely 1.2564x faster

Nice.

It would be worthwhile to open a bug to rewrite the deltablue benchmark
(or some other benchmark that tests function calling) to use
arrow functions everywhere. That way we can compare that performance
to the performance of just using ordinary functions. And we can track
performance in arrow functions w/ that benchmark.

[GSkachkov]

Created attachment 271939 [details]
Patch

Fix last comments

[GSkachkov]

Comment on attachment 271854 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4050
>> +    return m_codeBlock->doAnyInnerArrowFunctionsUseNewTarget() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseEval();
> 
> I was wrong about this. new.target is prohibited inside eval. Sorry for stating the wrong thing in my previous review.

Fixed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4055
>> +    return m_codeBlock->doAnyInnerArrowFunctionsUseSuperCall() || m_codeBlock->doAnyInnerArrowFunctionsUseSuperProperty() || m_codeBlock->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();
> 
> Also, 'super' isn't allowed inside an eval. It's a SyntaxError.

I fixed. However we have issue where will fix this SyntaxError :-)

>> Source/JavaScriptCore/parser/Parser.h:473
>> +            setInnerArrowFunctionUseArgumetns();
> 
> An alternative to doing this at every useVariable(.) call
> is to just do it once when popScope happens.

OK. Refactored.

>> Source/JavaScriptCore/parser/Parser.h:953
>> +        // We do not up arrow function features for ordinary function
> 
> comment not needed.

Removed

[GSkachkov]

(In reply to comment #20)
> (In reply to comment #17)
> > (In reply to comment #3)
> > > Comment on attachment 271327 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=271327&action=review
> > > 
> > > LGTM, just a few design suggestions and renaming suggestions.
> > > Have you run perf numbers?
> > > 
> > 
> > I think it quite good figures for arrow function:
> >                                                       before_patch          
> > after_patch
> >    arrowfunction-call-in-class-constructor          300.1710+-13.5061    ^  
> > 191.4837+-4.5846        ^ definitely 1.5676x faster
> >    arrowfunction-call-in-class-method                91.2163+-7.7129     ^  
> > 29.3135+-10.2887       ^ definitely 3.1118x faster
> >    arrowfunction-call-in-function                    96.5411+-31.9533    ^  
> > 19.6813+-1.5141        ^ definitely 4.9052x faster
> >    arrowfunction-call                                33.9300+-3.3111     ^  
> > 27.0056+-0.9594        ^ definitely 1.2564x faster
> 
> Nice.
> 
> It would be worthwhile to open a bug to rewrite the deltablue benchmark
> (or some other benchmark that tests function calling) to use
> arrow functions everywhere. That way we can compare that performance
> to the performance of just using ordinary functions. And we can track
> performance in arrow functions w/ that benchmark.

I"ve added your comment to the issue that related to arrow function benchmark tests
https://bugs.webkit.org/show_bug.cgi?id=148034

[Saam Barati]

Comment on attachment 271939 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.h:493
> +    void setInnerArrowFunctionUseArgumetns() { m_innerArrowFunctionFeatures |= ArgumentsArrowFunctionFeature; }

I missed this earlier, but the spelling is wrong here.

[GSkachkov]

Created attachment 271952 [details]
Patch

Fix TYPO

[GSkachkov]

Comment on attachment 271939 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.h:493
>> +    void setInnerArrowFunctionUseArgumetns() { m_innerArrowFunctionFeatures |= ArgumentsArrowFunctionFeature; }
> 
> I missed this earlier, but the spelling is wrong here.

Oh, sorry. Fixed.

[Saam Barati]

Comment on attachment 271952 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4010
> +RegisterID* BytecodeGenerator::emitLoadArrowFunctionLexicalEnvironment(Identifier identifier)

This type should be "const Identfifier&"

[GSkachkov]

Created attachment 272107 [details]
Patch

Patch comming

[GSkachkov]

(In reply to comment #27)
> Comment on attachment 271952 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271952&action=review
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4010
> > +RegisterID* BytecodeGenerator::emitLoadArrowFunctionLexicalEnvironment(Identifier identifier)
> 
> This type should be "const Identfifier&"

Fixed

[WebKit Commit Bot]

Comment on attachment 272107 [details]
Patch

Clearing flags on attachment: 272107

Committed r197033: <http://trac.webkit.org/changeset/197033>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change may have caused 14 new JSC test failures

** The following JSC stress test failures have been introduced:
	stress/proxy-call.js.always-trigger-copy-phase
	stress/proxy-call.js.default
	stress/proxy-call.js.default-ftl
	stress/proxy-call.js.dfg-eager
	stress/proxy-call.js.dfg-eager-no-cjit-validate
	stress/proxy-call.js.dfg-maximal-flush-validate-no-cjit
	stress/proxy-call.js.ftl-eager
	stress/proxy-call.js.ftl-eager-no-cjit
	stress/proxy-call.js.ftl-no-cjit-no-inline-validate
	stress/proxy-call.js.ftl-no-cjit-no-put-stack-validate
	stress/proxy-call.js.ftl-no-cjit-small-pool
	stress/proxy-call.js.ftl-no-cjit-validate-sampling-profiler
	stress/proxy-call.js.no-cjit-validate-phases
	stress/proxy-call.js.no-llint

<https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/3604/steps/jscore-test/logs/stdio>

[Saam Barati]

(In reply to comment #32)
> This change may have caused 14 new JSC test failures
> 
> ** The following JSC stress test failures have been introduced:
> 	stress/proxy-call.js.always-trigger-copy-phase
> 	stress/proxy-call.js.default
> 	stress/proxy-call.js.default-ftl
> 	stress/proxy-call.js.dfg-eager
> 	stress/proxy-call.js.dfg-eager-no-cjit-validate
> 	stress/proxy-call.js.dfg-maximal-flush-validate-no-cjit
> 	stress/proxy-call.js.ftl-eager
> 	stress/proxy-call.js.ftl-eager-no-cjit
> 	stress/proxy-call.js.ftl-no-cjit-no-inline-validate
> 	stress/proxy-call.js.ftl-no-cjit-no-put-stack-validate
> 	stress/proxy-call.js.ftl-no-cjit-small-pool
> 	stress/proxy-call.js.ftl-no-cjit-validate-sampling-profiler
> 	stress/proxy-call.js.no-cjit-validate-phases
> 	stress/proxy-call.js.no-llint
> 
> <https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/3604/steps/jscore-
> test/logs/stdio>

It did. I'm looking into a fix now.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 154649

[GSkachkov]

Created attachment 272326 [details]
Patch

Fixed version

[Saam Barati]

Comment on attachment 272326 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:575
> +    if (SourceParseMode::ArrowFunctionMode == parseMode && (functionNode->usesThis() || isThisUsedInInnerArrowFunction()))

Is this change here correct?
Does it work for:
class A { getValue() { ... } }
class B extends A { constructor() { arr = ()=>super.getValue(); arr(); super(); }}

[GSkachkov]

It should work, we emit TDZ always when access to the super. 
https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp#L922

[GSkachkov]

(In reply to comment #36)
> Comment on attachment 272326 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272326&action=review
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:575
> > +    if (SourceParseMode::ArrowFunctionMode == parseMode && (functionNode->usesThis() || isThisUsedInInnerArrowFunction()))
> 
> Is this change here correct?
> Does it work for:
> class A { getValue() { ... } }
> class B extends A { constructor() { arr = ()=>super.getValue(); arr();
> super(); }}

It should work, we emit TDZ always when access to the super. 
https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp#L922

[GSkachkov]

Comment on attachment 272326 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:3890
> +        if (currentFunctionScope()->isArrowFunction()) {
> +            if (currentFunctionScope()->hasDirectSuper())
> +                currentFunctionScope()->setInnerArrowFunctionUseSuperCall();
> +        
> +            if (currentFunctionScope()->needsSuperBinding())
> +                currentFunctionScope()->setInnerArrowFunctionUseSuperProperty();
> +        }
> +

Main change to fix error in tests

[GSkachkov]

Created attachment 272424 [details]
Patch

Fix comments & Stress tests are executed

[Build Bot]

Comment on attachment 272424 [details]
Patch

Attachment 272424 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/892438

New failing tests:
storage/indexeddb/key-generator.html

[Build Bot]

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

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

[Saam Barati]

Comment on attachment 272424 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:3883
> +        if (currentFunctionScope()->isArrowFunction()) {

I don't understand this code. Why does this happen unconditionally?
Shouldn't it depend on if "baseIsSuper == true"?
Also, does this miss the case where you just have "super" without
a property access or function call afterward?
I.e:
()=>super;

[Saam Barati]

Comment on attachment 272424 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:3883
> +        if (currentFunctionScope()->isArrowFunction()) {

I don't understand this code. Why does this happen unconditionally?
Shouldn't it depend on if "baseIsSuper == true"?
Also, does this miss the case where you just have "super" without
a property access or function call afterward?
I.e:
()=>super;

[Saam Barati]

Comment on attachment 272424 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:3883
> +        if (currentFunctionScope()->isArrowFunction()) {

I don't understand this code. Why does this happen unconditionally?
Shouldn't it depend on if "baseIsSuper == true"?
Also, does this miss the case where you just have "super" without
a property access or function call afterward?
I.e:
()=>super;

[Saam Barati]

Comment on attachment 272424 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.cpp:3883
> +        if (currentFunctionScope()->isArrowFunction()) {

I don't understand this code. Why does this happen unconditionally?
Shouldn't it depend on if "baseIsSuper == true"?
Also, does this miss the case where you just have "super" without
a property access or function call afterward?
I.e:
()=>super;

[Saam Barati]

(Ah, sorry for the repeated same comment. Bugzilla doesn't like me today.)

[GSkachkov]

Created attachment 272456 [details]
Patch

Update patch

[GSkachkov]

Comment on attachment 272424 [details]
Patch

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

>>>>> Source/JavaScriptCore/parser/Parser.cpp:3883
>>>>> +        if (currentFunctionScope()->isArrowFunction()) {
>>>> 
>>>> I don't understand this code. Why does this happen unconditionally?
>>>> Shouldn't it depend on if "baseIsSuper == true"?
>>>> Also, does this miss the case where you just have "super" without
>>>> a property access or function call afterward?
>>>> I.e:
>>>> ()=>super;
>>> 
>>> I don't understand this code. Why does this happen unconditionally?
>>> Shouldn't it depend on if "baseIsSuper == true"?
>>> Also, does this miss the case where you just have "super" without
>>> a property access or function call afterward?
>>> I.e:
>>> ()=>super;
>> 
>> I don't understand this code. Why does this happen unconditionally?
>> Shouldn't it depend on if "baseIsSuper == true"?
>> Also, does this miss the case where you just have "super" without
>> a property access or function call afterward?
>> I.e:
>> ()=>super;
> 
> I don't understand this code. Why does this happen unconditionally?
> Shouldn't it depend on if "baseIsSuper == true"?
> Also, does this miss the case where you just have "super" without
> a property access or function call afterward?
> I.e:
> ()=>super;

My idea was to check hasDirectSuper & needsSuperBinding that is set when baseIsSuper == true by (setNeedsSuperBinding & setHasDirectSuper), but I see you point, that my current solution is not optimal, it is my fault. I'll rewrite. 

In general '()=>super;' should return syntax error: 'Cannot reference super', and it do.

[Saam Barati]

Comment on attachment 272456 [details]
Patch

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

> Source/JavaScriptCore/parser/Parser.h:488
> +    void setInnerArrowFunctionUseSuperCall() { m_innerArrowFunctionFeatures |= SuperCallInnerArrowFunctionFeature; }

Nit: all of these setInner* should replace "use" with "uses"

[GSkachkov]

Committed r 197296: <http://trac.webkit.org/changeset/197296>

[GSkachkov]

All reviewed patches have been landed.  Closing bug.

[GSkachkov]

*** Bug 149933 has been marked as a duplicate of this bug. ***

[Brady Eidson]

Comment on attachment 272424 [details]
Patch

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.

[GSkachkov]

(In reply to Brady Eidson from comment #54)
> Comment on attachment 272424 [details]
> Patch
> 
> r-, as this has been pending review for over a year now. It is
> near-impossible that this patch still applies to trunk and unlikely to still
> be relevant in its current form.

Ohh it is already landed patch. Will clear all flags