fix[eve]: handle all modern typing spellings at annotation dispatch sites - #2841
Conversation
f0b7b75 to
be948cb
Compare
havogt
left a comment
There was a problem hiding this comment.
All 4 of the 5 comments are actually 1. It seems we are missing an abstraction for the pattern "if Annotated then do something on the type, not the metadata".
I didn't pay too close attention to the test/test structure. Did you?
|
Three small things from reviewing this at 1. assert len(type_args) == 1
make_recursive(type_args[0]) # <- result discarded
if (member_validator := make_recursive(type_args[0])) is None:The first call's result is thrown away and the same validator is built again on the next line, so every 2. if (
isinstance(value, (StdGenericAliasType, _TypingSpecialFormType, _types.UnionType))
or is_type_alias(value)
or get_origin(value) is not None
):
3. Minor — part of the
Confirmed by mutation: reverting any of the Not a defect — the direct-call observers are exactly the compensation, and the file is upfront about why they exist. The gap is that |
be948cb to
c597562
Compare
|
Thanks — all three addressed, and the abstraction you asked for is in. Rebased onto current 1. Duplicated 2. 3. The On your question about the test structure: it was worth asking. Three things surfaced after your review that the suite was not catching:
One correction to my own earlier text: I wrote " Also recorded, not fixed: the Verified on 3.12, 3.13 and 3.14: 851 passed + 56 doctests each; |
|
cscs-ci run default |
egparedes
left a comment
There was a problem hiding this comment.
Address my comments to make the source code comments more concise.
| # A union of two or more non-'None' types has no single type to coerce to. | ||
| # Saying so explicitly is what keeps this branch honest on Python 3.14, where | ||
| # 'typing.Union' *is* 'types.UnionType' -- a real class, and therefore accepted | ||
| # by the 'is_actual_type(origin_type)' fallback below, which would build a | ||
| # converter calling 'types.UnionType(value)'. That is the same silent breakage | ||
| # this function already avoids for 'X | Y' on 3.12, reached by another route. |
There was a problem hiding this comment.
Too verbose, rewrite it in a concise way .
There was a problem hiding this comment.
Cut from six lines to three — kept the 3.14 reason, dropped the "same breakage by another route" aside.
| The extra arguments of an ``Annotated`` are metadata values, not types, so every | ||
| funnel that dispatches on an annotation has to look straight through them to | ||
| ``X``. What makes this worth naming is that the failure is quiet, and quiet in a | ||
| different way on each interpreter. On 3.12 ``typing.Annotated`` is a class, so a | ||
| funnel dispatching on "is this a class" claims it by accident and builds | ||
| something meaningless: a converter calling ``typing.Annotated(value)``, an | ||
| ``isinstance`` check no value can satisfy, or a mutability verdict read off the | ||
| metadata instead of the type. From 3.13 on it is a special form again, so those | ||
| funnels stop claiming it and it falls through to a no-match branch instead -- | ||
| a raise, or a wrong answer, depending on the funnel. | ||
|
|
||
| ``typing`` flattens nested ``Annotated``, so one step is always enough. Any other | ||
| annotation is returned unchanged (as the identical object), which lets a caller | ||
| detect the no-op with ``is``. |
There was a problem hiding this comment.
The content of the comment is good but the style is too verbose. Make it concise
There was a problem hiding this comment.
Tightened to nine lines from fourteen, with the content intact: both interpreter regimes and the "one step is enough" note are still there, just said once each.
| # An annotation passed in as a value is returned unchanged. Only the PEP 585 | ||
| # spelling ('list[int]') and the bare special forms used to be recognized here: a | ||
| # parametrized 'typing' alias ('typing.List[int]', 'Optional[int]'), a PEP 604 | ||
| # union ('int | str') and a PEP 695 alias all fell through to the 'type(value)' | ||
| # fallback below and were reported as the CPython-internal class implementing | ||
| # them ('typing._GenericAlias', 'types.UnionType', ...). |
There was a problem hiding this comment.
Make the comment document the current behavior, not the history of how it arrived to this state.
There was a problem hiding this comment.
Rewritten to state what the check does — which annotation shapes are returned unchanged, and what the type(value) fallback would otherwise report. No more "used to be".
c597562 to
a009efc
Compare
|
Done — all three rewritten, pushed in I also applied the same standard to the comments you did not flag but which were mine in that style, so you do not have to point at each one: the self-referential-alias guard (7 lines → 4), the Two I compressed rather than cut, since earlier review rounds turned on them: the ordering note in the Re-verified: |
62880cd to
ed1eac2
Compare
|
Two nits from re-reviewing at nit 1 — The docstring says any other annotation "is returned unchanged (as the identical object)", which invites callers to detect the no-op with Measured by wrapping the function and counting calls where the result
The results are equivalent, so nothing is wrong today — no in-tree caller identity-checks this particular return value ( nit 2 — Building a chain of PEP 695 aliases (
Same boundary for the interleaved form ( For what it is worth, the rest held up: One correction to my earlier round: I had flagged the |
havogt
left a comment
There was a problem hiding this comment.
lgtm, see comment just before this one for 2 small things, you can decide what to do with it.
'eve' builds datamodel validators and converters at runtime from live annotation objects, dispatching on annotation identity at several independent sites. Python has more than one spelling for the same type, and a site that has not been taught one does not raise: it falls through to a no-match branch and silently builds the wrong thing. Teach the funnels the spellings they were missing: - '_make_type_converter' compared against 'typing.Union' only, so 'int | None' (whose origin is 'types.UnionType') fell through to 'is_actual_type(origin_type)' -- which accepts 'types.UnionType', since it is a real class -- and built a converter calling 'types.UnionType(value)'. Class creation succeeded and every instantiation raised. Add 'xtyping.normalize_union' and call it at the head of the funnel, so the recursive calls are covered too. - The same function's Optional branch took 'args[0]' as the wrapped type, but 'typing' preserves the written order, so 'Union[None, int]' recursed into 'NoneType' and raised. - Neither '_make_type_converter' nor the validator funnel had an 'Annotated' branch. 'typing.Annotated' is a class since 3.12, so both accepted it as an ordinary type: the converter built one calling 'typing.Annotated(value)', and the validator built an 'is instance of typing.Annotated' check that no value can satisfy. - 'get_represented_types' reported the 'Annotated' special form itself as the represented type, and returned an empty tuple for 'Literal', whose arguments are values rather than types. Callers turn the result into an 'isinstance()' argument, so both made every check silently false. - '_is_strictly_immutable_type' had no 'Annotated' branch either, so it type-checked the metadata and reported immutable fields as mutable. - 'infer_type' passed through only the PEP 585 spelling, reporting 'typing.List[int]', 'Optional[int]', 'int | str' and PEP 695 aliases as the CPython-internal class implementing them. 'type_validation' keeps its behaviour; its inline union normalization is replaced by the shared helper. Add 'tests/eve_tests/unit_tests/test_annotation_spelling.py': a conformance matrix crossing every spelling with every funnel, asserting that two spellings of one type are indistinguishable. It is written as an agreement property rather than against fixed expectations, so it keeps working as funnels are added -- adding one means adding an observer. Two funnels are observed by calling them directly rather than through a datamodel, because 'get_partial_type_hints' strips 'Annotated' before a field annotation reaches them: routed through a datamodel those observers compare 'X' against 'X' and cannot fail. Each fix here was checked by reverting it alone and confirming the matrix goes red. 'infer_type' is not a matrix column: it returns annotations unchanged, so two spellings correctly give two different objects. It gets its own pass-through and value-inference tests instead.
ed1eac2 to
2f4b999
Compare
|
Thanks — both verified, and they split differently than they look. nit 1: real, and mine. Fixed. Reproduced exactly as you describe:
if isinstance(annotation, _types.UnionType) and get_origin(annotation) is not _typing.Union:so the Pinned by nit 2: pre-existing, and not The boundary is
I left it alone: changing that loop's bound in a PR about annotation dispatch would need its own test, and nothing quotes the number in a message or a doc. Happy to fix it as a one-liner separately if you would rather have it exact. And thanks for the correction on the Rebased onto |
`DimensionType: TypeAlias = type[DimensionBase]` names "a concrete dimension, i.e. the class itself", and replaces the 208 `type[Dimension]` annotations in `src/gt4py/next`. It is spelled against `DimensionBase` rather than `Dimension` so that parameterized dimension types, which are not user-declarable, are admitted once they exist. Exported as `gtx.DimensionType`. Two exceptions keep the narrower `type[Dimension]`, both noted in place: the `Dimension(...)` factory overload, which always returns a user-declarable dimension and whose exact type call sites assert, and `TYPE_BUILTINS`, which derives DSL names from `__name__`. `DimensionBase` also gains an explicit `value`. The metaclass reads `value` in `__repr__`, `__eq__` and `__hash__`, but `__init_subclass__` only sets it on *subclasses*, so hashing or printing the root itself raised `AttributeError`. Nothing reached it before; a `type[DimensionBase]` annotation does, because `eve` memoizes its validator factory on the annotation object. A `TypeAlias` and not a PEP 695 `type` statement: `src/` deliberately has no PEP 695 aliases, and #2841 documented eight latent `eve` dispatch gaps that need one to trigger.
`DimensionType: TypeAlias = type[DimensionBase]` names "a concrete dimension, i.e. the class itself", and replaces the 208 `type[Dimension]` annotations in `src/gt4py/next`. It is spelled against `DimensionBase` rather than `Dimension` so that parameterized dimension types, which are not user-declarable, are admitted once they exist. Exported as `gtx.DimensionType`. Two exceptions keep the narrower `type[Dimension]`, both noted in place: the `Dimension(...)` factory overload, which always returns a user-declarable dimension and whose exact type call sites assert, and `TYPE_BUILTINS`, which derives DSL names from `__name__`. `DimensionBase` also gains an explicit `value`. The metaclass reads `value` in `__repr__`, `__eq__` and `__hash__`, but `__init_subclass__` only sets it on *subclasses*, so hashing or printing the root itself raised `AttributeError`. Nothing reached it before; a `type[DimensionBase]` annotation does, because `eve` memoizes its validator factory on the annotation object. A `TypeAlias` and not a PEP 695 `type` statement: `src/` deliberately has no PEP 695 aliases, and #2841 documented eight latent `eve` dispatch gaps that need one to trigger.
`DimensionClass: TypeAlias = type[DimensionBase]` names "a concrete dimension, i.e. the class itself", and replaces the 208 `type[Dimension]` annotations in `src/gt4py/next`. It is spelled against `DimensionBase` rather than `Dimension` so that parameterized dimension types, which are not user-declarable, are admitted once they exist. Exported as `gtx.DimensionClass`. Named `DimensionClass`, not `DimensionType`, because `*Type` already means something else here: `ts.DimensionType` and its 14 siblings are the *DSL* types of a value. Nine modules refer to both concepts, so reusing the suffix would have put `dims: list[common.DimensionType]` and `ts.DimensionType(dim=value)` in the same file meaning different things. `*T` (TypeVar, as in `DimT`) and `*Like` (coercible-to, as in `RangeLike`) are likewise taken. Two exceptions keep the narrower `type[Dimension]`, both noted in place: the `Dimension(...)` factory overload, which always returns a user-declarable dimension and whose exact type call sites assert, and `TYPE_BUILTINS`, which derives DSL names from `__name__`. `DimensionBase` also gains an explicit `value`. The metaclass reads `value` in `__repr__`, `__eq__` and `__hash__`, but `__init_subclass__` only sets it on *subclasses*, so hashing or printing the root itself raised `AttributeError`. Nothing reached it before; a `type[DimensionBase]` annotation does, because `eve` memoizes its validator factory on the annotation object. A `TypeAlias` and not a PEP 695 `type` statement: `src/` deliberately has no PEP 695 aliases, and #2841 documented eight latent `eve` dispatch gaps that need one to trigger.
…ndices A concrete dimension is now declared as a class, and an index along it is an instance of that class -- the shape `enum.Enum` uses, where the class is the collection and the instances are its members: ```python class IDim(gtx.DimensionIndex): ... class KDim(gtx.DimensionIndex, kind=gtx.DimensionKind.VERTICAL): ... IDim # the dimension -- annotated `gtx.Dimension` IDim(0) # an index into it -- annotated `IDim` ``` so `gtx.Field[gtx.Dims[IDim], gtx.float64]` is a valid annotation for any PEP 484 type checker, with no gt4py mypy plugin. `DimensionMeta` carries the API that belongs to the dimension itself (`I + 1`, `I > 5`, `I == 5`, `repr`, equality, hashing); binary operators on a class object dispatch through the metaclass, so that is the only place they can live. This drops the dimension half of `mypy_plugin.py`, which substituted at most four distinct placeholders per run (`_DimA`..`_DimD`, then `_AnyDim` for everything after), made `TypeVar`s over dimensions impossible, and served only mypy. Because `IDim(0)` is now ordinary instantiation, the whole apparatus that a foreign return type required is gone: no `__new__` returning a non-instance, no duplicated overloads on both `DimensionMeta.__call__` and `__new__`, and no `# type: ignore[misc]` anywhere on the instantiation path. mypy and pyright agree natively -- pinned by the typing tests in #2845. Indices also carry their dimension in the type, so mixing them is a static error; `common.NamedIndex` is deleted. Naming: the dimension's name is `.tag` (typed `common.Tag`, which already existed), and `.value` keeps its meaning as the index position. The reverse split does not type-check at all -- an instance attribute cannot shadow a `ClassVar` -- and this direction leaves every index expression, downstream included, untouched. `.dim` survives as a property returning `type(self)`. `common.Dimension` is a plain `TypeAlias` for `type[DimensionIndex]`, re-exported as `gtx.Dimension`, and `common.dimension(tag, kind)` is the programmatic constructor for the IR boundaries that rebuild a dimension from its tag. A `TYPE_CHECKING`-split callable shim was tried first and rejected: `eve.datamodels` resolves annotations at run time, so a `Dimension` field would see the shim rather than a type. A PEP 695 alias was also rejected: `get_origin()` of one is `None` rather than `type`, which silently misroutes the `get_origin(t) is type` dispatch in `ffront.fbuiltins` (see #2841). Reading `.value` on a dimension *class* would otherwise return the `__slots__` member descriptor rather than raising, and the nonsense value only surfaces much later as a missing offset-provider key or an `AxisLiteral` validation failure. A metaclass property makes it a loud `AttributeError` pointing at `.tag`; instance access is unaffected, since a metaclass attribute is not on an instance's lookup path. Also: pickling is registered through `copyreg` because `pickle.Pickler.save` routes anything whose type subclasses `type` to `save_global` before consulting `__reduce_ex__`, and `fingerprinting.py` gets a `DimensionMeta` deconstructor keyed on `(tag, kind)` so a dimension is not fingerprinted by qualified name. Behaviour change: `repr()` of a dimension is now `I[horizontal]`; `str()` is unchanged, so error messages are byte-identical. Design record: ADR 0028, added here. Implements the `shared/dimensions-as-types` proposal (gt4py_knowledge#27, @havogt) and closes the static-typing gap reported in #2503. Deliberately not here: a `DimensionBase` root above the user-declarable class, deferred until the requirements of non-user-declarable dimensions such as `Staggered[D]` are known; ICON4Py migration, which needs a note for `.tag` and for the removal of `NamedIndex`.
…ndices A concrete dimension is now declared as a class, and an index along it is an instance of that class -- the shape `enum.Enum` uses, where the class is the collection and the instances are its members: ```python class IDim(gtx.DimensionIndex): ... class KDim(gtx.DimensionIndex, kind=gtx.DimensionKind.VERTICAL): ... IDim # the dimension -- annotated `gtx.Dimension` IDim(0) # an index into it -- annotated `IDim` ``` so `gtx.Field[gtx.Dims[IDim], gtx.float64]` is a valid annotation for any PEP 484 type checker, with no gt4py mypy plugin. `DimensionMeta` carries the API that belongs to the dimension itself (`I + 1`, `I > 5`, `I == 5`, `repr`, equality, hashing); binary operators on a class object dispatch through the metaclass, so that is the only place they can live. This drops the dimension half of `mypy_plugin.py`, which substituted at most four distinct placeholders per run (`_DimA`..`_DimD`, then `_AnyDim` for everything after), made `TypeVar`s over dimensions impossible, and served only mypy. Because `IDim(0)` is now ordinary instantiation, the whole apparatus that a foreign return type required is gone: no `__new__` returning a non-instance, no duplicated overloads on both `DimensionMeta.__call__` and `__new__`, and no `# type: ignore[misc]` anywhere on the instantiation path. mypy and pyright agree natively -- pinned by the typing tests in #2845. Indices also carry their dimension in the type, so mixing them is a static error; `common.NamedIndex` is deleted. Naming: the dimension's name is `.tag` (typed `common.Tag`, which already existed), and `.value` keeps its meaning as the index position. The reverse split does not type-check at all -- an instance attribute cannot shadow a `ClassVar` -- and this direction leaves every index expression, downstream included, untouched. `.dim` survives as a property returning `type(self)`. `common.Dimension` is a PEP 695 alias for `type[DimensionIndex]`, re-exported as `gtx.Dimension`, and `common.dimension(tag, kind)` is the programmatic constructor for the IR boundaries that rebuild a dimension from its tag. The PEP 695 spelling is what makes the deprecated `gtx.Dimension("I")` raise rather than silently misbehave. A plain `TypeAlias` for `type[X]` is a `types.GenericAlias`, and calling one forwards to its `__origin__` while discarding the arguments -- so `Dimension("I")` would evaluate to `type("I")`, i.e. `str`, with no error. Its cost is that `get_origin()` of such an alias is `None`, so a site dispatching on an annotation's shape must resolve it first; exactly one in-tree site needed that, `ffront.fbuiltins._type_conversion_helper`, via the `xtyping.resolve_annotation` helper added in #2841. A `TYPE_CHECKING`-split callable shim was also tried and rejected: `eve.datamodels` resolves annotations at run time, so a `Dimension` field would see the shim rather than a type. Reading `.value` on a dimension *class* would otherwise return the `__slots__` member descriptor rather than raising, and the nonsense value only surfaces much later as a missing offset-provider key or an `AxisLiteral` validation failure. A metaclass property makes it a loud `AttributeError` pointing at `.tag`; instance access is unaffected, since a metaclass attribute is not on an instance's lookup path. Also: pickling is registered through `copyreg` because `pickle.Pickler.save` routes anything whose type subclasses `type` to `save_global` before consulting `__reduce_ex__`, and `fingerprinting.py` gets a `DimensionMeta` deconstructor keyed on `(tag, kind)` so a dimension is not fingerprinted by qualified name. Behaviour change: `repr()` of a dimension is now `I[horizontal]`; `str()` is unchanged, so error messages are byte-identical. Design record: ADR 0028, added here. Implements the `shared/dimensions-as-types` proposal (gt4py_knowledge#27, @havogt) and closes the static-typing gap reported in #2503. Deliberately not here: a `DimensionBase` root above the user-declarable class, deferred until the requirements of non-user-declarable dimensions such as `Staggered[D]` are known; ICON4Py migration, which needs a note for `.tag` and for the removal of `NamedIndex`.
evebuilds datamodel validators and converters at run time from live annotationobjects, dispatching on annotation identity. Python spells the same type more than one
way, and a dispatch site that has not been taught a spelling does not raise — it falls
through to a no-match branch and silently builds the wrong validator or converter.
Fixed across
_make_type_converter,type_validation,_is_strictly_immutable_type,get_represented_typesandinfer_type:origin_type is Unionmissedtypes.UnionType, soint | Nonereached
is_actual_type()— which accepts it, being a real class — and built aconverter calling
types.UnionType(value): the class built, every instantiationraised. On 3.14
typing.Unionistypes.UnionType, so non-optional unions reachthat same fallback; they are now rejected explicitly, at class creation.
Optionalargument order.typingpreserves the written order, soUnion[None, int]recursed intoNoneTypeand raised.Annotated. No funnel saw through it. On 3.12typing.Annotatedis a class, sotwo of them took it for an ordinary type — building a
typing.Annotated(value)call,and an
isinstancecheck nothing can satisfy; the immutability check tested themetadata and called immutable fields mutable;
get_represented_typesreported thespecial form itself. Inside
type[...]neither the metadata nor a nested PEP 695alias was resolved, so
type[Annotated[int, "meta"]]acceptedstr.Literal.get_represented_typesreturned(), and callers pass the result toisinstance(), making every such checkFalse.infer_typerecognized only PEP 585 generics, sotyping.List[int],Optional[int],int | strand PEP 695 aliases came back as the CPython-internalclass implementing them.
Annotated.type R = Annotated[R, ...], or a mutuallywrapping pair, resolves only to itself. In
type_validationthe placeholder becameits own validator, so the annotation built cleanly and exhausted the stack on the
first value checked; the other funnels walked the two wrappers until the interpreter
stack ran out. Both are now reported as unresolvable while building.
normalize_union,strip_annotatedandresolve_annotationcarry the reasoning thefunnels would otherwise each repeat. Each returns the identical object when it has
nothing to do, so a caller can detect the no-op with
is-- whichnormalize_unionhas to check for explicitly, since from 3.14
typing.Unionistypes.UnionTypeandan
isinstancetest alone also matches annotations that are already normalized. The last resolves aliases andAnnotatedtogetheras a bounded fixpoint with cycle detection, which is what keeps the cycles above from
recursing: applied in separate branches the two wrappers walk each other forever. Also removed: a discarded duplicate
make_recursive()call that builtevery
Sequence/Setmember validator twice.Behaviour change:
x: int | str = field(converter="coerce")now raisesEveTypeErrorat class creation instead of building a converter that fails on every value. No in-tree
Coerced[...]site is affected.tests/eve_tests/unit_tests/test_annotation_spelling.pyis a conformance matrixcrossing each spelling with each dispatch site, asserting that two spellings of one
type agree rather than checking fixed expectations, so it keeps working as sites are
added. Every fix above was verified by reverting it alone and confirming the matrix
fails. Three ways the matrix could quietly stop asserting anything are themselves
tested: PEP 563 string annotations,
typing's parametrization cache turning a stringround-trip into an identity, and
get_partial_type_hintsstrippingAnnotatedbeforedatamodel-mediated funnels ever see it.
Not fixed here: 13 further gaps found auditing the dispatch sites. Eight need a PEP 695
alias to trigger and
src/has none — the worst iseve/traits.py:118, where aSymbolRefbehind an alias is never collected and dangling-symbol validation becomes ano-op; fixing them means normalizing annotations where they are stored, which is an ADR
decision. The other four activate with the upcoming annotation sweep and must land in
the same commit as the rewrite that triggers them, notably
next/otf/workflow.py:159,where
issubclass(get_origin(X | None), ...)isFalseand an optional pipeline stepis silently dropped from
step_order.Verified on 3.12, 3.13 and 3.14: 858 passed + 57 doctests each.
next/storage/cartesianunit tests: 3057 passed, 108 skipped, 13 xfailed.pre-commit run --all-filesclean, including mypy and tach. Not run locally: the backend matrix.