Skip to content

Add a --warn-unused-strictness-exceptions flag #4018

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

Open
gvanrossum opened this issue Sep 27, 2017 · 5 comments
Open

Add a --warn-unused-strictness-exceptions flag #4018

gvanrossum opened this issue Sep 27, 2017 · 5 comments
Assignees
Labels
feature topic-configuration Configuration files and flags

Comments

@gvanrossum
Copy link
Member

In an ideal world all code passes with mypy’s most strict flag settings. Sadly most strictness flags have many violations in the current codebase. We want all new files to be checked with the most strict flag settings, while allowing existing files to still have violations. This can be done with a blacklist. But we want the set of files with violations (i.e. the blacklist) to gradually decrease. Therefore we want a “ratchet” in place where once a file stops having violations it is removed from the blacklist (so from then on it will remain clean). And we want a separate blacklist per strictness flag.

At Dropbox, for strict_optional we’ve got a ratchet in place, but it’s expensive — it uses a separate mypy build and some scripts. Here’s how it works:

  • In our mypy.ini the main (default) section has strict_optional = True
  • The ground truth for the blacklist is file-specific exceptions in mypy.ini
  • A separate CI build modifies the mypy.ini to erase the file-specific exceptions, then collects errors from mypy, and emits an error only for files that have an exception but no mypy errors
  • Users must make this CI build pass before they can land their change, which they do by removing the exceptions from mypy.ini for files that are now clear
  • Users are discouraged to add new exceptions to mypy.ini

We'd like to create a similar ratchet for disallow_any = generics, because it masks type checking for cases where people should have written Future[X] (for some type X) but accidentally write Future, which is interpreted as Future[Any]. But this flag has many other violations in existing code (too many to just fix before we turn on the flag), so we need a blacklist, and we want it to be a ratchet.

The simplest solution is to just have another CI build that does the same as what we do for strict_optional but for disallow_any. However if we consider a future with many different strictness flags, it would be nice to have a solution that doesn’t require another CI build per flag.

For flags like disallow_any = generics there is actually a better solution possible. We can make a small change to mypy that tracks two sets of files while it is analyzing the code: the set of files for which the flag is disabled (i.e. the blacklist), and the set of files for which an error would have been issued if the flag were enabled (but wasn’t, because the flag was disabled). At the end of the analysis we subtract the latter set from the former, and the remainder is the list of files that don’t need the flag to be disabled.

There is already a similar feature in mypy, --warn-unused-configs, which does a similar thing for unused config sections (a more serious offense, where a section in mypy.ini references a file that doesn’t exist, or at least isn’t analyzed).

The proposed new feature could take the form of a single new flag, e.g. --warn-unused-strictness-exceptions. It would, in the way sketched above, track exceptions for each strictness flag that is (a) off by mypy default, (b) selectable on a per-file basis, and (c) easily trackable. By the latter condition I mean that there is a small number of places in mypy where an error is generated only if the given strictness flag is enabled. Tracking would only be done for flags that are enabled in the main (default) section of mypy.ini.

I believe that at least the various disallow_any flags added by @ilinum are all easily trackable; some others (e.g. disallow_untyped_calls) are also in this category. Some other strictness flags are not easily trackable, e.g. strict_optional or disallow_unchecked_defs can cause inferred types to change, and then it’s not easy to correlate errors emitted with the value of the flag.

@ilevkivskyi
Copy link
Member

I like the idea. But I have two questions:

  • Why would we need to enable --warn-unused-strictness-exceptions on a per file basis? Also it is not clear what would happen if the set of files with --warn-unused-strictness-exceptions is partially overlapping with set of exceptions to other flags.
  • Maybe we need this on per flag basis, so that the ratchet can be turned on for disallow_any=generic but not for disallow-any=unimported?

@gvanrossum
Copy link
Member Author

Why would we need to enable --warn-unused-strictness-exceptions on a per file basis?

We wouldn't -- that wasn't my intention. Where did you read that in the proposal?

Maybe we need this on per flag basis [...]

Maybe, though I think that would just complicate the flag syntax, and I don't see a great use case. The implementation does need to keep track of "strictness flag usage" on a per-strictness-flag basis, so if we ended up needing a per-flag ratchet it wouldn't change the implementation much.

I envision an implementation similar to --warn-unused-configs, which initially fills a dict with an item for each per-module config entry, removes the item (if present) when it is used, and at the end prints the remaining items. We would have a separate dict for each strictness flag supported but only if it's turned on explicitly in the main [mypy] section.

@ilevkivskyi
Copy link
Member

We wouldn't -- that wasn't my intention. Where did you read that in the proposal?

Ah OK, sorry I just misunderstood (b) in your (a), (b), (c) list.

@gvanrossum
Copy link
Member Author

(a), (b) and (c) are all meant to qualify the strictness flags that are supported. E.g. --strict-optional is ruled out by (c). warn_no_return is disqualified by (a). --warn-unused-ignores doesn't satisfy (b).

@ilevkivskyi
Copy link
Member

(a), (b) and (c) are all meant to qualify the strictness flags that are supported. E.g. --strict-optional is ruled out by (c). warn_no_return is disqualified by (a). --warn-unused-ignores doesn't satisfy (b).

Yes, thanks, this makes perfect sense.

@ilinum ilinum self-assigned this Nov 5, 2017
ilinum added a commit to ilinum/mypy that referenced this issue Nov 8, 2017
Warn about strictness flags unnecessarily disabled for particular modules.
If a flag is enabled globally (either explicitly or by default) and it is
disabled for a particular module that does not have errors related to this
flag mypy will output an error.

Currently, this is implemented only for --disallow-any-generics but it should be
very straightforward to support other strictness flags.

Fixes python#4018
ilinum added a commit to ilinum/mypy that referenced this issue Mar 21, 2018
Warn about strictness flags unnecessarily disabled for particular modules.
If a flag is enabled globally (either explicitly or by default) and it is
disabled for a particular module that does not have errors related to this
flag mypy will output an error.

Currently, this is implemented only for --disallow-any-generics but it should be
very straightforward to support other strictness flags.

Fixes python#4018
@AlexWaygood AlexWaygood added the topic-configuration Configuration files and flags label Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature topic-configuration Configuration files and flags
Projects
None yet
Development

No branches or pull requests

4 participants