fix[eve]: support type aliases recursing through a container - #2814
Conversation
`NestedTuple` and friends were defined as plain assignments with a string
forward reference to themselves. Subscripting such an alias does not
substitute the type parameter inside the forward reference, so
`NestedTuple[Foo]` expands to `tuple[Foo | ForwardRef('NestedTuple[_T_co]'), ...]`,
which raises a `NameError` when `get_type_hints()` tries to resolve it in
the namespace of the annotated object. As a consequence these aliases
could not be used as datamodel/node field annotations at all.
They are now defined with the PEP 695 `type` statement, which supports
recursion and parameter substitution properly. Such an alias is well
founded, unlike the `type A = A` cycle `eval_type_alias()` rejects: a
single resolution step already yields an annotation which is not an alias
itself. What it does not yield is a *finite* one, so
`SimpleTypeValidatorFactory` now hands the alias occurring inside its own
definition a deferred validator, filled in as soon as the definition has
been processed.
egparedes
left a comment
egparedes
left a comment
There was a problem hiding this comment.
I have a couple of comments to make the PR cleaner.
| # Note: these recursive aliases have to be defined with the PEP 695 `type` statement. Written as | ||
| # plain assignments (`NestedTuple = tuple[Union[_T_co, "NestedTuple[_T_co]"], ...]`), subscription | ||
| # does not substitute the type parameter inside the string annotation, so `NestedTuple[int]` keeps | ||
| # an unparametrized `ForwardRef("NestedTuple[_T_co]")` which cannot be resolved in the namespace | ||
| # of the annotated object (see `get_partial_type_hints`). |
There was a problem hiding this comment.
I find this comment verbose and basically unnecessary since the codebase should be upgraded soon to >= 3.12 only, so all aliases will become PEP 695 type statements anyway.
| type NestedSequence[T] = Sequence[T | NestedSequence[T]] | ||
| type NestedList[T] = list[T | NestedList[T]] | ||
| type NestedTuple[T] = tuple[T | NestedTuple[T], ...] | ||
|
|
||
| type MaybeNested[T] = T | NestedSequence[T] | ||
| type MaybeNestedInSequence[T] = T | NestedSequence[T] | ||
| type MaybeNestedInList[T] = T | NestedList[T] | ||
| type MaybeNestedInTuple[T] = T | NestedTuple[T] |
There was a problem hiding this comment.
Why changing from _T_co to T as type variable here? Shouldn't the covariant type var be used here since a tuple is an immutable container?
There was a problem hiding this comment.
@tehrengruber-ai: Keep the previous style as requested.
There was a problem hiding this comment.
No good reason — that was collateral from switching to the PEP 695 syntax, not intentional. Reverted: the aliases take _T_co again.
Note that PEP 695 has no syntax for declaring variance on a type parameter (it is inferred at the use site), so unlike the old TypeVar("_T_co", covariant=True) the name is now the only thing carrying the intent. But you are right that it is the correct one for these containers, and keeping it also keeps the aliases consistent with is_nested_tuple_of / is_maybe_nested_in_tuple_of below, which still use the module-level _T_co.
|
|
||
| # Implementations | ||
| @dataclasses.dataclass | ||
| class _DeferredTypeValidator: |
There was a problem hiding this comment.
I might be wrong but isn't this class a duplication of the datamodels.core.ForwardRefValidator? Would it be worth to refactor the code to define it only once in the proper place (probably defined here and used from datamodels)?
There was a problem hiding this comment.
Investigated. They look alike, but the contract differs in both directions, so I kept them separate:
- Different protocol.
ForwardRefValidatoris anattrsfield validator,(instance, attribute, value)— that signature is whatfield_type_validator_factoryhas to hand back toattrs._DeferredTypeValidatoris a plainFixedTypeValidator,(value, **kwargs), which is what gets composed into the surrounding validators here.type_validationalso has no notion of datamodels/attrs, and the dependency only runs the other way. - Different filling mechanism.
ForwardRefValidatoris lazy and self-resolving: on its first call it goes throughinstanceto the model class, runsupdate_forward_refs(model_cls)and only then can look up the field annotation and build the real validator — it cannot do any of that at construction time._DeferredTypeValidatorresolves nothing: the annotation is already in hand, and the factory assigns.validatoron the very next line, before the composed validator is returned and therefore before any value can reach it. Hence theassertwhere the other one has a resolution step.
A shared class would have to carry both call signatures and both filling modes, which is more machinery than the ~8 lines each costs today. The one unification that would type-check is ForwardRefValidator holding a _DeferredTypeValidator instead of its own validator field, but that is pure indirection — it removes no code and hides the lazy resolution one level deeper.
I did add a sentence to the _DeferredTypeValidator docstring pointing at the distinction, so the next reader does not have to redo this comparison. Happy to go the other way if you still prefer a single place.
- Drop the explanatory comment above the `Nested*` aliases. - Name the type parameter of the `Nested*` / `MaybeNested*` aliases `_T_co` again, as before the switch to the PEP 695 `type` statement. - Note in `_DeferredTypeValidator` how it differs from the similar-looking `datamodels.ForwardRefValidator`.
Now that eve supports type aliases recursing through a container (GridTools#2814), annotate the comprehension target precisely as 'MaybeNestedInTuple[DataSymbol]' — the 'Maybe' variant since a bare-name target yields a lone symbol. Align 'func_to_foast.parse_target' and the 'type_deduction' helper signatures with the same spelling.
3 participants
NestedTupleand its siblings ingt4py.eve.extended_typingwere defined as plain assignments holding a string forward reference to themselves:Subscripting such an alias does not substitute the type parameter inside the forward reference, so
NestedTuple[Foo]expands totuple[Foo | ForwardRef('NestedTuple[_T_co]'), ...]andget_type_hints()fails withNameError: name '_T_co' is not definedwhen resolving it in the namespace of the annotated object. These aliases could therefore not be used as datamodel/node field annotations at all, which is why #2487 had to fall back totarget: Anyinstead ofNestedTuple[DataSymbol]infoast.TupleComprehensionMapper.Changes:
Nested*/MaybeNested*aliases are now defined with the PEP 695typestatement, which substitutes type parameters properly across the recursion.SimpleTypeValidatorFactorybreaks the resulting cycle. Such an alias is well founded, unlike thetype A = Acycleeval_type_alias()rejects: a single resolution step already yields an annotation which is not an alias itself. What it does not yield is a finite one, so the alias occurring inside its own definition is now handed a deferred validator, filled in as soon as the definition has been processed.test_recursive_type_alias_is_not_supportedwas renamed totest_cyclic_type_alias_is_not_supported, since only degenerate cycles remain unsupported.Once this is merged, the
target: Anyworkaround in #2487 can be removed.Disclaimer: This PR and its description were written largely with the help of AI. Code was reviewed briefly by me and in more detail by the reviewer.