1360839 - returning from yield inside for-of doesn't perform IteratorClose.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

discovered in bug 1360824.

returning from yield with IteratorClose doesn't close the enclosing for-of.

Code:

function* wrap() {
  let iter = {
    [Symbol.iterator]() {
      return this;
    },
    next() {
      return { value: 10, done: false };
    },
    return() {
      console.log("return is called");
      return {};
    }
  };
  for (const i of iter) {
    yield i;
  }
}
for (const i of wrap()) {
  break;
}

Expected result:
  "return is called" is printed

Actual result:
  nothing is printed.

Comment 1

[Tooru Fujisawa [:arai]]

might be a dupe of bug 1115868 tho (I haven't read whole bug...

Comment 2

[Shu-yu Guo [:shu]]

IIUC, the semantics should be that, upon calling Generator.return:

- IteratorClose is called for enclosing for-of loops.
- The Generator.return can be "cancelled" into a throw by IteratorClose itself throwing, since the generator is resumed by an abrupt completion of type "return".

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Call IteratorClose due to abrupt completion from yield.

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8863286 [details] [diff] [review]
Call IteratorClose due to abrupt completion from yield.

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

Thank you for taking this :)
please add some testcases (comment #0, and also the "workaround" case in bug 1360824)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1562,5 @@
>      //     catch-block
> +    //
> +    // Additionally, a finally block may be emitted if when DontUseControl
> +    // even if the kind is not TryCatchFinally or TryFinally, because GOSUBs
> +    // are not emitted.

it would be nice to enumerate the condition for this case like above, and make it clear what the differences are.
(like, it could have non-local-jump, etc)

@@ +2106,5 @@
> +        // If any yields were emitted, then this for-of loop is inside a star
> +        // generator and must handle the case of Generator.return. Like in
> +        // yield*, it is handled with a finally block.
> +        uint32_t numYieldsEmitted = bce->yieldAndAwaitOffsetList.length();
> +        if (numYieldsEmitted > numYieldsAtBeginCodeNeedingIterClose_) {

this condition would match also if there's await.
might be nice to reduce the case, like excluding Async Function (but including Async Generator)

@@ +2115,5 @@
> +            if (!bce->emit1(JSOP_ISGENCLOSING))   // ITER ... FTYPE FVALUE CLOSING
> +                return false;
> +            if (!ifGeneratorClosing.emitIf())     // ITER ... FTYPE FVALUE
> +                return false;
> +            if (!bce->emitDupAt(slotFromTop + 1)) // ITER ... FTYPE FVALUE ITER

Can you sanity-check me that ITER couldn't be undefined for this case?
I cannot think of any case that ISGENCLOSING==true and ITER==undefined case tho.

Comment 5

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #4)
> Comment on attachment 8863286 [details] [diff] [review]
> Call IteratorClose due to abrupt completion from yield.
> 
> Review of attachment 8863286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for taking this :)
> please add some testcases (comment #0, and also the "workaround" case in bug
> 1360824)
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +1562,5 @@
> >      //     catch-block
> > +    //
> > +    // Additionally, a finally block may be emitted if when DontUseControl
> > +    // even if the kind is not TryCatchFinally or TryFinally, because GOSUBs
> > +    // are not emitted.
> 
> it would be nice to enumerate the condition for this case like above, and
> make it clear what the differences are.
> (like, it could have non-local-jump, etc)

It should be the same conditions as listed above that comment. I'll explicitly write that.

> 
> @@ +2106,5 @@
> > +        // If any yields were emitted, then this for-of loop is inside a star
> > +        // generator and must handle the case of Generator.return. Like in
> > +        // yield*, it is handled with a finally block.
> > +        uint32_t numYieldsEmitted = bce->yieldAndAwaitOffsetList.length();
> > +        if (numYieldsEmitted > numYieldsAtBeginCodeNeedingIterClose_) {
> 
> this condition would match also if there's await.
> might be nice to reduce the case, like excluding Async Function (but
> including Async Generator)

Yeah, good call. I'll just track num of JSOP_AWAITs separately from the yield ops inside emitYieldOp.

> 
> @@ +2115,5 @@
> > +            if (!bce->emit1(JSOP_ISGENCLOSING))   // ITER ... FTYPE FVALUE CLOSING
> > +                return false;
> > +            if (!ifGeneratorClosing.emitIf())     // ITER ... FTYPE FVALUE
> > +                return false;
> > +            if (!bce->emitDupAt(slotFromTop + 1)) // ITER ... FTYPE FVALUE ITER
> 
> Can you sanity-check me that ITER couldn't be undefined for this case?
> I cannot think of any case that ISGENCLOSING==true and ITER==undefined case
> tho.

Yeah, I think if we enter this finally block then ITER must be non-null. Are you worried that we should null-check it?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/570faeca35ab
Call IteratorClose due to abrupt completion from yield. (r=arai)

Comment 7

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/570faeca35ab

Comment 8

[Tooru Fujisawa [:arai]]

> > ::: js/src/frontend/BytecodeEmitter.cpp
> > @@ +1562,5 @@
> > >      //     catch-block
> > > +    //
> > > +    // Additionally, a finally block may be emitted if when DontUseControl
> > > +    // even if the kind is not TryCatchFinally or TryFinally, because GOSUBs
> > > +    // are not emitted.
> > 
> > it would be nice to enumerate the condition for this case like above, and
> > make it clear what the differences are.
> > (like, it could have non-local-jump, etc)
> 
> It should be the same conditions as listed above that comment. I'll
> explicitly write that.

wait, the conditions should be different.
code between {emitBeginCodeNeedingIteratorClose,emitEndCodeNeedingIteratorClose} are for-of head and for-of body, and body can contain break/continue/return.

so at least the following condition is not met.

>    //   * have no non-local-jump

maybe I overlooked some case that is not covered properly by TryEmitter.


> > Can you sanity-check me that ITER couldn't be undefined for this case?
> > I cannot think of any case that ISGENCLOSING==true and ITER==undefined case
> > tho.
> 
> Yeah, I think if we enter this finally block then ITER must be non-null. Are
> you worried that we should null-check it?

Yes, was thinking about ITER==undefined case that is done by emitPrepareForNonLocalJump.

Comment 9

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #8)
> > > ::: js/src/frontend/BytecodeEmitter.cpp
> > > @@ +1562,5 @@
> > > >      //     catch-block
> > > > +    //
> > > > +    // Additionally, a finally block may be emitted if when DontUseControl
> > > > +    // even if the kind is not TryCatchFinally or TryFinally, because GOSUBs
> > > > +    // are not emitted.
> > > 
> > > it would be nice to enumerate the condition for this case like above, and
> > > make it clear what the differences are.
> > > (like, it could have non-local-jump, etc)
> > 
> > It should be the same conditions as listed above that comment. I'll
> > explicitly write that.
> 
> wait, the conditions should be different.
> code between
> {emitBeginCodeNeedingIteratorClose,emitEndCodeNeedingIteratorClose} are
> for-of head and for-of body, and body can contain break/continue/return.
> 
> so at least the following condition is not met.
> 
> >    //   * have no non-local-jump
> 
> maybe I overlooked some case that is not covered properly by TryEmitter.
> 

Then I misunderstood the original restrictions. I thought that meant they have no non-local jumps in the catch/finally block, which is definitely true.

> 
> > > Can you sanity-check me that ITER couldn't be undefined for this case?
> > > I cannot think of any case that ISGENCLOSING==true and ITER==undefined case
> > > tho.
> > 
> > Yeah, I think if we enter this finally block then ITER must be non-null. Are
> > you worried that we should null-check it?
> 
> Yes, was thinking about ITER==undefined case that is done by
> emitPrepareForNonLocalJump.

Yeah, I believe that is mutually exclusive with yield.

Comment 10

[Tooru Fujisawa [:arai]]

> > wait, the conditions should be different.
> > code between
> > {emitBeginCodeNeedingIteratorClose,emitEndCodeNeedingIteratorClose} are
> > for-of head and for-of body, and body can contain break/continue/return.
> > 
> > so at least the following condition is not met.
> > 
> > >    //   * have no non-local-jump
> > 
> > maybe I overlooked some case that is not covered properly by TryEmitter.
> > 
> 
> Then I misunderstood the original restrictions. I thought that meant they
> have no non-local jumps in the catch/finally block, which is definitely true.

the restriction about "no non-local-jump" is because we need to GOSUB to finally block before doing syntactic non-local-jump (in NonLocalExitControl::prepareForNonLocalJump), at least for syntactic try-catch-finally.  otherwise, the finally block is *not* used by non-local-jump (because there's no control and NonLocalExitControl::prepareForNonLocalJump doesn't detect the try-finally).

but in this case, that restriction can be ignored, since the finally block is only for implicit return, and even if non-local-jumps skip finally block, the behavior doesn't change.

so, the restriction for for-of case should be something like:

  * don't use finally block for syntactic non-local-jump

also I realized the comment should explicitly say when the finally block is used (implicit return, uncaught exception, and maybe some more?), for both case...