fix: a heredoc ends its own line wherever it is written (#338) - #352
Open
livingstaccato wants to merge 9 commits into
Open
fix: a heredoc ends its own line wherever it is written (#338)#352livingstaccato wants to merge 9 commits into
livingstaccato wants to merge 9 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.
…ation#338) A heredoc ends at its closing marker, on a line of its own, so whatever follows has to start the next line. Inside a list or an object that is the separator, and `EOF,` closes nothing -- the file this library had just written did not parse, here or in Terraform. A top-level attribute survived only because the newline after it comes from the document rather than from the heredoc. The earlier attempt at this appended the newline to the token, which fixed containers and gave every top-level heredoc a blank line, because the reconstructor already supplies one there. The distinction it was missing: `HEREDOC_TEMPLATE` matches through the newline after the marker, so a token that came from the parser already ends the line and a token built by the deserializer does not. Only the second needs help. So the rule lives where the tokens are joined, and asks whether the heredoc just written ended its line rather than assuming either way. Reconstructing a parsed document is byte for byte what it was, which the round-trip fixtures already assert. OpenTofu reads the emitted list back as ["line1\n", "p"].
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. 🤖 Drafted with Claude Code. |
Contributor
Author
|
Review pass done, so the hold above no longer applies — this is ready for review now. Rebased on current 🤖 Drafted with Claude Code. |
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 #338.
Stacked on #335: this branch starts from
fix/heredoc-body-values, because the defect is only reachable once that PR's fix makes values eligible for conversion. Read its diff against that branch.What
A heredoc ends at its closing marker, on a line of its own, so whatever follows has to start the next line. Inside a list or an object that is the separator:
EOF,closes nothing.hcl2.loadsof that raisesUnexpectedTokenand OpenTofu reports "Unterminated template string / No closing marker was found" — the file this library had just written does not parse. The same happens for an object value, an object key and a block label. A top-level attribute survived only because the newline after it comes from the document rather than from the heredoc.Why not simply append a newline to the token
That was my first attempt, and it fixes containers while giving every top-level heredoc a blank line after it — the round-trip fixture grows one per heredoc — because the reconstructor already supplies one there.
The distinction it was missing is that
HEREDOC_TEMPLATEmatches through the newline after the closing marker. A token that came from the parser therefore already ends its line; one built by the deserializer does not. Only the second needs help.So the rule lives where the tokens are joined, and asks whether the heredoc just written ended its line rather than assuming. Reconstructing a parsed document is byte for byte what it was, which the existing round-trip fixtures assert and a new test states directly.
Evidence
OpenTofu v1.12.5 reads the emitted list back as
["line1\n", "p"], and the values on both sides of the round trip compare equal.Merging
This branch is stacked on #335 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 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). 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.