[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 #62 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 264685
  --> https://bugs.webkit.org/attachment.cgi?id=264685
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:831
> +        auto addTarget = environment.add(propertyNames().target);

This should be the private name as well, right?

It might be good to have tests that this code
would fail using the public identifier.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:837
> +        auto protoObject = environment.add(propertyNames().underscoreProtoScopeLocalPrivateName);

Instead of "underscoreProtoScopeLocalPrivateName" we can maybe give this a more descriptive name.
This is just the current class we're in super class, right?
like:
class A extends B { constructor() { let arr = () => { ... }  }
this will be "B", right?
Maybe we can just name this using some form of "super class", then?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3843
> +        // 'this' can be uninitialized in constructor of derived class so we resolve with underscoreProtoScope variable that stored in arrow function lexical environment in of such cases

Even though this is uninitialized, don't we always store it in the symbol table?
So if we read from the scope, we might get jsTDZValue(), but since we're just
resolving the scope, shouldn't "this" always resolve to the proper scope?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3892
> +    emitPutToScope(isDerivedConstructorContext() ? emitResolveScope(nullptr, thisVar) : m_arrowFunctionContextLexicalEnvironmentRegister, thisVar, thisRegister(), DoNotThrowIfNotFound, NotInitialization);

Why can't we always use "m_arrowFunctionContextLexicalEnvironmentRegister" here?
We could always call "emitLoadArrowFunctionContextScope()", too, just to make sure we've resolved to it.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:296
> +        Variable variable(const Identifier&, bool = true);

I think an "enum class" would be better than bool here, that way all call sites of variable() are more descriptive.
maybe: "enum class ThisResolutionType { Local, Scoped };"
or something like that.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:828
> +        RefPtr<RegisterID> m_arrowFunctionResolvedLexicalEnvRegister { nullptr };

I would make "env" to "environment" here, or maybe rename to something shorter like:
"m_resolvedArrowFunctionScopeContext"

> Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:-1024
> -                exactRead = ArrowFunctionBoundThisPLoc;

I think we should remove the definition and other code that looks at "ArrowFunctionBoundThisPLoc" since it's no longer used.

-- 
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/20151104/17719fd8/attachment-0001.html>