Repository navigation
Refine how overload selection handles *args, **kwargs, and Any - #5166
ilevkivskyi merged 4 commits into
Conversation
This pull request implements the changes discussed in python#5124. Specifically... 1. When two overload alternatives match due to Any, we return the last matching return type if it's a supertype of all of the previous ones. If it's not a supertype, we give up and return 'Any' as before. 2. If a user calls an overload with a starred expression, we try matching alternatives with a starred arg or kwarg first, even if those alternatives do not appear first in the list. If none of the starred alternatives are a valid match, we fall back to checking the other remaining alternatives in order.
|
This pull request should be orthogonal to my other pending one -- the two can be reviewed independently. |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks and sorry for a delay! Just few minor comments.
| @@ -1313,15 +1313,15 @@ def f(x): pass | |||
|
|
|||
| a: Any | |||
| # Any causes ambiguity | |||
There was a problem hiding this comment.
This comment is now less clear, I would rather remove it or clarify.
| @@ -1365,7 +1365,7 @@ def f(x): pass | |||
|
|
|||
| a: Any | |||
| # TODO: We could infer 'int' here | |||
There was a problem hiding this comment.
Hm, this TODO looks interesting. Would it be hard to fix?
| @overload | ||
| def foo(x: int, y: int) -> B: ... | ||
| @overload | ||
| def foo(x: int, y: int, z: int, *args: int) -> C: ... |
There was a problem hiding this comment.
I would add a test with overload like this:
@overload
def f(x: int) -> Tuple[int]: ...
@overload
def f(x: int, y: int) -> Tuple[int, int]: ...
@overload
def f(*xs: int) -> Tuple[int, ...]: ...And check how it various calls with and without star.
Specific changes made: 1. Modify the code that checks for ambiguity due to Any to handle the case where an argument maps to multiple formals. 2. Add extra test cases. 3. Clarify an out-of-date comment.
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks! Just one minor suggestion.
Also looking at the tests, it seems to me we don't have enough tests for overload calls where overload definition had errors (that were type ignored). Could you please add this to the TODO list if you agree?
|
|
||
| reveal_type(f(*[])) # E: Revealed type is 'builtins.tuple[builtins.int]' | ||
| reveal_type(f(*[i])) # E: Revealed type is 'builtins.tuple[builtins.int]' | ||
| reveal_type(f(*[i, i])) # E: Revealed type is 'builtins.tuple[builtins.int]' |
There was a problem hiding this comment.
What happens if there is a tuple instead of list? Ideally f(*(i, i)) should infer Tuple[int, int]. If this is the case, then I would add this call to this test.
|
@ilevkivskyi -- fyi I need to run to a thing; I'll check the tuple thing once I get back. |
|
@ilevkivskyi -- ok, I think this is ready for another round of review. I discovered that Also, I ran into an interesting edge case in the very last test I added ( |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
OK, this is now ready to land. Thanks!
| reveal_type(f(*a)) # E: Revealed type is '__main__.B' | ||
| reveal_type(f(*b)) # E: Revealed type is 'Any' | ||
|
|
||
| # TODO: Should this be 'Any' instead? |
There was a problem hiding this comment.
I am OK with the current behaviour. This is exactly one of those cases where the rule "when in doubt -- take first" applies.
2 participants
This pull request implements the changes discussed in #5124.
Specifically...
When two overload alternatives match due to Any, we return the last matching return type if it's a supertype of all of the previous ones. If it's not a supertype, we give up and return 'Any' as before.
If a user calls an overload with a starred expression, we try matching alternatives with a starred arg or kwarg first, even if those alternatives do not appear first in the list. If none of the starred alternatives are a valid match, we fall back to checking the other remaining alternatives in order.