fix: annotate the query views with the classes they return (#328) - #332
livingstaccato wants to merge 5 commits into
Conversation
`BodyView.blocks()` only ever appends a `BlockView`, `attributes()` only an `AttributeView`, and `BlockView.body` is always a `BodyView`, but all of them were annotated `NodeView`. Callers under a strict type checker could not reach `block_type`, `labels`, `name_labels` or `AttributeView.name` without an `isinstance` narrowing or a cast for a runtime type that is never anything else. Narrow the annotations on `DocumentView`, `BodyView` and `BlockView`. The view classes stay imported inside the method bodies -- the cycle is real -- with `TYPE_CHECKING` imports added for the annotations alone, so there is no runtime change of any kind. The new tests assert the annotations rather than the runtime types: a runtime check passed before this change too, which is why nothing caught it.
The narrowed return annotations named `BlockView`, `BodyView` and `AttributeView` as forward references while the classes were imported inside each method. `typing.get_type_hints` reads a function's own globals, so every one of those annotations raised `NameError` for any caller that introspected it -- pydantic, a documentation builder, a runtime validator -- even though the classes were importable. Before the narrowing the annotations named `NodeView`, which is imported at module level, so this was a regression rather than a pre-existing gap. `AttributeView` has no cycle to break and moves to a plain top-level import. `BodyView` and `BlockView` do name each other, so each module imports the other at the bottom, after its own classes exist: the name lands in module globals, which is what resolution needs, and the cycle still cannot bite because neither import runs before the classes are defined. Verified under all three import orders. The tests asked for the hints with a hand-built `localns`, which supplied exactly the names that were missing and so could not see this. They now call `get_type_hints` bare, the way a consumer does.
|
Please hold off on merging this one for now — I want to do another review pass over it before it goes in. Opened as a draft for that reason; I will mark it ready and say so here once I am done. |
|
Review pass done, so the hold above no longer applies — this is ready for review now. Rebased on current 🤖 Drafted with Claude Code. |
|
hi there! i hope you're okay with this torrent of issues and PRs. if you'd rather me submit things/work through things differently please just let me know and i'll tweak my workflow. thanks for this project! :) |
…ducation#328) `body` and `blocks` name each other, so each imports the other below its own classes. That ordering is the whole reason the cycle resolves, and nothing asserted it: a class appended under either bottom import, or a tool that hoists the import to the top, turns the annotations back into something `get_type_hints` cannot resolve -- the defect this fixed. A single process cannot catch that, because whichever module the suite imported first stays cached for every test after it. So each of the five entry points -- `hcl2`, `hcl2.query`, and the three modules directly -- gets a fresh interpreter that re-runs the resolution assertions, and a second test states the below-the-classes rule where it can be checked. Hoisting the import fails the first; appending a class under it fails the second. Also records the static break in the CHANGELOG rather than only in the pull request: `views: List[NodeView] = document.blocks()` stops type-checking, since `list` is invariant. Confirmed with mypy, which suggests `Sequence` itself. The entry previously said only that the returned values are unchanged, which is true and not the part a caller needs warning about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Hi @kkozik-amplify, thank you for your patience while I worked through this batch. It landed all at once at the start of the month, and I'd rather work to your pace than keep adding to it. Since then I've tidied all twelve PRs:
If a suggested order helps, smallest and most independent first:
I'd also welcome any guidance for the future, both on contributing and on helping with issues. I've read your contributing guide in #321, and in hindsight I should have waited for agreement on the approach in each issue before opening PRs. Going forward I'm glad to work however suits you: discuss first and one PR at a time, or take only issues you point me at. The same goes for this batch. I'm happy to close anything you'd rather not take, split the larger heredoc PRs, or hold the rest until you've had a chance to look. Thanks again. |
Fixes #328.
What
BodyView.blocks()and.attributes(), and theirDocumentViewandBlockViewdelegates, were annotatedList[NodeView]. Each only ever returns the concrete class, so under a strict type checkerblock_type,labels,name_labelsandAttributeView.nameneeded anisinstancenarrowing or a cast for a runtime type that is never anything else. They now sayList[BlockView]/List[AttributeView], andBlockView.bodysaysBodyView.Why the second commit exists
Naming those classes as forward references is not enough on its own.
typing.get_type_hintsresolves an annotation against the defining function's own globals, and the view classes were imported inside each method to break thebody<->blocksimport cycle. So every narrowed annotation raisedNameErrorfor anything that introspected it -- pydantic, a docs builder, a runtime validator -- while the previousNodeViewannotation resolved fine, since that one is imported at module level.AttributeViewhas no cycle and moves to a plain top-level import.BodyViewandBlockViewdo name each other, so each module imports the other at the bottom, after its own classes exist: the name lands in module globals, and neither import can run before the classes are defined. Verified under all three import orders.Tests
test/unit/query/test_view_annotations.pyasserts the annotations rather than the runtime types, because a runtime check passed before the change too. They callget_type_hintsbare -- an earlier draft passed a hand-builtlocalns, which supplied exactly the names that were missing and so could not see the problem. Against the tree without the second commit, those tests produce 18 errors.Compatibility
Return values are unchanged; this narrows declarations and moves two imports.
It is a static break for one shape of caller, and the earlier wording here got that wrong.
listis invariant, soviews: List[NodeView] = document.blocks()stops type-checking even thoughBlockViewis aNodeView-- mypy reports Incompatible types in assignment (expression has type "list[BlockView]", variable has type "list[NodeView]"). Runtime behaviour is identical, and the fix for such a caller is to narrow the annotation or drop it. Annotating the return asSequence[BlockView]would keep those callers compiling, but it would also removeappendand friends from the declared contract, which is a bigger decision than this PR should make on its own.Merging
Up to date with
mainas of 2f6d718, by merging rather than rebasing (themerge=unionCHANGELOG duplicates entries under a rebase).It merges cleanly on its own, and all twelve open PRs are merged together, with the full suite passing on 3.8-3.14, at
livingstaccato/python-hcl2@int/pyvider-hcl-9. Landing alongside the others needs one small follow-up from whichever lands second:hcl2/query/blocks.py; keep both.This pull request, and the investigation behind it, were produced by an AI assistant (Claude) working on behalf of the author. Please review with that provenance in mind.