diff --git a/CHANGELOG.md b/CHANGELOG.md index 87ceca04..bddd5471 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## \[Unreleased\] +### Changed + +- `blocks()` and `attributes()` on the query views are annotated as returning `List[BlockView]` and `List[AttributeView]` rather than `List[NodeView]`, and `BlockView.body` as `BodyView`. Each only ever returns the concrete class; the wider annotation put `block_type`, `labels`, `name_labels` and `AttributeView.name` behind an `isinstance` narrowing or a cast for callers under a strict type checker. The view classes are now imported at module level rather than inside each method, so `typing.get_type_hints` can resolve the annotations the way any consumer reads them. Thanks, @livingstaccato ([#332](https://github.com/amplify-education/python-hcl2/pull/332)) + - Runtime behaviour and the values returned are unchanged, but this is a static break for one shape of caller: `list` is invariant, so `views: List[NodeView] = document.blocks()` no longer type-checks even though `BlockView` is a `NodeView`. Narrow the annotation, drop it, or use `Sequence[NodeView]`. ### Fixed - Parse blocks whose type or unquoted label is an HCL keyword, such as the `in` block of the Snowflake provider's `snowflake_schemas` data source. HCL does not reserve its keywords, so `if`, `in`, `for`, `for_each`, `else`, `endif`, `endfor`, `true`, `false`, and `null` are now accepted in every block label position and normalized to identifiers — matching the existing behaviour for keyword attribute names. The block-side grammar gap was diagnosed independently in [#355](https://github.com/amplify-education/python-hcl2/pull/355). ([#357](https://github.com/amplify-education/python-hcl2/pull/357)) diff --git a/hcl2/query/blocks.py b/hcl2/query/blocks.py index 269f2209..260de9ac 100644 --- a/hcl2/query/blocks.py +++ b/hcl2/query/blocks.py @@ -4,6 +4,7 @@ from hcl2.const import COMMENTS_KEY from hcl2.query._base import NodeView, register_view +from hcl2.query.attributes import AttributeView from hcl2.rules.abstract import LarkElement from hcl2.rules.base import BlockRule from hcl2.rules.literal_rules import IdentifierRule @@ -54,10 +55,8 @@ def name_labels(self) -> List[str]: return self.labels[1:] @property - def body(self) -> "NodeView": + def body(self) -> "BodyView": """Return the block body as a BodyView.""" - from hcl2.query.body import BodyView - node: BlockRule = self._node # type: ignore[assignment] return BodyView(node.body) @@ -76,23 +75,23 @@ def to_dict(self, options: Optional[SerializationOptions] = None) -> Any: result[COMMENTS_KEY] = self._adjacent_comments + existing return result - def blocks(self, block_type: Optional[str] = None, *labels: str) -> List["NodeView"]: + def blocks(self, block_type: Optional[str] = None, *labels: str) -> List["BlockView"]: """Delegate to body.""" - from hcl2.query.body import BodyView - node: BlockRule = self._node # type: ignore[assignment] return BodyView(node.body).blocks(block_type, *labels) - def attributes(self, name: Optional[str] = None) -> List["NodeView"]: + def attributes(self, name: Optional[str] = None) -> List["AttributeView"]: """Delegate to body.""" - from hcl2.query.body import BodyView - node: BlockRule = self._node # type: ignore[assignment] return BodyView(node.body).attributes(name) - def attribute(self, name: str) -> Optional["NodeView"]: + def attribute(self, name: str) -> Optional["AttributeView"]: """Delegate to body.""" - from hcl2.query.body import BodyView - node: BlockRule = self._node # type: ignore[assignment] return BodyView(node.body).attribute(name) + + +# See the note in `hcl2/query/body.py`: the two modules name each other in their +# annotations, and binding the name here rather than inside each method is what +# lets `typing.get_type_hints` resolve them. +from hcl2.query.body import BodyView # noqa: E402 pylint: disable=wrong-import-position,cyclic-import diff --git a/hcl2/query/body.py b/hcl2/query/body.py index b9f2ce54..a3415d17 100644 --- a/hcl2/query/body.py +++ b/hcl2/query/body.py @@ -3,6 +3,7 @@ from typing import List, Optional from hcl2.query._base import NodeView, register_view +from hcl2.query.attributes import AttributeView from hcl2.rules.base import AttributeRule, BlockRule, BodyRule, StartRule from hcl2.rules.whitespace import NewLineOrCommentRule @@ -59,15 +60,15 @@ def body(self) -> "BodyView": node: StartRule = self._node # type: ignore[assignment] return BodyView(node.body) - def blocks(self, block_type: Optional[str] = None, *labels: str) -> List["NodeView"]: + def blocks(self, block_type: Optional[str] = None, *labels: str) -> List["BlockView"]: """Return matching blocks, delegating to body.""" return self.body.blocks(block_type, *labels) - def attributes(self, name: Optional[str] = None) -> List["NodeView"]: + def attributes(self, name: Optional[str] = None) -> List["AttributeView"]: """Return matching attributes, delegating to body.""" return self.body.attributes(name) - def attribute(self, name: str) -> Optional["NodeView"]: + def attribute(self, name: str) -> Optional["AttributeView"]: """Return a single attribute by name, or None.""" return self.body.attribute(name) @@ -76,12 +77,10 @@ def attribute(self, name: str) -> Optional["NodeView"]: class BodyView(NodeView): """View over an HCL2 body (BodyRule).""" - def blocks(self, block_type: Optional[str] = None, *labels: str) -> List["NodeView"]: + def blocks(self, block_type: Optional[str] = None, *labels: str) -> List["BlockView"]: """Return blocks, optionally filtered by type and labels.""" - from hcl2.query.blocks import BlockView - node: BodyRule = self._node # type: ignore[assignment] - results: List[NodeView] = [] + results: List["BlockView"] = [] for child in node.children: if not isinstance(child, BlockRule): continue @@ -98,12 +97,10 @@ def blocks(self, block_type: Optional[str] = None, *labels: str) -> List["NodeVi results.append(block_view) return results - def attributes(self, name: Optional[str] = None) -> List["NodeView"]: + def attributes(self, name: Optional[str] = None) -> List["AttributeView"]: """Return attributes, optionally filtered by name.""" - from hcl2.query.attributes import AttributeView - node: BodyRule = self._node # type: ignore[assignment] - results: List[NodeView] = [] + results: List["AttributeView"] = [] for child in node.children: if not isinstance(child, AttributeRule): continue @@ -114,7 +111,15 @@ def attributes(self, name: Optional[str] = None) -> List["NodeView"]: results.append(attr_view) return results - def attribute(self, name: str) -> Optional["NodeView"]: + def attribute(self, name: str) -> Optional["AttributeView"]: """Return a single attribute by name, or None.""" attrs = self.attributes(name) return attrs[0] if attrs else None + + +# `BlockView` subclasses nothing here but names `BodyView` in its own annotations, +# so the two modules refer to each other. Importing at the bottom -- after both +# classes exist -- breaks the cycle while still binding the name in this module's +# globals, which is where `typing.get_type_hints` looks. Deferring it into the +# methods instead would leave the public annotations unresolvable to any caller. +from hcl2.query.blocks import BlockView # noqa: E402 pylint: disable=wrong-import-position,cyclic-import diff --git a/test/unit/query/test_view_annotations.py b/test/unit/query/test_view_annotations.py new file mode 100644 index 00000000..ae01d3a8 --- /dev/null +++ b/test/unit/query/test_view_annotations.py @@ -0,0 +1,200 @@ +# pylint: disable=C0103,C0114,C0115,C0116 +"""The query views' return annotations name the class they actually return. + +`blocks()` only ever appends a `BlockView` and `attributes()` only ever an +`AttributeView`, but both were annotated `List[NodeView]`. Under a strict type +checker that put `block_type`, `labels`, `name_labels` and `AttributeView.name` +out of reach without an `isinstance` narrowing or a cast for a runtime type +that is never anything else. + +These assert the annotations rather than the runtime types, because a runtime +check passes either way -- it is only the declaration that was wrong. + +They resolve them the way a consumer does: a bare `get_type_hints`, with no +namespace supplied. Passing one would hide a name the annotation cannot reach +on its own, which is the failure mode a forward reference invites. +""" + +import subprocess +import sys +from pathlib import Path +from typing import List, Optional, get_type_hints +from unittest import TestCase + +import hcl2 +from hcl2.query import blocks as blocks_module +from hcl2.query import body as body_module +from hcl2.query.attributes import AttributeView +from hcl2.query.blocks import BlockView +from hcl2.query.body import BodyView, DocumentView + +REPO_ROOT = Path(hcl2.__file__).resolve().parent.parent + + +def _returns(method): + return get_type_hints(method)["return"] + + +class TestBodyViewAnnotations(TestCase): + def test_blocks_returns_block_views(self): + self.assertEqual(_returns(BodyView.blocks), List[BlockView]) + + def test_attributes_returns_attribute_views(self): + self.assertEqual(_returns(BodyView.attributes), List[AttributeView]) + + def test_attribute_returns_an_optional_attribute_view(self): + self.assertEqual(_returns(BodyView.attribute), Optional[AttributeView]) + + +class TestDocumentViewAnnotations(TestCase): + """The document-level methods delegate to the body and must not re-widen.""" + + def test_blocks_returns_block_views(self): + self.assertEqual(_returns(DocumentView.blocks), List[BlockView]) + + def test_attributes_returns_attribute_views(self): + self.assertEqual(_returns(DocumentView.attributes), List[AttributeView]) + + def test_attribute_returns_an_optional_attribute_view(self): + self.assertEqual(_returns(DocumentView.attribute), Optional[AttributeView]) + + +class TestBlockViewAnnotations(TestCase): + def test_blocks_returns_block_views(self): + self.assertEqual(_returns(BlockView.blocks), List[BlockView]) + + def test_attributes_returns_attribute_views(self): + self.assertEqual(_returns(BlockView.attributes), List[AttributeView]) + + def test_attribute_returns_an_optional_attribute_view(self): + self.assertEqual(_returns(BlockView.attribute), Optional[AttributeView]) + + def test_body_returns_a_body_view(self): + self.assertEqual(_returns(BlockView.body.fget), BodyView) + + +class TestAnnotationsMatchRuntime(TestCase): + """The declarations above are only worth having if they stay true.""" + + SOURCE = 'resource "aws_instance" "web" {\n ami = "ami-1"\n}\n' + + def test_blocks_are_block_views(self): + doc = DocumentView.parse(self.SOURCE) + self.assertTrue(all(isinstance(block, BlockView) for block in doc.blocks())) + + def test_attributes_are_attribute_views(self): + doc = DocumentView.parse(self.SOURCE) + block = doc.blocks("resource")[0] + self.assertTrue(all(isinstance(attr, AttributeView) for attr in block.attributes())) + + def test_block_body_is_a_body_view(self): + doc = DocumentView.parse(self.SOURCE) + self.assertIsInstance(doc.blocks("resource")[0].body, BodyView) + + +class TestAnnotationsResolveUnaided(TestCase): + """The names the annotations use have to live in the defining module. + + `get_type_hints` reads a function's own globals. While the view classes were + imported inside the methods, every one of these annotations raised + `NameError` for anyone who introspected them -- pydantic, a documentation + builder, a runtime validator -- even though the classes were importable. + """ + + def test_body_module_binds_block_view(self): + self.assertIs(body_module.BlockView, BlockView) + + def test_blocks_module_binds_body_view(self): + self.assertIs(blocks_module.BodyView, BodyView) + + def test_every_annotated_member_resolves_without_a_namespace(self): + members = [ + BodyView.blocks, + BodyView.attributes, + BodyView.attribute, + DocumentView.blocks, + DocumentView.attributes, + DocumentView.attribute, + DocumentView.body.fget, + BlockView.blocks, + BlockView.attributes, + BlockView.attribute, + BlockView.body.fget, + ] + for member in members: + with self.subTest(member=member.__qualname__): + self.assertIn("return", get_type_hints(member)) + + +class TestTheyResolveWhicheverModuleLoadsFirst(TestCase): + """`body` and `blocks` name each other, so each imports the other last. + + That is what makes the cycle work: by the time either bottom import runs, + the importing module has already defined its own classes, so the other one + finds them. It is also what makes it fragile -- a class added *below* one + of those imports, or a tool that hoists it to the top, turns this into an + `ImportError` or an annotation nothing can resolve. + + A single process cannot see any of that: whichever module the suite + imported first is cached for every test after it. So each entry point gets + a fresh interpreter, and the assertions above are re-run inside it. + """ + + ENTRY_POINTS = ( + "hcl2", + "hcl2.query", + "hcl2.query.body", + "hcl2.query.blocks", + "hcl2.query.attributes", + ) + + PROBE = """ +import importlib, sys +importlib.import_module({entry!r}) + +from typing import List, Optional, get_type_hints +from hcl2.query.attributes import AttributeView +from hcl2.query.blocks import BlockView +from hcl2.query.body import BodyView, DocumentView +import hcl2.query.blocks as blocks_module +import hcl2.query.body as body_module + +assert body_module.BlockView is BlockView +assert blocks_module.BodyView is BodyView + +assert get_type_hints(BodyView.blocks)["return"] == List[BlockView] +assert get_type_hints(BodyView.attributes)["return"] == List[AttributeView] +assert get_type_hints(BodyView.attribute)["return"] == Optional[AttributeView] +assert get_type_hints(BlockView.blocks)["return"] == List[BlockView] +assert get_type_hints(BlockView.attributes)["return"] == List[AttributeView] +assert get_type_hints(BlockView.body.fget)["return"] is BodyView +assert get_type_hints(DocumentView.blocks)["return"] == List[BlockView] +""" + + def test_each_entry_point(self): + for entry in self.ENTRY_POINTS: + with self.subTest(entry=entry): + result = subprocess.run( + [sys.executable, "-c", self.PROBE.format(entry=entry)], + cwd=REPO_ROOT, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 0, result.stderr) + + def test_the_imports_stay_below_the_classes(self): + """The ordering the cycle depends on, stated where it can be checked. + + Both modules must import the other after their last `class`, so a + class appended to either file lands above it rather than below. + """ + for module, imported in (("body", "blocks"), ("blocks", "body")): + with self.subTest(module=module): + source = (REPO_ROOT / "hcl2" / "query" / f"{module}.py").read_text() + import_line = f"from hcl2.query.{imported} import " + self.assertGreater( + source.index(import_line), + source.rindex("\nclass "), + f"hcl2/query/{module}.py must import {imported} below its classes", + )