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

Problems using class type variable in class methods #3645

Closed
LiraNuna opened this issue Jul 1, 2017 · 12 comments · Fixed by #6418
Closed

Problems using class type variable in class methods #3645

LiraNuna opened this issue Jul 1, 2017 · 12 comments · Fixed by #6418

Comments

@LiraNuna
Copy link

LiraNuna commented Jul 1, 2017

Follow up from a discussion on gitter:

from typing import *


T = TypeVar('T')
class Factory(Generic[T]):
    @classmethod
    def instance(cls) -> 'Factory[T]':
        return cls()

    def produce(self) -> T:
        raise NotImplementedError()

    @classmethod
    def get(cls) -> T:
        return cls.instance().produce()


class HelloWorldFactory(Factory[str]):
    def produce(self) -> str:
        return 'Hello World'


# factory.py:25: error: Incompatible return value type (got "T", expected "str")
def error_with_mypy() -> str:
    return HelloWorldFactory.get()

@ilevkivskyi mentioned this should not error and that this code is valid. I don't have the necessary context to understand how to classify this issue.

@ilevkivskyi
Copy link
Member

I would simplify the example to show only the essential part:

class Factory(Generic[T]):
    def produce(self) -> T:
        ...
    @classmethod
    def get(cls) -> T:
        return cls().produce()

class HelloWorldFactory(Factory[str]):
    def produce(self) -> str:
        return 'Hello World'

reveal_type(HelloWorldFactory.get())  # mypy should be able to infer 'str' here

On one hand, type variables are bound on instances, not on classes, but in case of a non-generic subclass I think the type variable can be bound on that class. The problem however is that we need to give a clear error message when this is not possible, something like (using above example):

Factory.get()  # E: cannot bind type variable on class (use an instance or a non-generic subclass)
Facrory[int].get()   # E: cannot bind type variable on class (use an instance or a non-generic subclass)

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 3, 2017

Similar issues have come up previously, so this would perhaps be a reasonable feature to have eventually -- at least assuming the implementation isn't very complex.

@JukkaL JukkaL changed the title "Incompatible return value" error for classmethod generic subclasses Problems using class type variable in class methods Jul 3, 2017
@gvanrossum
Copy link
Member

The produce() method is also irrelevant to the example. Shorter yet self-contained:

from typing import Generic, TypeVar
T = TypeVar('T')
class Factory(Generic[T]):
    @classmethod
    def get(cls) -> T: ...
class HelloWorldFactory(Factory[str]): pass
reveal_type(HelloWorldFactory.get())  # mypy should be able to infer 'str' here

@rowillia
Copy link
Contributor

@JukkaL Do you have any thoughts on what it would take to fix this? It's blocking us here at Lyft.

@JukkaL
Copy link
Collaborator

JukkaL commented Feb 26, 2018

@rowillia My recommendation is to use a regular method instead of a class method. Example:

from typing import Generic, TypeVar

T = TypeVar('T')

class Factory(Generic[T]):
    def get(cls) -> T: ...

class _HelloWorldFactory(Factory[str]): pass

HelloWorldFactory = _HelloWorldFactory()  # This singleton is mostly for convenience

reveal_type(HelloWorldFactory.get())  # this is 'str'

You can avoid the singleton by using HelloWorldFactory().get() instead.

@glyph
Copy link

glyph commented Apr 18, 2018

I bumped into this bug today as well, and was surprised to see this comment:

type variables are bound on instances, not on classes

If this is the case, then why is

class G(Generic[T]):
    @classmethod
    def m(cls) -> 'G[T]':
        ...

not itself an error? being able to say G[T] at least implies to me that T is bound.

@JelleZijlstra
Copy link
Member

That method returns an instance of G[T].

@glyph
Copy link

glyph commented Apr 18, 2018

According to reveal_type it returns an instance of G[<nothing>].

@glyph
Copy link

glyph commented Apr 18, 2018

More specifically: G[int].m() returns G[<nothing>].

@JelleZijlstra
Copy link
Member

Oh, I think in this case it's basically equivalent to a top-level function returning G[T]: there is nothing to constrain the value of the typevar, so mypy infers "". I believe there's a separate issue about that.

@ilevkivskyi
Copy link
Member

@glyph

If this is the case, then why is ... not itself an error?

Because it is not, it may be bound in a subclass as said few word later in the comment you quote, there is even a simple example just few comments above #3645 (comment)

ilevkivskyi added a commit that referenced this issue Feb 24, 2019
Fixes #3645
Fixes #1337
Fixes #5664

The fix is straightforward, I just add/propagate the bound type variable values by mapping to supertype.

I didn't find any corner cases with class methods, and essentially follow the same logic as when we generate the callable from `__init__` for generic classes in calls like `C()` or `C[int]()`.

For class attributes there are two things I fixed. First we used to prohibit ambiguous access:
```python
class C(Generic[T]):
    x: T
C.x  # Error!
C[int].x  # Error!
```
but the type variables were leaking after an error, now they are erased to `Any`. Second, I now make an exception and allow accessing attributes on `Type[C]`, this is very similar to how we allow instantiation of `Type[C]` even if it is abstract (because we expect concrete subclasses there), plus this allows accessing variables on `cls` (first argument in class methods), for example:
```python
class C(Generic[T]):
    x: T
    def get(cls) -> T:
        return cls.x  # OK
```

(I also added a bunch of more detailed comments in this part of code.)
@LiraNuna
Copy link
Author

@ilevkivskyi Thank you for fixing this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants