Closed
Bug 1204028
Opened 9 years ago
Closed 7 years ago
Incorrect evaluation order in destructuring
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: anba, Assigned: arai)
References
Details
Attachments
(1 file, 2 obsolete files)
86.69 KB,
patch
|
shu
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•9 years ago
|
Blocks: es6destructuring
Assignee | ||
Comment 1•7 years ago
|
||
looks like this can be fixed separately from bug 1147371.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
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
Attachment #8816668 -
Flags: review?(shu)
Assignee | ||
Comment 3•7 years ago
|
||
forgot to update some stack comment in array destructuring
Attachment #8816668 -
Attachment is obsolete: true
Attachment #8816668 -
Flags: review?(shu)
Attachment #8816731 -
Flags: review?(shu)
Comment 4•7 years ago
|
||
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?
Attachment #8816731 -
Flags: review?(shu) → review+
Assignee | ||
Comment 5•7 years ago
|
||
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.
Attachment #8816731 -
Attachment is obsolete: true
Attachment #8820130 -
Flags: review?(shu)
Comment 6•7 years ago
|
||
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.
Attachment #8820130 -
Flags: review?(shu) → review+
Assignee | ||
Comment 7•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/126cbbb0afcd6123fa538cad01ba4eec9660e455 Bug 1204028 - Evaluate LHS reference before RHS in destructuring. r=shu
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/126cbbb0afcd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•