1066229 - Runtime support for ES6 ClassDefinitions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Actually support declaring and using basic ES6 classes (no extends, super, or static).

This will probably require new bytecode, so JIT support needs to be addressed somehow, or a follow-up bug filed.

Comment 1

[Eric Faust [:efaust]]

OK, So: Taking a look at this and planning bytecode, I think I have the following ideas:

If HeritageExpression is present, then: (bug 1066234)
    Emit HeritageExpression
Else: (Where we start, for this bug)
    Emit magic "no heritage" value
Emit JSOP_CLASSHERITAGE (pops the result of the heritage expression, pushes constructorParent, protoParent (throws appropriately)
Emit JSOP_OBJECTWITHPROTO // Object.create(stackSlot)
Emit JSOP_SWAP
Emit JSOP_FUNCTIONWITHPROTO with index of function object parsed, pops constructorParent
Push scope
Emit PropertyInitializers (making use of DUP2 and POP for statics (bug 1066238))
intialize internal binding
leave scope
initialize external binding

The reason for the magic value is that we need to get the intrinsic objects %ObjectPrototype% and %FunctionPrototype%, which is not necessarily around by querying anything dynamically in script.

The above also assumes that |constructor| was provided in the class definition. There is some mild trouble here (perhaps worthy of another bug of its own accord). One critical wrinkle is that because they are script-accessible (via .constructor), we actually need a real JSFunction object, and not just a flag or magic value.

Waldo and I talked about this yesterday, and he suggests, and I like, putting the two default constructors in self-hosted code, and then just cloning them out every time (via another new opcode). If we are worried about the scope objects not being as if that string had been evaluated in the ClassDefinition's context, we can just manually mangle them when we are setting various other things about them ([[HomeObject]] and so on), but it shouldn't actually matter since the scope object is not consulted.

Barring that, I'm not sure what we would do. We could synthesize them manually at the parsenode level during parsing if no "constructor" property was parsed in the class body, but that seems like a gross (and fragile!) hack.

Jason, what do you think of all of this? I think the Bytecode listed above looks stable enough to implement, and puts the work nicely into bitesized opcodes, so should work out, and leave JIT implementations (at first just a VM stub) relatively trivial, and that cloning out the default constructors seems the easiest way to solve that problem, unless I've missed some particulars.

Comment 2

[Eric Faust [:efaust]]

Having slept on it, I no longer like some of those ideas :). Let's try this instead, I think it's much nicer, and maybe better structured:

PROPOSED OPCODES:

JSOP_CLASSHERITAGE -
    Takes on stack: Object or null (throw otherwise) representing the evaluation of the 
                    HeritageExpression of the ClassDefinition
    Operands: None
    Leaves on stack: A prototype for both the new proto object and new class constructor

JSOP_CLASSDEFAULT
    Takes on stack: Object to be the [[Prototype]] of the cloned default constructor
    Operands: bool representing whether or not the default constructor needs a super binding
    Leaves on stack: Cloned function object.

POTENTIALL PROPOSED OPCODES:

There is to be some violence below, mostly based on not adding opcodes that are compositions of ones we already have. If we think it's too violent, then I propose these instead.

JSOP_OBJECTWITHPROTO -
    Takes on stack: Object or null to be the [[Prototype]] of the created object.
                    Assert otherwise.
    Operands: None
    Leaves on stack: Created object

JSOP_FUNCTIONWITHPROTO -
    Takes on stack: Object to be the [[Prototype]] of the created Function.
                    Assert otherwise.
    Operands: Index into script function table for the script to clone.
    Leaves on stack: Created function object

PSEUDOCODE FOR EMITTING:

There's a lot at play here, and this encompasses the plan to emit ClassDefinitions to be implemented in 3 or 4 bugs, now. What follows is pseudocode for the emitter in full, to show the final complexity, and then I propose to take a look at individual cases, and their emitted opcodes.

