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

Conversation

Projects
None yet
5 participants
@kirbyfan64
Contributor

kirbyfan64 commented Apr 26, 2017

Fixes #3262.

I removed testOverloadedOperatorMethodOverrideWithSwitchedItemOrder, because the test case had been added back in 2013, long before @overload was banned from non-stubs. Now that it's stub-only, switching the order shouldn't really change anything anymore, since it's unaffected by the runtime.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Apr 26, 2017

@overload was banned from non-stubs. Now that it's stub-only

What do you mean by "now it's stub only"? I think it is not.

@kirbyfan64

This comment has been minimized.

Contributor

kirbyfan64 commented Apr 26, 2017

@ilevkivskyi Haha, cough cough I'm tired. :) What I meant was that the order no longer matters. @overload only signals mypy; it no longer performs any kind of runtime dispatch.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Apr 26, 2017

It never did.

@chadrik

This comment has been minimized.

Contributor

chadrik commented Apr 26, 2017

@overload was stub-only and just recently allowed in code, but it doesn't do anything but error if used, because, well, I'll let the error speak for itself:

def _overload_dummy(*args, **kwds):
    """Helper for @overload to raise when called."""
    raise NotImplementedError(
        "You should not call an overloaded function. "
        "A series of @overload-decorated functions "
        "outside a stub module should always be followed "
        "by an implementation that is not @overload-ed.")
@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Apr 26, 2017

I know, I pretty much wrote that code. :-)

@chadrik

This comment has been minimized.

Contributor

chadrik commented Apr 26, 2017

Sorry, @gvanrossum, that wasn't for you it was for @kirbyfan64.

But if there's anything else you'd like to know about python or mypy maybe I can help. j/k!

@kirbyfan64

This comment has been minimized.

Contributor

kirbyfan64 commented Apr 26, 2017

Huh...I thought a looong time ago (maybe ~2013?) @overload had actually tried multiple dispatch, back when mypy was trying to be its own Python-based language (e.g. not a type checker).

Side note: it'd be kinda nice if @JukkaL still had the code in the old mypy-py repo, for historical reasons. :)

@sixolet

Subtyping of overloaded functions breaks my head a little, and has, I think, more weird little corner cases than I've yet thought of.

I'll give it a little more thought on my ridiculously long bike ride tomorrow, but here's some commentary to get you started. The summary of the changes I'm requesting is, I think, "dig hard for weird corner cases, I suspect there are lots hiding"

isinstance(original, Overloaded) and
name not in nodes.reverse_op_methods.keys()):
# Allow subtype overloads to be greater than their supertype.
fail = is_subtype(original, override, ignore_pos_arg_names=True)

This comment has been minimized.

@sixolet

sixolet Apr 27, 2017

Collaborator

So, I read this block as "if they're both overloaded functions, and the override isn't a subtype of the original, it's still ok as long as they're completely incomparable. If the original is a subtype of the override, then it's an error". That doesn't make sense to me.

My naïve understanding of what should be happening here instead has this block unchanged, and handles all worrying about whether overloads are subtypes for each other in is_subtype.

This comment has been minimized.

@kirbyfan64

kirbyfan64 Apr 27, 2017

Contributor

@sixolet

So, I read this block as "if they're both overloaded functions, and the override isn't a subtype of the original, it's still ok as long as they're completely incomparable. If the original is a subtype of the override, then it's an error". That doesn't make sense to me.

cough I think I might have written that code backwards... cough

if not is_subtype(left.items()[i], right.items()[i], self.check_type_parameter,
# Ensure each overload in the left side is accounted for.
super_overloads = left.items()[:]

This comment has been minimized.

@sixolet

sixolet Apr 27, 2017

Collaborator

This looks backwards. It should be "is left a subtype of right", not vice versa. See line 106 and line 39. That might be why you had to hack something to work up above in check_override

This comment has been minimized.

@kirbyfan64

kirbyfan64 Apr 27, 2017

Contributor

@sixolet Bad variable name (I had originally written the code backwards by accident but forgot about this name), but not necessarily wrong.

I interpreted subtypes kind of like subsets: if X is a subset of Y, then every element in X is also an element in Y. The code here is basically checking that, if the left Overload is a subset of the right Overload, then every "element" in left should also be present in right (even though right can actually contain more elements).

@@ -2494,6 +2477,33 @@ reveal_type(f(BChild())) # E: Revealed type is 'foo.B'
[builtins fixtures/classmethod.pyi]
[out]
[case testSubtypeWithMoreOverloadsThanSupertypeSucceeds]

This comment has been minimized.

@sixolet

sixolet Apr 27, 2017

Collaborator

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.

This comment has been minimized.

@kirbyfan64

kirbyfan64 Apr 27, 2017

Contributor

@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!).

@@ -249,13 +249,22 @@ 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()):
if len(left.items()) < len(right.items()):

This comment has been minimized.

@sixolet

sixolet Apr 27, 2017

Collaborator

I posted a suggestion for a test below that violates this rule.

# Ensure each overload in the left side is accounted for.
super_overloads = left.items()[:]
while super_overloads:

This comment has been minimized.

@sixolet

sixolet Apr 27, 2017

Collaborator

Every overload in the supertype must have at least one corresponding overload in the subtype, yes. There are also some weird cases I suspect exist for each overload in the subtype and how it interacts with the supertype's overloads whose arguments it overlaps. I gave an example in the test section below.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Apr 27, 2017

The related discussion about order of overloads on typing tracker python/typing#253

