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. ✅ |
|
""" WalkthroughThe changes update the logic in the Changes
Poem
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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) |
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