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

InputCommon: Fix input expression assignment operator behavior.

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_LTHAN:
+      return lhs_value < rhs_value;
+    case TOK_GTHAN:
+      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:
     {
-      // Use this carefully as it's extremely powerful and can end up in unforeseen situations
+      Expression* const lvalue = lhs->GetLValue();
-      lhs->SetValue(rhs->GetValue());
+      const ControlState rvalue = rhs->GetValue();
-      return lhs->GetValue();
+      lvalue->SetValue(rvalue);
+      return lvalue;
     }
-    case TOK_LTHAN:
-      return lhs->GetValue() < rhs->GetValue();
-    case TOK_GTHAN:
-      return lhs->GetValue() > rhs->GetValue();
     case TOK_COMMA:
-    {
-      // Eval and discard lhs:
       lhs->GetValue();
-      return rhs->GetValue();
+      return rhs->GetLValue();
-    }
-    case TOK_XOR:
-    {
-      const auto lval = lhs->GetValue();
-      const auto rval = rhs->GetValue();
-      return std::max(std::min(1 - lval, rval), std::min(lval, 1 - rval));
-    }
     default:
-      ASSERT(false);
+      return this;
-      return 0;
     }
   }
 
@@ -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;
+  HotkeySuppressions::Suppressor m_suppressor;
-  mutable bool m_is_blocked = false;
+  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));

Source/Core/InputCommon/ControlReference/ExpressionParser.h

@@ -170,10 +170,13 @@ class Expression
 {
 public:
   virtual ~Expression() = default;
-  virtual ControlState GetValue() const = 0;
+  virtual ControlState GetValue() = 0;
   virtual void SetValue(ControlState state) = 0;
   virtual int CountNumControls() const = 0;
   virtual void UpdateReferences(ControlEnvironment& finder) = 0;
+
+  // Perform any side effects and return Expression to be SetValue'd.
+  virtual Expression* GetLValue();
 };
 
 class ParseResult

Source/Core/InputCommon/ControlReference/FunctionExpression.cpp

@@ -26,7 +26,7 @@ private:
       return ExpectedArguments{"toggle_state_input, [clear_state_input]"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     const ControlState inner_value = GetArg(0).GetValue();
 
@@ -48,8 +48,8 @@ private:
     return m_state;
   }
 
-  mutable bool m_released{};
+  bool m_released{};
-  mutable bool m_state{};
+  bool m_state{};
 };
 
 // usage: not(expression)
@@ -65,7 +65,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override { return 1.0 - GetArg(0).GetValue(); }
+  ControlState GetValue() override { return 1.0 - GetArg(0).GetValue(); }
   void SetValue(ControlState value) override { GetArg(0).SetValue(1.0 - value); }
 };
 
@@ -82,7 +82,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override { return std::abs(GetArg(0).GetValue()); }
+  ControlState GetValue() override { return std::abs(GetArg(0).GetValue()); }
 };
 
 // usage: sin(expression)
@@ -98,7 +98,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override { return std::sin(GetArg(0).GetValue()); }
+  ControlState GetValue() override { return std::sin(GetArg(0).GetValue()); }
 };
 
 // usage: cos(expression)
@@ -114,7 +114,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override { return std::cos(GetArg(0).GetValue()); }
+  ControlState GetValue() override { return std::cos(GetArg(0).GetValue()); }
 };
 
 // usage: tan(expression)
@@ -130,7 +130,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override { return std::tan(GetArg(0).GetValue()); }
+  ControlState GetValue() override { return std::tan(GetArg(0).GetValue()); }
 };
 
 // usage: asin(expression)
@@ -146,7 +146,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override { return std::asin(GetArg(0).GetValue()); }
+  ControlState GetValue() override { return std::asin(GetArg(0).GetValue()); }
 };
 
 // usage: acos(expression)
@@ -162,7 +162,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override { return std::acos(GetArg(0).GetValue()); }
+  ControlState GetValue() override { return std::acos(GetArg(0).GetValue()); }
 };
 
 // usage: atan(expression)
@@ -178,7 +178,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override { return std::atan(GetArg(0).GetValue()); }
+  ControlState GetValue() override { return std::atan(GetArg(0).GetValue()); }
 };
 
 // usage: atan2(y, x)
@@ -194,7 +194,7 @@ private:
       return ExpectedArguments{"y, x"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     return std::atan2(GetArg(0).GetValue(), GetArg(1).GetValue());
   }
@@ -213,7 +213,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override { return std::sqrt(GetArg(0).GetValue()); }
+  ControlState GetValue() override { return std::sqrt(GetArg(0).GetValue()); }
 };
 
 // usage: pow(base, exponent)
@@ -229,10 +229,7 @@ private:
       return ExpectedArguments{"base, exponent"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override { return std::pow(GetArg(0).GetValue(), GetArg(1).GetValue()); }
-  {
-    return std::pow(GetArg(0).GetValue(), GetArg(1).GetValue());
-  }
 };
 
 // usage: min(a, b)
@@ -248,10 +245,7 @@ private:
       return ExpectedArguments{"a, b"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override { return std::min(GetArg(0).GetValue(), GetArg(1).GetValue()); }
-  {
-    return std::min(GetArg(0).GetValue(), GetArg(1).GetValue());
-  }
 };
 
 // usage: max(a, b)
@@ -267,10 +261,7 @@ private:
       return ExpectedArguments{"a, b"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override { return std::max(GetArg(0).GetValue(), GetArg(1).GetValue()); }
-  {
-    return std::max(GetArg(0).GetValue(), GetArg(1).GetValue());
-  }
 };
 
 // usage: clamp(value, min, max)
@@ -286,7 +277,7 @@ private:
       return ExpectedArguments{"value, min, max"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     return std::clamp(GetArg(0).GetValue(), GetArg(1).GetValue(), GetArg(2).GetValue());
   }
@@ -305,7 +296,7 @@ private:
       return ExpectedArguments{"seconds"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     const auto now = Clock::now();
     const auto elapsed = now - m_start_time;
@@ -332,7 +323,7 @@ private:
   }
 
 private:
-  mutable Clock::time_point m_start_time = Clock::now();
+  Clock::time_point m_start_time = Clock::now();
 };
 
 // usage: if(condition, true_expression, false_expression)
@@ -348,10 +339,11 @@ private:
       return ExpectedArguments{"condition, true_expression, false_expression"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override { return GetLValue()->GetValue(); }
+
+  Expression* GetLValue() override
   {
-    return (GetArg(0).GetValue() > CONDITION_THRESHOLD) ? GetArg(1).GetValue() :
+    return (GetArg(0).GetValue() > CONDITION_THRESHOLD) ? &GetArg(1) : &GetArg(2);
-                                                          GetArg(2).GetValue();
   }
 };
 
@@ -368,7 +360,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     // Subtraction for clarity:
     return 0.0 - GetArg(0).GetValue();
@@ -388,7 +380,7 @@ private:
       return ExpectedArguments{"expression"};
   }
 
-  ControlState GetValue() const override { return GetArg(0).GetValue(); }
+  ControlState GetValue() override { return GetArg(0).GetValue(); }
 };
 
 // usage: deadzone(input, amount)
@@ -403,7 +395,7 @@ class DeadzoneExpression : public FunctionExpression
       return ExpectedArguments{"input, amount"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     const ControlState val = GetArg(0).GetValue();
     const ControlState deadzone = GetArg(1).GetValue();
@@ -424,7 +416,7 @@ class SmoothExpression : public FunctionExpression
       return ExpectedArguments{"input, seconds_up, seconds_down = seconds_up"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     const auto now = Clock::now();
     const auto elapsed = now - m_last_update;
@@ -452,8 +444,8 @@ class SmoothExpression : public FunctionExpression
   }
 
 private:
-  mutable ControlState m_value = 0.0;
+  ControlState m_value = 0.0;
-  mutable Clock::time_point m_last_update = Clock::now();
+  Clock::time_point m_last_update = Clock::now();
 };
 
 // usage: hold(input, seconds)
@@ -468,7 +460,7 @@ class HoldExpression : public FunctionExpression
       return ExpectedArguments{"input, seconds"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     const auto now = Clock::now();
 
@@ -491,8 +483,8 @@ class HoldExpression : public FunctionExpression
   }
 
 private:
-  mutable bool m_state = false;
+  bool m_state = false;
-  mutable Clock::time_point m_start_time = Clock::now();
+  Clock::time_point m_start_time = Clock::now();
 };
 
 // usage: tap(input, seconds, taps=2)
@@ -507,7 +499,7 @@ class TapExpression : public FunctionExpression
       return ExpectedArguments{"input, seconds, taps = 2"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     const auto now = Clock::now();
 
@@ -549,9 +541,9 @@ class TapExpression : public FunctionExpression
   }
 
 private:
-  mutable bool m_released = true;
+  bool m_released = true;
-  mutable u32 m_taps = 0;
+  u32 m_taps = 0;
-  mutable Clock::time_point m_start_time = Clock::now();
+  Clock::time_point m_start_time = Clock::now();
 };
 
 // usage: relative(input, speed, [max_abs_value, [shared_state]])
@@ -567,7 +559,7 @@ class RelativeExpression : public FunctionExpression
       return ExpectedArguments{"input, speed, [max_abs_value, [shared_state]]"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     // There is a lot of funky math in this function but it allows for a variety of uses:
     //
@@ -610,8 +602,8 @@ class RelativeExpression : public FunctionExpression
   }
 
 private:
-  mutable ControlState m_state = 0.0;
+  ControlState m_state = 0.0;
-  mutable Clock::time_point m_last_update = Clock::now();
+  Clock::time_point m_last_update = Clock::now();
 };
 
 // usage: pulse(input, seconds)
@@ -626,7 +618,7 @@ class PulseExpression : public FunctionExpression
       return ExpectedArguments{"input, seconds"};
   }
 
-  ControlState GetValue() const override
+  ControlState GetValue() override
   {
     const auto now = Clock::now();
 
@@ -662,9 +654,9 @@ class PulseExpression : public FunctionExpression
   }
 
 private:
-  mutable bool m_released = false;
+  bool m_released = false;
-  mutable bool m_state = false;
+  bool m_state = false;
-  mutable Clock::time_point m_release_time = Clock::now();
+  Clock::time_point m_release_time = Clock::now();
 };
 
 std::unique_ptr<FunctionExpression> MakeFunctionExpression(std::string_view name)
@@ -752,11 +744,6 @@ Expression& FunctionExpression::GetArg(u32 number)
   return *m_args[number];
 }
 
-const Expression& FunctionExpression::GetArg(u32 number) const
-{
-  return *m_args[number];
-}
-
 u32 FunctionExpression::GetArgCount() const
 {
   return u32(m_args.size());

Source/Core/InputCommon/ControlReference/FunctionExpression.h

@@ -42,7 +42,6 @@ protected:
   ValidateArguments(const std::vector<std::unique_ptr<Expression>>& args) = 0;
 
   Expression& GetArg(u32 number);
-  const Expression& GetArg(u32 number) const;
   u32 GetArgCount() const;
 
 private: