Refine some `perflint` rules (#5484)

## Summary

Removing some false positives based on running over `zulip`.

`PERF401` now also detects cases like:

```py
original = list(range(10000))
filtered = []
for i in original:
    filtered.append(i * i)
```

Previously, these were caught by the list-copy rule, but these too need
comprehensions.
This commit is contained in:
Charlie Marsh 2023-07-03 13:53:17 -04:00 committed by GitHub
parent ca497fabbd
commit ed1dd09d02
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 122 additions and 44 deletions

View File

@ -1,4 +1,4 @@
def foo(): def f():
items = [1, 2, 3, 4] items = [1, 2, 3, 4]
result = [] result = []
for i in items: for i in items:
@ -6,8 +6,15 @@ def foo():
result.append(i) # PERF401 result.append(i) # PERF401
def foo(): def f():
items = [1,2,3,4] items = [1, 2, 3, 4]
result = []
for i in items:
result.append(i * i) # PERF401
def f():
items = [1, 2, 3, 4]
result = [] result = []
for i in items: for i in items:
if i % 2: if i % 2:
@ -16,3 +23,10 @@ def foo():
result.append(i) # PERF401 result.append(i) # PERF401
else: else:
result.append(i) # PERF401 result.append(i) # PERF401
def f():
items = [1, 2, 3, 4]
result = []
for i in items:
result.append(i) # OK

View File

@ -1,12 +1,19 @@
def foo(): def f():
items = [1, 2, 3, 4] items = [1, 2, 3, 4]
result = [] result = []
for i in items: for i in items:
result.append(i) # PERF402 result.append(i) # PERF402
def foo(): def f():
items = [1, 2, 3, 4] items = [1, 2, 3, 4]
result = [] result = []
for i in items: for i in items:
result.insert(0, i) # PERF402 result.insert(0, i) # PERF402
def f():
items = [1, 2, 3, 4]
result = []
for i in items:
result.append(i * i) # OK

View File

@ -1525,10 +1525,10 @@ where
perflint::rules::incorrect_dict_iterator(self, target, iter); perflint::rules::incorrect_dict_iterator(self, target, iter);
} }
if self.enabled(Rule::ManualListComprehension) { if self.enabled(Rule::ManualListComprehension) {
perflint::rules::manual_list_comprehension(self, body); perflint::rules::manual_list_comprehension(self, target, body);
} }
if self.enabled(Rule::ManualListCopy) { if self.enabled(Rule::ManualListCopy) {
perflint::rules::manual_list_copy(self, body); perflint::rules::manual_list_copy(self, target, body);
} }
if self.enabled(Rule::UnnecessaryListCast) { if self.enabled(Rule::UnnecessaryListCast) {
perflint::rules::unnecessary_list_cast(self, iter); perflint::rules::unnecessary_list_cast(self, iter);

View File

@ -9,7 +9,7 @@ use crate::checkers::ast::Checker;
/// Checks for `for` loops that can be replaced by a list comprehension. /// Checks for `for` loops that can be replaced by a list comprehension.
/// ///
/// ## Why is this bad? /// ## Why is this bad?
/// When creating a filtered list from an existing list using a for-loop, /// When creating a transformed list from an existing list using a for-loop,
/// prefer a list comprehension. List comprehensions are more readable and /// prefer a list comprehension. List comprehensions are more readable and
/// more performant. /// more performant.
/// ///
@ -34,43 +34,87 @@ use crate::checkers::ast::Checker;
/// original = list(range(10000)) /// original = list(range(10000))
/// filtered = [x for x in original if x % 2] /// filtered = [x for x in original if x % 2]
/// ``` /// ```
///
/// If you're appending to an existing list, use the `extend` method instead:
/// ```python
/// original = list(range(10000))
/// filtered.extend(x for x in original if x % 2)
/// ```
#[violation] #[violation]
pub struct ManualListComprehension; pub struct ManualListComprehension;
impl Violation for ManualListComprehension { impl Violation for ManualListComprehension {
#[derive_message_formats] #[derive_message_formats]
fn message(&self) -> String { fn message(&self) -> String {
format!("Use a list comprehension to create a new filtered list") format!("Use a list comprehension to create a transformed list")
} }
} }
/// PERF401 /// PERF401
pub(crate) fn manual_list_comprehension(checker: &mut Checker, body: &[Stmt]) { pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, body: &[Stmt]) {
let [stmt] = body else { let (stmt, conditional) = match body {
// ```python
// for x in y:
// if z:
// filtered.append(x)
// ```
[Stmt::If(ast::StmtIf { body, orelse, .. })] => {
if !orelse.is_empty() {
return;
}
let [stmt] = body.as_slice() else {
return;
};
(stmt, true)
}
// ```python
// for x in y:
// filtered.append(f(x))
// ```
[stmt] => (stmt, false),
_ => return,
};
let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else {
return; return;
}; };
let Stmt::If(ast::StmtIf { body, .. }) = stmt else { let Expr::Call(ast::ExprCall {
func,
range,
args,
keywords,
}) = value.as_ref()
else {
return; return;
}; };
for stmt in body { if !keywords.is_empty() {
let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else { return;
continue; }
};
let Expr::Call(ast::ExprCall { func, range, .. }) = value.as_ref() else { let [arg] = args.as_slice() else {
continue; return;
}; };
let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func.as_ref() else { // Ignore direct list copies (e.g., `for x in y: filtered.append(x)`).
continue; if !conditional {
}; if arg.as_name_expr().map_or(false, |arg| {
target
if attr.as_str() == "append" { .as_name_expr()
checker .map_or(false, |target| arg.id == target.id)
.diagnostics }) {
.push(Diagnostic::new(ManualListComprehension, *range)); return;
} }
} }
let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func.as_ref() else {
return;
};
if attr.as_str() == "append" {
checker
.diagnostics
.push(Diagnostic::new(ManualListComprehension, *range));
}
} }

View File

@ -44,7 +44,7 @@ impl Violation for ManualListCopy {
} }
/// PERF402 /// PERF402
pub(crate) fn manual_list_copy(checker: &mut Checker, body: &[Stmt]) { pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stmt]) {
let [stmt] = body else { let [stmt] = body else {
return; return;
}; };
@ -53,10 +53,33 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, body: &[Stmt]) {
return; return;
}; };
let Expr::Call(ast::ExprCall { func, range, .. }) = value.as_ref() else { let Expr::Call(ast::ExprCall {
func,
range,
args,
keywords,
}) = value.as_ref()
else {
return; return;
}; };
if !keywords.is_empty() {
return;
}
let [arg] = args.as_slice() else {
return;
};
// Only flag direct list copies (e.g., `for x in y: filtered.append(x)`).
if !arg.as_name_expr().map_or(false, |arg| {
target
.as_name_expr()
.map_or(false, |target| arg.id == target.id)
}) {
return;
}
let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func.as_ref() else { let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func.as_ref() else {
return; return;
}; };

