Skip to content

Unsafe use of a generic class method of a generic class allowed #7846

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

Closed
JukkaL opened this issue Nov 1, 2019 · 3 comments · Fixed by #7862
Closed

Unsafe use of a generic class method of a generic class allowed #7846

JukkaL opened this issue Nov 1, 2019 · 3 comments · Fixed by #7862
Assignees
Labels

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 1, 2019

Mypy associates class type variables to generic class methods in some contexts, but this is inconsistent, which results in type unsafety. Here is an example that mypy is okay with but that is actually unsafe:

from typing import TypeVar, Generic

T = TypeVar('T')
T2 = TypeVar('T2')

class C(Generic[T]):
    @classmethod
    def f(cls, x: T) -> T:
        return x

    @classmethod
    def g(cls, x: T2) -> T2:
        cls.f(x)  # This is accepted
        return x

class D(C[int]):
    @classmethod
    def f(cls, x: int) -> int:  # This override is accepted
        return x + 1

D.g('')   # Runtime error!

If the methods are instance methods, T2 is not treated as compatible with T, however.

@ilevkivskyi
Copy link
Member

Mypy associates class type variables to generic class methods in some contexts

This is done to support alternative class constructors (a popular feature request in the past). I think this idea is not unsafe by itself but the implementation likely have some bugs.

I actually remember hitting something like this while working on some class method issue. I don't remember what was exactly the problem, but most likely it is related to what type cls is given. I vaguely remember this is because cls looks like C, not like C[T] at the point where the helper adding type variables is called. So that in your example cls.f has type def [T] (T) -> T instead of def (T) -> T.

There is a test case testGenericClassMethodUnboundOnClassNonMatchingIdNonGeneric. From it's name it is clear that the method should be non-generic but it doesn't work (see revealed type for cls.foo). I wanted to add a TODO here but probably forgot.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Nov 1, 2019

Yeah, the underlying feature looks useful. The unsafety is a bit worrying though, since it recently hid quite a few bad annotations in some code is was annotating. Increasing priority to high since the false negatives can be hard to track down.

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Nov 3, 2019

While working on this I discovered there is the same issue for normal constructors:

class C(Generic[T]):
    def __init__(self, item: T) -> None:
        self.item = item
    @classmethod
    def f(cls) -> None:
        cls(1)  # This is in fact unsafe, cls is def (T) -> T, not def [T] (T) -> T

class D(C[str]):
    def __init__(self, item: str) -> None:
        self.item = item + 'no way'

D('hm').f()

I can prohibit this too (for the same reason described above), but I feel like people may be not happy about this. Probably we can test this against internal code bases to estimate fallout.

ilevkivskyi added a commit that referenced this issue Nov 12, 2019
Fixes #7846

The fix is mostly straightforward, instead of ad-hoc guessing of type variables from the return type, we pass the them all the way through intermediate helper functions.

The `ctypes` plugin change is because it used the bare representation of types (the one used by `reveal_type()`) for user errors. So after my change arrangement of stars changed, instead of adjusting it, I switched to the proper type formatting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants