bpo-44975: [typing] Support issubclass for ClassVar data members #27883
Conversation
|
CC @uriyyo please take a look. Thank you. |
| @@ -1396,8 +1396,15 @@ def _get_protocol_attrs(cls): | |||
|
|
|||
|
|
|||
| def _is_callable_members_only(cls): | |||
| attr_names = _get_protocol_attrs(cls) | |||
| annotations = getattr(cls, '__annotations__', {}) | |||
There was a problem hiding this comment.
What about using typing.get_type_hints to resolve annotations for a class?
It will allow having ClassVar annotated as str:
@runtime_checkable
class P(Protocol):
x: 'ClassVar[int]' = 1| annotations = getattr(cls, '__annotations__', {}) | |
| annotations = get_type_hints(cls) |
There was a problem hiding this comment.
Good question! I'd considered that too but decided against it for two main reasons:
get_type_hintsis slowget_type_hintswon't work with forward references/ PEP 563 ifClassVaris not defined in the caller's namespace
Consider the following:
# foo.py
from __future__ import annotations
from typing import *
@runtime_checkable
class X(Protocol):
x: ClassVar[int] = 1
y: SomeUndeclaredType = None# bar.py
from .foo import X
class Y: ...
# Error! get_type_hints cannot resolve 'ClassVar' and 'SomeUndeclaredType'
issubclass(X, Y)Nonetheless, your suggestion has reminded me of a basic workaround for string annotations. Thanks.
Co-authored-by: Łukasz Langa <[email protected]>
|
|
||
| class C: pass | ||
|
|
||
| class D: |
There was a problem hiding this comment.
Let's also add a case with the same class prop, but different value. Example:
class E:
x = 2Because right now all names / values always match. It is not clear whether value is a part of the protocol or not.
There was a problem hiding this comment.
Agreed.
It is not clear whether value is a part of the protocol or not.
It shouldn't be. IMO, we should only care about the "shape". Trying to runtime-check the value is difficult and should be left to third-party libs to do.
Nevermind, I changed my mind. Let's do it!
| x = 1 | ||
| self.assertNotIsSubclass(C, P) | ||
| self.assertIsSubclass(D, P) | ||
|
|
There was a problem hiding this comment.
Let's also test these classes:
class G:
x: ClassVar[int] = 1class H:
x: 'ClassVar[int]' = 1There was a problem hiding this comment.
These aren't testing more code paths, the code doesn't look at the annotations of the first class argument in issubclass.
|
TODO: Docs need updating. |
| instance_attr = getattr(instance, attr_name, None) | ||
| if (instance_attr is not None | ||
| and attr is not None | ||
| and type(instance_attr) != type(attr)): |
There was a problem hiding this comment.
This is incorrect. Consider this example:
class P(Protocol):
x: ClassVar[object] = object()
class Y:
x: ClassVar[object] = 1Y implements protocol P, but your code rejects it. There are also problems with None here that could produce an incorrect result.
I'd recommend removing this whole check, and just checking that the attribute exists. Full runtime type checking is hard and the standard library shouldn't try to do it.
|
@Fidget-Spinner are you still interested in this PR? |
|
@JelleZijlstra hmm not really. I'm not too inclined on runtime checking in the standard library. IMO it's something we should leave to expert 3rd party libs. |
|
See #89138 -- maybe we should just abandon this? |
https://bugs.python.org/issue44975