ziyunfei Reporter
	Description • 9 years ago

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 }

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

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.

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

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!

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

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.

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

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.

	Andy Wingo [:wingo]
	Comment 5 • 9 years ago

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.

	Andy Wingo [:wingo]
	Comment 6 • 9 years ago

bug 965717 rather

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

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

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

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

	André Bargull [:anba]
	Comment 9 • 9 years ago

(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

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

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

	Andy Wingo [:wingo]
	Comment 11 • 9 years ago

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?

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

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.

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

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.

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

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 9 years ago

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

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

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.

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

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.
---

	Andy Wingo [:wingo]
	Comment 18 • 9 years ago

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 }

	Andy Wingo [:wingo]
	Comment 19 • 9 years ago

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

	Andy Wingo [:wingo]
	Comment 20 • 9 years ago

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?

	Marc Nieper-Wißkirchen
	Comment 21 • 9 years ago

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

	Florian Scholz (Open Web Docs)
	Comment 22 • 9 years ago

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

	Andy Wingo [:wingo]
	Comment 23 • 9 years ago

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.

	Eric Faust [:efaust]
	Comment 24 • 9 years ago

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?

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

(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).

	Eric Faust [:efaust]
	Comment 26 • 9 years ago

(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.

	Eric Faust [:efaust]
	Comment 28 • 9 years ago

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.

	Jason Orendorff [:jorendorff]
	Comment 29 • 8 years ago

efaust: Can we do anything with this?

	Eric Faust [:efaust]
	Comment 30 • 8 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 31 • 8 years ago

(...dependency tree tweaking...)

	yomi sosanya
	Comment 32 • 8 years ago

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.

	yomi sosanya
	Comment 33 • 8 years ago

(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.

	Jan de Mooij [:jandem] Assignee
	Comment 34 • 6 years ago

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