@runtime_checkable Protocol doesn't check if __class_getitem__ exists.

[randolf-scholz]

Bug report

Bug description:

Basically, the isinstance-check is ignoring that I manually specified that the class must have __class_getitem__.

from types import GenericAlias
from typing import runtime_checkable, Protocol, Iterator, TypeVar

T_co = TypeVar("T_co", covariant=True)


@runtime_checkable
class GenericIterable(Protocol[T_co]):
    def __class_getitem__(cls, item: type) -> GenericAlias: ...
    def __iter__(self) -> Iterator[T_co]: ...


x = ["a", "b", "c"]
assert hasattr(x, "__class_getitem__")  # ✅
assert isinstance(x, GenericIterable)   # ✅

y = "abc"
assert not hasattr(y, "__class_getitem__")  # ✅
assert not isinstance(y, GenericIterable)   # ❌

I was considering this Protocol as a partial workaround for the Iterable[str] vs str issue python/typing#256.

CPython versions tested on:

3.12

Operating systems tested on:

Linux

Linked PRs

• gh-112319: Allow special protocol members #112340

[AlexWaygood]

Well, the fix here is easy -- all we have to do is this:

--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -1676,7 +1676,7 @@ class _TypingEllipsis:
 _SPECIAL_NAMES = frozenset({
     '__abstractmethods__', '__annotations__', '__dict__', '__doc__',
     '__init__', '__module__', '__new__', '__slots__',
-    '__subclasshook__', '__weakref__', '__class_getitem__',
+    '__subclasshook__', '__weakref__',
     '__match_args__',
 })

With that change applied, your test passes fine:

Python 3.13.0a1+ (heads/main:e5dfcc2b6e, Nov 15 2023, 17:23:51) [MSC v.1932 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from types import GenericAlias
>>> from typing import runtime_checkable, Protocol, Iterator, TypeVar
>>> T_co = TypeVar("T_co", covariant=True)
>>> @runtime_checkable
... class GenericIterable(Protocol[T_co]):
...     def __class_getitem__(cls, item: type) -> GenericAlias: ...
...     def __iter__(self) -> Iterator[T_co]: ...
...
>>> y = "abc"
>>> isinstance(y, GenericIterable)
False

The only trouble is, an existing test starts failing :)

cpython>python -m test test_typing
Running Debug|x64 interpreter...
Using random seed: 2331315661
0:00:00 Run 1 test sequentially
0:00:00 [1/1] test_typing
test test_typing failed -- Traceback (most recent call last):
  File "C:\Users\alexw\coding\cpython\Lib\test\test_typing.py", line 3910, in test_collections_protocols_allowed
    self.assertIsSubclass(B, Custom)
  File "C:\Users\alexw\coding\cpython\Lib\test\test_typing.py", line 63, in assertIsSubclass
    raise self.failureException(message)
AssertionError: <class 'test.test_typing.ProtocolTests.test_collections_protocols_allowed.<locals>.B'> is not a subclass of <class 'test.test_typing.ProtocolTests.test_collections_protocols_allowed.<locals>.Custom'>

test_typing failed (1 failure)

== Tests result: FAILURE ==

1 test failed:
    test_typing

Total duration: 2.0 sec
Total tests: run=607 failures=1 skipped=1
Total test files: run=1/1 failed=1
Result: FAILURE

The test in question is here:

cpython/Lib/test/test_typing.py

Lines 3898 to 3927 in 1619f43

	def test_collections_protocols_allowed(self):
	@runtime_checkable
	class Custom(collections.abc.Iterable, Protocol):
	def close(self): ...
	class A: pass
	class B:
	def __iter__(self):
	return []
	def close(self):
	return 0
	self.assertIsSubclass(B, Custom)
	self.assertNotIsSubclass(A, Custom)
	@runtime_checkable
	class ReleasableBuffer(collections.abc.Buffer, Protocol):
	def __release_buffer__(self, mv: memoryview) -> None: ...
	class C: pass
	class D:
	def __buffer__(self, flags: int) -> memoryview:
	return memoryview(b'')
	def __release_buffer__(self, mv: memoryview) -> None:
	pass
	self.assertIsSubclass(D, ReleasableBuffer)
	self.assertIsInstance(D(), ReleasableBuffer)
	self.assertNotIsSubclass(C, ReleasableBuffer)
	self.assertNotIsInstance(C(), ReleasableBuffer)

[randolf-scholz]

It is probably incorrectly checking __class_getitem__ in this case. I think Protocol should only check for manually specified attributes, I think _get_protocol_attrs needs to be patched; it should probably check if __protocol_attrs__ exists on the parent class.

