Clone bugfix/gh 18901 reachability #15
Conversation
… any stmt - there are no error reports. Make MYPY_* vs ALWAYS_* consistent.
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors mypy's condition value inference logic to explicitly handle unary negation and binary boolean operators using truth-table-driven evaluation, replacing prior ad-hoc negation tracking, with comprehensive unit test additions covering conditional evaluation, short-circuit logic, and unreachable code scenarios. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant infer_condition_value
participant Truth_Table
Caller->>infer_condition_value: expr: Expression
alt Unary Not
infer_condition_value->>infer_condition_value: Evaluate inner expression
infer_condition_value->>Truth_Table: inverted_truth_mapping
Truth_Table-->>infer_condition_value: Inverted result
else Binary And/Or
infer_condition_value->>infer_condition_value: Evaluate left side
infer_condition_value->>infer_condition_value: Evaluate right side
infer_condition_value->>Truth_Table: Look up (left, right, operator)
Truth_Table-->>infer_condition_value: Combined truth value
else Unsupported Op
infer_condition_value-->>Caller: TRUTH_VALUE_UNKNOWN
else Other Expr
infer_condition_value->>infer_condition_value: Direct evaluation
infer_condition_value-->>Caller: Result
end
infer_condition_value-->>Caller: int (truth value)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@refacto-visz |
codeant-ai
bot
added
the
size:L
fix: Improve reachability analysis for compound boolean expressionsTL;DR: This PR fixes a bug in mypy's reachability analysis by correctly evaluating compound boolean expressions involving Refacto PR SummaryThis change overhauls the logic within The
This enhancement allows mypy to more accurately detect unreachable code, preventing false positives and correctly identifying errors in code blocks that are truly reachable. Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant Mypy as Mypy Checker
participant R as reachability.py
Mypy->>R: infer_condition_value(expr='A and B', ...)
R->>R: Recurse: infer_condition_value(expr='A', ...)
R-->>R: Return truth_value_A
R->>R: Recurse: infer_condition_value(expr='B', ...)
R-->>R: Return truth_value_B
R->>R: Combine truth_value_A and truth_value_B based on 'and' logic
R-->>Mypy: Return final combined truth value
Testing GuideClick to expand
|
|
CodeAnt AI finished reviewing your PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the logic for inferring the truth value of conditional expressions in mypy/reachability.py, specifically for and, or, and not operators. The new implementation is more robust and correct, especially for binary logical operators, as it now evaluates both operands to determine the result. The handling of not expressions has also been simplified. The new test cases in test-data/unit/check-unreachable-code.test are thorough and provide good coverage for the changes. Overall, this is a solid improvement. I've left one minor suggestion to remove a redundant check.
| if expr.op not in ("or", "and"): | ||
| return TRUTH_VALUE_UNKNOWN | ||
|
|
There was a problem hiding this comment.
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.
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Reachability Logic Enhancements👍 Well Done
📁 Selected files for review (2)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/in/1889637?s=60&v=4){kind=small}
| 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 |
There was a problem hiding this comment.
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
| 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test-data/unit/check-unreachable-code.test (1)
553-577: Nice coverage of MYPY/TYPE_CHECKING in binary ops; consider adding a mixed-negation case
testMypyFalseValuesInBinaryOps_no_emptydoes a good job checking that:
TYPE_CHECKING/MYPYcombined withsys.platform == 'linux'underandproduce mypy-only guarded defs (foo1–foo4), and- the same under
orproduce always-taken branches that must satisfy normal return checking (bar1–bar4).Given the updated truth table, an additional case like:
if TYPE_CHECKING or not TYPE_CHECKING: ...(or a similar
MYPYvariant) would help pin down the behavior when mixing MYPY_TRUE and MYPY_FALSE in the sameorexpression, which is currently where the implementation has the most subtle edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mypy/reachability.py(1 hunks)test-data/unit/check-unreachable-code.test(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
mypy/reachability.py (1)
mypy/nodes.py (17)
expr(2290-2296)UnaryExpr(2052-2071)name(234-235)name(353-354)name(532-533)name(571-577)name(809-810)name(907-908)name(1068-1069)name(2287-2288)name(2572-2573)name(3205-3207)name(3752-3753)name(3864-3865)NameExpr(1875-1895)MemberExpr(1898-1914)OpExpr(2090-2133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Test suite with py313-ubuntu, mypyc-compiled
- GitHub Check: Formatting and code style with Black + ruff
- GitHub Check: mypyc runtime tests with py39-macos
- GitHub Check: Run mypy_primer (2)
- GitHub Check: Test suite with py312-ubuntu, mypyc-compiled
- GitHub Check: Run mypy_primer (3)
- GitHub Check: Type check our own code (py39-ubuntu)
- GitHub Check: Run mypy_primer (1)
- GitHub Check: Run mypy_primer (0)
- GitHub Check: Type check our own code (py39-windows-64)
- GitHub Check: Run mypy_primer (4)
- GitHub Check: Test suite with py39-windows-64
- GitHub Check: Test suite with py310-ubuntu
- GitHub Check: Test suite with py311-ubuntu, mypyc-compiled
- GitHub Check: Test suite with py39-ubuntu, mypyc-compiled
- GitHub Check: Test mypyc suite with 32-bit Python
🔇 Additional comments (4)
mypy/reachability.py (1)
118-121: Unarynothandling looks good and is simpler than the old negation trackingEarly-special-casing
UnaryExpr("not", ...)and inverting viainverted_truth_mappingkeeps the logic straightforward and correctly composes with the existingNameExpr/MemberExpr/ComparisonExprpaths. No issues spotted here.test-data/unit/check-unreachable-code.test (3)
503-523: Binary-op conditional tests nicely cover all true/false combinations
testConditionalValuesBinaryOpsexercises the full matrix ofand/orcombinations forsys.platform == 'linux'/'windows'under a fixed--platform linux, and the expectedreveal_typeresults line up with the new reachability logic for ALWAYS_TRUE vs ALWAYS_FALSE. Looks solid.
525-542: Negation tests give good coverage fornotover simple expressions andand/orgroups
testConditionalValuesNegationvalidates that unarynoton both primitive platform comparisons and groupedand/orexpressions produces the expected literal vs bool types, which should catch regressions in the newUnaryExpr("not")handling and its interaction with the boolean truth table. No issues here.
543-552: Unsupported-op tests correctly assert we don't over-special-case arbitrary operators
testConditionalValuesUnsupportedOpsensures that non-boolean operators like unary/binary-on conditions are left to normal type inference and not fed into the reachability truth table, with the expectedUnion[Literal[0], str]results. This is a useful guardrail against over-eager condition folding.
| if expr.op not in ("or", "and"): | ||
| return TRUTH_VALUE_UNKNOWN | ||
|
|
||
| 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 |
There was a problem hiding this comment.
🧩 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 theelif MYPY_TRUE in resultsbranch.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 viaresults <= {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 viaelif 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 -30Length 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 50Length 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 -A5Length 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 -C3Length 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:
-
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). -
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). -
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.
|
Diff from mypy_primer, showing the effect of this PR on open source code: pydantic (https://github.com/pydantic/pydantic)
- pydantic/_internal/_docs_extraction.py:104: error: Incompatible types in assignment (expression has type "list[str] | None", variable has type "list[str]") [assignment]
discord.py (https://github.com/Rapptz/discord.py)
- discord/ext/commands/hybrid.py:631: error: Overlap between argument names and ** TypedDict items: "name", "description" [misc]
+ discord/ext/commands/hybrid.py:631: error: Overlap between argument names and ** TypedDict items: "description", "name" [misc]
- discord/ext/commands/hybrid.py:836: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/hybrid.py:836: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/hybrid.py:860: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/hybrid.py:860: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/bot.py:290: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
- discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "name", "with_app_command" [misc]
+ discord/ext/commands/bot.py:314: error: Overlap between argument names and ** TypedDict items: "with_app_command", "name" [misc]
|
CodeAnt-AI Description
Handle combined boolean checks when inferring reachability
What Changed
not,and, andorexpressions by checking both operands so dual platform guards and their negations yield precise reachability outcomes instead of relying on short-circuit assumptionsImpact
✅ Clearer unreachable-code detection✅ More accurate missing-return reports for TYPE_CHECKING guards✅ Better type reveals for platform-dependent conditions💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.