-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Conversation
This comment has been minimized.
This comment has been minimized.
a4fba62
to
3f102d3
Compare
a7ffa2c
to
d9ceadf
Compare
151393b
to
50dfa0d
Compare
50dfa0d
to
425a8d4
Compare
@JukkaL Maybe you would like to take a look at this, too? I believe it could be another good usability improvement, especially considering that |
425a8d4
to
23439c6
Compare
23439c6
to
1e914d8
Compare
1e914d8
to
301072d
Compare
301072d
to
7e7502b
Compare
There was a problem hiding this 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" |
There was a problem hiding this comment.
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.
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. |
@cdce8p Oh, I was checking the status of existing PRs. Just take your time. |
test-data/unit/check-functions.test
Outdated
@@ -1400,8 +1400,9 @@ def top() -> None: | |||
from typing import Any | |||
x = None # type: Any | |||
if x: | |||
pass # some other node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
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
.
def f3(g: A) -> A: ... | ||
@overload | ||
def f3(g: B) -> B: ... | ||
if maybe_true: # E: Name "maybe_true" is not defined |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
f529270
to
4776070
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Thanks for taking the time to review my PR @JelleZijlstra! |
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.
Closes #9744
Test Plan
Unit tests have been added.
Limitations
Only
if
is supported. Support forelif
andelse
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 forelif
andelse
is required for the tests to pass.Update: Added support for
elif
andelse
.Current workaround