fix: two rules that ignored they were writing expression source (#340, #341)

CHANGELOG.md

@@ -24,6 +24,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Python 3.14 is now tested and declared as supported. No source changes were needed; the full
   suite passes on 3.14 as-is.
 
+### Fixed
+
+- `strip_string_quotes` now writes a heredoc inside an expression as a string instead of splicing its body in bare. `StringRule` checks `inside_dollar_string` to keep its quotes for exactly this reason; the heredoc rules did not, so `upper(<<E\nx\nE\n)` came back as `${upper(x)}` — a reference to a variable nobody declared — and a multi-line body put raw newlines into source that would not parse. Both `<<` and `<<-` are fixed, in every expression context. Thanks, @livingstaccato ([#350](https://github.com/amplify-education/python-hcl2/pull/350))
+- `strip_string_quotes` now keeps the delimiters of a string literal inside a template directive. `TemplateStringRule` only ever appears inside `%{ ... }`, where the text is expression source and the quotes belong to a literal written in it, so dropping them turned `%{ if x == "y" }` into `%{ if x == y }`: a comparison against a variable rather than against a string. Thanks, @livingstaccato ([#350](https://github.com/amplify-education/python-hcl2/pull/350))
+- A parenthesised expression writes what it wraps as HCL rather than as a Python value, on the default options as well: `(true)` used to come back as `${(True)}` and `(null)` as `${(None)}`, which `dumps` wrote back as references to variables nobody declared (OpenTofu rejects both with "Invalid reference"); a tuple or object inside the parentheses came back as a Python repr that `dumps` could not parse; and with `strip_string_quotes`, `("s")` lost its quotes and became `${(s)}`. They now come back as `${(true)}`, `${(null)}`, `${([1, "a"])}` and `${("s")}`, and each round trip evaluates to the value the source does. Thanks, @livingstaccato ([#350](https://github.com/amplify-education/python-hcl2/pull/350))
+
 ## \[8.1.4\] - 2026-09-08
 
 ### Fixed

hcl2/rules/expressions.py

@@ -100,7 +100,16 @@ def expression(self) -> ExpressionRule:
 
     def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any:
         """Serialize, handling parenthesized expression wrapping."""
-        with context.modify(inside_parentheses=self.parentheses or context.inside_parentheses):
+        # A parenthesised term is written as `${(...)}`, so what it wraps is
+        # expression source, exactly as a function's arguments are. Serialized
+        # as a value, `(true)` came back as `${(True)}` and `(null)` as
+        # `${(None)}` -- Python's spelling, which OpenTofu reads as references
+        # to undeclared variables -- and a tuple or object inside came back as
+        # a Python repr that did not parse.
+        with context.modify(
+            inside_parentheses=self.parentheses or context.inside_parentheses,
+            inside_dollar_string=self.parentheses or context.inside_dollar_string,
+        ):
             result = self.expression.serialize(options, context)
 
         if self.parentheses:

hcl2/rules/strings.py

@@ -170,10 +170,18 @@ def serialize(self, options=SerializationOptions(), context=SerializationContext
             if not match:
                 raise RuntimeError(f"Invalid Heredoc token: {heredoc}")
             heredoc = _strip_closing_marker_line(match.group(2))
-            if options.strip_string_quotes:
+            if options.strip_string_quotes and not context.inside_dollar_string:
                 # The caller asked for the value, so hand back the body as-is:
                 # real newlines, no escaping. The escaping below exists only to
                 # build the quoted-string *source* form returned otherwise.
+                #
+                # Not inside an expression, though. There the heredoc is an
+                # argument, and its text is part of that expression's source:
+                # `upper(<<E\nx\nE\n)` has to come back as `upper("x")` and not
+                # as `upper(x)`, which asks for a variable nobody declared. A
+                # multi-line body made it worse, splicing raw newlines into
+                # source that then did not parse. `StringRule` checks the same
+                # flag one class away, for the same reason.
                 return heredoc
             heredoc = heredoc.replace("\\", "\\\\").replace('"', '\\"').replace("\n", "\\n")
             return f'"{heredoc}"'
@@ -232,9 +240,12 @@ def serialize(self, options=SerializationOptions(), context=SerializationContext
         if not options.preserve_heredocs:
             lines = [line.replace("\\", "\\\\").replace('"', '\\"') for line in lines]
 
-        if options.strip_string_quotes:
+        if options.strip_string_quotes and not context.inside_dollar_string:
             # Value, not source: join with real newlines regardless of
             # preserve_heredocs, and skip the escaping done for the quoted form.
+            # Inside an expression the text is that expression's source, so the
+            # quoted form below is what belongs there -- see the note in
+            # `HeredocTemplateRule.serialize`.
             return "\n".join(lines)
 
         sep = "\\n" if not options.preserve_heredocs else "\n"
@@ -272,8 +283,14 @@ def serialize(self, options=SerializationOptions(), context=SerializationContext
         Inside template directive expressions, strings are delimited by \\"
         rather than plain ". We preserve these as \\" in serialized form so
         the deserializer can reconstruct them correctly.
+
+        `strip_string_quotes` asks for a value, and this rule only ever appears
+        inside a directive -- where the text is expression source and the
+        delimiters belong to a string literal written in it. Dropping them
+        there turned `%{ if x == "y" }` into `%{ if x == y }`: a comparison
+        against a variable rather than against a string.
         """
         raw = self.raw_value
-        if options.strip_string_quotes:
+        if options.strip_string_quotes and not context.inside_dollar_string:
             return self.inner_value
         return raw

test/unit/rules/test_expression_source.py

@@ -0,0 +1,240 @@
+# pylint: disable=C0103,C0114,C0115,C0116
+r"""Rules that are serialized into expression source (GH #340, #341).
+
+`SerializationContext.inside_dollar_string` tells a rule it is being written
+into an expression rather than handed to a caller as a value. `StringRule`
+checks it and keeps its quotes, because `upper("x")` becoming `upper(x)` asks
+for a variable nobody declared. Two rules did not check it.
+
+Both defects are in the value form only -- `strip_string_quotes=True`. What
+the other modes emit is unchanged; `TestTheOtherModesAreUntouched` states so,
+including the round trip, because that is the half a change here can break
+without any of the assertions above noticing. The one exception is a
+parenthesised term, `TestAParenthesisedTermIsExpressionSource`, whose default
+output was itself not HCL -- `(true)` came back as `${(True)}` -- so fixing it
+changes only output that no reader could evaluate.
+
+Checked against Terraform v1.11.4:
+
+    upper(<<EOT\nx\nEOT\n)              -> "X\n", so the argument is a string
+    "%{ if local.x == "y" }t%{ endif }" -> "t", with plain quotes
+
+The escaped spelling this grammar also accepts, `\"y\"` inside a directive,
+Terraform rejects outright with "Invalid character". That divergence is filed
+as #341's sibling, #353; the tests below only pin that the value form stops
+mangling it into a reference, which is what #341 asks for.
+"""
+
+from unittest import TestCase
+
+from hcl2.api import dumps, loads
+from hcl2.utils import SerializationOptions
+
+VALUE = SerializationOptions(preserve_heredocs=False, strip_string_quotes=True)
+QUOTED = SerializationOptions(strip_string_quotes=True)
+SOURCE = SerializationOptions(preserve_heredocs=False)
+DEFAULT = SerializationOptions()
+
+
+class TestAHeredocInsideAnExpression(TestCase):
+    """#340: the body was spliced in bare, so it read as a reference."""
+
+    def value(self, source: str) -> str:
+        return loads(source, serialization_options=VALUE)["a"]
+
+    def test_it_stays_a_string(self):
+        self.assertEqual(self.value("a = upper(<<E\nx\nE\n)\n"), '${upper("x")}')
+
+    def test_it_matches_the_quoted_equivalent(self):
+        self.assertEqual(
+            self.value("a = upper(<<E\nx\nE\n)\n"),
+            self.value('a = upper("x")\n'),
+        )
+
+    def test_a_multi_line_body_does_not_splice_raw_newlines(self):
+        result = self.value("a = upper(<<E\nx\ny\nE\n)\n")
+        self.assertEqual(result, '${upper("x\\ny")}')
+        self.assertNotIn("\n", result)
+
+    def test_the_trim_form_too(self):
+        self.assertEqual(self.value("a = upper(<<-E\n  x\n  E\n)\n"), '${upper("x")}')
+
+    def test_a_heredoc_that_is_not_in_an_expression_is_unaffected(self):
+        # Nothing wraps this one, so the caller does get the bare body.
+        self.assertEqual(self.value("a = <<E\nx\nE\n"), "x")
+
+    def test_a_heredoc_in_a_container_is_unaffected(self):
+        # A tuple element and an object value are values, not expression
+        # source, so they keep handing back the body.
+        self.assertEqual(self.value("a = [<<E\nx\nE\n]\n"), ["x"])
+        self.assertEqual(self.value("a = {k = <<E\nx\nE\n}\n"), {"k": "x"})
+
+
+class TestEveryExpressionContextQuotesIt(TestCase):
+    """`inside_dollar_string` is set by more rules than the function call.
+
+    #340 was reported against an argument, but every rule that marks its
+    children as expression source had the same hole. One case each, so a rule
+    that stops threading the context is caught here rather than in whichever
+    document happens to use it.
+    """
+
+    def value(self, source: str) -> str:
+        return loads(source, serialization_options=VALUE)["a"]
+
+    def test_a_nested_call(self):
+        self.assertEqual(self.value("a = upper(lower(<<E\nx\nE\n))\n"), '${upper(lower("x"))}')
+
+    def test_a_later_argument(self):
+        self.assertEqual(self.value('a = join(",", <<E\nx\nE\n)\n'), '${join(",", "x")}')
+
+    def test_a_binary_operand(self):
+        self.assertEqual(self.value("a = b + <<E\nx\nE\n"), '${b + "x"}')
+
+    def test_a_conditional_branch(self):
+        self.assertEqual(self.value('a = c ? <<E\nx\nE\n : "z"\n'), '${c ? "x" : "z"}')
+
+    def test_an_indexed_tuple_inside_a_call(self):
+        self.assertEqual(self.value("a = upper([<<E\nx\nE\n][0])\n"), '${upper(["x"][0])}')
+
+    def test_an_interpolation_in_a_quoted_string(self):
+        self.assertEqual(self.value('a = "${upper(<<E\nx\nE\n)}"\n'), '${upper("x")}')
+
+    def test_none_of_them_leak_a_raw_newline(self):
+        for source in (
+            "a = upper(<<E\nx\ny\nE\n)\n",
+            "a = b + <<E\nx\ny\nE\n",
+            'a = c ? <<E\nx\ny\nE\n : "z"\n',
+        ):
+            with self.subTest(source=source):
+                self.assertNotIn("\n", self.value(source))
+
+
+class TestAStringLiteralInsideADirective(TestCase):
+    """#341: the delimiters were dropped, turning a literal into a reference."""
+
+    ESCAPED = 'a = "%{ if x == \\"y\\" }t%{ endif }"\n'
+    PLAIN = 'a = "%{ if x == "y" }t%{ endif }"\n'
+
+    def test_the_escaped_delimiters_survive_the_value_form(self):
+        self.assertEqual(
+            loads(self.ESCAPED, serialization_options=QUOTED)["a"], '%{ if x == \\"y\\" }t%{ endif }'
+        )
+
+    def test_the_plain_delimiters_survive_too(self):
+        # The spelling Terraform accepts; unchanged by this fix, asserted so it
+        # stays that way.
+        self.assertEqual(loads(self.PLAIN, serialization_options=QUOTED)["a"], '%{ if x == "y" }t%{ endif }')
+
+    def test_the_literal_is_not_reduced_to_a_reference(self):
+        # The point of the fix: `== y` would compare against a variable.
+        for source in (self.ESCAPED, self.PLAIN):
+            with self.subTest(source=source):
+                self.assertNotIn("== y ", loads(source, serialization_options=QUOTED)["a"])
+
+    def test_the_source_form_is_unchanged(self):
+        self.assertEqual(loads(self.ESCAPED)["a"], '"%{ if x == \\"y\\" }t%{ endif }"')
+
+    def test_a_directive_without_a_literal_is_unaffected(self):
+        self.assertEqual(
+            loads('a = "%{ if x }t%{ endif }"\n', serialization_options=QUOTED)["a"],
+            "%{ if x }t%{ endif }",
+        )
+
+
+class TestTheOtherModesAreUntouched(TestCase):
+    """Neither fix changes what the non-value modes emit, or the round trip."""
+
+    HEREDOC_IN_EXPRESSION = (
+        "a = upper(<<E\nx\nE\n)\n",
+        "a = trimspace(<<EOF\nhi\nEOF\n)\n",
+        "a = trimspace(<<EOF\nEOF\n)\n",
+        "a = b + <<E\nx\nE\n",
+    )
+
+    def test_a_heredoc_argument_keeps_its_quoted_source(self):
+        self.assertEqual(
+            loads("a = upper(<<E\nx\nE\n)\n", serialization_options=SOURCE)["a"], '${upper("x")}'
+        )
+
+    def test_default_options_keep_the_heredoc_as_quoted_source(self):
+        r"""The default still quotes the heredoc's own text, markers and all.
+
+        That form is not valid HCL -- Terraform rejects a quoted string split
+        over lines with "Invalid multi-line string", and a heredoc is a legal
+        argument as itself. Changing it needs the emitting side to give a
+        heredoc its own line first, which is #338; until then this asserts what
+        the default does rather than what it should, so the two fixes here stay
+        confined to the value form.
+        """
+        self.assertEqual(loads("a = upper(<<E\nx\nE\n)\n")["a"], '${upper("<<E\nx\nE")}')
+
+    def test_the_default_dict_still_round_trips(self):
+        # `dumps` has to read back whatever `loads` produced. Emitting the
+        # heredoc unquoted here breaks this, which is why that belongs with
+        # #338 rather than in this change.
+        for source in self.HEREDOC_IN_EXPRESSION:
+            with self.subTest(source=source):
+                written = dumps(loads(source, serialization_options=DEFAULT))
+                self.assertEqual(dumps(loads(written)), written)
+
+    def test_the_value_dict_still_round_trips(self):
+        for source in self.HEREDOC_IN_EXPRESSION:
+            with self.subTest(source=source):
+                written = dumps(loads(source, serialization_options=VALUE))
+                self.assertEqual(dumps(loads(written)), written)
+
+
+class TestAParenthesisedTermIsExpressionSource(TestCase):
+    r"""`(...)` is written as `${(...)}`, so what it wraps is expression source.
+
+    The term wrapped its result in `${(` and `)}` but serialized the inside as
+    a value, so every literal in it came back in Python's spelling rather than
+    HCL's. This was not confined to the value form: on the default options
+    `(true)` came back as `${(True)}` and `(null)` as `${(None)}`, and `dumps`
+    wrote those back as `(True)` and `(None)` -- references to variables
+    nobody declared, which OpenTofu v1.12.6 rejects with "Invalid reference"
+    where the source evaluates to `true` and `null`. A tuple or an object
+    inside the parentheses came back as a Python repr, which `dumps` could not
+    parse at all. With `strip_string_quotes`, `("s")` lost its quotes and became
+    `${(s)}`, another reference.
+
+    Checked against OpenTofu v1.12.6 (`jsonencode` of each local):
+
+        (true) -> "true"    (null) -> "null"    ("s") -> "\"s\""
+        ([1, "a"]) -> "[1,\"a\"]"               ({a = 1}) -> "{\"a\":1}"
+    """
+
+    CASES = {
+        "(true)": "${(true)}",
+        "(null)": "${(null)}",
+        '("s")': '${("s")}',
+        "(1)": "${(1)}",
+        '([1, "a"])': '${([1, "a"])}',
+        "(1 + 2)": "${(1 + 2)}",
+        "((true))": "${((true))}",
+    }
+
+    def test_default_options(self):
+        for source, expected in self.CASES.items():
+            with self.subTest(source=source):
+                self.assertEqual(loads(f"x = {source}\n")["x"], expected)
+
+    def test_the_value_form(self):
+        for source, expected in self.CASES.items():
+            with self.subTest(source=source):
+                self.assertEqual(loads(f"x = {source}\n", serialization_options=QUOTED)["x"], expected)
+
+    def test_an_object_inside_parentheses(self):
+        self.assertEqual(loads("x = ({a = 1})\n")["x"], "${({a = 1})}")
+
+    def test_inside_a_tuple(self):
+        self.assertEqual(loads("x = [(true)]\n")["x"], ["${(true)}"])
+
+    def test_the_round_trip_reads_back_the_same_value(self):
+        for source in (*self.CASES, "({a = 1})", "[(true)]"):
+            for options in (DEFAULT, QUOTED, VALUE):
+                with self.subTest(source=source, options=options):
+                    original = loads(f"x = {source}\n", serialization_options=options)
+                    written = dumps(original)
+                    self.assertEqual(loads(written, serialization_options=options), original)

test/unit/test_api.py

@@ -359,6 +359,10 @@ def test_empty_heredoc_as_an_object_value(self):
         self.assertEqual(loads("a = {\n  k = <<EOF\nEOF\n}\n"), {"a": {"k": '"<<EOF\nEOF"'}})
 
     def test_empty_heredoc_as_a_function_argument(self):
+        # Quoting the heredoc's own text is not valid HCL -- Terraform rejects
+        # a quoted string split over lines -- but emitting it as a heredoc
+        # needs the writer to give it its own line first, which is #338. This
+        # pins what the default does today so that fix is a deliberate step.
         self.assertEqual(loads("a = trimspace(<<EOF\nEOF\n)\n"), {"a": '${trimspace("<<EOF\nEOF")}'})