[pylint] Implement use-implicit-booleaness-not-len (PLC1802)
#14309
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLC1802 | 16 | 16 | 0 | 0 | 0 |
Lokejoke
changed the title
C1802)C1802)
Lokejoke
changed the title
C1802)[pylint] Implement use-implicit-booleaness-not-len (C1802)
Lokejoke
changed the title
[pylint] Implement use-implicit-booleaness-not-len (C1802)[pylint] Implement use-implicit-booleaness-not-len (C1802)
Lokejoke
changed the title
[pylint] Implement use-implicit-booleaness-not-len (C1802)[pylint] Implement use-implicit-booleaness-not-len (PLC1802)
crates/ruff_linter/resources/test/fixtures/pylint/len_as_condition.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Could you test some more complicated examples involving rebindings/scope since you use those properties of the semantic model? Maybe something like:
def f(cond:bool):
x = [1,2,3]
if cond:
x = [4,5,6]
if len(x):
print(x)and
def g(cond:bool):
x = [1,2,3]
if cond:
x = [4,5,6]
if len(x):
print(x)
del xand
def h(cond:bool):
x = [1,2,3]
x = 123
if len(x):
print(x)and
def outer():
x = [1,2,3]
def inner(x:int):
return x+1
if len(x):
print(x)or any others you can think of to exercise the implementation.
There was a problem hiding this comment.
Good point, this implementation does support only one name assignment in current scope.
It would be fair to check all prior assignments to this.
There was a problem hiding this comment.
I'm unsure, there are 3 options:
- check only those without rebindings (as is now)
- check recursively if all of possible rebindings bounds to a sequence-like object
- leave it for type inference
dylwil3
added
rule
There was a problem hiding this comment.
This looks really great, thank you! I think even with the possible misses around control flow, this makes sense to implement. You would have to be pretty pathological to create a custom class C that implements __len__ and __bool__ where bool(x) != bool(len(x)) for x: C... so I think it's okay.
| | PythonType::List | ||
| | PythonType::Set | ||
| | PythonType::Tuple | ||
| | PythonType::Generator |
There was a problem hiding this comment.
Do generators have a length? I think len((x for x in range(10)) gives a TypeError, whereas (x for x in range(10)) is truthy, so these would not have the same behavior in a boolean test.
|
|
||
| // Attempt to find the binding's value | ||
| let Some(binding_value) = find_binding_value(binding, semantic) else { | ||
| // check for `vararg` |
There was a problem hiding this comment.
Nice! Should we also see if its bound to kwargs? Those should have a length too.
| // Match against specific built-in constructors that return sequences | ||
| return semantic | ||
| .resolve_builtin_symbol(func) | ||
| .is_some_and(|func| matches!(func, "list" | "set" | "dict" | "tuple")); |
There was a problem hiding this comment.
How about range, str, repr, or other builtins that return sets, dicts, etc.?
There was a problem hiding this comment.
I'm gonna explore that, good point.
Summary
This PR implements
use-implicit-booleaness-not-len/C1802Test Plan
Checked against pylint tests:
Notes
len-as-conditionmight be solution)References
PEP 8 on empty sequences and implicit booleaness