Skip to content

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

Open
livingstaccato wants to merge 3 commits into
amplify-education:mainfrom
livingstaccato:fix/force-parens-nested
Open

fix: force_operation_parentheses works under a parenthesised ancestor (#342)#348
livingstaccato wants to merge 3 commits into
amplify-education:mainfrom
livingstaccato:fix/force-parens-nested

Conversation

@livingstaccato

@livingstaccato livingstaccato commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes #342.

What

force_operation_parentheses exists to make precedence explicit, and it did so for a top-level expression. Inside one the caller had already parenthesised, it added nothing:

options = SerializationOptions(force_operation_parentheses=True)

loads("a = b + c * d\n",   serialization_options=options)["a"]  # '${b + (c * d)}'
loads("a = (b + c * d)\n", serialization_options=options)["a"]  # '${(b + c * d)}'  ← unchanged

So the documents most likely to want explicit precedence — the ones already carrying parentheses — got the least of it.

Why

inside_parentheses answers "did my immediate container already wrap me". _wrap_into_parentheses reads it to avoid doubling. Two places widened it to "some ancestor is parenthesised":

  • ExprTermRule.serialize carried it down as self.parentheses or context.inside_parentheses, so one parenthesised term set it for its whole subtree;
  • the binary, unary and conditional rules passed it unchanged 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.

What does not change

((b + c) * d) is not doubled, (b + c) * d keeps its single pair, and the option-less path emits exactly what it did before — asserted for six shapes, since that is the half most easily broken by a change like this.

With b=2, c=3, d=4, OpenTofu v1.12.5 evaluates both (b + c * d) and the (b + (c * d)) this produces to 14.

Merging

It touches the same code as #334 (hcl2/rules/expressions.py). Whichever of those lands first, this one needs a rebase rather than a merge — the overlaps are real edits to the same methods, not adjacent lines, so resolving them by hand risks losing one of the two fixes. Say the word and I will rebase and re-run the suite.


This pull request, and the investigation behind it, were produced by an AI assistant (Claude) working on behalf of the author. Please review with that provenance in mind.

…amplify-education#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.
@livingstaccato

livingstaccato commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Please hold off on merging this one for now — I want to do another review pass over it before it goes in. Opened as a draft for that reason; I will mark it ready and say so here once I am done.

@livingstaccato

livingstaccato commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Review pass done, so the hold above no longer applies — this is ready for review now.

Rebased on current main; GitHub reports it mergeable as it stands.

🤖 Drafted with Claude Code.

kkozik-amplify and others added 2 commits September 7, 2026 14:22
…n#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) <noreply@anthropic.com>
@livingstaccato

Copy link
Copy Markdown
Contributor Author

Brought up to date with current main (0f74596) — GitHub had this flagged as
conflicting, which was only CHANGELOG.md and the merge=union driver that the
web UI does not honour. This one conflicts with #334, which removes the context.modify() these
rules call (hcl2/rules/expressions.py), rather than with main.

Merge order. Eight of the twelve open PRs apply to main in sequence with no
conflict at all — verified by merging each and running the suite, not by
inspection:

#332 → #334 → #335 → #345 → #346 → #349 → #352 → #354

(1552 → 1563 → 1591 → 1627 → 1671 → 1696 → 1703 → 1711 passing, from a 1534
baseline; each adds its own tests.) The other four — #333, #348, #350, #351
conflict with siblings rather than with main, so no ordering avoids them.

A resolved integration of all twelve is on the fork if it is useful:
livingstaccato/python-hcl2:int/pyvider-hcl-9 — 1784 passing, ruff and mypy
clean. Not proposed as a PR; the twelve are more reviewable apart.

Drafted with the help of an LLM, working on behalf of the author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

force_operation_parentheses adds no parentheses inside an already-parenthesized expression

2 participants