918083 - Disable function* parsing on aurora

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

That function* parses, and semi-works, is likely to lead to misapprehensions about its correctness, stability, etc.  I think we should consider turning it and yield* parsing off until the semantics are fully implemented, on branches.  This would also mean issues like bug 918075 (mis-implementation of the iteration protocol) won't poison the well, either for developer understanding or for general use on the web.

Comment 1

[Jason Orendorff [:jorendorff]]

I think we have to do this. I'll take it in a few days, unless Waldo takes it first.

Comment 2

[Andy Wingo [:wingo]]

After bug 666396 lands (the yield* work), there are only two things wrong with the generators implementation:

  * https://bugs.ecmascript.org/show_bug.cgi?id=1901
  * That yield* doesn't call @@iterator on the RHS.

Both are minor issues.  The latter is fixed in bug 907077, for which I hope to have an updated patch today.  So TBH I don't understand the hesitation here.

Comment 3

[Jason Orendorff [:jorendorff]]

Andy, the current, half-finished state of things got onto the train for FF26. We definitely don't want that to ship.

We do want this stuff in FF27, but it's got to be #ifdef'd out of FF26.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Potential patch

I just remembered this on the train this morning, decided it better get done now or we'll forget before uplift, which...wouldn't be the end of the world, but is clearly not desirable.

Obviously this needs a whole ton of test-disabling too, but that part maybe doesn't need review so much, and/or is something I can Schlemiel-the-painter my way through in parallel with reviewing.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 817180 [details] [diff] [review]
Potential patch

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

r=me. Patch is for mozilla-aurora only.

Comment 6

[Jeff Walden [:Waldo]]

https://tbpl.mozilla.org/?tree=Try&rev=cfcb1dfbc079

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Patch with test changes

Most of the test changes were to JS tests, easily done with skips or commenting-out or whatever.  But it seems like this stuff got adopted too quickly (I think it's a seriously bad idea to start adopting something when it's only half-implemented, particularly because of cases like this!), so we have non-js-test uses to fix up.  I...think I hacked things into submission on that front.  Yoric can tell me if I did anything wrong with this, I'm sure.  :-)

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 819127 [details] [diff] [review]
Patch with test changes

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

Such a shame, I was so eager to use it.

Comment 9

[Brandon Benvie [:benvie]]

The changes to test_task.js aren't really useful. Those tests are already just copies of tests for legacy generators (or test the interaction between legacy and star generators). It would be better to just comment them out for now than to alter them to (re-)test legacy generators.

Comment 10

[Brandon Benvie [:benvie]]

If this is something that will be become more commonplace in the future, would it be possible to put things that won't immediately be available in Aurora behind a flag (like what V8 does) or something to that effect? In order to allow it to be worked on and tested, but to prevent Mozilla developers from inadvertently using the feature. I had followed the development of ES6 generators in SM pretty closely and still managed to miss this bug, and didn't realize this feature would be disabled.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 819127 [details] [diff] [review]
Patch with test changes

[Approval Request Comment]
Bug caused by (feature/regressing bug #): incomplete implementation of function*/yield*
User impact if declined: exposing a buggy, incomplete feature in beta before it's ready
Testing completed (on m-c, etc.): tests disabled in it failed with the substantive changes, pass with
Risk to taking this patch (and alternatives if risky): basically none, just shunting away from certain code paths; could break stuff that's using function* already, but really we don't *want* them using it yet
String or IDL/UUID changes made by this patch: N/A

Comment 12

[Jeff Walden [:Waldo]]

(In reply to Brandon Benvie [:bbenvie] from comment #10)
> If this is something that will be become more commonplace in the future,
> would it be possible to put things that won't immediately be available in
> Aurora behind a flag (like what V8 does) or something to that effect?

Hm, possibly.  Here you got bitten by implementation taking a little longer than a cycle.  We could maybe do flags, but for that short a duration it seems ponderous.  Maybe better communication of "it's ready" is more in order for this.

> In order to allow it to be worked on and tested, but to prevent Mozilla
> developers from inadvertently using the feature. I had followed the
> development of ES6 generators in SM pretty closely and still managed to miss
> this bug, and didn't realize this feature would be disabled.

Better communication of "go!" would solve this, I think.  jorendorff may have ideas for how to make this work, and/or how to do the flags approach.  Me, I think flags for simple-ish things are too heavyweight, but I don't feel super-strongly about it.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Triage drive-by, still waiting for this to land to Nightly before considering the uplift request.

Comment 14

[Jason Orendorff [:jorendorff]]

This patch should land in Aurora. We don't want to disable this feature in Nightly, only in Aurora.

Sorry if I am not doing this right. I thought the ability to disable things that weren't ready by landing minimal patches directly to Aurora was an intentional feature of the train model.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 819127 [details] [diff] [review]
Patch with test changes

Yes, it's find to do that to disable something - would have been good to point out in the approval nomination comments otherwise the status 27 affected makes it look like it needs to land there as well by default.

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a3b1b556e054

Comment 17

[bhavana bajaj [:bajaj]]

NI on :jorendorff/:Waldo to help understand if the feature is ready to ship in Firefox 27(current Beta) or needs disabling like in Firefox 26 ?

Comment 18

[Jason Orendorff [:jorendorff]]

This is ready to ship in 27.