1191486 - Generators should not have [[Construct]]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

According to TC39's 28 July 2015 meeting notes:
> ## 6.7 new & GeneratorFunction 
> 
> ...
> 
> AK (offline): Further spec reading suggests that GeneratorMethods need to have their [[Construct]] removed just like GeneratorFunctions (they both pass "generator" to FunctionInitialize, which is where the decision to set up [[Construct]] is made).
> 
> #### Conclusion/Resolution  
> 
> - Maintain spec that cannot construct generator methods (or change spec to make them non-constructible, see above)
> - Spec change: generator function do not have a [[construct]] trap, so `new` throws
> - Call a generator, `this` should be set by the caller, as is normal (global or undefined)
> - File Errata

Glad to see this change!

Comment 1

[Jason Orendorff [:jorendorff]]

The bug here is that we currently allow this, which makes no sense and should be a TypeError:

    function* f() {}
    var obj = new f;  // this should throw

So a test for this would be something like:

    function* f() {}
    assertThrowsInstanceOf(() => new f, TypeError);

    var obj2 = {
        *method(x) { yield x; }
    };
    assertThrowsInstanceOf(() => new obj2.method(0), TypeError);

    reportCompare(0, 0);

Comment 2

[Morgan Phillips [:mrrrgn]]

Giving this a go.

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: spidermonkey-generators.diff

Just looking for feedback here. I spent time walking through the interpreter with gdb until I found this codepath where generators could be detected during a "new" throw. Because I used a greedy approach, and don't know the codebase well yet, I suspect there is a more sensible way to do this.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8648802 [details] [diff] [review]
spidermonkey-generators.diff

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

Clearing review. This only solves the problem when you say `new g` where g is a generator.

There are other places where the [[Construct]] internal method can be called, including Reflect.construct. What you really want is for generators to hit this error path in js::InvokeConstructor:

    JSNative construct = callee.constructHook();
    if (!construct)
        return ReportIsNotFunction(cx, args.calleev(), args.length() + 2, CONSTRUCT);

that is, changing generators to not have this internal method at all.

Comment 5

[Jason Orendorff [:jorendorff]]

Whoa, correction, generator functions are just regular functions, so the error path you want to hit in js::InvokeConstructor is this one:

        if (!fun->isConstructor())
            return ReportIsNotFunction(cx, args.calleev(), args.length() + 2, CONSTRUCT);

Comment 6

[Morgan Phillips [:mrrrgn]]

Attachment: spidermonkey-generators.diff

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=297df63ff3fd

Comment 7

[Morgan Phillips [:mrrrgn]]

Attachment: spidermonkey-generators.diff

Comment 8

[Morgan Phillips [:mrrrgn]]

Attachment: spidermonkey-generators.diff

** formatting change

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8649869 [details] [diff] [review]
spidermonkey-generators.diff

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

r=me with the following minor comments addressed. Thanks for the patch, Morgan!

Next, it needs to go to try server---I guess you know how this goes from here on out? If you have any questions, ping me on IRC.

::: js/src/frontend/Parser.cpp
@@ +1305,5 @@
>          break;
>        case Method:
>          MOZ_ASSERT(generatorKind == NotGenerator || generatorKind == StarGenerator);
> +        flags = (generatorKind == NotGenerator ?
> +                 JSFunction::INTERPRETED_METHOD : JSFunction::INTERPRETED_METHOD_GENERATOR);

Thanks. Style nit: per SpiderMonkey's house style, please write it like this:

    flags = (generatorKind == NotGenerator
             ? JSFunction::INTERPRETED_METHOD
             : JSFunction::INTERPRETED_METHOD_GENERATOR);

and the same below.

::: js/src/tests/ecma_6/Class/methodsPrototype.js
@@ +18,5 @@
>  ]
>  
>  for (var fun of hasPrototype) {
>      assertEq(fun.hasOwnProperty('prototype'), true);
>  }

Feel free to collapse lines 16-22 to a single line:

    assertEq(test.constructor.hasOwnProperty('prototype'), true);

::: js/src/tests/ecma_6/Generators/runtime.js
@@ -22,5 @@
> -// A generator function should have the same set of properties as any
> -// other function.
> -function TestGeneratorFunctionInstance() {
> -    var f_own_property_names = Object.getOwnPropertyNames(f);
> -    var g_own_property_names = Object.getOwnPropertyNames(g);

It seems like this whole test could be kept, except with some extra code here to remove "prototype" from f_own_property_names:

    f_own_property_names.splice(f_own_property_names.indexOf("prototype"), 1);

And of course a change to the comment.

Any reason not to keep it?

Comment 10

[Morgan Phillips [:mrrrgn]]

Attachment: spidermonkey-generators.diff

Updated patch with try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df3e8921af56 will push to inbound if this passes.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09db2e7860ed

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/09db2e7860ed

Comment 13

[Morgan Phillips [:mrrrgn]]

Attachment: spidermonkey-generators-part2.diff

This also makes expressions like: var g = function* (){} non-constructors.

Comment 14

[Morgan Phillips [:mrrrgn]]

try push: plication to mirrors completed successfully in 12.9s

Comment 15

[Morgan Phillips [:mrrrgn]]

try push (take two) ^.^ : https://treeherder.mozilla.org/#/jobs?repo=try&revision=43bb876cfbc2

Comment 16

[Morgan Phillips [:mrrrgn]]

Attachment: backout-generators.diff

Back to the drawing board *sad trumpet* -- sorry for my misunderstanding here.

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 8653800 [details] [diff] [review]
backout-generators.diff

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

Thanks. r=me.

Comment 18

[Pulsebot]

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

Comment 19

[Morgan Phillips [:mrrrgn]]

Attachment: spidermonkey-generators-with-prototype.diff

I was able to maintain having prototypes on generators by tweaking fun_resolve. I modified a comment, but wasn't confident in its clarity: 

"* In ES6 9.2.8 MakeConstructor the .prototype property is only assigned
 * to constructors and generators."

Otherwise, tests are passing; new throws and (function *(){}).hasOwnProperty("prototype") is true. I'll give this a second look in the morning to make sure I didn't goof something.

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 8655288 [details] [diff] [review]
spidermonkey-generators-with-prototype.diff

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

Good work. Thanks, r=me.

::: js/src/jsfun.cpp
@@ +433,5 @@
>           * isBuiltin() test covers this case because bound functions are native
>           * (and thus built-in) functions by definition/construction.
>           *
>           * In ES6 9.2.8 MakeConstructor the .prototype property is only assigned
> +         * to constructors and generators.

Great - let's say this:

    * ES6 9.2.8 MakeConstructor defines the .prototype property on constructors.
    * Generators are not constructors, but they have a .prototype property anyway,
    * according to errata to ES6. See bug 1191486.

@@ +445,1 @@
>              return true;

The way this is written seems upside down to me. If you think so too, please rewrite it to be like this:

    if (!fun->isBuiltin() && (fun->isConstructor() || fun->isGenerator())) {
        ... resolve .prototype ...
    }
    return true;

A separate commit is fine, r=me.

Comment 22

[Morgan Phillips [:mrrrgn]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5ee84daaf72

Comment 23

[Morgan Phillips [:mrrrgn]]

Attachment: spidermonkey-generators-quatre.diff

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e6e91688937

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e6e91688937

Comment 26

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/43#JavaScript

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*#Generators_are_not_constructable
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions