extend C1802 to detect len() comparisons with zero #10658
Conversation
Add detection for len() comparisons with 0/1 that can be simplified: - len(x) == 0 → not x - len(x) > 0 → x (or bool(x)) - len(x) != 0 → x (or bool(x)) - Reversed operands: 0 == len(x), etc. Implementation: - Add _check_len_comparison_with_zero() method - Add _get_len_comparison_suggestion() for context-aware suggestions - Reuse existing inference and boolean context detection - Handle edge cases: chained comparisons, custom __bool__
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
- Added logic to identify when len() comparisons are nested within boolean operations or are directly returned.
for more information, see https://pre-commit.ci
| self._check_compare_to_str_or_zero(node) | ||
| if self.linter.is_message_enabled("use-implicit-booleaness-not-len"): | ||
| self._check_len_comparison_with_zero(node) |
There was a problem hiding this comment.
It seems highly likely that optimization or code deduplication can be found by re-using check_compare_to_str_or_zero here.
There was a problem hiding this comment.
From what i understood i have added _extract_comparison_operands() helper method that both _check_compare_to_str_or_zero() and _check_len_comparison_with_zero() now use.
There was a problem hiding this comment.
I have issued new commits resolving this
| if True or len('TEST'): # [use-implicit-booleaness-not-len] | ||
| pass | ||
|
|
||
| if len('TEST') == 0: # Should be fine |
There was a problem hiding this comment.
I think we need to check the initial reasoning for this before changing, I didn't have this context when labelling the initial issue a false negative.
Pierre-Sassoulas
added
the
False Negative 🦋
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (94.11%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #10658 +/- ##
==========================================
- Coverage 95.96% 95.96% -0.01%
==========================================
Files 176 176
Lines 19502 19604 +102
==========================================
+ Hits 18715 18812 +97
- Misses 787 792 +5
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
…th 0 - Add detection for len(iterable) == 0, != 0, > 0, >= 0, < 1, <= 0 patterns - Support reversed operands (0 == len(x), 0 < len(x), etc.) - Extract _extract_comparison_operands() helper to reduce code duplication - Fix critical bug in constant-on-left comparison logic (1 <= len(x) was wrong) - Add comprehensive test coverage for edge cases - Update documentation with new pattern examples - Fix too-many-returns linting issue by consolidating logic - All tests passing, 100% coverage for new functionality
for more information, see https://pre-commit.ci
|
Tbh I'm not sure we should emit a message for these. Yes technically they could be rewritten. However, being explicit is often times quite useful. |
|
The primer look especially bad too. |
Based on your feedback that "being explicit is often times quite useful," I understand that my current implementation might be too aggressive in flagging len() comparisons. Rather than catching all patterns like len(x) > 0 → x, should we focus only on the most obviously redundant cases (like len(x) == 0 → not x) while preserving explicit length checks that might have semantic value? |
We could check what the primer results look like with just the changes for |
|
We're basically discussing the reversion of 4714a65 #2684 / #2815 here. I should have looked into the history of this and discussed it in more detail before accepting the initial issue, sorry @Kaos599 The fact that the primer is bad might hint that it's useful to be reminded that we're basically checking that an iterator is empty (i.e. the primer show that the "mistake" is common, not that the check is opinionated). I think that with typing becoming more and more common the implicitness is less and less problematic, and also it's faster to cast bool than to cast len then compare the length to another value (so there's a performance advantage to do this). I think it's something that could be discussed in the official python discourse so that PEP8 become explicit about what it means when it says "For sequences, (strings, lists, tuples), use the fact that empty sequences are false." (i.e. does it recommends to change |
Pierre-Sassoulas
added
the
Needs decision 🔒
|
|
||
| # Now also catches comparisons with zero | ||
| if len(fruits) == 0: # [use-implicit-booleaness-not-len] | ||
| print("empty") | ||
|
|
||
| if len(fruits) > 0: # [use-implicit-booleaness-not-len] | ||
| print("has items") | ||
|
|
||
| if len(fruits) != 0: # [use-implicit-booleaness-not-len] | ||
| print("not empty") |
There was a problem hiding this comment.
No need to upgrade the doc with all the examples, we should keep it simple.
3 participants
Description
This PR extends the
use-implicit-booleaness-not-len(C1802) checker to detect additional patterns wherelen()is compared against zero or one, enabling more comprehensive detection of implicit booleaness violations.Changes Made
Core Implementation (
pylint/checkers/refactoring/implicit_booleaness_checker.py):_check_len_comparison_with_zero()method to detectlen()comparisons with constants_get_len_comparison_suggestion()helper for context-aware suggestion generationvisit_compare()to call the new detection logicuse-set-for-membershipandno-else-returnwarningsTest Updates:
tests/functional/u/use/use_implicit_booleaness_not_len.pywith new test casestests/functional/u/use/use_implicit_booleaness_not_len.txtwith expected violationsNew Detection Patterns
The checker now catches these previously missed patterns:
This enhancement significantly improves pylint's ability to catch implicit booleaness violations while maintaining the checker's existing behavior and performance characteristics.
Closes #10281