904701 - Implement prototype madness for ES6 generators

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andy Wingo [:wingo]]

Attachment: prototypes

The attached patch defines the ES6 generator meta-objects: GeneratorFunction, GeneratorFunctionPrototype, and GeneratorObjectPrototype.

It makes each ES6 generator function have its own prototype, descending from GeneratorObjectPrototype.  Generator functions themselves descend from GeneratorFunctionPrototype, whose constructor is GeneratorFunction.

X "descends from" Y if Object.getPrototypeOf(X) === Y.  Not sure what the right name for this relationship is.

Comment 1

[André Bargull [:anba]]

This seems to replicate the same issue as in V8 where `(function*(){}).prototype instanceof Object.getPrototypeOf(function*(){})` returns true instead of false. Per ES6 this is false, because the intrinsic %Generator% has no @@hasInstance hook and therefore the default @@hasInstance implementation from %FunctionPrototype% is used. This default implementation calls OrdinaryHasInstance which directly returns in step 1 because C (here: %Generator%) is not callable.

Comment 2

[Andy Wingo [:wingo]]

(In reply to André Bargull from comment #1)

This is related to the point in es-discuss about whether GeneratorFunctionPrototype is a function: https://mail.mozilla.org/pipermail/es-discuss/2013-August/032677.html

The issue I ran into when making GeneratorFunctionProtototype an object was this:

js> function*foo(){}
js> typeof foo.__proto__
"object"
js> foo.__proto__
Function.prototype.toString called on incompatible object

But I guess something is going wrong, incorrectly calling toString() for printing a non-function.

Comment 3

[Andy Wingo [:wingo]]

Attachment: Implement prototype madness for ES6 generators v2

Add a Generator constructor, like the Function constructor.

Define the GeneratorFunctionPrototype, GeneratorFunction, and
GeneratorObjectPrototype meta-objects.

When cloning or creating a star generator, give it
GeneratorFunctionPrototype as its prototype.

Each star generator function has a ".prototype" property, which has
GeneratorObjectPrototype as its prototype.  That prototype does not have
a ".constructor" link.

If Function.prototype.toSource is called on a non-function, chain up to
Object.prototype.toSource instead.

Prototypes of generator objects are no longer made with
GeneratorObject::class_.  This allows us to elide some "null" checks
from bug 352885.  Instead calling a generator on a method now signals a
typeerror.

Make the "send" generator method a simple alias to "next".

Comment 4

[Andy Wingo [:wingo]]

Comment on attachment 790179 [details] [diff] [review]
Implement prototype madness for ES6 generators v2

Updated patch fixes GeneratorFunctionPrototype to be a non-function object, adds a better changelog, a number of other stylistic fixes.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 790179 [details] [diff] [review]
Implement prototype madness for ES6 generators v2

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

Looks good, r=me with the comments addressed. Setting additional r?bhackett for the question about ExclusiveContext, immediately below.

::: js/src/frontend/Parser.cpp
@@ +1987,5 @@
>          return pn;
>  
> +    RootedObject proto(context);
> +    if (generatorKind == StarGenerator) {
> +        // FIXME: Is this asJSContext() correct?

Hmm. I don't know if that's safe here. I'll ask bhackett for additional review on this point.

::: js/src/jsfun.cpp
@@ +211,5 @@
> +    if (isStarGenerator) {
> +        objProto = obj->global().getOrCreateStarGeneratorObjectPrototype(cx);
> +    } else {
> +        objProto = obj->global().getOrCreateObjectPrototype(cx);
> +    }

Style nit: no {} around the two branches of the if statement, since they fit on one line each and the condition also fits on a single line.

@@ +216,3 @@
>      if (!objProto)
>          return NULL;
> +    Class *classp = &JSObject::class_;

Style nit: For whatever reason we spell it "clasp" not "classp".

@@ +834,5 @@
> +    if (!obj->is<JSFunction>() && !obj->is<FunctionProxyObject>()) {
> +        // It's possible for this function to be called on a non-function.  For
> +        // example, the ES6 GeneratorFunctionPrototype is an object, but its
> +        // prototype is Function.prototype.  In that case, chain up to
> +        // Object.prototype.toSource.

Oh, yuck. :-P

Well, all right... a few notes here:

1. I'd be fine with just directly tail-calling obj_toSource (defined in builtin/Object.cpp; you'd have to declare it in the header). That is less code and the whole situation is deeply dontcare, so I'd slightly prefer this approach.

2. If you do retain this code, this should be doing effectively `super.toSource()`, which means the `receiver` argument to JSObject::getProperty should be args.thisv(), not proto. (It usually would not matter, admittedly.)

3. Needs a test either way, I'm afraid.

@@ +849,5 @@
> +        str = ToString<CanGC>(cx, rval);
> +    } else {
> +        str = fun_toStringHelper(cx, obj, JS_DONT_PRETTY_PRINT);
> +    }
> +    

Nit: delete whitespace on blank line

::: js/src/jsiter.cpp
@@ +1498,5 @@
> +        RootedObject fun(cx, stackfp->fun());
> +        // FIXME: We shouldn't have to do a lookup to get the prototype for the
> +        // instance.
> +        if (!JSObject::getProperty(cx, fun, fun, cx->names().classPrototype, &pval))
> +            return NULL;

File a follow-up bug for this, please, and cite its bug number in the FIXME comment. (Also make it clear in the comment that this is an optimization opportunity, not a correctness bug.)

@@ +1502,5 @@
> +            return NULL;
> +        JSObject *proto = pval.isObject() ? &pval.toObject() : NULL;
> +        if (!proto) {
> +            // FIXME
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_ES6_UNIMPLEMENTED);

Isn't this FIXME pretty easy to fix? The fallback proto is supposed to be global->getOrCreateStarGeneratorObjectPrototype(cx), right?

@@ +1651,3 @@
>  {
> +    JS_ASSERT(IsLegacyGenerator(ObjectValue(*obj)));
> +    

trailing whitespace on the blank line

@@ +1670,2 @@
>          return js_ThrowStopIteration(cx);
>      }

Style nit: Please remove the curly braces, since the body fits on one line now.

@@ +1811,5 @@
>          global->setReservedSlot(ELEMENT_ITERATOR_PROTO, ObjectValue(*proto));
>      }
>  
> +    if (global->getSlot(LEGACY_GENERATOR_OBJECT_PROTO).isUndefined()) {
> +        proto = NewObjectWithClassProto(cx, &JSObject::class_, NULL, cx->global());

Consider using getOrCreateObjectPrototype with NewObjectWithGivenProto
instead of adding another call to NewObjectWithClassProto (which is a mess).

Same thing in a few more places below. A new helper function that does this would be fine.

@@ +1862,5 @@
>      }
>  
> +    return LinkConstructorAndPrototype(cx,
> +                                       global->getOrCreateStarGeneratorFunctionPrototype(cx),
> +                                       global->getOrCreateStarGeneratorObjectPrototype(cx))

