Bug 149615

Created attachment 264232 [details] Patch Init commit

Created attachment 264803 [details] Patch Small refactoring

Created attachment 267097 [details] Patch Init load

Created attachment 267103 [details] Patch Fix build

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

Created attachment 267762 [details] Patch Fix comments

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 on attachment 267762 [details] Patch F

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

Created attachment 267799 [details] Patch Fixes small comments

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

Created attachment 267988 [details] Patch Fix comments

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

Created attachment 267990 [details] Patch Fix comments & Rebase

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

Created attachment 268003 [details] Patch Small change in conditions

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 on attachment 268003 [details] Patch LGTM

Comment on attachment 268003 [details] Patch Clearing flags on attachment: 268003 Committed r194449: <http://trac.webkit.org/changeset/194449>

All reviewed patches have been landed. Closing bug.