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

Allow subtypes to define more overloads than their supertype #3263

Merged
merged 5 commits into from
Jan 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions mypy/subtypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,46 @@ def visit_overloaded(self, left: Overloaded) -> bool:
return True
return False
elif isinstance(right, Overloaded):
# TODO: this may be too restrictive
if len(left.items()) != len(right.items()):
return False
for i in range(len(left.items())):
if not is_subtype(left.items()[i], right.items()[i], self.check_type_parameter,
ignore_pos_arg_names=self.ignore_pos_arg_names):
# Ensure each overload in the right side (the supertype) is accounted for.
previous_match_left_index = -1
matched_overloads = set()
possible_invalid_overloads = set()

for right_index, right_item in enumerate(right.items()):
found_match = False

for left_index, left_item in enumerate(left.items()):
subtype_match = is_subtype(left_item, right_item, self.check_type_parameter,
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using is_subtype here but is_callable_subtype below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because is_subtype works perfectly here, but below, I need to pass ignore_return=True, which can only be passed to is_callable_subtype.

ignore_pos_arg_names=self.ignore_pos_arg_names)

# Order matters: we need to make sure that the index of
# this item is at least the index of the previous one.
if subtype_match and previous_match_left_index <= left_index:
Copy link
Member

Choose a reason for hiding this comment

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

While python/typing#253 is not fixed, it is not clear how order should be treated. Moreover, there is a major overload rework on the way, so I would limit this PR to a simple logic: Every item in the supertype must have a subtype item on the left.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well you had previously said that order still matters, and so the test shouldn't be removed. In order to avoid from removing the test, I had to make order matter...

if not found_match:
# Update the index of the previous match.
previous_match_left_index = left_index
found_match = True
matched_overloads.add(left_item)
possible_invalid_overloads.discard(left_item)
else:
# If this one overlaps with the supertype in any way, but it wasn't
# an exact match, then it's a potential error.
if (is_callable_subtype(left_item, right_item, ignore_return=True,
ignore_pos_arg_names=self.ignore_pos_arg_names) or
is_callable_subtype(right_item, left_item, ignore_return=True,
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this part, sorry. Why one would check the opposite subtype relationship for overload items?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was due to this test (an edge case pointed out by @sixolet in another comment). That overlapping overload would mean that Sub.foo could return a different type than Super.foo for the same argument set. This catches that edge case.

ignore_pos_arg_names=self.ignore_pos_arg_names)):
# If this is an overload that's already been matched, there's no
# problem.
if left_item not in matched_overloads:
possible_invalid_overloads.add(left_item)

if not found_match:
return False

if possible_invalid_overloads:
# There were potentially invalid overloads that were never matched to the
# supertype.
return False
return True
elif isinstance(right, UnboundType):
return True
Expand Down
84 changes: 82 additions & 2 deletions test-data/unit/check-classes.test
Original file line number Diff line number Diff line change
Expand Up @@ -1416,8 +1416,6 @@ class B(A):
def __add__(self, x: str) -> A: pass
@overload
def __add__(self, x: type) -> A: pass
[out]
tmp/foo.pyi:8: error: Signature of "__add__" incompatible with supertype "A"

[case testOverloadedOperatorMethodOverrideWithSwitchedItemOrder]
from foo import *
Expand Down Expand Up @@ -2494,6 +2492,88 @@ reveal_type(f(BChild())) # E: Revealed type is 'foo.B'
[builtins fixtures/classmethod.pyi]
[out]

[case testSubtypeWithMoreOverloadsThanSupertypeSucceeds]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's have some tests of nasty complicated overloads involving subtyping.

  • One overload in the subtype may cover more than one overload in the supertype (contrived example, might not be a great one):
class Super:
    @overload
    def foo(a: int) -> float: ...
    @overload
    def foo(a: float) -> Number: ...
    @overload
    def foo(a: str) -> str: ...
    
class Sub(Super):
    @overload
    def foo(a: Number) -> float: ...
    @overload
    def foo(a: str) -> str: ...
  • If an overload in the supertype Y_sup accepts an overlapping set of arguments to an overload Y_sub in the subtype, Y_sub must return a subtype of Y_sup's return type. For example, I think this is an error
class Super:
    @overload
    def foo(a: Number) -> Number: ...
    @overload
    def foo(a: str) -> str: ...
    
class Sub(Super):
    @overload
    def foo(a: Number) -> Number: ...
    @overload
    def foo(a: int) -> str: ...
    @overload
    def foo(a: str) -> str: ...

(look at testPartiallyContravariantOverloadSignatures, for another reference)

There's probably even more complexity to this. There usually is.

Copy link
Contributor Author

@refi64 refi64 Apr 27, 2017

Choose a reason for hiding this comment

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

@sixolet FWIW my goal with this PR wasn't necessarily to 100% make this work perfectly, but it was more to allow a bit more than was originally allowed (without breaking anything, of course!).

from foo import *
[file foo.pyi]
from typing import overload


class X: pass
class Y: pass
class Z: pass


class A:
@overload
def f(self, x: X) -> X: pass
@overload
def f(self, y: Y) -> Y: pass

class B(A):
@overload
def f(self, x: X) -> X: pass
@overload
def f(self, y: Y) -> Y: pass
@overload
def f(self, z: Z) -> Z: pass
[builtins fixtures/classmethod.pyi]
[out]

[case testSubtypeOverloadCoveringMultipleSupertypeOverloadsSucceeds]
from foo import *
[file foo.pyi]
from typing import overload


class A: pass
class B(A): pass
class C(A): pass
class D: pass


class Super:
@overload
def foo(self, a: B) -> C: pass
@overload
def foo(self, a: C) -> A: pass
@overload
def foo(self, a: D) -> D: pass

class Sub(Super):
@overload
def foo(self, a: A) -> C: pass
@overload
def foo(self, a: D) -> D: pass
[builtins fixtures/classmethod.pyi]
[out]

[case testSubtypeOverloadWithOverlappingArgumentsButWrongReturnType]
from foo import *
[file foo.pyi]
from typing import overload


class A: pass
class B(A): pass
class C: pass


class Super:
@overload
def foo(self, a: A) -> A: pass
@overload
def foo(self, a: C) -> C: pass

class Sub(Super):
@overload # E: Signature of "foo" incompatible with supertype "Super"
def foo(self, a: A) -> A: pass
@overload
def foo(self, a: B) -> C: pass
@overload
def foo(self, a: C) -> C: pass
[builtins fixtures/classmethod.pyi]
[out]

[case testTypeTypeOverlapsWithObjectAndType]
from foo import *
[file foo.pyi]
Expand Down