<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 created before super() causes TDZ, should it?"
href="https://bugs.webkit.org/show_bug.cgi?id=149338#c45">Comment # 45</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [ES6] Arrow function created before super() causes TDZ, should it?"
href="https://bugs.webkit.org/show_bug.cgi?id=149338">bug 149338</a>
from <span class="vcard"><a class="email" href="mailto:gskachkov@gmail.com" title="GSkachkov <gskachkov@gmail.com>"> <span class="fn">GSkachkov</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=264022&action=diff" name="attach_264022" title="Patch">attachment 264022</a> <a href="attachment.cgi?id=264022&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=264022&action=review">https://bugs.webkit.org/attachment.cgi?id=264022&action=review</a>
<span class="quote">>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:163
>> + unsigned m_isDerivedConstructorContext: 1;
>
> style: "m_isDerivedConstructorContext:" => "m_isDerivedConstructorContext :"</span >
Done
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:807
>> +void BytecodeGenerator::initializeEmptyVarLexicalEnvironmentIfNeeded(SymbolTable* systemTable)
>
> "systemTable" => "symbolTable"
> I also don't like the name "initializeEmptyVarLexicalEnvironmentIfNeeded". It doesn't really mean what you're doing here.
> I think a better name would be something along the lines of: "initializeArrowFunctionContextScopeIfNeeded".</span >
Done
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:821
>> + if (m_codeType == FunctionCode) {
>
> Aren't there more specific situations where we need this? Like if we're in a derived constructor context?</span >
I've decided to include into this patch the implementation of lexically binding of new.target for all cases, because it is required small changes in comparison to patch without it( current condition + condition to load new.target into function context + tests)
new.target is always exist for function and lexically bound to the arrow function
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:849
>> + m_arrowFunctionVarLexicalEnvRegister = emitMove(newRegister(), m_scopeRegister);
>
> I think you want "newBlockScopeVariable" instead of "newRegister"</span >
fixed
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2981
>> +void BytecodeGenerator::emitUpdateScopeIfNeeded()
>
> I think a better name for this would be something like:
> "emitUpdateArrowFunctionContextScope".
> This names seems to general. It would be good to pin it down to being arrowfunction-related.</span >
OK. I think emitUpdateVariablesInArrowFunctionContextScope will be better, because we change values of the variables in arrow function scope?
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2996
>> +void BytecodeGenerator::emitPutThisToScope()
>
> Again, I would signify in the name that this is related to arrow functions.</span >
Renamed to emitPutThisToArrowFunctionContextScope
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3897
>> +void BytecodeGenerator::emitGetNewTargeFromArrowFunctionLexicalEnvironment()
>
> typo: "emitGetNewTarge..." => "emitGetNewTarget..."</span >
fixed
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:827
>> + RegisterID* m_arrowFunctionVarLexicalEnvRegister { nullptr };
>
> nit: I would call this something like: "m_arrowFunctionContextLexicalEnvironmentRegister".
> I think "var" is the wrong word here.</span >
renamed
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:828
>> + RegisterID* m_arrowFunctionResolvedLexicalEnvRegister { nullptr };
>
> style: I would spell out "Environment" in this name, even though it's verbose.</span >
Done
<span class="quote">>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:699
>> +// We need try to load 'this' before call eval, because it can created by 'super' in some of the arrow function
>
> style: Indent this.
> Also, I'm a bit confused about this. Do we get the current function's "this" when making a function call?</span >
I think it has to rewritten -> // We need try to load 'this' before call eval in constructor, because 'this' can created by 'super' in some of the arrow function
It is necessary to cover following case :
var A = class A {
constructor () { this.id = 'A'; }
}
var B = class B extend A {
constructor () {
var arrow = () => super();
arrow();
eval("this.id = 'B'");
}
}
new B();
<span class="quote">>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:738
>> + && (generator.usesArrowFunction() || generator.usesEval())))
>
> style: I'd move this to the above line.
> WebKit style says that this should be 4 spaces from the indentation of the "if", which would look bad here. I would just move it one line up.</span >
I've changed, now it has two variable and condition in one line:
bool isConstructorKindDerived = generator.constructorKind() == ConstructorKind::Derived;
bool usesArrowFunctionOrEval = generator.usesArrowFunction() || generator.usesEval();
if (generator.isDerivedConstructorContext() || (isConstructorKindDerived && usesArrowFunctionOrEval))</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>