Add error codes to many additional messages #7334
Conversation
mypy/errorcodes.py
Outdated
| OPERATOR = ErrorCode( | ||
| 'operator', "Check that operator is valid for operands", 'General') # type: Final | ||
| LIST_ITEM = ErrorCode( | ||
| 'list-item', "Check list items in [item, ...]", 'General') # type: Final |
There was a problem hiding this comment.
This is a bit too terse, I would phrase this as:
| 'list-item', "Check list items in [item, ...]", 'General') # type: Final | |
| 'list-item', "Check items in a list literal [item, ...]", 'General') # type: Final |
mypy/errorcodes.py
Outdated
| LIST_ITEM = ErrorCode( | ||
| 'list-item', "Check list items in [item, ...]", 'General') # type: Final | ||
| DICT_ITEM = ErrorCode( | ||
| 'dict-item', "Check dict items in {key: value, ...}", 'General') # type: Final |
There was a problem hiding this comment.
Probably also here:
| 'dict-item', "Check dict items in {key: value, ...}", 'General') # type: Final | |
| 'dict-item', "Check items in a dict literal {key: value, ...}", 'General') # type: Final |
mypy/errorcodes.py
Outdated
| 'redundant-cast', "Check that cast changes type of expression", 'General') # type: Final | ||
| COMPARISON_OVERLAP = ErrorCode( | ||
| 'comparison-overlap', | ||
| "Check that types in comparisons have overlap", 'General') # type: Final |
There was a problem hiding this comment.
Also membership checks? I mean:
x: int
lst: List[str]
x in lst # Error here: non-overlapping types| 'abstract', "Prevent instantiation of classes with abstract attributes", | ||
| 'General') # type: Final | ||
| VALID_NEWTYPE = ErrorCode( | ||
| 'valid-newtype', "Check that argument 2 to NewType is valid", 'General') # type: Final |
There was a problem hiding this comment.
Should we have the same code for all ill-defined NewTypes not just specifically second argument?
There was a problem hiding this comment.
This error seems to be much more common than any other NewType-related errors, so I think it's worth having an error code for specifically this case.
| VALID_NEWTYPE = ErrorCode( | ||
| 'valid-newtype', "Check that argument 2 to NewType is valid", 'General') # type: Final | ||
|
|
||
| NO_UNTYPED_DEF = ErrorCode( |
There was a problem hiding this comment.
I am just curious why the below block is separated with an empty line. Maybe add a comment?
There was a problem hiding this comment.
Ok. These errors aren't enabled by default.
| else: | ||
| self.fail('No overload variant{} matches argument types {}' | ||
| .format(name_str, arg_types_str), context) | ||
| .format(name_str, arg_types_str), context, code=codes.CALL_OVERLOAD) |
There was a problem hiding this comment.
I remember there was an idea to add a utility decorator (implemented as a simple context manager) that would set default for every fail() call within a method. For example:
@with_error_code(code.CALL_OVERLOAD)
def no_variant_matches_arguments(...):
# No need to repeat code.CALL_OVERLOAD in every call here.
self.fail(msg, context)
# A code explicitly passed naturally takes precedence.
self.fail(msg, context, code=codes.MISC)It is not very important, but will be quite handy.
There was a problem hiding this comment.
This would need some magic or global state and I'd rather avoid both. It can also be error-prone in case we call other functions that can call fail. I think that explicit is better than implicit here.
There was a problem hiding this comment.
It is debatable what is more error-prone: having some state for error reporting (we actually already have some in e.g. SemanticAnalyzer.file_context() etc) or having to repeat codes many times. This is however not important to delay this, we can easily add this later if someone likes the idea.
| else: | ||
| self.fail('{} not callable'.format(format_type(original_type)), context) | ||
| self.fail('{} not callable'.format(format_type(original_type)), context, | ||
| code=codes.OPERATOR) |
There was a problem hiding this comment.
This make me think about how we will evolve the error codes. Imagine we will decide next year that codes.OPERATOR is too general, but some people have a hundred of type: ignore[operator]. If we will introduce two new error codes instead, the existing ignores will stop working.
Do we have a mechanism to allow this? For example sub-codes like operator:arithmetic or allowing aliases (to rename codes).
There was a problem hiding this comment.
I don't expect that we'll need to split the error codes much further. I have some ideas but I'd want to make most of the changes before the next release. If we rename error codes, it will be easy enough to add support for aliases in the future.
There was a problem hiding this comment.
OK, we would also need to add docs before the next release.
|
Great to see this land. Why not show error codes by default? (Still with an option to turn them off.) It seems an important new feature that we ought to advertise, and I doubt that there are a lot of people with setups that break when details of error messages change (we've changed many messages in the past). |
If we document everything and implement related features like |
|
Agreed that those make this more useful. But the feature is usable right now in '# type: ignore' comments. Selectively disabling and listing the codes are not required for that. (It's the other way around, this PR is required to start implementing those features.) |
This adds error codes to the most common messages seen at Dropbox
plus a few others that seem useful.
There are still some others I'm planning to add, but this should cover
the essentials.