Repository: Mathics3/mathics-core (language: python)

Issue:
`Increment` and related functions hit the recursion limit when the input is undefined
<!-- Feel free to remove sections marked optional if they are not relevant. The other sections please fill out. See also https://github.com/Mathics3/mathics-core/blob/master/REPORTING-BUGS.md -->

## Description

<!-- Please add a clear and concise description of the bug. Try to reduce the bug to the minimum expression that exhibits the bug.
     For example, if an an expression occurs with several terms, can these be removed and the bug still appears? If so
	 we would like the reduced, simpler expression.

	 If the expression can be written in SymPy, does SymPy also exhibit the problem?
-->

## How to Reproduce

```mathematica
In[1]:= Clear[i]; Increment[i]
$RecursionLimit::reclim: Recursion depth of 200 exceeded.
Out[1]= $Aborted

In[2]:=  Clear[i]; PreIncrement[i]
$RecursionLimit::reclim: Recursion depth of 200 exceeded.
Out[2]= $Aborted

In[3]:=  Clear[i]; Decrement[i]
$RecursionLimit::reclim: Recursion depth of 200 exceeded.
Out[3]= $Aborted

In[4]:=  Clear[i]; PreDecrement[i]
$RecursionLimit::reclim: Recursion depth of 200 exceeded.
Out[4]= $Aborted

In[5]:= i
$RecursionLimit::reclim: Recursion depth of 200 exceeded.
Out[5]= $Aborted
```

## Output Given

```mathematica
$RecursionLimit::reclim: Recursion depth of 200 exceeded.
```

## Expected behavior

No recursion. An error message indicating that the input is undefined would be good.

## Your Environment

```python
Mathics 7.0.1dev0
on CPython 3.11.10 | packaged by conda-forge | (main, Oct 16 2024, 01:27:36) [GCC 13.3.0]
using SymPy 1.13.3, mpmath 1.3.0, numpy 2.1.3, cython Not installed
```

## Workarounds

Workaround example for increment:
```mathematica
Unprotect[Increment]; Increment[i] := Undefined /; Not[ValueQ[i]]; Protect[Increment];
```

## Priority

N/A

## Additional context

N/A


Discussion / hints:

Fix patch:
diff --git a/mathics/builtin/assignments/assign_binaryop.py b/mathics/builtin/assignments/assign_binaryop.py
index b04ae4833..49b8d226e 100644
--- a/mathics/builtin/assignments/assign_binaryop.py
+++ b/mathics/builtin/assignments/assign_binaryop.py
@@ -16,11 +16,33 @@
 """
 
 
+from mathics.core.atoms import Integer1, IntegerM1
 from mathics.core.attributes import A_HOLD_FIRST, A_PROTECTED, A_READ_PROTECTED
 from mathics.core.builtin import InfixOperator, PostfixOperator, PrefixOperator
+from mathics.core.evaluation import Evaluation
+from mathics.core.systemsymbols import SymbolPlus
+from mathics.eval.assignments.assign_binaryop import eval_inplace_op
 
 
-class AddTo(InfixOperator):
+class InplaceInfixOperator:
+    grouping = "Right"
+    operator_symbol = SymbolPlus
+    increment_symbol = Integer1
+    returns_updated_value: bool = True
+
+    def eval(self, expr, evaluation: Evaluation):
+        """%(name)s[expr_]"""
+        return eval_inplace_op(
+            self,
+            expr,
+            self.operator_symbol,
+            self.increment_symbol,
+            self.returns_updated_value,
+            evaluation,
+        )
+
+
+class AddTo(InfixOperator, InplaceInfixOperator):
     """
     <url>:WMA link:https://reference.wolfram.com/language/ref/AddTo.html</url>
 
@@ -38,16 +60,27 @@ class AddTo(InfixOperator):
     """
 
     attributes = A_HOLD_FIRST | A_PROTECTED
-    grouping = "Right"
     operator = "+="
 
-    rules = {
-        "x_ += dx_": "x = x + dx",
-    }
+    operator_symbol = SymbolPlus
+    return_before_value: bool = True
     summary_text = "add a value and assigns that returning the new value"
 
-
-class Decrement(PostfixOperator):
+    def eval(self, expr, increment, evaluation: Evaluation):
+        """%(name)s[expr_, increment_]"""
+        return eval_inplace_op(
+            self,
+            expr,
+            self.operator_symbol,
+            increment,
+            self.return_before_value,
+            evaluation,
+        )
+
+
+# Decrement has Infix properties for evaluation purposes, but Postfix
+# properties for operator association.
+class Decrement(InplaceInfixOperator, InfixOperator, PostfixOperator):
     """
     <url>:WMA link
     :https://reference.wolfram.com/language/ref/Decrement.html</url>
