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

Improve error message for overload overrides w/ swapped variants #5369

Merged
merged 1 commit into from Aug 2, 2018

Conversation

Projects
None yet
2 participants
@Michael0x2a
Collaborator

Michael0x2a commented Jul 17, 2018

This commit is intended to resolve #1270.

Currently, running mypy on the following program:

from typing import overload

class Parent:
    @overload
    def foo(self, x: A) -> A: ...
    @overload
    def foo(self, x: B) -> B: ...

class Child(Parent):
    @overload
    def foo(self, x: B) -> B: ...
    @overload
    def foo(self, x: A) -> A: ...

...will make mypy report the following error -- it considers the fact that we've swapped the order of the two variants to be unsafe:

test.pyi:10: error: Signature of "foo" incompatible with supertype "Parent"

This error message can be confusing for some users who may not be aware that the order in which your overloads are defined can sometimes matter/is something mypy relies on.

This commit modifies the error message to the following to try
and make this more clear:

test.pyi:10: error: Signature of "foo" incompatible with supertype "Parent"
test.pyi:10: note: Overload variants must be defined in the same order as they are in "Parent"
Improve error message for overload overrides w/ swapped variants
This commit is intended to resolve #1270.

Currently, running mypy on the following program:

    from typing import overload

    class Parent:
        @overload
        def foo(self, x: A) -> A: ...
        @overload
        def foo(self, x: B) -> B: ...

    class Child(Parent):
        @overload
        def foo(self, x: B) -> B: ...
        @overload
        def foo(self, x: A) -> A: ...

...will make mypy report the following error -- it considers the fact
that we've swapped the order of the two variants to be unsafe:

    test.pyi:10: error: Signature of "foo" incompatible with supertype "Parent"

This error message can be confusing for some users who may not be
aware that the order in which your overloads are defined can sometimes
matter/is something mypy relies on.

This commit modifies the error message to the following to try
and make this more clear:

    test.pyi:10: error: Signature of "foo" incompatible with supertype "Parent"
    test.pyi:10: note: Overload variants must be defined in the same order as they are in "Parent"
@Michael0x2a

This comment has been minimized.

Show comment
Hide comment
@Michael0x2a

Michael0x2a Jul 17, 2018

Collaborator

As a misc note, one idea the issue I linked to above mentioned was that we could maybe print out the signature of the parent method. I decided not to do this for now because it felt inconsistent: we don't do this in other cases where the user incorrectly overrides a method.

If printing out the parent signature seems like a helpful thing to do in general, I'm happy to make a new PR w/ that change.

Collaborator

Michael0x2a commented Jul 17, 2018

As a misc note, one idea the issue I linked to above mentioned was that we could maybe print out the signature of the parent method. I decided not to do this for now because it felt inconsistent: we don't do this in other cases where the user incorrectly overrides a method.

If printing out the parent signature seems like a helpful thing to do in general, I'm happy to make a new PR w/ that change.

@ilevkivskyi

Thanks! This additional note makes sense. Also I think it is right to only show in in obvious cases.

@ilevkivskyi ilevkivskyi merged commit bfb47e1 into python:master Aug 2, 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:refine-overload-overloading-error-message branch Aug 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment