Skip to content

Cleanup generic class variable access #19292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions mypy/checkmember.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
freeze_all_type_vars,
function_type,
get_all_type_vars,
get_type_vars,
make_simplified_union,
supported_self_type,
tuple_fallback,
Expand Down Expand Up @@ -1198,31 +1197,36 @@ def analyze_class_attribute_access(

if isinstance(node.node, Var):
assert isuper is not None
object_type = get_proper_type(mx.self_type)
# Check if original variable type has type variables. For example:
# class C(Generic[T]):
# x: T
# C.x # Error, ambiguous access
# C[int].x # Also an error, since C[int] is same as C at runtime
# Exception is Self type wrapped in ClassVar, that is safe.
prohibit_self = not node.node.is_classvar
def_vars = set(node.node.info.defn.type_vars)
if not node.node.is_classvar and node.node.info.self_type:
if prohibit_self and node.node.info.self_type:
def_vars.add(node.node.info.self_type)
# TODO: should we include ParamSpec etc. here (i.e. use get_all_type_vars)?
typ_vars = set(get_type_vars(t))
if def_vars & typ_vars:
# Exception: access on Type[...], including first argument of class methods is OK.
if not isinstance(get_proper_type(mx.original_type), TypeType) or node.implicit:
if node.node.is_classvar:
message = message_registry.GENERIC_CLASS_VAR_ACCESS
else:
message = message_registry.GENERIC_INSTANCE_VAR_CLASS_ACCESS
mx.fail(message)
# Exception: access on Type[...], including first argument of class methods is OK.
prohibit_generic = not isinstance(object_type, TypeType) or node.implicit
if prohibit_generic and def_vars & set(get_all_type_vars(t)):
if node.node.is_classvar:
message = message_registry.GENERIC_CLASS_VAR_ACCESS
else:
message = message_registry.GENERIC_INSTANCE_VAR_CLASS_ACCESS
mx.fail(message)
t = expand_self_type_if_needed(t, mx, node.node, itype, is_class=True)
t = expand_type_by_instance(t, isuper)
# Erase non-mapped variables, but keep mapped ones, even if there is an error.
# In the above example this means that we infer following types:
# C.x -> Any
# C[int].x -> int
t = erase_typevars(expand_type_by_instance(t, isuper), {tv.id for tv in def_vars})
if prohibit_generic:
erase_vars = set(itype.type.defn.type_vars)
if prohibit_self and itype.type.self_type:
erase_vars.add(itype.type.self_type)
t = erase_typevars(t, {tv.id for tv in erase_vars})

is_classmethod = (is_decorated and cast(Decorator, node.node).func.is_class) or (
isinstance(node.node, SYMBOL_FUNCBASE_TYPES) and node.node.is_class
Expand Down
1 change: 0 additions & 1 deletion mypy/message_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,6 @@ def with_additional_msg(self, info: str) -> ErrorMessage:
'Cannot override class variable (previously declared on base class "{}") with instance '
"variable"
)
CLASS_VAR_WITH_TYPEVARS: Final = "ClassVar cannot contain type variables"
CLASS_VAR_WITH_GENERIC_SELF: Final = "ClassVar cannot contain Self type in generic classes"
CLASS_VAR_OUTSIDE_OF_CLASS: Final = "ClassVar can only be used for assignments in class body"

Expand Down
8 changes: 0 additions & 8 deletions mypy/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -5079,14 +5079,6 @@ def check_classvar(self, s: AssignmentStmt) -> None:
node.is_classvar = True
analyzed = self.anal_type(s.type)
assert self.type is not None
if analyzed is not None and set(get_type_vars(analyzed)) & set(
self.type.defn.type_vars
):
# This means that we have a type var defined inside of a ClassVar.
# This is not allowed by PEP526.
# See https://github.com/python/mypy/issues/11538

self.fail(message_registry.CLASS_VAR_WITH_TYPEVARS, s)
if (
analyzed is not None
and self.type.self_type in get_type_vars(analyzed)
Expand Down
23 changes: 20 additions & 3 deletions test-data/unit/check-classvar.test
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ main:3: error: Cannot assign to class variable "x" via instance
from typing import ClassVar, Generic, TypeVar
T = TypeVar('T')
class A(Generic[T]):
x: ClassVar[T] # E: ClassVar cannot contain type variables
x: ClassVar[T] # Error reported at access site
@classmethod
def foo(cls) -> T:
return cls.x # OK
Expand All @@ -308,7 +308,7 @@ from typing import ClassVar, Generic, Tuple, TypeVar, Union, Type
T = TypeVar('T')
U = TypeVar('U')
class A(Generic[T, U]):
x: ClassVar[Union[T, Tuple[U, Type[U]]]] # E: ClassVar cannot contain type variables
x: ClassVar[Union[T, Tuple[U, Type[U]]]] # Error reported at access site
@classmethod
def foo(cls) -> Union[T, Tuple[U, Type[U]]]:
return cls.x # OK
Expand All @@ -319,7 +319,9 @@ A[int, str].x # E: Access to generic class variables is ambiguous

class Bad(A[int, str]):
pass
Bad.x # E: Access to generic class variables is ambiguous
reveal_type(Bad.x) # E: Access to generic class variables is ambiguous \
# N: Revealed type is "Union[builtins.int, tuple[builtins.str, type[builtins.str]]]"
reveal_type(Bad().x) # N: Revealed type is "Union[builtins.int, tuple[builtins.str, type[builtins.str]]]"

class Good(A[int, str]):
x = 42
Expand All @@ -343,3 +345,18 @@ class C:
g: ClassVar[Union[Callable[[C], int], int]] = f

reveal_type(C().g) # N: Revealed type is "Union[def () -> builtins.int, builtins.int]"

[case testGenericSubclassAccessNoLeak]
from typing import ClassVar, Generic, TypeVar

T = TypeVar("T")
class B(Generic[T]):
x: T
y: ClassVar[T]

class C(B[T]): ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to also test attribute access in cases like class D(B[int]): .... Should D.y have type int then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some tests (here and above). As I mentioned in the PR description the way we consider some use cases safe, and some unsafe is a bit arbitrary (but maybe there are reasons for this and I just don't remember). Here I am not changing any of that, just making sure that we don't leak type variables, don't silently erase them, etc.


reveal_type(C.x) # E: Access to generic instance variables via class is ambiguous \
# N: Revealed type is "Any"
reveal_type(C.y) # E: Access to generic class variables is ambiguous \
# N: Revealed type is "Any"
22 changes: 22 additions & 0 deletions test-data/unit/check-selftype.test
Original file line number Diff line number Diff line change
Expand Up @@ -2220,6 +2220,28 @@ class C(A, B): # OK: both methods take Self
pass
[builtins fixtures/tuple.pyi]

[case testSelfTypeClassMethodNotSilentlyErased]
from typing import Self, Optional

class X:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test subclassing and accessing _inst outside the class body?

_inst: Optional[Self] = None
@classmethod
def default(cls) -> Self:
reveal_type(cls._inst) # N: Revealed type is "Union[Self`0, None]"
if cls._inst is None:
cls._inst = cls()
return cls._inst

reveal_type(X._inst) # E: Access to generic instance variables via class is ambiguous \
# N: Revealed type is "Union[__main__.X, None]"
reveal_type(X()._inst) # N: Revealed type is "Union[__main__.X, None]"

class Y(X): ...
reveal_type(Y._inst) # E: Access to generic instance variables via class is ambiguous \
# N: Revealed type is "Union[__main__.Y, None]"
reveal_type(Y()._inst) # N: Revealed type is "Union[__main__.Y, None]"
[builtins fixtures/tuple.pyi]

[case testSelfInFuncDecoratedClassmethod]
from collections.abc import Callable
from typing import Self, TypeVar
Expand Down
22 changes: 22 additions & 0 deletions test-data/unit/check-typevar-tuple.test
Original file line number Diff line number Diff line change
Expand Up @@ -2629,6 +2629,28 @@ def test(*args: Unpack[tuple[T]]) -> int: ...
reveal_type(fn(test)) # N: Revealed type is "def [T] (T`1) -> builtins.int"
[builtins fixtures/tuple.pyi]

[case testNoGenericTypeVarTupleClassVarAccess]
from typing import Generic, Tuple, TypeVarTuple, Unpack

Ts = TypeVarTuple("Ts")
class C(Generic[Unpack[Ts]]):
x: Tuple[Unpack[Ts]]

reveal_type(C.x) # E: Access to generic instance variables via class is ambiguous \
# N: Revealed type is "builtins.tuple[Any, ...]"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test also defining a subclass and then accessing the attribute via the subclass, and an instance of the subclass?


class Bad(C[int, int]):
pass
reveal_type(Bad.x) # E: Access to generic instance variables via class is ambiguous \
# N: Revealed type is "tuple[builtins.int, builtins.int]"
reveal_type(Bad().x) # N: Revealed type is "tuple[builtins.int, builtins.int]"

class Good(C[int, int]):
x = (1, 1)
reveal_type(Good.x) # N: Revealed type is "tuple[builtins.int, builtins.int]"
reveal_type(Good().x) # N: Revealed type is "tuple[builtins.int, builtins.int]"
[builtins fixtures/tuple.pyi]

[case testConstraintsIncludeTupleFallback]
from typing import Generic, TypeVar
from typing_extensions import TypeVarTuple, Unpack
Expand Down
11 changes: 0 additions & 11 deletions test-data/unit/semanal-classvar.test
Original file line number Diff line number Diff line change
Expand Up @@ -207,14 +207,3 @@ class B:
pass
[out]
main:4: error: ClassVar can only be used for assignments in class body

[case testClassVarWithTypeVariable]
from typing import ClassVar, TypeVar, Generic, List

T = TypeVar('T')

class Some(Generic[T]):
error: ClassVar[T] # E: ClassVar cannot contain type variables
nested: ClassVar[List[List[T]]] # E: ClassVar cannot contain type variables
ok: ClassVar[int]
[out]