classmethod typing is weird

[davidhalter]

It feels like the classmethod typing is slightly wrong.

Typeshed defines it as:

class classmethod(Generic[_T, _P, _R_co]):                                         
    @property                                                                   
    def __isabstractmethod__(self) -> bool: ...                                 
    def __init__(self, __f: Callable[Concatenate[_T, _P], _R_co]) -> None: ...                    
    def __get__(self, __instance: _T, __owner: type[_T] | None = None) -> Callable[_P, _R_co]: ...
    ...

With mypy the following program fails:

from typing import *                                                               
                                                                                               
                                                                                   
def foo(cls: Type['G'], x: int) -> str: return ""                                  
def bar(cls: 'G' | None, x: int) -> str: return ""                                 
                                                                                   
class G:                                                                           
    x = classmethod(foo)                                                           
    y = classmethod(bar)                                                           
                                                                                   
G.x(1)  # Fails                                                                           
G.y(1)  # Works 

new.py:11: error: Argument 1 to "__get__" of "classmethod" has incompatible type "None"; expected "type[G]"  [arg-type]
new.py:11: error: Argument 2 to "__get__" of "classmethod" has incompatible type "type[G]"; expected "type[type[G]] | None"  [arg-type]

The proper solution would probably be to type __init__ as:

def __init__(self, __f: Callable[Concatenate[Type[_T], _P], _R_co]) -> None: ...   

Would such a pull request be accepted? If I change this in Mypy it fails with error: Need type annotation for "x", which I think is much better (and Mypy could probably still infer the type variable _T properly in the future). What do you think?

[AlexWaygood]

The proper solution woud probably be to type __init__ as:

def __init__(self, __f: Callable[Concatenate[_T, _P], _R_co]) -> None: ...   

I think you may have a typo in your message here — this is exactly the signature we currently have for classmethod.__init__

[davidhalter]

Thanks Alex. I changed it to Type[T] above. Sorry, this was was obviously the most important part 😕

[AlexWaygood]

Yes, I think you're right about the __init__ method. Good catch!

The __get__ method looks off as well. Here's the behaviour you get on CPython:

>>> @classmethod
... def bar(cls): return cls.X
...
>>> class Foo:
...     X = 42
...
>>> bar.__get__(None, Foo)()
42
>>> bar.__get__(Foo(), None)()
42
>>> bar.__get__(Foo(), Foo)()
42
>>> bar.__get__('idk_whatever', Foo)()
42
>>> bar.__get__(None, None)()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __get__(None, None) is invalid
>>> bar.__get__(Foo(), 'baz')()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 2, in bar
AttributeError: 'str' object has no attribute 'X'

That implies the __init__ and __get__ methods should together be:

class classmethod(Generic[_T, _P, _R_co]):
    def __init__(self, __f: Callable[Concatenate[type[_T], _P], _R_co]) -> None: ...
    @overload
    def __get__(self, __instance: object, __owner: type[T]) -> Callable[_P, _R_co]: ...
    @overload
    def __get__(self, __instance: _T, __owner: None = None) -> Callable[_P, _R_co]: ...

Feel free to make a PR :)

[davidhalter]

Makes sense. But shouldn't it be:

    @overload
    def __get__(self, __instance: None, __owner: type[_T]) -> Callable[_P, _R_co]: ...
    @overload
    def __get__(self, __instance: _T, __owner: type[_T] = ...) -> Callable[_P, _R_co]: ...

I feel like that's closer to the descriptor protocol. I know that it's probably possible to pass anything for __instance but it feels weird to allow that, when descriptors would never do that normally (and we should probably reject weird inputs).

[AlexWaygood]

Yes, you're right, the object annotation I suggested in the first overload is probably taking it a bit too far. Maybe something like this:

    @overload
    def __get__(self, __instance: _T | None, __owner: type[T]) -> Callable[_P, _R_co]: ...
    @overload
    def __get__(self, __instance: _T, __owner: None = None) -> Callable[_P, _R_co]: ...

[davidhalter]

Thanks. Sorry I was a bit slower, but it was definitely on my todo list!