-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
gh-102433: Use inspect.getattr_static
in typing._ProtocolMeta.__instancecheck__
#103034
Merged
AlexWaygood
merged 11 commits into
python:main
from
AlexWaygood:runtime-protocols-getattr_static
Apr 2, 2023
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
212fce2
Implementation
AlexWaygood ef71395
Tests
AlexWaygood 4407d60
docs
AlexWaygood 44eba6b
Wrong issue number
AlexWaygood 9989eea
Merge branch 'main' into runtime-protocols-getattr_static
AlexWaygood 102772c
Merge remote-tracking branch 'origin/main' into runtime-protocols-get…
AlexWaygood 5c15fbc
Fix an edge case
AlexWaygood b0a9dd1
Merge branch 'runtime-protocols-getattr_static' of https://github.com…
AlexWaygood 0812183
Move tests around, add some more
AlexWaygood 83d7ec2
Merge branch 'main' into runtime-protocols-getattr_static
AlexWaygood 6ba97e7
Simplify and optimise
AlexWaygood File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
10 changes: 10 additions & 0 deletions
10
Misc/NEWS.d/next/Library/2023-03-25-16-57-18.gh-issue-102433.L-7x2Q.rst
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
:func:`isinstance` checks against :func:`runtime-checkable protocols | ||
<typing.runtime_checkable>` now use :func:`inspect.getattr_static` rather | ||
than :func:`hasattr` to lookup whether attributes exist. This means that | ||
descriptors and :meth:`~object.__getattr__` methods are no longer | ||
unexpectedly evaluated during ``isinstance()`` checks against | ||
runtime-checkable protocols. However, it may also mean that some objects | ||
which used to be considered instances of a runtime-checkable protocol may no | ||
longer be considered instances of that protocol on Python 3.12+, and vice | ||
versa. Most users are unlikely to be affected by this change. Patch by Alex | ||
Waygood. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this really worth it?
inspect
is likely already imported by a lot of programs (e.g. anything that usesdataclasses
orasyncio
), so is it really so bad to import it here?Relatedly, my intuition was that the
@cache
wouldn't help much because the import is already cached, but in fact the cache makes it 20x faster on my computer: I guess there's still a lot of overhead in going through the import system.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.
I think it is worth it, yes:
inspect
is a pretty heavy import; the impact of adding aninspect
import at the top oftyping.py
was clearly measurable in microbenchmarks for measuring the import time oftyping
.typing
used to be very slow, but that a lot of work was done a few years ago to improve the import time by e.g. reducing the number of metaclasses. I don't want to undermine that work now.typing
.I don't think this is a great argument.
asyncio
anddataclasses
might importinspect
now, but that doesn't mean that they'll necessarily continue to do so indefinitely. If somebody decides to try to improve import the import time ofdataclasses
by moving to a lazy import ofinspect
(for example), we'd be left looking pretty silly by making a decision on the basis that "dataclasses
importsinspect
, so it's okay for us to do it too". Besides, there's lots of code that doesn't make any use ofdataclasses
orasyncio
.Yes, I was also surprised at how much the
@cache
sped things up here :)