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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is a good question. I initially implemented it so in 229e4a6. However, in the last function definition of test case testNewRedefineTryStatement, the first response to reveal_type does provide complete information, but the second (and final) response does not:

def f4() -> None:
    while int():
        try:
            x = 1
            if int():
                x = ""
                break
            if int():
                while int():
                    if int():
                        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]"

I have no experience with the new --allow-redefinition-new option so far, and have not tried to investigate why this happens. I just extended my approach in c24c950.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't that case also allow something not allowed for str? Eg does that error for:

if x is not None:
  print(x + 5)

Above the reveal_type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, after re-reading, it seems that I have not hit the point of your question. Another try: There might be some scope for optimisations. However, I am unsure what can possibly happen when running Mypy with different configurations (like in the f4 example discussed above), so I preferred to implement this part of the approach as robustly as possible.

Copy link
Collaborator

@A5rocks A5rocks May 25, 2025

Choose a reason for hiding this comment

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

On the other hand it's possible to be too robust: mypy only prints the errors for the last round right? So it's possible (through some bug elsewhere, like forgetting to propagate line numbers maybe?) to have unreachable code in the last run that isn't marked -- and so have no signal that the code there isn't checked.

On the other other hand, just using the last iteration's unreachability errors might be wrong in presence of bugs like in f4, but it's simpler (and implementation is trivial to get right) and could never mislead the user about what code could get errors.


Basically I think it's better to match the semantics of how error reporting happens (ie during the last round of checking) then to have only one type of error be correct in face of bugs.


Edit: actually I might be wrong about my assumptions here, let me play with your PR a bit and I'll update this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well there seems to be a bug in your logic anyways (I can't see where, it looks right to me):

def f4() -> None:
    while int():
        try:
            x = 1
            if int():
                x = ""
                break
            if int():
                while int():
                    if int():
                        x = None
                        break
        finally:
            if isinstance(x, str):
                print("hello :)")  # E: Statement is unreachable
            reveal_type(x) # N: Revealed type is "Union[builtins.int, builtins.str, None]"

Anyways, turns out I was wrong about mypy only erroring in the final cycle. But the above bug should be enough for now and IMO a type becoming a subtype at the same place in the code seems like a bug.

@JukkaL since maybe some of my other assumptions are wrong. Do you think that looping over code multiple times but with subtypes should be considered a bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you certain? This is what I get:

# flags: --allow-redefinition-new --local-partial-types

def f4() -> None:
    while int():
        try:
            x = 1
            if int():
                x = ""
                break
            if int():
                while int():
                    if int():
                        x = None
                        break
        finally:
            if isinstance(x, str):
                reveal_type(x)  # N: Revealed type is "builtins.str"
            reveal_type(x)  # N: Revealed type is "Union[builtins.int, builtins.str, None]"

(I had to add isinstance to exception.pyi to get this running.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just added that to a repro.py file and ran mypy repro.py --allow-redefinition-new --local-partial-types --warn-unreachable but maybe I checked your branch out incorrectly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad; the -warn-unreachable flag was missing in the test case. Now I get both Statement is unreachable and Revealed type is "builtins.str". However, the same happens when I check the modified test with Mypy-Master. Therefore, I don't know if this actually points to a flaw in my current approach. I will investigate tomorrow....

):
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