1141862 - Implement ES6 SuperProperty and SuperMember

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

super.prop and super[expr] are vaid in any method definition, both in classes and object litterals.

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 1: Free up bits in JSFunction for isMethod() and later isClassConstructor()

Asking Jan for review so he can make sure the changed jitcode is sane.

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 2: Allow Lazy scripts to pass Method as the FunctionSyntaxKind during delazification and remove FunctionSyntaxKind::Lazy

Now that we have a way to mark method functions as methods, there's no need for FunctionSyntaxKind::Lazy, which was added to deal with this problem.

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 3: Remove unused argument from EmitPropLHS()

This got rs over IRC, but dumping here again for posterity.

Comment 4

[Eric Faust [:efaust]]

Attachment: Part 4: Make everything defined with MethodDefinition syntax use the Method FunctionSyntaxKind

Comment 5

[Eric Faust [:efaust]]

Attachment: Part 5: Implement ES6 SuperProperty and SuperMember

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8575693 [details] [diff] [review]
Part 1: Free up bits in JSFunction for isMethod() and later isClassConstructor()

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

Looks good. This adds an extra and + branch to branchIfNotInterpretedConstructor, but that path shouldn't be very hot.

::: js/src/jit/MacroAssembler.cpp
@@ +2230,5 @@
>      // Common case: if IS_FUN_PROTO, ARROW and SELF_HOSTED are not set,
>      // the function is an interpreted constructor and we're done.
> +    Label done, moreChecks;
> +
> +    // Start with the easy ones. Check IS_FUN_PROTO and SELF_HOSTED

Nit: '.' at the end.

@@ +2241,5 @@
> +
> +    bits = IMM32_16ADJ(JSFunction::ARROWKIND);
> +    branch32(Assembler::NotEqual, scratch, Imm32(bits), &done);
> +
> +    // Reload the smashed flags and nargs for more checks

And here.

::: js/src/jsfun.h
@@ +30,5 @@
>  
> +    enum FunctionKind {
> +        NormalFunction = 0,
> +        Arrow,
> +        ASMJs

Nit: "AsmJS" is what we use elsewhere (AsmJSFrameIterator, IsAsmJSModule, etc).

@@ +38,5 @@
>          INTERPRETED      = 0x0001,  /* function has a JSScript and environment. */
>          NATIVE_CTOR      = 0x0002,  /* native that can be called as a constructor */
>          EXTENDED         = 0x0004,  /* structure is FunctionExtended */
> +        IS_FUN_PROTO     = 0x0008,  /* function is Function.prototype for some global object */
> +        EXPR_CLOSURE     = 0x0010,  /* expression closure: function(x) x*x */

Maybe these two could be added to FunctionKind as well, but at least IS_FUN_PROTO will complicate the branchIfNotInterpretedConstructor code even more.

To fix that, we should probably add a new INTERPRETED_CONSTRUCTOR flag, then testing that is fast and easy, although it'll cost us an extra flag (but the enum makes up for that).

File a follow-up bug? I don't think this has to block your class work but it'd be nice to do at some point.

@@ +58,5 @@
> +        FUNCTION_KIND_MASK  = 0x6000,
> +
> +        /* Kind flags, for convenience */
> +        ASMJSKIND = ASMJs << FUNCTION_KIND_SHIFT,
> +        ARROWKIND = Arrow << FUNCTION_KIND_SHIFT,

Nit: slight preference for ASMJS_KIND and ARROW_KIND.

Comment 7

[Eric Faust [:efaust]]

Attachment: Part 4: Make everything defined with MethodDefinition syntax use the Method FunctionSyntaxKind v2

Updated to not change the toSource() behavior of the resulting function objects.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8575697 [details] [diff] [review]
Part 2: Allow Lazy scripts to pass Method as the FunctionSyntaxKind during delazification and remove FunctionSyntaxKind::Lazy

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

Yeah, this "Lazy" thing was a terrible idea. Who comes up with stuff like that? r=me twice.

::: js/src/frontend/Parser.cpp
@@ +2586,5 @@
>  #endif
>          if (tokenStream.hadError())
>              return false;
>          funbox->bufEnd = pos().end;
> +        if (kind == Statement && !MatchOrInsertSemicolon(tokenStream))

Once again it took me like ten minutes to puzzle out why we're looking for a semicolon here.  Would you mind adding a comment explaining that this is to handle the super-rare expression-closure-statement monster? You may have to include a photo or people will think they are mythical.

    // function getRandomNumber() 4;  /* Munroe 2007 */

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8576199 [details] [diff] [review]
Part 4: Make everything defined with MethodDefinition syntax use the Method FunctionSyntaxKind v2

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

::: js/src/frontend/Parser.cpp
@@ +1240,5 @@
>          break;
>        case Method:
> +        flags = methodIsExpr
> +                  ? JSFunction::INTERPRETED_LAMBDA_METHOD
> +                  : JSFunction::INTERPRETED_METHOD;

methodIsExpr and INTERPRETED_LAMBDA_METHOD seem like bad names to me. These are used exclusively for getter/setter syntax, right? Do we actually need to know this later? What breaks if we ditch the boolean and just set INTERPRETED_METHOD here unconditionally?

The documentation for LAMBDA says:

        LAMBDA           = 0x0080,  /* function comes from a FunctionExpression, ArrowFunction, or
                                       Function() call (not a FunctionDeclaration or nonstandard
                                       function-statement) */

I don't know why we have this, but in any case it doesn't seem right to set this bit on getters and setters.

@@ +2186,5 @@
>          proto = GlobalObject::getOrCreateStarGeneratorFunctionPrototype(cx, context->global());
>          if (!proto)
>              return null();
>      }
> +    bool methodIsExpr = kind == Method && type != Normal;

