diff --git a/mypy/build.py b/mypy/build.py index b61178be4ed9..d89a0f45c9cb 100644 --- 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'] @@ -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: diff --git a/mypy/checker.py b/mypy/checker.py index a7af74911c1e..175f1fbf4498 100644 --- 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 index b0a2c62f8a32..c6c183c1cedd 100644 --- a/mypy/errors.py +++ b/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,8 +188,9 @@ 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: @@ -179,6 +198,15 @@ def add_error_info(self, info: ErrorInfo) -> None: 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 diff --git a/mypy/main.py b/mypy/main.py index a45b4f163618..319cd9b383e0 100644 --- 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/parse.py b/mypy/parse.py index ec18bac8f7f8..fd88bd6f6e45 100644 --- a/mypy/parse.py +++ b/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 diff --git a/mypy/semanal.py b/mypy/semanal.py index c4d6ac2a80d6..459bbe6964b6 100644 --- 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: diff --git a/test-data/unit/check-ignore.test b/test-data/unit/check-ignore.test index bff161eeea5d..7c2fb250a38d 100644 --- a/test-data/unit/check-ignore.test +++ b/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 diff --git a/test-data/unit/check-warnings.test b/test-data/unit/check-warnings.test index 17ed833711f9..84a5d8ed83bd 100644 --- a/test-data/unit/check-warnings.test +++ b/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]