[pylint] Fix PLC1802 autofix creating a syntax error and mark autofix as unsafe if there's comments in the len call (#18836)

<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
  requests.)
- Does this pull request include references to any relevant issues?
-->

## Summary
I've also found another bug while fixing this, where the diagnostic
would not trigger if the `len` call argument variable was shadowed. This
fixed a few false negatives in the test cases.
Example:
```python
fruits = []
fruits = []
if len(fruits):  # comment
    ...
```

Fixes #18811
Fixes #18812
<!-- What's the purpose of the change? What does it do, and why? -->

## Test Plan
Add regression test
<!-- How was it tested? -->

---------

Co-authored-by: Charlie Marsh <crmarsh416@gmail.com>
This commit is contained in:
Victor Hugo Gomes 2025-06-22 21:32:57 -03:00 committed by GitHub
parent cfec89e8c3
commit 06a78d0bd0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 154 additions and 10 deletions

View file

@ -168,7 +168,7 @@ def github_issue_1879():
def function_returning_function(r): def function_returning_function(r):
return function_returning_generator(r) return function_returning_generator(r)
assert len(function_returning_list(z)) # [PLC1802] differs from pylint assert len(function_returning_list(z)) # [PLC1802] differs from pylint
assert len(function_returning_int(z)) assert len(function_returning_int(z))
# This should raise a PLC1802 once astroid can infer it # This should raise a PLC1802 once astroid can infer it
# See https://github.com/pylint-dev/pylint/pull/3821#issuecomment-743771514 # See https://github.com/pylint-dev/pylint/pull/3821#issuecomment-743771514
@ -196,7 +196,7 @@ def f(cond:bool):
def g(cond:bool): def g(cond:bool):
x = [1,2,3] x = [1,2,3]
if cond: if cond:
x = [4,5,6] x = [4,5,6]
if len(x): # this should be addressed if len(x): # this should be addressed
print(x) print(x)
del x del x
@ -236,3 +236,15 @@ def j():
# regression tests for https://github.com/astral-sh/ruff/issues/14690 # regression tests for https://github.com/astral-sh/ruff/issues/14690
bool(len(ascii(1))) bool(len(ascii(1)))
bool(len(sorted(""))) bool(len(sorted("")))
# regression tests for https://github.com/astral-sh/ruff/issues/18811
fruits = []
if(len)(fruits):
...
# regression tests for https://github.com/astral-sh/ruff/issues/18812
fruits = []
if len(
fruits # comment
):
...

View file

