Closed Bug 1360839 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: arai, Assigned: shu)

References

Details

Attachments

(1 file)

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.
might be a dupe of bug 1115868 tho (I haven't read whole bug...
Blocks: 1147371
Assignee: nobody → 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 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.
Attachment #8863286 - Flags: review?(arai.unmht) → review+
(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?
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/570faeca35ab
Call IteratorClose due to abrupt completion from yield. (r=arai)
https://hg.mozilla.org/mozilla-central/rev/570faeca35ab
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
> > ::: 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.
(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.
> > 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...
You need to log in before you can comment on or make changes to this bug.