fix: make strip_string_quotes yield values, not broken source (#308, #310) - #313
Merged
kkozik-amplify merged 3 commits intoAug 24, 2026
Merged
Conversation
…y-education#308, amplify-education#310) `strip_string_quotes` is documented as the option that returns a plain string instead of `'"hello"'`, and the v8 migration guide presents it as the v7 compatibility path. It did neither job completely. It unquoted every string literal, including those nested inside an expression, where the surrounding text is HCL source rather than a value: upper("x") -> ${upper(x)} var.x ? "yes" : "no" -> ${var.x ? yes : no} [for s in l : s if s != ""] -> ${[for s in l : s if s != ]} The first two silently change meaning, referring to identifiers that do not exist, and the third is not parseable at all. Restrict the stripping to strings that are values, using the `inside_dollar_string` context flag the serializer already threads through expression rules. It also left escape sequences unresolved, so a caller asking for the value of `"line1\nline2"` got a literal backslash and an `n`. Resolve them when stripping, as v7 did. The pass is single, so an escaped backslash cannot combine with the character after it -- v7 replaced sequentially and turned `\\n` into a backslash followed by a newline, where HCL specifies a backslash followed by "n". Only literal STRING_CHARS parts are processed; interpolations and escaped interpolation markers carry expression text, whose escapes are not this string's to resolve. Default output is untouched: it stays source-shaped so that dumps() can reconstruct it, and this option is already documented as one-way. Existing coverage exercised the option only on simple values, which is why neither defect showed up. Without the fix, 10 of the new tests fail.
Three loose ends in the strip_string_quotes fix.
_decode_unicode_escape called chr() on any well-formed hex run, so an
out-of-range codepoint escaped as an exception rather than as text:
\U00110000 raised ValueError and \UFFFFFFFF raised OverflowError, both
straight out of loads(). A lone surrogate was worse than a raise — it
decoded fine and then failed later on .encode("utf-8"). All three now
return None, which the caller already handles by preserving the escape
verbatim, matching the documented policy that a serializer should not
raise an error the parser did not.
isinstance(part.content, STRING_CHARS) failed mypy, since STRING_CHARS
is a subscripted factory it reads as a parameterized generic. Compare
lark_name() instead, which is how the rest of the codebase identifies
nodes.
Document what the option now does: docs/01 described only quote removal,
and docs/06 warned only that it is one-way. Neither mentioned escape
resolution or that expression interiors keep their quotes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kkozik-amplify
approved these changes
Aug 24, 2026
amplify-education#312 and amplify-education#311 have landed since this branch was opened. Both conflicts are append-only and keep both sides: CHANGELOG.md - landed entries first, so this branch's two lines are a pure append test_api.py - TestEmptyHeredocs, TestNegativeIntegerLiterals and TestNegatedKeywords from main, then this branch's TestStripStringQuotes No source overlap: this branch touches hcl2/rules/strings.py and hcl2/utils.py, which neither amplify-education#311 (expressions.py) nor amplify-education#312 (hcl2.lark) went near. 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
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
added a commit
to agu2347/python-hcl2
that referenced
this pull request
Aug 24, 2026
amplify-education#312, amplify-education#311 and amplify-education#313 have landed. Only CHANGELOG.md conflicted, resolved with the landed entries first and this branch's appended. hcl2/rules/strings.py auto-merged against amplify-education#313, which rewrote the same file's string-serialization path. Verified the two coexist: amplify-education#313's process_escape_sequences and lark_name() dispatch sit alongside this branch's _strip_closing_marker_line, and the full suite passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 25, 2026
kkozik-amplify
added a commit
that referenced
this pull request
Aug 26, 2026
…303) (#324) * fix: return heredoc bodies as real multi-line values when unquoting (#303) The flatten path escapes the body to build a quoted-string source form (`'"a\nb"'`). That escaping ran before the strip_string_quotes early return, so a caller asking for the *value* got escaped *source* back: every line break arrived as a literal backslash-n. Reported from production, where a heredoc-defined PGP private key came out as a single line and was silently unusable. Before this fix no combination of SerializationOptions reproduced v7's plain multi-line string -- confirmed by brute-forcing all sixteen combinations of strip_string_quotes, preserve_heredocs, wrap_objects and explicit_blocks. This is the same defect #313 fixed for quoted strings -- strip_string_quotes should yield values, not source -- which touched StringRule and left both heredoc rules behind. Moving the escaping after the early return in each is the whole change; the quoted form is unaffected. The only two tests that failed were the ones asserting the reported behaviour (`"line1\\nline2"`). Both now assert real newlines, and each gains a sibling pinning that the quoted form still escapes, so the two paths cannot drift again. Docs: the option table described preserve_heredocs only as "keep heredocs in their original form", and the migration guide's V7_COMPAT recipe omitted it entirely -- which is how the reporter ended up without a working combination. Both now cover it, and the guide's example output is verified against the code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: note the two heredoc value gotchas in the migration guide Self-review turned up an asymmetry the guide did not mention: with strip_string_quotes set, a quoted string's `\n` resolves to a newline while a heredoc body's stays two literal characters. Both are correct -- HCL processes escapes in quoted templates only -- but a reader coming from the V7_COMPAT recipe has no way to predict it. Same for line endings: a heredoc in a CRLF file yields a body containing `\r\n`, since a carriage return inside the body is content rather than structure. Example output verified against the code rather than written from memory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 26, 2026
Closed
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.
Fixes #310. Fixes #308.
strip_string_quotesis documented as returning a plain string instead of'"hello"', anddocs/06_migrating_to_v8.mdpresents it as the v7 compatibility path. It did neither job completely, and both defects live in the same method.It unquoted strings inside expressions (#310)
The option stripped every string literal, including those nested in an expression, where the surrounding text is HCL source rather than a value:
upper("x")${upper(x)}${upper("x")}var.x ? "yes" : "no"${var.x ? yes : no}${var.x ? "yes" : "no"}[for s in l : s if s != ""]${[for s in l : s if s != ]}${[for s in l : s if s != ""]}The first two silently change meaning, referring to identifiers that do not exist; the third is not parseable at all. The fix restricts stripping to strings that are values, using the
inside_dollar_stringflag the serializer already threads through expression rules.It left escapes unresolved (#308)
Asking for the value of
"line1\nline2"returned a literal backslash and ann. v7 stripped quotes and resolved escapes in the same call; v8 did only the first half, so the option delivered neither the source form nor the value. Since it is already documented as one-way and not round-trippable, resolving escapes there costs nothing in reconstruction fidelity.The pass is single, so an escaped backslash cannot combine with the character after it:
\\nis a backslash followed by "n". v7 replaced sequentially and produced a backslash followed by a newline; the single pass matches what OpenTofu evaluates the same source to, so this is deliberately not bug-compatible with v7.Only literal
STRING_CHARSparts are processed. Interpolations and escaped interpolation markers carry expression text, whose escapes are not the string's to resolve.Scope
Default output is untouched — it stays source-shaped so
dumps()can reconstruct it.Existing coverage exercised the option only on simple values, which is why neither defect showed up. Without the source change, 10 of the new tests fail.
nose2 --config tox.ini: 1419 tests, OK.ruffclean.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.