Sorry, something went wrong.
There was a problem hiding this comment.
Looks good! Is it important that the "issubclass() arg 1 must be a class" error message takes precedence over the others? It's not obvious to me that it should, but I'm not very familiar with this code.
Sorry, something went wrong.
|
Thanks for the review! :D Looks good! Is it important that the "issubclass() arg 1 must be a class" error message takes precedence over the others? It's not obvious to me that it should, but I'm not very familiar with this code. Yeah, so to clarify, with the type checks, this is the behaviour we get with this PR branch: Python 3.13.0a2+ (heads/main:9560e0d6d7, Dec 4 2023, 13:50:58) [MSC v.1932 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import typing as t
>>> @t.runtime_checkable
... class Foo(t.Protocol):
... X = 1
...
>>> issubclass("not a class", Foo)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
issubclass("not a class", Foo)
~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
File "C:\Users\alexw\coding\fast-cpython\Lib\typing.py", line 1842, in __subclasscheck__
_type_check_subclasscheck_second_arg(other)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^
File "C:\Users\alexw\coding\fast-cpython\Lib\typing.py", line 1796, in _type_check_subclasscheck_second_arg
raise TypeError('issubclass() arg 1 must be a class')
TypeError: issubclass() arg 1 must be a class
But without the type checks, we'd get this behaviour instead: >>> import typing as t
>>> @t.runtime_checkable
... class Foo(t.Protocol):
... X = 1
...
>>> issubclass("not a class", Foo)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
issubclass("not a class", Foo)
~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
File "C:\Users\alexw\coding\fast-cpython\Lib\typing.py", line 1847, in __subclasscheck__
raise TypeError(
...<2 lines>...
)
TypeError: Protocols with non-method members don't support issubclass(). Non-method members: 'X'.
It wouldn't be the worst thing in the world if we had that error message instead. But the fact that Foo has non-method members is clearly not the most significant problem with this person's code! So I definitely prefer to prioritise the "issubclass() arg 1 must be a class" message, if possible :) |
Sorry, something went wrong.
|
I added a docstring in 3c7eb19 -- does that make things a bit clearer? :) |
Sorry, something went wrong.
|
It does indeed! Thank you. |
Sorry, something went wrong.
This speeds up issubclass(int, typing.SupportsIndex) by around 6%.
The approach is to move the code around a bit so that we delegate the type-checking to type.__subclasscheck__ wherever possible; type.__subclasscheck__ can do it faster. We now only check from _ProtocolMeta.__subclasscheck__ whether other is an instance of type if we know we're about to raise an exception anyway, and we want the issubclass() arg 2 must be a type message to take priority over the other error message we could potentially give the user.