LibJS: Copy LHS of binary expression to preserve evaluation order

This error was found by asking an LLM to generate additional, related
test cases for the bug affecting https://volkswagen.de fixed in an
earlier commit.

An unconditional call to `copy_if_needed_to_preserve_evaluation_order`
in this place was showing up quiet significantly in the JS benchmarks.
To avoid the regression, there is now a small heuristic that avoids the
unnecessary Mov instruction in the vast majority of cases. This is
likely not the best way to deal with this. But the changes in the
current patch set are focussed on correctness, not performance. So I
opted for a localized, minimal-impact solution to the performance
regression.

Libraries/LibJS/AST.h

@@ -1173,6 +1173,9 @@ public:
     {
     }
 
+    auto const& lhs() const { return m_lhs; }
+    auto const& rhs() const { return m_rhs; }
+
     virtual void dump(ASTDumpState const& state = {}) const override;
     virtual Optional<Bytecode::ScopedOperand> generate_bytecode(Bytecode::Generator&, Optional<Bytecode::ScopedOperand> preferred_dst = {}) const override;
 

Libraries/LibJS/Bytecode/ASTCodegen.cpp

@@ -184,6 +184,24 @@ static ThrowCompletionOr<ScopedOperand> constant_fold_binary_expression(Generato
     }
 }
 
+static bool might_contain_assignment_expression(Expression const& expression)
+{
+    if (expression.is_numeric_literal() || expression.is_string_literal() || expression.is_boolean_literal() || expression.is_null_literal() || expression.is_identifier())
+        return false;
+
+    if (auto const* unary_expression = as_if<UnaryExpression>(expression))
+        return might_contain_assignment_expression(unary_expression->lhs());
+
+    if (auto const* binary_expression = as_if<BinaryExpression>(expression))
+        return might_contain_assignment_expression(binary_expression->lhs()) || might_contain_assignment_expression(binary_expression->rhs());
+
+    if (auto const* member_expression = as_if<MemberExpression>(expression))
+        return might_contain_assignment_expression(member_expression->object()) || might_contain_assignment_expression(member_expression->property());
+
+    // Conservatively consider everything else, including assignments themselves as potentially assigning.
+    return true;
+}
+
 Optional<ScopedOperand> BinaryExpression::generate_bytecode(Bytecode::Generator& generator, Optional<ScopedOperand> preferred_dst) const
 {
     Bytecode::Generator::SourceLocationScope scope(generator, *this);
@@ -247,6 +265,15 @@ Optional<ScopedOperand> BinaryExpression::generate_bytecode(Bytecode::Generator&
     };
 
     auto lhs = get_left_side(*m_lhs).value();
+    // OPTIMIZATION: We do need to make a copy of the LHS here in case evaluation of the RHS
+    // reassigns it. However, binary expressions are a pretty common thing, so doing the copy
+    // unconditionally is a noticable performance hit, especially because in practice, the copy is
+    // almost never needed. We add a small heuristic here that detects the most common cases.
+    // FIXME: This is a pretty narrow optimization. Maybe instead, it would make sense to have a
+    // more general "remove unnecessary mov-operations" as part of a bytecode optimization pass.
+    if (might_contain_assignment_expression(m_rhs))
+        lhs = generator.copy_if_needed_to_preserve_evaluation_order(lhs);
+
     auto rhs = get_right_side(*m_rhs).value();
     auto dst = choose_dst(generator, preferred_dst);
 

Libraries/LibJS/Rust/src/bytecode/codegen.rs

@@ -438,6 +438,31 @@ fn generate_unary_expression(
     Some(dst)
 }
 
+fn might_contain_assignment_expression(expression: &Expression) -> bool {
+    match &expression.inner {
+        ExpressionKind::NumericLiteral(_)
+        | ExpressionKind::StringLiteral(_)
+        | ExpressionKind::BooleanLiteral(_)
+        | ExpressionKind::NullLiteral
+        | ExpressionKind::Identifier(_) => false,
+        ExpressionKind::Unary { op: _, operand } => might_contain_assignment_expression(operand),
+        ExpressionKind::Binary { op: _, lhs, rhs } => {
+            might_contain_assignment_expression(lhs) || might_contain_assignment_expression(rhs)
+        }
+        ExpressionKind::Member {
+            object,
+            property,
+            computed: _,
+        } => {
+            might_contain_assignment_expression(object)
+                || might_contain_assignment_expression(property)
+        }
+        // Conservatively consider everything else, including assignments themselves as potentially
+        // assigning.
+        _ => true,
+    }
+}
+
 fn generate_binary_expression(
     generator: &mut Generator,
     op: BinaryOp,
@@ -476,6 +501,19 @@ fn generate_binary_expression(
         }
         _ => generate_expression(lhs, generator, None)?,
     };
+
+    // OPTIMIZATION: We do need to make a copy of the LHS here in case evaluation of the RHS
+    // reassigns it. However, binary expressions are a pretty common thing, so doing the copy
+    // unconditionally is a noticable performance hit, especially because in practice, the copy is
+    // almost never needed. We add a small heuristic here that detects the most common cases.
+    // FIXME: This is a pretty narrow optimization. Maybe instead, it would make sense to have a
+    // more general "remove unnecessary mov-operations" as part of a bytecode optimization pass.
+    let lhs_val = if might_contain_assignment_expression(rhs) {
+        generator.copy_if_needed_to_preserve_evaluation_order(&lhs_val)
+    } else {
+        lhs_val
+    };
+
     let rhs_val = match op {
         BinaryOp::BitwiseAnd | BinaryOp::BitwiseOr | BinaryOp::BitwiseXor => {
             if let ExpressionKind::NumericLiteral(n) = &rhs.inner {

Tests/LibJS/Runtime/evaluation-order.js

@@ -87,3 +87,19 @@ test("evaluation order for compound assignment", () => {
     let result = foo(2);
     expect(result).toBe(7);
 });
+
+test("evaluation order for binary operators (RHS reassigns)", () => {
+    function foo(value) {
+        return value + (value = 5);
+    }
+    let result = foo(2);
+    expect(result).toBe(7);
+});
+
+test("evaluation order for binary operators (LHS reassigns)", () => {
+    function foo(value) {
+        return (value = 5) + value;
+    }
+    let result = foo(2);
+    expect(result).toBe(10);
+});