From afe6d12cfa12b7ca8c7a2aec9b957ee399a27b55 Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Sun, 26 Jan 2025 13:36:31 -0500 Subject: [PATCH 1/7] Fix --warn-unreachable fall-through from ALWAYS_TRUE ifs Currently blocks marked as unreachable in `reachability.py` through semanal_pass1 are ignored by the `--warn-unreachable` flag through a `block.is_unreachable` check. Namely, reachability marks the if bodies and else-body as unreachable if it is not reachable on the current platform or Python version. This works for if-statements that have an else branch but if the if-statement has a short-circuiting early return or raise on a particular platform, then `reachability.py` places an empty else block and marks it unreachable but has no effect on the fall-through code. When `checker.py` correctly determines that the if-statement is always True and has a return inside it, it correctly marks any code after the if-statement as unreachable. This fix makes it so the code after if-statements that are known to be always true is marked reachable to surpress spurious `--warn-unreachable` warnings. --- mypy/checker.py | 8 +++++ mypy/nodes.py | 8 +++-- mypy/reachability.py | 1 + mypy/semanal_pass1.py | 4 +++ test-data/unit/check-unreachable-code.test | 34 ++++++++++++++++++++++ 5 files changed, 52 insertions(+), 3 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 3734f3170790..99aaa5b860e6 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -4871,6 +4871,14 @@ def visit_if_stmt(self, s: IfStmt) -> None: if s.else_body: self.accept(s.else_body) + # If this if-statement had a block that is considered always true for + # semantic reachability in semanal_pass1 and it has no else block; + # then we undo any unreachablilty properties for frames after the + # if-statement. These can be caused by early returns or raises and + # are only applicable on certain platforms or versions. + if s.has_pass1_always_true_block and (not s.else_body.body): + self.binder.frames[-1].unreachable = False + def visit_while_stmt(self, s: WhileStmt) -> None: """Type check a while statement.""" if_stmt = IfStmt([s.expr], [s.body], None) diff --git a/mypy/nodes.py b/mypy/nodes.py index 9364805d44d4..e9cf0a80437e 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -1523,19 +1523,21 @@ def accept(self, visitor: StatementVisitor[T]) -> T: class IfStmt(Statement): - __slots__ = ("expr", "body", "else_body") + __slots__ = ("expr", "body", "else_body", "has_pass1_always_true_block") - __match_args__ = ("expr", "body", "else_body") + __match_args__ = ("expr", "body", "else_body", "has_pass1_always_true_block") expr: list[Expression] body: list[Block] else_body: Block | None + pass1_always_true_block: bool - def __init__(self, expr: list[Expression], body: list[Block], else_body: Block | None) -> None: + def __init__(self, expr: list[Expression], body: list[Block], else_body: Block | None, has_pass1_always_true_block=False) -> None: super().__init__() self.expr = expr self.body = body self.else_body = else_body + self.has_pass1_always_true_block = has_pass1_always_true_block def accept(self, visitor: StatementVisitor[T]) -> T: return visitor.visit_if_stmt(self) diff --git a/mypy/reachability.py b/mypy/reachability.py index e69a857553d5..9bcb636cc984 100644 --- a/mypy/reachability.py +++ b/mypy/reachability.py @@ -57,6 +57,7 @@ def infer_reachability_of_if_statement(s: IfStmt, options: Options) -> None: # The condition is considered always false, so we skip the if/elif body. mark_block_unreachable(s.body[i]) elif result in (ALWAYS_TRUE, MYPY_TRUE): + s.has_pass1_always_true_block = True # This condition is considered always true, so all of the remaining # elif/else bodies should not be checked. if result == MYPY_TRUE: diff --git a/mypy/semanal_pass1.py b/mypy/semanal_pass1.py index aaa01969217a..98bc55c4cc3a 100644 --- a/mypy/semanal_pass1.py +++ b/mypy/semanal_pass1.py @@ -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: diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test index e6818ab5c3c7..914bb56f5fba 100644 --- a/test-data/unit/check-unreachable-code.test +++ b/test-data/unit/check-unreachable-code.test @@ -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: @@ -361,6 +372,29 @@ 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 testSysPlatformInFunctionEarlyReturnWithActualUnreachableCode] +# flags: --warn-unreachable +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 testSysPlatformInFunctionImport1] import sys def foo() -> None: From efe5b2afb2777c8ec900a441dfd0a9e7eee114fb Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 26 Jan 2025 18:56:31 +0000 Subject: [PATCH 2/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- mypy/nodes.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/mypy/nodes.py b/mypy/nodes.py index e9cf0a80437e..908d3f08f858 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -1532,7 +1532,13 @@ class IfStmt(Statement): else_body: Block | None pass1_always_true_block: bool - def __init__(self, expr: list[Expression], body: list[Block], else_body: Block | None, has_pass1_always_true_block=False) -> None: + def __init__( + self, + expr: list[Expression], + body: list[Block], + else_body: Block | None, + has_pass1_always_true_block=False, + ) -> None: super().__init__() self.expr = expr self.body = body From 2570bbee187adba615aee9b4c04b783feee037a7 Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Sun, 26 Jan 2025 13:59:45 -0500 Subject: [PATCH 3/7] Fix type checking issues --- mypy/checker.py | 2 +- mypy/nodes.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 99aaa5b860e6..137278f7627b 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -4876,7 +4876,7 @@ def visit_if_stmt(self, s: IfStmt) -> None: # then we undo any unreachablilty properties for frames after the # if-statement. These can be caused by early returns or raises and # are only applicable on certain platforms or versions. - if s.has_pass1_always_true_block and (not s.else_body.body): + if s.has_pass1_always_true_block and s.else_body and (not s.else_body.body): self.binder.frames[-1].unreachable = False def visit_while_stmt(self, s: WhileStmt) -> None: diff --git a/mypy/nodes.py b/mypy/nodes.py index 908d3f08f858..5c9c18b5f554 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -1537,7 +1537,7 @@ def __init__( expr: list[Expression], body: list[Block], else_body: Block | None, - has_pass1_always_true_block=False, + has_pass1_always_true_block: bool = False, ) -> None: super().__init__() self.expr = expr From 07e62f38fd18f1eb7d800c8fdbe69c23de5d27aa Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Sun, 26 Jan 2025 14:28:17 -0500 Subject: [PATCH 4/7] Use suppress_unreachable_warnings instead of marking reachable --- mypy/checker.py | 4 ++-- test-data/unit/check-unreachable-code.test | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 137278f7627b..d6e22433a7d5 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -4873,11 +4873,11 @@ def visit_if_stmt(self, s: IfStmt) -> None: # If this if-statement had a block that is considered always true for # semantic reachability in semanal_pass1 and it has no else block; - # then we undo any unreachablilty properties for frames after the + # then we surpress unreachable warnings for code after the # if-statement. These can be caused by early returns or raises and # are only applicable on certain platforms or versions. if s.has_pass1_always_true_block and s.else_body and (not s.else_body.body): - self.binder.frames[-1].unreachable = False + self.binder.suppress_unreachable_warnings() def visit_while_stmt(self, s: WhileStmt) -> None: """Type check a while statement.""" diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test index 914bb56f5fba..fa237717bc6f 100644 --- a/test-data/unit/check-unreachable-code.test +++ b/test-data/unit/check-unreachable-code.test @@ -383,7 +383,9 @@ def foo(self) -> int: [builtins fixtures/ops.pyi] [out] -[case testSysPlatformInFunctionEarlyReturnWithActualUnreachableCode] +-- TODO: We currently suppress unreachability warnings past semantic +-- reachability checks. This should be enabled and fixed. +[case testSysPlatformInFunctionEarlyReturnWithActualUnreachableCode-skip] # flags: --warn-unreachable import sys From 51756c023036863b10a3430592fe8554bf29f4c7 Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Sun, 26 Jan 2025 14:40:04 -0500 Subject: [PATCH 5/7] Add test case for running on fictional platform where unreachable gets triggered --- test-data/unit/check-unreachable-code.test | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test index fa237717bc6f..cf7640c1eccd 100644 --- a/test-data/unit/check-unreachable-code.test +++ b/test-data/unit/check-unreachable-code.test @@ -383,9 +383,21 @@ def foo(self) -> int: [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] + -- TODO: We currently suppress unreachability warnings past semantic --- reachability checks. This should be enabled and fixed. -[case testSysPlatformInFunctionEarlyReturnWithActualUnreachableCode-skip] +-- reachability checks. Need to decide if this should be fixed and enabled. +[case testSysPlatformEarlyReturnActualUnreachableCodeNotForPlatform-skip] # flags: --warn-unreachable import sys From 3d6ecd0b6548792a32d1e599318485dc614686e1 Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Sun, 26 Jan 2025 18:26:22 -0500 Subject: [PATCH 6/7] Use A5rock's approach of marking semanal unreachability. Co-authored-by: A5rocks --- mypy/binder.py | 39 +++++++++++++++++----- mypy/checker.py | 10 +----- mypy/nodes.py | 13 ++------ mypy/reachability.py | 1 - test-data/unit/check-unreachable-code.test | 7 ++-- 5 files changed, 38 insertions(+), 32 deletions(-) diff --git a/mypy/binder.py b/mypy/binder.py index 3d833153d628..33babdbd26c9 100644 --- a/mypy/binder.py +++ b/mypy/binder.py @@ -1,5 +1,6 @@ from __future__ import annotations +import enum from collections import defaultdict from collections.abc import Iterator from contextlib import contextmanager @@ -36,6 +37,11 @@ class CurrentType(NamedTuple): from_assignment: bool +class UnreachableType(enum.Enum): + 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 @@ -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 @@ -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 @@ -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: @@ -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 @@ -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: @@ -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: diff --git a/mypy/checker.py b/mypy/checker.py index d6e22433a7d5..0e9b8987c126 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -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(): @@ -4871,14 +4871,6 @@ def visit_if_stmt(self, s: IfStmt) -> None: if s.else_body: self.accept(s.else_body) - # If this if-statement had a block that is considered always true for - # semantic reachability in semanal_pass1 and it has no else block; - # then we surpress unreachable warnings for code after the - # if-statement. These can be caused by early returns or raises and - # are only applicable on certain platforms or versions. - if s.has_pass1_always_true_block and s.else_body and (not s.else_body.body): - self.binder.suppress_unreachable_warnings() - def visit_while_stmt(self, s: WhileStmt) -> None: """Type check a while statement.""" if_stmt = IfStmt([s.expr], [s.body], None) diff --git a/mypy/nodes.py b/mypy/nodes.py index 5c9c18b5f554..0b7675258c5a 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -1523,27 +1523,20 @@ def accept(self, visitor: StatementVisitor[T]) -> T: class IfStmt(Statement): - __slots__ = ("expr", "body", "else_body", "has_pass1_always_true_block") + __slots__ = ("expr", "body", "else_body") - __match_args__ = ("expr", "body", "else_body", "has_pass1_always_true_block") + __match_args__ = ("expr", "body", "else_body") expr: list[Expression] body: list[Block] else_body: Block | None pass1_always_true_block: bool - def __init__( - self, - expr: list[Expression], - body: list[Block], - else_body: Block | None, - has_pass1_always_true_block: bool = False, - ) -> None: + def __init__(self, expr: list[Expression], body: list[Block], else_body: Block | None) -> None: super().__init__() self.expr = expr self.body = body self.else_body = else_body - self.has_pass1_always_true_block = has_pass1_always_true_block def accept(self, visitor: StatementVisitor[T]) -> T: return visitor.visit_if_stmt(self) diff --git a/mypy/reachability.py b/mypy/reachability.py index 9bcb636cc984..e69a857553d5 100644 --- a/mypy/reachability.py +++ b/mypy/reachability.py @@ -57,7 +57,6 @@ def infer_reachability_of_if_statement(s: IfStmt, options: Options) -> None: # The condition is considered always false, so we skip the if/elif body. mark_block_unreachable(s.body[i]) elif result in (ALWAYS_TRUE, MYPY_TRUE): - s.has_pass1_always_true_block = True # This condition is considered always true, so all of the remaining # elif/else bodies should not be checked. if result == MYPY_TRUE: diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test index cf7640c1eccd..64b9cfb106d1 100644 --- a/test-data/unit/check-unreachable-code.test +++ b/test-data/unit/check-unreachable-code.test @@ -395,9 +395,7 @@ def foo() -> int: [builtins fixtures/ops.pyi] [out] --- TODO: We currently suppress unreachability warnings past semantic --- reachability checks. Need to decide if this should be fixed and enabled. -[case testSysPlatformEarlyReturnActualUnreachableCodeNotForPlatform-skip] +[case testSysPlatformEarlyReturnActualUnreachableCodeNotForPlatform] # flags: --warn-unreachable import sys @@ -405,7 +403,8 @@ def foo() -> int: if sys.platform != 'fictional': return 1 return 0 - return 0 + 1 # E: Statement is unreachable + return 0 + 1 # Will not throw `Statement is unreachable` because we are + # not on the fictional platform. [builtins fixtures/ops.pyi] [out] From ea87eef0643a3615c94c75b0764dc200c99b3d5a Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Sun, 26 Jan 2025 18:29:39 -0500 Subject: [PATCH 7/7] Remove old field type --- mypy/nodes.py | 1 - 1 file changed, 1 deletion(-) diff --git a/mypy/nodes.py b/mypy/nodes.py index 0b7675258c5a..9364805d44d4 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -1530,7 +1530,6 @@ class IfStmt(Statement): expr: list[Expression] body: list[Block] else_body: Block | None - pass1_always_true_block: bool def __init__(self, expr: list[Expression], body: list[Block], else_body: Block | None) -> None: super().__init__()