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

gh-74690: typing: Don't unnecessarily call _get_protocol_attrs twice in _ProtocolMeta.__instancecheck__ #103141

Merged
merged 4 commits into from
Mar 31, 2023
Merged

gh-74690: typing: Don't unnecessarily call _get_protocol_attrs twice in _ProtocolMeta.__instancecheck__ #103141

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51ca233
gh-74690: typing: Don't unnecessarily call `_get_protocol_attrs` twic…
AlexWaygood Mar 30, 2023
285a41b
Address review
AlexWaygood Mar 31, 2023
b6d1c95
Delete 2023-03-30-23-51-22.gh-issue-74690.p2c4Co.rst
AlexWaygood Mar 31, 2023
0f498a9
Merge branch 'main' into protocol-attrs-collection
AlexWaygood Mar 31, 2023
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
31 changes: 21 additions & 10 deletions Lib/typing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1931,9 +1931,9 @@ def _get_protocol_attrs(cls):
return attrs


def _is_callable_members_only(cls):
def _is_callable_members_only(cls, protocol_attrs):
# 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))
return all(callable(getattr(cls, attr, None)) for attr in protocol_attrs)


def _no_init_or_replace_init(self, *args, **kwargs):
Expand Down Expand Up @@ -2000,24 +2000,32 @@ class _ProtocolMeta(ABCMeta):
def __instancecheck__(cls, instance):
# We need this method for situations where attributes are
# assigned in __init__.
is_protocol_cls = getattr(cls, "_is_protocol", False)
if (
getattr(cls, '_is_protocol', False) and
is_protocol_cls and
not getattr(cls, '_is_runtime_protocol', False) and
not _allow_reckless_class_checks(depth=2)
):
raise TypeError("Instance and class checks can only be used with"
" @runtime_checkable protocols")

if ((not getattr(cls, '_is_protocol', False) or
_is_callable_members_only(cls)) and
issubclass(instance.__class__, cls)):
if not is_protocol_cls and issubclass(instance.__class__, cls):
return True
if cls._is_protocol:

protocol_attrs = _get_protocol_attrs(cls)
Copy link
Member Author

@AlexWaygood AlexWaygood Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could possibly cache the result of this call on the protocol class in a __protocol_attrs__ class attribute. But I think I'd rather look at that in a separate PR so that we can evaluate the impact of it in isolation. I'd also want to check to see if there were any behaviour changes, and compare different methods of caching the result of _get_protocol_attrs.

Copy link
Contributor

@posita posita Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could possibly cache the result of this call on the protocol class in a __protocol_attrs__ class attribute. ...

If you decide to tackle this, @beartype provides a caching mechanism for __instancecheck__ results that may be worth a look, if only for inspiration. I believe some of the same caveats would apply. (Full disclosure: I am an author. Happy to chat, if helpful.)

Copy link
Member Author

@AlexWaygood AlexWaygood Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I share @ilevkivskyi's reservations in #74690 (comment) about caching the whole call to __instancecheck__ in the way that I believe beartype does it, but I think caching the call to _get_protocol_attrs could get us a large speedup without the same risk of changing the behaviour. (Apologies if I was unclear in my comment above!) Anyway, TBD in a future PR :)

Copy link
Member

Choose a reason for hiding this comment

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

Is monkey-patching of the protocol class itself something we need to care about here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is monkey-patching of the protocol class itself something we need to care about here?

Pretty unlikely imo. But yeah, it's that kind of thing that I'd want to think through, which is why I'd probably like to leave that to another PR :)


if (
_is_callable_members_only(cls, protocol_attrs)
and issubclass(instance.__class__, cls)
):
return True

if is_protocol_cls:
if all(hasattr(instance, attr) and
# All *methods* can be blocked by setting them to None.
(not callable(getattr(cls, attr, None)) or
getattr(instance, attr) is not None)
for attr in _get_protocol_attrs(cls)):
for attr in protocol_attrs):
return True
return super().__instancecheck__(instance)

Expand Down Expand Up @@ -2074,7 +2082,10 @@ def _proto_hook(other):
return NotImplemented
raise TypeError("Instance and class checks can only be used with"
" @runtime_checkable protocols")
if not _is_callable_members_only(cls):

protocol_attrs = _get_protocol_attrs(cls)

if not _is_callable_members_only(cls, protocol_attrs):
if _allow_reckless_class_checks():
return NotImplemented
raise TypeError("Protocols with non-method members"
Expand All @@ -2084,7 +2095,7 @@ def _proto_hook(other):
raise TypeError('issubclass() arg 1 must be a class')

# Second, perform the actual structural compatibility check.
for attr in _get_protocol_attrs(cls):
for attr in protocol_attrs:
for base in other.__mro__:
# Check if the members appears in the class dictionary...
if attr in base.__dict__:
Expand Down