fix: force_operation_parentheses works under a parenthesised ancestor (#342)

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ### Fixed
 
+- `force_operation_parentheses` now adds parentheses inside an expression that is already parenthesised. The operation rules handed `inside_parentheses` — which means "my container already wrapped me", and stops the option doubling parentheses — down to their operands, which nothing wraps, so a single pair anywhere above an operation silenced the option for everything below it: `(b + c * d)` came back unchanged. It now reaches through parentheses, function calls, indexes and for-expressions alike. The option-less path is unaffected. Thanks, @livingstaccato ([#348](https://github.com/amplify-education/python-hcl2/pull/348))
 - Parse blocks whose type or unquoted label is an HCL keyword, such as the `in` block of the Snowflake provider's `snowflake_schemas` data source. HCL does not reserve its keywords, so `if`, `in`, `for`, `for_each`, `else`, `endif`, `endfor`, `true`, `false`, and `null` are now accepted in every block label position and normalized to identifiers — matching the existing behaviour for keyword attribute names. The block-side grammar gap was diagnosed independently in [#355](https://github.com/amplify-education/python-hcl2/pull/355). ([#357](https://github.com/amplify-education/python-hcl2/pull/357))
 - Parse keyword-named *object* keys reliably, fixing a regression of [#148](https://github.com/amplify-education/python-hcl2/issues/148). `object_elem_key` did not accept the keyword terminals, so a key such as `in` parsed only in states where the contextual lexer happened to fall back to `NAME` — which made the key's separator and position decide whether the file parsed. The comma-separated `{ name = "n", in = "header" }` parsed, but the newline-separated form the original report actually used did not, so its `jsonencode` OpenAPI body still raised. Keys such as `for` failed in every position. ([#357](https://github.com/amplify-education/python-hcl2/pull/357))
 

hcl2/rules/expressions.py

@@ -100,7 +100,13 @@ def expression(self) -> ExpressionRule:
 
     def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any:
         """Serialize, handling parenthesized expression wrapping."""
-        with context.modify(inside_parentheses=self.parentheses or context.inside_parentheses):
+        # Not `or context.inside_parentheses`: the flag answers "did my
+        # immediate parent already wrap me", which `_wrap_into_parentheses`
+        # reads to avoid doubling them, and `or` made it mean "some ancestor
+        # is parenthesised". Clearing it in the operation rules is what fixes
+        # the output; this keeps the flag matching its meaning at the source,
+        # so a term that is not itself wrapped never claims to be.
+        with context.modify(inside_parentheses=self.parentheses):
             result = self.expression.serialize(options, context)
 
         if self.parentheses:
@@ -152,7 +158,12 @@ def if_false(self) -> ExpressionRule:
 
     def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any:
         """Serialize to ternary expression string."""
-        with context.modify(inside_dollar_string=True):
+        # `inside_parentheses=False`: nothing wraps an operand, so whatever
+        # wrapped this operation says nothing about them. Leaving it set is
+        # what stopped `force_operation_parentheses` reaching inside `(...)`.
+        # The check after the block still reads the outer value, which is the
+        # one that says whether *this* result is already wrapped.
+        with context.modify(inside_dollar_string=True, inside_parentheses=False):
             result = (
                 f"{self.condition.serialize(options, context)} "
                 f"? {self.if_true.serialize(options, context)} "
@@ -266,7 +277,7 @@ def absorbed_comments(self):
 
     def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any:
         """Serialize to 'lhs operator rhs' string."""
-        with context.modify(inside_dollar_string=True):
+        with context.modify(inside_dollar_string=True, inside_parentheses=False):
             lhs = self.expr_term.serialize(options, context)
             operator = str(self.binary_term.binary_operator.serialize(options, context)).strip()
             rhs = self.binary_term.expr_term.serialize(options, context)
@@ -303,7 +314,13 @@ def expr_term(self):
 
     def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any:
         """Serialize to 'operator operand' string."""
-        with context.modify(inside_dollar_string=True):
+        # Clears the flag for the same reason ConditionalRule does. No input
+        # reaches it here -- a unary operand is an `expr_term`, so an operation
+        # inside one either carries its own parentheses or sits under a
+        # container that clears the flag itself -- but the rule that an
+        # operation never hands `inside_parentheses` to its operands should
+        # hold for all three operation rules rather than two of them.
+        with context.modify(inside_dollar_string=True, inside_parentheses=False):
             operator = self.operator.rstrip()
             operand = self.expr_term.serialize(options, context)
             result = f"{operator}{operand}"

test/unit/rules/test_force_parentheses.py

@@ -0,0 +1,216 @@
+# pylint: disable=C0103,C0114,C0115,C0116
+"""`force_operation_parentheses` under a parenthesised ancestor (GH #342).
+
+The option exists to make precedence explicit, and it did so for a top-level
+expression. Inside one the caller had already parenthesised, it added nothing:
+`(b + c * d)` came back unchanged, so the very documents most likely to want
+explicit precedence got the least of it.
+
+`inside_parentheses` answers "did my immediate container already wrap me",
+which `_wrap_into_parentheses` reads to avoid doubling them. Two places made
+it mean "some ancestor is parenthesised" instead -- `ExprTermRule` carried it
+down with `or`, and the operation rules passed it to their operands, which are
+never directly wrapped by anything.
+
+Of those two, only the operation rules change any output: clearing the flag
+for their operands is what reaches every shape below. The `ExprTermRule` edit
+restores the flag's stated meaning at its source and emits the same text
+either way, so it is covered by a rule-level test rather than an output one.
+
+The parentheses this adds group what precedence already grouped. Checked
+against Terraform v1.11.4 with b=2, c=3, d=4, e=5, f=6, g=7, v=9, w=8, y=10,
+z=11 and x=true: all 28 rewritten expressions evaluate to the same value as
+the source they came from, `(b + c * d)` and `(b + (c * d))` both being 14.
+"""
+
+from unittest import TestCase
+
+from hcl2.api import loads
+from hcl2.rules.expressions import ExpressionRule, ExprTermRule
+from hcl2.rules.tokens import LPAR, RPAR
+from hcl2.utils import SerializationContext, SerializationOptions
+
+FORCED = SerializationOptions(force_operation_parentheses=True)
+DEFAULT = SerializationOptions()
+
+# Every expression the tests below exercise, in source form. The default path
+# has to return each one exactly as written; see TestTheDefaultIsUntouched.
+SOURCES = (
+    "b + c * d",
+    "(b + c * d)",
+    "((b + c) * d)",
+    "(b + c) * d",
+    "-b + c",
+    "x ? y + z : w",
+    "b + c * d + e",
+    "b * c + d * e",
+    "(b + c) * (d + e)",
+    "((b + c * d))",
+    "b + (c * (d + e))",
+    "-(b + c * d)",
+    "!(b && c || d)",
+    "(x ? y + z * w : v)",
+    "(f(b + c * d)) * d",
+    "(b[c + d * e])",
+    "((b + c * d) + e)",
+    "(-(b + c * d))",
+    "(b + c * d) + (e + f * g)",
+    "(b + c * d)[0]",
+    "b[c + d * e]",
+    "f(b + c * d)",
+    "[for i in l : i + j * k]",
+    "{for k, v in m : k => v + w * x}",
+)
+
+
+class ForcedParenthesesTestCase(TestCase):
+    def forced(self, source: str) -> str:
+        return loads(f"a = {source}\n", serialization_options=FORCED)["a"]
+
+
+class TestForcedParentheses(ForcedParenthesesTestCase):
+    def test_a_top_level_operation_is_unchanged(self):
+        self.assertEqual(self.forced("b + c * d"), "${b + (c * d)}")
+
+    def test_a_parenthesised_ancestor_no_longer_suppresses_it(self):
+        self.assertEqual(self.forced("(b + c * d)"), "${(b + (c * d))}")
+
+    def test_parentheses_already_there_are_not_doubled(self):
+        self.assertEqual(self.forced("((b + c) * d)"), "${((b + c) * d)}")
+        self.assertEqual(self.forced("(b + c) * d"), "${(b + c) * d}")
+
+    def test_a_unary_operand_is_wrapped(self):
+        self.assertEqual(self.forced("-b + c"), "${(-b) + c}")
+
+    def test_a_conditional_branch_is_wrapped(self):
+        self.assertEqual(self.forced("x ? y + z : w"), "${x ? (y + z) : w}")
+
+
+class TestEachOperationClearsTheFlagForItsOperands(ForcedParenthesesTestCase):
+    """One case per rule that stopped handing `inside_parentheses` down.
+
+    Dropping the `inside_parentheses=False` argument from `BinaryOpRule` or
+    `ConditionalRule` fails a test here: the enclosing parentheses go back to
+    suppressing the option for the whole subtree, which is #342.
+
+    `UnaryOpRule` carries the same argument and no input reaches it, because a
+    unary operand is an `expr_term` -- an operation there either carries its
+    own parentheses, which set the flag anyway, or sits inside a container
+    whose own operation rule clears it. It is kept so the rule that an
+    operation never hands the flag to its operands holds for all three rather
+    than two, and noted here so its lack of a failing test is not mistaken for
+    an oversight.
+    """
+
+    def test_a_binary_operation_under_parentheses(self):
+        self.assertEqual(self.forced("(b + c * d)"), "${(b + (c * d))}")
+
+    def test_a_unary_operation_under_parentheses(self):
+        self.assertEqual(self.forced("-(b + c * d)"), "${-(b + (c * d))}")
+        self.assertEqual(self.forced("!(b && c || d)"), "${!((b && c) || d)}")
+
+    def test_a_conditional_under_parentheses(self):
+        self.assertEqual(self.forced("(x ? y + z * w : v)"), "${(x ? (y + (z * w)) : v)}")
+
+
+class TestItReachesThroughEveryContainer(ForcedParenthesesTestCase):
+    """Parentheses anywhere above an operation no longer silence the option."""
+
+    def test_through_a_function_call(self):
+        self.assertEqual(self.forced("(f(b + c * d)) * d"), "${(f(b + (c * d))) * d}")
+
+    def test_through_an_index(self):
+        self.assertEqual(self.forced("(b[c + d * e])"), "${(b[c + (d * e)])}")
+
+    def test_through_a_second_pair_of_parentheses(self):
+        self.assertEqual(self.forced("((b + c * d) + e)"), "${((b + (c * d)) + e)}")
+        self.assertEqual(self.forced("((b + c * d))"), "${((b + (c * d)))}")
+
+    def test_through_a_unary_operator_and_parentheses(self):
+        self.assertEqual(self.forced("(-(b + c * d))"), "${(-(b + (c * d)))}")
+
+    def test_both_sides_of_an_operation(self):
+        self.assertEqual(
+            self.forced("(b + c * d) + (e + f * g)"),
+            "${(b + (c * d)) + (e + (f * g))}",
+        )
+
+    def test_a_parenthesised_operation_that_is_then_indexed(self):
+        self.assertEqual(self.forced("(b + c * d)[0]"), "${(b + (c * d))[0]}")
+
+    def test_unparenthesised_containers_still_work(self):
+        self.assertEqual(self.forced("b[c + d * e]"), "${b[c + (d * e)]}")
+        self.assertEqual(self.forced("f(b + c * d)"), "${f(b + (c * d))}")
+        self.assertEqual(self.forced("[for i in l : i + j * k]"), "${[for i in l : (i + (j * k))]}")
+
+
+class TestTheExprTermFlagReflectsItsOwnParentheses(TestCase):
+    """`ExprTermRule` sets the flag from `self.parentheses`, not its ancestors.
+
+    This is the half of the fix that changes no output -- the operation rules
+    already clear the flag on the way down, so nothing observable depends on
+    it. It is asserted here so the `or context.inside_parentheses` it replaced
+    cannot come back unnoticed: with that back, an unparenthesised term
+    inherits `True` and the flag stops meaning what its docstring says.
+    """
+
+    class RecordingExpression(ExpressionRule):
+        """Serializes to a fixed string, remembering the context it was given."""
+
+        def __init__(self):
+            self.seen = None
+            super().__init__([], None)
+
+        def serialize(self, options=SerializationOptions(), context=SerializationContext()):
+            self.seen = context.inside_parentheses
+            return "x"
+
+    def child_sees(self, *, parenthesised: bool, ancestor_parenthesised: bool) -> bool:
+        child = self.RecordingExpression()
+        children = [LPAR(), child, RPAR()] if parenthesised else [child]
+        ExprTermRule(children).serialize(
+            SerializationOptions(),
+            SerializationContext(inside_parentheses=ancestor_parenthesised),
+        )
+        return child.seen
+
+    def test_a_parenthesised_term_tells_its_child_so(self):
+        self.assertTrue(self.child_sees(parenthesised=True, ancestor_parenthesised=False))
+
+    def test_an_unparenthesised_term_does_not(self):
+        self.assertFalse(self.child_sees(parenthesised=False, ancestor_parenthesised=False))
+
+    def test_an_ancestors_parentheses_are_not_inherited(self):
+        self.assertFalse(self.child_sees(parenthesised=False, ancestor_parenthesised=True))
+
+    def test_a_terms_own_parentheses_still_win(self):
+        self.assertTrue(self.child_sees(parenthesised=True, ancestor_parenthesised=True))
+
+
+class TestTheDefaultIsUntouched(TestCase):
+    """Nothing above changes what the option-less path emits."""
+
+    def test_sources_come_back_as_written(self):
+        for source in SOURCES:
+            with self.subTest(source=source):
+                self.assertEqual(
+                    loads(f"a = {source}\n", serialization_options=DEFAULT)["a"],
+                    f"${{{source}}}",
+                )
+
+
+class TestTheMeaningIsPreserved(ForcedParenthesesTestCase):
+    """The added parentheses group what precedence already grouped."""
+
+    def test_the_forced_form_parses_back_to_the_same_expression(self):
+        forced = self.forced("(b + c * d)")
+        reparsed = loads(f"a = {forced[2:-1]}\n", serialization_options=DEFAULT)["a"]
+        self.assertEqual(reparsed, "${(b + (c * d))}")
+
+    def test_forcing_twice_adds_nothing_further(self):
+        # The rewritten form is already explicit, so running it back through
+        # the option has to be a fixed point rather than growing a pair a run.
+        for source in SOURCES:
+            with self.subTest(source=source):
+                once = self.forced(source)
+                self.assertEqual(self.forced(once[2:-1]), once)