define EmitConstructor:


If ClassHeritage is NOT present:

Comment 3

[Eric Faust [:efaust]]

*Sigh* Misclick. Starting all over again:

Having slept on it, I no longer like some of those ideas :). Let's try this instead, I think it's much nicer, and maybe better structured:

PROPOSED OPCODES:

JSOP_CLASSHERITAGE -
    Takes on stack: Object or null (throw otherwise) representing the evaluation of the 
                    HeritageExpression of the ClassDefinition
    Operands: None
    Leaves on stack: A prototype for both the new proto object and new class constructor

JSOP_CLASSDEFAULT
    Takes on stack: Object to be the [[Prototype]] of the cloned default constructor.  
                    Undefined means "Use %FunctionPrototype". Assert otherwise.
    Operands: bool representing whether or not the default constructor needs a super binding
    Leaves on stack: Cloned function object.

JSOP_OBJECTWITHPROTO -
    Takes on stack: Object or null to be the [[Prototype]] of the created object.
                    Assert otherwise.
    Operands: None
    Leaves on stack: Created object

JSOP_FUNCTIONWITHPROTO -
    Takes on stack: Object to be the [[Prototype]] of the created Function.
                    Assert otherwise.
    Operands: Index into script function table for the script to clone.
    Leaves on stack: Created function object

PSEUDOCODE FOR EMITTING:

There's a lot at play here, and this encompasses the plan to emit ClassDefinitions to be implemented in 3 or 4 bugs, now. What follows is pseudocode for the emitter in full, to show the final complexity.

define EmitConstructor:
    If constructor was present in class body:
        Emit JSOP_FUNCTIONWITHPROTO with constructor's index
    Else:
        Emit JSOP_CLASSDEFAULT with (HeritageExpression present) as argument

If HeritageExpression was NOT present:
    undefined
    EmitConstructor()
    Emit JSOP_NEWINIT with type Object
Else: (Here's the aforementioned violence)
    Emit HeritageExpression
    Emit JSOP_CLASSHERITAGE
    EmitConstructor()
    Emit JSOP_SWAP
    Emit JSOP_OBJECTWITHPROTO

Emit JSOP_DUP2
Emit JSOP_INITPROP "prototype"
Emit JSOP_INITPROP "constructor"

if class was named (ClassStatement or named ClassExpression):
    Enter nested scope
For each method in ClassBody that is not named "constructor":
    if method is static:
        Emit JSOP_DUP2
        Emit JSOP_POP
        Emit method
        Emit method initializer op
        Emit JSOP_POP
    else:
        Emit method
        Emit method initializer op

Emit JSOP_POP

if Class was named:
    Emit JSOP_INITLEXICAL 0
    Leave Nested Scope
    Emit JSOP_INITLEXICAL (index for outer class binding)

if ClassStatement:
    Emit JSOP_POP

Comment 4

[Eric Faust [:efaust]]

One additional thought: In the Non-Present HeritageExpression case, can we just use JSOP_LAMBDA to clone the constructor, when it's present? It will save us from this |undefined; JSOP_FUNCTIONWITHPROTO| nonsense.

Comment 5

[Jason Orendorff [:jorendorff]]

Eric, all of this looks great. Just one question:

> JSOP_CLASSDEFAULT
>    Operands: bool representing whether or not the default constructor needs a super binding

Why is this bool needed? Doesn't the constructor function technically have an [[ObjectHome]] regardless of whether it uses 'super'?

It looks like the [[ObjectHome]] stuff is mostly thought through, but not explicit here. I don't see any issues with it.

Comment 6

[Eric Faust [:efaust]]

Attachment: Part 1: Create a clean way to emit lexical binding initializers.

Comment 7

[Eric Faust [:efaust]]

Attachment: Part 2: Rename EmitObject() EmitPropertyList() in preparation for classes.

Comment 8

[Eric Faust [:efaust]]

Attachment: Part 3: Emit bytecode for simple ES6 ClassDefinitions

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539075 [details] [diff] [review]
Part 1: Create a clean way to emit lexical binding initializers.

Review of attachment 8539075 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with lots of questions that I think can be addressed with comments.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6778,5 @@
>  }
>  
> +static bool
> +EmitLexicalInitialization(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn,
> +                          JSOp globalDefOp)

