Skip to content

Add code to help filtering warning and errors in mypy's output #2417

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

Closed
sametmax opened this issue Nov 7, 2016 · 9 comments
Closed

Add code to help filtering warning and errors in mypy's output #2417

sametmax opened this issue Nov 7, 2016 · 9 comments

Comments

@sametmax
Copy link

sametmax commented Nov 7, 2016

As mentioned in #2411, it can become difficult to select what you want to see or not in mypy's ouput. This can lead to all-or-nothing behaviors, clutter display/logs, and make CI servers fail on errors we don't wish to check or ignore errors we do want to check.

A possible solution to this is to adopt the behaviour from popular linters such as flake8, pylint, etc.

Each type of error/warning have a code, and you can pass the ones you wish to ignore as a coma separated list using either the command line, a code comment or a config file. Each code is also displayed at the beginning of each respective warning/error in the tool output, so that you can grep for it easily.

E.G:

$ flake8 tests
tests/test_s.py:8:1: F821 undefined name 'test'
tests/test_s.py:10:1: E402 module level import not at top of file
tests/test_s.py:10:10: E261 at least two spaces before inline comment
tests/test_s.py:12:1: E402 module level import not at top of file
tests/test_s.py:14:1: E402 module level import not at top of file
$ flake8 tests --ignore=E402,E402
tests/test_s.py:8:1: F821 undefined name 'test'
tests/test_s.py:10:10: E261 at least two spaces before inline comment

This is a general way of solving the filtering problem:

  • it works with one flag instead of many;
  • it's flexible enough to cover potential future new use cases;
  • with the config file, you can commit it in your VCS and share the checks you wish to apply with your team;
  • it plays well with all configuration mechanisms: env var, config file, cmd options, code comments, etc.
  • it plays well with the command line, allowing grep on specific code, or code type (in the example, F is a failure, E an error, W a warning, etc).
@JukkaL
Copy link
Collaborator

JukkaL commented Jan 16, 2017

Alternatively, we could have a command line option for each warning/error. We already have examples like --strict-optional and --warn-no-return. We could have two options for each error/warning that can be enabled/disabled: --<feature-name> and --no-<feature-name>.

Here are some tradeoffs:

  • An error code is more discoverable if we display it on every message.
  • If we display an error code on every message the messages become more noisy.
  • If we display an error code on every message many test cases need to be updated.
  • An error code by itself is mostly meaningless and the meaning has to be looked up from documentation.
  • A command-line option is easy to recognize and easy to remember if it has a descriptive name.
  • Figuring out the command-line option for a particular error generated by mypy can be hard unless these are documented clearly. However, it wouldn't be hard to have a section in the documentation that shows each option together with examples of typical generated messages, and then searchability would be fine.

@ilevkivskyi
Copy link
Member

Here is just an additional point, error codes can be also used in # type: ignore to selectively silence errors on a given line:

var.meth(other.attr)  # type: ignore E231

@elarivie
Copy link

Hi,

I would like to implement this feature.

Here is my plan:


Step 1 (Internally support error code)

  • Add a "error_code" attribute in the class "mypy/errors.ErrorInfo"
  • Add a positional parameter to the __init__ of ErrorInfo to specify the error_code

Step 2 (Fix compilation error introduced by step 1)

  • By doing so:
    • Create a new file "mypy/errorcode.py" which will contain a constant for each message error code
      Sample of what it would look like:
      ...
      CANNOT_FIND_BUILTINS_MODULE = "200"
      NO_LIBRARY_STUB_FILE_FOR_STANDARD_LIBRARY_MODULE = "201"
      NO_LIBRARY_STUB_FILE_FOR_MODULE = "202"
      CANNOT_FIND_MODULE = "203"
      ...
      At the end this file is probably going to contain few hundreds unique Ids.

Step 3 (Make this feature available)

  • Add a command line argument "--show-error-code" which when present would show the id of each message in the output.
  • It will also prefix the error code by the message severity E(error), W(warning), N(note)
  • Example:
    • without "--show-error-code" (Same as before, stays backward compatible with current output format):
      /a/b/c/d.py:1:2: error: The message
    • with "--show-error-code":
      /a/b/c/d.py:1:2: E345: The message

