coady / multimethod

Multiple argument dispatching.
https://coady.github.io/multimethod
Other
284 stars 23 forks source link

multimethod doesn't work nicely with Union types #121

Closed aaronhendry closed 4 months ago

aaronhendry commented 5 months ago

There appear to be a few issues with using Union types, or with the Python 3.12 aliased types, with the multimethod decorator. Consider the following:

import collections.abc
from multimethod import multimethod

type compound = collections.abc.Iterable | collections.abc.Sequence

@multimethod
def fun(a: int):
    print(a)

@multimethod
def fun(a: str):
    print(a)

@multimethod
def fun(a: compound):
    print(a)

fun(1)
fun("test")
fun([1, 2, 3])
fun({1, 2, 3})

When running this, it crashes im multimethod/__init__.py on line 221 with the error "TypeError: issubclass() arg 1 must be a class". I believe this is because the constructor for the subtype class does not recognise the new aliased types as a type of Union. This can be solved by injecting the lines

if (isinstance(tp, typing.TypeAliasType) or isinstance(tp, typing.GenericAlias)) and hasattr(tp, '__value__'):
    tp = tp.__value__

into the subtype.__new__ function around line 48. This checks if the type is one of the new alias types, and extracts the internal Union from it if it is.

Following this, the code crashes with an infinite recursion error in the __subclasscheck__ function of the subtype class. I believe this is because we're not properly iterating over the individual types of the Union. This can be fixed by changing the offending line: https://github.com/coady/multimethod/blob/2372ea6ffb14a7ac294972d4f3b9692209be6321/multimethod/__init__.py#L93 to

return any(issubclass(cls, subclass) for cls in self.__args__)

which properly iterates over the members of the Union, and prevents the infinite recursion.

After this, the code crashes with the error "multimethod.DispatchError: ('fun: 0 methods found', (<class 'list'>,), set())" when trying to match the fun([1,2,3]) call. I believe that this is similar to the last one, in that the check carried out by signature.__le__ is not properly passing the types to __subclasscheck__ when the the type is a Union. I'm not quite so sure about the fix for this one, but I was able to get it working by changing the contents of signature.__le__ to:

        return (self.required <= len(other) and 
                all([issubclass(sc[0], sc[1].__args__ if hasattr(sc[1], '__args__') else sc[1]) 
                     for sc in zip(other, self)]))

This was enough, I think, to get the code working for my own purposes, but I have not done further testing to check if there are more edge cases. I also do not have enough of an understanding of the multimethod package to know if what I have done is correct, and doesn't mess anything else up (I only found the package today, and spent relatively little time trying to fix the errors). This is why I have not bothered with a pull-request. I can do so if you would like, but it won't be much more than what I have detailed above, as I do not have enough time to properly test the code.

gregbrowndev commented 5 months ago

This also occurs if you have the union as the top-level type that you are overloading.

Note: I'm using Pydantic's discriminated unions to define TaskEvent:

TaskEvent = Annotated[
    StartedEvent | CompletedEvent | FailedEvent,
    Field(discriminator="task_type"),
]

@multimethod
def handle_event(self, event: TaskEvent) -> None: 
    ...

@handle_event.register
def _(self, event: StartedEvent) -> None:
    print("Started")

@handle_event.register
def _(self, event: CompletedEvent) -> None:
    print("Completed")

@handle_event.register
def _(self, event: FailedEvent) -> None:
    print("Failed")

Error:

>           raise TypeError("Cannot inherit from plain Generic")
E           TypeError: Cannot inherit from plain Generic
coady commented 5 months ago

Thanks for the report. There are a few issues here.

There appear to be a few issues with using Union types, or with the Python 3.12 aliased types, with the multimethod decorator. Consider the following: ...

if (isinstance(tp, typing.TypeAliasType) or isinstance(tp, typing.GenericAlias)) and hasattr(tp, '__value__'):
    tp = tp.__value__

Yes, type aliases can definitely be supported.

Following this, the code crashes with an infinite recursion error in the __subclasscheck__ function of the subtype class. I believe this is because we're not properly iterating over the individual types of the Union. This can be fixed by changing the offending line:

https://github.com/coady/multimethod/blob/2372ea6ffb14a7ac294972d4f3b9692209be6321/multimethod/__init__.py#L93

to

return any(issubclass(cls, subclass) for cls in self.__args__)

which properly iterates over the members of the Union, and prevents the infinite recursion.

The equivalent loop would be:

return any(issubclass(subclass, cls) for cls in self.__args__)

which still results in infinite recursion. Which is a long-standing issue in (C)Python: python/cpython#73407.

The broader issue is how to support type-like objects that don't support issubclass. UnionType claims to, but only works as the right arg to issubclass.

Another thing to note, the example doesn't need a union.

type compound = collections.abc.Iterable | collections.abc.Sequence

is equivalent to collections.abc.Iterable. Switching to collections.abc.Set | collections.abc.Sequence does work.

coady commented 4 months ago

If you can try out main, it supports type aliases, annotated aliases, and redundant unions. The original example works.

aaronhendry commented 4 months ago

As far as I can tell, that works with my code now. Thanks for sorting this out so quickly!