[Webkit-unassigned] [Bug 149338] [ES6] Arrow function created before super() causes TDZ, should it?

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=149338

--- Comment #18 from GSkachkov <gskachkov at gmail.com> ---
Comment on attachment 262540
  --> https://bugs.webkit.org/attachment.cgi?id=262540
Patch

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

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

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1663
>> +    , m_needsActivation(unlinkedCodeBlock->hasActivationRegister() && (unlinkedCodeBlock->codeType() == FunctionCode || unlinkedCodeBlock->codeType() == GlobalCode || unlinkedCodeBlock->codeType() == EvalCode) )
> 
> Why is this needed?

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

>> Source/JavaScriptCore/bytecode/CodeBlock.h:350
>> +    void setNeedActivation(bool value)
> 
> this is unused.

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:170
>> +    if (programNode->usesArrowFunction())
> 
> Should this be "if (programNode->usesArrowFunction() || usesEval())"?

Done

>> 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);
>     }
> }

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.

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

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:541
>> +    else
> 
> This shouldn't be in an else clause. It should happen regardless of the above condition.

Done

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

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2937
>> +void BytecodeGenerator::emitUpdateScopeIfNeed()
> 
> Lets call this "emitPutThisToScopeIfNeeded" or something along those lines.

Done

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

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151008/8b1ff329/attachment-0001.html>