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

Return type of "__eq__" incompatible with supertype "object" #2783

Closed
zero323 opened this issue Jan 31, 2017 · 13 comments
Closed

Return type of "__eq__" incompatible with supertype "object" #2783

zero323 opened this issue Jan 31, 2017 · 13 comments

Comments

@zero323
Copy link

zero323 commented Jan 31, 2017

Let's say I have a simple class intended for symbolical computations where == and != evaluate to another expression:

class Expr:
    def __eq__(self, other: object) -> 'Expr': ...
    def __ne__(self, other: object) -> 'Expr': ...
    def __add__(self, other: 'Expr') -> 'Expr': ...
    def __sub__(self, other: 'Expr') -> 'Expr': ...

This will result in following errors:

Return type of "__eq__" incompatible with supertype "object"
Return type of "__ne__" incompatible with supertype "object"

As far as I understand Python data model doesn't require rich comparison methods to return bool. Is it Mypy specific requirement or is there something wrong with my class definition?

@ilevkivskyi
Copy link
Member

This is because of the return type of object.__eq__ in typeshed. Formally you are right that any return type is accepted at runtime, but changing the static type could hide some "design errors" that are otherwise difficult to catch.

@zero323
Copy link
Author

zero323 commented Jan 31, 2017

Thank you for a quick response @ilevkivskyi. How would you proceed in case like this? I thought about Any:

class Expr:
    def __eq__(self, other: Any -> Any: ...
    def __ne__(self, other: Any) -> Any: ...

which is a bit suboptimal but seems acceptable. Does it make sense to you?

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Feb 1, 2017

Any is a possible solution. However, using #type: ignore comments on those two particular lines could be more strict, since mypy will catch errors below in code:

class Expr:
    def __eq__(self, other: Any) -> 'Expr':  # type: ignore
        return Expr()
    def __ne__(self, other: Any) -> 'Expr':  # type: ignore
        return 42  # Error: Incompatible return value type (got "int", expected "Expr")

x = Expr() == 0

x.bad  # Error: "Expr" has no attribute "bad"

while with Any such error will be unnoticed by mypy.

EDIT: extended the example.

@JukkaL
Copy link
Collaborator

JukkaL commented Feb 1, 2017

I wonder if the return type of object.__eq__ in typeshed should be object, since the bool return type is not quite correct. The same applies to __lt__ and friends.

@zero323
Copy link
Author

zero323 commented Feb 1, 2017

Thanks @ilevkivskyi

@exhuma
Copy link

exhuma commented Sep 12, 2018

@ilevkivskyi Using your suggestion of appending # type: ignore to the line does indeed work quite well. But I find it personally very counter-intuitive, seeing a line with type-hints and an additional type: ignore annotation. It makes it look like the type is completely ignored on that line.

@mvaled
Copy link

mvaled commented Sep 25, 2018

I got this is issue by looking for: Argument 1 of "eq" incompatible with supertype "object"

from typing import Union

class Expr:
    def __eq__(self, other):
        # type: (Union[Expr, str]) -> bool
        return False

In this case, I tried to put the '# type: ignore' in the same line as the comment type annotation without luck.

@msullivan
Copy link
Collaborator

It needs to be on the line with the function declaration

@Victor-Savu
Copy link

@JukkaL commented on Feb 1, 2017
I wonder if the return type of object.__eq__ in typeshed should be object, since the bool return type is not quite correct. The same applies to __lt__ and friends.

Given the implementation, I would suggest the return type should be Union[NoReturn, Any]. object.__eq__ always throws a NotImplementedError (hence the NoReturn) and Any allows sub-classes to override the return type to anything they like.

@llchan
Copy link
Contributor

llchan commented May 1, 2019

  • Glancing at the CPython code, I don't think object.__eq__ can throw a NotImplementedError, it only returns bools (assuming it's not overridden in a subclass). I could be reading this wrong or looking in the wrong place though.
  • The union with NoReturn isn't necessary, as that's implied on every function (see discussions in other issues).
  • If we make it return Any, then x == y loses all type information, which isnt desirable for 99% of use cases, so existing signatures should remain bool.

That said, I agree with you that we need to broaden the use cases that are supported. I like the unsafe override decorator proposal in #5704, and I think that that along with addressing some usability issues would go a long way.

@llchan
Copy link
Contributor

llchan commented May 1, 2019

Some related discussion in #6710, which is more about the unexpected downstream effects of the # type: ignore.

@Victor-Savu
Copy link

Victor-Savu commented May 2, 2019

* Glancing at the CPython code, I don't think `object.__eq__` can throw a `NotImplementedError`, it only returns bools

I am so sorry, you are right! It does not throw NotImplementedError, instead it returns NotImplemented:

assert object().__eq__(object()) == NotImplemented

Which would be fine, because it is consistent with Any. But for the reasons mentioned above, the type annotation in typeshed is bool which makes it more restrictive than it needs to be.

Some related discussion in #6710, which is more about the unexpected downstream effects of the # type: ignore.

Thanks a lot for linking to this! It is a difficult problem and I am happy to see you and the maintainers of mypy give it well-deserved attention.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 29, 2020

Now it's possible to use # type: ignore[override], which is more explicit and safer than # type: ignore. Closing this issue on the assumption that this is good enough (better documentation would be useful, however).

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

No branches or pull requests

8 participants