1066234 - Support 'extends' in ES6 classes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Combined with @@create support, this should be sufficient to test subclassing of builtin classes like Array and Map.

Comment 1

[Eric Faust [:efaust]]

Attachment: Parser support for "extends"

Comment 2

[Eric Faust [:efaust]]

Attachment: Interpreter support for JSOP_CLASSHERITAGE

Comment 3

[Eric Faust [:efaust]]

Attachment: Refactor js::CloneFunctionObject to take a proto parameter

Conveniently, because of the used spec here, we can still use nullptr as a sentinel value for %%FunctionPrototype%%

Comment 4

[Eric Faust [:efaust]]

Attachment: Interpreter support for JSOP_FUNWITHPROTO

Comment 5

[Eric Faust [:efaust]]

Attachment: Interpreter support for JSOP_OBJWITHPROTO

Comment 6

[Eric Faust [:efaust]]

Attachment: Emitter support for "extends"

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539082 [details] [diff] [review]
Parser support for "extends"

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

Clearing r? on a bunch of patches that need tests.

Comment 8

[Eric Faust [:efaust]]

Attachment: Test support for "extends"

If you like this, you'll love the rest ;)

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8555514 [details] [diff] [review]
Test support for "extends"

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

::: js/src/tests/ecma_6/Class/classHeritage.js
@@ +45,5 @@
> +assertEq(overrideCalled, "derived");
> +
> +// What about the statics?
> +derived.staticMethod();
> +assertEq(staticMethodCalled, true);

Check that the 'this' argument passed to the static method in this situation is correct?

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +1277,5 @@
>      assertError("class Foo { constructor()", SyntaxError);
>      assertError("class Foo { constructor()", SyntaxError);
>      assertError("class Foo { constructor() {", SyntaxError);
>      assertError("class Foo { constructor() { }", SyntaxError);
> +    assertError("class Foo extends", SyntaxError);

Also "class Foo extends Bar" please.

Comment 10

[Jason Orendorff [:jorendorff]]

Please also test this situation.

    class Foo {}
    {
        class Foo extends Foo {}  // inner extends outer, or ReferenceError?
    }

I have not attempted to figure out what is supposed to happen.

Comment 11

[Jason Orendorff [:jorendorff]]

Please throw this in there too please.

    // Non-writable, non-configurable base-class methods can be overridden.
    class Base {
        x() { return 2; }
    }
    Object.freeze(Base.prototype);

    class Derived extends Base {
        x() { return 3; }
    }
    assertEq(new Derived().x(), 3);

Comment 12

[Eric Faust [:efaust]]