@@ -59,26 +92,43 @@ class Decrement(PostfixOperator):
       <dd>decrements $x$ by 1, returning the original value of $x$.
     </dl>
 
-    >> a = 5;
-    X> a--
+    >> a = 5; a--
      = 5
-    X> a
+    >> a
      = 4
+
+    Decrement a numerical value:
+
+    >> a = 1.6; a--; a
+     = 0.6
+
+    Decrement all values in a list:
+
+    >> a = {1, 3, 5}
+     = {1, 3, 5}
+
+    >> a--; a
+     = {0, 2, 4}
+
+    Compare with <url>:PreDecrement:
+    /doc/reference-of-built-in-symbols/assignments/in-place-binary-assignment-operator/predecrement
+    </url> which returns the value before updating, and <url>:Increment:
+    /doc/reference-of-built-in-symbols/assignments/in-place-binary-assignment-operator/increment
+    </url> which goes the other way.
     """
 
-    operator = "--"
     attributes = A_HOLD_FIRST | A_PROTECTED | A_READ_PROTECTED
+    increment_symbol = IntegerM1
+    operator = "--"
+    operator_symbol = SymbolPlus
 
-    rules = {
-        "x_--": "Module[{t=x}, x = x - 1; t]",
-    }
-
+    returns_updated_value = False
     summary_text = (
         "decreases the value by one and assigns that returning the original value"
     )
 
 
-class DivideBy(InfixOperator):
+class DivideBy(InplaceInfixOperator, InfixOperator):
     """
     <url>:WMA link:https://reference.wolfram.com/language/ref/DivideBy.html</url>
 
@@ -105,7 +155,7 @@ class DivideBy(InfixOperator):
     summary_text = "divide a value and assigns that returning the new value"
 
 
-class Increment(PostfixOperator):
+class Increment(InplaceInfixOperator, InfixOperator, PostfixOperator):
     """
     <url>:WMA link:
     https://reference.wolfram.com/language/ref/Increment.html</url>
@@ -117,34 +167,90 @@ class Increment(PostfixOperator):
       <dd>increments $x$ by 1, returning the original value of $x$.
     </dl>
 
-    >> a = 2;
-    >> a++
+    >> a = 1; a++
+     = 1
+    >> a
      = 2
+
+    Increment a numeric value:
+
+    >> a = 1.5; a++
+     = 1.5
+
     >> a
-     = 3
+     = 2.5
+
+    Increment a symbolic value:
+
+    >> y = 2 x; y++; y
+     = 1 + 2 x
+
+    Increment all values in a list:
+
+    >> x = {1, 3, 5}
+     = {1, 3, 5}
+
+    x++; x
+     = {2, 4, 6}
+
     Grouping of 'Increment', 'PreIncrement' and 'Plus':
     >> ++++a+++++2//Hold//FullForm
      = Hold[Plus[PreIncrement[PreIncrement[Increment[Increment[a]]]], 2]]
-    """
 
-    operator = "++"
-    attributes = A_HOLD_FIRST | A_PROTECTED | A_READ_PROTECTED
+    Compare with <url>:PreIncrement:
+    /doc/reference-of-built-in-symbols/assignments/in-place-binary-assignment-operator/preincrement
+    </url> which returns the value before update.
 
-    rules = {
-        "x_++": (
-            "Module[{Internal`IncrementTemporary = x},"
-            "       x = x + 1;"
-            "       Internal`IncrementTemporary"
-            "]"
-        ),
-    }
+    #> Clear[a, x, y]
+    """
 
+    attributes = A_HOLD_FIRST | A_PROTECTED | A_READ_PROTECTED
+    increment_symbol = Integer1
+    operator = "++"
+    operator_symbol = SymbolPlus
+    returns_updated_value: bool = False
     summary_text = (
         "increases the value by one and assigns that returning the original value"
     )
 
 
-class PreIncrement(PrefixOperator):
+class PreDecrement(InplaceInfixOperator, PrefixOperator):
+    """
+    <url>:WMA link:
+    https://reference.wolfram.com/language/ref/PreDecrement.html</url>
+
+    <dl>
+      <dt>'PreDecrement[$x$]'
+
+      <dt>'--$x$'
+      <dd>decrements $x$ by 1, returning the new value of $x$.
+    </dl>
+
+    '--$a$' is equivalent to '$a$ = $a$ - 1':
+    >> a = 2;
+    >> --a
+     = 1
+    >> a
+     = 1
+
+    Compare with <url>:Decrement:
+    /doc/reference-of-built-in-symbols/assignments/in-place-binary-assignment-operator/decrement
+    </url> which returns the updated value, and <url>:Increment:
+    /doc/reference-of-built-in-symbols/assignments/in-place-binary-assignment-operator/increment
+    </url> which goes the other way.
+
+    #> Clear[a]
+    """
+
+    attributes = A_HOLD_FIRST | A_PROTECTED | A_READ_PROTECTED
+    increment_symbol = IntegerM1
+    operator = "--"
+    operator_symbol = SymbolPlus
+    returns_updated_value: bool = True
+    summary_text = "decrease the value by one and assigns that returning the new value"
+
+
+class PreIncrement(InplaceInfixOperator, PrefixOperator):
     """
     <url>:WMA link:
     https://reference.wolfram.com/language/ref/PreIncrement.html</url>
