diff --git a/CHANGELOG.md b/CHANGELOG.md index 61dc143a..b934b46a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,10 @@ 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 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/abstract.py b/hcl2/rules/abstract.py index c8ba063e..dcbde90d 100644 --- a/hcl2/rules/abstract.py +++ b/hcl2/rules/abstract.py @@ -36,7 +36,9 @@ def to_lark(self) -> Any: raise NotImplementedError() @abstractmethod - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() @@ -63,7 +65,9 @@ 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: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize this token using its serialize_conversion callable.""" return self.serialize_conversion(self.value) @@ -89,7 +93,9 @@ class LarkRule(LarkElement, ABC): """ @abstractmethod - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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 625bd835..a276d662 100644 --- a/hcl2/rules/base.py +++ b/hcl2/rules/base.py @@ -39,9 +39,13 @@ 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: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to a single-entry dict.""" - return {self.identifier.serialize(options): self.expression.serialize(options)} + 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)} class BodyRule(LarkRule): @@ -60,8 +64,12 @@ def lark_name() -> str: """Return the grammar rule name.""" return "body" - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() attribute_names = set() comments = [] inline_comments = [] @@ -70,14 +78,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 +119,13 @@ def lark_name() -> str: """Return the grammar rule name.""" return "start" - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize by delegating to the body.""" - return self.body.serialize(options) + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + return self.body.serialize(options, context) class BlockRule(LarkRule): @@ -147,14 +159,18 @@ def body(self) -> BodyRule: """Return the block body.""" return self._body - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to a nested dict with labels as keys.""" - result = self._body.serialize(options) + 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: 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/hcl2/rules/containers.py b/hcl2/rules/containers.py index 8b811ce8..41d68b29 100644 --- a/hcl2/rules/containers.py +++ b/hcl2/rules/containers.py @@ -60,15 +60,19 @@ 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: 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() 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) @@ -93,8 +97,12 @@ 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: 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() result = self.value.serialize(options, context) # Object keys must be strings for JSON compatibility if isinstance(result, (int, float)): @@ -123,10 +131,14 @@ def expression(self) -> ExpressionRule: """Return the key expression.""" return self._children[0] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to '${expression}' string.""" - with context.modify(inside_dollar_string=True): - result = str(self.expression.serialize(options, context)) + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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 @@ -156,8 +168,12 @@ def expression(self): """Return the value expression.""" return self._children[2] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() return {self.key.serialize(options, context): self.expression.serialize(options, context)} @@ -186,23 +202,25 @@ 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: 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() if not options.wrap_objects and not context.inside_dollar_string: dict_result: dict = {} for element in self.elements: 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 4a74fc66..24e0e808 100644 --- a/hcl2/rules/directives.py +++ b/hcl2/rules/directives.py @@ -86,10 +86,14 @@ 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: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to %{ if EXPR } or %{~ if EXPR ~}.""" - with context.modify(inside_dollar_string=True): - cond_str = self.condition.serialize(options, context) + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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}}}" @@ -125,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=SerializationOptions(), context=SerializationContext()) -> 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) @@ -162,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=SerializationOptions(), context=SerializationContext()) -> 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) @@ -247,15 +255,19 @@ 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: 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() 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}}}" @@ -289,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=SerializationOptions(), context=SerializationContext()) -> 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) @@ -339,8 +353,12 @@ def __init__( # pylint: disable=R0917 children.append(endif) super().__init__(children, meta) - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() result = self._if_start.serialize(options, context) for part in self._if_body: result += part.serialize(options, context) @@ -395,8 +413,12 @@ def __init__( children = [for_start, *body, endfor] super().__init__(children, meta) - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() 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..af02abde 100644 --- a/hcl2/rules/expressions.py +++ b/hcl2/rules/expressions.py @@ -37,10 +37,11 @@ def __init__(self, children, meta: Optional[Meta] = None, parentheses: bool = Fa def _wrap_into_parentheses( self, value: str, - _options=SerializationOptions(), - context=SerializationContext(), + _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() # do not wrap into parentheses if # 1. already wrapped or # 2. is top-level expression (unless explicitly wrapped) @@ -98,10 +99,14 @@ def expression(self) -> ExpressionRule: """Return the inner expression.""" return self._children[2] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize, handling parenthesized expression wrapping.""" - with context.modify(inside_parentheses=self.parentheses or context.inside_parentheses): - result = self.expression.serialize(options, context) + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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) @@ -150,14 +155,18 @@ 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: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to ternary expression string.""" - 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)}" - ) + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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) @@ -197,8 +206,12 @@ 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: 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() op_str = self.binary_operator.serialize(options, context) term_str = self.expr_term.serialize(options, context) return f"{op_str} {term_str}" @@ -264,12 +277,16 @@ def absorbed_comments(self): return trailing.to_list() or [] return [] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'lhs operator rhs' string.""" - 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) + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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}" @@ -301,12 +318,16 @@ def expr_term(self): """Return the operand.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'operator operand' string.""" - with context.modify(inside_dollar_string=True): - operator = self.operator.rstrip() - operand = self.expr_term.serialize(options, context) - result = f"{operator}{operand}" + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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 6013072e..0e515d54 100644 --- a/hcl2/rules/for_expressions.py +++ b/hcl2/rules/for_expressions.py @@ -92,8 +92,12 @@ 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: 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() result = "for " result += f"{self.first_iterator.serialize(options, context)}" @@ -127,8 +131,12 @@ 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: 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() return f"if {self.condition_expr.serialize(options, context)}" @@ -191,16 +199,20 @@ 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: 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() 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: @@ -287,19 +299,23 @@ 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: 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() 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 c48a7c2a..6e26300c 100644 --- a/hcl2/rules/functions.py +++ b/hcl2/rules/functions.py @@ -50,8 +50,12 @@ 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: 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() result = ", ".join(str(argument.serialize(options, context)) for argument in self.arguments) if self.has_ellipsis: result += " ..." @@ -90,13 +94,17 @@ def arguments(self) -> Optional[ArgumentsRule]: return child return None - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'func(args)' string.""" - 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})" + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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 9bdab541..02e7f861 100644 --- a/hcl2/rules/indexing.py +++ b/hcl2/rules/indexing.py @@ -44,8 +44,12 @@ def index(self): """Return the index token.""" return self.children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() return f".{self.index.serialize(options, context)}" @@ -70,8 +74,12 @@ 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: 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() return f"[{self.index_expression.serialize(options, context)}]" def __init__(self, children, meta: Optional[Meta] = None): @@ -89,12 +97,16 @@ 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: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'expr[index]' string.""" - 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}" + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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 @@ -118,8 +130,12 @@ def identifier(self) -> IdentifierRule: """Return the accessed identifier.""" return self._children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() return f".{self.identifier.serialize(options, context)}" @@ -146,12 +162,16 @@ 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: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'expr.attr' string.""" - 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}" + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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 @@ -177,8 +197,12 @@ def get_attrs( """Return the trailing accessor chain.""" return self._children[1:] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() return ".*" + "".join(get_attr.serialize(options, context) for get_attr in self.get_attrs) @@ -202,12 +226,16 @@ 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: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'expr.*...' string.""" - 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}" + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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) @@ -234,8 +262,12 @@ def get_attrs( """Return the trailing accessor chain.""" return self._children[1:] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() return "[*]" + "".join(get_attr.serialize(options, context) for get_attr in self.get_attrs) @@ -259,12 +291,16 @@ 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: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to 'expr[*]...' string.""" - 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}" + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + 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/literal_rules.py b/hcl2/rules/literal_rules.py index 317d149e..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=SerializationOptions(), context=SerializationContext()) -> 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,8 +43,11 @@ def lark_name() -> str: """Return the grammar rule name.""" return "literal_value" - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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 if context.inside_dollar_string: return str(value) @@ -75,8 +80,12 @@ def lark_name() -> str: """Return the grammar rule name.""" return "float_lit" - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() 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..eed654d4 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,10 +65,14 @@ def expression(self): """Return the interpolated expression.""" return self.children[1] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> Any: + def serialize( + self, options: Optional[SerializationOptions] = None, context: Optional[SerializationContext] = None + ) -> Any: """Serialize to ${expression} string.""" - with context.modify(inside_dollar_string=True): - return to_dollar_string(self.expression.serialize(options, context)) + options = options if options is not None else SerializationOptions() + context = context if context is not None else SerializationContext() + inner = context.replace(inside_dollar_string=True) + return to_dollar_string(self.expression.serialize(options, inner)) class StringPartRule(LarkRule): @@ -92,8 +96,12 @@ 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: 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() return self.content.serialize(options, context) @@ -112,7 +120,9 @@ 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: 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 @@ -121,6 +131,8 @@ 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)`). """ + 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( self._serialize_part_as_value(part, options, context) for part in self.string_parts @@ -161,8 +173,12 @@ def heredoc(self): """Return the raw heredoc token.""" return self.children[0] - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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() heredoc = self.heredoc.serialize(options, context) if not options.preserve_heredocs: @@ -194,8 +210,12 @@ 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: 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() # 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,13 +286,16 @@ def inner_value(self) -> str: return raw[2:-2] return raw - def serialize(self, options=SerializationOptions(), context=SerializationContext()) -> 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 \\" 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 cb43590b..8221e240 100644 --- a/hcl2/rules/whitespace.py +++ b/hcl2/rules/whitespace.py @@ -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=SerializationOptions(), context=SerializationContext()) -> 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) @@ -36,8 +38,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/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/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 new file mode 100644 index 00000000..16aebd5c --- /dev/null +++ b/test/unit/test_thread_safety.py @@ -0,0 +1,311 @@ +# 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` 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 +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. +""" + +import dataclasses +import inspect +import threading +from concurrent.futures import ThreadPoolExecutor +from importlib import import_module +from unittest import TestCase + +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 +# 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' + + +def _parameters_named(*names): + """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. 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 + + 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, member in vars(cls).items(): + method = getattr(member, "__func__", member) + 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 + + 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") + + +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 test_every_context_parameter_defaults_to_none(self): + offenders = [ + name + for name, parameter in _parameters_named("context") + 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. 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.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. + """ + # 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): + """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_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": + with self.assertRaises(dataclasses.FrozenInstanceError): + context.inside_dollar_string = True + barrier.wait() + observed["mutator-could-not-set"] = 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-could-not-set": False, "observer-saw": False}) + + +class TestNoDefaultOptionsIsShared(TestCase): + """`options` carried the same declaration, and loses it for the same reason. + + 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 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, []) + + def test_the_walk_actually_found_the_methods(self): + 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, []) + + +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): 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