<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#c18">Comment # 18</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=262540&action=diff" name="attach_262540" title="Patch">attachment 262540</a> <a href="attachment.cgi?id=262540&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=262540&action=review">https://bugs.webkit.org/attachment.cgi?id=262540&action=review</a>
Thanks for the review!
Now I'm running the tests in debug mode.
After success I'll upload the patch. I hope that it will be happened today :-)
<span class="quote">>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1663
>> + , m_needsActivation(unlinkedCodeBlock->hasActivationRegister() && (unlinkedCodeBlock->codeType() == FunctionCode || unlinkedCodeBlock->codeType() == GlobalCode || unlinkedCodeBlock->codeType() == EvalCode) )
>
> Why is this needed?</span >
It allow to avoid ASSERT error in method needsActivation for Eval and Program case. As I understand we create lexical scope for them so we need activate it.
See assert ASSERT(m_lexicalEnvironmentRegister.isValid() == m_needsActivation);
<span class="quote">>> Source/JavaScriptCore/bytecode/CodeBlock.h:350
>> + void setNeedActivation(bool value)
>
> this is unused.</span >
Done
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:170
>> + if (programNode->usesArrowFunction())
>
> Should this be "if (programNode->usesArrowFunction() || usesEval())"?</span >
Done
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:485
>> + if (functionNode->usesThis() || codeBlock->usesEval() || functionNode->usesArrowFunction()) {
>
> I think we need to emitResolveScoe then emitGetFromScope for every "this" node, right?
> Just doing it once I think is not sufficient. Consider:
> class C {
> constructor() {
> let f = (c) => c ? this : {};
> f(false);
> super();
> f(true);
> }
> }</span >
Hmm, it seems that I don't understand clearly what does this methods. Your examples very close to scripts that I use to test TDZ in 'super()', see tests with name arrowfunction-tdz-1.js.
How I understand this code. Each time when arrow function is invoked, before run body of function it loads 'this' value to thisRegister. So if I this value is changed by parent function by 'super()', we get from scope of arrow function new 'this' value, because 'this' in lexical_scope will be updated by emitUpdateScopeIfNeed method that we call just after calling 'super()' see change in Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp line 730.
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:490
>> }
>
> I think you should add a line of code here that calls "emitUpdateScopeIfNeeded()"
> Currently, you emit code for "emitUpdateScopeIfNeeded()" on each arrow function that's emitted.
> This is excessive. If you simply call "emitUpdateScopeIfNeeded()" here and anytime you call super(),
> we should always have the latest "this" value.
>
> Also, I think you probably need to call "initializeEmptyVarLexicalEnvironment()" somewhere
> inside this function to be consistent with ProgramNode/EvalNode.</span >
Done
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:541
>> + else
>
> This shouldn't be in an else clause. It should happen regardless of the above condition.</span >
Done
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:788
>> +void BytecodeGenerator::initializeEmptyVarLexicalEnvironment(VM& vm)
>
> I think we can do this in a better way. We can write this purely in terms of pushLexicalScopeInternal().
> The reason is that pushLexicalScopeInternal does necessary bookkeeping for handling
> scopes, etc (especially important within try/catch).
>
> If you look at initializeDefaultParameterValuesAndSetupFunctionScopeStack() it interfaces with this function.
> I think something like this would work:
>
> ```
> VariableEnvironment environment;
> auto addResult = environment.add(thisIdentifier);
> addResult.iterator->value.setIsCaptured();
> pushLexicalScopeInternal(environment, false, nullptr, TDZRequirement::(Whatever it doesn't matter), ScopeType::LetConstScope, ScopeRegisterType::Block);
> ```
> Note that this works because the ::variable() function explicitly looks for the "this" identifier and
> therefore we won't ever search the scope stack for "this".</span >
Done
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2937
>> +void BytecodeGenerator::emitUpdateScopeIfNeed()
>
> Lets call this "emitPutThisToScopeIfNeeded" or something along those lines.</span >
Done
<span class="quote">>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2942
>> + emitPutToScope(scopeRegister(), thisVar, thisRegister(), DoNotThrowIfNotFoundAndReturnEmpty, NotInitialization);
>
> I don't like the idea of adding new resolve modes like "DoNotThrowIfNotFoundAndReturnEmpty".
> Why would we ever not find "this"?
> We should always find it.
> We should make this guarantee by making sure every function places "this" into the lexical environment if needed.</span >
In case if we put 'this' inside of the constructor before 'super()' without flag we will receive ASSERT error. If we will try to get 'this' inside of arrow function that invokes before 'super()' we will receive undefined and miss the TDZ error.
So that why I made changes in JITOperation.cpp operationGetFromScope/operationPutToScope</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>