1115868 - Implement Generator.prototype.return

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[ziyunfei]

function* gen() { 
  yield 1;
  yield 2;
  yield 3;
}

var g = gen();

g.next(); // { value: 1, done: false }
g.return("foo"); // { value: "foo", done: true }
g.next(); // { value: undefined, done: true }

Comment 1

[Jan de Mooij [:jandem]]

return() is like the legacy generator's close() method, so hopefully this should be pretty easy to implement. There might be minor differences though; we should double check.

When we do this we should probably rename some internal things as well, s/close/return/, s/closing/returning/ etc.

Comment 2

[Jan de Mooij [:jandem]]

With this fixed we'll score 12/12 for Generators on http://kangax.github.io/compat-table/es6/

I can fix soon but please feel free to steal!

Comment 3

[Jan de Mooij [:jandem]]

Attachment: WIP

This seems to work. We should probably still rename some things from "closing" to "returning" though.

Legacy generators have this thing where yielding in a finally block while closing the generator throws an exception. I think ES6 generators allow this so the patch just removes the code that throws this exception; should probably move this change to a separate bug. I doubt anybody relies on the exception being thrown for legacy generators.

I think this patch does the right thing in all cases, but the spec for this is really hard to understand.

Comment 4

[Jan de Mooij [:jandem]]

I'm not sure what should happen here:

function *gen() {
    try {
	yield 1;
	yield 2;
    } finally {
	yield 3;
	yield 4;
    }
}
var it = gen();
it.next();    // {value 1, done: false}
it.return(5); // {value: 3, done: false}   ??
it.next();    // {value: 4, done: false}   ??
it.next();    // {value: 5, done: true}    ??

If the last one should indeed yield 5, that would be pretty annoying to implement.

Comment 5

[Andy Wingo [:wingo]]

One option regarding comment 4 would be if "finally" is implemented via code duplication as in 965717 and every "yield" point gets an associated duplicated finally block.  Pretty nasty though.

Comment 6

[Andy Wingo [:wingo]]

bug 965717 rather

Comment 7

[Jan de Mooij [:jandem]]

I used André Bargull's ES6 implementation [0] to confirm the behavior in comment 4 is indeed correct. I also ran the tests I wrote through it to confirm the behavior on those is the same. Incredibly useful to be able to compare to another implementation :)

New patch with updated tests coming soon.

[0] https://github.com/anba/es6draft

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch

This patch handles the case in comment 4 by temporarily storing the return value in the .genrval CallObject slot that was added a while ago, and retrieving it when we do the actual forced return from the generator frame.

We could also store the value in a slot on the generator itself, but since we already have a CallObject slot I thought we might as well use that one.

This is a bit hackish but I can't think of a better/simpler way to do this. Suggestions welcome.

The patch also changes a GetElements call to a PodCopy: if we yield in a finally block, we will have a MagicValue(JS_GENERATOR_CLOSING) on the stack and GetElements asserts in this case, the PodCopy avoids that (and should also be a bit faster).

Comment 9

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> I used André Bargull's ES6 implementation [0] to confirm the behavior in
> comment 4 is indeed correct. I also ran the tests I wrote through it to
> confirm the behavior on those is the same. Incredibly useful to be able to
> compare to another implementation :)

Yay! :-D

If you need additional tests for Generator.prototype.return, feel free to use the following ones:

https://github.com/anba/es6draft/blob/master/src/test/scripts/suite/semantic/generator/return_catch_finally.js
https://github.com/anba/es6draft/blob/master/src/test/scripts/suite/semantic/generator/return_finally.js

Comment 10

[Jan de Mooij [:jandem]]

