Prepack: Implement Classes In The Interpreter

[sebmarkbage]

This would be a good start to being able to fully serialize classes. The serialization part is difficult because we have to serialize some features like methods with super calls using the class syntax.

However this issue is only about implementing the interpreter.

A good starting point is here:

https://github.com/facebook/prepack/blob/master/src/evaluators/ClassDeclaration.js

Then copying the spec piece by piece.

https://www.ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-classdefinitionevaluation

The syntax is already in the Babel AST. Most of the Value model such as HomeObject and new.target is already in place.

Any previously unimplemented methods of the spec will need to be implemented. Such as MakeClassConstructor.

Running yarn test-test262 will run the full test suite which can be used to validate the final result.

yarn repl is good for experiments.

[wdhorton]

I'd like to try tackling this issue!

[hermanventer]

Go for it.

[sebmarkbage]

@wdhorton Have you made any progress? Is there something blocking you?

[wdhorton]

@sebmarkbage I think I'm most of the way there with #669—I've manually confirmed that the basic behavior is working in the REPL. I just need to dig into the test262 results and potentially adjust for edge cases, planning on doing that today.

[wdhorton]

So I'm looking through the test262 results, and I've run into some behavior that I can't understand.

The code is failing the test default-constructor-2 with this message:

    ✗ (es6) (strict): default-constructor-2.js
        Got an error, but was not expecting one:
        Interpreter: Thrown value was not an object!
        Native: Error
            at new ThrowCompletion (/home/ubuntu/prepack/lib/completions.js:74:41)
            at exports.default (/home/ubuntu/prepack/lib/evaluators/ThrowStatement.js:10:9)
            at LexicalEnvironment.evaluateAbstract (/home/ubuntu/prepack/lib/environment.js:1309:16)
            at LexicalEnvironment.evaluateAbstractCompletion (/home/ubuntu/prepack/lib/environment.js:1197:21)
            at EvaluateStatements (/home/ubuntu/prepack/lib/methods/function.js:1468:28)
            at exports.default (/home/ubuntu/prepack/lib/evaluators/BlockStatement.js:51:12)
            at LexicalEnvironment.evaluateAbstract (/home/ubuntu/prepack/lib/environment.js:1309:16)
            at LexicalEnvironment.evaluateAbstractCompletion (/home/ubuntu/prepack/lib/environment.js:1197:21)
            at OrdinaryCallEvaluateBody (/home/ubuntu/prepack/lib/methods/call.js:346:58)
            at InternalCall (/home/ubuntu/prepack/lib/methods/function.js:958:14)

If you look at where the message "Thrown value was not an object!" comes from in the harness, you find it in the definition of assert.throws:

assert.throws = function (expectedErrorConstructor, func, message) {
    if (typeof func !== "function") {
        $ERROR('assert.throws requires two arguments: the error constructor ' +
            'and a function to run');
        return;
    }
    if (message === undefined) {
        message = '';
    } else {
        message += ' ';
    }

    try {
        func();
    } catch (thrown) {
        if (typeof thrown !== 'object' || thrown === null) {
            message += 'Thrown value was not an object!';
            $ERROR(message);
        } else if (thrown.constructor !== expectedErrorConstructor) {
            message += 'Expected a ' + expectedErrorConstructor.name + ' but got a ' + thrown.constructor.name;
            $ERROR(message);
        }
        return;
    }

    message += 'Expected a ' + expectedErrorConstructor.name + ' to be thrown but no exception was thrown at all';
    $ERROR(message);
};

And the relevant code from the failing default-constructor-2 test:

class Base1 { }
assert.throws(TypeError, function() { Base1(); });

But when I run that same code manually in the REPL I get:

> class Base1 {}
undefined
> let func = function () { Base1() }
undefined
> func()
Error: TypeError
    at serialize (/Users/William/Desktop/prepack/lib/repl-cli.js:46:15)
    at REPLServer._eval (/Users/William/Desktop/prepack/lib/repl-cli.js:67:13)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:536:10)
    at emitOne (events.js:96:13)
    at REPLServer.emit (events.js:191:7)
    at REPLServer.Interface._onLine (readline.js:241:10)
    at REPLServer.Interface._line (readline.js:590:8)
    at REPLServer.Interface._ttyWrite (readline.js:869:14)

