fix[eve]: validate 'type[X]' annotations by subclass, not by 'is a class' by egparedes · Pull Request #2843 · GridTools/gt4py

egparedes · 2026-08-27T17:06:06Z

eve.type_validation had no case for type[X], so the annotation fell through to the
custom-generic-type branch and validated only isinstance(value, type). A DataModel field
annotated type[Foo] therefore accepted any class at all, including int:

class Holder(datamodels.DataModel):
    dim: type[common.Dimension]

Holder(dim=int)   # accepted before this change

Adds a type[X] case that checks the actual subclass relationship:

type[SomeClass] validates by issubclass.
type[A | B] validates as an OR of subclass checks.
type[SomeAlias] resolves a nested PEP 695 alias first — whole-annotation alias resolution
runs once at the top of the factory and does not reach inside type[...].
type[T] honours T.__bound__ when there is one, mirroring the plain-TypeVar branch.
Bare type / typing.Type, type[Any] and unbound TypeVars keep the loose "is a class"
check, which is all they can mean.
type[SomeProtocol] also keeps the loose check. issubclass is not generally usable with
protocols: it is rejected outright unless the protocol is @runtime_checkable, and rejected
again for @runtime_checkable protocols that have non-method members. A strict check would
raise TypeError for every value, making the field unusable.

Any other shape falls back to that same loose check rather than raising, so no annotation that
validated before can start failing — neither at class-creation time nor at validation time.

One existing field becomes strictly validated: ts.DeferredType.constraint, the only
type[...] annotation on any DataModel in the tree. It was already correct — every in-tree
constraint= call site passes a ts.*Type class, None, or a tuple of them.

Also fixes documentation drift found alongside:

docs/development/ADRs/next/README.md linked two files that do not exist (the 0026 entry
pointed at 0024-Staggered_Dimensions.md, the 0012 entry at 0011-_GridTools_Cpp_OTF.md),
listed ADR 0018 twice, and omitted six ADRs entirely: 0014, 0019, 0020, 0021, 0024, 0025.
common.connectivity_for_cartesian_shift cited ADR 0024 (Compilation Runners) for the
staggered-index convention; that convention is ADR 0026.
Records the mypy plugin's undocumented *Dim naming requirement
(fullname.endswith("Dim")), which our own QuickstartGuide violated.

Prerequisite for #2844, which annotates every dimension-typed DataModel field as
type[common.Dimension] — including CartesianConnectivity.domain_dim: type[DomainDimT],
which relies on the TypeVar bound being honoured.

egparedes · 2026-08-27T18:05:15Z

cscs-ci run default

Copilot

Pull request overview

This PR fixes a validation gap in gt4py.eve.type_validation where annotations of the form type[X] were previously only checked as “is any class” (isinstance(value, type)), and now correctly validate “is a subclass of X” (via issubclass). It also updates gt4py.next documentation references: correcting an ADR citation in code comments and repairing/expanding the docs/development/ADRs/next index.

Changes:

Add dedicated handling for type[X] annotations in Eve type validation (strict issubclass for concrete X, permissive “any class” for type[Any] and type[TypeVar]).
Add unit tests covering type[...] validation behavior.
Fix ADR references: update an incorrect ADR number in common.connectivity_for_cartesian_shift, and correct/complete the ADR index; document a known mypy plugin limitation.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
tests/eve_tests/unit_tests/test_type_validation.py	Adds test cases ensuring `type[X]` enforces subclass validation and `type[Any]` remains permissive.
src/gt4py/eve/type_validation.py	Implements `type[X]`-specific validation and introduces a `make_is_subclass_of` validator.
src/gt4py/next/common.py	Corrects the cited ADR number for the staggered-index convention in `connectivity_for_cartesian_shift`.
docs/development/ADRs/next/README.md	Fixes broken ADR links and adds missing ADR entries to the index.
src/gt4py/next/type_system/mypy_plugin.py	Documents a known (transitional) naming-based limitation in the plugin behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

egparedes

Self-review of the new type[X] branch. Two findings, each verified by running the shapes against this branch and against upstream/main.

egparedes · 2026-08-28T13:12:08Z

