Warn about unused '# type: ignore' comments

[rwbarton]

Still needs tests and some cleanup.

[rwbarton]

The results on mypy itself, for those curious:

rbarton-mbp:mypy rbarton$ python3 -m mypy --warn-unused-ignores mypy
typeshed/stdlib/3/builtins.pyi:406: note: unused 'type: ignore' comment
typeshed/stdlib/3/io.pyi:230: note: unused 'type: ignore' comment
typeshed/stdlib/3/io.pyi:231: note: unused 'type: ignore' comment
typeshed/stdlib/3/io.pyi:236: note: unused 'type: ignore' comment
mypy/errors.py:395: note: unused 'type: ignore' comment
mypy/main.py:133: note: unused 'type: ignore' comment

[gvanrossum]

I can't wait to use this!

Unfortunately there's also #1294 which someone at the sprints was going to tackle (though they haven't sent a PR yet). Wouldn't it be confusing if these notes appeared at the end and out of order? (Then again, out of order is better than not at all.)

[gvanrossum]

Be sure to add "Fixes #1345" to the commit when merging.

Consider combining this with #958 and changing the flag name to be less specific.

[rwbarton]

I made a separate PR (#1705) for redundant casts since that one is actually much simpler. I'm happy to have these two flags controlled by the same command-line option if someone wants to suggest a name :)

Unfortunately there's also #1294 which someone at the sprints was going to tackle (though they haven't sent a PR yet). Wouldn't it be confusing if these notes appeared at the end and out of order? (Then again, out of order is better than not at all.)

I thought about that PR, but I'd rather not block this on waiting for that one, and as you say a confusing message order is better than not having the messages at all. With that PR it'd certainly make sense to generate the unused # type: ignore warnings per-SCC rather than all at the end, though.

[gvanrossum]

Curious, what is this waiting on? I really want this feature!

[rwbarton]

Oh, thanks for the reminder about this one. When I wrote this I didn't know how to write tests that set build flags, but I do now (using # flags: ...). There are also some edge cases involving # type: ignore comments on import statements that I want to take another look at.

[gvanrossum]

Please do!

[rwbarton]

This still needs new tests (will add them a bit later) but I'm much happier with this version which handles ignored_lines in a simpler and more uniform way (adding them once to the Errors object after parsing).

[rwbarton]

OK, this should be ready to go except that one of the new tests fails for now; I filed #1737 to fix it separately because I'm not 100% sure I understand what is going on there. That PR should be landed (or otherwise resolved somehow) before this one.

[rwbarton]

OK, #1737 was wrong, so another try. I made it never warn about # type: ignore comments on imports since that is simple and probably correct most of the time anyways. In order to do that, it was convenient to fix #1739 at the same time.

[gvanrossum]

There's a problem with this PR that we discovered today: #1911. In an attempt to make it so that you can use # type: ignore on an import to silence a module with a syntax error (or perhaps with any kind of error), this PR accidentally makes mypy entirely ignore any line of the form import x # type: ignore (or similar for import as). That can't be the intention! We're fixing that in #1912. @rwbarton: holler now if you think we're making a grave mistake.

[rwbarton]

Well that's what I intended and I think what the old behavior was (though maybe not with the fast parser): see discussion at #1737. But the new behavior seems fine to me too.

[gvanrossum]

Oh dear. I bisected but it never occurred to me that this would differ between the old and the new parser. It also seems I disagree with my past self -- I currently strongly feel it's surprising that `import x # type: ignore` never looks at x (or only when it's already been imported without an ignore elsewhere). So I'd still like to see this fixed, for both the old and the new parser. Seems you're fine with that. Thanks for the additional color!