From 86803cdd8eb6474276b974793a70c6e5de429ad1 Mon Sep 17 00:00:00 2001 From: Tim Date: Tue, 1 Sep 2026 14:51:55 -0700 Subject: [PATCH 1/6] fix: stop concurrent parses from sharing one serialization context `serialize()` declared `context=SerializationContext()` as a default argument. Python evaluates a default once, at import, so every rule in the process shared a single mutable context -- and expressions.py, functions.py and indexing.py mutate it in place through `context.modify(inside_dollar_string=True)`. Nothing supplies a context at any public entry point: `api.serialize` calls `tree.serialize()`, and `NodeView.to_dict` calls `self._node.serialize(options=...)`. Both landed on that shared object. So a thread serializing a function call set `inside_dollar_string` for every other thread, and any tuple or object those threads were serializing came back as its inline HCL source -- `[1, 2, 3]` as the string `'[1, 2, 3]'`, `{a = 1}` as `'{a = 1}'`. No exception, just a different type, which a caller doing schema validation downstream then reports as a type error in the user's configuration. The structural rules in base.py now build a fresh context when called without one, and thread the context they were given into every child call -- eight sites that were dropping it and re-defaulting to the shared object. The rules below them already threaded it. The regression test interleaves 800 parses across 8 threads. Before this change, 400 of the 400 plain parses came back corrupted; the effect needs roughly 400 interleaved parses to show at all, which is why the count is what it is rather than a token handful. Other `serialize()` signatures keep the mutable default. They are no longer reachable with it -- every caller passes a context now -- but the defaults remain a trap for a future call site and would be worth removing separately. --- CHANGELOG.md | 4 ++- hcl2/rules/base.py | 28 ++++++++++--------- test/unit/test_thread_safety.py | 48 +++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 13 deletions(-) create mode 100644 test/unit/test_thread_safety.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 61dc143a..691e632e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## \[Unreleased\] -- Nothing yet. +### Fixed + +- Concurrent calls to `load`/`loads` no longer corrupt each other's values. `serialize()` declared `context=SerializationContext()` as a default argument, which Python evaluates once at import, so every rule in the process shared one mutable context — and the expression, function and indexing rules mutate it in place via `context.modify(inside_dollar_string=True)`. A thread serializing a function call therefore set that flag for every other thread, and a tuple or object being serialized elsewhere came back as its inline HCL source (`'[1, 2, 3]'`) rather than a list, with no exception raised. Measured before the fix: 400 of 400 interleaved parses corrupted. ## \[8.1.3\] - 2026-08-26 diff --git a/hcl2/rules/base.py b/hcl2/rules/base.py index 625bd835..377ad3bb 100644 --- a/hcl2/rules/base.py +++ b/hcl2/rules/base.py @@ -39,9 +39,10 @@ def expression(self) -> ExprTermRule: """Return the attribute value expression.""" return self._children[2] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to a single-entry dict.""" - return {self.identifier.serialize(options): self.expression.serialize(options)} + context = context if context is not None else SerializationContext() + return {self.identifier.serialize(options, context): self.expression.serialize(options, context)} class BodyRule(LarkRule): @@ -60,8 +61,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "body" - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to a dict, grouping blocks under their type name.""" + context = context if context is not None else SerializationContext() attribute_names = set() comments = [] inline_comments = [] @@ -70,14 +72,14 @@ def serialize(self, options=SerializationOptions(), context=SerializationContext for child in self._children: if isinstance(child, BlockRule): - name = child.labels[0].serialize(options) + name = child.labels[0].serialize(options, context) if name in attribute_names: raise RuntimeError(f"Attribute {name} is already defined.") - result[name].append(child.serialize(options)) + result[name].append(child.serialize(options, context)) if isinstance(child, AttributeRule): - attribute_names.add(child.identifier.serialize(options)) - result.update(child.serialize(options)) + attribute_names.add(child.identifier.serialize(options, context)) + result.update(child.serialize(options, context)) if options.with_comments: inline_comments.extend(child.expression.inline_comments()) comments.extend(child.expression.absorbed_comments()) @@ -111,9 +113,10 @@ def lark_name() -> str: """Return the grammar rule name.""" return "start" - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize by delegating to the body.""" - return self.body.serialize(options) + context = context if context is not None else SerializationContext() + return self.body.serialize(options, context) class BlockRule(LarkRule): @@ -147,14 +150,15 @@ def body(self) -> BodyRule: """Return the block body.""" return self._body - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to a nested dict with labels as keys.""" - result = self._body.serialize(options) + context = context if context is not None else SerializationContext() + result = self._body.serialize(options, context) if options.explicit_blocks: result.update({IS_BLOCK: True}) labels = self._labels for label in reversed(labels[1:]): - result = {label.serialize(options): result} + result = {label.serialize(options, context): result} return result diff --git a/test/unit/test_thread_safety.py b/test/unit/test_thread_safety.py new file mode 100644 index 00000000..a30ae5fb --- /dev/null +++ b/test/unit/test_thread_safety.py @@ -0,0 +1,48 @@ +# pylint: disable=C0103,C0114,C0115,C0116 +"""Concurrent calls to `loads` must not corrupt each other's values. + +`serialize()` declared `context=SerializationContext()` as a default argument. +Python evaluates that once, at import, so every rule in the process shared one +mutable context -- and `expressions.py`, `functions.py` and `indexing.py` mutate +it in place through `context.modify(inside_dollar_string=True)`. + +A thread serializing a function call therefore set `inside_dollar_string` for +every other thread, and any tuple or object those threads were serializing came +back as its inline HCL source (`'[1, 2, 3]'`) instead of a list. Silently: no +exception, just a different type. + +The structural rules now thread the context they were given and build a fresh +one when called without it, so a parse can no longer see another parse's state. +""" + +from concurrent.futures import ThreadPoolExecutor +from unittest import TestCase + +from hcl2.api import loads + +# Serializing a function call is what sets `inside_dollar_string`; the plain +# document is what reads it. Interleaving the two is what made it observable. +TOGGLES_CONTEXT = "z = f([1, 2, 3], {a = 1})\n" +PLAIN = "x = [1, 2, 3]\ny = {a = 1}\n" +EXPECTED = {"x": [1, 2, 3], "y": {"a": 1}} + + +class TestConcurrentLoads(TestCase): + maxDiff = None + + def test_a_concurrent_parse_does_not_change_another_parse_result(self): + def work(index): + if index % 2: + loads(TOGGLES_CONTEXT) + return None + return loads(PLAIN) + + # 800 interleaved parses. Below roughly 400 the threads do not overlap + # enough for the shared context to be observed at all -- measured + # against the unfixed code, which corrupts 0/50 at 100 and 400/400 here. + with ThreadPoolExecutor(max_workers=8) as pool: + results = [result for result in pool.map(work, range(800)) if result is not None] + + self.assertEqual(len(results), 400) + corrupted = [result for result in results if result != EXPECTED] + self.assertEqual(corrupted, [], f"{len(corrupted)} of {len(results)} parses were corrupted") From 6272aa6f79bf75ae4f9d61a05283e056101bf05e Mon Sep 17 00:00:00 2001 From: Tim Date: Tue, 1 Sep 2026 18:28:48 -0700 Subject: [PATCH 2/6] fix: remove the shared default context from every remaining rule Threading a context through the four structural rules fixed every parse the public API performs, because all of them enter at `StartRule`. It left the same declaration standing on 46 other methods: a default argument is evaluated once at import, `SerializationContext.modify` mutates in place, and so a caller serializing a rule directly -- not through `loads` -- still shared one object with every other thread doing the same. Nothing reachable from `loads` used those defaults, so this changes no value the library returns; it removes a trap rather than a live defect. Each of them now takes `context=None` and builds its own when it is not given one. The nine methods that never read the context keep the parameter but skip the construction. The new test walks the shipped rule modules and asserts every `context` parameter defaults to None, so a rule added later is covered without anyone remembering, and a second test asserts the walk actually found methods rather than passing over an empty list. --- CHANGELOG.md | 2 +- hcl2/rules/abstract.py | 8 +++--- hcl2/rules/containers.py | 15 +++++++---- hcl2/rules/directives.py | 18 ++++++++----- hcl2/rules/expressions.py | 18 ++++++++----- hcl2/rules/for_expressions.py | 12 ++++++--- hcl2/rules/functions.py | 6 +++-- hcl2/rules/indexing.py | 27 ++++++++++++------- hcl2/rules/literal_rules.py | 8 +++--- hcl2/rules/strings.py | 17 +++++++----- hcl2/rules/whitespace.py | 4 +-- test/unit/test_thread_safety.py | 47 +++++++++++++++++++++++++++++++++ 12 files changed, 133 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 691e632e..620c31ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed -- Concurrent calls to `load`/`loads` no longer corrupt each other's values. `serialize()` declared `context=SerializationContext()` as a default argument, which Python evaluates once at import, so every rule in the process shared one mutable context — and the expression, function and indexing rules mutate it in place via `context.modify(inside_dollar_string=True)`. A thread serializing a function call therefore set that flag for every other thread, and a tuple or object being serialized elsewhere came back as its inline HCL source (`'[1, 2, 3]'`) rather than a list, with no exception raised. Measured before the fix: 400 of 400 interleaved parses corrupted. +- Concurrent calls to `load`/`loads` no longer corrupt each other's values. `serialize()` declared `context=SerializationContext()` as a default argument, which Python evaluates once at import, so every rule in the process shared one mutable context — and the expression, function and indexing rules mutate it in place via `context.modify(inside_dollar_string=True)`. A thread serializing a function call therefore set that flag for every other thread, and a tuple or object being serialized elsewhere came back as its inline HCL source (`'[1, 2, 3]'`) rather than a list, with no exception raised. Measured before the fix: 400 of 400 interleaved parses corrupted. Every other rule's `serialize` carried the same default and is changed with them: the public API always enters at `StartRule`, so those were unreachable with a shared context, but a caller serializing a rule directly still had one. ## \[8.1.3\] - 2026-08-26 diff --git a/hcl2/rules/abstract.py b/hcl2/rules/abstract.py index c8ba063e..a67e84e6 100644 --- a/hcl2/rules/abstract.py +++ b/hcl2/rules/abstract.py @@ -6,7 +6,7 @@ from lark import Token, Tree from lark.tree import Meta -from hcl2.utils import SerializationContext, SerializationOptions +from hcl2.utils import SerializationOptions class LarkElement(ABC): @@ -36,7 +36,7 @@ def to_lark(self) -> Any: raise NotImplementedError() @abstractmethod - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize this element to a Python object (dict, list, str, etc.).""" raise NotImplementedError() @@ -63,7 +63,7 @@ def set_value(self, value: Any): """Set the raw value of this token.""" self._value = value - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize this token using its serialize_conversion callable.""" return self.serialize_conversion(self.value) @@ -89,7 +89,7 @@ class LarkRule(LarkElement, ABC): """ @abstractmethod - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize this rule and its children to a Python object.""" raise NotImplementedError() diff --git a/hcl2/rules/containers.py b/hcl2/rules/containers.py index 8b811ce8..ba6283a4 100644 --- a/hcl2/rules/containers.py +++ b/hcl2/rules/containers.py @@ -60,8 +60,9 @@ def elements(self) -> List[ExpressionRule]: """Return the expression elements of the tuple.""" return [child for child in self.children[1:-1] if isinstance(child, ExpressionRule)] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to a Python list or bracketed string.""" + context = context if context is not None else SerializationContext() if not options.wrap_tuples and not context.inside_dollar_string: return [element.serialize(options, context) for element in self.elements] @@ -93,8 +94,9 @@ def value(self) -> key_T: """Return the key value (identifier, string, or number).""" return self._children[0] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize the key, coercing numbers to strings.""" + context = context if context is not None else SerializationContext() result = self.value.serialize(options, context) # Object keys must be strings for JSON compatibility if isinstance(result, (int, float)): @@ -123,8 +125,9 @@ def expression(self) -> ExpressionRule: """Return the key expression.""" return self._children[0] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to '${expression}' string.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): result = str(self.expression.serialize(options, context)) if not context.inside_dollar_string: @@ -156,8 +159,9 @@ def expression(self): """Return the value expression.""" return self._children[2] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to a single-entry dict.""" + context = context if context is not None else SerializationContext() return {self.key.serialize(options, context): self.expression.serialize(options, context)} @@ -186,8 +190,9 @@ def elements(self) -> List[ObjectElemRule]: """Return the list of object element rules.""" return [child for child in self.children[1:-1] if isinstance(child, ObjectElemRule)] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to a Python dict or braced string.""" + context = context if context is not None else SerializationContext() if not options.wrap_objects and not context.inside_dollar_string: dict_result: dict = {} for element in self.elements: diff --git a/hcl2/rules/directives.py b/hcl2/rules/directives.py index 4a74fc66..3e4c6d4c 100644 --- a/hcl2/rules/directives.py +++ b/hcl2/rules/directives.py @@ -86,8 +86,9 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[4] is not None - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to %{ if EXPR } or %{~ if EXPR ~}.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): cond_str = self.condition.serialize(options, context) prefix = _strip_prefix(self.strip_open) @@ -125,7 +126,7 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[3] is not None - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to %{ else } or %{~ else ~}.""" prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) @@ -162,7 +163,7 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[3] is not None - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to %{ endif } or %{~ endif ~}.""" prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) @@ -247,8 +248,9 @@ def collection(self) -> ExpressionRule: """Return the collection expression after IN.""" return self._children[7] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to %{ for VAR in EXPR } or %{~ for VAR in EXPR ~}.""" + context = context if context is not None else SerializationContext() prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) with context.modify(inside_dollar_string=True): @@ -289,7 +291,7 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[3] is not None - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to %{ endfor } or %{~ endfor ~}.""" prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) @@ -339,8 +341,9 @@ def __init__( # pylint: disable=R0917 children.append(endif) super().__init__(children, meta) - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize the full if/else/endif directive.""" + context = context if context is not None else SerializationContext() result = self._if_start.serialize(options, context) for part in self._if_body: result += part.serialize(options, context) @@ -395,8 +398,9 @@ def __init__( children = [for_start, *body, endfor] super().__init__(children, meta) - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize the full for/endfor directive.""" + context = context if context is not None else SerializationContext() result = self._for_start.serialize(options, context) for part in self._body: result += part.serialize(options, context) diff --git a/hcl2/rules/expressions.py b/hcl2/rules/expressions.py index 15caa1c3..a11e7957 100644 --- a/hcl2/rules/expressions.py +++ b/hcl2/rules/expressions.py @@ -38,9 +38,10 @@ def _wrap_into_parentheses( self, value: str, _options=SerializationOptions(), - context=SerializationContext(), + context=None, ) -> str: """Wrap value in parentheses if inside a nested expression.""" + context = context if context is not None else SerializationContext() # do not wrap into parentheses if # 1. already wrapped or # 2. is top-level expression (unless explicitly wrapped) @@ -98,8 +99,9 @@ def expression(self) -> ExpressionRule: """Return the inner expression.""" return self._children[2] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize, handling parenthesized expression wrapping.""" + context = context if context is not None else SerializationContext() with context.modify(inside_parentheses=self.parentheses or context.inside_parentheses): result = self.expression.serialize(options, context) @@ -150,8 +152,9 @@ def if_false(self) -> ExpressionRule: """Return the false-branch expression.""" return self._children[8] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to ternary expression string.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): result = ( f"{self.condition.serialize(options, context)} " @@ -197,8 +200,9 @@ def expr_term(self) -> ExprTermRule: """Return the right-hand operand.""" return self._children[3] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to 'operator operand' string.""" + context = context if context is not None else SerializationContext() op_str = self.binary_operator.serialize(options, context) term_str = self.expr_term.serialize(options, context) return f"{op_str} {term_str}" @@ -264,8 +268,9 @@ def absorbed_comments(self): return trailing.to_list() or [] return [] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to 'lhs operator rhs' string.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): lhs = self.expr_term.serialize(options, context) operator = str(self.binary_term.binary_operator.serialize(options, context)).strip() @@ -301,8 +306,9 @@ def expr_term(self): """Return the operand.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to 'operator operand' string.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): operator = self.operator.rstrip() operand = self.expr_term.serialize(options, context) diff --git a/hcl2/rules/for_expressions.py b/hcl2/rules/for_expressions.py index 6013072e..3dd5f7da 100644 --- a/hcl2/rules/for_expressions.py +++ b/hcl2/rules/for_expressions.py @@ -92,8 +92,9 @@ def iterable(self) -> ExpressionRule: """Return the collection expression being iterated over.""" return self._children[8] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> str: + def serialize(self, options=SerializationOptions(), context=None) -> str: """Serialize to 'for key, value in collection : ' string.""" + context = context if context is not None else SerializationContext() result = "for " result += f"{self.first_iterator.serialize(options, context)}" @@ -127,8 +128,9 @@ def condition_expr(self) -> ExpressionRule: """Return the condition expression.""" return self._children[2] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> str: + def serialize(self, options=SerializationOptions(), context=None) -> str: """Serialize to 'if condition' string.""" + context = context if context is not None else SerializationContext() return f"if {self.condition_expr.serialize(options, context)}" @@ -191,8 +193,9 @@ def condition(self) -> Optional[ForCondRule]: """Return the optional condition rule.""" return self._children[6] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to '[for ... : expr]' string.""" + context = context if context is not None else SerializationContext() result = "[" with context.modify(inside_dollar_string=True): @@ -287,8 +290,9 @@ def condition(self) -> Optional[ForCondRule]: """Return the optional condition rule.""" return self._children[11] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to '{for ... : key => value}' string.""" + context = context if context is not None else SerializationContext() result = "{" with context.modify(inside_dollar_string=True): result += self.for_intro.serialize(options, context) diff --git a/hcl2/rules/functions.py b/hcl2/rules/functions.py index c48a7c2a..76a66841 100644 --- a/hcl2/rules/functions.py +++ b/hcl2/rules/functions.py @@ -50,8 +50,9 @@ def arguments(self) -> List[ExpressionRule]: """Return the list of expression arguments.""" return [child for child in self._children if isinstance(child, ExpressionRule)] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to a comma-separated argument string.""" + context = context if context is not None else SerializationContext() result = ", ".join(str(argument.serialize(options, context)) for argument in self.arguments) if self.has_ellipsis: result += " ..." @@ -90,8 +91,9 @@ def arguments(self) -> Optional[ArgumentsRule]: return child return None - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to 'func(args)' string.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): name = "::".join(identifier.serialize(options, context) for identifier in self.identifiers) args = self.arguments diff --git a/hcl2/rules/indexing.py b/hcl2/rules/indexing.py index 9bdab541..1db56cfa 100644 --- a/hcl2/rules/indexing.py +++ b/hcl2/rules/indexing.py @@ -44,8 +44,9 @@ def index(self): """Return the index token.""" return self.children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to '.N' string.""" + context = context if context is not None else SerializationContext() return f".{self.index.serialize(options, context)}" @@ -70,8 +71,9 @@ def index_expression(self): """Return the index expression inside the brackets.""" return self.children[2] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to '[expr]' string.""" + context = context if context is not None else SerializationContext() return f"[{self.index_expression.serialize(options, context)}]" def __init__(self, children, meta: Optional[Meta] = None): @@ -89,8 +91,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "index_expr_term" - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to 'expr[index]' string.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): expr = self.children[0].serialize(options, context) index = self.children[1].serialize(options, context) @@ -118,8 +121,9 @@ def identifier(self) -> IdentifierRule: """Return the accessed identifier.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to '.identifier' string.""" + context = context if context is not None else SerializationContext() return f".{self.identifier.serialize(options, context)}" @@ -146,8 +150,9 @@ def get_attr(self) -> GetAttrRule: """Return the attribute access rule.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to 'expr.attr' string.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): expr = self.expr_term.serialize(options, context) attr = self.get_attr.serialize(options, context) @@ -177,8 +182,9 @@ def get_attrs( """Return the trailing accessor chain.""" return self._children[1:] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to '.*...' string.""" + context = context if context is not None else SerializationContext() return ".*" + "".join(get_attr.serialize(options, context) for get_attr in self.get_attrs) @@ -202,8 +208,9 @@ def attr_splat(self) -> AttrSplatRule: """Return the attribute splat rule.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to 'expr.*...' string.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): expr = self.expr_term.serialize(options, context) splat = self.attr_splat.serialize(options, context) @@ -234,8 +241,9 @@ def get_attrs( """Return the trailing accessor chain.""" return self._children[1:] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to '[*]...' string.""" + context = context if context is not None else SerializationContext() return "[*]" + "".join(get_attr.serialize(options, context) for get_attr in self.get_attrs) @@ -259,8 +267,9 @@ def attr_splat(self) -> FullSplatRule: """Return the full splat rule.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to 'expr[*]...' string.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): expr = self.expr_term.serialize(options, context) splat = self.attr_splat.serialize(options, context) diff --git a/hcl2/rules/literal_rules.py b/hcl2/rules/literal_rules.py index 317d149e..0a62a878 100644 --- a/hcl2/rules/literal_rules.py +++ b/hcl2/rules/literal_rules.py @@ -17,7 +17,7 @@ def token(self) -> LarkToken: """Return the single token child.""" return self._children[0] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize by delegating to the token's own serialization.""" return self.token.serialize() @@ -41,8 +41,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "literal_value" - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to Python True, False, or None.""" + context = context if context is not None else SerializationContext() value = self.token.value if context.inside_dollar_string: return str(value) @@ -75,8 +76,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "float_lit" - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize, preserving scientific notation when configured.""" + context = context if context is not None else SerializationContext() value = self.token.value # Scientific notation (e.g. 1.23e5) cannot survive a Python float() # round-trip, so preserve it as a ${...} expression string. diff --git a/hcl2/rules/strings.py b/hcl2/rules/strings.py index 76ce0660..9cf5924b 100644 --- a/hcl2/rules/strings.py +++ b/hcl2/rules/strings.py @@ -65,8 +65,9 @@ def expression(self): """Return the interpolated expression.""" return self.children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to ${expression} string.""" + context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): return to_dollar_string(self.expression.serialize(options, context)) @@ -92,8 +93,9 @@ def content(self): """Return the content element (string chars, escape, interpolation, or directive).""" return self._children[0] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize this string part.""" + context = context if context is not None else SerializationContext() return self.content.serialize(options, context) @@ -112,7 +114,7 @@ def string_parts(self): """Return the list of string parts between quotes.""" return self.children[1:-1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to a quoted string. `strip_string_quotes` asks for the string's value rather than its @@ -121,6 +123,7 @@ def serialize(self, options=SerializationOptions(), context=SerializationContext and unquoting it there would produce something that is no longer valid HCL (`upper("x")` becoming `upper(x)`). """ + context = context if context is not None else SerializationContext() 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 @@ -161,8 +164,9 @@ def heredoc(self): """Return the raw heredoc token.""" return self.children[0] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize the heredoc, optionally stripping to a plain string.""" + context = context if context is not None else SerializationContext() heredoc = self.heredoc.serialize(options, context) if not options.preserve_heredocs: @@ -194,8 +198,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "heredoc_template_trim" - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize the trim heredoc, stripping common leading whitespace.""" + context = context if context is not None else SerializationContext() # See https://github.com/hashicorp/hcl2/blob/master/hcl/hclsyntax/spec.md#template-expressions # This is a special version of heredocs that are declared with "<<-" # This will calculate the minimum number of leading spaces in each line of a heredoc @@ -266,7 +271,7 @@ def inner_value(self) -> str: return raw[2:-2] return raw - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize preserving escaped-quote delimiters for round-trip fidelity. Inside template directive expressions, strings are delimited by \\" diff --git a/hcl2/rules/whitespace.py b/hcl2/rules/whitespace.py index cb43590b..314c0f23 100644 --- a/hcl2/rules/whitespace.py +++ b/hcl2/rules/whitespace.py @@ -6,7 +6,7 @@ from hcl2.rules.abstract import LarkRule from hcl2.rules.literal_rules import TokenRule from hcl2.rules.tokens import NL_OR_COMMENT -from hcl2.utils import SerializationContext, SerializationOptions +from hcl2.utils import SerializationOptions class NewLineOrCommentRule(TokenRule): @@ -22,7 +22,7 @@ def from_string(cls, string: str) -> "NewLineOrCommentRule": """Create an instance from a raw comment or newline string.""" return cls([NL_OR_COMMENT(string)]) # type: ignore[abstract] # pylint: disable=abstract-class-instantiated - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize(self, options=SerializationOptions(), context=None) -> Any: """Serialize to the raw comment/newline string.""" return "".join(child.serialize() for child in self._children) diff --git a/test/unit/test_thread_safety.py b/test/unit/test_thread_safety.py index a30ae5fb..8dbdeda8 100644 --- a/test/unit/test_thread_safety.py +++ b/test/unit/test_thread_safety.py @@ -16,6 +16,7 @@ """ from concurrent.futures import ThreadPoolExecutor +from importlib import import_module from unittest import TestCase from hcl2.api import loads @@ -46,3 +47,49 @@ def work(index): self.assertEqual(len(results), 400) corrupted = [result for result in results if result != EXPECTED] self.assertEqual(corrupted, [], f"{len(corrupted)} of {len(results)} parses were corrupted") + + +class TestNoDefaultContextIsShared(TestCase): + """No rule may declare a `SerializationContext()` default again. + + A default argument is evaluated once, at import, so any method that + declares one hands every caller in the process the same mutable object -- + and `SerializationContext.modify` mutates in place. Threading a context + through the four structural rules fixes the parses that start at + `StartRule`, which is every parse the public API performs, but it leaves + the trap armed for anything that serializes a rule directly. + + This walks the shipped rule modules rather than naming methods, so a rule + added later is covered without anyone remembering to add it here. + """ + + def _context_parameters(self): + import inspect + import pkgutil + + import hcl2.rules + + for module_info in pkgutil.iter_modules(hcl2.rules.__path__): + module = import_module(f"hcl2.rules.{module_info.name}") + for class_name, cls in vars(module).items(): + if not inspect.isclass(cls) or cls.__module__ != module.__name__: + continue + for method_name, method in vars(cls).items(): + if not inspect.isfunction(method): + continue + parameter = inspect.signature(method).parameters.get("context") + if parameter is not None: + yield f"{module.__name__}.{class_name}.{method_name}", parameter + + def test_every_context_parameter_defaults_to_none(self): + offenders = [ + name + for name, parameter in self._context_parameters() + if parameter.default is not None and parameter.default is not parameter.empty + ] + self.assertEqual(offenders, []) + + def test_the_walk_actually_found_the_methods(self): + # A test that asserts "no offenders" over an empty list would pass + # while inspecting nothing at all. + self.assertGreater(len(list(self._context_parameters())), 30) From 2c73410d239c09133ca671a6733fb5fbaba9d7cc Mon Sep 17 00:00:00 2001 From: Tim Date: Tue, 1 Sep 2026 18:44:44 -0700 Subject: [PATCH 3/6] test: prove the isolation without waiting for a race The existing regression test submits 800 parses and trusts that their critical sections overlap. That is how the defect was found and it is worth keeping, but it can only ever be evidence: on a single-core or differently-scheduled worker the same run passes over unfixed code because the two halves never meet. These force the overlap. One thread holds a mutated context open on a barrier while another serializes, and a second test asserts two serializations are handed different context objects at all. Both fail against the unfixed structural rules rather than depending on the scheduler to reveal it. Also pins why `options` may keep the shared default the context could not: nothing in the package assigns to it. If something ever does, the new test fails rather than the default quietly becoming a second cross-thread channel. --- test/unit/test_thread_safety.py | 121 +++++++++++++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-) diff --git a/test/unit/test_thread_safety.py b/test/unit/test_thread_safety.py index 8dbdeda8..c610f18d 100644 --- a/test/unit/test_thread_safety.py +++ b/test/unit/test_thread_safety.py @@ -15,17 +15,23 @@ one when called without it, so a parse can no longer see another parse's state. """ +import inspect +import threading from concurrent.futures import ThreadPoolExecutor +from dataclasses import asdict from importlib import import_module from unittest import TestCase -from hcl2.api import loads +from hcl2.api import loads, parses, serialize +from hcl2.rules.base import AttributeRule, BlockRule +from hcl2.utils import SerializationContext, SerializationOptions # Serializing a function call is what sets `inside_dollar_string`; the plain # document is what reads it. Interleaving the two is what made it observable. TOGGLES_CONTEXT = "z = f([1, 2, 3], {a = 1})\n" PLAIN = "x = [1, 2, 3]\ny = {a = 1}\n" EXPECTED = {"x": [1, 2, 3], "y": {"a": 1}} +BLOCK = 'resource "aws_instance" "web" {\n ami = "ami-1"\n}\n' class TestConcurrentLoads(TestCase): @@ -93,3 +99,116 @@ def test_the_walk_actually_found_the_methods(self): # A test that asserts "no offenders" over an empty list would pass # while inspecting nothing at all. self.assertGreater(len(list(self._context_parameters())), 30) + + +class TestIsolationWithoutRelyingOnScheduling(TestCase): + """The same property as above, proved without waiting for a race. + + `TestConcurrentLoads` submits 800 parses and trusts that their critical + sections overlap. That is how the defect was found, and it is worth + keeping, but it can only ever be evidence: on a single-core or + differently-scheduled machine the same run can pass over unfixed code + because the two halves never met. + + These force the overlap instead. One thread holds a mutated context open + on a barrier while another serializes, so a shared context is not + something the scheduler might reveal -- it is something the assertions + cannot avoid seeing. + """ + + SOURCE = "x = 1\n" + + def _spy_on_attribute_serialization(self, hook): + original = AttributeRule.serialize + + def spy(rule, options=SerializationOptions(), context=None): + context = context if context is not None else SerializationContext() + hook(context) + return original(rule, options, context) + + AttributeRule.serialize = spy # type: ignore[method-assign] + self.addCleanup(setattr, AttributeRule, "serialize", original) + + def test_two_serializations_are_handed_different_contexts(self): + seen = [] + self._spy_on_attribute_serialization(seen.append) + + serialize(parses(self.SOURCE)) + serialize(parses(self.SOURCE)) + + self.assertEqual(len(seen), 2) + self.assertIsNot(seen[0], seen[1]) + + def test_a_held_mutation_is_invisible_to_a_concurrent_parse(self): + barrier = threading.Barrier(2, timeout=30) + observed = {} + + def hook(context): + role = threading.current_thread().name + if role == "mutator": + # Hold the flag set across the other thread's serialization. + context.inside_dollar_string = True + barrier.wait() + observed["mutator-kept-its-own"] = context.inside_dollar_string + else: + barrier.wait() + observed["observer-saw"] = context.inside_dollar_string + + self._spy_on_attribute_serialization(hook) + + trees = [parses(self.SOURCE), parses(self.SOURCE)] + threads = [ + threading.Thread(target=serialize, args=(tree,), name=name) + for tree, name in zip(trees, ("mutator", "observer")) + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join(timeout=30) + + self.assertEqual(observed, {"mutator-kept-its-own": True, "observer-saw": False}) + + +class TestTheSharedOptionsDefaultIsNeverWritten(TestCase): + """`options` keeps a shared default, and that is only safe while it is read-only. + + The context had to stop being a default argument because the rules mutate + it in place. `SerializationOptions` is the same kind of object in the same + position, and the same reasoning would condemn it -- except that nothing + in the package assigns to it. This pins that difference, so the day + something does write to `options`, this fails rather than the shared + default quietly becoming a second cross-thread channel. + """ + + def _default_options(self): + return inspect.signature(AttributeRule.serialize).parameters["options"].default + + def test_every_call_that_omits_options_gets_the_same_object(self): + # Not an endorsement -- the premise the test below is guarding. Each + # method evaluates its own default once, at import, so the sharing is + # per method rather than global; either way two parses that omit + # `options` are handed one object between them. + seen = [] + original = BlockRule.serialize + + def spy(rule, options=SerializationOptions(), context=None): + seen.append(options) + return original(rule, options, context) + + BlockRule.serialize = spy # type: ignore[method-assign] + self.addCleanup(setattr, BlockRule, "serialize", original) + + loads(BLOCK) + loads(BLOCK) + + self.assertEqual(len(seen), 2) + self.assertIs(seen[0], seen[1]) + + def test_parsing_does_not_write_to_it(self): + before = asdict(self._default_options()) + + loads(PLAIN) + loads(TOGGLES_CONTEXT) + loads(PLAIN, serialization_options=SerializationOptions(with_meta=True)) + + self.assertEqual(asdict(self._default_options()), before) From c1cd0387517226eea937b8b9edd9a5ffe1fd97b4 Mon Sep 17 00:00:00 2001 From: Tim Date: Tue, 1 Sep 2026 19:04:57 -0700 Subject: [PATCH 4/6] fix: remove the shared default options as well `options=SerializationOptions()` is the same construct as the context was, in the same position: one object evaluated at import and handed to every caller that omits the argument. It was not a live defect, because nothing in the package assigns to a `SerializationOptions` -- but that is a property of today's code, not of the design, and it is reachable by any subclass or hook a consumer writes. Keeping it meant defending a distinction that rests on nobody ever writing to it. The earlier tests here asserted exactly that distinction. They are replaced by the guard the context already has: a walk over the shipped rule modules asserting every `options` parameter defaults to None. It immediately earned it, catching one the change had missed -- `NewLineOrCommentRule.to_list` declares its default with an annotation, which the mechanical pass did not match. --- CHANGELOG.md | 2 +- hcl2/rules/abstract.py | 8 +-- hcl2/rules/base.py | 12 ++-- hcl2/rules/containers.py | 15 +++-- hcl2/rules/directives.py | 18 +++--- hcl2/rules/expressions.py | 17 +++-- hcl2/rules/for_expressions.py | 12 ++-- hcl2/rules/functions.py | 6 +- hcl2/rules/indexing.py | 27 +++++--- hcl2/rules/literal_rules.py | 7 ++- hcl2/rules/strings.py | 18 ++++-- hcl2/rules/whitespace.py | 5 +- test/unit/test_thread_safety.py | 106 ++++++++++++++------------------ 13 files changed, 138 insertions(+), 115 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 620c31ad..b09ab16e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed -- Concurrent calls to `load`/`loads` no longer corrupt each other's values. `serialize()` declared `context=SerializationContext()` as a default argument, which Python evaluates once at import, so every rule in the process shared one mutable context — and the expression, function and indexing rules mutate it in place via `context.modify(inside_dollar_string=True)`. A thread serializing a function call therefore set that flag for every other thread, and a tuple or object being serialized elsewhere came back as its inline HCL source (`'[1, 2, 3]'`) rather than a list, with no exception raised. Measured before the fix: 400 of 400 interleaved parses corrupted. Every other rule's `serialize` carried the same default and is changed with them: the public API always enters at `StartRule`, so those were unreachable with a shared context, but a caller serializing a rule directly still had one. +- Concurrent calls to `load`/`loads` no longer corrupt each other's values. `serialize()` declared `context=SerializationContext()` as a default argument, which Python evaluates once at import, so every rule in the process shared one mutable context — and the expression, function and indexing rules mutate it in place via `context.modify(inside_dollar_string=True)`. A thread serializing a function call therefore set that flag for every other thread, and a tuple or object being serialized elsewhere came back as its inline HCL source (`'[1, 2, 3]'`) rather than a list, with no exception raised. Measured before the fix: 400 of 400 interleaved parses corrupted. Every other rule's `serialize` carried the same default and is changed with them: the public API always enters at `StartRule`, so those were unreachable with a shared context, but a caller serializing a rule directly still had one. `options` loses its shared default too. Nothing in the package assigns to a `SerializationOptions`, so that one was not a live defect, but it is the same construct in the same position -- one mutable object handed to every caller that omits the argument, reachable by any subclass or hook a consumer writes. ## \[8.1.3\] - 2026-08-26 diff --git a/hcl2/rules/abstract.py b/hcl2/rules/abstract.py index a67e84e6..81ce29e3 100644 --- a/hcl2/rules/abstract.py +++ b/hcl2/rules/abstract.py @@ -6,8 +6,6 @@ from lark import Token, Tree from lark.tree import Meta -from hcl2.utils import SerializationOptions - class LarkElement(ABC): """Base class for all elements in the LarkElement tree.""" @@ -36,7 +34,7 @@ def to_lark(self) -> Any: raise NotImplementedError() @abstractmethod - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize this element to a Python object (dict, list, str, etc.).""" raise NotImplementedError() @@ -63,7 +61,7 @@ def set_value(self, value: Any): """Set the raw value of this token.""" self._value = value - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize this token using its serialize_conversion callable.""" return self.serialize_conversion(self.value) @@ -89,7 +87,7 @@ class LarkRule(LarkElement, ABC): """ @abstractmethod - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize this rule and its children to a Python object.""" raise NotImplementedError() diff --git a/hcl2/rules/base.py b/hcl2/rules/base.py index 377ad3bb..12599100 100644 --- a/hcl2/rules/base.py +++ b/hcl2/rules/base.py @@ -39,8 +39,9 @@ def expression(self) -> ExprTermRule: """Return the attribute value expression.""" return self._children[2] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to a single-entry dict.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() return {self.identifier.serialize(options, context): self.expression.serialize(options, context)} @@ -61,8 +62,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "body" - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to a dict, grouping blocks under their type name.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() attribute_names = set() comments = [] @@ -113,8 +115,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "start" - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize by delegating to the body.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() return self.body.serialize(options, context) @@ -150,8 +153,9 @@ def body(self) -> BodyRule: """Return the block body.""" return self._body - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to a nested dict with labels as keys.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() result = self._body.serialize(options, context) if options.explicit_blocks: diff --git a/hcl2/rules/containers.py b/hcl2/rules/containers.py index ba6283a4..935cf310 100644 --- a/hcl2/rules/containers.py +++ b/hcl2/rules/containers.py @@ -60,8 +60,9 @@ def elements(self) -> List[ExpressionRule]: """Return the expression elements of the tuple.""" return [child for child in self.children[1:-1] if isinstance(child, ExpressionRule)] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to a Python list or bracketed string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() if not options.wrap_tuples and not context.inside_dollar_string: return [element.serialize(options, context) for element in self.elements] @@ -94,8 +95,9 @@ def value(self) -> key_T: """Return the key value (identifier, string, or number).""" return self._children[0] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize the key, coercing numbers to strings.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() result = self.value.serialize(options, context) # Object keys must be strings for JSON compatibility @@ -125,8 +127,9 @@ def expression(self) -> ExpressionRule: """Return the key expression.""" return self._children[0] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to '${expression}' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): result = str(self.expression.serialize(options, context)) @@ -159,8 +162,9 @@ def expression(self): """Return the value expression.""" return self._children[2] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to a single-entry dict.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() return {self.key.serialize(options, context): self.expression.serialize(options, context)} @@ -190,8 +194,9 @@ def elements(self) -> List[ObjectElemRule]: """Return the list of object element rules.""" return [child for child in self.children[1:-1] if isinstance(child, ObjectElemRule)] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to a Python dict or braced string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() if not options.wrap_objects and not context.inside_dollar_string: dict_result: dict = {} diff --git a/hcl2/rules/directives.py b/hcl2/rules/directives.py index 3e4c6d4c..67f0b336 100644 --- a/hcl2/rules/directives.py +++ b/hcl2/rules/directives.py @@ -86,8 +86,9 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[4] is not None - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to %{ if EXPR } or %{~ if EXPR ~}.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): cond_str = self.condition.serialize(options, context) @@ -126,7 +127,7 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[3] is not None - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to %{ else } or %{~ else ~}.""" prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) @@ -163,7 +164,7 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[3] is not None - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to %{ endif } or %{~ endif ~}.""" prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) @@ -248,8 +249,9 @@ def collection(self) -> ExpressionRule: """Return the collection expression after IN.""" return self._children[7] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to %{ for VAR in EXPR } or %{~ for VAR in EXPR ~}.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) @@ -291,7 +293,7 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[3] is not None - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to %{ endfor } or %{~ endfor ~}.""" prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) @@ -341,8 +343,9 @@ def __init__( # pylint: disable=R0917 children.append(endif) super().__init__(children, meta) - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize the full if/else/endif directive.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() result = self._if_start.serialize(options, context) for part in self._if_body: @@ -398,8 +401,9 @@ def __init__( children = [for_start, *body, endfor] super().__init__(children, meta) - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize the full for/endfor directive.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() result = self._for_start.serialize(options, context) for part in self._body: diff --git a/hcl2/rules/expressions.py b/hcl2/rules/expressions.py index a11e7957..96c63fcb 100644 --- a/hcl2/rules/expressions.py +++ b/hcl2/rules/expressions.py @@ -37,7 +37,7 @@ def __init__(self, children, meta: Optional[Meta] = None, parentheses: bool = Fa def _wrap_into_parentheses( self, value: str, - _options=SerializationOptions(), + _options=None, context=None, ) -> str: """Wrap value in parentheses if inside a nested expression.""" @@ -99,8 +99,9 @@ def expression(self) -> ExpressionRule: """Return the inner expression.""" return self._children[2] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize, handling parenthesized expression wrapping.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_parentheses=self.parentheses or context.inside_parentheses): result = self.expression.serialize(options, context) @@ -152,8 +153,9 @@ def if_false(self) -> ExpressionRule: """Return the false-branch expression.""" return self._children[8] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to ternary expression string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): result = ( @@ -200,8 +202,9 @@ def expr_term(self) -> ExprTermRule: """Return the right-hand operand.""" return self._children[3] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to 'operator operand' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() op_str = self.binary_operator.serialize(options, context) term_str = self.expr_term.serialize(options, context) @@ -268,8 +271,9 @@ def absorbed_comments(self): return trailing.to_list() or [] return [] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to 'lhs operator rhs' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): lhs = self.expr_term.serialize(options, context) @@ -306,8 +310,9 @@ def expr_term(self): """Return the operand.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to 'operator operand' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): operator = self.operator.rstrip() diff --git a/hcl2/rules/for_expressions.py b/hcl2/rules/for_expressions.py index 3dd5f7da..6ce9643b 100644 --- a/hcl2/rules/for_expressions.py +++ b/hcl2/rules/for_expressions.py @@ -92,8 +92,9 @@ def iterable(self) -> ExpressionRule: """Return the collection expression being iterated over.""" return self._children[8] - def serialize(self, options=SerializationOptions(), context=None) -> str: + def serialize(self, options=None, context=None) -> str: """Serialize to 'for key, value in collection : ' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() result = "for " @@ -128,8 +129,9 @@ def condition_expr(self) -> ExpressionRule: """Return the condition expression.""" return self._children[2] - def serialize(self, options=SerializationOptions(), context=None) -> str: + def serialize(self, options=None, context=None) -> str: """Serialize to 'if condition' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() return f"if {self.condition_expr.serialize(options, context)}" @@ -193,8 +195,9 @@ def condition(self) -> Optional[ForCondRule]: """Return the optional condition rule.""" return self._children[6] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to '[for ... : expr]' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() result = "[" @@ -290,8 +293,9 @@ def condition(self) -> Optional[ForCondRule]: """Return the optional condition rule.""" return self._children[11] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to '{for ... : key => value}' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() result = "{" with context.modify(inside_dollar_string=True): diff --git a/hcl2/rules/functions.py b/hcl2/rules/functions.py index 76a66841..2665be6a 100644 --- a/hcl2/rules/functions.py +++ b/hcl2/rules/functions.py @@ -50,8 +50,9 @@ def arguments(self) -> List[ExpressionRule]: """Return the list of expression arguments.""" return [child for child in self._children if isinstance(child, ExpressionRule)] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to a comma-separated argument string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() result = ", ".join(str(argument.serialize(options, context)) for argument in self.arguments) if self.has_ellipsis: @@ -91,8 +92,9 @@ def arguments(self) -> Optional[ArgumentsRule]: return child return None - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to 'func(args)' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): name = "::".join(identifier.serialize(options, context) for identifier in self.identifiers) diff --git a/hcl2/rules/indexing.py b/hcl2/rules/indexing.py index 1db56cfa..974c0a62 100644 --- a/hcl2/rules/indexing.py +++ b/hcl2/rules/indexing.py @@ -44,8 +44,9 @@ def index(self): """Return the index token.""" return self.children[1] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to '.N' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() return f".{self.index.serialize(options, context)}" @@ -71,8 +72,9 @@ def index_expression(self): """Return the index expression inside the brackets.""" return self.children[2] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to '[expr]' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() return f"[{self.index_expression.serialize(options, context)}]" @@ -91,8 +93,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "index_expr_term" - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to 'expr[index]' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): expr = self.children[0].serialize(options, context) @@ -121,8 +124,9 @@ def identifier(self) -> IdentifierRule: """Return the accessed identifier.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to '.identifier' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() return f".{self.identifier.serialize(options, context)}" @@ -150,8 +154,9 @@ def get_attr(self) -> GetAttrRule: """Return the attribute access rule.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to 'expr.attr' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): expr = self.expr_term.serialize(options, context) @@ -182,8 +187,9 @@ def get_attrs( """Return the trailing accessor chain.""" return self._children[1:] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to '.*...' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() return ".*" + "".join(get_attr.serialize(options, context) for get_attr in self.get_attrs) @@ -208,8 +214,9 @@ def attr_splat(self) -> AttrSplatRule: """Return the attribute splat rule.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to 'expr.*...' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): expr = self.expr_term.serialize(options, context) @@ -241,8 +248,9 @@ def get_attrs( """Return the trailing accessor chain.""" return self._children[1:] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to '[*]...' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() return "[*]" + "".join(get_attr.serialize(options, context) for get_attr in self.get_attrs) @@ -267,8 +275,9 @@ def attr_splat(self) -> FullSplatRule: """Return the full splat rule.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to 'expr[*]...' string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): expr = self.expr_term.serialize(options, context) diff --git a/hcl2/rules/literal_rules.py b/hcl2/rules/literal_rules.py index 0a62a878..197c00d3 100644 --- a/hcl2/rules/literal_rules.py +++ b/hcl2/rules/literal_rules.py @@ -17,7 +17,7 @@ def token(self) -> LarkToken: """Return the single token child.""" return self._children[0] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize by delegating to the token's own serialization.""" return self.token.serialize() @@ -41,7 +41,7 @@ def lark_name() -> str: """Return the grammar rule name.""" return "literal_value" - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to Python True, False, or None.""" context = context if context is not None else SerializationContext() value = self.token.value @@ -76,8 +76,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "float_lit" - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize, preserving scientific notation when configured.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() value = self.token.value # Scientific notation (e.g. 1.23e5) cannot survive a Python float() diff --git a/hcl2/rules/strings.py b/hcl2/rules/strings.py index 9cf5924b..ffa6989d 100644 --- a/hcl2/rules/strings.py +++ b/hcl2/rules/strings.py @@ -65,8 +65,9 @@ def expression(self): """Return the interpolated expression.""" return self.children[1] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to ${expression} string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() with context.modify(inside_dollar_string=True): return to_dollar_string(self.expression.serialize(options, context)) @@ -93,8 +94,9 @@ def content(self): """Return the content element (string chars, escape, interpolation, or directive).""" return self._children[0] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize this string part.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() return self.content.serialize(options, context) @@ -114,7 +116,7 @@ def string_parts(self): """Return the list of string parts between quotes.""" return self.children[1:-1] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to a quoted string. `strip_string_quotes` asks for the string's value rather than its @@ -123,6 +125,7 @@ def serialize(self, options=SerializationOptions(), context=None) -> Any: and unquoting it there would produce something that is no longer valid HCL (`upper("x")` becoming `upper(x)`). """ + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() if options.strip_string_quotes and not context.inside_dollar_string: return "".join( @@ -164,8 +167,9 @@ def heredoc(self): """Return the raw heredoc token.""" return self.children[0] - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize the heredoc, optionally stripping to a plain string.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() heredoc = self.heredoc.serialize(options, context) @@ -198,8 +202,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "heredoc_template_trim" - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize the trim heredoc, stripping common leading whitespace.""" + options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() # See https://github.com/hashicorp/hcl2/blob/master/hcl/hclsyntax/spec.md#template-expressions # This is a special version of heredocs that are declared with "<<-" @@ -271,13 +276,14 @@ def inner_value(self) -> str: return raw[2:-2] return raw - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize preserving escaped-quote delimiters for round-trip fidelity. 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. """ + options = options if options is not None else SerializationOptions() raw = self.raw_value if options.strip_string_quotes: return self.inner_value diff --git a/hcl2/rules/whitespace.py b/hcl2/rules/whitespace.py index 314c0f23..9ed71cee 100644 --- a/hcl2/rules/whitespace.py +++ b/hcl2/rules/whitespace.py @@ -22,7 +22,7 @@ def from_string(cls, string: str) -> "NewLineOrCommentRule": """Create an instance from a raw comment or newline string.""" return cls([NL_OR_COMMENT(string)]) # type: ignore[abstract] # pylint: disable=abstract-class-instantiated - def serialize(self, options=SerializationOptions(), context=None) -> Any: + def serialize(self, options=None, context=None) -> Any: """Serialize to the raw comment/newline string.""" return "".join(child.serialize() for child in self._children) @@ -36,8 +36,9 @@ def is_inline(self) -> bool: """ return not self.serialize().startswith("\n") - def to_list(self, options: SerializationOptions = SerializationOptions()) -> Optional[List[dict]]: + def to_list(self, options: Optional[SerializationOptions] = None) -> Optional[List[dict]]: """Extract comment objects, or None if only a newline.""" + options = options if options is not None else SerializationOptions() raw = self.serialize(options) if raw == "\n": return None diff --git a/test/unit/test_thread_safety.py b/test/unit/test_thread_safety.py index c610f18d..5a9cef10 100644 --- a/test/unit/test_thread_safety.py +++ b/test/unit/test_thread_safety.py @@ -18,12 +18,11 @@ import inspect import threading from concurrent.futures import ThreadPoolExecutor -from dataclasses import asdict from importlib import import_module from unittest import TestCase from hcl2.api import loads, parses, serialize -from hcl2.rules.base import AttributeRule, BlockRule +from hcl2.rules.base import AttributeRule from hcl2.utils import SerializationContext, SerializationOptions # Serializing a function call is what sets `inside_dollar_string`; the plain @@ -34,6 +33,31 @@ BLOCK = 'resource "aws_instance" "web" {\n ami = "ami-1"\n}\n' +def _parameters_named(*names): + """Yield every parameter with one of *names* across the shipped rule modules. + + Walking the modules rather than naming methods means a rule added later is + covered without anyone remembering to come back here. + """ + import pkgutil + + import hcl2.rules + + for module_info in pkgutil.iter_modules(hcl2.rules.__path__): + module = import_module(f"hcl2.rules.{module_info.name}") + for class_name, cls in vars(module).items(): + if not inspect.isclass(cls) or cls.__module__ != module.__name__: + continue + for method_name, method in vars(cls).items(): + if not inspect.isfunction(method): + continue + parameters = inspect.signature(method).parameters + for name in names: + parameter = parameters.get(name) + if parameter is not None: + yield f"{module.__name__}.{class_name}.{method_name}({name})", parameter + + class TestConcurrentLoads(TestCase): maxDiff = None @@ -69,28 +93,10 @@ class TestNoDefaultContextIsShared(TestCase): added later is covered without anyone remembering to add it here. """ - def _context_parameters(self): - import inspect - import pkgutil - - import hcl2.rules - - for module_info in pkgutil.iter_modules(hcl2.rules.__path__): - module = import_module(f"hcl2.rules.{module_info.name}") - for class_name, cls in vars(module).items(): - if not inspect.isclass(cls) or cls.__module__ != module.__name__: - continue - for method_name, method in vars(cls).items(): - if not inspect.isfunction(method): - continue - parameter = inspect.signature(method).parameters.get("context") - if parameter is not None: - yield f"{module.__name__}.{class_name}.{method_name}", parameter - def test_every_context_parameter_defaults_to_none(self): offenders = [ name - for name, parameter in self._context_parameters() + for name, parameter in _parameters_named("context") if parameter.default is not None and parameter.default is not parameter.empty ] self.assertEqual(offenders, []) @@ -98,7 +104,7 @@ def test_every_context_parameter_defaults_to_none(self): def test_the_walk_actually_found_the_methods(self): # A test that asserts "no offenders" over an empty list would pass # while inspecting nothing at all. - self.assertGreater(len(list(self._context_parameters())), 30) + self.assertGreater(len(list(_parameters_named("context"))), 30) class TestIsolationWithoutRelyingOnScheduling(TestCase): @@ -169,46 +175,24 @@ def hook(context): self.assertEqual(observed, {"mutator-kept-its-own": True, "observer-saw": False}) -class TestTheSharedOptionsDefaultIsNeverWritten(TestCase): - """`options` keeps a shared default, and that is only safe while it is read-only. +class TestNoDefaultOptionsIsShared(TestCase): + """`options` carried the same declaration, and loses it for the same reason. - The context had to stop being a default argument because the rules mutate - it in place. `SerializationOptions` is the same kind of object in the same - position, and the same reasoning would condemn it -- except that nothing - in the package assigns to it. This pins that difference, so the day - something does write to `options`, this fails rather than the shared - default quietly becoming a second cross-thread channel. + Nothing in the package assigns to a `SerializationOptions`, so the shared + default was not a live defect the way the context was. It was still the + same construct in the same position: one mutable object handed to every + caller that omits the argument, reachable by any subclass or hook a + consumer writes. Keeping it would have meant defending a distinction that + rests on nobody ever writing to it. """ - def _default_options(self): - return inspect.signature(AttributeRule.serialize).parameters["options"].default - - def test_every_call_that_omits_options_gets_the_same_object(self): - # Not an endorsement -- the premise the test below is guarding. Each - # method evaluates its own default once, at import, so the sharing is - # per method rather than global; either way two parses that omit - # `options` are handed one object between them. - seen = [] - original = BlockRule.serialize - - def spy(rule, options=SerializationOptions(), context=None): - seen.append(options) - return original(rule, options, context) - - BlockRule.serialize = spy # type: ignore[method-assign] - self.addCleanup(setattr, BlockRule, "serialize", original) - - loads(BLOCK) - loads(BLOCK) - - self.assertEqual(len(seen), 2) - self.assertIs(seen[0], seen[1]) - - def test_parsing_does_not_write_to_it(self): - before = asdict(self._default_options()) - - loads(PLAIN) - loads(TOGGLES_CONTEXT) - loads(PLAIN, serialization_options=SerializationOptions(with_meta=True)) + def test_every_options_parameter_defaults_to_none(self): + offenders = [ + name + for name, parameter in _parameters_named("options", "_options") + if parameter.default is not None and parameter.default is not parameter.empty + ] + self.assertEqual(offenders, []) - self.assertEqual(asdict(self._default_options()), before) + def test_the_walk_actually_found_the_methods(self): + self.assertGreater(len(list(_parameters_named("options", "_options"))), 30) From 29d8d7d1e2bf8dbb0fdcd2510e8f2d21822e7899 Mon Sep 17 00:00:00 2001 From: Tim Date: Tue, 1 Sep 2026 19:23:35 -0700 Subject: [PATCH 5/6] test: close the gaps in the shared-default guards The walk backing both guards missed three things. It used `iter_modules`, so a future subpackage would have been invisible; it looked at `hcl2.rules` alone, though the pattern is package-wide; and it tested `inspect.isfunction` against what `vars(cls)` returns, which is the descriptor for a staticmethod or classmethod rather than a function. That last one was not hypothetical: `StringRule._serialize_part_as_value` takes a context and was never inspected. It walks `hcl2` recursively and unwraps `__func__` now, taking the count from 50 to 51 and 59. The floors were `> 30` against real counts of 51 and 59, so a 40% loss of coverage would have passed. They now sit just under the true numbers. Defaulting to None was also only half the invariant: a rule that takes `None` and dereferences it without constructing one passes every guard and then raises `AttributeError` for exactly the direct caller this protects. Both parameters are annotated `Optional[...]`, which makes that a mypy error -- confirmed by deleting a guard and watching three `union-attr` errors appear -- and a test asserts the annotation is there, for the parameters that default to None rather than the required ones. Finally, thirteen test doubles still declared the mutable defaults. They are the nearest template anyone copies, so they now match. --- CHANGELOG.md | 2 +- hcl2/rules/abstract.py | 14 ++++-- hcl2/rules/base.py | 16 +++++-- hcl2/rules/containers.py | 20 ++++++-- hcl2/rules/directives.py | 28 +++++++++--- hcl2/rules/expressions.py | 24 +++++++--- hcl2/rules/for_expressions.py | 16 +++++-- hcl2/rules/functions.py | 8 +++- hcl2/rules/indexing.py | 36 +++++++++++---- hcl2/rules/literal_rules.py | 14 ++++-- hcl2/rules/strings.py | 26 ++++++++--- hcl2/rules/whitespace.py | 6 ++- test/unit/query/test_base.py | 4 +- test/unit/rules/test_abstract.py | 3 +- test/unit/rules/test_base.py | 4 +- test/unit/rules/test_containers.py | 2 +- test/unit/rules/test_expressions.py | 8 ++-- test/unit/rules/test_for_expressions.py | 2 +- test/unit/rules/test_functions.py | 4 +- test/unit/rules/test_strings.py | 2 +- test/unit/rules/test_whitespace.py | 3 +- test/unit/test_thread_safety.py | 61 +++++++++++++++++++++---- test/unit/test_walk.py | 3 +- 23 files changed, 223 insertions(+), 83 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b09ab16e..22e87b80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed -- Concurrent calls to `load`/`loads` no longer corrupt each other's values. `serialize()` declared `context=SerializationContext()` as a default argument, which Python evaluates once at import, so every rule in the process shared one mutable context — and the expression, function and indexing rules mutate it in place via `context.modify(inside_dollar_string=True)`. A thread serializing a function call therefore set that flag for every other thread, and a tuple or object being serialized elsewhere came back as its inline HCL source (`'[1, 2, 3]'`) rather than a list, with no exception raised. Measured before the fix: 400 of 400 interleaved parses corrupted. Every other rule's `serialize` carried the same default and is changed with them: the public API always enters at `StartRule`, so those were unreachable with a shared context, but a caller serializing a rule directly still had one. `options` loses its shared default too. Nothing in the package assigns to a `SerializationOptions`, so that one was not a live defect, but it is the same construct in the same position -- one mutable object handed to every caller that omits the argument, reachable by any subclass or hook a consumer writes. +- Concurrent calls to `load`/`loads` no longer corrupt each other's values. `serialize()` declared `context=SerializationContext()` as a default argument, which Python evaluates once at import, so every rule in the process shared one mutable context — and the expression, function and indexing rules mutate it in place via `context.modify(inside_dollar_string=True)`. A thread serializing a function call therefore set that flag for every other thread, and a tuple or object being serialized elsewhere came back as its inline HCL source (`'[1, 2, 3]'`) rather than a list, with no exception raised. Measured before the fix: 400 of 400 interleaved parses corrupted. Every other rule's `serialize` carried the same default and is changed with them: the public API always enters at `StartRule`, so those were unreachable with a shared context, but a caller serializing a rule directly still had one. Both parameters are annotated `Optional[...]`, so a rule that takes `None` and then dereferences it without constructing one is a mypy error rather than an `AttributeError` for the direct caller this protects. `options` loses its shared default too. Nothing in the package assigns to a `SerializationOptions`, so that one was not a live defect, but it is the same construct in the same position -- one mutable object handed to every caller that omits the argument, reachable by any subclass or hook a consumer writes. ## \[8.1.3\] - 2026-08-26 diff --git a/hcl2/rules/abstract.py b/hcl2/rules/abstract.py index 81ce29e3..dcbde90d 100644 --- a/hcl2/rules/abstract.py +++ b/hcl2/rules/abstract.py @@ -6,6 +6,8 @@ from lark import Token, Tree from lark.tree import Meta +from hcl2.utils import SerializationContext, SerializationOptions + class LarkElement(ABC): """Base class for all elements in the LarkElement tree.""" @@ -34,7 +36,9 @@ def to_lark(self) -> Any: raise NotImplementedError() @abstractmethod - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize this element to a Python object (dict, list, str, etc.).""" raise NotImplementedError() @@ -61,7 +65,9 @@ def set_value(self, value: Any): """Set the raw value of this token.""" self._value = value - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize this token using its serialize_conversion callable.""" return self.serialize_conversion(self.value) @@ -87,7 +93,9 @@ class LarkRule(LarkElement, ABC): """ @abstractmethod - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize this rule and its children to a Python object.""" raise NotImplementedError() diff --git a/hcl2/rules/base.py b/hcl2/rules/base.py index 12599100..a276d662 100644 --- a/hcl2/rules/base.py +++ b/hcl2/rules/base.py @@ -39,7 +39,9 @@ def expression(self) -> ExprTermRule: """Return the attribute value expression.""" return self._children[2] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to a single-entry dict.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -62,7 +64,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "body" - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to a dict, grouping blocks under their type name.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -115,7 +119,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "start" - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize by delegating to the body.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -153,7 +159,9 @@ def body(self) -> BodyRule: """Return the block body.""" return self._body - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to a nested dict with labels as keys.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() diff --git a/hcl2/rules/containers.py b/hcl2/rules/containers.py index 935cf310..e0d3c908 100644 --- a/hcl2/rules/containers.py +++ b/hcl2/rules/containers.py @@ -60,7 +60,9 @@ def elements(self) -> List[ExpressionRule]: """Return the expression elements of the tuple.""" return [child for child in self.children[1:-1] if isinstance(child, ExpressionRule)] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to a Python list or bracketed string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -95,7 +97,9 @@ def value(self) -> key_T: """Return the key value (identifier, string, or number).""" return self._children[0] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize the key, coercing numbers to strings.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -127,7 +131,9 @@ def expression(self) -> ExpressionRule: """Return the key expression.""" return self._children[0] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to '${expression}' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -162,7 +168,9 @@ def expression(self): """Return the value expression.""" return self._children[2] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to a single-entry dict.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -194,7 +202,9 @@ def elements(self) -> List[ObjectElemRule]: """Return the list of object element rules.""" return [child for child in self.children[1:-1] if isinstance(child, ObjectElemRule)] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to a Python dict or braced string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() diff --git a/hcl2/rules/directives.py b/hcl2/rules/directives.py index 67f0b336..0038443a 100644 --- a/hcl2/rules/directives.py +++ b/hcl2/rules/directives.py @@ -86,7 +86,9 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[4] is not None - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to %{ if EXPR } or %{~ if EXPR ~}.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -127,7 +129,9 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[3] is not None - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to %{ else } or %{~ else ~}.""" prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) @@ -164,7 +168,9 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[3] is not None - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to %{ endif } or %{~ endif ~}.""" prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) @@ -249,7 +255,9 @@ def collection(self) -> ExpressionRule: """Return the collection expression after IN.""" return self._children[7] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to %{ for VAR in EXPR } or %{~ for VAR in EXPR ~}.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -293,7 +301,9 @@ def strip_close(self) -> bool: """Check if there's a strip marker before }.""" return self._children[3] is not None - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to %{ endfor } or %{~ endfor ~}.""" prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) @@ -343,7 +353,9 @@ def __init__( # pylint: disable=R0917 children.append(endif) super().__init__(children, meta) - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize the full if/else/endif directive.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -401,7 +413,9 @@ def __init__( children = [for_start, *body, endfor] super().__init__(children, meta) - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize the full for/endfor directive.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() diff --git a/hcl2/rules/expressions.py b/hcl2/rules/expressions.py index 96c63fcb..6edd4f54 100644 --- a/hcl2/rules/expressions.py +++ b/hcl2/rules/expressions.py @@ -37,8 +37,8 @@ def __init__(self, children, meta: Optional[Meta] = None, parentheses: bool = Fa def _wrap_into_parentheses( self, value: str, - _options=None, - context=None, + _options: Optional[SerializationOptions] = None, + context: Optional[SerializationContext] = None, ) -> str: """Wrap value in parentheses if inside a nested expression.""" context = context if context is not None else SerializationContext() @@ -99,7 +99,9 @@ def expression(self) -> ExpressionRule: """Return the inner expression.""" return self._children[2] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize, handling parenthesized expression wrapping.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -153,7 +155,9 @@ def if_false(self) -> ExpressionRule: """Return the false-branch expression.""" return self._children[8] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to ternary expression string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -202,7 +206,9 @@ def expr_term(self) -> ExprTermRule: """Return the right-hand operand.""" return self._children[3] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'operator operand' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -271,7 +277,9 @@ def absorbed_comments(self): return trailing.to_list() or [] return [] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'lhs operator rhs' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -310,7 +318,9 @@ def expr_term(self): """Return the operand.""" return self._children[1] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'operator operand' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() diff --git a/hcl2/rules/for_expressions.py b/hcl2/rules/for_expressions.py index 6ce9643b..c43c2adf 100644 --- a/hcl2/rules/for_expressions.py +++ b/hcl2/rules/for_expressions.py @@ -92,7 +92,9 @@ def iterable(self) -> ExpressionRule: """Return the collection expression being iterated over.""" return self._children[8] - def serialize(self, options=None, context=None) -> str: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> str: """Serialize to 'for key, value in collection : ' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -129,7 +131,9 @@ def condition_expr(self) -> ExpressionRule: """Return the condition expression.""" return self._children[2] - def serialize(self, options=None, context=None) -> str: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> str: """Serialize to 'if condition' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -195,7 +199,9 @@ def condition(self) -> Optional[ForCondRule]: """Return the optional condition rule.""" return self._children[6] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to '[for ... : expr]' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -293,7 +299,9 @@ def condition(self) -> Optional[ForCondRule]: """Return the optional condition rule.""" return self._children[11] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to '{for ... : key => value}' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() diff --git a/hcl2/rules/functions.py b/hcl2/rules/functions.py index 2665be6a..e22778cc 100644 --- a/hcl2/rules/functions.py +++ b/hcl2/rules/functions.py @@ -50,7 +50,9 @@ def arguments(self) -> List[ExpressionRule]: """Return the list of expression arguments.""" return [child for child in self._children if isinstance(child, ExpressionRule)] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to a comma-separated argument string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -92,7 +94,9 @@ def arguments(self) -> Optional[ArgumentsRule]: return child return None - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'func(args)' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() diff --git a/hcl2/rules/indexing.py b/hcl2/rules/indexing.py index 974c0a62..6a1d2f46 100644 --- a/hcl2/rules/indexing.py +++ b/hcl2/rules/indexing.py @@ -44,7 +44,9 @@ def index(self): """Return the index token.""" return self.children[1] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to '.N' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -72,7 +74,9 @@ def index_expression(self): """Return the index expression inside the brackets.""" return self.children[2] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to '[expr]' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -93,7 +97,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "index_expr_term" - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'expr[index]' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -124,7 +130,9 @@ def identifier(self) -> IdentifierRule: """Return the accessed identifier.""" return self._children[1] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to '.identifier' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -154,7 +162,9 @@ def get_attr(self) -> GetAttrRule: """Return the attribute access rule.""" return self._children[1] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'expr.attr' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -187,7 +197,9 @@ def get_attrs( """Return the trailing accessor chain.""" return self._children[1:] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to '.*...' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -214,7 +226,9 @@ def attr_splat(self) -> AttrSplatRule: """Return the attribute splat rule.""" return self._children[1] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'expr.*...' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -248,7 +262,9 @@ def get_attrs( """Return the trailing accessor chain.""" return self._children[1:] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to '[*]...' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -275,7 +291,9 @@ def attr_splat(self) -> FullSplatRule: """Return the full splat rule.""" return self._children[1] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'expr[*]...' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() diff --git a/hcl2/rules/literal_rules.py b/hcl2/rules/literal_rules.py index 197c00d3..9dc22296 100644 --- a/hcl2/rules/literal_rules.py +++ b/hcl2/rules/literal_rules.py @@ -1,7 +1,7 @@ """Rule classes for literal values (keywords, identifiers, numbers, operators).""" from abc import ABC -from typing import Any, Tuple +from typing import Any, Optional, Tuple from hcl2.rules.abstract import LarkRule, LarkToken from hcl2.utils import SerializationContext, SerializationOptions, to_dollar_string @@ -17,7 +17,9 @@ def token(self) -> LarkToken: """Return the single token child.""" return self._children[0] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize by delegating to the token's own serialization.""" return self.token.serialize() @@ -41,7 +43,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "literal_value" - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to Python True, False, or None.""" context = context if context is not None else SerializationContext() value = self.token.value @@ -76,7 +80,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "float_lit" - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize, preserving scientific notation when configured.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() diff --git a/hcl2/rules/strings.py b/hcl2/rules/strings.py index ffa6989d..27e900c1 100644 --- a/hcl2/rules/strings.py +++ b/hcl2/rules/strings.py @@ -2,7 +2,7 @@ import re import sys -from typing import Any, List, Tuple, Union +from typing import Any, List, Optional, Tuple, Union from hcl2.rules.abstract import LarkRule from hcl2.rules.expressions import ExpressionRule @@ -65,7 +65,9 @@ def expression(self): """Return the interpolated expression.""" return self.children[1] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to ${expression} string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -94,7 +96,9 @@ def content(self): """Return the content element (string chars, escape, interpolation, or directive).""" return self._children[0] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize this string part.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -116,7 +120,9 @@ def string_parts(self): """Return the list of string parts between quotes.""" return self.children[1:-1] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to a quoted string. `strip_string_quotes` asks for the string's value rather than its @@ -167,7 +173,9 @@ def heredoc(self): """Return the raw heredoc token.""" return self.children[0] - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize the heredoc, optionally stripping to a plain string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -202,7 +210,9 @@ def lark_name() -> str: """Return the grammar rule name.""" return "heredoc_template_trim" - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize the trim heredoc, stripping common leading whitespace.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() @@ -276,7 +286,9 @@ def inner_value(self) -> str: return raw[2:-2] return raw - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize preserving escaped-quote delimiters for round-trip fidelity. Inside template directive expressions, strings are delimited by \\" diff --git a/hcl2/rules/whitespace.py b/hcl2/rules/whitespace.py index 9ed71cee..8221e240 100644 --- a/hcl2/rules/whitespace.py +++ b/hcl2/rules/whitespace.py @@ -6,7 +6,7 @@ from hcl2.rules.abstract import LarkRule from hcl2.rules.literal_rules import TokenRule from hcl2.rules.tokens import NL_OR_COMMENT -from hcl2.utils import SerializationOptions +from hcl2.utils import SerializationContext, SerializationOptions class NewLineOrCommentRule(TokenRule): @@ -22,7 +22,9 @@ def from_string(cls, string: str) -> "NewLineOrCommentRule": """Create an instance from a raw comment or newline string.""" return cls([NL_OR_COMMENT(string)]) # type: ignore[abstract] # pylint: disable=abstract-class-instantiated - def serialize(self, options=None, context=None) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to the raw comment/newline string.""" return "".join(child.serialize() for child in self._children) diff --git a/test/unit/query/test_base.py b/test/unit/query/test_base.py index ef745884..c875c320 100644 --- a/test/unit/query/test_base.py +++ b/test/unit/query/test_base.py @@ -8,7 +8,7 @@ from hcl2.rules.expressions import ExpressionRule, ExprTermRule from hcl2.rules.literal_rules import IdentifierRule from hcl2.rules.tokens import EQ, NAME -from hcl2.utils import SerializationContext, SerializationOptions +from hcl2.utils import SerializationOptions class StubExpression(ExpressionRule): @@ -16,7 +16,7 @@ def __init__(self, value): self._stub_value = value super().__init__([], None) - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): return self._stub_value diff --git a/test/unit/rules/test_abstract.py b/test/unit/rules/test_abstract.py index d2f4bb56..546c454e 100644 --- a/test/unit/rules/test_abstract.py +++ b/test/unit/rules/test_abstract.py @@ -5,7 +5,6 @@ from lark.tree import Meta from hcl2.rules.abstract import LarkRule, LarkToken -from hcl2.utils import SerializationContext, SerializationOptions # --- Concrete stubs for testing ABCs --- @@ -35,7 +34,7 @@ class ConcreteRule(LarkRule): def lark_name() -> str: return "test_rule" - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): return "test" diff --git a/test/unit/rules/test_base.py b/test/unit/rules/test_base.py index d007d600..92bab287 100644 --- a/test/unit/rules/test_base.py +++ b/test/unit/rules/test_base.py @@ -16,7 +16,7 @@ STRING_CHARS, ) from hcl2.rules.whitespace import NewLineOrCommentRule -from hcl2.utils import SerializationContext, SerializationOptions +from hcl2.utils import SerializationOptions # --- Stubs & helpers --- @@ -28,7 +28,7 @@ def __init__(self, value): self._stub_value = value super().__init__([], None) - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): return self._stub_value diff --git a/test/unit/rules/test_containers.py b/test/unit/rules/test_containers.py index d3b14d6d..916371f3 100644 --- a/test/unit/rules/test_containers.py +++ b/test/unit/rules/test_containers.py @@ -39,7 +39,7 @@ def __init__(self, value): self._stub_value = value super().__init__([], None) - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): return self._stub_value diff --git a/test/unit/rules/test_expressions.py b/test/unit/rules/test_expressions.py index ea59209a..4ce4632b 100644 --- a/test/unit/rules/test_expressions.py +++ b/test/unit/rules/test_expressions.py @@ -31,7 +31,7 @@ def __init__(self, value, children=None): self._stub_value = value super().__init__(children or [], None) - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): return self._stub_value @@ -42,7 +42,7 @@ class NonExpressionRule(LarkRule): def lark_name(): return "non_expression" - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): return "non_expr" @@ -147,7 +147,7 @@ def test_serialize_sets_inside_parentheses_context(self): seen_context = {} class ContextCapture(ExpressionRule): - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): seen_context["inside_parentheses"] = context.inside_parentheses return "x" @@ -160,7 +160,7 @@ def test_serialize_no_parens_preserves_inside_parentheses(self): seen_context = {} class ContextCapture(ExpressionRule): - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): seen_context["inside_parentheses"] = context.inside_parentheses return "x" diff --git a/test/unit/rules/test_for_expressions.py b/test/unit/rules/test_for_expressions.py index bf3c7fe9..0de0682c 100644 --- a/test/unit/rules/test_for_expressions.py +++ b/test/unit/rules/test_for_expressions.py @@ -36,7 +36,7 @@ def __init__(self, value): self._last_options = None super().__init__([], None) - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): self._last_options = options return self._stub_value diff --git a/test/unit/rules/test_functions.py b/test/unit/rules/test_functions.py index 5a40538b..4586535e 100644 --- a/test/unit/rules/test_functions.py +++ b/test/unit/rules/test_functions.py @@ -8,7 +8,7 @@ ) from hcl2.rules.literal_rules import IdentifierRule from hcl2.rules.tokens import COMMA, ELLIPSIS, LPAR, NAME, RPAR, StringToken -from hcl2.utils import SerializationContext, SerializationOptions +from hcl2.utils import SerializationContext # --- Stubs & helpers --- @@ -20,7 +20,7 @@ def __init__(self, value): self._stub_value = value super().__init__([], None) - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): return self._stub_value diff --git a/test/unit/rules/test_strings.py b/test/unit/rules/test_strings.py index ef800d91..329979e2 100644 --- a/test/unit/rules/test_strings.py +++ b/test/unit/rules/test_strings.py @@ -30,7 +30,7 @@ def __init__(self, value): self._stub_value = value super().__init__([], None) - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): return self._stub_value diff --git a/test/unit/rules/test_whitespace.py b/test/unit/rules/test_whitespace.py index bb6410c7..c8c0b372 100644 --- a/test/unit/rules/test_whitespace.py +++ b/test/unit/rules/test_whitespace.py @@ -3,7 +3,6 @@ from hcl2.rules.tokens import NAME, NL_OR_COMMENT from hcl2.rules.whitespace import InlineCommentMixIn, NewLineOrCommentRule -from hcl2.utils import SerializationContext, SerializationOptions # --- Concrete stub for testing InlineCommentMixIn --- @@ -13,7 +12,7 @@ class ConcreteInlineComment(InlineCommentMixIn): def lark_name() -> str: return "test_inline" - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): return "test" diff --git a/test/unit/test_thread_safety.py b/test/unit/test_thread_safety.py index 5a9cef10..61d3945f 100644 --- a/test/unit/test_thread_safety.py +++ b/test/unit/test_thread_safety.py @@ -34,21 +34,36 @@ def _parameters_named(*names): - """Yield every parameter with one of *names* across the shipped rule modules. + """Yield every parameter with one of *names* across the shipped package. Walking the modules rather than naming methods means a rule added later is - covered without anyone remembering to come back here. + covered without anyone remembering to come back here. Three things it has + to get right, each of which it did not at first: + + * `walk_packages`, not `iter_modules`, so a future subpackage is not + invisible. + * the whole of `hcl2`, not `hcl2.rules` alone -- the defaults are a + package-wide pattern, and the serializer is not the only place they can + appear. + * `__func__` unwrapped before the function test, because `vars(cls)` hands + back the descriptor for a staticmethod or classmethod and + `inspect.isfunction` is False for those. `StringRule._serialize_part_as_value` + takes a context and is a staticmethod, and was missed until this did. """ import pkgutil - import hcl2.rules + import hcl2 - for module_info in pkgutil.iter_modules(hcl2.rules.__path__): - module = import_module(f"hcl2.rules.{module_info.name}") + for module_info in pkgutil.walk_packages(hcl2.__path__, prefix="hcl2."): + try: + module = import_module(module_info.name) + except ImportError: # pragma: no cover - nothing optional ships today + continue for class_name, cls in vars(module).items(): if not inspect.isclass(cls) or cls.__module__ != module.__name__: continue - for method_name, method in vars(cls).items(): + for method_name, member in vars(cls).items(): + method = getattr(member, "__func__", member) if not inspect.isfunction(method): continue parameters = inspect.signature(method).parameters @@ -103,8 +118,28 @@ def test_every_context_parameter_defaults_to_none(self): def test_the_walk_actually_found_the_methods(self): # A test that asserts "no offenders" over an empty list would pass - # while inspecting nothing at all. - self.assertGreater(len(list(_parameters_named("context"))), 30) + # while inspecting nothing at all. The floor sits just under the real + # count, so losing a module's worth of coverage fails here rather than + # passing quietly. + self.assertGreaterEqual(len(list(_parameters_named("context"))), 50) + + def test_every_context_parameter_is_annotated_optional(self): + """`None` only works if the body builds one, and mypy has to see that. + + The default alone is not the invariant: a rule written `context=None` + whose body calls `context.modify(...)` passes the check above and then + raises `AttributeError` for the direct caller this all exists to + protect. Annotated, mypy reports `union-attr` on the missing guard -- + verified by deleting one. + """ + # Only the ones that default to None: a required parameter cannot be + # None, and calling it Optional would say something untrue. + unannotated = [ + name + for name, parameter in _parameters_named("context") + if parameter.default is None and parameter.annotation is parameter.empty + ] + self.assertEqual(unannotated, []) class TestIsolationWithoutRelyingOnScheduling(TestCase): @@ -195,4 +230,12 @@ def test_every_options_parameter_defaults_to_none(self): self.assertEqual(offenders, []) def test_the_walk_actually_found_the_methods(self): - self.assertGreater(len(list(_parameters_named("options", "_options"))), 30) + self.assertGreaterEqual(len(list(_parameters_named("options", "_options"))), 55) + + def test_every_options_parameter_is_annotated_optional(self): + unannotated = [ + name + for name, parameter in _parameters_named("options", "_options") + if parameter.default is None and parameter.annotation is parameter.empty + ] + self.assertEqual(unannotated, []) diff --git a/test/unit/test_walk.py b/test/unit/test_walk.py index ae718c0d..fd15ae97 100644 --- a/test/unit/test_walk.py +++ b/test/unit/test_walk.py @@ -6,7 +6,6 @@ from hcl2.rules.literal_rules import IdentifierRule from hcl2.rules.tokens import EQ, LBRACE, NAME, NL_OR_COMMENT, RBRACE from hcl2.rules.whitespace import NewLineOrCommentRule -from hcl2.utils import SerializationContext, SerializationOptions from hcl2.walk import ( ancestors, find_all, @@ -23,7 +22,7 @@ def __init__(self, value): self._stub_value = value super().__init__([], None) - def serialize(self, options=SerializationOptions(), context=SerializationContext()): + def serialize(self, options=None, context=None): return self._stub_value From 92881e55f2308b173802003ed1863d8ab02f08e0 Mon Sep 17 00:00:00 2001 From: Tim Date: Wed, 2 Sep 2026 11:45:33 -0700 Subject: [PATCH 6/6] fix: make SerializationContext immutable (#344) Removing the shared default fixed the contexts the package builds for itself. A consumer that builds one and hands it to concurrent calls still had several writers: modify() was a context manager that set fields on whatever context it was given and restored them on exit, so two threads sharing one object raced on it and produced the same silent corruption -- a tuple or object serialized as its inline HCL source -- with nothing to warn them. A traversal now descends by building a child with replace(), the dataclass is frozen, and modify() is gone. The post-block checks are unchanged in meaning: they already read the outer value, which is now simply the context that was never written to. Racing two threads does not demonstrate the old defect -- modify() restored the field within a single call, so a repetition test came back green against the unfixed code. The regression test reads the caller's own context from inside the scope that used to mutate it, and sees True there before this change. --- CHANGELOG.md | 3 +- hcl2/rules/containers.py | 28 +++++------ hcl2/rules/directives.py | 14 +++--- hcl2/rules/expressions.py | 32 ++++++------ hcl2/rules/for_expressions.py | 26 +++++----- hcl2/rules/functions.py | 10 ++-- hcl2/rules/indexing.py | 32 ++++++------ hcl2/rules/strings.py | 4 +- hcl2/utils.py | 26 +++------- test/unit/test_thread_safety.py | 86 ++++++++++++++++++++++++++++++--- test/unit/test_utils.py | 37 +++++++------- 11 files changed, 177 insertions(+), 121 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22e87b80..b934b46a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed -- Concurrent calls to `load`/`loads` no longer corrupt each other's values. `serialize()` declared `context=SerializationContext()` as a default argument, which Python evaluates once at import, so every rule in the process shared one mutable context — and the expression, function and indexing rules mutate it in place via `context.modify(inside_dollar_string=True)`. A thread serializing a function call therefore set that flag for every other thread, and a tuple or object being serialized elsewhere came back as its inline HCL source (`'[1, 2, 3]'`) rather than a list, with no exception raised. Measured before the fix: 400 of 400 interleaved parses corrupted. Every other rule's `serialize` carried the same default and is changed with them: the public API always enters at `StartRule`, so those were unreachable with a shared context, but a caller serializing a rule directly still had one. Both parameters are annotated `Optional[...]`, so a rule that takes `None` and then dereferences it without constructing one is a mypy error rather than an `AttributeError` for the direct caller this protects. `options` loses its shared default too. Nothing in the package assigns to a `SerializationOptions`, so that one was not a live defect, but it is the same construct in the same position -- one mutable object handed to every caller that omits the argument, reachable by any subclass or hook a consumer writes. +- Concurrent calls to `load`/`loads` no longer corrupt each other's values. `serialize()` declared `context=SerializationContext()` as a default argument, which Python evaluates once at import, so every rule in the process shared one mutable context — and the expression, function and indexing rules mutated it in place via `context.modify(inside_dollar_string=True)`. A thread serializing a function call therefore set that flag for every other thread, and a tuple or object being serialized elsewhere came back as its inline HCL source (`'[1, 2, 3]'`) rather than a list, with no exception raised. Measured before the fix: 400 of 400 interleaved parses corrupted. Every other rule's `serialize` carried the same default and is changed with them: the public API always enters at `StartRule`, so those were unreachable with a shared context, but a caller serializing a rule directly still had one. Both parameters are annotated `Optional[...]`, so a rule that takes `None` and then dereferences it without constructing one is a mypy error rather than an `AttributeError` for the direct caller this protects. `options` loses its shared default too. Nothing in the package assigns to a `SerializationOptions`, so that one was not a live defect, but it is the same construct in the same position -- one mutable object handed to every caller that omits the argument, reachable by any subclass or hook a consumer writes. +- `SerializationContext` is immutable, so a context a caller builds and hands to concurrent calls is safe to share. Removing the shared default fixed only the contexts the package creates for itself; `modify` was a context manager that set fields on whatever context it was given and restored them on exit, so a consumer passing one object to several threads still had several writers and the same silent corruption, with nothing to warn them. A traversal now descends by building a child with `replace`, the dataclass is `frozen=True`, and `modify` is gone -- an assignment that used to be a temporary mutation is a `FrozenInstanceError` where it is written. Racing two threads does not demonstrate the old defect, since `modify` restored the field within one call; the regression test reads the caller's own context from inside the scope that used to mutate it, and sees `True` there before this change. ([#344](https://github.com/amplify-education/python-hcl2/issues/344)) ## \[8.1.3\] - 2026-08-26 diff --git a/hcl2/rules/containers.py b/hcl2/rules/containers.py index e0d3c908..41d68b29 100644 --- a/hcl2/rules/containers.py +++ b/hcl2/rules/containers.py @@ -69,10 +69,10 @@ def serialize( if not options.wrap_tuples and not context.inside_dollar_string: return [element.serialize(options, context) for element in self.elements] - with context.modify(inside_dollar_string=True): - result = "[" - result += ", ".join(str(element.serialize(options, context)) for element in self.elements) - result += "]" + inner = context.replace(inside_dollar_string=True) + result = "[" + result += ", ".join(str(element.serialize(options, inner)) for element in self.elements) + result += "]" if not context.inside_dollar_string: result = to_dollar_string(result) @@ -137,8 +137,8 @@ def serialize( """Serialize to '${expression}' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - result = str(self.expression.serialize(options, context)) + inner = context.replace(inside_dollar_string=True) + result = str(self.expression.serialize(options, inner)) if not context.inside_dollar_string: result = to_dollar_string(result) return result @@ -214,15 +214,13 @@ def serialize( dict_result.update(element.serialize(options, context)) return dict_result - with context.modify(inside_dollar_string=True): - str_result = "{" - str_result += ", ".join( - f"{element.key.serialize(options, context)}" - f" = " - f"{element.expression.serialize(options, context)}" - for element in self.elements - ) - str_result += "}" + inner = context.replace(inside_dollar_string=True) + str_result = "{" + str_result += ", ".join( + f"{element.key.serialize(options, inner)} = {element.expression.serialize(options, inner)}" + for element in self.elements + ) + str_result += "}" if not context.inside_dollar_string: str_result = to_dollar_string(str_result) diff --git a/hcl2/rules/directives.py b/hcl2/rules/directives.py index 0038443a..24e0e808 100644 --- a/hcl2/rules/directives.py +++ b/hcl2/rules/directives.py @@ -92,8 +92,8 @@ def serialize( """Serialize to %{ if EXPR } or %{~ if EXPR ~}.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - cond_str = self.condition.serialize(options, context) + inner = context.replace(inside_dollar_string=True) + cond_str = self.condition.serialize(options, inner) prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) return f"%{{{prefix}if {cond_str}{suffix}}}" @@ -263,11 +263,11 @@ def serialize( context = context if context is not None else SerializationContext() prefix = _strip_prefix(self.strip_open) suffix = _strip_suffix(self.strip_close) - with context.modify(inside_dollar_string=True): - iter_str = self.iterator.serialize(options, context) - if self.key_iterator is not None: - iter_str += f", {self.key_iterator.serialize(options, context)}" - coll_str = self.collection.serialize(options, context) + inner = context.replace(inside_dollar_string=True) + iter_str = self.iterator.serialize(options, inner) + if self.key_iterator is not None: + iter_str += f", {self.key_iterator.serialize(options, inner)}" + coll_str = self.collection.serialize(options, inner) return f"%{{{prefix}for {iter_str} in {coll_str}{suffix}}}" diff --git a/hcl2/rules/expressions.py b/hcl2/rules/expressions.py index 6edd4f54..af02abde 100644 --- a/hcl2/rules/expressions.py +++ b/hcl2/rules/expressions.py @@ -105,8 +105,8 @@ def serialize( """Serialize, handling parenthesized expression wrapping.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_parentheses=self.parentheses or context.inside_parentheses): - result = self.expression.serialize(options, context) + inner = context.replace(inside_parentheses=self.parentheses or context.inside_parentheses) + result = self.expression.serialize(options, inner) if self.parentheses: result = wrap_into_parentheses(result) @@ -161,12 +161,12 @@ def serialize( """Serialize to ternary expression string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - result = ( - f"{self.condition.serialize(options, context)} " - f"? {self.if_true.serialize(options, context)} " - f": {self.if_false.serialize(options, context)}" - ) + inner = context.replace(inside_dollar_string=True) + result = ( + f"{self.condition.serialize(options, inner)} " + f"? {self.if_true.serialize(options, inner)} " + f": {self.if_false.serialize(options, inner)}" + ) if not context.inside_dollar_string: result = to_dollar_string(result) @@ -283,10 +283,10 @@ def serialize( """Serialize to 'lhs operator rhs' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - lhs = self.expr_term.serialize(options, context) - operator = str(self.binary_term.binary_operator.serialize(options, context)).strip() - rhs = self.binary_term.expr_term.serialize(options, context) + inner = context.replace(inside_dollar_string=True) + lhs = self.expr_term.serialize(options, inner) + operator = str(self.binary_term.binary_operator.serialize(options, inner)).strip() + rhs = self.binary_term.expr_term.serialize(options, inner) result = f"{lhs} {operator} {rhs}" @@ -324,10 +324,10 @@ def serialize( """Serialize to 'operator operand' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - operator = self.operator.rstrip() - operand = self.expr_term.serialize(options, context) - result = f"{operator}{operand}" + inner = context.replace(inside_dollar_string=True) + operator = self.operator.rstrip() + operand = self.expr_term.serialize(options, inner) + result = f"{operator}{operand}" if not context.inside_dollar_string: # A negated numeric literal is a number, not an expression. The diff --git a/hcl2/rules/for_expressions.py b/hcl2/rules/for_expressions.py index c43c2adf..0e515d54 100644 --- a/hcl2/rules/for_expressions.py +++ b/hcl2/rules/for_expressions.py @@ -207,12 +207,12 @@ def serialize( context = context if context is not None else SerializationContext() result = "[" - with context.modify(inside_dollar_string=True): - result += self.for_intro.serialize(options, context) - result += self.value_expr.serialize(options, context) + inner = context.replace(inside_dollar_string=True) + result += self.for_intro.serialize(options, inner) + result += self.value_expr.serialize(options, inner) - if self.condition is not None: - result += f" {self.condition.serialize(options, context)}" + if self.condition is not None: + result += f" {self.condition.serialize(options, inner)}" result += "]" if not context.inside_dollar_string: @@ -306,16 +306,16 @@ def serialize( options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() result = "{" - with context.modify(inside_dollar_string=True): - result += self.for_intro.serialize(options, context) - result += f"{self.key_expr.serialize(options, context)} => " + inner = context.replace(inside_dollar_string=True) + result += self.for_intro.serialize(options, inner) + result += f"{self.key_expr.serialize(options, inner)} => " - result += self.value_expr.serialize(replace(options, wrap_objects=True), context) - if self.ellipsis is not None: - result += self.ellipsis.serialize(options, context) + result += self.value_expr.serialize(replace(options, wrap_objects=True), inner) + if self.ellipsis is not None: + result += self.ellipsis.serialize(options, inner) - if self.condition is not None: - result += f" {self.condition.serialize(options, context)}" + if self.condition is not None: + result += f" {self.condition.serialize(options, inner)}" result += "}" if not context.inside_dollar_string: diff --git a/hcl2/rules/functions.py b/hcl2/rules/functions.py index e22778cc..6e26300c 100644 --- a/hcl2/rules/functions.py +++ b/hcl2/rules/functions.py @@ -100,11 +100,11 @@ def serialize( """Serialize to 'func(args)' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - name = "::".join(identifier.serialize(options, context) for identifier in self.identifiers) - args = self.arguments - args_str = args.serialize(options, context) if args else "" - result = f"{name}({args_str})" + inner = context.replace(inside_dollar_string=True) + name = "::".join(identifier.serialize(options, inner) for identifier in self.identifiers) + args = self.arguments + args_str = args.serialize(options, inner) if args else "" + result = f"{name}({args_str})" if not context.inside_dollar_string: result = to_dollar_string(result) diff --git a/hcl2/rules/indexing.py b/hcl2/rules/indexing.py index 6a1d2f46..02e7f861 100644 --- a/hcl2/rules/indexing.py +++ b/hcl2/rules/indexing.py @@ -103,10 +103,10 @@ def serialize( """Serialize to 'expr[index]' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - expr = self.children[0].serialize(options, context) - index = self.children[1].serialize(options, context) - result = f"{expr}{index}" + inner = context.replace(inside_dollar_string=True) + expr = self.children[0].serialize(options, inner) + index = self.children[1].serialize(options, inner) + result = f"{expr}{index}" if not context.inside_dollar_string: result = to_dollar_string(result) return result @@ -168,10 +168,10 @@ def serialize( """Serialize to 'expr.attr' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - expr = self.expr_term.serialize(options, context) - attr = self.get_attr.serialize(options, context) - result = f"{expr}{attr}" + inner = context.replace(inside_dollar_string=True) + expr = self.expr_term.serialize(options, inner) + attr = self.get_attr.serialize(options, inner) + result = f"{expr}{attr}" if not context.inside_dollar_string: result = to_dollar_string(result) return result @@ -232,10 +232,10 @@ def serialize( """Serialize to 'expr.*...' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - expr = self.expr_term.serialize(options, context) - splat = self.attr_splat.serialize(options, context) - result = f"{expr}{splat}" + inner = context.replace(inside_dollar_string=True) + expr = self.expr_term.serialize(options, inner) + splat = self.attr_splat.serialize(options, inner) + result = f"{expr}{splat}" if not context.inside_dollar_string: result = to_dollar_string(result) @@ -297,10 +297,10 @@ def serialize( """Serialize to 'expr[*]...' string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - expr = self.expr_term.serialize(options, context) - splat = self.attr_splat.serialize(options, context) - result = f"{expr}{splat}" + inner = context.replace(inside_dollar_string=True) + expr = self.expr_term.serialize(options, inner) + splat = self.attr_splat.serialize(options, inner) + result = f"{expr}{splat}" if not context.inside_dollar_string: result = to_dollar_string(result) diff --git a/hcl2/rules/strings.py b/hcl2/rules/strings.py index 27e900c1..eed654d4 100644 --- a/hcl2/rules/strings.py +++ b/hcl2/rules/strings.py @@ -71,8 +71,8 @@ def serialize( """Serialize to ${expression} string.""" options = options if options is not None else SerializationOptions() context = context if context is not None else SerializationContext() - with context.modify(inside_dollar_string=True): - return to_dollar_string(self.expression.serialize(options, context)) + inner = context.replace(inside_dollar_string=True) + return to_dollar_string(self.expression.serialize(options, inner)) class StringPartRule(LarkRule): diff --git a/hcl2/utils.py b/hcl2/utils.py index 6e79f007..ab9bbf41 100644 --- a/hcl2/utils.py +++ b/hcl2/utils.py @@ -1,7 +1,6 @@ """Serialization options, context tracking, and string utility helpers.""" import re -from contextlib import contextmanager from dataclasses import dataclass, replace from typing import Optional, Tuple @@ -120,9 +119,15 @@ def process_escape_sequences(value: str) -> str: return "".join(parts) -@dataclass +@dataclass(frozen=True) class SerializationContext: - """Mutable state tracked during serialization traversal.""" + """State tracked during serialization traversal, and never mutated. + + A traversal descends into a nested expression by building a child with + `replace`, so what a rule is handed cannot be changed underneath it. The + field values are read on the way down and never written back, which is + what makes one context safe to hand to two threads at once. + """ inside_dollar_string: bool = False inside_parentheses: bool = False @@ -131,21 +136,6 @@ def replace(self, **kwargs) -> "SerializationContext": """Return a new context with the given fields overridden.""" return replace(self, **kwargs) - @contextmanager - def modify(self, **kwargs): - """Context manager that temporarily mutates fields, restoring on exit.""" - original_values = {key: getattr(self, key) for key in kwargs} - - for key, value in kwargs.items(): - setattr(self, key, value) - - try: - yield - finally: - # Restore original values - for key, value in original_values.items(): - setattr(self, key, value) - def is_dollar_string(value: str) -> bool: """Return True if value is a ${...} interpolation wrapper.""" diff --git a/test/unit/test_thread_safety.py b/test/unit/test_thread_safety.py index 61d3945f..16aebd5c 100644 --- a/test/unit/test_thread_safety.py +++ b/test/unit/test_thread_safety.py @@ -3,8 +3,8 @@ `serialize()` declared `context=SerializationContext()` as a default argument. Python evaluates that once, at import, so every rule in the process shared one -mutable context -- and `expressions.py`, `functions.py` and `indexing.py` mutate -it in place through `context.modify(inside_dollar_string=True)`. +mutable context -- and `expressions.py`, `functions.py` and `indexing.py` mutated +it in place to descend into a nested expression. A thread serializing a function call therefore set `inside_dollar_string` for every other thread, and any tuple or object those threads were serializing came @@ -15,6 +15,7 @@ one when called without it, so a parse can no longer see another parse's state. """ +import dataclasses import inspect import threading from concurrent.futures import ThreadPoolExecutor @@ -23,6 +24,7 @@ from hcl2.api import loads, parses, serialize from hcl2.rules.base import AttributeRule +from hcl2.rules.containers import TupleRule from hcl2.utils import SerializationContext, SerializationOptions # Serializing a function call is what sets `inside_dollar_string`; the plain @@ -127,7 +129,7 @@ def test_every_context_parameter_is_annotated_optional(self): """`None` only works if the body builds one, and mypy has to see that. The default alone is not the invariant: a rule written `context=None` - whose body calls `context.modify(...)` passes the check above and then + whose body calls `context.replace(...)` passes the check above and then raises `AttributeError` for the direct caller this all exists to protect. Annotated, mypy reports `union-attr` on the missing guard -- verified by deleting one. @@ -180,17 +182,24 @@ def test_two_serializations_are_handed_different_contexts(self): self.assertEqual(len(seen), 2) self.assertIsNot(seen[0], seen[1]) - def test_a_held_mutation_is_invisible_to_a_concurrent_parse(self): + def test_a_mutation_cannot_be_held_across_a_concurrent_parse(self): + """Isolation no longer rests on the two contexts merely being distinct. + + This used to hold a flag set in one thread across the other's + serialization and assert the other never saw it. There is now no way to + set one: the context is frozen, so the write this guards against is + refused where it is made rather than contained after the fact. + """ barrier = threading.Barrier(2, timeout=30) observed = {} def hook(context): role = threading.current_thread().name if role == "mutator": - # Hold the flag set across the other thread's serialization. - context.inside_dollar_string = True + with self.assertRaises(dataclasses.FrozenInstanceError): + context.inside_dollar_string = True barrier.wait() - observed["mutator-kept-its-own"] = context.inside_dollar_string + observed["mutator-could-not-set"] = context.inside_dollar_string else: barrier.wait() observed["observer-saw"] = context.inside_dollar_string @@ -207,7 +216,7 @@ def hook(context): for thread in threads: thread.join(timeout=30) - self.assertEqual(observed, {"mutator-kept-its-own": True, "observer-saw": False}) + self.assertEqual(observed, {"mutator-could-not-set": False, "observer-saw": False}) class TestNoDefaultOptionsIsShared(TestCase): @@ -239,3 +248,64 @@ def test_every_options_parameter_is_annotated_optional(self): if parameter.default is None and parameter.annotation is parameter.empty ] self.assertEqual(unannotated, []) + + +class TestOneContextCanBeSharedDeliberately(TestCase): + """The half a fresh default per call does not reach: a caller's own context. + + Defaulting the parameter to `None` stops the *package* from sharing one + context between threads, but a consumer that builds a context and hands it + to concurrent calls was still giving one mutable object to several writers. + Nothing warned them, and the corruption looked exactly like the one the + shared default caused. + + Racing two threads does not demonstrate this: `modify` set the flag and + restored it within one call, so the window is far too small to land on by + repetition -- a test that tried came back green against the unfixed code. + What settles it is reading the caller's own context from inside the scope + that used to mutate it. + """ + + SOURCE = TOGGLES_CONTEXT + + def _observe_during_a_nested_serialization(self, shared): + """Record `shared.inside_dollar_string` from inside the `${...}` scope. + + `TupleRule` serializes the `[1, 2, 3]` argument, which happens while + `FunctionCallRule` is in the scope that used to set the flag on + whatever context it was handed. + """ + seen = [] + original = TupleRule.serialize + + def spy(rule, options=None, context=None): + seen.append(shared.inside_dollar_string) + return original(rule, options, context) + + TupleRule.serialize = spy # type: ignore[method-assign] + self.addCleanup(setattr, TupleRule, "serialize", original) + return seen + + def test_a_nested_scope_does_not_write_to_the_callers_context(self): + shared = SerializationContext() + seen = self._observe_during_a_nested_serialization(shared) + + parses(self.SOURCE).serialize(SerializationOptions(), shared) + + # The spy has to have run, or this asserts nothing. + self.assertEqual(len(seen), 1) + # Was True here before the context became immutable. + self.assertEqual(seen, [False]) + self.assertEqual(shared, SerializationContext()) + + def test_the_same_context_serves_several_calls(self): + shared = SerializationContext() + with ThreadPoolExecutor(max_workers=4) as pool: + produced = list( + pool.map( + lambda src: repr(parses(src).serialize(SerializationOptions(), shared)), + [TOGGLES_CONTEXT, PLAIN] * 8, + ) + ) + self.assertEqual(set(produced[1::2]), {repr(EXPECTED)}) + self.assertEqual(shared, SerializationContext()) diff --git a/test/unit/test_utils.py b/test/unit/test_utils.py index c428da0e..21ffc5f5 100644 --- a/test/unit/test_utils.py +++ b/test/unit/test_utils.py @@ -1,4 +1,5 @@ # pylint: disable=C0103,C0114,C0115,C0116 +import dataclasses from unittest import TestCase from hcl2.utils import ( @@ -47,34 +48,30 @@ def test_replace_returns_new_instance(self): self.assertFalse(ctx.inside_dollar_string) self.assertTrue(new_ctx.inside_dollar_string) - def test_modify_mutates_and_restores(self): + def test_replace_multiple_fields(self): ctx = SerializationContext() + both = ctx.replace(inside_dollar_string=True, inside_parentheses=True) + self.assertTrue(both.inside_dollar_string) + self.assertTrue(both.inside_parentheses) self.assertFalse(ctx.inside_dollar_string) + self.assertFalse(ctx.inside_parentheses) - with ctx.modify(inside_dollar_string=True): - self.assertTrue(ctx.inside_dollar_string) - - self.assertFalse(ctx.inside_dollar_string) + def test_a_field_cannot_be_assigned(self): + """The point of the type: a caller's context cannot be changed under it. - def test_modify_restores_on_exception(self): + A traversal descends by building a child, so nothing writes back. An + assignment that used to be a temporary mutation is now an error at the + point it is written rather than a value another thread can observe. + """ ctx = SerializationContext() - - with self.assertRaises(ValueError): - with ctx.modify(inside_dollar_string=True, inside_parentheses=True): - self.assertTrue(ctx.inside_dollar_string) - self.assertTrue(ctx.inside_parentheses) - raise ValueError("test") - + with self.assertRaises(dataclasses.FrozenInstanceError): + ctx.inside_dollar_string = True # type: ignore[misc] self.assertFalse(ctx.inside_dollar_string) - self.assertFalse(ctx.inside_parentheses) - def test_modify_multiple_fields(self): + def test_it_is_hashable_now_that_it_is_frozen(self): ctx = SerializationContext() - with ctx.modify(inside_dollar_string=True, inside_parentheses=True): - self.assertTrue(ctx.inside_dollar_string) - self.assertTrue(ctx.inside_parentheses) - self.assertFalse(ctx.inside_dollar_string) - self.assertFalse(ctx.inside_parentheses) + self.assertEqual(len({ctx, SerializationContext()}), 1) + self.assertEqual(len({ctx, ctx.replace(inside_parentheses=True)}), 2) class TestIsDollarString(TestCase):