coady / multimethod

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

Version 1.11 breaks dispatch on Union objects with custom metaclass #110

Closed ieaves closed 7 months ago

ieaves commented 7 months ago

I'm still investigating where the regression was introduced but I wanted to bring it to your attention first.

We heavily utilize multimethod in conjunction with pydantic to dispatch on various data objects. Take the following

from multimethod import multimethod
from typing import Any, Union

from pydantic import BaseModel

class Thing(BaseModel):
    pass

class Thing2(BaseModel):
    pass

Things = Union[Thing, Thing2]

@multimethod
def foo(thing: Any):
    print('generic')

@foo.register
def _(thing: Things):
    print("thing or thing2")

Prior to 1.11.0 this script would run without error. However, since 1.11.0 this now errors with

Traceback (most recent call last):
  File "/Users/ian/repos/grai/grai-core/grai-schemas/example.py", line 23, in <module>
    @foo.register
     ^^^^^^^^^^^^
  File "/Users/ian/Library/Caches/pypoetry/virtualenvs/grai-schemas-jCF57o3v-py3.12/lib/python3.12/site-packages/multimethod/__init__.py", line 278, in register
    multimethod.__init__(self, *args)
  File "/Users/ian/Library/Caches/pypoetry/virtualenvs/grai-schemas-jCF57o3v-py3.12/lib/python3.12/site-packages/multimethod/__init__.py", line 262, in __init__
    self[signature.from_hints(func)] = func
         ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ian/Library/Caches/pypoetry/virtualenvs/grai-schemas-jCF57o3v-py3.12/lib/python3.12/site-packages/multimethod/__init__.py", line 212, in from_hints
    return cls(hints, required)
           ^^^^^^^^^^^^^^^^^^^^
  File "/Users/ian/Library/Caches/pypoetry/virtualenvs/grai-schemas-jCF57o3v-py3.12/lib/python3.12/site-packages/multimethod/__init__.py", line 193, in __new__
    return tuple.__new__(cls, map(subtype, types))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ian/Library/Caches/pypoetry/virtualenvs/grai-schemas-jCF57o3v-py3.12/lib/python3.12/site-packages/multimethod/__init__.py", line 68, in __new__
    return type.__new__(cls, str(tp), bases, namespace)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

The issue specifically arises when the Union contains two objects inheriting from BaseModel. It does not trigger for something like Union[Thing, None] or Union[Thing, Thing3] where Thing3's parent class is just object.

ieaves commented 7 months ago

It looks like the offending change occurred here. In previous versions of multimethod bases would be left as an empty tuple for Union's. Now, when every subtype of the Union inherits from the same base class multimethod identifies their shared base class and attempts to instantiate a new type from the same base. Unfortunately, as far as I can tell, this will always fail when the shared base class inherits from a parent with a custom Meta, e.g.

class Meta(type):
    def __new__(cls, name, bases, dct):
        x = super().__new__(cls, name, bases, dct)
        return x

class Thing(metaclass=Meta):
    pass

class Thing2(Thing):
    pass

class Thing3(Thing):
    pass

Things = Union[Thing2, Thing3]

I'm not confident enough with the rest of the code base to know whether this would create undesirable side effects but one solution would be to remove bases which contain custom metaclasses. That could look something like

        if origin is Union:
            def has_custom_metaclass(cls):
                for base in cls.__mro__:
                    if base.__class__ is not type:
                        return True
                return False

            counts = collections.Counter(cls for arg in args for cls in get_mro(arg))
            bases = tuple(cls for cls in counts if counts[cls] == len(args) and not has_custom_metaclass(cls))[:1]
ieaves commented 7 months ago

Thanks for taking a look at this @coady. I can confirm the fix in that commit will resolve the issue in all cases where we use multimethod. Thanks for your work on this amazing package.

coady commented 7 months ago

In v1.11.1. Thanks for the report.