diff --git a/packtype/common/expression.py b/packtype/common/expression.py index fef7bd4..2d24344 100644 --- a/packtype/common/expression.py +++ b/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] diff --git a/tests/grammar/test_constant.py b/tests/grammar/test_constant.py index 0cb1537..b30f4c8 100644 --- a/tests/grammar/test_constant.py +++ b/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