diff --git a/CHANGELOG.md b/CHANGELOG.md index 87ceca04..efb897f2 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/hcl2/rules/expressions.py b/hcl2/rules/expressions.py index 15caa1c3..342f9308 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, 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}" diff --git a/test/unit/rules/test_force_parentheses.py b/test/unit/rules/test_force_parentheses.py new file mode 100644 index 00000000..eda652bf --- /dev/null +++ b/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)