Closed
Bug 1191486
Opened 8 years ago
Closed 8 years ago
Generators should not have [[Construct]]
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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)
10.56 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
10.37 KB,
patch
|
Details | Diff | Splinter Review |
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!
Reporter | ||
Comment 1•8 years ago
|
||
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]
Assignee | ||
Comment 2•8 years ago
|
||
Giving this a go.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → winter2718
Assignee | ||
Comment 3•8 years ago
|
||
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)
Reporter | ||
Comment 4•8 years ago
|
||
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)
Reporter | ||
Comment 5•8 years ago
|
||
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);
Assignee | ||
Comment 6•8 years ago
|
||
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=297df63ff3fd
Attachment #8648802 -
Attachment is obsolete: true
Attachment #8649573 -
Flags: review?(jorendorff)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8649573 -
Attachment is obsolete: true
Attachment #8649573 -
Flags: review?(jorendorff)
Attachment #8649866 -
Flags: review?(jorendorff)
Assignee | ||
Comment 8•8 years ago
|
||
** formatting change
Attachment #8649866 -
Attachment is obsolete: true
Attachment #8649866 -
Flags: review?(jorendorff)
Attachment #8649869 -
Flags: review?(jorendorff)
Reporter | ||
Comment 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
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: 8 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee | ||
Updated•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 13•8 years ago
|
||
This also makes expressions like: var g = function* (){} non-constructors.
Attachment #8650722 -
Flags: review?(jorendorff)
Assignee | ||
Comment 14•8 years ago
|
||
try push: plication to mirrors completed successfully in 12.9s
Assignee | ||
Comment 15•8 years ago
|
||
try push (take two) ^.^ : https://treeherder.mozilla.org/#/jobs?repo=try&revision=43bb876cfbc2
Assignee | ||
Comment 16•8 years ago
|
||
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)
Reporter | ||
Comment 17•8 years ago
|
||
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+
Assignee | ||
Comment 19•8 years ago
|
||
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)
Comment 20•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b4d5deb4eb38
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 21•8 years ago
|
||
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+
Assignee | ||
Comment 22•8 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5ee84daaf72
Assignee | ||
Comment 23•8 years ago
|
||
Attachment #8655288 -
Attachment is obsolete: true
Comment 25•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0e6e91688937
Flags: in-testsuite+
Keywords: dev-doc-needed
See Also: → https://bugs.webkit.org/show_bug.cgi?id=152383
Whiteboard: [mentor=jorendorff] → [mentor=jorendorff][DocArea=JS]
Comment 26•8 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•