@@ -156,50 +262,46 @@ class PreIncrement(PrefixOperator):
     </dl>
 
     '++$a$' is equivalent to '$a$ = $a$ + 1':
-    >> a = 2;
+    >> a = 2
+     = 2
+
     >> ++a
      = 3
+
     >> a
      = 3
-    """
 
-    attributes = A_HOLD_FIRST | A_PROTECTED | A_READ_PROTECTED
-    operator = "++"
+    PreIncrement a numeric value:
 
-    rules = {
-        "++x_": "x = x + 1",
-    }
+    >> a + 1.6
+     = 4.6
 
-    summary_text = "increase the value by one and assigns that returning the new value"
 
+    PreIncrement a symbolic value:
 
-class PreDecrement(PrefixOperator):
-    """
-    <url>:WMA link:
-    https://reference.wolfram.com/language/ref/PreDecrement.html</url>
+    #> Clear[x, y]
 
-    <dl>
-      <dt>'PreDecrement[$x$]'
+    >> y = x; ++y
+     = 1 + x
 
-      <dt>'--$x$'
-      <dd>decrements $x$ by 1, returning the new value of $x$.
-    </dl>
+    >> y
+     = 1 + x
 
-    '--$a$' is equivalent to '$a$ = $a$ - 1':
-    >> a = 2;
-    >> --a
-     = 1
-    >> a
-     = 1
+    Compare with <url>:Increment:
+    /doc/reference-of-built-in-symbols/assignments/in-place-binary-assignment-operator/increment
+    </url> which returns the updated value, and <url>:PreDecrement:
+    /doc/reference-of-built-in-symbols/assignments/in-place-binary-assignment-operator/predecrement
+    </url> which goes the other way.
+
+    #> Clear[a, x, y]
     """
 
-    operator = "--"
     attributes = A_HOLD_FIRST | A_PROTECTED | A_READ_PROTECTED
+    operator = "++"
+    operator_symbol = SymbolPlus
+    return_before_value: bool = False
 
-    rules = {
-        "--x_": "x = x - 1",
-    }
-    summary_text = "decrease the value by one and assigns that returning the new value"
+    summary_text = "increase the value by one and assigns that returning the new value"
 
 
 class SubtractFrom(InfixOperator):
@@ -218,6 +320,8 @@ class SubtractFrom(InfixOperator):
      = 8
     >> a
      = 8
+
+    #> Clear[a]
     """
 
     attributes = A_HOLD_FIRST | A_PROTECTED
@@ -245,6 +349,8 @@ class TimesBy(InfixOperator):
      = 20
     >> a
      = 20
