From 32b3fc5fff98e519449100ec248020c41f4457d2 Mon Sep 17 00:00:00 2001 From: Tim Date: Tue, 1 Sep 2026 21:56:21 -0700 Subject: [PATCH] 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))}")