diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 55ccc7d0162a94..7329b9b9bf333a 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -653,9 +653,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { ); } if checker.enabled(Rule::UnnecessaryListComprehensionSet) { - flake8_comprehensions::rules::unnecessary_list_comprehension_set( - checker, expr, func, args, keywords, - ); + flake8_comprehensions::rules::unnecessary_list_comprehension_set(checker, call); } if checker.enabled(Rule::UnnecessaryListComprehensionDict) { flake8_comprehensions::rules::unnecessary_list_comprehension_dict( diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs index b177a494009f57..aebd3e6b158259 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs @@ -1,3 +1,5 @@ +use std::iter; + use anyhow::{bail, Result}; use itertools::Itertools; use libcst_native::{ @@ -7,7 +9,6 @@ use libcst_native::{ RightCurlyBrace, RightParen, RightSquareBracket, Set, SetComp, SimpleString, SimpleWhitespace, TrailingWhitespace, Tuple, }; -use std::iter; use ruff_diagnostics::{Edit, Fix}; use ruff_python_ast::Expr; @@ -151,46 +152,6 @@ pub(crate) fn fix_unnecessary_generator_dict(expr: &Expr, checker: &Checker) -> )) } -/// (C403) Convert `set([x for x in y])` to `{x for x in y}`. -pub(crate) fn fix_unnecessary_list_comprehension_set( - expr: &Expr, - checker: &Checker, -) -> Result { - let locator = checker.locator(); - let stylist = checker.stylist(); - // Expr(Call(ListComp)))) -> - // Expr(SetComp))) - let module_text = locator.slice(expr); - let mut tree = match_expression(module_text)?; - let call = match_call_mut(&mut tree)?; - let arg = match_arg(call)?; - - let list_comp = match_list_comp(&arg.value)?; - - tree = Expression::SetComp(Box::new(SetComp { - elt: list_comp.elt.clone(), - for_in: list_comp.for_in.clone(), - lbrace: LeftCurlyBrace { - whitespace_after: call.whitespace_before_args.clone(), - }, - rbrace: RightCurlyBrace { - whitespace_before: arg.whitespace_after_arg.clone(), - }, - lpar: list_comp.lpar.clone(), - rpar: list_comp.rpar.clone(), - })); - - Ok(Edit::range_replacement( - pad_expression( - tree.codegen_stylist(stylist), - expr.range(), - checker.locator(), - checker.semantic(), - ), - expr.range(), - )) -} - /// (C404) Convert `dict([(i, i) for i in range(3)])` to `{i: i for i in /// range(3)}`. pub(crate) fn fix_unnecessary_list_comprehension_dict( @@ -544,7 +505,7 @@ pub(crate) fn fix_unnecessary_collection_call(expr: &Expr, checker: &Checker) -> /// However, this is a syntax error under the f-string grammar. As such, /// this method will pad the start and end of an expression as needed to /// avoid producing invalid syntax. -fn pad_expression( +pub(crate) fn pad_expression( content: String, range: TextRange, locator: &Locator, @@ -575,6 +536,48 @@ fn pad_expression( } } +/// Like [`pad_expression`], but only pads the start of the expression. +pub(crate) fn pad_start( + content: &str, + range: TextRange, + locator: &Locator, + semantic: &SemanticModel, +) -> String { + if !semantic.in_f_string() { + return content.into(); + } + + // If the expression is immediately preceded by an opening brace, then + // we need to add a space before the expression. + let prefix = locator.up_to(range.start()); + if matches!(prefix.chars().next_back(), Some('{')) { + format!(" {content}") + } else { + content.into() + } +} + +/// Like [`pad_expression`], but only pads the end of the expression. +pub(crate) fn pad_end( + content: &str, + range: TextRange, + locator: &Locator, + semantic: &SemanticModel, +) -> String { + if !semantic.in_f_string() { + return content.into(); + } + + // If the expression is immediately preceded by an opening brace, then + // we need to add a space before the expression. + let suffix = locator.after(range.end()); + if matches!(suffix.chars().next(), Some('}')) { + format!("{content} ") + } else { + content.into() + } +} + /// (C409) Convert `tuple([1, 2])` to `tuple(1, 2)` pub(crate) fn fix_unnecessary_literal_within_tuple_call( expr: &Expr, diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs index 6e3eb4f4a3a626..4e21015c9f2220 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs @@ -76,7 +76,7 @@ pub(crate) fn unnecessary_collection_call( { // `dict()` or `dict(a=1)` (as opposed to `dict(**a)`) } - "list" | "tuple" => { + "list" | "tuple" if keywords.is_empty() => { // `list()` or `tuple()` } _ => return, diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs index 3c791385696a1e..2919688f2d83be 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs @@ -1,12 +1,10 @@ -use ruff_python_ast::{Expr, Keyword}; - -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_text_size::Ranged; +use ruff_python_ast as ast; +use ruff_text_size::{Ranged, TextSize}; use crate::checkers::ast::Checker; - -use crate::rules::flake8_comprehensions::fixes; +use crate::rules::flake8_comprehensions::fixes::{pad_end, pad_start}; use super::helpers; @@ -45,25 +43,46 @@ impl AlwaysFixableViolation for UnnecessaryListComprehensionSet { } /// C403 (`set([...])`) -pub(crate) fn unnecessary_list_comprehension_set( - checker: &mut Checker, - expr: &Expr, - func: &Expr, - args: &[Expr], - keywords: &[Keyword], -) { - let Some(argument) = - helpers::exactly_one_argument_with_matching_function("set", func, args, keywords) - else { +pub(crate) fn unnecessary_list_comprehension_set(checker: &mut Checker, call: &ast::ExprCall) { + let Some(argument) = helpers::exactly_one_argument_with_matching_function( + "set", + &call.func, + &call.arguments.args, + &call.arguments.keywords, + ) else { return; }; if !checker.semantic().is_builtin("set") { return; } if argument.is_list_comp_expr() { - let mut diagnostic = Diagnostic::new(UnnecessaryListComprehensionSet, expr.range()); + let mut diagnostic = Diagnostic::new(UnnecessaryListComprehensionSet, call.range()); diagnostic.try_set_fix(|| { - fixes::fix_unnecessary_list_comprehension_set(expr, checker).map(Fix::unsafe_edit) + // Replace `set(` with `{`. + let call_start = Edit::replacement( + pad_start("{", call.range(), checker.locator(), checker.semantic()), + call.start(), + call.arguments.start() + TextSize::from(1), + ); + + // Replace `)` with `}`. + let call_end = Edit::replacement( + pad_end("}", call.range(), checker.locator(), checker.semantic()), + call.arguments.end() - TextSize::from(1), + call.end(), + ); + + // Delete the open bracket (`[`). + let argument_start = + Edit::deletion(argument.start(), argument.start() + TextSize::from(1)); + + // Delete the close bracket (`]`). + let argument_end = Edit::deletion(argument.end() - TextSize::from(1), argument.end()); + + Ok(Fix::unsafe_edits( + call_start, + [argument_start, argument_end, call_end], + )) }); checker.diagnostics.push(diagnostic); }