[ES6] Class expression should have lexical environment that has itself as an imutable binding

[Yusuke Suzuki]

[ES6] Class expression should have lexical environment that has itself as an imutable binding

[Yusuke Suzuki]

Created attachment 262990 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 262990 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3014
> +        generator.pushLexicalScope(this, true);

section 14.5.14, step 4, 6-a, 11.

http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3084
> +        generator.emitPutToScope(scope.get(), classNameVar, constructor.get(), ThrowIfNotFound, Initialization);

section 14.5.14, step 23.

http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation

[Geoffrey Garen]

Comment on attachment 262990 [details]
Patch

r=me

[Saam Barati]

Comment on attachment 262990 [details]
Patch

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

LGTM

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3014
>> +        generator.pushLexicalScope(this, true);
> 
> section 14.5.14, step 4, 6-a, 11.
> 
> http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation

You could store the constructor into the scope earlier, once the constructor is generated.
That way, all the method/static method properties don't get compile with the class name being under TDZ.

You could also get more fancy and eliminate the TDZ check in the constructor, but there is probably
not an obvious way to do this that doesn't clutter the code.

[Saam Barati]

(In reply to comment #4)
> Comment on attachment 262990 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262990&action=review
> 
> LGTM
> 
> >> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3014
> >> +        generator.pushLexicalScope(this, true);
> > 
> > section 14.5.14, step 4, 6-a, 11.
> > 
> > http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation
> 
> You could store the constructor into the scope earlier, once the constructor
> is generated.
> That way, all the method/static method properties don't get compile with the
> class name being under TDZ.
> 
> You could also get more fancy and eliminate the TDZ check in the
> constructor, but there is probably
> not an obvious way to do this that doesn't clutter the code.
Actually, there is an easy way to have nothing be under TDZ.
You could call BytecodeGenerator::pushLexicalScopeInternal instead of
pushLexicalScope. It has more customizability. If you do this, though,
you should probably rename pushLexicalScopeInternal because it will
no longer be internal to BytecodeGenerator.

[Build Bot]

Comment on attachment 262990 [details]
Patch

Attachment 262990 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/280047

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-with-mutations.html
inspector/css/stylesheet-events-inspector-stylesheet.html
inspector/unit-tests/event-listener-set.html
inspector/css/stylesheet-events-basic.html
inspector/console/messagesCleared.html
inspector/console/clearMessages.html
inspector/console/command-line-api.html
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
inspector/console/messageRepeatCountUpdated.html
inspector/css/stylesheet-events-multiple-documents.html
inspector/unit-tests/event-listener.html

[Build Bot]

Created attachment 262999 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 262990 [details]
Patch

Attachment 262990 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/280049

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-with-mutations.html
inspector/css/stylesheet-events-inspector-stylesheet.html
inspector/unit-tests/event-listener-set.html
inspector/css/stylesheet-events-basic.html
inspector/console/messagesCleared.html
inspector/console/clearMessages.html
inspector/console/command-line-api.html
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
inspector/console/messageRepeatCountUpdated.html
inspector/css/stylesheet-events-multiple-documents.html
inspector/unit-tests/event-listener.html

[Build Bot]

Created attachment 263000 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Yusuke Suzuki]

Comment on attachment 262990 [details]
Patch

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

>>>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3014
>>>> +        generator.pushLexicalScope(this, true);
>>> 
>>> section 14.5.14, step 4, 6-a, 11.
>>> 
>>> http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation
>> 
>> You could store the constructor into the scope earlier, once the constructor is generated.
>> That way, all the method/static method properties don't get compile with the class name being under TDZ.
>> 
>> You could also get more fancy and eliminate the TDZ check in the constructor, but there is probably
>> not an obvious way to do this that doesn't clutter the code.
> 
> Actually, there is an easy way to have nothing be under TDZ.
> You could call BytecodeGenerator::pushLexicalScopeInternal instead of
> pushLexicalScope. It has more customizability. If you do this, though,
> you should probably rename pushLexicalScopeInternal because it will
> no longer be internal to BytecodeGenerator.

Nice catch.
I think moving initialization part just before the following can solve the problem :)

if (m_staticMethods)
    generator.emitNode(constructor.get(), m_staticMethods);

And we should keep TDZ in superClass phase. I've just added the test to cover it.

class A extends A {
};
=> "ReferenceError: Cannot access uninitialized variable."

