Support 'extends' in ES6 classes

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: efaust)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla39
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(7 attachments)

(Reporter)

Description

4 years ago
Combined with @@create support, this should be sufficient to test subclassing of builtin classes like Array and Map.
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(Assignee)

Comment 1

3 years ago
Created attachment 8539082 [details] [diff] [review]
Parser support for "extends"
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8539082 - Flags: review?(jorendorff)
(Assignee)

Comment 2

3 years ago
Created attachment 8539083 [details] [diff] [review]
Interpreter support for JSOP_CLASSHERITAGE
Attachment #8539083 - Flags: review?(jorendorff)
(Assignee)

Comment 3

3 years ago
Created attachment 8539084 [details] [diff] [review]
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%%
Attachment #8539084 - Flags: review?(jorendorff)
(Assignee)

Comment 4

3 years ago
Created attachment 8539085 [details] [diff] [review]
Interpreter support for JSOP_FUNWITHPROTO
Attachment #8539085 - Flags: review?(jorendorff)
(Assignee)

Comment 5

3 years ago
Created attachment 8539086 [details] [diff] [review]
Interpreter support for JSOP_OBJWITHPROTO
Attachment #8539086 - Flags: review?(jorendorff)
(Assignee)

Comment 6

3 years ago
Created attachment 8539087 [details] [diff] [review]
Emitter support for "extends"
Attachment #8539087 - Flags: review?(jorendorff)
(Reporter)

Comment 7

3 years ago
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.
Attachment #8539082 - Flags: review?(jorendorff)
(Reporter)

Updated

3 years ago
Attachment #8539083 - Flags: review?(jorendorff)
(Reporter)

Updated

3 years ago
Attachment #8539084 - Flags: review?(jorendorff)
(Reporter)

Updated

3 years ago
Attachment #8539085 - Flags: review?(jorendorff)
(Reporter)

Updated

3 years ago
Attachment #8539086 - Flags: review?(jorendorff)
(Reporter)

Updated

3 years ago
Attachment #8539087 - Flags: review?(jorendorff)
(Assignee)

Comment 8

3 years ago
Created attachment 8555514 [details] [diff] [review]
Test support for "extends"

If you like this, you'll love the rest ;)
Attachment #8555514 - Flags: review?(jorendorff)
Blocks: 1130744
(Reporter)

Comment 9

3 years ago
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.
Attachment #8555514 - Flags: review?(jorendorff) → review+
(Reporter)

Updated

3 years ago
Attachment #8539082 - Flags: review?(jorendorff)
(Reporter)

Updated

3 years ago
Attachment #8539083 - Flags: review?(jorendorff)
(Reporter)

Updated

3 years ago
Attachment #8539084 - Flags: review?(jorendorff)
(Reporter)

Updated

3 years ago
Attachment #8539085 - Flags: review?(jorendorff)
(Reporter)

Updated

3 years ago
Attachment #8539086 - Flags: review?(jorendorff)
(Reporter)

Comment 10

3 years ago
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.
(Reporter)

Comment 11

3 years ago
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);
(Reporter)

Updated

3 years ago
Attachment #8539087 - Flags: review?(jorendorff)
(Assignee)

Comment 12

3 years ago
(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.
(Reporter)

Updated

3 years ago
Attachment #8539082 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 13

3 years ago
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.
Attachment #8539083 - Flags: review?(jorendorff) → review+
(Reporter)

Updated

3 years ago
Attachment #8539084 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 14

3 years ago
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.
Attachment #8539085 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 15

3 years ago
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.
Attachment #8539086 - Flags: review?(jorendorff) → review+
Also don't forget to increment the subtrahend ;)
(Reporter)

Comment 17

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

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

woot
Attachment #8539087 - Flags: review?(jorendorff) → review+
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
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/extends
(marked as Nightly only)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.