<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Arrow function syntax. Arrow function specific features. Lexical bind "super" property"
href="https://bugs.webkit.org/show_bug.cgi?id=149615#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Arrow function syntax. Arrow function specific features. Lexical bind "super" property"
href="https://bugs.webkit.org/show_bug.cgi?id=149615">bug 149615</a>
from <span class="vcard"><a class="email" href="mailto:sbarati@apple.com" title="Saam Barati <sbarati@apple.com>"> <span class="fn">Saam Barati</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=267103&action=diff" name="attach_267103" title="Patch">attachment 267103</a> <a href="attachment.cgi?id=267103&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=267103&action=review">https://bugs.webkit.org/attachment.cgi?id=267103&action=review</a>
Mostly looks good to me but I have a question and some basic comments.
<span class="quote">> Source/JavaScriptCore/ChangeLog:9
> + inside of the arrow function in case if arrow function is nested in method, </span >
Might be worth explicitly adding constructor to this list of available contexts inside a class.
<span class="quote">> Source/JavaScriptCore/ChangeLog:11
> + class, lead to wrong type of error, should be SyntaxError and this will be fixed in separete patch.</span >
Do you have a bug open for this? If you don't, you should make one.
You should link to it here
<span class="quote">> 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)</span >
style: Remove the "_" prefixes.
<span class="quote">> 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;</span >
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.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:578
> +</span >
style: revert newline.
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:799
> + bool newisDerivedConstructorContext = constructorKind() == ConstructorKind::Derived || (derivedContextType() == DerivedContextType::DerivedConstructorContext && metadata->parseMode() == SourceParseMode::ArrowFunctionMode);</span >
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?
<span class="quote">> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:806
> + bool isArrowFunctionInClassContext = m_codeBlock->isClassContext() || (m_codeBlock->isArrowFunction() && m_derivedContextType == DerivedContextType::DerivedMethodContext);
> + derivedContextType = isArrowFunctionInClassContext ? DerivedContextType::DerivedMethodContext : DerivedContextType::None;</span >
same here.
<span class="quote">> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:189
> +</span >
style: revert newline.
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:1
> +var testCase = function (actual, expected, message) {</span >
Style: 4-space indent this file.
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:158
> +// Fixme: should by check if e instanceof SyntaxError <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Arrow function syntax. Using 'super' in arrow function that declared out of the class should lead to Syntax error"
href="show_bug.cgi?id=150893">https://bugs.webkit.org/show_bug.cgi?id=150893</a></span >
Style: "Fixme" => "FIXME" and also indented properly.
<span class="quote">> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:215
> +// FIXME: Problem with access to the super before super in constructor
> +// <a class="bz_bug_link
bz_status_NEW "
title="NEW - ES6 Class syntax. Invoking method of parent class in constructor before super() lead to crash"
href="show_bug.cgi?id=152108">https://bugs.webkit.org/show_bug.cgi?id=152108</a>
> +// let j2 = new J(true);
> +// testCase(j2._value, testValue, 'Error: Some problem with using "super" inside of the constructor');</span >
Style: Indent this comment and all others to be at the level of block they're in.
<span class="quote">> LayoutTests/js/script-tests/arrowfunction-superproperty.js:1
> +description('Tests for ES6 arrow function, access to the super property in arrow function');</span >
Style: 4-space indent this file.
<span class="quote">> LayoutTests/js/script-tests/arrowfunction-superproperty.js:80
> +//shouldThrow('(new C(false))', 'ReferenceError');</span >
This should be "new C(true)"</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>