Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-104555: Runtime-checkable protocols: Don't let previous calls to isinstance() influence whether issubclass() raises an exception#104559
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
AlexWaygood commented May 16, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
…ng._ProtocolMeta.__instancecheck__
Uh oh!
There was an error while loading. Please reload this page.
AlexWaygood commented May 16, 2023
On |
sunmy2019 left a comment
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.
LGTM
But I still think __subclasshook__ depending on the caller is the weirdest part (of #104555 (comment)). Though it cannot be changed or is hard to change.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Carl Meyer <carl@oddbird.net>
…d/cpython into fix-runtime-protocols
Lib/typing.py Outdated
| ifcls.__callable_proto_members_only__andissubclass(type(instance), cls): | ||
| returnTrue |
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.
Without this check, I realised that this patch would also have slowed down isinstance(3, SupportsIndex) quite dramatically.
sunmy2019 commented May 17, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Oh, we will still hit the problematic Consider this fromtypingimportruntime_checkable, Generic, Protocol, TypeVarT=TypeVar("T") @runtime_checkableclassSpam(Protocol[T]): x: TclassEggs(Generic[T]): def__init__(self, x: T) ->None: self._x=xdef__getattr__(self, attr: str) ->T: ifattr=="x": returnself._xraiseAttributeError(attr) print(isinstance(Eggs(42), Spam)) print(issubclass(Eggs, Spam))Running on 3.10, 3.11 -> TypeError |
sunmy2019 commented May 17, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I think one possible solution is that "do not let |
AlexWaygood commented May 17, 2023
@sunmy2019, I think we were thinking the same thought simultaneously :) How does it look to you after 5f72b82? (It's basically a completely different approach now.) |
Lib/test/test_typing.py Outdated
| deftest_no_weird_caching_with_issubclass_after_isinstance2(self): | ||
| @runtime_checkable | ||
| classSpam(Protocol): | ||
| x: int | ||
| classEggs: ... | ||
| # gh-104555: ABCMeta might cache the result of this isinstance check | ||
| # if we called super().__instancecheck__ in the wrong place | ||
| # in _ProtocolMeta.__instancecheck__... | ||
| self.assertNotIsInstance(Eggs(), Spam) | ||
| # ...and if it did, then TypeError wouldn't be raised here! | ||
| withself.assertRaises(TypeError): | ||
| issubclass(Eggs, Spam) |
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.
This test fails on 3.11. We could consider backporting a version of this PR to 3.11, but I'm not sure if that would be a good idea or not. It's a somewhat invasive bugfix.
ABCMeta.__instancecheck__ cache in typing._ProtocolMeta.__instancecheck__isinstance() influence whether issubclass() raises an exceptionAlexWaygood commented May 17, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
This new approach also has the advantage that it seems like it might provide a speedup relative to |
sunmy2019 commented May 17, 2023
This new approach looks better. Great Job! |
carljm left a comment
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.
LGTM
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Carl Meyer <carl@oddbird.net>
AlexWaygood commented May 17, 2023
Thanks @JelleZijlstra for helping debug this, @sunmy2019 for spotting the flaws in my original approach, and everybody for reviewing! |
bedevere-bot commented May 18, 2023
|
…s to `isinstance()` influence whether `issubclass()` raises an exception (python#104559) Co-authored-by: Carl Meyer <carl@oddbird.net>
ABCMeta.__instancecheck__cachesisinstance()calls against classes that haveABCMetaas their metaclass. It uses these cache entries not only to inform how futureisinstance()calls behave, but also howissubclass()calls behave. That means that onmainwe now have some rather unfortunate behaviour when it comes to runtime-checkable protocols, due to the fact thattyping._ProtocolMetais a subclass ofABCMeta, andtyping._ProtocolMeta.__instancecheck__callssuper().__instancecheck__()too soon (following 47753ec):This PR fixes the incorrect behaviour. It means that these
isinstance()checks will be about twice as slow as they are onmain:But they'll still be much faster than they are on 3.11. Use of
ABCMeta.registerwith protocols is pretty rare anyway, as far as I know, sinceABCMeta.registerisn't supported by type checkers. Other kinds ofisinstance()checks do not suffer a significant performance regression.Fixes#104555.
(Skipping news, as this bug doesn't exist on any released version of Python.)
main#104555