So it seems like it's throwing a TypeError, which should pass the test.

After some experimenting, I think the issue comes down to this:

> class Base1 {}
undefined
> let func = function () { Base1() }
undefined
> try { func() } catch (err) { console.log(typeof err) }
string

So my question for someone who knows more about how the error throwing behaves: is the test failing because the code is throwing a string rather than an object? Or does it just appear that way in the repl because it's serializing err?

[wdhorton]

I looked into another failing test, prepack/test/test262/test/language/statements/class/subclass/class-definition-null-proto-this.js, and found this note:

  The behavior under test was introduced in the "ES2017" revision of the
  specification and conflicts with prior editions.

You can see from the MDN docs that previously you couldn't instantiate a class that extended null:

class nullExtends extends null {
  constructor() {}
}

Object.getPrototypeOf(nullExtends); // Function.prototype
Object.getPrototypeOf(nullExtends.prototype) // null

new nullExtends(); //ReferenceError: this is not defined

The spec linked in this issue was the 6.0 (2015) version, but should I have been working off a more recent one?

[sebmarkbage]

It's probably better to work off the https://tc39.github.io/ecma262/ spec which is living spec so you get all the recent considerations. There shouldn't be any additional stuff in there yet anyway.

[sebmarkbage]

@wdhorton The TypeError you're seeing in the repl does seems like it's a TypeError thrown inside the Node environment that Prepack is running in. It's not a TypeError inside of Prepack itself. I.e. it's due to a bug in the Prepack repl trying to deal with the error handling.

So it would indicate to me that something is throwing a string.

I would expect this line to be the one to trigger an error. https://github.com/facebook/prepack/blob/4b106e3974967c16304e8ff7e46f9e57c2e25289/src/methods/call.js#L361

Maybe isCallable returns true?

https://github.com/facebook/prepack/blob/1eacf0cbe4a2d5a47ea619766a6d09df4c39fddf/src/methods/is.js#L99

I believe the constructor shouldn't have a [[Call]] slot defined.

[sebmarkbage]

^ Never mind, I'm wrong. It seems like this check should fire:

https://tc39.github.io/ecma262/#sec-ecmascript-function-objects-call-thisargument-argumentslist

https://github.com/facebook/prepack/blob/57cc59c07d164e4827c24637af9c1002b0c4a3c8/src/methods/function.js#L597

[sebmarkbage]

I believe that line is wrong in master. It should do:

throw realm.createErrorThrowCompletion(realm.intrinsics.TypeError, "not callable");

[wdhorton]

@sebmarkbage thanks for tracking that down!

[wdhorton]

@sebmarkbage what's next in terms of getting classes working in prepack? would love to keep working on this. I ignored all the failing test262 tests with super when I implemented ClassDeclaration, so maybe I can work on getting super supported?

[sebmarkbage]

Yea, I think it might be worth while getting super supported in the interpreter first because that'll set up you well for the next challenging part.

The next challenging part will be to implement the serialization of classes back into JS. This will need to preserve things like super. Even if the method has been copied over between different objects and such.

[wdhorton]

@sebmarkbage I got super working via #762 and #764. Could you give me some examples of what serialization of classes should look like, so I know the right direction to go? Like, for example, say you have this code:

(function() {
  class Foo {
    constructor(x) {
      this.x = x;
    }
  }

  global.foo = new Foo(5);
})();

How should that be serialized? Does it change if we add a method to the class? And what does it mean to preserve super? There's obviously a lot of complexity here, just want to be sure I don't head too far down the wrong track from the outset

[sebmarkbage]

I don't actually know the best strategy but there are a few interesting things that I can think of. I'd start by considering class constructors and methods separately, where methods on classes is just a special case of where methods can go.

__When should something get serialized as a class syntax?__

I think the only thing that determines this is the constructor function. If its FunctionKind is "classConstructor" but otherwise simple, then it should be a class.

let x = class {
  constructor() { }
};

