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

gh-132493: lazy evaluation of annotations in `typing._proto_hook` by felixscherz · Pull Request #132534 · python/cpython · GitHub

Repository navigation

gh-132493: lazy evaluation of annotations in typing._proto_hook - #132534

Merged
JelleZijlstra merged 13 commits into
python:mainfrom
felixscherz:fix/protocol-isinstance-lazy-annotations
Apr 16, 2025
Merged

JelleZijlstra merged 13 commits into
python:mainfrom
felixscherz:fix/protocol-isinstance-lazy-annotations

Conversation

felixscherz commented Apr 14, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

Hi, this is in regards to #132493. This PR avoids eager evaluation of annotations during isinstance checks for protocols.

JelleZijlstra left a comment

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

Thank you!

Comment thread Lib/test/test_typing.py Outdated
Comment thread Lib/test/test_typing.py
x: undefined

self.assertFalse(isinstance(DeferredClass(), P))

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

Also add a test with a class that does fulfill the protocol.

Also, please add a NEWS entry.

Copy link
Copy Markdown
Contributor 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 added a class that satisfies the protocol and a NEWS entry:)

Co-authored-by: sobolevn <mail@sobolevn.me>
Comment thread Lib/typing.py Outdated
…aranteed

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Comment thread Lib/test/test_typing.py
Comment thread Lib/typing.py Outdated
annotations = _lazy_annotationlib.get_annotations(base, format=_lazy_annotationlib.Format.FORWARDREF)
if (attr in annotations
and issubclass(other, Generic)
and getattr(other, '_is_protocol', False)):

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

We can probably gain some performance by doing the issubclass() and getattr() checks before doing the annotations check, since they should be fast and for most use cases they'll return False (it's surely more common to do isinstance(<non protocol>, Protocol), not isinstance(<other protocol>, Protocol)).

Copy link
Copy Markdown
Contributor 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 moved the issubclass() and geattr() checks up:)

Copy link
Copy Markdown
Contributor 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

With the new order of conditions we could also move the call to _lazy_annotationlib.get_annotations so we only do that in case the other two conditions are true, what do you think?
Like this:

    if (
        issubclass(other, Generic)
        and getattr(other, "_is_protocol", False)
        and attr in _lazy_annotationlib.get_annotations(base, format=_lazy_annotationlib.Format.FORWARDREF)
    ):

Comment thread Lib/typing.py Outdated
Comment thread Lib/test/test_typing.py Outdated
meth: undefined


self.assertTrue(issubclass(SubProtocol, P))

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

These can use helpers assertIsSubclass, assertIsInstance, assertNotIsinstance.

Copy link
Copy Markdown
Contributor 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

Updated it to use the helper methods

JelleZijlstra merged commit 71af090 into python:main Apr 16, 2025
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL