| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
…ng._ProtocolMeta.__instancecheck__
|
On main, the pyperformance bm_typing_runtime_protocols benchmark takes 177us +- 5us on my machine. With this PR, it takes 192us +- 5us. So, a slight slowdown. But that's okay if we avoid incorrect behaviour. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
Oh, we will still hit the problematic super().__instancecheck__ sooner or later. Consider this from typing import runtime_checkable, Generic, Protocol, TypeVar
T = TypeVar("T")
@runtime_checkable
class Spam(Protocol[T]):
x: T
class Eggs(Generic[T]):
def __init__(self, x: T) -> None:
self._x = x
def __getattr__(self, attr: str) -> T:
if attr == "x":
return self._x
raise AttributeError(attr)
print(isinstance(Eggs(42), Spam))
print(issubclass(Eggs, Spam))Running on 3.10, 3.11 -> TypeError |
Sorry, something went wrong.
|
I think one possible solution is that "do not let super().__subclasscheck()__ cache __subclasscheck__ result in this case". |
Sorry, something went wrong.
|
@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.) |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
|
This new approach also has the advantage that it seems like it might provide a speedup relative to main, for isinstance checks that don't involve ABCMeta.register()? (But might just be noise.) |
Sorry, something went wrong.
|
This new approach looks better. Great Job! |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
Co-authored-by: Carl Meyer <carl@oddbird.net>
|
Thanks @JelleZijlstra for helping debug this, @sunmy2019 for spotting the flaws in my original approach, and everybody for reviewing! |
Sorry, something went wrong.
⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️Hi! The buildbot AMD64 Windows11 Bigmem 3.x has failed when building commit b27fe67. What do you need to do:
You can take a look at the buildbot page here: https://buildbot.python.org/all/#builders/1079/builds/1325 Failed tests:
Failed subtests:
Summary of the results of the build (if available): == Tests result: FAILURE then FAILURE == 428 tests OK. 10 slowest tests:
1 test failed: 36 tests skipped: 1 re-run test: Total duration: 47 min 52 sec Click to see traceback logsTraceback (most recent call last):
File "R:\buildarea\3.x.ambv-bb-win11.bigmem\build\Lib\test\support\__init__.py", line 979, in wrapper
return f(self, maxsize)
^^^^^^^^^^^^^^^^
File "R:\buildarea\3.x.ambv-bb-win11.bigmem\build\Lib\test\test_hashlib.py", line 540, in test_case_md5_uintmax
self.check('md5', b'A'*size, '28138d306ff1b8281f1a9067e1a1a2b3')
File "R:\buildarea\3.x.ambv-bb-win11.bigmem\build\Lib\test\test_hashlib.py", line 394, in check
self.check_file_digest(name, data, hexdigest)
File "R:\buildarea\3.x.ambv-bb-win11.bigmem\build\Lib\test\test_hashlib.py", line 407, in check_file_digest
f.write(data)
OSError: [Errno 28] No space left on device
Traceback (most recent call last):
File "R:\buildarea\3.x.ambv-bb-win11.bigmem\build\Lib\test\support\__init__.py", line 979, in wrapper
return f(self, maxsize)
^^^^^^^^^^^^^^^^
File "R:\buildarea\3.x.ambv-bb-win11.bigmem\build\Lib\test\test_hashlib.py", line 535, in test_case_md5_huge
self.check('md5', b'A'*size, 'c9af2dff37468ce5dfee8f2cfc0a9c6d')
File "R:\buildarea\3.x.ambv-bb-win11.bigmem\build\Lib\test\test_hashlib.py", line 394, in check
self.check_file_digest(name, data, hexdigest)
File "R:\buildarea\3.x.ambv-bb-win11.bigmem\build\Lib\test\test_hashlib.py", line 407, in check_file_digest
f.write(data)
OSError: [Errno 28] No space left on device
|
Sorry, something went wrong.
…s to `isinstance()` influence whether `issubclass()` raises an exception (python#104559) Co-authored-by: Carl Meyer <carl@oddbird.net>
| Back | FazBrowse Home | New Git URL |
ABCMeta.__instancecheck__ caches isinstance() calls against classes that have ABCMeta as their metaclass. It uses these cache entries not only to inform how future isinstance() calls behave, but also how issubclass() calls behave. That means that on main we now have some rather unfortunate behaviour when it comes to runtime-checkable protocols, due to the fact that typing._ProtocolMeta is a subclass of ABCMeta, and typing._ProtocolMeta.__instancecheck__ calls super().__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 on main:
But they'll still be much faster than they are on 3.11. Use of ABCMeta.register with protocols is pretty rare anyway, as far as I know, since ABCMeta.register isn't supported by type checkers. Other kinds of isinstance() checks do not suffer a significant performance regression.
Fixes #104555.
(Skipping news, as this bug doesn't exist on any released version of Python.)