1140759 - Fix in tree consumers that use non-standard Function.prototype.isGenerator in Add-on SDK.

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Tooru Fujisawa [:arai]]

Function.prototype.isGenerator is non-standard, and I'd like to remove it in bug 1119777.

For ES6 generator, it could be tested by `f instanceof GeneratorFunction` where `GeneratorFunction = Object.getPrototypeOf(function*(){}).constructor`, so if Add-on SDK does not support legacy generator (or after legacy generator support was dropped from SpiderMonkey), isGenerator() in lib/sdk/lang/type.js
 could be rewritten by that.

Also, normal function can behave like generator, by returning iterable object, so it might be better to test returned value instead of function itself.

Comment 1

[Tooru Fujisawa [:arai]]

Eric, how do you think?

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Tooru Fujisawa [:arai] from comment #0)
> Function.prototype.isGenerator is non-standard, and I'd like to remove it in
> bug 1119777.
> 
> For ES6 generator, it could be tested by `f instanceof GeneratorFunction`
> where `GeneratorFunction =
> Object.getPrototypeOf(function*(){}).constructor`, so if Add-on SDK does not
> support legacy generator (or after legacy generator support was dropped from
> SpiderMonkey), isGenerator() in lib/sdk/lang/type.js
>  could be rewritten by that.

This change sounds find to me.

> Also, normal function can behave like generator, by returning iterable
> object, so it might be better to test returned value instead of function
> itself.

I'm not sure we should go this far just yet for the implementation of isGenerator() in lib/sdk/lang/type.js.  I think we should hold off on this latter change until it seems necessary, we may just create a separate function to handle those two use cases.

Comment 3

[Tooru Fujisawa [:arai]]

Add-on SDK still support legacy generator, right? (it seems there are a few legacy generators in addon-sdk source. not sure about actual addon's code though)
if so, is there any plan to deprecate or drop support for them near future?
can we show deprecation warning inside isGenerator function, like following?

  let GeneratorFunction = Object.getPrototypeOf(function*(){}).constructor;
  function isGenerator(aValue) {
    if (!aValue) {
      return false;
    }
    if (aValue instanceof GeneratorFunction) {
      return true;
    }
    if (aValue.isGenerator && aValue.isGenerator()) {
      deprecateUsage("JS1.7 legacy generator is deprecated; consider using ES6 generator instead");
      return true;
    }
    return false;
  }

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Tooru Fujisawa [:arai] from comment #3)
> Add-on SDK still support legacy generator, right? (it seems there are a few
> legacy generators in addon-sdk source. not sure about actual addon's code
> though)
> if so, is there any plan to deprecate or drop support for them near future?
> can we show deprecation warning inside isGenerator function, like following?
> 
>   let GeneratorFunction = Object.getPrototypeOf(function*(){}).constructor;
>   function isGenerator(aValue) {
>     if (!aValue) {
>       return false;
>     }
>     if (aValue instanceof GeneratorFunction) {
>       return true;
>     }
>     if (aValue.isGenerator && aValue.isGenerator()) {
>       deprecateUsage("JS1.7 legacy generator is deprecated; consider using
> ES6 generator instead");
>       return true;
>     }
>     return false;
>   }

This function is exported from a low level modules so we don't need to worry about which add-ons use it.  I think we should just removed it from our code if it is deprecated, no need to keep it around with a deprecation message.

Comment 5

[Tooru Fujisawa [:arai]]

wow, I tried with throwing Error instead of warning, and it throws so much errors in try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e14783b914c

they should be fixed before landing isGenerator change.

Comment 6

[Tooru Fujisawa [:arai]]

Hmm, `aValue instanceof GeneratorFunction` returns false even if aValue is ES6 generator.
maybe a issue related to cross-compartment or something?

Comment 7

[Tooru Fujisawa [:arai]]

more robust (but somehow incorrect) way here
  Object.getPrototypeOf(aValue).constructor.name === "GeneratorFunction"

similar checking logic is used in isArray/isArguments/isMap/isSet.

Comment 8

[Tooru Fujisawa [:arai]]

With isGenerator impl in comment #7, a few tests failed.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1daa6465d28c

Following 3 tests should be updated to use ES6 generator.
  https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-addon-bootstrap.js
  https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-event-core.js
  https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-event-dom.js

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Use ES6 generator instead of legacy generator in some tests. / Part 2: Check only GeneratorFunction in isGenerator.

Fixed 3 tests, and make isGenerator not to use Function.prototype.isGenerator.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9639ebe9b8d2

I wonder if this change also breaks tests in add-ons which use Add-on SDK, in same way as 3 tests in Part 1.
If so, feel free to reject this patch. It should be better to notify add-on authors before landing this, or add warning message and keep previous behavior.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8583749 [details] [review]
Part 1: Use ES6 generator instead of legacy generator in some tests. / Part 2: Check only GeneratorFunction in isGenerator.

There appear to be some unnecessary changes in the pr, please see the pull request for more details.

Comment 11

[Tooru Fujisawa [:arai]]

Sorry, my explanation wasn't clear.

Those 3 changes in Part 1 are required before landing Part 2. because those functions are passed to isGenerator here:

https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/test.js#52
>          if (isGenerator(test)) {
>            options.waitUntilDone();
>            Task.spawn(test.bind(null, assert)).
>                catch(assert.fail).
>                then(assert.end);
>          }

Without those changes, they're legacy generator, and updated isGenerator returns false for them, and they won't be executed correctly.

So, if same test harness is shared with add-ons, change in Part 2 also affects their tests. that's what I concern.

Comment 12

[Tooru Fujisawa [:arai]]

maybe WONTFIX now? (since the whole sdk is getting removed)

Comment 13

[Andy McKay]

Once the SDK is removed we'll basically won't fix everything in this product and graveyard through a script.