[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 #262540|review?                     |review-
              Flags|                            |

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

This patch is much better than before, but I think there are a few things that still need to be done to make it cleaner and correct.

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

Why is this needed?

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

this is unused.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:170
> +    if (programNode->usesArrowFunction())

Should this be "if (programNode->usesArrowFunction() || usesEval())"?

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

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:540
> +        initializeEmptyVarLexicalEnvironment(vm);

(I have a note on this function below. I think it can be nicer).

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:541
> +    else

This shouldn't be in an else clause. It should happen regardless of the above condition.

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2937
> +void BytecodeGenerator::emitUpdateScopeIfNeed()

Lets call this "emitPutThisToScopeIfNeeded" or something along those lines.

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

-- 
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/20151007/7a8009ef/attachment.html>