Closed Bug 1191486 Opened 9 years ago Closed 9 years ago

Generators should not have [[Construct]]

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jorendorff, Assigned: mrrrgn)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [mentor=jorendorff][DocArea=JS])

Attachments

(2 files, 7 obsolete files)

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!
Blocks: es6
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);
Whiteboard: [mentor=jorendorff]
Giving this a go.
Assignee: nobody → winter2718
Attached patch spidermonkey-generators.diff (obsolete) — Splinter Review
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.
Attachment #8648802 - Flags: feedback?(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.
Attachment #8648802 - Flags: feedback?(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);
Attached patch spidermonkey-generators.diff (obsolete) — Splinter Review
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=297df63ff3fd
Attachment #8648802 - Attachment is obsolete: true
Attachment #8649573 - Flags: review?(jorendorff)
Attached patch spidermonkey-generators.diff (obsolete) — Splinter Review
Attachment #8649573 - Attachment is obsolete: true
Attachment #8649573 - Flags: review?(jorendorff)
Attachment #8649866 - Flags: review?(jorendorff)
Attached patch spidermonkey-generators.diff (obsolete) — Splinter Review
** formatting change
Attachment #8649866 - Attachment is obsolete: true
Attachment #8649866 - Flags: review?(jorendorff)
Attachment #8649869 - Flags: review?(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?
Attachment #8649869 - Flags: review?(jorendorff) → review+
Attached patch spidermonkey-generators.diff (obsolete) — Splinter Review
Updated patch with try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df3e8921af56 will push to inbound if this passes.
Attachment #8649869 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/09db2e7860ed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This also makes expressions like: var g = function* (){} non-constructors.
Attachment #8650722 - Flags: review?(jorendorff)
try push: plication to mirrors completed successfully in 12.9s
Back to the drawing board *sad trumpet* -- sorry for my misunderstanding here.
Attachment #8649998 - Attachment is obsolete: true
Attachment #8650722 - Attachment is obsolete: true
Attachment #8650722 - Flags: review?(jorendorff)
Attachment #8653800 - Flags: review?(jorendorff)
Comment on attachment 8653800 [details] [diff] [review]
backout-generators.diff

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

Thanks. r=me.
Attachment #8653800 - Flags: review?(jorendorff) → review+
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.
Attachment #8655288 - Flags: review?(jorendorff)
https://hg.mozilla.org/mozilla-central/rev/b4d5deb4eb38
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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.
Attachment #8655288 - Flags: review?(jorendorff) → review+
Attachment #8655288 - Attachment is obsolete: true
Blocks: es7
No longer blocks: es6
Keywords: dev-doc-needed
Whiteboard: [mentor=jorendorff] → [mentor=jorendorff][DocArea=JS]
You need to log in before you can comment on or make changes to this bug.