-
-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Changes from 1 commit
a613af6
f045d2e
58c69e1
2ed2ee5
c842370
f44d3b6
9278a34
ba57bb1
43ec2ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1610,6 +1610,18 @@ class P(Protocol): | |
with self.assertRaisesRegex(TypeError, "@runtime_checkable"): | ||
isinstance(1, P) | ||
|
||
def test_runtime_issubclass_with_classvar_data_members(self): | ||
@runtime_checkable | ||
class P(Protocol): | ||
x: ClassVar[int] = 1 | ||
|
||
class C: pass | ||
|
||
class D: | ||
x = 1 | ||
self.assertNotIsSubclass(C, P) | ||
self.assertIsSubclass(D, P) | ||
Fidget-Spinner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
|
||
class GenericTests(BaseTestCase): | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. What about using It will allow having @runtime_checkable
class P(Protocol):
x: 'ClassVar[int]' = 1
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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. |
||||||
# PEP 544 prohibits using issubclass() with protocols that have non-method members. | ||||||
return all(callable(getattr(cls, attr, None)) for attr in _get_protocol_attrs(cls)) | ||||||
for attr_name in attr_names: | ||||||
attr = getattr(cls, attr_name, None) | ||||||
if not (callable(attr) | ||||||
or (getattr(annotations.get(attr_name), '__name__', None) == 'ClassVar')): | ||||||
Fidget-Spinner marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
return False | ||||||
return True | ||||||
|
||||||
|
||||||
def _no_init(self, *args, **kwargs): | ||||||
|
@@ -1511,8 +1518,9 @@ def _proto_hook(other): | |||||
if not _is_callable_members_only(cls): | ||||||
if _allow_reckless_class_checks(): | ||||||
return NotImplemented | ||||||
raise TypeError("Protocols with non-method members" | ||||||
" don't support issubclass()") | ||||||
raise TypeError("Protocol members must be methods or data" | ||||||
" attributes annotated with ClassVar to support" | ||||||
" issubclass()") | ||||||
if not isinstance(other, type): | ||||||
# Same error message as for issubclass(1, int). | ||||||
raise TypeError('issubclass() arg 1 must be a class') | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Runtime protocols with data members now support :func:`issubclass` as long | ||
as those members are annotated with :data:`typing.ClassVar`. |
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:
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 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!