Fix CRLF (\r\n) line endings failing to parse - #317
Merged
kkozik-amplify merged 3 commits intoAug 24, 2026
Merged
Conversation
hcl2.lark's %ignore rule only skipped spaces and tabs, so a bare '\r' preceding the newline in a CRLF-terminated line had no terminal that could consume it: NL_OR_COMMENT only matches starting from '\n', and the ignore rule didn't cover '\r'. The stray '\r' fell through to STRING_CHARS and the parse failed with UnexpectedToken for every construct (bare attributes, blocks, quoted strings) as soon as a single CRLF line appeared anywhere in the source. Add '\r' to the %ignore whitespace pattern. Terminals that need to preserve '\r' verbatim as part of their own content (STRING_CHARS, HEREDOC_TEMPLATE*, comments) still win via lark's longest-match rule, so this doesn't affect round-trip fidelity of quoted strings or heredocs -- verified with a dedicated test plus the full existing suite (1391 tests, unchanged, all still passing). Fixes amplify-education#315
%ignore handles \r only between tokens, so heredocs -- whose terminals
match \n inside their own pattern -- still failed under CRLF. HCL's
reference scanner accepts both \n and \r\n around heredoc markers, so
these are valid inputs. Three places needed it:
- hcl2.lark: \r? around the markers in both heredoc terminals
- utils.py: the same in HEREDOC_PATTERN / HEREDOC_TRIM_PATTERN, which
run on an already-parsed token and otherwise raise RuntimeError when
flattening a heredoc the grammar just accepted
- strings.py: \r in _trim_chars, so the preserved form does not keep a
stray carriage return after the closing marker
Body text keeps its carriage returns, matching HCL: \r around markers is
structure, \r inside the body is content.
Corrected the %ignore comment. It claimed the heredoc terminals "win via
Lark's longest-match rule" -- they never matched at all, so the comment
would have sent the next reader looking in the wrong place.
test_embedded_cr_inside_string_is_preserved asserted on \r escape *text*
(two ASCII characters, never at risk) and was the only test that passed
without the grammar change. Replaced with a real CR byte, and added the
cases the fix newly enables: heredocs, comments, tuples/objects, and the
CRLF-to-LF reconstruction that this approach implies.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kkozik-amplify
added a commit
to agu2347/python-hcl2
that referenced
this pull request
Aug 24, 2026
Heredocs parse in CRLF files once amplify-education#317 lands, so the newline before the closing marker may be \r\n. Stripping only \n leaves the \r stranded on the end of the value. Inert without amplify-education#317 -- no CRLF heredoc parses today -- but it belongs with this helper rather than with the grammar change that exposes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
amplify-education#312, amplify-education#311 and amplify-education#313 have landed. Two conflicts: hcl2/hcl2.lark - real overlap with amplify-education#312, both editing the heredoc terminals. Resolved as the union: amplify-education#312's lazy optional body group plus this branch's \r?, giving /<<MARKER\r?\n(?:(?:.|\n)*?\r?\n)??\s*MARKER\r?\n/. Verified an empty heredoc in a CRLF file parses, two consecutive empty CRLF heredocs stay separate, and a line merely ending in the marker still does not terminate the body. CHANGELOG.md - landed entries first, this branch's appended. hcl2/rules/strings.py and hcl2/utils.py auto-merged against amplify-education#313, which touched both. Checked rather than assumed: strip_string_quotes on CRLF source, escapes resolved through CRLF line endings, a literal CR inside a string surviving, and amplify-education#313's out-of-range and lone-surrogate escape guards all still behave. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kkozik-amplify
approved these changes
Aug 24, 2026
kkozik-amplify
added a commit
to agu2347/python-hcl2
that referenced
this pull request
Aug 24, 2026
amplify-education#317 has landed since the last update. Only CHANGELOG.md conflicted, resolved with the landed entries first and this branch's appended. hcl2/rules/strings.py auto-merged: amplify-education#317's `_trim_chars = "\r\n\t "` for the preserve path now sits alongside this branch's CRLF-aware `_strip_closing_marker_line` for the flatten paths. Verified rather than assumed -- CRLF heredocs flatten and preserve correctly, empty CRLF heredocs parse, and this branch's own cases (trailing blank line, trailing spaces, indented closing marker, blank line not cancelling the `<<-` dedent) all still hold. Full suite: 1504 passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kkozik-amplify
added a commit
that referenced
this pull request
Aug 24, 2026
…rip (#318) * Fix heredoc bodies losing trailing blank lines/spaces to a greedy rstrip HeredocTemplateRule.serialize's preserve_heredocs=False path did match.group(2).rstrip("\n\t "), which removes every trailing newline, tab, and space character -- not just the single newline separating the last body line from the closing marker. A trailing blank line, multiple trailing blank lines, and trailing spaces on the last line all collapsed to the same flattened value as a plain one-line body. Replace the blanket rstrip with a helper that removes exactly the one trailing newline that separates content from the marker line, per the issue's proposed scope. This is limited to the plain <<MARKER heredoc; the indented <<-MARKER variant has separate, pre-existing leading-whitespace-trim semantics with its own golden tests, which are unaffected by this change (verified against the full suite). Fixes #316 * fix: apply the trailing-whitespace fix to <<- too, and keep the marker line out The blanket rstrip was doing two jobs: dropping the newline that separates the last body line from the closing marker, and dropping that marker line's own indentation. Removing only the newline leaves the indentation behind, so an indented closing marker leaked into the body: <<MARKER\nx\n MARKER -> "x\n " (was "x") No existing test flattens a heredoc with an indented closing marker, so the suite stayed green. The spec is explicit that the marker "may also have an arbitrary number of spaces preceding it on its line", so that whitespace is not content. Splitting the two concerns in _strip_closing_marker_line also removes the reason to skip <<-MARKER, which still collapsed trailing blank lines and trailing spaces exactly as #316 describes. Its rstrip was the same double-duty call, which is why swapping in a newline-only strip broke the dedent; handling the marker line separately does not. Fixing <<- exposed a second bug that the old rstrip had been masking: a blank line has zero leading spaces, so once blank lines survive they drag min_spaces to zero and cancel the dedent for every other line. The spec measures "any literal string at the start of each line" -- a blank line has none -- so blank lines are now skipped when measuring. Tests cover the indented closing marker (spaces and tab, with and without a trailing blank line) and the <<- variant throughout. The module docstring is now raw; its `rstrip("\n\t ")` reference was being read as real control characters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: make _strip_closing_marker_line CRLF-aware Heredocs parse in CRLF files once #317 lands, so the newline before the closing marker may be \r\n. Stripping only \n leaves the \r stranded on the end of the value. Inert without #317 -- no CRLF heredoc parses today -- but it belongs with this helper rather than with the grammar change that exposes it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Kamil Kozik <kkozik@amplify.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Problem
Any HCL2 source with CRLF (
\r\n) line endings fails to parse, regardless of the surrounding construct:A single CRLF line anywhere in an otherwise-LF file is enough to fail the whole file. Windows-authored
.tffiles, or files checked out withcore.autocrlf=true, are unparseable.Fixes #315.
Root cause
hcl2/hcl2.lark's%ignorerule only skips spaces and tabs:NL_OR_COMMENTonly matches starting from\n. So when a\rimmediately precedes a significant\n, no terminal can consume it, and it falls through toSTRING_CHARS, breaking the parse.Fix
Add
\rto the%ignorewhitespace pattern:%ignore /[ \t\r]+/.This only affects
\rthat is insignificant whitespace between tokens. Terminals that need to preserve\rverbatim as part of their own content (STRING_CHARS,HEREDOC_TEMPLATE*, comment alternatives) still win via lark's longest-match rule, since they match longer contiguous spans that include the\r. Verified this holds for a quoted string containing an embedded\r(round-trip fidelity is preserved).Testing
Added
test/unit/test_crlf.pycovering every construct from the issue: a bare attribute, a block, a quoted string, a CRLF line embedded among LF lines, and confirmation that an embedded\rinside a string literal survives untouched. Confirmed each new test fails with the exact reportedUnexpectedTokenerror against the pre-fix grammar, and passes after the fix.Ran the full existing suite (1391 tests) against the fix: all pass, zero regressions.