-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add beginnings of error code support #7267
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this looks really solid.
I have two design questions about using facing behavior we should hammer out answers to.
self.blocker = blocker | ||
self.only_once = only_once | ||
self.origin = origin or (file, line, line) | ||
self.target = target | ||
|
||
|
||
# Type used internally to represent errors: | ||
# (path, line, column, severity, message, code) | ||
ErrorTuple = Tuple[Optional[str], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this a namedtuple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do it, but since we never access the items by index, the benefit would be minor, I think, so I'd rather keep it as it is.
@@ -124,7 +124,7 @@ def ast3_parse(source: Union[str, bytes], filename: str, mode: str, | |||
|
|||
TYPE_COMMENT_SYNTAX_ERROR = 'syntax error in type comment' # type: Final | |||
|
|||
TYPE_IGNORE_PATTERN = re.compile(r'[^#]*#\s*type:\s*ignore\s*($|#)') | |||
TYPE_IGNORE_PATTERN = re.compile(r'[^#]*#\s*type:\s*ignore\s*(\[[^[#]*\]\s*)?($|#)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a design question here of how permissive we want to be about type ignores that don't match our exact format and whether we should interpret them as generic type ignores?
We definitely should have the same behavior between ones reported by typed_ast as type_ignores and ones pulled off of type comments, though.
typed_ast itself is very permissive about what it will accept as type ignores, allowing the ignore to be followed by any ascii non alphanumeric character. The regex that I used in my (not yet merged) pyflakes PR is:
ASCII_NON_ALNUM = ''.join([chr(i) for i in range(128) if not chr(i).isalnum()])
TYPE_IGNORE_RE = re.compile(r'^#\s*type:\s*ignore([{}]|$)'.format(ASCII_NON_ALNUM))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to make this less permissive in another PR. I'm not quite sure what the behavior should be, but likely something different from what we have here.
else: | ||
raise | ||
else: | ||
extra_ignore = TYPE_IGNORE_PATTERN.match(type_comment) is not None | ||
extra_ignore = TYPE_IGNORE_PATTERN.match(type_comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize that typed_ast would include additional comments. That's kind of hinky but probably too late to fix in 3.8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it can be understood as a bug, maybe it can still be fixed? I'm not sure.
SYNTAX = ErrorCode( | ||
'syntax', "Report syntax errors", 'General') # type: Final | ||
|
||
MISC = ErrorCode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One design question is whether we care about a backwards compatibility story as uncategorized errors get moved from misc
to specific categories (assuming we don't categorize everything immediately), breaking type ignores that ignored misc
.
Some options here:
- Don't care, we break backwards compat all the time
- Make
misc
a catch-all group that encompasses either everything else or everything else that used to be included undermisc
- Rename
misc
to something that makes it more clear it is ephermeral, likeuncategorized
- Disallow
# type: ignore[misc]
I'm partial to 3 and maybe also 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal is to categorize all common errors so that the misc code should not be needed that often, making the backwards compatibility issue less of a problem. For example, if error codes could cover ~98% of errors, all the remaining errors in the misc category would each be relatively rare, so changing some of them to non-misc codes should not have a big impact.
@@ -322,6 +345,21 @@ def translate_stmt_list(self, | |||
|
|||
return res | |||
|
|||
def translate_type_comment(self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good refactor 👍
def __init__(self, code: str, description: str, category: str) -> None: | ||
self.code = code | ||
self.description = description | ||
self.category = category |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the plans for category
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea is to use it for grouping the available error codes in the output from --show-error-codes
, once that's implemented. This way error codes defined by plugins, for example, can be shown in a separate section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually meant a separate command such as --list-error-codes
that shows all the available error codes.
I'm going to look at the mypyc crash in CI (other two failures are old semantic analyzer) |
serious: bool = False, *, | ||
serious: bool = False, | ||
*, | ||
code: Optional[ErrorCode] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add the code argument to the versions of this method in SemanticAnalyzerCoreInterface and maybe in TypeAnalyzerPluginInterface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I'd rather do this only after we've removed the old semantic analyzer, since changing this would require changes in it.
The build is finally green. |
This PR adds a foundation for error codes and implements a few error
codes. It also adds support for
# type: ignore[code1, ...]
whichignores only specific error codes on a line.
Only a few errors include interesting error codes at this point. I'll add
support for more error codes in additional PRs. Most errors will implicitly
fall back to a
misc
error code.Error codes are only shown if
--show-error-codes
is used.The error codes look like this in mypy output:
Error codes are intended to be short but human-readable. The name
of an error code refers to the check that produces this error. In the above
example we generate a "no attribute" error when we check whether an
attribute is defined.
Work towards #7239.