Bug 609756 - {Pre,Post}{in,de}crements on function calls must ToNumber after evaluating the call, in case ToNumber's effects are observable via toString/valueOf. r=arai

--HG--
extra : rebase_source : 2c63da1980238a0d1c7c002506b0961064444129

js/src/frontend/BytecodeEmitter.cpp

@@ -3425,6 +3425,8 @@ BytecodeEmitter::emitPropIncDec(ParseNode* pn)
 bool
 BytecodeEmitter::emitNameIncDec(ParseNode* pn)
 {
+    MOZ_ASSERT(pn->pn_kid->isKind(PNK_NAME));
+
     bool post;
     JSOp binop = GetIncDecInfo(pn->getKind(), &post);
 
@@ -3648,6 +3650,27 @@ BytecodeEmitter::emitElemIncDec(ParseNode* pn)
     return true;
 }
 
+bool
+BytecodeEmitter::emitCallIncDec(ParseNode* incDec)
+{
+    MOZ_ASSERT(incDec->isKind(PNK_PREINCREMENT) ||
+               incDec->isKind(PNK_POSTINCREMENT) ||
+               incDec->isKind(PNK_PREDECREMENT) ||
+               incDec->isKind(PNK_POSTDECREMENT));
+
+    MOZ_ASSERT(incDec->pn_kid->isKind(PNK_CALL));
+
+    ParseNode* call = incDec->pn_kid;
+    if (!emitTree(call))                                // CALLRESULT
+        return false;
+    if (!emit1(JSOP_POS))                               // N
+        return false;
+
+    // The increment/decrement has no side effects, so proceed to throw for
+    // invalid assignment target.
+    return emitUint16Operand(JSOP_THROWMSG, JSMSG_BAD_LEFTSIDE_OF_ASS);
+}
+
 bool
 BytecodeEmitter::emitNumberOp(double dval)
 {
@@ -4418,17 +4441,9 @@ BytecodeEmitter::emitDestructuringLHS(ParseNode* target, DestructuringFlavor fla
           }
 
           case PNK_CALL:
-            MOZ_ASSERT(target->pn_xflags & PNX_SETCALL);
-            if (!emitTree(target))
-                return false;
-
-            // Pop the call return value. Below, we pop the RHS too, balancing
-            // the stack --- presumably for the benefit of bytecode
-            // analysis. (The interpreter will never reach these instructions
-            // since we just emitted JSOP_SETCALL, which always throws. It's
-            // possible no analyses actually depend on this either.)
-            if (!emit1(JSOP_POP))
-                return false;
+            MOZ_ASSERT_UNREACHABLE("Parser::reportIfNotValidSimpleAssignmentTarget "
+                                   "rejects function calls as assignment "
+                                   "targets in destructuring assignments");
             break;
 
           default:
@@ -4944,9 +4959,15 @@ BytecodeEmitter::emitAssignment(ParseNode* lhs, JSOp op, ParseNode* rhs)
       case PNK_OBJECT:
         break;
       case PNK_CALL:
-        MOZ_ASSERT(lhs->pn_xflags & PNX_SETCALL);
         if (!emitTree(lhs))
             return false;
+
+        // Assignment to function calls is forbidden, but we have to make the
+        // call first.  Now we can throw.
+        if (!emitUint16Operand(JSOP_THROWMSG, JSMSG_BAD_LEFTSIDE_OF_ASS))
+            return false;
+
+        // Rebalance the stack to placate stack-depth assertions.
         if (!emit1(JSOP_POP))
             return false;
         break;
@@ -4993,12 +5014,9 @@ BytecodeEmitter::emitAssignment(ParseNode* lhs, JSOp op, ParseNode* rhs)
             break;
           }
           case PNK_CALL:
-            /*
-             * We just emitted a JSOP_SETCALL (which will always throw) and
-             * popped the call's return value. Push a random value to make sure
-             * the stack depth is correct.
-             */
-            MOZ_ASSERT(lhs->pn_xflags & PNX_SETCALL);
+            // We just emitted a JSOP_THROWMSG and popped the call's return
+            // value.  Push a random value to make sure the stack depth is
+            // correct.
             if (!emit1(JSOP_NULL))
                 return false;
             break;
@@ -5028,8 +5046,7 @@ BytecodeEmitter::emitAssignment(ParseNode* lhs, JSOp op, ParseNode* rhs)
         break;
       }
       case PNK_CALL:
-        /* Do nothing. The JSOP_SETCALL we emitted will always throw. */
-        MOZ_ASSERT(lhs->pn_xflags & PNX_SETCALL);
+        // We threw above, so nothing to do here.
         break;
       case PNK_ELEM: {
         JSOp setOp = lhs->as<PropertyByValue>().isSuper() ?
@@ -7527,7 +7544,6 @@ BytecodeEmitter::emitDeleteExpression(ParseNode* node)
         return false;
 
     if (useful) {
-        MOZ_ASSERT_IF(expression->isKind(PNK_CALL), !(expression->pn_xflags & PNX_SETCALL));
         if (!emitTree(expression))
             return false;
         if (!emit1(JSOP_POP))
@@ -7787,9 +7803,6 @@ BytecodeEmitter::emitCallOrNew(ParseNode* pn)
     switch (pn2->getKind()) {
       case PNK_NAME:
         if (emitterMode == BytecodeEmitter::SelfHosting && !spread) {
-            // We shouldn't see foo(bar) = x in self-hosted code.
-            MOZ_ASSERT(!(pn->pn_xflags & PNX_SETCALL));
-
             // Calls to "forceInterpreter", "callFunction",
             // "callContentFunction", or "resumeGenerator" in self-hosted
             // code generate inline bytecode.
@@ -7953,10 +7966,6 @@ BytecodeEmitter::emitCallOrNew(ParseNode* pn)
         if (!emitUint32Operand(JSOP_LINENO, lineNum))
             return false;
     }
-    if (pn->pn_xflags & PNX_SETCALL) {
-        if (!emitUint16Operand(JSOP_THROWMSG, JSMSG_BAD_LEFTSIDE_OF_ASS))
-            return false;
-    }
 
     return true;
 }
@@ -8065,18 +8074,14 @@ BytecodeEmitter::emitSequenceExpr(ParseNode* pn)
 MOZ_NEVER_INLINE bool
 BytecodeEmitter::emitIncOrDec(ParseNode* pn)
 {
-    /* Emit lvalue-specialized code for ++/-- operators. */
-    ParseNode* pn2 = pn->pn_kid;
-    switch (pn2->getKind()) {
+    switch (pn->pn_kid->getKind()) {
       case PNK_DOT:
         return emitPropIncDec(pn);
       case PNK_ELEM:
         return emitElemIncDec(pn);
       case PNK_CALL:
-        MOZ_ASSERT(pn2->pn_xflags & PNX_SETCALL);
-        return emitTree(pn2);
+        return emitCallIncDec(pn);
       default:
-        MOZ_ASSERT(pn2->isKind(PNK_NAME));
         return emitNameIncDec(pn);
     }
 

js/src/frontend/BytecodeEmitter.h

@@ -501,6 +501,7 @@ struct MOZ_STACK_CLASS BytecodeEmitter
     MOZ_MUST_USE bool emitJumpTargetAndPatch(JumpList jump);
 
     MOZ_MUST_USE bool emitCall(JSOp op, uint16_t argc, ParseNode* pn = nullptr);
+    MOZ_MUST_USE bool emitCallIncDec(ParseNode* incDec);
 
     MOZ_MUST_USE bool emitLoopHead(ParseNode* nextpn, JumpTarget* top);
     MOZ_MUST_USE bool emitLoopEntry(ParseNode* nextpn, JumpList entryJump);

js/src/frontend/FullParseHandler.h

@@ -196,10 +196,6 @@ class FullParseHandler
         return new_<ConditionalExpression>(cond, thenExpr, elseExpr);
     }
 
-    void markAsSetCall(ParseNode* pn) {
-        pn->pn_xflags |= PNX_SETCALL;
-    }
-
     ParseNode* newDelete(uint32_t begin, ParseNode* expr) {
         if (expr->isKind(PNK_NAME)) {
             expr->setOp(JSOP_DELNAME);

js/src/frontend/ParseNode.h

@@ -621,11 +621,10 @@ class ParseNode
 
 /* PN_LIST pn_xflags bits. */
 #define PNX_FUNCDEFS    0x01            /* contains top-level function statements */
-#define PNX_SETCALL     0x02            /* call expression in lvalue context */
-#define PNX_ARRAYHOLESPREAD 0x04        /* one or more of
+#define PNX_ARRAYHOLESPREAD 0x02        /* one or more of
                                            1. array initialiser has holes
                                            2. array initializer has spread node */
-#define PNX_NONCONST    0x08            /* initialiser has non-constants */
+#define PNX_NONCONST    0x04            /* initialiser has non-constants */
 
     bool functionIsHoisted() const {
         MOZ_ASSERT(pn_arity == PN_CODE && getKind() == PNK_FUNCTION);

js/src/frontend/Parser.cpp

@@ -3741,7 +3741,7 @@ Parser<ParseHandler>::PossibleError::transferErrorTo(PossibleError* other)
 
 template <typename ParseHandler>
 bool
-Parser<ParseHandler>::makeSetCall(Node target, unsigned msg)
+Parser<ParseHandler>::checkAssignmentToCall(Node target, unsigned msg)
 {
     MOZ_ASSERT(handler.isFunctionCall(target));
 
@@ -3749,11 +3749,7 @@ Parser<ParseHandler>::makeSetCall(Node target, unsigned msg)
     // concerned about sites using this in dead code, so forbid it only in
     // strict mode code (or if the werror option has been set), and otherwise
     // warn.
-    if (!report(ParseStrictError, pc->sc()->strict(), target, msg))
-        return false;
-
-    handler.markAsSetCall(target);
-    return true;
+    return report(ParseStrictError, pc->sc()->strict(), target, msg);
 }
 
 template <>
@@ -5046,7 +5042,7 @@ Parser<ParseHandler>::validateForInOrOfLHSExpression(Node target)
     }
 
     if (handler.isFunctionCall(target))
-        return makeSetCall(target, JSMSG_BAD_FOR_LEFTSIDE);
+        return checkAssignmentToCall(target, JSMSG_BAD_FOR_LEFTSIDE);
 
     report(ParseError, false, target, JSMSG_BAD_FOR_LEFTSIDE);
     return false;
@@ -7163,7 +7159,7 @@ Parser<ParseHandler>::checkAndMarkAsAssignmentLhs(Node target, AssignmentFlavor
     }
 
     MOZ_ASSERT(handler.isFunctionCall(target));
-    return makeSetCall(target, JSMSG_BAD_LEFTSIDE_OF_ASS);
+    return checkAssignmentToCall(target, JSMSG_BAD_LEFTSIDE_OF_ASS);
 }
 
 class AutoClearInDestructuringDecl
@@ -7483,7 +7479,7 @@ Parser<ParseHandler>::checkAndMarkAsIncOperand(Node target, AssignmentFlavor fla
         if (!reportIfArgumentsEvalTarget(target))
             return false;
     } else if (handler.isFunctionCall(target)) {
-        if (!makeSetCall(target, JSMSG_BAD_INCOP_OPERAND))
+        if (!checkAssignmentToCall(target, JSMSG_BAD_INCOP_OPERAND))
             return false;
     }
     return true;

js/src/frontend/Parser.h

@@ -1319,7 +1319,7 @@ class Parser final : private JS::AutoGCRooter, public StrictModeGetter
     bool checkDestructuringObject(Node objectPattern, mozilla::Maybe<DeclarationKind> maybeDecl);
     bool checkDestructuringName(Node expr, mozilla::Maybe<DeclarationKind> maybeDecl);
 
-    bool makeSetCall(Node node, unsigned errnum);
+    bool checkAssignmentToCall(Node node, unsigned errnum);
 
     Node newNumber(const Token& tok) {
         return handler.newNumber(tok.number(), tok.decimalPoint(), tok.pos);

js/src/frontend/SyntaxParseHandler.h

@@ -219,10 +219,6 @@ class SyntaxParseHandler
 
     Node newElision() { return NodeGeneric; }
 
-    void markAsSetCall(Node node) {
-        MOZ_ASSERT(node == NodeFunctionCall);
-    }
-
     Node newDelete(uint32_t begin, Node expr) {
         return NodeGeneric;
     }

js/src/tests/ecma_5/extensions/inc-dec-functioncall.js

@@ -0,0 +1,90 @@
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ */
+
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 609756;
+var summary =
+  "Perform ToNumber on the result of the |fun()| in |fun()++| before " +
+  "throwing";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var hadSideEffect;
+
+function f()
+{
+  return { valueOf: function() { hadSideEffect = true; return 0; } };
+}
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() { f()++; }, ReferenceError);
+assertEq(hadSideEffect, true);
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() {
+  for (var i = 0; i < 20; i++)
+  {
+    if (i > 18)
+      f()++;
+  }
+}, ReferenceError);
+assertEq(hadSideEffect, true);
+
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() { f()--; }, ReferenceError);
+assertEq(hadSideEffect, true);
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() {
+  for (var i = 0; i < 20; i++)
+  {
+    if (i > 18)
+      f()--;
+  }
+}, ReferenceError);
+assertEq(hadSideEffect, true);
+
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() { ++f(); }, ReferenceError);
+assertEq(hadSideEffect, true);
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() {
+  for (var i = 0; i < 20; i++)
+  {
+    if (i > 18)
+      ++f();
+  }
+}, ReferenceError);
+assertEq(hadSideEffect, true);
+
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() { --f(); }, ReferenceError);
+assertEq(hadSideEffect, true);
+
+hadSideEffect = false;
+assertThrowsInstanceOf(function() {
+  for (var i = 0; i < 20; i++)
+  {
+    if (i > 18)
+      --f();
+  }
+}, ReferenceError);
+assertEq(hadSideEffect, true);
+
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");