From 2601138e6379e5e4305a07181a80a266488d9968 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Tue, 16 May 2023 18:46:06 +0100 Subject: [PATCH 01/11] gh-104555: Sidestep the ABCMeta.__instancecheck__ cache in typing._ProtocolMeta.__instancecheck__ --- Lib/test/test_typing.py | 36 ++++++++++++++++++++++++++++++++++++ Lib/typing.py | 7 +++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index 0cd67c51e50838..c5e48d7d71defb 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -2654,6 +2654,42 @@ class D(PNonCall): ... with self.assertRaises(TypeError): issubclass(D, PNonCall) + def test_no_weird_caching_with_issubclass_after_isinstance(self): + @runtime_checkable + class Spam(Protocol[T]): + x: T + + class Eggs(Generic[T]): + def __init__(self, x: T) -> None: + self.x = x + + # gh-104555: ABCMeta might cache the result of this isinstance check + # if we called super().__instancecheck__ in the wrong place + # in _ProtocolMeta.__instancheck__... + self.assertIsInstance(Eggs(42), Spam) + + # ...and if it did, then TypeError wouldn't be raised here! + with self.assertRaises(TypeError): + issubclass(Eggs, Spam) + + def test_no_weird_caching_with_issubclass_after_isinstance_pep695(self): + @runtime_checkable + class Spam[T](Protocol): + x: T + + class Eggs[T]: + def __init__(self, x: T) -> None: + self.x = x + + # gh-104555: ABCMeta might cache the result of this isinstance check + # if we called super().__instancecheck__ in the wrong place + # in _ProtocolMeta.__instancheck__... + self.assertIsInstance(Eggs(42), Spam) + + # ...and if it did, then TypeError wouldn't be raised here! + with self.assertRaises(TypeError): + issubclass(Eggs, Spam) + def test_protocols_isinstance(self): T = TypeVar('T') diff --git a/Lib/typing.py b/Lib/typing.py index 50a8f515945804..0bfb39fb0c0103 100644 --- a/Lib/typing.py +++ b/Lib/typing.py @@ -1798,7 +1798,10 @@ def __instancecheck__(cls, instance): raise TypeError("Instance and class checks can only be used with" " @runtime_checkable protocols") - if super().__instancecheck__(instance): + # gh-104555: Don't call super().__instancecheck__ here, + # ABCMeta.__instancecheck__ would erroneously use it to populate the cache, + # which would cause incorrect results for *issubclass()* calls + if type.__instancecheck__(cls, instance): return True if is_protocol_cls: @@ -1813,7 +1816,7 @@ def __instancecheck__(cls, instance): else: return True - return False + return super().__instancecheck__(instance) class Protocol(Generic, metaclass=_ProtocolMeta): From f3a4551bbfcbb7cdfb9207af76fc739ea9c19611 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Tue, 16 May 2023 19:24:07 +0100 Subject: [PATCH 02/11] Simplify test --- Lib/test/test_typing.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index c5e48d7d71defb..d4d9d71c158b99 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -2656,17 +2656,17 @@ class D(PNonCall): ... def test_no_weird_caching_with_issubclass_after_isinstance(self): @runtime_checkable - class Spam(Protocol[T]): - x: T + class Spam(Protocol): + x: int - class Eggs(Generic[T]): - def __init__(self, x: T) -> None: - self.x = x + class Eggs: + def __init__(self) -> None: + self.x = 42 # gh-104555: ABCMeta might cache the result of this isinstance check # if we called super().__instancecheck__ in the wrong place # in _ProtocolMeta.__instancheck__... - self.assertIsInstance(Eggs(42), Spam) + self.assertIsInstance(Eggs(), Spam) # ...and if it did, then TypeError wouldn't be raised here! with self.assertRaises(TypeError): From 714badc0f3ecef0cfc070a6d2dab37847aaeb344 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Tue, 16 May 2023 19:46:53 +0100 Subject: [PATCH 03/11] Optimise --- Lib/typing.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Lib/typing.py b/Lib/typing.py index 0bfb39fb0c0103..f1720e97c0d42e 100644 --- a/Lib/typing.py +++ b/Lib/typing.py @@ -1804,6 +1804,9 @@ def __instancecheck__(cls, instance): if type.__instancecheck__(cls, instance): return True + if cls.__callable_proto_members_only__ and issubclass(type(instance), cls): + return True + if is_protocol_cls: getattr_static = _lazy_load_getattr_static() for attr in cls.__protocol_attrs__: From 04ed9747351f16b86aea6177661d56103cf4aa71 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 16 May 2023 20:11:42 +0100 Subject: [PATCH 04/11] who needs codespell when we have carl Co-authored-by: Carl Meyer --- Lib/test/test_typing.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index d4d9d71c158b99..009790a60b7fdd 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -2665,7 +2665,7 @@ def __init__(self) -> None: # gh-104555: ABCMeta might cache the result of this isinstance check # if we called super().__instancecheck__ in the wrong place - # in _ProtocolMeta.__instancheck__... + # in _ProtocolMeta.__instancecheck__... self.assertIsInstance(Eggs(), Spam) # ...and if it did, then TypeError wouldn't be raised here! @@ -2683,7 +2683,7 @@ def __init__(self, x: T) -> None: # gh-104555: ABCMeta might cache the result of this isinstance check # if we called super().__instancecheck__ in the wrong place - # in _ProtocolMeta.__instancheck__... + # in _ProtocolMeta.__instancecheck__... self.assertIsInstance(Eggs(42), Spam) # ...and if it did, then TypeError wouldn't be raised here! From 49373a9b123e19c90365ed7add2358e2ef69b889 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Tue, 16 May 2023 20:24:34 +0100 Subject: [PATCH 05/11] be more careful --- Lib/typing.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/typing.py b/Lib/typing.py index f1720e97c0d42e..0d9d7c5d4c95d6 100644 --- a/Lib/typing.py +++ b/Lib/typing.py @@ -1804,10 +1804,10 @@ def __instancecheck__(cls, instance): if type.__instancecheck__(cls, instance): return True - if cls.__callable_proto_members_only__ and issubclass(type(instance), cls): - return True - if is_protocol_cls: + if cls.__callable_proto_members_only__ and issubclass(type(instance), cls): + return True + getattr_static = _lazy_load_getattr_static() for attr in cls.__protocol_attrs__: try: From e114fe95049ef27f36774fbc9d17436b4a9343fd Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Tue, 16 May 2023 20:27:37 +0100 Subject: [PATCH 06/11] Add comment --- Lib/typing.py | 1 + 1 file changed, 1 insertion(+) diff --git a/Lib/typing.py b/Lib/typing.py index 0d9d7c5d4c95d6..911b8f6b1050bc 100644 --- a/Lib/typing.py +++ b/Lib/typing.py @@ -1805,6 +1805,7 @@ def __instancecheck__(cls, instance): return True if is_protocol_cls: + # Fast path for protocols with only callable members if cls.__callable_proto_members_only__ and issubclass(type(instance), cls): return True From b5cc9cd59072c950f8c399c6e54b25c0333cf1ac Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Wed, 17 May 2023 13:02:41 +0100 Subject: [PATCH 07/11] Add more failing tests (thanks sunmy2019!) --- Lib/test/test_typing.py | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index 009790a60b7fdd..7467e444f67bbf 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -2672,6 +2672,42 @@ def __init__(self) -> None: with self.assertRaises(TypeError): issubclass(Eggs, Spam) + 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) + + def test_no_weird_caching_with_issubclass_after_isinstance3(self): + @runtime_checkable + class Spam(Protocol): + x: int + + class Eggs: + def __getattr__(self, attr): + if attr == "x": + return 42 + raise AttributeError(attr) + + # 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) + def test_no_weird_caching_with_issubclass_after_isinstance_pep695(self): @runtime_checkable class Spam[T](Protocol): From 5f72b820e68c63c44c51f6c7f7f068e6cfab57c2 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Wed, 17 May 2023 13:15:49 +0100 Subject: [PATCH 08/11] Better fix --- Lib/typing.py | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/Lib/typing.py b/Lib/typing.py index 911b8f6b1050bc..36f6be3c61b279 100644 --- a/Lib/typing.py +++ b/Lib/typing.py @@ -1775,8 +1775,8 @@ def _pickle_pskwargs(pskwargs): class _ProtocolMeta(ABCMeta): - # This metaclass is really unfortunate and exists only because of - # the lack of __instancehook__. + # This metaclass is somewhat unfortunate, + # but is necessary for several reasons... def __init__(cls, *args, **kwargs): super().__init__(*args, **kwargs) cls.__protocol_attrs__ = _get_protocol_attrs(cls) @@ -1786,6 +1786,17 @@ def __init__(cls, *args, **kwargs): callable(getattr(cls, attr, None)) for attr in cls.__protocol_attrs__ ) + def __subclasscheck__(cls, other): + if ( + getattr(cls, '_is_protocol', False) + and not cls.__callable_proto_members_only__ + and not _allow_reckless_class_checks(depth=2) + ): + raise TypeError( + "Protocols with non-method members don't support issubclass()" + ) + return super().__subclasscheck__(other) + def __instancecheck__(cls, instance): # We need this method for situations where attributes are # assigned in __init__. @@ -1798,17 +1809,10 @@ def __instancecheck__(cls, instance): raise TypeError("Instance and class checks can only be used with" " @runtime_checkable protocols") - # gh-104555: Don't call super().__instancecheck__ here, - # ABCMeta.__instancecheck__ would erroneously use it to populate the cache, - # which would cause incorrect results for *issubclass()* calls - if type.__instancecheck__(cls, instance): + if super().__instancecheck__(instance): return True if is_protocol_cls: - # Fast path for protocols with only callable members - if cls.__callable_proto_members_only__ and issubclass(type(instance), cls): - return True - getattr_static = _lazy_load_getattr_static() for attr in cls.__protocol_attrs__: try: @@ -1820,7 +1824,7 @@ def __instancecheck__(cls, instance): else: return True - return super().__instancecheck__(instance) + return False class Protocol(Generic, metaclass=_ProtocolMeta): @@ -1876,11 +1880,6 @@ def _proto_hook(other): raise TypeError("Instance and class checks can only be used with" " @runtime_checkable protocols") - if not cls.__callable_proto_members_only__ : - if _allow_reckless_class_checks(): - return NotImplemented - raise TypeError("Protocols with non-method members" - " don't support issubclass()") if not isinstance(other, type): # Same error message as for issubclass(1, int). raise TypeError('issubclass() arg 1 must be a class') From bb56e1318a56a41ea21e21aec179e751cad0197f Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Wed, 17 May 2023 16:00:18 +0100 Subject: [PATCH 09/11] Improve comments --- Lib/test/test_typing.py | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index 7467e444f67bbf..a8b8434d87fa0b 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -2663,12 +2663,13 @@ class Eggs: def __init__(self) -> None: self.x = 42 - # gh-104555: ABCMeta might cache the result of this isinstance check - # if we called super().__instancecheck__ in the wrong place - # in _ProtocolMeta.__instancecheck__... self.assertIsInstance(Eggs(), Spam) - # ...and if it did, then TypeError wouldn't be raised here! + # gh-104555: If we didn't override ABCMeta.__subclasscheck__ in _ProtocolMeta, + # TypeError wouldn't be raised here, + # as the cached result of the isinstance() check immediately above + # would mean the issubclass() call would short-circuit + # before we got to the "raise TypeError" line with self.assertRaises(TypeError): issubclass(Eggs, Spam) @@ -2679,12 +2680,13 @@ class Spam(Protocol): 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! + # gh-104555: If we didn't override ABCMeta.__subclasscheck__ in _ProtocolMeta, + # TypeError wouldn't be raised here, + # as the cached result of the isinstance() check immediately above + # would mean the issubclass() call would short-circuit + # before we got to the "raise TypeError" line with self.assertRaises(TypeError): issubclass(Eggs, Spam) @@ -2699,12 +2701,13 @@ def __getattr__(self, attr): return 42 raise AttributeError(attr) - # 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! + # gh-104555: If we didn't override ABCMeta.__subclasscheck__ in _ProtocolMeta, + # TypeError wouldn't be raised here, + # as the cached result of the isinstance() check immediately above + # would mean the issubclass() call would short-circuit + # before we got to the "raise TypeError" line with self.assertRaises(TypeError): issubclass(Eggs, Spam) @@ -2717,12 +2720,13 @@ class Eggs[T]: def __init__(self, x: T) -> None: self.x = x - # gh-104555: ABCMeta might cache the result of this isinstance check - # if we called super().__instancecheck__ in the wrong place - # in _ProtocolMeta.__instancecheck__... self.assertIsInstance(Eggs(42), Spam) - # ...and if it did, then TypeError wouldn't be raised here! + # gh-104555: If we didn't override ABCMeta.__subclasscheck__ in _ProtocolMeta, + # TypeError wouldn't be raised here, + # as the cached result of the isinstance() check immediately above + # would mean the issubclass() call would short-circuit + # before we got to the "raise TypeError" line with self.assertRaises(TypeError): issubclass(Eggs, Spam) From 430cacd7795ea6603c16f759483e4fbaa44067d3 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Wed, 17 May 2023 16:58:27 +0000 Subject: [PATCH 10/11] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Library/2023-05-17-16-58-23.gh-issue-104555.5rb5oM.rst | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2023-05-17-16-58-23.gh-issue-104555.5rb5oM.rst diff --git a/Misc/NEWS.d/next/Library/2023-05-17-16-58-23.gh-issue-104555.5rb5oM.rst b/Misc/NEWS.d/next/Library/2023-05-17-16-58-23.gh-issue-104555.5rb5oM.rst new file mode 100644 index 00000000000000..2992346484c5fa --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-05-17-16-58-23.gh-issue-104555.5rb5oM.rst @@ -0,0 +1,7 @@ +Fix issue where an :func:`issubclass` check comparing a class ``X`` against a +:func:`runtime-checkable protocol ` ``Y`` with +non-callable members would not cause :exc:`TypeError` to be raised if an +:func:`isinstance` call had previously been made comparing an instance of ``X`` +to ``Y``. This issue was present in edge cases on Python 3.11, but became more +prominent in 3.12 due to some unrelated changes that were made to +runtime-checkable protocols. Patch by Alex Waygood. From f9273fc978d52d3c870c8d4d14d3cafce36c5286 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 18 May 2023 00:20:48 +0100 Subject: [PATCH 11/11] Apply suggestions from code review Co-authored-by: Carl Meyer --- Lib/test/test_typing.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py index a8b8434d87fa0b..8eadf905de791e 100644 --- a/Lib/test/test_typing.py +++ b/Lib/test/test_typing.py @@ -2673,7 +2673,7 @@ def __init__(self) -> None: with self.assertRaises(TypeError): issubclass(Eggs, Spam) - def test_no_weird_caching_with_issubclass_after_isinstance2(self): + def test_no_weird_caching_with_issubclass_after_isinstance_2(self): @runtime_checkable class Spam(Protocol): x: int @@ -2690,7 +2690,7 @@ class Eggs: ... with self.assertRaises(TypeError): issubclass(Eggs, Spam) - def test_no_weird_caching_with_issubclass_after_isinstance3(self): + def test_no_weird_caching_with_issubclass_after_isinstance_3(self): @runtime_checkable class Spam(Protocol): x: int