@kirbyfan64

This comment has been minimized.

Contributor

kirbyfan64 commented Apr 27, 2017

Ok, I think there's a slightly bigger problem here: what's the definition of a subtype with regard to overloaded functions?

For instance, this should be valid:

class A:
    @overload
    def f(x: int) -> int: pass
    @overload
    def f(x: str) -> str: pass

class B:
    @overload
    def f(x: int) -> int: pass
    @overload
    def f(x: Union[str, bytes]) -> str: pass

should be valid, but B.f is technically a supertype, not subtype of A.f. However:

class S(str): pass

class A:
    @overload
    def f(x: int) -> int: pass
    @overload
    def f(x: str) -> str: pass

class B:
    @overload
    def f(x: int) -> int: pass
    @overload
    def f(x: S) -> str: pass

is not valid, but B.f is still a supertype of A.f.

TBH I think the best solution would be to not actually use is_subtype in overload checks. There could be another function, check_overloads_compatible(strict_subtype=Flase), which would allow it to either do a basic subtype check or a more loose, general override check. Then, is_subtype could just call that with strict_subtype=True.

Thoughts?

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Apr 27, 2017

@kirbyfan64

... B.f is technically a supertype, not subtype of A.f. However: ...

Hmm... Maybe I am missing something, but why do you think so? Naively, I would expect just a contravariant behaviour in argument type. So that B.f is a subtype of A.f (assuming you just omit self everywhere for simplicity).

@sixolet

This comment has been minimized.

Collaborator

sixolet commented Apr 27, 2017

Yep. Trying to think about function arguments in terms of subtyping as supersets/subsets always confuses me. I do my best to limit my thinking to substitutability, which treats me better -- A function is a subtype of another function if its arguments are as permissive or more and its return type is as strict or more. Thank you, Barbara Liskov.

@gvanrossum

This comment has been minimized.

Member

gvanrossum commented Aug 22, 2017

@kirbyfan64 This PR has not been updated for almost half a year. I think it's actually a good idea to address this, I think I've seen this kind of false positive a few times in real code. Are you interested in working on this still? TBH I don't understand why your test is failing -- presumably your implementation is too naive? (I haven't tried to understand it.)

@ilevkivskyi ilevkivskyi self-assigned this Aug 31, 2017

sub_overloads.pop()
break
else:
# One of the overloads was not present in the right side.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 2, 2017

Collaborator

I think the logic is reversed here. It should be like this: for every item on the right (supertype) there must be an item on the left that is its subtype.

@kirbyfan64

This comment has been minimized.

Contributor

kirbyfan64 commented Sep 15, 2017

Hey, turns out I'm not dead. ;)

FWIW it seems @ilevkivskyi was right that I somehow got all the logic backwards. The only remaining failing test case is:

[case testOverloadedOperatorMethodOverrideWithNewItem]
from foo import *
[file foo.pyi]
from typing import overload, Any
class A:
    @overload
    def __add__(self, x: int) -> 'A': pass
    @overload
    def __add__(self, x: str) -> 'A': pass
class B(A):
    @overload
    def __add__(self, x: int) -> A: pass
    @overload
    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"

What was the reason for this one? IMO it seems ok (A() + int would still be disallowed, but B() + int would be ok?).

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 17, 2017

What was the reason for this one?

Maybe there is something about __add__ and __radd__ being in agreement?
But most likely this is just an old test that needs to be updated.

@overload
def __add__(self, x: 'B') -> 'B': pass
[out]
tmp/foo.pyi:8: error: Signature of "__add__" incompatible with supertype "A"

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 30, 2017

Collaborator

Why have you removed this test? Does it fail? The semantics of overload is not 100% fixed by PEP 484, see python/typing#253, but IIUC the consensus is that order of overloads does matter, so that in this case the override is indeed incompatible. @JukkaL could you please clarify this?

@kirbyfan64

This comment has been minimized.

Contributor

kirbyfan64 commented Nov 7, 2017

Ok, so if Travis looks happy, then this should finally be done-ish. All of the cases @sixolet mentioned now work properly, all the tests pass (well, we'll see what CI thinks), order works properly as @ilevkivskyi, and most importantly, the original issue still works.

EDIT: Yup, I broke something. Hold on...

@kirbyfan64

This comment has been minimized.

Contributor

kirbyfan64 commented Nov 7, 2017

Ok, tests pass now!

@ilevkivskyi

Sorry for a long delay, here are my comments. This looks good, but I think it could be simplified.

found_match = False
for left_index, left_item in enumerate(left.items()):
subtype_match = is_subtype(left_item, right_item, self.check_type_parameter,

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 17, 2017

Collaborator

Why are you using is_subtype here but is_callable_subtype below?

This comment has been minimized.

@kirbyfan64

kirbyfan64 Dec 15, 2017

Contributor

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

# 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,

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 17, 2017

Collaborator

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

This comment has been minimized.

@kirbyfan64

kirbyfan64 Dec 15, 2017

Contributor

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.

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 17, 2017

Collaborator

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.

This comment has been minimized.

@kirbyfan64

kirbyfan64 Dec 15, 2017

Contributor

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

@ilevkivskyi

OK, I am going to merge this now, since this is already an improvement and does introduce any controversy. Latter we will have a more "global" discussion about how overloads should work and will update this if needed.

Thanks for working on this, and sorry that you waited so long!

@ilevkivskyi ilevkivskyi merged commit d187ab3 into python:master Jan 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment