Make infer_condition_value recognize the whole truth table #1
Conversation
… any stmt - there are no error reports. Make MYPY_* vs ALWAYS_* consistent.
|
Diff from mypy_primer, showing the effect of this PR on open source code: vision (https://github.com/pytorch/vision)
+ torchvision/utils.py:271: error: Unused "type: ignore" comment [unused-ignore]
aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/client.py:400:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:400:9: note: See https://mypy.rtfd.io/en/stable/_refs.html#code-empty-body for more info
+ aiohttp/client.py:1092:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1098:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1104:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1110:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1116:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1122:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1128:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1383:5: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:353:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:357:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:363:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:369:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:375:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:381:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:387:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:393:9: error: Missing return statement [empty-body]
|
1 similar comment
|
Diff from mypy_primer, showing the effect of this PR on open source code: vision (https://github.com/pytorch/vision)
+ torchvision/utils.py:271: error: Unused "type: ignore" comment [unused-ignore]
aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/client.py:400:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:400:9: note: See https://mypy.rtfd.io/en/stable/_refs.html#code-empty-body for more info
+ aiohttp/client.py:1092:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1098:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1104:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1110:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1116:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1122:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1128:9: error: Missing return statement [empty-body]
+ aiohttp/client.py:1383:5: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:353:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:357:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:363:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:369:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:375:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:381:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:387:9: error: Missing return statement [empty-body]
+ aiohttp/test_utils.py:393:9: error: Missing return statement [empty-body]
|
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
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. Warning Rate limit exceeded@arvi18 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
""" WalkthroughThe changes update the logic in the Changes
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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)
mypy/reachability.py (1)
131-133: Redundant guard – dead codeThe
if expr.op not in ("or", "and"):check can never be true, because the surroundingelifalready guaranteesexpr.op in ("and", "or").
Removing it will simplify the flow and silence future linters.- if expr.op not in ("or", "and"): - return TRUTH_VALUE_UNKNOWN
📜 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)
🔇 Additional comments (5)
mypy/reachability.py (1)
118-121: Negation branch looks solidRecursively delegating to
infer_condition_value()and then using theinverted_truth_mappingis much simpler than the old flag‑based approach – nice clean‑up!test-data/unit/check-unreachable-code.test (4)
503-523: Great coverage of binaryand/orcasesThe new tests exercise the full matrix of (true/false)×(true/false) platform checks and catch regressions nicely. 👍
525-541: Negation tests are spot‑onGood to see both direct and grouped negations covered – this verifies the new
notbranch you added inreachability.py.
547-551: Edge‑case arithmetic on booleans is intentionally coveredThese cases look weird at first glance, but they highlight fallback behaviour when we leave the supported boolean‑operator set. Nice inclusion.
554-577: Useful MYPY / TYPE_CHECKING combinationsThese scenarios ensure the new truth‑table still interacts correctly with compile‑time‑only constants. Test expectations match the intended semantics.
| 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.
🛠️ Refactor suggestion
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_FALSEThis 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.
| 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
mypy/reachability.py (1)
134-155: 🛠️ Refactor suggestionBetter handling of binary logical operators
The new implementation provides more comprehensive logic for handling both operands in binary operations, correctly evaluating all combinations of truth values. This addresses the previous issue noted in code reviews where mixed ALWAYS_FALSE and MYPY_FALSE values were incorrectly processed.
However, there's still a potential issue with the truth table for
oroperator. When MYPY_FALSE is in results but not equal on both sides, it should still return MYPY_FALSE as per the previous comment.if expr.op == "or": if ALWAYS_TRUE in results: return ALWAYS_TRUE elif MYPY_TRUE in results: return MYPY_TRUE + 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
🧹 Nitpick comments (1)
mypy/reachability.py (1)
131-132: Redundant conditional checkThis conditional check is unreachable because we're already inside a block that checks for
expr.op in ("and", "or")on line 130.- if expr.op not in ("or", "and"): - return TRUTH_VALUE_UNKNOWN
📜 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)
🔇 Additional comments (5)
mypy/reachability.py (1)
118-120: Good improvement to handle negation directlyThis change simplifies the logic by recursively evaluating the negated expression and looking up its opposite truth value, which is more readable and maintainable than using a flag.
test-data/unit/check-unreachable-code.test (4)
503-523: Good test cases for binary logical operationsThese test cases comprehensively cover the various combinations of truth values in binary operations, which is essential for verifying the improved behavior of the
infer_condition_valuefunction.
525-541: Good test coverage for negation handlingThese tests effectively verify the negation handling logic, ensuring that expressions like
not sys.platform == 'linux'are properly evaluated under different conditions.
543-551: Good tests for unsupported operationsThese test cases verify the behavior when unsupported operations are applied to boolean expressions, ensuring proper type inference.
553-577: Comprehensive tests for MYPY and TYPE_CHECKING in binary operationsThese test cases verify the behavior when mypy-specific values (TYPE_CHECKING, MYPY) are used in binary operations with platform checks. The tests ensure that functions in unreachable blocks are properly detected.
|
/gemini review |
![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
The pull request aims to improve the accuracy of condition evaluation in mypy, particularly for logical negation and binary logical expressions. The changes involve modifications to the infer_condition_value function in mypy/reachability.py and the addition of new test cases in test-data/unit/check-unreachable-code.test. Overall, the changes seem well-structured and address the identified issue. However, there are a few areas that could benefit from further refinement.
Summary of Findings
- Truth table completeness: The updated code in
infer_condition_valuenow recognizes the whole truth table forandandoroperations, leading to more accurate detection of unreachable code. - Test coverage: The added test cases cover various scenarios, including platform checks, logical operators, and negations, ensuring the correct behavior of the modified code.
- Redundant check: The check
if expr.op not in ("or", "and")on line 131 inmypy/reachability.pyseems redundant, as the function already handles only 'and' and 'or' operations in the precedingelifcondition.
Merge Readiness
The pull request is almost ready for merging. The added functionality and tests are valuable additions. However, the redundant check identified in mypy/reachability.py should be addressed before merging. Additionally, consider the question raised in the pull request description regarding boolean literals. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
| if expr.op not in ("or", "and"): | ||
| return TRUTH_VALUE_UNKNOWN |
There was a problem hiding this comment.
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?
| 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) |
|
/do-refacto |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
/refacto-test |
🤖 Refacto.AI: PR Summary & Reviewer GuideWarning: Requirements context (e.g., linked ticket) was not provided. Analysis is based solely on PR metadata and code diff, which may be incomplete. 🎯 1. The Goal (Why was this PR raised?)This PR aims to enhance the 🗺️ 2. The Changes (What was implemented?)
🤔 3. Key Areas for Human ReviewArea of Concern: Core Logic Change in infer_condition_value
Area of Concern: Handling of Special Cases
Area of Concern: Comprehensive Test Coverage
|
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
PR already reviewed at the latest commit: 648bd84. |
1 similar comment
|
PR already reviewed at the latest commit: 648bd84. |
|
/refacto-test |
|
PR already reviewed at the latest commit: 648bd84. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Boolean Logic Evaluation in Reachability Analysis👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| if expr.op not in ("or", "and"): | ||
| return TRUTH_VALUE_UNKNOWN |
There was a problem hiding this comment.
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'.
| 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
| return result | ||
|
|
There was a problem hiding this comment.
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
| name = expr.name | ||
| elif isinstance(expr, MemberExpr): | ||
| name = expr.name | ||
| elif isinstance(expr, OpExpr) and expr.op in ("and", "or"): |
There was a problem hiding this comment.
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.
| 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
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Code Review: Conditional Logic Evaluation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| if expr.op not in ("or", "and"): | ||
| return TRUTH_VALUE_UNKNOWN |
There was a problem hiding this comment.
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
| 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 |
There was a problem hiding this comment.
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
3 participants
Fixes python#18901.
Side question: why don't we support boolean literals there (as in
if True or sys.platform == "linux")? Is it an escape hatch to make mypy check some {platform,version}-dependent code on all {platform,version}s?Summary by CodeRabbit
Bug Fixes
Tests