From aafb6891b7e027f293e583b36374bb29310d3ccf Mon Sep 17 00:00:00 2001 From: Sebastian Date: Fri, 15 Nov 2024 11:28:42 -0500 Subject: [PATCH 01/21] fix invalid hoist --- .../test/fixtures/perflint/PERF401.py | 24 +++ .../rules/manual_list_comprehension.rs | 56 +++---- ...__perflint__tests__PERF401_PERF401.py.snap | 42 ++++++ ...t__tests__preview__PERF401_PERF401.py.snap | 141 ++++++++++++++---- 4 files changed, 201 insertions(+), 62 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index bead4d5ce0afa..bccb34115850c 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -125,3 +125,27 @@ def f(param): new_layers = [] for value in param: new_layers.append(value * 3) + +def f(): + result = [] + var = 1 + for _ in range(10): + result.append(var + 1) # PERF401 + +def f(): + # make sure that `tmp` is not deleted + tmp = 1; result = [] # commment should be protected + for i in range(10): + result.append(i + 1) # PERF401 + +def f(): + # make sure that `tmp` is not deleted + result = []; tmp = 1 # commment should be protected + for i in range(10): + result.append(i + 1) # PERF401 + + +def f(): + result = [] # comment should be protected + for i in range(10): + result.append(i*2) # PERF401 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 e9c3a35da0da0..eb6993ebfd296 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 @@ -315,22 +315,20 @@ fn convert_to_list_extend( .map(|range| locator.slice(range).trim_whitespace_start()) .collect() }; - let for_stmt_end = for_stmt.range.end(); + + let variable_name = locator.slice(binding); 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(); + let indent_range = TextRange::new( + locator.line_start(for_stmt.range.start()), + for_stmt.range.start(), + ); match fix_type { ComprehensionType::Extend => { - let variable_name = checker.locator().slice(binding.range); - let comprehension_body = format!("{variable_name}.extend({generator_str})"); - let indent_range = TextRange::new( - locator.line_start(for_stmt.range.start()), - for_stmt.range.start(), - ); let indentation = if for_loop_inline_comments.is_empty() { String::new() } else { @@ -352,40 +350,26 @@ fn convert_to_list_extend( .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 comprehension_body = format!("[{generator_str}]"); - let indent_range = TextRange::new( - locator.line_start(binding_stmt.range.start()), - binding_stmt.range.start(), - ); - - let mut for_loop_comments = for_loop_inline_comments; - for_loop_comments.extend(for_loop_trailing_comment); + let mut comments_to_move = + comment_strings_in_range(locator.full_lines_range(binding_stmt.range)); + comments_to_move.extend(for_loop_inline_comments); - let indentation = if for_loop_comments.is_empty() { + let indentation = if comments_to_move.is_empty() { String::new() } else { format!("{newline}{}", locator.slice(indent_range)) }; - let leading_comments = format!("{}{indentation}", for_loop_comments.join(&indentation)); - - let mut additional_fixes = vec![Edit::range_deletion( - locator.full_lines_range(for_stmt.range), - )]; - // if comments are empty, trying to insert them panics - if !leading_comments.is_empty() { - additional_fixes.push(Edit::insertion( - leading_comments, - binding_stmt.range.start(), - )); - } + let leading_comments = format!("{}{indentation}", comments_to_move.join(&indentation)); + + let comprehension_body = + format!("{leading_comments}{variable_name} = [{generator_str}]"); + // TODO: protect comment from after assignment statement + let binding_stmt_range = locator.full_lines_range(binding_stmt.range); + Ok(Fix::unsafe_edits( - Edit::range_replacement(comprehension_body, empty_list_to_replace.range), - additional_fixes, + Edit::range_deletion(binding_stmt_range), + [Edit::range_replacement(comprehension_body, 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 31cf524b6d3a7..13abff49088d2 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 @@ -70,5 +70,47 @@ PERF401.py:127:13: PERF401 Use a list comprehension to create a transformed list 126 | for value in param: 127 | new_layers.append(value * 3) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF401 +128 | +129 | def f(): + | + = help: Replace for loop with list comprehension + +PERF401.py:133:9: PERF401 Use a list comprehension to create a transformed list + | +131 | var = 1 +132 | for _ in range(10): +133 | result.append(var + 1) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 +134 | +135 | def f(): + | + = help: Replace for loop with list comprehension + +PERF401.py:139:9: PERF401 Use a list comprehension to create a transformed list + | +137 | tmp = 1; result = [] # commment should be protected +138 | for i in range(10): +139 | result.append(i + 1) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 +140 | +141 | def f(): + | + = help: Replace for loop with list comprehension + +PERF401.py:145:9: PERF401 Use a list comprehension to create a transformed list + | +143 | result = []; tmp = 1 # commment should be protected +144 | for i in range(10): +145 | result.append(i + 1) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +PERF401.py:151:9: PERF401 Use a list comprehension to create a transformed list + | +149 | result = [] # comment should be protected +150 | for i in range(10): +151 | result.append(i*2) # PERF401 + | ^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension 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 a981285c8b4a5..fdc7d2092f2f5 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 @@ -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 | @@ -18,11 +17,10 @@ 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 |+ # PERF401 - 4 |+ result = [i for i in items if i % 2] -7 5 | -8 6 | -9 7 | def f(): + 3 |+ result = [i for i in items if i % 2] # PERF401 +7 4 | +8 5 | +9 6 | def f(): PERF401.py:13:9: PERF401 [*] Use a list comprehension to create a transformed list | @@ -40,11 +38,10 @@ 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 |+ # PERF401 - 12 |+ result = [i * i for i in items] -14 13 | -15 14 | -16 15 | def f(): + 11 |+ result = [i * i for i in items] # PERF401 +14 12 | +15 13 | +16 14 | def f(): PERF401.py:82:13: PERF401 [*] Use an async list comprehension to create a transformed list | @@ -63,11 +60,10 @@ 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 |+ # PERF401 - 80 |+ result = [i async for i in items if i % 2] -83 81 | -84 82 | -85 83 | def f(): + 79 |+ result = [i async for i in items if i % 2] # PERF401 +83 80 | +84 81 | +85 82 | def f(): PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transformed list | @@ -85,11 +81,10 @@ 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 |+ # PERF401 - 88 |+ result = [i async for i in items] -90 89 | -91 90 | -92 91 | def f(): + 87 |+ result = [i async for i in items] # PERF401 +90 88 | +91 89 | +92 90 | def f(): PERF401.py:95:9: PERF401 [*] Use `list.extend` to create a transformed list | @@ -154,11 +149,11 @@ PERF401.py:112:13: PERF401 [*] Use a list comprehension to create a transformed 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 + 108 |+ # comment after assignment should be protected + 109 |+ # single-line comment 1 should be protected + 110 |+ # single-line comment 2 should be protected + 111 |+ # single-line comment 3 should be protected + 112 |+ result = [i for i in range(10) if i % 2] # PERF401 113 113 | 114 114 | 115 115 | def f(): @@ -169,6 +164,8 @@ PERF401.py:127:13: PERF401 [*] Use a list comprehension to create a transformed 126 | for value in param: 127 | new_layers.append(value * 3) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF401 +128 | +129 | def f(): | = help: Replace for loop with list comprehension @@ -180,3 +177,95 @@ PERF401.py:127:13: PERF401 [*] Use a list comprehension to create a transformed 126 |- for value in param: 127 |- new_layers.append(value * 3) 125 |+ new_layers = [value * 3 for value in param] +128 126 | +129 127 | def f(): +130 128 | result = [] + +PERF401.py:133:9: PERF401 [*] Use a list comprehension to create a transformed list + | +131 | var = 1 +132 | for _ in range(10): +133 | result.append(var + 1) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 +134 | +135 | def f(): + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +127 127 | new_layers.append(value * 3) +128 128 | +129 129 | def f(): +130 |- result = [] +131 130 | var = 1 +132 |- for _ in range(10): +133 |- result.append(var + 1) # PERF401 + 131 |+ result = [var + 1 for _ in range(10)] # PERF401 +134 132 | +135 133 | def f(): +136 134 | # make sure that `tmp` is not deleted + +PERF401.py:139:9: PERF401 [*] Use a list comprehension to create a transformed list + | +137 | tmp = 1; result = [] # commment should be protected +138 | for i in range(10): +139 | result.append(i + 1) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 +140 | +141 | def f(): + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +134 134 | +135 135 | def f(): +136 136 | # make sure that `tmp` is not deleted +137 |- tmp = 1; result = [] # commment should be protected +138 |- for i in range(10): +139 |- result.append(i + 1) # PERF401 + 137 |+ # commment should be protected + 138 |+ result = [i + 1 for i in range(10)] # PERF401 +140 139 | +141 140 | def f(): +142 141 | # make sure that `tmp` is not deleted + +PERF401.py:145:9: PERF401 [*] Use a list comprehension to create a transformed list + | +143 | result = []; tmp = 1 # commment should be protected +144 | for i in range(10): +145 | result.append(i + 1) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +140 140 | +141 141 | def f(): +142 142 | # make sure that `tmp` is not deleted +143 |- result = []; tmp = 1 # commment should be protected +144 |- for i in range(10): +145 |- result.append(i + 1) # PERF401 + 143 |+ # commment should be protected + 144 |+ result = [i + 1 for i in range(10)] # PERF401 +146 145 | +147 146 | +148 147 | def f(): + +PERF401.py:151:9: PERF401 [*] Use a list comprehension to create a transformed list + | +149 | result = [] # comment should be protected +150 | for i in range(10): +151 | result.append(i*2) # PERF401 + | ^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +146 146 | +147 147 | +148 148 | def f(): +149 |- result = [] # comment should be protected +150 |- for i in range(10): +151 |- result.append(i*2) # PERF401 + 149 |+ # comment should be protected + 150 |+ result = [i*2 for i in range(10)] # PERF401 From fd71328df0d99573579bdb8f83d1a6e2e645d948 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Fri, 15 Nov 2024 13:47:15 -0500 Subject: [PATCH 02/21] tentative fix --- .../rules/manual_list_comprehension.rs | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 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 eb6993ebfd296..4108b003c57bd 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,5 +1,6 @@ use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; +use crate::checkers::ast::Checker; use anyhow::{anyhow, Result}; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; @@ -10,8 +11,6 @@ use ruff_python_trivia::PythonWhitespace; use ruff_source_file::LineRanges; use ruff_text_size::{Ranged, TextRange}; -use crate::checkers::ast::Checker; - /// ## What it does /// Checks for `for` loops that can be replaced by a list comprehension. /// @@ -250,13 +249,20 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S } }; + // If the binding has multiple statements on its line, it gets more complicated to fix, so we use an extend + // TODO: make this work + let binding_only_stmt_on_line = binding_stmt.is_some_and(|_binding_stmt| true); + // A list extend works in every context, while a list comprehension only works when all the criteria are true - let comprehension_type = - if binding_is_empty_list && assignment_in_same_statement && binding_has_one_target { - ComprehensionType::ListComprehension - } else { - ComprehensionType::Extend - }; + let comprehension_type = if binding_is_empty_list + && assignment_in_same_statement + && binding_has_one_target + && binding_only_stmt_on_line + { + ComprehensionType::ListComprehension + } else { + ComprehensionType::Extend + }; let mut diagnostic = Diagnostic::new( ManualListComprehension { @@ -318,7 +324,7 @@ fn convert_to_list_extend( let variable_name = locator.slice(binding); let for_loop_inline_comments: Vec<&str> = comment_strings_in_range(for_stmt.range); - + let newline = checker.stylist().line_ending().as_str(); let indent_range = TextRange::new( locator.line_start(for_stmt.range.start()), @@ -364,7 +370,7 @@ fn convert_to_list_extend( let comprehension_body = format!("{leading_comments}{variable_name} = [{generator_str}]"); - // TODO: protect comment from after assignment statement + let binding_stmt_range = locator.full_lines_range(binding_stmt.range); Ok(Fix::unsafe_edits( From f30118e7e754f3ca8baf7c900121005a9a5ab388 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Fri, 15 Nov 2024 16:00:11 -0500 Subject: [PATCH 03/21] don't use a comprehension when the list is referenced --- .../test/fixtures/perflint/PERF401.py | 7 +++++++ .../rules/manual_list_comprehension.rs | 14 +++++++++++++ ...__perflint__tests__PERF401_PERF401.py.snap | 9 +++++++++ ...t__tests__preview__PERF401_PERF401.py.snap | 20 +++++++++++++++++++ 4 files changed, 50 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index bccb34115850c..3c131f045e95c 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -149,3 +149,10 @@ def f(): result = [] # comment should be protected for i in range(10): result.append(i*2) # PERF401 + + +def f(): + result = [] + result.append(1) + for i in range(10): + result.append(i*2) # PERF401 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 4108b003c57bd..6196e91e8f472 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 @@ -249,6 +249,19 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S } }; + // If the binding gets used in between the assignment and the for loop, a list comprehension is no longer safe + let binding_unused_between = binding_stmt.is_some_and(|binding_stmt| { + let from_assign_to_loop = binding_stmt.range.cover(for_stmt.range); + // count the number of references between the assignment and the for loop + let count = binding + .references() + .map(|ref_id| checker.semantic().reference(ref_id).range()) + .filter(|text_range| from_assign_to_loop.contains_range(*text_range)) + .count(); + // if there's more than one, then it's been accessed in the middle somewhere, so it's not safe to change into a list comprehension + count < 2 + }); + // If the binding has multiple statements on its line, it gets more complicated to fix, so we use an extend // TODO: make this work let binding_only_stmt_on_line = binding_stmt.is_some_and(|_binding_stmt| true); @@ -258,6 +271,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S && assignment_in_same_statement && binding_has_one_target && binding_only_stmt_on_line + && binding_unused_between { ComprehensionType::ListComprehension } else { 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 13abff49088d2..8426b653a2f43 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 @@ -114,3 +114,12 @@ PERF401.py:151:9: PERF401 Use a list comprehension to create a transformed list | ^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension + +PERF401.py:158:9: PERF401 Use `list.extend` to create a transformed list + | +156 | result.append(1) +157 | for i in range(10): +158 | result.append(i*2) # PERF401 + | ^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list.extend 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 fdc7d2092f2f5..b667f052e1131 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 @@ -269,3 +269,23 @@ PERF401.py:151:9: PERF401 [*] Use a list comprehension to create a transformed l 151 |- result.append(i*2) # PERF401 149 |+ # comment should be protected 150 |+ result = [i*2 for i in range(10)] # PERF401 +152 151 | +153 152 | +154 153 | def f(): + +PERF401.py:158:9: PERF401 [*] Use `list.extend` to create a transformed list + | +156 | result.append(1) +157 | for i in range(10): +158 | result.append(i*2) # PERF401 + | ^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list.extend + +ℹ Unsafe fix +154 154 | def f(): +155 155 | result = [] +156 156 | result.append(1) +157 |- for i in range(10): +158 |- result.append(i*2) # PERF401 + 157 |+ result.extend(i*2 for i in range(10)) # PERF401 From 238d17a54fbf797b1ef1e314163fd64401c1b37e Mon Sep 17 00:00:00 2001 From: Sebastian Date: Sun, 17 Nov 2024 20:58:07 -0500 Subject: [PATCH 04/21] check for external references to the for loop variable --- .../test/fixtures/perflint/PERF401.py | 12 ++++++++ .../rules/manual_list_comprehension.rs | 30 ++++++++++++++++++- ...__perflint__tests__PERF401_PERF401.py.snap | 12 ++++++++ ...t__tests__preview__PERF401_PERF401.py.snap | 25 ++++++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index 3c131f045e95c..23143287d9fe1 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -156,3 +156,15 @@ def f(): result.append(1) for i in range(10): result.append(i*2) # PERF401 + +def f(): + result = [] + result += [1] + for i in range(10): + result.append(i*2) # PERF401 + +def f(): + result = [] + for val in range(5): + result.append(val * 2) # Ok + print(val) 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 6196e91e8f472..60f4d30d0d9ef 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 @@ -218,6 +218,34 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S return; } + // Avoid if the for-loop target is used outside of the for loop, e.g., + // + // ```python + // for x in y: + // filtered.append(x) + // print(x) + // ``` + // + // If this were a comprehension, x would no longer have the correct scope: + // + // ```python + // filtered = [x for x in y] + // print(x) + // ``` + let for_loop_target = checker + .semantic() + .lookup_symbol(id.as_str()) + .map(|id| checker.semantic().binding(id)) + .expect("for loop target must exist"); + // TODO: this currently does not properly find usages outside the for loop; figure out why + if for_loop_target + .references() + .map(|id| checker.semantic().reference(id)) + .any(|reference| !for_stmt.range.contains_range(reference.range())) + { + return; + } + let binding_stmt = binding .statement(checker.semantic()) .and_then(|stmt| stmt.as_assign_stmt()); @@ -261,7 +289,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S // if there's more than one, then it's been accessed in the middle somewhere, so it's not safe to change into a list comprehension count < 2 }); - + // If the binding has multiple statements on its line, it gets more complicated to fix, so we use an extend // TODO: make this work let binding_only_stmt_on_line = binding_stmt.is_some_and(|_binding_stmt| true); 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 8426b653a2f43..1c94cf8c39ba0 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 @@ -121,5 +121,17 @@ PERF401.py:158:9: PERF401 Use `list.extend` to create a transformed list 157 | for i in range(10): 158 | result.append(i*2) # PERF401 | ^^^^^^^^^^^^^^^^^^ PERF401 +159 | +160 | def f(): | = help: Replace for loop with list.extend + +PERF401.py:169:9: PERF401 Use a list comprehension to create a transformed list + | +167 | result = [] +168 | for val in range(5): +169 | result.append(val * 2) # Ok + | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 +170 | print(val) + | + = help: Replace for loop with list comprehension 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 b667f052e1131..999a591bfeb89 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 @@ -279,6 +279,8 @@ PERF401.py:158:9: PERF401 [*] Use `list.extend` to create a transformed list 157 | for i in range(10): 158 | result.append(i*2) # PERF401 | ^^^^^^^^^^^^^^^^^^ PERF401 +159 | +160 | def f(): | = help: Replace for loop with list.extend @@ -289,3 +291,26 @@ PERF401.py:158:9: PERF401 [*] Use `list.extend` to create a transformed list 157 |- for i in range(10): 158 |- result.append(i*2) # PERF401 157 |+ result.extend(i*2 for i in range(10)) # PERF401 +159 158 | +160 159 | def f(): +161 160 | result = [] + +PERF401.py:169:9: PERF401 [*] Use a list comprehension to create a transformed list + | +167 | result = [] +168 | for val in range(5): +169 | result.append(val * 2) # Ok + | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 +170 | print(val) + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +164 164 | result.append(i*2) # PERF401 +165 165 | +166 166 | def f(): +167 |- result = [] +168 |- for val in range(5): +169 |- result.append(val * 2) # Ok + 167 |+ result = [val * 2 for val in range(5)] # Ok +170 168 | print(val) From 46bd85114c7378dde8516bcf0df1f6ec193b8c16 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Sun, 17 Nov 2024 21:12:02 -0500 Subject: [PATCH 05/21] stop duplicating comments inside the fixed append --- .../test/fixtures/perflint/PERF401.py | 11 ++++++ .../rules/manual_list_comprehension.rs | 3 +- ...__perflint__tests__PERF401_PERF401.py.snap | 16 +++++++++ ...t__tests__preview__PERF401_PERF401.py.snap | 34 +++++++++++++++++++ 4 files changed, 63 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index 23143287d9fe1..a0e29b660b649 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -168,3 +168,14 @@ def f(): for val in range(5): result.append(val * 2) # Ok print(val) + +def f(): + result = [] + for i in range(2): + result.append( + ( + i+1, + # Comment should not be duplicated + 2 + ) + ) # PERF401 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 60f4d30d0d9ef..6ae5836874971 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 @@ -360,6 +360,8 @@ fn convert_to_list_extend( .comment_ranges() .comments_in_range(range) .iter() + // Ignore comments inside of the append, since these are preserved + .filter(|comment| !to_append.range().contains_range(**comment)) .map(|range| locator.slice(range).trim_whitespace_start()) .collect() }; @@ -398,7 +400,6 @@ fn convert_to_list_extend( .ok_or(anyhow!( "Binding must have a statement to convert into a list comprehension" ))?; - let mut comments_to_move = comment_strings_in_range(locator.full_lines_range(binding_stmt.range)); comments_to_move.extend(for_loop_inline_comments); 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 1c94cf8c39ba0..b432b3d38d092 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 @@ -135,3 +135,19 @@ PERF401.py:169:9: PERF401 Use a list comprehension to create a transformed list 170 | print(val) | = help: Replace for loop with list comprehension + +PERF401.py:175:9: PERF401 Use a list comprehension to create a transformed list + | +173 | result = [] +174 | for i in range(2): +175 | result.append( + | _________^ +176 | | ( +177 | | i+1, +178 | | # Comment should not be duplicated +179 | | 2 +180 | | ) +181 | | ) # PERF401 + | |_________^ PERF401 + | + = help: Replace for loop with list comprehension 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 999a591bfeb89..4d6b32f0f5a52 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 @@ -314,3 +314,37 @@ PERF401.py:169:9: PERF401 [*] Use a list comprehension to create a transformed l 169 |- result.append(val * 2) # Ok 167 |+ result = [val * 2 for val in range(5)] # Ok 170 168 | print(val) +171 169 | +172 170 | def f(): + +PERF401.py:175:9: PERF401 [*] Use a list comprehension to create a transformed list + | +173 | result = [] +174 | for i in range(2): +175 | result.append( + | _________^ +176 | | ( +177 | | i+1, +178 | | # Comment should not be duplicated +179 | | 2 +180 | | ) +181 | | ) # PERF401 + | |_________^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +170 170 | print(val) +171 171 | +172 172 | def f(): +173 |- result = [] +174 |- for i in range(2): +175 |- result.append( +176 |- ( + 173 |+ result = [( +177 174 | i+1, +178 175 | # Comment should not be duplicated +179 176 | 2 +180 |- ) +181 |- ) # PERF401 + 177 |+ ) for i in range(2)] # PERF401 From 9a336fd13ce08abf161764afcf609a259c860ba9 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 18 Nov 2024 11:09:27 -0500 Subject: [PATCH 06/21] fix annotated returns, but lose type info --- .../test/fixtures/perflint/PERF401.py | 5 +++ .../rules/manual_list_comprehension.rs | 35 ++++++++++++------- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index a0e29b660b649..f6d2fcb73ebc2 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -179,3 +179,8 @@ def f(): 2 ) ) # PERF401 + +def f(): + result: list[int] = [] + for i in range(10): + result.append(i*2) # PERF401 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 6ae5836874971..9809fee6416de 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 @@ -246,14 +246,17 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S return; } - let binding_stmt = binding - .statement(checker.semantic()) - .and_then(|stmt| stmt.as_assign_stmt()); - + let binding_stmt = binding.statement(checker.semantic()); + let binding_value = binding_stmt.and_then(|binding_stmt| match binding_stmt { + ast::Stmt::AnnAssign(assign) => assign.value.as_ref(), + ast::Stmt::Assign(assign) => Some(&assign.value), + _ => None, + }); + dbg!(binding_value); // 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 = - binding_stmt.is_some_and(|binding_stmt| match binding_stmt.value.as_list_expr() { + binding_value.is_some_and(|binding_value| match binding_value.as_list_expr() { Some(list_expr) => list_expr.elts.is_empty(), None => false, }); @@ -270,8 +273,13 @@ 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_target = binding_stmt.and_then(|binding_stmt| match binding_stmt { + ast::Stmt::AnnAssign(assign) => Some(std::slice::from_ref(assign.target.as_ref())), + ast::Stmt::Assign(assign) => Some(assign.targets.as_slice()), + _ => None, + }); let binding_has_one_target = { - match binding_stmt.map(|binding_stmt| binding_stmt.targets.as_slice()) { + match binding_target { Some([only_target]) => only_target.is_name_expr(), _ => false, } @@ -279,7 +287,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S // If the binding gets used in between the assignment and the for loop, a list comprehension is no longer safe let binding_unused_between = binding_stmt.is_some_and(|binding_stmt| { - let from_assign_to_loop = binding_stmt.range.cover(for_stmt.range); + let from_assign_to_loop = binding_stmt.range().cover(for_stmt.range); // count the number of references between the assignment and the for loop let count = binding .references() @@ -394,14 +402,19 @@ fn convert_to_list_extend( ))) } ComprehensionType::ListComprehension => { - let binding_stmt = binding + let binding_stmt_range = binding .statement(checker.semantic()) - .and_then(|stmt| stmt.as_assign_stmt()) + .and_then(|stmt| match stmt { + ast::Stmt::AnnAssign(assign) => Some(assign.range), + ast::Stmt::Assign(assign) => Some(assign.range), + _ => None, + }) + .map(|range| locator.full_lines_range(range)) .ok_or(anyhow!( "Binding must have a statement to convert into a list comprehension" ))?; let mut comments_to_move = - comment_strings_in_range(locator.full_lines_range(binding_stmt.range)); + comment_strings_in_range(binding_stmt_range); comments_to_move.extend(for_loop_inline_comments); let indentation = if comments_to_move.is_empty() { @@ -414,8 +427,6 @@ fn convert_to_list_extend( let comprehension_body = format!("{leading_comments}{variable_name} = [{generator_str}]"); - let binding_stmt_range = locator.full_lines_range(binding_stmt.range); - Ok(Fix::unsafe_edits( Edit::range_deletion(binding_stmt_range), [Edit::range_replacement(comprehension_body, for_stmt.range)], From 63b0e1e4dc3cbd7724a564e591c994d8e9366130 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 18 Nov 2024 11:16:10 -0500 Subject: [PATCH 07/21] apply fix to type-annotated lists --- .../rules/manual_list_comprehension.rs | 15 ++++++++---- ...__perflint__tests__PERF401_PERF401.py.snap | 11 +++++++++ ...t__tests__preview__PERF401_PERF401.py.snap | 23 +++++++++++++++++++ 3 files changed, 45 insertions(+), 4 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 9809fee6416de..44baf54c4c5b5 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 @@ -252,7 +252,6 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S ast::Stmt::Assign(assign) => Some(&assign.value), _ => None, }); - dbg!(binding_value); // 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 = @@ -413,8 +412,16 @@ fn convert_to_list_extend( .ok_or(anyhow!( "Binding must have a statement to convert into a list comprehension" ))?; - let mut comments_to_move = - comment_strings_in_range(binding_stmt_range); + + let annotations = match binding + .statement(checker.semantic()) + .and_then(|stmt| stmt.as_ann_assign_stmt()) + { + Some(assign) => format!(": {}", locator.slice(assign.annotation.range())), + None => String::new(), + }; + + let mut comments_to_move = comment_strings_in_range(binding_stmt_range); comments_to_move.extend(for_loop_inline_comments); let indentation = if comments_to_move.is_empty() { @@ -425,7 +432,7 @@ fn convert_to_list_extend( let leading_comments = format!("{}{indentation}", comments_to_move.join(&indentation)); let comprehension_body = - format!("{leading_comments}{variable_name} = [{generator_str}]"); + format!("{leading_comments}{variable_name}{annotations} = [{generator_str}]"); Ok(Fix::unsafe_edits( Edit::range_deletion(binding_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 b432b3d38d092..679574466b816 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 @@ -149,5 +149,16 @@ PERF401.py:175:9: PERF401 Use a list comprehension to create a transformed list 180 | | ) 181 | | ) # PERF401 | |_________^ PERF401 +182 | +183 | def f(): + | + = help: Replace for loop with list comprehension + +PERF401.py:186:9: PERF401 Use a list comprehension to create a transformed list + | +184 | result: list[int] = [] +185 | for i in range(10): +186 | result.append(i*2) # PERF401 + | ^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension 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 4d6b32f0f5a52..403c88f9ec67e 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 @@ -330,6 +330,8 @@ PERF401.py:175:9: PERF401 [*] Use a list comprehension to create a transformed l 180 | | ) 181 | | ) # PERF401 | |_________^ PERF401 +182 | +183 | def f(): | = help: Replace for loop with list comprehension @@ -348,3 +350,24 @@ PERF401.py:175:9: PERF401 [*] Use a list comprehension to create a transformed l 180 |- ) 181 |- ) # PERF401 177 |+ ) for i in range(2)] # PERF401 +182 178 | +183 179 | def f(): +184 180 | result: list[int] = [] + +PERF401.py:186:9: PERF401 [*] Use a list comprehension to create a transformed list + | +184 | result: list[int] = [] +185 | for i in range(10): +186 | result.append(i*2) # PERF401 + | ^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +181 181 | ) # PERF401 +182 182 | +183 183 | def f(): +184 |- result: list[int] = [] +185 |- for i in range(10): +186 |- result.append(i*2) # PERF401 + 184 |+ result: list[int] = [i*2 for i in range(10)] # PERF401 From 527a050cb0f1890b5317a9b29c7229cb190c9ef8 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Tue, 3 Dec 2024 15:10:03 -0500 Subject: [PATCH 08/21] fix async extends, implicit tuples, and comments in iterator --- .../test/fixtures/perflint/PERF401.py | 89 ++-- .../rules/manual_list_comprehension.rs | 33 +- ...__perflint__tests__PERF401_PERF401.py.snap | 153 +++--- ...t__tests__preview__PERF401_PERF401.py.snap | 477 ++++++++++-------- 4 files changed, 432 insertions(+), 320 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index f6d2fcb73ebc2..778ee4a6bfdcc 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -74,7 +74,7 @@ def f(): result.append(i) # Ok -def f(): +async def f(): items = [1, 2, 3, 4] result = [] async for i in items: @@ -82,17 +82,24 @@ def f(): result.append(i) # PERF401 -def f(): +async def f(): items = [1, 2, 3, 4] result = [] async for i in items: result.append(i) # PERF401 +async def f(): + items = [1, 2, 3, 4] + result = [1, 2] + async for i in items: + result.append(i) # PERF401 + + def f(): - result, _ = [1,2,3,4], ... + result, _ = [1, 2, 3, 4], ... for i in range(10): - result.append(i*2) # PERF401 + result.append(i * 2) # PERF401 def f(): @@ -100,23 +107,24 @@ def f(): if True: 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 + if i % 2: # single-line comment 3 should be protected + result.append(i) # PERF401 def f(): - result = [] # comment after assignment should be protected + 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 - result.append(i) # PERF401 + 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 + result.append(i * 2) # Ok + def f(param): # PERF401 @@ -126,61 +134,84 @@ def f(param): for value in param: new_layers.append(value * 3) + def f(): result = [] var = 1 for _ in range(10): - result.append(var + 1) # PERF401 + result.append(var + 1) # PERF401 + def f(): # make sure that `tmp` is not deleted - tmp = 1; result = [] # commment should be protected + tmp = 1 + result = [] # commment should be protected for i in range(10): - result.append(i + 1) # PERF401 + result.append(i + 1) # PERF401 + def f(): # make sure that `tmp` is not deleted - result = []; tmp = 1 # commment should be protected + result = [] + tmp = 1 # commment should be protected for i in range(10): - result.append(i + 1) # PERF401 + result.append(i + 1) # PERF401 def f(): - result = [] # comment should be protected + result = [] # comment should be protected for i in range(10): - result.append(i*2) # PERF401 + result.append(i * 2) # PERF401 def f(): result = [] result.append(1) for i in range(10): - result.append(i*2) # PERF401 + result.append(i * 2) # PERF401 + def f(): result = [] result += [1] for i in range(10): - result.append(i*2) # PERF401 + result.append(i * 2) # PERF401 + def f(): result = [] for val in range(5): - result.append(val * 2) # Ok + result.append(val * 2) # Ok print(val) + def f(): result = [] - for i in range(2): - result.append( - ( - i+1, - # Comment should not be duplicated - 2 - ) - ) # PERF401 + for i in range( # Comment 1 should not be duplicated + ( + 2 # Comment 2 + + 1 + ) + ): # Comment 3 + if i % 2: # Comment 4 + result.append( + ( + i + 1, + # Comment 5 + 2, + ) + ) # PERF401 + def f(): result: list[int] = [] for i in range(10): - result.append(i*2) # PERF401 + result.append(i * 2) # PERF401 + + +def f(): + a, b = [1, 2, 3], [4, 5, 6] + result = [] + for i in a, b: + result.append(i[0] + i[1]) # PERF401 + return result 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 44baf54c4c5b5..ad9b789993c41 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 @@ -352,7 +352,24 @@ fn convert_to_list_extend( None => String::new(), }; - let for_iter_str = locator.slice(for_stmt.iter.range()); + // if the loop target was an implicit tuple, add parentheses around it + // ```python + // for i in a, b: + // ... + // ``` + // becomes + // [... for i in (a, b)] + let for_iter_str = if for_stmt + .iter + .as_ref() + .as_tuple_expr() + .is_some_and(|expr| !expr.parenthesized) + { + format!("({})", locator.slice(for_stmt.iter.range())) + } else { + locator.slice(for_stmt.iter.range()).to_string() + }; + let for_type = if for_stmt.is_async { "async for" } else { @@ -367,8 +384,11 @@ fn convert_to_list_extend( .comment_ranges() .comments_in_range(range) .iter() - // Ignore comments inside of the append, since these are preserved - .filter(|comment| !to_append.range().contains_range(**comment)) + // Ignore comments inside of the append or iterator, since these are preserved + .filter(|comment| { + !to_append.range().contains_range(**comment) + && !for_stmt.iter.range().contains_range(**comment) + }) .map(|range| locator.slice(range).trim_whitespace_start()) .collect() }; @@ -384,6 +404,13 @@ fn convert_to_list_extend( match fix_type { ComprehensionType::Extend => { + let generator_str = if for_stmt.is_async { + // generators do not implement __iter__, so `async for` requires the generator to be a list + format!("[{generator_str}]") + } else { + generator_str + }; + let comprehension_body = format!("{variable_name}.extend({generator_str})"); let indentation = if for_loop_inline_comments.is_empty() { 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 679574466b816..eda59a513172a 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 @@ -37,128 +37,137 @@ PERF401.py:89:9: PERF401 Use an async list comprehension to create a transformed | = help: Replace for loop with list comprehension -PERF401.py:95:9: PERF401 Use `list.extend` to create a transformed list +PERF401.py:96:9: PERF401 Use `list.extend` with an async 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 +94 | result = [1, 2] +95 | async for i in items: +96 | result.append(i) # PERF401 + | ^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list.extend -PERF401.py:104:17: PERF401 Use `list.extend` to create a transformed list +PERF401.py:102:9: PERF401 Use `list.extend` to create a transformed list + | +100 | result, _ = [1, 2, 3, 4], ... +101 | for i in range(10): +102 | result.append(i * 2) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list.extend + +PERF401.py:111: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 -104 | result.append(i) # PERF401 +109 | # single-line comment 2 should be protected +110 | if i % 2: # single-line comment 3 should be protected +111 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list.extend -PERF401.py:112:13: PERF401 Use a list comprehension to create a transformed list +PERF401.py:119: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 +117 | # single-line comment 2 should be protected +118 | if i % 2: # single-line comment 3 should be protected +119 | result.append(i) # PERF401 | ^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:127:13: PERF401 Use a list comprehension to create a transformed list +PERF401.py:135:13: PERF401 Use a list comprehension to create a transformed list | -125 | new_layers = [] -126 | for value in param: -127 | new_layers.append(value * 3) +133 | new_layers = [] +134 | for value in param: +135 | new_layers.append(value * 3) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF401 -128 | -129 | def f(): | = help: Replace for loop with list comprehension -PERF401.py:133:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:142:9: PERF401 Use a list comprehension to create a transformed list | -131 | var = 1 -132 | for _ in range(10): -133 | result.append(var + 1) # PERF401 +140 | var = 1 +141 | for _ in range(10): +142 | result.append(var + 1) # PERF401 | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 -134 | -135 | def f(): | = help: Replace for loop with list comprehension -PERF401.py:139:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:150:9: PERF401 Use a list comprehension to create a transformed list | -137 | tmp = 1; result = [] # commment should be protected -138 | for i in range(10): -139 | result.append(i + 1) # PERF401 +148 | result = [] # commment should be protected +149 | for i in range(10): +150 | result.append(i + 1) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 -140 | -141 | def f(): | = help: Replace for loop with list comprehension -PERF401.py:145:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:158:9: PERF401 Use a list comprehension to create a transformed list | -143 | result = []; tmp = 1 # commment should be protected -144 | for i in range(10): -145 | result.append(i + 1) # PERF401 +156 | tmp = 1 # commment should be protected +157 | for i in range(10): +158 | result.append(i + 1) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:151:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:164:9: PERF401 Use a list comprehension to create a transformed list | -149 | result = [] # comment should be protected -150 | for i in range(10): -151 | result.append(i*2) # PERF401 - | ^^^^^^^^^^^^^^^^^^ PERF401 +162 | result = [] # comment should be protected +163 | for i in range(10): +164 | result.append(i * 2) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:158:9: PERF401 Use `list.extend` to create a transformed list +PERF401.py:171:9: PERF401 Use `list.extend` to create a transformed list | -156 | result.append(1) -157 | for i in range(10): -158 | result.append(i*2) # PERF401 - | ^^^^^^^^^^^^^^^^^^ PERF401 -159 | -160 | def f(): +169 | result.append(1) +170 | for i in range(10): +171 | result.append(i * 2) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list.extend -PERF401.py:169:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:184:9: PERF401 Use a list comprehension to create a transformed list | -167 | result = [] -168 | for val in range(5): -169 | result.append(val * 2) # Ok +182 | result = [] +183 | for val in range(5): +184 | result.append(val * 2) # Ok | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 -170 | print(val) +185 | print(val) | = help: Replace for loop with list comprehension -PERF401.py:175:9: PERF401 Use a list comprehension to create a transformed list - | -173 | result = [] -174 | for i in range(2): -175 | result.append( - | _________^ -176 | | ( -177 | | i+1, -178 | | # Comment should not be duplicated -179 | | 2 -180 | | ) -181 | | ) # PERF401 - | |_________^ PERF401 -182 | -183 | def f(): +PERF401.py:197:13: PERF401 Use a list comprehension to create a transformed list + | +195 | ): # Comment 3 +196 | if i % 2: # Comment 4 +197 | result.append( + | _____________^ +198 | | ( +199 | | i + 1, +200 | | # Comment 5 +201 | | 2, +202 | | ) +203 | | ) # PERF401 + | |_____________^ PERF401 + | + = help: Replace for loop with list comprehension + +PERF401.py:209:9: PERF401 Use a list comprehension to create a transformed list + | +207 | result: list[int] = [] +208 | for i in range(10): +209 | result.append(i * 2) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:186:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:216:9: PERF401 Use a list comprehension to create a transformed list | -184 | result: list[int] = [] -185 | for i in range(10): -186 | result.append(i*2) # PERF401 - | ^^^^^^^^^^^^^^^^^^ PERF401 +214 | result = [] +215 | for i in a, b: +216 | result.append(i[0] + i[1]) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF401 +217 | return result | = help: Replace for loop with list comprehension 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 403c88f9ec67e..0641bc52c996c 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 @@ -54,7 +54,7 @@ PERF401.py:82:13: PERF401 [*] Use an async list comprehension to create a transf ℹ Unsafe fix 76 76 | -77 77 | def f(): +77 77 | async def f(): 78 78 | items = [1, 2, 3, 4] 79 |- result = [] 80 |- async for i in items: @@ -63,7 +63,7 @@ PERF401.py:82:13: PERF401 [*] Use an async list comprehension to create a transf 79 |+ result = [i async for i in items if i % 2] # PERF401 83 80 | 84 81 | -85 82 | def f(): +85 82 | async def f(): PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transformed list | @@ -76,7 +76,7 @@ PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transfo ℹ Unsafe fix 84 84 | -85 85 | def f(): +85 85 | async def f(): 86 86 | items = [1, 2, 3, 4] 87 |- result = [] 88 |- async for i in items: @@ -84,290 +84,335 @@ PERF401.py:89:9: PERF401 [*] Use an async list comprehension to create a transfo 87 |+ result = [i async for i in items] # PERF401 90 88 | 91 89 | -92 90 | def f(): +92 90 | async def f(): -PERF401.py:95:9: PERF401 [*] Use `list.extend` to create a transformed list +PERF401.py:96:9: PERF401 [*] Use `list.extend` with an async 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 +94 | result = [1, 2] +95 | async for i in items: +96 | result.append(i) # 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 | +92 92 | async def f(): +93 93 | items = [1, 2, 3, 4] +94 94 | result = [1, 2] +95 |- async for i in items: +96 |- result.append(i) # PERF401 + 95 |+ result.extend([i async for i in items]) # PERF401 97 96 | -98 97 | def f(): +98 97 | +99 98 | def f(): + +PERF401.py:102:9: PERF401 [*] Use `list.extend` to create a transformed list + | +100 | result, _ = [1, 2, 3, 4], ... +101 | for i in range(10): +102 | result.append(i * 2) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list.extend + +ℹ Unsafe fix +98 98 | +99 99 | def f(): +100 100 | result, _ = [1, 2, 3, 4], ... +101 |- for i in range(10): +102 |- result.append(i * 2) # PERF401 + 101 |+ result.extend(i * 2 for i in range(10)) # PERF401 +103 102 | +104 103 | +105 104 | def f(): -PERF401.py:104:17: PERF401 [*] Use `list.extend` to create a transformed list +PERF401.py:111: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 -104 | result.append(i) # PERF401 +109 | # single-line comment 2 should be protected +110 | if i % 2: # single-line comment 3 should be protected +111 | 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 +105 105 | def f(): +106 106 | result = [] +107 107 | if True: +108 |- for i in range(10): # single-line comment 1 should be protected +109 |- # single-line comment 2 should be protected +110 |- if i % 2: # single-line comment 3 should be protected +111 |- 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 |+ result.extend(i for i in range(10) if i % 2) # PERF401 +112 112 | +113 113 | +114 114 | def f(): + +PERF401.py:119: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 +117 | # single-line comment 2 should be protected +118 | if i % 2: # single-line comment 3 should be protected +119 | 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 |+ # comment after assignment should be protected - 109 |+ # single-line comment 1 should be protected - 110 |+ # single-line comment 2 should be protected - 111 |+ # single-line comment 3 should be protected - 112 |+ result = [i for i in range(10) if i % 2] # PERF401 +112 112 | 113 113 | -114 114 | -115 115 | def f(): +114 114 | def f(): +115 |- result = [] # comment after assignment should be protected +116 |- for i in range(10): # single-line comment 1 should be protected +117 |- # single-line comment 2 should be protected +118 |- if i % 2: # single-line comment 3 should be protected +119 |- result.append(i) # PERF401 + 115 |+ # comment after assignment should be protected + 116 |+ # single-line comment 1 should be protected + 117 |+ # single-line comment 2 should be protected + 118 |+ # single-line comment 3 should be protected + 119 |+ result = [i for i in range(10) if i % 2] # PERF401 +120 120 | +121 121 | +122 122 | def f(): -PERF401.py:127:13: PERF401 [*] Use a list comprehension to create a transformed list +PERF401.py:135:13: PERF401 [*] Use a list comprehension to create a transformed list | -125 | new_layers = [] -126 | for value in param: -127 | new_layers.append(value * 3) +133 | new_layers = [] +134 | for value in param: +135 | new_layers.append(value * 3) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF401 -128 | -129 | def f(): | = help: Replace for loop with list comprehension ℹ Unsafe fix -122 122 | # PERF401 -123 123 | # make sure the fix does not panic if there is no comments -124 124 | if param: -125 |- new_layers = [] -126 |- for value in param: -127 |- new_layers.append(value * 3) - 125 |+ new_layers = [value * 3 for value in param] -128 126 | -129 127 | def f(): -130 128 | result = [] - -PERF401.py:133:9: PERF401 [*] Use a list comprehension to create a transformed list +130 130 | # PERF401 +131 131 | # make sure the fix does not panic if there is no comments +132 132 | if param: +133 |- new_layers = [] +134 |- for value in param: +135 |- new_layers.append(value * 3) + 133 |+ new_layers = [value * 3 for value in param] +136 134 | +137 135 | +138 136 | def f(): + +PERF401.py:142:9: PERF401 [*] Use a list comprehension to create a transformed list | -131 | var = 1 -132 | for _ in range(10): -133 | result.append(var + 1) # PERF401 +140 | var = 1 +141 | for _ in range(10): +142 | result.append(var + 1) # PERF401 | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 -134 | -135 | def f(): | = help: Replace for loop with list comprehension ℹ Unsafe fix -127 127 | new_layers.append(value * 3) -128 128 | -129 129 | def f(): -130 |- result = [] -131 130 | var = 1 -132 |- for _ in range(10): -133 |- result.append(var + 1) # PERF401 - 131 |+ result = [var + 1 for _ in range(10)] # PERF401 -134 132 | -135 133 | def f(): -136 134 | # make sure that `tmp` is not deleted - -PERF401.py:139:9: PERF401 [*] Use a list comprehension to create a transformed list +136 136 | +137 137 | +138 138 | def f(): +139 |- result = [] +140 139 | var = 1 +141 |- for _ in range(10): +142 |- result.append(var + 1) # PERF401 + 140 |+ result = [var + 1 for _ in range(10)] # PERF401 +143 141 | +144 142 | +145 143 | def f(): + +PERF401.py:150:9: PERF401 [*] Use a list comprehension to create a transformed list | -137 | tmp = 1; result = [] # commment should be protected -138 | for i in range(10): -139 | result.append(i + 1) # PERF401 +148 | result = [] # commment should be protected +149 | for i in range(10): +150 | result.append(i + 1) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 -140 | -141 | def f(): | = help: Replace for loop with list comprehension ℹ Unsafe fix -134 134 | -135 135 | def f(): -136 136 | # make sure that `tmp` is not deleted -137 |- tmp = 1; result = [] # commment should be protected -138 |- for i in range(10): -139 |- result.append(i + 1) # PERF401 - 137 |+ # commment should be protected - 138 |+ result = [i + 1 for i in range(10)] # PERF401 -140 139 | -141 140 | def f(): -142 141 | # make sure that `tmp` is not deleted - -PERF401.py:145:9: PERF401 [*] Use a list comprehension to create a transformed list +145 145 | def f(): +146 146 | # make sure that `tmp` is not deleted +147 147 | tmp = 1 +148 |- result = [] # commment should be protected +149 |- for i in range(10): +150 |- result.append(i + 1) # PERF401 + 148 |+ # commment should be protected + 149 |+ result = [i + 1 for i in range(10)] # PERF401 +151 150 | +152 151 | +153 152 | def f(): + +PERF401.py:158:9: PERF401 [*] Use a list comprehension to create a transformed list | -143 | result = []; tmp = 1 # commment should be protected -144 | for i in range(10): -145 | result.append(i + 1) # PERF401 +156 | tmp = 1 # commment should be protected +157 | for i in range(10): +158 | result.append(i + 1) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension ℹ Unsafe fix -140 140 | -141 141 | def f(): -142 142 | # make sure that `tmp` is not deleted -143 |- result = []; tmp = 1 # commment should be protected -144 |- for i in range(10): -145 |- result.append(i + 1) # PERF401 - 143 |+ # commment should be protected - 144 |+ result = [i + 1 for i in range(10)] # PERF401 -146 145 | -147 146 | -148 147 | def f(): - -PERF401.py:151:9: PERF401 [*] Use a list comprehension to create a transformed list +152 152 | +153 153 | def f(): +154 154 | # make sure that `tmp` is not deleted +155 |- result = [] +156 155 | tmp = 1 # commment should be protected +157 |- for i in range(10): +158 |- result.append(i + 1) # PERF401 + 156 |+ result = [i + 1 for i in range(10)] # PERF401 +159 157 | +160 158 | +161 159 | def f(): + +PERF401.py:164:9: PERF401 [*] Use a list comprehension to create a transformed list | -149 | result = [] # comment should be protected -150 | for i in range(10): -151 | result.append(i*2) # PERF401 - | ^^^^^^^^^^^^^^^^^^ PERF401 +162 | result = [] # comment should be protected +163 | for i in range(10): +164 | result.append(i * 2) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension ℹ Unsafe fix -146 146 | -147 147 | -148 148 | def f(): -149 |- result = [] # comment should be protected -150 |- for i in range(10): -151 |- result.append(i*2) # PERF401 - 149 |+ # comment should be protected - 150 |+ result = [i*2 for i in range(10)] # PERF401 -152 151 | -153 152 | -154 153 | def f(): +159 159 | +160 160 | +161 161 | def f(): +162 |- result = [] # comment should be protected +163 |- for i in range(10): +164 |- result.append(i * 2) # PERF401 + 162 |+ # comment should be protected + 163 |+ result = [i * 2 for i in range(10)] # PERF401 +165 164 | +166 165 | +167 166 | def f(): -PERF401.py:158:9: PERF401 [*] Use `list.extend` to create a transformed list +PERF401.py:171:9: PERF401 [*] Use `list.extend` to create a transformed list | -156 | result.append(1) -157 | for i in range(10): -158 | result.append(i*2) # PERF401 - | ^^^^^^^^^^^^^^^^^^ PERF401 -159 | -160 | def f(): +169 | result.append(1) +170 | for i in range(10): +171 | result.append(i * 2) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list.extend ℹ Unsafe fix -154 154 | def f(): -155 155 | result = [] -156 156 | result.append(1) -157 |- for i in range(10): -158 |- result.append(i*2) # PERF401 - 157 |+ result.extend(i*2 for i in range(10)) # PERF401 -159 158 | -160 159 | def f(): -161 160 | result = [] +167 167 | def f(): +168 168 | result = [] +169 169 | result.append(1) +170 |- for i in range(10): +171 |- result.append(i * 2) # PERF401 + 170 |+ result.extend(i * 2 for i in range(10)) # PERF401 +172 171 | +173 172 | +174 173 | def f(): -PERF401.py:169:9: PERF401 [*] Use a list comprehension to create a transformed list +PERF401.py:184:9: PERF401 [*] Use a list comprehension to create a transformed list | -167 | result = [] -168 | for val in range(5): -169 | result.append(val * 2) # Ok +182 | result = [] +183 | for val in range(5): +184 | result.append(val * 2) # Ok | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 -170 | print(val) +185 | print(val) | = help: Replace for loop with list comprehension ℹ Unsafe fix -164 164 | result.append(i*2) # PERF401 -165 165 | -166 166 | def f(): -167 |- result = [] -168 |- for val in range(5): -169 |- result.append(val * 2) # Ok - 167 |+ result = [val * 2 for val in range(5)] # Ok -170 168 | print(val) -171 169 | -172 170 | def f(): - -PERF401.py:175:9: PERF401 [*] Use a list comprehension to create a transformed list +179 179 | +180 180 | +181 181 | def f(): +182 |- result = [] +183 |- for val in range(5): +184 |- result.append(val * 2) # Ok + 182 |+ result = [val * 2 for val in range(5)] # Ok +185 183 | print(val) +186 184 | +187 185 | + +PERF401.py:197:13: PERF401 [*] Use a list comprehension to create a transformed list | -173 | result = [] -174 | for i in range(2): -175 | result.append( - | _________^ -176 | | ( -177 | | i+1, -178 | | # Comment should not be duplicated -179 | | 2 -180 | | ) -181 | | ) # PERF401 - | |_________^ PERF401 -182 | -183 | def f(): +195 | ): # Comment 3 +196 | if i % 2: # Comment 4 +197 | result.append( + | _____________^ +198 | | ( +199 | | i + 1, +200 | | # Comment 5 +201 | | 2, +202 | | ) +203 | | ) # PERF401 + | |_____________^ PERF401 | = help: Replace for loop with list comprehension ℹ Unsafe fix -170 170 | print(val) -171 171 | -172 172 | def f(): -173 |- result = [] -174 |- for i in range(2): -175 |- result.append( -176 |- ( - 173 |+ result = [( -177 174 | i+1, -178 175 | # Comment should not be duplicated -179 176 | 2 -180 |- ) -181 |- ) # PERF401 - 177 |+ ) for i in range(2)] # PERF401 -182 178 | -183 179 | def f(): -184 180 | result: list[int] = [] - -PERF401.py:186:9: PERF401 [*] Use a list comprehension to create a transformed list +186 186 | +187 187 | +188 188 | def f(): +189 |- result = [] +190 |- for i in range( # Comment 1 should not be duplicated + 189 |+ # Comment 3 + 190 |+ # Comment 4 + 191 |+ result = [( + 192 |+ i + 1, + 193 |+ # Comment 5 + 194 |+ 2, + 195 |+ ) for i in range( # Comment 1 should not be duplicated +191 196 | ( +192 197 | 2 # Comment 2 +193 198 | + 1 +194 199 | ) +195 |- ): # Comment 3 +196 |- if i % 2: # Comment 4 +197 |- result.append( +198 |- ( +199 |- i + 1, +200 |- # Comment 5 +201 |- 2, +202 |- ) +203 |- ) # PERF401 + 200 |+ ) if i % 2] # PERF401 +204 201 | +205 202 | +206 203 | def f(): + +PERF401.py:209:9: PERF401 [*] Use a list comprehension to create a transformed list + | +207 | result: list[int] = [] +208 | for i in range(10): +209 | result.append(i * 2) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +204 204 | +205 205 | +206 206 | def f(): +207 |- result: list[int] = [] +208 |- for i in range(10): +209 |- result.append(i * 2) # PERF401 + 207 |+ result: list[int] = [i * 2 for i in range(10)] # PERF401 +210 208 | +211 209 | +212 210 | def f(): + +PERF401.py:216:9: PERF401 [*] Use a list comprehension to create a transformed list | -184 | result: list[int] = [] -185 | for i in range(10): -186 | result.append(i*2) # PERF401 - | ^^^^^^^^^^^^^^^^^^ PERF401 +214 | result = [] +215 | for i in a, b: +216 | result.append(i[0] + i[1]) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF401 +217 | return result | = help: Replace for loop with list comprehension ℹ Unsafe fix -181 181 | ) # PERF401 -182 182 | -183 183 | def f(): -184 |- result: list[int] = [] -185 |- for i in range(10): -186 |- result.append(i*2) # PERF401 - 184 |+ result: list[int] = [i*2 for i in range(10)] # PERF401 +211 211 | +212 212 | def f(): +213 213 | a, b = [1, 2, 3], [4, 5, 6] +214 |- result = [] +215 |- for i in a, b: +216 |- result.append(i[0] + i[1]) # PERF401 + 214 |+ result = [i[0] + i[1] for i in (a, b)] # PERF401 +217 215 | return result From f339d7013ec1334235fa106a51e7a6d8f14f360b Mon Sep 17 00:00:00 2001 From: Sebastian Date: Thu, 5 Dec 2024 19:21:45 -0500 Subject: [PATCH 09/21] stop deleting semicolon statements in the same line as binding --- .../test/fixtures/perflint/PERF401.py | 6 +- .../rules/manual_list_comprehension.rs | 60 +++- ...__perflint__tests__PERF401_PERF401.py.snap | 80 ++--- ...t__tests__preview__PERF401_PERF401.py.snap | 292 +++++++++--------- 4 files changed, 236 insertions(+), 202 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index 778ee4a6bfdcc..aef5bd63ecbc3 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -144,16 +144,14 @@ def f(): def f(): # make sure that `tmp` is not deleted - tmp = 1 - result = [] # commment should be protected + tmp = 1; result = [] # commment should be protected for i in range(10): result.append(i + 1) # PERF401 def f(): # make sure that `tmp` is not deleted - result = [] - tmp = 1 # commment should be protected + result = []; tmp = 1 # commment should be protected for i in range(10): result.append(i + 1) # PERF401 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 ad9b789993c41..1d3c79fd109a5 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,11 +6,11 @@ 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_parser::{parse, Mode, TokenKind}; use ruff_python_semantic::{analyze::typing::is_list, Binding}; -use ruff_python_trivia::PythonWhitespace; +use ruff_python_trivia::{is_python_whitespace, PythonWhitespace}; use ruff_source_file::LineRanges; -use ruff_text_size::{Ranged, TextRange}; - +use ruff_text_size::{Ranged, TextRange, TextSize}; /// ## What it does /// Checks for `for` loops that can be replaced by a list comprehension. /// @@ -346,6 +346,7 @@ fn convert_to_list_extend( to_append: &Expr, checker: &Checker, ) -> Result { + let semantic = checker.semantic(); let locator = checker.locator(); let if_str = match if_test { Some(test) => format!(" if {}", locator.slice(test.range())), @@ -397,10 +398,11 @@ fn convert_to_list_extend( let for_loop_inline_comments: Vec<&str> = comment_strings_in_range(for_stmt.range); let newline = checker.stylist().line_ending().as_str(); - let indent_range = TextRange::new( + + let indent = locator.slice(TextRange::new( locator.line_start(for_stmt.range.start()), for_stmt.range.start(), - ); + )); match fix_type { ComprehensionType::Extend => { @@ -416,7 +418,7 @@ fn convert_to_list_extend( let indentation = if for_loop_inline_comments.is_empty() { String::new() } else { - format!("{newline}{}", locator.slice(indent_range)) + format!("{newline}{indent}") }; let text_to_replace = format!( "{}{indentation}{comprehension_body}", @@ -429,32 +431,66 @@ fn convert_to_list_extend( } ComprehensionType::ListComprehension => { let binding_stmt_range = binding - .statement(checker.semantic()) + .statement(semantic) .and_then(|stmt| match stmt { ast::Stmt::AnnAssign(assign) => Some(assign.range), ast::Stmt::Assign(assign) => Some(assign.range), _ => None, }) - .map(|range| locator.full_lines_range(range)) + // .map(|range| locator.full_lines_range(range)) .ok_or(anyhow!( "Binding must have a statement to convert into a list comprehension" ))?; + let binding_is_multiple_exprs = { + let binding_text = locator + .slice(locator.full_lines_range(binding_stmt_range)) + .trim_whitespace_start(); + let tokens = parse(binding_text, Mode::Module).map_err(|err| { + anyhow!( + "Failed to tokenize binding statement: {err} {}", + binding_text + ) + })?; + tokens + .tokens() + .iter() + .any(|token| matches!(token.kind(), TokenKind::Semi)) + }; + // If there are multiple binding statements in one line, we don't want to accidentally delete them + // Instead, we just delete the binding statement and leave any comments where they are + let binding_stmt_range = if binding_is_multiple_exprs { + let end_of_line = locator.slice(TextRange::new( + binding_stmt_range.end(), + locator.full_line_end(binding_stmt_range.end()), + )); + let first_safe: u32 = end_of_line + .chars() + .position(|c| !is_python_whitespace(c) && c != ';') + .expect("end of line must always include a newline, since for loop is after binding") + .try_into() + .expect("semicolon/whitespace should be at most a few characters"); + binding_stmt_range.add_end(TextSize::new(first_safe)) + } else { + locator.full_lines_range(binding_stmt_range) + }; let annotations = match binding - .statement(checker.semantic()) + .statement(semantic) .and_then(|stmt| stmt.as_ann_assign_stmt()) { Some(assign) => format!(": {}", locator.slice(assign.annotation.range())), None => String::new(), }; - let mut comments_to_move = comment_strings_in_range(binding_stmt_range); - comments_to_move.extend(for_loop_inline_comments); + let mut comments_to_move = for_loop_inline_comments; + if !binding_is_multiple_exprs { + comments_to_move.extend(comment_strings_in_range(binding_stmt_range)); + } let indentation = if comments_to_move.is_empty() { String::new() } else { - format!("{newline}{}", locator.slice(indent_range)) + format!("{newline}{indent}") }; let leading_comments = format!("{}{indentation}", comments_to_move.join(&indentation)); 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 eda59a513172a..6c8a89e5a6049 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 @@ -91,83 +91,83 @@ PERF401.py:142:9: PERF401 Use a list comprehension to create a transformed list | = help: Replace for loop with list comprehension -PERF401.py:150:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:149:9: PERF401 Use a list comprehension to create a transformed list | -148 | result = [] # commment should be protected -149 | for i in range(10): -150 | result.append(i + 1) # PERF401 +147 | tmp = 1; result = [] # commment should be protected +148 | for i in range(10): +149 | result.append(i + 1) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:158:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:156:9: PERF401 Use a list comprehension to create a transformed list | -156 | tmp = 1 # commment should be protected -157 | for i in range(10): -158 | result.append(i + 1) # PERF401 +154 | result = []; tmp = 1 # commment should be protected +155 | for i in range(10): +156 | result.append(i + 1) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:164:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:162:9: PERF401 Use a list comprehension to create a transformed list | -162 | result = [] # comment should be protected -163 | for i in range(10): -164 | result.append(i * 2) # PERF401 +160 | result = [] # comment should be protected +161 | for i in range(10): +162 | result.append(i * 2) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:171:9: PERF401 Use `list.extend` to create a transformed list +PERF401.py:169:9: PERF401 Use `list.extend` to create a transformed list | -169 | result.append(1) -170 | for i in range(10): -171 | result.append(i * 2) # PERF401 +167 | result.append(1) +168 | for i in range(10): +169 | result.append(i * 2) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list.extend -PERF401.py:184:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:182:9: PERF401 Use a list comprehension to create a transformed list | -182 | result = [] -183 | for val in range(5): -184 | result.append(val * 2) # Ok +180 | result = [] +181 | for val in range(5): +182 | result.append(val * 2) # Ok | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 -185 | print(val) +183 | print(val) | = help: Replace for loop with list comprehension -PERF401.py:197:13: PERF401 Use a list comprehension to create a transformed list +PERF401.py:195:13: PERF401 Use a list comprehension to create a transformed list | -195 | ): # Comment 3 -196 | if i % 2: # Comment 4 -197 | result.append( +193 | ): # Comment 3 +194 | if i % 2: # Comment 4 +195 | result.append( | _____________^ -198 | | ( -199 | | i + 1, -200 | | # Comment 5 -201 | | 2, -202 | | ) -203 | | ) # PERF401 +196 | | ( +197 | | i + 1, +198 | | # Comment 5 +199 | | 2, +200 | | ) +201 | | ) # PERF401 | |_____________^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:209:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:207:9: PERF401 Use a list comprehension to create a transformed list | -207 | result: list[int] = [] -208 | for i in range(10): -209 | result.append(i * 2) # PERF401 +205 | result: list[int] = [] +206 | for i in range(10): +207 | result.append(i * 2) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:216:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:214:9: PERF401 Use a list comprehension to create a transformed list | -214 | result = [] -215 | for i in a, b: -216 | result.append(i[0] + i[1]) # PERF401 +212 | result = [] +213 | for i in a, b: +214 | result.append(i[0] + i[1]) # PERF401 | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF401 -217 | return result +215 | return result | = help: Replace for loop with list comprehension 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 0641bc52c996c..d81d3f439ed27 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 @@ -169,10 +169,10 @@ PERF401.py:119:13: PERF401 [*] Use a list comprehension to create a transformed 117 |- # single-line comment 2 should be protected 118 |- if i % 2: # single-line comment 3 should be protected 119 |- result.append(i) # PERF401 - 115 |+ # comment after assignment should be protected - 116 |+ # single-line comment 1 should be protected - 117 |+ # single-line comment 2 should be protected - 118 |+ # single-line comment 3 should be protected + 115 |+ # single-line comment 1 should be protected + 116 |+ # single-line comment 2 should be protected + 117 |+ # single-line comment 3 should be protected + 118 |+ # comment after assignment should be protected 119 |+ result = [i for i in range(10) if i % 2] # PERF401 120 120 | 121 121 | @@ -221,198 +221,198 @@ PERF401.py:142:9: PERF401 [*] Use a list comprehension to create a transformed l 144 142 | 145 143 | def f(): -PERF401.py:150:9: PERF401 [*] Use a list comprehension to create a transformed list +PERF401.py:149:9: PERF401 [*] Use a list comprehension to create a transformed list | -148 | result = [] # commment should be protected -149 | for i in range(10): -150 | result.append(i + 1) # PERF401 +147 | tmp = 1; result = [] # commment should be protected +148 | for i in range(10): +149 | result.append(i + 1) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension ℹ Unsafe fix +144 144 | 145 145 | def f(): 146 146 | # make sure that `tmp` is not deleted -147 147 | tmp = 1 -148 |- result = [] # commment should be protected -149 |- for i in range(10): -150 |- result.append(i + 1) # PERF401 - 148 |+ # commment should be protected - 149 |+ result = [i + 1 for i in range(10)] # PERF401 +147 |- tmp = 1; result = [] # commment should be protected +148 |- for i in range(10): +149 |- result.append(i + 1) # PERF401 + 147 |+ tmp = 1; # commment should be protected + 148 |+ result = [i + 1 for i in range(10)] # PERF401 +150 149 | 151 150 | -152 151 | -153 152 | def f(): +152 151 | def f(): -PERF401.py:158:9: PERF401 [*] Use a list comprehension to create a transformed list +PERF401.py:156:9: PERF401 [*] Use a list comprehension to create a transformed list | -156 | tmp = 1 # commment should be protected -157 | for i in range(10): -158 | result.append(i + 1) # PERF401 +154 | result = []; tmp = 1 # commment should be protected +155 | for i in range(10): +156 | result.append(i + 1) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension ℹ Unsafe fix -152 152 | -153 153 | def f(): -154 154 | # make sure that `tmp` is not deleted -155 |- result = [] -156 155 | tmp = 1 # commment should be protected -157 |- for i in range(10): -158 |- result.append(i + 1) # PERF401 - 156 |+ result = [i + 1 for i in range(10)] # PERF401 -159 157 | -160 158 | -161 159 | def f(): - -PERF401.py:164:9: PERF401 [*] Use a list comprehension to create a transformed list +151 151 | +152 152 | def f(): +153 153 | # make sure that `tmp` is not deleted +154 |- result = []; tmp = 1 # commment should be protected +155 |- for i in range(10): +156 |- result.append(i + 1) # PERF401 + 154 |+ tmp = 1 # commment should be protected + 155 |+ result = [i + 1 for i in range(10)] # PERF401 +157 156 | +158 157 | +159 158 | def f(): + +PERF401.py:162:9: PERF401 [*] Use a list comprehension to create a transformed list | -162 | result = [] # comment should be protected -163 | for i in range(10): -164 | result.append(i * 2) # PERF401 +160 | result = [] # comment should be protected +161 | for i in range(10): +162 | result.append(i * 2) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension ℹ Unsafe fix -159 159 | -160 160 | -161 161 | def f(): -162 |- result = [] # comment should be protected -163 |- for i in range(10): -164 |- result.append(i * 2) # PERF401 - 162 |+ # comment should be protected - 163 |+ result = [i * 2 for i in range(10)] # PERF401 -165 164 | -166 165 | -167 166 | def f(): - -PERF401.py:171:9: PERF401 [*] Use `list.extend` to create a transformed list +157 157 | +158 158 | +159 159 | def f(): +160 |- result = [] # comment should be protected +161 |- for i in range(10): +162 |- result.append(i * 2) # PERF401 + 160 |+ # comment should be protected + 161 |+ result = [i * 2 for i in range(10)] # PERF401 +163 162 | +164 163 | +165 164 | def f(): + +PERF401.py:169:9: PERF401 [*] Use `list.extend` to create a transformed list | -169 | result.append(1) -170 | for i in range(10): -171 | result.append(i * 2) # PERF401 +167 | result.append(1) +168 | for i in range(10): +169 | result.append(i * 2) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list.extend ℹ Unsafe fix -167 167 | def f(): -168 168 | result = [] -169 169 | result.append(1) -170 |- for i in range(10): -171 |- result.append(i * 2) # PERF401 - 170 |+ result.extend(i * 2 for i in range(10)) # PERF401 -172 171 | -173 172 | -174 173 | def f(): - -PERF401.py:184:9: PERF401 [*] Use a list comprehension to create a transformed list +165 165 | def f(): +166 166 | result = [] +167 167 | result.append(1) +168 |- for i in range(10): +169 |- result.append(i * 2) # PERF401 + 168 |+ result.extend(i * 2 for i in range(10)) # PERF401 +170 169 | +171 170 | +172 171 | def f(): + +PERF401.py:182:9: PERF401 [*] Use a list comprehension to create a transformed list | -182 | result = [] -183 | for val in range(5): -184 | result.append(val * 2) # Ok +180 | result = [] +181 | for val in range(5): +182 | result.append(val * 2) # Ok | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 -185 | print(val) +183 | print(val) | = help: Replace for loop with list comprehension ℹ Unsafe fix -179 179 | -180 180 | -181 181 | def f(): -182 |- result = [] -183 |- for val in range(5): -184 |- result.append(val * 2) # Ok - 182 |+ result = [val * 2 for val in range(5)] # Ok -185 183 | print(val) -186 184 | -187 185 | - -PERF401.py:197:13: PERF401 [*] Use a list comprehension to create a transformed list +177 177 | +178 178 | +179 179 | def f(): +180 |- result = [] +181 |- for val in range(5): +182 |- result.append(val * 2) # Ok + 180 |+ result = [val * 2 for val in range(5)] # Ok +183 181 | print(val) +184 182 | +185 183 | + +PERF401.py:195:13: PERF401 [*] Use a list comprehension to create a transformed list | -195 | ): # Comment 3 -196 | if i % 2: # Comment 4 -197 | result.append( +193 | ): # Comment 3 +194 | if i % 2: # Comment 4 +195 | result.append( | _____________^ -198 | | ( -199 | | i + 1, -200 | | # Comment 5 -201 | | 2, -202 | | ) -203 | | ) # PERF401 +196 | | ( +197 | | i + 1, +198 | | # Comment 5 +199 | | 2, +200 | | ) +201 | | ) # PERF401 | |_____________^ PERF401 | = help: Replace for loop with list comprehension ℹ Unsafe fix -186 186 | -187 187 | -188 188 | def f(): -189 |- result = [] -190 |- for i in range( # Comment 1 should not be duplicated - 189 |+ # Comment 3 - 190 |+ # Comment 4 - 191 |+ result = [( - 192 |+ i + 1, - 193 |+ # Comment 5 - 194 |+ 2, - 195 |+ ) for i in range( # Comment 1 should not be duplicated -191 196 | ( -192 197 | 2 # Comment 2 -193 198 | + 1 -194 199 | ) -195 |- ): # Comment 3 -196 |- if i % 2: # Comment 4 -197 |- result.append( -198 |- ( -199 |- i + 1, -200 |- # Comment 5 -201 |- 2, -202 |- ) -203 |- ) # PERF401 - 200 |+ ) if i % 2] # PERF401 -204 201 | -205 202 | -206 203 | def f(): - -PERF401.py:209:9: PERF401 [*] Use a list comprehension to create a transformed list +184 184 | +185 185 | +186 186 | def f(): +187 |- result = [] +188 |- for i in range( # Comment 1 should not be duplicated + 187 |+ # Comment 3 + 188 |+ # Comment 4 + 189 |+ result = [( + 190 |+ i + 1, + 191 |+ # Comment 5 + 192 |+ 2, + 193 |+ ) for i in range( # Comment 1 should not be duplicated +189 194 | ( +190 195 | 2 # Comment 2 +191 196 | + 1 +192 197 | ) +193 |- ): # Comment 3 +194 |- if i % 2: # Comment 4 +195 |- result.append( +196 |- ( +197 |- i + 1, +198 |- # Comment 5 +199 |- 2, +200 |- ) +201 |- ) # PERF401 + 198 |+ ) if i % 2] # PERF401 +202 199 | +203 200 | +204 201 | def f(): + +PERF401.py:207:9: PERF401 [*] Use a list comprehension to create a transformed list | -207 | result: list[int] = [] -208 | for i in range(10): -209 | result.append(i * 2) # PERF401 +205 | result: list[int] = [] +206 | for i in range(10): +207 | result.append(i * 2) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension ℹ Unsafe fix -204 204 | -205 205 | -206 206 | def f(): -207 |- result: list[int] = [] -208 |- for i in range(10): -209 |- result.append(i * 2) # PERF401 - 207 |+ result: list[int] = [i * 2 for i in range(10)] # PERF401 -210 208 | -211 209 | -212 210 | def f(): - -PERF401.py:216:9: PERF401 [*] Use a list comprehension to create a transformed list +202 202 | +203 203 | +204 204 | def f(): +205 |- result: list[int] = [] +206 |- for i in range(10): +207 |- result.append(i * 2) # PERF401 + 205 |+ result: list[int] = [i * 2 for i in range(10)] # PERF401 +208 206 | +209 207 | +210 208 | def f(): + +PERF401.py:214:9: PERF401 [*] Use a list comprehension to create a transformed list | -214 | result = [] -215 | for i in a, b: -216 | result.append(i[0] + i[1]) # PERF401 +212 | result = [] +213 | for i in a, b: +214 | result.append(i[0] + i[1]) # PERF401 | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF401 -217 | return result +215 | return result | = help: Replace for loop with list comprehension ℹ Unsafe fix -211 211 | -212 212 | def f(): -213 213 | a, b = [1, 2, 3], [4, 5, 6] -214 |- result = [] -215 |- for i in a, b: -216 |- result.append(i[0] + i[1]) # PERF401 - 214 |+ result = [i[0] + i[1] for i in (a, b)] # PERF401 -217 215 | return result +209 209 | +210 210 | def f(): +211 211 | a, b = [1, 2, 3], [4, 5, 6] +212 |- result = [] +213 |- for i in a, b: +214 |- result.append(i[0] + i[1]) # PERF401 + 212 |+ result = [i[0] + i[1] for i in (a, b)] # PERF401 +215 213 | return result From f6ccb39b173a385621d100056d967efa298827df Mon Sep 17 00:00:00 2001 From: Sebastian Date: Fri, 6 Dec 2024 10:33:07 -0500 Subject: [PATCH 10/21] move manual list comprehension to deferred check --- .../ast/analyze/deferred_for_loops.rs | 4 +++- .../src/checkers/ast/analyze/statement.rs | 4 +--- ...__perflint__tests__PERF401_PERF401.py.snap | 10 --------- ...t__tests__preview__PERF401_PERF401.py.snap | 22 ------------------- 4 files changed, 4 insertions(+), 36 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs index 35fe8168e0eaa..7bc5cf097f9e3 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs @@ -14,7 +14,6 @@ pub(crate) fn deferred_for_loops(checker: &mut Checker) { let Stmt::For(stmt_for) = checker.semantic.current_statement() else { unreachable!("Expected Stmt::For"); }; - if checker.enabled(Rule::UnusedLoopControlVariable) { flake8_bugbear::rules::unused_loop_control_variable(checker, stmt_for); } @@ -36,6 +35,9 @@ pub(crate) fn deferred_for_loops(checker: &mut Checker) { if checker.enabled(Rule::DictIndexMissingItems) { pylint::rules::dict_index_missing_items(checker, stmt_for); } + if checker.enabled(Rule::ManualListComprehension) { + perflint::rules::manual_list_comprehension(checker, stmt_for); + } } } } diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 9bdc28a1c5863..c70846471a692 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1379,6 +1379,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { Rule::UnnecessaryEnumerate, Rule::UnusedLoopControlVariable, Rule::YieldInForLoop, + Rule::ManualListComprehension, ]) { checker.analyze.for_loops.push(checker.semantic.snapshot()); } @@ -1403,9 +1404,6 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::DictIterMissingItems) { pylint::rules::dict_iter_missing_items(checker, target, iter); } - if checker.enabled(Rule::ManualListComprehension) { - perflint::rules::manual_list_comprehension(checker, for_stmt); - } if checker.enabled(Rule::ManualListCopy) { perflint::rules::manual_list_copy(checker, for_stmt); } 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 6c8a89e5a6049..c8a2141241978 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 @@ -127,16 +127,6 @@ PERF401.py:169:9: PERF401 Use `list.extend` to create a transformed list | = help: Replace for loop with list.extend -PERF401.py:182:9: PERF401 Use a list comprehension to create a transformed list - | -180 | result = [] -181 | for val in range(5): -182 | result.append(val * 2) # Ok - | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 -183 | print(val) - | - = help: Replace for loop with list comprehension - PERF401.py:195:13: PERF401 Use a list comprehension to create a transformed list | 193 | ): # Comment 3 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 d81d3f439ed27..948e4d844d1ab 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 @@ -307,28 +307,6 @@ PERF401.py:169:9: PERF401 [*] Use `list.extend` to create a transformed list 171 170 | 172 171 | def f(): -PERF401.py:182:9: PERF401 [*] Use a list comprehension to create a transformed list - | -180 | result = [] -181 | for val in range(5): -182 | result.append(val * 2) # Ok - | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 -183 | print(val) - | - = help: Replace for loop with list comprehension - -ℹ Unsafe fix -177 177 | -178 178 | -179 179 | def f(): -180 |- result = [] -181 |- for val in range(5): -182 |- result.append(val * 2) # Ok - 180 |+ result = [val * 2 for val in range(5)] # Ok -183 181 | print(val) -184 182 | -185 183 | - PERF401.py:195:13: PERF401 [*] Use a list comprehension to create a transformed list | 193 | ): # Comment 3 From 6a98542b5e996a3d60d6a9a2c2c8321fc2ebda70 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Fri, 6 Dec 2024 12:14:06 -0500 Subject: [PATCH 11/21] add debugging code to show binding --- .../rules/manual_list_comprehension.rs | 19 +++++++++++++++++-- 1 file changed, 17 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 1d3c79fd109a5..d5fbbf5273d0f 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 @@ -92,9 +92,10 @@ impl Violation for ManualListComprehension { /// PERF401 pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::StmtFor) { - let Expr::Name(ast::ExprName { id, .. }) = &*for_stmt.target else { + let Expr::Name(loop_target_name) = &*for_stmt.target else { return; }; + let id = &loop_target_name.id; let (stmt, if_test) = match &*for_stmt.body { // ```python @@ -234,15 +235,29 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S // ``` let for_loop_target = checker .semantic() - .lookup_symbol(id.as_str()) + .lookup_symbol(id) .map(|id| checker.semantic().binding(id)) .expect("for loop target must exist"); // TODO: this currently does not properly find usages outside the for loop; figure out why if for_loop_target .references() .map(|id| checker.semantic().reference(id)) + .inspect(|reference| { + // println!("{}", checker.locator().slice(reference.range())); + let binding_string = checker + .locator() + .slice(for_loop_target.statement(checker.semantic()).unwrap()); + dbg!(binding_string); + println!( + "{}", + checker + .locator() + .slice(checker.locator().full_lines_range(reference.range())) + ); + }) .any(|reference| !for_stmt.range.contains_range(reference.range())) { + println!("found reference outside of loop"); return; } From e2eec2e04d8c76e14d698e26a0053a98e4c23b41 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 9 Dec 2024 14:39:13 -0500 Subject: [PATCH 12/21] find the right binding for the loop variable --- .../test/fixtures/perflint/PERF401.py | 15 ++ .../rules/manual_list_comprehension.rs | 62 +++--- ...__perflint__tests__PERF401_PERF401.py.snap | 58 ++++-- ...t__tests__preview__PERF401_PERF401.py.snap | 176 +++++++++++------- 4 files changed, 197 insertions(+), 114 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index aef5bd63ecbc3..6b4e6822f167f 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -183,6 +183,21 @@ def f(): print(val) +def f(): + result = [] + for val in range(5): + result.append(val * 2) # PERF401 + val = 1 + print(val) + + +def f(): + i = [1, 2, 3] + result = [] + for i in i: + result.append(i + 1) # PERF401 + + def f(): result = [] for i in range( # Comment 1 should not be duplicated 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 d5fbbf5273d0f..7e49b49160a51 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 @@ -92,10 +92,10 @@ impl Violation for ManualListComprehension { /// PERF401 pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::StmtFor) { - let Expr::Name(loop_target_name) = &*for_stmt.target else { + let Expr::Name(ast::ExprName { id, .. }) = &*for_stmt.target else { return; }; - let id = &loop_target_name.id; + let for_stmt_target_id = id; let (stmt, if_test) = match &*for_stmt.body { // ```python @@ -233,31 +233,40 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S // filtered = [x for x in y] // print(x) // ``` - let for_loop_target = checker + let last_target_binding = checker .semantic() - .lookup_symbol(id) - .map(|id| checker.semantic().binding(id)) + .lookup_symbol(for_stmt_target_id) .expect("for loop target must exist"); - // TODO: this currently does not properly find usages outside the for loop; figure out why - if for_loop_target - .references() - .map(|id| checker.semantic().reference(id)) - .inspect(|reference| { - // println!("{}", checker.locator().slice(reference.range())); - let binding_string = checker - .locator() - .slice(for_loop_target.statement(checker.semantic()).unwrap()); - dbg!(binding_string); - println!( - "{}", - checker - .locator() - .slice(checker.locator().full_lines_range(reference.range())) - ); + + let mut bindings = [last_target_binding].into_iter().chain( + checker + .semantic() + .shadowed_bindings(checker.semantic().scope_id, last_target_binding) + .filter_map(|shadowed| shadowed.same_scope().then_some(shadowed.shadowed_id())), + ); + + let target_binding = bindings + .find_map(|binding_id| { + let binding = checker.semantic().binding(binding_id); + if binding + .statement(checker.semantic()) + .and_then(Stmt::as_for_stmt) + == Some(for_stmt) + { + Some(binding) + } else { + None + } }) - .any(|reference| !for_stmt.range.contains_range(reference.range())) + .expect("for target binding must exist"); + + drop(bindings); + + if target_binding + .references() + .map(|reference| checker.semantic().reference(reference)) + .any(|r| !for_stmt.range.contains_range(r.range())) { - println!("found reference outside of loop"); return; } @@ -312,15 +321,10 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S count < 2 }); - // If the binding has multiple statements on its line, it gets more complicated to fix, so we use an extend - // TODO: make this work - let binding_only_stmt_on_line = binding_stmt.is_some_and(|_binding_stmt| true); - // A list extend works in every context, while a list comprehension only works when all the criteria are true let comprehension_type = if binding_is_empty_list && assignment_in_same_statement && binding_has_one_target - && binding_only_stmt_on_line && binding_unused_between { ComprehensionType::ListComprehension @@ -452,10 +456,10 @@ fn convert_to_list_extend( ast::Stmt::Assign(assign) => Some(assign.range), _ => None, }) - // .map(|range| locator.full_lines_range(range)) .ok_or(anyhow!( "Binding must have a statement to convert into a list comprehension" ))?; + // If the binding has multiple statements on its line, only remove the list assignment let binding_is_multiple_exprs = { let binding_text = locator .slice(locator.full_lines_range(binding_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 c8a2141241978..5317deebcccc4 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 @@ -127,37 +127,57 @@ PERF401.py:169:9: PERF401 Use `list.extend` to create a transformed list | = help: Replace for loop with list.extend -PERF401.py:195:13: PERF401 Use a list comprehension to create a transformed list +PERF401.py:189:9: PERF401 Use a list comprehension to create a transformed list | -193 | ): # Comment 3 -194 | if i % 2: # Comment 4 -195 | result.append( +187 | result = [] +188 | for val in range(5): +189 | result.append(val * 2) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 +190 | val = 1 +191 | print(val) + | + = help: Replace for loop with list comprehension + +PERF401.py:198:9: PERF401 Use a list comprehension to create a transformed list + | +196 | result = [] +197 | for i in i: +198 | result.append(i + 1) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +PERF401.py:210:13: PERF401 Use a list comprehension to create a transformed list + | +208 | ): # Comment 3 +209 | if i % 2: # Comment 4 +210 | result.append( | _____________^ -196 | | ( -197 | | i + 1, -198 | | # Comment 5 -199 | | 2, -200 | | ) -201 | | ) # PERF401 +211 | | ( +212 | | i + 1, +213 | | # Comment 5 +214 | | 2, +215 | | ) +216 | | ) # PERF401 | |_____________^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:207:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:222:9: PERF401 Use a list comprehension to create a transformed list | -205 | result: list[int] = [] -206 | for i in range(10): -207 | result.append(i * 2) # PERF401 +220 | result: list[int] = [] +221 | for i in range(10): +222 | result.append(i * 2) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension -PERF401.py:214:9: PERF401 Use a list comprehension to create a transformed list +PERF401.py:229:9: PERF401 Use a list comprehension to create a transformed list | -212 | result = [] -213 | for i in a, b: -214 | result.append(i[0] + i[1]) # PERF401 +227 | result = [] +228 | for i in a, b: +229 | result.append(i[0] + i[1]) # PERF401 | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF401 -215 | return result +230 | return result | = help: Replace for loop with list comprehension 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 948e4d844d1ab..03251a151bb8c 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 @@ -307,19 +307,14 @@ PERF401.py:169:9: PERF401 [*] Use `list.extend` to create a transformed list 171 170 | 172 171 | def f(): -PERF401.py:195:13: PERF401 [*] Use a list comprehension to create a transformed list +PERF401.py:189:9: PERF401 [*] Use a list comprehension to create a transformed list | -193 | ): # Comment 3 -194 | if i % 2: # Comment 4 -195 | result.append( - | _____________^ -196 | | ( -197 | | i + 1, -198 | | # Comment 5 -199 | | 2, -200 | | ) -201 | | ) # PERF401 - | |_____________^ PERF401 +187 | result = [] +188 | for val in range(5): +189 | result.append(val * 2) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^^^ PERF401 +190 | val = 1 +191 | print(val) | = help: Replace for loop with list comprehension @@ -328,69 +323,118 @@ PERF401.py:195:13: PERF401 [*] Use a list comprehension to create a transformed 185 185 | 186 186 | def f(): 187 |- result = [] -188 |- for i in range( # Comment 1 should not be duplicated - 187 |+ # Comment 3 - 188 |+ # Comment 4 - 189 |+ result = [( - 190 |+ i + 1, - 191 |+ # Comment 5 - 192 |+ 2, - 193 |+ ) for i in range( # Comment 1 should not be duplicated -189 194 | ( -190 195 | 2 # Comment 2 -191 196 | + 1 -192 197 | ) -193 |- ): # Comment 3 -194 |- if i % 2: # Comment 4 -195 |- result.append( -196 |- ( -197 |- i + 1, -198 |- # Comment 5 -199 |- 2, -200 |- ) -201 |- ) # PERF401 - 198 |+ ) if i % 2] # PERF401 -202 199 | -203 200 | -204 201 | def f(): - -PERF401.py:207:9: PERF401 [*] Use a list comprehension to create a transformed list +188 |- for val in range(5): +189 |- result.append(val * 2) # PERF401 + 187 |+ result = [val * 2 for val in range(5)] # PERF401 +190 188 | val = 1 +191 189 | print(val) +192 190 | + +PERF401.py:198:9: PERF401 [*] Use a list comprehension to create a transformed list + | +196 | result = [] +197 | for i in i: +198 | result.append(i + 1) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +193 193 | +194 194 | def f(): +195 195 | i = [1, 2, 3] +196 |- result = [] +197 |- for i in i: +198 |- result.append(i + 1) # PERF401 + 196 |+ result = [i + 1 for i in i] # PERF401 +199 197 | +200 198 | +201 199 | def f(): + +PERF401.py:210:13: PERF401 [*] Use a list comprehension to create a transformed list + | +208 | ): # Comment 3 +209 | if i % 2: # Comment 4 +210 | result.append( + | _____________^ +211 | | ( +212 | | i + 1, +213 | | # Comment 5 +214 | | 2, +215 | | ) +216 | | ) # PERF401 + | |_____________^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +199 199 | +200 200 | +201 201 | def f(): +202 |- result = [] +203 |- for i in range( # Comment 1 should not be duplicated + 202 |+ # Comment 3 + 203 |+ # Comment 4 + 204 |+ result = [( + 205 |+ i + 1, + 206 |+ # Comment 5 + 207 |+ 2, + 208 |+ ) for i in range( # Comment 1 should not be duplicated +204 209 | ( +205 210 | 2 # Comment 2 +206 211 | + 1 +207 212 | ) +208 |- ): # Comment 3 +209 |- if i % 2: # Comment 4 +210 |- result.append( +211 |- ( +212 |- i + 1, +213 |- # Comment 5 +214 |- 2, +215 |- ) +216 |- ) # PERF401 + 213 |+ ) if i % 2] # PERF401 +217 214 | +218 215 | +219 216 | def f(): + +PERF401.py:222:9: PERF401 [*] Use a list comprehension to create a transformed list | -205 | result: list[int] = [] -206 | for i in range(10): -207 | result.append(i * 2) # PERF401 +220 | result: list[int] = [] +221 | for i in range(10): +222 | result.append(i * 2) # PERF401 | ^^^^^^^^^^^^^^^^^^^^ PERF401 | = help: Replace for loop with list comprehension ℹ Unsafe fix -202 202 | -203 203 | -204 204 | def f(): -205 |- result: list[int] = [] -206 |- for i in range(10): -207 |- result.append(i * 2) # PERF401 - 205 |+ result: list[int] = [i * 2 for i in range(10)] # PERF401 -208 206 | -209 207 | -210 208 | def f(): - -PERF401.py:214:9: PERF401 [*] Use a list comprehension to create a transformed list +217 217 | +218 218 | +219 219 | def f(): +220 |- result: list[int] = [] +221 |- for i in range(10): +222 |- result.append(i * 2) # PERF401 + 220 |+ result: list[int] = [i * 2 for i in range(10)] # PERF401 +223 221 | +224 222 | +225 223 | def f(): + +PERF401.py:229:9: PERF401 [*] Use a list comprehension to create a transformed list | -212 | result = [] -213 | for i in a, b: -214 | result.append(i[0] + i[1]) # PERF401 +227 | result = [] +228 | for i in a, b: +229 | result.append(i[0] + i[1]) # PERF401 | ^^^^^^^^^^^^^^^^^^^^^^^^^^ PERF401 -215 | return result +230 | return result | = help: Replace for loop with list comprehension ℹ Unsafe fix -209 209 | -210 210 | def f(): -211 211 | a, b = [1, 2, 3], [4, 5, 6] -212 |- result = [] -213 |- for i in a, b: -214 |- result.append(i[0] + i[1]) # PERF401 - 212 |+ result = [i[0] + i[1] for i in (a, b)] # PERF401 -215 213 | return result +224 224 | +225 225 | def f(): +226 226 | a, b = [1, 2, 3], [4, 5, 6] +227 |- result = [] +228 |- for i in a, b: +229 |- result.append(i[0] + i[1]) # PERF401 + 227 |+ result = [i[0] + i[1] for i in (a, b)] # PERF401 +230 228 | return result From 9d41fb7feeb07be1285306b63663e42009b72eed Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 9 Dec 2024 14:47:22 -0500 Subject: [PATCH 13/21] check that the correct for loop is picked --- .../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 7e49b49160a51..4fc582c1e254f 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 @@ -251,7 +251,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S if binding .statement(checker.semantic()) .and_then(Stmt::as_for_stmt) - == Some(for_stmt) + .is_some_and(|stmt| stmt.range == for_stmt.range) { Some(binding) } else { From 104b69e3dd2d73897d0a11461b2ef5c105548834 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 9 Dec 2024 16:08:15 -0500 Subject: [PATCH 14/21] don't stop fix if a reference to target is actually another binding --- .../test/fixtures/perflint/PERF401.py | 9 +++++++ .../rules/manual_list_comprehension.rs | 25 ++++++++++++------- ...__perflint__tests__PERF401_PERF401.py.snap | 9 +++++++ ...t__tests__preview__PERF401_PERF401.py.snap | 22 ++++++++++++++++ 4 files changed, 56 insertions(+), 9 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py index 6b4e6822f167f..553742fe26c0c 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF401.py @@ -228,3 +228,12 @@ def f(): for i in a, b: result.append(i[0] + i[1]) # PERF401 return result + + +def f(): + values = [1, 2, 3] + result = [] + for a in values: + print(a) + for a in values: + result.append(a + 1) # PERF401 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 4fc582c1e254f..d850ffd787850 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 @@ -245,25 +245,32 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S .filter_map(|shadowed| shadowed.same_scope().then_some(shadowed.shadowed_id())), ); - let target_binding = bindings - .find_map(|binding_id| { - let binding = checker.semantic().binding(binding_id); - if binding + let target_binding_id = bindings + .find(|binding_id| { + let binding = checker.semantic().binding(*binding_id); + binding .statement(checker.semantic()) .and_then(Stmt::as_for_stmt) .is_some_and(|stmt| stmt.range == for_stmt.range) - { - Some(binding) - } else { - None - } }) .expect("for target binding must exist"); + let target_binding = checker.semantic().binding(target_binding_id); + // TODO: should this be a HashMap? + let shadowed_references: Vec<_> = checker + .semantic() + .shadowed_bindings(checker.semantic().scope_id, target_binding_id) + .filter_map(|shadowed| shadowed.same_scope().then_some(shadowed.shadowed_id())) + .flat_map(|shadowed_id| { + let shadowed_binding = checker.semantic().binding(shadowed_id); + shadowed_binding.references() + }) + .collect(); drop(bindings); if target_binding .references() + .filter(|r_id| !shadowed_references.contains(r_id)) .map(|reference| checker.semantic().reference(reference)) .any(|r| !for_stmt.range.contains_range(r.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 5317deebcccc4..a443c9f101f92 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 @@ -181,3 +181,12 @@ PERF401.py:229:9: PERF401 Use a list comprehension to create a transformed list 230 | return result | = help: Replace for loop with list comprehension + +PERF401.py:239:9: PERF401 Use a list comprehension to create a transformed list + | +237 | print(a) +238 | for a in values: +239 | result.append(a + 1) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension 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 03251a151bb8c..8f6e30364a4aa 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 @@ -438,3 +438,25 @@ PERF401.py:229:9: PERF401 [*] Use a list comprehension to create a transformed l 229 |- result.append(i[0] + i[1]) # PERF401 227 |+ result = [i[0] + i[1] for i in (a, b)] # PERF401 230 228 | return result +231 229 | +232 230 | + +PERF401.py:239:9: PERF401 [*] Use a list comprehension to create a transformed list + | +237 | print(a) +238 | for a in values: +239 | result.append(a + 1) # PERF401 + | ^^^^^^^^^^^^^^^^^^^^ PERF401 + | + = help: Replace for loop with list comprehension + +ℹ Unsafe fix +232 232 | +233 233 | def f(): +234 234 | values = [1, 2, 3] +235 |- result = [] +236 235 | for a in values: +237 236 | print(a) +238 |- for a in values: +239 |- result.append(a + 1) # PERF401 + 237 |+ result = [a + 1 for a in values] # PERF401 From e44fcaa1f327c6c03cbe8392ac2bafb5ed784d67 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Mon, 9 Dec 2024 16:32:27 -0500 Subject: [PATCH 15/21] allow shadowed bindings from any scope --- .../src/rules/perflint/rules/manual_list_comprehension.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 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 d850ffd787850..9a196c5812bcc 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 @@ -259,9 +259,8 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S let shadowed_references: Vec<_> = checker .semantic() .shadowed_bindings(checker.semantic().scope_id, target_binding_id) - .filter_map(|shadowed| shadowed.same_scope().then_some(shadowed.shadowed_id())) - .flat_map(|shadowed_id| { - let shadowed_binding = checker.semantic().binding(shadowed_id); + .flat_map(|shadowed| { + let shadowed_binding = checker.semantic().binding(shadowed.shadowed_id()); shadowed_binding.references() }) .collect(); From 4696878fcea66b8152f1386715486a2711d63296 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Tue, 10 Dec 2024 14:27:08 -0500 Subject: [PATCH 16/21] simplify check for loop variable usages --- .../perflint/rules/manual_list_comprehension.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 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 9a196c5812bcc..20b1c6591106b 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 @@ -255,23 +255,15 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S }) .expect("for target binding must exist"); let target_binding = checker.semantic().binding(target_binding_id); - // TODO: should this be a HashMap? - let shadowed_references: Vec<_> = checker - .semantic() - .shadowed_bindings(checker.semantic().scope_id, target_binding_id) - .flat_map(|shadowed| { - let shadowed_binding = checker.semantic().binding(shadowed.shadowed_id()); - shadowed_binding.references() - }) - .collect(); drop(bindings); + // If any references to the loop target variable are after the loop, + // then converting it into a comprehension would cause a NameError if target_binding .references() - .filter(|r_id| !shadowed_references.contains(r_id)) .map(|reference| checker.semantic().reference(reference)) - .any(|r| !for_stmt.range.contains_range(r.range())) + .any(|other_reference| for_stmt.end() < other_reference.start()) { return; } From 74afdb07502d357395aa2075091eee9934ed61da Mon Sep 17 00:00:00 2001 From: Sebastian Date: Tue, 10 Dec 2024 14:29:04 -0500 Subject: [PATCH 17/21] cargo fmt --- .../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 20b1c6591106b..3f10084e38b26 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 @@ -258,7 +258,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S drop(bindings); - // If any references to the loop target variable are after the loop, + // If any references to the loop target variable are after the loop, // then converting it into a comprehension would cause a NameError if target_binding .references() From db8d0d6ad216bdf006887bc56de9193457a4ade1 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 11 Dec 2024 13:54:06 +0100 Subject: [PATCH 18/21] Few simplifications --- .../rules/manual_list_comprehension.rs | 135 +++++++++--------- 1 file changed, 65 insertions(+), 70 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 3f10084e38b26..489692776885e 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,9 @@ -use ruff_python_ast::{self as ast, Arguments, Expr, Stmt}; +use ruff_python_ast::{self as ast, Arguments, Expr}; use crate::checkers::ast::Checker; use anyhow::{anyhow, Result}; 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_parser::{parse, Mode, TokenKind}; use ruff_python_semantic::{analyze::typing::is_list, Binding}; @@ -92,10 +91,13 @@ impl Violation for ManualListComprehension { /// PERF401 pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::StmtFor) { - let Expr::Name(ast::ExprName { id, .. }) = &*for_stmt.target else { + let Expr::Name(ast::ExprName { + id: for_stmt_target_id, + .. + }) = &*for_stmt.target + else { return; }; - let for_stmt_target_id = id; let (stmt, if_test) = match &*for_stmt.body { // ```python @@ -103,7 +105,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S // if z: // filtered.append(x) // ``` - [Stmt::If(ast::StmtIf { + [ast::Stmt::If(ast::StmtIf { body, elif_else_clauses, test, @@ -125,7 +127,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S _ => return, }; - let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else { + let ast::Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else { return; }; @@ -151,53 +153,53 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S return; }; - let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() else { + let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = &**func else { return; }; if attr.as_str() != "append" { return; } + + // Avoid non-list values. + let Some(list_name) = value.as_name_expr() else { + 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 { if if_test.is_none() { - if arg.as_name_expr().is_some_and(|arg| arg.id == *id) { + if arg + .as_name_expr() + .is_some_and(|arg| arg.id == *for_stmt_target_id) + { return; } } } - // Avoid, e.g., `for x in y: filtered[x].append(x * x)`. - if any_over_expr(value, &|expr| { - expr.as_name_expr().is_some_and(|expr| expr.id == *id) - }) { - return; - } - // Avoid, e.g., `for x in y: filtered.append(filtered[-1] * 2)`. if any_over_expr(arg, &|expr| { - ComparableExpr::from(expr) == ComparableExpr::from(value) + expr.as_name_expr() + .is_some_and(|expr| expr.id == list_name.id) }) { return; } - // Avoid non-list values. - let Some(name) = value.as_name_expr() else { - return; - }; - let Some(binding) = checker + let Some(list_binding) = checker .semantic() - .only_binding(name) + .only_binding(list_name) .map(|id| checker.semantic().binding(id)) else { return; }; - if !is_list(binding, checker.semantic()) { + + if !is_list(list_binding, checker.semantic()) { return; } - // Avoid if the value is used in the conditional test, e.g., + // Avoid if the list is used in the conditional test, e.g., // // ```python // for x in y: @@ -213,13 +215,14 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S // ``` if if_test.is_some_and(|test| { any_over_expr(test, &|expr| { - expr.as_name_expr().is_some_and(|expr| expr.id == name.id) + expr.as_name_expr() + .is_some_and(|expr| expr.id == list_name.id) }) }) { return; } - // Avoid if the for-loop target is used outside of the for loop, e.g., + // Avoid if the for-loop target is used outside the for loop, e.g., // // ```python // for x in y: @@ -238,25 +241,25 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S .lookup_symbol(for_stmt_target_id) .expect("for loop target must exist"); - let mut bindings = [last_target_binding].into_iter().chain( - checker - .semantic() - .shadowed_bindings(checker.semantic().scope_id, last_target_binding) - .filter_map(|shadowed| shadowed.same_scope().then_some(shadowed.shadowed_id())), - ); - - let target_binding_id = bindings - .find(|binding_id| { - let binding = checker.semantic().binding(*binding_id); - binding - .statement(checker.semantic()) - .and_then(Stmt::as_for_stmt) - .is_some_and(|stmt| stmt.range == for_stmt.range) - }) - .expect("for target binding must exist"); - let target_binding = checker.semantic().binding(target_binding_id); - - drop(bindings); + let target_binding = { + let mut bindings = [last_target_binding].into_iter().chain( + checker + .semantic() + .shadowed_bindings(checker.semantic().scope_id, last_target_binding) + .filter_map(|shadowed| shadowed.same_scope().then_some(shadowed.shadowed_id())), + ); + + bindings + .find_map(|binding_id| { + let binding = checker.semantic().binding(binding_id); + binding + .statement(checker.semantic()) + .and_then(ast::Stmt::as_for_stmt) + .is_some_and(|stmt| stmt.range == for_stmt.range) + .then_some(binding) + }) + .expect("for target binding must exist") + }; // If any references to the loop target variable are after the loop, // then converting it into a comprehension would cause a NameError @@ -268,24 +271,24 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S return; } - let binding_stmt = binding.statement(checker.semantic()); - let binding_value = binding_stmt.and_then(|binding_stmt| match binding_stmt { - ast::Stmt::AnnAssign(assign) => assign.value.as_ref(), + let list_binding_stmt = list_binding.statement(checker.semantic()); + let list_binding_value = list_binding_stmt.and_then(|binding_stmt| match binding_stmt { + ast::Stmt::AnnAssign(assign) => assign.value.as_deref(), ast::Stmt::Assign(assign) => Some(&assign.value), _ => None, }); // 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 = - binding_value.is_some_and(|binding_value| match binding_value.as_list_expr() { + list_binding_value.is_some_and(|binding_value| match binding_value.as_list_expr() { Some(list_expr) => list_expr.elts.is_empty(), None => false, }); // 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. + // deleted and replaced with a list comprehension. This does not apply when using `extend`. let assignment_in_same_statement = { - binding.source.is_some_and(|binding_source| { + list_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 @@ -294,29 +297,21 @@ 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_target = binding_stmt.and_then(|binding_stmt| match binding_stmt { - ast::Stmt::AnnAssign(assign) => Some(std::slice::from_ref(assign.target.as_ref())), - ast::Stmt::Assign(assign) => Some(assign.targets.as_slice()), - _ => None, + let binding_has_one_target = list_binding_stmt.is_some_and(|binding_stmt| match binding_stmt { + ast::Stmt::AnnAssign(_) => true, + ast::Stmt::Assign(assign) => assign.targets.len() == 1, + _ => false, }); - let binding_has_one_target = { - match binding_target { - Some([only_target]) => only_target.is_name_expr(), - _ => false, - } - }; // If the binding gets used in between the assignment and the for loop, a list comprehension is no longer safe - let binding_unused_between = binding_stmt.is_some_and(|binding_stmt| { - let from_assign_to_loop = binding_stmt.range().cover(for_stmt.range); - // count the number of references between the assignment and the for loop - let count = binding + let binding_unused_between = list_binding_stmt.is_some_and(|binding_stmt| { + let from_assign_to_loop = TextRange::new(binding_stmt.end(), for_stmt.start()); + // Test if there's any reference to the list symbol between its definition and the for loop. + // if there's at least one, then it's been accessed in the middle somewhere, so it's not safe to change into a list comprehension + !list_binding .references() .map(|ref_id| checker.semantic().reference(ref_id).range()) - .filter(|text_range| from_assign_to_loop.contains_range(*text_range)) - .count(); - // if there's more than one, then it's been accessed in the middle somewhere, so it's not safe to change into a list comprehension - count < 2 + .any(|text_range| from_assign_to_loop.contains_range(text_range)) }); // A list extend works in every context, while a list comprehension only works when all the criteria are true @@ -343,7 +338,7 @@ pub(crate) fn manual_list_comprehension(checker: &mut Checker, for_stmt: &ast::S diagnostic.try_set_fix(|| { convert_to_list_extend( comprehension_type, - binding, + list_binding, for_stmt, if_test.map(std::convert::AsRef::as_ref), arg, From c9b394c2af4a55b6161abeaf2b403c12323447af Mon Sep 17 00:00:00 2001 From: Sebastian Date: Wed, 11 Dec 2024 15:46:15 -0500 Subject: [PATCH 19/21] fix merge --- .../src/rules/perflint/rules/manual_list_comprehension.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 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 7167eaf1d22e7..5921c004807bc 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,8 +4,7 @@ use crate::checkers::ast::Checker; use anyhow::{anyhow, Result}; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; -use ruff_macros::{derive_message_formats, violation, ViolationMetadata}; -use ruff_python_ast::comparable::ComparableExpr; +use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::any_over_expr; use ruff_python_parser::{parse, Mode, TokenKind}; use ruff_python_semantic::{analyze::typing::is_list, Binding}; @@ -514,7 +513,7 @@ fn convert_to_list_extend( Ok(Fix::unsafe_edits( Edit::range_deletion(binding_stmt_range), [Edit::range_replacement(comprehension_body, for_stmt.range)], - )) + )) } } } From 9c55f1b91b37b2ccd9039c88de6296683f3e5890 Mon Sep 17 00:00:00 2001 From: Sebastian Date: Wed, 11 Dec 2024 16:23:57 -0500 Subject: [PATCH 20/21] replace tokenization with simple character iterator --- .../rules/manual_list_comprehension.rs | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 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 5921c004807bc..b3e0da7079877 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,6 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::any_over_expr; -use ruff_python_parser::{parse, Mode, TokenKind}; use ruff_python_semantic::{analyze::typing::is_list, Binding}; use ruff_python_trivia::{is_python_whitespace, PythonWhitespace}; use ruff_source_file::LineRanges; @@ -453,21 +452,28 @@ fn convert_to_list_extend( .ok_or(anyhow!( "Binding must have a statement to convert into a list comprehension" ))?; - // If the binding has multiple statements on its line, only remove the list assignment + // If the binding has multiple statements on its line, the fix would be substantially more complicated let binding_is_multiple_exprs = { - let binding_text = locator - .slice(locator.full_lines_range(binding_stmt_range)) - .trim_whitespace_start(); - let tokens = parse(binding_text, Mode::Module).map_err(|err| { - anyhow!( - "Failed to tokenize binding statement: {err} {}", - binding_text + let full_range = locator.full_lines_range(binding_stmt_range); + let part_before = locator.slice(TextRange::new( + full_range.start(), + binding_stmt_range.start(), + )); + let part_after = + locator.slice(TextRange::new(binding_stmt_range.end(), full_range.end())); + // determine whether there's a semicolon either before or after the binding statement. + // Since it's a binding statement, we can just check whether there's a semicolon immediately + // after the whitespace in front of or behind it + part_before + .chars() + .rev() + .take_while(|c| is_python_whitespace(*c) || *c == ';') + .chain( + part_after + .chars() + .take_while(|c| is_python_whitespace(*c) || *c == ';'), ) - })?; - tokens - .tokens() - .iter() - .any(|token| matches!(token.kind(), TokenKind::Semi)) + .any(|c| c == ';') }; // If there are multiple binding statements in one line, we don't want to accidentally delete them // Instead, we just delete the binding statement and leave any comments where they are @@ -513,7 +519,7 @@ fn convert_to_list_extend( Ok(Fix::unsafe_edits( Edit::range_deletion(binding_stmt_range), [Edit::range_replacement(comprehension_body, for_stmt.range)], - )) + )) } } } From 523808d7a73a3d31292187ab5686e476b272d664 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 12 Dec 2024 09:04:15 +0100 Subject: [PATCH 21/21] Use simple tokenizer --- .../rules/manual_list_comprehension.rs | 106 ++++++++++-------- ...t__tests__preview__PERF401_PERF401.py.snap | 3 +- 2 files changed, 63 insertions(+), 46 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 b3e0da7079877..415f7313d005a 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,9 +7,10 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::any_over_expr; use ruff_python_semantic::{analyze::typing::is_list, Binding}; -use ruff_python_trivia::{is_python_whitespace, PythonWhitespace}; +use ruff_python_trivia::{BackwardsTokenizer, PythonWhitespace, SimpleTokenKind, SimpleTokenizer}; use ruff_source_file::LineRanges; -use ruff_text_size::{Ranged, TextRange, TextSize}; +use ruff_text_size::{Ranged, TextRange}; + /// ## What it does /// Checks for `for` loops that can be replaced by a list comprehension. /// @@ -442,8 +443,8 @@ fn convert_to_list_extend( ))) } ComprehensionType::ListComprehension => { - let binding_stmt_range = binding - .statement(semantic) + let binding_stmt = binding.statement(semantic); + let binding_stmt_range = binding_stmt .and_then(|stmt| match stmt { ast::Stmt::AnnAssign(assign) => Some(assign.range), ast::Stmt::Assign(assign) => Some(assign.range), @@ -453,57 +454,72 @@ fn convert_to_list_extend( "Binding must have a statement to convert into a list comprehension" ))?; // If the binding has multiple statements on its line, the fix would be substantially more complicated - let binding_is_multiple_exprs = { - let full_range = locator.full_lines_range(binding_stmt_range); - let part_before = locator.slice(TextRange::new( - full_range.start(), - binding_stmt_range.start(), - )); - let part_after = - locator.slice(TextRange::new(binding_stmt_range.end(), full_range.end())); + let (semicolon_before, after_semicolon) = { // determine whether there's a semicolon either before or after the binding statement. // Since it's a binding statement, we can just check whether there's a semicolon immediately // after the whitespace in front of or behind it - part_before - .chars() - .rev() - .take_while(|c| is_python_whitespace(*c) || *c == ';') - .chain( - part_after - .chars() - .take_while(|c| is_python_whitespace(*c) || *c == ';'), - ) - .any(|c| c == ';') + let mut after_tokenizer = + SimpleTokenizer::starts_at(binding_stmt_range.end(), locator.contents()) + .skip_trivia(); + + let after_semicolon = if after_tokenizer + .next() + .is_some_and(|token| token.kind() == SimpleTokenKind::Semi) + { + after_tokenizer.next() + } else { + None + }; + + let semicolon_before = BackwardsTokenizer::up_to( + binding_stmt_range.start(), + locator.contents(), + checker.comment_ranges(), + ) + .skip_trivia() + .next() + .filter(|token| token.kind() == SimpleTokenKind::Semi); + + (semicolon_before, after_semicolon) }; // If there are multiple binding statements in one line, we don't want to accidentally delete them // Instead, we just delete the binding statement and leave any comments where they are - let binding_stmt_range = if binding_is_multiple_exprs { - let end_of_line = locator.slice(TextRange::new( - binding_stmt_range.end(), - locator.full_line_end(binding_stmt_range.end()), - )); - let first_safe: u32 = end_of_line - .chars() - .position(|c| !is_python_whitespace(c) && c != ';') - .expect("end of line must always include a newline, since for loop is after binding") - .try_into() - .expect("semicolon/whitespace should be at most a few characters"); - binding_stmt_range.add_end(TextSize::new(first_safe)) - } else { - locator.full_lines_range(binding_stmt_range) - }; - - let annotations = match binding - .statement(semantic) - .and_then(|stmt| stmt.as_ann_assign_stmt()) - { + let (binding_stmt_deletion_range, binding_is_multiple_stmts) = + match (semicolon_before, after_semicolon) { + // ```python + // a = [] + // ``` + (None, None) => (locator.full_lines_range(binding_stmt_range), false), + + // ```python + // a = 1; b = [] + // ^^^^^^^^ + // a = 1; b = []; c = 3 + // ^^^^^^^^ + // ``` + (Some(semicolon_before), Some(_) | None) => ( + TextRange::new(semicolon_before.start(), binding_stmt_range.end()), + true, + ), + + // ```python + // a = []; b = 3 + // ^^^^^^^ + // ``` + (None, Some(after_semicolon)) => ( + TextRange::new(binding_stmt_range.start(), after_semicolon.start()), + true, + ), + }; + + let annotations = match binding_stmt.and_then(|stmt| stmt.as_ann_assign_stmt()) { Some(assign) => format!(": {}", locator.slice(assign.annotation.range())), None => String::new(), }; let mut comments_to_move = for_loop_inline_comments; - if !binding_is_multiple_exprs { - comments_to_move.extend(comment_strings_in_range(binding_stmt_range)); + if !binding_is_multiple_stmts { + comments_to_move.extend(comment_strings_in_range(binding_stmt_deletion_range)); } let indentation = if comments_to_move.is_empty() { @@ -517,7 +533,7 @@ fn convert_to_list_extend( format!("{leading_comments}{variable_name}{annotations} = [{generator_str}]"); Ok(Fix::unsafe_edits( - Edit::range_deletion(binding_stmt_range), + Edit::range_deletion(binding_stmt_deletion_range), [Edit::range_replacement(comprehension_body, for_stmt.range)], )) } 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 8f6e30364a4aa..a8c6adde82002 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 @@ -1,5 +1,6 @@ --- 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 | @@ -237,7 +238,7 @@ PERF401.py:149:9: PERF401 [*] Use a list comprehension to create a transformed l 147 |- tmp = 1; result = [] # commment should be protected 148 |- for i in range(10): 149 |- result.append(i + 1) # PERF401 - 147 |+ tmp = 1; # commment should be protected + 147 |+ tmp = 1 # commment should be protected 148 |+ result = [i + 1 for i in range(10)] # PERF401 150 149 | 151 150 |