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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: anba, Assigned: arai)

Tracking

Trunk
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 affected, firefox52 fixed)

Details

Attachments

(7 attachments, 5 obsolete attachments)

1.35 KB, patch
shu
: review+
Details | Diff | Splinter Review
7.07 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.14 KB, patch
shu
: review+
Details | Diff | Splinter Review
8.09 KB, patch
shu
: review+
Details | Diff | Splinter Review
12.43 KB, patch
shu
: review+
Details | Diff | Splinter Review
1.97 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.59 KB, patch
shu
: review+
Details | Diff | Splinter Review
Reporter

Description

4 years ago
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
Assignee

Comment 1

3 years ago
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.
Assignee: nobody → arai.unmht
Assignee

Comment 2

3 years ago
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
Attachment #8767549 - Flags: review?(jorendorff)
Assignee

Comment 3

3 years ago
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              #
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:
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!
Assignee

Comment 6

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

Comment 7

3 years ago
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.
Assignee

Comment 8

3 years ago
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);
Flags: needinfo?(shu)

Comment 9

3 years ago
(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.
Flags: needinfo?(shu)
Assignee

Comment 10

3 years ago
Thank you :)

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

Comment 11

3 years ago
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
Attachment #8767549 - Flags: review?(jorendorff)
Assignee

Comment 12

3 years ago
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()|.
Attachment #8795188 - Flags: review?(shu)
Assignee

Comment 13

3 years ago
changed the bytecode to follow comment #4.

Also, added SRC_ARRAY_DEST note and IonBuilder::processForwardJump to support simple forward jump in Ion.
Attachment #8767549 - Attachment is obsolete: true
Attachment #8795189 - Flags: review?(shu)
Assignee

Comment 14

3 years ago
Fixed existing testcase that does not follow the spec.
Attachment #8795190 - Flags: review?(shu)
Assignee

Comment 15

3 years ago
Added simple testcase for completed iterator.
Attachment #8795191 - Flags: review?(shu)
Assignee

Comment 16

3 years ago
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.
Attachment #8795192 - Flags: review?(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;
};
Attachment #8795188 - Flags: review?(shu)
Assignee

Comment 18

3 years ago
(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?
(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.
Assignee

Comment 20

3 years ago
* 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
Attachment #8795188 - Attachment is obsolete: true
Attachment #8797406 - Flags: review?(shu)
Assignee

Comment 21

3 years ago
Removed AutoEmittingSecondTime related code
Attachment #8795189 - Attachment is obsolete: true
Attachment #8795189 - Flags: review?(shu)
Attachment #8797407 - Flags: review?(shu)
Assignee

Comment 22

3 years ago
Removed debug comment, and reduced redundant test code, to avoid timeout.
Attachment #8795192 - Attachment is obsolete: true
Attachment #8795192 - Flags: review?(shu)
Attachment #8797408 - Flags: review?(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!
Attachment #8797406 - Flags: review?(shu) → review+
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.
Attachment #8797407 - Flags: review?(shu)

Updated

3 years ago
Attachment #8795190 - Flags: review?(shu) → review+

Updated

3 years ago
Attachment #8795191 - Flags: review?(shu) → review+
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?
Attachment #8797408 - Flags: review?(shu) → review+
Assignee

Comment 26

3 years ago
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          |
  +-------------------+
Flags: needinfo?(jdemooij)
Assignee

Comment 27

3 years ago
discussed in IRC.
we can use simple if-else without adding extra branches, as we can now emit pattern twice.
Flags: needinfo?(jdemooij)
Assignee

Comment 28

3 years ago
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                #
Attachment #8797407 - Attachment is obsolete: true
Attachment #8798346 - Flags: review?(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."
Attachment #8798346 - Flags: review?(shu) → review+
Assignee

Comment 30

3 years ago
(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.
Assignee

Updated

3 years ago
Blocks: 1308383
Assignee

Comment 31

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

Comment 32

3 years ago
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
Depends on: 1308744
Depends on: 1308745
Assignee

Comment 35

3 years ago
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")
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 36

3 years ago
(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
Assignee

Comment 37

3 years ago
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.
Assignee

Comment 38

3 years ago
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.
Assignee

Comment 39

3 years ago
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.
(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.
Assignee

Comment 41

3 years ago
(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.
Assignee

Updated

3 years ago
Status: REOPENED → ASSIGNED
Assignee

Comment 42

3 years ago
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.
Attachment #8799223 - Flags: review?(shu)
Assignee

Comment 43

3 years ago
Corresponds to bug 1308745.

Added BytecodeEmitter::emitConditionallyExecutedDestructuringLHS, that allocates TDZCheckCache and calls emitDestructuringLHS.
Attachment #8799224 - Flags: review?(shu)

Updated

3 years ago
Attachment #8799223 - Flags: review?(shu) → review+
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.
Attachment #8799224 - Flags: review?(shu) → review+
Assignee

Comment 46

3 years ago
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.
Assignee

Comment 47

3 years ago
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
Depends on: 1322314
You need to log in before you can comment on or make changes to this bug.