Actually, aren't all base classes (except object) supposed to be Protocols for a Protocol? Then one could replace the whole _get_protocol_attrs mechanism with something like

class _ProtocolMeta(ABCMeta):
    def __init__(cls, name, bases, namespace, **kwds):
        if getattr(cls, "_is_protocol", False):
             cls.__protocol_attrs__ = (
                 _get_local_members(namespace).union(*(base.__protocol_attrs__ for base in bases))
            )

[AlexWaygood]

Actually, aren't all base classes (except object) supposed to be Protocols for a Protocol?

there are various classes in the stdlib that we pretend are Protocols, even though they actually aren't (collections.abc.Iterator, collections.abc.Buffer, contextlib.AbstractAsyncContextManager, etc.). this is so we don't have to make collections.abc import typing, which (because collections.abc is imported at startup), would slow down Python startup itself and make a lot of people quite annoyed. also import cycles would probably make importing typing in collections.abc difficult-to-impossible anyway.

[randolf-scholz]

I created a draft pull request that addresses this issue, however I have a few question/remarks:

1. Should _get_protocol_attrs still be available/work? My patch relies on inspecting namespace as it is passed to the class constructor. cls.__dict__ contains additional attributes.
2. I added some tests, for example, the following works now:

        @runtime_checkable
        class Slotted(Protocol):
            """Matches classes that have a __slots__ attribute."""
            __slots__: tuple

        class Unslotted:
            pass

        class WithSLots:
            __slots__ = ("foo", "bar")

        assert Slotted.__protocol_attrs__ == {"__slots__"}
        self.assertNotIsInstance(Unslotted(), Slotted)
        self.assertIsInstance(WithSLots(), Slotted)

And we can even do things like

        class Documented(Protocol):
            """Matches classes that have a docstring."""
            __doc__: str

        assert Documented.__protocol_attrs__ == {"__doc__"}

However, this will only be useful in a static context, since __doc__ is set to None automatically if not given, so isinstance would potentially give unexpected results to users not aware of this.

[randolf-scholz]

Also, I fixed a small bug of __qualname__ missing in EXCLUDED_ATTRIBUTES.

[AlexWaygood]

Also, I fixed a small bug of __qualname__ missing in EXCLUDED_ATTRIBUTES.

Could you put that in a separate PR, please? For the __class_getitem__ stuff, I'm happy to consider a change (though I need to think about it more), but it probably won't be backported if we accept it. It's too risky a change, and it's arguably more of a feature than a bug (everything's currently working as intended).

__qualname__ missing from EXCLUDED_ATTRIBUTES sounds like it might be a clear-cut bugfix that should be backported, however.

[randolf-scholz]

Oh, actually I think for the original it might be fine as is. The reason I needed to exclude __qualname__ is because I work with the namespace passed to _ProtocolMeta.__init__, which always contains __qualname__. The original code works with cls.__dict__, which for an empty class only consists of ['__module__', '__dict__', '__weakref__', '__doc__'].

__name__ is also missing from _SPECIAL_NAMES, since it's also not a standard part of cls.__dict__ I suppose.

[randolf-scholz]

The reason I consider the current behavior a bug is because the Protocol PEP doesn't seem to mention anything about excluded attributes. So any method and any annotated attribute in the Protocol body should be valid, imo.

But I agree that backporting might be potentially dangerous.

[AlexWaygood]

The reason I consider the current behavior a bug is because the Protocol PEP doesn't seem to mention anything about excluded attributes. So any method and any annotated attribute in the Protocol body should be valid, imo.

Understood, but this still feels too large a change to established behaviour for us to consider backporting it

[JelleZijlstra]

I'm not sure we should change this behavior. Runtime-checkable protocols should generally reflect the way type checkers treat protocols, and type checkers heavily special-case __class_getitem__.

[randolf-scholz]

To me, it seems like reducing the amount of special casing is a good thing. The Protocol-PEP doesn't mention that certain names are excluded.

[AlexWaygood]

I'm also not yet sure we should change this behaviour. __class_getitem__ is really an implementation detail of the stdlib; I feel like it's an antipattern for third-party code to define interfaces using __class_getitem__ in the way @randolf-scholz is trying to.

[randolf-scholz]

For what it's worth, PEP560 introduces __class_getitem__ as a core part of the language and even gives examples of user-defined classes with custom implementation of __class_getitem__. And there are quite a few people doing that: https://github.com/search?q=def+__class_getitem__+language%3APython&type=code&l=Python.