1184922 - Array destructuring assignment/binding calls next() after iterator completed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Array destructuring assignment and binding must not call next() on the iterator after the iteration has finished. The ES2015 spec uses a `{[[iterator]]: iterator, [[done]]: false}` record to keep track of the current iterator state. 

I'm not sure if the plan is to fix this issue as part of bug 1147371, or if it makes sense to keep it separate from IteratorClose. In the former case this bug report should be closed as a dup, obviously.


Test cases:
---
var [a, b] = {
  [Symbol.iterator]() {
    return this;
  },
  next() {
    print("called next!");
    return {value: 0, done: true};
  }
};

void ([a, b] = {
  [Symbol.iterator]() {
    return this;
  },
  next() {
    print("called next!");
    return {value: 0, done: true};
  }
});
---

Expected: "called next!" is called once per each array destructuring
Actual: "called next!" is called twice

Comment 1

[Tooru Fujisawa [:arai]]

we should keep previous RESULT.done throughout the destructuring, and avoid calling next() if RESULT.done==true.
it could be done by tweaking bytecode there.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Do not call iter.next() if the previous iter.next().done was true in array destructuring.

In the spec [1], `iter.next()` is called inside `IteratorStep`, only if current `iteratorRecord.[[Done]]` value is `false`.
the value is updated to `true` when `IteratorStep` returns `false`.

So, we should keep a boolen value that corresponds to `iteratorRecord.[[Done]]` at the middle of the stack throughout the array destructuring.


Changed the Array destructuring bytecode to keep the last `RESULT.done` value on the stack while assignment, and check the `RESULT.done` value after that, and jump to next element's `RESULT.done == true` case if iterator already completes.

For rest element, added an extra code branch to create an empty array, for the case iterator already completes.


Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4cc8377fb12

[1] http://www.ecma-international.org/ecma-262/7.0/#sec-destructuring-binding-patterns-runtime-semantics-iteratorbindinginitialization

Comment 3

[Tooru Fujisawa [:arai]]

Here are bytecode examples.
Opcode marked with "*" are added by this patch.

[code]
  x => { var [a, b, c] = x }

[bytecode]
    getarg 0             # obj
    dup                  # obj obj
    dup                  # obj obj obj
    symbol 1             # obj obj obj @@iterator
    callelem             # obj obj obj[@@iterator]
    swap                 # obj obj[@@iterator] a
    calliter 0           # obj iter
    dup                  # obj iter iter
    dup                  # obj iter iter iter
    callprop "next"      # obj iter iter next
    swap                 # obj iter next iter
    call 0               # obj iter R
    dup                  # obj iter R R
    getprop "done"       # obj iter R done
*   swap                 # obj iter done R          -- keep extra `done` at
*   dupat 1              # obj iter done R done     -- the middle of the stack
    ifeq A-NOT-DONE      # obj iter done R

A-DONE:
    jumptarget           # obj iter done R
    pop                  # obj iter done
    undefined            # obj iter done undefined
    nop-destructuring    # obj iter done undefined
    goto A-SET           # obj iter done undefined

A-NOT-DONE:
    jumptarget           # obj iter done R
    getprop "value"      # obj iter done value

A-SET:
    jumptarget           # obj iter done value
    setlocal 0           # obj iter done value
    pop                  # obj iter done
*   dup                  # obj iter done done       -- skip next() call if
*   not                  # obj iter done !done      -- iterator already
*   ifeq B-COMPLETE      # obj iter done            -- completes

B-NEXT:
*   jumptarget           # obj iter done
    pop                  # obj iter
    dup                  # obj iter iter
    dup                  # obj iter iter iter
    callprop "next"      # obj iter iter next
    swap                 # obj iter next iter
    call 0               # obj iter R
    dup                  # obj iter R R
    getprop "done"       # obj iter R done
*   swap                 # obj iter done R
*   dupat 1              # obj iter done R done
    ifeq B-NOT-DONE      # obj iter done R

B-DONE:
    jumptarget           # obj iter done R
    pop                  # obj iter done

B-COMPLETE:
*   jumptarget           # obj iter done
    undefined            # obj iter done undefined
    nop-destructuring    # obj iter done undefined
    goto B-SET           # obj iter done undefined

B-NOT-DONE:
    jumptarget           # obj iter done R
    getprop "value"      # obj iter done value

B-SET:
    jumptarget           # obj iter done value
    setlocal 1           # obj iter done value
    pop                  # obj iter done
*   dup                  # obj iter done done
*   not                  # obj iter done !done
*   ifeq C-COMPLETE      # obj iter done

C-NEXT:
*   jumptarget           # obj iter done
    pop                  # obj iter
    dup                  # obj iter iter
    dup                  # obj iter iter iter
    callprop "next"      # obj iter iter next
    swap                 # obj iter next iter
    call 0               # obj iter R
    dup                  # obj iter R R
    getprop "done"       # obj iter R done
*   swap                 # obj iter done R
*   dupat 1              # obj iter done R done
    ifeq C-NOT-DONE      # obj iter done R

C-DONE:
    jumptarget           # obj iter done R
    pop                  # obj iter done

C-COMPLETE:
*   jumptarget           # obj iter done
    undefined            # obj iter done undefined
    nop-destructuring    # obj iter done undefined
    goto C-SET           # obj iter done undefined

C-NOT-DONE:
    jumptarget           # obj iter done R
    getprop "value"      # obj iter done value

C-SET:
    jumptarget           # obj iter done value
    setlocal 2           # obj iter done value
    pop                  # obj iter done
*   pop                  # obj iter                 -- pop kept `done`
    pop                  # obj
    pop                  #
    retrval              #
    retrval              #


