Warn about unused '# type: ignore' comments

This commit also simplifies the handling of ignored_lines slightly,
setting them once in the Errors object for each file file after it is
parsed.

Fixes python#1345 and python#1739.

Author: rwbarton

mypy/build.py

@@ -70,6 +70,8 @@
 WARN_INCOMPLETE_STUB = 'warn-incomplete-stub'
 # Warn about casting an expression to its inferred type
 WARN_REDUNDANT_CASTS = 'warn-redundant-casts'
+# Warn about unused '# type: ignore' comments
+WARN_UNUSED_IGNORES = 'warn-unused-ignores'
 
 PYTHON_EXTENSIONS = ['.pyi', '.py']
 
@@ -456,9 +458,26 @@ def parse_file(self, id: str, path: str, source: str) -> MypyFile:
                      custom_typing_module=self.custom_typing_module,
                      fast_parser=FAST_PARSER in self.flags)
         tree._fullname = id
+
+        # We don't want to warn about 'type: ignore' comments on
+        # imports, but we're about to modify tree.imports, so grab
+        # these first.
+        import_lines = set(node.line for node in tree.imports)
+
+        # Skip imports that have been ignored (so that we can ignore a C extension module without
+        # stub, for example), except for 'from x import *', because we wouldn't be able to
+        # determine which names should be defined unless we process the module. We can still
+        # ignore errors such as redefinitions when using the latter form.
+        imports = [node for node in tree.imports
+                   if node.line not in tree.ignored_lines or isinstance(node, ImportAll)]
+        tree.imports = imports
+
         if self.errors.num_messages() != num_errs:
             self.log("Bailing due to parse errors")
             self.errors.raise_error()
+
+        self.errors.set_file_ignored_lines(path, tree.ignored_lines)
+        self.errors.mark_file_ignored_lines_used(path, import_lines)
         return tree
 
     def module_not_found(self, path: str, line: int, id: str) -> None:
@@ -1088,8 +1107,7 @@ def __init__(self,
                     suppress_message = ((SILENT_IMPORTS in manager.flags and
                                         ALMOST_SILENT not in manager.flags) or
                                         (caller_state.tree is not None and
-                                         (caller_line in caller_state.tree.ignored_lines or
-                                          'import' in caller_state.tree.weak_opts)))
+                                         'import' in caller_state.tree.weak_opts))
                     if not suppress_message:
                         save_import_context = manager.errors.import_context()
                         manager.errors.set_import_context(caller_state.import_context)
@@ -1333,6 +1351,8 @@ def dispatch(sources: List[BuildSource], manager: BuildManager) -> None:
     graph = load_graph(sources, manager)
     manager.log("Loaded graph with %d nodes" % len(graph))
     process_graph(graph, manager)
+    if WARN_UNUSED_IGNORES in manager.flags:
+        manager.errors.generate_unused_ignore_notes()
 
 
 def load_graph(sources: List[BuildSource], manager: BuildManager) -> Graph:

mypy/checker.py

@@ -420,7 +420,6 @@ def visit_file(self, file_node: MypyFile, path: str) -> None:
         self.pass_num = 0
         self.is_stub = file_node.is_stub
         self.errors.set_file(path)
-        self.errors.set_ignored_lines(file_node.ignored_lines)
         self.globals = file_node.names
         self.weak_opts = file_node.weak_opts
         self.enter_partial_types()
@@ -435,7 +434,6 @@ def visit_file(self, file_node: MypyFile, path: str) -> None:
         if self.deferred_nodes:
             self.check_second_pass()
 
-        self.errors.set_ignored_lines(set())
         self.current_node_deferred = False
 
         all_ = self.globals.get('__all__')
