Bug 149615 - [ES6] Arrow function syntax. Arrow function specific features. Lexical bind "super" property
Summary: [ES6] Arrow function syntax. Arrow function specific features. Lexical bind "...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 149338
Blocks: 140855 150893
  Show dependency treegraph
 
Reported: 2015-09-29 00:09 PDT by GSkachkov
Modified: 2015-12-30 13:08 PST (History)
6 users (show)

See Also:


Attachments
Patch (53.76 KB, patch)
2015-10-28 12:23 PDT, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (54.36 KB, patch)
2015-11-04 11:37 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (55.30 KB, patch)
2015-12-10 05:51 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (55.28 KB, patch)
2015-12-10 06:39 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (55.85 KB, patch)
2015-12-21 14:06 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (55.50 KB, patch)
2015-12-22 13:13 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (55.18 KB, patch)
2015-12-29 12:18 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (54.51 KB, patch)
2015-12-29 12:41 PST, GSkachkov
no flags Details | Formatted Diff | Diff
Patch (54.47 KB, patch)
2015-12-30 03:11 PST, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description GSkachkov 2015-09-29 00:09:59 PDT
Arrow function doesn't raise early semantic errors if we are using 'super/super()', in comparison to the usual function (https://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-definitions-static-semantics-early-errors, )
Also If I correctly understand this topic http://www.ecma-international.org/ecma-262/6.0/#sec-arrow-function-definitions-static-semantics-contains, then arrow function can contain 'super’ as SuperProperty and SuperCall.
At this topic  http://www.ecma-international.org/ecma-262/6.0/#sec-arrow-function-definitions-runtime-semantics-evaluation said: 
"Any reference to arguments, super, this, or new.target within an ArrowFunction must resolve to a binding in a lexically enclosing environment. Typically this will be the Function Environment of an immediately enclosing function. Even though an ArrowFunction may contain references to super, the function object created in step 4 is not made into a method by performing MakeMethod. An ArrowFunction that references super is always contained within a non-ArrowFunction and the necessary state to implement super is accessible via the scope that is captured by the function object of the ArrowFunction."

So for my understanding arrow function can operate by 'super' property and invoke the 'super()' function. 
Later if it necessary we can split this issue into two issues for SuperProperty and SuperCall.
Comment 1 GSkachkov 2015-10-28 12:23:30 PDT
Created attachment 264232 [details]
Patch

Init commit
Comment 2 GSkachkov 2015-11-04 11:37:34 PST
Created attachment 264803 [details]
Patch

Small refactoring
Comment 3 GSkachkov 2015-12-10 05:51:28 PST
Created attachment 267097 [details]
Patch

Init load
Comment 4 GSkachkov 2015-12-10 06:39:47 PST
Created attachment 267103 [details]
Patch

Fix build
Comment 5 GSkachkov 2015-12-10 07:24:10 PST
In short patch for this issue should implement following script

var expectedValue  = 'test-value';

class A {
   getValue () {
      return expectedValue;
   }
};

class B extends A {
  getValueParentFunction() {
    var arrow  = () => super.getValue();
    return arrow();
  }
};

let b = new B();
b.getValueParentFunction() === expectedValue;
Comment 6 Saam Barati 2015-12-10 10:58:32 PST
Comment on attachment 267103 [details]
Patch

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

Mostly looks good to me but I have a question and some basic comments.

> Source/JavaScriptCore/ChangeLog:9
> +        inside of the arrow function in case if arrow function is nested in method, 

Might be worth explicitly adding constructor to this list of available contexts inside a class.

> Source/JavaScriptCore/ChangeLog:11
> +        class, lead to wrong type of error, should be SyntaxError and this will be fixed in separete patch.

Do you have a bug open for this? If you don't, you should make one.
You should link to it here

> Source/JavaScriptCore/bytecode/ExecutableInfo.h:39
> +    ExecutableInfo(bool needsActivation, bool usesEval, bool isStrictMode, bool isConstructor, bool isBuiltinFunction, ConstructorKind constructorKind, GeneratorThisMode generatorThisMode, SuperBinding superBinding, SourceParseMode parseMode, DerivedContextType _derivedContextType, bool _isArrowFunctionContext, bool isClassContext)

