[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 #21 from GSkachkov <gskachkov at gmail.com> ---
Comment on attachment 263614
  --> https://bugs.webkit.org/attachment.cgi?id=263614
Patch

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

Thanks for the review! I'll hope to land the patch tomorrow

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:163
>> +    unsigned m_isDerivedContext: 1;
> 
> What exactly does m_isDerivedContext mean?
> Does it strictly mean I'm inheriting "this/super/new.target/arguments" from my parent context?
> If so, maybe a better name is m_isInheritedLexicalEnvironment.
> If not, what does it mean?

It means: arrow function was created in constructor of class that has parent (constructorKind() == ConstructorKind::Derived). So in such arrow function we can invoke super and we need put 'this' to arrow function lexical scope after invoking 'super'  and etc.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:492
>> +                emitGetThisFromArrowFunctionLexicalEnvironment();
> 
> Is this needed here?
> Doesn't each "this" AST node call this function? Maybe it's needed for other things that assume m_thisRegister contains the correct value?

We call this function in 'this' AST node only when we in constructor that contains arrow function, and there is possibility that 'this' is created by 'super()' in arrow function. In current place we load bound value to m_thisRegister only at start of arrow function if 'this' is used in body of current function.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:493
>> +                emitGetNewTargeFromArrowFunctionLexicalEnvironment();
> 
> why does this depend on "usesThis()"? I think this should happen regardless.

I'll fix

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1955
>> +Variable BytecodeGenerator::createVariableFromStack(Identifier varIdentifier)
> 
> This seems very wrong to me. Why would this always be in the top-most scope on the stack? I don't think that's true.
> We already know what lexicalEnvironmentRegister holds the scoped "this", we should use that to construct our
> own "this" variable. I would remove this function entirely and do the logic in the function above.
> 
> Maybe one way to create the proper scoped "this" variable is to just implement your own walking
> of the m_symbolTableStack. We know for sure that it must be in there, so we should just assert
> that we always find it. That way we ensure any callers of this function only call it if the "this"
> is indeed on the scope stack.

I double checked and I see now that this function is unnecessary. It is covered by variable() function.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3919
>> +void BytecodeGenerator::emitLoadArrowFunctionLexicalEnvironment()
> 
> nit: I think a better name for this is "emitResolveArrowFunctionContextLexicalEnvironment".
> Also, I think this only ever has to happen once. It should always resolve to the same scope.

OK. Will do, but can we avoid using 'Context' word? Name so long

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3920
>> +{
> 
> It's probably good form to assert that we're an arrow function in this function.

For now I'm calling this function in following cases
1) In arrow function
2) In 'eval' in case if 'eval' is called inside of arrow function
3) In constructor during access to 'this', because arrow function can put 'this' back.

So assert has to be ASSERT(m_codeBlock.isArrowFunction() || m_codeBlock.isArrowFunctionContext() || constructorKind() == ConstructorKind::Derived); is it OK?

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:683
>> +        bool instantiateLexicalVariables2(const VariableEnvironment&, SymbolTable*, ScopeRegisterType, LookUpVarKindFunctor);
> 
> debugging code?

removed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:831
>> +        RegisterID* m_arrowFunctionlexicalEnvRegister { nullptr };
> 
> This is a very weird member variable. It means two different things depending on what context we're generating code in.
> I think it'd be clearer to separate this out into two different member variables.
> One member variable should represent the environment that this gets assigned to in "initializeEmptyVarLexicalEnvironmentIfNeeded()" (or whatever you rename it to).
> The other member variable should represent the result of "emitLoadArrowFunctionEnvironment"
> 
> Even though these things are similar because they're the environment registers that hold "this"/"new.target"/etc,
> they're not exactly equal because we decide to create one, and we resolve to the other. I think separating them
> is cleaner. It only costs us an extra pointer in memory which is fine inside the BytecodeGenerator.

OK. I'll separate cases.

-- 
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/20151021/d7f87e76/attachment.html>