(In reply to André Bargull from comment #9)
> Yay! :-D
> 
> If you need additional tests for Generator.prototype.return, feel free to
> use the following ones:

Oh nice, I didn't see tests in the commit that added Generator.prototype.return, but there they are. Many tests are very similar to the ones I added. Testing break/continue in finally is a good idea, will add some of those :)

Comment 11

[Andy Wingo [:wingo]]

Comment on attachment 8547719 [details] [diff] [review]
Patch

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

LGTM.  I missed the addition of dotGenRval; that's a pretty strange thing!  I guess the frame rval slot didn't work because it's not saved by the generator, so resuming a yield would not restore its value?

Comment 12

[Jan de Mooij [:jandem]]

Thanks for the (fast) review.

(In reply to Andy Wingo [:wingo] from comment #11)
> LGTM.  I missed the addition of dotGenRval; that's a pretty strange thing! 
> I guess the frame rval slot didn't work because it's not saved by the
> generator, so resuming a yield would not restore its value?

Yes, exactly. See bug 958949 comment 4. Now when we have a return in a try-with-finally in a star generator, we use .genrval to store the value instead of the frame's rval slot.

Comment 13

[Jan de Mooij [:jandem]]

Pushed with some extra tests for break/continue in finally:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ecbda2b89b0

260 lines (out of 340) are tests, not bad.

Comment 14

[Jan de Mooij [:jandem]]

Hrm, Jason and Shu were just discussing yield* on IRC and I realized this patch doesn't handle that correctly with .return... Will look into this tomorrow, if there's no easy fix I'll backout for now.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2ecbda2b89b0

Comment 16

[Jan de Mooij [:jandem]]

OK, I have yield* working with .return now and it passes all my tests. Should have a patch tomorrow but want to write more tests first.

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Delegating yield

This patch makes yield* forward .return() correctly.

Also, I noticed a difference between V8/SM and es6draft with some .throw() tests I wrote. It turns out the spec for forwarding .throw() changed, see below (thanks to André for pointing this out). I updated our .throw forwarding to match the latest spec and also added step numbers. I had to modify a number of tests and I verified all those tests also pass in André's ES6 implementation.

rev26, 14.4.14, step 10b.
---
ii.   If HasProperty( iterator, "throw")  is true, then
  1.  Let innerResult  be Invoke(iterator, "throw", (received.[[value]])).
  2.  ReturnIfAbrupt(innerResult).
  3.  If Type(innerResu lt) is not Object, then throw a  TypeError  exception.
iii.   Else, return received.
---

rev27, 14.4.14, step 6c. (same in rev30, 14.4.15, step 6b.)
---
i.   Let hasThrow  be  HasProperty(iterator, "throw");
ii.   ReturnIfAbrupt(hasThrow).
iii.   If hasThrow  is true, then
  1.  Let innerResult  be Invoke(iterator, "throw", (received.[[value]])).
  2.  ReturnIfAbrupt(innerResult).
  3.  NOTE:  Exceptions from the inner iterator throw method are propagated.
iv.   Return received.
---

Comment 18

[Andy Wingo [:wingo]]

What is up with this spec change?  Given this idiom:

function* ones_coroutine() {
  while (true) {
    try {
      yield 1;
    } catch (e) {}
  }
}

And the yield* wrapper:

function* wrap(iterable) {
  return yield* iterable
}

I thought that this intention was that wrap(ones_coroutine()) would be equivalent to ones_coroutine().

However now they are different; consider

var i1 = ones_coroutine()
i1.next() // { value: 1, done: false }
i1.throw('foo') // { value: 1, done: false }
i1.next() // { value: 1, done: false }

var i2 = wrap(ones_coroutine())
i2.next() // { value: 1, done: false }
i2.throw(42) // { value: undefined, done: true }
i2.next() // { value: undefined, done: true }

Comment 19

[Andy Wingo [:wingo]]

https://bugs.ecmascript.org/show_bug.cgi?id=3526 for the change in throw() + yield*.

Comment 20

[Andy Wingo [:wingo]]

Hi Jan!  Sorry for the delay.  Given Allen's comments in https://bugs.ecmascript.org/show_bug.cgi?id=3526, it seems that (something like) the original "throw" behavior is what was intended.  Would you mind posting an updated patch without the "throw" changes?

Comment 21

[Marc Nieper-Wißkirchen]

https://bugs.ecmascript.org/show_bug.cgi?id=3526 is fixed.

Comment 22

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/38#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Generator/return

Any chance that we can land part 2 in the same release (38) ?

Comment 23

[Andy Wingo [:wingo]]

Comment on attachment 8549529 [details] [diff] [review]
Part 2 - Delegating yield

Clearing r? as the patch needs updating.  LMK if I did the wrong thing here.

Comment 24

[Eric Faust [:efaust]]

So this is done. Or not? It's hard to tell what the state of the work here is. It sounds like part 2 is unnecessary because a spec change was made?

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Eric Faust [:efaust] from comment #24)
> So this is done. Or not? It's hard to tell what the state of the work here
> is. It sounds like part 2 is unnecessary because a spec change was made?

No, we still need part 2, at least for delegating .return(). My patch also fixed .throw() behavior to match the then-latest spec, but those spec changes were reverted later.

So we still need to fix delegating .return() (and we should also make sure our .throw() code is indeed correct).

Comment 26

[Eric Faust [:efaust]]

(In reply to Jan de Mooij [:jandem] from comment #25)
> So we still need to fix delegating .return() (and we should also make sure
> our .throw() code is indeed correct).

OK, that matches what Jason, Mariusz and I were able to grok last night. I think Mariusz is going to try and take a look at this.

Comment 27

[mkierski]

Attachment: Implemented return and throw

Comment 28

[Eric Faust [:efaust]]

Comment on attachment 8634310 [details] [diff] [review]
Implemented return and throw

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

This looks great! Just a bunch of formatting stuff. Please upload a new patch, and I'll get it landed.

If you don't have the patch anymore, just download this one, patch it in, and work from that.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6261,5 @@
> +        return false;
> +
> +    if (!emitJump(JSOP_GOTO, tryStart - offset()))               
> +        return false;
> +    

trailing whitespace on this line, and the end of the if above.

@@ +6266,5 @@
> +    // Step 6.b.iv.
> +    setJumpOffsetAt(checkThrow);
> +    if (!emit1(JSOP_POP))                                        // EXCEPTION
> +        return false;
> +    

trailing whitespace on empty line.

@@ +6279,5 @@
> +     // indicating if we're throwing an exception and FVALUE is the exception,
> +     // if there is one.
> + 
> +     this->stackDepth = depth;
> + 

trailing whitespace on empty line, and above this statement, on the empty line after the comment

@@ +6284,5 @@
> +     ptrdiff_t finallyStart = offset();
> +     if (!emit1(JSOP_FINALLY))                                   // ITER RESULT FTYPE FVALUE
> +         return false;
> +     MOZ_ASSERT(stackDepth == depth + 2);
> + 

trailing whitespace on empty line as well as several below.

@@ +6311,5 @@
> + 
> +     // Steps 6.c.v-vi. RVAL = ITER.return(RESULT)
> +     if (!emitDupAt(this->stackDepth - 1 - 3))                   // ITER RESULT FTYPE FVALUE ITER
> +         return false;
> +     if (!emit1(JSOP_DUP))                                      // ITER RESULT FTYPE FVALUE ITER ITER

we should line up these comment about the stack in one neat row.

@@ +6359,5 @@
> +        return false;
> +
> +    setJumpOffsetAt(checkClosing);
> +    setJumpOffsetAt(checkReturn);
> +    // Return from the finally block.

This comment should go just above the line below, with a blank line above

@@ +6426,5 @@
>          return false;
>      if (!emitAtomOp(cx->names().value, JSOP_GETPROP))            // VALUE
>          return false;
>  
> +

extraneous added line. Please remove.

::: js/src/vm/Interpreter.cpp
@@ +4313,5 @@
>      JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, errorNum);
>      return false;
>  }
>  
> +    

trailing whitespace on extra line. Please just remove a while blank line here.

Comment 29

[Jason Orendorff [:jorendorff]]

efaust: Can we do anything with this?

Comment 30

[Eric Faust [:efaust]]

*blink*. Uh, yes. I will do my own whitespace fixes on a rebase of this, and land it

Comment 31

[Jason Orendorff [:jorendorff]]

(...dependency tree tweaking...)

Comment 32

[yomi sosanya]

I am new here (and I don't know how to post a new bug) and I found what I think is a problem with generator.prototype.next() and yield, the code below explains my problem:

function* gen(){                            //  line 1
  var rv = (yield 1) + 3;                   //  line 2 
  console.log('rv = '+rv);                  //  line 3 
  yield 2;                                  //  line 4
}
// [code one]
var iter = gen();
console.log(iter.next(5));
console.log(iter.next()); 

****************************************************
:output: (below)
  1                //normal
  rv = undefined   //abnormal: expected 'rv = 8'
  2                //normal
****************************************************
// the workaround (I think the logic is twisted [no insult])
// [code two]
var iter = gen();
console.log(iter.next());
console.log(iter.next(5));

****************************************************
:output: (below)
1
rv = 8
2
****************************************************

Why was the variable rv (in line 2) in the generator function set only after the next yield (in line 4) was called? 
The value that was sent back using next() was then evaluated (in line 3);
I know Python and Javascript are different languages, but the first code would work just fine in Python: 
1. line 2 would yield it value.
2. the first call to next will send back its argument (undefine or another value) to the generator and return the first yield value from the generator.
3. line 3 then executes with variable rv already set.
4. line 4 then yield its value.

Comment 33

[yomi sosanya]

(In reply to yomi sosanya from comment #32)
> I am new here (and I don't know how to post a new bug) and I found what I
> think is a problem with generator.prototype.next() and yield, the code below
> explains my problem:
> 
> function* gen(){                            //  line 1
>   var rv = (yield 1) + 3;                   //  line 2 
>   console.log('rv = '+rv);                  //  line 3 
>   yield 2;                                  //  line 4
> }
> // [code one]
> var iter = gen();
> console.log(iter.next(5));
> console.log(iter.next()); 
> 
> ****************************************************
> :output: (below)
>   1                //normal
>   rv = undefined   //abnormal: expected 'rv = 8'
>   2                //normal
> ****************************************************
> // the workaround (I think the logic is twisted [no insult])
> // [code two]
> var iter = gen();
> console.log(iter.next());
> console.log(iter.next(5));
> 
> ****************************************************
> :output: (below)
> 1
> rv = 8
> 2
> ****************************************************
> 
> Why was the variable rv (in line 2) in the generator function set only after
> the next yield (in line 4) was called? 
> The value that was sent back using next() was then evaluated (in line 3);
> I know Python and Javascript are different languages, but the first code
> would work just fine in Python: 
> 1. line 2 would yield it value.
> 2. the first call to next will send back its argument (undefine or another
> value) to the generator and return the first yield value from the generator.
> 3. line 3 then executes with variable rv already set.
> 4. line 4 then yield its value.

In my opinion, when the state is SUSPENDEDSTART just like SUSPENDEDYIELD the value passed to generator.prototype.next() as in next(arg), should not be discarded.

Comment 34

[Jan de Mooij [:jandem]]

Let's close this - if there's more to do we should file new bugs.