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

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

>> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:163
>> +    unsigned m_isDerivedConstructorContext: 1;
> 
> style: "m_isDerivedConstructorContext:" => "m_isDerivedConstructorContext :"

Done

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

Done

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

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

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:849
>> +    m_arrowFunctionVarLexicalEnvRegister = emitMove(newRegister(), m_scopeRegister);
> 
> I think you want "newBlockScopeVariable" instead of "newRegister"

fixed

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

OK. I think  emitUpdateVariablesInArrowFunctionContextScope will be better, because we change values of the variables in arrow function scope?

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2996
>> +void BytecodeGenerator::emitPutThisToScope()
> 
> Again, I would signify in the name that this is related to arrow functions.

Renamed to emitPutThisToArrowFunctionContextScope

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3897
>> +void BytecodeGenerator::emitGetNewTargeFromArrowFunctionLexicalEnvironment()
> 
> typo: "emitGetNewTarge..." => "emitGetNewTarget..."

fixed

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

renamed

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:828
>> +        RegisterID* m_arrowFunctionResolvedLexicalEnvRegister { nullptr };
> 
> style: I would spell out "Environment" in this name, even though it's verbose.

Done

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

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

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

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

-- 
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/20151026/8f2744fe/attachment.html>