(In reply to Jason Orendorff [:jorendorff] from comment #11)
> Please throw this in there too please.
> 
>     // Non-writable, non-configurable base-class methods can be overridden.
>     class Base {
>         x() { return 2; }
>     }
>     Object.freeze(Base.prototype);
> 
>     class Derived extends Base {
>         x() { return 3; }
>     }
>     assertEq(new Derived().x(), 3);

This is good. It should work, but we should test it.(In reply to Jason Orendorff 

> Please also test this situation.
> 
>     class Foo {}
>     {
>         class Foo extends Foo {}  // inner extends outer, or ReferenceError?
>     }
> 
> I have not attempted to figure out what is supposed to happen.

Inner should extend outer, as by the time we are able to put the class definition (inside a method), we are in a shadowing block scope.

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539083 [details] [diff] [review]
Interpreter support for JSOP_CLASSHERITAGE

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

::: js/src/vm/Interpreter.cpp
@@ +3465,5 @@
> +    RootedObject &funcProto = rootObject2;
> +    if (val.isNull()) {
> +        objProto = nullptr;
> +        if (!GetBuiltinPrototype(cx, JSProto_Function, &funcProto))
> +            goto error;

Micro-nit: This would be more direct:

    funcProto = cx->global()->getOrCreateFunctionPrototype(cx);
    if (!funcProto)
        goto error;

Either way is correct, your call.

@@ +3479,5 @@
> +        if (!JSObject::getProperty(cx, funcProto, funcProto, cx->names().prototype, &protoProp))
> +            goto error;
> +
> +        if (!protoProp.isObjectOrNull()) {
> +            js_ReportValueError(cx, JSMSG_NOT_OBJORNULL, JSDVG_IGNORE_STACK, protoProp, NullPtr());

This is one of those rare cases where the expression decompiler should be used!

Pass -1 instead of JSDVG_IGNORE_STACK, and change the error message like this:

>-MSG_DEF(JSMSG_NOT_OBJORNULL,           1, JSEXN_TYPEERR, "{0} is not an object or null")
>+MSG_DEF(JSMSG_PROTO_NOT_OBJORNULL,     1, JSEXN_TYPEERR, "{0}.prototype is not an object or null")

@@ +3482,5 @@
> +        if (!protoProp.isObjectOrNull()) {
> +            js_ReportValueError(cx, JSMSG_NOT_OBJORNULL, JSDVG_IGNORE_STACK, protoProp, NullPtr());
> +            goto error;
> +        }
> +        objProto = protoProp.toObjectOrNull();

Seems a little awkward to call toObjectOrNull here and then immediately pass the result to setObjectOrNull. Make objProto a RootedValue & and you can get rid of protoProp.

::: js/src/vm/Opcodes.h
@@ +479,5 @@
>       *   Operands:
>       *   Stack: callee, this, args => rval
>       */ \
>      macro(JSOP_STRICTSPREADEVAL,      50, "strict-spreadeval", NULL,         1,  3,  1, JOF_BYTE|JOF_INVOKE|JOF_TYPESET|JOF_CHECKSTRICT) \
> +    macro(JSOP_CLASSHERITAGE,  51, "classheritage",   NULL,         1,  1,  2,  JOF_BYTE) \

Needs a comment like every other opcode.

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539085 [details] [diff] [review]
Interpreter support for JSOP_FUNWITHPROTO

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

::: js/src/vm/Opcodes.h
@@ +480,5 @@
>       *   Stack: callee, this, args => rval
>       */ \
>      macro(JSOP_STRICTSPREADEVAL,      50, "strict-spreadeval", NULL,         1,  3,  1, JOF_BYTE|JOF_INVOKE|JOF_TYPESET|JOF_CHECKSTRICT) \
>      macro(JSOP_CLASSHERITAGE,  51, "classheritage",   NULL,         1,  1,  2,  JOF_BYTE) \
> +    macro(JSOP_FUNWITHPROTO,  52, "funcwithproto",   NULL,         5, 1,  1,  JOF_OBJECT) \

Needs a comment.

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539086 [details] [diff] [review]
Interpreter support for JSOP_OBJWITHPROTO

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

::: js/src/vm/Interpreter.cpp
@@ +3511,5 @@
> +{
> +    RootedObject &proto = rootObject0;
> +    proto = REGS.sp[-1].toObjectOrNull();
> +
> +    JSObject *obj = NewObjectWithGivenProto(cx, &JSObject::class_, proto, cx->global());

It's PlainObject::class_ now (just in case you end up wondering where this went).

Do we have tests for cases like

    function F() {}
    F.prototype = null;
    class G extends F {...}

And the error case where F.prototype is undefined or a string or something?

Add 'em if we don't. No need for a new review.

::: js/src/vm/Opcodes.h
@@ +753,5 @@
>       *   nuses: (argc+2)
>       */ \
>      macro(JSOP_NEW,       82, js_new_str,   NULL,         3, -1,  1,  JOF_UINT16|JOF_INVOKE|JOF_TYPESET) \
>      \
> +    macro(JSOP_OBJWITHPROTO,  83, "objwithproto",   NULL,         1,  1,  1,  JOF_BYTE) \

Needs a comment.

Comment 16

[Jan de Mooij [:jandem]]

Also don't forget to increment the subtrahend ;)

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 8539087 [details] [diff] [review]
Emitter support for "extends"

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

woot

Comment 18

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73f4dd8f8d97
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/478d797266b1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b44a3c3c6f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a40045b47bba
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/99933582c7d8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e06ad6b69b25
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ce28663e61d4

Comment 19

[Phil Ringnalda (:philor)]

Dunno yet whether anything else plans on burning, but SM(e) says "/js/src/frontend/Parser.cpp:5869:16: error: converting 'false' to pointer type 'js::frontend::ParseNode*' [-Werror=conversion-null]"

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b43067fa81e7

Comment 20

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5bfaed9ef2d0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b049159001f8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bbff49b06fad
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bf7a3ed34a20
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73afc3f3890c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4f3e48c28f4b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/94a0b2bdf4de

relanded with return false changed to return null();

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5bfaed9ef2d0
https://hg.mozilla.org/mozilla-central/rev/b049159001f8
https://hg.mozilla.org/mozilla-central/rev/bbff49b06fad
https://hg.mozilla.org/mozilla-central/rev/bf7a3ed34a20
https://hg.mozilla.org/mozilla-central/rev/73afc3f3890c
https://hg.mozilla.org/mozilla-central/rev/4f3e48c28f4b
https://hg.mozilla.org/mozilla-central/rev/94a0b2bdf4de

Comment 22

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/extends
(marked as Nightly only)