Needs a comment explaining what this is / what it's for.

The implementation here kind of makes me nervous because it feels like most of this code shouldn't be here in the long run, because once we have proper top-level lexical bindings, pn should always be bound and get JSOP_INITLEXICAL... is that right? If so, comment it *please*.

Look to common up code with EmitVariables if anything makes sense.

(EmitVariables is horrible. Senseless special cases everywhere, no indication of which combinations are possible, and of course no comment explaining things. This is why some people claim that avoiding booleans is an important style rule.)

@@ +6787,5 @@
> +        return false;
> +
> +    jsatomid atomIndex;
> +    if (!MaybeEmitVarDecl(cx, bce, globalDefOp, pn, &atomIndex))
> +        return false;

OK, so this does emit a var decl in some cases, because we don't have proper top-level lexical bindings yet; but really what we're doing here is populating atomIndex, is that right?

The purpose here is unclear enough to be worth a comment, I think; *or* write a good function-level comment above MaybeEmitVarDecl, you pick.

@@ +6802,5 @@
> +        if (!EmitVarOp(cx, pn, pn->getOp(), bce))
> +            return false;
> +    } else {
> +        if (!EmitIndexOp(cx, pn->getOp(), atomIndex, bce))
> +            return false;

This case seems to contradict the name of the function; again, this happens *only* because we don't have proper top-level lexical bindings, right?

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539076 [details] [diff] [review]
Part 2: Rename EmitObject() EmitPropertyList() in preparation for classes.

Review of attachment 8539076 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ -6473,5 @@
>  
> -/*
> - * Using MOZ_NEVER_INLINE in here is a workaround for llvm.org/pr14047. See
> - * the comment on EmitSwitch.
> - */

Either duplicate the comment or don't duplicate the MOZ_NEVER_INLINE. (I'd prefer the latter.)

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539078 [details] [diff] [review]
Part 3: Emit bytecode for simple ES6 ClassDefinitions

Review of attachment 8539078 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks great, very nice and clean; but this can't land without dozens and dozens of tests. Clearing review for now.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6823,5 @@
>      return true;
>  }
>  
> +
> +

Style nit: just one blank line here.

