Repository: python/cpython
Pull request: gh-104555: Runtime-checkable protocols: Don't let previous calls to `isinstance()` influence whether `issubclass()` raises an exception
`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 https://github.com/python/cpython/commit/47753ecde21b79b5c5f11d883946fda2a340e427):

```pycon
Python 3.12.0a7+ (heads/main:1163782868, May 16 2023, 18:09:28) [MSC v.1932 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from typing import *
>>> @runtime_checkable
... class Foo(Protocol):
...     x: int
...
>>> class Bar:
...     x = 42
...
>>> issubclass(Bar, Foo)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\alexw\coding\cpython\Lib\abc.py", line 123, in __subclasscheck__
    return _abc_subclasscheck(cls, subclass)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\coding\cpython\Lib\typing.py", line 1875, in _proto_hook
    raise TypeError("Protocols with non-method members"
TypeError: Protocols with non-method members don't support issubclass()
>>> isinstance(Bar(), Foo)
True
>>> issubclass(Bar, Foo)
False
```

This PR fixes the incorrect behaviour. It means that these `isinstance()` checks will be about twice as slow as they are on `main`:

```py
@runtime_checkable
class Foo:
    def meth(self): ...

@Foo.register
class Bar: ...

isinstance(Bar(), Foo)
```

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.)

<!-- gh-issue-number: gh-104555 -->
* Issue: gh-104555
<!-- /gh-issue-number -->


Problem statement:
Typing: runtime-checkable protocols are broken on `main`
PEP-695 protocols don't work as intended:

Here's the behaviour you get with protocols that use pre-PEP 695 syntax, which is correct:

```pycon
>>> from typing import Protocol, runtime_checkable, TypeVar
>>> T_co = TypeVar("T_co", covariant=True)
>>> @runtime_checkable
... class SupportsAbsOld(Protocol[T_co]):
...     def __abs__(self) -> T_co:
...         ...
...
>>> isinstance(0, SupportsAbsOld)
True
>>> issubclass(float, SupportsAbsOld)
True
```

And here's the behaviour you get on `main` with protocols that use PEP 695 syntax, which is incorrect:

```pycon
>>> @runtime_checkable
... class SupportsAbsNew[T_co](Protocol):
...     def __abs__(self) -> T_co:
...         ...
...
>>> isinstance(0, SupportsAbsNew)
False
>>> issubclass(float, SupportsAbsNew)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\alexw\coding\cpython\Lib\abc.py", line 123, in __subclasscheck__
    return _abc_subclasscheck(cls, subclass)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\coding\cpython\Lib\typing.py", line 1875, in _proto_hook
    raise TypeError("Protocols with non-method members"
TypeError: Protocols with non-method members don't support issubclass()
```

<!-- gh-linked-prs -->
### Linked PRs
* gh-104556
* gh-104559
<!-- /gh-linked-prs -->

Hints from issue discussion:
(I'm working on a fix)
Thanks for catching! I suppose this is about `__type_params__`?
> Thanks for catching! I suppose this is about `__type_params__`?

Yes.
I'm getting very confused here. The fix for the original issue is simple, and I've fixed it locally. But as part of the PR, I tried adding this test, and the last assertion fails (the test reports that `TypeError` _wasn't_ raised):

```py
    def test_pep695_generic_protocol_noncallable_members(self):
        @runtime_checkable
        class Spam[T](Protocol):
            x: T

        class Eggs[T]:
            def __init__(self, x: T) -> None:
                self.x = x

        self.assertIsInstance(Eggs(42), Spam)
        with self.assertRaises(TypeError):
            issubclass(Eggs, Spam)
```

But I can't reproduce the failure in the REPL:

```pycon
>>> from typing import *
>>> @runtime_checkable
... class Spam[T](Protocol):
...     x: T
... 
...     
>>> class Eggs[T]:
...     def __init__(self, x: T) -> None:
...         self.x = x
... 
...         
>>> issubclass(Eggs, Spam)
Traceback (most recent call last):
  File "<pyshell#5>", line 1, in <module>
    issubclass(Eggs, Spam)
  File "C:\Users\alexw\coding\cpython\Lib\abc.py", line 123, in __subclasscheck__
    return _abc_subclasscheck(cls, subclass)
  File "C:\Users\alexw\coding\cpython\Lib\typing.py", line 1875, in _proto_hook
    raise TypeError("Protocols with non-method members"
TypeError: Protocols with non-method members don't support issubclass()
```
Anyway, I'll submit a PR without that test for now...
I'll look into this more.
Okay, cool. I'll enable automerge on my PR for now, as it definitely gets us closer to correct behaviour.
The issue you saw also reproduces for me without PEP 695 syntax. It seems to be due to the `_allow_reckless_class_checks` check, which somehow thinks the caller is `abc` if we're in a test, but not if we're in the REPL.
Aha, so we've discovered a bug, it just isn't a new bug.
It's possible this bug was introduced by the fact that Generic is now a C class, haven't tried on 3.11 yet.

Commit message: 📜🤖 Added by blurb_it.

Patch to review:
diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py
index 0cd67c51e50838..a8b8434d87fa0b 100644
--- a/Lib/test/test_typing.py
+++ b/Lib/test/test_typing.py
@@ -2654,6 +2654,82 @@ 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):
+            x: int
+
+        class Eggs:
+            def __init__(self) -> None:
+                self.x = 42
+
+        self.assertIsInstance(Eggs(), Spam)
+
+        # 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)
+
+    def test_no_weird_caching_with_issubclass_after_isinstance2(self):
+        @runtime_checkable
+        class Spam(Protocol):
+            x: int
+
+        class Eggs: ...
+
+        self.assertNotIsInstance(Eggs(), Spam)
+
+        # 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)
+
+    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)
+
+        self.assertNotIsInstance(Eggs(), Spam)
+
+        # 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)
+
+    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
+
+        self.assertIsInstance(Eggs(42), Spam)
+
+        # 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)
+
     def test_protocols_isinstance(self):
         T = TypeVar('T')
 
diff --git a/Lib/typing.py b/Lib/typing.py
index 50a8f515945804..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__.
@@ -1869,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')
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 <typing.runtime_checkable>` ``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.


Code review comments:
diff_hunk: @@ -2654,6 +2654,82 @@ 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):
+            x: int
+
+        class Eggs:
+            def __init__(self) -> None:
+                self.x = 42
+
+        self.assertIsInstance(Eggs(), Spam)
+
+        # 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)
+
+    def test_no_weird_caching_with_issubclass_after_isinstance2(self):
path: Lib/test/test_typing.py
text: @user1:
tiny nit for readability (took me a couple looks to realize this wasn't a duplicate of the previous test name)
```suggestion
    def test_no_weird_caching_with_issubclass_after_isinstance_2(self):
```

diff_hunk: @@ -2654,6 +2654,82 @@ 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):
+            x: int
+
+        class Eggs:
+            def __init__(self) -> None:
+                self.x = 42
+
+        self.assertIsInstance(Eggs(), Spam)
+
+        # 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)
+
+    def test_no_weird_caching_with_issubclass_after_isinstance2(self):
+        @runtime_checkable
+        class Spam(Protocol):
+            x: int
+
+        class Eggs: ...
+
+        self.assertNotIsInstance(Eggs(), Spam)
+
+        # 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)
+
+    def test_no_weird_caching_with_issubclass_after_isinstance3(self):
path: Lib/test/test_typing.py
text: @user1:
```suggestion
    def test_no_weird_caching_with_issubclass_after_isinstance_3(self):
```