Implement IteratorClose

RESOLVED FIXED in Firefox 53

Status

()

P1
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: evilpie, Assigned: shu)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla53
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

Attachments

(7 attachments, 4 obsolete attachments)

8.34 KB, patch
arai
: review+
Details | Diff | Splinter Review
11.30 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.01 KB, patch
arai
: review+
Details | Diff | Splinter Review
51.10 KB, patch
arai
: review+
Details | Diff | Splinter Review
33.92 KB, patch
arai
: review+
Details | Diff | Splinter Review
22.43 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.04 KB, patch
arai
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Iterators in ES6 now have returning/closing semantics like we have for our old generators.
Depends on: 1180306
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
Duplicate of this bug: 1163348
(Reporter)

Comment 3

3 years ago
Somebody needs to start looking into this. We are missing quite a lot of points on kangax just because of this.
We haven't implemented because TC39 keeps hovering on the edge of removing it from the standard.
Keywords: dev-doc-needed

Comment 5

3 years ago
Can we get an update on this?
Depends on: 1115868
(Reporter)

Comment 6

2 years ago
Is somebody interested in implementing this? This is definitely one of the biggest missing pieces that we have left for ES6.
(Assignee)

Updated

2 years ago
Assignee: nobody → shu
(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.
Priority: -- → P2
(Assignee)

Comment 8

2 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.
(Reporter)

Updated

2 years ago
Blocks: 652780
(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?
Priority: P2 → P1
IteratorClose in built-in are implemented in bug 1180306.

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

all those are now handled in bytecode.
See Also: → bug 1322069
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1326109

Comment 12

2 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.
(Assignee)

Comment 13

2 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!
(Assignee)

Comment 14

2 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?
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 16

2 years ago
Created attachment 8824291 [details] [diff] [review]
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.
Attachment #8824291 - Flags: review?(arai.unmht)
(Assignee)

Comment 17

2 years ago
Created attachment 8824292 [details] [diff] [review]
Rename allowContentSpread to allowContentIter.

This is to prepare for reimplementing several self-hosted methods to use
for-of.
Attachment #8824292 - Flags: review?(arai.unmht)
(Assignee)

Comment 18

2 years ago
Created attachment 8824293 [details] [diff] [review]
Convert self-hosted code that need to call IteratorClose to use for-of.
Attachment #8824293 - Flags: review?(arai.unmht)
(Assignee)

Comment 19

2 years ago
Created attachment 8824294 [details] [diff] [review]
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.
Attachment #8824294 - Flags: review?(arai.unmht)
(Assignee)

Comment 20

2 years ago
Created attachment 8824295 [details] [diff] [review]
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.
Attachment #8824295 - Flags: review?(jdemooij)
(Assignee)

Comment 21

2 years ago
Created attachment 8824296 [details] [diff] [review]
Implement JSOP_PICK and JSOP_UNPICK in the expression decompiler.
Attachment #8824296 - Flags: review?(arai.unmht)
(Assignee)

Comment 22

2 years ago
Jan, the bytecode emitted for the yield* is found in emitIteratorClose in the for-of patch. Forgot to split it out, sorry.
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.
Attachment #8824296 - Flags: review?(arai.unmht) → review+
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.
Attachment #8824291 - Flags: review?(arai.unmht) → review+
Attachment #8824292 - Flags: review?(arai.unmht) → review+
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.
Attachment #8824293 - Flags: review?(arai.unmht) → review+
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.
Attachment #8824294 - Flags: review?(arai.unmht) → review+
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);|
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
(Assignee)

Comment 29

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

Comment 30

2 years ago
Created attachment 8824622 [details] [diff] [review]
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.
Attachment #8824622 - Flags: review?(arai.unmht)
(Assignee)

Updated

2 years ago
Attachment #8824291 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
Created attachment 8824623 [details] [diff] [review]
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.
Attachment #8824623 - Flags: review?(arai.unmht)
(Assignee)

Updated

2 years ago
Attachment #8824295 - Attachment is obsolete: true
Attachment #8824295 - Flags: review?(jdemooij)
(Assignee)

Comment 32

2 years ago
I haven't gotten through all the test262 failures yet -- early next week at the latest.
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?
Attachment #8824622 - Flags: review?(arai.unmht) → review+
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.
Attachment #8824623 - Flags: review?(jdemooij)
Attachment #8824623 - Flags: review?(arai.unmht)
Attachment #8824623 - Flags: feedback+
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?
Attachment #8824623 - Flags: review?(jdemooij)
(Assignee)

Comment 36

2 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.
(Assignee)

Comment 37

2 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.
(Assignee)

Comment 38

2 years ago
Created attachment 8825589 [details] [diff] [review]
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.
Attachment #8825589 - Flags: review?(arai.unmht)
(Assignee)

Updated

2 years ago
Attachment #8824294 - Attachment is obsolete: true
(Assignee)

Comment 39

2 years ago
Created attachment 8825591 [details] [diff] [review]
The old version didn't correctly throw when doing a forced return in a

generator, since we implement that as an uncatchable magic exception.
Attachment #8825591 - Flags: review?(jdemooij)
(Assignee)

Updated

2 years ago
Attachment #8824623 - Attachment is obsolete: true
(Assignee)

Comment 40

2 years ago
Locally, all the previously failing IteratorClose test262 tests now pass.
Attachment #8825591 - Flags: review?(jdemooij) → review+
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)
Attachment #8825589 - Flags: review?(arai.unmht) → review+

Comment 42

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

Comment 43

2 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
backed out because seems this somehow caused https://treeherder.mozilla.org/logviewer.html#?job_id=68732808&repo=mozilla-inbound failing a lot
Flags: needinfo?(shu)
(Assignee)

Comment 45

2 years ago
Created attachment 8826786 [details] [diff] [review]
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.
Attachment #8826786 - Flags: review?(arai.unmht)
(Assignee)

Updated

2 years ago
Flags: needinfo?(shu)
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.
Attachment #8826786 - Flags: review?(arai.unmht) → review+
(Assignee)

Comment 47

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

Comment 48

2 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)
Depends on: 1331444
Depends on: 1332155
Depends on: 1332881
Depends on: 1333946
Depends on: 1334799
Depends on: 1338796
Depends on: 1342049
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.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1346862
Depends on: 1360839
Depends on: 1357075
You need to log in before you can comment on or make changes to this bug.