Skip to content

Migrate remaining failure messages to mypy.messages (part1: mypy.checker) #6118

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 5 commits into from
Jan 4, 2019

Conversation

chadrik
Copy link
Contributor

@chadrik chadrik commented Dec 30, 2018

Consolidate all mypy error messages into one place (mypy.messages). This opens the door to some interesting features, such as adding language localization or a more systematic way of working with error messages, akin to error codes (#2417). It also helps create more consistent messages. e.g. it's now more apparent that there are a few outliers that use single quotes around type names.

I'm breaking this up change up into several PRs by module, this is the first and simplest. Once all are complete, this will address #6116.

It's pretty straight-foward. The biggest challenge is coming up with good constant names. Let me know what you think.

@chadrik chadrik force-pushed the message_constants_checker branch from d4397e0 to e659c30 Compare December 30, 2018 19:33
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Thanks! I like this change, have just one optional suggestion.

@chadrik
Copy link
Contributor Author

chadrik commented Dec 31, 2018

I added in some of the satellite modules for checker since this PR was a bit too easy :)

mypy/checker.py Outdated
@@ -2543,7 +2543,8 @@ def check_member_assignment(self, instance_type: Type, attribute_type: Type,
[nodes.ARG_POS, nodes.ARG_POS], context)

if not isinstance(inferred_dunder_set_type, CallableType):
self.fail(messages.DESCRIPTOR_SET_NOT_CALLABLE, context)
self.fail(messages.DESCRIPTOR_SET_NOT_CALLABLE
.format(inferred_dunder_set_type), context)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code has two variations of this error message:

  1. "__set__ is not callable" : used here
  2. "{}.__set__ is not callable" : used elsewhere

I'd like to standardize on the second, but I'm not familiar enough with the code to know how to get the descriptor name here. My naive attempt above did not trigger any test errors, but it should have, so I think it's not tested. How should I proceed?

Copy link
Member

Choose a reason for hiding this comment

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

What you are doing is not correct. I think you should use attribute_type.type.name() here, and add a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This particular error is only generated in the case of an inferred __set__. I'm not sure how to trigger this. I tried a few variations along the lines of the tests in check-inference.test but all went through different code paths. Any suggestions? Is there a chance this is a dead code path?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it looks like this is currently dead code: we only allow __set__ to be a method above, and a method (i.e. function definition) can't have a non-callable type. I think this can be replaced with an assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in my latest commit

@@ -2892,19 +2891,19 @@ def check_super_arguments(self, e: SuperExpr) -> None:
if not isinstance(instance_type, (Instance, TupleType)):
# Too tricky, give up.
# TODO: Do something more clever here.
self.chk.fail(messages.UNSUPPORTED_ARGUMENT_2_FOR_SUPER, e)
self.chk.fail(messages.UNSUPPORTED_ARG_2_FOR_SUPER, e)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed this constant because I wanted to standardize all of them on the shorthand "ARG" for argument, since the constants can get fairly lengthy.

mypy/checker.py Outdated
@@ -2543,7 +2543,8 @@ def check_member_assignment(self, instance_type: Type, attribute_type: Type,
[nodes.ARG_POS, nodes.ARG_POS], context)

if not isinstance(inferred_dunder_set_type, CallableType):
self.fail(messages.DESCRIPTOR_SET_NOT_CALLABLE, context)
self.fail(messages.DESCRIPTOR_SET_NOT_CALLABLE
.format(inferred_dunder_set_type), context)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, it looks like this is currently dead code: we only allow __set__ to be a method above, and a method (i.e. function definition) can't have a non-callable type. I think this can be replaced with an assert.

@@ -259,11 +259,11 @@ from typing import TypeVar, Type

T = TypeVar('T', bound=str)
class A:
def foo(self: T) -> T: # E: The erased type of self 'builtins.str' is not a supertype of its class '__main__.A'
def foo(self: T) -> T: # E: The erased type of self "builtins.str" is not a supertype of its class "__main__.A"
Copy link
Member

Choose a reason for hiding this comment

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

Looking at these errors, I think we should switch them to using "short form", as we do for many other errors (in a separate PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@chadrik
Copy link
Contributor Author

chadrik commented Jan 4, 2019

I think this should be ready to go.

@ilevkivskyi ilevkivskyi merged commit d5e0f3f into python:master Jan 4, 2019
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