coady / multimethod

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

Version 1.11 does not allow concrete protocol subclasses #109

Closed vnmabus closed 7 months ago

vnmabus commented 7 months ago

I have several multimethods which I use with concrete (non-protocol) classes subclassing a particular Protocol. For example:

import multimethod
from typing import Any, Protocol

class Kk(Protocol):
    def kk(self) -> str:
        pass

class ConcreteKk(Kk):
    def kk(self) -> str:
        return "kk"

@multimethod.multidispatch
def method(a: Any):
    return a.kk()

@method.register
def method2(a: ConcreteKk):
   return "concrete"

method(ConcreteKk())

This used to return 'concrete' in version 1.10.

However, in the most recent version (1.11) this produces the following exception instead:

TypeError                                 Traceback (most recent call last)
Cell In[1], line 20
     16 @method.register
     17 def method2(a: ConcreteKk):
     18    return "concrete"
---> 20 method(ConcreteKk())

File .../lib/python3.11/site-packages/multimethod/__init__.py:414, in multidispatch.__call__(self, *args, **kwargs)
    412 """Resolve and dispatch to best method."""
    413 params = self.signature.bind(*args, **kwargs).args if (kwargs and self.signature) else args
--> 414 func = self.dispatch(*params)
    415 return func(*args, **kwargs)

File .../lib/python3.11/site-packages/multimethod/__init__.py:352, in multimethod.dispatch(self, *args)
    350 def dispatch(self, *args) -> Callable:
    351     types = tuple(map(type, args))
--> 352     if not any(map(issubclass, types, self.generics)):
    353         return self[types]
    354     matches = {key for key in list(self) if isinstance(key, signature) and key.instances(*args)}

File <frozen abc>:123, in __subclasscheck__(cls, subclass)

File .../lib/python3.11/typing.py:2061, in Protocol.__init_subclass__.<locals>._proto_hook(other)
   2059     if _allow_reckless_class_checks():
   2060         return NotImplemented
-> 2061     raise TypeError("Instance and class checks can only be used with"
   2062                     " @runtime_checkable protocols")
   2063 if not _is_callable_members_only(cls):
   2064     if _allow_reckless_class_checks():

TypeError: Instance and class checks can only be used with @runtime_checkable protocols
coady commented 7 months ago

It's trying to do an instance check because of the Protocol subclass. While I work on a fix, a workaround would be to use @ runtime_checkable.

Thanks for the report.

vnmabus commented 7 months ago

It's trying to do an instance check because of the Protocol subclass. While I work on a fix, a workaround would be to use @ runtime_checkable.

Note that the superclass may come from a different package, and in that case it would not be possible to use runtime_checkable.

vnmabus commented 7 months ago

Also, it would be awesome if there were a way to annotate method with a non-runtime-checkable protocol Kk in the example instead of Any, for typing purposes.

coady commented 7 months ago

Fixed in main now.

Also, it would be awesome if there were a way to annotate method with a non-runtime-checkable protocol Kk in the example instead of Any, for typing purposes.

I don't think that's possible, in that a type either supports issubclass or it doesn't. runtime_checkable could also be called explicitly (though it modifies the type).

@multimethod.multidispatch
def method(a: runtime_checkable(Kk)):
    return a.kk()
vnmabus commented 7 months ago

I don't think that's possible, in that a type either supports issubclass or it doesn't. runtime_checkable could also be called explicitly (though it modifies the type).

But as multidispatch is the base case, it shouldn't be necessary to check its types with isinstance, should it?

coady commented 7 months ago

But as multidispatch is the base case, it shouldn't be necessary to check its types with isinstance, should it?

Interesting. singledispatch originally didn't support annotations at all, so of course the base implementation wasn't "registered". Even now the example shows the base unannotated (and only later clarifies that it behaves as object would).

I wonder if this is a case of an implementation detail becoming a feature. Anyway, multidispatch is supposed to be consistent, but I think the current dispatch behavior will have to be deprecated first.

coady commented 7 months ago

In v1.11.1. Thanks for the report.