python-attrs / cattrs

Composable custom class converters for attrs, dataclasses and friends.
https://catt.rs
MIT License
779 stars 108 forks source link

Incorrect dispatch in Python 3.12 #525

Closed AdrianSosic closed 3 months ago

AdrianSosic commented 3 months ago

Description

Hi, I think the following bug is not caused by cattrs but rather has something to do with how Protocols are handled in Python 3.12, but I stumbled upon it while working with cattrs and I'm not exactly sure what is the root cause. Perhaps @Tinche has an idea?

What I Did

Consider the following code, which works as intended before 3.12 but incorrectly dispatches to the defined MyProtocol in 3.12. I think it is caused by the fact that the issubclass check with Protocols is handled different across these Python versions.

from typing import Protocol

from attrs import define, field
from cattrs import Converter

class MyProtocol(Protocol):
    pass

converter = Converter()
converter.register_structure_hook(MyProtocol, lambda *x: "Incorrect dispatch")

@define
class MyClass:
    x: int = field()

# Before 3.12, this works correctly.
# In 3.12, there is an incorrect dispatch to `MyProtocol`
print(converter.structure({"x": 0}, MyClass))

# The problem seems related to the fact that the following gives `False` before 3.12 but
# raises an (expected) error in 3.12 because the protocol is not declared as 
# `@runtime_checkable`
print(issubclass(MyClass, MyProtocol))

Interestingly enough, when placing a breakpoint at the is_related function in _compose_mro of the functools module, the issubclass call there simply evaluates to True in 3.12 (instead of throwing the exception), which causes the wrong dispatch.

Any thoughts?

Tinche commented 3 months ago

I think this is the essence of the problem:

from functools import singledispatch
from typing import Protocol

from attrs import define

class MyProtocol(Protocol):
    pass

@define
class MyClass:
    x: int

s = singledispatch(lambda x: None)
s.register(MyProtocol, lambda x: 1)

print(s(MyClass(1)))  # '1' on 3.12, 'None' otherwise

Looks like the behavior of singledispatch was changed :/

AdrianSosic commented 3 months ago

Yes, exactly. What do you suggest? I think this is a bug, right? Because MyClass is treated as a subclass of MyProtocol, which should not be the case. Do we need to raise a python ticket in this case, or what is the best approach?

Tinche commented 3 months ago

It's a difficult question. I would open an issue over there and see what they think.

As for cattrs, you might want to switch to register_structure_hook_func instead - that uses a predicate function instead of singledispatch.

I'm trying to keep the number of open issues down so I'm going to close this. We lean on singledispatch a lot (by definition) so there's nothing we can do here I think. Let me know if you have any other feedback!

AdrianSosic commented 3 months ago

Thanks for your help. As you suggested, I've opened the ticket 👍🏼 Let's see what we get ...