Prevent crash with Unpack of a fixed tuple in PEP695 type alias

[sterliakov]

Fixes #18309.

Add missing visit_type_alias_stmt() implementation to mixedtraverser.py to visit the alias target directly.

[sterliakov]

Primer results are terrible, I'll look closer tomorrow - all those errors should have moved to another location, not disappeared entirely.

[sterliakov]

This is more complicated than I expected.

The problem reduces to the following (playground):

from collections.abc import Callable, Coroutine
from typing import Any, Concatenate, Generic, TypeVar

IntFun = TypeVar("IntFun", bound=Callable[Concatenate[int, ...], str])
StrFun = TypeVar("StrFun", bound=Callable[Concatenate[str, ...], str])
AnyFun = TypeVar("AnyFun", bound=Callable[..., str])

class IntC(Generic[IntFun]): ...
class StrC(Generic[StrFun]): ...

AnyC = IntC[AnyFun] | StrC[AnyFun]

def run(_: AnyC[IntFun]) -> IntFun: ...  # E: Type argument "IntFun" of "StrC" must be a subtype of "Callable[[str, VarArg(Any), KwArg(Any)], str]"  [type-var]

(the error exists on master and does not exist with this patch applied)

And I have no idea what is more reasonable here. AnyC generic arg allows any callable, so AnyC[IntFun] should be allowed. However, this feels like a type error, because that means allowing StrC[IntFun]? However, I don't see how such type can appear anywhere: if something resolves to StrC[IntFun], that would be an error?

[A5rocks]

Even simpler reproducer (no callables):

import typing

IntT = typing.TypeVar("IntT", bound=int)
StrT = typing.TypeVar("StrT", bound=str)
AnyT = typing.TypeVar("AnyT", bound=typing.Any)

class IntC(typing.Generic[IntT]): ...
class StrC(typing.Generic[StrT]): ...

AnyC = IntC[AnyT] | StrC[AnyT]

def run(_: AnyC[StrT]) -> StrT: ...  # E: Type argument "StrT" of "IntC" must be a subtype of "int"

IMO there should be an error on AnyC, rather than at usage time. Yes, the bound=Any is there, but... like... it's not necessarily Any. If the user wants Any they can say AnyC = IntC[Any] | StrC[Any]. I'll go open an issue about this :^)

[sterliakov]

@A5rocks I'm not sure I want to see any errors on the AnyC definition: it would be extremely weird to read that something bound to Any can't be used to substitute some typevar. Having errors when aliases are defined with incompatible typevars is great (we didn't before, this PR makes such aliases fail early). But rejecting a compatible typevar (and one with Any bound should probably be considered compatible) is questionable.

[A5rocks]

I'm not sure I want to see any errors on the AnyC definition: it would be extremely weird to read that something bound to Any can't be used to substitute some typevar.

I think Any in the bound is actually not useful information. IMO, the type alias is obviously wrong if you cannot use it (other than passing Any), so it should get an error. I tried looking at PEP 484 for bound=... and it just says:

while an upper bound just requires that the actual type is a subtype of the boundary type.

... which isn't very useful. I mean at best we could pretend "subtype" here means the PEP 483 version of it (which doesn't include Any) but the typing spec says:

an upper bound just requires that the actual type is assignable to the bound.

and like, assignability allows Any I'm pretty sure. And every type is "assignable" to Any I think. So it doesn't help.

[ilevkivskyi]

Not giving error on type alias definition is intentional. It is to allow a pattern like this:

# in one file
T = TypeVar("T", bound=int)
class C(Generic[T]): ...

# in other file
T = TypeVar("T")  # same as bound=object
A = list[C[T]]

x: A[int]  # OK
y: A[str]  # Error

People often use "throw away" type variable for type aliases. With the error at alias definition, one would be forced to copy over all type variable bounds for no added safety.

[sterliakov]

I agree that this strictness with definition-time checks somewhat harms usability. Anyway, let's see where #18456 goes. I don't believe that changing behaviour of aliases to fix the tree traversal is reasonable.

Is there something else we can visit instead of get_proper_type or the alias as here? We need to make the visitor work on the original tuple.

In the meantime #18456 takes another approach that doesn't affect checking existing aliases, but suffers from not modifying the tuple where it is expected. Maybe that's good enough?

[ilevkivskyi]

@sterliakov A proper fix would be to simply update mixedtraverser.py to have visit_type_alias_stmt() that would accept the alias target (similar to visit_type_allias_expr() you will need to set/unset self.in_type_lias_expr flag to avoid errors about bounds in type alias definitions). I am kind of surprised that nothing else broke, as it is a relatively major omission in the implementation of new syntax.

Btw another thing (that I noticed while looking at your other attempt) is that you may need to add this line

                # Invalidate recursive status cache in case it was previously set.
                existing.node._is_recursive = None

to the new syntax code path, as it looks like I forgot to copy it in #17961 (if you do, please also add a corresponding test), you can make a separate PR for this.

[sterliakov]

Amazing, thanks! I got lost in visit_type_alias{,_stmt,_expr,_type} when looking through these overrides, probably due to this long common prefix, and failed to notice that this one isn't overridden.

you may need to add this line

It's there in reused def _update_type_alias, yes, and I'll keep that PR to reduce code duplication - let's focus on the traverser here.

[github-actions]

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

[ilevkivskyi]

Please update the PR description so that it reflects the current state of the PR.