WIP: Warn on unused # type: ignores

rwbarton · rwbarton · commit 116bedfda64b · 2016-06-22T06:21:49.000-07:00
Still needs tests.
diff --git a/mypy/build.py b/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']
 
@@ -459,6 +461,7 @@ def parse_file(self, id: str, path: str, source: str) -> MypyFile:
         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)
         return tree
 
     def module_not_found(self, path: str, line: int, id: str) -> None:
@@ -1088,8 +1091,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 +1335,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:
diff --git a/mypy/checker.py b/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:
diff --git a/mypy/errors.py b/mypy/errors.py
@@ -2,8 +2,9 @@
 import os.path
 import sys
 import traceback
+from collections import OrderedDict
 
-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 = {}
         self.only_once_messages = set()
 
     def copy(self) -> 'Errors':
@@ -109,13 +114,22 @@ 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_ignored_lines(self, ignored_lines: Set[int]) -> None:
-        self.ignored_lines = ignored_lines
+    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 push_function(self, name: str) -> None:
         """Set the current function or member short name (it can be None)."""
@@ -170,15 +184,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.setdefault(info.file, set()).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.get(file, set()):
+                # 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 +278,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
diff --git a/mypy/main.py b/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)
diff --git a/mypy/semanal.py b/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:
