fix: decline to flatten a heredoc whose interpolation spans lines (#347) - #354
Draft
livingstaccato wants to merge 16 commits into
Draft
fix: decline to flatten a heredoc whose interpolation spans lines (#347)#354livingstaccato wants to merge 16 commits into
livingstaccato wants to merge 16 commits into
Conversation
Three things about a flattened heredoc body differed from the value
Terraform and OpenTofu evaluate the same source to. Every expectation
added here was produced by running the source through OpenTofu v1.12.5
rather than read off the spec.
- The newline terminating the last content line was dropped, so
`<<EOT\nline\nEOT` came back as "line" rather than "line\n". The spec
ends the template where the delimiter "subsequently appears again on a
line of its own", so every content line, the last included, is
terminated by its own newline. Only the closing marker's indentation
is not content, and that is still removed.
- `<<-` measured its indent with `lstrip(" ")`. A tab-indented body
measured zero on every line, so it was not dedented at all. The spec
says "spaces", but the reference implementation does not read it that
narrowly, and measuring whitespace characters is identical to counting
spaces on space-indented input.
- A whitespace-only line was correctly excluded from the measurement and
then trimmed anyway. OpenTofu leaves such a line as written: a
six-space line inside a four-space heredoc stays six spaces.
Fixing the read exposed the matching bug in the write. With
`strings_to_heredocs`, the emitter appended a newline before the closing
marker that the value already carried, so the body came out one line
longer. The two errors cancelled inside this library's own round trip
but not against Terraform: five of the eleven values in the round-trip
fixture changed when OpenTofu evaluated the restored file. They no
longer do. A value that does not end in a newline is now left as a
quoted string, since no heredoc can express it.
This is not a regression. 7.2.1 returns the same values as 8.1.3 on all
four inputs, so nothing here arrived with the v8 rewrite and no fix is
restoring anything -- it changes long-standing behaviour to match the
reference implementation.
…aform `test_heredoc_matches_terraform.py` asserts values that came from running each source through OpenTofu rather than from this library or from the spec. That provenance was a docstring: a reader had to take it on trust, and nothing re-checked it if the reference implementation moved. `bin/heredoc_ground_truth` reads the `CASES` table out of the test module, evaluates every source with `tofu console` (or `terraform console`), and reports any disagreement, exiting non-zero. `--print` emits the evaluated table as Python for pasting. It is not wired into the test run on purpose. The suite must pass without a Terraform binary present, and these values move about as often as the HCL spec does -- this is an audit tool for a reviewer who would rather check than trust, not a gate. Both paths are exercised: all 16 cases agree with OpenTofu v1.12.5, and feeding it the pre-fix value for a case makes it report the mismatch and exit 1.
`preserve_heredocs=False` without `strip_string_quotes` returns the body as quoted-string source -- the text a parser has to read back. Newlines were escaped for that; carriage returns were not. A heredoc from a CRLF file flattened to `"x<CR>\ny<CR>\n"`, which OpenTofu rejects with "No closing marker was found for the string", so the form documented as reconstructable was not. `\r` is an escape both this package's `process_escape_sequences` and OpenTofu resolve back to a carriage return, so the value survives the round trip unchanged. The trimmed form had the same gap and gets the same treatment. The value form keeps handing back real characters. Two existing CRLF tests asserted the raw-carriage-return output; they now assert the escaped source and say why.
Escaping carriage returns in the flattened form left the writer half a step behind: `_unescape_heredoc_body` resolved `\n`, `\"` and `\\` but not `\r`, so a heredoc read out of a CRLF file and written back came out holding a literal backslash and an `r`. A heredoc interprets no escape -- its body is the characters themselves -- so that is a different value, and OpenTofu reads it as one. The two halves have to be inverses. Flatten writes `\r` because a quoted string cannot hold a raw carriage return; the writer therefore has to resolve it, exactly as it already resolved `\n` for the same reason. Each half was covered on its own -- flattening a CRLF heredoc, restoring an LF string -- which is why the combination could break with the suite green. The new tests run the whole path: CRLF source, flatten, write, read the value back, against the string OpenTofu evaluates the original file to. Escapes other than these four are still not resolved when writing a heredoc, which is a separate pre-existing defect (amplify-education#329).
…ion#330) `strings_to_heredocs` wrote `<<EOF` over every value without looking at it. A string holding a line reading `EOF` therefore closed its own heredoc at that line, and everything after became stray tokens: the file this library had just written no longer parsed, here or in Terraform. The values people put in heredocs -- log excerpts, shell scripts, embedded configs -- are exactly the ones that contain the word. The delimiter is now chosen against the body: `EOF` when no line could end the heredoc there, a numbered variant otherwise, so ordinary output is byte-for-byte what it was. Which lines count is Terraform's rule rather than this grammar's, which is stricter. OpenTofu v1.12.5 ends a heredoc on `EOF ` and evaluates `<<EOF\nbody\nEOF \n` to `"body\n"`; `HEREDOC_TEMPLATE` here requires the newline to follow the word, and rejects that file outright. Choosing against the looser reading is what keeps the written file readable by both -- the stricter one would emit a body Terraform treats as closed.
`strings_to_heredocs` leaves a value that does not end in a newline quoted, and the comments said a heredoc body always ends in one. An empty heredoc does not: `<<EOF\nEOF` evaluates to "" in Terraform and here, so the empty string is a value the rule excludes that a heredoc could express. The behaviour is unchanged -- `x = ""` says it in one line rather than three -- but the reason stated was wrong, and someone reading it would have concluded the exclusion was forced.
The dedent measures whitespace rather than spaces and tabs, because that is what OpenTofu does -- it dedents a body indented with a non-breaking space, a vertical tab, a form feed or an ideographic space exactly as it dedents a space-indented one. The closing marker's own indentation was still stripped as `[ \t]*`, so those bodies came back with the marker's indent character appended to the value: `'a\nb\n\xa0'` where OpenTofu evaluates `'a\nb\n'`. It is now any whitespace but a newline, which is the same rule the dedent uses. Trailing spaces on a content line still survive, for the reason they always did: such a line ends with its own newline, and the match cannot cross one. The four cases are in `CASES`, so `bin/heredoc_ground_truth` re-derives them from Terraform along with the rest rather than trusting this reading of the spec. All 20 agree.
Two cases where writing one produced a file Terraform cannot read. A lone carriage return is not expressible. A heredoc body is read literally, so a `\r` may only appear where one ends a line: OpenTofu rejects `<<EOF\nx\ry\nEOF` with "No closing marker was found for the string", while the quoted `"x\ry\n"` it came from is valid and evaluates to that character. Resolving `\r` into the body therefore turned a wrong value into an unreadable file. Such a value now stays quoted, for the same reason a value that does not end in a newline does -- and the test that pinned the old output was asserting a file OpenTofu rejects, which passed only because this parser is more permissive than its scanner. The delimiter search was blind to CRLF. The body is split on `\n`, so a CRLF line hands back its own `\r`, and a marker check allowing only spaces and tabs never matched `EOF\r`. OpenTofu ends a heredoc there as readily as on `EOF `, so a CRLF body carrying the delimiter was written under `<<EOF` and closed at its own line. Both verified against OpenTofu v1.12.5 in both directions: the three forms it accepts are now the three this library writes, and it reads all three back to the same values.
…n#329, amplify-education#336, amplify-education#339) Three defects with one shape: a quoted string or a heredoc body is not one run of literal characters, and every path that rewrote such text treated it as one. `$${` and `%%{` are HCL's escapes for a literal `${` and `%{`, exactly as `\"` is for a quote. The value form returned them doubled, so the value differed from the one Terraform reads -- in the single mode whose whole purpose is to give the value. (amplify-education#336) `strings_to_heredocs` resolved four escapes where the reader resolves nine, because it spelled its own alphabet. A tab written `\t` reached the heredoc body as a backslash and a `t`, and `\uNNNN` fared the same. It now calls `process_escape_sequences`, which is the package's one implementation of that alphabet. (amplify-education#329) Both the escaping and the unescaping ran over interpolation text. That belongs to an expression, not to this string: OpenTofu reads `"${upper("a")}"` as `A` and rejects `"${upper(\"a\")}"` outright, so escaping through an interpolation produced source the reference implementation will not parse, while resolving through one closed a nested string literal early and changed what the expression said. (amplify-education#339) `hcl2/template.py` recovers the spans from text in one left-to-right pass. It has to be one pass: a splitter run before escapes are resolved sees the `${` inside `$${` and opens a span that is not there. The grammar already separates these for a quoted string, which is why `StringRule._serialize_part_as_value` could do the right thing by asking each part for its terminal; a heredoc body arrives as one token, so the distinction is recovered rather than given. One existing test asserted the doubled sigil in the value form. It was pinning amplify-education#336, and now states what OpenTofu evaluates.
The span scan skipped string literals, on the grounds that they are what
carries a brace that does not nest. Comments do too, and HCL writes them
three ways: OpenTofu evaluates `${1 /* } */ + 2}` to 3, so counting that
brace ended the interpolation in the middle of itself and handed the
rest back as literal text -- which flattening then escaped, rewriting
expression source into something that would not parse.
`#` and `//` run to the end of the line, `/* */` to its terminator, and
an unterminated one runs to the end rather than hanging.
A brace inside a heredoc body written inline in an expression is still
counted. That needs the delimiter matched to recognise, and is recorded
in the docstring as a known gap rather than left to be discovered.
A backslash pair is one unit. The scan entered string mode at the quote
of a `\"`, read the real closing quote as another escape, and ran to the
end of the text -- so a span containing the grammar's own `\"..\"` form
swallowed everything after it into one interpolation.
Resolving escapes can spell a sigil that was not in the source.
`"${foo}"` is the six literal characters `${foo}` to
Terraform, because escapes resolve at token level and the result is not
rescanned; written into a heredoc body, which is not escaped at all,
those characters are a live interpolation. The reverse demotes one. The
conversion is refused in both directions now, as it already was for a
lone carriage return, by comparing the spans of the source with the
spans of the resolved content.
An unbalanced span is literal rather than an expression. Calling it an
expression meant nothing escaped it, so the heredoc paths wrote raw
newlines and unescaped quotes into what the API calls quoted-string
source -- turning a loud failure into silent bad output. Two tests
pinned the old behaviour and now state this one.
`split_template` returns immediately for text holding no `$` or `%`,
which is nearly all of it: a scan for two characters answers the
question that a per-character loop was answering. `process_escape_
sequences` next door already had the guard.
One finding is refuted rather than fixed: escapes inside `$${...}` are
not resolved by OpenTofu either. `"$${a\tb}"` evaluates to `${a\tb}`
with a literal backslash and a `t`, which is what this returns -- the
whole marker is one token, and its interior is not a template.
`_unescape_heredoc_body` ran before the check that decides whether its
result is wanted, so for a document where nothing ends in a newline
every pass over every value was discarded. The last two characters
answer it: a value ends with a newline only if its source does, escaped
or real. Conservative -- `"a\\n"` passes here and is rejected by the
check that matters -- and cheap.
The escaper had no test at all, which is how the reader and writer drift
apart. Its four markers are now pinned against `process_escape_
sequences` reading them back, and the two it does not write -- `\t` and
the unicode forms -- are stated as deliberate: those characters are
legal inside a quoted string as themselves.
The unicode test asserted nothing, because its input held a literal
e-acute rather than the escape. It uses `é` and `\U0001F600` now,
and fails against the four-escape implementation this replaced.
The `$${` and `%%{` expectations are in `CASES`, so
`bin/heredoc_ground_truth` re-derives them from OpenTofu with the rest:
23 cases, 0 disagreements. The `${keep}` case is not there on purpose --
an undefined reference has no value for `tofu console` to print.
`_serialize_part_as_value` decides what to do by asking each part for its
terminal, which works while the parts are flat. A template directive is
not: the whole `%{ if }...%{ endif }` construct, and everything between,
arrives as one part. So a `$${` written inside one never reached the
branch that resolves it and stayed doubled -- while the same content in
a heredoc resolved, because that path works on text rather than parts.
The two source forms disagreed about identical content.
Nested parts now go through the same span-aware helper the heredoc path
uses: it resolves the markers in the literal stretches and leaves the
directives themselves, which are expression source, alone.
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. |
This was referenced Sep 2, 2026
`test_a_unicode_escape_becomes_its_character` fed a literal e-acute rather than a backslash-u escape spelling one, so it passed against the unfixed writer and proved nothing about the escape form its name claimed. `TestTheWriterResolvesUnicodeEscapes` already covers both escape forms, so rename this one to say what it does test -- that a non-ASCII character written literally survives the trip into a heredoc body -- and point the newer class's docstring at it rather than at "the earlier test".
livingstaccato
force-pushed
the
fix/multiline-interpolation
branch
from
September 2, 2026 17:56
6bc3b58 to
77013ab
Compare
_skip_string scanned for the next quote, so in `${upper("v${ "{" }w")}` it
ended the outer literal at the quote that opens the innermost one. The brace
after it was then counted as structural, the span came back unbalanced, and
the whole text was handed to the literal path -- where its quotes get escaped,
which is the corruption amplify-education#339 exists to prevent.
It now recurses into a nested `${...}` or `%{...}`, and leaves `$${` and `%%{`
as the escapes they are. Checked against OpenTofu v1.12.5, which evaluates
`"a ${upper("v${ "{" }w")} b"` to `a V{W b` and `"a ${upper("v$${x}w")} b"`
to `a V${X}W b`; three levels of nesting and a nested directive are covered
too. Found by cross-examination of the review of this branch.
…plify-education#347) The quoted form cannot express one. The newlines inside `${...}` are expression source: OpenTofu rejects an escaped newline there with "This character is not used within the language", and a raw one makes the quoted string span lines, which it rejects as an invalid multi-line string. There is no third spelling. It emitted the raw version, so `preserve_heredocs=False` produced output that neither Terraform nor this library could read -- `loads` of its own result raised `UnexpectedToken` -- with no error at the point it was written. It now hands the heredoc back as it was written, which is the form `preserve_heredocs=True` produces and reads back as that heredoc. So the value survives in the shape that can carry it, and a document round trips unchanged. Declining is the only answer that does not change what the document means. Collapsing the interpolation onto one line is semantically identical for most expressions and silently wrong for one holding a `#` comment or a nested heredoc; raising would break callers flattening documents that happen to contain one, including through the CLI. The value form is untouched -- it hands back the body, which has no such limit.
livingstaccato
force-pushed
the
fix/multiline-interpolation
branch
from
September 2, 2026 18:37
77013ab to
545b8e2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #347.
Stacked on #346: this branch starts from
fix/escape-handling, because the check reuses the span scanner that PR adds. Read its diff against that branch.What
preserve_heredocs=Falsereturns a heredoc as quoted-string source. That form cannot hold an interpolation running across lines:OpenTofu evaluates that to
"3\n". Flattened, it produced"${\n 1 + 2\n}\n"with raw newlines inside the quotes — which OpenTofu rejects as an invalid multi-line string, and whichhcl2.loadscannot read back either. Escaping those newlines does not help: inside${...}they are expression source, and OpenTofu rejects\nthere with "This character is not used within the language". There is no third spelling.So it was writing output nothing could read, with no error at the point it was written.
What it does now
Hands the heredoc back as it was written — the form
preserve_heredocs=Trueproduces, which the deserializer reads back as that heredoc. The value survives in the shape that can carry it, and the document round trips unchanged.Why declining rather than the alternatives
It is the only one of the three that never changes what a document means, and it is the shape
strings_to_heredocsalready uses twice: a value that does not end in a newline stays quoted, and one carrying a lone carriage return stays quoted, both because the target form cannot express them.Collapsing the interpolation onto one line is semantically identical for most expressions, and silently wrong for one holding a
#comment — which would swallow the rest of the joined line — or a nested heredoc, which cannot be collapsed at all. Raising would be an improvement on silent corruption, but breaks callers flattening documents that happen to contain one, including anyone runninghcl2tojsonin a pipeline, where the failure is a stack trace rather than a diagnostic.The value form (
strip_string_quotes=True) is untouched: it hands back the body, which has no such limit.Merging
This branch is stacked on #335, #346 and therefore contains those commits. It is opened against
mainbecause GitHub will not base a cross-fork pull request on a branch in the fork, so merging this merges #335, #346 with it — please take them first, or ask and I will rebase this onto whatever lands.It touches the same code as #350 (
hcl2/rules/strings.py), #351 (hcl2/deserializer.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. Every reproduction, test run and benchmark cited was executed rather than inferred, but please review with that provenance in mind.