+                # `type[X]`. Without this case the annotation falls through to the
+                # generic-collection branch below and degrades to `isinstance(value, type)`,
+                # i.e. "is any class at all".
+                if len(type_args) != 1:


Regression: shapes that validated fine on main now raise at class-creation time.

Three annotation shapes reach this branch and fall through to the EveValueError, where before they landed in the generic-collection fallback and got loose isinstance(value, type) validation:

type[A | B] — one arg, but it is a UnionType, so is_actual_type is false

bare typing.Type — no args at all, so len(type_args) != 1

type[SomeAlias] where SomeAlias is a PEP 695 type X = ... — the arg is never resolved

Verified by running all three against upstream/main (all create fine) and against this branch (all raise EveValueError). No in-repo field uses these shapes, so the suite stays green, but it is an import-time break for downstream code.

The PEP 695 case is the one that stings: alias resolution happens once at the top of the factory for the whole annotation, so an alias nested inside type[...] is never resolved. This branch is a fourth annotation-dispatch funnel and needs the same treatment as the other three.

Fix: handle bare type, resolve the argument, OR-combine subclass validators for unions, and fall back to make_is_instance_of(name, type) rather than raising — never regress a shape that used to work.

Fixed in 759715fad. Unrecognized type[X] shapes now fall back to make_is_instance_of(name, type) instead of raising, so nothing that validated before can break at class-creation time. Unions of classes validate as an OR of subclass checks, and a nested PEP 695 alias is resolved with eval_type_alias first.

Regression tests added for type, typing.Type, type[A | B] and type[SomeAlias] — 8 of the new parametrizations fail against the previous version of this branch and pass now.

egparedes · 2026-08-28T13:12:08Z

+                    raise exceptions.EveValueError(
+                        f"{type_annotation} type annotation is not supported."
+                    )
+                if xtyping.is_Any(type_args[0]) or isinstance(type_args[0], typing.TypeVar):


type[T] ignores the TypeVar bound.

This collapses every TypeVar to "is any class", so with T = TypeVar("T", bound=Dimension) a field annotated type[T] accepts int. The plain-TypeVar branch a few lines above does honour __bound__, so the two paths disagree.

Not a regression — main behaves the same way — but it matters for #2844, which annotates CartesianConnectivity.domain_dim: type[DomainDimT] and codomain: type[DimT] with DimT bound to Dimension. Those fields would get no real validation.

Fix: make_is_subclass_of(name, arg.__bound__) when a bound is present, mirroring the branch above.

Fixed in 759715fad. type[T] now uses make_is_subclass_of(name, T.__bound__) when a bound is present, matching the plain-TypeVar branch a few lines above; unbound TypeVars keep the loose "is a class" check. Covered by new cases for both a bound and an unbound TypeVar.

havogt · 2026-08-28T14:16:46Z

+                # Every shape that is not recognized here falls back to that same loose
+                # check rather than raising: `type[X]` annotations validated (loosely)
+                # before this branch existed, so refusing one now would turn a working
+                # downstream datamodel into an error at class-creation time.


do we need this? What are the examples?

Let's drop this paragraph

Dropped in bb848a08c — the paragraph is gone, the fallback behaviour it described stays.

My question was actually: why not raise? Because we don't need to be backward compatible I believe.

I'm not sure if understand the question. IIUC, the paragraph documented the current behavior, I don't see how the changes in that function are keeping backward compatibility with anything....

havogt

