Merge pull request #13314 from jordan-woyak/input-expressions-assignment-op-fix

InputCommon: Fix input expression assignment operator behavior.

1 files changed, 66 insertions, 39 deletions

diff --git a/Source/Core/InputCommon/ControlReference/ExpressionParser.cpp b/Source/Core/InputCommon/ControlReference/ExpressionParser.cpp
index 192e8a78bd..84f856367e 100644
--- a/Source/Core/InputCommon/ControlReference/ExpressionParser.cpp
+++ b/Source/Core/InputCommon/ControlReference/ExpressionParser.cpp
@@ -4,9 +4,9 @@
 #include "InputCommon/ControlReference/ExpressionParser.h"
 
 #include <algorithm>
+#include <cassert>
 #include <cmath>
 #include <functional>
-#include <iostream>
 #include <map>
 #include <memory>
 #include <regex>
@@ -14,7 +14,6 @@
 #include <utility>
 #include <vector>
 
-#include "Common/Assert.h"
 #include "Common/MsgHandler.h"
 #include "Common/StringUtil.h"
 
@@ -248,12 +247,17 @@ ParseStatus Lexer::Tokenize(std::vector<Token>& tokens)
   return ParseStatus::Successful;
 }
 
+Expression* Expression::GetLValue()
+{
+  return this;
+}
+
 class ControlExpression : public Expression
 {
 public:
   explicit ControlExpression(ControlQualifier qualifier) : m_qualifier(std::move(qualifier)) {}
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     if (s_hotkey_suppressions.IsSuppressed(m_input))
       return 0;
@@ -348,55 +352,67 @@ public:
   {
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
+    if (op == TOK_ASSIGN || op == TOK_COMMA)
+    {
+      return GetLValue()->GetValue();
+    }
+
+    // Strict evaluation order of lhs,rhs in case of side effects.
+    const ControlState lhs_value = lhs->GetValue();
+    const ControlState rhs_value = rhs->GetValue();
+
     switch (op)
     {
     case TOK_AND:
-      return std::min(lhs->GetValue(), rhs->GetValue());
+      return std::min(lhs_value, rhs_value);
     case TOK_OR:
-      return std::max(lhs->GetValue(), rhs->GetValue());
+      return std::max(lhs_value, rhs_value);
     case TOK_ADD:
-      return lhs->GetValue() + rhs->GetValue();
+      return lhs_value + rhs_value;
     case TOK_SUB:
-      return lhs->GetValue() - rhs->GetValue();
+      return lhs_value - rhs_value;
     case TOK_MUL:
-      return lhs->GetValue() * rhs->GetValue();
+      return lhs_value * rhs_value;
     case TOK_DIV:
     {
-      const ControlState result = lhs->GetValue() / rhs->GetValue();
+      const ControlState result = lhs_value / rhs_value;
       return std::isinf(result) ? 0.0 : result;
     }
     case TOK_MOD:
     {
-      const ControlState result = std::fmod(lhs->GetValue(), rhs->GetValue());
+      const ControlState result = std::fmod(lhs_value, rhs_value);
       return std::isnan(result) ? 0.0 : result;
     }
-    case TOK_ASSIGN:
-    {
-      // Use this carefully as it's extremely powerful and can end up in unforeseen situations
-      lhs->SetValue(rhs->GetValue());
-      return lhs->GetValue();
-    }
     case TOK_LTHAN:
-      return lhs->GetValue() < rhs->GetValue();
+      return lhs_value < rhs_value;
     case TOK_GTHAN:
-      return lhs->GetValue() > rhs->GetValue();
-    case TOK_COMMA:
-    {
-      // Eval and discard lhs:
-      lhs->GetValue();
-      return rhs->GetValue();
-    }
+      return lhs_value > rhs_value;
     case TOK_XOR:
+      return std::max(std::min(1 - lhs_value, rhs_value), std::min(lhs_value, 1 - rhs_value));
+    default:
+      assert(false);
+      return 0;
+    }
+  }
+
+  Expression* GetLValue() override
+  {
+    switch (op)
+    {
+    case TOK_ASSIGN:
     {
-      const auto lval = lhs->GetValue();
-      const auto rval = rhs->GetValue();
-      return std::max(std::min(1 - lval, rval), std::min(lval, 1 - rval));
+      Expression* const lvalue = lhs->GetLValue();
+      const ControlState rvalue = rhs->GetValue();
+      lvalue->SetValue(rvalue);
+      return lvalue;
     }
+    case TOK_COMMA:
+      lhs->GetValue();
+      return rhs->GetLValue();
     default:
-      ASSERT(false);
-      return 0;
+      return this;
     }
   }
 
@@ -444,7 +460,7 @@ class LiteralReal : public LiteralExpression
 public:
   explicit LiteralReal(ControlState value) : m_value(value) {}
 
-  ControlState GetValue() const override { return m_value; }
+  ControlState GetValue() override { return m_value; }
 
   std::string GetName() const override { return ValueToString(m_value); }
 
@@ -466,7 +482,7 @@ class VariableExpression : public Expression
 public:
   explicit VariableExpression(std::string name) : m_name(std::move(name)) {}
 
-  ControlState GetValue() const override { return m_variable_ptr ? *m_variable_ptr : 0; }
+  ControlState GetValue() override { return m_variable_ptr ? *m_variable_ptr : 0; }
 
   void SetValue(ControlState value) override
   {
@@ -496,7 +512,7 @@ public:
     m_modifiers.pop_back();
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     // True if we have no modifiers
     const bool modifiers_pressed = std::ranges::all_of(
@@ -557,7 +573,7 @@ public:
   }
 
 private:
-  void EnableSuppression(bool force = false) const
+  void EnableSuppression(bool force = false)
   {
     if (!m_suppressor || force)
       m_suppressor = s_hotkey_suppressions.MakeSuppressor(&m_modifiers, &m_final_input);
@@ -565,8 +581,8 @@ private:
 
   HotkeySuppressions::Modifiers m_modifiers;
   std::unique_ptr<ControlExpression> m_final_input;
-  mutable HotkeySuppressions::Suppressor m_suppressor;
-  mutable bool m_is_blocked = false;
+  HotkeySuppressions::Suppressor m_suppressor;
+  bool m_is_blocked = false;
 };
 
 // This class proxies all methods to its either left-hand child if it has bound controls, or its
@@ -582,7 +598,7 @@ public:
   {
   }
 
-  ControlState GetValue() const override { return GetActiveChild()->GetValue(); }
+  ControlState GetValue() override { return GetActiveChild()->GetValue(); }
   void SetValue(ControlState value) override { GetActiveChild()->SetValue(value); }
 
   int CountNumControls() const override { return GetActiveChild()->CountNumControls(); }
@@ -886,6 +902,18 @@ private:
     }
   }
 
+  static bool IsRTLBinaryOp(TokenType type) { return type == TOK_ASSIGN; }
+
+  static bool IsBinaryOpWithPrecedence(Token tok, int precedence)
+  {
+    if (!tok.IsBinaryOperator())
+      return false;
+
+    const int tok_precedence = OperatorPrecedence(tok.type);
+    return (tok_precedence < precedence) ||
+           (IsRTLBinaryOp(tok.type) && tok_precedence <= precedence);
+  }
+
   ParseResult ParseInfixOperations(int precedence = OperatorPrecedence())
   {
     ParseResult lhs = ParseAtom(Chew());
@@ -895,11 +923,10 @@ private:
 
     std::unique_ptr<Expression> expr = std::move(lhs.expr);
 
-    // TODO: handle LTR/RTL associativity?
     while (true)
     {
       const Token op = Peek();
-      if (op.IsBinaryOperator() && OperatorPrecedence(op.type) < precedence)
+      if (IsBinaryOpWithPrecedence(op, precedence))
       {
         Chew();
         ParseResult rhs = ParseInfixOperations(OperatorPrecedence(op.type));