Refactor some inplace ops for error handling... (#1208)

* Add an eval_ function for common inplace operators.
* Remove the use of rewrite rules which don't handle initialized variables properly.
* Add test for uninitialized variables in Increment/Decrement routines.

Fixes #1205

Author: rocky

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 = "*="

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"

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.",

mathics/eval/assignments/__init__.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

mathics/eval/assignments/assign_binaryop.py

@@ -0,0 +1 @@
+"""Unit tests for test.builtin.assignments"""