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

Refine how overload selection handles *args, **kwargs, and Any #5166

Merged
merged 4 commits into from Jun 13, 2018

Conversation

Projects
None yet
2 participants
@Michael0x2a
Copy link
Collaborator

Michael0x2a commented Jun 7, 2018

This pull request implements the changes discussed in #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.

Refine how overload selection handles *args, **kwargs, and Any
This pull request implements the changes discussed in
#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.
@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Jun 7, 2018

This pull request should be orthogonal to my other pending one -- the two can be reviewed independently.

@ilevkivskyi ilevkivskyi self-assigned this Jun 7, 2018

@ilevkivskyi ilevkivskyi self-requested a review Jun 7, 2018

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks and sorry for a delay! Just few minor comments.

@@ -1313,15 +1313,15 @@ def f(x): pass

a: Any
# Any causes ambiguity

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 10, 2018

Collaborator

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 10, 2018

Collaborator

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: ...

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 10, 2018

Collaborator

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.

Respond to code review
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
Copy link
Collaborator

ilevkivskyi left a 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]'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 10, 2018

Collaborator

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.

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Jun 10, 2018

@ilevkivskyi -- fyi I need to run to a thing; I'll check the tuple thing once I get back.

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Jun 11, 2018

@ilevkivskyi -- ok, I think this is ready for another round of review.

I discovered that f(*(int, int)) was indeed not being inferred to Tuple[int, int]. I decided to fix this by just disabling the "let's move the alternative to the front" thing if the starred arg we're passing in is a type that has a "shape".

Also, I ran into an interesting edge case in the very last test I added (testOverloadVarargsAndKwargsSelection) -- do you have any thoughts about how we should handle that case?

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a 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?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 13, 2018

Collaborator

I am OK with the current behaviour. This is exactly one of those cases where the rule "when in doubt -- take first" applies.

@ilevkivskyi ilevkivskyi merged commit 0f76dfc into python:master Jun 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Michael0x2a Michael0x2a deleted the Michael0x2a:overloads-refine-handling-of-starargs-and-any branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.