It's funny, calling global->setReservedSlot() to store each object in the global before we're finished initializing that object.

I think it would be better to do it like this:

    if (global->getSlot(STAR_GENERATOR_OBJECT_PROTO).isUndefined()) {
        ...create all three objects, using RootedObject to keep them rooted...
        ...two LinkConstructorAndPrototype calls...
        ...three global->setReservedSlot calls.
    }

Of course it only matters if something fails due to OOM, which shouldn't happen.

::: js/src/tests/ecma_6/Generators/runtime.js
@@ +14,5 @@
> +    for (var i = 0; i < a.length; i++)
> +        assertEq(a[i], b[i]);
> +}
> +function assertEquals(a, b) { assertEq(a, b); }
> +function assertSame(a, b) { assertEq(a, b); assertEq(a === b, true); }

assertEq(a, b) asserts that SameValue(a, b) is true, so these two are the same.

(It should have been called assertSame(), but I think we added it before ES5 introduced SameValue. The "eq" in assertEq was a Scheme reference; nobody gets it.)

@@ +67,5 @@
> +function TestGeneratorObjectPrototype() {
> +    assertSame(Object.prototype,
> +               Object.getPrototypeOf(GeneratorObjectPrototype));
> +    assertSame(GeneratorObjectPrototype,
> +               Object.getPrototypeOf((function*(){yield 1}).prototype));

FYI, the error message when assertEq(A, B) fails is "got A, expected B"
so the order of operands carries just the slightest whiff of meaning:
"the value of A should be B" rather than vice versa.

(If that makes no sense to you, *please* feel free to ignore it!)

@@ +97,5 @@
> +    assertTrue(f instanceof Function);  // Sanity check.
> +    assertTrue(!(f instanceof GeneratorFunction));
> +
> +    assertTrue((new GeneratorFunction()) instanceof GeneratorFunction);
> +    assertTrue(GeneratorFunction() instanceof GeneratorFunction);

This needs a lot more tests, if it's not already being tested elsewhere.

- that GeneratorFunction("yield 1; yield 2;") returns a generator function that you can call and it produces generator-iterators.
- that you can also pass argument names to it
- that 'yield' is not a valid argument name
- that GeneratorFunction(code).toString() does something sensible 
- that 'use strict' works

etc. etc. etc.

::: js/src/vm/GlobalObject.h
@@ +135,5 @@
>      inline void setProtoGetter(JSFunction *protoGetter);
>  
>      inline void setIntrinsicsHolder(JSObject *obj);
>  
> +  public:

I don't think they need to be public, since GlobalObject::initIteratorClasses is a member of this class. If that's correct, please leave them private.

Comment 6

[Brian Hackett [Laid off!]]

Comment on attachment 790179 [details] [diff] [review]
Implement prototype madness for ES6 generators v2

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

::: js/src/frontend/Parser.cpp
@@ +1987,5 @@
>          return pn;
>  
> +    RootedObject proto(context);
> +    if (generatorKind == StarGenerator) {
> +        // FIXME: Is this asJSContext() correct?

In general, calling asJSContext() isn't correct to do without doing an isJSContext() first.  And putting isJSContext() in parser code is bad if it affects whether the parse succeeds.  Right now we mainly do isJSContext() when reporting errors in the frontend because that happens differently on the main thread vs. off thread.  The best approach is:

- Update js::StartOffThreadParseScript so that the StarGeneratorFunctionPrototype is eagerly created in the original and new compartments, as is already done for the Function/Array/RegExp protos.  Eventually I want to make this lazier but whenever that happens it will encompass all these eagerly instantiated protos.

- At this spot, you can then get the StarGeneratorFunctionPrototype infallibly off the global if it !isJSContext(), and create it lazily if isJSContext().

- In WorkerThreadState::finishParseTaskForScript, you want to make sure that the final loop is able to correctly repoint prototype pointers from the new compartment's StarGeneneratorFunctionPrototype to the old compartment's one.  Since it looks like StarGeneneratorFunctionPrototype is a plain Object, I don't think this will work right as written and the functions' protos will end up being Object.prototype or something.  You could fix this by making StarGeneneratorFunctionPrototype a special case here or by using a new JSProtoKey for StarGeneneratorFunctionPrototype.

Currently you can only test off thread parsing of code using this functionality by loading the scripts from a XUL document.  Before too long though HTML <script> tags will also use off thread parsing (bug 906371).

Comment 7

[Andy Wingo [:wingo]]

Whew.  Thank you for slogging through that review!  I have an updated patch that addresses all the feedback.

Thanks for the comment about obj_toSource.  I had tried that first, but got a linking error because I forgot to add "js::" to obj_toSource when making it extern, so I fell back on that nasty code you reviewed.  However exposing obj_toSource is much better, and the patch will use this approach.

> assertEq(a, b) asserts that SameValue(a, b) is true, so these two are the
> same.
> 
> (It should have been called assertSame(), but I think we added it before ES5
> introduced SameValue. The "eq" in assertEq was a Scheme reference; nobody
> gets it.)

Funny; I'm a Schemer, and I assumed that assertEq always meant ===, as eq? is pretty much ===.  Will fix.

> @@ +97,5 @@
> > +    assertTrue(f instanceof Function);  // Sanity check.
> > +    assertTrue(!(f instanceof GeneratorFunction));
> > +
> > +    assertTrue((new GeneratorFunction()) instanceof GeneratorFunction);
> > +    assertTrue(GeneratorFunction() instanceof GeneratorFunction);
> 
> This needs a lot more tests, if it's not already being tested elsewhere.
> 
> - that GeneratorFunction("yield 1; yield 2;") returns a generator function
> that you can call and it produces generator-iterators.
> - that you can also pass argument names to it
> - that 'yield' is not a valid argument name
> - that GeneratorFunction(code).toString() does something sensible 
> - that 'use strict' works
> 
> etc. etc. etc.

Some of this is tested in syntax.js in ecma_6/Generators, from bug 666399.  We are limited as to what we can test currently because the ES6 semantics aren't implemented yet.  But yes, I will add more tests here; good idea.

> ::: js/src/jsiter.cpp
> @@ +1502,5 @@
> > +            return NULL;
> > +        JSObject *proto = pval.isObject() ? &pval.toObject() : NULL;
> > +        if (!proto) {
> > +            // FIXME
> > +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_ES6_UNIMPLEMENTED);

> Isn't this FIXME pretty easy to fix? The fallback proto is supposed to be global->getOrCreateStarGeneratorObjectPrototype(cx), right?

I don't know!  This case happens when you do:

  function*foo(){}
  delete foo.prototype;
  var iter = foo();
  iter.__proto__ // ???

I guess http://people.mozilla.org/~jorendorff/es6-draft.html#sec-9.3.15 describes what happens in this case, so it seems to be as you say. Will add some tests.

Comment 8

[Andy Wingo [:wingo]]

Attachment: Implement prototype madness for ES6 generators v2

Add a Generator constructor, like the Function constructor.

Define the GeneratorFunctionPrototype, GeneratorFunction, and
GeneratorObjectPrototype meta-objects.

When cloning or creating a star generator, give it
GeneratorFunctionPrototype as its prototype.

Each star generator function has a ".prototype" property, which has
GeneratorObjectPrototype as its prototype.  That prototype does not have
a ".constructor" link.

If Function.prototype.toSource is called on a non-function, chain up to
Object.prototype.toSource instead.

Prototypes of generator objects are no longer made with
GeneratorObject::class_.  This allows us to elide some "null" checks
from bug 352885.  Instead calling a generator on a method now signals a
typeerror.

Make the "send" generator method a simple alias to "next".

Comment 9

[Andy Wingo [:wingo]]

Comment on attachment 792741 [details] [diff] [review]
Implement prototype madness for ES6 generators v2

Attached patch fixes nits.

I allocated a JSProtoKey for STAR_GENERATOR_FUNCTION, to make the OMT pointer-swap easier.  Re-adding jorendorff for review, given significant patch churn there.

I also re-worked the test suite in terms of assertEq, and added GeneratorFunction tests.  I didn't add the "delete foo.prototype" tests in the end, because those need generator instantiation tests, and I was holding off on those until I get the iterator return value patches in, hopefully this week.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 792741 [details] [diff] [review]
Implement prototype madness for ES6 generators v2

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

r=me with the comment addressed

::: js/src/jsfun.cpp
@@ +827,5 @@
>      RootedObject obj(cx, ToObject(cx, args.thisv()));
>      if (!obj)
>          return false;
>  
> +    if (!obj->is<JSFunction>() && !obj->is<FunctionProxyObject>()) 

trailing whitespace here

::: js/src/jsworkers.cpp
@@ +241,5 @@
>      // pointers can be changed infallibly after parsing finishes.
>      if (!js_GetClassObject(cx, cx->global(), JSProto_Function, &obj) ||
>          !js_GetClassObject(cx, cx->global(), JSProto_Array, &obj) ||
> +        !js_GetClassObject(cx, cx->global(), JSProto_RegExp, &obj) ||
> +        !js_GetClassObject(cx, cx->global(), JSProto_GeneratorFunction, &obj))

I'd rather not add a JSProtoKey just for this, if
    !cx->global()->getOrCreateStarGeneratorFunctionPrototype(cx)
would be sufficient here. I think it would.

Comment 11

[Andy Wingo [:wingo]]

Thanks for the review.

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> ::: js/src/jsfun.cpp
> > +    if (!obj->is<JSFunction>() && !obj->is<FunctionProxyObject>()) 
> 
> trailing whitespace here

Yep, sorry bout that.  Will fix.

> ::: js/src/jsworkers.cpp
> @@ +241,5 @@
> >      // pointers can be changed infallibly after parsing finishes.
> >      if (!js_GetClassObject(cx, cx->global(), JSProto_Function, &obj) ||
> >          !js_GetClassObject(cx, cx->global(), JSProto_Array, &obj) ||
> > +        !js_GetClassObject(cx, cx->global(), JSProto_RegExp, &obj) ||
> > +        !js_GetClassObject(cx, cx->global(), JSProto_GeneratorFunction, &obj))
> 
> I'd rather not add a JSProtoKey just for this, if
>     !cx->global()->getOrCreateStarGeneratorFunctionPrototype(cx)
> would be sufficient here. I think it would.

I added the JSProtoKey not for this but for WorkerThreadState::finishParseTaskForScript, as bhackett noted above.  It seemed better to do that than add more special cases there.  Will land as-is unless you think that there is a better option.

Comment 12

[Andy Wingo [:wingo]]

Attachment: Implement prototype madness for ES6 generators v3

Add a Generator constructor, like the Function constructor.

Define the GeneratorFunctionPrototype, GeneratorFunction, and
GeneratorObjectPrototype meta-objects.

When cloning or creating a star generator, give it
GeneratorFunctionPrototype as its prototype.

Each star generator function has a ".prototype" property, which has
GeneratorObjectPrototype as its prototype.  That prototype does not have
a ".constructor" link.

If Function.prototype.toSource is called on a non-function, chain up to
Object.prototype.toSource instead.

Prototypes of generator objects are no longer made with
GeneratorObject::class_.  This allows us to elide some "null" checks
from bug 352885.  Instead calling a generator on a method now signals a
typeerror.

Make the "send" generator method a simple alias to "next".

Comment 13

[Andy Wingo [:wingo]]

Comment on attachment 793989 [details] [diff] [review]
Implement prototype madness for ES6 generators v3

Humm, forgot to include a whitespace change.  Updating again...

Comment 14

[Andy Wingo [:wingo]]

Attachment: Implement prototype madness for ES6 generators v4

r=jorendorff

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to Andy Wingo from comment #11)
> I added the JSProtoKey not for this but for
> WorkerThreadState::finishParseTaskForScript, as bhackett noted above.  It
> seemed better to do that than add more special cases there.

OK, sounds good.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a182052fafe

Comment 17

[Ed Morley [:emorley]]

Backed out for warnings as errors failures, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=26872091&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=26871878&tree=Mozilla-Inbound

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

Comment 18

[Andy Wingo [:wingo]]

Attachment: Implement prototype madness for ES6 generators v5

Yep, sorry 'bout that.  Attached patch fixes this build error.  r=jorendorff as before.

Comment 19

[Andy Wingo [:wingo]]

After discussion, running a try build seems to be the thing.  Ms2ger is on it.

Comment 20

[Andy Wingo [:wingo]]

https://tbpl.mozilla.org/?tree=Try&rev=f7defa1c8366

Comment 21

[Andy Wingo [:wingo]]

Tinderbox appears to be green

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32e6af3f6a05

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32e6af3f6a05

Comment 24

[Tooru Fujisawa [:arai]]

Updated following documents:
  https://developer.mozilla.org/en-US/Firefox/Releases/26
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/GeneratorFunction
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/GeneratorFunction/prototype