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

Fix --warn-unreachable fall-through from ALWAYS_TRUE ifs #18539

Open
wants to merge 7 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
39 changes: 31 additions & 8 deletions mypy/binder.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import enum
from collections import defaultdict
from collections.abc import Iterator
from contextlib import contextmanager
Expand Down Expand Up @@ -36,6 +37,11 @@ class CurrentType(NamedTuple):
from_assignment: bool


class UnreachableType(enum.Enum):
Copy link
Collaborator

Choose a reason for hiding this comment

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

As part of #18707 I've been thinking about related concepts, and I think it's possible to do this without an enum actually: simply mark skipped blocks as suppressed (currently they're only marked as unreachable) and if all the blocks flowing into a block are unreachable, check if any of them are suppressed -- if so, then the current block is suppressed.

BINDER_UNREACHABLE = enum.auto()
SEMANAL_UNREACHABLE = enum.auto()


class Frame:
"""A Frame represents a specific point in the execution of a program.
It carries information about the current types of expressions at
Expand All @@ -51,7 +57,7 @@ class Frame:
def __init__(self, id: int, conditional_frame: bool = False) -> None:
self.id = id
self.types: dict[Key, CurrentType] = {}
self.unreachable = False
self.unreachable: UnreachableType | None = None
self.conditional_frame = conditional_frame
self.suppress_unreachable_warnings = False

Expand Down Expand Up @@ -161,8 +167,11 @@ def put(self, expr: Expression, typ: Type, *, from_assignment: bool = True) -> N
self._add_dependencies(key)
self._put(key, typ, from_assignment)

def unreachable(self) -> None:
self.frames[-1].unreachable = True
def unreachable(self, from_semanal: bool = False) -> None:
unreachable_type = UnreachableType.BINDER_UNREACHABLE
if from_semanal:
unreachable_type = UnreachableType.SEMANAL_UNREACHABLE
self.frames[-1].unreachable = unreachable_type

def suppress_unreachable_warnings(self) -> None:
self.frames[-1].suppress_unreachable_warnings = True
Expand All @@ -175,12 +184,22 @@ def get(self, expr: Expression) -> Type | None:
return None
return found.type

def is_unreachable(self) -> bool:
def is_unreachable(self) -> UnreachableType | None:
# TODO: Copy the value of unreachable into new frames to avoid
# this traversal on every statement?
return any(f.unreachable for f in self.frames)
unreachable_type = None
for f in self.frames:
if f.unreachable and not unreachable_type:
unreachable_type = f.unreachable
elif f.unreachable == UnreachableType.SEMANAL_UNREACHABLE:
unreachable_type = f.unreachable
return unreachable_type

def is_unreachable_warning_suppressed(self) -> bool:
# Do not report unreachable warnings from frames that were marked
# unreachable by the semanal_pass1.
if self.is_unreachable() == UnreachableType.SEMANAL_UNREACHABLE:
return True
return any(f.suppress_unreachable_warnings for f in self.frames)

def cleanse(self, expr: Expression) -> None:
Expand All @@ -202,6 +221,12 @@ def update_from_options(self, frames: list[Frame]) -> bool:
If a key is declared as AnyType, only update it if all the
options are the same.
"""
if all(f.unreachable for f in frames):
semanal_unreachable = any(
f.unreachable == UnreachableType.SEMANAL_UNREACHABLE for f in frames
)
self.unreachable(from_semanal=semanal_unreachable)

all_reachable = all(not f.unreachable for f in frames)
frames = [f for f in frames if not f.unreachable]
changed = False
Expand Down Expand Up @@ -262,8 +287,6 @@ def update_from_options(self, frames: list[Frame]) -> bool:
self._put(key, type, from_assignment=True)
changed = True

self.frames[-1].unreachable = not frames

return changed

def pop_frame(self, can_skip: bool, fall_through: int) -> Frame:
Expand Down Expand Up @@ -411,7 +434,7 @@ def allow_jump(self, index: int) -> None:
for f in self.frames[index + 1 :]:
frame.types.update(f.types)
if f.unreachable:
frame.unreachable = True
frame.unreachable = f.unreachable
self.options_on_return[index].append(frame)

def handle_break(self) -> None:
Expand Down
2 changes: 1 addition & 1 deletion mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -3021,7 +3021,7 @@ def visit_block(self, b: Block) -> None:
# This block was marked as being unreachable during semantic analysis.
# It turns out any blocks marked in this way are *intentionally* marked
# as unreachable -- so we don't display an error.
self.binder.unreachable()
self.binder.unreachable(from_semanal=True)
return
for s in b.body:
if self.binder.is_unreachable():
Expand Down
4 changes: 4 additions & 0 deletions mypy/semanal_pass1.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ def do_stuff() -> None:

The block containing 'import xyz' is unreachable in Python 3 mode. The import
shouldn't be processed in Python 3 mode, even if the module happens to exist.

Note: Blocks marked unreachable here will not be reported by the
`--warn-unreachable` option. They are considered intentionally unreachable,
such as platform and version checks.
"""

def visit_file(self, file: MypyFile, fnam: str, mod_id: str, options: Options) -> None:
Expand Down
47 changes: 47 additions & 0 deletions test-data/unit/check-unreachable-code.test
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,17 @@ def foo() -> None:
[builtins fixtures/ops.pyi]
[out]

[case testSysVersionInfoInFunctionEarlyReturn]
# flags: --warn-unreachable
import sys

def foo(self) -> int:
if sys.version_info >= (3, 5):
return 1
return 0
[builtins fixtures/ops.pyi]
[out]

[case testSysPlatformInMethod]
import sys
class C:
Expand All @@ -361,6 +372,42 @@ class C:
[builtins fixtures/ops.pyi]
[out]

[case testSysPlatformInFunctionEarlyReturn]
# flags: --warn-unreachable
import sys

def foo(self) -> int:
if sys.platform != 'fictional':
return 1
return 0
[builtins fixtures/ops.pyi]
[out]

[case testSysPlatformEarlyReturnActualUnreachableCodeForPlatform]
# flags: --warn-unreachable --platform fictional
import sys

def foo() -> int:
if sys.platform != 'fictional':
return 1
return 0
return 0 + 1 # E: Statement is unreachable
[builtins fixtures/ops.pyi]
[out]

[case testSysPlatformEarlyReturnActualUnreachableCodeNotForPlatform]
# flags: --warn-unreachable
import sys

def foo() -> int:
if sys.platform != 'fictional':
return 1
return 0
return 0 + 1 # Will not throw `Statement is unreachable` because we are
# not on the fictional platform.
[builtins fixtures/ops.pyi]
[out]

[case testSysPlatformInFunctionImport1]
import sys
def foo() -> None:
Expand Down
Loading