Closed
Bug 1115868
Opened 10 years ago
Closed 7 years ago
Implement Generator.prototype.return
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: 446240525, Assigned: jandem)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])
Attachments
(3 files, 1 obsolete file)
18.18 KB,
patch
|
wingo
:
review+
|
Details | Diff | Splinter Review |
40.71 KB,
patch
|
Details | Diff | Splinter Review | |
17.84 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
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•10 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•10 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•10 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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 4•10 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•10 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•10 years ago
|
||
bug 965717 rather
Assignee | ||
Comment 7•10 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•10 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).
Attachment #8543295 -
Attachment is obsolete: true
Attachment #8547719 -
Flags: review?(wingo)
Comment 9•10 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
Assignee | ||
Comment 10•10 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•10 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•10 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•10 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•10 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•10 years ago
|
Flags: needinfo?(jdemooij)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 16•10 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•10 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.
---
Flags: needinfo?(jdemooij)
Attachment #8549529 -
Flags: review?(wingo)
Comment 18•10 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•10 years ago
|
||
https://bugs.ecmascript.org/show_bug.cgi?id=3526 for the change in throw() + yield*.
Comment 20•10 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 21•10 years ago
|
||
Comment 22•10 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) ?
Keywords: dev-doc-needed → dev-doc-complete
Comment 23•10 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)
Comment 24•10 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?
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 25•10 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)
Comment 26•10 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.
Comment 27•10 years ago
|
||
Attachment #8634310 -
Flags: review?(efaustbmo)
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.
Attachment #8634310 -
Flags: review?(efaustbmo) → review+
Comment 30•9 years ago
|
||
*blink*. Uh, yes. I will do my own whitespace fixes on a rebase of this, and land it
Flags: needinfo?(efaustbmo)
Comment 31•9 years ago
|
||
(...dependency tree tweaking...)
Comment 32•9 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•9 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•7 years ago
|
||
Let's close this - if there's more to do we should file new bugs.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•