From 98c14e3a0cb20a832406b6bcf7c6634188f94177 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Thu, 24 Oct 2024 19:07:47 -0400 Subject: [PATCH 01/16] implement quick fix for manual list comprehensions --- .../rules/manual_list_comprehension.rs | 72 +++++++++++++++++-- 1 file changed, 66 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index cc3ced50830db..8d8647415721f 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -1,10 +1,11 @@ -use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; +use ruff_python_ast::{self as ast, Arguments, Expr, ExprName, Stmt}; -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, 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::{analyze::typing::is_list, Binding}; +use ruff_text_size::TextRange; use crate::checkers::ast::Checker; @@ -49,6 +50,8 @@ pub struct ManualListComprehension { } impl Violation for ManualListComprehension { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Always; + #[derive_message_formats] fn message(&self) -> String { let ManualListComprehension { is_async } = self; @@ -187,11 +190,68 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S }) { return; } - - checker.diagnostics.push(Diagnostic::new( + let mut diagnostic = Diagnostic::new( ManualListComprehension { is_async: for_stmt.is_async, }, *range, - )); + ); + diagnostic.try_set_fix(|| { + Ok(convert_to_list_extend( + binding, + for_stmt, + if_test.map(std::convert::AsRef::as_ref), + arg, + checker, + )) + }); + checker.diagnostics.push(diagnostic); +} + +fn convert_to_list_extend( + binding: &Binding, + for_stmt: &ast::StmtFor, + if_test: Option<&ast::Expr>, + to_append: &Expr, + checker: &Checker, +) -> Fix { + let comprehension = ast::Comprehension { + target: (*for_stmt.target).clone(), + iter: (*for_stmt.iter).clone(), + is_async: for_stmt.is_async, + ifs: if_test.into_iter().cloned().collect(), + range: TextRange::default(), + }; + + let generator = ast::ExprGenerator { + elt: Box::new(to_append.clone()), + generators: vec![comprehension], + parenthesized: false, + range: TextRange::default(), + }; + + let extend = ast::ExprAttribute { + value: Box::new(Expr::Name(ExprName { + id: binding.name(checker.locator()).into(), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + })), + attr: ast::Identifier::new("extend", TextRange::default()), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; + + let list_extend = ast::ExprCall { + func: Box::new(ast::Expr::Attribute(extend)), + arguments: Arguments { + args: Box::new([ast::Expr::Generator(generator)]), + range: TextRange::default(), + keywords: Box::new([]), + }, + range: TextRange::default(), + }; + + let comprehension_body = checker.generator().expr(&Expr::Call(list_extend)); + + Fix::unsafe_edit(Edit::range_replacement(comprehension_body, for_stmt.range)) } From d89950f9fc8c9d9ca53db28c7c67cc5265b43266 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Fri, 25 Oct 2024 18:21:49 -0400 Subject: [PATCH 02/16] convert into list comprehension when the variable is an empty list literal --- .../rules/manual_list_comprehension.rs | 155 +++++++++++++----- 1 file changed, 118 insertions(+), 37 deletions(-) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index 8d8647415721f..023f1adfe98ab 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -1,12 +1,14 @@ -use ruff_python_ast::{self as ast, Arguments, Expr, ExprName, Stmt}; +use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, 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, Binding}; +use ruff_python_semantic::analyze::typing::is_list; use ruff_text_size::TextRange; +use anyhow::{anyhow, Result}; + use crate::checkers::ast::Checker; /// ## What it does @@ -47,19 +49,30 @@ use crate::checkers::ast::Checker; #[violation] pub struct ManualListComprehension { is_async: bool, + comprehension_type: Option, } impl Violation for ManualListComprehension { - const FIX_AVAILABILITY: FixAvailability = FixAvailability::Always; + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; #[derive_message_formats] fn message(&self) -> String { - let ManualListComprehension { is_async } = self; + let ManualListComprehension { is_async, .. } = self; match is_async { false => format!("Use a list comprehension to create a transformed list"), true => format!("Use an async list comprehension to create a transformed list"), } } + + fn fix_title(&self) -> Option { + self.comprehension_type + .map(|comprehension_type| match comprehension_type { + ComprehensionType::ListComprehension => { + format!("Replace for loop with list comprehension") + } + ComprehensionType::Extend => format!("Replace for loop with list.extend"), + }) + } } /// PERF401 @@ -190,31 +203,68 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S }) { return; } + + let Some(Stmt::Assign(binding_stmt)) = binding.statement(checker.semantic()) else { + return; + }; + + // If the variable is an empty list literal, then we might be able to replace it with a full list comprehension + // otherwise, it has to be replaced with a `list.extend` + let binding_is_empty_list = match binding_stmt.value.as_ref() { + Expr::List(ast::ExprList { elts, .. }) => elts.is_empty(), + _ => false, + }; + let comprehension_type = if binding_is_empty_list { + ComprehensionType::ListComprehension + } else { + ComprehensionType::Extend + }; + + // If the for loop does not have the same parent element as the binding, then it cannot be + // deleted and replaced with a list comprehension. + let assignment_in_same_statement = { + let for_loop_parent = checker.semantic().current_statement_parent_id(); + let Some(binding_source) = binding.source else { + return; + }; + let binding_parent = checker.semantic().parent_statement_id(binding_source); + for_loop_parent == binding_parent + }; + + let comprehension_type = Some(comprehension_type).filter(|_| assignment_in_same_statement); + let mut diagnostic = Diagnostic::new( ManualListComprehension { is_async: for_stmt.is_async, + comprehension_type, }, *range, ); - diagnostic.try_set_fix(|| { - Ok(convert_to_list_extend( - binding, + + diagnostic.try_set_optional_fix(|| match comprehension_type { + Some(comprehension_type) => convert_to_list_extend( + comprehension_type, + binding_stmt, for_stmt, if_test.map(std::convert::AsRef::as_ref), arg, checker, - )) + ) + .map(Some), + None => Ok(None), }); + checker.diagnostics.push(diagnostic); } fn convert_to_list_extend( - binding: &Binding, + fix_type: ComprehensionType, + binding_stmt: &ast::StmtAssign, for_stmt: &ast::StmtFor, if_test: Option<&ast::Expr>, to_append: &Expr, checker: &Checker, -) -> Fix { +) -> Result { let comprehension = ast::Comprehension { target: (*for_stmt.target).clone(), iter: (*for_stmt.iter).clone(), @@ -222,36 +272,67 @@ fn convert_to_list_extend( ifs: if_test.into_iter().cloned().collect(), range: TextRange::default(), }; + match fix_type { + ComprehensionType::Extend => { + let generator = ast::ExprGenerator { + elt: Box::new(to_append.clone()), + generators: vec![comprehension], + parenthesized: false, + range: TextRange::default(), + }; - let generator = ast::ExprGenerator { - elt: Box::new(to_append.clone()), - generators: vec![comprehension], - parenthesized: false, - range: TextRange::default(), - }; + let [variable_name] = &binding_stmt.targets[..] else { + return Err(anyhow!( + "Binding now has multiple targets when it previously had one" + )); + }; - let extend = ast::ExprAttribute { - value: Box::new(Expr::Name(ExprName { - id: binding.name(checker.locator()).into(), - ctx: ast::ExprContext::Load, - range: TextRange::default(), - })), - attr: ast::Identifier::new("extend", TextRange::default()), - ctx: ast::ExprContext::Load, - range: TextRange::default(), - }; + let extend = ast::ExprAttribute { + value: Box::new(variable_name.clone()), + attr: ast::Identifier::new("extend", TextRange::default()), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + }; - let list_extend = ast::ExprCall { - func: Box::new(ast::Expr::Attribute(extend)), - arguments: Arguments { - args: Box::new([ast::Expr::Generator(generator)]), - range: TextRange::default(), - keywords: Box::new([]), - }, - range: TextRange::default(), - }; + let list_extend = ast::ExprCall { + func: Box::new(ast::Expr::Attribute(extend)), + arguments: Arguments { + args: Box::new([ast::Expr::Generator(generator)]), + range: TextRange::default(), + keywords: Box::new([]), + }, + range: TextRange::default(), + }; + + let comprehension_body = checker.generator().expr(&Expr::Call(list_extend)); - let comprehension_body = checker.generator().expr(&Expr::Call(list_extend)); + Ok(Fix::unsafe_edit(Edit::range_replacement( + comprehension_body, + for_stmt.range, + ))) + } + ComprehensionType::ListComprehension => { + let Expr::List(assignment_value) = binding_stmt.value.as_ref() else { + return Err(anyhow!( + "Assignment value changed from list literal into another type" + )); + }; + let list_comp = ast::ExprListComp { + elt: Box::new(to_append.clone()), + generators: vec![comprehension], + range: TextRange::default(), + }; + let comprehension_body = checker.generator().expr(&Expr::ListComp(list_comp)); + Ok(Fix::unsafe_edits( + Edit::range_replacement(comprehension_body, assignment_value.range), + [Edit::range_deletion(for_stmt.range)], + )) + } + } +} - Fix::unsafe_edit(Edit::range_replacement(comprehension_body, for_stmt.range)) +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum ComprehensionType { + Extend, + ListComprehension, } From f3867eeeadeb35f3208e32cb00b8b1704a211323 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 28 Oct 2024 11:28:20 -0400 Subject: [PATCH 03/16] update tests and only offer fix when binding is a single assignment --- .../test/fixtures/perflint/PERF401.py | 13 ++++ .../rules/manual_list_comprehension.rs | 27 +++++--- ...__perflint__tests__PERF401_PERF401.py.snap | 63 +++++++++++++++++-- 3 files changed, 92 insertions(+), 11 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index f8b8014745017..bfa37b2fb9e6a 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -87,3 +87,16 @@ def f(): result = [] async for i in items: result.append(i) # PERF401 + + +def f(): + result, _ = [1,2,3,4], ... + for i in range(10): + result.append(i*2) # PERF401 + +def f(): + result = [] + if True: + for i in range(10): + if i % 2: + result.append(i) # PERF401 \ No newline at end of file diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index 023f1adfe98ab..32ead5053ef5d 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -142,7 +142,6 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S if attr.as_str() != "append" { return; } - // Ignore direct list copies (e.g., `for x in y: filtered.append(x)`), unless it's async, which // `manual-list-copy` doesn't cover. if !for_stmt.is_async { @@ -204,23 +203,27 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S return; } - let Some(Stmt::Assign(binding_stmt)) = binding.statement(checker.semantic()) else { + let Some(binding_stmt) = binding + .statement(checker.semantic()) + .and_then(|stmt| stmt.as_assign_stmt()) + else { return; }; // If the variable is an empty list literal, then we might be able to replace it with a full list comprehension // otherwise, it has to be replaced with a `list.extend` - let binding_is_empty_list = match binding_stmt.value.as_ref() { - Expr::List(ast::ExprList { elts, .. }) => elts.is_empty(), - _ => false, + let binding_is_empty_list = match binding_stmt.value.as_list_expr() { + Some(list_expr) => list_expr.elts.is_empty(), + None => false, }; + let comprehension_type = if binding_is_empty_list { ComprehensionType::ListComprehension } else { ComprehensionType::Extend }; - // If the for loop does not have the same parent element as the binding, then it cannot be + // If the for loop does not have the same parent element as the binding, then it cannot always be // deleted and replaced with a list comprehension. let assignment_in_same_statement = { let for_loop_parent = checker.semantic().current_statement_parent_id(); @@ -231,7 +234,17 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S for_loop_parent == binding_parent }; - let comprehension_type = Some(comprehension_type).filter(|_| assignment_in_same_statement); + // If the binding is not a single name expression, it could be replaced with a list comprehension, + // but not necessarily, so this needs to be manually fixed + let binding_has_one_target = { + let only_target = binding_stmt.targets.len() == 1; + let is_name = binding_stmt.targets[0].is_name_expr(); + only_target && is_name + }; + + let comprehension_type = Some(comprehension_type) + .filter(|_| assignment_in_same_statement) + .filter(|_| binding_has_one_target); let mut diagnostic = Diagnostic::new( ManualListComprehension { diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap index ae0dcb8e5a10f..d7f3dde8f821a 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap @@ -1,34 +1,89 @@ --- source: crates/ruff_linter/src/rules/perflint/mod.rs +snapshot_kind: text --- -PERF401.py:6:13: PERF401 Use a list comprehension to create a transformed list +PERF401.py:6:13: PERF401 [*] Use a list comprehension to create a transformed list | 4 | for i in items: 5 | if i % 2: 6 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 | + = help: Replace for loop with list comprehension -PERF401.py:13:9: PERF401 Use a list comprehension to create a transformed list +ℹ Unsafe fix +1 1 | def f(): +2 2 | items = [1, 2, 3, 4] +3 |- result = [] +4 |- for i in items: +5 |- if i % 2: +6 |- result.append(i) # PERF401 + 3 |+ result = [i for i in items if i % 2] + 4 |+ # PERF401 +7 5 | +8 6 | +9 7 | def f(): + +PERF401.py:13:9: PERF401 [*] Use a list comprehension to create a transformed list | 11 | result = [] 12 | for i in items: 13 | result.append(i * i) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +8 8 | +9 9 | def f(): +10 10 | items = [1, 2, 3, 4] +11 |- result = [] +12 |- for i in items: +13 |- result.append(i * i) # PERF401 + 11 |+ result = [i * i for i in items] + 12 |+ # PERF401 +14 13 | +15 14 | +16 15 | def f(): -PERF401.py:82:13: PERF401 Use an async list comprehension to create a transformed list +PERF401.py:82:13: PERF401 [*] Use an async list comprehension to create a transformed list | 80 | async for i in items: 81 | if i % 2: 82 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 | + = help: Replace for loop with list comprehension -PERF401.py:89:9: PERF401 Use an async list comprehension to create a transformed list +ℹ Unsafe fix +76 76 | +77 77 | def f(): +78 78 | items = [1, 2, 3, 4] +79 |- result = [] +80 |- async for i in items: +81 |- if i % 2: +82 |- result.append(i) # PERF401 + 79 |+ result = [i async for i in items if i % 2] + 80 |+ # PERF401 +83 81 | +84 82 | +85 83 | def f(): + +PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transformed list | 87 | result = [] 88 | async for i in items: 89 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +84 84 | +85 85 | def f(): +86 86 | items = [1, 2, 3, 4] +87 |- result = [] +88 |- async for i in items: +89 |- result.append(i) # PERF401 + 87 |+ result = [i async for i in items] + 88 |+ # PERF401 From 13592b4d3b7513438fcf6c4971902f96459ee932 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 28 Oct 2024 12:50:51 -0400 Subject: [PATCH 04/16] update test output --- ...__perflint__tests__PERF401_PERF401.py.snap | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap index d7f3dde8f821a..a272d4bdad30e 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap @@ -87,3 +87,24 @@ PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transfo 89 |- result.append(i) # PERF401 87 |+ result = [i async for i in items] 88 |+ # PERF401 +90 89 | +91 90 | +92 91 | def f(): + +PERF401.py:95:9: PERF401 Use a list comprehension to create a transformed list + | +93 | result, _ = [1,2,3,4], ... +94 | for i in range(10): +95 | result.append(i*2) # PERF401 + | ^^^^^^^^^^^^^^^^^^ PERF401 +96 | +97 | def f(): + | + +PERF401.py:102:17: PERF401 Use a list comprehension to create a transformed list + | +100 | for i in range(10): +101 | if i % 2: +102 | result.append(i) # PERF401 + | ^^^^^^^^^^^^^^^^ PERF401 + | From 27f8ba8e8ec283ec60011c4de172059b90ee821c Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 28 Oct 2024 14:02:29 -0400 Subject: [PATCH 05/16] fix false negative when binding statement did not exist --- .../rules/manual_list_comprehension.rs | 82 +++++++++++-------- ...__perflint__tests__PERF401_PERF401.py.snap | 15 +++- 2 files changed, 59 insertions(+), 38 deletions(-) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index 32ead5053ef5d..673013f6c5f17 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -4,7 +4,7 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, 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::{analyze::typing::is_list, Binding}; use ruff_text_size::TextRange; use anyhow::{anyhow, Result}; @@ -203,19 +203,17 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S return; } - let Some(binding_stmt) = binding + let binding_stmt = binding .statement(checker.semantic()) - .and_then(|stmt| stmt.as_assign_stmt()) - else { - return; - }; + .and_then(|stmt| stmt.as_assign_stmt()); // If the variable is an empty list literal, then we might be able to replace it with a full list comprehension // otherwise, it has to be replaced with a `list.extend` - let binding_is_empty_list = match binding_stmt.value.as_list_expr() { - Some(list_expr) => list_expr.elts.is_empty(), - None => false, - }; + let binding_is_empty_list = + binding_stmt.is_some_and(|binding_stmt| match binding_stmt.value.as_list_expr() { + Some(list_expr) => list_expr.elts.is_empty(), + None => false, + }); let comprehension_type = if binding_is_empty_list { ComprehensionType::ListComprehension @@ -224,27 +222,33 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S }; // If the for loop does not have the same parent element as the binding, then it cannot always be - // deleted and replaced with a list comprehension. + // deleted and replaced with a list comprehension. This does not apply when using an extend. let assignment_in_same_statement = { let for_loop_parent = checker.semantic().current_statement_parent_id(); - let Some(binding_source) = binding.source else { - return; - }; - let binding_parent = checker.semantic().parent_statement_id(binding_source); - for_loop_parent == binding_parent + + binding.source.is_some_and(|binding_source| { + let binding_parent = checker.semantic().parent_statement_id(binding_source); + for_loop_parent == binding_parent + }) }; // If the binding is not a single name expression, it could be replaced with a list comprehension, - // but not necessarily, so this needs to be manually fixed + // but not necessarily, so this needs to be manually fixed. This does not apply when using an extend. let binding_has_one_target = { - let only_target = binding_stmt.targets.len() == 1; - let is_name = binding_stmt.targets[0].is_name_expr(); + let only_target = binding_stmt.is_some_and(|binding_stmt| binding_stmt.targets.len() == 1); + let is_name = + binding_stmt.is_some_and(|binding_stmt| binding_stmt.targets[0].is_name_expr()); only_target && is_name }; - let comprehension_type = Some(comprehension_type) - .filter(|_| assignment_in_same_statement) - .filter(|_| binding_has_one_target); + // A list extend works in every context, while a list comprehension only works when all the criteria are true + let comprehension_type = + Some(comprehension_type).filter(|comprehension_type| match comprehension_type { + ComprehensionType::ListComprehension => { + binding_stmt.is_some() && assignment_in_same_statement && binding_has_one_target + } + ComprehensionType::Extend => true, + }); let mut diagnostic = Diagnostic::new( ManualListComprehension { @@ -257,7 +261,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S diagnostic.try_set_optional_fix(|| match comprehension_type { Some(comprehension_type) => convert_to_list_extend( comprehension_type, - binding_stmt, + binding, for_stmt, if_test.map(std::convert::AsRef::as_ref), arg, @@ -272,7 +276,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S fn convert_to_list_extend( fix_type: ComprehensionType, - binding_stmt: &ast::StmtAssign, + binding: &Binding, for_stmt: &ast::StmtFor, if_test: Option<&ast::Expr>, to_append: &Expr, @@ -294,14 +298,14 @@ fn convert_to_list_extend( range: TextRange::default(), }; - let [variable_name] = &binding_stmt.targets[..] else { - return Err(anyhow!( - "Binding now has multiple targets when it previously had one" - )); - }; + let variable_name = binding.name(checker.locator()); let extend = ast::ExprAttribute { - value: Box::new(variable_name.clone()), + value: Box::new(Expr::Name(ast::ExprName { + id: ast::name::Name::new(variable_name), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + })), attr: ast::Identifier::new("extend", TextRange::default()), ctx: ast::ExprContext::Load, range: TextRange::default(), @@ -325,19 +329,25 @@ fn convert_to_list_extend( ))) } ComprehensionType::ListComprehension => { - let Expr::List(assignment_value) = binding_stmt.value.as_ref() else { - return Err(anyhow!( - "Assignment value changed from list literal into another type" - )); - }; + let binding_stmt = binding + .statement(checker.semantic()) + .and_then(|stmt| stmt.as_assign_stmt()) + .ok_or(anyhow!( + "Binding must have a statement to convert into a list comprehension" + ))?; + let empty_list_to_replace = binding_stmt.value.as_list_expr().ok_or(anyhow!( + "Assignment value must be an empty list literal in order to replace with a list comprehension" + ))?; + let list_comp = ast::ExprListComp { elt: Box::new(to_append.clone()), generators: vec![comprehension], range: TextRange::default(), }; + let comprehension_body = checker.generator().expr(&Expr::ListComp(list_comp)); Ok(Fix::unsafe_edits( - Edit::range_replacement(comprehension_body, assignment_value.range), + Edit::range_replacement(comprehension_body, empty_list_to_replace.range), [Edit::range_deletion(for_stmt.range)], )) } diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap index a272d4bdad30e..dab791d3dcf9d 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/perflint/mod.rs -snapshot_kind: text --- PERF401.py:6:13: PERF401 [*] Use a list comprehension to create a transformed list | @@ -91,7 +90,7 @@ PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transfo 91 90 | 92 91 | def f(): -PERF401.py:95:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:95:9: PERF401 [*] Use a list comprehension to create a transformed list | 93 | result, _ = [1,2,3,4], ... 94 | for i in range(10): @@ -100,6 +99,18 @@ PERF401.py:95:9: PERF401 Use a list comprehension to create a transformed list 96 | 97 | def f(): | + = help: Replace for loop with list.extend + +ℹ Unsafe fix +91 91 | +92 92 | def f(): +93 93 | result, _ = [1,2,3,4], ... +94 |- for i in range(10): +95 |- result.append(i*2) # PERF401 + 94 |+ result.extend(i * 2 for i in range(10)) # PERF401 +96 95 | +97 96 | def f(): +98 97 | result = [] PERF401.py:102:17: PERF401 Use a list comprehension to create a transformed list | From 0aece9cc51e8a41681a188edc539397fad3161b7 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 28 Oct 2024 14:13:10 -0400 Subject: [PATCH 06/16] use slice instead of .name() --- .../src/rules/perflint/rules/manual_list_comprehension.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index 673013f6c5f17..7d2dc4d4e92f3 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -298,7 +298,7 @@ fn convert_to_list_extend( range: TextRange::default(), }; - let variable_name = binding.name(checker.locator()); + let variable_name = checker.locator().slice(binding.range); let extend = ast::ExprAttribute { value: Box::new(Expr::Name(ast::ExprName { From 1f7f4c0e966924c5cb4d44c03e9e686ded2a9623 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Sun, 3 Nov 2024 15:29:03 -0500 Subject: [PATCH 07/16] modify fix to preserve comments --- .../test/fixtures/perflint/PERF401.py | 19 ++- .../rules/manual_list_comprehension.rs | 119 ++++++++++++------ 2 files changed, 96 insertions(+), 42 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index bfa37b2fb9e6a..b5fcdd8a4e442 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -97,6 +97,19 @@ def f(): def f(): result = [] if True: - for i in range(10): - if i % 2: - result.append(i) # PERF401 \ No newline at end of file + for i in range(10): # single-line comment 1 should be protected + # single-line comment 2 should be protected + if i % 2: # single-line comment 3 should be protected + result.append(i) # PERF401 +def f(): + result = [] + for i in range(10): # single-line comment 1 should be protected + # single-line comment 2 should be protected + if i % 2: # single-line comment 3 should be protected + result.append(i) # PERF401 + +def f(): + result = [] + for i in range(10): + """block comment stops the fix""" + result.append(i*2) # Ok diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index 7d2dc4d4e92f3..d7e22a0fb08f9 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -1,10 +1,11 @@ use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; -use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; 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, Binding}; +use ruff_python_trivia::PythonWhitespace; use ruff_text_size::TextRange; use anyhow::{anyhow, Result}; @@ -49,29 +50,33 @@ use crate::checkers::ast::Checker; #[violation] pub struct ManualListComprehension { is_async: bool, - comprehension_type: Option, + comprehension_type: ComprehensionType, } -impl Violation for ManualListComprehension { - const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; - +impl AlwaysFixableViolation for ManualListComprehension { #[derive_message_formats] fn message(&self) -> String { - let ManualListComprehension { is_async, .. } = self; + let ManualListComprehension { + is_async, + comprehension_type, + } = self; + let comprehension_str = match comprehension_type { + ComprehensionType::Extend => "extend", + ComprehensionType::ListComprehension => "comprehension", + }; match is_async { - false => format!("Use a list comprehension to create a transformed list"), - true => format!("Use an async list comprehension to create a transformed list"), + false => format!("Use a list {comprehension_str} to create a transformed list"), + true => format!("Use an async list {comprehension_str} to create a transformed list"), } } - fn fix_title(&self) -> Option { - self.comprehension_type - .map(|comprehension_type| match comprehension_type { - ComprehensionType::ListComprehension => { - format!("Replace for loop with list comprehension") - } - ComprehensionType::Extend => format!("Replace for loop with list.extend"), - }) + fn fix_title(&self) -> String { + match self.comprehension_type { + ComprehensionType::ListComprehension => { + format!("Replace for loop with list comprehension") + } + ComprehensionType::Extend => format!("Replace for loop with list.extend"), + } } } @@ -215,12 +220,6 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S None => false, }); - let comprehension_type = if binding_is_empty_list { - ComprehensionType::ListComprehension - } else { - ComprehensionType::Extend - }; - // If the for loop does not have the same parent element as the binding, then it cannot always be // deleted and replaced with a list comprehension. This does not apply when using an extend. let assignment_in_same_statement = { @@ -235,20 +234,19 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S // If the binding is not a single name expression, it could be replaced with a list comprehension, // but not necessarily, so this needs to be manually fixed. This does not apply when using an extend. let binding_has_one_target = { - let only_target = binding_stmt.is_some_and(|binding_stmt| binding_stmt.targets.len() == 1); - let is_name = - binding_stmt.is_some_and(|binding_stmt| binding_stmt.targets[0].is_name_expr()); - only_target && is_name + match binding_stmt.map(|binding_stmt| binding_stmt.targets.as_slice()) { + Some([only_target]) => only_target.is_name_expr(), + _ => false, + } }; // A list extend works in every context, while a list comprehension only works when all the criteria are true let comprehension_type = - Some(comprehension_type).filter(|comprehension_type| match comprehension_type { - ComprehensionType::ListComprehension => { - binding_stmt.is_some() && assignment_in_same_statement && binding_has_one_target - } - ComprehensionType::Extend => true, - }); + if binding_is_empty_list && assignment_in_same_statement && binding_has_one_target { + ComprehensionType::ListComprehension + } else { + ComprehensionType::Extend + }; let mut diagnostic = Diagnostic::new( ManualListComprehension { @@ -258,8 +256,9 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S *range, ); - diagnostic.try_set_optional_fix(|| match comprehension_type { - Some(comprehension_type) => convert_to_list_extend( + // if checker.settings.preview.is_enabled() { + diagnostic.try_set_fix(|| { + convert_to_list_extend( comprehension_type, binding, for_stmt, @@ -267,10 +266,8 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S arg, checker, ) - .map(Some), - None => Ok(None), }); - + // } checker.diagnostics.push(diagnostic); } @@ -289,6 +286,23 @@ fn convert_to_list_extend( ifs: if_test.into_iter().cloned().collect(), range: TextRange::default(), }; + + let locator = checker.locator(); + let for_stmt_end = for_stmt.range.end(); + + let comment_strings_in_range = |range| { + checker + .comment_ranges() + .comments_in_range(range) + .iter() + .map(|range| locator.slice(range).trim_whitespace_start()) + .collect() + }; + let for_loop_inline_comments: Vec<&str> = comment_strings_in_range(for_stmt.range); + let for_loop_trailing_comment = + comment_strings_in_range(TextRange::new(for_stmt_end, locator.line_end(for_stmt_end))); + let newline = checker.stylist().line_ending().as_str(); + match fix_type { ComprehensionType::Extend => { let generator = ast::ExprGenerator { @@ -323,8 +337,17 @@ fn convert_to_list_extend( let comprehension_body = checker.generator().expr(&Expr::Call(list_extend)); + let indent_range = TextRange::new( + locator.line_start(for_stmt.range.start()), + for_stmt.range.start(), + ); + let indentation = format!("{newline}{}", locator.slice(indent_range)); + let text_to_replace = format!( + "{}{indentation}{comprehension_body}", + for_loop_inline_comments.join(&indentation) + ); Ok(Fix::unsafe_edit(Edit::range_replacement( - comprehension_body, + text_to_replace, for_stmt.range, ))) } @@ -346,9 +369,27 @@ fn convert_to_list_extend( }; let comprehension_body = checker.generator().expr(&Expr::ListComp(list_comp)); + + let indent_range = TextRange::new( + locator.line_start(binding_stmt.range.start()), + binding_stmt.range.start(), + ); + let indentation = format!("{newline}{}", locator.slice(indent_range)); + + let mut for_loop_comments = for_loop_inline_comments; + for_loop_comments.extend(for_loop_trailing_comment); + let leading_comments = format!("{}{indentation}", for_loop_comments.join(&indentation)); + Ok(Fix::unsafe_edits( Edit::range_replacement(comprehension_body, empty_list_to_replace.range), - [Edit::range_deletion(for_stmt.range)], + [ + Edit::range_deletion( + for_stmt + .range + .cover(TextRange::new(for_stmt_end, locator.line_end(for_stmt_end))), + ), + Edit::insertion(leading_comments, binding_stmt.range.start()), + ], )) } } From 499bdcad9a12bdc943459d32878583c7b9c41d98 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Sun, 3 Nov 2024 15:47:31 -0500 Subject: [PATCH 08/16] modify fix to use direct string modification instead of generation --- .../test/fixtures/perflint/PERF401.py | 2 +- .../rules/manual_list_comprehension.rs | 62 +++++-------------- ...__perflint__tests__PERF401_PERF401.py.snap | 10 +-- 3 files changed, 19 insertions(+), 55 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index b5fcdd8a4e442..15369ee1d7291 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -102,7 +102,7 @@ def f(): if i % 2: # single-line comment 3 should be protected result.append(i) # PERF401 def f(): - result = [] + result = [] # comment after assignment should be protected for i in range(10): # single-line comment 1 should be protected # single-line comment 2 should be protected if i % 2: # single-line comment 3 should be protected diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index d7e22a0fb08f9..badc54db5fb25 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -6,7 +6,7 @@ use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::helpers::any_over_expr; use ruff_python_semantic::{analyze::typing::is_list, Binding}; use ruff_python_trivia::PythonWhitespace; -use ruff_text_size::TextRange; +use ruff_text_size::{Ranged, TextRange}; use anyhow::{anyhow, Result}; @@ -279,17 +279,23 @@ fn convert_to_list_extend( to_append: &Expr, checker: &Checker, ) -> Result { - let comprehension = ast::Comprehension { - target: (*for_stmt.target).clone(), - iter: (*for_stmt.iter).clone(), - is_async: for_stmt.is_async, - ifs: if_test.into_iter().cloned().collect(), - range: TextRange::default(), - }; - let locator = checker.locator(); let for_stmt_end = for_stmt.range.end(); + let if_str = match if_test { + Some(test) => format!(" if {}", locator.slice(test.range())), + None => String::new(), + }; + let for_iter_str = locator.slice(for_stmt.iter.range()); + let for_type = if for_stmt.is_async { + "async for" + } else { + "for" + }; + let target_str = locator.slice(for_stmt.target.range()); + let elt_str = locator.slice(to_append.range()); + let generator_str = format!("{elt_str} {for_type} {target_str} in {for_iter_str}{if_str}"); + let comment_strings_in_range = |range| { checker .comment_ranges() @@ -305,37 +311,9 @@ fn convert_to_list_extend( match fix_type { ComprehensionType::Extend => { - let generator = ast::ExprGenerator { - elt: Box::new(to_append.clone()), - generators: vec![comprehension], - parenthesized: false, - range: TextRange::default(), - }; - let variable_name = checker.locator().slice(binding.range); - let extend = ast::ExprAttribute { - value: Box::new(Expr::Name(ast::ExprName { - id: ast::name::Name::new(variable_name), - ctx: ast::ExprContext::Load, - range: TextRange::default(), - })), - attr: ast::Identifier::new("extend", TextRange::default()), - ctx: ast::ExprContext::Load, - range: TextRange::default(), - }; - - let list_extend = ast::ExprCall { - func: Box::new(ast::Expr::Attribute(extend)), - arguments: Arguments { - args: Box::new([ast::Expr::Generator(generator)]), - range: TextRange::default(), - keywords: Box::new([]), - }, - range: TextRange::default(), - }; - - let comprehension_body = checker.generator().expr(&Expr::Call(list_extend)); + let comprehension_body = format!("{variable_name}.extend({generator_str})"); let indent_range = TextRange::new( locator.line_start(for_stmt.range.start()), @@ -362,13 +340,7 @@ fn convert_to_list_extend( "Assignment value must be an empty list literal in order to replace with a list comprehension" ))?; - let list_comp = ast::ExprListComp { - elt: Box::new(to_append.clone()), - generators: vec![comprehension], - range: TextRange::default(), - }; - - let comprehension_body = checker.generator().expr(&Expr::ListComp(list_comp)); + let comprehension_body = format!("[{generator_str}]"); let indent_range = TextRange::new( locator.line_start(binding_stmt.range.start()), diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap index dab791d3dcf9d..9d2196697e436 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap @@ -90,7 +90,7 @@ PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transfo 91 90 | 92 91 | def f(): -PERF401.py:95:9: PERF401 [*] Use a list comprehension to create a transformed list +PERF401.py:95:9: PERF401 [*] Use a list extend to create a transformed list | 93 | result, _ = [1,2,3,4], ... 94 | for i in range(10): @@ -111,11 +111,3 @@ PERF401.py:95:9: PERF401 [*] Use a list comprehension to create a transformed li 96 95 | 97 96 | def f(): 98 97 | result = [] - -PERF401.py:102:17: PERF401 Use a list comprehension to create a transformed list - | -100 | for i in range(10): -101 | if i % 2: -102 | result.append(i) # PERF401 - | ^^^^^^^^^^^^^^^^ PERF401 - | From 158b2e2f821f8b948423ed6fcc316e3ebceae3bb Mon Sep 17 00:00:00 2001 From: Sebastian Date: Sun, 3 Nov 2024 15:51:10 -0500 Subject: [PATCH 09/16] use LineRanges trait --- .../src/rules/perflint/rules/manual_list_comprehension.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index badc54db5fb25..c90cc70c21542 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -1,15 +1,15 @@ use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; +use anyhow::{anyhow, Result}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; 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, Binding}; use ruff_python_trivia::PythonWhitespace; +use ruff_source_file::LineRanges; use ruff_text_size::{Ranged, TextRange}; -use anyhow::{anyhow, Result}; - use crate::checkers::ast::Checker; /// ## What it does From 4d441c365d5d05d42d27c573f0de81e23af097ef Mon Sep 17 00:00:00 2001 From: Sebastian Date: Sun, 3 Nov 2024 15:58:18 -0500 Subject: [PATCH 10/16] update snapshot --- ...__perflint__tests__PERF401_PERF401.py.snap | 110 ++++++++++++++---- 1 file changed, 86 insertions(+), 24 deletions(-) diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap index 9d2196697e436..594f1c17d69a9 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap @@ -17,11 +17,12 @@ PERF401.py:6:13: PERF401 [*] Use a list comprehension to create a transformed li 4 |- for i in items: 5 |- if i % 2: 6 |- result.append(i) # PERF401 - 3 |+ result = [i for i in items if i % 2] - 4 |+ # PERF401 -7 5 | -8 6 | -9 7 | def f(): + 3 |+ # PERF401 + 4 |+ result = [i for i in items if i % 2] + 5 |+ +7 6 | +8 7 | +9 8 | def f(): PERF401.py:13:9: PERF401 [*] Use a list comprehension to create a transformed list | @@ -39,11 +40,12 @@ PERF401.py:13:9: PERF401 [*] Use a list comprehension to create a transformed li 11 |- result = [] 12 |- for i in items: 13 |- result.append(i * i) # PERF401 - 11 |+ result = [i * i for i in items] - 12 |+ # PERF401 -14 13 | -15 14 | -16 15 | def f(): + 11 |+ # PERF401 + 12 |+ result = [i * i for i in items] + 13 |+ +14 14 | +15 15 | +16 16 | def f(): PERF401.py:82:13: PERF401 [*] Use an async list comprehension to create a transformed list | @@ -62,11 +64,12 @@ PERF401.py:82:13: PERF401 [*] Use an async list comprehension to create a transf 80 |- async for i in items: 81 |- if i % 2: 82 |- result.append(i) # PERF401 - 79 |+ result = [i async for i in items if i % 2] - 80 |+ # PERF401 -83 81 | -84 82 | -85 83 | def f(): + 79 |+ # PERF401 + 80 |+ result = [i async for i in items if i % 2] + 81 |+ +83 82 | +84 83 | +85 84 | def f(): PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transformed list | @@ -84,11 +87,12 @@ PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transfo 87 |- result = [] 88 |- async for i in items: 89 |- result.append(i) # PERF401 - 87 |+ result = [i async for i in items] - 88 |+ # PERF401 -90 89 | -91 90 | -92 91 | def f(): + 87 |+ # PERF401 + 88 |+ result = [i async for i in items] + 89 |+ +90 90 | +91 91 | +92 92 | def f(): PERF401.py:95:9: PERF401 [*] Use a list extend to create a transformed list | @@ -107,7 +111,65 @@ PERF401.py:95:9: PERF401 [*] Use a list extend to create a transformed list 93 93 | result, _ = [1,2,3,4], ... 94 |- for i in range(10): 95 |- result.append(i*2) # PERF401 - 94 |+ result.extend(i * 2 for i in range(10)) # PERF401 -96 95 | -97 96 | def f(): -98 97 | result = [] + 94 |+ + 95 |+ result.extend(i*2 for i in range(10)) # PERF401 +96 96 | +97 97 | def f(): +98 98 | result = [] + +PERF401.py:103:17: PERF401 [*] Use a list extend to create a transformed list + | +101 | # single-line comment 2 should be protected +102 | if i % 2: # single-line comment 3 should be protected +103 | result.append(i) # PERF401 + | ^^^^^^^^^^^^^^^^ PERF401 +104 | def f(): +105 | result = [] # comment after assignment should be protected + | + = help: Replace for loop with list.extend + +ℹ Unsafe fix +97 97 | def f(): +98 98 | result = [] +99 99 | if True: +100 |- for i in range(10): # single-line comment 1 should be protected +101 |- # single-line comment 2 should be protected +102 |- if i % 2: # single-line comment 3 should be protected +103 |- result.append(i) # PERF401 + 100 |+ # single-line comment 1 should be protected + 101 |+ # single-line comment 2 should be protected + 102 |+ # single-line comment 3 should be protected + 103 |+ result.extend(i for i in range(10) if i % 2) # PERF401 +104 104 | def f(): +105 105 | result = [] # comment after assignment should be protected +106 106 | for i in range(10): # single-line comment 1 should be protected + +PERF401.py:109:13: PERF401 [*] Use a list comprehension to create a transformed list + | +107 | # single-line comment 2 should be protected +108 | if i % 2: # single-line comment 3 should be protected +109 | result.append(i) # PERF401 + | ^^^^^^^^^^^^^^^^ PERF401 +110 | +111 | def f(): + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +102 102 | if i % 2: # single-line comment 3 should be protected +103 103 | result.append(i) # PERF401 +104 104 | def f(): +105 |- result = [] # comment after assignment should be protected +106 |- for i in range(10): # single-line comment 1 should be protected +107 |- # single-line comment 2 should be protected +108 |- if i % 2: # single-line comment 3 should be protected +109 |- result.append(i) # PERF401 + 105 |+ # single-line comment 1 should be protected + 106 |+ # single-line comment 2 should be protected + 107 |+ # single-line comment 3 should be protected + 108 |+ # PERF401 + 109 |+ result = [i for i in range(10) if i % 2] # comment after assignment should be protected + 110 |+ +110 111 | +111 112 | def f(): +112 113 | result = [] From 5dbc764643dd3bf4ea5634b9d60cf694fcbc8372 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Sun, 3 Nov 2024 16:28:16 -0500 Subject: [PATCH 11/16] remove leftover indentation from for loop --- .../test/fixtures/perflint/PERF401.py | 4 + .../rules/manual_list_comprehension.rs | 19 ++- ...__perflint__tests__PERF401_PERF401.py.snap | 120 ++++++++---------- 3 files changed, 70 insertions(+), 73 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index 15369ee1d7291..6af7b5f01ae16 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -94,6 +94,7 @@ def f(): for i in range(10): result.append(i*2) # PERF401 + def f(): result = [] if True: @@ -101,6 +102,8 @@ def f(): # single-line comment 2 should be protected if i % 2: # single-line comment 3 should be protected result.append(i) # PERF401 + + def f(): result = [] # comment after assignment should be protected for i in range(10): # single-line comment 1 should be protected @@ -108,6 +111,7 @@ def f(): if i % 2: # single-line comment 3 should be protected result.append(i) # PERF401 + def f(): result = [] for i in range(10): diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index c90cc70c21542..769ff3418ce86 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -319,7 +319,11 @@ fn convert_to_list_extend( locator.line_start(for_stmt.range.start()), for_stmt.range.start(), ); - let indentation = format!("{newline}{}", locator.slice(indent_range)); + let indentation = if for_loop_inline_comments.is_empty() { + String::new() + } else { + format!("{newline}{}", locator.slice(indent_range)) + }; let text_to_replace = format!( "{}{indentation}{comprehension_body}", for_loop_inline_comments.join(&indentation) @@ -346,20 +350,21 @@ fn convert_to_list_extend( locator.line_start(binding_stmt.range.start()), binding_stmt.range.start(), ); - let indentation = format!("{newline}{}", locator.slice(indent_range)); let mut for_loop_comments = for_loop_inline_comments; for_loop_comments.extend(for_loop_trailing_comment); + + let indentation = if for_loop_comments.is_empty() { + String::new() + } else { + format!("{newline}{}", locator.slice(indent_range)) + }; let leading_comments = format!("{}{indentation}", for_loop_comments.join(&indentation)); Ok(Fix::unsafe_edits( Edit::range_replacement(comprehension_body, empty_list_to_replace.range), [ - Edit::range_deletion( - for_stmt - .range - .cover(TextRange::new(for_stmt_end, locator.line_end(for_stmt_end))), - ), + Edit::range_deletion(locator.full_lines_range(for_stmt.range)), Edit::insertion(leading_comments, binding_stmt.range.start()), ], )) diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap index 594f1c17d69a9..f96719ba5cd47 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap @@ -19,10 +19,9 @@ PERF401.py:6:13: PERF401 [*] Use a list comprehension to create a transformed li 6 |- result.append(i) # PERF401 3 |+ # PERF401 4 |+ result = [i for i in items if i % 2] - 5 |+ -7 6 | -8 7 | -9 8 | def f(): +7 5 | +8 6 | +9 7 | def f(): PERF401.py:13:9: PERF401 [*] Use a list comprehension to create a transformed list | @@ -42,10 +41,9 @@ PERF401.py:13:9: PERF401 [*] Use a list comprehension to create a transformed li 13 |- result.append(i * i) # PERF401 11 |+ # PERF401 12 |+ result = [i * i for i in items] - 13 |+ -14 14 | -15 15 | -16 16 | def f(): +14 13 | +15 14 | +16 15 | def f(): PERF401.py:82:13: PERF401 [*] Use an async list comprehension to create a transformed list | @@ -66,10 +64,9 @@ PERF401.py:82:13: PERF401 [*] Use an async list comprehension to create a transf 82 |- result.append(i) # PERF401 79 |+ # PERF401 80 |+ result = [i async for i in items if i % 2] - 81 |+ -83 82 | -84 83 | -85 84 | def f(): +83 81 | +84 82 | +85 83 | def f(): PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transformed list | @@ -89,10 +86,9 @@ PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transfo 89 |- result.append(i) # PERF401 87 |+ # PERF401 88 |+ result = [i async for i in items] - 89 |+ -90 90 | -91 91 | -92 92 | def f(): +90 89 | +91 90 | +92 91 | def f(): PERF401.py:95:9: PERF401 [*] Use a list extend to create a transformed list | @@ -100,8 +96,6 @@ PERF401.py:95:9: PERF401 [*] Use a list extend to create a transformed list 94 | for i in range(10): 95 | result.append(i*2) # PERF401 | ^^^^^^^^^^^^^^^^^^ PERF401 -96 | -97 | def f(): | = help: Replace for loop with list.extend @@ -111,65 +105,59 @@ PERF401.py:95:9: PERF401 [*] Use a list extend to create a transformed list 93 93 | result, _ = [1,2,3,4], ... 94 |- for i in range(10): 95 |- result.append(i*2) # PERF401 - 94 |+ - 95 |+ result.extend(i*2 for i in range(10)) # PERF401 -96 96 | -97 97 | def f(): -98 98 | result = [] + 94 |+ result.extend(i*2 for i in range(10)) # PERF401 +96 95 | +97 96 | +98 97 | def f(): -PERF401.py:103:17: PERF401 [*] Use a list extend to create a transformed list +PERF401.py:104:17: PERF401 [*] Use a list extend to create a transformed list | -101 | # single-line comment 2 should be protected -102 | if i % 2: # single-line comment 3 should be protected -103 | result.append(i) # PERF401 +102 | # single-line comment 2 should be protected +103 | if i % 2: # single-line comment 3 should be protected +104 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 -104 | def f(): -105 | result = [] # comment after assignment should be protected | = help: Replace for loop with list.extend ℹ Unsafe fix -97 97 | def f(): -98 98 | result = [] -99 99 | if True: -100 |- for i in range(10): # single-line comment 1 should be protected -101 |- # single-line comment 2 should be protected -102 |- if i % 2: # single-line comment 3 should be protected -103 |- result.append(i) # PERF401 - 100 |+ # single-line comment 1 should be protected - 101 |+ # single-line comment 2 should be protected - 102 |+ # single-line comment 3 should be protected - 103 |+ result.extend(i for i in range(10) if i % 2) # PERF401 -104 104 | def f(): -105 105 | result = [] # comment after assignment should be protected -106 106 | for i in range(10): # single-line comment 1 should be protected +98 98 | def f(): +99 99 | result = [] +100 100 | if True: +101 |- for i in range(10): # single-line comment 1 should be protected +102 |- # single-line comment 2 should be protected +103 |- if i % 2: # single-line comment 3 should be protected +104 |- result.append(i) # PERF401 + 101 |+ # single-line comment 1 should be protected + 102 |+ # single-line comment 2 should be protected + 103 |+ # single-line comment 3 should be protected + 104 |+ result.extend(i for i in range(10) if i % 2) # PERF401 +105 105 | +106 106 | +107 107 | def f(): -PERF401.py:109:13: PERF401 [*] Use a list comprehension to create a transformed list +PERF401.py:112:13: PERF401 [*] Use a list comprehension to create a transformed list | -107 | # single-line comment 2 should be protected -108 | if i % 2: # single-line comment 3 should be protected -109 | result.append(i) # PERF401 +110 | # single-line comment 2 should be protected +111 | if i % 2: # single-line comment 3 should be protected +112 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 -110 | -111 | def f(): | = help: Replace for loop with list comprehension ℹ Unsafe fix -102 102 | if i % 2: # single-line comment 3 should be protected -103 103 | result.append(i) # PERF401 -104 104 | def f(): -105 |- result = [] # comment after assignment should be protected -106 |- for i in range(10): # single-line comment 1 should be protected -107 |- # single-line comment 2 should be protected -108 |- if i % 2: # single-line comment 3 should be protected -109 |- result.append(i) # PERF401 - 105 |+ # single-line comment 1 should be protected - 106 |+ # single-line comment 2 should be protected - 107 |+ # single-line comment 3 should be protected - 108 |+ # PERF401 - 109 |+ result = [i for i in range(10) if i % 2] # comment after assignment should be protected - 110 |+ -110 111 | -111 112 | def f(): -112 113 | result = [] +105 105 | +106 106 | +107 107 | def f(): +108 |- result = [] # comment after assignment should be protected +109 |- for i in range(10): # single-line comment 1 should be protected +110 |- # single-line comment 2 should be protected +111 |- if i % 2: # single-line comment 3 should be protected +112 |- result.append(i) # PERF401 + 108 |+ # single-line comment 1 should be protected + 109 |+ # single-line comment 2 should be protected + 110 |+ # single-line comment 3 should be protected + 111 |+ # PERF401 + 112 |+ result = [i for i in range(10) if i % 2] # comment after assignment should be protected +113 113 | +114 114 | +115 115 | def f(): From a3b42e69c258bbc9d9f802e08354bb061044c52f Mon Sep 17 00:00:00 2001 From: Sebastian Date: Sun, 3 Nov 2024 16:36:43 -0500 Subject: [PATCH 12/16] uncomment preview gate --- .../rules/manual_list_comprehension.rs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index 769ff3418ce86..82db44614d3e3 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -256,18 +256,18 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S *range, ); - // if checker.settings.preview.is_enabled() { - diagnostic.try_set_fix(|| { - convert_to_list_extend( - comprehension_type, - binding, - for_stmt, - if_test.map(std::convert::AsRef::as_ref), - arg, - checker, - ) - }); - // } + if checker.settings.preview.is_enabled() { + diagnostic.try_set_fix(|| { + convert_to_list_extend( + comprehension_type, + binding, + for_stmt, + if_test.map(std::convert::AsRef::as_ref), + arg, + checker, + ) + }); + } checker.diagnostics.push(diagnostic); } From aa36f1b6c4ec6a2888123f6cb1cf8c2099460260 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 4 Nov 2024 10:46:54 -0500 Subject: [PATCH 13/16] add preview test case --- crates/ruff_linter/src/rules/perflint/mod.rs | 21 ++- ...t__tests__preview__PERF401_PERF401.py.snap | 163 ++++++++++++++++++ 2 files changed, 183 insertions(+), 1 deletion(-) create mode 100644 crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap diff --git a/crates/ruff_linter/src/rules/perflint/mod.rs b/crates/ruff_linter/src/rules/perflint/mod.rs index 2d073e639e242..554539b69c112 100644 --- a/crates/ruff_linter/src/rules/perflint/mod.rs +++ b/crates/ruff_linter/src/rules/perflint/mod.rs @@ -10,7 +10,7 @@ mod tests { use crate::assert_messages; use crate::registry::Rule; - use crate::settings::types::PythonVersion; + use crate::settings::types::{PreviewMode, PythonVersion}; use crate::settings::LinterSettings; use crate::test::test_path; @@ -29,4 +29,23 @@ mod tests { assert_messages!(snapshot, diagnostics); Ok(()) } + + #[test_case(Rule::ManualListComprehension, Path::new("PERF401.py"))] + fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!( + "preview__{}_{}", + rule_code.noqa_code(), + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("perflint").join(path).as_path(), + &LinterSettings { + preview: PreviewMode::Enabled, + target_version: PythonVersion::Py310, + ..LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } } diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap new file mode 100644 index 0000000000000..f96719ba5cd47 --- /dev/null +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap @@ -0,0 +1,163 @@ +--- +source: crates/ruff_linter/src/rules/perflint/mod.rs +--- +PERF401.py:6:13: PERF401 [*] Use a list comprehension to create a transformed list + | +4 | for i in items: +5 | if i % 2: +6 | result.append(i) # PERF401 + | ^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +1 1 | def f(): +2 2 | items = [1, 2, 3, 4] +3 |- result = [] +4 |- for i in items: +5 |- if i % 2: +6 |- result.append(i) # PERF401 + 3 |+ # PERF401 + 4 |+ result = [i for i in items if i % 2] +7 5 | +8 6 | +9 7 | def f(): + +PERF401.py:13:9: PERF401 [*] Use a list comprehension to create a transformed list + | +11 | result = [] +12 | for i in items: +13 | result.append(i * i) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +8 8 | +9 9 | def f(): +10 10 | items = [1, 2, 3, 4] +11 |- result = [] +12 |- for i in items: +13 |- result.append(i * i) # PERF401 + 11 |+ # PERF401 + 12 |+ result = [i * i for i in items] +14 13 | +15 14 | +16 15 | def f(): + +PERF401.py:82:13: PERF401 [*] Use an async list comprehension to create a transformed list + | +80 | async for i in items: +81 | if i % 2: +82 | result.append(i) # PERF401 + | ^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +76 76 | +77 77 | def f(): +78 78 | items = [1, 2, 3, 4] +79 |- result = [] +80 |- async for i in items: +81 |- if i % 2: +82 |- result.append(i) # PERF401 + 79 |+ # PERF401 + 80 |+ result = [i async for i in items if i % 2] +83 81 | +84 82 | +85 83 | def f(): + +PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transformed list + | +87 | result = [] +88 | async for i in items: +89 | result.append(i) # PERF401 + | ^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +84 84 | +85 85 | def f(): +86 86 | items = [1, 2, 3, 4] +87 |- result = [] +88 |- async for i in items: +89 |- result.append(i) # PERF401 + 87 |+ # PERF401 + 88 |+ result = [i async for i in items] +90 89 | +91 90 | +92 91 | def f(): + +PERF401.py:95:9: PERF401 [*] Use a list extend to create a transformed list + | +93 | result, _ = [1,2,3,4], ... +94 | for i in range(10): +95 | result.append(i*2) # PERF401 + | ^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list.extend + +ℹ Unsafe fix +91 91 | +92 92 | def f(): +93 93 | result, _ = [1,2,3,4], ... +94 |- for i in range(10): +95 |- result.append(i*2) # PERF401 + 94 |+ result.extend(i*2 for i in range(10)) # PERF401 +96 95 | +97 96 | +98 97 | def f(): + +PERF401.py:104:17: PERF401 [*] Use a list extend to create a transformed list + | +102 | # single-line comment 2 should be protected +103 | if i % 2: # single-line comment 3 should be protected +104 | result.append(i) # PERF401 + | ^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list.extend + +ℹ Unsafe fix +98 98 | def f(): +99 99 | result = [] +100 100 | if True: +101 |- for i in range(10): # single-line comment 1 should be protected +102 |- # single-line comment 2 should be protected +103 |- if i % 2: # single-line comment 3 should be protected +104 |- result.append(i) # PERF401 + 101 |+ # single-line comment 1 should be protected + 102 |+ # single-line comment 2 should be protected + 103 |+ # single-line comment 3 should be protected + 104 |+ result.extend(i for i in range(10) if i % 2) # PERF401 +105 105 | +106 106 | +107 107 | def f(): + +PERF401.py:112:13: PERF401 [*] Use a list comprehension to create a transformed list + | +110 | # single-line comment 2 should be protected +111 | if i % 2: # single-line comment 3 should be protected +112 | result.append(i) # PERF401 + | ^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +105 105 | +106 106 | +107 107 | def f(): +108 |- result = [] # comment after assignment should be protected +109 |- for i in range(10): # single-line comment 1 should be protected +110 |- # single-line comment 2 should be protected +111 |- if i % 2: # single-line comment 3 should be protected +112 |- result.append(i) # PERF401 + 108 |+ # single-line comment 1 should be protected + 109 |+ # single-line comment 2 should be protected + 110 |+ # single-line comment 3 should be protected + 111 |+ # PERF401 + 112 |+ result = [i for i in range(10) if i % 2] # comment after assignment should be protected +113 113 | +114 114 | +115 115 | def f(): From 0078d628da38406d6084ea6aa613a169b9235241 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Tue, 5 Nov 2024 14:34:47 -0500 Subject: [PATCH 14/16] change fix to FixAvailability::Sometimes --- crates/ruff_linter/src/rules/perflint/mod.rs | 1 + .../rules/manual_list_comprehension.rs | 33 +++-- ...__perflint__tests__PERF401_PERF401.py.snap | 119 ++---------------- 3 files changed, 30 insertions(+), 123 deletions(-) diff --git a/crates/ruff_linter/src/rules/perflint/mod.rs b/crates/ruff_linter/src/rules/perflint/mod.rs index 554539b69c112..55477f8d1f924 100644 --- a/crates/ruff_linter/src/rules/perflint/mod.rs +++ b/crates/ruff_linter/src/rules/perflint/mod.rs @@ -30,6 +30,7 @@ mod tests { Ok(()) } + // TODO: remove this test case when the fix for `perf401` is stabilized #[test_case(Rule::ManualListComprehension, Path::new("PERF401.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index 82db44614d3e3..e733570f3ef84 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -1,13 +1,13 @@ use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; use anyhow::{anyhow, Result}; -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, 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, Binding}; use ruff_python_trivia::PythonWhitespace; -use ruff_source_file::LineRanges; +// use ruff_source_file::LineRanges; use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; @@ -50,10 +50,12 @@ use crate::checkers::ast::Checker; #[violation] pub struct ManualListComprehension { is_async: bool, - comprehension_type: ComprehensionType, + comprehension_type: Option, } -impl AlwaysFixableViolation for ManualListComprehension { +impl Violation for ManualListComprehension { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { let ManualListComprehension { @@ -61,8 +63,9 @@ impl AlwaysFixableViolation for ManualListComprehension { comprehension_type, } = self; let comprehension_str = match comprehension_type { - ComprehensionType::Extend => "extend", - ComprehensionType::ListComprehension => "comprehension", + Some(ComprehensionType::Extend) => "extend", + Some(ComprehensionType::ListComprehension) => "comprehension", + None => "comprehension", }; match is_async { false => format!("Use a list {comprehension_str} to create a transformed list"), @@ -70,12 +73,12 @@ impl AlwaysFixableViolation for ManualListComprehension { } } - fn fix_title(&self) -> String { - match self.comprehension_type { + fn fix_title(&self) -> Option { + match self.comprehension_type? { ComprehensionType::ListComprehension => { - format!("Replace for loop with list comprehension") + Some(format!("Replace for loop with list comprehension")) } - ComprehensionType::Extend => format!("Replace for loop with list.extend"), + ComprehensionType::Extend => Some(format!("Replace for loop with list.extend")), } } } @@ -251,12 +254,20 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S let mut diagnostic = Diagnostic::new( ManualListComprehension { is_async: for_stmt.is_async, - comprehension_type, + comprehension_type: None, }, *range, ); + // TODO: once this fix is stabilized, change the rule to completely fixable if checker.settings.preview.is_enabled() { + diagnostic = Diagnostic::new( + ManualListComprehension { + is_async: for_stmt.is_async, + comprehension_type: Some(comprehension_type), + }, + *range, + ); diagnostic.try_set_fix(|| { convert_to_list_extend( comprehension_type, diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap index f96719ba5cd47..e96cfca5879d4 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF401_PERF401.py.snap @@ -1,163 +1,58 @@ --- source: crates/ruff_linter/src/rules/perflint/mod.rs --- -PERF401.py:6:13: PERF401 [*] Use a list comprehension to create a transformed list +PERF401.py:6:13: PERF401 Use a list comprehension to create a transformed list | 4 | for i in items: 5 | if i % 2: 6 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 | - = help: Replace for loop with list comprehension -ℹ Unsafe fix -1 1 | def f(): -2 2 | items = [1, 2, 3, 4] -3 |- result = [] -4 |- for i in items: -5 |- if i % 2: -6 |- result.append(i) # PERF401 - 3 |+ # PERF401 - 4 |+ result = [i for i in items if i % 2] -7 5 | -8 6 | -9 7 | def f(): - -PERF401.py:13:9: PERF401 [*] Use a list comprehension to create a transformed list +PERF401.py:13:9: PERF401 Use a list comprehension to create a transformed list | 11 | result = [] 12 | for i in items: 13 | result.append(i * i) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | - = help: Replace for loop with list comprehension - -ℹ Unsafe fix -8 8 | -9 9 | def f(): -10 10 | items = [1, 2, 3, 4] -11 |- result = [] -12 |- for i in items: -13 |- result.append(i * i) # PERF401 - 11 |+ # PERF401 - 12 |+ result = [i * i for i in items] -14 13 | -15 14 | -16 15 | def f(): -PERF401.py:82:13: PERF401 [*] Use an async list comprehension to create a transformed list +PERF401.py:82:13: PERF401 Use an async list comprehension to create a transformed list | 80 | async for i in items: 81 | if i % 2: 82 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 | - = help: Replace for loop with list comprehension -ℹ Unsafe fix -76 76 | -77 77 | def f(): -78 78 | items = [1, 2, 3, 4] -79 |- result = [] -80 |- async for i in items: -81 |- if i % 2: -82 |- result.append(i) # PERF401 - 79 |+ # PERF401 - 80 |+ result = [i async for i in items if i % 2] -83 81 | -84 82 | -85 83 | def f(): - -PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transformed list +PERF401.py:89:9: PERF401 Use an async list comprehension to create a transformed list | 87 | result = [] 88 | async for i in items: 89 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 | - = help: Replace for loop with list comprehension - -ℹ Unsafe fix -84 84 | -85 85 | def f(): -86 86 | items = [1, 2, 3, 4] -87 |- result = [] -88 |- async for i in items: -89 |- result.append(i) # PERF401 - 87 |+ # PERF401 - 88 |+ result = [i async for i in items] -90 89 | -91 90 | -92 91 | def f(): -PERF401.py:95:9: PERF401 [*] Use a list extend to create a transformed list +PERF401.py:95:9: PERF401 Use a list comprehension to create a transformed list | 93 | result, _ = [1,2,3,4], ... 94 | for i in range(10): 95 | result.append(i*2) # PERF401 | ^^^^^^^^^^^^^^^^^^ PERF401 | - = help: Replace for loop with list.extend -ℹ Unsafe fix -91 91 | -92 92 | def f(): -93 93 | result, _ = [1,2,3,4], ... -94 |- for i in range(10): -95 |- result.append(i*2) # PERF401 - 94 |+ result.extend(i*2 for i in range(10)) # PERF401 -96 95 | -97 96 | -98 97 | def f(): - -PERF401.py:104:17: PERF401 [*] Use a list extend to create a transformed list +PERF401.py:104:17: PERF401 Use a list comprehension to create a transformed list | 102 | # single-line comment 2 should be protected 103 | if i % 2: # single-line comment 3 should be protected 104 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 | - = help: Replace for loop with list.extend - -ℹ Unsafe fix -98 98 | def f(): -99 99 | result = [] -100 100 | if True: -101 |- for i in range(10): # single-line comment 1 should be protected -102 |- # single-line comment 2 should be protected -103 |- if i % 2: # single-line comment 3 should be protected -104 |- result.append(i) # PERF401 - 101 |+ # single-line comment 1 should be protected - 102 |+ # single-line comment 2 should be protected - 103 |+ # single-line comment 3 should be protected - 104 |+ result.extend(i for i in range(10) if i % 2) # PERF401 -105 105 | -106 106 | -107 107 | def f(): -PERF401.py:112:13: PERF401 [*] Use a list comprehension to create a transformed list +PERF401.py:112:13: PERF401 Use a list comprehension to create a transformed list | 110 | # single-line comment 2 should be protected 111 | if i % 2: # single-line comment 3 should be protected 112 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 | - = help: Replace for loop with list comprehension - -ℹ Unsafe fix -105 105 | -106 106 | -107 107 | def f(): -108 |- result = [] # comment after assignment should be protected -109 |- for i in range(10): # single-line comment 1 should be protected -110 |- # single-line comment 2 should be protected -111 |- if i % 2: # single-line comment 3 should be protected -112 |- result.append(i) # PERF401 - 108 |+ # single-line comment 1 should be protected - 109 |+ # single-line comment 2 should be protected - 110 |+ # single-line comment 3 should be protected - 111 |+ # PERF401 - 112 |+ result = [i for i in range(10) if i % 2] # comment after assignment should be protected -113 113 | -114 114 | -115 115 | def f(): From 9aa5627019a53c3f1afbca60397cd7bc1c8297bd Mon Sep 17 00:00:00 2001 From: Sebastian Date: Tue, 5 Nov 2024 14:38:20 -0500 Subject: [PATCH 15/16] uncomment LineRanges import --- .../src/rules/perflint/rules/manual_list_comprehension.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index e733570f3ef84..b7c16aff074ff 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -7,7 +7,7 @@ use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::helpers::any_over_expr; use ruff_python_semantic::{analyze::typing::is_list, Binding}; use ruff_python_trivia::PythonWhitespace; -// use ruff_source_file::LineRanges; +use ruff_source_file::LineRanges; use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; From 64f0f56818de7d0fed1239d8fa8f7ea802827f70 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Wed, 6 Nov 2024 20:50:00 -0500 Subject: [PATCH 16/16] fix nits and improve diagnostic message --- .../rules/manual_list_comprehension.rs | 34 +++++++++++-------- ...t__tests__preview__PERF401_PERF401.py.snap | 4 +-- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs index b7c16aff074ff..674288fb25322 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_list_comprehension.rs @@ -62,15 +62,23 @@ impl Violation for ManualListComprehension { is_async, comprehension_type, } = self; - let comprehension_str = match comprehension_type { - Some(ComprehensionType::Extend) => "extend", - Some(ComprehensionType::ListComprehension) => "comprehension", - None => "comprehension", + let message_str = match comprehension_type { + Some(ComprehensionType::Extend) => { + if *is_async { + "list.extend with an async comprehension" + } else { + "list.extend" + } + } + Some(ComprehensionType::ListComprehension) | None => { + if *is_async { + "an async list comprehension" + } else { + "a list comprehension" + } + } }; - match is_async { - false => format!("Use a list {comprehension_str} to create a transformed list"), - true => format!("Use an async list {comprehension_str} to create a transformed list"), - } + format!("Use {message_str} to create a transformed list") } fn fix_title(&self) -> Option { @@ -226,9 +234,8 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S // If the for loop does not have the same parent element as the binding, then it cannot always be // deleted and replaced with a list comprehension. This does not apply when using an extend. let assignment_in_same_statement = { - let for_loop_parent = checker.semantic().current_statement_parent_id(); - binding.source.is_some_and(|binding_source| { + let for_loop_parent = checker.semantic().current_statement_parent_id(); let binding_parent = checker.semantic().parent_statement_id(binding_source); for_loop_parent == binding_parent }) @@ -259,7 +266,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S *range, ); - // TODO: once this fix is stabilized, change the rule to completely fixable + // TODO: once this fix is stabilized, change the rule to always fixable if checker.settings.preview.is_enabled() { diagnostic = Diagnostic::new( ManualListComprehension { @@ -291,8 +298,6 @@ fn convert_to_list_extend( checker: &Checker, ) -> Result { let locator = checker.locator(); - let for_stmt_end = for_stmt.range.end(); - let if_str = match if_test { Some(test) => format!(" if {}", locator.slice(test.range())), None => String::new(), @@ -304,7 +309,7 @@ fn convert_to_list_extend( "for" }; let target_str = locator.slice(for_stmt.target.range()); - let elt_str = locator.slice(to_append.range()); + let elt_str = locator.slice(to_append); let generator_str = format!("{elt_str} {for_type} {target_str} in {for_iter_str}{if_str}"); let comment_strings_in_range = |range| { @@ -315,6 +320,7 @@ fn convert_to_list_extend( .map(|range| locator.slice(range).trim_whitespace_start()) .collect() }; + let for_stmt_end = for_stmt.range.end(); let for_loop_inline_comments: Vec<&str> = comment_strings_in_range(for_stmt.range); let for_loop_trailing_comment = comment_strings_in_range(TextRange::new(for_stmt_end, locator.line_end(for_stmt_end))); diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap index f96719ba5cd47..be01246c07b80 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF401_PERF401.py.snap @@ -90,7 +90,7 @@ PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transfo 91 90 | 92 91 | def f(): -PERF401.py:95:9: PERF401 [*] Use a list extend to create a transformed list +PERF401.py:95:9: PERF401 [*] Use list.extend to create a transformed list | 93 | result, _ = [1,2,3,4], ... 94 | for i in range(10): @@ -110,7 +110,7 @@ PERF401.py:95:9: PERF401 [*] Use a list extend to create a transformed list 97 96 | 98 97 | def f(): -PERF401.py:104:17: PERF401 [*] Use a list extend to create a transformed list +PERF401.py:104:17: PERF401 [*] Use list.extend to create a transformed list | 102 | # single-line comment 2 should be protected 103 | if i % 2: # single-line comment 3 should be protected