Bug 140491 – Implement ES6 class syntax

• Component: JavaScriptCore
• Status: RESOLVED CONFIGURATION CHANGED

[Ryosuke Niwa]

We should support class:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-class-definitions

[Ryosuke Niwa]

<rdar://problem/19467094>

[Ryosuke Niwa]

Created attachment 244682 [details]
Super naive WIP 1

With this patch, the following code runs successfully.  I still need to add the support for super() as well as make the object initialization check (throw ReferenceError whenever this is accessed before the initialization) work.

class A {
    someMethod() { alert('hi'); }
    static someStaticMethod(o) { o.someMethod(); }
    constructor() {}
    static constructor() {}
};
class B extends A {
    constructor() { }
}
var b = new B();
A.someStaticMethod(b);

[Oliver Hunt]

Comment on attachment 244682 [details]
Super naive WIP 1

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

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2771
> +        // FIXME: Throw TypeError if isConstructor(parentClass) is false or it's a generator function.

I'm honestly leaning towards planting a private property on the type and using that to make you decision (e.g. foo.@IsConstructor or some such)

[Geoffrey Garen]

> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2771
> > +        // FIXME: Throw TypeError if isConstructor(parentClass) is false or it's a generator function.
> 
> I'm honestly leaning towards planting a private property on the type and
> using that to make you decision (e.g. foo.@IsConstructor or some such)

Another option is to use a special JSFunction subclass for constructors.

[Ryosuke Niwa]

Created attachment 244853 [details]
Added "super" support

[Ryosuke Niwa]

The new WIP is still missing two major pieces:

1. Throw ReferneceError when "this" is still in "uninitialized" state (i.e. before "super()" is called) in a derived class' constructor.
2. Auto generate the constructor when it's not supplied.

I think I'll do 2 in a separate follow-up patch since JSC doesn't have a facility to generate a Miranda function.

I've talked about 1 with Oliver and Gavin, and the approach we came up so far (if I remember correctly) is to allocate a local variable/register that stores "uninitialized-ness" of the "this" variable. Inside ThisNode or wherever else "this" is accessed, we'll generate a code to check this state and throw ReferenceError when it's not initialized.

Once that's implemented, we can optimize this in the parser as well as in the bytecode generator for common cases.  e.g. in cases where we can guarantee that super() is definitively called before "this" is used, we can omit this check.  Even if we didn't implement such an optimization, DFG might be able to optimize them away.

[Geoffrey Garen]

> I've talked about 1 with Oliver and Gavin, and the approach we came up so
> far (if I remember correctly) is to allocate a local variable/register that
> stores "uninitialized-ness" of the "this" variable.

I think you can just use undefined to mean uninitialized.

The ThisNode in the AST can emit explicit branches around access to 'this' to check for undefined, if and only if you're compiling a constructor that extends a builtin.

The DFG will probably dead-code-eliminate those branches in simple cases. The FTL should get them in all cases.

[Ryosuke Niwa]

Created attachment 244964 [details]
Added ReferenceError for this inside derived class' constructor

I've added the support for throwing ReferenceError inside a derived class' constructor.  There is one ugliness here, which is having to remember whether a given constructor is for a derived class or not.  I don't think we can deduce this from prototype or constructor as far as I read the spec so I'm introducing a yet another private property name to store this information on the constructor object.

Now I need to figure out how to pass an argument to eval operator so that every access to this inside an eval operator in a derived class' constructor would check the extra register allocated in BytecodeGenerator for the constructor.

[Ryosuke Niwa]

Created attachment 245050 [details]
Set this to undefined instead of throwing

Geoff, Gavin, and I had an extensive discussion on this matter and decided that we should just set "this" to undefined for now. I've emailed es-discuss to change the spec accordingly.

[Filip Pizlo]

(In reply to comment #9)
> Created attachment 245050 [details]
> Set this to undefined instead of throwing
> 
> Geoff, Gavin, and I had an extensive discussion on this matter and decided
> that we should just set "this" to undefined for now. I've emailed es-discuss
> to change the spec accordingly.

I take it this is no longer true? ;-)

[Ryosuke Niwa]

Created attachment 245268 [details]
Updated for ToT

[Ryosuke Niwa]

Created attachment 245506 [details]
Diff against my patch on 140918

I've also made the [[Construct]] use the right structure. This involves changing how op_create_this works. At the moment, we use the "callee" object to find the structure for the new object but we need to be using the most derived class's constructor (called newTarget).

After discussing with Gavin & Geoff, I'm using Gavin's idea to use "this" register for this purpose. The idea is to put the newTarget into the "this" register and then op_create_this will obtain the structure out of that value instead of the callee object.

The alternatives invoke modifying the way arguments work in "construct" and that seemed much worse hack than this one.

[Ryosuke Niwa]

Created attachment 247489 [details]
Added op_check_tdz in 64-bit LLint & Baseline JIT

[Ryosuke Niwa]

I think I'm going to just implement subclassing without TDZ first, and then add TDZ in a separate patch to keep the patch sizes small.

[WebKit Commit Bot]

Attachment 247489 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.h:1622:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/JavaScriptCore/parser/Nodes.h:1623:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:83:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:175:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:543:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 5 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anne van Kesteren]

This has been fixed for a while.