958949 - yield in finally overrides return completion from try

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Maybe related to bug 718531?

Test case:
---
js> function*g(){ try { return 0 } finally { yield 1 } }
js> o = g(); o.next(); o.next()

Expected: ({value:0, done:true})
Actual: ({value:1, done:false})
---

Comment 1

[Till Schneidereit [:till]]

Can you point out where the spec says that your expected result is correct?

By my reading, this is correct behavior:
- `return 0` sets the rval to `0`
- the finally block is executed
- `yield 1` sets the rval to the result of `GeneratorYield(CreateIterResultObject(1, false))`
- control is returned to the callee, with the current rval as the returned value
- the callee interprets the returned value as always

Comment 2

[André Bargull [:anba]]

There are two calls to next() in the example code. The first call does return the correct result, i.e. {value: 1, done: false}, but the second call does not:
---
js> function*g(){ try { return 0 } finally { yield 1 } }
js> o = g()
({})
js> o.next()
({value:1, done:false})
js> o.next()
({value:1, done:false})
js> o.next()
typein:5:0 TypeError: generator has already finished
---

Comment 3

[Michał Gołębiowski-Owczarek [:mgol]]

This shouldn't be blocking bug 950547 now that generators are out of scope of it; I'm removing it.

Comment 4

[Tooru Fujisawa [:arai]]

Now situation is a bit different, but still incorrect.

---
js> function*g(){ try { return 0 } finally { yield 1 } }
js> o = g();
({})
js> o.next()
({value:1, done:false})
js> o.next()
js> o.next()
({value:(void 0), done:true})
---

the second next() returns undefined.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #4)
> the second next() returns undefined.

Oh, this is interesting. What happens is this:

(1) For the "return 0" we set the frame's rval slot to {value: 0, done: true}. We do *not* return.
(2) We execute the finally block. This yields {value: 1, done: false}.
(3) We resume and RETSUB returns to the return inside the try block, which does finalyieldrval but rval is |undefined| because we didn't save/restore the rval slot as part of (2).

This is most likely a regression from bug 987560. We could add a GeneratorObject slot for the frame's rval but it'd be nice if we could avoid that.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Save return value for generator function into local variable before running finally-block.

Added .genrval local variable (like .generator variable),
save return value into it before executing finally-block,
and restore the value from it after retsub.

Also, changed PNK_RETURN to binary, to store name node of .genrval,
which is needed to emit JSOP_SETALIASEDVAR, where hops might be != 0.


Now bytecode for generator function `function*g(){ try { return 0 } finally { yield 1 } }` is following:

---
00000:  generator
00001:  setaliasedvar ".generator" (hops = 0, slot = 2)
00006:  initialyield 0
00010:  debugafteryield
00011:  pop
00012:  try
00013:  newobject ({value:(void 0), done:(void 0)})
00018:  zero
00019:  initprop "value"
00024:  true
00025:  initprop "done"
00030:  setaliasedvar ".genrval" (hops = 0, slot = 3)   // save return value
00035:  pop
00036:  gosub 63 (+27)
00041:  getaliasedvar ".genrval" (hops = 0, slot = 3)   // restore return value
00046:  setrval
00047:  getaliasedvar ".generator" (hops = 0, slot = 2)
00052:  finalyieldrval
00053:  gosub 63 (+10)
00058:  goto 94 (+36)
00063:  finally
00064:  newobject ({value:(void 0), done:(void 0)})
00069:  one
00070:  initprop "value"
00075:  false
00076:  initprop "done"
00081:  getaliasedvar ".generator" (hops = 0, slot = 2)
00086:  yield 1
00090:  debugafteryield
00091:  pop
00092:  retsub
00093:  nop
00094:  newobject ({value:(void 0), done:(void 0)})
00099:  undefined
00100:  initprop "value"
00105:  true
00106:  initprop "done"
00111:  setrval
00112:  getaliasedvar ".generator" (hops = 0, slot = 2)
00117:  finalyieldrval
00118:  retrval
---

No bytecode change happened if there is no finally block.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=862649f14379

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Save return value for generator function into local variable before running finally-block.

just rebased :)

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8533519 [details] [diff] [review]
Save return value for generator function into local variable before running finally-block.

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

