1204028 - Incorrect evaluation order in destructuring

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Split off from bug 1199546, comment 4.

Test case:
---
({a: (print("lhs"), {}).a} = {get a(){print("rhs")}})
---

Expected: Prints "lhs - rhs"
Acual: Prints "rhs - lhs"

ES2015, 12.14.5.4, step 1.

Similar issues are present for destructuring binding patterns and array destructuring.

Comment 1

[Tooru Fujisawa [:arai]]

looks like this can be fixed separately from bug 1147371.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Evaluate LHS reference before RHS in destructuring.

Currently we're emitting the destructuring like following
  { a: OBJ[PROP] } = X;
(1) we first put X.a onto the stack
  ...
  getprop "a"     // X.a
(2) then evaluate OBJ and PROP
  getgname "OBJ"  // X.a OBJ
  getgname "PROP" // X.a OBJ PROP
(3) then rotate the stack and set property
  pick 2          // OBJ PROP X.a
  setelem         // X.a

Unpatched emitDestructuringLHS does (2) and (3) at once.
We need to do (2) before (1).
This patch splits emitDestructuringLHS into 2 parts, (2) and partial (3).

emitDestructuringLHSRef corresponds to (2), it does the former part of current emitDestructuringLHS, that is basically a reference if LHS is property reference.
If LHS is |OBJ.a|, emitDestructuringLHSRef pushes OBJ onto the stack.
If LHS is |OBJ[PROP]|, emitDestructuringLHSRef pushes OBJ and PROP onto the stack. (This also applies to spread operand.)
Otherwise it does nothing, since other operations has no effect while getting reference.
emitDestructuringLHSRef is called before evaluating RHS, that corresponds to (1).

EmitElemOption::Ref is added to avoid doing stack operation in emitElemOperands and emitSuperElemOperands.

Patched emitDestructuringLHS corresponds to (3), that does the remaining part of current emitDestructuringLHS, except JSOP_PICK etc, since we evaluate RHS after emitDestructuringLHSRef and RHS is already on the top of the stack.


Also, applied same for array destructuring.
that has some tricky part.
To reduce the number of branch, we're doing |emitNext| of non-first element in previous branch.  So, emitDestructuringLHSRef (emitDestructuringLHSRefInBranch in this case) is also called in previous branch, that is called before |emitNext|.
It makes the number of values pushed/popped by the if-then-else very complicated.  It depends on the kind of current and next element.
For now I removed the assertion (|MOZ_ASSERT(ifThenElse.pushed() == 1)| etc), but maybe I should calculate correct value on debug build...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2f94d84bfe547f88bf6cd1351dd5878abf589db

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Evaluate LHS reference before RHS in destructuring.

forgot to update some stack comment in array destructuring

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8816731 [details] [diff] [review]
Evaluate LHS reference before RHS in destructuring.

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

I think I grok this.

I mainly got tripped up on emitDestructuringLHSRef not recurring, because the recursion is done in emitDestructuringLHS itself. I wonder if emitDestructuringLHS should be renamed at this point to emitSetOrInitializeDestructuring, because it's not really emitting the LHS but the assignment itself.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4336,5 @@
> +    else if (target->isKind(PNK_ASSIGN))
> +        target = target->pn_left;
> +
> +    if (target->isKind(PNK_ARRAY) || target->isKind(PNK_OBJECT))
> +        return true;

1. Either move PNK_ARRAY and PNK_OBJECT below into the switch or move the PNK_NAME below up here.

2. Please add a comment about why it's okay to not recur into PNK_ARRAY and PNK_OBJECT subpatterns -- because emitDestructuringLHS does the recursion.

@@ +4489,3 @@
>              if (target->as<PropertyByValue>().isSuper()) {
>                  JSOp setOp = sc->strict() ? JSOP_STRICTSETELEM_SUPER : JSOP_SETELEM_SUPER;
> +                if (!emitElemOpBase(setOp))

At first glance it's hard to tell why emitElemOpBase here is okay since this is the 'super' case. I think a more detailed comment here would be nice, about how emitSuperElemOp's work had to be split up, and emitSuperElemOperands was already called by emitDestructuringLHSRef.

@@ +4931,5 @@
>              subpattern = subpattern->pn_left;
>          }
>  
>          bool isElision = subpattern->isKind(PNK_ELISION);
> +        bool hasNextNonElision = member->pn_next && !member->pn_next->isKind(PNK_ELISION);

A comment about why we care about the next case being a non-elision would be good. This complicates the function enough that we should make it clearer.

@@ +5082,5 @@
> +        else
> +            subpattern = member->pn_right;
> +        ParseNode* lhs = subpattern;
> +        if (lhs->isKind(PNK_ASSIGN))
> +            lhs = subpattern->pn_left;

Nit: this would read clearer to me as |lhs = lhs->pn_left|.

