Not sure if we should really do this, because as far as I can tell there is no other way to find out if a function is a generator in ES6.

I guess we can check whether the given function is ES6 generator or not with following code. > let GeneratorFunction = Object.getPrototypeOf(function*(){}).constructor; > let isGenerator = f => f instanceof GeneratorFunction; > isGenerator(function(){}); false > isGenerator(function*(){}); true But it's different than Function.prototype.isGenerator with legacy generator. > isGenerator(function(){yield 1}); false So, if there is any other way to detect legacy generator (or after the removal of legacy generator support), we can remove Function.prototype.isGenerator :)

Currently Function.prototype.isGenerator is used in addon-sdk and jstests (and might be used in some add-ons). https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/test.js#52 > if (test.isGenerator && test.isGenerator()) { https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/util/sequence.js#29 > if (iterator.isGenerator && iterator.isGenerator()) https://dxr.mozilla.org/mozilla-central/source/js/src/tests/js1_8/genexps/regress-665286.js#28 > reportCompare(reported.isGenerator(), true, "reported case: is generator"); > reportCompare(simplified1.isGenerator(), true, "simplified case 1: is generator"); > reportCompare(simplified2.isGenerator(), true, "simplified case 2: is generator"); https://dxr.mozilla.org/mozilla-central/source/js/src/tests/js1_8/genexps/regress-666852.js#17 > reportCompare(reported.isGenerator(), true, "reported case: is generator"); https://dxr.mozilla.org/mozilla-central/source/js/src/tests/js1_8/genexps/regress-667131.js#14 > reportCompare(f.isGenerator(), true, desc + ": is generator"); https://dxr.mozilla.org/mozilla-central/source/js/src/tests/js1_8_5/extensions/is-generator.js#10 > reportCompare(true, "isGenerator" in Function.prototype, "Function.prototype.isGenerator present"); > ... A jit-test also uses it, but it's not required to be `isGenerator`, so we can just replace it with some other property. https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/jaeger/bug704138.js#6 > reportCompare(true, "isGenerator" in Function, "Function.prototype.isGenerator present"); Then, to remove Function.prototype.isGenerator before removing legacy generator, I have following 4 plans. (Of course, if we can just remove Function.prototype.isGenerator, it's the simplest solution...) Plan A: Move `isGenerator` method to legacy generator instance, We can detect legacy generator by calling it (or just by the existence): if ("isGenerator" in f && f.isGenerator()) ... https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc46976efe62 Plan B: Define `isLegacyGenerator` property in legacy generator instance. We can detect legacy generator by the value (or just by the existence): if ("isLegacyGenerator" in f && f.isLegacyGenerator) ... This might be better than Plan A, because this prevents following bad workaround. let isGenerator = (function(){yield}).isGenerator; if (isGenerator.call(f)) ... Plan C: Add `LegacyGeneratorFunction` constructor, like `GeneratorFunction` for ES6 generator, We can detect legacy generator by following code, like comment #1: let LegacyGeneratorFunction = Object.getPrototypeOf(function(){yield}).constructor; if (f instanceof LegacyGeneratorFunction) ... https://treeherder.mozilla.org/#/jobs?repo=try&revision=998ed72422d7 Plan D: Add `IsLegacyGenerator` testing function to js shell, and use it in jstests. So, in jstests, we can detect legacy generator by it: if (IsLegacyGenerator(f)) ... But this plan has a problem with addon-sdk, if addon-sdk should handle legacy generator, this cannot be applied. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1a0149f4c9a In any way, we can remove non-standard method from standard class.

I wonder if it's worth bringing this up on es-discuss. It might not be, though. The only non-test usage (from the addon-sdk's Sequence utility) looks to me like it shouldn't use `isGenerator`: the fact that the iterator is implemented as a generator should be treated as an implementation detail. IIUC, the temptation to do these narrow checks is precisely why people are opposed to having `isFoo`-type methods.

Not sure the benefit of making it the standard :/ Anyway, for addon-sdk's case, we can test the returned value. Plan D': Check the returned value from the function call, and test the property (.next method), instead of using Function.prototype.isGenerator. https://tbpl.mozilla.org/?tree=Try&rev=88784e230164 https://treeherder.mozilla.org/#/jobs?repo=try&revision=88784e230164 (not working now?)

bug 993389 complains a problem around isGenerator. js> (function*(){}).bind(this).isGenerator() false So checking returned value would also be better for this aspect.

> let GeneratorFunction = Object.getPrototypeOf(function*(){}).constructor; > let isGenerator = f => f instanceof GeneratorFunction; Sadly, it doesn't work if f comes from different global :/

now that bug 1140759 is marked as WONTFIX, and we can just remove it at the same time as legacy generator (bug 1083482)

Reopening as suggested in bug 1083482 comment 22.

Linux64 looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=052744d9467691b72cdee58325a8ee28efbb02c8&group_state=expanded

Comment on attachment 8924697 [details] [diff] [review] Patch Review of attachment 8924697 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! it would be better asking extra review for DeferredTask.jsm

Comment on attachment 8924697 [details] [diff] [review] Patch Kris can review the DeferredTask.jsm change? That line was added in https://hg.mozilla.org/mozilla-central/rev/8154fcbcb707191b3526ff10d78a1ea36550213b

Er, *can you review...

Comment on attachment 8924697 [details] [diff] [review] Patch Review of attachment 8924697 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/DeferredTask.jsm @@ +118,5 @@ > this._taskFn = aTaskFn; > this._delayMs = aDelayMs; > this._timeoutMs = aIdleTimeoutMs; > > + if (Object.prototype.toString.call(aTaskFn) === "[object GeneratorFunction]") { I'd rather we use `ChromeUtils.getClassName(aTaskFn) === "GeneratorFunction"`. Object.prototype.toString is pretty expensive. But it looks like that just returns "Function", so let's just remove this check entirely. It shouldn't be necessary anymore.

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a19aef803cdb Remove non-standard Function.prototype.isGenerator. r=arai,kmag

Developer release notes https://developer.mozilla.org/en-US/Firefox/Releases/58#JavaScript Reference page https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/isGenerator Compat data https://github.com/mdn/browser-compat-data/pull/602

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/legacy-generator-support-has-been-removed/