@@ -1525,7 +1523,7 @@ def set_inference_error_fallback_type(self, var: Var, lvalue: Node, type: Type,
 
         We implement this here by giving x a valid type (Any).
         """
-        if context.get_line() in self.errors.ignored_lines:
+        if context.get_line() in self.errors.ignored_lines[self.errors.file]:
             self.set_inferred_type(var, lvalue, AnyType())
 
     def narrow_type_from_binder(self, expr: Node, known_type: Type) -> Type:

mypy/errors.py

@@ -2,8 +2,9 @@
 import os.path
 import sys
 import traceback
+from collections import OrderedDict, defaultdict
 
-from typing import Tuple, List, TypeVar, Set
+from typing import Tuple, List, TypeVar, Set, Dict, Optional
 
 
 T = TypeVar('T')
@@ -79,8 +80,11 @@ class Errors:
     # Stack of short names of current functions or members (or None).
     function_or_member = None  # type: List[str]
 
-    # Ignore errors on these lines.
-    ignored_lines = None  # type: Set[int]
+    # Ignore errors on these lines of each file.
+    ignored_lines = None  # type: Dict[str, Set[int]]
+
+    # Lines on which an error was actually ignored.
+    used_ignored_lines = None  # type: Dict[str, Set[int]]
 
     # Collection of reported only_once messages.
     only_once_messages = None  # type: Set[str]
@@ -90,7 +94,8 @@ def __init__(self) -> None:
         self.import_ctx = []
         self.type_name = [None]
         self.function_or_member = [None]
-        self.ignored_lines = set()
+        self.ignored_lines = OrderedDict()
+        self.used_ignored_lines = defaultdict(set)
         self.only_once_messages = set()
 
     def copy(self) -> 'Errors':
@@ -109,13 +114,26 @@ def set_ignore_prefix(self, prefix: str) -> None:
             prefix += os.sep
         self.ignore_prefix = prefix
 
-    def set_file(self, file: str) -> None:
-        """Set the path of the current file."""
+    def simplify_path(self, file: str) -> str:
         file = os.path.normpath(file)
-        self.file = remove_path_prefix(file, self.ignore_prefix)
+        return remove_path_prefix(file, self.ignore_prefix)
+
+    def set_file(self, file: str, ignored_lines: Set[int] = None) -> None:
+        """Set the path of the current file."""
+        # The path will be simplified later, in render_messages. That way
+        #  * 'file' is always a key that uniquely identifies a source file
+        #    that mypy read (simplified paths might not be unique); and
+        #  * we only have to simplify in one place, while still supporting
+        #    reporting errors for files other than the one currently being
+        #    processed.
+        self.file = file
+
+    def set_file_ignored_lines(self, file: str, ignored_lines: Set[int] = None) -> None:
+        self.ignored_lines[file] = ignored_lines
 
-    def set_ignored_lines(self, ignored_lines: Set[int]) -> None:
-        self.ignored_lines = ignored_lines
+    def mark_file_ignored_lines_used(self, file: str, used_ignored_lines: Set[int] = None
+                                     ) -> None:
+        self.used_ignored_lines[file] |= used_ignored_lines
 
     def push_function(self, name: str) -> None:
         """Set the current function or member short name (it can be None)."""
@@ -170,15 +188,25 @@ def report(self, line: int, message: str, blocker: bool = False,
         self.add_error_info(info)
 
     def add_error_info(self, info: ErrorInfo) -> None:
-        if info.line in self.ignored_lines:
+        if info.file in self.ignored_lines and info.line in self.ignored_lines[info.file]:
             # Annotation requests us to ignore all errors on this line.
+            self.used_ignored_lines[info.file].add(info.line)
             return
         if info.only_once:
             if info.message in self.only_once_messages:
                 return
             self.only_once_messages.add(info.message)
         self.error_info.append(info)
 
+    def generate_unused_ignore_notes(self) -> None:
+        for file, ignored_lines in self.ignored_lines.items():
+            for line in ignored_lines - self.used_ignored_lines[file]:
+                # Don't use report since add_error_info will ignore the error!
+                info = ErrorInfo(self.import_context(), file, None, None,
+                                 line, 'note', "unused 'type: ignore' comment",
+                                 False, False)
+                self.error_info.append(info)
+
     def num_messages(self) -> int:
         """Return the number of generated messages."""
         return len(self.error_info)
@@ -254,32 +282,34 @@ def render_messages(self, errors: List[ErrorInfo]) -> List[Tuple[str, int,
                     result.append((None, -1, 'note', fmt.format(path, line)))
                     i -= 1
 
+            file = self.simplify_path(e.file)
+
             # Report context within a source file.
             if (e.function_or_member != prev_function_or_member or
                     e.type != prev_type):
                 if e.function_or_member is None:
                     if e.type is None:
-                        result.append((e.file, -1, 'note', 'At top level:'))
+                        result.append((file, -1, 'note', 'At top level:'))
                     else:
-                        result.append((e.file, -1, 'note', 'In class "{}":'.format(
+                        result.append((file, -1, 'note', 'In class "{}":'.format(
                             e.type)))
                 else:
                     if e.type is None:
-                        result.append((e.file, -1, 'note',
+                        result.append((file, -1, 'note',
                                        'In function "{}":'.format(
                                            e.function_or_member)))
                     else:
-                        result.append((e.file, -1, 'note',
+                        result.append((file, -1, 'note',
                                        'In member "{}" of class "{}":'.format(
                                            e.function_or_member, e.type)))
             elif e.type != prev_type:
                 if e.type is None:
-                    result.append((e.file, -1, 'note', 'At top level:'))
+                    result.append((file, -1, 'note', 'At top level:'))
                 else:
-                    result.append((e.file, -1, 'note',
+                    result.append((file, -1, 'note',
                                    'In class "{}":'.format(e.type)))
 
-            result.append((e.file, e.line, e.severity, e.message))
+            result.append((file, e.line, e.severity, e.message))
 
             prev_import_context = e.import_ctx
             prev_function_or_member = e.function_or_member

mypy/main.py

@@ -155,6 +155,8 @@ def parse_version(v):
                         " --check-untyped-defs enabled")
     parser.add_argument('--warn-redundant-casts', action='store_true',
                         help="warn about casting an expression to its inferred type")
+    parser.add_argument('--warn-unused-ignores', action='store_true',
+                        help="warn about unneeded '# type: ignore' comments")
     parser.add_argument('--fast-parser', action='store_true',
                         help="enable experimental fast parser")
     parser.add_argument('-i', '--incremental', action='store_true',
@@ -256,6 +258,9 @@ def parse_version(v):
     if args.warn_redundant_casts:
         options.build_flags.append(build.WARN_REDUNDANT_CASTS)
 
+    if args.warn_unused_ignores:
+        options.build_flags.append(build.WARN_UNUSED_IGNORES)
+
     # experimental
     if args.fast_parser:
         options.build_flags.append(build.FAST_PARSER)

mypy/parse.py

@@ -176,13 +176,7 @@ def parse_file(self) -> MypyFile:
         defs = self.parse_defs()
         weak_opts = self.weak_opts()
         self.expect_type(Eof)
-        # Skip imports that have been ignored (so that we can ignore a C extension module without
-        # stub, for example), except for 'from x import *', because we wouldn't be able to
-        # determine which names should be defined unless we process the module. We can still
-        # ignore errors such as redefinitions when using the latter form.
-        imports = [node for node in self.imports
-                   if node.line not in self.ignored_lines or isinstance(node, ImportAll)]
-        node = MypyFile(defs, imports, is_bom, self.ignored_lines,
+        node = MypyFile(defs, self.imports, is_bom, self.ignored_lines,
                         weak_opts=weak_opts)
         return node
 

mypy/semanal.py

@@ -220,7 +220,6 @@ def __init__(self,
 
     def visit_file(self, file_node: MypyFile, fnam: str) -> None:
         self.errors.set_file(fnam)
-        self.errors.set_ignored_lines(file_node.ignored_lines)
         self.cur_mod_node = file_node
         self.cur_mod_id = file_node.fullname()
         self.is_stub_file = fnam.lower().endswith('.pyi')
@@ -243,8 +242,6 @@ def visit_file(self, file_node: MypyFile, fnam: str) -> None:
         if self.cur_mod_id == 'builtins':
             remove_imported_names_from_symtable(self.globals, 'builtins')
 
-        self.errors.set_ignored_lines(set())
-
         if '__all__' in self.globals:
             for name, g in self.globals.items():
                 if name not in self.all_exports:
@@ -2477,9 +2474,7 @@ def __init__(self, modules: Dict[str, MypyFile], errors: Errors) -> None:
 
     def visit_file(self, file_node: MypyFile, fnam: str) -> None:
         self.errors.set_file(fnam)
-        self.errors.set_ignored_lines(file_node.ignored_lines)
         self.accept(file_node)
-        self.errors.set_ignored_lines(set())
 
     def accept(self, node: Node) -> None:
         try:

test-data/unit/check-ignore.test

@@ -169,3 +169,14 @@ def bar(x: Base[str, str]) -> None: pass
 bar(Child())
 [out]
 main:19: error: Argument 1 to "bar" has incompatible type "Child"; expected Base[str, str]
+
+[case testTypeIgnoreLineNumberWithinFile]
+import m
+pass # type: ignore
+m.f(kw=1)
+[file m.py]
+pass
+def f() -> None: pass
+[out]
+main:3: error: Unexpected keyword argument "kw" for "f"
+tmp/m.py:2: note: "f" defined here

test-data/unit/check-warnings.test

@@ -34,3 +34,24 @@ b = B()
 # Without the cast, the following line would fail to type check.
 c = add([cast(A, b)], [a])
 [builtins fixtures/list.py]
+
+
+-- Unused 'type: ignore' comments
+-- ------------------------------
+
+[case testUnusedTypeIgnore]
+# flags: warn-unused-ignores
+a = 1
+a = 'a' # type: ignore
+a = 2 # type: ignore # N: unused 'type: ignore' comment
+a = 'b' # E: Incompatible types in assignment (expression has type "str", variable has type "int")
+
+[case testUnusedTypeIgnoreImport]
+# flags: warn-unused-ignores
+# Never warn about `type: ignore` comments on imports.
+import banana # type: ignore
+import m # type: ignore
+from m import * # type: ignore
+[file m.py]
+pass
+[out]