::: js/src/tests/ecma_6/Destructuring/order.js
@@ +416,5 @@
> +
> +new C2();
> +
> +if (typeof reportCompare === "function")
> +    reportCompare(true, true);

Awesome test.

Could you also add a few additional tests with nested array and object patterns?

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Evaluate LHS reference before RHS in destructuring.

Rebased on bug 1322314, and now the bytecode is different than before,
can you review that part?

Now we have to carry `done` information across multiple branches, so I put it at the middle of the stack (for now, putting it there instead of lexical scope seems to be simpler, not sure once IteratorClose is implemented).

I added JSOP_UNPICK, that does the reverse-operation of JSOP_PICK, so that we ca put a value under the LHS reference and RHS.


here's the bytecode for: [O[A], O[B], O[C]] = OBJ

  # ==== emitted before loop ====

  00000:  getgname "OBJ"           # OBJ
  00005:  dup                      # OBJ OBJ
  00006:  dup                      # OBJ OBJ OBJ
  00007:  symbol 1                 # OBJ OBJ OBJ @@iterator
  00009:  callelem                 # OBJ OBJ OBJ[@@iterator]
  00010:  swap                     # OBJ OBJ[@@iterator] OBJ
  00011:  calliter 0               # OBJ ITER
  00014:  checkisobj 1             # OBJ ITER

  # ==== emitted by loop for O[A] ====

  # get reference for O[A]

  00016:  getgname "O"             # OBJ ITER O
  00021:  getgname "A"             # OBJ ITER O A

  00026:  dupat 2                  # OBJ ITER O A ITER
  00030:  dup                      # OBJ ITER O A ITER ITER
  00031:  callprop "next"          # OBJ ITER O A ITER ITER[next]
  00036:  swap                     # OBJ ITER O A ITER[next] ITER
  00037:  call 0                   # OBJ ITER O A RESULT
  00040:  checkisobj 0             # OBJ ITER O A RESULT
  00042:  dup                      # OBJ ITER O A RESULT RESULT
  00043:  getprop "done"           # OBJ ITER O A RESULT DONE

  # store DONE value for next loop

  00048:  dup                      # OBJ ITER O A RESULT DONE DONE
  00049:  unpick 4                 # OBJ ITER DONE O A RESULT DONE

  00051:  ifeq 65 (+14)            # OBJ ITER DONE O A RESULT
  # {
  00056:  jumptarget               # OBJ ITER DONE O A RESULT
  00057:  pop                      # OBJ ITER DONE O A
  00058:  undefined                # OBJ ITER DONE O A undefined
  00059:  nop-destructuring        # OBJ ITER DONE O A undefined
  00060:  goto 71 (+11)            # OBJ ITER DONE O A undefined
  # } else {
  00065:  jumptarget               # OBJ ITER DONE O A RESULT
  00066:  getprop "value"          # OBJ ITER DONE O A VALUE
  # }
  00071:  jumptarget               # OBJ ITER DONE O A VALUE
  00072:  setelem                  # OBJ ITER DONE VALUE
  00073:  pop                      # OBJ ITER DONE

  # ==== emitted by loop for O[B] ====

  # get reference for O[B]

  00074:  getgname "O"             # OBJ ITER DONE O
  00079:  getgname "B"             # OBJ ITER DONE O B

  # check if already done

  00084:  dupat 2                  # OBJ ITER DONE O B DONE
  00088:  ifeq 101 (+13)           # OBJ ITER DONE O B
  # {
  00093:  jumptarget               # OBJ ITER DONE O B
  00094:  undefined                # OBJ ITER DONE O B undefined
  00095:  nop-destructuring        # OBJ ITER DONE O B undefined
  00096:  goto 150 (+54)           # OBJ ITER DONE O B undefined
  # } else {
  00101:  jumptarget               # OBJ ITER DONE O B
  00102:  pick 2                   # OBJ ITER O B DONE
  00104:  pop                      # OBJ ITER O B
  00105:  dupat 2                  # OBJ ITER O B ITER
  00109:  dup                      # OBJ ITER O B ITER ITER
  00110:  callprop "next"          # OBJ ITER O B ITER ITER[next]
  00115:  swap                     # OBJ ITER O B ITER[next] ITER
  00116:  call 0                   # OBJ ITER O B RESULT
  00119:  checkisobj 0             # OBJ ITER O B RESULT
  00121:  dup                      # OBJ ITER O B RESULT RESULT
  00122:  getprop "done"           # OBJ ITER O B RESULT DONE

  # store DONE value for next loop

  00127:  dup                      # OBJ ITER O B RESULT DONE DONE
  00128:  unpick 4                 # OBJ ITER DONE O B RESULT DONE

  00130:  ifeq 144 (+14)           # OBJ ITER DONE O B RESULT
  #   {
  00135:  jumptarget               # OBJ ITER DONE O B RESULT
  00136:  pop                      # OBJ ITER DONE O B
  00137:  undefined                # OBJ ITER DONE O B undefined
  00138:  nop-destructuring        # OBJ ITER DONE O B undefined
  00139:  goto 150 (+11)           # OBJ ITER DONE O B undefined
  #   } else {
  00144:  jumptarget               # OBJ ITER DONE O B RESULT
  00145:  getprop "value"          # OBJ ITER DONE O B VALUE
  #   }
  # }
  00150:  jumptarget               # OBJ ITER DONE O B VALUE
  00151:  setelem                  # OBJ ITER DONE VALUE
  00152:  pop                      # OBJ ITER DONE

  # ==== emitted by loop for O[C] ====

  # get reference for O[C]

  00153:  getgname "O"             # OBJ ITER DONE O
  00158:  getgname "C"             # OBJ ITER DONE O C

  00163:  pick 2                   # OBJ ITER O C DONE
  00165:  ifeq 178 (+13)           # OBJ ITER O C
  # {
  00170:  jumptarget               # OBJ ITER O C
  00171:  undefined                # OBJ ITER O C undefined
  00172:  nop-destructuring        # OBJ ITER O C undefined
  00173:  goto 221 (+48)           # OBJ ITER O C undefined
  # } else {
  00178:  jumptarget               # OBJ ITER O C
  00179:  dupat 2                  # OBJ ITER O C ITER
  00183:  dup                      # OBJ ITER O C ITER ITER
  00184:  callprop "next"          # OBJ ITER O C ITER ITER[next]
  00189:  swap                     # OBJ ITER O C ITER[next] ITER
  00190:  call 0                   # OBJ ITER O C RESULT
  00193:  checkisobj 0             # OBJ ITER O C RESULT
  00195:  dup                      # OBJ ITER O C RESULT RESULT
  00196:  getprop "done"           # OBJ ITER O C RESULT DONE

  # do not store DONE value here, since it's the last element

  00201:  ifeq 215 (+14)           # OBJ ITER O C RESULT
  #   {
  00206:  jumptarget               # OBJ ITER O C RESULT
  00207:  pop                      # OBJ ITER O C
  00208:  undefined                # OBJ ITER O C undefined
  00209:  nop-destructuring        # OBJ ITER O C undefined
  00210:  goto 221 (+11)           # OBJ ITER O C undefined
  #   } else {
  00215:  jumptarget               # OBJ ITER O C RESULT
  00216:  getprop "value"          # OBJ ITER O C VALUE
  #   }
  # }
  00221:  jumptarget               # OBJ ITER O C VALUE
  00222:  setelem                  # OBJ ITER VALUE
  00223:  pop                      # OBJ ITER

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

  00224:  pop                      # OBJ
  00225:  pop                      #
  00226:  retrval                  #


