fix: make strip_string_quotes yield values, not broken source (#308, #310) - #313
Open
livingstaccato wants to merge 1 commit into
Open
fix: make strip_string_quotes yield values, not broken source (#308, #310)#313livingstaccato wants to merge 1 commit into
livingstaccato wants to merge 1 commit into
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.
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 #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.