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

Added signature information to incompatible signature failure. #3410

Closed

Conversation

markkohdev
Copy link

@markkohdev markkohdev commented May 22, 2017

Currently, if a method is incompatible with its base class's method, it only says "Hey, this subclass method is incompatible with its superclass method" which is not very helpful. This branch makes it so that incompatible methods will actually print out readable signatures, arguments, etc. in a human-readable manner.

In order to accomplish this, we've added a new PrettyTypeStrVisitor which will do actual "pretty-printing" that's safe for human eyes. We've also enabled allowing duplicate messages in output.

The new output structure looks like

main:6: error: Signature of "f" incompatible with supertype "A"
main:6: note:     Method "f" of "A":
main:6: note:       def f(self, x: __main__.A) -> None
main:6: note:     Method "f" of "B":
main:6: note:       def f(self, x: __main__.A, y: __main__.A) -> None

Fixes #3379.

mypy/types.py Outdated

if t.definition is not None and t.definition.name() is not None:
# If we got a "special arg" (i.e: self, cls, etc...), prepend it to the arg list
if t.arg_names != t.definition.arg_names and len(t.definition.arg_names) > 0:
Copy link
Author

Choose a reason for hiding this comment

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

I'm trying to get special args like self or cls here, but since they're not in t.arg_names I have to reach through to t.definiton.arg_names and do a comparison. This this the most effective way to do this?

@@ -787,7 +787,11 @@ class D(A):
def f(self, x: str) -> str: return ''
[out]
tmp/foo.pyi:12: error: Signature of "f" incompatible with supertype "A"
tmp/foo.pyi:12: note: Signature of "f" in superclass: "Overload(def f(self, x: builtins.int) -> builtins.int, def f(self, x: builtins.str) -> builtins.str)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nicer to have a multi-line note if you have an overloaded function, and output it like this or something:

...: note: Signature of "f" in superclass:
...: note:     @overload
...: note:     def f(...) -> ...
...: note:     @overload
...: note:     def f(...) -> ...

Copy link
Author

Choose a reason for hiding this comment

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

@JukkaL it would probably be difficult/messy to split the output between multiple note outputs. Would it suffice to have one note output multiple lines?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fine to repeat note multiple times. Some tools might expect the path and the line number to be always present on every line. If there are very many overload items (say, more than 3) we can just show the first 3 and something like (<additional 3 overload variants omitted>).

@markkohdev markkohdev changed the title [WIP] Added signature information to incompatible signature failure. Added signature information to incompatible signature failure. May 24, 2017
@JukkaL
Copy link
Collaborator

JukkaL commented May 24, 2017

Here's my suggestion for how the messages could look like:

rt.py:6: error: Signature of "f" incompatible with superclass "A"
rt.py:6: note:      Superclass:
rt.py:6: note:          def f(self, x: int) -> None
rt.py:6: note:      Subclass:
rt.py:6: note:          def f(self, x: str, y: List[int]) -> None

Notes about the above idea:

  • Superclass: is indented 4 spaces deeper than Signature
  • def f(...) is indented 4 spaces deeper than Superclass:
  • I'd prefer simple formatting (see format_simple in messages.py) for more readable and less verbose type names. This may require moving the conversion code to messages.py, but it may be a good idea anyway. This would display List[int] instead of builtins.list[builtins.int], for example.

Other things:

  • It would be good to include @classmethod and @staticmethod for class and static methods, respectively.

@markkohdev
Copy link
Author

Still working on this. I should have some time this week to move the formatting stuff into its own module so that we can print valid types like List[int] as requested above.

@gvanrossum
Copy link
Member

Hey Mark, Are you still interested in this? This PR has not received any updates in over three months and your last comment suggested you'd like to get to it "sometimes this week". It's fine if you're no longer interested in this -- if we don't hear from you soon we will just close the PR (rather than it accumulating dust in our attic). If after that you decide you want to pick it up still, please just submit a new PR (but mention this one for context).

@markkohdev
Copy link
Author

@gvanrossum Hey Guido, sorry about the radio silence - I got busy adding MyPy to some code bases at work and never got the chance to finish this up. I'll close this PR for now and will try my best to finish this branch in the near future since the issue is still open.

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

Successfully merging this pull request may close these issues.

3 participants