diff --git a/crates/ruff_linter/resources/test/fixtures/perflint/PERF403.py b/crates/ruff_linter/resources/test/fixtures/perflint/PERF403.py index 8ef430c8fc..89afca39ff 100644 --- a/crates/ruff_linter/resources/test/fixtures/perflint/PERF403.py +++ b/crates/ruff_linter/resources/test/fixtures/perflint/PERF403.py @@ -18,7 +18,9 @@ def foo(): result = {} for idx, name in enumerate(fruit): if idx % 2: - result[idx] = name # Ok (false negative: edge case where `else` is same as `if`) + result[idx] = ( + name # Ok (false negative: edge case where `else` is same as `if`) + ) else: result[idx] = name @@ -85,7 +87,67 @@ def foo(): def foo(): from builtins import dict as SneakyDict + fruit = ["apple", "pear", "orange"] result = SneakyDict() for idx, name in enumerate(fruit): result[name] = idx # PERF403 + + +def foo(): + fruit = ["apple", "pear", "orange"] + result: dict[str, int] = { + # comment 1 + } + for idx, name in enumerate( + fruit # comment 2 + ): + # comment 3 + result[ + name # comment 4 + ] = idx # PERF403 + + +def foo(): + fruit = ["apple", "pear", "orange"] + a = 1; result = {}; b = 2 + for idx, name in enumerate(fruit): + result[name] = idx # PERF403 + + +def foo(): + fruit = ["apple", "pear", "orange"] + result = {"kiwi": 3} + for idx, name in enumerate(fruit): + result[name] = idx # PERF403 + + +def foo(): + fruit = ["apple", "pear", "orange"] + (_, result) = (None, {"kiwi": 3}) + for idx, name in enumerate(fruit): + result[name] = idx # PERF403 + + +def foo(): + fruit = ["apple", "pear", "orange"] + result = {} + print(len(result)) + for idx, name in enumerate(fruit): + result[name] = idx # PERF403 + + +def foo(): + fruit = ["apple", "pear", "orange"] + result = {} + for idx, name in enumerate(fruit): + if last_idx := idx % 3: + result[name] = idx # PERF403 + + +def foo(): + fruit = ["apple", "pear", "orange"] + indices = [0, 1, 2] + result = {} + for idx, name in indices, fruit: + result[name] = idx # PERF403 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 7bc5cf097f..abb9110b6e 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 @@ -35,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::ManualDictComprehension) { + perflint::rules::manual_dict_comprehension(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 e503e7df1a..84563726a2 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1319,6 +1319,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { Rule::UnnecessaryEnumerate, Rule::UnusedLoopControlVariable, Rule::YieldInForLoop, + Rule::ManualDictComprehension, Rule::ManualListComprehension, ]) { checker.analyze.for_loops.push(checker.semantic.snapshot()); @@ -1347,9 +1348,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::ManualListCopy) { perflint::rules::manual_list_copy(checker, for_stmt); } - if checker.enabled(Rule::ManualDictComprehension) { - perflint::rules::manual_dict_comprehension(checker, target, body); - } + if checker.enabled(Rule::ModifiedIteratingSet) { pylint::rules::modified_iterating_set(checker, for_stmt); } diff --git a/crates/ruff_linter/src/rules/perflint/helpers.rs b/crates/ruff_linter/src/rules/perflint/helpers.rs new file mode 100644 index 0000000000..271975246c --- /dev/null +++ b/crates/ruff_linter/src/rules/perflint/helpers.rs @@ -0,0 +1,98 @@ +use ruff_python_trivia::{ + BackwardsTokenizer, PythonWhitespace, SimpleToken, SimpleTokenKind, SimpleTokenizer, +}; +use ruff_source_file::LineRanges; +use ruff_text_size::{Ranged, TextRange}; + +use crate::checkers::ast::Checker; + +pub(super) fn comment_strings_in_range<'a>( + checker: &'a Checker, + range: TextRange, + ranges_to_ignore: &[TextRange], +) -> Vec<&'a str> { + checker + .comment_ranges() + .comments_in_range(range) + .iter() + // Ignore comments inside of the append or iterator, since these are preserved + .filter(|comment| { + !ranges_to_ignore + .iter() + .any(|to_ignore| to_ignore.contains_range(**comment)) + }) + .map(|range| checker.locator().slice(range).trim_whitespace_start()) + .collect() +} + +fn semicolon_before_and_after( + checker: &Checker, + statement: TextRange, +) -> (Option, Option) { + // 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 + let mut after_tokenizer = + SimpleTokenizer::starts_at(statement.end(), checker.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( + statement.start(), + checker.locator().contents(), + checker.comment_ranges(), + ) + .skip_trivia() + .next() + .filter(|token| token.kind() == SimpleTokenKind::Semi); + + (semicolon_before, after_semicolon) +} + +/// Finds the range necessary to delete a statement (including any semicolons around it). +/// Returns the range and whether there were multiple statements on the line +pub(super) fn statement_deletion_range( + checker: &Checker, + statement_range: TextRange, +) -> (TextRange, bool) { + let locator = checker.locator(); + // If the binding has multiple statements on its line, the fix would be substantially more complicated + let (semicolon_before, after_semicolon) = semicolon_before_and_after(checker, statement_range); + + // 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 + + match (semicolon_before, after_semicolon) { + // ```python + // a = [] + // ``` + (None, None) => (locator.full_lines_range(statement_range), false), + + // ```python + // a = 1; b = [] + // ^^^^^^^^ + // a = 1; b = []; c = 3 + // ^^^^^^^^ + // ``` + (Some(semicolon_before), Some(_) | None) => ( + TextRange::new(semicolon_before.start(), statement_range.end()), + true, + ), + + // ```python + // a = []; b = 3 + // ^^^^^^^ + // ``` + (None, Some(after_semicolon)) => ( + TextRange::new(statement_range.start(), after_semicolon.start()), + true, + ), + } +} diff --git a/crates/ruff_linter/src/rules/perflint/mod.rs b/crates/ruff_linter/src/rules/perflint/mod.rs index 07d67d8363..74baf85e65 100644 --- a/crates/ruff_linter/src/rules/perflint/mod.rs +++ b/crates/ruff_linter/src/rules/perflint/mod.rs @@ -1,6 +1,6 @@ //! Rules from [perflint](https://pypi.org/project/perflint/). +mod helpers; pub(crate) mod rules; - #[cfg(test)] mod tests { use std::path::Path; @@ -31,7 +31,8 @@ mod tests { Ok(()) } - // TODO: remove this test case when the fix for `perf401` is stabilized + // TODO: remove this test case when the fixes for `perf401` and `perf403` are stabilized + #[test_case(Rule::ManualDictComprehension, Path::new("PERF403.py"))] #[test_case(Rule::ManualListComprehension, Path::new("PERF401.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( diff --git a/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs b/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs index 97f53cb55c..c2b87b6efe 100644 --- a/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs +++ b/crates/ruff_linter/src/rules/perflint/rules/manual_dict_comprehension.rs @@ -1,11 +1,14 @@ -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; -use ruff_python_ast::comparable::ComparableExpr; -use ruff_python_ast::helpers::any_over_expr; -use ruff_python_ast::{self as ast, Expr, Stmt}; -use ruff_python_semantic::analyze::typing::is_dict; +use ruff_python_ast::{ + self as ast, comparable::ComparableExpr, helpers::any_over_expr, Expr, Stmt, +}; +use ruff_python_semantic::{analyze::typing::is_dict, Binding}; +use ruff_source_file::LineRanges; +use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; +use crate::rules::perflint::helpers::{comment_strings_in_range, statement_deletion_range}; /// ## What it does /// Checks for `for` loops that can be replaced by a dictionary comprehension. @@ -42,17 +45,45 @@ use crate::checkers::ast::Checker; /// result.update({x: y for x, y in pairs if y % 2}) /// ``` #[derive(ViolationMetadata)] -pub(crate) struct ManualDictComprehension; +pub(crate) struct ManualDictComprehension { + fix_type: DictComprehensionType, + is_async: bool, +} impl Violation for ManualDictComprehension { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { - "Use a dictionary comprehension instead of a for-loop".to_string() + let modifier = if self.is_async { "an async" } else { "a" }; + + match self.fix_type { + DictComprehensionType::Comprehension => { + format!("Use a dictionary comprehension instead of {modifier} for-loop") + } + DictComprehensionType::Update => { + format!("Use `dict.update` instead of {modifier} for-loop") + } + } + } + fn fix_title(&self) -> Option { + let modifier = if self.is_async { "async " } else { "" }; + match self.fix_type { + DictComprehensionType::Comprehension => Some(format!( + "Replace {modifier}for loop with dict comprehension" + )), + DictComprehensionType::Update => { + Some(format!("Replace {modifier}for loop with `dict.update`")) + } + } } } /// PERF403 -pub(crate) fn manual_dict_comprehension(checker: &Checker, target: &Expr, body: &[Stmt]) { +pub(crate) fn manual_dict_comprehension(checker: &Checker, for_stmt: &ast::StmtFor) { + let ast::StmtFor { body, target, .. } = for_stmt; + let body = body.as_slice(); + let target = target.as_ref(); let (stmt, if_test) = match body { // ```python // for idx, name in enumerate(names): @@ -94,18 +125,42 @@ pub(crate) fn manual_dict_comprehension(checker: &Checker, target: &Expr, body: let [Expr::Subscript(ast::ExprSubscript { value: subscript_value, - slice, + slice: key, .. })] = targets.as_slice() else { return; }; + // If any references to a target variable are after the loop, + // then removing the loop would cause a NameError + let any_references_after_for_loop = |target: &Expr| { + let target_binding = checker + .semantic() + .bindings + .iter() + .find(|binding| target.range() == binding.range); + debug_assert!( + target_binding.is_some(), + "for-loop target binding must exist" + ); + + let Some(target_binding) = target_binding else { + // All uses of this function will early-return if this returns true, so this must early-return the rule + return true; + }; + + target_binding + .references() + .map(|reference| checker.semantic().reference(reference)) + .any(|other_reference| other_reference.start() > for_stmt.end()) + }; + match target { Expr::Tuple(tuple) => { if !tuple .iter() - .any(|element| ComparableExpr::from(slice) == ComparableExpr::from(element)) + .any(|element| ComparableExpr::from(key) == ComparableExpr::from(element)) { return; } @@ -115,14 +170,24 @@ pub(crate) fn manual_dict_comprehension(checker: &Checker, target: &Expr, body: { return; } + // Make sure none of the variables are used outside the for loop + if tuple.iter().any(any_references_after_for_loop) { + return; + } } Expr::Name(_) => { - if ComparableExpr::from(slice) != ComparableExpr::from(target) { + if ComparableExpr::from(key) != ComparableExpr::from(target) { return; } if ComparableExpr::from(value) != ComparableExpr::from(target) { return; } + + // We know that `target` contains an ExprName, but closures can't take `&impl Ranged`, + // so we pass `target` itself instead of the inner ExprName + if any_references_after_for_loop(target) { + return; + } } _ => return, } @@ -164,5 +229,242 @@ pub(crate) fn manual_dict_comprehension(checker: &Checker, target: &Expr, body: return; } - checker.report_diagnostic(Diagnostic::new(ManualDictComprehension, *range)); + if checker.settings.preview.is_enabled() { + 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_deref(), + ast::Stmt::Assign(assign) => Some(&assign.value), + _ => None, + }); + + // If the variable is an empty dict literal, then we might be able to replace it with a full dict comprehension. + // otherwise, it has to be replaced with a `dict.update` + let binding_is_empty_dict = + binding_value.is_some_and(|binding_value| match binding_value { + // value = {} + Expr::Dict(dict_expr) => dict_expr.is_empty(), + // value = dict() + Expr::Call(call) => { + checker + .semantic() + .resolve_builtin_symbol(&call.func) + .is_some_and(|name| name == "dict") + && call.arguments.is_empty() + } + _ => false, + }); + + let assignment_in_same_statement = 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 + }); + // If the binding is not a single name expression, it could be replaced with a dict comprehension, + // but not necessarily, so this needs to be manually fixed. This does not apply when using an update. + let binding_has_one_target = binding_stmt.is_some_and(|binding_stmt| match binding_stmt { + ast::Stmt::AnnAssign(_) => true, + ast::Stmt::Assign(assign) => assign.targets.len() == 1, + _ => false, + }); + // If the binding gets used in between the assignment and the for loop, a comprehension is no longer safe + + // If the binding is after the for loop, then it can't be fixed, and this check would panic, + // so we check that they are in the same statement first + let binding_unused_between = assignment_in_same_statement + && 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 result dictionary 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 comprehension + !binding + .references() + .map(|ref_id| checker.semantic().reference(ref_id).range()) + .any(|text_range| from_assign_to_loop.contains_range(text_range)) + }); + // A dict update works in every context, while a dict comprehension only works when all the criteria are true + let fix_type = if binding_is_empty_dict + && assignment_in_same_statement + && binding_has_one_target + && binding_unused_between + { + DictComprehensionType::Comprehension + } else { + DictComprehensionType::Update + }; + + let mut diagnostic = Diagnostic::new( + ManualDictComprehension { + fix_type, + is_async: for_stmt.is_async, + }, + *range, + ); + diagnostic.try_set_optional_fix(|| { + Ok(convert_to_dict_comprehension( + fix_type, + binding, + for_stmt, + if_test.map(std::convert::AsRef::as_ref), + key.as_ref(), + value.as_ref(), + checker, + )) + }); + + checker.report_diagnostic(diagnostic); + } else { + checker.report_diagnostic(Diagnostic::new( + ManualDictComprehension { + fix_type: DictComprehensionType::Comprehension, + is_async: for_stmt.is_async, + }, + *range, + )); + } +} + +fn convert_to_dict_comprehension( + fix_type: DictComprehensionType, + binding: &Binding, + for_stmt: &ast::StmtFor, + if_test: Option<&ast::Expr>, + key: &Expr, + value: &Expr, + checker: &Checker, +) -> Option { + let locator = checker.locator(); + + let if_str = match if_test { + Some(test) => { + // If the test is an assignment expression, + // we must parenthesize it when it appears + // inside the comprehension to avoid a syntax error. + // + // Notice that we do not need `any_over_expr` here, + // since if the assignment expression appears + // internally (e.g. as an operand in a boolean + // operation) then it will already be parenthesized. + if test.is_named_expr() { + format!(" if ({})", locator.slice(test.range())) + } else { + format!(" if {}", locator.slice(test.range())) + } + } + None => String::new(), + }; + + // 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 iter_str = if let Expr::Tuple(ast::ExprTuple { + parenthesized: false, + .. + }) = &*for_stmt.iter + { + format!("({})", locator.slice(for_stmt.iter.range())) + } else { + locator.slice(for_stmt.iter.range()).to_string() + }; + + let target_str = locator.slice(for_stmt.target.range()); + let for_type = if for_stmt.is_async { + "async for" + } else { + "for" + }; + let elt_str = format!( + "{}: {}", + locator.slice(key.range()), + locator.slice(value.range()) + ); + + let comprehension_str = format!("{{{elt_str} {for_type} {target_str} in {iter_str}{if_str}}}"); + + let for_loop_inline_comments = comment_strings_in_range( + checker, + for_stmt.range, + &[key.range(), value.range(), for_stmt.iter.range()], + ); + + let newline = checker.stylist().line_ending().as_str(); + + let indent = locator.slice(TextRange::new( + locator.line_start(for_stmt.range.start()), + for_stmt.range.start(), + )); + + let variable_name = locator.slice(binding); + match fix_type { + DictComprehensionType::Update => { + let indentation = if for_loop_inline_comments.is_empty() { + String::new() + } else { + format!("{newline}{indent}") + }; + + let comprehension_body = format!("{variable_name}.update({comprehension_str})"); + + let text_to_replace = format!( + "{}{indentation}{comprehension_body}", + for_loop_inline_comments.join(&indentation) + ); + + Some(Fix::unsafe_edit(Edit::range_replacement( + text_to_replace, + for_stmt.range, + ))) + } + DictComprehensionType::Comprehension => { + let binding_stmt = binding.statement(checker.semantic()); + debug_assert!( + binding_stmt.is_some(), + "must be passed a binding with a statement" + ); + let binding_stmt = binding_stmt?; + + let binding_stmt_range = binding_stmt.range(); + + let annotations = match binding_stmt.as_ann_assign_stmt() { + Some(assign) => format!(": {}", locator.slice(assign.annotation.range())), + None => String::new(), + }; + + // 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_deletion_range, binding_is_multiple_stmts) = + statement_deletion_range(checker, binding_stmt_range); + + let comments_to_move = if binding_is_multiple_stmts { + for_loop_inline_comments + } else { + let mut new_comments = + comment_strings_in_range(checker, binding_stmt_deletion_range, &[]); + new_comments.extend(for_loop_inline_comments); + new_comments + }; + + let indentation = if comments_to_move.is_empty() { + String::new() + } else { + format!("{newline}{indent}") + }; + let leading_comments = format!("{}{indentation}", comments_to_move.join(&indentation)); + + let comprehension_body = + format!("{leading_comments}{variable_name}{annotations} = {comprehension_str}"); + Some(Fix::unsafe_edits( + Edit::range_deletion(binding_stmt_deletion_range), + [Edit::range_replacement(comprehension_body, for_stmt.range)], + )) + } + } +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum DictComprehensionType { + Update, + Comprehension, } 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 752678546b..35a0cd2d3e 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,16 +1,15 @@ use ruff_python_ast::{self as ast, Arguments, Expr}; -use crate::checkers::ast::Checker; +use crate::{checkers::ast::Checker, rules::perflint::helpers::statement_deletion_range}; use anyhow::{anyhow, Result}; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use crate::rules::perflint::helpers::comment_strings_in_range; 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::{BackwardsTokenizer, PythonWhitespace, SimpleTokenKind, SimpleTokenizer}; use ruff_source_file::LineRanges; use ruff_text_size::{Ranged, TextRange}; - /// ## What it does /// Checks for `for` loops that can be replaced by a list comprehension. /// @@ -264,12 +263,14 @@ pub(crate) fn manual_list_comprehension(checker: &Checker, for_stmt: &ast::StmtF 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` + // otherwise, it has to be replaced with a `list.extend`. let binding_is_empty_list = - list_binding_value.is_some_and(|binding_value| match binding_value.as_list_expr() { - Some(list_expr) => list_expr.elts.is_empty(), - None => false, + list_binding_value.is_some_and(|binding_value| match binding_value { + // `value = []` + Expr::List(list_expr) => list_expr.is_empty(), + _ => false, }); // If the for loop does not have the same parent element as the binding, then it cannot always be @@ -397,22 +398,12 @@ fn convert_to_list_extend( let elt_str = locator.slice(to_append); let generator_str = format!("{elt_str} {for_type} {target_str} in {for_iter_str}{if_str}"); - let comment_strings_in_range = |range| { - checker - .comment_ranges() - .comments_in_range(range) - .iter() - // 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() - }; - let variable_name = locator.slice(binding); - let for_loop_inline_comments: Vec<&str> = comment_strings_in_range(for_stmt.range); + let for_loop_inline_comments = comment_strings_in_range( + checker, + for_stmt.range, + &[to_append.range(), for_stmt.iter.range()], + ); let newline = checker.stylist().line_ending().as_str(); @@ -457,74 +448,25 @@ 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, the fix would be substantially more complicated - 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 - 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_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, - ), - }; + statement_deletion_range(checker, binding_stmt_range); 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_stmts { - comments_to_move.extend(comment_strings_in_range(binding_stmt_deletion_range)); - } + let comments_to_move = if binding_is_multiple_stmts { + for_loop_inline_comments + } else { + let mut new_comments = + comment_strings_in_range(checker, binding_stmt_deletion_range, &[]); + new_comments.extend(for_loop_inline_comments); + new_comments + }; let indentation = if comments_to_move.is_empty() { String::new() diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF403_PERF403.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF403_PERF403.py.snap index 6f26d4eeee..2615a1772d 100644 --- a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF403_PERF403.py.snap +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__PERF403_PERF403.py.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/perflint/mod.rs -snapshot_kind: text --- PERF403.py:5:9: PERF403 Use a dictionary comprehension instead of a for-loop | @@ -9,6 +8,7 @@ PERF403.py:5:9: PERF403 Use a dictionary comprehension instead of a for-loop 5 | result[idx] = name # PERF403 | ^^^^^^^^^^^^^^^^^^ PERF403 | + = help: Replace for loop with dict comprehension PERF403.py:13:13: PERF403 Use a dictionary comprehension instead of a for-loop | @@ -17,43 +17,114 @@ PERF403.py:13:13: PERF403 Use a dictionary comprehension instead of a for-loop 13 | result[idx] = name # PERF403 | ^^^^^^^^^^^^^^^^^^ PERF403 | + = help: Replace for loop with dict comprehension -PERF403.py:31:13: PERF403 Use a dictionary comprehension instead of a for-loop +PERF403.py:33:13: PERF403 Use a dictionary comprehension instead of a for-loop | -29 | for idx, name in enumerate(fruit): -30 | if idx % 2: -31 | result[idx] = name # PERF403 +31 | for idx, name in enumerate(fruit): +32 | if idx % 2: +33 | result[idx] = name # PERF403 | ^^^^^^^^^^^^^^^^^^ PERF403 | + = help: Replace for loop with dict comprehension -PERF403.py:61:13: PERF403 Use a dictionary comprehension instead of a for-loop +PERF403.py:63:13: PERF403 Use a dictionary comprehension instead of a for-loop | -59 | for idx, name in enumerate(fruit): -60 | if idx % 2: -61 | result[idx] = name # PERF403 +61 | for idx, name in enumerate(fruit): +62 | if idx % 2: +63 | result[idx] = name # PERF403 | ^^^^^^^^^^^^^^^^^^ PERF403 | + = help: Replace for loop with dict comprehension -PERF403.py:76:9: PERF403 Use a dictionary comprehension instead of a for-loop +PERF403.py:78:9: PERF403 Use a dictionary comprehension instead of a for-loop | -74 | result = {} -75 | for name in fruit: -76 | result[name] = name # PERF403 +76 | result = {} +77 | for name in fruit: +78 | result[name] = name # PERF403 | ^^^^^^^^^^^^^^^^^^^ PERF403 | + = help: Replace for loop with dict comprehension -PERF403.py:83:9: PERF403 Use a dictionary comprehension instead of a for-loop +PERF403.py:85:9: PERF403 Use a dictionary comprehension instead of a for-loop | -81 | result = {} -82 | for idx, name in enumerate(fruit): -83 | result[name] = idx # PERF403 +83 | result = {} +84 | for idx, name in enumerate(fruit): +85 | result[name] = idx # PERF403 | ^^^^^^^^^^^^^^^^^^ PERF403 | + = help: Replace for loop with dict comprehension -PERF403.py:91:9: PERF403 Use a dictionary comprehension instead of a for-loop +PERF403.py:94:9: PERF403 Use a dictionary comprehension instead of a for-loop | -89 | result = SneakyDict() -90 | for idx, name in enumerate(fruit): -91 | result[name] = idx # PERF403 +92 | result = SneakyDict() +93 | for idx, name in enumerate(fruit): +94 | result[name] = idx # PERF403 | ^^^^^^^^^^^^^^^^^^ PERF403 | + = help: Replace for loop with dict comprehension + +PERF403.py:106:9: PERF403 Use a dictionary comprehension instead of a for-loop + | +104 | ): +105 | # comment 3 +106 | / result[ +107 | | name # comment 4 +108 | | ] = idx # PERF403 + | |_______________^ PERF403 + | + = help: Replace for loop with dict comprehension + +PERF403.py:115:9: PERF403 Use a dictionary comprehension instead of a for-loop + | +113 | a = 1; result = {}; b = 2 +114 | for idx, name in enumerate(fruit): +115 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +PERF403.py:122:9: PERF403 Use a dictionary comprehension instead of a for-loop + | +120 | result = {"kiwi": 3} +121 | for idx, name in enumerate(fruit): +122 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +PERF403.py:129:9: PERF403 Use a dictionary comprehension instead of a for-loop + | +127 | (_, result) = (None, {"kiwi": 3}) +128 | for idx, name in enumerate(fruit): +129 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +PERF403.py:137:9: PERF403 Use a dictionary comprehension instead of a for-loop + | +135 | print(len(result)) +136 | for idx, name in enumerate(fruit): +137 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +PERF403.py:145:13: PERF403 Use a dictionary comprehension instead of a for-loop + | +143 | for idx, name in enumerate(fruit): +144 | if last_idx := idx % 3: +145 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +PERF403.py:153:9: PERF403 Use a dictionary comprehension instead of a for-loop + | +151 | result = {} +152 | for idx, name in indices, fruit: +153 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict 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 ff568d1821..8bcbb47d03 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 |+ # 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 + 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 | diff --git a/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF403_PERF403.py.snap b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF403_PERF403.py.snap new file mode 100644 index 0000000000..9e01e0db30 --- /dev/null +++ b/crates/ruff_linter/src/rules/perflint/snapshots/ruff_linter__rules__perflint__tests__preview__PERF403_PERF403.py.snap @@ -0,0 +1,307 @@ +--- +source: crates/ruff_linter/src/rules/perflint/mod.rs +--- +PERF403.py:5:9: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +3 | result = {} +4 | for idx, name in enumerate(fruit): +5 | result[idx] = name # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +1 1 | def foo(): +2 2 | fruit = ["apple", "pear", "orange"] +3 |- result = {} +4 |- for idx, name in enumerate(fruit): +5 |- result[idx] = name # PERF403 + 3 |+ result = {idx: name for idx, name in enumerate(fruit)} # PERF403 +6 4 | +7 5 | +8 6 | def foo(): + +PERF403.py:13:13: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +11 | for idx, name in enumerate(fruit): +12 | if idx % 2: +13 | result[idx] = name # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +7 7 | +8 8 | def foo(): +9 9 | fruit = ["apple", "pear", "orange"] +10 |- result = {} +11 |- for idx, name in enumerate(fruit): +12 |- if idx % 2: +13 |- result[idx] = name # PERF403 + 10 |+ result = {idx: name for idx, name in enumerate(fruit) if idx % 2} # PERF403 +14 11 | +15 12 | +16 13 | def foo(): + +PERF403.py:33:13: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +31 | for idx, name in enumerate(fruit): +32 | if idx % 2: +33 | result[idx] = name # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +26 26 | +27 27 | +28 28 | def foo(): +29 |- result = {} +30 29 | fruit = ["apple", "pear", "orange"] +31 |- for idx, name in enumerate(fruit): +32 |- if idx % 2: +33 |- result[idx] = name # PERF403 + 30 |+ result = {idx: name for idx, name in enumerate(fruit) if idx % 2} # PERF403 +34 31 | +35 32 | +36 33 | def foo(): + +PERF403.py:63:13: PERF403 [*] Use `dict.update` instead of a for-loop + | +61 | for idx, name in enumerate(fruit): +62 | if idx % 2: +63 | result[idx] = name # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with `dict.update` + +ℹ Unsafe fix +58 58 | def foo(): +59 59 | result = {1: "banana"} +60 60 | fruit = ["apple", "pear", "orange"] +61 |- for idx, name in enumerate(fruit): +62 |- if idx % 2: +63 |- result[idx] = name # PERF403 + 61 |+ result.update({idx: name for idx, name in enumerate(fruit) if idx % 2}) # PERF403 +64 62 | +65 63 | +66 64 | def foo(): + +PERF403.py:78:9: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +76 | result = {} +77 | for name in fruit: +78 | result[name] = name # PERF403 + | ^^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +73 73 | +74 74 | def foo(): +75 75 | fruit = ["apple", "pear", "orange"] +76 |- result = {} +77 |- for name in fruit: +78 |- result[name] = name # PERF403 + 76 |+ result = {name: name for name in fruit} # PERF403 +79 77 | +80 78 | +81 79 | def foo(): + +PERF403.py:85:9: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +83 | result = {} +84 | for idx, name in enumerate(fruit): +85 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +80 80 | +81 81 | def foo(): +82 82 | fruit = ["apple", "pear", "orange"] +83 |- result = {} +84 |- for idx, name in enumerate(fruit): +85 |- result[name] = idx # PERF403 + 83 |+ result = {name: idx for idx, name in enumerate(fruit)} # PERF403 +86 84 | +87 85 | +88 86 | def foo(): + +PERF403.py:94:9: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +92 | result = SneakyDict() +93 | for idx, name in enumerate(fruit): +94 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +89 89 | from builtins import dict as SneakyDict +90 90 | +91 91 | fruit = ["apple", "pear", "orange"] +92 |- result = SneakyDict() +93 |- for idx, name in enumerate(fruit): +94 |- result[name] = idx # PERF403 + 92 |+ result = {name: idx for idx, name in enumerate(fruit)} # PERF403 +95 93 | +96 94 | +97 95 | def foo(): + +PERF403.py:106:9: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +104 | ): +105 | # comment 3 +106 | / result[ +107 | | name # comment 4 +108 | | ] = idx # PERF403 + | |_______________^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +96 96 | +97 97 | def foo(): +98 98 | fruit = ["apple", "pear", "orange"] +99 |- result: dict[str, int] = { +100 |- # comment 1 +101 |- } +102 |- for idx, name in enumerate( + 99 |+ # comment 1 + 100 |+ # comment 3 + 101 |+ # comment 4 + 102 |+ result: dict[str, int] = {name: idx for idx, name in enumerate( +103 103 | fruit # comment 2 +104 |- ): +105 |- # comment 3 +106 |- result[ +107 |- name # comment 4 +108 |- ] = idx # PERF403 + 104 |+ )} # PERF403 +109 105 | +110 106 | +111 107 | def foo(): + +PERF403.py:115:9: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +113 | a = 1; result = {}; b = 2 +114 | for idx, name in enumerate(fruit): +115 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +110 110 | +111 111 | def foo(): +112 112 | fruit = ["apple", "pear", "orange"] +113 |- a = 1; result = {}; b = 2 +114 |- for idx, name in enumerate(fruit): +115 |- result[name] = idx # PERF403 + 113 |+ a = 1; b = 2 + 114 |+ result = {name: idx for idx, name in enumerate(fruit)} # PERF403 +116 115 | +117 116 | +118 117 | def foo(): + +PERF403.py:122:9: PERF403 [*] Use `dict.update` instead of a for-loop + | +120 | result = {"kiwi": 3} +121 | for idx, name in enumerate(fruit): +122 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with `dict.update` + +ℹ Unsafe fix +118 118 | def foo(): +119 119 | fruit = ["apple", "pear", "orange"] +120 120 | result = {"kiwi": 3} +121 |- for idx, name in enumerate(fruit): +122 |- result[name] = idx # PERF403 + 121 |+ result.update({name: idx for idx, name in enumerate(fruit)}) # PERF403 +123 122 | +124 123 | +125 124 | def foo(): + +PERF403.py:129:9: PERF403 [*] Use `dict.update` instead of a for-loop + | +127 | (_, result) = (None, {"kiwi": 3}) +128 | for idx, name in enumerate(fruit): +129 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with `dict.update` + +ℹ Unsafe fix +125 125 | def foo(): +126 126 | fruit = ["apple", "pear", "orange"] +127 127 | (_, result) = (None, {"kiwi": 3}) +128 |- for idx, name in enumerate(fruit): +129 |- result[name] = idx # PERF403 + 128 |+ result.update({name: idx for idx, name in enumerate(fruit)}) # PERF403 +130 129 | +131 130 | +132 131 | def foo(): + +PERF403.py:137:9: PERF403 [*] Use `dict.update` instead of a for-loop + | +135 | print(len(result)) +136 | for idx, name in enumerate(fruit): +137 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with `dict.update` + +ℹ Unsafe fix +133 133 | fruit = ["apple", "pear", "orange"] +134 134 | result = {} +135 135 | print(len(result)) +136 |- for idx, name in enumerate(fruit): +137 |- result[name] = idx # PERF403 + 136 |+ result.update({name: idx for idx, name in enumerate(fruit)}) # PERF403 +138 137 | +139 138 | +140 139 | def foo(): + +PERF403.py:145:13: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +143 | for idx, name in enumerate(fruit): +144 | if last_idx := idx % 3: +145 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +139 139 | +140 140 | def foo(): +141 141 | fruit = ["apple", "pear", "orange"] +142 |- result = {} +143 |- for idx, name in enumerate(fruit): +144 |- if last_idx := idx % 3: +145 |- result[name] = idx # PERF403 + 142 |+ result = {name: idx for idx, name in enumerate(fruit) if (last_idx := idx % 3)} # PERF403 +146 143 | +147 144 | +148 145 | def foo(): + +PERF403.py:153:9: PERF403 [*] Use a dictionary comprehension instead of a for-loop + | +151 | result = {} +152 | for idx, name in indices, fruit: +153 | result[name] = idx # PERF403 + | ^^^^^^^^^^^^^^^^^^ PERF403 + | + = help: Replace for loop with dict comprehension + +ℹ Unsafe fix +148 148 | def foo(): +149 149 | fruit = ["apple", "pear", "orange"] +150 150 | indices = [0, 1, 2] +151 |- result = {} +152 |- for idx, name in indices, fruit: +153 |- result[name] = idx # PERF403 + 151 |+ result = {name: idx for idx, name in (indices, fruit)} # PERF403