From ed1dd09d02af7972df301dac0c6b5d084f26cc1b Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 3 Jul 2023 13:53:17 -0400 Subject: [PATCH] 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. --- .../test/fixtures/perflint/PERF401.py | 20 ++++- .../test/fixtures/perflint/PERF402.py | 11 ++- crates/ruff/src/checkers/ast/mod.rs | 4 +- .../rules/manual_list_comprehension.rs | 84 ++++++++++++++----- .../rules/perflint/rules/manual_list_copy.rs | 27 +++++- ...__perflint__tests__PERF401_PERF401.py.snap | 12 ++- ...__perflint__tests__PERF402_PERF402.py.snap | 8 -- 7 files changed, 122 insertions(+), 44 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/perflint/PERF401.py b/crates/ruff/resources/test/fixtures/perflint/PERF401.py index ac19d19876..beb3d4546c 100644 --- a/crates/ruff/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff/resources/test/fixtures/perflint/PERF401.py @@ -1,4 +1,4 @@ -def foo(): +def f(): items = [1, 2, 3, 4] result = [] for i in items: @@ -6,8 +6,15 @@ def foo(): result.append(i) # PERF401 -def foo(): - items = [1,2,3,4] +def f(): + items = [1, 2, 3, 4] + result = [] + for i in items: + result.append(i * i) # PERF401 + + +def f(): + items = [1, 2, 3, 4] result = [] for i in items: if i % 2: @@ -16,3 +23,10 @@ def foo(): result.append(i) # PERF401 else: result.append(i) # PERF401 + + +def f(): + items = [1, 2, 3, 4] + result = [] + 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 0d6842dce7..4db9a3dc52 100644 --- a/crates/ruff/resources/test/fixtures/perflint/PERF402.py +++ b/crates/ruff/resources/test/fixtures/perflint/PERF402.py @@ -1,12 +1,19 @@ -def foo(): +def f(): items = [1, 2, 3, 4] result = [] for i in items: result.append(i) # PERF402 -def foo(): +def f(): items = [1, 2, 3, 4] result = [] for i in items: result.insert(0, i) # PERF402 + + +def f(): + items = [1, 2, 3, 4] + result = [] + for i in items: + result.append(i * i) # OK diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 257dcdcf17..6eb1f6b25c 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1525,10 +1525,10 @@ where perflint::rules::incorrect_dict_iterator(self, target, iter); } 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) { - perflint::rules::manual_list_copy(self, body); + perflint::rules::manual_list_copy(self, target, body); } if self.enabled(Rule::UnnecessaryListCast) { perflint::rules::unnecessary_list_cast(self, iter); 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 d7143bb080..eb6e56735b 100644 --- a/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff/src/rules/perflint/rules/manual_list_comprehension.rs @@ -9,7 +9,7 @@ use crate::checkers::ast::Checker; /// Checks for `for` loops that can be replaced by a list comprehension. /// /// ## 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 /// more performant. /// @@ -34,43 +34,87 @@ use crate::checkers::ast::Checker; /// original = list(range(10000)) /// 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] pub struct ManualListComprehension; impl Violation for ManualListComprehension { #[derive_message_formats] 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 -pub(crate) fn manual_list_comprehension(checker: &mut Checker, body: &[Stmt]) { - let [stmt] = body else { +pub(crate) fn manual_list_comprehension(checker: &mut Checker, target: &Expr, body: &[Stmt]) { + 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; }; - let Stmt::If(ast::StmtIf { body, .. }) = stmt else { + let Expr::Call(ast::ExprCall { + func, + range, + args, + keywords, + }) = value.as_ref() + else { return; }; - for stmt in body { - let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else { - continue; - }; + if !keywords.is_empty() { + return; + } - let Expr::Call(ast::ExprCall { func, range, .. }) = value.as_ref() else { - continue; - }; + let [arg] = args.as_slice() else { + return; + }; - let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func.as_ref() else { - continue; - }; - - if attr.as_str() == "append" { - checker - .diagnostics - .push(Diagnostic::new(ManualListComprehension, *range)); + // Ignore direct list copies (e.g., `for x in y: filtered.append(x)`). + if !conditional { + 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 { + return; + }; + + if attr.as_str() == "append" { + checker + .diagnostics + .push(Diagnostic::new(ManualListComprehension, *range)); + } } 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 dd7545129d..13554488bd 100644 --- a/crates/ruff/src/rules/perflint/rules/manual_list_copy.rs +++ b/crates/ruff/src/rules/perflint/rules/manual_list_copy.rs @@ -44,7 +44,7 @@ impl Violation for ManualListCopy { } /// 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 { return; }; @@ -53,10 +53,33 @@ pub(crate) fn manual_list_copy(checker: &mut Checker, body: &[Stmt]) { 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; }; + 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 { return; }; diff --git a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF401_PERF401.py.snap b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF401_PERF401.py.snap index e59eae4adc..cf2e2677c5 100644 --- a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF401_PERF401.py.snap +++ b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF401_PERF401.py.snap @@ -1,7 +1,7 @@ --- 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: 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.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: -13 | if i % 2: -14 | result.append(i) # PERF401 - | ^^^^^^^^^^^^^^^^ PERF401 -15 | elif i % 2: -16 | result.append(i) # PERF401 +13 | result.append(i * i) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 | diff --git a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF402_PERF402.py.snap b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF402_PERF402.py.snap index 55cd69db8b..e56584c95e 100644 --- a/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF402_PERF402.py.snap +++ b/crates/ruff/src/rules/perflint/snapshots/ruff__rules__perflint__tests__PERF402_PERF402.py.snap @@ -9,12 +9,4 @@ PERF402.py:5:9: PERF402 Use `list` or `list.copy` to create a copy of a list | ^^^^^^^^^^^^^^^^ 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 - | -