According to the step 6-b, ClassHeritage is executed under this class scope. But at that time, the variable is not initialized yet (TDZ).

[Yusuke Suzuki]

The errors raised in EWS;

WebInspector JS code has the following,

WebInspector.Object = class Object
{
    static method()
    {
       Object.keys(...)
    }
}

Object inside method is shadowed by the user-defined Object (not global Object constructor). So there is no Object.keys.
Before this patch, we don't define the variable Object anywhere because this is the class expression. But after this patch, it correctly defines class scope with `Object` variable and it makes the current code an error.

[Saam Barati]

(In reply to comment #10)
> Comment on attachment 262990 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262990&action=review
> 
> >>>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3014
> >>>> +        generator.pushLexicalScope(this, true);
> >>> 
> >>> section 14.5.14, step 4, 6-a, 11.
> >>> 
> >>> http://ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation
> >> 
> >> You could store the constructor into the scope earlier, once the constructor is generated.
> >> That way, all the method/static method properties don't get compile with the class name being under TDZ.
> >> 
> >> You could also get more fancy and eliminate the TDZ check in the constructor, but there is probably
> >> not an obvious way to do this that doesn't clutter the code.
> > 
> > Actually, there is an easy way to have nothing be under TDZ.
> > You could call BytecodeGenerator::pushLexicalScopeInternal instead of
> > pushLexicalScope. It has more customizability. If you do this, though,
> > you should probably rename pushLexicalScopeInternal because it will
> > no longer be internal to BytecodeGenerator.
> 
> Nice catch.
> I think moving initialization part just before the following can solve the
> problem :)
> 
> if (m_staticMethods)
>     generator.emitNode(constructor.get(), m_staticMethods);
> 
> And we should keep TDZ in superClass phase. I've just added the test to
> cover it.
> 
> class A extends A {
> };
> => "ReferenceError: Cannot access uninitialized variable."
> 
> According to the step 6-b, ClassHeritage is executed under this class scope.
> But at that time, the variable is not initialized yet (TDZ).
Nice. I forgot about this.

[Yusuke Suzuki]

https://bugs.webkit.org/show_bug.cgi?id=150093 After this is fixed, I'll land the fixed patch.

[Yusuke Suzuki]

Ah, oops. There is another edge case. (This is included in tests/es6)


class C {
    [C]() {    // This should touch TDZ.
    }
}

So in the meantime, I'll land the patch as is (with the added tests).
And I'll open the bug to optimize the common case.

[Saam Barati]

(In reply to comment #14)
> Ah, oops. There is another edge case. (This is included in tests/es6)
> 
> 
> class C {
>     [C]() {    // This should touch TDZ.
>     }
> }
> 
> So in the meantime, I'll land the patch as is (with the added tests).
> And I'll open the bug to optimize the common case.

Lol. This is funny.
Your plan sounds good to me. 
Are computed methods the only way we could try to resolve the constructor
before we putToScope?

[Yusuke Suzuki]

Committed r191030: <http://trac.webkit.org/changeset/191030>

[Yusuke Suzuki]

Debug build causes CRASH. (With assertion).
Investigating...

[Yusuke Suzuki]

OK, figured out.

Current Class expression implementation declares method names as a variable.
For example,

class A {
    method() {
    }
}

In the above case, we declare a variable `method` in the current implementation.
I guess this is the old draft behavior because I can't see any step in the current spec.

http://www.ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-definemethod

As a result, if we evaluate the following,

class A {
    1() {
    }
}

We attempt to declare the variable `1`. We now wrap the class body with the class lexical scope. However, the lexical scope does not allow us to declare incorrect Identifier variable (like `1`). So assertion is fired.

[Yusuke Suzuki]

We will fix this in the other bug.

[Yusuke Suzuki]

(In reply to comment #19)
> We will fix this in the other bug.

Handled in https://bugs.webkit.org/show_bug.cgi?id=150115

[WebKit Commit Bot]

Re-opened since this is blocked by bug 150116

[Yusuke Suzuki]

After https://bugs.webkit.org/show_bug.cgi?id=150115 is landed, I'll attempt to reland.

[Yusuke Suzuki]

Committed r191110: <http://trac.webkit.org/changeset/191110>

[Yusuke Suzuki]

Since  https://bugs.webkit.org/show_bug.cgi?id=150115 is landed, I've landed the roll-outed patch.