-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
bpo-44975: [typing] Support issubclass for ClassVar data members #27883
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
bpo-44975: [typing] Support issubclass for ClassVar data members #27883
Conversation
CC @uriyyo please take a look. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM💫
I have few minor questions
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! I'd considered that too but decided against it for two main reasons:
get_type_hints
is slowget_type_hints
won't work with forward references/ PEP 563 ifClassVar
is 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM💫
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
class C: pass | ||
|
||
class D: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also add a case with the same class prop, but different value. Example:
class E:
x = 2
Because 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also test these classes:
class G:
x: ClassVar[int] = 1
class H:
x: 'ClassVar[int]' = 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect. Consider this example:
class P(Protocol):
x: ClassVar[object] = object()
class Y:
x: ClassVar[object] = 1
Y 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