Remove CST-based fixers for C405 and C409 (#9821)

crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C409.py

@@ -8,3 +8,11 @@ t4 = tuple([
 t5 = tuple(
     (1, 2)
 )
+
+tuple(  # comment
+    [1, 2]
+)
+
+tuple([  # comment
+    1, 2
+])

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(
+                flake8_comprehensions::rules::unnecessary_literal_set(checker, call);
-                    checker, expr, func, args, keywords,
-                );
             }
             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(
+                flake8_comprehensions::rules::unnecessary_literal_within_tuple_call(checker, call);
-                    checker, expr, func, args, keywords,
-                );
             }
             if checker.enabled(Rule::UnnecessaryLiteralWithinListCall) {
                 flake8_comprehensions::rules::unnecessary_literal_within_list_call(checker, call);

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<Element<'a>>,
-    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<Edit> {
-    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<Edit> {
     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<Edit> {
-    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,

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, Edit, Fix};
-
-use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, 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::{pad_end, pad_start};
-use crate::rules::flake8_comprehensions::fixes;
 
 use super::helpers;
 
@@ -53,16 +51,13 @@ impl AlwaysFixableViolation for UnnecessaryLiteralSet {
 }
 
 /// C405 (`set([1, 2])`)
-pub(crate) fn unnecessary_literal_set(
+pub(crate) fn unnecessary_literal_set(checker: &mut Checker, call: &ast::ExprCall) {
-    checker: &mut Checker,
+    let Some(argument) = helpers::exactly_one_argument_with_matching_function(
-    expr: &Expr,
+        "set",
-    func: &Expr,
+        &call.func,
-    args: &[Expr],
+        &call.arguments.args,
-    keywords: &[Keyword],
+        &call.arguments.keywords,
-) {
+    ) else {
-    let Some(argument) =
-        helpers::exactly_one_argument_with_matching_function("set", func, args, 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);
 }

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, Edit, Fix};
-
-use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, 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` \
+                "Unnecessary `{literal}` literal passed to `tuple()` (rewrite as a `tuple` literal)"
-                 literal)"
             )
         } else {
             format!(
-                "Unnecessary `{literal}` literal passed to `tuple()` (remove the outer call to \
+                "Unnecessary `{literal}` literal passed to `tuple()` (remove the outer call to `tuple()`)"
-                 `tuple()`)"
             )
         }
     }
@@ -72,17 +67,13 @@ impl AlwaysFixableViolation for UnnecessaryLiteralWithinTupleCall {
 }
 
 /// C409
-pub(crate) fn unnecessary_literal_within_tuple_call(
+pub(crate) fn unnecessary_literal_within_tuple_call(checker: &mut Checker, call: &ast::ExprCall) {
-    checker: &mut Checker,
+    if !call.arguments.keywords.is_empty() {
-    expr: &Expr,
-    func: &Expr,
-    args: &[Expr],
-    keywords: &[Keyword],
-) {
-    if !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())
+    // Convert `tuple([1, 2])` to `tuple(1, 2)`
-            .map(Fix::unsafe_edit)
+    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);
 }

crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C409_C409.py.snap

@@ -93,6 +93,8 @@ 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
 
@@ -104,5 +106,54 @@ C409.py:8:6: C409 [*] Unnecessary `tuple` literal passed to `tuple()` (remove th
 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 |+)