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.

Sign up for GitHub

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

Jump to bottom

Fix @classproperty decorator #1287

Merged
merged 2 commits into from
Dec 9, 2022
Merged

Fix @classproperty decorator #1287

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2b426a
Fix classproperty generics
intgr Dec 8, 2022
40d40a6
Self TypeVar -> Any
intgr Dec 9, 2022
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
6 changes: 3 additions & 3 deletions django-stubs/utils/functional.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,9 @@ _Get = TypeVar("_Get", covariant=True)

class classproperty(Generic[_Get]):
fget: Callable[[Self], _Get] | None
def __init__(self: Self, method: Callable[[Self], _Get] | None = ...) -> None: ...
def __get__(self: Self, instance: Self | None, cls: type[Self] = ...) -> _Get: ...
def getter(self: Self, method: Callable[[Self], _Get]) -> classproperty[_Get]: ...
def __init__(self, method: Callable[[Self], _Get] | None = ...) -> None: ...
Copy link
Member

Choose a reason for hiding this comment

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

Now Self seems unbound. It can be Any

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't actually understand how this mechanism works 😆 I just reverted the unwanted changes and verified via the test that it works.

If I add self: Self here, it no longer works.

Copy link
Member

Choose a reason for hiding this comment

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

mypy binds variables in two cases:

  1. If they are in Generic[] definition, like we have here with _Get
  2. If it is used more than once in a function: def some(a: A) -> A: return a

Here Self is unbound and should be changed to Any with a comment, I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it could be done like in python/typeshed stdlib/builtins.pyi classmethod

_R_co = TypeVar("_R_co", covariant=True)
...
class classmethod(Generic[_R_co]):
    @property
    def __func__(self) -> Callable[..., _R_co]: ...
    @property
    def __isabstractmethod__(self) -> bool: ...
    def __init__(self: classmethod[_R_co], __f: Callable[..., _R_co]) -> None: ...
    def __get__(self, __obj: _T, __type: type[_T] | None = ...) -> Callable[..., _R_co]: ...
    if sys.version_info >= (3, 10):
        __name__: str
        __qualname__: str
        @property
        def __wrapped__(self) -> Callable[..., _R_co]: ...

PEP 673 - Self Type describes Self as shorthand for: TClassName = TypeVar("TClassName", bound="ClassName"). So maybe covariant TypeVar would be the fix here?
Also PEP 673 was only just introducted in python 3.11. Maybe that could be a problem too?

Copy link
Contributor

Choose a reason for hiding this comment

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

But then again djangos classproperty is implemented a bit differently than CPythons classmethod

Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw that _Get already is the covariant TypeVar, so that should be fine.
Looking at the typehints for @property I think I now understand what @sobolevn means (simply replace the remaining Self with Any):

    fget: Callable[[Any], _Get] | None
    def __init__(self, method: Callable[[Any], _Get] | None = ...) -> None: ...
    def __get__(self, instance: Any | None, cls: type[Any] = ...) -> _Get: ...
    def getter(self, method: Callable[[Any], _Get]) -> classproperty[_Get]: ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed works just as well with Any. I've pushed this change.

def __get__(self, instance: Self | None, cls: type[Self] = ...) -> _Get: ...
def getter(self, method: Callable[[Self], _Get]) -> classproperty[_Get]: ...

class _Getter(Protocol[_Get]):
"""Type fake to declare some read-only properties (until `property` builtin is generic)
Expand Down
18 changes: 18 additions & 0 deletions tests/typecheck/utils/test_functional.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,3 +49,21 @@

foo(s) # E: Argument 1 to "foo" has incompatible type "_StrPromise"; expected "str"
bar(s)

- case: classproperty
main: |
from typing import Any
from django.utils.functional import classproperty

class Foo:
@classproperty
def attr(cls: Any) -> str: ...

reveal_type(attr) # N: Revealed type is "django.utils.functional.classproperty[builtins.str]"
reveal_type(attr.getter) # N: Revealed type is "def [Self] (method: def (Self`126) -> builtins.str) -> django.utils.functional.classproperty[builtins.str]"

reveal_type(Foo.attr) # N: Revealed type is "builtins.str"

class Bar(Foo):
def method(self) -> None:
reveal_type(self.attr) # N: Revealed type is "builtins.str"