Skip to content

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

gh-132493: lazy evaluation of annotations in typing._proto_hook#132534
JelleZijlstra merged 13 commits into
python:mainfrom
felixscherz:fix/protocol-isinstance-lazy-annotations

Conversation

@felixscherz

@felixscherzfelixscherz commented Apr 14, 2025

Copy link
Copy Markdown
Contributor

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

@JelleZijlstraJelleZijlstra 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.

Thank you!

Comment threadLib/test/test_typing.py Outdated
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.

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

Also, please add a NEWS entry.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I added a class that satisfies the protocol and a NEWS entry:)

Comment threadMisc/NEWS.d/next/Library/2025-04-15-08-39-14.gh-issue-132493.V0gLkU.rst Outdated
Comment threadMisc/NEWS.d/next/Library/2025-04-15-08-39-14.gh-issue-132493.V0gLkU.rst Outdated
Co-authored-by: sobolevn <mail@sobolevn.me>
Comment threadLib/typing.py Outdated
…aranteed
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Comment threadLib/test/test_typing.py
Comment threadLib/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.

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
ContributorAuthor

Choose a reason for hiding this comment

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

I moved the issubclass() and geattr() checks up:)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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)
andgetattr(other, "_is_protocol", False)
andattrin_lazy_annotationlib.get_annotations(base, format=_lazy_annotationlib.Format.FORWARDREF)
):

Comment threadLib/typing.py Outdated
Comment threadLib/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.

These can use helpers assertIsSubclass, assertIsInstance, assertNotIsinstance.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated it to use the helper methods

@JelleZijlstra
JelleZijlstra merged commit 71af090 into python:mainApr 16, 2025
Sign up for freeto 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

@felixscherz@JelleZijlstra@sobolevn@AlexWaygood