From 45cddfcc1164169e52ded29815134488fb66542f Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 21 Nov 2023 09:59:07 -0600 Subject: [PATCH 1/2] Avoid `PERF101` if there's an append in loop body --- .../test/fixtures/perflint/PERF101.py | 9 +++++ .../src/checkers/ast/analyze/statement.rs | 2 +- .../perflint/rules/unnecessary_list_cast.rs | 36 ++++++++++++++++++- 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py index 6123e6d6526bfe..365e8938cae5e0 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py @@ -50,3 +50,12 @@ 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_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 0ad8dc16e57772..48343b3825c7c0 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 9769eed8d71e3e..93321cf6b070cf 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( From 655f1e6703edab63a82c2a207477236131a93f0e Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Tue, 21 Nov 2023 14:02:15 -0600 Subject: [PATCH 2/2] Add test case when variable name doesn't match --- .../test/fixtures/perflint/PERF101.py | 4 ++++ ...__perflint__tests__PERF101_PERF101.py.snap | 21 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py index 365e8938cae5e0..fbef6a7b2a709e 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF101.py @@ -54,6 +54,10 @@ 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) 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 645b5e51777849..1b4b456af3f15c 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 | +