[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

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #263614|review?                     |review-
              Flags|                            |

--- Comment #20 from Saam Barati <sbarati at apple.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

Overall, I like this approach. There are some details that need fixing. I've commented below.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1658
> +    , m_scopeForThisRegister(unlinkedCodeBlock->scopeForThisRegister())

not used.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1660
> +    , m_needsActivation(unlinkedCodeBlock->hasActivationRegister() && (unlinkedCodeBlock->codeType() == FunctionCode || unlinkedCodeBlock->codeType() == GlobalCode || unlinkedCodeBlock->codeType() == EvalCode) )

Why is this change needed?

> Source/JavaScriptCore/bytecode/CodeBlock.h:349
> +    VirtualRegister scopeForThisRegister()

It doesn't look like this is used. And this also seems weird and unnecessary, we should be able to accomplish whatever this can without caching the scope.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:121
> +    bool isDerivedContext() const { return m_isDerivedContext;}
> +    bool isArrowFunctionContext() const { return m_isArrowFunctionContext;}

style: ";}" => "; }"

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:488
> +        if (functionNode->usesThis() || m_isDerivedContext) {

When is an arrow function ever not an "m_isDerivedContext"?

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:493
> +                emitGetNewTargeFromArrowFunctionLexicalEnvironment();

why does this depend on "usesThis()"? I think this should happen regardless.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1883
> +Variable BytecodeGenerator::variable(const Identifier& property, bool isLocal)

naming nit: "isLocal" => "isLocalThis"

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1950
> +Variable BytecodeGenerator::createThisVariable()

I would name this to: "createScopedThisVariable"

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

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3920
> +{

It's probably good form to assert that we're an arrow function in this function.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:492
> +        void emitLoadArrowFunctionLexicalEnvironment();

I think you can make this private (if it's not already) and only have it be called from emitGetThisFromArrowFunctionLexicalEnvironment

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

debugging code?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:789
> +        void initializeEmptyVarLexicalEnvironmentIfNeeded(SymbolTable* = nullptr);

I think this needs a better name, something like:
"initializeLexicalEnvironmentContextForArrowFunctions"

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

-- 
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/304801e2/attachment-0001.html>