From 32b3fc5fff98e519449100ec248020c41f4457d2 Mon Sep 17 00:00:00 2001 From: Tim Date: Tue, 1 Sep 2026 21:56:21 -0700 Subject: [PATCH 1/2] fix: force_operation_parentheses works under a parenthesised ancestor (#342) The option makes precedence explicit, and 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 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 widened it to "some ancestor is parenthesised": `ExprTermRule` carried the flag down with `or`, and the binary, unary and conditional rules passed it to their operands, which nothing directly wraps. Each term now sets the flag from its own `self.parentheses`, and the operation rules clear it for their operands. `((b + c) * d)` is still not doubled, and the option-less path emits exactly what it did before. With b=2, c=3, d=4, OpenTofu evaluates both `(b + c * d)` and the `(b + (c * d))` this now produces to 14. --- CHANGELOG.md | 4 +- hcl2/rules/expressions.py | 14 +++-- test/unit/rules/test_force_parentheses.py | 75 +++++++++++++++++++++++ 3 files changed, 88 insertions(+), 5 deletions(-) create mode 100644 test/unit/rules/test_force_parentheses.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 61dc143a..c1ea1ea9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## \[Unreleased\] -- Nothing yet. +### Fixed + +- `force_operation_parentheses` adds parentheses inside a parenthesised expression again. `inside_parentheses` answers "did my immediate container already wrap me", which is what stops the option doubling them, but two places made it mean "some ancestor is parenthesised": `ExprTermRule` carried it down with `or`, and the operation rules passed it to their operands, which nothing directly wraps. `(b + c * d)` therefore came back unchanged, so the documents most likely to want explicit precedence got the least of it. ([#342](https://github.com/amplify-education/python-hcl2/issues/342)) ## \[8.1.3\] - 2026-08-26 diff --git a/hcl2/rules/expressions.py b/hcl2/rules/expressions.py index 15caa1c3..7950234e 100644 --- a/hcl2/rules/expressions.py +++ b/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. Carrying it down made it mean "some + # ancestor is parenthesised", so `force_operation_parentheses` stopped + # adding any inside `(b + c * d)`. Each inner term sets it from its own + # `self.parentheses`, so a genuinely wrapped one still says so. + with context.modify(inside_parentheses=self.parentheses): result = self.expression.serialize(options, context) if self.parentheses: @@ -152,7 +158,7 @@ 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): + 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 +272,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 +309,7 @@ def expr_term(self): def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: """Serialize to 'operator operand' string.""" - with context.modify(inside_dollar_string=True): + 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}" diff --git a/test/unit/rules/test_force_parentheses.py b/test/unit/rules/test_force_parentheses.py new file mode 100644 index 00000000..e0bf13da --- /dev/null +++ b/test/unit/rules/test_force_parentheses.py @@ -0,0 +1,75 @@ +# 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. +""" + +from unittest import TestCase + +from hcl2.api import loads +from hcl2.utils import SerializationOptions + +FORCED = SerializationOptions(force_operation_parentheses=True) +DEFAULT = SerializationOptions() + + +class TestForcedParentheses(TestCase): + def _forced(self, source: str) -> str: + return loads(f"a = {source}\n", serialization_options=FORCED)["a"] + + 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 TestTheDefaultIsUntouched(TestCase): + """Nothing above changes what the option-less path emits.""" + + def test_sources_come_back_as_written(self): + for source in ( + "b + c * d", + "(b + c * d)", + "((b + c) * d)", + "(b + c) * d", + "-b + c", + "x ? y + z : w", + ): + with self.subTest(source=source): + self.assertEqual( + loads(f"a = {source}\n", serialization_options=DEFAULT)["a"], + f"${{{source}}}", + ) + + +class TestTheMeaningIsPreserved(TestCase): + """The added parentheses group what precedence already grouped. + + Checked with OpenTofu v1.12.5: with b=2, c=3, d=4, both + `(b + c * d)` and `(b + (c * d))` evaluate to 14. + """ + + def test_the_forced_form_parses_back_to_the_same_expression(self): + forced = loads("a = (b + c * d)\n", serialization_options=FORCED)["a"] + reparsed = loads(f"a = {forced[2:-1]}\n", serialization_options=DEFAULT)["a"] + self.assertEqual(reparsed, "${(b + (c * d))}") From 17c95b54ed4b05650ee0d4c121d8ce37ef7457c3 Mon Sep 17 00:00:00 2001 From: Kamil Kozik Date: Mon, 7 Sep 2026 14:22:38 +0200 Subject: [PATCH 2/2] test: cover the operand-clearing that actually fixes #342 Of the four lines the fix touches, only one had a test that failed without it. `test_a_unary_operand_is_wrapped` and `test_a_conditional_branch_is_wrapped` both pass on the unfixed tree -- neither expression has a parenthesised ancestor, so neither reaches the leak. Adds a case per operation rule that stopped handing `inside_parentheses` to its operands, and cases for the shapes the fix newly reaches: a function call, an index, a second pair of parentheses, both sides of an operation, and an indexed parenthesised operation. Reverting `BinaryOpRule`'s argument now fails eight assertions and `ConditionalRule`'s one. `ExprTermRule` emits the same text either way -- the operation rules clear the flag on the way down, so nothing observable depends on it -- so it is pinned by a rule-level test that a term reports its own parentheses rather than its ancestors'. `UnaryOpRule` has no reachable case at all; the comment and the test docstring say so, so its lack of a failing test is not mistaken for an oversight. Widens the default-path check from six sources to the full set, and adds a fixed-point check that forcing an already-forced expression adds nothing. Verified against Terraform v1.11.4: all 28 rewritten expressions evaluate to the same value as the source they came from. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 +- hcl2/rules/expressions.py | 19 ++- test/unit/rules/test_force_parentheses.py | 191 +++++++++++++++++++--- 3 files changed, 182 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1ea1ea9..c9fc4d81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed -- `force_operation_parentheses` adds parentheses inside a parenthesised expression again. `inside_parentheses` answers "did my immediate container already wrap me", which is what stops the option doubling them, but two places made it mean "some ancestor is parenthesised": `ExprTermRule` carried it down with `or`, and the operation rules passed it to their operands, which nothing directly wraps. `(b + c * d)` therefore came back unchanged, so the documents most likely to want explicit precedence got the least of it. ([#342](https://github.com/amplify-education/python-hcl2/issues/342)) +- `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)) ## \[8.1.3\] - 2026-08-26 diff --git a/hcl2/rules/expressions.py b/hcl2/rules/expressions.py index 7950234e..342f9308 100644 --- a/hcl2/rules/expressions.py +++ b/hcl2/rules/expressions.py @@ -102,10 +102,10 @@ def serialize(self, options=SerializationOptions(), context=SerializationContext """Serialize, handling parenthesized expression wrapping.""" # Not `or context.inside_parentheses`: the flag answers "did my # immediate parent already wrap me", which `_wrap_into_parentheses` - # reads to avoid doubling them. Carrying it down made it mean "some - # ancestor is parenthesised", so `force_operation_parentheses` stopped - # adding any inside `(b + c * d)`. Each inner term sets it from its own - # `self.parentheses`, so a genuinely wrapped one still says so. + # 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) @@ -158,6 +158,11 @@ def if_false(self) -> ExpressionRule: def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: """Serialize to ternary expression string.""" + # `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)} " @@ -309,6 +314,12 @@ def expr_term(self): def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: """Serialize to 'operator operand' string.""" + # 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) diff --git a/test/unit/rules/test_force_parentheses.py b/test/unit/rules/test_force_parentheses.py index e0bf13da..eda652bf 100644 --- a/test/unit/rules/test_force_parentheses.py +++ b/test/unit/rules/test_force_parentheses.py @@ -11,50 +11,187 @@ 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.utils import SerializationOptions +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() - -class TestForcedParentheses(TestCase): - def _forced(self, source: str) -> str: +# 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)}") + 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))}") + 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}") + 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}") + 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}") + 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 ( - "b + c * d", - "(b + c * d)", - "((b + c) * d)", - "(b + c) * d", - "-b + c", - "x ? y + z : w", - ): + for source in SOURCES: with self.subTest(source=source): self.assertEqual( loads(f"a = {source}\n", serialization_options=DEFAULT)["a"], @@ -62,14 +199,18 @@ def test_sources_come_back_as_written(self): ) -class TestTheMeaningIsPreserved(TestCase): - """The added parentheses group what precedence already grouped. - - Checked with OpenTofu v1.12.5: with b=2, c=3, d=4, both - `(b + c * d)` and `(b + (c * d))` evaluate to 14. - """ +class TestTheMeaningIsPreserved(ForcedParenthesesTestCase): + """The added parentheses group what precedence already grouped.""" def test_the_forced_form_parses_back_to_the_same_expression(self): - forced = loads("a = (b + c * d)\n", serialization_options=FORCED)["a"] + 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)