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

Better error message when __eq__ has unexpected signature #6106

Merged
merged 9 commits into from Jan 3, 2019

Conversation

Projects
None yet
3 participants
@David-Wobrock
Copy link
Contributor

David-Wobrock commented Dec 26, 2018

Hey there,

This PR tackles an old issue #2055 opened in 2016 - to get started in the mypy source code.

This PR will add a message and an example when the arguments of the dunder method __eq__ do not match the expected signature. With this PR, the added message looks like this:

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 <logic to compare two {class_name} instances>

The 3 commits can and should be reviewed individually since they represent independent bulks of work:

  1. Fix a docstring of errors.render_messages
  2. Add an option so that reporting an error/warning/note eventually doesn't strip the message. This was necessary because the new message needs to preserve indentation.
  3. The core of the work: add the message when one of these methods is used and add a test

Looking forward for some reviews
Cheers,
David

Fixes #2055

David-Wobrock added some commits Dec 26, 2018

Fix docstring of errors.render_messages
The result tuple type was missing a field
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
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thank you for the PR! Here I have some comments and suggestions.

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:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

I would add the new strip_msg argument only where it is actually used, i.e. in notes. It is unlikely that we will have multiline errors or warnings.


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:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

Same here.


def note_multiline(self, messages: str, context: Context, file: Optional[str] = None,
origin: Optional[Context] = None, offset: int = 0,
strip_msg: bool = True) -> None:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

I think strip_msg is not needed for note_multiline. It is quite clear that multiline notes will be often indented.

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,

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

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?

@@ -860,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__"):

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

I think this note should be applied only for __eq__.

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(...):

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

Can we use the actual class name instead here?

class Foo(...):
...
def {method_name}(self, other: object) -> bool:
if not isinstance(other, Foo):

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

...and here

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:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

This "preamble" is too wordy. I would use shorter It is recommended for "__eq__" to work with arbitrary objects, for example: and skip the empty line.

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: ...

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

I would actually skip these two lines (however I would keep the indentation of the next lines).

def {method_name}(self, other: object) -> bool:
if not isinstance(other, Foo):
raise NotImplementedError
return <logic to compare two Foo instances>'''.format(method_name=method_name)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 27, 2018

Collaborator

And here I would also use the actual class name instead of Foo.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Dec 27, 2018

(I also edited the PR description to add the "magic" text for GitHub.)

David-Wobrock added some commits Dec 28, 2018

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

@David-Wobrock David-Wobrock force-pushed the David-Wobrock:master branch from 7ee376e to a630c67 Dec 28, 2018

@David-Wobrock

This comment has been minimized.

Copy link
Contributor Author

David-Wobrock commented Dec 28, 2018

Hey @ilevkivskyi, thanks for the comments.

The PR is open for review again and I took your changes into account in the last 2 commits:

  1. Completely removing the strip() call on the message reporting works like charm. No test broke. So my best guess is to go without it.
  2. Changing the displayed messaged
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks for updates! This is now almost ready, I just have three small comments.

offset=offset)

def note_multiline(self, messages: str, context: Context, file: Optional[str] = None,
origin: Optional[Context] = None, offset: int = 0) -> None:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 30, 2018

Collaborator

Please use the same style as everywhere else: indent for argument coincides with the start of first argument.

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 30, 2018

Collaborator

I think return NotImplemented is more idiomatic, but I don't think this should delay this PR if there are other opinions.

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Dec 31, 2018

Collaborator

Yes, NotImplemented is right here. It signals to the runtime to try the other object's __eq__.

@@ -860,6 +869,20 @@ def argument_incompatible_with_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())

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 30, 2018

Collaborator

Getting anything except line/column from context is a bad idea. First, this kind of defeats the purpose of the type annotation. Second, what if this is an OverloadedFuncDef, or a Decorator?

I think it is better to add an optional argument current_type_name to argument_incompatible_with_supertype, and if name == '__eq__': assert current_type_name is not None etc. (Or maybe even make it a required argument if there are only couple of call sites to update).

David-Wobrock added some commits Dec 30, 2018

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.

@David-Wobrock David-Wobrock force-pushed the David-Wobrock:master branch from d247fde to 9854898 Dec 31, 2018

@David-Wobrock

This comment has been minimized.

Copy link
Contributor Author

David-Wobrock commented Dec 31, 2018

Hi @ilevkivskyi, thanks again for the feedback, and thank you @JelleZijlstra.

Again, the last 3 commits contain the suggested changes.
The messages method argument_incompatible_with_supertype is only called in one place, so that is easy to update. But to get the class/type name, I fall back on a hard-coded Foo string when I don't have access to the function definition.
I don't know the codebase enough to know in what cases this can happen - and if it makes sense in mypy to fallback on such a class name for the note. Do you have a cleaner suggestion for this?

Cheers and happy new year!
David

if isinstance(override.definition, FuncDef):
type_name = override.definition.info.name()
else:
type_name = "Foo"

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jan 3, 2019

Collaborator

I think a better alternative is to not show the note at all if __eq__ is not a function definition. So you can just set type_name to None here, and add a check in argument_incompatible_with_supertype() that will not show the note if the name is None.

@David-Wobrock David-Wobrock force-pushed the David-Wobrock:master branch from 4ba86f6 to b087cdb Jan 3, 2019

@David-Wobrock David-Wobrock force-pushed the David-Wobrock:master branch from b087cdb to 6fc602e Jan 3, 2019

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks, looks ready now!

@ilevkivskyi ilevkivskyi merged commit c33da74 into python:master Jan 3, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment