Bug 140491 - Implement ES6 class syntax
Summary: Implement ES6 class syntax
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar, WebExposed
Depends on: 152597 157461 157972 140752 140754 140760 140761 140908 140918 141019 142200 142388 142527 142563 142566 142567 142591 142600 142689 142690 142774 142840 142842 142862 142882 142883 143037 143038 143170 143180 143181 144238 144243 144244 144252 144254 144258 144278 144280 144281 144282 144283 144284 144285 144514 147064 147150 148057 149001 150089 151113 151160 151365 152108 153864 155545 157136
Blocks:
  Show dependency treegraph
 
Reported: 2015-01-14 23:46 PST by Ryosuke Niwa
Modified: 2016-05-22 21:47 PDT (History)
13 users (show)

See Also:


Attachments
Super naive WIP 1 (44.48 KB, patch)
2015-01-14 23:53 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added "super" support (65.47 KB, patch)
2015-01-17 21:16 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added ReferenceError for this inside derived class' constructor (68.91 KB, patch)
2015-01-19 21:09 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Set this to undefined instead of throwing (80.41 KB, patch)
2015-01-20 23:09 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated for ToT (55.35 KB, patch)
2015-01-23 18:05 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Diff against my patch on 140918 (48.75 KB, patch)
2015-01-27 19:33 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added op_check_tdz in 64-bit LLint & Baseline JIT (56.49 KB, patch)
2015-02-26 20:19 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2015-01-14 23:46:16 PST
We should support class:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-class-definitions
Comment 1 Ryosuke Niwa 2015-01-14 23:46:57 PST
<rdar://problem/19467094>
Comment 2 Ryosuke Niwa 2015-01-14 23:53:03 PST
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);
Comment 3 Oliver Hunt 2015-01-15 09:06:37 PST
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)
Comment 4 Geoffrey Garen 2015-01-15 10:37:45 PST
> > 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.
Comment 5 Ryosuke Niwa 2015-01-17 21:16:05 PST
Created attachment 244853 [details]
Added "super" support
Comment 6 Ryosuke Niwa 2015-01-17 21:28:28 PST
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.
Comment 7 Geoffrey Garen 2015-01-19 11:45:57 PST
> 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.
Comment 8 Ryosuke Niwa 2015-01-19 21:09:30 PST
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.
Comment 9 Ryosuke Niwa 2015-01-20 23:09:27 PST
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.
Comment 10 Filip Pizlo 2015-01-21 21:20:25 PST
(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? ;-)
Comment 11 Ryosuke Niwa 2015-01-23 18:05:35 PST
Created attachment 245268 [details]
Updated for ToT
Comment 12 Ryosuke Niwa 2015-01-27 19:33:44 PST
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.
Comment 13 Ryosuke Niwa 2015-02-26 20:19:45 PST
Created attachment 247489 [details]
Added op_check_tdz in 64-bit LLint & Baseline JIT
Comment 14 Ryosuke Niwa 2015-02-26 20:21:08 PST
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.
Comment 15 WebKit Commit Bot 2015-03-02 07:34:29 PST
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.