Skip to content

Classes break overload checking #11606

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

Closed
A5rocks opened this issue Nov 24, 2021 · 4 comments · Fixed by #11608
Closed

Classes break overload checking #11606

A5rocks opened this issue Nov 24, 2021 · 4 comments · Fixed by #11608
Labels
bug mypy got something wrong

Comments

@A5rocks
Copy link
Collaborator

A5rocks commented Nov 24, 2021

Bug Report

A certain overloaded function works, but when put in a class no longer works.

To Reproduce

import typing

# this works:
@typing.overload
def f(a: str, /) -> None: ...

@typing.overload
def f(*, b: bool = False) -> None: ...

def f(a: str | None = None, /, *, b: bool = False) -> None:
    ...

# this does not work:
class Blah:
    @typing.overload
    def f(self, a: str, /) -> None: ...
    
    @typing.overload
    def f(self, *, b: bool = False) -> None: ...
    
    def f(self, a: str | None = None, /, *, b: bool = False) -> None:
        ...

Expected Behavior

For this case, as the self argument is not annotated and is always there, mypy shouldn't report any issues.

Actual Behavior

main.py:21: error: Overloaded function implementation does not accept all possible arguments of signature 2
Found 1 error in 1 file (checked 1 source file)

Your Environment

Reproduced in mypy playground: https://mypy-play.net/?mypy=master&python=3.10&flags=strict&gist=d2c0e39b68def2cd02633caeded68e3f

@A5rocks A5rocks added the bug mypy got something wrong label Nov 24, 2021
@sobolevn
Copy link
Member

sobolevn commented Nov 24, 2021

This happens because for some reason left and right are def (ex.Blah, Union[builtins.str, None] =, *, b: builtins.bool =) and def (self: ex.Blah, *, b: builtins.bool =) (notice self difference).

It does not pass this check:

mypy/mypy/subtypes.py

Lines 1058 to 1077 in b47245a

def is_different(left_item: Optional[object], right_item: Optional[object]) -> bool:
"""Checks if the left and right items are different.
If the right item is unspecified (e.g. if the right callable doesn't care
about what name or position its arg has), we default to returning False.
If we're allowing partial overlap, we also default to returning False
if the left callable also doesn't care."""
if right_item is None:
return False
if allow_partial_overlap and left_item is None:
return False
return left_item != right_item
# If right has a specific name it wants this argument to be, left must
# have the same.
if is_different(left.name, right.name):
# But pay attention to whether we're ignoring positional arg names
if not ignore_pos_arg_names or right.pos is None:
return False

In contrast to regular functions:
def (Union[builtins.str, None] =, *, b: builtins.bool =) and def (*, b: builtins.bool =).

Notice that self is named in classes on the right, but not on the left.
I will try to fix this today! 👍

@sobolevn
Copy link
Member

sobolevn commented Nov 24, 2021

This is a corner case of using / in a method. Because in this case you make two arguments positional-only, here: def f(self, a: str, /) -> None: ... you say: both self and a are positional only. This is why it is different from functions.

But, another question is: should we support this corner case?
Because we actually can use self as a named argument, when we use ClassName.method(self=instance). It is rare, but a valid case.

When declaring self as a positional-only, we break this. So, technically these two signatures are really incompatible. @A5rocks, do you agree?

@sobolevn
Copy link
Member

Here's the valid code for this case:

class Blah:
    @overload
    def f(self, a: str, /) -> None: ...

    @overload
    def f(self, /, *, b: bool = False) -> None: ...

    def f(self, a: str | None = None, /, *, b: bool = False) -> None:
        ...

It works: Revealed type is "Overload(def (ex.Blah, builtins.str), def (ex.Blah, *, b: builtins.bool =))"

@A5rocks
Copy link
Collaborator Author

A5rocks commented Nov 24, 2021

When declaring self as a positional-only, we break this. So, technically these two signatures are really incompatible. @A5rocks, do you agree?

Didn't think of it that way, that is true.

That's really an interesting point though, so... Hmmmm...

I think I see what I need to do, thanks.

hauntsaninja pushed a commit that referenced this issue Nov 24, 2021
A5rocks added a commit to A5rocks/Tanjun that referenced this issue Nov 25, 2021
FasterSpeeding pushed a commit to Cursed-Solutions/Tanjun that referenced this issue Jan 2, 2022
* Add a thin veneer of compatibility with mypy

* Run nox

* Remove unused type ignore

* Fix overloads based on python/mypy#11606

* PR feedback

* Update to latest changes

* PR review

* More PR feedback

* Revert slots from protocols
tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this issue Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants