Add an option to require ignore comments have error codes

[PeterJCLaw]

Description

Fixes #11509

Pitch # type:ignore comments risk suppressing an unintended error, which is why it's preferred to specify the exact error code you're suppressing

This introduces a new error code ignore-without-code which achieves that. I've gone with naming this warn- rather than disallow- since the ignores are still obeyed, just alerted about (and also as the most closely related options are also warn- style). This is disabled by default but can be controlled via the --enable-error-code machinery.

This new option is aware of --warn-unused-ignores (since they're likely to be used together) such that we don't emit warnings about a lack of error codes if the ignore is unused and that option is turned on.

One thing I'm not sure about is what should happen for ignore comments that apply to a whole file. I've raised this on that issue, so discussion should probably happen there. Currently the new option obeys that instruction to ignore the file.

Test Plan

Some manual testing, plus automated tests added.

Code Review

I'm fairly happy with the code approach here, though I'm not at all sure that I've put the command line option in the right places. I also suspect I've missed some places that would need updating in the docs. Guidance around the proper places to put the docs/CLI arg/etc. is most welcome. (I believe I've now caught everywhere that this needs to go)

[JelleZijlstra]

The mypy-primer output here is unexpected, right? The new error is off by default so it shouldn't trigger on existing code.

[PeterJCLaw]

The mypy-primer output here is unexpected, right? The new error is off by default so it shouldn't trigger on existing code.

Indeed, I'm not sure what's causing that. When I run mypy from this branch by default I'm not seeing the option take effect.

Separately, I have noticed that if I run mypy from this branch initially without the new option and then run it with the new option the errors aren't found when they should be:

$ cat foo.py
foo  # type: ignore
$ mypy foo.py
Success: no issues found in 1 source file
$ mypy foo.py --disallow-ignore-without-code
Success: no issues found in 1 source file

Changing the test file seems to get the error to show up, so I'm wondering if there's some caching that I've missed?

[hauntsaninja]

re: primer, I think you might have automatically included it in --strict (see the strict_group stuff)

[PeterJCLaw]

I think you might have automatically included it in --strict (see the strict_group stuff)

Ahhh, yeah. That was actually intentional and I'd forgotten I'd done it. I'm not sure whether this qualifies for --strict (or how that gets decided), but it's something I'd be interested in including in the strict mode.

Perhaps it would be better to introduce this flag first and then later move it into --strict?
(If --warn-unused-ignores isn't in --strict then adding it at the same time might also make sense)

[hauntsaninja]

Yeah, I think we should introduce this first and only later consider moving it into --strict, since this has a pretty large impact on users of --strict. I'd also want to make sure that the error codes mypy provides for common errors are useful before doing so to reduce future breakage; I think currently we have a lot of error code "misc".

My approximate mental model for --strict is "you shouldn't ever have type errors without an explicit circumvention somewhere". Or "this is what the default behaviour of mypy should be if all Python were typed".
If today was the first day of mypy I'd be more willing to include it; in our current world I lean towards avoiding breaking unsuspecting users since it's just an adjustment of something that's already explicitly opting in to type unsafety.

--warn-unused-ignores is already part of --strict, but --show-error-codes is not (but should be, if this is incorporated into --strict). Speaking of which, if possible, it would be really nice to show which error code(s) to add in your error message.

[PeterJCLaw]

Yeah, I think we should introduce this first and only later consider moving it into --strict, since this has a pretty large impact on users of --strict. I'd also want to make sure that the error codes mypy provides for common errors are useful before doing so to reduce future breakage; I think currently we have a lot of error code "misc".

Ah, fair. I've personally found that while a lot of things were misc, most things now aren't (:+1: to whoever was pushing that a while back). I can definitely see the desire to get misc even lower before requiring codes though 👍

... if possible, it would be really nice to show which error code(s) to add in your error message.

Good point, I think it should be easy to add that.

[PeterJCLaw]

Separately, I have noticed that if I run mypy from this branch initially without the new option and then run it with the new option the errors aren't found when they should be

I believe I've fixed this in c811aa7.

[JukkaL]

I think this would be better enabled via --enable-error-code <something>, instead of adding a new command-line option. The motivation is to avoid continuously increasing the size and complexity of the command-line interface.

(Some of the existing flags would work better using --enable-error-code, but they were added before --enable-error-code was available and maintaining backward compatibility is useful.)

[PeterJCLaw]

@JukkaL thanks, I've changed over to this being an error code (still disabled by default).

[JelleZijlstra]

I can handle getting this through, since Jukka and Shantanu already commented here without objections to the overall idea. If you fix the conflict, I'll do another round of review, then merge if everything looks good.

[PeterJCLaw]

Thanks! I'll fix that conflict now 👍

[JelleZijlstra]

Looks like a test needs to be updated.

[JelleZijlstra]

Some minor feedback on the docs

[NeilGirdhar]

Just curious, what happens if you have a # type: ignore[pyright] or some other code that doesn't exist?

[PeterJCLaw]

Just curious, what happens if you have a # type: ignore[pyright] or some other code that doesn't exist?

Currently nothing specific from this new error code. --warn-unused-ignores does pick it up though.

[JelleZijlstra]

Thank you!

[PeterJCLaw]

One thing I have spotted here is that it's now possible to have several errors from the same problem, as shown by the testErrorCodeMissingDoesntTrampleUnusedIgnoresWarning test:

# flags: --enable-error-code ignore-without-code --warn-unused-ignores
z # type: ignore[ignore-without-code]

generates three messages:

• Error: Unused "type: ignore" comment
• Error: Name "z" is not defined [name-defined]
• Note: Error code "name-defined" not covered by "type: ignore" comment

While I'm not sure if this is something created by this PR, is this something we want to address here?

[JelleZijlstra]

While I'm not sure if this is something created by this PR, is this something we want to address here?

I'd rather cover that in a separate PR. Maybe we should suppress the Unused "type: ignore" comment error?

[github-actions]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[DanielNoord]

Just want to express my gratitude to @PeterJCLaw for writing and @JelleZijlstra for reviewing this PR. We have been waiting patiently for this function, nice to see it got merged 🎉 Thanks guys!