From 1e63e40ca473e4b3c2da81acab2ab8fb9e87c11b Mon Sep 17 00:00:00 2001 From: Tim Date: Tue, 18 Aug 2026 11:23:35 -0700 Subject: [PATCH] fix: make strip_string_quotes yield values, not broken source (#308, #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. --- CHANGELOG.md | 2 + hcl2/rules/strings.py | 30 ++++++++++++-- hcl2/utils.py | 65 +++++++++++++++++++++++++++++ test/unit/rules/test_strings.py | 17 ++++++++ test/unit/test_api.py | 72 +++++++++++++++++++++++++++++++++ test/unit/test_utils.py | 39 ++++++++++++++++++ 6 files changed, 222 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b0148ee..992ecc9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed - Restore `py.typed` marker so type checkers recognize `hcl2` (and `cli`) as typed packages. ([#298](https://github.com/amplify-education/python-hcl2/issues/298)) +- `strip_string_quotes` no longer unquotes string literals nested inside expressions, which produced invalid HCL such as `${upper(x)}` from `upper("x")`. ([#310](https://github.com/amplify-education/python-hcl2/issues/310)) +- `strip_string_quotes` now resolves escape sequences, so the values it yields match what the option documents. ([#308](https://github.com/amplify-education/python-hcl2/issues/308)) ## \[8.1.2\] - 2026-04-10 diff --git a/hcl2/rules/strings.py b/hcl2/rules/strings.py index 4be161cc..e6bec8fb 100644 --- a/hcl2/rules/strings.py +++ b/hcl2/rules/strings.py @@ -21,6 +21,7 @@ HEREDOC_TRIM_PATTERN, SerializationContext, SerializationOptions, + process_escape_sequences, to_dollar_string, ) @@ -92,12 +93,35 @@ def string_parts(self): return self.children[1:-1] def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: - """Serialize to a quoted string.""" + """Serialize to a quoted string. + + `strip_string_quotes` asks for the string's value rather than its + source form, so it applies only where a value is what the caller gets: + a string nested inside an expression is part of that expression's text, + and unquoting it there would produce something that is no longer valid + HCL (`upper("x")` becoming `upper(x)`). + """ + if options.strip_string_quotes and not context.inside_dollar_string: + return "".join( + self._serialize_part_as_value(part, options, context) for part in self.string_parts + ) + inner = "".join(part.serialize(options, context) for part in self.string_parts) - if options.strip_string_quotes: - return inner return '"' + inner + '"' + @staticmethod + def _serialize_part_as_value(part, options, context) -> str: + """Serialize one part, resolving escapes in literal text only. + + Interpolations and escaped interpolation/directive markers are passed + through untouched: their text is expression source, not literal + content, so an escape inside them is not this string's to resolve. + """ + serialized = part.serialize(options, context) + if isinstance(part.content, STRING_CHARS): + return process_escape_sequences(serialized) + return serialized + class HeredocTemplateRule(LarkRule): """Rule for heredoc template strings (< Optional[Tuple[str, int]]: + """Decode a \\uNNNN or \\UNNNNNNNN escape whose marker sits at `index`.""" + width = _UNICODE_ESCAPE_WIDTHS[text[index]] + digits = text[index + 1 : index + 1 + width] + if len(digits) != width or any(char not in _HEX_DIGITS for char in digits): + return None + return chr(int(digits, 16)), index + 1 + width + + +def process_escape_sequences(value: str) -> str: + """Resolve the escape sequences HCL defines inside a quoted template. + + Used when `strip_string_quotes` is set, which asks for the *value* of a + string rather than its source form. Escapes are resolved in a single pass, + so an escaped backslash cannot combine with the character after it: `\\\\n` + is a backslash followed by "n", not a newline. + + An unrecognized escape is preserved verbatim, backslash included. Terraform + rejects those outright, but the grammar here accepts them, and a serializer + is the wrong place to raise an error the parser did not. + """ + if "\\" not in value: + return value + + parts = [] + index = 0 + length = len(value) + while index < length: + char = value[index] + if char != "\\" or index + 1 >= length: + parts.append(char) + index += 1 + continue + + marker = value[index + 1] + if marker in _SIMPLE_ESCAPES: + parts.append(_SIMPLE_ESCAPES[marker]) + index += 2 + continue + if marker in _UNICODE_ESCAPE_WIDTHS: + decoded = _decode_unicode_escape(value, index + 1) + if decoded is not None: + parts.append(decoded[0]) + index = decoded[1] + continue + + parts.append(char) + parts.append(marker) + index += 2 + + return "".join(parts) + + @dataclass class SerializationContext: """Mutable state tracked during serialization traversal.""" diff --git a/test/unit/rules/test_strings.py b/test/unit/rules/test_strings.py index d5eac752..ce65aac3 100644 --- a/test/unit/rules/test_strings.py +++ b/test/unit/rules/test_strings.py @@ -178,6 +178,23 @@ def test_serialize_strip_string_quotes_with_interpolation(self): opts = SerializationOptions(strip_string_quotes=True) self.assertEqual(rule.serialize(opts), "prefix:${var.name}") + def test_serialize_strip_string_quotes_inside_expression_keeps_quotes(self): + """A string nested in an expression is that expression's source text.""" + rule = _make_string([_make_string_part_chars("x")]) + opts = SerializationOptions(strip_string_quotes=True) + ctx = SerializationContext(inside_dollar_string=True) + self.assertEqual(rule.serialize(opts, ctx), '"x"') + + def test_serialize_strip_string_quotes_resolves_escapes(self): + rule = _make_string([_make_string_part_chars(r"say \"hi\"")]) + opts = SerializationOptions(strip_string_quotes=True) + self.assertEqual(rule.serialize(opts), 'say "hi"') + + def test_serialize_without_strip_keeps_escapes_raw(self): + """Default output stays source-shaped so it can be reconstructed.""" + rule = _make_string([_make_string_part_chars(r"say \"hi\"")]) + self.assertEqual(rule.serialize(), r'"say \"hi\""') + # --- HeredocTemplateRule tests --- diff --git a/test/unit/test_api.py b/test/unit/test_api.py index 6af029a5..5ab9a3eb 100644 --- a/test/unit/test_api.py +++ b/test/unit/test_api.py @@ -288,3 +288,75 @@ def test_query_file_object(self): self.assertIsInstance(result, DocumentView) attr = result.attribute("x") self.assertIsNotNone(attr) + + +class TestStripStringQuotes(TestCase): + """`strip_string_quotes=True` asks for values, not source text. + + It is the documented v7-compatibility path, so it has to yield what v7 + yielded: quotes removed from string *values*, escape sequences resolved, + and expressions left as valid HCL. + """ + + _OPTIONS = SerializationOptions(strip_string_quotes=True) + + def _load(self, source): + return loads(source, serialization_options=self._OPTIONS) + + def test_plain_value_loses_its_quotes(self): + self.assertEqual(self._load('a = "plain"\n'), {"a": "plain"}) + + def test_value_in_object_and_list(self): + self.assertEqual( + self._load('a = { k = "v" }\nb = ["x"]\n'), + {"a": {"k": "v"}, "b": ["x"]}, + ) + + def test_function_argument_keeps_its_quotes(self): + """Unquoting here would turn a string into an identifier.""" + self.assertEqual(self._load('a = upper("x")\n'), {"a": '${upper("x")}'}) + + def test_conditional_branches_keep_their_quotes(self): + self.assertEqual( + self._load('a = var.x ? "yes" : "no"\n'), + {"a": '${var.x ? "yes" : "no"}'}, + ) + + def test_comparison_against_a_string_keeps_its_quotes(self): + """An unquoted empty string would leave `s != ` behind.""" + self.assertEqual( + self._load('a = [for s in var.l : upper(s) if s != ""]\n'), + {"a": '${[for s in var.l : upper(s) if s != ""]}'}, + ) + + def test_nested_call_keeps_every_quote(self): + self.assertEqual( + self._load('a = join(",", ["x", "y"])\n'), + {"a": '${join(",", ["x", "y"])}'}, + ) + + def test_escaped_quote_is_resolved(self): + self.assertEqual(self._load(r'a = "quote \"in\" here"' + "\n"), {"a": 'quote "in" here'}) + + def test_escaped_whitespace_is_resolved(self): + self.assertEqual(self._load(r'a = "x\ny\tz"' + "\n"), {"a": "x\ny\tz"}) + + def test_escaped_backslash_is_resolved(self): + self.assertEqual(self._load(r'a = "back\\slash"' + "\n"), {"a": "back\\slash"}) + + def test_escaped_backslash_does_not_combine_with_the_next_character(self): + r"""`\\n` is a backslash followed by "n", not a newline.""" + self.assertEqual(self._load(r'a = "back\\nslash"' + "\n"), {"a": "back\\nslash"}) + + def test_unknown_escape_is_preserved(self): + self.assertEqual(self._load(r'a = "keep \q intact"' + "\n"), {"a": r"keep \q intact"}) + + def test_interpolation_is_left_alone(self): + self.assertEqual(self._load('a = "pre${var.x}post"\n'), {"a": "pre${var.x}post"}) + + def test_escaped_interpolation_marker_is_left_alone(self): + self.assertEqual(self._load('a = "lit $${x}"\n'), {"a": "lit $${x}"}) + + def test_default_options_still_preserve_source_form(self): + """Without the option, the source form is kept for reconstruction.""" + self.assertEqual(loads(r'a = "line1\nline2"' + "\n"), {"a": r'"line1\nline2"'}) diff --git a/test/unit/test_utils.py b/test/unit/test_utils.py index ab5335c5..5dc04d13 100644 --- a/test/unit/test_utils.py +++ b/test/unit/test_utils.py @@ -5,6 +5,7 @@ SerializationContext, SerializationOptions, is_dollar_string, + process_escape_sequences, to_dollar_string, unwrap_dollar_string, wrap_into_parentheses, @@ -140,3 +141,41 @@ def test_expression_string(self): def test_dollar_expression(self): self.assertEqual(wrap_into_parentheses("${a + b}"), "${(a + b)}") + + +class TestProcessEscapeSequences(TestCase): + """Escape resolution used by `strip_string_quotes`.""" + + def test_no_backslash_is_returned_unchanged(self): + self.assertEqual(process_escape_sequences("plain text"), "plain text") + + def test_simple_escapes(self): + self.assertEqual(process_escape_sequences(r"a\nb\tc\rd"), "a\nb\tc\rd") + + def test_escaped_quote(self): + self.assertEqual(process_escape_sequences(r"say \"hi\""), 'say "hi"') + + def test_escaped_backslash(self): + self.assertEqual(process_escape_sequences(r"back\\slash"), "back\\slash") + + def test_escaped_backslash_is_not_reused_by_the_next_character(self): + r"""A single pass: `\\n` cannot become a newline.""" + self.assertEqual(process_escape_sequences(r"back\\nslash"), "back\\nslash") + + def test_unicode_escape(self): + self.assertEqual(process_escape_sequences(r"café"), "café") + + def test_long_unicode_escape(self): + self.assertEqual(process_escape_sequences(r"\U0001F600"), "\U0001f600") + + def test_malformed_unicode_escape_is_preserved(self): + self.assertEqual(process_escape_sequences(r"\u12"), r"\u12") + + def test_non_hex_unicode_escape_is_preserved(self): + self.assertEqual(process_escape_sequences(r"\uZZZZ"), r"\uZZZZ") + + def test_unknown_escape_is_preserved(self): + self.assertEqual(process_escape_sequences(r"keep \q intact"), r"keep \q intact") + + def test_trailing_backslash_is_preserved(self): + self.assertEqual(process_escape_sequences("trailing\\"), "trailing\\")