Hi arai, I'm very sorry for the delay. Excellent fix, thanks so much for taking this!

Just some small comments, if you can post an updated patch I'll review quickly.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5585,5 @@
>      return EmitGoto(cx, bce, stmt, &stmt->continues, SRC_CONTINUE) >= 0;
>  }
>  
>  static bool
> +hasFinally(BytecodeEmitter *bce)

Style nit: HasFinally

Also STMT_FINALLY is confusing because it's pushed when we're emitting the try-code, not the finally-block itself. So to make this more obvious, maybe InTryBlockWithFinally()?

@@ +5637,5 @@
>      bool isGenerator = bce->sc->isFunctionBox() && bce->sc->asFunctionBox()->isGenerator();
> +    bool useGenRVal = false;
> +    if (isGenerator) {
> +        if (bce->sc->asFunctionBox()->isStarGenerator() && hasFinally(bce)) {
> +            useGenRVal = true;

Nit: I'd add a short comment here to explain to the reader what's going on:

Emit JSOP_SETALIASEDVAR .genrval to store the return value on the scope chain, so it's not lost when we yield in a finally block.

::: js/src/frontend/Parser.cpp
@@ +1086,5 @@
>              return null();
>          if (!pc->define(tokenStream, context->names().dotGenerator, generator, Definition::VAR))
>              return null();
>  
> +        if (pc->generatorKind() == StarGenerator) {

Nit: if (pc->isStarGenerator())

@@ +3119,5 @@
>           * can potentially override any static bindings introduced by statements
>           * further up the stack, we have to abort the search.
>           */
> +        if (stmt->type == STMT_WITH && atom != ct->sc->context->names().dotGenerator &&
> +            atom != ct->sc->context->names().dotGenRVal) {

Can you add a helper function somewhere to check if a name is dotGenerator or dotGenRVal and call that everywhere? Like IsDotVariable or something better.

(Either make it a static helper function with an ExclusiveContext* argument, or add a method to sc and use context.)

::: js/src/jit-test/tests/generators/bug958949.js
@@ +6,5 @@
> +function* g1() {
> +  try {
> +    return 42;
> +  } finally {
> +    yield 43;

Great tests, thank you.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Save return value for generator function into local variable before running finally-block.

Thank you for reviewing! :)

(In reply to Jan de Mooij [:jandem] from comment #8)
> @@ +3119,5 @@
> >           * can potentially override any static bindings introduced by statements
> >           * further up the stack, we have to abort the search.
> >           */
> > +        if (stmt->type == STMT_WITH && atom != ct->sc->context->names().dotGenerator &&
> > +            atom != ct->sc->context->names().dotGenRVal) {
> 
> Can you add a helper function somewhere to check if a name is dotGenerator
> or dotGenRVal and call that everywhere? Like IsDotVariable or something
> better.
> 
> (Either make it a static helper function with an ExclusiveContext* argument,
> or add a method to sc and use context.)

Added IsDotVariable to SharedContext, and used the context property of itself to access `names()`,
In following 3 functions, previously different `ExclusiveContext *` variable was used.
 * Parser<FullParseHandler>::withStatement  --  Parser::context
 * LegacyCompExprTransplanter::transplant  --  Parser::context
 * EmitAtomOp  --  cx parameter
Since `ThreadSafeContext::names()` returns JSRuntime::commonNames,
they still return same JSAtomState instance even if those pointers refer different context instance,
as long as those contexts are created in same JSRuntime.

Is it possible situation that they are created in different JSRuntime's?
if so, this patch is wrong, and we should add a helper function in some header file.


Also, renamed test file to "yield-in-finally.js", and added one more test case with "with" statement.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=09ab93fc9aab

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8536957 [details] [diff] [review]
Save return value for generator function into local variable before running finally-block.

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

Great work, thanks again.

::: js/src/frontend/SharedContext.h
@@ +206,5 @@
>      bool needStrictChecks() {
>          return strict || extraWarnings;
>      }
> +
> +    bool IsDotVariable(JSAtom *atom) {

Style nit: it's a class method so it should be isDotVariable. Also I think you can make it const:

    bool isDotVariable(JSAtom *atom) const {

Comment 11

[Tooru Fujisawa [:arai]]

Thanks again!
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ce465f852c

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3ce465f852c