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

Feature/ignore errors by regex #8108

Closed

Conversation

Netzeband
Copy link
Contributor

@Netzeband Netzeband commented Dec 8, 2019

Hello

Please review the following pull-request and consider to merge it to your code base.
I'm happy about any feedback.

Intention

In my current project, I use a lot of decorated properties. This is not supported by mypy so far and lead to a huge amount of errors of type "Decorated property not supported". However, this is not a quality issue of my code, but just a missing feature of mypy. Thus I don't want to add hundreds of # type: ignore statements.

I decided to add an option to mypy to ignore errors messages by regex patterns.
The error above can then globally disabled by having a mypy.ini like this:

[mypy]
ignore_errors_by_regex = Decorated property not supported

It is also possible to add several patterns, which should be ignored. For example:

[mypy]
ignore_errors_by_regex = 
    Decorated property not supported
    Overloaded method has both abstract and non-abstract variants

Of course I added a small description of this option to the documentation.

Implementation

One part of the implementation is the definition of the config argument ignore_errors_by_regex. It is added to the file options.py. The type of this option is List[str]. In config_parser.py every line of the corresponding value is split into a separated string and added to the list. Empty strings are ignored in order to allow a more clean way of writing several patterns (see example above).

The attribute option.ignore_errors_by_regex is then passed to different instances of the class Errors. Here I was confused, since the class Errors is instantiated on many places and very often it does not get a single attribute, even when an Options object exists and it would be easy to pass the options to the newly created Errors instance. To be on the safe side, I passed option.ignore_errors_by_regex everywhere, where it was possible. To achieve this, I had sometime reverse the order of instantiation of the Options and Errors objects. If you don't want it like this, please tell me and highlight those instantiations of Errors which are important. For all others, I will undo my change, so that in those cases no arguments are passed to the init-method of Errors.

Inside the Errors init-method, the list of strings is translated to a list of compiled regex-objects. But only in case the list was defined and not None. Furthermore I had to store the original list, since it is needed to copy the Errors object inside the copy-method.

The core implementation of the ignore mechanism is another check inside the method Errors.is_ignored_error(...). Here I try to match the error-message against all compiled patterns. If one matches, the error is ignored. For better readability, I put this looping into another method.

There are some cases, where an error leads to more than just one message. For example if a function from another module is called with wrong arguments. In this case there is an error-message at the line of the wrong call and a note at the line of the function-definition. Luckily we already track all lines, which have been ignored so far and since the scope line of the notification also points to the line of the ignored message, it is easy to decide, if such a note should be ignored as well. This is done by the second if-condition, I added to Errors.is_ignored_error(...), where all notes are ignored which point to a line, which was ignored before. I decided here to only ignore notes in those cases to avoid situation, where two different errors appear on the same line and only one of them should be ignored. I added some unit-tests to check the note-ignore behavior (positive- and negative tests).

Testing

I added the test-file test-data/unit/check-ignore-by-regex.test, which contain 27 tests for this new feature. Most of the tests where adapted copies from test-data/unit/check-ignore.test but there are also some new tests like test, which checks if multiple errors on the same line still work, if one of those errors are ignored. I also added a test, if the property & decorator error can be ignored, since this was my original intention to implement this feature here.

Some more words

I'm not an expert for the internals of mypy. But if there already exists a concept which fixes the two issues with properties (properties + decorators and abstract properties with setters), I would be happy if you can share it with me. Maybe I can help implementing it?

Thanks for your time and for considering merging this pull-request.
Best regards,
André

@ilevkivskyi
Copy link
Member

Yesterday I submitted a similar PR #8102

TBH I am not sure I like the idea of using the regex based approach mainly for two reasons:

  • There is literally zero backwards compatibility guarantee in error messages, they can change completely at any moment. The error codes are however much more stable.
  • The regex approach can be easily emulated by just mypy src | grep -v "Decorated properties not supported".

@Netzeband
Copy link
Contributor Author

Hello

Thanks for your feedback. I already saw PR #8102 but it is very different to this change here. One problem with the "error-codes" here is, that they are actually not error-codes like those in lint, but they are error-categories. For example the error I need to suppress is Decorated property not supported [misc]. If I would suppress "misc" with your code change, I would also suppress the error message The type alias is invalid in runtime context [misc]. Both are completely unrelated to each other and while the first one is just a missing feature of mypy, the second could be a serious issue in my code.

If mypy would have unique error-codes for every single error, then of course suppressing by code would be much better than suppressing by regex.

About your concerns:

  1. Yes the missing backward compatibility is an issue here. However in order to solve this, mypy would need unique error-codes. Introducing such error-codes would first need a clear design decision by you and the other maintainers, because this would for sure be a change, which breaks the backward compatibility. And second one would need to touch almost every file in this repository. So it would be a big code change, while this PR is fairly isolated. If you decide to go in this direction, I would be happy to help you.

  2. Suppressing strings as a post-procesing step is not the same. Because:

  • It only works on linux-like systems.
  • It only suppresses on line level. When an error-message is related to different lines (some messages, give context information) it does not work anymore
  • It only suppresses the original line of the error, but not notifications, which are related to that (while my change would do this)
  • It only works if mypy is running as a standalone step. When running mypy from PyCharm or pytest environment (pytest-mypy), it does not work anymore
  • It would introduce difficulties to interpret the return code of mypy correctly (which is important for CI pipelines)

Another alternative to my code-change would be to allow plugins to read config options and to suppress error-messages. Then I would write a suppression plugin instead of a PR. So if you don't like to have such a feature directly in mypy, I would be happy to help you extending the plugin system in order to allow plugins to do this job. What do you think?

Best regards,
André

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 9, 2019

I agree with Ivan, and I'd go even further and say that a (built-in) regexp-based approach isn't feasible, since error messages are not going to be stable. Error codes are the way go. We can't add an error code to every single message, but we can create new error codes for various errors under 'misc' (at least until 1.0). This breaks backwards compatibility, but at least we'll be trending towards a stable error categorization. My hope is that soon enough all the errors that users would like to ignore regularly will have their individual error codes.

Plugin-based error filtering might be an acceptable alternative. It would best to open an issue first to discuss it.

@ilevkivskyi
Copy link
Member

Note there is an existing issue/proposal for silencing errors in plugins, but it is also mostly error-code based #7468. I could however imagine that plugin may do some internal "magic" using regexp and then use public error code API to silence a given error.

@JelleZijlstra
Copy link
Member

For what it's worth, we use a wrapper around mypy that silences a lot of errors based on regexes; this is for a large old codebase that we haven't yet had time to make fully mypy-clean. Error message changes from new releases haven't been much of an issue for us—we always have to make some minor changes around a mypy release, and in practice error messages don't change all that much.

@Netzeband
Copy link
Contributor Author

@ilevkivskyi @JukkaL Thanks for your feedback. I can understand your concerns. Then maybe the plugin based error suppression would be a better approach. I will start a conversation about that in the issue #7468 and close this PR.

Thanks and best regards,
André

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.

4 participants