Likewise if it has super() in it, it must be defined as a class constructor with extends.

let y = ...;
let x = class extends y {
  constructor() { super(); }
};

Note that it is perfectly fine for y to be a different prototype property and [[Prototype]] than when it was first created.

class A { }
class B { }
class C extends A {

}
Object.setPrototypeOf(C, B);

This can't get serialized as:

let C = class extends B { };

This will need to get serialized as a class and then mutation.

In terms of serializing the methods and properties of the class, the most primitive solution is using the defineProperty (there are helpers in the serializer for this already).

class Foo {
  bar() {}
}

let bar = function() { };
let Foo = class { };
Object.defineProperty(Foo.prototype, 'bar', {
  value: bar,
  enumerable: false,
  configurable: true,
  writable: true,
});

This covers all the cases where a property might have been mutated and configured after initialization. It doesn't work for methods with super.method() calls in them though.

__How to serialize "methods"?__

Any function with a super.method() or super[method]() syntax in them will have a [[HomeObject]]. This is the object that was first used to initialize the method. To be able faithfully restore such a method it needs to be serialized on to the object it was originally created on. This may be tricky because that may not always be the case anymore.

Imagine this scenario:

let foo = {
  bar() {
    super.bar();
  }
};
let baz = {
  bar: foo.bar
};
delete foo.bar;
inspect = function() {
  return baz;
};

You can't just serialize this as:

let baz = {
  bar() {
    super.bar();
  }
};
inspect = function() {
  return baz;
};

Because the [[HomeObject]] of baz.bar would now be the wrong object. So the super call would look in the wrong prototype chain. The foo object is still reachable through the [[HomeObject]]. So it needs to get serialized:

let foo = {
  bar() {
    super.bar();
  }
};
...

However, now the value of bar is wrong. In fact the property is even deleted, so you have to add some code that deletes it... but only after you've already stored it somewhere.

let bar = foo.bar;
delete foo.bar;

No you can initialize baz:

let baz = {
  bar: bar
};

The same thing goes for classes but it is extra tricky because a class creates two objects that can have methods on them.

class Foo {
  bar() {
    super.bar();
  }
  static baz() {
    super.baz();
  }

}
let foo = {
  bar: Foo.prototype.bar,
  baz: Foo.baz,
};
delete Foo.prototype.bar;
delete Foo.baz;
inspect = function() {
  return foo;
};

__Cycles might be tricky.__ Since the heap created by Prepack running initialization can yield cycles that are not expressable in a declarative way, you might have to do some trickery to preserve cycles. I can't think of a scenario where this is more tricky than the normal problems Prepack has to deal with.

@NTillmann Can probably point you in the right direction in the Prepack serializer code.

[NTillmann]

Looks like @sebmarkbage thought of most of the relevant issues...

As Sebastian said, the serializer already issues Object.defineProperty, that's easy. Deleting properties on the home object when needed should also be okay. Cycles in general are a problem, and are handled via the _shouldDelayValue / _delay mechanism in the serializer.

To begin with, changes will be needed in two places:
1) The ResidualHeapVisitor.js will have to announce dependencies on the [[HomeObject]] in visitValueFunction.
2) The serializer.js will have to... a) emit custom code in _serializeValueFunction for functions that are in fact classes, and b) write up functions part of the home object probably in the default case of _serializeValueObject, possibly deleting them afterwards.

If it is known that a function is a class, then I'd prefer to keep the class structure in the serialized code instead of always falling back to defineProperty calls, which is more verbose and likely more expensive. And when there are super references, then we of course have to stick to the original structure.

There's a general assumption in the serializer that function definitions get hoisted. If the only way to declare a function properly is by sticking it within in the home object declaration, then this assumption doesn't hold anymore. Fixable, as the serializer can generally deal with arbitrary dependencies, but it will need a careful review.

This is a huge undertaking. I suggest to break it up into manageable chunks, where Prepack simply detects unsupported scenarios and issues a this.logger.logError to report the limitation --- there are already a handful of existing unsupported cases that you can find a ResidualHeapVisitor.js. Then you can later lift the limitations one by one.