coady / multimethod

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

Regression in 1.10 regarding typing.Literal #100

Closed adam-urbanczyk closed 11 months ago

adam-urbanczyk commented 1 year ago

See https://github.com/CadQuery/cadquery/issues/1431 , MRE will follow

adam-urbanczyk commented 1 year ago

I cannot reproduce the reported issue, but updating to 1.10 does cause a regression for cq. Did something change in the dispatch code?

I'm still busy distilling some kind of MRE, but the regression happens here:

-> func = self[tuple(func(arg) for func, arg in zip(self.type_checkers, args))]
(Pdb) d
> d:\mambaforge\envs\cq3\lib\site-packages\multimethod\__init__.py(326)<genexpr>()
-> func = self[tuple(func(arg) for func, arg in zip(self.type_checkers, args))]
(Pdb) d
> d:\mambaforge\envs\cq3\lib\site-packages\multimethod\__init__.py(120)get_type()
-> if any(arg == param and type(arg) is type(param) for param in self.__args__):
(Pdb) d
> d:\mambaforge\envs\cq3\lib\site-packages\multimethod\__init__.py(120)<genexpr>()
-> if any(arg == param and type(arg) is type(param) for param in self.__args__):

even though arg and param are of different type, the == operator is called.

adam-urbanczyk commented 1 year ago

Here is the example:

from typing import List, Literal, Union
from multimethod import multimethod

class Opt:

    def __init__(self, val):

        self.val = val

    def __eq__(self, other):

        return self.val == other.val

class W:

    pass

class E:

    pass

class F:

    pass

class S:

    @multimethod
    def sweep(
        cls,
        a: W,
        b: List[W],
        c: Union[W, E],
        d: bool = True,
        e: bool = False,
        f: Union[Opt, W, E, None] = None,
        g: Literal["transformed", "round", "right"] = "transformed",
    ):

        pass

    @classmethod
    @sweep.register
    def sweep(
        cls,
        a: F,
        b: Union[W, E],
        c: bool = True,
        d: bool = False,
        e: Union[Opt, W, E, None] = None,
        f: Literal["transformed", "round", "right"] = "transformed",
    ):

        pass

S.sweep(W(), [], W(), True, True, Opt(1))

results in

AttributeError: 'str' object has no attribute 'val'
adam-urbanczyk commented 1 year ago

@coady the following change seems to solve the issue above

--- a/multimethod/__init__.py
+++ b/multimethod/__init__.py
@@ -134,7 +134,7 @@ class subtype(abc.ABCMeta):
         if self.__origin__ is type:  # a class argument is expected
             return arg
         if self.__origin__ is Literal:
-            if any(arg == param and type(arg) is type(param) for param in self.__args__):
+            if any(arg == param for param in self.__args__ if type(arg) is type(param)):
                 return subtype(Literal, arg)
             return type(arg)
         if self.__origin__ is Union:  # find the most specific match
coady commented 1 year ago

I'm ok with this change because it's harmless, but I think there are more root causes that could be fixed.

  1. Opt.__eq__ raises an error when compared to an object of a different type. That's unusual in Python for ==. It could easily add return isinstance(other, Opt) and ... or return hasattr(other, 'val') and ....
  2. sweep is dispatching on a different number of arguments, and appear shifted starting from the second parameter. So Opt(1) is being compared against f: Union[Opt... not g: Literal["... in one case, but f: Literal["... not e: Union[Opt... in the other. 5 of the arguments aren't of the "wrong" type so much as they're called on the "wrong" parameter. The example will inherently cause confusion.
coady commented 11 months ago

ae4f9a0 refactored generic dispatching, and includes a strict type check for literals.

adam-urbanczyk commented 11 months ago

Thank you!