style: Remove the "_" prefixes.

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:70
> +    // method, getter & setter are part of the class, so this functionCodeBlock is part of the class
> +    bool isClassContext = executable->parseMode() == SourceParseMode::MethodMode || executable->parseMode() == SourceParseMode::GetterMode || executable->parseMode() == SourceParseMode::SetterMode;

Is this always correct?
We could have code like this:
```
let x = {
    get y() { return 20; }
};
x.y
```
I'm not sure if this would count as SourceParseMode::GetterMode.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:578
> +

style: revert newline.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:799
> +            bool newisDerivedConstructorContext = constructorKind() == ConstructorKind::Derived || (derivedContextType() == DerivedContextType::DerivedConstructorContext && metadata->parseMode() == SourceParseMode::ArrowFunctionMode);

This calculation doesn't seem right to me. Shouldn't we be interested in the fact that we're creating an arrow function or not?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:806
> +                bool isArrowFunctionInClassContext = m_codeBlock->isClassContext() || (m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext);
> +                derivedContextType = isArrowFunctionInClassContext ? DerivedContextType::DerivedMethodContext : DerivedContextType::None;

same here.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:189
> +

style: revert newline.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:1
> +var testCase = function (actual, expected, message) {

Style: 4-space indent this file.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:158
> +// Fixme: should by check if e instanceof SyntaxError https://bugs.webkit.org/show_bug.cgi?id=150893

Style: "Fixme" => "FIXME" and also indented properly.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:215
> +// FIXME: Problem with access to the super before super in constructor
> +// https://bugs.webkit.org/show_bug.cgi?id=152108
> +//  let j2 = new J(true);
> +//  testCase(j2._value, testValue, 'Error: Some problem with using "super" inside of the constructor');

Style: Indent this comment and all others to be at the level of block they're in.

> LayoutTests/js/script-tests/arrowfunction-superproperty.js:1
> +description('Tests for ES6 arrow function, access to the super property in arrow function');

Style: 4-space indent this file.

> LayoutTests/js/script-tests/arrowfunction-superproperty.js:80
> +//shouldThrow('(new C(false))', 'ReferenceError');

This should be "new C(true)"
Comment 7 GSkachkov 2015-12-21 14:06:47 PST
Created attachment 267762 [details]
Patch

Fix comments
Comment 8 GSkachkov 2015-12-21 14:47:51 PST
Comment on attachment 267103 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        inside of the arrow function in case if arrow function is nested in method, 
> 
> Might be worth explicitly adding constructor to this list of available contexts inside a class.

Done

>> Source/JavaScriptCore/ChangeLog:11
>> +        class, lead to wrong type of error, should be SyntaxError and this will be fixed in separete patch.
> 
> Do you have a bug open for this? If you don't, you should make one.
> You should link to it here

Done

>> Source/JavaScriptCore/bytecode/ExecutableInfo.h:39
>> +    ExecutableInfo(bool needsActivation, bool usesEval, bool isStrictMode, bool isConstructor, bool isBuiltinFunction, ConstructorKind constructorKind, GeneratorThisMode generatorThisMode, SuperBinding superBinding, SourceParseMode parseMode, DerivedContextType _derivedContextType, bool _isArrowFunctionContext, bool isClassContext)
> 
> style: Remove the "_" prefixes.

Removed

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:70
>> +    bool isClassContext = executable->parseMode() == SourceParseMode::MethodMode || executable->parseMode() == SourceParseMode::GetterMode || executable->parseMode() == SourceParseMode::SetterMode;
> 
> Is this always correct?
> We could have code like this:
> ```
> let x = {
>     get y() { return 20; }
> };
> x.y
> ```
> I'm not sure if this would count as SourceParseMode::GetterMode.

Changed. Now I'm rely on executable->superBinding() property that has value SuperBinding::Needed when it part of the class

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:578
>> +
> 
> style: revert newline.

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:799
>> +            bool newisDerivedConstructorContext = constructorKind() == ConstructorKind::Derived || (derivedContextType() == DerivedContextType::DerivedConstructorContext && metadata->parseMode() == SourceParseMode::ArrowFunctionMode);
> 
> This calculation doesn't seem right to me. Shouldn't we be interested in the fact that we're creating an arrow function or not?

Added additional condition

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:189
>> +
> 
> style: revert newline.

Done

>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:1
>> +var testCase = function (actual, expected, message) {
> 
> Style: 4-space indent this file.

Done

>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:158
>> +// Fixme: should by check if e instanceof SyntaxError https://bugs.webkit.org/show_bug.cgi?id=150893
> 
> Style: "Fixme" => "FIXME" and also indented properly.

Done

>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:215
>> +//  testCase(j2._value, testValue, 'Error: Some problem with using "super" inside of the constructor');
> 
> Style: Indent this comment and all others to be at the level of block they're in.

Done

>> LayoutTests/js/script-tests/arrowfunction-superproperty.js:1
>> +description('Tests for ES6 arrow function, access to the super property in arrow function');
> 
> Style: 4-space indent this file.

Done

>> LayoutTests/js/script-tests/arrowfunction-superproperty.js:80
>> +//shouldThrow('(new C(false))', 'ReferenceError');
> 
> This should be "new C(true)"

Yes, fixed
Comment 9 GSkachkov 2015-12-21 14:48:16 PST
Comment on attachment 267762 [details]
Patch

F
Comment 10 Saam Barati 2015-12-22 12:42:51 PST
Comment on attachment 267762 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:805
> +            if (needsToUpdateArrowFunctionContext()) {

This condition confuses me here. Don't we only care about this if the function we're creating is an arrow function?
If it's not an arrow function, shouldn't we give it DerivedContextType::None?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:763
> +

style: revert newline
Comment 11 GSkachkov 2015-12-22 13:13:01 PST
Created attachment 267799 [details]
Patch

Fixes small comments
Comment 12 GSkachkov 2015-12-22 13:15:50 PST
Comment on attachment 267762 [details]
Patch

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:805
>> +            if (needsToUpdateArrowFunctionContext()) {
> 
> This condition confuses me here. Don't we only care about this if the function we're creating is an arrow function?
> If it's not an arrow function, shouldn't we give it DerivedContextType::None?

Ohh, OK. I got it. I've added new condition to check if we're going to create new arrow function

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:763
>> +
> 
> style: revert newline

Removed
Comment 13 Saam Barati 2015-12-28 20:12:53 PST
Comment on attachment 267799 [details]
Patch

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

> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:69
> +    // this executable is part of the class

comment isn't needed

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

Why are these new conditions needed?
Why do we need to load this if we're "SourceParseMode::ArrowFunctionMode == parseMode && (isDerivedClassContext() || isDerivedConstructorContext())"?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4050
> +    if ((isConstructor() && constructorKind() == ConstructorKind::Derived) || (m_codeBlock->isClassContext())) {

style: the parens around "m_codeBlock->isClassContext()" aren't needed.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:813
> +                bool newisDerivedConstructorContext = constructorKind() == ConstructorKind::Derived || (derivedContextType() == DerivedContextType::DerivedConstructorContext);
> +            
> +                if (newisDerivedConstructorContext)
> +                    newDerivedContextType = DerivedContextType::DerivedConstructorContext;
> +                else  {
> +                    bool isArrowFunctionInClassContext = m_codeBlock->isClassContext() || (m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext);
> +                    newDerivedContextType = isArrowFunctionInClassContext ? DerivedContextType::DerivedMethodContext : DerivedContextType::None;
> +                }

Nit: I think this whole section of code would be easier to read like this:
```
if (constructorKind() == ConstructorKind::Derived || isDerivedConstructorContext())
    newDerivedContextType = DerivedContextType::DerivedConstructorContext;
else if (m_codeBlock->isClassContext() || (m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext))
    newDerivedContextType =  DerivedContextType::DerivedMethodContext
```

Also, why do we need to check "m_codeBlock->isArrowFunction()" when checking "m_derivedContextType == DerivedContextType::DerivedMethodContext"?
When would we have "!m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext"?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:182
> +        RegisterID* scope = generator.emitLoadDerivedConstructorFromArrowFunctionLexicalEnvironment();    

This isn't a JSScope. Maybe a better variable name is "derivedConstructor" or something similar.

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:195
> +    EvalExecutable* eval = EvalExecutable::create(callFrame, makeSource(script), codeBlock.isStrictMode(), thisTDZMode, codeBlock.unlinkedCodeBlock()->derivedContextType(), codeBlock.unlinkedCodeBlock()->isArrowFunction(), &variablesUnderTDZ);

have you tried this out inside the inspector to make sure it works?
I.e, pausing inside an arrow function and typing in "super" into the console?

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:160
> +      erorr = true;

typo: "erorr" => "error"

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:162
> +    testCase(erorr, true, 'Error: using "super" should lead to error');

ditto

> LayoutTests/js/script-tests/arrowfunction-superproperty.js:78
> +// FIXME: Problem with access to the super before super in constructor

nit: "super before super" => "super before super()"
Comment 14 GSkachkov 2015-12-29 12:18:53 PST
Created attachment 267988 [details]
Patch

Fix comments
Comment 15 GSkachkov 2015-12-29 12:21:52 PST
Comment on attachment 267799 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:69
>> +    // this executable is part of the class
> 
> comment isn't needed

Removed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:575
>> +    if (SourceParseMode::ArrowFunctionMode == parseMode && (functionNode->usesThis() || isDerivedClassContext() || isDerivedConstructorContext()))
> 
> Why are these new conditions needed?
> Why do we need to load this if we're "SourceParseMode::ArrowFunctionMode == parseMode && (isDerivedClassContext() || isDerivedConstructorContext())"?

Without this condition following code will raise ReferenceError 'this' is undefined so to fix this error I've added this condition.

var A = class A {
    constructor() {
        this.value = 'testValue';
    }
    getValue () {
        return this.value;
    }
    
};

var B = class B extends A {
    getParentValue() {
        var arrow  = () => super.getValue();
        return arrow();
    }
};

var b = new B();
b. getParentValue()

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4050
>> +    if ((isConstructor() && constructorKind() == ConstructorKind::Derived) || (m_codeBlock->isClassContext())) {
> 
> style: the parens around "m_codeBlock->isClassContext()" aren't needed.

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:813
>> +                }
> 
> Nit: I think this whole section of code would be easier to read like this:
> ```
> if (constructorKind() == ConstructorKind::Derived || isDerivedConstructorContext())
>     newDerivedContextType = DerivedContextType::DerivedConstructorContext;
> else if (m_codeBlock->isClassContext() || (m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext))
>     newDerivedContextType =  DerivedContextType::DerivedMethodContext
> ```
> 
> Also, why do we need to check "m_codeBlock->isArrowFunction()" when checking "m_derivedContextType == DerivedContextType::DerivedMethodContext"?
> When would we have "!m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext"?

Ohh, I see. We don't need to check m_codeBlock->isArrowFunction() because it already true because of this condition metadata->parseMode() == SourceParseMode::ArrowFunctionMode.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:182
>> +        RegisterID* scope = generator.emitLoadDerivedConstructorFromArrowFunctionLexicalEnvironment();    
> 
> This isn't a JSScope. Maybe a better variable name is "derivedConstructor" or something similar.

Renamed

>> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:195
>> +    EvalExecutable* eval = EvalExecutable::create(callFrame, makeSource(script), codeBlock.isStrictMode(), thisTDZMode, codeBlock.unlinkedCodeBlock()->derivedContextType(), codeBlock.unlinkedCodeBlock()->isArrowFunction(), &variablesUnderTDZ);
> 
> have you tried this out inside the inspector to make sure it works?
> I.e, pausing inside an arrow function and typing in "super" into the console?

Good question. I vent done this. I've checked and it does not work. When I typing 'super' into console it raise exception 'SyntaxError: super is only valid inside functions.' The same behavior inside of the method of class and inside of the arrow function in class method. Looks like bug in class implementation.

>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:160
>> +      erorr = true;
> 
> typo: "erorr" => "error"

Fixed

>> LayoutTests/js/script-tests/arrowfunction-superproperty.js:78
>> +// FIXME: Problem with access to the super before super in constructor
> 
> nit: "super before super" => "super before super()"

Done
Comment 16 GSkachkov 2015-12-29 12:41:14 PST
Created attachment 267990 [details]
Patch

Fix comments & Rebase
Comment 17 Saam Barati 2015-12-29 16:21:35 PST
Comment on attachment 267799 [details]
Patch

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

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:575
>>> +    if (SourceParseMode::ArrowFunctionMode == parseMode && (functionNode->usesThis() || isDerivedClassContext() || isDerivedConstructorContext()))
>> 
>> Why are these new conditions needed?
>> Why do we need to load this if we're "SourceParseMode::ArrowFunctionMode == parseMode && (isDerivedClassContext() || isDerivedConstructorContext())"?
> 
> Without this condition following code will raise ReferenceError 'this' is undefined so to fix this error I've added this condition.
> 
> var A = class A {
>     constructor() {
>         this.value = 'testValue';
>     }
>     getValue () {
>         return this.value;
>     }
>     
> };
> 
> var B = class B extends A {
>     getParentValue() {
>         var arrow  = () => super.getValue();
>         return arrow();
>     }
> };
> 
> var b = new B();
> b. getParentValue()

Gotcha. Makes sense.

>>> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:195
>>> +    EvalExecutable* eval = EvalExecutable::create(callFrame, makeSource(script), codeBlock.isStrictMode(), thisTDZMode, codeBlock.unlinkedCodeBlock()->derivedContextType(), codeBlock.unlinkedCodeBlock()->isArrowFunction(), &variablesUnderTDZ);
>> 
>> have you tried this out inside the inspector to make sure it works?
>> I.e, pausing inside an arrow function and typing in "super" into the console?
> 
> Good question. I vent done this. I've checked and it does not work. When I typing 'super' into console it raise exception 'SyntaxError: super is only valid inside functions.' The same behavior inside of the method of class and inside of the arrow function in class method. Looks like bug in class implementation.

It seems like we should just give our parser the ability to allow this behind some flag. super isn't allowed in 'eval', but it's obviously useful to have this ability inside the debugger when paused inside one of these methods/constructors.
Comment 18 Saam Barati 2015-12-29 16:32:45 PST
Comment on attachment 267799 [details]
Patch

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

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:813
>>> +                }
>> 
>> Nit: I think this whole section of code would be easier to read like this:
>> ```
>> if (constructorKind() == ConstructorKind::Derived || isDerivedConstructorContext())
>>     newDerivedContextType = DerivedContextType::DerivedConstructorContext;
>> else if (m_codeBlock->isClassContext() || (m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext))
>>     newDerivedContextType =  DerivedContextType::DerivedMethodContext
>> ```
>> 
>> Also, why do we need to check "m_codeBlock->isArrowFunction()" when checking "m_derivedContextType == DerivedContextType::DerivedMethodContext"?
>> When would we have "!m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext"?
> 
> Ohh, I see. We don't need to check m_codeBlock->isArrowFunction() because it already true because of this condition metadata->parseMode() == SourceParseMode::ArrowFunctionMode.

No, that's not true.
m_codeBlock is the function we're generating code for. metadata is the inner function we're creating.
We could have 'metadata->parseMode() == ArrowFunctionMode' but also have m_codeBlock be the global scope or an eval or a regular function.
The interesting thing here is m_derivedContextType.

My question is this:
Does 'm_derivedContextType == DerivedContextType::DerivedMethodSyntax' imply that 'm_codeBlock->isArrowFunction()'?
If that statement is not true, when would we have 'm_derivedContextType == DerivedContextType::DerivedMethodSyntax' but also have '!m_codeBlock->isArrowFunction()'.
Is that even possible to have DerivedMethodSyntax when the parent function is not an arrow function? This seems like it should be impossible.
Comment 19 GSkachkov 2015-12-30 03:11:40 PST
Created attachment 268003 [details]
Patch

Small change in conditions
Comment 20 GSkachkov 2015-12-30 08:41:18 PST
Comment on attachment 267799 [details]
Patch

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

>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:813
>>>> +                }
>>> 
>>> Nit: I think this whole section of code would be easier to read like this:
>>> ```
>>> if (constructorKind() == ConstructorKind::Derived || isDerivedConstructorContext())
>>>     newDerivedContextType = DerivedContextType::DerivedConstructorContext;
>>> else if (m_codeBlock->isClassContext() || (m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext))
>>>     newDerivedContextType =  DerivedContextType::DerivedMethodContext
>>> ```
>>> 
>>> Also, why do we need to check "m_codeBlock->isArrowFunction()" when checking "m_derivedContextType == DerivedContextType::DerivedMethodContext"?
>>> When would we have "!m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext"?
>> 
>> Ohh, I see. We don't need to check m_codeBlock->isArrowFunction() because it already true because of this condition metadata->parseMode() == SourceParseMode::ArrowFunctionMode.
> 
> No, that's not true.
> m_codeBlock is the function we're generating code for. metadata is the inner function we're creating.
> We could have 'metadata->parseMode() == ArrowFunctionMode' but also have m_codeBlock be the global scope or an eval or a regular function.
> The interesting thing here is m_derivedContextType.
> 
> My question is this:
> Does 'm_derivedContextType == DerivedContextType::DerivedMethodSyntax' imply that 'm_codeBlock->isArrowFunction()'?
> If that statement is not true, when would we have 'm_derivedContextType == DerivedContextType::DerivedMethodSyntax' but also have '!m_codeBlock->isArrowFunction()'.
> Is that even possible to have DerivedMethodSyntax when the parent function is not an arrow function? This seems like it should be impossible.

OK I see, I think also it impossible. At time when I start develop this patch I didn't use  parseMode. Now DerivedContextType::DerivedMethodSyntax can be set only for inner arrow function because of the condition metadata->parseMode() == ArrowFunctionMode.

Before idea of this condition was to explicitly allow inherit only of DerivedMethodContext for arrow function and prevent inherit for regular function.
var A = class A { 
      getValue () { 
            return this.value; 
      } 
}; 

var B = class B extends A { 
      getParentValueCase1() { 
            var arrow = () =>  { 
                 var f = function () {
                        debug('There is no super'); 
                        return '';
                 };
                 return f();
            };
            return arrow(); 
      } 
      getParentValueCase2() { 
            var f = function () {
                   debug('There is no super');
                   return '';
            };       
            return f(); 
      } 
};

>>>> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:195
>>>> +    EvalExecutable* eval = EvalExecutable::create(callFrame, makeSource(script), codeBlock.isStrictMode(), thisTDZMode, codeBlock.unlinkedCodeBlock()->derivedContextType(), codeBlock.unlinkedCodeBlock()->isArrowFunction(), &variablesUnderTDZ);
>>> 
>>> have you tried this out inside the inspector to make sure it works?
>>> I.e, pausing inside an arrow function and typing in "super" into the console?
>> 
>> Good question. I vent done this. I've checked and it does not work. When I typing 'super' into console it raise exception 'SyntaxError: super is only valid inside functions.' The same behavior inside of the method of class and inside of the arrow function in class method. Looks like bug in class implementation.
> 
> It seems like we should just give our parser the ability to allow this behind some flag. super isn't allowed in 'eval', but it's obviously useful to have this ability inside the debugger when paused inside one of these methods/constructors.

I've created issue https://bugs.webkit.org/show_bug.cgi?id=152597
Comment 21 Saam Barati 2015-12-30 12:23:41 PST
Comment on attachment 268003 [details]
Patch

LGTM
Comment 22 WebKit Commit Bot 2015-12-30 13:08:39 PST
Comment on attachment 268003 [details]
Patch

Clearing flags on attachment: 268003

Committed r194449: <http://trac.webkit.org/changeset/194449>
Comment 23 WebKit Commit Bot 2015-12-30 13:08:43 PST
All reviewed patches have been landed.  Closing bug.