fix: annotate the query views with the classes they return (#328)

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))

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

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

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",
+                )