Skip to content
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

Avoid false unreachable and redundant-expr warnings in loops. #18433

Merged
merged 3 commits into from
Jan 12, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
30 changes: 25 additions & 5 deletions mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -584,24 +584,44 @@ def accept_loop(
*,
exit_condition: Expression | None = None,
) -> None:
"""Repeatedly type check a loop body until the frame doesn't change.
If exit_condition is set, assume it must be False on exit from the loop.
"""Repeatedly type check a loop body until the frame doesn't change."""

Then check the else_body.
"""
# The outer frame accumulates the results of all iterations
# The outer frame accumulates the results of all iterations:
with self.binder.frame_context(can_skip=False, conditional_frame=True):

# Check for potential decreases in the number of partial types so as not to stop the
# iteration too early:
partials_old = sum(len(pts.map) for pts in self.partial_types)

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

while True:
with self.binder.frame_context(can_skip=True, break_frame=2, continue_frame=1):
self.accept(body)
partials_new = sum(len(pts.map) for pts in self.partial_types)
if (partials_new == partials_old) and not self.binder.last_pop_changed:
break
partials_old = partials_new

# If necessary, reset the modified options and make up for the postponed error checks:
if warn_unreachable or warn_redundant:
Copy link
Member

Choose a reason for hiding this comment

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

IMO this if and the two ifs above are only making the logic more obscure. It should be simply: store, set, restore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the two ifs above (one of them by using set.discard instead of set.remove). However, removing the third if would mean unnecessary calls of self.accept(body) in cases where neither unreachable nor redundant-expr are enabled. But I adjusted the latter according to @hauntsaninja's suggestion, which might look a little clearer.

self.options.warn_unreachable = warn_unreachable
self.options.enabled_error_codes.add(codes.REDUNDANT_EXPR)
with self.binder.frame_context(can_skip=True, break_frame=2, continue_frame=1):
self.accept(body)
hauntsaninja marked this conversation as resolved.
Show resolved Hide resolved

# If exit_condition is set, assume it must be False on exit from the loop:
if exit_condition:
_, else_map = self.find_isinstance_check(exit_condition)
self.push_type_map(else_map)

# Check the else body:
if else_body:
self.accept(else_body)

Expand Down
26 changes: 26 additions & 0 deletions test-data/unit/check-narrowing.test
Original file line number Diff line number Diff line change
Expand Up @@ -2390,3 +2390,29 @@ class A:
z.append(1)

[builtins fixtures/primitives.pyi]

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

def f() -> int | None: ...
def b() -> bool: ...

x: int | None
x = 1
while x is not None or b():
x = f()

[builtins fixtures/bool.pyi]

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

def f() -> int | None: ...
def b() -> bool: ...

x: int | None
x = 1
while x is not None and b():
x = f()

[builtins fixtures/primitives.pyi]
Loading