From a6bc4b2e48deb761168a937b091955a08e388fb6 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 4 Feb 2024 18:17:34 -0800 Subject: [PATCH] Remove CST-based fixers for `C405` and `C409` (#9821) --- .../fixtures/flake8_comprehensions/C409.py | 8 + .../src/checkers/ast/analyze/expression.rs | 8 +- .../src/rules/flake8_comprehensions/fixes.rs | 141 +----------------- .../rules/unnecessary_literal_set.rs | 89 ++++++++--- .../unnecessary_literal_within_tuple_call.rs | 54 ++++--- ...8_comprehensions__tests__C409_C409.py.snap | 65 +++++++- 6 files changed, 170 insertions(+), 195 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C409.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C409.py index 01f764c39bd0f..acd98fd930358 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C409.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C409.py @@ -8,3 +8,11 @@ t5 = tuple( (1, 2) ) + +tuple( # comment + [1, 2] +) + +tuple([ # 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 cbe490e95f457..df9f36e533070 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -657,9 +657,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { ); } if checker.enabled(Rule::UnnecessaryLiteralSet) { - flake8_comprehensions::rules::unnecessary_literal_set( - checker, expr, func, args, keywords, - ); + flake8_comprehensions::rules::unnecessary_literal_set(checker, call); } if checker.enabled(Rule::UnnecessaryLiteralDict) { flake8_comprehensions::rules::unnecessary_literal_dict( @@ -677,9 +675,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { ); } if checker.enabled(Rule::UnnecessaryLiteralWithinTupleCall) { - flake8_comprehensions::rules::unnecessary_literal_within_tuple_call( - checker, expr, func, args, keywords, - ); + flake8_comprehensions::rules::unnecessary_literal_within_tuple_call(checker, call); } if checker.enabled(Rule::UnnecessaryLiteralWithinListCall) { flake8_comprehensions::rules::unnecessary_literal_within_list_call(checker, call); diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs index cc0ecee856aa4..65063ddee24ce 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs @@ -6,7 +6,7 @@ use libcst_native::{ Arg, AssignEqual, AssignTargetExpression, Call, Comment, CompFor, Dict, DictComp, DictElement, Element, EmptyLine, Expression, GeneratorExp, LeftCurlyBrace, LeftParen, LeftSquareBracket, List, ListComp, Name, ParenthesizableWhitespace, ParenthesizedNode, ParenthesizedWhitespace, - RightCurlyBrace, RightParen, RightSquareBracket, Set, SetComp, SimpleString, SimpleWhitespace, + RightCurlyBrace, RightParen, RightSquareBracket, SetComp, SimpleString, SimpleWhitespace, TrailingWhitespace, Tuple, }; @@ -145,95 +145,6 @@ pub(crate) fn fix_unnecessary_list_comprehension_dict( )) } -/// Drop a trailing comma from a list of tuple elements. -fn drop_trailing_comma<'a>( - tuple: &Tuple<'a>, -) -> Result<( - Vec>, - ParenthesizableWhitespace<'a>, - ParenthesizableWhitespace<'a>, -)> { - let whitespace_after = tuple - .lpar - .first() - .ok_or_else(|| anyhow::anyhow!("Expected at least one set of parentheses"))? - .whitespace_after - .clone(); - let whitespace_before = tuple - .rpar - .first() - .ok_or_else(|| anyhow::anyhow!("Expected at least one set of parentheses"))? - .whitespace_before - .clone(); - - let mut elements = tuple.elements.clone(); - if elements.len() == 1 { - if let Some(Element::Simple { - value, - comma: Some(..), - .. - }) = elements.last() - { - if whitespace_before == ParenthesizableWhitespace::default() - && whitespace_after == ParenthesizableWhitespace::default() - { - elements[0] = Element::Simple { - value: value.clone(), - comma: None, - }; - } - } - } - - Ok((elements, whitespace_after, whitespace_before)) -} - -/// (C405) Convert `set((1, 2))` to `{1, 2}`. -pub(crate) fn fix_unnecessary_literal_set(expr: &Expr, checker: &Checker) -> Result { - let locator = checker.locator(); - let stylist = checker.stylist(); - - // Expr(Call(List|Tuple)))) -> Expr(Set))) - 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) => drop_trailing_comma(inner)?, - Expression::List(inner) => ( - inner.elements.clone(), - inner.lbracket.whitespace_after.clone(), - inner.rbracket.whitespace_before.clone(), - ), - _ => { - bail!("Expected Expression::Tuple | Expression::List"); - } - }; - - if elements.is_empty() { - call.args = vec![]; - } else { - tree = Expression::Set(Box::new(Set { - elements, - lbrace: LeftCurlyBrace { whitespace_after }, - rbrace: RightCurlyBrace { whitespace_before }, - lpar: vec![], - rpar: vec![], - })); - } - - Ok(Edit::range_replacement( - pad_expression( - tree.codegen_stylist(stylist), - expr.range(), - checker.locator(), - checker.semantic(), - ), - expr.range(), - )) -} - /// (C406) Convert `dict([(1, 2)])` to `{1: 2}`. pub(crate) fn fix_unnecessary_literal_dict(expr: &Expr, checker: &Checker) -> Result { let locator = checker.locator(); @@ -511,56 +422,6 @@ pub(crate) fn pad_end( } } -/// (C409) Convert `tuple([1, 2])` to `tuple(1, 2)` -pub(crate) fn fix_unnecessary_literal_within_tuple_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::Tuple(Box::new(Tuple { - elements: elements.clone(), - lpar: vec![LeftParen { - whitespace_after: whitespace_after.clone(), - }], - rpar: vec![RightParen { - whitespace_before: whitespace_before.clone(), - }], - })); - - 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, diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_set.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_set.rs index 4e3d03aaf5c69..0a952bbcee13d 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_set.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_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::{self as ast, Expr}; +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; @@ -53,16 +51,13 @@ impl AlwaysFixableViolation for UnnecessaryLiteralSet { } /// C405 (`set([1, 2])`) -pub(crate) fn unnecessary_literal_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_literal_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") { @@ -73,13 +68,69 @@ pub(crate) fn unnecessary_literal_set( Expr::Tuple(_) => "tuple", _ => return, }; + let mut diagnostic = Diagnostic::new( UnnecessaryLiteralSet { obj_type: kind.to_string(), }, - expr.range(), + call.range(), ); - diagnostic - .try_set_fix(|| fixes::fix_unnecessary_literal_set(expr, checker).map(Fix::unsafe_edit)); + + // Convert `set((1, 2))` to `{1, 2}`. + diagnostic.set_fix({ + let elts = match &argument { + Expr::List(ast::ExprList { elts, .. }) => elts, + Expr::Tuple(ast::ExprTuple { elts, .. }) => elts, + _ => unreachable!(), + }; + + match elts.as_slice() { + // If the list or tuple is empty, replace the entire call with `set()`. + [] => Fix::unsafe_edit(Edit::range_replacement("set()".to_string(), call.range())), + // If it's a single-element tuple (with no whitespace around it), remove the trailing + // comma. + [elt] + if argument.is_tuple_expr() + // The element must start right after the `(`. + && elt.start() == argument.start() + TextSize::new(1) + // The element must be followed by exactly one comma and a closing `)`. + && elt.end() + TextSize::new(2) == argument.end() => + { + // Replace from the start of the call to the start of the inner element. + let call_start = Edit::replacement( + pad_start("{", call.range(), checker.locator(), checker.semantic()), + call.start(), + elt.start(), + ); + + // Replace from the end of the inner element to the end of the call with `}`. + let call_end = Edit::replacement( + pad_end("}", call.range(), checker.locator(), checker.semantic()), + elt.end(), + call.end(), + ); + + Fix::unsafe_edits(call_start, [call_end]) + } + _ => { + // Replace from the start of the call to the start of the inner list or tuple with `{`. + let call_start = Edit::replacement( + pad_start("{", call.range(), checker.locator(), checker.semantic()), + call.start(), + argument.start() + TextSize::from(1), + ); + + // Replace from the end of the inner list or tuple to the end of the call with `}`. + let call_end = Edit::replacement( + pad_end("}", call.range(), checker.locator(), checker.semantic()), + argument.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_literal_within_tuple_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.rs index c3ca088c27c8e..daa094ed485b4 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_tuple_call.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::{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 @@ -48,13 +45,11 @@ impl AlwaysFixableViolation for UnnecessaryLiteralWithinTupleCall { let UnnecessaryLiteralWithinTupleCall { literal } = self; if literal == "list" { format!( - "Unnecessary `{literal}` literal passed to `tuple()` (rewrite as a `tuple` \ - literal)" + "Unnecessary `{literal}` literal passed to `tuple()` (rewrite as a `tuple` literal)" ) } else { format!( - "Unnecessary `{literal}` literal passed to `tuple()` (remove the outer call to \ - `tuple()`)" + "Unnecessary `{literal}` literal passed to `tuple()` (remove the outer call to `tuple()`)" ) } } @@ -72,17 +67,13 @@ impl AlwaysFixableViolation for UnnecessaryLiteralWithinTupleCall { } /// C409 -pub(crate) fn unnecessary_literal_within_tuple_call( - checker: &mut Checker, - expr: &Expr, - func: &Expr, - args: &[Expr], - keywords: &[Keyword], -) { - if !keywords.is_empty() { +pub(crate) fn unnecessary_literal_within_tuple_call(checker: &mut Checker, call: &ast::ExprCall) { + if !call.arguments.keywords.is_empty() { return; } - let Some(argument) = helpers::first_argument_with_matching_function("tuple", func, args) else { + let Some(argument) = + helpers::first_argument_with_matching_function("tuple", &call.func, &call.arguments.args) + else { return; }; if !checker.semantic().is_builtin("tuple") { @@ -93,15 +84,32 @@ pub(crate) fn unnecessary_literal_within_tuple_call( Expr::List(_) => "list", _ => return, }; + let mut diagnostic = Diagnostic::new( UnnecessaryLiteralWithinTupleCall { literal: argument_kind.to_string(), }, - expr.range(), + call.range(), ); - diagnostic.try_set_fix(|| { - fixes::fix_unnecessary_literal_within_tuple_call(expr, checker.locator(), checker.stylist()) - .map(Fix::unsafe_edit) + + // Convert `tuple([1, 2])` to `tuple(1, 2)` + diagnostic.set_fix({ + // Replace from the start of the call to the start of the inner list or tuple with `(`. + let call_start = Edit::replacement( + "(".to_string(), + call.start(), + argument.start() + TextSize::from(1), + ); + + // Replace from the end of the inner list or tuple to the end of the call with `)`. + let call_end = Edit::replacement( + ")".to_string(), + argument.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/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap index c4531ac6d9648..45726f1f3aed2 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap @@ -93,16 +93,67 @@ C409.py:8:6: C409 [*] Unnecessary `tuple` literal passed to `tuple()` (remove th 9 | | (1, 2) 10 | | ) | |_^ C409 +11 | +12 | tuple( # comment | = help: Remove outer `tuple` call ℹ Unsafe fix -5 5 | 1, -6 6 | 2 -7 7 | ]) -8 |-t5 = tuple( -9 |- (1, 2) -10 |-) - 8 |+t5 = (1, 2) +5 5 | 1, +6 6 | 2 +7 7 | ]) +8 |-t5 = tuple( +9 |- (1, 2) +10 |-) + 8 |+t5 = (1, 2) +11 9 | +12 10 | tuple( # comment +13 11 | [1, 2] + +C409.py:12:1: C409 [*] Unnecessary `list` literal passed to `tuple()` (rewrite as a `tuple` literal) + | +10 | ) +11 | +12 | / tuple( # comment +13 | | [1, 2] +14 | | ) + | |_^ C409 +15 | +16 | tuple([ # comment + | + = help: Rewrite as a `tuple` literal + +ℹ Unsafe fix +9 9 | (1, 2) +10 10 | ) +11 11 | +12 |-tuple( # comment +13 |- [1, 2] +14 |-) + 12 |+(1, 2) +15 13 | +16 14 | tuple([ # comment +17 15 | 1, 2 + +C409.py:16:1: C409 [*] Unnecessary `list` literal passed to `tuple()` (rewrite as a `tuple` literal) + | +14 | ) +15 | +16 | / tuple([ # comment +17 | | 1, 2 +18 | | ]) + | |__^ C409 + | + = help: Rewrite as a `tuple` literal + +ℹ Unsafe fix +13 13 | [1, 2] +14 14 | ) +15 15 | +16 |-tuple([ # comment + 16 |+( # comment +17 17 | 1, 2 +18 |-]) + 18 |+)