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

--- Comment #50 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 264078
  --> https://bugs.webkit.org/attachment.cgi?id=264078
Patch

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

Patch looks mostly good. Just have some comments regarding some tricky problems with the implementation for some edge cases.

> Source/JavaScriptCore/ChangeLog:9
> +        'this' is stored inside of the lexical environment of the function. To store and load are used 

nit: "are used" => "we use"

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:849
> +    m_arrowFunctionContextLexicalEnvironmentRegister = emitMove(newBlockScopeVariable(), m_scopeRegister);

I was wrong about advising you on this earlier.
I forgot that pushLexicalScopeInternal already creates a variable for us.
Because you have captured variables, we will create a block scoped variable for
the scope. 
So all you have to do is this:
m_arrowFunctionContextLexcialEnvironmentRegister = m_symbolTableStack.last().m_scope.
Feel free to add asserts around the pushLexicalScopeInternal call to make sure it actually adds exactly one item to the symbol table stack.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2989
> +        if (m_codeType == FunctionCode)  {

I realized we don't need this.
We should only need to do this once per function.
If you look how new_target is materialized now, it happens
before we create a "this". So I think it'd be wrong to update
this value over time. It should just happen once at the very beginning of the program.

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

There are a few interesting things to consider here that seem like they are problems.
1) consider the problem of using the literal string "__proto__":
class A extends B {
    let arr1 = () => {
        let __proto__ = "blah";
        let arr2 = () => super();
        arr2();
    }
}
This strategy worked with "this" because no variable can be named "this". But variables can be named "__proto__"

2) what happens when __proto__ is overwritten (I'm not really sure if this is a problem or what's supposed to happen with respect to the specification, but it's probably worth looking into).
class A extends B {
    let arr1 = () => { ... }
    A.__proto__ = null;
    arr1();
}
I wonder what happens just in normal classes with this.

3) If we're storing values in m_calleeRegister, maybe we're doing this wrong. I think this might break stack traces because it will look like an arrow functions parent frame is displayed twice on a stack trace if an error is thrown.
It's worth considering this problem.

For all problems above, if they really are problems, we should have tests for them, too.

-- 
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/20151031/4a3886bd/attachment.html>