Step 4

  • Add any required unit tests.
    • Since the output would by default be the same as before, the existing unit tests will probably still be valid and not need to be updated.

PRO:

  • Greatly simplify and speed-up the output parsing done by other tools integrating Mypy (technically allow to create a simple switch case base on the error code)
    • Note: currently tools need to create regex to match the dynamic English messages text
  • Provide an easily searcheable list/count of every distinct possible error type/message
  • Open the door for many other feature
    • Ease documentation (allows to create documentation URL for a specific error code)
    • Allows to easily filter message output (with command line?, through mypy.ini?... or in the meantime simply with grep)

CON:

  • The step 2 above is a long and tedious manual task

Note: This issue is currently tagged "Needs discussion"... so I am opening the discussion, any comment about my above plan?

Note: Implementing all this will take time, I would like to make sure before starting that it would get merge once done.

@ilevkivskyi
Copy link
Member

@elarivie In general I like your plan, but there is one weak point. Currently some error messages are not defined in messages.py, but are scattered across the whole code in some self.fail() and other method calls. If you really want to bring systematic to this, then you need to make a large refactoring, to generate all those error messages in a centralized manner. This is a good and helpful thing to do, but don't underestimate the amount of work (including merge conflicts) this will require.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 24, 2018

To be honest, this will be such a big change that the core team may not have the capacity to review it in the near future, resulting in a long-lived PR that will be a pain to maintain. I think that the only reasonable approach for moving forward with this would be to do something like this:

  • First get approval for an overall design from core devs (we might still decide that the time is not right for this feature).
  • Create a PR that adds error codes to a few messages only, and only enable error codes through a hidden and undocumented command line option. For example, handle one parse error, one semantic analysis error and one type check error.
  • Merge the PR once we are happy with the overall approach and the additional maintenance burden for managing error codes.
  • Gradually create additional PRs that add more error codes.
  • Once all or most error messages (or all useful ones to ignore) have error codes, publicly document the option.

This would require a relatively long-term commitment from you, since reviewing all the changes would likely take a long time.

I actually doubt that it's going to very useful to ignore completely arbitrary errors. Mypy is different from a linter, as most errors aren't style-related but more "actionable". Before moving forward, I'd like to see additional real-world examples where ignoring a specific error would be useful. Here are a few examples which might be reasonable to ignore, but I'd like to see more:

  • Can't find module (this can already be ignored)
  • Need type annotation for variable
  • Function does not return a value (this can already be ignored)
  • Some redefinition related errors (these can make it hard to migrate legacy code)

If there is only a small number of additional errors that are useful to ignore, we can just continue to add new command-line options instead of doing a more major change to errors are categorized.

Additional thoughts:

  • Maybe we'd have some error codes used for multiple related errors to avoid a very large number of error codes (which could be a pain to maintain). We could then later split some of these into more detailed categories if new needs arise. Say, 200 is the top-level category for import-related errors, and initially all import errors would have this code. Later we could add 201, 202, etc. for more specific errors. We'd focus on giving specific codes to errors that there is a common/known reason to ignore.
  • I don't like the idea of having all the error codes defined in a separate place away from error strings, at least if there are many codes. That would make maintenance of one-off error messages kind of painful. I'd prefer to define the error code and message together for simple constant messages. Say, NO_RETURN_VALUE_EXPECTED = Message('E123', 'No return value expected'). Shared error codes could be defined as constants in the same file, but I don't think that we necessarily need a constant for each error code.

@chadrik
Copy link
Contributor

chadrik commented Dec 30, 2018

I'm going to take a shot at this, but I think I will do it as a separate tool at first, so that I can iterate faster, and also to add possibly more controversial ideas like colorized output. But the whole thing will start with a consolidation of error messages into mypy.messages.

An error code is more discoverable if we display it on every message.

