Make OP_MUL_ASSIGN in rvalue context a runtime error. Also fix want_result semantics

Author: HaroldCindy

Compiler/src/LSLCompiler.cpp

@@ -1042,8 +1042,6 @@ bool LuauVisitor::visit(LSLUnaryExpression *un_expr)
     }
 
     // Everything below here should be a (post|pre)(incr|decr) operator operating on an lvalue.
-    auto *parent = un_expr->getParent();
-
     LUAU_ASSERT(un_expr->getChildExpr()->getNodeSubType() == NODE_LVALUE_EXPRESSION);
     auto *lvalue = (LSLLValueExpression *)un_expr->getChildExpr();
     auto *lvalue_sym = lvalue->getSymbol();
@@ -1069,7 +1067,7 @@ bool LuauVisitor::visit(LSLUnaryExpression *un_expr)
     // Especially important of mutating unary operators like `++bar` where
     // they may be their own statement, and we don't necessarily care about
     // pushing the result.
-    const bool want_result = (parent && parent->getNodeType() != NODE_STATEMENT);
+    const bool want_result = un_expr->getResultNeeded();
 
     // Pre-allocated by buildFunction() to ensure index < 256 for LOP_ADDK/SUBK
     const auto one_const_idx = addConstantUnder(lvalue->getType()->getOneValue(), UINT8_MAX);
@@ -1146,12 +1144,11 @@ bool LuauVisitor::visit(LSLBinaryExpression* bin_expr)
     // assignment is very special
     if (op == '=')
     {
-        auto *parent = bin_expr->getParent();
         // Basically, do we have to move this result for whoever asked for it.
         // Often `=` is used as if it were a statement, but it's also an expression
         // in LSL, which means that it can be used like `foo = bar = baz = 1`;
         // Only MOVE the result to the target register if we'll actually end up using it.
-        const bool want_result = (parent && parent->getNodeType() != NODE_STATEMENT);
+        const bool want_result = bin_expr->getResultNeeded();
 
         // The left-hand side of `=` _must_ be an lvalue
         LUAU_ASSERT(lhs->getNodeSubType() == NODE_LVALUE_EXPRESSION);
@@ -1160,7 +1157,7 @@ bool LuauVisitor::visit(LSLBinaryExpression* bin_expr)
 
         uint8_t source_reg;
         bool have_truncated_float = false;
-        if (auto *member = lval->getMember())
+        if (lval->getMember() != nullptr)
         {
             // Evaluate this first, it may be re-used if we need to use the result of the
             // assignment expression.
@@ -1223,8 +1220,7 @@ bool LuauVisitor::visit(LSLBinaryExpression* bin_expr)
     // An evil, demonic operator, only exists for int *= float
     if (op == OP_MUL_ASSIGN)
     {
-        auto *parent = bin_expr->getParent();
-        const bool want_result = (parent && parent->getNodeType() != NODE_STATEMENT);
+        const bool want_result = bin_expr->getResultNeeded();
 
         // assignment is special
         LUAU_ASSERT(lhs->getNodeSubType() == NODE_LVALUE_EXPRESSION);
@@ -1261,11 +1257,14 @@ bool LuauVisitor::visit(LSLBinaryExpression* bin_expr)
         mBuilder->emitABC(LOP_MOVE, source_reg, func_reg, 0);
 
         // Something actually wants to capture the result of the `*=`,
-        // pass it along.
+        // but that's not allowed in LSL. Throw a runtime error.
         if (want_result)
         {
-            mBuilder->emitABC(LOP_LSL_DOUBLE2FLOAT, floaty_reg, floaty_reg, 0);
-            maybeMove(expected_target, floaty_reg);
+            auto err_func_reg = allocReg(bin_expr);
+            pushImport(err_func_reg, "error");
+            const auto msg_const_idx = addConstantString(sref("cannot take result of integer *= float"), INT16_MAX);
+            mBuilder->emitAD(LOP_LOADK, allocReg(bin_expr), msg_const_idx);
+            mBuilder->emitABC(LOP_CALL, err_func_reg, 1 + 1, 0 + 1);
         }
         return false;
     }

tests/LSL.test.cpp

@@ -87,7 +87,7 @@ static void checkStatus(lua_State *L, int status)
     }
 }
 
