Remove CST-based fixer for C408 (#9822)

## Summary

We have to keep the fixer for a specific case: `dict` calls that include
keyword-argument members.

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

@@ -20,3 +20,10 @@ f"{dict(x='y') | dict(y='z')}"
 f"{ dict(x='y') | dict(y='z') }"
 f"a {dict(x='y') | dict(y='z')} b"
 f"a { dict(x='y') | dict(y='z') } b"
+
+dict(
+    # comment
+)
+
+tuple(  # comment
+)

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

@@ -667,10 +667,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
             if checker.enabled(Rule::UnnecessaryCollectionCall) {
                 flake8_comprehensions::rules::unnecessary_collection_call(
                     checker,
-                    expr,
+                    call,
-                    func,
-                    args,
-                    keywords,
                     &checker.settings.flake8_comprehensions,
                 );
             }

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

@@ -5,13 +5,13 @@ use itertools::Itertools;
 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,
+    ListComp, Name, ParenthesizableWhitespace, ParenthesizedNode, ParenthesizedWhitespace,
     RightCurlyBrace, RightParen, RightSquareBracket, SetComp, SimpleString, SimpleWhitespace,
     TrailingWhitespace, Tuple,
 };
 
 use ruff_diagnostics::{Edit, Fix};
-use ruff_python_ast::Expr;
+use ruff_python_ast::{self as ast, Expr};
 use ruff_python_codegen::Stylist;
 use ruff_python_semantic::SemanticModel;
 use ruff_source_file::Locator;
@@ -25,7 +25,7 @@ use crate::{
     checkers::ast::Checker,
     cst::matchers::{
         match_arg, match_call, match_call_mut, match_expression, match_generator_exp, match_lambda,
-        match_list_comp, match_name, match_tuple,
+        match_list_comp, match_tuple,
     },
 };
 
@@ -213,126 +213,82 @@ pub(crate) fn fix_unnecessary_literal_dict(expr: &Expr, checker: &Checker) -> Re
     ))
 }
 