It occurs to me that Normal is not a good name for an enum. Please change it. It could be like this:

    enum FunctionType { Getter, Setter, NotGetterOrSetter };

@@ +8342,3 @@
>                                         bool isStatic, JSOp op)
>  {
> +    /* NB: Getter function in { get x(){} } is unnamed. */

Why remove the comment?

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8575706 [details] [diff] [review]
Part 5: Implement ES6 SuperProperty and SuperMember

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

Publishing partial review so you can work on tests while I'm looking at other stuff.

::: js/src/js.msg
@@ +201,5 @@
>  MSG_DEF(JSMSG_BAD_RETURN_OR_YIELD,     1, JSEXN_SYNTAXERR, "{0} not in function")
>  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")

Just "within methods" is sufficient for an error message. People seeing this error message will not be in a situation where they need to hear about eval code.

::: js/src/tests/ecma_6/Class/superProp.js
@@ +1,3 @@
> +var test = `
> +
> +// Super prop works in any class method. Getters, setters, constructors, what

Please break this up into many tiny files. I don't expect everybody to understand super very well. It's tricky. Regressions in a monolithic test would therefore be amazingly painful. (Also it is kind of hard to review like this as it's not always obvious what's being tested.)

Here are more things to test, with apologies for the ones that are already tested here.

Also I actually have not checked that any of these are actually what the spec says, so please treat these as guesses.

In general, use separate files, within reason.

The first few of these are super basic from the user's point of view. I think they should go quick.

* super works in classes without extends - a toString() method calling super.toString() makes a nice example

* if Derived.method contains a call to `super.method()` it calls Base.method as expected

    * same for a getter or setter

* a super method call can use the spread operator: `super.fun(a, b, ...moreArgs)`

* `super.prop` ignores a `this.prop` property defined on more-derived classes/prototypes

* `super.prop` evaluates to undefined when `this.prop` exists but nothing further along the prototype chain from [[HomeObject]] has `.prop`

* `super.prop = 3` defines a new property on the original receiver (that is, `this.prop`) if there's no `.prop` anywhere on the prototype chain

* given a chain of 3 classes (Derived extends Base, MoreDerived extends Derived): in MoreDerived.prototype.m, `super.m()` calls Derived.prototype.m; and in that method, `super.m()` calls Base.prototype.m.
    (of course it follows from other unit tests that this should work, but an occasional end-to-end test is nice)

* show that if Base *doesn't* have a method (let's say toString()), and Derived contains `super.toString()`, we correctly find Object.prototype.toString().

* in a method of class Derived, `super.prop = 3` defines a new property on the original receiver even if Derived.prototype.prop exists...

* ...and even if it's non-writable

* the presence of super in a method doesn't affect behavior of `this` (silly, but easy to imagine it being wrong)

* super works in static methods

    * and it skips non-static methods in base classes

* super works in object literals (looks at Object.prototype)

* setPrototypeOf

    * after an object's prototype is changed, `super` in its methods starts searching in new prototype.

    * ordering: in `super[f()]` and `super[x] = f()` and `super.method(f())`, if f() changes the home object's prototype, check that this correctly does/doesn't affect the search

* fixed-ness of [[HomeObject]]:

    * taking a super-using method and sticking it on some other object, and calling it as a method of that object, doesn't cause `super.foo` to do anything different (it still searches its original HomeObject's proto chain)

    * taking a super-using method and sticking it in a variable, then calling it using plain `f()` syntax, doesn't cause `super` to do anything different

    * using `method.call()` to pass a random `this` value to a super-using method doesn't cause `super` to do anything different

        * and super still passes `this` along to a super-method even if it's not an object, as in derivedObj.method.call("oh no it's a string").

    * if you evaluate a single class-expression twice, with an `extends` clause that evaluates to two different base classes, `super` in either class correctly walks its proto chain.

* long chains:

    * `super.x` and `super.method()` can walk extremely long proto chains without crashing or throwing "too much recursion"

    * `super.x` and `super.method()` will throw "too much recursion" if the prototype chain has a loop in it, something like `a = new Proxy(b, {}); a.__proto__ = b;`

* an arrow function can still use `super` after control exits the enclosing function.

* when a super-assignment happens in an object literal in non-strict code, are we supposed to get non-strict assignment semantics? either way, it's something to test.

* test that if a class extends a plain old function, `super` works as specified even if you tamper with the plain old function's .prototype property.

* when you use super to get a property, and that property has a getter, the `this` value received by the getter is unchanged (i.e. it's the original receiver)

    * same when the `super` expression is in direct eval or an arrow function

    * same for assigning to super.x when that property has a setter (the setter gets the original receiver as `this`)

* the usual assignment weirdness:

    * test that `super[x] = f()` does the right thing when f() changes the value of x.

    * test what happens in `super.x = f()` when f() adds or deletes properties named 'x' on this object or on its proto chain
        (in other words the spec says exactly when that `.x` is *bound* to a particular object, either before or after f() is called; make sure it is bound at the right time)

    * same for augmented assignment `super.x += f()`: make sure the property is got first, before f() is called, and assignment affects the right object after f() is called and addition happens.

    * test that super works in destructuring: `[super.x, super.y] = [0, 0]`

* proxies

    * when a proxy is on the base class's prototype chain, super.x goes through the get trap, with the original receiver

    * likewise assignment goes through the set trap

    * when a proxy is on a *more-derived* class's prototype chain, super in a more-base class doesn't fire any proxy traps

    * please test cross-compartment wrappers (easy to create with var g = newGlobal(); g.eval('some object literal'))
        as well as scripted proxies

    * when a derived class's `extends` clause itself evaluates to a proxy, `super` references in the derived class's static methods search the proxy.

* expression autopsy: super.x() should produce the error message "super.x is not a function" (ideally -- but at least something reasonable)

@@ +4,5 @@
> +// have you.
> +class base {
> +    constructor() { }
> +    init() {
> +        // When super prop is invoked, the |this| valus is the this of the

typo: "valus"

@@ +38,5 @@
> +        // And, by extension, all operator assignments.
> +        yield (super.prop += 2);
> +    }
> +
> +    // Inside functions, you can even inherity from a super property

typo

@@ +83,5 @@
> +    }();
> +}
> +assertThrowsInstanceOf(nope, SyntaxError);
> +
> +// Super also works in object litterals

typo: "litterals"

::: js/src/tests/js1_8_5/extensions/reflect-parse.js
@@ +1432,5 @@
>  
> +    /* SuperProperty */
> +    // NOTE: Some of these tests involve object literals, as SuperProperty is a
> +    // valid production in any method definition, including in objectl
> +    // litterals. These tests still fall here, as |super| is not implemented in

Three typos: "objectl" "litterals" and I think "fall" should be "fail". Late night? :)

@@ +1437,5 @@
> +    // any form without classes.
> +
> +    // This started in my queue as a really big, REALLY ugly set of nested for
> +    // loops to explore the state space. In the end, I ripped it out and landed
> +    // here. I hope this is better....

You can delete this comment.

For the record I like

    function assertComplicatedThings(lots, of, args) {
        ...many lines...
    }

    for (let a of ...) {
        for (let b of ...) {
            for (let c of ...) {
               assertComplicatedThings(a, b, c);
            }
        }
    }

...However in most cases building massive Cartesian cross-products is easier than it is useful. Knowing that both the parser and the Reflect.parse implementation are recursive descent, and the various pieces are supposed to be orthogonal, it just seems unlikely for this technique to discover bugs. ---Did it actually catch bugs? I'd be very interested in being wrong on this!

@@ +1477,5 @@
> +            ["super['prop'] >>>= 4", aExpr(">>>=", superMember, lit(4))],
> +            ["super.prop |= 4", aExpr("|=", superProperty, lit(4))],
> +            ["super['prop'] |= 4", aExpr("|=", superMember, lit(4))],
> +            ["super.prop ^= 4", aExpr("^=", superProperty, lit(4))],
> +            ["super['prop'] ^= 4", aExpr("^=", superMember, lit(4))],

I tend to think a handful of representative tests here would be fine...

@@ +1572,5 @@
> +    assertError("super.prop", SyntaxError);
> +    assertError("function foo () { super['prop']; }", SyntaxError);
> +    assertError("(function () { super['prop']; }", SyntaxError);
> +    assertError("(()=>super['prop'])", SyntaxError);
> +    assertError("function *foo() { super['prop']; }", SyntaxError);

assertError("class X { constructor() { function nested() { super.prop; }}}");

@@ +1580,5 @@
> +    assertError("super", SyntaxError);
> +
> +    // Even where super is otherwise allowed
> +    assertError("{ foo() { super } }", SyntaxError);
> +    assertClassError("class NAME { constructor() { super; } }", SyntaxError);

assertClassError("class Name { constructor() { super", SyntaxError);
assertClassError("class Name { constructor() { super.", SyntaxError);
assertClassError("class Name { constructor() { super.x", SyntaxError);
assertClassError("class Name { constructor() { super.m(", SyntaxError);
assertClassError("class Name { constructor() { super[", SyntaxError);
assertClassError("class Name { constructor() { super(", SyntaxError);

::: js/src/vm/Opcodes.h
@@ +842,5 @@
> +    /*
> +     * Initialize the home object for functions with super bindings.
> +     *
> +     * This opcode takes the function and the object to be the home object, does
> +     * the set, and leaves both on the stack.

Please add the "Category:" and other junk here, run make_opcode_doc.py, and paste the updated output to
    https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode

I might actually like
     *   Stack: homeObject fun => homeObject fun
better than the sentence you've written here...

@@ +964,5 @@
>       *   Stack: => obj
>       */ \
>      macro(JSOP_NEWARRAY_COPYONWRITE, 102, "newarray_copyonwrite", NULL, 5, 0, 1, JOF_OBJECT) \
>      \
> +    macro(JSOP_SUPERBASE,  103, "superbase",   NULL,         1,  0,  1,  JOF_BYTE) \

New opcode needs a comment.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8575706 [details] [diff] [review]
Part 5: Implement ES6 SuperProperty and SuperMember

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

The core parts of this, in vm/Interpreter.cpp and frontend/Parser.cpp, are good code. I know Parser::memberExpr() is a mess, but that's pre-existing. The patch wasn't hard to read.

I think this test will fail:

    class X {
        constructor() {}
        set name(v) { return this._name; }
    }
    class Y extends X {
        constructor() { this._name = "y"; }
        set name(v) { return "super " + super.name; }
    }
    assertEq(new Y().name, "super y");

Assuming the spec says this should pass (again, I didn't really look), we'll need variants of JSOP_GETPROP and JSOP_SETPROP that take an additional 'receiver' operand.

Clearing the review flag for now.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2136,1 @@
>              *answer = true;

Style nit: Either put the whole condition on a single line or add braces.

::: js/src/frontend/ParseNode.h
@@ +1451,5 @@
> +        MOZ_ASSERT_IF(match, node.isArity(PN_UNARY));
> +        return match;
> +    }
> +
> +    ParseNode *expr() const {

Might be nice if this were named `propExpr()` or `propertyExpr()`, your call.

::: js/src/frontend/Parser.cpp
@@ +7678,5 @@
> +            if (sc->isFunctionBox())
> +                sc->asFunctionBox()->setNeedsHomeObject();
> +            return true;
> +        } else if (sc->isFunctionBox() && !sc->asFunctionBox()->function()->isArrow()) {
> +            // super is not legal is normal functions.

typo: "is normal functions"

You already have a test for indirect eval. Please also test that:

*   Function("super.man") is a SyntaxError
*   function foo() { eval("super.prop"); } is a SyntaxError
*   ({foo() { eval('eval("super.prop")'); }}) is OK

@@ +7739,5 @@
> +        if (tt == TOK_EOF) {
> +            if (isSuper) {
> +                report(ParseError, false, null(), JSMSG_BAD_SUPER);
> +                return null();
> +            }

Please put this outside the loop.

@@ +7791,5 @@
>          {
> +            if (isSuper) {
> +                report(ParseError, false, null(), JSMSG_BAD_SUPER);
> +                return null();
> +            }

LOL, test for this please.

@@ +7814,5 @@
>                          pc->sc->asFunctionBox()->setHasExtensibleScope();
> +
> +                    // Deliberately ignore whether anything was marked. We
> +                    // only care that we did mark if we had to.
> +                    checkAndMarkSuperScope();

The comment confused me -- how about 

    // If we're in a method, mark the method as requiring support for 'super',
    // since direct eval code can use it. (If we're not in a method, that's fine,
    // so ignore the return value.)

@@ +7856,2 @@
>              tokenStream.ungetToken();
>              return lhs;

Instead of the duplicate check here, how about:

    } else {
        tokenStream.ungetToken();
        break;
    }

::: js/src/js.msg
@@ +201,5 @@
>  MSG_DEF(JSMSG_BAD_RETURN_OR_YIELD,     1, JSEXN_SYNTAXERR, "{0} not in function")
>  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")

I think JSMSG_BAD_SUPERPROP would be clearer without extra nouns:
    "'super' is only allowed in methods"
If you change this, don't forget to change the number of arguments 1 -> 0 and update the place in Parser.cpp where this message is emitted.

::: js/src/jsfun.h
@@ +113,5 @@
>          // Note: this should be kept in sync with FunctionBox::isHeavyweight().
>          return nonLazyScript()->hasAnyAliasedBindings() ||
>                 nonLazyScript()->funHasExtensibleScope() ||
>                 nonLazyScript()->funNeedsDeclEnvObject() ||
> +               nonLazyScript()->needsHomeObject()       ||

Why is this necessary? Can we not get at the callee from a non-heavyweight frame?

::: js/src/jsscript.h
@@ +1251,5 @@
>      }
>  
> +    void setNeedsHomeObject() {
> +        needsHomeObject_ = true;
> +    }

I think this is never called. Delete it?

::: js/src/vm/Interpreter.cpp
@@ +3757,5 @@
>          objProto = protoProp.toObjectOrNull();
>      }
>  
> +    REGS.sp[-1].setObject(*funcProto);
> +    PUSH_OBJECT_OR_NULL(objProto);

Update the comment in Opcodes.h for JSOP_CLASSHERITAGE, please.

@@ +3815,5 @@
> +{
> +    ScopeIter si(cx, REGS.fp()->scopeChain(), REGS.fp()->script()->innermostStaticScope(REGS.pc));
> +    for (; !si.done(); ++si) {
> +        if (si.hasScopeObject() && si.type() == ScopeIter::Call) {
> +            JSFunction &callee = si.scope().as<CallObject>().callee();

It seems like you should be able to skip the ScopeIter and do

    JSFunction &callee = REGS.fp()->callee();

in which case methods that use super do not need to be heavyweight.

@@ +3830,5 @@
> +
> +            if (!superBase) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_CONVERT_TO,
> +                                     "null", "object");
> +                goto error;

Awesome error. Definitely need a test or two for this.

Comment 12

[Eric Faust [:efaust]]

Attachment: Part 5a: Change JSOP_SETCALL to JSOP_THROWMSG, envisioning use in later patch

Comment 13

[Eric Faust [:efaust]]

Attachment: Fix the embarassingly wrong initial SuperProperty patch, as a diff from the original.

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 8585730 [details] [diff] [review]
Part 5a: Change JSOP_SETCALL to JSOP_THROWMSG, envisioning use in later patch

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

::: js/src/vm/Opcodes.h
@@ +1218,5 @@
>       *   Stack: callee, this, args[0], ..., args[argc-1] => rval
>       *   nuses: (argc+2)
>       */ \
>      macro(JSOP_STRICTEVAL,       124, "strict-eval",       NULL,         3, -1,  1, JOF_UINT16|JOF_INVOKE|JOF_TYPESET|JOF_CHECKSTRICT) \
> +    macro(JSOP_UNUSED125,  125, "unused125", NULL,       1,  0,  0,  JOF_BYTE) \

Looks like this whitespace change was unintentional.

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 8585735 [details] [diff] [review]
Fix the embarassingly wrong initial SuperProperty patch, as a diff from the original.

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

Is still missing some tests? I didn't see any tests for strict assignment failure, for example...

Please add a test that looks at `for (super.prop in obj) STMT`.

Also, I guess `new super.prop(args)` is legal syntax? Please test it, even though it isn't extra magical the way `super.prop(args)` is.

    class Base {
        constructor() {}
    }
    class Mid extends Base {
        constructor() {}
        f() { return new super.constructor(); }
    }
    class Derived extends Mid {
        constructor() {}
    }
    let d = new Derived();
    var df = d.f();
    assertEq(df.constructor, Base);

Apart from that, this looks great. Excited to see it land. r=me with these comments addressed.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2438,5 @@
> +            return false;
> +    }
> +
> +    JSOp setOp = sc->strict() ? JSOP_STRICTSETPROP_SUPER
> +                                   : JSOP_SETPROP_SUPER;

Style micro-nit: All this fits on one line. If you like two lines, indent : to line up with ?.

@@ +2507,5 @@
> +    return true;
> +}
> +
> +bool
> +BytecodeEmitter::emitSuperElemOperands(ParseNode *pn, SuperElemOptions opts)

The body of this method is handling so many cases that I found it a bit mind-bending, but I have no recommendation how to make it clearer.

@@ +2514,5 @@
> +
> +    // The ordering here is somewhat screwy. We need to evaluate the propval
> +    // first, by spec. Do a little dance to not emit more than one JSOP_THIS.
> +    // Since JSOP_THIS might throw in derived class constructors, we cannot
> +    // just push it earlier as the receiver. We have to swap it down instead.

Very good comment.

@@ +2634,5 @@
> +{
> +    MOZ_ASSERT(pn->pn_kid->isKind(PNK_SUPERELEM));
> +
> +    // We need to convert the key to an object id first, so that we do not do
> +    // it inside both the GETELEM and the SETELEM.

Maybe this comment belongs on the code that emits JSOP_TOID in emitSuperElemOperands.

@@ +4128,2 @@
>              return false;
>          offset++;

Why doesn't this have to be `offset += 2`?

(If it should be 2, and tests didn't catch this, please add a test...)

@@ +6147,5 @@
>          break;
>        }
> +      case PNK_SUPERPROP:
> +        // Still have to calculate the base, even though we are are going
> +        // to throw unconditionally, as calculating the base coul also

typo "coul"

::: js/src/frontend/BytecodeEmitter.h
@@ +588,5 @@
>      bool emitClass(ParseNode *pn);
> +    bool emitSuperPropLHS(bool isCall = false);
> +    bool emitSuperPropOp(ParseNode *pn, JSOp op, bool isCall = false);
> +    bool emitSuperPropIncDec(ParseNode *pn);
> +    enum SuperElemOptions { SuperElem_Normal, SuperElem_Set, SuperElem_Call, SuperElem_IncDec };

Instead of Normal, how about SuperElem_Get?

::: js/src/jsopcode.cpp
@@ +1555,5 @@
> +      {
> +        RootedAtom prop(cx, loadAtom(pc));
> +        return write("super.") &&
> +               quote(prop, '\0');
> +      }

Needs a test.

@@ +1565,5 @@
>                 write("]");
> +      case JSOP_GETELEM_SUPER:
> +        return write("super[") &&
> +               decompilePCForStackOperand(pc, -3) &&
> +               write("]");

This too.

::: js/src/tests/ecma_6/Class/superPropBasicCalls.js
@@ +4,5 @@
> +// litterals.
> +class toStringTest {
> +    constructor() {
> +        // Install a property to make it plausible that it's the same this
> +        this.foo = "rhinocerous";

Just as there's no "i" in "team", there's no "u" in "rhinoceros".

::: js/src/tests/ecma_6/Class/superPropDelete.js
@@ +1,3 @@
> +var test = `
> +
> +// |delete super.prop| and |delete super[expr]| throw universally.

Why this is not an early error in strict mode code I'm sure I will never know.

@@ +20,5 @@
> +            // must get here. The fallthrough is far less pleasant.
> +            return;
> +        }
> +        // Not reached
> +        assertEq(false, true);

