1141863 - Implement ES6 SuperCall

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

super() is valid only in class constructors. It initializes the |this| binding for that function to the return value of the superconstructor.

Also necessary, but in a sub-bug, is the passing of new.target.

Comment 1

[Eric Faust [:efaust]]

super() is a mandatory feature for class adoption, but the new lexically scoped this imposes design challenges that will take longer to solve than we want to wait for the feature. To that end, we will disable arrow functions and eval (as |this| becomes hard to provide) inside derived class constructors, and implement the required semantics to get derived class constructors working for the "easier" cases, and then go back and flush out a more complete solution, probably based on converting |this| to act more like a lexical binding that's just always present.

Comment 2

[Patrick Cloke [:clokep]]

Will this allow subclassing of built-in objects (e.g. Array or Map)?

Comment 3

[Eric Faust [:efaust]]

No, even after super() works in classes, we still have to tweak the builtin construction algorithms to handle @@species, which will then allow proper subclassing of those objects to work, as far as I understand.

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Patrick Cloke [:clokep] from comment #2)
> Will this allow subclassing of built-in objects (e.g. Array or Map)?

Necessary, but not sufficient.

In any event, subclassing builtins is stupid.  Composition beats inheritance for these things any day.  Don't inherit!</rant>

Comment 5

[aleth [:aleth]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> (In reply to Patrick Cloke [:clokep] from comment #2)
> > Will this allow subclassing of built-in objects (e.g. Array or Map)?
> 
> Necessary, but not sufficient.
> 
> In any event, subclassing builtins is stupid.  Composition beats inheritance
> for these things any day.  Don't inherit!</rant>

I'm intrigued by this <rant/>. If you have the time, could you take a look at the subclassing WIP in bug 142337 - do you see an obvious way to do the same thing more elegantly using composition?

Comment 6

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #5)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> > (In reply to Patrick Cloke [:clokep] from comment #2)
> > > Will this allow subclassing of built-in objects (e.g. Array or Map)?
> > 
> > Necessary, but not sufficient.
> > 
> > In any event, subclassing builtins is stupid.  Composition beats inheritance
> > for these things any day.  Don't inherit!</rant>
> 
> I'm intrigued by this <rant/>. If you have the time, could you take a look
> at the subclassing WIP in bug 142337 - do you see an obvious way to do the
> same thing more elegantly using composition?

aleth means bug 1142337. My second patch on there is broken cause I was stupid when creating it, but the concept is reasonable.

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to aleth [:aleth] from comment #5)
> > In any event, subclassing builtins is stupid.  Composition beats inheritance
> > for these things any day.  Don't inherit!</rant>
> 
> I'm intrigued by this <rant/>. If you have the time, could you take a look
> at the subclassing WIP in bug 142337 - do you see an obvious way to do the
> same thing more elegantly using composition?

OK. So, the way to do this with subclassing is:

    class NormalizedMap extends Map {
      constructor(normalize, iterable = []) {
        super();
        if (typeof(normalize) != "function")
          throw "NormalizedMap must have a normalize function!";
        this._normalize = normalize;
        for (let [key, val] of iterable)
          this.set(key, val);
      }

      has(key) { return super.has(this._normalize(key)); }
      get(key) { return super.get(this._normalize(key)); }

      set(key, val) {
        super.set(this._normalize(key), val);
        return this;
      }
    }

Composition:

    class NormalizedMap {
      constructor(normalize, iterable = []) {
        if (typeof(normalize) != "function")
          throw "NormalizedMap must have a normalize function!";
        this._normalize = normalize;
        this._map = new Map;
        for (let [key, val] of iterable)
          this.set(key, val);
      }

      has(key) { return this._map.has(this._normalize(key)); }
      get(key) { return this._map.get(this._normalize(key)); }

      set(key, val) {
        this._map.set(this._normalize(key), val);
        return this;
      }

      // The remaining methods are unaffected. Delegate.
      get size() { return this._map.size(); }
      [Symbol.iterator]() { return this._map[Symbol.iterator](); }
      entries() { return this._map.entries(); }
      keys() { return this._map.keys(); }
      values() { return this._map.values(); }
      clear() { this._map.clear(); }
    }

Composition seems less sloppy to me:

* Any methods you don't implement (perhaps because they get added in later standards) simply won't exist on NormalizedMap, instead of existing and not normalizing keys.

* The encapsulation is stronger, though maybe with subclassing it's already strong enough that you shouldn't care. `Map.prototype.set.call(nm, badkey, val)` will not put a non-normalized key into a NormalizedMap.

* It is easy to adapt NormalizedMap to delegate to a Map-like object that isn't just a Map. In other words, it's stackable with other Map hacks. If you have both NormalizedMap and DefaultMap, and you want to create NormalizedDefaultMap, that's pretty easy with composition. With subclassing, you're stuck with the base class.

Comment 8

[aleth [:aleth]]

(In reply to Jason Orendorff [:jorendorff] from comment #7)
> Composition seems less sloppy to me:
> 
> * Any methods you don't implement (perhaps because they get added in later
> standards) simply won't exist on NormalizedMap, instead of existing and not
> normalizing keys.
> 
> * The encapsulation is stronger, though maybe with subclassing it's already
> strong enough that you shouldn't care. `Map.prototype.set.call(nm, badkey,
> val)` will not put a non-normalized key into a NormalizedMap.
> 
> * It is easy to adapt NormalizedMap to delegate to a Map-like object that
> isn't just a Map. In other words, it's stackable with other Map hacks. If
> you have both NormalizedMap and DefaultMap, and you want to create
> NormalizedDefaultMap, that's pretty easy with composition. With subclassing,
> you're stuck with the base class.

Many thanks for this detailed response! I see where you're coming from now. It seems to me the choice mainly comes down to whether one prefers a "whitelisting or blacklisting" approach to inherited methods etc.

Comment 9

[Jeff Walden [:Waldo]]

(In reply to aleth [:aleth] from comment #8)
> It seems to me the choice mainly comes down to whether one prefers a
> "whitelisting or blacklisting" approach to inherited methods etc.

Somewhat that, but also that because inheritance is potentially "easier" -- quicker, more seductive -- it encourages people to inherit when really they should compose.

Inheritance doesn't make sense when your class isn't *really* an instance of the base class.  But people often won't think it through at first, and they'll go with inheritance because it has easy syntax.  Then when they realize later they shouldn't have inherited, they're stuck, because not inheriting would be (in Java, at least) an API change.  The canonical example of this problem is Java's Stack class:

http://docs.oracle.com/javase/7/docs/api/java/util/Stack.html

Someone wasn't thinking and made it *inherit* from Vector -- but that's not what you want from a stack!  Random access into a stack is just fundamentally wrong: you should only be able to access the top of the stack.  Hence that page's suggestion *not* to use Stack but instead to use Deque, ArrayDeque, and so on.

Comment 10

[Eric Faust [:efaust]]

Attachment: Part 1: Clean up |this| object creation to account for new.target

Comment 11

[Eric Faust [:efaust]]

Attachment: Part 2: Parse and emit super(), and support it in the interpreter

Comment 12

[Eric Faust [:efaust]]

Attachment: Part 3: Fix up supercall to work properly in the jits

Comment 13

[Jan de Mooij [:jandem]]

While reviewing the first part I was wondering: is the new.target JSObject passed around always a JSFunction? If yes I think it'd be great if we could make that more explicit by using JSFunction.

Asking this before I continue with the review, because the answer affects that a lot.

Comment 14

[Jeff Walden [:Waldo]]

No, because proxies that are constructors, cross-compartment wrappers and cross-compartment construction, etc.

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8658310 [details] [diff] [review]
Part 1: Clean up |this| object creation to account for new.target

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

::: js/src/jit/VMFunctions.cpp
@@ +570,5 @@
>                  rval.set(MagicValue(JS_UNINITIALIZED_LEXICAL));
>              } else {
> +                // XXX: This should take a new.target value, but we can't
> +                //      compile super(), so we can get by with the callee
> +                //      for now.

It *does* take a new.target value now?

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8658314 [details] [diff] [review]
Part 3: Fix up supercall to work properly in the jits

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

Looks good, I expected this patch to be more complicated.

::: js/src/vm/Opcodes.h
@@ +1658,5 @@
>      \
>      macro(JSOP_SETTHIS,       161,"setthis",    NULL,     1,  1,  1,  JOF_BYTE) \
>      macro(JSOP_SUPERFUN,      162,"superfun",   NULL,     1,  0,  1,  JOF_BYTE) \
> +    macro(JSOP_SUPERCALL,     163, "supercall", NULL,     3, -1,  1,  JOF_UINT16|JOF_INVOKE|JOF_TYPESET) \
> +    macro(JSOP_SPREADSUPERCALL, 164, "spreadsupercall",  NULL, 1,  4,  1, JOF_BYTE|JOF_INVOKE|JOF_TYPESET) \

These should all have a comment block like the other ones and we should update vm/Xdr.h

Comment 17

[Jan de Mooij [:jandem]]

File a followup bug to JIT compile JSOP_SETTHIS and JSOP_SUPERFUN?

These ops probably don't have to be super fast (no pun intended) right now, so VM calls are fine, even in that case Baseline beats the interpreter hands down.

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 8658310 [details] [diff] [review]
Part 1: Clean up |this| object creation to account for new.target

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

r=me on non-jit parts.

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 8658313 [details] [diff] [review]
Part 2: Parse and emit super(), and support it in the interpreter

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

Looks great, only extremely minor comments... except I don't see any tests except Reflect.parse() tests and some pre-existing tests changing to use plain `super()`.

We need to test that:
- it works correctly (base class is [[Construct]]ed; new.target is passed through correctly; arguments are passed; base class actually uses the right prototype object) when the base class is:
  - an ES6 non-derived class
  - another ES6 derived class
  - Object
  - a plain old ES5 function
  - a proxy
- spread-call works
- the right base-class constructor is called even if you've messed with the proto chain
  - order of operations: caller.[[Prototype]] is fetched before arguments are evaluated (detect by doing Object.setPrototypeOf(MyClass, blah) in the argument list)
- it throws in GetSuperConstructor() step 6 if proto isn't a constructor
  - before arguments are evaluated
- it works correctly when used in a class that `extends Object`
- it throws correctly (right?) when used in a class that `extends null`
- it initializes `this`
- the exception is correctly thrown if you try to super() twice
  - even if there is physically only one super() in the code, in a loop
  - that it's ok to trigger this exception and then return normally
- if the constructor returns an object, or throws, without calling super(), `this` remains uninitialized (actually I guess this is unobservable without arrow functions, which we don't support yet? never mind then)
- that we correctly ban `super()` outside of derived-class constructors:
  - at toplevel
  - in Function-constructor code: `Function("", "super()")` throws a SyntaxError.
  - in legacy generator-comprehensions: `(super() for (x in y))`

r=me with tests. I wouldn't mind reviewing them here. But if you're in a hurry just ping me on IRC and I can look at them while you are landing it. (In case I'm not being clear: of course the tests have to be done before you land.)

::: js/src/frontend/Parser.cpp
@@ +8350,5 @@
> +                    return null();
> +
> +                // Despite the fact that it's impossible to have |super()| is a
> +                // generator, we still inherity the yieldHandling of the
> +                // memberExpression, per spec. Curious.

I don't think this comment is really necessary, and it gives an air of mystery that is only going to distract future readers.

The spec says to inherit yieldHandling from the syntactic context because that's what the spec *always* does; specifying the argument rather than inheriting would give the impression that the yieldHandling is being *changed*, a false impression here.

Since we're in a derived class constructor, and `yield` is always a keyword in strict-mode code, you could even assert that yieldHandling == YieldIsKeyword. But I'm not sure any of this is remarkable enough to warrant a comment at all...

::: js/src/js.msg
@@ +211,5 @@
>  MSG_DEF(JSMSG_BAD_STRICT_ASSIGN,       1, JSEXN_SYNTAXERR, "can't assign to {0} in strict mode")
>  MSG_DEF(JSMSG_BAD_SWITCH,              0, JSEXN_SYNTAXERR, "invalid switch statement")
>  MSG_DEF(JSMSG_BAD_SUPER,               0, JSEXN_SYNTAXERR, "invalid use of keyword 'super'")
>  MSG_DEF(JSMSG_BAD_SUPERPROP,           1, JSEXN_SYNTAXERR, "use of super {0} accesses only valid within methods or eval code within methods")
> +MSG_DEF(JSMSG_BAD_SUPERCALL,           0, JSEXN_SYNTAXERR, "use of super call only valid within derived class constructors")

suggest: "super() is only valid in derived class constructors"

@@ +505,5 @@
>  MSG_DEF(JSMSG_NO_INDEXED_SETTER,         2, JSEXN_TYPEERR, "{0} doesn't have an indexed property setter for '{1}'")
>  
>  // Super
>  MSG_DEF(JSMSG_CANT_DELETE_SUPER, 0, JSEXN_REFERENCEERR, "invalid delete involving 'super'")
> +MSG_DEF(JSMSG_REINIT_THIS,       0, JSEXN_REFERENCEERR, "invalid reinitialization of |this| in derived class constructor")

"super() called twice in constructor"

::: js/src/tests/ecma_6/Class/derivedConstructorDisabled.js
@@ +28,5 @@
>  var dbg = Debugger(g);
>  dbg.onDebuggerStatement = function(frame) { assertThrowsInstanceOf(()=>frame.eval(''), InternalError); };
>  
>  // Remove the assertion and add super() when super() is implemented!
> +g.eval("new class foo extends null { constructor() { debugger; return {}; } }()");

Please remove the leftover comment.

Also, the comment says to add super(), I guess instead of returning {}. But either one works, so, whatever...

::: js/src/vm/Interpreter.cpp
@@ +1744,5 @@
>             result.checkStrictErrorOrWarning(cx, obj, id, strict);
>  }
>  
> +static JSFunction&
> +GetSuperEnvFunction(JSContext *cx, InterpreterRegs& regs)

Please add a comment since it's a new function:

/*
 * Get the innermost enclosing function that has a 'this' binding.
 *
 * Implements ES6 12.3.5.2 GetSuperConstructor() steps 1-3, including
 * the loop in ES6 8.3.2 GetThisEnvironment(). Our implementation of
 * ES6 12.3.5.3 MakeSuperPropertyReference() also uses this code.
 */

::: js/src/vm/Stack.h
@@ +742,5 @@
> +    void setDerivedConstructorThis(HandleObject thisv) {
> +        MOZ_ASSERT(isNonEvalFunctionFrame());
> +        MOZ_ASSERT(script()->isDerivedClassConstructor());
> +        MOZ_ASSERT(callee().isClassConstructor());
> +        argv()[-1] = ObjectValue(*thisv);

Hey, these assertions look familiar - I just saw them in Interpreter.cpp, case JSOP_SETTHIS. :) They make even more sense here. I don't mind having both; but if you want to delete them from Interpreter.cpp as redundant, fine.

Might as well assert thisValue().isMagic(JS_UNINITIALIZED_LEXICAL) too.

Comment 20

[Eric Faust [:efaust]]

Attachment: Tests

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 8663079 [details] [diff] [review]
Tests

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

Go go go!

::: js/src/tests/ecma_6/Class/superCallBaseInvoked.js
@@ +13,5 @@
> +class base {
> +    // Base class must be [[Construct]]ed, as you cannot [[Call]] a class
> +    // constructor
> +    constructor(nt, one) {
> +        assertEq(new.target, nt);

Do something effectful in here, like `this.ok = true`, and later assert that the effect happened.

@@ +31,5 @@
> +             }
> +         });
> +function baseFunc(nt, one) {
> +    assertEq(new.target, nt);
> +    assertEq(one, 1);

and here

::: js/src/tests/ecma_6/Class/superCallSpreadCall.js
@@ +1,5 @@
> +var test = `
> +
> +class base {
> +    constructor(a, b, c) {
> +        assertEq(a, 1);

and here

Comment 22

[Tom S [:evilpie]]

Comment on attachment 8663079 [details] [diff] [review]
Tests

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

::: js/src/tests/ecma_6/Class/superCallSpreadCall.js
@@ +13,5 @@
> +        super(...arr);
> +    }
> +}
> +
> +dis(test);

^.^

I would at least add one test with rest parameters:

class test2 extends base {
   constructor(args) {
       super(...args);
   }
}

new test(1, 2, 3)

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd25dbf77e57
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6598d998f3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5a67fbf0805

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f83a07527af8

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38f4ce29926d

Comment 26

[Jeff Walden [:Waldo]]

Given the agony of things repeatedly being backed out, I investigated the one last jstests issues that popped up today and landed comment 25 -- feel free to skim the changes as a last double-check, because I'm not intimately familiar with all the nuances of classes, just been the subject of sufficient pontification to make those changes (after a double-check spec consult).

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fed29692284

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd25dbf77e57
https://hg.mozilla.org/mozilla-central/rev/f6598d998f3e
https://hg.mozilla.org/mozilla-central/rev/b5a67fbf0805
https://hg.mozilla.org/mozilla-central/rev/f83a07527af8
https://hg.mozilla.org/mozilla-central/rev/38f4ce29926d
https://hg.mozilla.org/mozilla-central/rev/4fed29692284

Comment 29

[Florian Scholz (Open Web Docs)]

The documentation for super is here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/super