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

feat: better message if method incompatible with base class #10572

Merged
merged 18 commits into from Jul 10, 2021

Conversation

atakiar
Copy link
Contributor

@atakiar atakiar commented Jun 1, 2021

Description

Closes #3379. See PR #3410 for some context

Currently if a method is incompatible with a base class, mypy just states the name of the base class but doesn't actually give more details about the expected signature

This PR makes it so that incompatible methods will print out easily-readable signatures, like follows:

error: Signature of "f" incompatible with supertype "A"
note:      Superclass:
note:          def f(self) -> str
note:      Subclass:
note:          def f(self, x: str) -> None

Test Plan

Updated the relevant test cases. No new test cases were added

@atakiar
Copy link
Contributor Author

atakiar commented Jun 1, 2021

A couple interesting observations with the existing behavior of CallableType and mypy/messages.py's pretty_callable (below)

mypy/mypy/messages.py

Lines 1932 to 1949 in 10ea6ad

# If we got a "special arg" (i.e: self, cls, etc...), prepend it to the arg list
if isinstance(tp.definition, FuncDef) and tp.definition.name is not None:
definition_args = tp.definition.arg_names
if definition_args and tp.arg_names != definition_args \
and len(definition_args) > 0:
if s:
s = ', ' + s
s = definition_args[0] + s
s = '{}({})'.format(tp.definition.name, s)
elif tp.name:
first_arg = tp.def_extras.get('first_arg')
if first_arg:
if s:
s = ', ' + s
s = first_arg + s
s = '{}({})'.format(tp.name.split()[0], s) # skip "of Class" part
else:
s = '({})'.format(s)

First, when printing B.f in testInvalidOverrideArgumentCountWithImplicitSignature2 and I.g in testAbstractClassWithAllDynamicTypes, the output of both did not contain "self". Upon some investigation, isinstance(tp.definition, FuncDef) is False and then tp.def_extras.get('first_arg') is None. I'm guessing this might be since both have implicit return types and that might be interacting somehow with CallableType?

[case testInvalidOverrideArgumentCountWithImplicitSignature2]
...
class B:
    def f(self, x, y): pass
...
main:5: note:      Superclass:
main:5: note:          def f(x: Any, y: Any) -> Any
-- expected this instead? def f(self, x: Any, y: Any) -> Any

Second, I had to split testOverloadedMethodSupertype and testFinalBodyReprocessedAndStillFinalOverloaded into an only_when_cache and only_when_nocache version. Similar to what I described above, in the only_when_nocache versions, isinstance(tp.definition, FuncDef) is False. However, in the only_when_cache versions, it is True and "self" is included in the output

Not sure if this is expected behavior or a bug. Let me know what you think!

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@msullivan
Copy link
Collaborator

@atakiar Could you look into some of the mypy_primer errors? I'm a little nervous about all of the missing errors, though it wouldn't shock me if some of them had been wrong?

@atakiar
Copy link
Contributor Author

atakiar commented Jun 17, 2021

@msullivan I think new commits made their way to master between my last merge and when the mypy_primer workflow ran (triggered manually for first-time mypy contributors). Trying this again, hopefully it’ll come out looking better this time!

@github-actions

This comment has been minimized.

@atakiar
Copy link
Contributor Author

atakiar commented Jun 18, 2021

@msullivan let me know what you think!

By the way, the missing mypy_primer errors are the result of #10579. The new multiline note along with the restriction on number of errors shown, results in the last few errors now being hidden

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

aioredis (https://github.com/aio-libs/aioredis.git)
+ aioredis/client.py:4286: note:      Superclass:
+ aioredis/client.py:4286: note:          def (self: Redis) -> Any
+ aioredis/client.py:4286: note:      Subclass:
+ aioredis/client.py:4286: note:          def __aenter__(self) -> Coroutine[Any, Any, Pipeline]

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

OK, this is absolutely fantastic. Thanks so much for this!

@msullivan msullivan merged commit 612d59a into python:master Jul 10, 2021
@atakiar atakiar deleted the better-incompatible-messages branch July 29, 2021 00:20
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.

Better message if method incompatible with base class
2 participants