Skip to content

Commit

Permalink
Add a secondary context for this error message
Browse files Browse the repository at this point in the history
Add another context to this message reporting. This means that we'll now check both contexts when looking at what errors to ignore.
  • Loading branch information
koogoro committed Feb 13, 2023
1 parent 7a3d4ab commit 4d0993a
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 18 deletions.
1 change: 1 addition & 0 deletions mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -2082,6 +2082,7 @@ def erase_override(t: Type) -> Type:
arg_type_in_super,
supertype,
context,
secondary_context=node,
)
emitted_msg = True

Expand Down
18 changes: 9 additions & 9 deletions mypy/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import sys
import traceback
from collections import defaultdict
from typing import Callable, NoReturn, Optional, TextIO, Tuple, TypeVar
from typing import Callable, NoReturn, Optional, TextIO, Tuple, TypeVar, Iterable
from typing_extensions import Final, Literal, TypeAlias as _TypeAlias

from mypy import errorcodes as codes
Expand Down Expand Up @@ -78,7 +78,7 @@ class ErrorInfo:

# Actual origin of the error message as tuple (path, line number, end line number)
# If end line number is unknown, use line number.
origin: tuple[str, int, int]
origin: tuple[str, Iterable[int]]

# Fine-grained incremental target where this was reported
target: str | None = None
Expand All @@ -104,7 +104,7 @@ def __init__(
blocker: bool,
only_once: bool,
allow_dups: bool,
origin: tuple[str, int, int] | None = None,
origin: tuple[str, Iterable[int]] | None = None,
target: str | None = None,
) -> None:
self.import_ctx = import_ctx
Expand All @@ -122,7 +122,7 @@ def __init__(
self.blocker = blocker
self.only_once = only_once
self.allow_dups = allow_dups
self.origin = origin or (file, line, line)
self.origin = origin or (file, [line])
self.target = target


Expand Down Expand Up @@ -367,7 +367,7 @@ def report(
file: str | None = None,
only_once: bool = False,
allow_dups: bool = False,
origin_span: tuple[int, int] | None = None,
origin_span: Iterable[int] | None = None,
offset: int = 0,
end_line: int | None = None,
end_column: int | None = None,
Expand Down Expand Up @@ -411,7 +411,7 @@ def report(
message = " " * offset + message

if origin_span is None:
origin_span = (line, line)
origin_span = [line]

if end_line is None:
end_line = line
Expand All @@ -434,7 +434,7 @@ def report(
blocker,
only_once,
allow_dups,
origin=(self.file, *origin_span),
origin=(self.file, origin_span),
target=self.current_target(),
)
self.add_error_info(info)
Expand Down Expand Up @@ -467,7 +467,7 @@ def _filter_error(self, file: str, info: ErrorInfo) -> bool:
return False

def add_error_info(self, info: ErrorInfo) -> None:
file, line, end_line = info.origin
file, lines = info.origin
# process the stack of ErrorWatchers before modifying any internal state
# in case we need to filter out the error entirely
# NB: we need to do this both here and in _add_error_info, otherwise we
Expand All @@ -478,7 +478,7 @@ def add_error_info(self, info: ErrorInfo) -> None:
if file in self.ignored_lines:
# Check each line in this context for "type: ignore" comments.
# line == end_line for most nodes, so we only loop once.
for scope_line in range(line, end_line + 1):
for scope_line in lines:
if self.is_ignored_error(scope_line, info, self.ignored_lines[file]):
# Annotation requests us to ignore all errors on this line.
self.used_ignored_lines[file][scope_line].append(
Expand Down
34 changes: 25 additions & 9 deletions mypy/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from __future__ import annotations

import difflib
import itertools
import re
from contextlib import contextmanager
from textwrap import dedent
Expand Down Expand Up @@ -136,6 +137,7 @@
}



class MessageBuilder:
"""Helper class for reporting type checker error messages with parameters.
Expand Down Expand Up @@ -210,34 +212,40 @@ def report(
origin: Context | None = None,
offset: int = 0,
allow_dups: bool = False,
secondary_context: Context | None = None,
) -> None:
"""Report an error or note (unless disabled).
Note that context controls where error is reported, while origin controls
where # type: ignore comments have effect.
"""

def span_from_context(ctx: Context) -> tuple[int, int]:
def span_from_context(ctx: Context) -> Iterable[int]:
"""This determines where a type: ignore for a given context has effect.
Current logic is a bit tricky, to keep as much backwards compatibility as
possible. We may reconsider this to always be a single line (or otherwise
simplify it) when we drop Python 3.7.
"""
if isinstance(ctx, (ClassDef, FuncDef)):
return ctx.deco_line or ctx.line, ctx.line
return range(ctx.deco_line or ctx.line, ctx.line + 1)
elif not isinstance(ctx, Expression):
return ctx.line, ctx.line
return [ctx.line]
else:
return ctx.line, ctx.end_line or ctx.line
return range(ctx.line, (ctx.end_line or ctx.line) + 1)

origin_span: tuple[int, int] | None
origin_span: Iterable[int] | None
if origin is not None:
origin_span = span_from_context(origin)
elif context is not None:
origin_span = span_from_context(context)
else:
origin_span = None

if secondary_context is not None:
assert origin_span is not None
origin_span = itertools.chain(origin_span, span_from_context(secondary_context))

self.errors.report(
context.line if context else -1,
context.column if context else -1,
Expand All @@ -260,9 +268,10 @@ def fail(
code: ErrorCode | None = None,
file: str | None = None,
allow_dups: bool = False,
secondary_context: Context | None = None,
) -> None:
"""Report an error message (unless disabled)."""
self.report(msg, context, "error", code=code, file=file, allow_dups=allow_dups)
self.report(msg, context, "error", code=code, file=file, allow_dups=allow_dups, secondary_context=secondary_context)

def note(
self,
Expand All @@ -274,6 +283,7 @@ def note(
allow_dups: bool = False,
*,
code: ErrorCode | None = None,
secondary_context: Context | None = None,
) -> None:
"""Report a note (unless disabled)."""
self.report(
Expand All @@ -285,6 +295,7 @@ def note(
offset=offset,
allow_dups=allow_dups,
code=code,
secondary_context=secondary_context,
)

def note_multiline(
Expand All @@ -295,11 +306,13 @@ def note_multiline(
offset: int = 0,
allow_dups: bool = False,
code: ErrorCode | None = None,
*,
secondary_context: Context | None = None,
) -> None:
"""Report as many notes as lines in the message (unless disabled)."""
for msg in messages.splitlines():
self.report(
msg, context, "note", file=file, offset=offset, allow_dups=allow_dups, code=code
msg, context, "note", file=file, offset=offset, allow_dups=allow_dups, code=code, secondary_context=secondary_context,
)

#
Expand Down Expand Up @@ -1153,6 +1166,7 @@ def argument_incompatible_with_supertype(
arg_type_in_supertype: Type,
supertype: str,
context: Context,
secondary_context: Context,
) -> None:
target = self.override_target(name, name_in_supertype, supertype)
arg_type_in_supertype_f = format_type_bare(arg_type_in_supertype)
Expand All @@ -1163,17 +1177,19 @@ def argument_incompatible_with_supertype(
),
context,
code=codes.OVERRIDE,
secondary_context=secondary_context,
)
self.note("This violates the Liskov substitution principle", context, code=codes.OVERRIDE)
self.note("This violates the Liskov substitution principle", context, code=codes.OVERRIDE, secondary_context=secondary_context)
self.note(
"See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides",
context,
code=codes.OVERRIDE,
secondary_context=secondary_context,
)

if name == "__eq__" and type_name:
multiline_msg = self.comparison_method_example_msg(class_name=type_name)
self.note_multiline(multiline_msg, context, code=codes.OVERRIDE)
self.note_multiline(multiline_msg, context, code=codes.OVERRIDE, secondary_context=secondary_context)

def comparison_method_example_msg(self, class_name: str) -> str:
return dedent(
Expand Down
53 changes: 53 additions & 0 deletions test-data/unit/check-classes.test
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,59 @@ main:7: note: This violates the Liskov substitution principle
main:7: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
main:9: error: Return type "object" of "h" incompatible with return type "A" in supertype "A"

[case testMethodOverridingWithIncompatibleTypesOnMultipleLines]
class A:
def f(self, x: int, y: str) -> None: pass
class B(A):
def f(
self,
x: int,
y: bool,
) -> None:
pass
[out]
main:7: error: Argument 2 of "f" is incompatible with supertype "A"; supertype defines the argument type as "str"
main:7: note: This violates the Liskov substitution principle
main:7: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

[case testMultiLineMethodOverridingWithIncompatibleTypesIgnorableAtArgument]
class A:
def f(self, x: int, y: str) -> None: pass

class B(A):
def f(
self,
x: int,
y: bool, # type: ignore[override]
) -> None:
pass

[case testMultiLineMethodOverridingWithIncompatibleTypesIgnorableAtDefinition]
class A:
def f(self, x: int, y: str) -> None: pass
class B(A):
def f( # type: ignore[override]
self,
x: int,
y: bool,
) -> None:
pass

[case testMultiLineMethodOverridingWithIncompatibleTypesWrongIgnore]
class A:
def f(self, x: int, y: str) -> None: pass
class B(A):
def f( # type: ignore[return-type]
self,
x: int,
y: bool,
) -> None:
pass
[out]
main:7: error: Argument 2 of "f" is incompatible with supertype "A"; supertype defines the argument type as "str"
main:7: note: This violates the Liskov substitution principle
main:7: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

[case testEqMethodsOverridingWithNonObjects]
class A:
def __eq__(self, other: A) -> bool: pass # Fail
Expand Down

0 comments on commit 4d0993a

Please sign in to comment.