Read-only review of this PR only (#2844/#2845 read for context, not reviewed). The change is correct and does what the title says — the type[X] branch behaves as described for every shape I probed: subclass check for a plain class, OR-of-subclass for type[A | B] and type[Union[A, B]], nested PEP 695 alias resolved first, T.__bound__ honoured, and type[Any] / bare type / typing.Type / unbound T kept loose. Optional[type[X]] works through the Union branch, and forward references are not a gap — type["Later"] in a DataModel resolves via the deferred-validator path and validates strictly.

Four comments below: one correction to the PR description, one leftover in the file this PR is fixing, one latent trap that #2844/#2845 make reachable, and a coverage note.

Ran locally, on 759715fa:

uv run pytest tests/eve_tests/                       462 passed in 10.29s
uv run mypy src/gt4py/eve/type_validation.py \
            src/gt4py/next/type_system/mypy_plugin.py    Success, no issues
uv run pre-commit run --files <the 5 changed files>      all hooks Passed

CI: I queried check-runs on the head commit and filtered explicitly for non-success conclusions. There are no failures and no completed test job — 3× get-python-versions success, and test-package 3.12/3.14, Build Python distribution, define-test-sessions-exclusions and cscs/default all still queued/pending. So please do not read this review as "reviewed against green CI"; the local runs above are the only executed evidence.

havogt · 2026-08-28T14:20:38Z

                )
                return self.combine_optional(name, validator) if has_none else validator

+            if origin_type is type:


PR description correction — the body says two fields become strictly validated, ts.DeferredType.constraint and ffront.stages.node_class. Only the first one does.

DSLFieldOperatorDef is a plain @dataclasses.dataclass(frozen=True) (stages.py:35,74), not an eve.datamodels.DataModel, and eve.type_validation is imported by exactly one module — eve/datamodels/core.py. A plain dataclass field never reaches this factory, so node_class is unaffected.

I imported all of gt4py (0 import failures) and walked every DataModel subclass's __datamodel_fields__ looking for a type[...] annotation. There is exactly one in the tree:

gt4py.next.type_system.type_specifications.DeferredType.constraint Union[type[TypeSpec], tuple[type[TypeSpec], ...], None]

That one behaves as the body claims, verified end to end:

DeferredType(constraint=ts.ScalarType) ACCEPTED DeferredType(constraint=None) ACCEPTED DeferredType(constraint=(ts.ScalarType, ts.FieldType)) ACCEPTED DeferredType(constraint=int) TypeError <- the bug, now caught DeferredType(constraint=ts.ScalarType(...)) (instance) TypeError

and every in-tree constraint= call site passes a ts.*Type class, None, or a tuple of them, so nothing regresses. Only the node_class half of the sentence needs dropping.

You are right, and thank you for the receipt. I reproduced it independently: walking __datamodel_fields__ over every is_datamodel class in eve/_core/storage/cartesian/next (322 classes, recursing into get_args so nested type[...] counts) turns up exactly one field — DeferredType.constraint. And grep -rn "type_validation" src/gt4py --include=*.py has a single importer, eve/datamodels/core.py, so a plain frozen dataclass like DSLFieldOperatorDef never reaches the factory.

PR description corrected to name only DeferredType.constraint.

havogt · 2026-08-28T14:20:38Z

+- [0014 - DaCe backend](0014-DaCe_backend.md)
 - [0016 - Multiple Backends and Build Systems](0016-Multiple-Backends-and-Build-Systems.md)
 - [0017 - Toolchain Configuration](0017-Toolchain-Configuration.md)
 - [0018 - Canonical Form of an SDFG in GT4Py (Especially for Optimizations)](0018-Canonical_SDFG_in_GT4Py_Transformations.md)


This entry is a duplicate of line 44 (same title, same link) — 0018 is listed under both Transformations and Backends and Code Generation. After this PR the file has 27 ADR files and 28 index entries.

Since the PR exists to fix exactly this kind of index drift, worth removing one of the two while you are here.

Everything else in the doc fix checks out: I resolved every (*.md) target in the file against the directory listing — all links now exist, and no ADR file is missing from the index any more.

Fixed in bb848a08c — removed the 0018 entry under Backends and Code Generation, kept the one under Transformations. Now 27 files, 27 entries, 27 unique. Embarrassing miss in a PR whose whole point is index drift; the check I ran only looked for broken links and missing files, not duplicates. Added the uniqueness count to the check.

havogt · 2026-08-28T14:20:38Z

+        """Create a ``FixedTypeValidator`` validator for ``type[type_]`` annotations."""
+
+        def _is_subclass_of(value: Any, **kwargs: Any) -> None:
+            if not (isinstance(value, type) and issubclass(value, type_)):


