Skip to content

Make infer_condition_value recognize the whole truth table #1

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

Make infer_condition_value recognize the whole truth table #1

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"):
Copy link

Choose a reason for hiding this comment

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

Unhandled Operator Case

The function handles only 'and' and 'or' operators explicitly, but the redundant check suggests other operators were intended. This could cause unexpected behavior if other operators are passed.

Suggested change
elif isinstance(expr, OpExpr) and expr.op in ("and", "or"):
elif isinstance(expr, OpExpr):
if expr.op in ("and", "or"):
left = infer_condition_value(expr.left, options)
right = infer_condition_value(expr.right, options)
results = {left, right}
if expr.op == "or":
# ... existing or logic ...
else:
return TRUTH_VALUE_UNKNOWN
Standards
  • ISO-IEC-25010-Functional-Correctness-Completeness
  • ISO-IEC-25010-Reliability-Fault-Tolerance

if expr.op not in ("or", "and"):
return TRUTH_VALUE_UNKNOWN
Comment on lines +131 to +132
Copy link

Choose a reason for hiding this comment

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

medium

This check seems redundant, as the elif condition on line 130 already ensures that expr.op is either "and" or "or". Can this be removed to simplify the code?

Suggested change
if expr.op not in ("or", "and"):
return TRUTH_VALUE_UNKNOWN
left = infer_condition_value(expr.left, options)
right = infer_condition_value(expr.right, options)

Comment on lines +131 to +132
Copy link

Choose a reason for hiding this comment

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

Redundant Operator Check Creating Unreachable Code

Redundant check for operator type after already confirming expr.op is 'and' or 'or' at line 130. This creates unreachable code since the outer if-statement already ensures expr.op is either 'and' or 'or'.

Suggested change
if expr.op not in ("or", "and"):
return TRUTH_VALUE_UNKNOWN
left = infer_condition_value(expr.left, options)
right = infer_condition_value(expr.right, options)
Standards
  • ISO-IEC-25010-Reliability-Maturity
  • DbC-Code-Clarity

Comment on lines +131 to +132
Copy link

Choose a reason for hiding this comment

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

Redundant Condition Check

Redundant check for operators not in ("or", "and") after already confirming expr.op is in ("and", "or"). This dead code branch is unreachable, causing unnecessary complexity.

Standards
  • ISO-IEC-25010-Functional-Correctness-Appropriateness
  • ISO-IEC-25010-Reliability-Maturity


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
Comment on lines +137 to +154
Copy link

Choose a reason for hiding this comment

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

Complex Conditional Logic

Complex nested conditionals with multiple return paths make the truth table logic difficult to maintain. Consider using a lookup table or mapping approach for better readability and easier future modifications.

Standards
  • Clean-Code-Complexity
  • Design-Pattern-Table-Lookup

return TRUTH_VALUE_UNKNOWN
Comment on lines +137 to +155
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Incorrect truth‑table for or when mixing ALWAYS_FALSE and MYPY_FALSE

Example:
cond = ALWAYS_FALSE or MYPY_FALSE

• mypy sees False or False -> False
• runtime sees False or True -> True
→ The result should be MYPY_FALSE, but the current logic returns ALWAYS_FALSE via the
results <= {ALWAYS_FALSE, MYPY_FALSE} branch.

A minimal fix is to short‑circuit on MYPY_FALSE before the subset test:

@@
         if expr.op == "or":
             if ALWAYS_TRUE in results:
                 return ALWAYS_TRUE
             elif MYPY_TRUE in results:
                 return MYPY_TRUE
+            # If at least one side is MYPY_FALSE the overall value is
+            # false for mypy but true at runtime.
+            elif MYPY_FALSE in results:
+                return MYPY_FALSE
-            elif left == right == MYPY_FALSE:
-                return MYPY_FALSE
-            elif results <= {ALWAYS_FALSE, MYPY_FALSE}:
-                return ALWAYS_FALSE
+            elif results == {ALWAYS_FALSE}:
+                return ALWAYS_FALSE

This preserves the existing left == right == MYPY_FALSE optimisation and fixes the mixed case.

(Optional) : consider extracting a small helper (e.g. combine_truth_values(op, left, right)) so the full 5‑state truth table is unit‑tested and easier to reason about.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

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
elif MYPY_TRUE in results:
return MYPY_TRUE
# If at least one side is MYPY_FALSE the overall value is
# false for mypy but may be true at runtime.
elif MYPY_FALSE in results:
return MYPY_FALSE
elif results == {ALWAYS_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

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

Comment on lines 171 to 172
Copy link

Choose a reason for hiding this comment

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

Negation Logic Redundancy

The 'negated' flag is never set in the code but is used in this condition. This creates dead code that will never execute, potentially masking logical errors in negation handling.

Standards
  • Logic-Verification-Dead-Code
  • Algorithm-Correctness-Control-Flow


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