@@ +6827,5 @@
> +
> +MOZ_NEVER_INLINE static bool
> +EmitClass(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn)
> +{
> +    ParseNode *names = pn->pn_kid1;

Oh. Please do this:

    class ClassNode : public TernaryNode {
      public:
        ClassNode(ParseNode *names, ParseNode *heritage, ParseNode *scope)
          : TernaryNode(PNK_CLASS, names, heritage, scope)
        {
            MOZ_ASSERT_IF(names, names->is<ClassNames>());
            ...
        }

        ClassNamesNode *names() const { return pn_kid1 ? pn_kid1->as<ClassNames>() : nullptr; }
        ParseNode *heritage() const { return pn_kid2; }
        LexicalScopeNode *scope() const { return pn_kid2->as<LexicalScopeNode>(); }

        static bool test(const ParseNode &node) { return node.isKind(PNK_CLASS); }
    };

Then change the type of EmitClass's third argument to ClassNode *, and change the type of `names` to ClassNames *.

@@ +6831,5 @@
> +    ParseNode *names = pn->pn_kid1;
> +    // For now, no unnamed classes (ClassExpressions)
> +    MOZ_ASSERT(names);
> +    // For now, no heritage expressions
> +    MOZ_ASSERT(!pn->pn_kid2);

As always, blank line before a comment. Or put the comment in the assertion:

    MOZ_ASSERT(names, "unnamed classes (ClassExpressions) not implemented yet");
    MOZ_ASSERT(!pn->pn_kid2, "ClassHeritage expressions (extends clauses) not implemented yet");

@@ +6833,5 @@
> +    MOZ_ASSERT(names);
> +    // For now, no heritage expressions
> +    MOZ_ASSERT(!pn->pn_kid2);
> +    ParseNode *innerBlock = pn->pn_kid3;
> +    MOZ_ASSERT(innerBlock->isKind(PNK_LEXICALSCOPE));

Yay, can kill this assertion since ClassNode::scope() is doing it for you.

@@ +6840,5 @@
> +
> +    ParseNode *constructor = nullptr;
> +    for (ParseNode *mn = classMethods->pn_head; mn; mn = mn->pn_next) {
> +        ParseNode *methodName = mn->pn_left;
> +        if (methodName->pn_atom == cx->names().constructor) {

Before accessing pn_atom, assert that it's safe? (Or use/invent some method for doing this that asserts.)

@@ +6846,5 @@
> +            break;
> +        }
> +    }
> +    // For now, no default constructors
> +    MOZ_ASSERT(constructor);

Blank line before a comment.

@@ +6865,5 @@
> +        return false;
> +    if (!EmitAtomOp(cx, cx->names().prototype, JSOP_INITPROP, bce))
> +        return false;
> +    if (!EmitAtomOp(cx, cx->names().constructor, JSOP_INITPROP, bce))
> +        return false;

While you're in here, please rewrite the entire JS engine to use C++ exceptions for errors, catching them at the API boundaries.

@@ +6877,5 @@
> +
> +    // That DEFCONST is never gonna be used, but use it here for logical consistency.
> +    ParseNode *innerName = names->as<ClassNames>().innerBinding();
> +    if (!EmitLexicalInitialization(cx, bce, innerName, JSOP_DEFCONST))
> +        return false;

You can get rid of local variables innerName and outerName, since they'll be convenient to inline now:

    if (!EmitBlah(cx, bce, names->innerBinding(), JSOP_DEFCONST))

@@ +6889,5 @@
> +
> +    if (Emit1(cx, bce, JSOP_POP) < 0)
> +        return false;
> +
> +    JS_ALWAYS_TRUE(bce->sc->setLocalStrictMode(savedStrictness));

MOZ_ALWAYS_TRUE

::: js/src/vm/Interpreter.cpp
@@ +3159,5 @@
>  
>      /* Load the object being initialized into lval/obj. */
>      RootedNativeObject &obj = rootNativeObject0;
>      obj = &REGS.sp[-2].toObject().as<NativeObject>();
> +    MOZ_ASSERT(obj->is<JSObject>() || obj->is<JSFunction>());

Huh. I think this should be is<PlainObject>() || is<JSFunction>(). I'm surprised it didn't get changed when PlainObject was introduced. Could you look into it?

Comment 12

[Eric Faust [:efaust]]

Attachment: Tests, v1

Comment 13

[Eric Faust [:efaust]]

Attachment: Part 2.5: Implement initfrozenprop

This should also include a diff to part three making the "prototype" initprop in EmitClass into initfrozenprop.

Make sure that .prototype is installed non-writable non-configurable.

Comment 14

[Eric Faust [:efaust]]

Comment on attachment 8549304 [details] [diff] [review]
Tests, v1

If this goes r+, then we should reexamine part 3

Comment 15

[Eric Faust [:efaust]]

Attachment: Part 2.75: Implement inithiddenprop

Comment 16

[Eric Faust [:efaust]]

Attachment: Part 3: Emit bytecode for simple ES6 ClassDefinitions

Comment 17

[Eric Faust [:efaust]]

Attachment: Tests, v2

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 8554843 [details] [diff] [review]
Part 2.5: Implement initfrozenprop

Review of attachment 8554843 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Interpreter.cpp
@@ +3179,1 @@
>      /* Load the property's initial value into rval. */

Style nit: Blank line before a comment, please.

::: js/src/vm/Opcodes.h
@@ +1415,5 @@
> +     *   Type: Object
> +     *   Operands: uint32_t nameIndex
> +     *   Stack: obj, val => obj
> +     */ \
> +    macro(JSOP_INITLOCKEDPROP, 146, "initlockedprop", NULL, 5,  2,  1, JOF_ATOM|JOF_PROP|JOF_SET|JOF_DETECTING) \

JOF_DETECTING. Sigh. (no change required)

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558228 [details] [diff] [review]
Part 2.75: Implement inithiddenprop

Review of attachment 8558228 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/CodeGenerator.cpp
@@ +4419,5 @@
>  
>      pushArg(ToValue(lir, LInitProp::ValueIndex));
>      pushArg(ImmGCPtr(lir->mir()->propertyName()));
>      pushArg(objReg);
> +    pushArg(ImmPtr(lir->mir()->resumePoint()->pc()));

Consider passing the desired attrs as argument rather than the pc.

::: js/src/jit/VMFunctions.cpp
@@ +215,3 @@
>  {
>      RootedId id(cx, NameToId(name));
> +    unsigned propFlags = GetInitDataPropFlags(JSOp(*pc));

Oh, I should have mentioned in the previous patch: please call this attrs for consistency, not flags.

::: js/src/vm/Opcodes.h
@@ +1424,5 @@
> +     * 'nameIndex' property of 'obj' as 'val', pushes 'obj' onto the stack.
> +     *   Category: Literals
> +     *   Type: Object
> +     *   Operands: uint32_t nameIndex
> +     *   Stack: obj, val => obj

Nit: Instead of copying and pasting comments, just say

    /*
     * Exactly like JSOP_INITPROP, except define the property as non-enumerable.
     * Used in the code for class declarations.
     */

and likewise for JSOP_INITLOCKEDPROP.

That will probably cause make_opcode_doc.py to choke. Fix it to cope, please.

Comment 20

[Jason Orendorff [:jorendorff]]

Please add a parser test, if we don't have one, that

    for (class X {}; ;) {}

is a SyntaxError.

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558230 [details] [diff] [review]
Tests, v2

Review of attachment 8558230 [details] [diff] [review]:
-----------------------------------------------------------------

See also comment 20.

I wouldn't ask for so many tests about the bindings if our frontend binding code wasn't so... let's say subtle.

::: js/src/tests/ecma_6/Class/classPrototype.js
@@ +4,5 @@
> +class a { constructor() { } }
> +var protoDesc = Object.getOwnPropertyDescriptor(a, "prototype");
> +assertEq(protoDesc.writable, false);
> +assertEq(protoDesc.configurable, false);
> +assertEq(protoDesc.enumerable, false);

Also assert that a.prototype is an object, that it has no properties except 'constructor', it's extensible, and its prototype is Object.prototype. Check the attributes for a.prototype.constructor.

Also check a.[[Prototype]].

(However, disregard if later `extends` tests already check this stuff. ISTR some of these are covered.)

::: js/src/tests/ecma_6/Class/innerBinding.js
@@ +1,2 @@
> +// Class statements should create an immutable inner binding. Since all code in
> +// classes is in strict mode, attempts to mutate it should throw.

Please also test that:

*   TDZ applies to the inner binding (computed property names that refer to it throw at runtime)
*   classes can nest and both inner bindings are still visible
*   the inner binding can be shadowed by a method argument etc.
*   the inner binding is (or isn't) visible from computed property name expressions (I don't actually know what the spec says about that)
*   the inner binding is distinct from the outer one:

{
    let orig_X;

    class X {
        f() { assertEq(X, orig_X); }
    }

    orig_X = X;
    X = 13;
    assertEq(X, 13);
    new orig_X().f();
}

::: js/src/tests/ecma_6/Class/methodInstallation.js
@@ +9,5 @@
> +class a {
> +    constructor() { constructorCalled = true; }
> +    method() { methodCalled = true }
> +    get getter() { getterCalled = true; }
> +    set setter(x) { setterCalled = true; }

Another thing to add here that would test three things at once is:
    *[Symbol.iterator]() { yield "cow"; yield "pig"; }
and with that you might as well throw in a higher-level test, since it's a one-liner:
    assertEq([...new a].join(), "cow,pig");

::: js/src/tests/ecma_6/Class/methodOverwrites.js
@@ +2,5 @@
> +
> +var test = `
> +var result = 0;
> +// Regardless of order, the constructor is overridden by any CPN, because it's
> +// processed seperately.

This comment confused me, but the test is exactly right, so ... ok!

@@ +21,5 @@
> +assertEq(result, 8);
> +
> +class d { constructor() { } get method() { result += 1 } method() { result += 2 } }
> +new d().method();
> +assertEq(result, 10);

Test that
    class x { get method() {} set method(v) {} }
creates a single accessor property with getter and setter.

Overkill, maybe, but also check that
    class y { get method() {} method() {} set method(v) {} }
ends up having only the setter.

::: js/src/tests/ecma_6/Class/outerBinding.js
@@ +1,1 @@
> +// A class creates a mutable lexical outer binding.

Also test these cases (note that in many of them I don't know exactly what the spec says should happen):

*    TDZ applies to the outer binding
*    evaluating "class Foo{}" when a global const Foo has already been declared throws the appropriate error
*    and that error is thrown early, so in `const Foo=0; eval("print('FAIL'); class Foo{}");` nothing is printed
*    the outer binding can't be deleted, even from non-strict code
*    a class in a loop creates a binding per iteration
*    `if (1) class X {}` is a SyntaxError; no X is declared
*    after eval("class X{} throw 'FAIL'; class Y{}"), X is initialized and Y is not
*    strict direct eval("class X{}") works; the outer binding for X is on the strict-direct-eval scope and shadows the enclosing scope

Comment 22

[Jason Orendorff [:jorendorff]]

Please test that __proto__ as a method name is just like any other identifier.

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 8558229 [details] [diff] [review]
Part 3: Emit bytecode for simple ES6 ClassDefinitions

Review of attachment 8558229 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6925,5 @@
>  
> +
> +
> +MOZ_NEVER_INLINE static bool
> +EmitClass(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn)

Please at least add a comment pointing to 14.5.14 (which this follows quite closely). Step number comments would fit in, if you're so inclined.

@@ +6931,5 @@
> +    ParseNode *names = pn->pn_kid1;
> +    // For now, no unnamed classes (ClassExpressions)
> +    MOZ_ASSERT(names);
> +    // For now, no heritage expressions
> +    MOZ_ASSERT(!pn->pn_kid2);

Blank line before a comment --- but these in particular belong in a second argument to MOZ_ASSERT:

    MOZ_ASSERT(names, "for now, no unnamed classes (ClassExpressions)");
    MOZ_ASSERT(!pn->pn_kid2, "for now, no heritage expressions");

@@ +6946,5 @@
> +            break;
> +        }
> +    }
> +    // For now, no default constructors
> +    MOZ_ASSERT(constructor);

Same thing here.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/699bcd3cc384
https://hg.mozilla.org/mozilla-central/rev/3abec11e3ea6
https://hg.mozilla.org/mozilla-central/rev/b38a935d043a
https://hg.mozilla.org/mozilla-central/rev/1702843b0e17
https://hg.mozilla.org/mozilla-central/rev/49cea7ec085b
https://hg.mozilla.org/mozilla-central/rev/513d6a986d14
https://hg.mozilla.org/mozilla-central/rev/ffceda31c4e1
https://hg.mozilla.org/mozilla-central/rev/6c010b941832