issubclass is called unguarded here, and it raises for Protocols. A Protocol class passes xtyping.is_actual_type, so type[SomeProtocol] takes the strict path above and then fails for every value:

type[P] (Protocol, not runtime_checkable), value = a conforming class -> TypeError: Instance and class checks can only be used with @runtime_checkable protocols type[Q] (runtime_checkable, has a data member), value = a conforming class -> TypeError: Protocols with non-method members don't support issubclass()

That is inside the letter of "never reject a shape that used to validate" — it fails at validation time rather than class-creation time — but not its spirit, and the message points at issubclass rather than at the field.

Nothing breaks today: no DataModel in gt4py has such a field, and I checked the icon4py side too — its only type[...] annotations are two exc_type: type[BaseException] __exit__ parameters, neither a datamodel field. But #2844/#2845 add a lot of type[...] annotations and eve is a library, so this is worth closing now.

Cheapest fix: treat a Protocol as an unrecognized shape in the type[...] branch and fall back to the loose isinstance(value, type) check, same as the other unrecognized shapes.

Good catch, and confirmed — this was a real bug. Reproduced both variants against 759715fad before fixing:

type[P] plain Protocol TypeError: Instance and class checks can only be used with @runtime_checkable pro... type[Q] runtime+data member TypeError: Protocols with non-method members do not support issubclass()

both raised for a conforming class, i.e. the field was unusable for every value. Fixed in bb848a08c by routing all three strict-path decisions through

def is_strict_arg(a: Any) -> xtyping.TypeGuard[type]: return xtyping.is_actual_type(a) and not xtyping.is_protocol(a)

so a protocol falls back to the loose check like any other unsupported shape. xtyping.is_protocol already existed (re-exported from typing_extensions).

One judgement call worth your sign-off: this also loosens the case where issubclass does work — a @runtime_checkable protocol whose members are all methods. I chose the blanket fallback so type[P] does not silently change behaviour when someone later adds a non-method member to P (which would turn a working datamodel into a class-creation-time error). The narrow alternative would have to reach into __non_callable_proto_members__. Happy to switch if you prefer precision here.

havogt · 2026-08-28T14:20:38Z

    (typing.List[int], ([1, 2, 3], []), (1, [1.0]), None, None),
    (typing.Set[int], ({1, 2, 3}, set()), (1, [1], (1,), {1: None}), None, None),
    (typing.Dict[int, str], ({}, {3: "three"}), ([(3, "three")], 3, "three", []), None, None),
+    (type[SampleEmptyClass], [SampleEmptyClass], [SampleEmptyClass(), int, 3], None, None),


The table covers the recognized shapes well. Three gaps worth closing, the first one most:

Nothing asserts, at datamodel level, that DeferredType(constraint=int) now raises. That is the only behaviour change this PR actually causes anywhere in the tree, and it is the example in the PR description — but the assertion for it lives only in this annotation-level table, against SampleEmptyClass.

typing.Union[A, B] is not exercised — only the A | B spelling is. They are different runtime objects and the branch handles them on different paths (types.UnionType normalisation vs. get_origin(...) is Union).

The except TypeError fallback around eval_type_alias is untested. A recursive PEP 695 alias reaches it; I confirmed by hand that type[Rec] with type Rec = Rec falls back to the loose check rather than raising, but nothing in the suite pins that.

All three closed in bb848a08c, each verified to fail without the fix:

tests/next_tests/unit_tests/type_system_tests/test_type_specifications.py (new) asserts at datamodel level that DeferredType(constraint=int) raises, and that None, ts.ScalarType, ts.TypeSpec and tuples of them are still accepted. Put under next_tests rather than eve_tests because nothing in eve_tests imports gt4py.next today — tach would not have objected (source_roots is ["src"]), but the layering intent is clear.

type[typing.Union[A, B]] row added — with the branch disabled it fails, so it genuinely exercises the get_origin(...) is Union path rather than the types.UnionType one.

type Rec = Rec row added. Removing the try/except TypeError makes it fail with Type alias 'Rec' cannot be resolved (recursive definition) at validator-construction time.

