Uh oh!
There was an error while loading. Please reload this page.
gh-104555: Runtime-checkable protocols: Don't let previous calls to isinstance() influence whether issubclass() raises an exception - #104559
Conversation
…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.
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
| if cls.__callable_proto_members_only__ and issubclass(type(instance), cls): | ||
| return True |
There was a problem hiding this comment.
Without this check, I realised that this patch would also have slowed down isinstance(3, SupportsIndex) quite dramatically.
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 |
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.) |
| def test_no_weird_caching_with_issubclass_after_isinstance2(self): | ||
| @runtime_checkable | ||
| class Spam(Protocol): | ||
| x: int | ||
| class Eggs: ... | ||
| # 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! | ||
| with self.assertRaises(TypeError): | ||
| issubclass(Eggs, Spam) |
There was a problem hiding this comment.
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 exceptionThis 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! |
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