Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add plum.overload #93

Merged
merged 20 commits into from
Aug 19, 2023
Merged

Add plum.overload #93

merged 20 commits into from
Aug 19, 2023

Conversation

wesselb
Copy link
Member

@wesselb wesselb commented Aug 13, 2023

@gabrieldemarmiesse, @machow, and @wch, I've taken a stab at adding plum.overload as described in #89. With this PR, the following would be functional and mypy-compliant:

from plum.overload import dispatch, overload


@overload
def f(x: int) -> int:
    return x


@overload
def f(x: str) -> str:
    return x


@dispatch
def f(x):
    pass

How would something like this look?

@coveralls
Copy link

coveralls commented Aug 13, 2023

Pull Request Test Coverage Report for Build 5912664992

  • 13 of 13 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 5912658569: 0.0%
Covered Lines: 935
Relevant Lines: 935

💛 - Coveralls

@wch
Copy link
Contributor

wch commented Aug 14, 2023

I have two suggestions:

  • make the typing-extensions dependency a hard dependency (not in dev), but conditional on Python version.
  • make the typing_extensions import conditional on Python version.

For the first one, the pyproject.toml would have something like this:

dependencies = [
    ...,
    "typing-extensions; python_version<='3.10'"
]

(Note that I've done this kind of thing with setup.cfg, but I'm not 100% sure this is the correct syntax for pyproject.toml.)

If you do this, then the try wrapping the import won't be necessary.

Next, to make the typing_extensions import conditional on Python version, the code should look like this:

import sys

if sys.version_info >= (3, 11):
    from typing import get_overloads, overload
else:
    from typing_extensions import get_overloads, overload

With this change, Python 3.11+ won't require typing-extensions. Also, as time goes on and Python 3.10 reaches end of life, then it will be clear in the code that you can you remove the if sys.version_info >= (3, 11) and always do from typing import ....


One more thing that would be helpful for pyright users (like me and my team) is to add pyright tests in addition to mypy.

plum/overload.py Outdated
T = TypeVar("T", bound=Callable)


def dispatch(f: T) -> T:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def dispatch(f: T) -> T:
def dispatch(f: Callable) -> Function:

Maybe mypy didn't catch this? I'm unsure

Copy link
Member Author

@wesselb wesselb Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this is technically correct, I believe this might not have the intended effect. Specifically, I tried changing the type stub to the following:

from typing import Callable, TypeVar, overload

from plum import Function

__all__ = ["overload", "dispatch"]

T = TypeVar("T", bound=Callable)

def dispatch(f: Callable) -> Function: ...

Then mypy is happy with the following:

from plum.overload import dispatch, overload


@overload
def f(x: int) -> int:
    return x


@overload
def f(x: str) -> str:
    return x


@dispatch
def f(x):
    pass


f(1)
f("1")
f(1.0)  # Should complain about this!!

Note that this is wrong, because it should complain about f(1.0).

I believe that's what happening is the following. If you say that dispatch takes in Callable and gives back a Function, then f(1.0) will be matched against Function.__call__. Function.__call__ takes in any arguments, so f(1.0) checks out.

However, if you say that dispatch takes in T and gives back T, then mypy knows that what f decorates by dispatch must correspond to the implementation of f in the file. The @overloads in the file correspond to the implementation of f in the file, so mypy knows that f(1.0) is wrong and then correctly complains.

Copy link
Member Author

@wesselb wesselb Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hold on, I screwed up the test on my part! mypy actually does correctly complain about f(1.0) with the corrected type stub. I'm now a bit confused, because I don't exactly understand what's going on.

EDIT: I've committed the suggestion, because I think you're right, but I'm still not quite sure what's going on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now added tests for both mypy and pyright. It seems that pyright does indeed require the T -> T construction. If you'd like to experiment with it, you can run the linter assertion tests with

python check_linter_assertions.py tests/typechecked

from the repo root.

plum/overload.pyi Outdated Show resolved Hide resolved
plum/overload.py Outdated Show resolved Hide resolved
Comment on lines 32 to 33
In the above, `plum.overload.overload` is `typing_extensions.overload`.
For this pattern to work in all Python versions, you must use `typing_extensions.overload`, not `typing.overload`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the first line is true based on the try-except in the code.

The second sentence is not really helpful since everything is imported from plum anyway, I'd rather avoir using it unless there is something actionnable on the user side (but I don't see what this is)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! First line is corrected now.

With the second line I intended to explain that things might break if you try to from typing import overload. I've added a brief second sentence to elaborate.

plum/overload.py Outdated

__all__ = ["overload", "dispatch"]

T = TypeVar("T", bound=Callable[..., Any])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unused now.

Copy link
Member Author

@wesselb wesselb Aug 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this back in, because it seems that pyright does require the T -> T construction. :( Please see the other conversation.

plum/overload.py Outdated Show resolved Hide resolved
@wesselb
Copy link
Member Author

wesselb commented Aug 17, 2023

@wch, I've integrated pyright into the unit test. Whereas the code is not fully pyright-compliant, pyright does pick up the dispatch pattern and complains whenever f is given an argument of the wrong type, so that's at least something! This is asserted by the comments in tests/typechecked/test_overload.py in combination with check_linter_assertions.py. It's a little hacked together, but it does the job.

docs/api.rst Outdated Show resolved Hide resolved
@wesselb
Copy link
Member Author

wesselb commented Aug 19, 2023

Unless there are any objections, I think that I'll be merging this. :)

Let's address further improvements in separate PRs!

Thanks @gabrieldemarmiesse and @wch for your suggested improvements and feedback!!

@wesselb wesselb merged commit a0f59d6 into master Aug 19, 2023
11 checks passed
@wesselb wesselb deleted the wbruinsma/overload branch August 19, 2023 17:26
@gabrieldemarmiesse
Copy link
Contributor

LGTM!

@PhilipVinc
Copy link
Collaborator

I've played a bit around with this, and judging from my personal use case, where dispatch is used to multiple expand a base library, I can say that it's not easy to ensure that the function defined with @dispatch be the last one defined.
I think this interface is a bit... not easy to use.

Also, we had added a while back @abstract to be used as a first 'abstract' definition. Ideally one would define the @abstract function once and then define all implementations with @overload and plum would automatically register those at call time (though it might not be easy to detect when one has to re-register).

Can't we make dispatch(method, ...) automatically call _ = typing.overload(method) inside once so that (hopefully) type-checkers are happy?

@wesselb
Copy link
Member Author

wesselb commented Aug 20, 2023

@PhilipVinc That's an interesting idea!

I've hacked around a bit to see if I could come up with a mypy-compliant way where a function is defined in one file and extended in another, but I've had no success. Some problems are that (1) type checkers assume that all overloads must come before the implementation, (2) you cannot import a function and then add more overloads, even if the imported function only has overloads defined, and (3) overloaded functions must have an implementation in the same file.

I'm almost thinking that some kind of rewriting of the source before running mypy or a linter would be the simplest way to make this work...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants