Fixed operator equal-precedence bug

packtype/common/expression.py

@@ -30,26 +30,17 @@ class Expression:
     OP_GE = ">="
 
     # Matched to: https://docs.python.org/3/reference/expressions.html#operator-precedence
+    # The groups at each tier cover equal precedence (where order is LR parsed)
     OP_PRECEDENCE = [
-        OP_POW,
-        OP_INV,
-        OP_MUL,
-        OP_TRUEDIV,
-        OP_FLOORDIV,
-        OP_MOD,
-        OP_ADD,
-        OP_SUB,
-        OP_LSHIFT,
-        OP_RSHIFT,
-        OP_AND,
-        OP_XOR,
-        OP_OR,
-        OP_LT,
-        OP_LE,
-        OP_GT,
-        OP_GE,
-        OP_NE,
-        OP_EQ,
+        [OP_POW],
+        [OP_INV],
+        [OP_MUL, OP_TRUEDIV, OP_FLOORDIV, OP_MOD],
+        [OP_ADD, OP_SUB],
+        [OP_LSHIFT, OP_RSHIFT],
+        [OP_AND],
+        [OP_XOR],
+        [OP_OR],
+        [OP_LT, OP_LE, OP_GT, OP_GE, OP_NE, OP_EQ],
     ]
 
     def __init__(
@@ -121,18 +112,21 @@ def digest(cls, expr: list[str | int | float | Self]) -> Self:
         """
         # Take a copy so as not to mutate the original
         expr = list(expr)
-        # Search for each operator in precedence order
-        for search_op in cls.OP_PRECEDENCE:
+        # Reduce one precedence tier at a time; within a tier, collapse co-equal
+        # operators left-to-right so associativity matches Python's.
+        for op_group in cls.OP_PRECEDENCE:
             # If only a single term remains, break out early
             if len(expr) == 1:
                 break
-            # Look for every position an operator could exist (every other term)
+            # Look for every position an operator from this tier could exist (every other term)
             offset = 0
-            for op_pos in [x for x in range(1, len(expr), 2) if expr[x] == search_op]:
-                # Replace the term with a Expression instance
-                *before, lhs = expr[: op_pos + offset]
-                rhs, *after = expr[op_pos + offset + 1 :]
-                expr = [*before, cls.operate(lhs, search_op, rhs), *after]
+            for op_pos in [x for x in range(1, len(expr), 2) if expr[x] in op_group]:
+                # Replace the term with a Expression instance, using the actual
+                # operator at this (offset-adjusted) position within the tier.
+                pos = op_pos + offset
+                *before, lhs = expr[:pos]
+                rhs, *after = expr[pos + 1 :]
+                expr = [*before, cls.operate(lhs, expr[pos], rhs), *after]
                 offset -= 2
         # Ensure that even a single term is returned as a Expression instance
         expr = expr[0]

tests/grammar/test_constant.py

@@ -218,3 +218,32 @@ def test_parse_constant_expression():
         )
     )
     assert int(pkg.F) == (32 * 9) ** 2 // -4 + 43
+
+
+def test_parse_constant_operator_associativity():
+    """Co-equal operators (+/-, * / // %, << >>) must evaluate left-to-right.
+
+    Regression for digest() collapsing a higher-listed operator before a
+    co-equal one (e.g. `a - b + c` -> `a - (b + c)`), which made mixed
+    subtract/add width expressions evaluate to the wrong (often negative) value.
+    """
+    pkg = next(
+        parse_string(
+            """
+        package the_package {
+            SUB_THEN_ADD : constant = 14 - 6 - 8 + 6 - 2 - 1
+            ADD_THEN_SUB : constant = 2 + 3 - 4 + 5
+            DIV_THEN_MUL : constant = 20 / 2 * 5
+            MUL_BINDS    : constant = 100 - 10 * 3
+            MUL_THEN_ADD : constant = 1 + 2 * 3 - 4
+            SHIFTS       : constant = 64 >> 2 << 1
+        }
+        """
+        )
+    )
+    assert pkg.SUB_THEN_ADD.value == 14 - 6 - 8 + 6 - 2 - 1 == 3
+    assert pkg.ADD_THEN_SUB.value == 2 + 3 - 4 + 5 == 6
+    assert pkg.DIV_THEN_MUL.value == 20 / 2 * 5 == 50
+    assert pkg.MUL_BINDS.value == 100 - 10 * 3 == 70
+    assert pkg.MUL_THEN_ADD.value == 1 + 2 * 3 - 4 == 3
+    assert pkg.SHIFTS.value == 64 >> 2 << 1 == 32