From cf81474884f41b1ef5208dd7376259e6ffeed754 Mon Sep 17 00:00:00 2001 From: David-Wobrock Date: Wed, 26 Dec 2018 12:57:53 +0100 Subject: [PATCH 1/9] Fix docstring of errors.render_messages The result tuple type was missing a field --- mypy/errors.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mypy/errors.py b/mypy/errors.py index b5982aaf5be5..a177b5d6805a 100644 --- a/mypy/errors.py +++ b/mypy/errors.py @@ -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) From 80bc5eb06b77a60124d9129747e5530c47cd4116 Mon Sep 17 00:00:00 2001 From: David-Wobrock Date: Wed, 26 Dec 2018 17:16:42 +0100 Subject: [PATCH 2/9] Make it optional to strip reported message --- mypy/messages.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/mypy/messages.py b/mypy/messages.py index 0bb754501a68..b4fecb745d86 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -182,28 +182,31 @@ def is_errors(self) -> bool: def report(self, msg: str, context: Optional[Context], severity: str, file: Optional[str] = None, origin: Optional[Context] = None, - offset: int = 0) -> None: + offset: int = 0, strip_msg: bool = True) -> None: """Report an error or note (unless disabled).""" if self.disable_count <= 0: + msg = msg.strip() if strip_msg else msg 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, + 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, - origin: Optional[Context] = None) -> None: + origin: Optional[Context] = None, strip_msg: bool = True) -> None: """Report an error message (unless disabled).""" - self.report(msg, context, 'error', file=file, origin=origin) + self.report(msg, context, 'error', file=file, origin=origin, strip_msg=strip_msg) def note(self, msg: str, context: Context, file: Optional[str] = None, - origin: Optional[Context] = None, offset: int = 0) -> None: + origin: Optional[Context] = None, offset: int = 0, + strip_msg: bool = True) -> 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, strip_msg=strip_msg) def warn(self, msg: str, context: Context, file: Optional[str] = None, - origin: Optional[Context] = None) -> None: + origin: Optional[Context] = None, strip_msg: bool = True) -> None: """Report a warning message (unless disabled).""" - self.report(msg, context, 'warning', file=file, origin=origin) + self.report(msg, context, 'warning', file=file, origin=origin, strip_msg=strip_msg) def quote_type_string(self, type_string: str) -> str: """Quotes a type representation for use in messages.""" From d524a45f001da33c6f06b6879bbcd527bd5dc26f Mon Sep 17 00:00:00 2001 From: David-Wobrock Date: Wed, 26 Dec 2018 17:50:38 +0100 Subject: [PATCH 3/9] Improve error message for unexpected __eq__ and __ne__ dunder methods signatures. When the 'other' argument is not an 'object' type, we show a better error message with an example code. * And add a test --- mypy/messages.py | 23 +++++++++++++++++++++++ test-data/unit/check-classes.test | 27 +++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/mypy/messages.py b/mypy/messages.py index b4fecb745d86..b3c74e1c22fd 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -203,6 +203,14 @@ def note(self, msg: str, context: Context, file: Optional[str] = None, self.report(msg, context, 'note', file=file, origin=origin, offset=offset, strip_msg=strip_msg) + def note_multiline(self, messages: str, context: Context, file: Optional[str] = None, + origin: Optional[Context] = None, offset: int = 0, + strip_msg: bool = True) -> 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, strip_msg=strip_msg) + def warn(self, msg: str, context: Context, file: Optional[str] = None, origin: Optional[Context] = None, strip_msg: bool = True) -> None: """Report a warning message (unless disabled).""" @@ -863,6 +871,21 @@ def argument_incompatible_with_supertype( self.fail('Argument {} of "{}" incompatible with {}' .format(arg_num, name, target), context) + if name in ("__eq__", "__ne__"): + multiline_msg = self.comparison_method_example_msg(name) + self.note_multiline(multiline_msg, context, strip_msg=False) + + def comparison_method_example_msg(self, method_name: str) -> str: + return '''It is recommended for "{method_name}" to work with arbitrary objects. +The snippet below shows an example of how you can implement "{method_name}": + +class Foo(...): + ... + def {method_name}(self, other: object) -> bool: + if not isinstance(other, Foo): + raise NotImplementedError + return '''.format(method_name=method_name) + def return_type_incompatible_with_supertype( self, name: str, name_in_supertype: str, supertype: str, context: Context) -> None: diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index e97ce38ae4b0..a7dd46173c8b 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -282,6 +282,33 @@ 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 testEqNeMethodsOverridingWithNonObjects] +class A: + def __eq__(self, other: A) -> bool: pass # Fail + def __ne__(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. +main:2: note: The snippet below shows an example of how you can implement "__eq__": +main:2: note: +main:2: note: class Foo(...): +main:2: note: ... +main:2: note: def __eq__(self, other: object) -> bool: +main:2: note: if not isinstance(other, Foo): +main:2: note: raise NotImplementedError +main:2: note: return +main:3: error: Argument 1 of "__ne__" incompatible with supertype "object" +main:3: note: It is recommended for "__ne__" to work with arbitrary objects. +main:3: note: The snippet below shows an example of how you can implement "__ne__": +main:3: note: +main:3: note: class Foo(...): +main:3: note: ... +main:3: note: def __ne__(self, other: object) -> bool: +main:3: note: if not isinstance(other, Foo): +main:3: note: raise NotImplementedError +main:3: note: return + [case testMethodOverridingWithIncompatibleArgumentCount] import typing class A: From 16178b2b68a2382a3888d4a1865d1751a39dc354 Mon Sep 17 00:00:00 2001 From: David-Wobrock Date: Fri, 28 Dec 2018 13:21:03 +0100 Subject: [PATCH 4/9] Completely remove striping the message before reporting it * Delete outdated comment --- mypy/messages.py | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/mypy/messages.py b/mypy/messages.py index b3c74e1c22fd..7256e07df1b1 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -182,39 +182,36 @@ def is_errors(self) -> bool: def report(self, msg: str, context: Optional[Context], severity: str, file: Optional[str] = None, origin: Optional[Context] = None, - offset: int = 0, strip_msg: bool = True) -> None: + offset: int = 0) -> None: """Report an error or note (unless disabled).""" if self.disable_count <= 0: - msg = msg.strip() if strip_msg else msg self.errors.report(context.get_line() if context else -1, context.get_column() if context else -1, 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, - origin: Optional[Context] = None, strip_msg: bool = True) -> None: + origin: Optional[Context] = None) -> None: """Report an error message (unless disabled).""" - self.report(msg, context, 'error', file=file, origin=origin, strip_msg=strip_msg) + self.report(msg, context, 'error', file=file, origin=origin) def note(self, msg: str, context: Context, file: Optional[str] = None, - origin: Optional[Context] = None, offset: int = 0, - strip_msg: bool = True) -> 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, strip_msg=strip_msg) + offset=offset) def note_multiline(self, messages: str, context: Context, file: Optional[str] = None, - origin: Optional[Context] = None, offset: int = 0, - strip_msg: bool = True) -> 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, strip_msg=strip_msg) + offset=offset) def warn(self, msg: str, context: Context, file: Optional[str] = None, - origin: Optional[Context] = None, strip_msg: bool = True) -> None: + origin: Optional[Context] = None) -> None: """Report a warning message (unless disabled).""" - self.report(msg, context, 'warning', file=file, origin=origin, strip_msg=strip_msg) + self.report(msg, context, 'warning', file=file, origin=origin) def quote_type_string(self, type_string: str) -> str: """Quotes a type representation for use in messages.""" @@ -873,7 +870,7 @@ def argument_incompatible_with_supertype( if name in ("__eq__", "__ne__"): multiline_msg = self.comparison_method_example_msg(name) - self.note_multiline(multiline_msg, context, strip_msg=False) + self.note_multiline(multiline_msg, context) def comparison_method_example_msg(self, method_name: str) -> str: return '''It is recommended for "{method_name}" to work with arbitrary objects. @@ -1136,8 +1133,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) From a630c67e32aa266aa6e026ec0e48d95f7825969d Mon Sep 17 00:00:00 2001 From: David-Wobrock Date: Fri, 28 Dec 2018 13:56:24 +0100 Subject: [PATCH 5/9] Adapt __eq__ note: * Use textwrap.dedent to have a nicer multiline string * Only show the note for __eq__ * Use the actual class name in the shown code snippet * Adapt the test --- mypy/messages.py | 24 ++++++++++++------------ test-data/unit/check-classes.test | 23 ++++------------------- 2 files changed, 16 insertions(+), 31 deletions(-) diff --git a/mypy/messages.py b/mypy/messages.py index 7256e07df1b1..ede47e4c34d4 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -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 @@ -868,20 +869,19 @@ def argument_incompatible_with_supertype( self.fail('Argument {} of "{}" incompatible with {}' .format(arg_num, name, target), context) - if name in ("__eq__", "__ne__"): - multiline_msg = self.comparison_method_example_msg(name) + if name == "__eq__": + assert isinstance(context, FuncDef) + multiline_msg = self.comparison_method_example_msg(class_name=context.info.name()) self.note_multiline(multiline_msg, context) - def comparison_method_example_msg(self, method_name: str) -> str: - return '''It is recommended for "{method_name}" to work with arbitrary objects. -The snippet below shows an example of how you can implement "{method_name}": - -class Foo(...): - ... - def {method_name}(self, other: object) -> bool: - if not isinstance(other, Foo): - raise NotImplementedError - return '''.format(method_name=method_name) + 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}): + raise NotImplementedError + return + '''.format(class_name=class_name)) def return_type_incompatible_with_supertype( self, name: str, name_in_supertype: str, supertype: str, diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index a7dd46173c8b..0d59537faf1f 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -282,32 +282,17 @@ 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 testEqNeMethodsOverridingWithNonObjects] +[case testEqMethodsOverridingWithNonObjects] class A: def __eq__(self, other: A) -> bool: pass # Fail - def __ne__(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. -main:2: note: The snippet below shows an example of how you can implement "__eq__": -main:2: note: -main:2: note: class Foo(...): -main:2: note: ... +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, Foo): +main:2: note: if not isinstance(other, A): main:2: note: raise NotImplementedError -main:2: note: return -main:3: error: Argument 1 of "__ne__" incompatible with supertype "object" -main:3: note: It is recommended for "__ne__" to work with arbitrary objects. -main:3: note: The snippet below shows an example of how you can implement "__ne__": -main:3: note: -main:3: note: class Foo(...): -main:3: note: ... -main:3: note: def __ne__(self, other: object) -> bool: -main:3: note: if not isinstance(other, Foo): -main:3: note: raise NotImplementedError -main:3: note: return +main:2: note: return [case testMethodOverridingWithIncompatibleArgumentCount] import typing From a09fdc4703bcc43e3113b239201691e9771b85ee Mon Sep 17 00:00:00 2001 From: David-Wobrock Date: Sun, 30 Dec 2018 22:33:23 +0100 Subject: [PATCH 6/9] Use return NotImplemented instead of raise NotImplError --- mypy/messages.py | 2 +- test-data/unit/check-classes.test | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mypy/messages.py b/mypy/messages.py index ede47e4c34d4..668e399e2941 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -879,7 +879,7 @@ def comparison_method_example_msg(self, class_name: str) -> str: It is recommended for "__eq__" to work with arbitrary objects, for example: def __eq__(self, other: object) -> bool: if not isinstance(other, {class_name}): - raise NotImplementedError + return NotImplemented return '''.format(class_name=class_name)) diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index 0d59537faf1f..cfd0f634be36 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -291,7 +291,7 @@ 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: raise NotImplementedError +main:2: note: return NotImplemented main:2: note: return [case testMethodOverridingWithIncompatibleArgumentCount] From 15f1d06ce83cb7e8ec44d5c0e7cfe33230aecacf Mon Sep 17 00:00:00 2001 From: David-Wobrock Date: Sun, 30 Dec 2018 22:34:20 +0100 Subject: [PATCH 7/9] Start argument on same indent as first arg --- mypy/messages.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mypy/messages.py b/mypy/messages.py index 668e399e2941..6560493ac2c6 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -203,7 +203,7 @@ def note(self, msg: str, context: Context, file: Optional[str] = None, offset=offset) def note_multiline(self, messages: str, context: Context, file: Optional[str] = None, - origin: Optional[Context] = None, offset: int = 0) -> 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, From 9854898fb83231e26849270679ab8a204f902309 Mon Sep 17 00:00:00 2001 From: David-Wobrock Date: Mon, 31 Dec 2018 18:09:34 +0100 Subject: [PATCH 8/9] Pass the type name to the messages method Instead of inspecting the context, we directly pass the type name to the messages methods as argument. --- mypy/checker.py | 6 +++++- mypy/messages.py | 7 +++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index b88278d3d0ba..418a03b5d9cb 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -1476,6 +1476,10 @@ def check_override(self, override: FunctionLike, original: FunctionLike, # are erased, then it is definitely an incompatibility. override_ids = override.type_var_ids() + if isinstance(override.definition, FuncDef): + type_name = override.definition.info.name() + else: + type_name = "Foo" def erase_override(t: Type) -> Type: return erase_typevars(t, ids_to_erase=override_ids) @@ -1484,7 +1488,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), diff --git a/mypy/messages.py b/mypy/messages.py index 6560493ac2c6..cbf455999511 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -863,15 +863,14 @@ 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: 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__": - assert isinstance(context, FuncDef) - multiline_msg = self.comparison_method_example_msg(class_name=context.info.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: From 6fc602e90987f7406cc3e84bf43ca88e4f16c6e5 Mon Sep 17 00:00:00 2001 From: David-Wobrock Date: Thu, 3 Jan 2019 20:06:38 +0100 Subject: [PATCH 9/9] Do not display __eq__ note when not function definition --- mypy/checker.py | 3 +-- mypy/messages.py | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 418a03b5d9cb..da36a425908b 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -1476,10 +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() - else: - type_name = "Foo" def erase_override(t: Type) -> Type: return erase_typevars(t, ids_to_erase=override_ids) diff --git a/mypy/messages.py b/mypy/messages.py index cbf455999511..c4f174d526f3 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -863,13 +863,13 @@ def signature_incompatible_with_supertype( name, target), context) def argument_incompatible_with_supertype( - self, arg_num: int, name: str, type_name: str, + 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__": + if name == "__eq__" and type_name: multiline_msg = self.comparison_method_example_msg(class_name=type_name) self.note_multiline(multiline_msg, context)