FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-74690: typing: Don't unnecessarily call `_get_protocol_attrs` twice in `_ProtocolMeta.__instancecheck__` by AlexWaygood · Pull Request #103141 · python/cpython · GitHub

/ cpython Public

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

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

Merged
AlexWaygood merged 4 commits into from
Mar 31, 2023
Conversation 12 Commits 4 Checks 0 Files changed
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 .py  (1) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
31 changes: 21 additions & 10 deletions Lib/typing.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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)

AlexWaygood Mar 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

posita Mar 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.)

AlexWaygood Mar 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 :)

carljm Mar 31, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

AlexWaygood Mar 31, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Back | FazBrowse Home | New Git URL