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 support for conditionally defined overloads #10712

Merged
merged 28 commits into from
Mar 3, 2022

Conversation

cdce8p
Copy link
Collaborator

@cdce8p cdce8p commented Jun 25, 2021

Description

This PR allows users to define overloads conditionally, e.g., based on the Python version. At the moment this is only possible if all overloads are contained in the same block which requires duplications.

from typing import overload, Any
import sys

class A: ...
class B: ...

if sys.version_info >= (3, 9):
    class C: ...


@overload
def func(g: int) -> A: ...

@overload
def func(g: bytes) -> B: ...

if sys.version_info >= (3, 9):
    @overload
    def func(g: str) -> C: ...

def func(g: Any) -> Any: ...

Closes #9744

Test Plan

Unit tests have been added.

Limitations

Only if is supported. Support for elif and else might be added in the future. However, I believe that the single if as shown in the example is also the most common use case.

The change itself is fully backwards compatible, i.e. the current workaround (see below) will continue to function as expected.

Update: Seems like support for elif and else is required for the tests to pass.

Update: Added support for elif and else.

Current workaround

from typing import overload, Any
import sys

class A: ...
class B: ...

if sys.version_info >= (3, 9):
    class C: ...


if sys.version_info >= (3, 9):
    @overload
    def func(g: int) -> A: ...

    @overload
    def func(g: bytes) -> B: ...

    @overload
    def func(g: str) -> C: ...

    def func(g: Any) -> Any: ...

else:
    @overload
    def func(g: int) -> A: ...

    @overload
    def func(g: bytes) -> B: ...

    def func(g: Any) -> Any: ...

@github-actions

This comment has been minimized.

@cdce8p cdce8p marked this pull request as draft June 25, 2021 11:31
@cdce8p cdce8p marked this pull request as ready for review June 27, 2021 12:03
@cdce8p cdce8p force-pushed the conditional-overloads branch 2 times, most recently from a7ffa2c to d9ceadf Compare July 7, 2021 19:53
@cdce8p cdce8p force-pushed the conditional-overloads branch 3 times, most recently from 151393b to 50dfa0d Compare August 2, 2021 07:13
@cdce8p
Copy link
Collaborator Author

cdce8p commented Sep 22, 2021

@JukkaL Maybe you would like to take a look at this, too? I believe it could be another good usability improvement, especially considering that pyright already supports it.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Sorry for the slow response! Left a question about elif/else blocks. Overall it seems like this could be a useful feature, especially in stubs.

@overload
def f1(g: str) -> B: ...
def f1(g: Any) -> Any: ...
reveal_type(f1(42)) # N: Revealed type is "__main__.A"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens in cases like this:

@overload
def f(x: A) -> A: ...
if x:
    @overload
    def f(x: B) -> B: ...
elif y:
    @overload
    def f(x: C) -> C: ...
else:
    @overload
    def f(x: D) -> D: ...

Consider all combinations of bool values for x and y. For example, if x is always false but y is always true, should we pick up C -> C from the elif block? We should have a test case that covers this. If this isn't supported, there should be a useful error message.

@97littleleaf11
Copy link
Collaborator

Any progress on this?

@cdce8p
Copy link
Collaborator Author

cdce8p commented Nov 26, 2021

Any progress on this?

@97littleleaf11 I hadn't had much time to look at it so far. Sorry about that. If it's urgent I can look at it sooner but from what I've seen the cutoff for the next release has already happened. I hope to address the comment next week.

@97littleleaf11
Copy link
Collaborator

@cdce8p Oh, I was checking the status of existing PRs. Just take your time.

@JelleZijlstra JelleZijlstra self-requested a review March 1, 2022 22:58
@@ -1400,8 +1400,9 @@ def top() -> None:
from typing import Any
x = None # type: Any
if x:
pass # some other node
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 3d0397a. Starting If stmts are no longer merged if they don't contain at least one overload.

mypy/fastparse.py Outdated Show resolved Hide resolved
mypy/fastparse.py Outdated Show resolved Hide resolved
def f3(g: A) -> A: ...
@overload
def f3(g: B) -> B: ...
if maybe_true: # E: Name "maybe_true" is not defined
Copy link
Member

Choose a reason for hiding this comment

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

What if maybe_true is defined but of type bool? It seems like you just silently ignore all conditionally defined overloads. It would make more sense to me to produce an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

914d517, mypy would now emit an error for it.

Condition cannot be inferred, unable to merge overloads [misc]

@JelleZijlstra JelleZijlstra self-requested a review March 2, 2022 20:21
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

docs/source/more_types.rst Outdated Show resolved Hide resolved
docs/source/more_types.rst Outdated Show resolved Hide resolved
docs/source/more_types.rst Outdated Show resolved Hide resolved
docs/source/more_types.rst Outdated Show resolved Hide resolved
docs/source/more_types.rst Outdated Show resolved Hide resolved
docs/source/more_types.rst Outdated Show resolved Hide resolved
docs/source/more_types.rst Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra self-assigned this Mar 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit 0777c10 into python:master Mar 3, 2022
@cdce8p cdce8p deleted the conditional-overloads branch March 3, 2022 09:58
@cdce8p
Copy link
Collaborator Author

cdce8p commented Mar 3, 2022

Thanks for taking the time to review my PR @JelleZijlstra!
I've been looking forward to this one for a long time.

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.

@overload cannot be used with conditional "if TYPE_CHECKING"
4 participants