+
+    #> Clear[a]
     """
 
     operator = "*="
diff --git a/mathics/builtin/list/eol.py b/mathics/builtin/list/eol.py
index 9dbbb3686..01da60253 100644
--- a/mathics/builtin/list/eol.py
+++ b/mathics/builtin/list/eol.py
@@ -129,9 +129,6 @@ class AppendTo(Builtin):
 
     attributes = A_HOLD_FIRST | A_PROTECTED
 
-    messages = {
-        "rvalue": "`1` is not a variable with a value, so its value cannot be changed.",
-    }
     summary_text = "add an element at the end of an stored list or expression"
 
     def eval(self, s, element, evaluation):
@@ -1344,7 +1341,6 @@ class PrependTo(Builtin):
     attributes = A_HOLD_FIRST | A_PROTECTED
 
     messages = {
-        "rvalue": "`1` is not a variable with a value, so its value cannot be changed.",
         "normal": "Nonatomic expression expected at position 1 in `1`.",
     }
     summary_text = "add an element at the beginning of an stored list or expression"
diff --git a/mathics/builtin/messages.py b/mathics/builtin/messages.py
index 31dc48d38..725a15897 100644
--- a/mathics/builtin/messages.py
+++ b/mathics/builtin/messages.py
@@ -224,6 +224,7 @@ class General(Builtin):
         "pspec": (
             "Part specification `1` is neither an integer nor " "a list of integer."
         ),
+        "rvalue": "`1` is not a variable with a value, so its value cannot be changed.",
         "seqs": "Sequence specification expected, but got `1`.",
         "setp": "Part assignment to `1` could not be made",
         "setps": "`1` in the part assignment is not a symbol.",
diff --git a/mathics/eval/assignments/__init__.py b/mathics/eval/assignments/__init__.py
new file mode 100644
index 000000000..e69de29bb
diff --git a/mathics/eval/assignments/assign_binaryop.py b/mathics/eval/assignments/assign_binaryop.py
new file mode 100644
index 000000000..318e187c7
--- /dev/null
+++ b/mathics/eval/assignments/assign_binaryop.py
@@ -0,0 +1,30 @@
+"""
+evaluation functions associated with the mathics.builtin.assign_binary_op
+module.
+"""
+from mathics.core.evaluation import Evaluation
+from mathics.core.expression import Expression
+from mathics.core.systemsymbols import SymbolSet
+
+
+def eval_inplace_op(
+    self, x, operator, increment, returns_updated_value: bool, evaluation: Evaluation
+):
+    """
+    Perform: Set[x, operator[x, increment]]
+
+    For example if operator is + and increment is 1, perform x += 1
+
+    If returns_updated_value is True, return the value after performing
+    the update operation. Otherwise return the value before it was
+    modified.
+    """
+    value_before_update = x.evaluate(evaluation)
+    if x == value_before_update:
+        evaluation.message(self.__class__.__name__, "rvalue", x)
+        return
+
+    updated_value = Expression(
+        SymbolSet, x, Expression(operator, x, increment).evaluate(evaluation)
+    ).evaluate(evaluation)
+    return updated_value if returns_updated_value else value_before_update


Test patch:
diff --git a/test/builtin/assignments/__init__.py b/test/builtin/assignments/__init__.py
new file mode 100644
index 000000000..71d310e06
--- /dev/null
+++ b/test/builtin/assignments/__init__.py
@@ -0,0 +1,1 @@
+"""Unit tests for test.builtin.assignments"""
diff --git a/test/builtin/assignments/test_assign_binaryop.py b/test/builtin/assignments/test_assign_binaryop.py
new file mode 100644
index 000000000..a02a5e31a
--- /dev/null
+++ b/test/builtin/assignments/test_assign_binaryop.py
@@ -0,0 +1,32 @@
+# -*- coding: utf-8 -*-
+"""
+Unit tests for mathics.builtins.assignments.assign_binaryop
+"""
+
+from test.helper import check_evaluation, session
+
+
+def test_uninitialized():
+    session.evaluate("Clear[a]")
+    for inplace_operator, str_expected in (
+        ("Decrement", "a--"),
+        ("Increment", "a++"),
+        ("PreDecrement", "--a"),
+        ("PreIncrement", "++a"),
+    ):
+        check_evaluation(
+            str_expr=f"{inplace_operator}[a]",
+            str_expected=str_expected,
+            to_string_expr=True,
+            expected_messages=(
+                "a is not a variable with a value, so its value cannot be changed.",
+            ),
+        ),
+
+
+def test_predecrement():
+    check_evaluation(
+        "--5",
+        "--5",
+        failure_message="PreDecrement::rvalue: 5 is not a variable with a value, so its value cannot be changed.",
+    )
diff --git a/test/builtin/test_assignment.py b/test/builtin/test_assignment.py
index 476180249..acea25312 100644
--- a/test/builtin/test_assignment.py
+++ b/test/builtin/test_assignment.py
@@ -375,12 +375,6 @@ def test_set_and_clear_messages(str_expr, str_expected, message, out_msgs):
     )
 
 
-def test_predecrement():
-    check_evaluation(
-        "--5", "4", failure_message="Set::setraw: Cannot assign to raw object 5."
-    )
-
-
 def test_assign_list():
     check_evaluation("G[x_Real]=x^2; a={G[x]}; {x=1.; a, x=.; a}", "{{1.}, {G[x]}}")