…ass' 'type[X]' fell through to the custom-generic-type branch, which validates only 'isinstance(value, type)'. A DataModel field annotated 'type[Foo]' therefore accepted any class at all, including 'int'.

The next ADR index pointed at two files that do not exist and omitted eight ADRs entirely. 'connectivity_for_cartesian_shift' cited ADR 0024 (Compilation Runners) for the staggered-index convention, which is ADR 0026. Also records the mypy plugin's undocumented '*Dim' naming requirement.

Review follow-up on the new 'type[X]' case. It raised 'EveValueError' at class-creation time for three shapes that validated fine before it existed: - 'type[A | B]' -- one arg, but a 'UnionType', so not an "actual type" - bare 'typing.Type' / 'type' -- no args at all - 'type[SomeAlias]' with a PEP 695 'type X = ...' -- the argument was never resolved, because alias resolution runs once at the top of the factory for the *whole* annotation and does not reach inside 'type[...]' None of them is used by an in-repo field, so the suite stayed green, but each was an import-time break for downstream datamodels. Unrecognized shapes now fall back to the loose 'isinstance(value, type)' check this branch replaced, instead of raising. Unions of classes validate as an OR of subclass checks, and a nested PEP 695 alias is resolved first. Also honours the bound on 'type[T]', mirroring the plain-TypeVar branch above it: previously every TypeVar collapsed to "is any class", so a field annotated 'type[T]' with 'T' bound to 'Dimension' accepted 'int'.

…gaps Addresses the review feedback on the 'type[X]' validation branch: - Drop the second paragraph of the explanatory comment in the 'origin_type is type' branch, as requested. - Fall back to the loose "is a class" check when the argument of 'type[...]' is a protocol class. 'issubclass()' rejects protocols which are not '@runtime_checkable', and also '@runtime_checkable' protocols with non-method members, so the strict path used to raise 'TypeError' for *every* value of such a field. The check happens in the branch itself, next to the other strict-vs-loose decisions, rather than inside 'make_is_subclass_of'. - Test the 'typing.Union[A, B]' spelling of 'type[A | B]' (a different runtime object handled by a different code path than 'A | B') and the 'except TypeError' fallback around 'eval_type_alias', which an unresolvable (recursive) PEP 695 alias reaches. - Add a datamodel-level test for 'ts.DeferredType.constraint', the only field in the whole tree whose annotation contains a 'type[...]' and hence the only observable behaviour change of this PR. It lives under 'tests/next_tests' because 'tests/eve_tests' must not depend on 'gt4py.next'. - Remove the duplicate ADR 0018 entry from the 'gt4py.next' ADR index, which listed it both under "Transformations" and under "Backends and Code Generation".

This was referenced Aug 27, 2026

feat[next]: make a concrete Dimension a class and its instances the indices #2844

Open

test[next]: adopt class-style dimension declarations and add pyright coverage #2845

Open

egparedes requested a lite review from Copilot August 28, 2026 08:50

Copilot started reviewing on behalf of egparedes August 28, 2026 08:51 View session

Copilot AI reviewed Aug 28, 2026

View reviewed changes

egparedes force-pushed the dimensions-as-types-1-eve-and-docs branch from ce5dac0 to 3de6931 Compare August 28, 2026 13:00

egparedes commented Aug 28, 2026

View reviewed changes

havogt reviewed Aug 28, 2026

View reviewed changes

egparedes added 4 commits August 28, 2026 17:50

fix[eve]: validate 'type[X]' annotations by subclass, not by 'is a cl…

6791489

…ass' 'type[X]' fell through to the custom-generic-type branch, which validates only 'isinstance(value, type)'. A DataModel field annotated 'type[Foo]' therefore accepted any class at all, including 'int'.

egparedes force-pushed the dimensions-as-types-1-eve-and-docs branch from bb848a0 to 99e76ef Compare August 28, 2026 15:57

havogt approved these changes Aug 28, 2026

View reviewed changes

egparedes merged commit dfc8953 into main Aug 28, 2026
30 checks passed

Uh oh!

Conversation

egparedes commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

egparedes commented Aug 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

egparedes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

havogt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

egparedes commented Aug 27, 2026 •

edited

Loading