It seems like this test could be shortened to:

    testDeleteElem() {
        let sideEffect = 0;
        assertThrowsInstanceOf(() => delete super[sideEffect = 1], ReferenceError);
        assertEq(sideEffect, 1);
    }

...and that would test arrows a bit, too. Does it work?

@@ +37,5 @@
> +
> +`;
> +
> +if (classesEnabled())
> +    eval(test);

Great tests; please add:

    // `delete super.x` does not delete anything before throwing.
    var thing1 = {
        go() { delete super.toString; }
    };
    let saved = Object.prototype.toString;
    assertThrowsInstanceOf(() => thing1.go(), ReferenceError);
    assertEq(Object.prototype.toString, saved);

    // `delete super.x` does not tell the prototype to delete anything, when it's a proxy.
    var thing2 = {
        go() { delete super.prop; }
    };
    Object.setPrototypeOf(thing2, new Proxy({}, {
        deleteProperty(x) { throw "FAIL"; }
    });
    assertThrowsInstanceOf(() => thing2.go(), TypeError);

::: js/src/tests/ecma_6/Class/superPropDerivedCalls.js
@@ +31,5 @@
> +        this.reset();
> +        // While we're here. Let's check on super spread calls...
> +        let spread = [1,2,3];
> +        super.method(...spread);
> +        super.prop++;

Please add these in 'class derived', to test that super.whatever is different from this.whatever:

    method() { throw "FAIL"; }
    get prop() { throw "FAIL"; }
    set prop(v) { throw "FAIL"; }

::: js/src/tests/ecma_6/Class/superPropOrdering.js
@@ +10,5 @@
> +
> +    // Test orderings of various evaluations relative to the superbase
> +    testElem() { super[ruin()]; }
> +    testProp() { super.method(ruin()); }
> +    testElemAssign() { super["prop"] = ruin(); }

1. Please add a couple comments here. I don't know what testElem is testing, but for testProp and testElemAssign, we are testing that:

    // The starting object ([[HomeObject]].[[Prototype]]) for looking up super.method
    // is determined before ruin() is called.
    testProp() { super.method(ruin()); }

    // The starting object for looking up super["prop"] is determined before ruin() is called.
    testElemAssign() { super["prop"] = ruin(); }

2. As an additional test following testProp, please test that in super.method(f()), super.method is actually fully evaluated before f() is called, i.e. we're done searching the prototype chain, so if f() deletes the method it doesn't affect the method call.

You have a nice test that assignment *doesn't* work that way, in testAssignProp. To me this is a parallel to that.

@@ +54,5 @@
> +}
> +
> +function reset() {
> +    Object.setPrototypeOf(derived.prototype, base.prototype);
> +}

Does Object.setPrototypeOf cause us to set some bit on an object that says "setPrototype has happened; abandon all optimizations"? If so, in this test and a few others, it seems like it'd be a better test of IC/typeset/shape behavior if, instead of using reset() before each test, we could somehow generate all the objects fresh and start over --- so that as many of the opcodes as possible did *not* have that bit as a warning that the test is up to something sneaky.

No worries if it's unclear how to do this, just a thought.

::: js/src/tests/ecma_6/Class/superPropProxies.js
@@ +73,5 @@
> +
> +// What about a CCW?
> +var g = newGlobal();
> +var wrappedSuper = g.eval("({ method() { return super.hasOwnProperty('method'); } })");
> +assertEq(wrappedSuper.method(), true);

The case where the CCW is on the prototype chain should be tested too, and in the method we need to assert something about 'this' because part of what we're testing here is that the receiver argument to set() is properly rewrapped for the target compartment.

::: js/src/tests/ecma_6/Class/superPropSkips.js
@@ +30,5 @@
> +        // Since the receiver is the instance, we can shadow inherited
> +        // properties, even non-writable ones.
> +        assertEq(this.nonWritableProp, "pony");
> +        super.nonWritableProp = "bear";
> +        assertEq(this.nonWritableProp, "bear");

Both things being tested in this method surprised me -- if I ever thought through this stuff, I forgot about it. Nice.

::: js/src/tests/ecma_6/Class/superPropStatics.js
@@ +12,5 @@
> +    constructor() { }
> +    static test() {
> +        assertEq(super["notFound"], undefined);
> +        super.found();
> +        assertEq(this.foundCalled, true);

Please test that this .foundCalled property was created on derived, not base.

Add a static found() method on derived that throws, to test that we're skipping derived, as intended.

And please test `super.accessor` where there are accessor properties on both base and derived.

::: js/src/vm/Interpreter.cpp
@@ +2446,5 @@
> +
> +    if (!GetProperty(cx, obj, receiver, script->getName(REGS.pc), rref))
> +        goto error;
> +
> +    REGS.sp--;

JSOP_GETPROP_SUPER is marked as JOF_TYPESET, but there's no TypeScript::Monitor call here.

Symptomless, if we're not using those typesets anywhere yet, but don't set a trap for your later self...

@@ +2581,5 @@
> +    obj = &REGS.sp[-1].toObject();
> +
> +    MutableHandleValue res = REGS.stackHandleAt(-3);
> +
> +    // Since we have asserted that lval has to be an object, it cannot be

"lval" should be "obj"

::: js/src/vm/Opcodes.h
@@ +990,5 @@
>      \
>      macro(JSOP_SUPERBASE,  103, "superbase",   NULL,         1,  0,  1,  JOF_BYTE) \
> +    /*
> +     * Pops top two stack values, pushes property of it onto the stack, using
> +     * the other as a receiver.

The comment here has kind of weird wording.

I think it might be clearest to use the ES6 terms for things. It might be nice to note what expression form each opcode implements too.

     * Pop an object 'obj' and a value 'receiver', call 'obj.[[Get]](name, receiver)',
     * and push the result. Used to implement 'super.name'.

     * Pop the top three values on the stack as 'val', 'obj', and 'receiver', and
     * call 'obj.[[Set]](name, val, receiver)'. Throw if it returns false.
     * Otherwise push 'val' back on the stack. Used to implement 'super.name = val'.

etc.

I did not carefully check that the comments match the actual stack layout.

Comment 16

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9826cc15d3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8efa1acc03
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1e959a3d61ea
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b448e719490b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f269369087

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b9826cc15d3
https://hg.mozilla.org/mozilla-central/rev/7c8efa1acc03
https://hg.mozilla.org/mozilla-central/rev/1e959a3d61ea
https://hg.mozilla.org/mozilla-central/rev/b448e719490b
https://hg.mozilla.org/mozilla-central/rev/e5f269369087

Comment 18

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/876c8ee6b132

Here goes nothing...

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for spidermonkey bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8b868b82fa

Comment 20

[Eric Faust [:efaust]]

When we made methods into extended functions, I missed initializing the extended slots when creating them in ion. Added this hunk, and that seems to have taken care of things.

>+    if (info.flags & JSFunction::EXTENDED) {
>+        MOZ_ASSERT(info.fun->isMethod());
>+        static_assert(FunctionExtended::NUM_EXTENDED_SLOTS == 2, "All slots must be initialized");
>+        masm.storeValue(UndefinedValue(), Address(output, FunctionExtended::offsetOfExtendedSlot(0)));
>+        masm.storeValue(UndefinedValue(), Address(output, FunctionExtended::offsetOfExtendedSlot(1)));
>+    }   
>+    

relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3f6306dd05e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3f6306dd05e
https://hg.mozilla.org/mozilla-central/rev/d2a1fc813b91

Comment 22

[Hannes Verschore [:h4writer]]

Thanks for the quick and swift action:
regression: http://arewefastyet.com/regressions/#/regression/177940
confirmed fixed: http://arewefastyet.com/regressions/#/regression/178010

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b03c2314ba95

Comment 24

[Florian Scholz (Open Web Docs)]

A first doc is here, with examples for super in classes and in object literals:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/super
Not in any release notes, as it is Nightly only.