Implement ES6 SuperCall

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

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

unspecified
mozilla44
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(4 attachments)

(Assignee)

Description

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

Updated

3 years ago
Depends on: 1141865
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(Assignee)

Updated

3 years ago
Depends on: 1169731
(Assignee)

Comment 1

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

Updated

3 years ago
Depends on: 1169734
(Assignee)

Updated

3 years ago
Depends on: 1169741
(Assignee)

Updated

3 years ago
Depends on: 1168992
Will this allow subclassing of built-in objects (e.g. Array or Map)?
(Assignee)

Comment 3

3 years ago
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

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

Updated

3 years ago
Blocks: 142337

Comment 5

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

Updated

3 years ago
Blocks: 1142337
No longer blocks: 142337
No longer depends on: 1168992
(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

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

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

Comment 10

3 years ago
Created attachment 8658310 [details] [diff] [review]
Part 1: Clean up |this| object creation to account for new.target
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8658310 - Flags: review?(jorendorff)
Attachment #8658310 - Flags: review?(jdemooij)
(Assignee)

Comment 11

3 years ago
Created attachment 8658313 [details] [diff] [review]
Part 2: Parse and emit super(), and support it in the interpreter
Attachment #8658313 - Flags: review?(jorendorff)
(Assignee)

Comment 12

3 years ago
Created attachment 8658314 [details] [diff] [review]
Part 3: Fix up supercall to work properly in the jits
Attachment #8658314 - Flags: review?(jdemooij)
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.
Flags: needinfo?(efaustbmo)
No, because proxies that are constructors, cross-compartment wrappers and cross-compartment construction, etc.
Flags: needinfo?(efaustbmo)
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?
Attachment #8658310 - Flags: review?(jdemooij) → review+
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
Attachment #8658314 - Flags: review?(jdemooij) → review+
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 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.
Attachment #8658310 - Flags: review?(jorendorff) → review+
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.
Attachment #8658313 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 20

3 years ago
Created attachment 8663079 [details] [diff] [review]
Tests
Attachment #8663079 - Flags: review?(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
Attachment #8663079 - Flags: review?(jorendorff) → review+
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)
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).
Depends on: 1235194
No longer depends on: 1235194
The documentation for super is here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/super
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1251921
You need to log in before you can comment on or make changes to this bug.