diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py index 6123e6d6526bf..fbef6a7b2a709 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py @@ -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) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 0ad8dc16e5777..48343b3825c7c 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -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) { diff --git a/crates/ruff_linter/src/rules/perflint/rules/unnecessary_list_cast.rs b/crates/ruff_linter/src/rules/perflint/rules/unnecessary_list_cast.rs index 9769eed8d71e3..93321cf6b070c 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/unnecessary_list_cast.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/unnecessary_list_cast.rs @@ -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: @@ -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); @@ -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( diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF101_PERF101.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF101_PERF101.py.snap index 645b5e5177784..1b4b456af3f15 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF101_PERF101.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF101_PERF101.py.snap @@ -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 | +