Implement Generator.prototype.return

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: 446240525, Assigned: jandem)

Tracking

({dev-doc-complete})

Trunk
mozilla38
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 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 }
(Assignee)

Comment 1

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

Comment 2

4 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!
Flags: needinfo?(jdemooij)
(Assignee)

Comment 3

4 years ago
Created attachment 8543295 [details] [diff] [review]
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

4 years ago
bug 965717 rather
(Assignee)

Comment 7

4 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
(Assignee)

Comment 8

4 years ago
Created attachment 8547719 [details] [diff] [review]
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).
Attachment #8543295 - Attachment is obsolete: true
Attachment #8547719 - Flags: review?(wingo)
(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
(Assignee)

Comment 10

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

Comment 11

4 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?
Attachment #8547719 - Flags: review?(wingo) → review+
(Assignee)

Comment 12

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

Comment 13

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

Comment 14

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

Updated

4 years ago
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/2ecbda2b89b0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 16

4 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 17

4 years ago
Created attachment 8549529 [details] [diff] [review]
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.
---
Flags: needinfo?(jdemooij)
Attachment #8549529 - Flags: review?(wingo)

Comment 18

4 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 }

Comment 19

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

Comment 20

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

Comment 23

4 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.
Attachment #8549529 - Flags: review?(wingo)
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?
Flags: needinfo?(jdemooij)
(Assignee)

Comment 25

3 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).
Flags: needinfo?(jdemooij)
(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

3 years ago
Created attachment 8634310 [details] [diff] [review]
Implemented return and throw
Attachment #8634310 - Flags: review?(efaustbmo)
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.
Attachment #8634310 - Flags: review?(efaustbmo) → review+
efaust: Can we do anything with this?
Flags: needinfo?(efaustbmo)
*blink*. Uh, yes. I will do my own whitespace fixes on a rebase of this, and land it
Flags: needinfo?(efaustbmo)
(...dependency tree tweaking...)
Blocks: 1147371
No longer blocks: 694100

Comment 32

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

Comment 33

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

Comment 34

6 months ago
Let's close this - if there's more to do we should file new bugs.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago6 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.