Skip to content

Better error message when __eq__ has unexpected signature #6106

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

Merged
merged 9 commits into from
Jan 3, 2019
5 changes: 4 additions & 1 deletion mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -1476,6 +1476,9 @@ def check_override(self, override: FunctionLike, original: FunctionLike,
# are erased, then it is definitely an incompatibility.

override_ids = override.type_var_ids()
type_name = None
if isinstance(override.definition, FuncDef):
type_name = override.definition.info.name()

def erase_override(t: Type) -> Type:
return erase_typevars(t, ids_to_erase=override_ids)
Expand All @@ -1484,7 +1487,7 @@ def erase_override(t: Type) -> Type:
if not is_subtype(original.arg_types[i],
erase_override(override.arg_types[i])):
self.msg.argument_incompatible_with_supertype(
i + 1, name, name_in_super, supertype, node)
i + 1, name, type_name, name_in_super, supertype, node)
emitted_msg = True

if not is_subtype(erase_override(override.ret_type),
Expand Down
8 changes: 4 additions & 4 deletions mypy/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -405,10 +405,10 @@ def render_messages(self, errors: List[ErrorInfo]) -> List[Tuple[Optional[str],
str, str]]:
"""Translate the messages into a sequence of tuples.

Each tuple is of form (path, line, col, message. The rendered
sequence includes information about error contexts. The path
item may be None. If the line item is negative, the line
number is not defined for the tuple.
Each tuple is of form (path, line, col, severity, message).
The rendered sequence includes information about error contexts.
The path item may be None. If the line item is negative, the
line number is not defined for the tuple.
"""
result = [] # type: List[Tuple[Optional[str], int, int, str, str]]
# (path, line, column, severity, message)
Expand Down
32 changes: 26 additions & 6 deletions mypy/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from collections import OrderedDict
import re
import difflib
from textwrap import dedent

from typing import cast, List, Dict, Any, Sequence, Iterable, Tuple, Set, Optional, Union

Expand Down Expand Up @@ -187,7 +188,7 @@ def report(self, msg: str, context: Optional[Context], severity: str,
if self.disable_count <= 0:
self.errors.report(context.get_line() if context else -1,
context.get_column() if context else -1,
msg.strip(), severity=severity, file=file, offset=offset,
Copy link
Member

Choose a reason for hiding this comment

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

After some thinking I am not sure why do we actually need to strip the message here. How many tests fail if you remove the .strip() here? Maybe we can instead update our test framework?

msg, severity=severity, file=file, offset=offset,
origin_line=origin.get_line() if origin else None)

def fail(self, msg: str, context: Optional[Context], file: Optional[str] = None,
Expand All @@ -198,7 +199,15 @@ def fail(self, msg: str, context: Optional[Context], file: Optional[str] = None,
def note(self, msg: str, context: Context, file: Optional[str] = None,
origin: Optional[Context] = None, offset: int = 0) -> None:
"""Report a note (unless disabled)."""
self.report(msg, context, 'note', file=file, origin=origin, offset=offset)
self.report(msg, context, 'note', file=file, origin=origin,
offset=offset)

def note_multiline(self, messages: str, context: Context, file: Optional[str] = None,
origin: Optional[Context] = None, offset: int = 0) -> None:
"""Report as many notes as lines in the message (unless disabled)."""
for msg in messages.splitlines():
self.report(msg, context, 'note', file=file, origin=origin,
offset=offset)

def warn(self, msg: str, context: Context, file: Optional[str] = None,
origin: Optional[Context] = None) -> None:
Expand Down Expand Up @@ -854,12 +863,25 @@ def signature_incompatible_with_supertype(
name, target), context)

def argument_incompatible_with_supertype(
self, arg_num: int, name: str, name_in_supertype: str,
supertype: str, context: Context) -> None:
self, arg_num: int, name: str, type_name: Optional[str],
name_in_supertype: str, supertype: str, context: Context) -> None:
target = self.override_target(name, name_in_supertype, supertype)
self.fail('Argument {} of "{}" incompatible with {}'
.format(arg_num, name, target), context)

if name == "__eq__" and type_name:
multiline_msg = self.comparison_method_example_msg(class_name=type_name)
self.note_multiline(multiline_msg, context)

def comparison_method_example_msg(self, class_name: str) -> str:
return dedent('''\
It is recommended for "__eq__" to work with arbitrary objects, for example:
def __eq__(self, other: object) -> bool:
if not isinstance(other, {class_name}):
return NotImplemented
return <logic to compare two {class_name} instances>
'''.format(class_name=class_name))

def return_type_incompatible_with_supertype(
self, name: str, name_in_supertype: str, supertype: str,
context: Context) -> None:
Expand Down Expand Up @@ -1110,8 +1132,6 @@ def reveal_locals(self, type_map: Dict[str, Optional[Type]], context: Context) -
# use an ordered dictionary sorted by variable name
sorted_locals = OrderedDict(sorted(type_map.items(), key=lambda t: t[0]))
self.fail("Revealed local types are:", context)
# Note that self.fail does a strip() on the message, so we cannot prepend with spaces
# for indentation
for line in ['{}: {}'.format(k, v) for k, v in sorted_locals.items()]:
self.fail(line, context)

Expand Down
12 changes: 12 additions & 0 deletions test-data/unit/check-classes.test
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,18 @@ class B(A):
main:7: error: Argument 1 of "f" incompatible with supertype "A"
main:9: error: Return type of "h" incompatible with supertype "A"

[case testEqMethodsOverridingWithNonObjects]
class A:
def __eq__(self, other: A) -> bool: pass # Fail
[builtins fixtures/attr.pyi]
[out]
main:2: error: Argument 1 of "__eq__" incompatible with supertype "object"
main:2: note: It is recommended for "__eq__" to work with arbitrary objects, for example:
main:2: note: def __eq__(self, other: object) -> bool:
main:2: note: if not isinstance(other, A):
main:2: note: return NotImplemented
main:2: note: return <logic to compare two A instances>

[case testMethodOverridingWithIncompatibleArgumentCount]
import typing
class A:
Expand Down