agreed.

If we display an error code on every message the messages become more noisy.

As @sametmax proposed, this should be opt-in via a flag like --show-error-codes

If we display an error code on every message many test cases need to be updated.

see above.

An error code by itself is mostly meaningless and the meaning has to be looked up from documentation.

My time with flake8/pycodestyle has engendered a great dislike of inscrutable error codes for exactly this reason. I prefer error keys with somewhat descriptive names, like return_expected.

A command-line option is easy to recognize and easy to remember if it has a descriptive name.

True, but there are a LOT of error types. Adding a --<feature-name> and --no-<feature-name> flag for each of these will turn mypy -h into a wall of text (even moreso than it already is).

My current thinking is to take an approach inspired by pycodestyle and provide --select-errors and --ignore-errors args that take a comma separated list of descriptive code names, as well as a --list-error-codes flag to list them all (and their unformatted message).

With this design, a config file would look like this:

[mypy]
select_errors =
    invalid_syntax,
    not_defined,
    invalid_type_arguments,
    return_expected,
    return_not_expected,
    incompatible_return,
    incompatible_yield,

Alternately, to take an exclusive approach, you could use ignore_errors instead of select_errors.

I'm still evaluating this design, so I may change my mind. There is one big advantage to on/off switches per error vs the select/ignore paradigm, which is that with individual on/off control you can create sparse overrides per module. With the select/ignore design I think you'd have to copy-paste the base config into each per-module section.

@JukkaL Once the error messages are consolidated I'm going to create a tool prototype and use it with my team at work to better answer some of your questions/concerns. Here's how I see us using it:

  1. select a set of errors such that we can get our code passing
  2. choose a new error code and eradicate it
  3. enforce the current state via CI
  4. repeat steps 2 through 4

As I'm sure you're aware, getting to that elusive 0 error status takes a huge amount of effort for a team with a large existing code-base, and mypy can't be used in CI to enforce standards until this is achieved. Fine grained error filtering gives us a gradual and non-disruptive path to adoption, and allows me to amortize the cost of conversion and team education over time.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 2, 2019

@chadrik I like your plan! I didn't consider the workflow of starting with a small set of enabled errors, and gradually enabling more errors. I'd love to hear how this works in practice.

There is one big advantage to on/off switches per error vs the select/ignore paradigm, which is that with individual on/off control you can create sparse overrides per module. With the select/ignore design I think you'd have to copy-paste the base config into each per-module section.

Maybe we can still support per-module configuration together with the select/ignore paradigm. Here's an idea:

  • We keep track of three sets of errors: selected, enabled and ignored errors.
  • By default, all/most errors are selected. No errors are enabled or ignored. Some command line options such as --strict could modify this.
  • select_errors overrides the entire selected errors set, as proposed above. This can only be defined globally.
  • enable_errors incrementally adds to the selected errors set. This can be defined globally or per file.
  • ignore_errors incrementally adds to the set of ignored errors. This can be defined globally or per file.
  • For each file, the active set of errors would be calculated like this (set arithmetic): selected - global_ignored + global_enabled - per_file_ignored + per_file_enabled.

This would also allow us to have some errors disabled by default (not included in the default selected errors set), while providing an easy way to turn them on. We could move many of the existing strictness flags such as --warn-redundant-casts to this new system.

@chadrik
Copy link
Contributor

chadrik commented Jan 2, 2019

@JukkaL I like this plan. I'll try it out in my prototype.

Once #6118 is complete, I'll also follow up with a PR proposing some ideas on how to track error codes through the reporting system.

ilevkivskyi pushed a commit that referenced this issue Jan 4, 2019
…ker) (#6118)

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.
JukkaL pushed a commit that referenced this issue Jan 15, 2019
This is in preparation for message filtering (#2417).

By moving constants to their own module we can easily inspect them to 
get both their name and value, for use within a message filtering system.
@ilevkivskyi
Copy link
Member

Now that #7334 is merged I think we can close this.

(Note there are few follow-up issues tracked separately.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants