Skip to content

Avoid false unreachable and redundant-expr warnings in loops more robustly and efficiently, and avoid multiple revealed type notes for the same line. #19118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 38 additions & 18 deletions mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
from mypy.constraints import SUPERTYPE_OF
from mypy.erasetype import erase_type, erase_typevars, remove_instance_last_known_values
from mypy.errorcodes import TYPE_VAR, UNUSED_AWAITABLE, UNUSED_COROUTINE, ErrorCode
from mypy.errors import Errors, ErrorWatcher, report_internal_error
from mypy.errors import Errors, ErrorWatcher, LoopErrorWatcher, report_internal_error
from mypy.expandtype import expand_type
from mypy.literals import Key, extract_var_from_literal_hash, literal, literal_hash
from mypy.maptype import map_instance_to_supertype
Expand Down Expand Up @@ -600,19 +600,26 @@ def accept_loop(
# on without bound otherwise)
widened_old = len(self.widened_vars)

# Disable error types that we cannot safely identify in intermediate iteration steps:
warn_unreachable = self.options.warn_unreachable
warn_redundant = codes.REDUNDANT_EXPR in self.options.enabled_error_codes
self.options.warn_unreachable = False
self.options.enabled_error_codes.discard(codes.REDUNDANT_EXPR)

# one set of `unreachable` and `redundant-expr`errors per iteration step:
uselessness_errors = []
# one set of unreachable line numbers per iteration step:
unreachable_lines = []
# one set of revealed types per line where `reveal_type` is used (each
# created set can grow during the iteration):
revealed_types = defaultdict(set)
iter = 1
while True:
with self.binder.frame_context(can_skip=True, break_frame=2, continue_frame=1):
if on_enter_body is not None:
on_enter_body()

self.accept(body)
with LoopErrorWatcher(self.msg.errors) as watcher:
self.accept(body)
uselessness_errors.append(watcher.uselessness_errors)
unreachable_lines.append(watcher.unreachable_lines)
for key, values in watcher.revealed_types.items():
revealed_types[key].update(values)

partials_new = sum(len(pts.map) for pts in self.partial_types)
widened_new = len(self.widened_vars)
# Perform multiple iterations if something changed that might affect
Expand All @@ -633,16 +640,29 @@ def accept_loop(
if iter == 20:
raise RuntimeError("Too many iterations when checking a loop")

# If necessary, reset the modified options and make up for the postponed error checks:
self.options.warn_unreachable = warn_unreachable
if warn_redundant:
self.options.enabled_error_codes.add(codes.REDUNDANT_EXPR)
if warn_unreachable or warn_redundant:
with self.binder.frame_context(can_skip=True, break_frame=2, continue_frame=1):
if on_enter_body is not None:
on_enter_body()

self.accept(body)
# Report only those `unreachable` and `redundant-expr` errors that could not
# be ruled out in any iteration step:
persistent_uselessness_errors = set()
for candidate in set(itertools.chain(*uselessness_errors)):
if all(
(candidate in errors) or (candidate[2] in lines)
for errors, lines in zip(uselessness_errors, unreachable_lines)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this just use the last round of unreachability warnings?

):
persistent_uselessness_errors.add(candidate)
for error_info in persistent_uselessness_errors:
context = Context(line=error_info[2], column=error_info[3])
context.end_line = error_info[4]
context.end_column = error_info[5]
self.msg.fail(error_info[1], context, code=error_info[0])

# Report all types revealed in at least one iteration step:
for note_info, types in revealed_types.items():
sorted_ = sorted(types, key=lambda typ: typ.lower())
revealed = sorted_[0] if len(types) == 1 else f"Union[{', '.join(sorted_)}]"
context = Context(line=note_info[1], column=note_info[2])
context.end_line = note_info[3]
context.end_column = note_info[4]
self.note(f'Revealed type is "{revealed}"', context)

# If exit_condition is set, assume it must be False on exit from the loop:
if exit_condition:
Expand Down
58 changes: 56 additions & 2 deletions mypy/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from collections import defaultdict
from collections.abc import Iterable
from typing import Callable, Final, NoReturn, Optional, TextIO, TypeVar
from typing_extensions import Literal, TypeAlias as _TypeAlias
from typing_extensions import Literal, Self, TypeAlias as _TypeAlias

from mypy import errorcodes as codes
from mypy.error_formatter import ErrorFormatter
Expand Down Expand Up @@ -179,7 +179,7 @@ def __init__(
self._filter_deprecated = filter_deprecated
self._filtered: list[ErrorInfo] | None = [] if save_filtered_errors else None

def __enter__(self) -> ErrorWatcher:
def __enter__(self) -> Self:
self.errors._watchers.append(self)
return self

Expand Down Expand Up @@ -220,6 +220,60 @@ def filtered_errors(self) -> list[ErrorInfo]:
return self._filtered


class LoopErrorWatcher(ErrorWatcher):
"""Error watcher that filters and separately collects `unreachable` errors,
`redundant-expr` errors, and revealed types when analysing loops iteratively
to help avoid making too-hasty reports."""

# Meaning of the tuple items: ErrorCode, message, line, column, end_line, end_column:
uselessness_errors: set[tuple[ErrorCode, str, int, int, int, int]]

# Meaning of the tuple items: function_or_member, line, column, end_line, end_column:
revealed_types: dict[tuple[str | None, int, int, int, int], set[str]]

# Not only the lines where the error report occurs but really all unreachable lines:
unreachable_lines: set[int]

def __init__(
self,
errors: Errors,
*,
filter_errors: bool | Callable[[str, ErrorInfo], bool] = False,
save_filtered_errors: bool = False,
filter_deprecated: bool = False,
) -> None:
super().__init__(
errors,
filter_errors=filter_errors,
save_filtered_errors=save_filtered_errors,
filter_deprecated=filter_deprecated,
)
self.uselessness_errors = set()
self.unreachable_lines = set()
self.revealed_types = defaultdict(set)

def on_error(self, file: str, info: ErrorInfo) -> bool:

if info.code in (codes.UNREACHABLE, codes.REDUNDANT_EXPR):
self.uselessness_errors.add(
(info.code, info.message, info.line, info.column, info.end_line, info.end_column)
)
if info.code == codes.UNREACHABLE:
self.unreachable_lines.update(range(info.line, info.end_line + 1))
return True

if info.code == codes.MISC and info.message.startswith("Revealed type is "):
key = info.function_or_member, info.line, info.column, info.end_line, info.end_column
types = info.message.split('"')[1]
if types.startswith("Union["):
self.revealed_types[key].update(types[6:-1].split(", "))
else:
self.revealed_types[key].add(types)
return True

return super().on_error(file, info)


class Errors:
"""Container for compile errors.

Expand Down
4 changes: 2 additions & 2 deletions test-data/unit/check-inference.test
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ for var2 in [g, h, i, j, k, l]:
reveal_type(var2) # N: Revealed type is "Union[builtins.int, builtins.str]"

for var3 in [m, n, o, p, q, r]:
reveal_type(var3) # N: Revealed type is "Union[builtins.int, Any]"
reveal_type(var3) # N: Revealed type is "Union[Any, builtins.int]"

T = TypeVar("T", bound=Type[Foo])

Expand Down Expand Up @@ -1247,7 +1247,7 @@ class X(TypedDict):

x: X
for a in ("hourly", "daily"):
reveal_type(a) # N: Revealed type is "Union[Literal['hourly']?, Literal['daily']?]"
reveal_type(a) # N: Revealed type is "Union[Literal['daily']?, Literal['hourly']?]"
reveal_type(x[a]) # N: Revealed type is "builtins.int"
reveal_type(a.upper()) # N: Revealed type is "builtins.str"
c = a
Expand Down
41 changes: 38 additions & 3 deletions test-data/unit/check-narrowing.test
Original file line number Diff line number Diff line change
Expand Up @@ -2346,8 +2346,7 @@ def f() -> bool: ...

y = None
while f():
reveal_type(y) # N: Revealed type is "None" \
# N: Revealed type is "Union[builtins.int, None]"
reveal_type(y) # N: Revealed type is "Union[builtins.int, None]"
y = 1
reveal_type(y) # N: Revealed type is "Union[builtins.int, None]"

Expand All @@ -2370,7 +2369,20 @@ class A:

[builtins fixtures/primitives.pyi]

[case testAvoidFalseUnreachableInLoop]
[case testPersistentUnreachableLinesNestedInInpersistentUnreachableLines]
# flags: --warn-unreachable --python-version 3.11

x = None
y = None
while True:
if x is not None:
if y is not None:
reveal_type(y) # E: Statement is unreachable
x = 1

[builtins fixtures/bool.pyi]

[case testAvoidFalseUnreachableInLoop1]
# flags: --warn-unreachable --python-version 3.11

def f() -> int | None: ...
Expand All @@ -2383,6 +2395,29 @@ while x is not None or b():

[builtins fixtures/bool.pyi]

[case testAvoidFalseUnreachableInLoop2]
# flags: --warn-unreachable --python-version 3.11

y = None
while y is None:
if y is None:
y = []
y.append(1)

[builtins fixtures/list.pyi]

[case testAvoidFalseUnreachableInLoop3]
# flags: --warn-unreachable --python-version 3.11

xs: list[int | None]
y = None
for x in xs:
if x is not None:
if y is None:
y = {} # E: Need type annotation for "y" (hint: "y: Dict[<type>, <type>] = ...")

[builtins fixtures/list.pyi]

[case testAvoidFalseRedundantExprInLoop]
# flags: --enable-error-code redundant-expr --python-version 3.11

Expand Down
11 changes: 4 additions & 7 deletions test-data/unit/check-redefine2.test
Original file line number Diff line number Diff line change
Expand Up @@ -628,8 +628,7 @@ def f1() -> None:
def f2() -> None:
x = None
while int():
reveal_type(x) # N: Revealed type is "None" \
# N: Revealed type is "Union[None, builtins.str]"
reveal_type(x) # N: Revealed type is "Union[builtins.str, None]"
if int():
x = ""
reveal_type(x) # N: Revealed type is "Union[None, builtins.str]"
Expand Down Expand Up @@ -709,8 +708,7 @@ def b() -> None:
def c() -> None:
x = 0
while int():
reveal_type(x) # N: Revealed type is "builtins.int" \
# N: Revealed type is "Union[builtins.int, builtins.str, None]"
reveal_type(x) # N: Revealed type is "Union[builtins.int, builtins.str, None]"
if int():
x = ""
continue
Expand Down Expand Up @@ -810,8 +808,7 @@ def f4() -> None:
x = None
break
finally:
reveal_type(x) # N: Revealed type is "Union[builtins.int, builtins.str, None]" \
# N: Revealed type is "Union[builtins.int, None]"
reveal_type(x) # N: Revealed type is "Union[builtins.int, builtins.str, None]"
reveal_type(x) # N: Revealed type is "Union[builtins.int, None]"
[builtins fixtures/exception.pyi]

Expand Down Expand Up @@ -927,7 +924,7 @@ class X(TypedDict):

x: X
for a in ("hourly", "daily"):
reveal_type(a) # N: Revealed type is "Union[Literal['hourly']?, Literal['daily']?]"
reveal_type(a) # N: Revealed type is "Union[Literal['daily']?, Literal['hourly']?]"
reveal_type(x[a]) # N: Revealed type is "builtins.int"
reveal_type(a.upper()) # N: Revealed type is "builtins.str"
c = a
Expand Down
2 changes: 1 addition & 1 deletion test-data/unit/check-typevar-tuple.test
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,7 @@ from typing_extensions import Unpack

def pipeline(*xs: Unpack[Tuple[int, Unpack[Tuple[float, ...]], bool]]) -> None:
for x in xs:
reveal_type(x) # N: Revealed type is "Union[builtins.int, builtins.float]"
reveal_type(x) # N: Revealed type is "Union[builtins.float, builtins.int]"
[builtins fixtures/tuple.pyi]

[case testFixedUnpackItemInInstanceArguments]
Expand Down