View File

@ -1,7 +1,7 @@
--- ---
source: crates/ruff/src/rules/perflint/mod.rs source: crates/ruff/src/rules/perflint/mod.rs
--- ---
PERF401.py:6:13: PERF401 Use a list comprehension to create a new filtered list PERF401.py:6:13: PERF401 Use a list comprehension to create a transformed list
| |
4 | for i in items: 4 | for i in items:
5 | if i % 2: 5 | if i % 2:
@ -9,14 +9,12 @@ PERF401.py:6:13: PERF401 Use a list comprehension to create a new filtered list
| ^^^^^^^^^^^^^^^^ PERF401 | ^^^^^^^^^^^^^^^^ PERF401
| |
PERF401.py:14:13: PERF401 Use a list comprehension to create a new filtered list PERF401.py:13:9: PERF401 Use a list comprehension to create a transformed list
| |
11 | result = []
12 | for i in items: 12 | for i in items:
13 | if i % 2: 13 | result.append(i * i) # PERF401
14 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401
| ^^^^^^^^^^^^^^^^ PERF401
15 | elif i % 2:
16 | result.append(i) # PERF401
| |

View File

@ -9,12 +9,4 @@ PERF402.py:5:9: PERF402 Use `list` or `list.copy` to create a copy of a list
| ^^^^^^^^^^^^^^^^ PERF402 | ^^^^^^^^^^^^^^^^ PERF402
| |
PERF402.py:12:9: PERF402 Use `list` or `list.copy` to create a copy of a list
|
10 | result = []
11 | for i in items:
12 | result.insert(0, i) # PERF402
| ^^^^^^^^^^^^^^^^^^^ PERF402
|