[code]
  x => { var [a, ...b] = x }

[bytecode]
    getarg 0             # obj
    dup                  # obj obj
    dup                  # obj obj obj
    symbol 1             # obj obj obj @@iterator
    callelem             # obj obj obj[@@iterator]
    swap                 # obj obj[@@iterator] a
    calliter 0           # obj iter
    dup                  # obj iter iter
    dup                  # obj iter iter iter
    callprop "next"      # obj iter iter next
    swap                 # obj iter next iter
    call 0               # obj iter R
    dup                  # obj iter R R
    getprop "done"       # obj iter R done
*   swap                 # obj iter done R
*   dupat 1              # obj iter done R done
    ifeq A-NOT-DONE      # obj iter done R

A-DONE:
    jumptarget           # obj iter done R
    pop                  # obj iter done
    undefined            # obj iter done undefined
    nop-destructuring    # obj iter done undefined
    goto A-SET           # obj iter done undefined

A-NOT-DONE:
    jumptarget           # obj iter done R
    getprop "value"      # obj iter done value

A-SET:
    jumptarget           # obj iter done value
    setlocal 0           # obj iter done value
    pop                  # obj iter done
*   dup                  # obj iter done done
*   not                  # obj iter done !done
*   ifeq B-COMPLETE      # obj iter done

B-DONE:
*   jumptarget           # obj iter done
    pop                  # obj iter
    newarray 0           # obj iter array
    zero                 # obj iter array 0

    SPREAD CODES HERE    # obj array n

    pop                  # obj array
*   goto B-SET           # obj array

B-COMPLETE:
*   jumptarget           # obj iter done            -- when iterator completes,
*   pop                  # obj iter                 -- remove all unnecessary
*   pop                  # obj                      -- items from stack,
*   newarray 0           # obj array                -- and push an empty array

B-SET:
*   jumptarget           # obj array
    setlocal 1           # obj array
    pop                  # obj
    pop                  #
    retrval              #

Comment 4

[Jason Orendorff [:jorendorff]]

I have not even looked at this patch yet, but I think the boolean should be implied by the pc rather than an actual stack slot. The behavior would be like this:

    // `var [a, b, c] = x;` results in bytecode like this

    var a = undefined, b = undefined, c = undefined;
    let iter = x[@@iterator]();
    let obj = iter.next();
    if (!obj.done) {
        a = obj.value;
        obj = iter.next();
        if (!obj.done) {
            b = obj.value;
            obj = iter.next();
            if (!obj.done) {
                init c = obj.value;
                //iter.return();
            }
        }
    }

With `let` there's more work, because you need to initialize the variables if obj.done is true:

    // `let [a, b, c] = x;` results in bytecode like this

    let a, b, c;
    let iter = x[@@iterator]();
    let obj = iter.next();
    if (obj.done) goto DoneA;
    init a = obj.value;
    obj = iter.next();
    if (obj.done) goto DoneB;
    init b = obj.value;
    obj = iter.next();
    if (obj.done) goto DoneC;
    init c = obj.value;
    //iter.return();
    goto Done;

    DoneA: init a = undefined;
    DoneB: init b = undefined;
    DoneC: init c = undefined;
    Done:

Comment 5

[Jason Orendorff [:jorendorff]]

Note: The point of doing it as shown in comment 4 would be to eliminate some conditional branches (the new JSOP_IFEQ instructions in comment 5). I haven't looked at both to see if it actually *does* eliminate any branches, though!

Comment 6

[Tooru Fujisawa [:arai]]

yeah, that will also work, but it duplicates each assignment element, and that won't be good if the element is also destructuring.

Comment 7

[Tooru Fujisawa [:arai]]

so far, we cannot emit pattern twice, as the pattern may contain function expression in default value, and we don't yet support cloning PNK_FUNCTION, nor emitting it twice.

Comment 8

[Tooru Fujisawa [:arai]]

Now with new frontend, is there any way to emit pattern (that includes arbitrary expression in default value) twice?
I hit assertions for function expression:

https://dxr.mozilla.org/mozilla-central/rev/26e22af660e543ebb69930f082188b69ec756185/js/src/frontend/BytecodeEmitter.cpp#6706
>        MOZ_ASSERT(pn->functionIsHoisted());

and regexp:

https://dxr.mozilla.org/mozilla-central/rev/26e22af660e543ebb69930f082188b69ec756185/js/src/frontend/BytecodeEmitter.cpp#9689
>        MOZ_ASSERT(!*cursor);

Comment 9

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #8)
> Now with new frontend, is there any way to emit pattern (that includes
> arbitrary expression in default value) twice?
> I hit assertions for function expression:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 26e22af660e543ebb69930f082188b69ec756185/js/src/frontend/BytecodeEmitter.
> cpp#6706
> >        MOZ_ASSERT(pn->functionIsHoisted());

I think you can change the condition from

