Skip to content

Clone bugfix/gh 18901 reachability #15

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.

Sign up for GitHub

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
visz11 wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Clone bugfix/gh 18901 reachability #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cae4615
Make infer_condition_value recognize the whole truth table
sterliakov Apr 20, 2025
a500826
Test that we do not try to handle other operators
sterliakov Apr 21, 2025
648bd84
Remove short-circuiting logic here, it is safe to check truthiness of…
sterliakov Apr 21, 2025
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
47 changes: 29 additions & 18 deletions mypy/reachability.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,31 +115,44 @@ def infer_condition_value(expr: Expression, options: Options) -> int:
MYPY_TRUE if true under mypy and false at runtime, MYPY_FALSE if
false under mypy and true at runtime, else TRUTH_VALUE_UNKNOWN.
"""
if isinstance(expr, UnaryExpr) and expr.op == "not":
positive = infer_condition_value(expr.expr, options)
return inverted_truth_mapping[positive]

pyversion = options.python_version
name = ""
negated = False
alias = expr
if isinstance(alias, UnaryExpr):
if alias.op == "not":
expr = alias.expr
negated = True

result = TRUTH_VALUE_UNKNOWN
if isinstance(expr, NameExpr):
name = expr.name
elif isinstance(expr, MemberExpr):
name = expr.name
elif isinstance(expr, OpExpr) and expr.op in ("and", "or"):
if expr.op not in ("or", "and"):
return TRUTH_VALUE_UNKNOWN

Comment on lines +131 to +133
Copy link

@gemini-code-assist gemini-code-assist bot Nov 21, 2025

Choose a reason for hiding this comment

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

medium

This check is redundant because the elif condition on line 130 already ensures that expr.op is either 'and' or 'or'. You can remove these lines.

left = infer_condition_value(expr.left, options)
if (left in (ALWAYS_TRUE, MYPY_TRUE) and expr.op == "and") or (
left in (ALWAYS_FALSE, MYPY_FALSE) and expr.op == "or"
):
# Either `True and <other>` or `False or <other>`: the result will
# always be the right-hand-side.
return infer_condition_value(expr.right, options)
else:
# The result will always be the left-hand-side (e.g. ALWAYS_* or
# TRUTH_VALUE_UNKNOWN).
return left
right = infer_condition_value(expr.right, options)
results = {left, right}
if expr.op == "or":
if ALWAYS_TRUE in results:
return ALWAYS_TRUE
elif MYPY_TRUE in results:
return MYPY_TRUE
elif left == right == MYPY_FALSE:
return MYPY_FALSE
elif results <= {ALWAYS_FALSE, MYPY_FALSE}:
return ALWAYS_FALSE
elif expr.op == "and":
if ALWAYS_FALSE in results:
return ALWAYS_FALSE
elif MYPY_FALSE in results:
return MYPY_FALSE
elif left == right == ALWAYS_TRUE:
return ALWAYS_TRUE
elif results <= {ALWAYS_TRUE, MYPY_TRUE}:
return MYPY_TRUE
return TRUTH_VALUE_UNKNOWN
Comment on lines +137 to +155
Copy link

@refacto-visz refacto-visz bot Nov 21, 2025

Choose a reason for hiding this comment

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

Incorrect Boolean Combination Logic

The logic for combining truth values for or and and operations is incorrect for several combinations. For example, MYPY_TRUE or MYPY_FALSE should evaluate to ALWAYS_TRUE, but the code returns MYPY_TRUE. Similarly, MYPY_FALSE and MYPY_TRUE should be ALWAYS_FALSE, but it returns MYPY_FALSE. This introduces functional correctness bugs into the reachability analysis.

        if expr.op == "or":
            if ALWAYS_TRUE in results:
                return ALWAYS_TRUE
            if TRUTH_VALUE_UNKNOWN in results:
                return TRUTH_VALUE_UNKNOWN
            if results == {MYPY_TRUE, MYPY_FALSE}:
                return ALWAYS_TRUE
            if MYPY_TRUE in results:
                return MYPY_TRUE
            if MYPY_FALSE in results:
                return MYPY_FALSE
            return ALWAYS_FALSE  # Both operands are ALWAYS_FALSE
        elif expr.op == "and":
            if ALWAYS_FALSE in results:
                return ALWAYS_FALSE
            if TRUTH_VALUE_UNKNOWN in results:
                return TRUTH_VALUE_UNKNOWN
            if results == {MYPY_TRUE, MYPY_FALSE}:
                return ALWAYS_FALSE
            if MYPY_FALSE in results:
                return MYPY_FALSE
            if MYPY_TRUE in results:
                return MYPY_TRUE
            return ALWAYS_TRUE  # Both operands are ALWAYS_TRUE
        return TRUTH_VALUE_UNKNOWN
Commitable Suggestion
Suggested change
if expr.op == "or":
if ALWAYS_TRUE in results:
return ALWAYS_TRUE
elif MYPY_TRUE in results:
return MYPY_TRUE
elif left == right == MYPY_FALSE:
return MYPY_FALSE
elif results <= {ALWAYS_FALSE, MYPY_FALSE}:
return ALWAYS_FALSE
elif expr.op == "and":
if ALWAYS_FALSE in results:
return ALWAYS_FALSE
elif MYPY_FALSE in results:
return MYPY_FALSE
elif left == right == ALWAYS_TRUE:
return ALWAYS_TRUE
elif results <= {ALWAYS_TRUE, MYPY_TRUE}:
return MYPY_TRUE
return TRUTH_VALUE_UNKNOWN
if expr.op == "or":
if ALWAYS_TRUE in results:
return ALWAYS_TRUE
if TRUTH_VALUE_UNKNOWN in results:
return TRUTH_VALUE_UNKNOWN
if results == {MYPY_TRUE, MYPY_FALSE}:
return ALWAYS_TRUE
if MYPY_TRUE in results:
return MYPY_TRUE
if MYPY_FALSE in results:
return MYPY_FALSE
return ALWAYS_FALSE # Both operands are ALWAYS_FALSE
elif expr.op == "and":
if ALWAYS_FALSE in results:
return ALWAYS_FALSE
if TRUTH_VALUE_UNKNOWN in results:
return TRUTH_VALUE_UNKNOWN
if results == {MYPY_TRUE, MYPY_FALSE}:
return ALWAYS_FALSE
if MYPY_FALSE in results:
return MYPY_FALSE
if MYPY_TRUE in results:
return MYPY_TRUE
return ALWAYS_TRUE # Both operands are ALWAYS_TRUE
return TRUTH_VALUE_UNKNOWN
Standards
  • ISO-IEC-25010-Functional-Correctness-Appropriateness

Comment on lines +131 to +155
Copy link

@coderabbitai coderabbitai bot Nov 21, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Truth-table for and/or misclassifies some MYPY_TRUE/MYPY_FALSE combinations

The overall approach of combining left/right via a small truth table is solid, but a few edge combinations are classified inconsistently with the documented semantics of the flags:

  • For or:

    • left = MYPY_TRUE, right = MYPY_FALSE (e.g. TYPE_CHECKING or not TYPE_CHECKING) should be ALWAYS_TRUE (true in both mypy and at runtime), but this logic returns MYPY_TRUE because of the elif MYPY_TRUE in results branch.
    • left = ALWAYS_FALSE, right = MYPY_FALSE (e.g. COMPILE_TIME_FALSE or not TYPE_CHECKING) should be MYPY_FALSE, but this logic returns ALWAYS_FALSE via results <= {ALWAYS_FALSE, MYPY_FALSE}.

  • For and:

    • left = MYPY_TRUE, right = MYPY_FALSE (e.g. TYPE_CHECKING and not TYPE_CHECKING) should be ALWAYS_FALSE, but is currently classified as MYPY_FALSE via elif MYPY_FALSE in results.

The and misclassification currently behaves the same as ALWAYS_FALSE in infer_reachability_of_if_statement, but the or misclassification (ALWAYS_TRUE vs MYPY_TRUE) affects whether mark_block_mypy_only is applied, so conditions like TYPE_CHECKING or not TYPE_CHECKING will be treated as mypy-only instead of always-true.

Also, the inner if expr.op not in ("or", "and") check is unreachable given the surrounding expr.op in ("and", "or") guard.

Consider tightening this truth table so the four (mypy, runtime) combinations are combined exactly (possibly via a small mapping from code → (bool, bool) and back) and removing the redundant inner guard. Adding a regression test around TYPE_CHECKING or not TYPE_CHECKING (and a COMPILE_TIME_FALSE or not TYPE_CHECKING variant) would help lock in the intended behavior.

I'll verify the claims in the review comment by examining the code context, understanding the semantics of the constants, and checking the logic.


---

🏁 Script executed:

```shell
#!/bin/bash
# Examine the function containing the problematic code and surrounding context
cat mypy/reachability.py | head -200 | tail -100

Length of output: 3758

🏁 Script executed:

#!/bin/bash
# Search for the constant definitions to understand semantics
rg -n "ALWAYS_TRUE|MYPY_TRUE|ALWAYS_FALSE|MYPY_FALSE" mypy/reachability.py | head -30

Length of output: 1370

🏁 Script executed:

#!/bin/bash
# Look at the full function to understand the outer guard
rg -n "def infer_condition_value" mypy/reachability.py -A 50

Length of output: 2196

🏁 Script executed:

#!/bin/bash
# Find where this function is called and understand the pattern
rg -n "expr\.op in.*and.*or" mypy/reachability.py -B5 -A5

Length of output: 461

🏁 Script executed:

#!/bin/bash
# Search for TYPE_CHECKING related checks to understand the use cases
rg -n "TYPE_CHECKING" mypy/reachability.py -C3

Length of output: 284

Truth-table for and/or has logic errors causing incorrect classifications

Verification confirms three truth-table misclassifications in the or/and logic:

  1. TYPE_CHECKING or not TYPE_CHECKING (MYPY_TRUE, MYPY_FALSE) returns MYPY_TRUE but should return ALWAYS_TRUE—it's true both in mypy (T or F) and at runtime (F or T).

  2. COMPILE_TIME_FALSE or not TYPE_CHECKING (ALWAYS_FALSE, MYPY_FALSE) returns ALWAYS_FALSE but should return MYPY_FALSE—it's false in mypy (F or F) but true at runtime (F or T).

  3. TYPE_CHECKING and not TYPE_CHECKING (MYPY_TRUE, MYPY_FALSE) returns MYPY_FALSE but should return ALWAYS_FALSE—it's false in both mypy (T and F) and at runtime (F and T).

The or misclassification directly impacts mark_block_mypy_only application, causing TYPE_CHECKING or not TYPE_CHECKING to be treated as mypy-only instead of always-true. The inner guard at line 131 (if expr.op not in ("or", "and")) is unreachable due to the outer condition at line 130.

Consider refactoring the truth table to systematically enumerate all four (mypy, runtime) combinations and remove the redundant inner guard. Adding regression tests for TYPE_CHECKING or not TYPE_CHECKING and COMPILE_TIME_FALSE or not TYPE_CHECKING would prevent future regressions.

else:
result = consider_sys_version_info(expr, pyversion)
if result == TRUTH_VALUE_UNKNOWN:
Expand All @@ -155,8 +168,6 @@ def infer_condition_value(expr: Expression, options: Options) -> int:
result = ALWAYS_TRUE
elif name in options.always_false:
result = ALWAYS_FALSE
if negated:
result = inverted_truth_mapping[result]
return result


Expand Down
76 changes: 76 additions & 0 deletions test-data/unit/check-unreachable-code.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,82 @@ reveal_type(h) # N: Revealed type is "builtins.bool"
[builtins fixtures/ops.pyi]
[out]

[case testConditionalValuesBinaryOps]
# flags: --platform linux
import sys

t_and_t = (sys.platform == 'linux' and sys.platform == 'linux') and 's'
t_or_t = (sys.platform == 'linux' or sys.platform == 'linux') and 's'
t_and_f = (sys.platform == 'linux' and sys.platform == 'windows') and 's'
t_or_f = (sys.platform == 'linux' or sys.platform == 'windows') and 's'
f_and_t = (sys.platform == 'windows' and sys.platform == 'linux') and 's'
f_or_t = (sys.platform == 'windows' or sys.platform == 'linux') and 's'
f_and_f = (sys.platform == 'windows' and sys.platform == 'windows') and 's'
f_or_f = (sys.platform == 'windows' or sys.platform == 'windows') and 's'
reveal_type(t_and_t) # N: Revealed type is "Literal['s']"
reveal_type(t_or_t) # N: Revealed type is "Literal['s']"
reveal_type(f_and_t) # N: Revealed type is "builtins.bool"
reveal_type(f_or_t) # N: Revealed type is "Literal['s']"
reveal_type(t_and_f) # N: Revealed type is "builtins.bool"
reveal_type(t_or_f) # N: Revealed type is "Literal['s']"
reveal_type(f_and_f) # N: Revealed type is "builtins.bool"
reveal_type(f_or_f) # N: Revealed type is "builtins.bool"
[builtins fixtures/ops.pyi]

[case testConditionalValuesNegation]
# flags: --platform linux
import sys

not_t = not sys.platform == 'linux' and 's'
not_f = not sys.platform == 'windows' and 's'
not_and_t = not (sys.platform == 'linux' and sys.platform == 'linux') and 's'
not_and_f = not (sys.platform == 'linux' and sys.platform == 'windows') and 's'
not_or_t = not (sys.platform == 'linux' or sys.platform == 'linux') and 's'
not_or_f = not (sys.platform == 'windows' or sys.platform == 'windows') and 's'
reveal_type(not_t) # N: Revealed type is "builtins.bool"
reveal_type(not_f) # N: Revealed type is "Literal['s']"
reveal_type(not_and_t) # N: Revealed type is "builtins.bool"
reveal_type(not_and_f) # N: Revealed type is "Literal['s']"
reveal_type(not_or_t) # N: Revealed type is "builtins.bool"
reveal_type(not_or_f) # N: Revealed type is "Literal['s']"
[builtins fixtures/ops.pyi]

[case testConditionalValuesUnsupportedOps]
# flags: --platform linux
import sys

unary_minus = -(sys.platform == 'linux') and 's'
binary_minus = ((sys.platform == 'linux') - (sys.platform == 'linux')) and 's'
reveal_type(unary_minus) # N: Revealed type is "Union[Literal[0], builtins.str]"
reveal_type(binary_minus) # N: Revealed type is "Union[Literal[0], builtins.str]"
[builtins fixtures/ops.pyi]

[case testMypyFalseValuesInBinaryOps_no_empty]
# flags: --platform linux
import sys
from typing import TYPE_CHECKING

MYPY = 0

if TYPE_CHECKING and sys.platform == 'linux':
def foo1() -> int: ...
if sys.platform == 'linux' and TYPE_CHECKING:
def foo2() -> int: ...
if MYPY and sys.platform == 'linux':
def foo3() -> int: ...
if sys.platform == 'linux' and MYPY:
def foo4() -> int: ...

if TYPE_CHECKING or sys.platform == 'linux':
def bar1() -> int: ... # E: Missing return statement
if sys.platform == 'linux' or TYPE_CHECKING:
def bar2() -> int: ... # E: Missing return statement
if MYPY or sys.platform == 'linux':
def bar3() -> int: ... # E: Missing return statement
if sys.platform == 'linux' or MYPY:
def bar4() -> int: ... # E: Missing return statement
[builtins fixtures/ops.pyi]

[case testShortCircuitAndWithConditionalAssignment]
# flags: --platform linux
import sys
Expand Down
Loading