diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C410.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C410.py index 4fbed5b5ce6acd..6e6c977bb6472e 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C410.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C410.py @@ -2,3 +2,12 @@ l2 = list((1, 2)) l3 = list([]) l4 = list(()) + + +list( # comment + [1, 2] +) + +list([ # comment + 1, 2 +]) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 7329b9b9bf333a..cbe490e95f4579 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -638,14 +638,10 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { flake8_bandit::rules::tarfile_unsafe_members(checker, call); } if checker.enabled(Rule::UnnecessaryGeneratorList) { - flake8_comprehensions::rules::unnecessary_generator_list( - checker, expr, func, args, keywords, - ); + flake8_comprehensions::rules::unnecessary_generator_list(checker, call); } if checker.enabled(Rule::UnnecessaryGeneratorSet) { - flake8_comprehensions::rules::unnecessary_generator_set( - checker, expr, func, args, keywords, - ); + flake8_comprehensions::rules::unnecessary_generator_set(checker, call); } if checker.enabled(Rule::UnnecessaryGeneratorDict) { flake8_comprehensions::rules::unnecessary_generator_dict( @@ -686,14 +682,10 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { ); } if checker.enabled(Rule::UnnecessaryLiteralWithinListCall) { - flake8_comprehensions::rules::unnecessary_literal_within_list_call( - checker, expr, func, args, keywords, - ); + flake8_comprehensions::rules::unnecessary_literal_within_list_call(checker, call); } if checker.enabled(Rule::UnnecessaryLiteralWithinDictCall) { - flake8_comprehensions::rules::unnecessary_literal_within_dict_call( - checker, expr, func, args, keywords, - ); + flake8_comprehensions::rules::unnecessary_literal_within_dict_call(checker, call); } if checker.enabled(Rule::UnnecessaryListCall) { flake8_comprehensions::rules::unnecessary_list_call(checker, expr, func, args); diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs index aebd3e6b158259..cc0ecee856aa43 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs @@ -29,73 +29,6 @@ use crate::{ }, }; -/// (C400) Convert `list(x for x in y)` to `[x for x in y]`. -pub(crate) fn fix_unnecessary_generator_list( - expr: &Expr, - locator: &Locator, - stylist: &Stylist, -) -> Result { - // Expr(Call(GeneratorExp)))) -> Expr(ListComp))) - 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 generator_exp = match_generator_exp(&arg.value)?; - - tree = Expression::ListComp(Box::new(ListComp { - elt: generator_exp.elt.clone(), - for_in: generator_exp.for_in.clone(), - lbracket: LeftSquareBracket { - whitespace_after: call.whitespace_before_args.clone(), - }, - rbracket: RightSquareBracket { - whitespace_before: arg.whitespace_after_arg.clone(), - }, - lpar: generator_exp.lpar.clone(), - rpar: generator_exp.rpar.clone(), - })); - - Ok(Edit::range_replacement( - tree.codegen_stylist(stylist), - expr.range(), - )) -} - -/// (C401) Convert `set(x for x in y)` to `{x for x in y}`. -pub(crate) fn fix_unnecessary_generator_set(expr: &Expr, checker: &Checker) -> Result { - let locator = checker.locator(); - let stylist = checker.stylist(); - - // Expr(Call(GeneratorExp)))) -> 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 generator_exp = match_generator_exp(&arg.value)?; - - tree = Expression::SetComp(Box::new(SetComp { - elt: generator_exp.elt.clone(), - for_in: generator_exp.for_in.clone(), - lbrace: LeftCurlyBrace { - whitespace_after: call.whitespace_before_args.clone(), - }, - rbrace: RightCurlyBrace { - whitespace_before: arg.whitespace_after_arg.clone(), - }, - lpar: generator_exp.lpar.clone(), - rpar: generator_exp.rpar.clone(), - })); - - let content = tree.codegen_stylist(stylist); - - Ok(Edit::range_replacement( - pad_expression(content, expr.range(), checker.locator(), checker.semantic()), - expr.range(), - )) -} - /// (C402) Convert `dict((x, x) for x in range(3))` to `{x: x for x in /// range(3)}`. pub(crate) fn fix_unnecessary_generator_dict(expr: &Expr, checker: &Checker) -> Result { @@ -628,58 +561,6 @@ pub(crate) fn fix_unnecessary_literal_within_tuple_call( )) } -/// (C410) Convert `list([1, 2])` to `[1, 2]` -pub(crate) fn fix_unnecessary_literal_within_list_call( - expr: &Expr, - locator: &Locator, - stylist: &Stylist, -) -> Result { - 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 (elements, whitespace_after, whitespace_before) = match &arg.value { - Expression::Tuple(inner) => ( - &inner.elements, - &inner - .lpar - .first() - .ok_or_else(|| anyhow::anyhow!("Expected at least one set of parentheses"))? - .whitespace_after, - &inner - .rpar - .first() - .ok_or_else(|| anyhow::anyhow!("Expected at least one set of parentheses"))? - .whitespace_before, - ), - Expression::List(inner) => ( - &inner.elements, - &inner.lbracket.whitespace_after, - &inner.rbracket.whitespace_before, - ), - _ => { - bail!("Expected Expression::Tuple | Expression::List"); - } - }; - - tree = Expression::List(Box::new(List { - elements: elements.clone(), - lbracket: LeftSquareBracket { - whitespace_after: whitespace_after.clone(), - }, - rbracket: RightSquareBracket { - whitespace_before: whitespace_before.clone(), - }, - lpar: vec![], - rpar: vec![], - })); - - Ok(Edit::range_replacement( - tree.codegen_stylist(stylist), - expr.range(), - )) -} - /// (C411) Convert `list([i * i for i in x])` to `[i * i for i in x]`. pub(crate) fn fix_unnecessary_list_call( expr: &Expr, @@ -1094,25 +975,6 @@ pub(crate) fn fix_unnecessary_map( Ok(Edit::range_replacement(content, expr.range())) } -/// (C418) Convert `dict({"a": 1})` to `{"a": 1}` -pub(crate) fn fix_unnecessary_literal_within_dict_call( - expr: &Expr, - locator: &Locator, - stylist: &Stylist, -) -> Result { - 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)?; - - tree = arg.value.clone(); - - Ok(Edit::range_replacement( - tree.codegen_stylist(stylist), - expr.range(), - )) -} - /// (C419) Convert `[i for i in a]` into `i for i in a` pub(crate) fn fix_unnecessary_comprehension_any_all( expr: &Expr, diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs index 526fe4970a1e75..e9d89a19403f9e 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs @@ -1,13 +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 super::helpers; /// ## What it does @@ -47,27 +44,40 @@ impl AlwaysFixableViolation for UnnecessaryGeneratorList { } /// C400 (`list(generator)`) -pub(crate) fn unnecessary_generator_list( - checker: &mut Checker, - expr: &Expr, - func: &Expr, - args: &[Expr], - keywords: &[Keyword], -) { - let Some(argument) = - helpers::exactly_one_argument_with_matching_function("list", func, args, keywords) - else { +pub(crate) fn unnecessary_generator_list(checker: &mut Checker, call: &ast::ExprCall) { + let Some(argument) = helpers::exactly_one_argument_with_matching_function( + "list", + &call.func, + &call.arguments.args, + &call.arguments.keywords, + ) else { return; }; if !checker.semantic().is_builtin("list") { return; } - if let Expr::GeneratorExp(_) = argument { - let mut diagnostic = Diagnostic::new(UnnecessaryGeneratorList, expr.range()); - diagnostic.try_set_fix(|| { - fixes::fix_unnecessary_generator_list(expr, checker.locator(), checker.stylist()) - .map(Fix::unsafe_edit) + if argument.is_generator_exp_expr() { + let mut diagnostic = Diagnostic::new(UnnecessaryGeneratorList, call.range()); + + // Convert `list(x for x in y)` to `[x for x in y]`. + diagnostic.set_fix({ + // Replace `list(` with `[`. + let call_start = Edit::replacement( + "[".to_string(), + call.start(), + call.arguments.start() + TextSize::from(1), + ); + + // Replace `)` with `]`. + let call_end = Edit::replacement( + "]".to_string(), + call.arguments.end() - TextSize::from(1), + call.end(), + ); + + Fix::unsafe_edits(call_start, [call_end]) }); + checker.diagnostics.push(diagnostic); } } diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_set.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_set.rs index f2cdbf86453a03..3fdb3efe86fb07 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_set.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_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; @@ -47,26 +45,40 @@ impl AlwaysFixableViolation for UnnecessaryGeneratorSet { } /// C401 (`set(generator)`) -pub(crate) fn unnecessary_generator_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_generator_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 let Expr::GeneratorExp(_) = argument { - let mut diagnostic = Diagnostic::new(UnnecessaryGeneratorSet, expr.range()); - diagnostic.try_set_fix(|| { - fixes::fix_unnecessary_generator_set(expr, checker).map(Fix::unsafe_edit) + if argument.is_generator_exp_expr() { + let mut diagnostic = Diagnostic::new(UnnecessaryGeneratorSet, call.range()); + + // Convert `set(x for x in y)` to `{x for x in y}`. + diagnostic.set_fix({ + // 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(), + ); + + Fix::unsafe_edits(call_start, [call_end]) }); + checker.diagnostics.push(diagnostic); } } 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 2919688f2d83be..6b6aab187eaef1 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 @@ -57,7 +57,7 @@ pub(crate) fn unnecessary_list_comprehension_set(checker: &mut Checker, call: &a } if argument.is_list_comp_expr() { let mut diagnostic = Diagnostic::new(UnnecessaryListComprehensionSet, call.range()); - diagnostic.try_set_fix(|| { + diagnostic.set_fix({ // Replace `set(` with `{`. let call_start = Edit::replacement( pad_start("{", call.range(), checker.locator(), checker.semantic()), @@ -79,10 +79,7 @@ pub(crate) fn unnecessary_list_comprehension_set(checker: &mut Checker, call: &a // 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], - )) + Fix::unsafe_edits(call_start, [argument_start, argument_end, call_end]) }); checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_dict_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_dict_call.rs index b407e9c0efca40..e39a6db24a9f09 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_dict_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_dict_call.rs @@ -1,8 +1,8 @@ use std::fmt; -use ruff_python_ast::{Expr, Keyword}; +use ruff_python_ast::{self as 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; @@ -68,17 +68,13 @@ impl AlwaysFixableViolation for UnnecessaryLiteralWithinDictCall { } /// C418 -pub(crate) fn unnecessary_literal_within_dict_call( - checker: &mut Checker, - expr: &Expr, - func: &Expr, - args: &[Expr], - keywords: &[Keyword], -) { - if !keywords.is_empty() { +pub(crate) fn unnecessary_literal_within_dict_call(checker: &mut Checker, call: &ast::ExprCall) { + if !call.arguments.keywords.is_empty() { return; } - let Some(argument) = helpers::first_argument_with_matching_function("dict", func, args) else { + let Some(argument) = + helpers::first_argument_with_matching_function("dict", &call.func, &call.arguments.args) + else { return; }; if !checker.semantic().is_builtin("dict") { @@ -89,15 +85,24 @@ pub(crate) fn unnecessary_literal_within_dict_call( Expr::Dict(_) => DictKind::Literal, _ => return, }; + let mut diagnostic = Diagnostic::new( UnnecessaryLiteralWithinDictCall { kind: argument_kind, }, - expr.range(), + call.range(), ); - diagnostic.try_set_fix(|| { - fixes::fix_unnecessary_literal_within_dict_call(expr, checker.locator(), checker.stylist()) - .map(Fix::unsafe_edit) + + // Convert `dict({"a": 1})` to `{"a": 1}` + diagnostic.set_fix({ + // Delete from the start of the call to the start of the argument. + let call_start = Edit::deletion(call.start(), argument.start()); + + // Delete from the end of the argument to the end of the call. + let call_end = Edit::deletion(argument.end(), call.end()); + + Fix::unsafe_edits(call_start, [call_end]) }); + checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs index cbf02452d28859..31d2c2f4e40504 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_list_call.rs @@ -1,12 +1,10 @@ -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{Expr, Keyword}; -use ruff_text_size::Ranged; +use ruff_python_ast::{self as ast, Expr}; +use ruff_text_size::{Ranged, TextSize}; use crate::checkers::ast::Checker; -use crate::rules::flake8_comprehensions::fixes; - use super::helpers; /// ## What it does @@ -70,17 +68,13 @@ impl AlwaysFixableViolation for UnnecessaryLiteralWithinListCall { } /// C410 -pub(crate) fn unnecessary_literal_within_list_call( - checker: &mut Checker, - expr: &Expr, - func: &Expr, - args: &[Expr], - keywords: &[Keyword], -) { - if !keywords.is_empty() { +pub(crate) fn unnecessary_literal_within_list_call(checker: &mut Checker, call: &ast::ExprCall) { + if !call.arguments.keywords.is_empty() { return; } - let Some(argument) = helpers::first_argument_with_matching_function("list", func, args) else { + let Some(argument) = + helpers::first_argument_with_matching_function("list", &call.func, &call.arguments.args) + else { return; }; if !checker.semantic().is_builtin("list") { @@ -91,15 +85,43 @@ pub(crate) fn unnecessary_literal_within_list_call( Expr::List(_) => "list", _ => return, }; + let mut diagnostic = Diagnostic::new( UnnecessaryLiteralWithinListCall { literal: argument_kind.to_string(), }, - expr.range(), + call.range(), ); - diagnostic.try_set_fix(|| { - fixes::fix_unnecessary_literal_within_list_call(expr, checker.locator(), checker.stylist()) - .map(Fix::unsafe_edit) + + // Convert `list([1, 2])` to `[1, 2]` + diagnostic.set_fix({ + // Delete from the start of the call to the start of the argument. + let call_start = Edit::deletion(call.start(), argument.start()); + + // Delete from the end of the argument to the end of the call. + let call_end = Edit::deletion(argument.end(), call.end()); + + // If this is a tuple, we also need to convert the inner argument to a list. + if argument.is_tuple_expr() { + // Replace `(` with `[`. + let argument_start = Edit::replacement( + "[".to_string(), + argument.start(), + argument.start() + TextSize::from(1), + ); + + // Replace `)` with `]`. + let argument_end = Edit::replacement( + "]".to_string(), + argument.end() - TextSize::from(1), + argument.end(), + ); + + Fix::unsafe_edits(call_start, [argument_start, argument_end, call_end]) + } else { + Fix::unsafe_edits(call_start, [call_end]) + } }); + checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C410_C410.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C410_C410.py.snap index 360ce977b01bfa..921547ca2dedc6 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C410_C410.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C410_C410.py.snap @@ -33,6 +33,7 @@ C410.py:2:6: C410 [*] Unnecessary `tuple` literal passed to `list()` (rewrite as 2 |+l2 = [1, 2] 3 3 | l3 = list([]) 4 4 | l4 = list(()) +5 5 | C410.py:3:6: C410 [*] Unnecessary `list` literal passed to `list()` (remove the outer call to `list()`) | @@ -50,6 +51,8 @@ C410.py:3:6: C410 [*] Unnecessary `list` literal passed to `list()` (remove the 3 |-l3 = list([]) 3 |+l3 = [] 4 4 | l4 = list(()) +5 5 | +6 6 | C410.py:4:6: C410 [*] Unnecessary `tuple` literal passed to `list()` (rewrite as a `list` literal) | @@ -66,5 +69,52 @@ C410.py:4:6: C410 [*] Unnecessary `tuple` literal passed to `list()` (rewrite as 3 3 | l3 = list([]) 4 |-l4 = list(()) 4 |+l4 = [] +5 5 | +6 6 | +7 7 | list( # comment + +C410.py:7:1: C410 [*] Unnecessary `list` literal passed to `list()` (remove the outer call to `list()`) + | + 7 | / list( # comment + 8 | | [1, 2] + 9 | | ) + | |_^ C410 +10 | +11 | list([ # comment + | + = help: Remove outer `list` call + +ℹ Unsafe fix +4 4 | l4 = list(()) +5 5 | +6 6 | +7 |-list( # comment +8 |- [1, 2] +9 |-) + 7 |+[1, 2] +10 8 | +11 9 | list([ # comment +12 10 | 1, 2 + +C410.py:11:1: C410 [*] Unnecessary `list` literal passed to `list()` (remove the outer call to `list()`) + | + 9 | ) +10 | +11 | / list([ # comment +12 | | 1, 2 +13 | | ]) + | |__^ C410 + | + = help: Remove outer `list` call + +ℹ Unsafe fix +8 8 | [1, 2] +9 9 | ) +10 10 | +11 |-list([ # comment + 11 |+[ # comment +12 12 | 1, 2 +13 |-]) + 13 |+]