Tom S [:evilpie] Reporter
	Description • 9 years ago

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.

	Tooru Fujisawa [:arai]
	Comment 1 • 9 years ago

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 :)

	Tooru Fujisawa [:arai]
	Comment 2 • 9 years ago

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.

	Till Schneidereit [:till]
	Comment 3 • 9 years ago

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.

	Tooru Fujisawa [:arai]
	Comment 4 • 9 years ago

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?)

	Tooru Fujisawa [:arai]
	Comment 5 • 9 years ago

bug 993389 complains a problem around isGenerator.

js> (function*(){}).bind(this).isGenerator()
false

So checking returned value would also be better for this aspect.

	Tooru Fujisawa [:arai]
	Comment 6 • 9 years ago

> let GeneratorFunction = Object.getPrototypeOf(function*(){}).constructor;
> let isGenerator = f => f instanceof GeneratorFunction;

Sadly, it doesn't work if f comes from different global :/

	Tooru Fujisawa [:arai]
	Comment 7 • 7 years ago

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

	Jan de Mooij [:jandem] Assignee
	Comment 9 • 7 years ago

Reopening as suggested in bug 1083482 comment 22.

	Jan de Mooij [:jandem] Assignee
	Comment 10 • 7 years ago

Linux64 looks green:

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

	Tooru Fujisawa [:arai]
	Comment 11 • 7 years ago

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

	Jan de Mooij [:jandem] Assignee
	Comment 12 • 7 years ago

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

	Jan de Mooij [:jandem] Assignee
	Comment 13 • 7 years ago

Er, *can you review...

	Kris Maglione [:kmag]
	Comment 14 • 7 years ago

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.

	Pulsebot
	Comment 15 • 7 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 16 • 7 years ago
bugherder

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

	Florian Scholz (Open Web Docs)
	Comment 17 • 7 years ago

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

	Kohei Yoshino [:kohei]
	Comment 18 • 7 years ago

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