if (funbox->wasEmitted) {

to

if (funbox->wasEmitted && pn->functionIsHoisted()) {

See if that works.

> 
> and regexp:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 26e22af660e543ebb69930f082188b69ec756185/js/src/frontend/BytecodeEmitter.
> cpp#9689
> >        MOZ_ASSERT(!*cursor);

I'm not familiar with this assert, sorry.

Comment 10

[Tooru Fujisawa [:arai]]

Thank you :)

when I add `pn->functionIsHoisted()`, function expression also hits the `MOZ_ASSERT(!*cursor);`.
will check if it can be fixed somehow.

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8767549 [details] [diff] [review]
Do not call iter.next() if the previous iter.next().done was true in array destructuring.

will post updated patch shortly

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Add BytecodeEmitter.emittingSecondTime and AutoEmittingSecondTime to make it possible to emit ParseNode twice.

To emit pattern twice, we need to make it possible to emit all expression.

There are following 2 issues in current implementation:
  (a) object cannot be emitted twice, because objectList cannot have duplicated entry
  (b) function expression cannot be emitted twice, just because not supposed

for (a), changed objectList to be able to reuse already-added object's index for same object.
  * added CGObjectList::addIfNotFound
    add given object to list if it's not in the list, and returns new item's index
    if it's already in the list, returns the index for the existing item
  * added AutoEmittingSecondTime class, that changes BytecodeEmitter's internal state
    BytecodeEmitter.emittingSecondTime to true.
    while it's true, we use CGObjectList::addIfNotFound instead of CGObjectList::add to add object to objectList

for (b), applied the change in comment #9, and also added |!pn->isOp(JSOP_FUNWITHPROTO)| condition, as JSOP_FUNWITHPROTO also means |!pn->functionIsHoisted()|.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Do not call iter.next() if the previous iter.next().done was true in array destructuring.

changed the bytecode to follow comment #4.

Also, added SRC_ARRAY_DEST note and IonBuilder::processForwardJump to support simple forward jump in Ion.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 1.1: Update existing test to follow the change in array destructuring with completed iterator.

Fixed existing testcase that does not follow the spec.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add a test for array destructuring with completed iterator.

Added simple testcase for completed iterator.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add a test for array destructuring with complicated default values.

Added emitter testcase for array destructuring, for:
  * emitting in several context
  * emitting expression twice

I separated them into multiple files, as it takes long time.

Comment 17

[Shu-yu Guo [:shu]]

Comment on attachment 8795188 [details] [diff] [review]
Part 0: Add BytecodeEmitter.emittingSecondTime and AutoEmittingSecondTime to make it possible to emit ParseNode twice.

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

I'm not a fan of a BCE flag + RAII here, seems hard to manage. I think the approach could be simplified here. So what poses a problem for remitting parse trees is the uniqueness invariant in ObjectList. This is a per-ObjectBox property. Could you simply check if the ObjectBox or FunctionBox has already been emitted by having a non-null emitLink? If an object has already been emitted, it must already be in the ObjectList, in which case you can use ObjectList::indexOf to find the index. If it hasn't been emitted yet, you can add as usual. This removes the need for a flag + RAII + extra method on ObjectList and should be simpler.

Let me know if this works.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +6679,5 @@
>       * Set the |wasEmitted| flag in the funbox once the function has been
>       * emitted. Function definitions that need hoisting to the top of the
>       * function will be seen by emitFunction in two places.
>       */
> +    if (funbox->wasEmitted && !pn->isOp(JSOP_FUNWITHPROTO) && pn->functionIsHoisted()) {

I don't understand this check. If JSOP_FUNWITHPROTO is only set when !functionIsHoisted(), why do we need to check for it here? That is, how do we get pn->functionIsHoisted() and op == JSOP_FUNWITHPROTO at the same time?

::: js/src/frontend/BytecodeEmitter.h
@@ +758,5 @@
> +{
> +    BytecodeEmitter* bce_;
> +    bool prev_;
> +
> +public:

Style nit: weirdly, SM has 2 spaces here, so like

class C {
  public:
    int foo;
};

Comment 18

[Tooru Fujisawa [:arai]]

(In reply to Shu-yu Guo [:shu] from comment #17)
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +6679,5 @@
> >       * Set the |wasEmitted| flag in the funbox once the function has been
> >       * emitted. Function definitions that need hoisting to the top of the
> >       * function will be seen by emitFunction in two places.
> >       */
> > +    if (funbox->wasEmitted && !pn->isOp(JSOP_FUNWITHPROTO) && pn->functionIsHoisted()) {
> 
> I don't understand this check. If JSOP_FUNWITHPROTO is only set when
> !functionIsHoisted(), why do we need to check for it here? That is, how do
> we get pn->functionIsHoisted() and op == JSOP_FUNWITHPROTO at the same time?

!pn->isOp(JSOP_FUNWITHPROTO) is there because of the assertion in functionIsHoisted.

https://dxr.mozilla.org/mozilla-central/rev/955840bfd3c20eb24dd5a01be27bdc55c489a285/js/src/frontend/ParseNode.h#631
>         MOZ_ASSERT(isOp(JSOP_LAMBDA) ||        // lambda, genexpr
>                    isOp(JSOP_LAMBDA_ARROW) ||  // arrow function
>                    isOp(JSOP_DEFFUN) ||        // non-body-level function statement
>                    isOp(JSOP_NOP) ||           // body-level function stmt in global code
>                    isOp(JSOP_GETLOCAL) ||      // body-level function stmt in function code
>                    isOp(JSOP_GETARG) ||        // body-level function redeclaring formal
>                    isOp(JSOP_INITLEXICAL));    // block-level function stmt

maybe I just need to add JSOP_FUNWITHPROTO case there?

Comment 19

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #18)
> (In reply to Shu-yu Guo [:shu] from comment #17)
> > ::: js/src/frontend/BytecodeEmitter.cpp
> > @@ +6679,5 @@
> > >       * Set the |wasEmitted| flag in the funbox once the function has been
> > >       * emitted. Function definitions that need hoisting to the top of the
> > >       * function will be seen by emitFunction in two places.
> > >       */
> > > +    if (funbox->wasEmitted && !pn->isOp(JSOP_FUNWITHPROTO) && pn->functionIsHoisted()) {
> > 
> > I don't understand this check. If JSOP_FUNWITHPROTO is only set when
> > !functionIsHoisted(), why do we need to check for it here? That is, how do
> > we get pn->functionIsHoisted() and op == JSOP_FUNWITHPROTO at the same time?
> 
> !pn->isOp(JSOP_FUNWITHPROTO) is there because of the assertion in
> functionIsHoisted.
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 955840bfd3c20eb24dd5a01be27bdc55c489a285/js/src/frontend/ParseNode.h#631
> >         MOZ_ASSERT(isOp(JSOP_LAMBDA) ||        // lambda, genexpr
> >                    isOp(JSOP_LAMBDA_ARROW) ||  // arrow function
> >                    isOp(JSOP_DEFFUN) ||        // non-body-level function statement
> >                    isOp(JSOP_NOP) ||           // body-level function stmt in global code
> >                    isOp(JSOP_GETLOCAL) ||      // body-level function stmt in function code
> >                    isOp(JSOP_GETARG) ||        // body-level function redeclaring formal
> >                    isOp(JSOP_INITLEXICAL));    // block-level function stmt
> 
> maybe I just need to add JSOP_FUNWITHPROTO case there?

Yeah, that's what it sounds like.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Make it possible to emit ParseNode twice.

* removed RAII class and flag for emitting second time
  * moved JSOP_FUNWITHPROTO check into functionIsHoisted
  * removed CGObjectList::addIfNotFound
  * added CGObjectList.firstbox that points the first element
  * added CGObjectList::isAdded to check if the given ObjectBox is in the list
    it checks if the emitLink is non-null, or the given ObjectBox is firstbox
    first element should match firstbox, and other elements should have non-null emitLink

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Do not call iter.next() if the previous iter.next().done was true in array destructuring.

Removed AutoEmittingSecondTime related code

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Add a test for array destructuring with complicated default values.

Removed debug comment, and reduced redundant test code, to avoid timeout.

Comment 23

[Shu-yu Guo [:shu]]

Comment on attachment 8797406 [details] [diff] [review]
Part 0: Make it possible to emit ParseNode twice.

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

I like this much better. Thanks for redoing it!

Comment 24

[Shu-yu Guo [:shu]]

Comment on attachment 8797407 [details] [diff] [review]
Part 1: Do not call iter.next() if the previous iter.next().done was true in array destructuring.

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

The BCE parts look fine. I'd like to see a new version with srcnotes fixed and Ion confirmed to be working.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4521,5 @@
>          return false;
>      if (!emitIterator())                                          // ... OBJ? ITER
>          return false;
>      bool needToPopIterator = true;
> +    Vector<JumpList, 8, SystemAllocPolicy> doneList;

Why is this SystemAllocPolicy? I'd just do Vector<JumpList, 8>(cx); I see other SystemAllocPolicy vectors in BCE as well, and I'm not sure why they're SystemAllocPolicy either.

@@ +4562,5 @@
> +                return false;
> +
> +            JumpList done;
> +            if (!emitJump(JSOP_IFEQ, &done))
> +                return false;

You deleted the srcnote on IFEQ. This will at least make IonBuilder abort.

@@ +4593,5 @@
>  
>      if (needToPopIterator && !emit1(JSOP_POP))
>          return false;
>  
> +    if (doneList.length() > 0) {

Nit: !doneList.empty()

@@ +4628,5 @@
> +                    return false;
> +                if (pndefault) {
> +                    if (!emitDefault(pndefault))                  // ... OBJ? VALUE
> +                        return false;
> +                }

It is unfortunate that in the case of pndefault, we emit:

undefined
dup
undefined
stricteq
ifeq
pop

We could simply do |emitConditionallyExecutedTree(pndefault)| with a comment explaining that the undefined check will always be true.

::: js/src/jit/IonBuilder.cpp
@@ +3033,5 @@
> +    MOZ_ASSERT(targetStart > pc);
> +
> +    pc = targetStart;
> +    return ControlStatus_Jumped;
> +}

This makes me uneasy because the name is too broad. Please rename it something like processArrayDestructuringDone.

I also don't know the control flow code well enough to know if this works. Without the srcnote on the IFEQ right now, though, array destructuring isn't be compiled.

Comment 25

[Shu-yu Guo [:shu]]

Comment on attachment 8797408 [details] [diff] [review]
Part 3: Add a test for array destructuring with complicated default values.

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

::: js/src/tests/ecma_6/Expressions/destructuring-array-default-class.js
@@ +43,5 @@
> +    no_gen: true,
> +    no_ctor: true,
> +    no_gen_arg: true,
> +    no_comp: true,
> +};

Would be nice to readers to list out explicitly the false values. Not a big deal, so up to you.

::: js/src/tests/ecma_6/Expressions/shell.js
@@ +159,5 @@
> +            i++;
> +        }
> +    }
> +
> +    function test(expr, opt={expr: {}, context: {}}) {

{expr: {}, context: {}} doesn't seem like a reasonable default for opt given how it's used in test_one. Leftover from an earlier version of the harness?

Comment 26

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)
About source notes, I checked it again and looks like it doesn't fit into current branch code generation in Ion (I hope I'm wrong :P

Here's the bytecode and it's expected graph for:
  function f(x) {
    var [a, b, c] = x;
    return a + b;
  }

There, ifeq's true-branch and false-branch don't join at single point, everything joins at the last block, but I'm not sure if it's supported.
jandem, is this supported, or possible/easy to support in Ion?


  +-------------------+
  |  getarg 0         |
  |  dup              |
  |  dup              |
  |  symbol 1         |
  |  callelem         |
  |  swap             |
  |  calliter 0       |
  |  dup              |
  |  dup              |
  |  callprop "next"  |
  |  swap             |
  |  call 0           |
  |  checkisobj 0     |
  |  dup              |
  |  getprop "done"   |
  |  not              |
  +-------------------+  false
  |  ifeq             |-------------+
  +-------------------+             |
           |                        |
           | true                   |
           v                        v
  +-------------------+    +-------------------+
  |  jumptarget       |    |  jumptarget       |
  |  getprop "value"  |    |  popn 2           |
  |  setlocal 0       |    |  undefined        |
  |  pop              |    |  setlocal 0       |
  |  dup              |    |  pop              |
  |  dup              |    |  undefined        |
  |  callprop "next"  |    |  undefined        |
  |  swap             |    +-------------------+
  |  call 0           |             |
  |  checkisobj 0     |             |
  |  dup              |             |
  |  getprop "done"   |             |
  |  not              |             |
  +-------------------+  false      |
  |  ifeq             |---------+   |
  +-------------------+         |   |
           |                    |   |
           | true               |   |
           |                    |   |
           v                    v   v
  +-------------------+    +-------------------+
  |  jumptarget       |    |  jumptarget       |
  |  getprop "value"  |    |  popn 2           |
  |  setlocal 1       |    |  undefined        |
  |  pop              |    |  setlocal 1       |
  |  dup              |    |  pop              |
  |  dup              |    |  undefined        |
  |  callprop "next"  |    |  undefined        |
  |  swap             |    +-------------------+
  |  call 0           |             |
  |  checkisobj 0     |             |
  |  dup              |             |
  |  getprop "done"   |             |
  |  not              |             |
  +-------------------+  false      |
  |  ifeq             |---------+   |
  +-------------------+         |   |
           |                    |   |
           | true               |   |
           |                    |   |
           v                    v   v
  +-------------------+    +-------------------+
  |  jumptarget       |    |  jumptarget       |
  |  getprop "value"  |    |  popn 2           |
  |  setlocal 2       |    |  undefined        |
  |  pop              |    |  setlocal 2       |
  |  pop              |    |  pop              |
  +-------------------+    +-------------------+
  |  goto             |             |
  +-------------------+             |
           |                        |
           |   +--------------------+
           |   |
           v   v
  +-------------------+
  |  jumptarget       |
  |  getlocal 0       |
  |  getlocal 1       |
  |  add              |
  |  return           |
  |  retrval          |
  +-------------------+

Comment 27

[Tooru Fujisawa [:arai]]

discussed in IRC.
we can use simple if-else without adding extra branches, as we can now emit pattern twice.

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Do not call iter.next() if the previous iter.next().done was true in array destructuring.

changed bytecode structure.

It's described in comment in BytecodeEmitter::emitDestructuringOpsArrayHelper.
before this patch, the code was using conditional expression like |a = done ? undefined : result.value|.
This patch changes it to if-else. and both branches execute next element's |iter.next()| call and |result.done| access, or equivalent for |done==true| case.
so that the number of branches doesn't change.

if |result.done| become true, |done| variable is kept |true| for remaining elements.

>    // Here's pseudo code for |let [a, b, , c=y, ...d] = x;|
>    //
>    //   let x, y;
>    //   let a, b, c, d;
>    //   let tmp, done, iter, result; // stack values
>    //
>    //   iter = x[Symbol.iterator]();
>    //
>    //   // ==== emitted by loop for a ====
>    //   result = iter.next();
>    //   done = result.done;
>    //
>    //   if (done) {
>    //     a = undefined;
>    //
>    //     result = undefined;
>    //     done = true;
>    //   } else {
>    //     a = result.value;
>    //
>    //     // Do next element's .next() and .done access here
>    //     result = iter.next();
>    //     done = result.done;
>    //   }
>    //
>    //   // ==== emitted by loop for b ====
>    //   if (done) {
>    //     b = undefined;
>    //
>    //     result = undefined;
>    //     done = true;
>    //   } else {
>    //     b = result.value;
>    //
>    //     result = iter.next();
>    //     done = result.done;
>    //   }
>    //
>    //   // ==== emitted by loop for elision ====
>    //   if (done) {
>    //     result = undefined
>    //     done = true
>    //   } else {
>    //     result.value;
>    //
>    //     result = iter.next();
>    //     done = result.done;
>    //   }
>    //
>    //   // ==== emitted by loop for c ====
>    //   if (done) {
>    //     c = y;
>    //   } else {
>    //     tmp = result.value;
>    //     if (tmp === undefined)
>    //       tmp = y;
>    //     c = tmp;
>    //
>    //     // Don't do next element's .next() and .done access if
>    //     // this is the last non-spread element.
>    //   }
>    //
>    //   // ==== emitted by loop for d ====
>    //   if (done) {
>    //     // Assing empty array when completed
>    //     d = [];
>    //   } else {
>    //     d = [...iter];
>    //   }

Here's actual bytecode.

# ==== emitted before loop ====
    getarg 0           # OBJ
    dup                # OBJ OBJ
    dup                # OBJ OBJ OBJ
    symbol 1           # OBJ OBJ OBJ @@ITER
    callelem           # OBJ OBJ ITERFUNC
    swap               # OBJ ITERFUNC OBJ
    calliter 0         # OBJ ITER

# ==== emitted by loop for a ====
    dup                # OBJ ITER ITER
    dup                # OBJ ITER ITER ITER
    callprop "next"    # OBJ ITER ITER NEXT
    swap               # OBJ ITER NEXT ITER
    call 0             # OBJ ITER RESULT
    checkisobj 0       # OBJ ITER RESULT
    dup                # OBJ ITER RESULT RESULT
    getprop "done"     # OBJ ITER RESULT DONE?
    ifeq notDoneA      # OBJ ITER RESULT
# {
    jumptarget         # OBJ ITER RESULT
    pop                # OBJ ITER
    undefined          # OBJ ITER UNDEFINED
    setlocal 0         # OBJ ITER UNDEFINED
    pop                # OBJ ITER
# result = undefined
    undefined          # OBJ ITER UNDEFINED
    nop-destructuring  # OBJ ITER UNDEFINED
# done = true
    true               # OBJ ITER UNDEFINED TRUE
    goto B             # OBJ ITER UNDEFINED TRUE
# } else {
notDoneA:
    jumptarget         # OBJ ITER RESULT
    getprop "value"    # OBJ ITER VALUE
    setlocal 0         # OBJ ITER VALUE
    pop                # OBJ ITER

    dup                # OBJ ITER ITER
    dup                # OBJ ITER ITER ITER
    callprop "next"    # OBJ ITER ITER NEXT
    swap               # OBJ ITER NEXT ITER
    call 0             # OBJ ITER RESULT
    checkisobj 0       # OBJ ITER RESULT
    dup                # OBJ ITER RESULT RESULT
    getprop "done"     # OBJ ITER RESULT DONE?
# }

B:
    jumptarget         # OBJ ITER RESULT DONE?

# ==== emitted by loop for b ====
    ifeq notDoneB      # OBJ ITER RESULT
# {
    jumptarget         # OBJ ITER RESULT
    pop                # OBJ ITER
    undefined          # OBJ ITER UNDEFINED
    setlocal 1         # OBJ ITER UNDEFINED
    pop                # OBJ ITER
# result = undefined
    undefined          # OBJ ITER UNDEFINED
    nop-destructuring  # OBJ ITER UNDEFINED
# done = true
    true               # OBJ ITER UNDEFINED TRUE
    goto E             #
# } else {
notDoneB:
    jumptarget         # OBJ ITER RESULT
    getprop "value"    # OBJ ITER VALUE
    setlocal 1         # OBJ ITER VALUE
    pop                # OBJ ITER

    dup                # OBJ ITER ITER
    dup                # OBJ ITER ITER ITER
    callprop "next"    # OBJ ITER ITER NEXT
    swap               # OBJ ITER NEXT ITER
    call 0             # OBJ ITER RESULT
    checkisobj 0       # OBJ ITER RESULT
    dup                # OBJ ITER RESULT RESULT
    getprop "done"     # OBJ ITER RESULT DONE?
}

E:
    jumptarget         # OBJ ITER RESULT DONE?

# ==== emitted by loop for elision ====
    ifeq notDoneE      # OBJ ITER RESULT
# {
    jumptarget         # OBJ ITER RESULT
    pop                # OBJ ITER
# result = undefined
    undefined          # OBJ ITER UNDEFINED
    nop-destructuring  # OBJ ITER UNDEFINED
# done = true
    true               # OBJ ITER UNDEFINED TRUE
    goto C             # OBJ ITER UNDEFINED TRUE
# } else {
notDoneE:
    jumptarget         # OBJ ITER RESULT
    getprop "value"    # OBJ ITER VALUE
    pop                # OBJ ITER

    dup                # OBJ ITER ITER
    dup                # OBJ ITER ITER ITER
    callprop "next"    # OBJ ITER ITER NEXT
    swap               # OBJ ITER NEXT ITER
    call 0             # OBJ ITER RESULT
    checkisobj 0       # OBJ ITER RESULT
    dup                # OBJ ITER RESULT RESULT
    getprop "done"     # OBJ ITER RESULT DONE?
# }

C:
    jumptarget         # OBJ ITER RESULT DONE?

# ==== emitted by loop for c ====
    ifeq notDoneC      # OBJ ITER RESULT
# {
    jumptarget         # OBJ ITER RESULT
    pop                # OBJ ITER
    getarg 1           # OBJ ITER DEFAULT
    setlocal 2         # OBJ ITER DEFAULT
    pop                # OBJ ITER

    true               # OBJ ITER TRUE
    goto D             #
# } else {
notDoneC:
    jumptarget         # OBJ ITER RESULT
    getprop "value"    # OBJ ITER VALUE
    dup                # OBJ ITER VALUE VALUE
    undefined          # OBJ ITER VALUE VALUE UNDEFINED
    stricteq           # OBJ ITER VALUE VALUE===UNDEFINED
    ifeq afterDefaultC # OBJ ITER VALUE
# {
    jumptarget         # OBJ ITER VALUE
    pop                # OBJ ITER
    getarg 1           # OBJ ITER DEFAULT
# }
afterDefaultC:
    jumptarget         # OBJ ITER VALUE
    setlocal 2         # OBJ ITER VALUE
    pop                # OBJ ITER

    false              # OBJ ITER FALSE
# }

D:
    jumptarget         # OBJ ITER DONE?

# ==== emitted by loop for d ====
    ifeq notDoneD      # OBJ ITER
# {
    jumptarget         # OBJ ITER
    pop                # OBJ
    newarray 0         # OBJ ARRAY
    setlocal 3         # OBJ ARRAY
    pop                # OBJ
    goto doneAll       # OBJ
# } else {
notDoneD:
    jumptarget         # OBJ ITER
    newarray 0         # OBJ ITER ARRAY
    zero               # OBJ ITER ARRAY N

    goto spreadEntry   # OBJ ITER ARRAY N

# loop {
spreadHead:
    loophead           # OBJ ITER ARRAY N RESULT
    getprop "value"    # OBJ ITER ARRAY N VALUE
    initelem_inc       # OBJ ITER ARRAY N+1

spreadEntry:
    loopentry 1        # OBJ ITER ARRAY N
    dupat 2            # OBJ ITER ARRAY N ITER
    dup                # OBJ ITER ARRAY N ITER ITER
    callprop "next"    # OBJ ITER ARRAY N ITER NEXT
    swap               # OBJ ITER ARRAY N NEXT ITER
    call 0             # OBJ ITER ARRAY N RESULT
    checkisobj 0       # OBJ ITER ARRAY N RESULT
    dup                # OBJ ITER ARRAY N RESULT RESULT
    getprop "done"     # OBJ ITER ARRAY N RESULT DONE?
    ifeq spreadHead    # OBJ ITER ARRAY N RESULT
# }

    jumptarget         # OBJ ITER ARRAY N RESULT
    pick 3             # OBJ ARRAY N RESULT ITER
    popn 2             # OBJ ARRAY N
    pop                # OBJ ARRAY

    setlocal 3         # OBJ ARRAY
    pop                # OBJ
# }

doneAll:
    jumptarget         # OBJ

# ==== emitted after loop ====
    pop                #

Comment 29

[Shu-yu Guo [:shu]]

Comment on attachment 8798346 [details] [diff] [review]
Part 1: Do not call iter.next() if the previous iter.next().done was true in array destructuring.

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

Looks good to me.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4577,5 @@
> +    //     // Assing empty array when completed
> +    //     d = [];
> +    //   } else {
> +    //     d = [...iter];
> +    //   }

This comment is awesome.

@@ +4603,5 @@
> +                    return false;
> +                if (!emitJump(JSOP_IFEQ, &beq))                   // ... OBJ? ITER
> +                    return false;
> +
> +                int32_t depth = stackDepth;

Would be clearer if the depth was saved at entry to the |if (!isHead)| block.

We should have some RAII depth-setting struct for conditionals like this, but no need to do it now.

@@ +4607,5 @@
> +                int32_t depth = stackDepth;
> +                if (!emit1(JSOP_POP))                             // ... OBJ?
> +                    return false;
> +                if (!emitUint32Operand(JSOP_NEWARRAY, 0))         // ... OBJ? ARRAY
> +                return false;

Missed indentation.

@@ +4704,5 @@
>              if (!emit1(JSOP_POP))                                 // ... OBJ? ITER
>                  return false;
> +        }
> +
> +        // Setup next element's result

I'd prefer "Setup next element's result when the iterator is done."

@@ +4725,5 @@
> +            return false;
> +
> +        stackDepth = depth;
> +        if (!emitAtomOp(cx->names().value, JSOP_GETPROP))         // ... OBJ? ITER VALUE
> +                return false;

Wrong indentation.

@@ +4740,5 @@
>              if (!emit1(JSOP_POP))                                 // ... OBJ? ITER
>                  return false;
> +        }
> +
> +        // Setup next element's result

I'd prefer "Setup next element's result when the iterator is not done."

Comment 30

[Tooru Fujisawa [:arai]]

(In reply to Shu-yu Guo [:shu] from comment #29)
> @@ +4603,5 @@
> > +                    return false;
> > +                if (!emitJump(JSOP_IFEQ, &beq))                   // ... OBJ? ITER
> > +                    return false;
> > +
> > +                int32_t depth = stackDepth;
> 
> Would be clearer if the depth was saved at entry to the |if (!isHead)| block.

it's saving the depth of the then-branch, to restore it in then else-branch.
so I need to save after IFEQ.

Will file a bug to create a class to handle if-then-else.

Comment 31

[Tooru Fujisawa [:arai]]

Will address the depth in bug 1308383.
anyway, thank you for reviewing :)

Comment 32

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/986b87b7d7c9e8f7965d77cd345873d1a012ebed
Bug 1184922 - Part 0: Make it possible to emit ParseNode twice. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/9716bcfed35d5c59659696a602fa1f4acfdc0a66
Bug 1184922 - Part 1: Do not call iter.next() if the previous iter.next().done was true in array destructuring. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/8122b345846fbab58322163340a318afffec0e11
Bug 1184922 - Part 1.1: Update existing test to follow the change in array destructuring with completed iterator. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/660c677d5e40c1e2008fef5d2add7114d3868812
Bug 1184922 - Part 2: Add a test for array destructuring with completed iterator. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf57bc9cf80144b363c65970d955e457d3a1cde
Bug 1184922 - Part 3: Add a test for array destructuring with complicated default values. r=shu

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/986b87b7d7c9
https://hg.mozilla.org/mozilla-central/rev/9716bcfed35d
https://hg.mozilla.org/mozilla-central/rev/8122b345846f
https://hg.mozilla.org/mozilla-central/rev/660c677d5e40
https://hg.mozilla.org/mozilla-central/rev/2cf57bc9cf80

Comment 34

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ded1f2781017c0ee95703c1622582af8b26a2245
Backed out changeset 2cf57bc9cf80 (bug 1184922)

https://hg.mozilla.org/integration/mozilla-inbound/rev/13d2b1ef3c9b114c2b70d1ece8c95d93952e3d54
Backed out changeset 660c677d5e40 (bug 1184922)

https://hg.mozilla.org/integration/mozilla-inbound/rev/c82893fb1eacf89b1a35268ad6ec6c5025d30ce2
Backed out changeset 8122b345846f (bug 1184922)

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6cbdf80b0aa5ad588f2825d2537d82fbb13d660
Backed out changeset 9716bcfed35d (bug 1184922)

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbabf1e1b6c0cd019c43928f37afa58876edd036
Backed out changeset 986b87b7d7c9 (bug 1184922)

Comment 35

[Tooru Fujisawa [:arai]]

we need totally different approach for emitting lexical binding pattern (bug 1308745).
also, there might be some more cases that binding related code is optimized for second access (2 hits by searching "already emitted")

Comment 36

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #35)
> we need totally different approach for emitting lexical binding pattern (bug
> 1308745).

I meant, emitting lexical binding pattern *twice

Comment 37

[Tooru Fujisawa [:arai]]

possible solution:
  * store ParseNode* into BytecodeEmitter::TDZCheckCache for each name,
    and use the cache only if the current node is not the same node
  * do not use BytecodeEmitter::TDZCheckCache when emitting second time (will neeed BCE flag + RAII ...)

or, fallback to the bytecode in comment #3, that doesn't need emitting pattern twice, but adds branches.

Comment 38

[Tooru Fujisawa [:arai]]

perhaps I'm missing something.
jsop_checklexical is emitted for each branch in the following code:

  let y = x ? (y = 1) : (y = 2)

it would mean we could do the same thing here.

Comment 39

[Tooru Fujisawa [:arai]]

oops, I just needed |TDZCheckCache tdzCache(this);| for each branch :P
I'll add BytecodeEmitter::emitConditionallyExecutedDestructuringLHS.

anyway, I'll post followup patches when a patch for bug 1308744 and another "already emitted" are ready.

Comment 40

[Shu-yu Guo [:shu]]

(In reply to Tooru Fujisawa [:arai] from comment #39)
> oops, I just needed |TDZCheckCache tdzCache(this);| for each branch :P
> I'll add BytecodeEmitter::emitConditionallyExecutedDestructuringLHS.
> 
> anyway, I'll post followup patches when a patch for bug 1308744 and another
> "already emitted" are ready.

Wow I should've totally caught that. Sorry arai. And you're right, you need a TDZCheckCache for each section in C++ where you're generating code that could be jumped over.

Comment 41

[Tooru Fujisawa [:arai]]

(In reply to Shu-yu Guo [:shu] from comment #40)
> Wow I should've totally caught that. Sorry arai. And you're right, you need
> a TDZCheckCache for each section in C++ where you're generating code that
> could be jumped over.

thanks, maybe we could do some cleanup in bug 1308383 after fixing this bug,
as we could move TDZCheckCache handling into the class.

Comment 42

[Tooru Fujisawa [:arai]]

Attachment: Bug 1184922 - Part 0 followup: Clear emitLink in CGObjectList::finish.

corresponds to bug 1308744.

We allocate another BytecodeEmitter on the stack to emit function.

https://dxr.mozilla.org/mozilla-central/rev/7a7ba250bb2f5a7cc7acf4b97145425c5292e894/js/src/frontend/BytecodeEmitter.cpp#6926

When emitting twice, the same ParseNode tree is passed to 2 different BytecodeEmitter instances.
We used |objbox->emitLink!=nullptr| to check if it's in objectList, but |emitLink!=nullptr| doesn't mean it's in current BytecodeEmitter's objectList.

Made CGObjectList::finish to clear emitLink member.
CGObjectList::finish should be executed on successful case, and we'll abort emitting when failed case.
so that emitLink is non-null only if it's in current BytecodeEmitter's objectList.

Comment 43

[Tooru Fujisawa [:arai]]

Attachment: Part 1 followup: Add BytecodeEmitter::emitConditionallyExecutedDestructuringLHS and use it in BytecodeEmitter::emitDestructuringOpsArrayHelper.

Corresponds to bug 1308745.

Added BytecodeEmitter::emitConditionallyExecutedDestructuringLHS, that allocates TDZCheckCache and calls emitDestructuringLHS.

Comment 44

[Shu-yu Guo [:shu]]

Comment on attachment 8799224 [details] [diff] [review]
Part 1 followup: Add BytecodeEmitter::emitConditionallyExecutedDestructuringLHS and use it in BytecodeEmitter::emitDestructuringOpsArrayHelper.

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

Great, thanks.

Comment 45

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ded1f2781017
https://hg.mozilla.org/mozilla-central/rev/13d2b1ef3c9b
https://hg.mozilla.org/mozilla-central/rev/c82893fb1eac
https://hg.mozilla.org/mozilla-central/rev/e6cbdf80b0aa
https://hg.mozilla.org/mozilla-central/rev/cbabf1e1b6c0

Comment 46

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Tooru Fujisawa [:arai] from comment #39)
> and another "already emitted" are ready.
this wasn't related to this bug.
I'll land patches again shortly.

Comment 47

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/908dce87d77184d5752990ec3c939488870e02aa
Bug 1184922 - Part 0: Make it possible to emit ParseNode twice. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/df5b995b08f5e0948a9252a44f34ec34b26e9a2c
Bug 1184922 - Part 1: Do not call iter.next() if the previous iter.next().done was true in array destructuring. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/05805e16029eb655d171af3976eea138953d54e3
Bug 1184922 - Part 1.1: Update existing test to follow the change in array destructuring with completed iterator. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/7533e8a0d588d7a16f79b02354a982c470758e3b
Bug 1184922 - Part 2: Add a test for array destructuring with completed iterator. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/0edf9443c455afc0b9c3506babdbb51d86664018
Bug 1184922 - Part 3: Add a test for array destructuring with complicated default values. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/f95d5fb76b4694a4e921ec276e080da559c35be8
Bug 1184922 - Part 4: Add a testcase for Array destructuring with accessing uninitialized lexical binding. r=shu

Comment 48

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/908dce87d771
https://hg.mozilla.org/mozilla-central/rev/df5b995b08f5
https://hg.mozilla.org/mozilla-central/rev/05805e16029e
https://hg.mozilla.org/mozilla-central/rev/7533e8a0d588
https://hg.mozilla.org/mozilla-central/rev/0edf9443c455
https://hg.mozilla.org/mozilla-central/rev/f95d5fb76b46