-static StateRef runConformance(const char* name, int32_t (*yield)(lua_State* L) = nullptr, void (*setup)(lua_State* L) = nullptr, lua_CompileOptions* options = nullptr)
+static StateRef runConformance(const char* name, int32_t (*yield)(lua_State* L) = nullptr, void (*setup)(lua_State* L) = nullptr, lua_CompileOptions* options = nullptr, const char* expectedError = nullptr)
 {
 #ifdef LUAU_CONFORMANCE_SOURCE_DIR
     std::string path = LUAU_CONFORMANCE_SOURCE_DIR;
@@ -209,11 +209,26 @@ static StateRef runConformance(const char* name, int32_t (*yield)(lua_State* L)
 
     if (status == 0)
     {
-        // Do nothing for now.
+        // Script succeeded - fail if we expected an error
+        if (expectedError)
+        {
+            FAIL("Expected error '" << expectedError << "' but script succeeded");
+        }
     }
     else
     {
-        checkStatus(Lhandler, status);
+        // Script errored
+        if (expectedError)
+        {
+            // Check if the error matches what we expected
+            REQUIRE(lua_isstring(Lhandler, -1));
+            std::string error_msg = lua_tostring(Lhandler, -1);
+            CHECK(error_msg.find(expectedError) != std::string::npos);
+        }
+        else
+        {
+            checkStatus(Lhandler, status);
+        }
     }
 
     return globalState;
@@ -1047,6 +1062,21 @@ static std::string mono_to_upper_string(const std::string &str)
     return to_upper_mono(str.c_str(), str.length());
 }
 
+TEST_CASE("Integer MulAssign Float Result Error")
+{
+    runConformance("mul_assign_error.lsl", nullptr, nullptr, nullptr, "cannot take result of integer *= float");
+}
+
+TEST_CASE("Integer MulAssign Float Valid Contexts")
+{
+    runConformance("mul_assign_valid.lsl");
+}
+
+TEST_CASE("Integer Division By Zero Error")
+{
+    runConformance("div_by_zero.lsl", nullptr, nullptr, nullptr, "Math error");
+}
+
 TEST_CASE("Mono Strings")
 {
     // upper end of BMP without case change (3-byte)

tests/conformance/div_by_zero.lsl

@@ -0,0 +1,9 @@
+// Test that integer division by zero throws a runtime error
+default {
+    state_entry() {
+        integer a = 1;
+        integer b = 0;
+        integer c = a / b;  // This should error
+        print((string)c);
+    }
+}

tests/conformance/mul_assign_error.lsl

@@ -0,0 +1,9 @@
+// Test that using the result of `integer *= float` throws a runtime error
+default {
+    state_entry() {
+        integer a = 1;
+        float b = 7.0;
+        float t = a *= b;  // This should error
+        print((string)t);
+    }
+}

tests/conformance/mul_assign_valid.lsl

@@ -0,0 +1,25 @@
+// Tests all the cases where `<int> MUL_ASSIGN <float>` won't cause an errors.
+// Essentially, all the cases where it's not treated as an rvalue.
+// The error case is in `mul_assign_error.lsl`.
+default {
+  state_entry() {
+    integer a;
+    float b = 2.0;
+
+    // Expression statement
+    a = 3;
+    a *= b;
+
+    // For loop increment
+    a = 1;
+    integer i;
+    for (i = 0; i < 3; a *= b) {
+      ++i;
+    }
+
+    // For loop init
+    a = 5;
+    for (a *= b; i < 6; ++i) {
+    }
+  }
+}