coady / multimethod

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

multimethod does not recognize correctly 'collections.abc.Container' types #82

Closed Dvergatal closed 1 year ago

Dvergatal commented 1 year ago

As in subject I'm having an issue that Container objects are not correctly verified just like in below example:

from typing import Any, List, Container
from multimethod import multimethod

@multimethod
def validate_enum_list(key: str, value: Any, allowed_values: Any):
    return NotImplemented

@validate_enum_list.register
def _(key: str, value: str, allowed_values: Container[str]) -> None:
    print("key: str, value: str, allowed_values: Container[str]")
    print(type(value))
    print(type(allowed_values))

@validate_enum_list.register
def _(key: str, value: Container[str], allowed_values: Container[Container[str]]) -> None:
    print("key: str, value: Container[str], allowed_values: Container[Container[str]]")
    print(type(value))
    print(type(allowed_values))

validate_enum_list("test1", "test1", ["test1"])
validate_enum_list("test2", ["test2"], [["test2"]])
validate_enum_list("test2", "test2", "test2")

According to this the third call of validate_enum_list should not be printed as it returns NotImplemented but instead the otuput looks like:

key: str, value: str, allowed_values: Container[str]
<class 'str'>
<class 'list'>
key: str, value: Container[str], allowed_values: Container[Container[str]]
<class 'list'>
<class 'list'>
key: str, value: str, allowed_values: Container[str]
<class 'str'>
<class 'str'>

I have verified that if I will add additional method to serve such case than it's all OK but I wanted to have it served with the NotImplemented pattern. Any better solution how to handle it or maybe there is a bug in multimethod code?

coady commented 1 year ago

It's actually working as intended; the catch is that strings are containers of strings.

>>> issubclass(str, Container)
True
>>> 't' in 'test'
True

You can try list[str] instead.

It's also worth mentioning that Container[] can't be fully supported, because there's no way to detect what it contains. Iterable[] or Collection[] checks the first element.

Dvergatal commented 1 year ago

Hi @coady, thx for your quick response, yes I know that i can use list[str] the problem is that I wanted to make this function more generic :disappointed:

coady commented 1 year ago

In that case, you want an abstract base class that is container-like but excludes strings. Something like

class MyIterable(abc.ABC):
    @classmethod
    def __subclasshook__(cls, subclass):
        return issubclass(subclass, Iterable) and not issubclass(subclass, str)

    def __class_getitem__(cls, key):
        return GenericAlias(cls, key)
Dvergatal commented 1 year ago

Ech this is really ugly.... Btw. I have verified one more thing that if I have changed the code to use List instead of Container like this:

from typing import Any, List, Container
from multimethod import multimethod

@multimethod
def validate_enum_list(key: str, value: str, allowed_values: List[str]) -> None:
    print("key: str, value: str, allowed_values: List[str]")
    print(type(value))
    print(type(allowed_values))

@validate_enum_list.register
def _(key: str, value: List[str], allowed_values: List[List[str]]) -> None:
    print("key: str, value: List[str], allowed_values: List[List[str]]")
    print(type(value))
    print(type(allowed_values))

validate_enum_list("test1", "test1", ["test1"])
validate_enum_list("test2", ["test2"], [["test2"]])
validate_enum_list("test2", "test2", "test2")

and I run mypy on it and it does not write result that the line validate_enum_list("test2", "test2", "test2") is wrong. Why is that so? The only error is during the run of code which is as expected to be.

Dvergatal commented 1 year ago

In that case, you want an abstract base class that is container-like but excludes strings. Something like

class MyIterable(abc.ABC):
    @classmethod
    def __subclasshook__(cls, subclass):
        return issubclass(subclass, Iterable) and not issubclass(subclass, str)

    def __class_getitem__(cls, key):
        return GenericAlias(cls, key)

Another thing is that if I use this class than mypy argues that error: "MyIterable" expects no type arguments, but 1 given

Dvergatal commented 1 year ago

In that case, you want an abstract base class that is container-like but excludes strings. Something like

class MyIterable(abc.ABC):
    @classmethod
    def __subclasshook__(cls, subclass):
        return issubclass(subclass, Iterable) and not issubclass(subclass, str)

    def __class_getitem__(cls, key):
        return GenericAlias(cls, key)

Another thing is that if I use this class than mypy argues that error: "MyIterable" expects no type arguments, but 1 given

@coady don't bother this error as it is only on --strict level of mypy and I have noticed on mypy that all ppl are writing it like that... Unluckily, I did some test with our CI and it uses --strict level so I have added typing and the code snippet looks like the last one in this comment.

But still I have a few more questions. First this example:

In that case, you want an abstract base class that is container-like but excludes strings. Something like

class MyIterable(abc.ABC):
    @classmethod
    def __subclasshook__(cls, subclass):
        return issubclass(subclass, Iterable) and not issubclass(subclass, str)

    def __class_getitem__(cls, key):
        return GenericAlias(cls, key)

Unfortunately it happens, that I would like to have in the body of the second method a call of sorted and it would look like that:

@validate_enum_list.register
def _(key: str, value: MyIterable[str], allowed_values: MyIterable[MyIterable[str]]) -> None:
    print("key: str, value: List[str], allowed_values: MyIterable[MyIterable[str]]")
    print(type(value))
    print(type(allowed_values))

    value = sorted(value)
    allowed_values = sorted([sorted(item) for item in allowed_values])

and on runtime it works, but mypy says an error like below:

multi2.py:29: error: No overload variant of "sorted" matches argument type "MyIterable[str]"  [call-overload]
multi2.py:29: note: Possible overload variants:
multi2.py:29: note:     def [SupportsRichComparisonT] sorted(Iterable[SupportsRichComparisonT], /, *, key: None = ..., reverse: bool = ...) -> List[SupportsRichComparisonT]
multi2.py:29: note:     def [_T] sorted(Iterable[_T], /, *, key: Callable[[_T], Union[SupportsDunderLT[Any], SupportsDunderGT[Any]]], reverse: bool = ...) -> List[_T]
multi2.py:30: error: Incompatible types in assignment (expression has type "List[List[Any]]", variable has type "MyIterable[MyIterable[str]]")  [assignment]
multi2.py:30: error: "MyIterable[MyIterable[str]]" has no attribute "__iter__" (not iterable)  [attr-defined]

and I have a question to you, because currently we have MyIterable class Container-like without str, shall i implement __iter__ in MyIterable or wouldn't it be better to inherit instead of abc over Iterable like that:

T = TypeVar("T")
class MyIterable(Iterable[T]):
    @classmethod
    def __subclasshook__(cls, subclass: type) -> bool:
        return not issubclass(subclass, str)

    def __class_getitem__(cls, key: Any) -> GenericAlias:
        return GenericAlias(cls, key)

Btw. the T = TypeVar("T") is due to mypy error: "MyIterable" expects no type arguments, but 1 given and than not implement __iter__ at all? Additionaly are these typings proper for __subclasshook__ and __class_getitem__? I do not know what type a key variable is...

Dvergatal commented 1 year ago

@coady correct my if my understand is wrong, but is __class_getitem__ supposed to handle type hinting issues? Because according to this __class_getitem__() is supposed to handle type hinting in such cases, but It is also mentioned that:

Custom implementations of __class_getitem__() on classes defined outside of the standard library may not be understood by third-party type-checkers such as mypy