non-recursive SameQ[] (#1077)

This PR is a proposal for a reimplementation of `Expression.sameQ` which
uses a tree transversal iterative algorithm instead of recursion. Using
this implementation, we avoid the recursion issue in the Pytests for
Python 3.12.


---------

Co-authored-by: R. Bernstein <[email protected]>

.github/workflows/ubuntu-cython.yml

@@ -13,7 +13,7 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        python-version: ['3.11']
+        python-version: ['3.12', '3.11']
     steps:
     - uses: actions/checkout@v4
     - name: Set up Python ${{ matrix.python-version }}

.github/workflows/ubuntu.yml

@@ -8,10 +8,10 @@ on:
 
 jobs:
   build:
-    runs-on: ubuntu-20.04
+    runs-on: ubuntu-latest
     strategy:
       matrix:
-        python-version: ['3.11', '3.8', '3.9', '3.10']
+        python-version: ['3.12', '3.11', '3.8', '3.9', '3.10']
     steps:
     - uses: actions/checkout@v4
     - name: Set up Python ${{ matrix.python-version }}

.github/workflows/windows.yml

@@ -14,7 +14,7 @@ jobs:
         os: [windows]
         # "make doctest" on MS Windows fails without showing much of a
         # trace of where things went wrong on Python before 3.11.
-        python-version: ['3.11']
+        python-version: ['3.12']
     steps:
     - uses: actions/checkout@v4
     - name: Set up Python ${{ matrix.python-version }}

mathics/core/element.py

@@ -5,7 +5,7 @@
 Here we have the base class and related function for element inside an Expression.
 """
 
-
+from abc import ABC
 from typing import Any, Optional, Tuple
 
 from mathics.core.attributes import A_NO_ATTRIBUTES
@@ -193,7 +193,7 @@ def __ne__(self, other) -> bool:
         ) or self.get_sort_key() != other.get_sort_key()
 
 
-class BaseElement(KeyComparable):
+class BaseElement(KeyComparable, ABC):
     """
     This is the base class from which all other Expressions are
     derived from.  If you think of an Expression as tree-like, then a
@@ -290,7 +290,7 @@ def get_float_value(self, permit_complex=False):
     def get_int_value(self):
         return None
 
-    def get_lookup_name(self):
+    def get_lookup_name(self) -> str:
         """
         Returns symbol name of leftmost head. This method is used
         to determine which definition must be asked for rules
@@ -398,7 +398,7 @@ def is_free(self, form, evaluation) -> bool:
     def is_inexact(self) -> bool:
         return self.get_precision() is not None
 
-    def sameQ(self, rhs) -> bool:
+    def sameQ(self, rhs: "BaseElement") -> bool:
         """Mathics SameQ"""
         return id(self) == id(rhs)
 

mathics/core/expression.py

@@ -5,7 +5,7 @@
 import time
 from bisect import bisect_left
 from itertools import chain
-from typing import Any, Callable, Iterable, List, Optional, Tuple, Type, Union
+from typing import Any, Callable, Iterable, Optional, Tuple, Type, Union
 
 import sympy
 
@@ -88,6 +88,73 @@
 )
 
 
+def eval_SameQ(self, other):
+    """
+    Iterative implementation of SameQ[].
+
+    Tree traversal comparison between `self` and `other`.
+    Return `True` if both tree structures are equal.
+
+    This non-recursive implementation reduces the Python stack needed
+    in evaluation. Staring in Python 3.12 there is a limit on the
+    recursion level.
+    """
+
+    len_elements = len(self.elements)
+    if len(other._elements) != len_elements:
+        return False
+
+    # Initializing a "stack"
+    parents = [
+        (
+            self,
+            other,
+        )
+    ]
+    current = (self._head, other._head)
+    pos = [0]
+
+    # The next element in the tree. Maybe should be an iterator?
+    def next_elem():
+        nonlocal len_elements
+        nonlocal parents
+        nonlocal pos
+
+        while pos and pos[-1] == len_elements:
+            pos.pop()
+            parents.pop()
+            assert len(pos) == len(parents)
+            if len(pos) > 0:
+                len_elements = len(parents[-1][0]._elements)
+                assert len(parents[-1][1]._elements) == len_elements
+
+        if len(pos) == 0:
+            return None
+
+        current = tuple(p._elements[pos[-1]] for p in parents[-1])
+        pos[-1] += 1
+        return current
+
+    while current:
+        if current[0] is current[1]:
+            current = next_elem()
+        elif all(isinstance(elem, Atom) for elem in current):
+            if not current[0].sameQ(current[1]):
+                return False
+            current = next_elem()
+        elif all(isinstance(elem, Expression) for elem in current):
+            len_elements = len(current[0]._elements)
+            if len_elements != len(current[1]._elements):
+                return False
+            parents.append(current)
+            current = tuple((c._head for c in current))
+            pos.append(0)
+        else:  # Atom is not the same than an expression
+            return False
+
+    return True
+
+
 class BoxError(Exception):
     def __init__(self, box, form) -> None:
         super().__init__("Box %s cannot be formatted as %s" % (box, form))
@@ -163,14 +230,6 @@ def union(expressions, evaluation) -> Optional["ExpressionCache"]:
             ):
                 return None
 
-        # FIXME: this is workaround the current situtation that some
-        # Atoms, like String, have a cache even though they don't need
-        # it, by virtue of this getting set up in
-        # BaseElement.__init__. Removing the self._cache in there the
-        # causes Boxing to mess up. Untangle this mess.
-        if expr._cache is None:
-            return None
-
         symbols = set.union(*[expr._cache.symbols for expr in expressions])
 
         return ExpressionCache(
@@ -179,12 +238,12 @@ def union(expressions, evaluation) -> Optional["ExpressionCache"]:
 
 
 class Expression(BaseElement, NumericOperators, EvalMixin):
-    """
-    A Mathics3 M-Expression.
+    """A Mathics3 (compound) M-Expression.
 
-    A Mathics3 M-Expression is a list where the head is a function designator.
-    (In the more common S-Expression the head is an a Symbol. In Mathics this can be
-    an expression that acts as a function.
+    A Mathics3 M-Expression is a list where the head is a function
+    designator.  (In the more common S-Expression the head is an a
+    Symbol. In Mathics3, this can be an expression that acts as a
+    function.
 
     positional Arguments:
         - head -- The head of the M-Expression
@@ -195,10 +254,11 @@ class Expression(BaseElement, NumericOperators, EvalMixin):
 
     Keyword Arguments:
         - elements_properties -- properties of the collection of elements
+
     """
 
     _head: BaseElement
-    _elements: List[BaseElement]
+    _elements: Tuple[BaseElement]
     _sequences: Any
     _cache: Optional[ExpressionCache]
     elements_properties: Optional[ElementsProperties]
@@ -421,7 +481,7 @@ def elements(self, values: Iterable):
         self.elements_properties = None
 
     def equal2(self, rhs: Any) -> Optional[bool]:
-        """Mathics two-argument Equal (==)
+        """Mathics3 two-argument Equal (==)
         returns True if self and rhs are identical.
         """
         if self.sameQ(rhs):
@@ -691,6 +751,9 @@ def get_head_name(self):
         return self._head.name if isinstance(self._head, Symbol) else ""
 
     def get_lookup_name(self) -> str:
+        """
+        Returns symbol name of leftmost head.
+        """
         lookup_symbol = self._head
         while True:
             if isinstance(lookup_symbol, Symbol):
@@ -1069,7 +1132,7 @@ def rewrite_apply_eval_step(self, evaluation) -> Tuple["Expression", bool]:
         # used later, include: HoldFirst / HoldAll / HoldRest / HoldAllComplete.
 
         # Note: self._head can be not just a symbol, but some arbitrary expression.
-        # This is what makes expressions in Mathics be M-expressions rather than
+        # This is what makes expressions in Mathics3 be M-expressions rather than
         # S-expressions.
         head = self._head.evaluate(evaluation)
 
@@ -1376,19 +1439,14 @@ def round_to_float(
         return None
 
     def sameQ(self, other: BaseElement) -> bool:
-        """Mathics SameQ"""
+        """Mathics3 SameQ"""
         if not isinstance(other, Expression):
             return False
         if self is other:
             return True
-        if not self._head.sameQ(other._head):
-            return False
-        if len(self._elements) != len(other._elements):
-            return False
-        return all(
-            (id(element) == id(oelement) or element.sameQ(oelement))
-            for element, oelement in zip(self._elements, other._elements)
-        )
+
+        # All this stuff maybe should be in mathics.eval.expression
+        return eval_SameQ(self, other)
 
     def sequences(self):
         cache = self._cache
@@ -1492,7 +1550,7 @@ def to_python(self, *args, **kwargs):
             #         )
             return py_obj
 
-        # Notice that in this case, `to_python` returns a Mathics Expression object,
+        # Notice that in this case, `to_python` returns a Mathics3 Expression object,
         # instead of a builtin native object.
         return self