coady / multimethod

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

Support limited return type static analysis #37

Closed JacobHayes closed 3 years ago

JacobHayes commented 3 years ago

Currently, the mypy return type of a multimethod call is Any, but a stricter return is possible by making multimethod a Generic. This will take the return of the first registered function. This should be fine as long as the initial function has -> Any (implicit default with no return annotation) or uses a base type (eg: object or another base class) matching subsequent function returns ("covariant" if I understand the terms correctly). Using the test suite as a baseline, these seem like "safe" assumptions.

For example, running mypy on this module:

from typing import Any, Union

from multimethod import multidispatch

@multidispatch
def base(x: object) -> object:
    return object()

@base.register
def integer(x: int) -> int:
    return int()

@base.register
def string(x: Union[str, bytes]) -> str:
    return str()

reveal_type(base)
reveal_type(base.__call__)
reveal_type(base())
reveal_type(integer)
reveal_type(integer())
reveal_type(integer(5))
reveal_type(string)
reveal_type(string())
reveal_type(string("a"))

before and after this PR shows:

$ mypy example.py # without PR
example.py:21: note: Revealed type is 'multimethod.multidispatch'
example.py:22: note: Revealed type is 'def (*args: Any, **kwargs: Any) -> Any'
example.py:23: note: Revealed type is 'Any'
example.py:24: note: Revealed type is 'def (*Any, **Any) -> Any'
example.py:25: note: Revealed type is 'Any'
example.py:26: note: Revealed type is 'Any'
example.py:27: note: Revealed type is 'def (*Any, **Any) -> Any'
example.py:28: note: Revealed type is 'Any'
example.py:29: note: Revealed type is 'Any'

$ mypy example.py # with PR
example.py:21: note: Revealed type is 'multimethod.multidispatch[builtins.object*]'
example.py:22: note: Revealed type is 'def (*args: Any, **kwargs: Any) -> builtins.object*'
example.py:23: note: Revealed type is 'builtins.object*'
example.py:24: note: Revealed type is 'def (x: builtins.int) -> builtins.int'
example.py:25: error: Missing positional argument "x" in call to "integer"
example.py:25: note: Revealed type is 'builtins.int'
example.py:26: note: Revealed type is 'builtins.int'
example.py:27: note: Revealed type is 'def (x: Union[builtins.str, builtins.bytes]) -> builtins.str'
example.py:28: error: Missing positional argument "x" in call to "string"
example.py:28: note: Revealed type is 'builtins.str'
example.py:29: note: Revealed type is 'builtins.str'
Found 2 errors in 1 file (checked 1 source file)

.

The registered funcs look great, but it's unfortunate the mypy signature for the multimethod is *args, **kwargs (and hence, should but does not error on line 23). I don't think it's possible to parametrize the input arguments in any sane way (mypy forces them to be contravariant).

This PR shouldn't break any runtime usage, but may break users' mypy if the initial function has a return type and it isn't covariant/base class of the other functions. In these cases, the function definitions themselves will be ok, but the type hint for the multimethod return value will now be that first function's return, rather than Any, so mypy will scrutinize a bit further. I'm not sure how common that case is, but suggesting the initial func be explicitly annotated with -> Any may be an easy work around.

codecov[bot] commented 3 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@23589d3). Click here to learn what that means. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #37   +/-   ##
=======================================
  Coverage        ?   99.12%           
=======================================
  Files           ?        1           
  Lines           ?      229           
  Branches        ?       37           
=======================================
  Hits            ?      227           
  Misses          ?        2           
  Partials        ?        0           
Impacted Files Coverage Δ
multimethod/__init__.py 99.12% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 23589d3...b7588e3. Read the comment docs.

coady commented 3 years ago

Thanks, looks good. Regarding potential mypy breakage, what do you think of adding this to only multidispatch? Since that requires a initial definition.

JacobHayes commented 3 years ago

Sounds good, just pushed that - I kept the @X.register improvement in multimethod since that maintains the original signature (as the func is returned unchanged).

After re-reading the @multimethod docs, I realized it is already somewhat incompatible with mypy since mypy doesn't support redefinition. @multidispatch only for the RETURN is definitely the safer choice, but I wonder how much it would affect @multimethod. I think it'd only be users w/ # type: ignore comments (to avoid the redefinition error) but the added RETURN would change __call__. Probably more food for thought... there's definitely some trickiness statically typing a lib made to expand dynamic typing. 😁