Tom S [:evilpie] Reporter
	Description • 9 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 1 • 9 years ago

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

	Tom S [:evilpie] Reporter
	Comment 3 • 8 years ago

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

	Jason Orendorff [:jorendorff]
	Comment 4 • 8 years ago

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

	Kyle Simpson
	Comment 5 • 8 years ago

Can we get an update on this?

	Tom S [:evilpie] Reporter
	Comment 6 • 8 years ago

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

	Hannes Verschore [:h4writer]
	Comment 7 • 8 years ago

(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.

	Shu-yu Guo [:shu] Assignee
	Comment 8 • 8 years ago

(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.

	Hannes Verschore [:h4writer]
	Comment 9 • 8 years ago

(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?

	Tooru Fujisawa [:arai]
	Comment 10 • 8 years ago

IteratorClose in built-in are implemented in bug 1180306.

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

all those are now handled in bytecode.

	Rob Wu [:robwu]
	Comment 12 • 7 years ago

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.

	Shu-yu Guo [:shu] Assignee
	Comment 13 • 7 years ago

(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!

	Shu-yu Guo [:shu] Assignee
	Comment 14 • 7 years ago

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?

	Jan de Mooij [:jandem]
	Comment 15 • 7 years ago

(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.

	Shu-yu Guo [:shu] Assignee
	Comment 16 • 7 years ago

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.

	Shu-yu Guo [:shu] Assignee
	Comment 17 • 7 years ago

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

	Shu-yu Guo [:shu] Assignee
	Comment 19 • 7 years ago

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.

	Shu-yu Guo [:shu] Assignee
	Comment 20 • 7 years ago

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.

	Shu-yu Guo [:shu] Assignee
	Comment 22 • 7 years ago

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

	Tooru Fujisawa [:arai]
	Comment 23 • 7 years ago

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.

	Tooru Fujisawa [:arai]
	Comment 24 • 7 years ago

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.

	Tooru Fujisawa [:arai]
	Comment 25 • 7 years ago

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.

	Tooru Fujisawa [:arai]
	Comment 26 • 7 years ago

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.

	André Bargull [:anba]
	Comment 27 • 7 years ago

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);|

	André Bargull [:anba]
	Comment 28 • 7 years ago

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

	Shu-yu Guo [:shu] Assignee
	Comment 29 • 7 years ago

(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. :)

	Shu-yu Guo [:shu] Assignee
	Comment 30 • 7 years ago

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.

	Shu-yu Guo [:shu] Assignee
	Comment 31 • 7 years ago

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.

	Shu-yu Guo [:shu] Assignee
	Comment 32 • 7 years ago

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

	Tooru Fujisawa [:arai]
	Comment 33 • 7 years ago

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?

	Tooru Fujisawa [:arai]
	Comment 34 • 7 years ago

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.

	Jan de Mooij [:jandem]
	Comment 35 • 7 years ago

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?

	Shu-yu Guo [:shu] Assignee
	Comment 36 • 7 years ago

(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.

	Shu-yu Guo [:shu] Assignee
	Comment 37 • 7 years ago

(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.

	Shu-yu Guo [:shu] Assignee
	Comment 38 • 7 years ago

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.

	Shu-yu Guo [:shu] Assignee
	Comment 39 • 7 years ago

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

	Shu-yu Guo [:shu] Assignee
	Comment 40 • 7 years ago

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

	Tooru Fujisawa [:arai]
	Comment 41 • 7 years ago

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)

	Pulsebot
	Comment 42 • 7 years ago

Pushed by shu@rfrn.org:
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)

	Pulsebot
	Comment 43 • 7 years ago

Backout by cbook@mozilla.com:
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

	Carsten Book [:Tomcat]
	Comment 44 • 7 years ago

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

	Shu-yu Guo [:shu] Assignee
	Comment 45 • 7 years ago

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.

	Tooru Fujisawa [:arai]
	Comment 46 • 7 years ago

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.

	Shu-yu Guo [:shu] Assignee
	Comment 47 • 7 years ago

(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!

	Pulsebot
	Comment 48 • 7 years ago

Pushed by shu@rfrn.org:
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)

	Phil Ringnalda (:philor)
	Comment 49 • 7 years ago
bugherder

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

	Florian Scholz (Open Web Docs)
	Comment 50 • 7 years ago

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.