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 detecting overloads with overlapping arities #5163

Merged
merged 5 commits into from Jun 16, 2018

Conversation

Projects
None yet
2 participants
@Michael0x2a
Copy link
Collaborator

Michael0x2a commented Jun 6, 2018

This commit addresses TODO 2 from #5119 by adding support for detecting overloads with partially overlapping arities.

It also refactors the is_callable_compatible method. Specifically, this pull request...

  1. Pulls out a lot of the logic for iterating over formal arguments into a helper method in CallableType.

  2. Pulls out logic for handling varargs and kwargs outside of loops.

  3. Rearranges some of the logic so we can return earlier slightly more frequently.

Add support for detecting overloads with overlapping arities
This commit addresses TODO 2 from #5119
by adding support for detecting overloads with partially overlapping
arities.

It also refactors the `is_callable_compatible` method. Specifically,
this pull request...

1. Pulls out a lot of the logic for iterating over formal arguments into
   a helper method in CallableType.

2. Pulls out logic for handling varargs and kwargs outside of loops.

3. Rearranges some of the logic so we can return earlier slightly more
   frequently.

@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! Looks very good. I have essentially only one non-trivial question about overlap due to empty arg list in a call.

@@ -3648,10 +3642,12 @@ def is_more_precise_or_partially_overlapping(t: Type, s: Type) -> bool:
return (is_callable_compatible(signature, other,
is_compat=is_more_precise_or_partially_overlapping,
is_compat_return=lambda l, r: not is_subtype(l, r),
check_args_covariantly=True) or
check_args_covariantly=True,
allow_potential_compatibility=True) or

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 9, 2018

Collaborator

TBH, I don't like the term "potential compatibility". Why not "partial overlap"? After all, IIUC, this is the same as Union[A, B] vs Union[B, C] but for arg counts.

@@ -616,6 +617,30 @@ def g(x: int) -> int: ...
In this case, the first call will succeed and the second will fail: f is a
valid stand-in for g but not vice-versa.
allow_potential_compatibility:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 9, 2018

Collaborator

See my comment above about terminology.

allow_potential_compatibility:
By default, this function returns True if and only if left is
definitely compatible with right.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 9, 2018

Collaborator

Instead of "definitely" maybe add an explanation in terms of subtyping? This shouldn't be mathematically precise (I could guess why you are mostly using "compatibility" instead of "subtyping", but for historical reasons in mypy code compatibility is checked by is_subtype, while subtyping by is_proper_subtype). Maybe say something like each call that succeeds for right, will guaranteed to succeed for left callable.

g(*args: int) -> int
However, they would be *potentially* compatible under certain conditions --
for example, if the user runs "f_or_g(3)". So, if this flag is

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 9, 2018

Collaborator

Again, this will be probably more clear (at least for me), if you say that partial overlap means that some calls that succeed for right, will also succeed for the left callable.

@@ -662,6 +662,8 @@ class CallableType(FunctionLike):
'min_args', # Minimum number of arguments; derived from arg_kinds
'is_var_arg', # Is it a varargs function? Derived from arg_kinds
'is_kw_arg', # Is it a **kwargs function? Derived from arg_kinds

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 9, 2018

Collaborator

Do we need both is_kw_arg and kw_arg etc. Maybe can make is_kw_ars a property that returns kw_arg is not None? CallableType has already loads of fields.

if they are not None.
If you really want to include star args in the yielded output, set the
'include_star_args' field to 'True'."""

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 9, 2018

Collaborator

set field -> pass argument

@@ -818,6 +837,30 @@ def max_possible_positional_args(self) -> int:
blacklist = (ARG_NAMED, ARG_NAMED_OPT)
return len([kind not in blacklist for kind in self.arg_kinds])

def formal_arguments(self, include_star_args: bool = False) -> Iterator[FormalArgument]:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 9, 2018

Collaborator

It is a good idea to make this a method.

@overload
def foo2(*args: int) -> int: ... # E: Overloaded function signature 2 will never be matched: function 1's parameter type(s) are the same or broader
def foo2(*args: int) -> int: ...

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 9, 2018

Collaborator

I am actually not so sure now we need to make this an error. This is very similar to empty list situation, maybe we can just pick the first overload?

@overload
def foo2(*args: int) -> str: ... # E: Overloaded function signatures 1 and 2 overlap with incompatible return types
@overload
def foo2(*args2: str) -> int: ...

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 9, 2018

Collaborator

Same here, this is similar to empty list.

Michael0x2a added some commits Jun 15, 2018

Respond to code reviews
Changes made:

- Rename 'allow_potential_compatibility' to 'allow_partial_overload'

- Refactor 'is_callable_compatible' to be symmetric when
  'allow_partial_overload' is set.

- Modify is_callable_compatible to no longer consider functions like
  "f(*int) -> int" and "f(*str) -> int" to be partially overlapping/
  potentially compatible.
@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Jun 15, 2018

  1. I originally went with allow_potential_compatibility because I was thinking about this problem in terms of universal vs existential checks, not overlaps. The term "overlap" also, to me, implied that is_callable_compatible would be "symmetric" in a way that it wasn't when the param was set. E.g. the following two functions weren't previously equivalent:

     is_callable_compatible(f, g,
                            is_compat=some_symmetric_check,
                            allow_potential_compatibility=True)
     is_callable_compatible(g, f,
                            is_compat=some_symmetric_check,
                            allow_potential_compatibility=True)
    

    After some reflection though, I decided it would make the logic more consistent if I just went ahead and added that symmetry.

  2. I previously used the term "compatible" mainly because that was how I renamed the function: I previously refactored out all of the subtyping-related logic so that what it means for something to be "compatible" can be customized by the caller by passing in an appropriate is_compat function. That way, we could reuse the same logic to perform checks unrelated to subtyping.

    I don't think I was really aware that the term "compatibility" already meant something specific within mypy. So to avoid confusion, maybe I should rename is_callable_compatible and its params to not use the word "compatible"? I'm not sure what a good alternative name would be though -- let me know what you think.

  3. Regarding the f(*args) thing -- I made the change you suggested.

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Great job! I didn't look carefully though all the changes to the code. But all the tests look good, except for one questionable test. If it is not easy to fix, please just go ahead and merge (this is really a corner case).

if they are not None.
If you really want to include star args in the yielded output, set the
'include_star_args' field to 'True'."""

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 15, 2018

Collaborator

set field -> pass argument

It seems you missed this comment. Or you think "set field" is an OK formulation for functions?

# Left must have some kind of corresponding argument.
# Phase 1: Confirm every argument in R has a corresponding argument in L.

# Phase 1a: If right and right can both accept an infinite number of args,

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 15, 2018

Collaborator

"right and right" -> "left and right"

@@ -1412,15 +1412,19 @@ reveal_type(f(d)) # E: Revealed type is 'builtins.list[builtins.int]'
from typing import overload, Any

@overload
def f(*, x: int = 3, y: int = 3) -> int: ...
def f(*, x: int = 3, y: int = 3) -> int: ... # E: Overloaded function signatures 1 and 2 overlap with incompatible return types

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 15, 2018

Collaborator

Hm, I don't think this this should be an error. The only call that is allowed for both is f().

@Michael0x2a Michael0x2a reopened this Jun 16, 2018

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Jun 16, 2018

I could have sworn I fixed the field/parameter thing, but I guess not -- thanks for the (re)-catch!

Regarding the test case: I think I'm going to just merge this as-is. I did a bit of poking and I think it's possible to convert that error case back into a pass, but it'll be a bit kludgy. We basically need to special-case the first match an *arg or **kwarg has so we can pretend that args/kwargs is never length 0. I'm thinking we can add in this kludge later if it turns out it's impossible to type certain things w/ the current semantics.

I think this is also more or less consistent with how we handle non-kwarg/non-arg signatures -- for example, this PR considers the following overload to be unsafely overlapping.

@overload
def foo(*, x: str = '...', y: str = '...') -> str: ...
@overload
def foo(*, x: int = 3, y: int = 3) -> int: ...
def foo(*args): 
    pass

I think it'd maybe be a bit surprising if swapping the last signature for a **kwargs: int were to suddenly make this the check.

@Michael0x2a Michael0x2a merged commit 4fe2220 into python:master Jun 16, 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-partially-overlapping-arities branch Jun 16, 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.