diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF401.py b/crates/ruff/resources/test/fixtures/perflint/PERF401.py index 304d1a6f80..86cd660d39 100644 --- a/crates/ruff/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff/resources/test/fixtures/perflint/PERF401.py @@ -60,3 +60,15 @@ def f(): for i in range(20): foo.fibonacci.append(sum(foo.fibonacci[-2:])) # OK print(foo.fibonacci) + + +class Foo: + def append(self, x): + pass + + +def f(): + items = [1, 2, 3, 4] + result = Foo() + for i in items: + result.append(i) # Ok diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF402.py b/crates/ruff/resources/test/fixtures/perflint/PERF402.py index 55f3e08cbc..b45a8c80c9 100644 --- a/crates/ruff/resources/test/fixtures/perflint/PERF402.py +++ b/crates/ruff/resources/test/fixtures/perflint/PERF402.py @@ -24,3 +24,22 @@ def f(): result = {} for i in items: result[i].append(i * i) # OK + + +class Foo: + def append(self, x): + pass + + +def f(): + items = [1, 2, 3, 4] + result = Foo() + for i in items: + result.append(i) # OK + + +def f(): + import sys + + for path in ("foo", "bar"): + sys.path.append(path) # OK diff --git a/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs index d3d4476b8f..bf04777f79 100644 --- a/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs @@ -4,6 +4,8 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::helpers::any_over_expr; +use ruff_python_semantic::analyze::typing::is_list; +use ruff_python_semantic::Binding; use crate::checkers::ast::Checker; @@ -112,13 +114,6 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo return; }; - // Ignore direct list copies (e.g., `for x in y: filtered.append(x)`). - if if_test.is_none() { - if arg.as_name_expr().is_some_and(|arg| arg.id == *id) { - return; - } - } - let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else { return; }; @@ -127,6 +122,13 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo return; } + // Ignore direct list copies (e.g., `for x in y: filtered.append(x)`). + if if_test.is_none() { + if arg.as_name_expr().is_some_and(|arg| arg.id == *id) { + return; + } + } + // Avoid, e.g., `for x in y: filtered[x].append(x * x)`. if any_over_expr(value, &|expr| { expr.as_name_expr().is_some_and(|expr| expr.id == *id) @@ -141,6 +143,25 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, bo return; } + // Avoid non-list values. + let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { + return; + }; + let bindings: Vec<&Binding> = checker + .semantic() + .current_scope() + .get_all(id) + .map(|binding_id| checker.semantic().binding(binding_id)) + .collect(); + + let [binding] = bindings.as_slice() else { + return; + }; + + if !is_list(binding, checker.semantic()) { + return; + } + // Avoid if the value is used in the conditional test, e.g., // // ```python diff --git a/crates/ruff/src/rules/perflint/rules/manual_list_copy.rs b/crates/ruff/src/rules/perflint/rules/manual_list_copy.rs index 7e12dccf76..39ad7dc059 100644 --- a/crates/ruff/src/rules/perflint/rules/manual_list_copy.rs +++ b/crates/ruff/src/rules/perflint/rules/manual_list_copy.rs @@ -2,6 +2,8 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::any_over_expr; use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; +use ruff_python_semantic::analyze::typing::is_list; +use ruff_python_semantic::Binding; use crate::checkers::ast::Checker; @@ -79,11 +81,6 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stm return; }; - // Only flag direct list copies (e.g., `for x in y: filtered.append(x)`). - if !arg.as_name_expr().is_some_and(|arg| arg.id == *id) { - return; - } - let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else { return; }; @@ -92,6 +89,11 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stm return; } + // Only flag direct list copies (e.g., `for x in y: filtered.append(x)`). + if !arg.as_name_expr().is_some_and(|arg| arg.id == *id) { + return; + } + // Avoid, e.g., `for x in y: filtered[x].append(x)`. if any_over_expr(value, &|expr| { expr.as_name_expr().is_some_and(|expr| expr.id == *id) @@ -99,6 +101,25 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, target: &Expr, body: &[Stm return; } + // Avoid non-list values. + let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else { + return; + }; + let bindings: Vec<&Binding> = checker + .semantic() + .current_scope() + .get_all(id) + .map(|binding_id| checker.semantic().binding(binding_id)) + .collect(); + + let [binding] = bindings.as_slice() else { + return; + }; + + if !is_list(binding, checker.semantic()) { + return; + } + checker .diagnostics .push(Diagnostic::new(ManualListCopy, *range));