@ -1,11 +1,13 @@
use ruff_diagnostics::Applicability;
use ruff_macros::{ViolationMetadata, derive_message_formats}; use ruff_macros::{ViolationMetadata, derive_message_formats};
use ruff_python_ast::{self as ast, Expr, ExprCall}; use ruff_python_ast::{self as ast, Expr, ExprCall};
use ruff_python_semantic::SemanticModel;
use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType}; use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType};
use ruff_python_semantic::analyze::typing::find_binding_value; use ruff_python_semantic::analyze::typing::find_binding_value;
use ruff_python_semantic::{BindingId, SemanticModel};
use ruff_text_size::Ranged; use ruff_text_size::Ranged;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
use crate::fix::edits;
use crate::fix::snippet::SourceCodeSnippet; use crate::fix::snippet::SourceCodeSnippet;
use crate::{AlwaysFixableViolation, Edit, Fix}; use crate::{AlwaysFixableViolation, Edit, Fix};
@ -41,6 +43,19 @@ use crate::{AlwaysFixableViolation, Edit, Fix};
/// print(vegetables) /// print(vegetables)
/// ``` /// ```
/// ///
/// ## Fix safety
/// This rule's fix is marked as unsafe when the `len` call includes a comment,
/// as the comment would be removed.
///
/// For example, the fix would be marked as unsafe in the following case:
/// ```python
/// fruits = []
/// if len(
/// fruits # comment
/// ):
/// ...
/// ```
///
/// ## References /// ## References
/// [PEP 8: Programming Recommendations](https://peps.python.org/pep-0008/#programming-recommendations) /// [PEP 8: Programming Recommendations](https://peps.python.org/pep-0008/#programming-recommendations)
#[derive(ViolationMetadata)] #[derive(ViolationMetadata)]
@ -98,10 +113,17 @@ pub(crate) fn len_test(checker: &Checker, call: &ExprCall) {
}, },
call.range(), call.range(),
) )
.set_fix(Fix::safe_edit(Edit::range_replacement( .set_fix(Fix::applicable_edit(
replacement, Edit::range_replacement(
call.range(), edits::pad(replacement, call.range(), checker.locator()),
))); call.range(),
),
if checker.comment_ranges().intersects(call.range()) {
Applicability::Unsafe
} else {
Applicability::Safe
},
));
} }
fn is_indirect_sequence(expr: &Expr, semantic: &SemanticModel) -> bool { fn is_indirect_sequence(expr: &Expr, semantic: &SemanticModel) -> bool {
@ -110,12 +132,11 @@ fn is_indirect_sequence(expr: &Expr, semantic: &SemanticModel) -> bool {
}; };
let scope = semantic.current_scope(); let scope = semantic.current_scope();
let bindings: Vec<BindingId> = scope.get_all(name).collect(); let Some(binding_id) = scope.get(name) else {
let [binding_id] = bindings.as_slice() else {
return false; return false;
}; };
let binding = semantic.binding(*binding_id); let binding = semantic.binding(binding_id);
// Attempt to find the binding's value // Attempt to find the binding's value
let Some(binding_value) = find_binding_value(binding, semantic) else { let Some(binding_value) = find_binding_value(binding, semantic) else {

View file

@ -396,6 +396,47 @@ len_as_condition.py:130:12: PLC1802 [*] `len(set((w + 1) for w in set()))` used
132 132 | 132 132 |
133 133 | import numpy 133 133 | import numpy
len_as_condition.py:193:8: PLC1802 [*] `len(x)` used as condition without comparison
|
191 | if cond:
192 | x = [4,5,6]
193 | if len(x): # this should be addressed
| ^^^^^^ PLC1802
194 | print(x)
|
= help: Remove `len`
Safe fix
190 190 | x = [1,2,3]
191 191 | if cond:
192 192 | x = [4,5,6]
193 |- if len(x): # this should be addressed
193 |+ if x: # this should be addressed
194 194 | print(x)
195 195 |
196 196 | def g(cond:bool):
len_as_condition.py:200:8: PLC1802 [*] `len(x)` used as condition without comparison
|
198 | if cond:
199 | x = [4,5,6]
200 | if len(x): # this should be addressed
| ^^^^^^ PLC1802
201 | print(x)
202 | del x
|
= help: Remove `len`
Safe fix
197 197 | x = [1,2,3]
198 198 | if cond:
199 199 | x = [4,5,6]
200 |- if len(x): # this should be addressed
200 |+ if x: # this should be addressed
201 201 | print(x)
202 202 | del x
203 203 |
len_as_condition.py:214:8: PLC1802 [*] `len(x)` used as condition without comparison len_as_condition.py:214:8: PLC1802 [*] `len(x)` used as condition without comparison
| |
212 | def inner(x:int): 212 | def inner(x:int):
@ -416,6 +457,26 @@ len_as_condition.py:214:8: PLC1802 [*] `len(x)` used as condition without compar
216 216 | 216 216 |
217 217 | def redefined(): 217 217 | def redefined():
len_as_condition.py:220:8: PLC1802 [*] `len(x)` used as condition without comparison
|
218 | x = 123
219 | x = [1, 2, 3]
220 | if len(x): # this should be addressed
| ^^^^^^ PLC1802
221 | print(x)
|
= help: Remove `len`
Safe fix
217 217 | def redefined():
218 218 | x = 123
219 219 | x = [1, 2, 3]
220 |- if len(x): # this should be addressed
220 |+ if x: # this should be addressed
221 221 | print(x)
222 222 |
223 223 | global_seq = [1, 2, 3]
len_as_condition.py:233:8: PLC1802 [*] `len(x)` used as condition without comparison len_as_condition.py:233:8: PLC1802 [*] `len(x)` used as condition without comparison
| |
231 | if False: 231 | if False:
@ -452,6 +513,8 @@ len_as_condition.py:237:6: PLC1802 [*] `len(ascii(1))` used as condition without
237 |-bool(len(ascii(1))) 237 |-bool(len(ascii(1)))
237 |+bool(ascii(1)) 237 |+bool(ascii(1))
238 238 | bool(len(sorted(""))) 238 238 | bool(len(sorted("")))
239 239 |
240 240 | # regression tests for https://github.com/astral-sh/ruff/issues/18811
len_as_condition.py:238:6: PLC1802 [*] `len(sorted(""))` used as condition without comparison len_as_condition.py:238:6: PLC1802 [*] `len(sorted(""))` used as condition without comparison
| |
@ -459,6 +522,8 @@ len_as_condition.py:238:6: PLC1802 [*] `len(sorted(""))` used as condition witho
237 | bool(len(ascii(1))) 237 | bool(len(ascii(1)))
238 | bool(len(sorted(""))) 238 | bool(len(sorted("")))
| ^^^^^^^^^^^^^^^ PLC1802 | ^^^^^^^^^^^^^^^ PLC1802
239 |
240 | # regression tests for https://github.com/astral-sh/ruff/issues/18811
| |
= help: Remove `len` = help: Remove `len`
@ -468,3 +533,49 @@ len_as_condition.py:238:6: PLC1802 [*] `len(sorted(""))` used as condition witho
237 237 | bool(len(ascii(1))) 237 237 | bool(len(ascii(1)))
238 |-bool(len(sorted(""))) 238 |-bool(len(sorted("")))
238 |+bool(sorted("")) 238 |+bool(sorted(""))
239 239 |
240 240 | # regression tests for https://github.com/astral-sh/ruff/issues/18811
241 241 | fruits = []
len_as_condition.py:242:3: PLC1802 [*] `len(fruits)` used as condition without comparison
|
240 | # regression tests for https://github.com/astral-sh/ruff/issues/18811
241 | fruits = []
242 | if(len)(fruits):
| ^^^^^^^^^^^^^ PLC1802
243 | ...
|
= help: Remove `len`
Safe fix
239 239 |
240 240 | # regression tests for https://github.com/astral-sh/ruff/issues/18811
241 241 | fruits = []
242 |-if(len)(fruits):
242 |+if fruits:
243 243 | ...
244 244 |
245 245 | # regression tests for https://github.com/astral-sh/ruff/issues/18812
len_as_condition.py:247:4: PLC1802 [*] `len(fruits)` used as condition without comparison
|
245 | # regression tests for https://github.com/astral-sh/ruff/issues/18812
246 | fruits = []
247 | if len(
| ____^
248 | | fruits # comment
249 | | ):
| |_^ PLC1802
250 | ...
|
= help: Remove `len`
Unsafe fix
244 244 |
245 245 | # regression tests for https://github.com/astral-sh/ruff/issues/18812
246 246 | fruits = []
247 |-if len(
248 |- fruits # comment
249 |-):
247 |+if fruits:
250 248 | ...