Also, rewrote the testcase to support nested array/object.
Almost similar to before, but now it's using proxy to trace the operations.
currently IteratorClose is ignored ("return" property)

order-super.js is for super-property.
it does the same thing for RHS, but LHS uses getter instead of proxy, since we cannot use proxy there.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8820130 [details] [diff] [review]
Evaluate LHS reference before RHS in destructuring.

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

This is a great patch. I am very happy you implemented unpick for Baseline and Ion.

Even if it turns out later it's easier to put "done" into a lexical slot, unpick I think is generally useful enough to keep.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4337,5 @@
> +        target = target->pn_left;
> +
> +    // No need to recur into PNK_ARRAY and PNK_OBJECT subpatterns here, since
> +    // emitSetOrInitializeDestructuring does the recursion when setting or
> +    // initializing value.  Getting reference don't recur.

Nit: don't -> doesn't

::: js/src/jit/BaselineCompiler.cpp
@@ +1170,5 @@
> +{
> +    frame.syncStack(0);
> +
> +    // Pick takes the top of the stack value and moves it under the nth value.
> +    // For instance, pick 2:

pick -> unpick

@@ +1179,5 @@
> +    masm.loadValue(frame.addressOfStackValue(frame.peek(-1)), R0);
> +
> +    // Move the other values up.
> +    int depth = -(GET_INT8(pc) + 1);
> +    for (int i = -1; i > depth; i--) {

I'd prefer int32_t here. I see that emit_JSOP_PICK uses int as well. Could you change that over to int32_t as well?

::: js/src/tests/ecma_6/Destructuring/order.js
@@ +719,5 @@
> +
> +           "lhs before obj length",
> +           "lhs before name length",
> +           "lhs set length",
> +         ].join(","));

Well that's a very long test.

Comment 7

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/126cbbb0afcd6123fa538cad01ba4eec9660e455
Bug 1204028 - Evaluate LHS reference before RHS in destructuring. r=shu

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/126cbbb0afcd