Remove CST-based fixers for C400, C401, C410, and C418 (#9819)

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

@@ -2,3 +2,12 @@ l1 = list([1, 2])
 l2 = list((1, 2))
 l3 = list([])
 l4 = list(())
+
+
+list(  # comment
+    [1, 2]
+)
+
+list([  # comment
+    1, 2
+])

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(
+                flake8_comprehensions::rules::unnecessary_generator_list(checker, call);
-                    checker, expr, func, args, keywords,
-                );
             }
             if checker.enabled(Rule::UnnecessaryGeneratorSet) {
-                flake8_comprehensions::rules::unnecessary_generator_set(
+                flake8_comprehensions::rules::unnecessary_generator_set(checker, call);
-                    checker, expr, func, args, keywords,
-                );
             }
             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(
+                flake8_comprehensions::rules::unnecessary_literal_within_list_call(checker, call);
-                    checker, expr, func, args, keywords,
-                );
             }
             if checker.enabled(Rule::UnnecessaryLiteralWithinDictCall) {
-                flake8_comprehensions::rules::unnecessary_literal_within_dict_call(
+                flake8_comprehensions::rules::unnecessary_literal_within_dict_call(checker, call);
-                    checker, expr, func, args, keywords,
-                );
             }
             if checker.enabled(Rule::UnnecessaryListCall) {
                 flake8_comprehensions::rules::unnecessary_list_call(checker, expr, func, args);

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<Edit> {
-    // 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<Edit> {
-    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<Edit> {
@@ -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<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::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<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)?;
-
-    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,

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, Edit, Fix};
-
-use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, 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(
+pub(crate) fn unnecessary_generator_list(checker: &mut Checker, call: &ast::ExprCall) {
-    checker: &mut Checker,
+    let Some(argument) = helpers::exactly_one_argument_with_matching_function(
-    expr: &Expr,
+        "list",
-    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("list", func, args, keywords)
-    else {
         return;
     };
     if !checker.semantic().is_builtin("list") {
         return;
     }
-    if let Expr::GeneratorExp(_) = argument {
+    if argument.is_generator_exp_expr() {
-        let mut diagnostic = Diagnostic::new(UnnecessaryGeneratorList, expr.range());
+        let mut diagnostic = Diagnostic::new(UnnecessaryGeneratorList, call.range());
-        diagnostic.try_set_fix(|| {
+
-            fixes::fix_unnecessary_generator_list(expr, checker.locator(), checker.stylist())
+        // Convert `list(x for x in y)` to `[x for x in y]`.
-                .map(Fix::unsafe_edit)
+        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);
     }
 }

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, Edit, Fix};
-
-use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, 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::{pad_end, pad_start};
-use crate::rules::flake8_comprehensions::fixes;
 
 use super::helpers;
 
@@ -47,26 +45,40 @@ impl AlwaysFixableViolation for UnnecessaryGeneratorSet {
 }
 
 /// C401 (`set(generator)`)
-pub(crate) fn unnecessary_generator_set(
+pub(crate) fn unnecessary_generator_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") {
         return;
     }
-    if let Expr::GeneratorExp(_) = argument {
+    if argument.is_generator_exp_expr() {
-        let mut diagnostic = Diagnostic::new(UnnecessaryGeneratorSet, expr.range());
+        let mut diagnostic = Diagnostic::new(UnnecessaryGeneratorSet, call.range());
-        diagnostic.try_set_fix(|| {
+
-            fixes::fix_unnecessary_generator_set(expr, checker).map(Fix::unsafe_edit)
+        // 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);
     }
 }

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(
+            Fix::unsafe_edits(call_start, [argument_start, argument_end, call_end])
-                call_start,
-                [argument_start, argument_end, call_end],
-            ))
         });
         checker.diagnostics.push(diagnostic);
     }

crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_within_dict_call.rs

@@ -1,15 +1,13 @@
 use std::fmt;
 
-use ruff_python_ast::{Expr, Keyword};
+use ruff_python_ast::{self as ast, Expr};
 
-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 crate::checkers::ast::Checker;
 
-use crate::rules::flake8_comprehensions::fixes;
-
 use super::helpers;
 
 #[derive(Debug, PartialEq, Eq)]
@@ -68,17 +66,13 @@ impl AlwaysFixableViolation for UnnecessaryLiteralWithinDictCall {
 }
 
 /// C418
-pub(crate) fn unnecessary_literal_within_dict_call(
+pub(crate) fn unnecessary_literal_within_dict_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("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 +83,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())
+    // Convert `dict({"a": 1})` to `{"a": 1}`
-            .map(Fix::unsafe_edit)
+    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);
 }

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_python_ast::{self as ast, Expr};
-use ruff_text_size::Ranged;
+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(
+pub(crate) fn unnecessary_literal_within_list_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("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())
+    // Convert `list([1, 2])` to `[1, 2]`
-            .map(Fix::unsafe_edit)
+    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);
 }

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 |+]