Skip to content
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

generic mutable class attributes introduce a vent in the type system #11538

Open
KotlinIsland opened this issue Nov 13, 2021 · 10 comments · Fixed by #11585
Open

generic mutable class attributes introduce a vent in the type system #11538

KotlinIsland opened this issue Nov 13, 2021 · 10 comments · Fixed by #11585
Labels
bug mypy got something wrong

Comments

@KotlinIsland
Copy link
Contributor

KotlinIsland commented Nov 13, 2021

from typing import Generic, TypeVar

T = TypeVar("T")


class A(Generic[T]):
    shared_attr: list[T] = []

a1 = A[str]()
a1.shared_attr.append("AMONGUS")

a2 = A[int]()
a2.shared_attr.append(1)

value = a2.class_attr[0]
reveal_type(value)  # revealed type: int, actual value: AMONGUS

In this example the shared attribute shared_attr is imposted upon by different instantiations of that class, causing a sus amongus to be appeared.

@KotlinIsland KotlinIsland added the bug mypy got something wrong label Nov 13, 2021
@KotlinIsland KotlinIsland changed the title generic mutable class attributes introduce a hole in the type system generic mutable class attributes introduce a vent in the type system Nov 13, 2021
@hauntsaninja
Copy link
Collaborator

Thanks.

I'm guessing part of this is the legacy of pre-PEP 526 days, but it looks like we also don't reject the following:

class A(Generic[T]):
    shared_attr: ClassVar[list[T]] = []

@hauntsaninja
Copy link
Collaborator

PEP 526 is actually quite clear about this:

Note that a ClassVar parameter cannot include any type variables, regardless of the level of nesting: ClassVar[T] and ClassVar[List[Set[T]]] are both invalid if T is a type variable.

Also cc @erictraut since it looks to me like Pyright doesn't handle this either?

@sobolevn
Copy link
Member

I can work on this if no one else has started yet 🙂

@hauntsaninja
Copy link
Collaborator

Keeping this open because the original issue still stands

@sobolevn
Copy link
Member

sobolevn commented Nov 24, 2021

@hauntsaninja thanks for reopening! I've closed it way too soon.

Maybe we can mark attributes with values as implicit ClassVars?

So, shared_attr: list[T] = [] turns into shared_attr: ClassVar[list[T]] = [].

It might break a lot of existing "patterns" where people override class-level values with instance-level ones, but I still think that this is a bad pattern.

Another scenario, do not use implicit ClassVar, but use the same rule we have for ClassVar and TypeVar, but for class-level attributes with values.

Which one do you like best?

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Nov 24, 2021

I'm not sure of the right thing. I tend to agree that class-level values that get overridden are a bad pattern, but that's likely too aggressive of a change to make by default.

We also need to be careful not to break pre-PEP 526 code, e.g. the following are probably pretty widespread:

class A:
    # definitely shouldn't break this
    shared_attr_1 = None  # type: List[T]

    # i'm okay breaking this, but might depend on how bad it is in practice
    shared_attr_2 = []  # type: List[T]

That is, "disallow type variables in class level attributes with values" seems good to me. We might need to special case the None value here to accommodate pre-PEP 526 code, but that should still be mostly enough to fix this "type system vent". I'd be happy to do some special-casing of type comments as an alternative, but in general mypy doesn't do a good job of keeping track of whether a type comes from a comment.

@sobolevn
Copy link
Member

I don't see any ways to solve this problem:

from dataclasses import dataclass, field
from typing import Generic, TypeVar

T = TypeVar('T')

@dataclass
class Some(Generic[T]):
    x: T = field()  # E: ClassVar cannot contain type variables

Technically it should be reported:

  1. It is a class-level variable
  2. It has a generic variable in its type
  3. It has an assigned value

But, in real life - we should not report it. It is a valid case.
The same goes for other class-level DSL, like pydantic.

@KotlinIsland
Copy link
Contributor Author

what about:

from dataclasses import dataclass, field
from typing import Generic, TypeVar

T = TypeVar('T')

@dataclass
class Some(Generic[T]):
    x: T = field()  # that's ok
    y: ClassVar[T] = field()  # that's NOT ok

Some[int].x = 1  # ERROR, SUSSS!!!!!

@hauntsaninja
Copy link
Collaborator

mypy already complains about that ("Access to generic instance variables via class is ambiguous")

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Nov 30, 2021

It looks like for the issue sobolevn fixed in #11585, mypy already complained about on access ("Access to generic class variables is ambiguous"). In fact it looks like we used to complain on definition, but that was removed in #7906 (I'm happy to have both checks, seems fine to me).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
3 participants