Skip to content

fix: two rules that ignored they were writing expression source (#340, #341) - #350

Open
livingstaccato wants to merge 3 commits into
amplify-education:mainfrom
livingstaccato:fix/expression-source-rules
Open

fix: two rules that ignored they were writing expression source (#340, #341)#350
livingstaccato wants to merge 3 commits into
amplify-education:mainfrom
livingstaccato:fix/expression-source-rules

Conversation

@livingstaccato

@livingstaccato livingstaccato commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes #340.
Fixes #341.

What

SerializationContext.inside_dollar_string tells a rule that the text it is producing is part of an expression rather than a value handed to the caller. StringRule checks it and keeps its quotes, for the obvious reason: upper("x") becoming upper(x) asks for a variable nobody declared. Two rules did not check it.

#340 — a heredoc argument was spliced in bare.

loads("a = upper(<<E\nx\nE\n)\n", serialization_options=value)["a"]
# '${upper(x)}'   ← a reference, not a string

The quoted equivalent one line away was already correct (${upper("x")}). With a multi-line body it was worse: raw newlines went into expression source that then did not parse.

#341 — a string literal inside a directive lost its delimiters.

loads('a = "%{ if x == \\"y\\" }t%{ endif }"\n', serialization_options=value)["a"]
# '%{ if x == y }t%{ endif }'   ← compares against a variable

TemplateStringRule only ever appears inside %{ ... }, where the text is expression source and the quotes belong to a literal written in it.

One thing that changes beyond the issues

With preserve_heredocs on, a heredoc inside an expression is now left as itself rather than wrapped in quotes. Quoting it put raw newlines inside a quoted string, which OpenTofu rejects with "Invalid multi-line string"; as a heredoc it is a legal argument, and trimspace(<<EOF\n hi \nEOF\n) evaluates to "hi". One existing test pinned the quoted form and now states this, with the reason.

Ground truth

OpenTofu v1.12.5: upper(<<EOT\nx\nEOT\n) evaluates to "X\n", so the argument is a string; a directive literal is written with plain quotes, "%{ if local.x == "y" }t%{ endif }", and evaluates to "t". That plain spelling round-trips here unchanged, and is asserted so it stays that way.

One divergence this does not touch: the grammar accepts \"y\" inside a directive, which OpenTofu rejects with "Invalid character". That is a separate defect, filed as #353. This PR only stops the value form from mangling that spelling into a reference; it neither blesses nor removes it.

Merging

It touches the same code as #335 (hcl2/rules/strings.py), #346 (hcl2/rules/strings.py), #352 (hcl2/rules/strings.py), #354 (hcl2/rules/strings.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.

…ify-education#340, amplify-education#341)

`inside_dollar_string` says the text being produced is part of an
expression rather than a value for the caller. `StringRule` checks it
and keeps its quotes, because `upper("x")` becoming `upper(x)` asks for
a variable nobody declared. Two rules did not.

A heredoc argument was spliced in bare: `upper(<<E\nx\nE\n)` came back
as `${upper(x)}`, and a multi-line body put raw newlines into source
that no longer parsed. Both heredoc rules check the flag now, in the
flattened and the trimmed paths.

With `preserve_heredocs` on, the heredoc is left as itself rather than
wrapped in quotes. That was quoting raw newlines, which OpenTofu rejects
with "Invalid multi-line string"; as a heredoc it is a legal argument
and `trimspace(<<EOF\n  hi  \nEOF\n)` evaluates to "hi". One existing
test pinned the quoted form and now states this.

`TemplateStringRule` dropped its delimiters in the value form, turning
`%{ if x == "y" }` into `%{ if x == y }` -- a comparison against a
variable rather than a string. It only ever appears inside a directive,
which is always expression source.
@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:31
…-education#340, amplify-education#341)

The branch also stopped quoting a heredoc that sits inside an expression on
the default path, which neither amplify-education#340 nor amplify-education#341 asks for -- both are scoped to
`strip_string_quotes`. It regressed the round trip: `dumps(loads(...))` of a
heredoc argument raised `UnexpectedToken`, because the emitted
`trimspace(<<EOF\nhi\nEOF)` puts the closing marker on the same line as the
`)`, which this grammar does not accept. On main that round trip works.

The change is right in the end -- Terraform v1.11.4 rejects the quoted form
with "Invalid multi-line string" and evaluates the heredoc argument to "hi" --
but it needs the writer to give a heredoc its own line first, which is amplify-education#338.
Landing it here breaks `dumps` for anyone whose document has a heredoc in a
call. Reverted, with `test_api.py` back to the expectation it had and a note
saying which fix supersedes it, so amplify-education#338 makes the change deliberately.

The two fixes the issues do ask for are untouched, and are now covered per
rule: reverting `HeredocTemplateRule`'s check fails twelve assertions,
`HeredocTrimTemplateRule`'s one, `TemplateStringRule`'s two.

Also widens amplify-education#340's coverage past the reported function argument to every rule
that marks its children as expression source -- nested call, later argument,
binary operand, conditional branch, indexed tuple, interpolation -- and adds
round-trip assertions for both the default and value dicts, which is the half
that a change here can break without any of the value assertions noticing.

Verified against Terraform v1.11.4: `upper(<<EOT\nx\nEOT\n)` is "X\n", and
`"%{ if local.x == "y" }t%{ endif }"` is "t".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
livingstaccato added a commit to livingstaccato/python-hcl2 that referenced this pull request Sep 7, 2026
# Conflicts:
#	hcl2/rules/strings.py
livingstaccato added a commit to livingstaccato/python-hcl2 that referenced this pull request Sep 7, 2026
Combines the expression-source guard with the heredoc body values amplify-education#335
returns: a body carries the newline ending its last content line, so the
expectations for a heredoc used as expression source carry it too.
@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 #335, #346, #352 and #354 (hcl2/rules/strings.py)
rather than with main; the second note is about this PR's expectations.

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.

#335 changes a value two of the others assert on. It makes a heredoc body
carry the newline that ends its last content line, matching what OpenTofu
evaluates:

from hcl2.utils import SerializationOptions
opts = SerializationOptions(strip_string_quotes=True, preserve_heredocs=False)
hcl2.loads("a = <<E\nx\nE\n", serialization_options=opts)["a"]

main alone:   'x'
main + #335:  'x\n'

#350 and #351 were written before that. Once #335 is in, their expectations need
the newline — in an integration carrying all twelve, every one of the 16
resulting failures was exactly that one character and nothing else. Worth knowing
because it presents as a regression and is not one.

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

2 participants