python / cpython

The Python programming language
https://www.python.org/
Other
60.91k stars 29.41k forks source link

Changed behavior of singledispatch in Python 3.12 #117097

Open AdrianSosic opened 3 months ago

AdrianSosic commented 3 months ago

Bug report

Bug description:

It seems the behavior of functools.singledispatch changed in Python 3.12 in situations where a typing.Protocol is involved.

When executing the following code on 3.12, MyClass no longer gets dispatched as via the default but as MyProtocol. The problem seems to be that issubclass now behaves differently when called on protocols. The issue was noticed here where it caused downstream problems.

from typing import Protocol
from functools import singledispatch

class MyProtocol(Protocol):
    pass

class MyClass:
    pass

s = singledispatch(lambda x: "default")
s.register(MyProtocol, lambda x: "protocol")

print(s(MyClass))  # yields "default" on 3.11 but "protocol" on 3.12

CPython versions tested on:

3.11, 3.12

Operating systems tested on:

macOS

sobolevn commented 3 months ago

cc @AlexWaygood

AlexWaygood commented 3 months ago

Okay, I strongly suspect this is because of this delightful part of typing.py:

https://github.com/python/cpython/blob/8ad88984200b2ccddc0a08229dd2f4c14d1a71fc/Lib/typing.py#L1784-L1790

I'll investigate more tomorrow.

AlexWaygood commented 3 months ago

Thanks for the clear bug report, @AdrianSosic!

AdrianSosic commented 3 months ago

Hi @AlexWaygood, just wanted to ask if you could already find some time to figure out what exactly causes the problem? 🙃

AlexWaygood commented 3 months ago

Sorry, I haven't had a chance to look at this properly yet. But thanks for the ping. Please feel free to ping me again if I haven't responded in ~2 weeks :-)

AdrianSosic commented 2 months ago

@AlexWaygood, as requested: ping ping 😅 don't want to annoy you so feel free to tell me you have no time for it 😊

AlexWaygood commented 2 months ago

Thanks, you're not annoying me!

AdrianSosic commented 2 months ago

Just lurking around a little bit … (disappears into nowhere)

AlexWaygood commented 2 months ago

If I haven't got to it by then, I'll definitely look at this at the PyCon Sprints (week commencing 20th May)

AlexWaygood commented 1 month ago

At least two commits changed the behaviour of this snippet between Python 3.11 and Python 3.12:

In between those two commits, the traceback was as follows:

```pytb Traceback (most recent call last): File "/Users/alexw/dev/cpython/Lib/functools.py", line 832, in dispatch impl = dispatch_cache[cls] ~~~~~~~~~~~~~~^^^^^ File "/Users/alexw/dev/cpython/Lib/weakref.py", line 415, in __getitem__ return self.data[ref(key)] ~~~~~~~~~^^^^^^^^^^ KeyError: During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/Users/alexw/dev/cpython/Lib/functools.py", line 835, in dispatch impl = registry[cls] ~~~~~~~~^^^^^ KeyError: During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/Users/alexw/dev/cpython/foo.py", line 14, in print(s(MyClass)) ^^^^^^^^^^ File "/Users/alexw/dev/cpython/Lib/functools.py", line 909, in wrapper return dispatch(args[0].__class__)(*args, **kw) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/alexw/dev/cpython/Lib/functools.py", line 837, in dispatch impl = _find_impl(cls, registry) ^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/alexw/dev/cpython/Lib/functools.py", line 784, in _find_impl mro = _compose_mro(cls, registry.keys()) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/alexw/dev/cpython/Lib/functools.py", line 745, in _compose_mro types = [n for n in types if is_related(n)] ^^^^^^^^^^^^^ File "/Users/alexw/dev/cpython/Lib/functools.py", line 744, in is_related and issubclass(cls, typ)) ^^^^^^^^^^^^^^^^^^^^ File "/Users/alexw/dev/cpython/Lib/typing.py", line 1799, in __subclasscheck__ return super().__subclasscheck__(other) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "", line 123, in __subclasscheck__ File "/Users/alexw/dev/cpython/Lib/typing.py", line 1882, in _proto_hook raise TypeError("Instance and class checks can only be used with" TypeError: Instance and class checks can only be used with @runtime_checkable protocols ```
AlexWaygood commented 1 month ago

My feeling is that the new behaviour is probably correct. Calling isinstance() and issubclass() against a protocol that isn't decorated with @runtime_checkable isn't usually allowed, but we make an explicit exception for the functools module. But if you create an empty protocol -- as you're doing here -- and decorate it with @runtime_checkable, then you'll see that all objects are considered instances of that protocol. This has been the case since runtime-checkable protocols were originally introduced in Python 3.8:

Python 3.8.18 (default, Feb 15 2024, 19:36:58) 
[Clang 15.0.0 (clang-1500.1.0.2.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from typing import Protocol, runtime_checkable
>>> @runtime_checkable
... class EmptyProtocol(Protocol): pass
... 
>>> isinstance([], EmptyProtocol)
True
>>> isinstance(object(), EmptyProtocol)
True
>>> issubclass(dict, EmptyProtocol)
True
>>> issubclass(object, EmptyProtocol)
True

As such, it makes sense that s.register(MyProtocol, lambda x: "protocol") would lead to lambda x: "protocol" being selected as the relevant function variant for any arbitrary object being passed in. Non-empty protocols also appear to be behaving correctly:

>>> from functools import singledispatch
>>> s = singledispatch(lambda x: "default")
>>> from typing import SupportsIndex
>>> _ = s.register(SupportsIndex, lambda x: "supports index")
>>> s('foo')
'default'
>>> s({})
'default'
>>> s(432)
'supports index'

So, I think the only thing to do here is to add a regression test, since it appears that there was a bug on Python <=3.11 that was accidentally fixed. @carljm / @JelleZijlstra, do you agree?

JelleZijlstra commented 1 month ago

I agree that everything should match an empty protocol and therefore the new behavior is correct.

However, it seems wrong that singledispatch allows dispatching on a non-runtime checkable protocol. I tried locally and test_functools and test_typing both pass if I remove functools from _allow_reckless_class_checks. I wonder if we can remove it.

AlexWaygood commented 1 month ago

However, it seems wrong that singledispatch allows dispatching on a non-runtime checkable protocol. I tried locally and test_functools and test_typing both pass if I remove functools from _allow_reckless_class_checks. I wonder if we can remove it.

I agree that this does seem wrong. However, changing it will break users like @AdrianSosic who have been relying on the existing behaviour where it was allowed. I don't think we can change this without a deprecation period.