coady / multimethod

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

Error defining a multimethod using typing package #4

Closed corradomio closed 4 years ago

corradomio commented 4 years ago

Using this code

import typing from multimethod import multidispatch

@multidispatch def f(s: int): print("int", s)

@multidispatch def f(s: typing.List): print("list", s)

f(1) f([1, 2])

the library generates the error

TypeError: issubclass() arg 1 must be a class

The error is in the usage of "issubclass" in

class signature(tuple): def __le__(self, other): return len(self) <= len(other) and all(map(**issubclass**, other, self)) ...

because other contains the tuple "(typing.List,)".

Can be a good idea to extend the support to all types that it is possible to use with the "type annotation". For example, if it is used "typing.List[int]" it is possible to check ONLY the FIRST element of the list. If the list is empty, ANY method that support "typing.List" is good.

coady commented 4 years ago

Interesting. That's arguably a bug in Python, because it means a typing alias is allowed as the left arg in issubclass, but only if the right arg is also a typing alias. Leading to inconsistencies like:

In []: issubclass(typing.List, (typing.Sequence, int))                                                                                                                                                                                       
Out[]: True

In []: issubclass(typing.List, (int, typing.Sequence))                                                                                                                                                                                       
TypeError: issubclass() arg 1 must be a class

And functools.singledispatch is no help because it doesn't support typing aliases at all, but does support abstract base classes in collections.abc. I'm not sure if this should be supported, or just documented. Does using list or collections.abc.Sequence work for your use case?

corradomio commented 4 years ago

Ciao.Yes, I have used 'list' and it works.But the idea is to support any type defined in 'types' and 'typing' packages because they are used in the annotations and, for example, PyCharm is able to find type errors statically.I suppose it is necessary to reimplement from scratch the function 'issubclass' to support objects like 'Union[int, float, None]', 'List[int]' etc..Note: it is possible that the solution can be very simple: the package 'types' contains aliases for all 'standard' Pyton types (int ->IntegerType, ...). One idea can be to replace 'int' with 'IntegerType', etc ...To try ;-) -------- Original message --------From: "A. Coady" notifications@github.com Date: 9/28/19 23:38 (GMT+01:00) To: coady/multimethod multimethod@noreply.github.com Cc: "Corrado Mio (PhD)" corrado.mio@gmail.com, Author author@noreply.github.com Subject: Re: [coady/multimethod] Error defining a multimethod  using typing package (#4) Interesting. That's arguably a bug in Python, because it means a typing alias is allowed as the left arg in issubclass, but only if the right arg is also a typing alias. Leading to inconsistencies like: In []: issubclass(typing.List, (typing.Sequence, int))
Out[]: True

In []: issubclass(typing.List, (int, typing.Sequence))
TypeError: issubclass() arg 1 must be a class And functools.singledispatch is no help because it doesn't support typing aliases at all, but does support abstract base classes in collections.abc. I'm not sure if this should be supported, or just documented. Does using list or collections.abc.Sequence work for your use case?

—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread. [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/coady/multimethod/issues/4?email_source=notifications\u0026email_token=AFUJJIC7LVOKFOOIVGGL2I3QL7FGDA5CNFSM4I3NZ63KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD73C5EY#issuecomment-536227475", "url": "https://github.com/coady/multimethod/issues/4?email_source=notifications\u0026email_token=AFUJJIC7LVOKFOOIVGGL2I3QL7FGDA5CNFSM4I3NZ63KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD73C5EY#issuecomment-536227475", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

coady commented 4 years ago

I preferred relying on issubclass over a custom implementation, because it already has a hook for any types to participate. And Python's core is actively discouraging using typing for anything other than static analysis anyway:

Subscripted generics have even more problematic edge cases:

Finally, there's performance. issubclass is already behind a cache, but type is called on every arg for every call. I think a slow isinstance-like scan is a non-starter; there's already overload for that.

On the other hand, the current behavior does look like a bug. So 7ad471c implements provisional support for typing generics, but not subscripts yet.

coady commented 4 years ago

1166b7d implements full support for subscripted generics.

It adds a custom type (called subtype) to support issubclass with subscripts. Other than having to handle special cases like Union, it's a relatively straight-forward recursive checker.

The problem, as expected, is the expensive checking of the type at call time. Knowing which args need further checking depends on the type of the root arg. So get_type is a multimethod itself (how meta). It was slightly faster than a case statement implementation, and is more extensible.

Still feels like fighting the core language a bit to me, so I called it "provisional" for now. But I don't expect any issues from it; get_type is completely bypassed if subscripts aren't in use.

coady commented 4 years ago

v1.2