-/// (C408)
+/// (C408) Convert `dict(a=1, b=2)` to `{"a": 1, "b": 2}`.
-pub(crate) fn fix_unnecessary_collection_call(expr: &Expr, checker: &Checker) -> Result<Edit> {
+pub(crate) fn fix_unnecessary_collection_call(
-    enum Collection {
+    expr: &ast::ExprCall,
-        Tuple,
+    checker: &Checker,
-        List,
+) -> Result<Edit> {
-        Dict,
-    }
-
     let locator = checker.locator();
     let stylist = checker.stylist();
 
-    // Expr(Call("list" | "tuple" | "dict")))) -> Expr(List|Tuple|Dict)
+    // Expr(Call("dict")))) -> Expr(Dict)
     let module_text = locator.slice(expr);
     let mut tree = match_expression(module_text)?;
     let call = match_call(&tree)?;
-    let name = match_name(&call.func)?;
-    let collection = match name.value {
-        "tuple" => Collection::Tuple,
-        "list" => Collection::List,
-        "dict" => Collection::Dict,
-        _ => bail!("Expected 'tuple', 'list', or 'dict'"),
-    };
 
     // Arena allocator used to create formatted strings of sufficient lifetime,
     // below.
     let mut arena: Vec<String> = vec![];
 
-    match collection {
+    let quote = checker.f_string_quote_style().unwrap_or(stylist.quote());
-        Collection::Tuple => {
+
-            tree = Expression::Tuple(Box::new(Tuple {
+    // Quote each argument.
-                elements: vec![],
+    for arg in &call.args {
-                lpar: vec![LeftParen::default()],
+        let quoted = format!(
-                rpar: vec![RightParen::default()],
+            "{}{}{}",
-            }));
+            quote,
-        }
+            arg.keyword
-        Collection::List => {
+                .as_ref()
-            tree = Expression::List(Box::new(List {
+                .expect("Expected dictionary argument to be kwarg")
-                elements: vec![],
+                .value,
-                lbracket: LeftSquareBracket::default(),
+            quote,
-                rbracket: RightSquareBracket::default(),
+        );
+        arena.push(quoted);
+    }
+
+    let elements = call
+        .args
+        .iter()
+        .enumerate()
+        .map(|(i, arg)| DictElement::Simple {
+            key: Expression::SimpleString(Box::new(SimpleString {
+                value: &arena[i],
                 lpar: vec![],
                 rpar: vec![],
-            }));
+            })),
-        }
+            value: arg.value.clone(),
-        Collection::Dict => {
+            comma: arg.comma.clone(),
-            if call.args.is_empty() {
+            whitespace_before_colon: ParenthesizableWhitespace::default(),
-                tree = Expression::Dict(Box::new(Dict {
+            whitespace_after_colon: ParenthesizableWhitespace::SimpleWhitespace(SimpleWhitespace(
-                    elements: vec![],
+                " ",
-                    lbrace: LeftCurlyBrace::default(),
+            )),
-                    rbrace: RightCurlyBrace::default(),
+        })
-                    lpar: vec![],
+        .collect();
-                    rpar: vec![],
-                }));
-            } else {
-                let quote = checker.f_string_quote_style().unwrap_or(stylist.quote());
 
-                // Quote each argument.
+    tree = Expression::Dict(Box::new(Dict {
-                for arg in &call.args {
+        elements,
-                    let quoted = format!(
+        lbrace: LeftCurlyBrace {
-                        "{}{}{}",
+            whitespace_after: call.whitespace_before_args.clone(),
-                        quote,
+        },
-                        arg.keyword
+        rbrace: RightCurlyBrace {
-                            .as_ref()
+            whitespace_before: call
-                            .expect("Expected dictionary argument to be kwarg")
+                .args
-                            .value,
+                .last()
-                        quote,
+                .expect("Arguments should be non-empty")
-                    );
+                .whitespace_after_arg
-                    arena.push(quoted);
+                .clone(),
-                }
+        },
-
+        lpar: vec![],
-                let elements = call
+        rpar: vec![],
-                    .args
+    }));
-                    .iter()
-                    .enumerate()
-                    .map(|(i, arg)| DictElement::Simple {
-                        key: Expression::SimpleString(Box::new(SimpleString {
-                            value: &arena[i],
-                            lpar: vec![],
-                            rpar: vec![],
-                        })),
-                        value: arg.value.clone(),
-                        comma: arg.comma.clone(),
-                        whitespace_before_colon: ParenthesizableWhitespace::default(),
-                        whitespace_after_colon: ParenthesizableWhitespace::SimpleWhitespace(
-                            SimpleWhitespace(" "),
-                        ),
-                    })
-                    .collect();
-
-                tree = Expression::Dict(Box::new(Dict {
-                    elements,
-                    lbrace: LeftCurlyBrace {
-                        whitespace_after: call.whitespace_before_args.clone(),
-                    },
-                    rbrace: RightCurlyBrace {
-                        whitespace_before: call
-                            .args
-                            .last()
-                            .expect("Arguments should be non-empty")
-                            .whitespace_after_arg
-                            .clone(),
-                    },
-                    lpar: vec![],
-                    rpar: vec![],
-                }));
-            }
-        }
-    };
 
     Ok(Edit::range_replacement(
-        if matches!(collection, Collection::Dict) {
+        pad_expression(
-            pad_expression(
+            tree.codegen_stylist(stylist),
-                tree.codegen_stylist(stylist),
+            expr.range(),
-                expr.range(),
+            checker.locator(),
-                checker.locator(),
+            checker.semantic(),
-                checker.semantic(),
+        ),
-            )
-        } else {
-            tree.codegen_stylist(stylist)
-        },
         expr.range(),
     ))
 }

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

@@ -1,11 +1,11 @@
-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 as ast;
-use ruff_text_size::Ranged;
+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 crate::rules::flake8_comprehensions::settings::Settings;
 
 /// ## What it does
@@ -56,42 +56,88 @@ impl AlwaysFixableViolation for UnnecessaryCollectionCall {
 /// C408
 pub(crate) fn unnecessary_collection_call(
     checker: &mut Checker,
-    expr: &Expr,
+    call: &ast::ExprCall,
-    func: &Expr,
-    args: &[Expr],
-    keywords: &[Keyword],
     settings: &Settings,
 ) {
-    if !args.is_empty() {
+    if !call.arguments.args.is_empty() {
         return;
     }
-    let Some(func) = func.as_name_expr() else {
+    let Some(func) = call.func.as_name_expr() else {
         return;
     };
-    match func.id.as_str() {
+    let collection = match func.id.as_str() {
         "dict"
-            if keywords.is_empty()
+            if call.arguments.keywords.is_empty()
                 || (!settings.allow_dict_calls_with_keyword_arguments
-                    && keywords.iter().all(|kw| kw.arg.is_some())) =>
+                    && call.arguments.keywords.iter().all(|kw| kw.arg.is_some())) =>
         {
             // `dict()` or `dict(a=1)` (as opposed to `dict(**a)`)
+            Collection::Dict
         }
-        "list" | "tuple" if keywords.is_empty() => {
+        "list" if call.arguments.keywords.is_empty() => {
-            // `list()` or `tuple()`
+            // `list()
+            Collection::List
+        }
+        "tuple" if call.arguments.keywords.is_empty() => {
+            // `tuple()`
+            Collection::Tuple
         }
         _ => return,
     };
     if !checker.semantic().is_builtin(func.id.as_str()) {
         return;
     }
+
     let mut diagnostic = Diagnostic::new(
         UnnecessaryCollectionCall {
             obj_type: func.id.to_string(),
         },
-        expr.range(),
+        call.range(),
     );
-    diagnostic.try_set_fix(|| {
+
-        fixes::fix_unnecessary_collection_call(expr, checker).map(Fix::unsafe_edit)
+    // Convert `dict()` to `{}`.
-    });
+    if call.arguments.keywords.is_empty() {
+        diagnostic.set_fix({
+            // Replace from the start of the call to the start of the argument.
+            let call_start = Edit::replacement(
+                match collection {
+                    Collection::Dict => {
+                        pad_start("{", call.range(), checker.locator(), checker.semantic())
+                    }
+                    Collection::List => "[".to_string(),
+                    Collection::Tuple => "(".to_string(),
+                },
+                call.start(),
+                call.arguments.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(
+                match collection {
+                    Collection::Dict => {
+                        pad_end("}", call.range(), checker.locator(), checker.semantic())
+                    }
+                    Collection::List => "]".to_string(),
+                    Collection::Tuple => ")".to_string(),
+                },
+                call.arguments.end() - TextSize::from(1),
+                call.end(),
+            );
+
+            Fix::unsafe_edits(call_start, [call_end])
+        });
+    } else {
+        // Convert `dict(a=1, b=2)` to `{"a": 1, "b": 2}`.
+        diagnostic.try_set_fix(|| {
+            fixes::fix_unnecessary_collection_call(call, checker).map(Fix::unsafe_edit)
+        });
+    }
+
     checker.diagnostics.push(diagnostic);
 }
+
+enum Collection {
+    Tuple,
+    List,
+    Dict,
+}

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

@@ -217,6 +217,7 @@ C408.py:20:5: C408 [*] Unnecessary `dict` call (rewrite as a literal)
    20 |+f"{ {'x': 'y'} | dict(y='z') }"
 21 21 | f"a {dict(x='y') | dict(y='z')} b"
 22 22 | f"a { dict(x='y') | dict(y='z') } b"
+23 23 | 
 
 C408.py:20:19: C408 [*] Unnecessary `dict` call (rewrite as a literal)
    |
@@ -236,6 +237,7 @@ C408.py:20:19: C408 [*] Unnecessary `dict` call (rewrite as a literal)
    20 |+f"{ dict(x='y') | {'y': 'z'} }"
 21 21 | f"a {dict(x='y') | dict(y='z')} b"
 22 22 | f"a { dict(x='y') | dict(y='z') } b"
+23 23 | 
 
 C408.py:21:6: C408 [*] Unnecessary `dict` call (rewrite as a literal)
    |
@@ -254,6 +256,8 @@ C408.py:21:6: C408 [*] Unnecessary `dict` call (rewrite as a literal)
 21    |-f"a {dict(x='y') | dict(y='z')} b"
    21 |+f"a { {'x': 'y'} | dict(y='z')} b"
 22 22 | f"a { dict(x='y') | dict(y='z') } b"
+23 23 | 
+24 24 | dict(
 
 C408.py:21:20: C408 [*] Unnecessary `dict` call (rewrite as a literal)
    |
@@ -272,6 +276,8 @@ C408.py:21:20: C408 [*] Unnecessary `dict` call (rewrite as a literal)
 21    |-f"a {dict(x='y') | dict(y='z')} b"
    21 |+f"a {dict(x='y') | {'y': 'z'} } b"
 22 22 | f"a { dict(x='y') | dict(y='z') } b"
+23 23 | 
+24 24 | dict(
 
 C408.py:22:7: C408 [*] Unnecessary `dict` call (rewrite as a literal)
    |
@@ -279,6 +285,8 @@ C408.py:22:7: C408 [*] Unnecessary `dict` call (rewrite as a literal)
 21 | f"a {dict(x='y') | dict(y='z')} b"
 22 | f"a { dict(x='y') | dict(y='z') } b"
    |       ^^^^^^^^^^^ C408
+23 | 
+24 | dict(
    |
    = help: Rewrite as a literal
 
@@ -288,6 +296,9 @@ C408.py:22:7: C408 [*] Unnecessary `dict` call (rewrite as a literal)
 21 21 | f"a {dict(x='y') | dict(y='z')} b"
 22    |-f"a { dict(x='y') | dict(y='z') } b"
    22 |+f"a { {'x': 'y'} | dict(y='z') } b"
+23 23 | 
+24 24 | dict(
+25 25 |     # comment
 
 C408.py:22:21: C408 [*] Unnecessary `dict` call (rewrite as a literal)
    |
@@ -295,6 +306,8 @@ C408.py:22:21: C408 [*] Unnecessary `dict` call (rewrite as a literal)
 21 | f"a {dict(x='y') | dict(y='z')} b"
 22 | f"a { dict(x='y') | dict(y='z') } b"
    |                     ^^^^^^^^^^^ C408
+23 | 
+24 | dict(
    |
    = help: Rewrite as a literal
 
@@ -304,5 +317,52 @@ C408.py:22:21: C408 [*] Unnecessary `dict` call (rewrite as a literal)
 21 21 | f"a {dict(x='y') | dict(y='z')} b"
 22    |-f"a { dict(x='y') | dict(y='z') } b"
    22 |+f"a { dict(x='y') | {'y': 'z'} } b"
+23 23 | 
+24 24 | dict(
+25 25 |     # comment
+
+C408.py:24:1: C408 [*] Unnecessary `dict` call (rewrite as a literal)
+   |
+22 |   f"a { dict(x='y') | dict(y='z') } b"
+23 |   
+24 | / dict(
+25 | |     # comment
+26 | | )
+   | |_^ C408
+27 |   
+28 |   tuple(  # comment
+   |
+   = help: Rewrite as a literal
+
+ℹ Unsafe fix
+21 21 | f"a {dict(x='y') | dict(y='z')} b"
+22 22 | f"a { dict(x='y') | dict(y='z') } b"
+23 23 | 
+24    |-dict(
+   24 |+{
+25 25 |     # comment
+26    |-)
+   26 |+}
+27 27 | 
+28 28 | tuple(  # comment
+29 29 | )
+
+C408.py:28:1: C408 [*] Unnecessary `tuple` call (rewrite as a literal)
+   |
+26 |   )
+27 |   
+28 | / tuple(  # comment
+29 | | )
+   | |_^ C408
+   |
+   = help: Rewrite as a literal
+
+ℹ Unsafe fix
+25 25 |     # comment
+26 26 | )
+27 27 | 
+28    |-tuple(  # comment
+   28 |+(  # comment
+29 29 | )
 
 

crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C408_C408.py_allow_dict_calls_with_keyword_arguments.snap

@@ -96,4 +96,48 @@ C408.py:17:6: C408 [*] Unnecessary `dict` call (rewrite as a literal)
 19 19 | f"{dict(x='y') | dict(y='z')}"
 20 20 | f"{ dict(x='y') | dict(y='z') }"
 
+C408.py:24:1: C408 [*] Unnecessary `dict` call (rewrite as a literal)
+   |
+22 |   f"a { dict(x='y') | dict(y='z') } b"
+23 |   
+24 | / dict(
+25 | |     # comment
+26 | | )
+   | |_^ C408
+27 |   
+28 |   tuple(  # comment
+   |
+   = help: Rewrite as a literal
+
+ℹ Unsafe fix
+21 21 | f"a {dict(x='y') | dict(y='z')} b"
+22 22 | f"a { dict(x='y') | dict(y='z') } b"
+23 23 | 
+24    |-dict(
+   24 |+{
+25 25 |     # comment
+26    |-)
+   26 |+}
+27 27 | 
+28 28 | tuple(  # comment
+29 29 | )
+
+C408.py:28:1: C408 [*] Unnecessary `tuple` call (rewrite as a literal)
+   |
+26 |   )
+27 |   
+28 | / tuple(  # comment
+29 | | )
+   | |_^ C408
+   |
+   = help: Rewrite as a literal
+
+ℹ Unsafe fix
+25 25 |     # comment
+26 26 | )
+27 27 | 
+28    |-tuple(  # comment
+   28 |+(  # comment
+29 29 | )
+