Skip to content

Commit

Permalink
Avoid PERF101 if there's an append in loop body (#8809)
Browse files Browse the repository at this point in the history
## Summary

Avoid `PERF101` if there's an append in loop body

## Test Plan

Add new test cases for this pattern.

fixes: #8746
  • Loading branch information
dhruvmanila authored Nov 21, 2023
1 parent 5373759 commit 5ce6299
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 2 deletions.
13 changes: 13 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,16 @@

for i in itertools.product(foo_int): # Ok
pass

for i in list(foo_list): # Ok
foo_list.append(i + 1)

for i in list(foo_list): # PERF101
# Make sure we match the correct list
other_list.append(i + 1)

for i in list(foo_tuple): # Ok
foo_tuple.append(i + 1)

for i in list(foo_set): # Ok
foo_set.append(i + 1)
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1269,7 +1269,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
perflint::rules::manual_dict_comprehension(checker, target, body);
}
if checker.enabled(Rule::UnnecessaryListCast) {
perflint::rules::unnecessary_list_cast(checker, iter);
perflint::rules::unnecessary_list_cast(checker, iter, body);
}
if !is_async {
if checker.enabled(Rule::ReimplementedBuiltin) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl AlwaysFixableViolation for UnnecessaryListCast {
}

/// PERF101
pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) {
pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr, body: &[Stmt]) {
let Expr::Call(ast::ExprCall {
func,
arguments:
Expand Down Expand Up @@ -98,6 +98,18 @@ pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) {
range: iterable_range,
..
}) => {
// If the variable is being appended to, don't suggest removing the cast:
//
// ```python
// items = ["foo", "bar"]
// for item in list(items):
// items.append("baz")
// ```
//
// Here, removing the `list()` cast would change the behavior of the code.
if body.iter().any(|stmt| match_append(stmt, id)) {
return;
}
let scope = checker.semantic().current_scope();
if let Some(binding_id) = scope.get(id) {
let binding = checker.semantic().binding(binding_id);
Expand Down Expand Up @@ -128,6 +140,28 @@ pub(crate) fn unnecessary_list_cast(checker: &mut Checker, iter: &Expr) {
}
}

/// Check if a statement is an `append` call to a given identifier.
///
/// For example, `foo.append(bar)` would return `true` if `id` is `foo`.
fn match_append(stmt: &Stmt, id: &str) -> bool {
let Some(ast::StmtExpr { value, .. }) = stmt.as_expr_stmt() else {
return false;
};
let Some(ast::ExprCall { func, .. }) = value.as_call_expr() else {
return false;
};
let Some(ast::ExprAttribute { value, attr, .. }) = func.as_attribute_expr() else {
return false;
};
if attr != "append" {
return false;
}
let Some(ast::ExprName { id: target_id, .. }) = value.as_name_expr() else {
return false;
};
target_id == id
}

/// Generate a [`Fix`] to remove a `list` cast from an expression.
fn remove_cast(list_range: TextRange, iterable_range: TextRange) -> Fix {
Fix::safe_edits(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,4 +180,25 @@ PERF101.py:34:10: PERF101 [*] Do not cast an iterable to `list` before iterating
38 36 |
39 37 | for i in list(foo_dict): # Ok

PERF101.py:57:10: PERF101 [*] Do not cast an iterable to `list` before iterating over it
|
55 | foo_list.append(i + 1)
56 |
57 | for i in list(foo_list): # PERF101
| ^^^^^^^^^^^^^^ PERF101
58 | # Make sure we match the correct list
59 | other_list.append(i + 1)
|
= help: Remove `list()` cast

Safe fix
54 54 | for i in list(foo_list): # Ok
55 55 | foo_list.append(i + 1)
56 56 |
57 |-for i in list(foo_list): # PERF101
57 |+for i in foo_list: # PERF101
58 58 | # Make sure we match the correct list
59 59 | other_list.append(i + 1)
60 60 |


0 comments on commit 5ce6299

Please sign in to comment.