1147371 - Implement IteratorClose

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Tom S [:evilpie]]

Iterators in ES6 now have returning/closing semantics like we have for our old generators.

Comment 1

[Jason Orendorff [:jorendorff]]

Some places where we need to close iterators:

- for-of break
- for-of throw
- array destructuring
- yield *
- yield * with throw()
- Map & Set constructors
- WeakMap & WeakSet constructors
- Array.from

Comment 3

[Tom S [:evilpie]]

Somebody needs to start looking into this. We are missing quite a lot of points on kangax just because of this.

Comment 4

[Jason Orendorff [:jorendorff]]

We haven't implemented because TC39 keeps hovering on the edge of removing it from the standard.

Comment 5

[Kyle Simpson]

Can we get an update on this?

Comment 6

[Tom S [:evilpie]]

Is somebody interested in implementing this? This is definitely one of the biggest missing pieces that we have left for ES6.

Comment 7

[Hannes Verschore [:h4writer]]

(In reply to Tom Schuster [:evilpie] from comment #6)
> Is somebody interested in implementing this? This is definitely one of the
> biggest missing pieces that we have left for ES6.

Given this statement I've put P2 (fix in next release) on this bug. I'm not sure how much work is involved to get this fixed, therefore I didn't know if P1 (fix in this release) was possible.

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Hannes Verschore [:h4writer] from comment #7)
> (In reply to Tom Schuster [:evilpie] from comment #6)
> > Is somebody interested in implementing this? This is definitely one of the
> > biggest missing pieces that we have left for ES6.
> 
> Given this statement I've put P2 (fix in next release) on this bug. I'm not
> sure how much work is involved to get this fixed, therefore I didn't know if
> P1 (fix in this release) was possible.

I might be able to get to this before next train. I agree with P2 for now, given other deadlines I have.

Comment 9

[Hannes Verschore [:h4writer]]

(In reply to Shu-yu Guo [:shu] from comment #8)
> I might be able to get to this before next train. I agree with P2 for now,
> given other deadlines I have.

Can I move this to P1? As I understood you wanted to get this done in FF53 right?

Comment 10

[Tooru Fujisawa [:arai]]

IteratorClose in built-in are implemented in bug 1180306.

remaining things are:
  * array destructuring
  * for-of
  * yield*

all those are now handled in bytecode.

Comment 12

[Rob Wu [:robwu]]

I was bitten by the difference between Firefox and Chrome, when I reused a generator after breaking out of a for...of loop.

To save others some trouble, I documented the expected (specified) behavior of generators in for-of loops, and added a compatibility note at
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of#Do_not_reuse_generators
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of#Browser_compatibility

Once this bug is fixed, please edit these sections to mention the Firefox version in which this bug is fixed.

Comment 13

[Shu-yu Guo [:shu]]

(In reply to Rob Wu [:robwu] from comment #12)
> I was bitten by the difference between Firefox and Chrome, when I reused a
> generator after breaking out of a for...of loop.
> 
> To save others some trouble, I documented the expected (specified) behavior
> of generators in for-of loops, and added a compatibility note at
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/
> for...of#Do_not_reuse_generators
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/
> for...of#Browser_compatibility
> 
> Once this bug is fixed, please edit these sections to mention the Firefox
> version in which this bug is fixed.

Thank you for taking the time!

Comment 14

[Shu-yu Guo [:shu]]

While doing this I ran into a snag implementing calling the "return" method for Generator.prototype.return() inside yield*. If we get a forced close inside a yield*, we need to call the "return" method on the iterator, if it exists. Moreover, the return value of calling "return" can result in iteration of yield* continuing. All in all, this means generator closing can no longer be implemented as an uncatchable exception.

My plan of implementation is:
  - Emit a new catch-like block in the yield* bytecode.
  - Make a new kind of try note, JSTRY_YIELD_STAR_RETURN, which will be the only kind of trynote that can intercept generator closing.
  - Implement the return logic from the spec in this new catch-like block.

Jan, you implemented Generator.return. Does the above sound reasonable?

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #14)
> Jan, you implemented Generator.return. Does the above sound reasonable?

You don't want to use a finally block for perf reasons? In a WIP patch for this I emitted a finally block where we checked whether the value on top of the stack was the generator-closing MagicValue: https://bug1115868.bmoattachments.org/attachment.cgi?id=8549529

If that doesn't work somehow or you think it's too slow, JSTRY_YIELD_STAR_RETURN seems reasonable.

FWIW, here's the V8 code desugaring yield*: https://github.com/v8/v8/blob/0663c225f8550ff8329c60a60f71038d9ae7bb74/src/parsing/parser.cc#L4236, not sure if they handle this correctly.

Comment 16

[Shu-yu Guo [:shu]]

Attachment: Implement IteratorClose for for-of.

Non-local jumps, i.e. |break| and |return| statements, are implemented
by emitting IteratorClose in bytecode.

On throws, the new trynote JSTRY_ITERCLOSE signals that there's an
iterator on the top of the stack that needs IteratorClose. Note that
this only applies to exceptions thrown from non-iterator code. If
iter.next or iter.return itself throws, IteratorClose should *not* be
called. This is why we can't use JSTRY_FOR_OF.

Comment 17

[Shu-yu Guo [:shu]]

Attachment: Rename allowContentSpread to allowContentIter.

This is to prepare for reimplementing several self-hosted methods to use
for-of.

Comment 18

[Shu-yu Guo [:shu]]

Attachment: Convert self-hosted code that need to call IteratorClose to use for-of.

Comment 19

[Shu-yu Guo [:shu]]

Attachment: Implement IteratorClose for array destructuring.

Since result.done is always needed now, always emit the code that pushes
it on the stack.

For throwing, like for-of, IteratorClose is only called when non-iterator
code throws.

Comment 20

[Shu-yu Guo [:shu]]

Attachment: Implement calling IteratorClose and "return" on iterators in yield*.

The forced return path is implemented as finally block. Unlike
IteratorClose, this path checks the result returned by the "return"
method. If !result.done, the yield loop continues.

This also changes checking for the "throw" method with a JSOP_CALLPROP
instead of a JSOP_IN to be in line with current spec.

Comment 21

[Shu-yu Guo [:shu]]

Attachment: Implement JSOP_PICK and JSOP_UNPICK in the expression decompiler.

Comment 22

[Shu-yu Guo [:shu]]

Jan, the bytecode emitted for the yield* is found in emitIteratorClose in the for-of patch. Forgot to split it out, sorry.

Comment 23

[Tooru Fujisawa [:arai]]

Comment on attachment 8824295 [details] [diff] [review]
Implement calling IteratorClose and "return" on iterators in yield*.

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

looks like the JIT implementation doesn't match to interpreter implementation.

::: js/src/jit/BaselineCompiler.cpp
@@ +4006,5 @@
>  
>  bool
> +BaselineCompiler::emit_JSOP_ISGENCLOSING()
> +{
> +    return emit_JSOP_ISNOITER();

ISNOITER uses the top of the stack value, and ISGENCLOSING uses the second value.
so they should be different implementation. (or helper function that takes stack depth?)

::: js/src/jit/IonBuilder.cpp
@@ +2190,5 @@
>        case JSOP_MOREITER:
>          return jsop_itermore();
>  
>        case JSOP_ISNOITER:
> +      case JSOP_ISGENCLOSING:

here too, they're different.

::: js/src/vm/Opcodes.h
@@ +1929,1 @@
>      /*

stack convention should be `val, ignored => val, ignored, res`, and nuses=2, ndefs=3.

Comment 24

[Tooru Fujisawa [:arai]]

Comment on attachment 8824291 [details] [diff] [review]
Implement IteratorClose for for-of.

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

Looks great :)

only some comment fixes.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +289,5 @@
> +    // IteratorClose. Since non-local jumps like break and return call
> +    // IteratorClose, whenever a non-local jump is emitted, we must terminate
> +    // the current JSTRY_ITERCLOSE note to skip the non-local jump code, then
> +    // start a new one.
> +    ptrdiff_t iterCloseTryStart_;

This needs some diagram or example that describes what "terminate the current note" and "start a new one" mean, also, something that clarifies that startNewIterCloseTryNote should be called after finishIterCloseTryNote.

how about following?


  for (x of y) {
      ...             instantiate ForOfLoopControl
      ...         +   <-- iterCloseTryStart_ points here
      ...         ^
      ...         |
      if (...)    v
                  +   call finishIterCloseTryNote before |break|
                      above range is noted with JSTRY_ITERCLOSE

        break;    <-- break and IteratorClose are not inside JSTRY_ITERCLOSE note

                      call startNewIterCloseTryNote after |break|
                  +   <-- next iterCloseTryStart_ points here
      ...         |
      ...         ~
  }

@@ +2313,5 @@
>            case StatementKind::ForOfLoop:
> +            hasForOfLoops = true;
> +            if (!control->as<ForOfLoopControl>().finishIterCloseTryNote(bce_))
> +                return false;
> +            // The iterator and the current value are on the stack.

Can we use stack comment (including initial condition) like other places?
  // ITER VALUE

@@ +2322,4 @@
>              break;
>  
>            case StatementKind::ForInLoop:
> +            // The iterator and the current value are on the stack.

here too.

@@ +2338,5 @@
> +        hasForOfLoops = true;
> +        if (!target->as<ForOfLoopControl>().finishIterCloseTryNote(bce_))
> +            return false;
> +
> +        // The iterator and the current value are on the stack.

and here.
so that it clarifies we're doing different thing for target and other for-of.

::: js/src/jsiter.cpp
@@ +1227,5 @@
>              // Nothing sensible to do.
>              return true;
>          }
>          return LegacyGeneratorObject::close(cx, obj);
> +    } else {

LegacyGeneratorObject that implements ES6 iterator protocol will be caught by previous branch |obj->is<LegacyGeneratorObject>()| even if ES6 iterator protocol was used until here.

  var g = (function() { yield 1; })();
  var iterCalled = false;
  var nextCalled = false;
  var returnCalled = false;
  Object.defineProperty(g, Symbol.iterator, {
    value: () => {
      iterCalled = true;
      return g;
    }
  });
  Object.defineProperty(g, "next", {
    value: () => {
      nextCalled = true;
      return { done: false, value: 10 };
    }
  });
  Object.defineProperty(g, "return", {
    value: () => {
      returnCalled = true;
      return {};
    }
  });
  try {
    for (let a of g)
      throw 10;
  } catch (e) {}
  assertEq(iterCalled, true);
  assertEq(nextCalled, true);
  assertEq(returnCalled, true); // fails

We could just ignore the case, since it won't happen in wild tho, adding some comment will be nice.

Comment 25

[Tooru Fujisawa [:arai]]

Comment on attachment 8824293 [details] [diff] [review]
Convert self-hosted code that need to call IteratorClose to use for-of.

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

r+ with the comment addressed :)

::: js/src/builtin/Array.js
@@ +824,5 @@
>  
> +            // Steps 5.e.ii (reordered), 5.e.viii.
> +            _DefineDataProperty(A, k++, mappedValue);
> +
> +            k++;

k is already incremented in _DefineDataProperty arguments.
please remove this extra one.

Comment 26

[Tooru Fujisawa [:arai]]

Comment on attachment 8824294 [details] [diff] [review]
Implement IteratorClose for array destructuring.

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

it's nice to see this fixed without adding extra many branches and try-finally :)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5060,5 @@
>      //   SetOrInitialize(lref, value);
> +    //
> +    //   // === emitted after loop ===
> +    //   if (!done)
> +    //      IteratorClose(iter);

Can you update the comments for each element to mention which expression is noted by ITERCLOSE ?
it would make it clear that IteratorClose is also handled there, not only after loop.

@@ +5181,2 @@
>              }
>              if (!ifAlreadyDone.emitIfElse())                      // ... OBJ ITER ?DONE *LREF

like this one, "?DONE" should be "DONE", since it's always there.

@@ +5269,5 @@
> +                                                                  // ... OBJ ITER
> +        if (!emitIteratorClose())                                 // ... OBJ
> +            return false;
> +    } else {
> +        // Otherwise, the last ?DONE value is on top of the stack. If not ?DONE,

"DONE" is always on the stack, so please remove "?" before "DONE", here and other stack comments in this method.

Comment 27

[André Bargull [:anba]]

Comment on attachment 8824291 [details] [diff] [review]
Implement IteratorClose for for-of.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4788,5 @@
> +    // Do nothing if "return" is undefined.
> +    IfThenElseEmitter ifReturnMethodIsDefined(this);
> +    if (!emit1(JSOP_DUP))                                 // ... ITER RET RET
> +        return false;
> +    if (!emit1(JSOP_UNDEFINED))                           // ... ITER RET RET UNDEFINED

IteratorClose calls GetMethod to retrieve the "return" method, so we also need to test for |ret !== null|.

::: js/src/jsiter.cpp
@@ +1241,5 @@
> +
> +        // Step 4.
> +        //
> +        // Do nothing if "return" is undefined.
> +        if (returnMethod.isUndefined())

s/isUndefined/isNullOrUndefined/

::: js/src/tests/ecma_6/Statements/for-of-iterator-return.js
@@ +11,5 @@
> +            return {};
> +        }
> +    });
> +
> +    // break calls iter.return

Maybe also add a test to test continue calls return()? |L: do for (var x of iterable) continue L; while(false);|

Comment 28

[André Bargull [:anba]]

The following test262 (still) fail with all patches applied:
    test262/built-ins/Map/iterator-close-after-set-failure.js
    test262/built-ins/Map/iterator-item-first-entry-returns-abrupt.js
    test262/built-ins/Map/iterator-item-second-entry-returns-abrupt.js
    test262/built-ins/Set/set-iterator-close-after-add-failure.js
    test262/built-ins/Array/from/iter-map-fn-err.js
    test262/built-ins/Array/from/iter-map-fn-return.js
    test262/built-ins/Array/from/from-array.js
    test262/built-ins/Array/from/iter-set-length.js
    test262/built-ins/Array/from/elements-updated-after.js
    test262/built-ins/Array/from/iter-set-elem-prop.js
    test262/built-ins/Array/from/iter-map-fn-args.js
    test262/built-ins/Array/from/elements-deleted-after.js
    test262/built-ins/Array/from/from-string.js
    test262/built-ins/Array/from/source-object-iterator-2.js
    test262/built-ins/Array/from/source-array-boundary.js
    test262/built-ins/WeakMap/iterator-close-after-set-failure.js
    test262/built-ins/WeakMap/iterator-item-first-entry-returns-abrupt.js
    test262/built-ins/WeakMap/iterator-item-second-entry-returns-abrupt.js
    test262/built-ins/WeakSet/iterator-close-after-add-failure.js
    test262/language/statements/for-of/iterator-next-result-value-attr-error.js
    test262/language/statements/for-of/continue-from-try.js
    test262/language/statements/for-of/dstr-array-elem-trlg-iter-rest-thrw-close-err.js
    test262/language/statements/for-of/dstr-array-rest-nested-array-iter-thrw-close-skip.js
    test262/language/statements/for-of/continue-from-finally.js
    test262/language/statements/for-of/body-put-error.js
    test262/language/statements/for-of/dstr-array-elem-iter-thrw-close-err.js
    test262/language/statements/for-of/dstr-array-elem-trlg-iter-list-thrw-close-err.js
    test262/language/statements/for-of/dstr-array-rest-put-prop-ref-user-err-iter-close-skip.js
    test262/language/statements/for-of/continue-from-catch.js
    test262/language/statements/for-of/continue.js
    test262/language/statements/for-of/dstr-array-rest-iter-thrw-close-err.js
    test262/language/statements/for-of/dstr-array-rest-lref-err.js
    test262/language/statements/for-of/dstr-array-elem-iter-thrw-close.js
    test262/language/statements/for-of/dstr-array-elem-trlg-iter-list-thrw-close.js
    test262/language/statements/for-of/body-dstr-assign-error.js
    test262/language/statements/for-of/dstr-array-rest-iter-thrw-close.js
    test262/language/statements/for-of/dstr-array-elem-trlg-iter-rest-thrw-close.js
    test262/language/expressions/yield/star-rhs-iter-thrw-violation-no-rtrn.js
    test262/language/expressions/yield/star-rhs-iter-rtrn-res-value-final.js
    test262/language/expressions/yield/star-rhs-iter-rtrn-rtrn-invoke.js
    test262/language/expressions/yield/star-rhs-iter-rtrn-res-done-no-value.js
    test262/language/expressions/yield/star-rhs-iter-thrw-thrw-call-non-obj.js
    test262/language/expressions/yield/star-rhs-iter-rtrn-res-value-err.js
    test262/language/expressions/assignment/dstr-array-elem-trlg-iter-rest-thrw-close-err.js
    test262/language/expressions/assignment/dstr-array-rest-nested-array-iter-thrw-close-skip.js
    test262/language/expressions/assignment/dstr-array-elem-iter-thrw-close-err.js
    test262/language/expressions/assignment/dstr-array-elem-trlg-iter-list-thrw-close-err.js
    test262/language/expressions/assignment/dstr-array-rest-put-prop-ref-user-err-iter-close-skip.js
    test262/language/expressions/assignment/dstr-array-rest-iter-thrw-close-err.js
    test262/language/expressions/assignment/dstr-array-rest-lref-err.js
    test262/language/expressions/assignment/dstr-array-elem-iter-thrw-close.js
    test262/language/expressions/assignment/dstr-array-elem-trlg-iter-list-thrw-close.js
    test262/language/expressions/assignment/dstr-array-rest-iter-thrw-close.js
    test262/language/expressions/assignment/dstr-array-elem-trlg-iter-rest-thrw-close.js

Comment 29

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #24)
> Comment on attachment 8824291 [details] [diff] [review]
> Implement IteratorClose for for-of.
> 
> Review of attachment 8824291 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great :)
> 
> only some comment fixes.
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +289,5 @@
> > +    // IteratorClose. Since non-local jumps like break and return call
> > +    // IteratorClose, whenever a non-local jump is emitted, we must terminate
> > +    // the current JSTRY_ITERCLOSE note to skip the non-local jump code, then
> > +    // start a new one.
> > +    ptrdiff_t iterCloseTryStart_;
> 
> This needs some diagram or example that describes what "terminate the
> current note" and "start a new one" mean, also, something that clarifies
> that startNewIterCloseTryNote should be called after finishIterCloseTryNote.
> 
> how about following?
> 
> 
>   for (x of y) {
>       ...             instantiate ForOfLoopControl
>       ...         +   <-- iterCloseTryStart_ points here
>       ...         ^
>       ...         |
>       if (...)    v
>                   +   call finishIterCloseTryNote before |break|
>                       above range is noted with JSTRY_ITERCLOSE
> 
>         break;    <-- break and IteratorClose are not inside JSTRY_ITERCLOSE
> note
> 
>                       call startNewIterCloseTryNote after |break|
>                   +   <-- next iterCloseTryStart_ points here
>       ...         |
>       ...         ~
>   }
> 

Beautiful, thank you for the diagram. :)

Comment 30

[Shu-yu Guo [:shu]]

Attachment: Implement IteratorClose for for-of.

Non-local jumps, i.e. |break| and |return| statements, are implemented
by emitting IteratorClose in bytecode.

On throws, the new trynote JSTRY_ITERCLOSE signals that there's an
iterator on the top of the stack that needs IteratorClose. Note that
this only applies to exceptions thrown from non-iterator code. If
iter.next or iter.return itself throws, IteratorClose should *not* be
called. This is why we can't use JSTRY_FOR_OF.

Comment 31

[Shu-yu Guo [:shu]]

Attachment: Implement calling IteratorClose and "return" on iterators in yield*.

This and the for-of parts could use re-review. Had to fix some correctness bugs
in |continue| and forced return that anba pointed out.

Comment 32

[Shu-yu Guo [:shu]]

I haven't gotten through all the test262 failures yet -- early next week at the latest.

Comment 33

[Tooru Fujisawa [:arai]]

Comment on attachment 8824622 [details] [diff] [review]
Implement IteratorClose for for-of.

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

Great :D

::: js/src/frontend/BytecodeEmitter.cpp
@@ +301,5 @@
> +    //     if (...)    v
> +    //                 +   call finishIterCloseTryNote before |break|
> +    //                     above range is noted with JSTRY_ITERCLOSE
> +    //
> +    //       break;    <-- break and IteratorClose are not inside JSTRY_ITERCLOSE note

sorry, can you wrap this line to 79 chars?

@@ +2242,5 @@
> +
> +        // A 'break' or 'return' statement does call IteratorClose for the
> +        // loop it is breaking out of or returning from, i.e. including the
> +        // target loop.
> +        EmitIteratorCloseExcludingAtTarget

Looks like the comments are swapped between EmitIteratorCloseIncludingAtTarget and EmitIteratorCloseExcludingAtTarget.

How about using throw/continue/break-or-return in these names instead?
with enum NonLocalExitKind typename or something, so it's clear that when to specify which kind.

and also maybe adding helper function/method like shouldEmitIteratorCloseForTarget/shouldEmitIteratorCloseForInnerLoop that receive the enum value and returns bool, to use in prepareForNonLocalJump.

@@ +2361,5 @@
> +                    return false;
> +            } else {
> +                if (!bce_->emit1(JSOP_POP))               // ... ITER
> +                    return false;
> +                if (!bce_->emit1(JSOP_POP))               // ...

emitUint16Operand(JSOP_POPN, 2) maybe?

Comment 34

[Tooru Fujisawa [:arai]]

Comment on attachment 8824623 [details] [diff] [review]
Implement calling IteratorClose and "return" on iterators in yield*.

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

looks almost good, except the stack depth in checkThrow branch.
also forwarding review to jandem.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +8060,1 @@
>      // THROW? = 'throw' in ITER

this comment is obsolete.

@@ +8075,2 @@
>      JumpList checkThrow;
> +    if (!emitJump(JSOP_IFEQ, &checkThrow))                       // ITER RESULT EXCEPTION ITER THROW

it would be nice if we can use IfThenElseEmitter here.

@@ +8082,1 @@
>          return false;

This branch hits `Assertion failure: stackDepth == depth` in js/src/jit/BytecodeAnalysis.h:30.

since the stack doesn't balance.
we may need to push undefined or something to balance the stack depth (or perhaps there's some trick to avoid that?)

@@ +8084,1 @@
>      if (!emitJumpTargetAndPatch(checkThrow))                     // delegate:

since "delegate" comment is removed above, we could move the stack comment " // ITER OLDRESULT EXCEPTION ITER THROW" from the next line to here.

Comment 35

[Jan de Mooij [:jandem]]

Comment on attachment 8824623 [details] [diff] [review]
Implement calling IteratorClose and "return" on iterators in yield*.

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

JSOP_ISGENCLOSING is added here and used in the next patch, but that's fine.

::: js/src/jit/BaselineCompiler.cpp
@@ +4006,5 @@
>  
>  bool
> +BaselineCompiler::emit_JSOP_ISGENCLOSING()
> +{
> +    return emit_JSOP_ISNOITER();

Since JSOP_ISGENCLOSING and JSOP_ISNOITER are pretty much unrelated, it would be nice to refactor this so we have emitIsMagicValue() or something, and then call that for both JSOP_ISGENCLOSING and JSOP_ISNOITER.

::: js/src/jit/IonBuilder.cpp
@@ +2195,1 @@
>          return jsop_isnoiter();

This will fail the following assert in IonBuilder::jsop_isnoiter:

    MOZ_ASSERT(def->isIteratorMore());

We should also rename the MIsNoIter stuff etc... Can we actually compile scripts with this op in Ion?

Comment 36

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #33)
> Comment on attachment 8824622 [details] [diff] [review]
> Implement IteratorClose for for-of.
> 
> Review of attachment 8824622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great :D
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +301,5 @@
> > +    //     if (...)    v
> > +    //                 +   call finishIterCloseTryNote before |break|
> > +    //                     above range is noted with JSTRY_ITERCLOSE
> > +    //
> > +    //       break;    <-- break and IteratorClose are not inside JSTRY_ITERCLOSE note
> 
> sorry, can you wrap this line to 79 chars?
> 
> @@ +2242,5 @@
> > +
> > +        // A 'break' or 'return' statement does call IteratorClose for the
> > +        // loop it is breaking out of or returning from, i.e. including the
> > +        // target loop.
> > +        EmitIteratorCloseExcludingAtTarget
> 
> Looks like the comments are swapped between
> EmitIteratorCloseIncludingAtTarget and EmitIteratorCloseExcludingAtTarget.
> 
> How about using throw/continue/break-or-return in these names instead?
> with enum NonLocalExitKind typename or something, so it's clear that when to
> specify which kind.
> 
> and also maybe adding helper function/method like
> shouldEmitIteratorCloseForTarget/shouldEmitIteratorCloseForInnerLoop that
> receive the enum value and returns bool, to use in prepareForNonLocalJump.

Sure, I'll add a Kind enum.

> 
> @@ +2361,5 @@
> > +                    return false;
> > +            } else {
> > +                if (!bce_->emit1(JSOP_POP))               // ... ITER
> > +                    return false;
> > +                if (!bce_->emit1(JSOP_POP))               // ...
> 
> emitUint16Operand(JSOP_POPN, 2) maybe?

I chose |pop; pop| because it's 2 bytes, and |popn 2| is 3 bytes.

Comment 37

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #35)
> Comment on attachment 8824623 [details] [diff] [review]
> Implement calling IteratorClose and "return" on iterators in yield*.
> 
> Review of attachment 8824623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> JSOP_ISGENCLOSING is added here and used in the next patch, but that's fine.
> 
> ::: js/src/jit/BaselineCompiler.cpp
> @@ +4006,5 @@
> >  
> >  bool
> > +BaselineCompiler::emit_JSOP_ISGENCLOSING()
> > +{
> > +    return emit_JSOP_ISNOITER();
> 
> Since JSOP_ISGENCLOSING and JSOP_ISNOITER are pretty much unrelated, it
> would be nice to refactor this so we have emitIsMagicValue() or something,
> and then call that for both JSOP_ISGENCLOSING and JSOP_ISNOITER.
> 

Will do this refactoring.

> ::: js/src/jit/IonBuilder.cpp
> @@ +2195,1 @@
> >          return jsop_isnoiter();
> 
> This will fail the following assert in IonBuilder::jsop_isnoiter:
> 
>     MOZ_ASSERT(def->isIteratorMore());
> 
> We should also rename the MIsNoIter stuff etc... Can we actually compile
> scripts with this op in Ion?

Good point. GENCLOSING is only emitted inside a finally block with this patch, so Ion will never compile it anyways. I'll remove that line from IonBuilder.

Comment 38

[Shu-yu Guo [:shu]]

Attachment: As pointed out by test262 failures, the previous implementation for

destructuring was wrong. The new implementation always keeps the iter obj and
the done value on the stack. There's a new JSTRY_DESTRUCTURING_ITERCLOSE that
unlike the for-of JSTRY_ITERCLOSE, only calls IteratorClose if !done.

Comment 39

[Shu-yu Guo [:shu]]

Attachment: The old version didn't correctly throw when doing a forced return in a

generator, since we implement that as an uncatchable magic exception.

Comment 40

[Shu-yu Guo [:shu]]

Locally, all the previously failing IteratorClose test262 tests now pass.

Comment 41

[Tooru Fujisawa [:arai]]

Comment on attachment 8825589 [details] [diff] [review]
As pointed out by test262 failures, the previous implementation for

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

Thank you for cleanup on array destructuring code :)

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5123,5 @@
>      //       value = result.value;
>      //   }
>      //
>      //   if (value === undefined)
>      //     value = y;

evaluating default (y here) is covered by trynote.
(only comment fix)

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b28ef89ebda2
Implement IteratorClose for for-of. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1304f6c59821
Rename allowContentSpread to allowContentIter. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/314efb239b64
Convert self-hosted code that need to call IteratorClose to use for-of. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7872a2456195
Implement IteratorClose for array destructuring. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9eef2331ae6
Implement calling IteratorClose and "return" on iterators in yield*. (r=jandem)
https://hg.mozilla.org/integration/mozilla-inbound/rev/408a37107c7f
Implement JSOP_PICK and JSOP_UNPICK in the expression decompiler. (r=arai)

Comment 43

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6584da54c13d
Backed out changeset 408a37107c7f 
https://hg.mozilla.org/integration/mozilla-inbound/rev/076bdd3f1f7a
Backed out changeset d9eef2331ae6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5fc270c79c
Backed out changeset 7872a2456195 
https://hg.mozilla.org/integration/mozilla-inbound/rev/3db3783de0f2
Backed out changeset 314efb239b64 
https://hg.mozilla.org/integration/mozilla-inbound/rev/831c8cfd2512
Backed out changeset 1304f6c59821 
https://hg.mozilla.org/integration/mozilla-inbound/rev/feb27da5b04c
Backed out changeset b28ef89ebda2 since it seems this made browser_console_addonsdk_loader_exception.js | Test timed out - more worse

Comment 44

[Carsten Book [:Tomcat]]

backed out because seems this somehow caused https://treeherder.mozilla.org/logviewer.html#?job_id=68732808&repo=mozilla-inbound failing a lot

Comment 45

[Shu-yu Guo [:shu]]

Attachment: Always decompile argument names in self-hosted code in the caller frame.

The devtools failure was caused by the conversion of self-hosted code from
using manual while loops to for-of. In the manual while loop version, we'd
throw JSMSG_NOT_ITERABLE with DecompileArg(0, iterable), which decompiles the
argument name correctly. In for-of, the error is generated in bytecode on the
argument, resulting in a different error message.

For example, |new Map(42)| used to throw "42 is not iterable", and now throws
"iterable is not iterable", because the 1st argument in the self-hosted
MapConstructorInit is named |iterable|.

This patch changes the behavior of the expression decompiler in self-hosted
scripts to *always* try decompiling argument slots one frame up.

Comment 46

[Tooru Fujisawa [:arai]]

Comment on attachment 8826786 [details] [diff] [review]
Always decompile argument names in self-hosted code in the caller frame.

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

::: js/src/jsopcode.cpp
@@ +1272,5 @@
> +            // not succeed.
> +            if (result) {
> +                if (!write(result))
> +                    return false;
> +                js_free(result);

`result` should be freed even if `write` fails.

Comment 47

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #46)
> Comment on attachment 8826786 [details] [diff] [review]
> Always decompile argument names in self-hosted code in the caller frame.
> 
> Review of attachment 8826786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsopcode.cpp
> @@ +1272,5 @@
> > +            // not succeed.
> > +            if (result) {
> > +                if (!write(result))
> > +                    return false;
> > +                js_free(result);
> 
> `result` should be freed even if `write` fails.

Good catch. Thanks!

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/757b50c0ee48
Implement IteratorClose for for-of. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad6c93e5162
Rename allowContentSpread to allowContentIter. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d332a5b2e8
Convert self-hosted code that need to call IteratorClose to use for-of. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce293b3c0a8b
Implement IteratorClose for array destructuring. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0dc4150f8ac
Implement calling IteratorClose and "return" on iterators in yield*. (r=jandem)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b9c4069ed0f
Implement JSOP_PICK and JSOP_UNPICK in the expression decompiler. (r=arai)
https://hg.mozilla.org/integration/mozilla-inbound/rev/142dbb4bffc0
Always decompile argument names in self-hosted code in the caller frame. (r=arai)

Comment 49

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/757b50c0ee48
https://hg.mozilla.org/mozilla-central/rev/8ad6c93e5162
https://hg.mozilla.org/mozilla-central/rev/d7d332a5b2e8
https://hg.mozilla.org/mozilla-central/rev/ce293b3c0a8b
https://hg.mozilla.org/mozilla-central/rev/e0dc4150f8ac
https://hg.mozilla.org/mozilla-central/rev/0b9c4069ed0f
https://hg.mozilla.org/mozilla-central/rev/142dbb4bffc0

Comment 50

[Florian Scholz (Open Web Docs)]

Added to https://developer.mozilla.org/en-US/Firefox/Releases/53#JavaScript
Updated https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

Not entirely sure if this is sufficient. Further improvements to the MDN wiki pages appreciated.