Add autofix functionality for F523 (#3613)

crates/ruff/resources/test/fixtures/pyflakes/F523.py

@@ -4,6 +4,7 @@
 "{1:{0}}".format(1, 2)  # No issues
 "{1:{0}}".format(1, 2, 3)  # F523
 "{0}{2}".format(1, 2)  # F523, # F524
+"{1.arg[1]!r:0{2['arg']}{1}}".format(1, 2, 3, 4) # F523
 
 # With no indexes
 "{}".format(1, 2)  # F523

crates/ruff/src/cst/matchers.rs

@@ -1,6 +1,7 @@
 use anyhow::{bail, Result};
 use libcst_native::{
-    Call, Comparison, Expr, Expression, Import, ImportFrom, Module, SmallStatement, Statement,
+    Attribute, Call, Comparison, Dict, Expr, Expression, Import, ImportFrom, Module, SimpleString,
+    SmallStatement, Statement,
 };
 
 pub fn match_module(module_text: &str) -> Result<Module> {
@@ -70,3 +71,31 @@ pub fn match_comparison<'a, 'b>(
         bail!("Expected Expression::Comparison")
     }
 }
+
+pub fn match_dict<'a, 'b>(expression: &'a mut Expression<'b>) -> Result<&'a mut Dict<'b>> {
+    if let Expression::Dict(dict) = expression {
+        Ok(dict)
+    } else {
+        bail!("Expected Expression::Dict")
+    }
+}
+
+pub fn match_attribute<'a, 'b>(
+    expression: &'a mut Expression<'b>,
+) -> Result<&'a mut Attribute<'b>> {
+    if let Expression::Attribute(attribute) = expression {
+        Ok(attribute)
+    } else {
+        bail!("Expected Expression::Attribute")
+    }
+}
+
+pub fn match_simple_string<'a, 'b>(
+    expression: &'a mut Expression<'b>,
+) -> Result<&'a mut SimpleString<'b>> {
+    if let Expression::SimpleString(simple_string) = expression {
+        Ok(simple_string)
+    } else {
+        bail!("Expected Expression::SimpleString")
+    }
+}

crates/ruff/src/rules/pyflakes/fixes.rs

@@ -1,14 +1,19 @@
-use anyhow::{bail, Result};
-use libcst_native::{Call, Codegen, CodegenState, Dict, DictElement, Expression};
+use anyhow::{bail, Ok, Result};
+use libcst_native::{Codegen, CodegenState, DictElement, Expression};
 use rustpython_parser::ast::{Excepthandler, Expr};
 use rustpython_parser::{lexer, Mode, Tok};
 
 use ruff_diagnostics::Fix;
 use ruff_python_ast::source_code::{Locator, Stylist};
 use ruff_python_ast::str::raw_contents;
 use ruff_python_ast::types::Range;
+use rustpython_common::format::{
+    FieldName, FieldNamePart, FieldType, FormatPart, FormatString, FromTemplate,
+};
 
-use crate::cst::matchers::{match_expr, match_module};
+use crate::cst::matchers::{
+    match_attribute, match_call, match_dict, match_expression, match_simple_string,
+};
 
 /// Generate a [`Fix`] to remove unused keys from format dict.
 pub fn remove_unused_format_arguments_from_dict(
@@ -18,34 +23,15 @@ pub fn remove_unused_format_arguments_from_dict(
     stylist: &Stylist,
 ) -> Result<Fix> {
     let module_text = locator.slice(stmt);
-    let mut tree = match_module(module_text)?;
-    let mut body = match_expr(&mut tree)?;
-
-    let new_dict = {
-        let Expression::Dict(dict) = &body.value else {
-            bail!("Expected Expression::Dict")
-        };
-
-        Dict {
-            lbrace: dict.lbrace.clone(),
-            lpar: dict.lpar.clone(),
-            rbrace: dict.rbrace.clone(),
-            rpar: dict.rpar.clone(),
-            elements: dict
-                .elements
-                .iter()
-                .filter_map(|e| match e {
-                    DictElement::Simple {
-                        key: Expression::SimpleString(name),
-                        ..
-                    } if unused_arguments.contains(&raw_contents(name.value)) => None,
-                    e => Some(e.clone()),
-                })
-                .collect(),
-        }
-    };
+    let mut tree = match_expression(module_text)?;
+    let dict = match_dict(&mut tree)?;
 
-    body.value = Expression::Dict(Box::new(new_dict));
+    dict.elements.retain(|e| {
+        !matches!(e, DictElement::Simple {
+            key: Expression::SimpleString(name),
+            ..
+        } if unused_arguments.contains(&raw_contents(name.value)))
+    });
 
     let mut state = CodegenState {
         default_newline: stylist.line_ending(),
@@ -61,40 +47,127 @@ pub fn remove_unused_format_arguments_from_dict(
     ))
 }
 
-/// Generate a [`Fix`] to remove unused keyword arguments from format call.
+/// Generate a [`Fix`] to remove unused keyword arguments from a `format` call.
 pub fn remove_unused_keyword_arguments_from_format_call(
     unused_arguments: &[&str],
     location: Range,
     locator: &Locator,
     stylist: &Stylist,
 ) -> Result<Fix> {
     let module_text = locator.slice(location);
-    let mut tree = match_module(module_text)?;
-    let mut body = match_expr(&mut tree)?;
-
-    let new_call = {
-        let Expression::Call(call) = &body.value else {
-            bail!("Expected Expression::Call")
-        };
-
-        Call {
-            func: call.func.clone(),
-            lpar: call.lpar.clone(),
-            rpar: call.rpar.clone(),
-            whitespace_before_args: call.whitespace_before_args.clone(),
-            whitespace_after_func: call.whitespace_after_func.clone(),
-            args: call
-                .args
-                .iter()
-                .filter_map(|e| match &e.keyword {
-                    Some(kw) if unused_arguments.contains(&kw.value) => None,
-                    _ => Some(e.clone()),
-                })
-                .collect(),
-        }
+    let mut tree = match_expression(module_text)?;
+    let call = match_call(&mut tree)?;
+
+    call.args
+        .retain(|e| !matches!(&e.keyword, Some(kw) if unused_arguments.contains(&kw.value)));
+
+    let mut state = CodegenState {
+        default_newline: stylist.line_ending(),
+        default_indent: stylist.indentation(),
+        ..CodegenState::default()
     };
+    tree.codegen(&mut state);
+
+    Ok(Fix::replacement(
+        state.to_string(),
+        location.location,
+        location.end_location,
+    ))
+}
 
-    body.value = Expression::Call(Box::new(new_call));
+fn unparse_format_part(format_part: FormatPart) -> String {
+    match format_part {
+        FormatPart::Literal(literal) => literal,
+        FormatPart::Field {
+            field_name,
+            conversion_spec,
+            format_spec,
+        } => {
+            let mut field_name = field_name;
+            if let Some(conversion) = conversion_spec {
+                field_name.push_str(&format!("!{conversion}"));
+            }
+            if !format_spec.is_empty() {
+                field_name.push_str(&format!(":{format_spec}"));
+            }
+            format!("{{{field_name}}}")
+        }
+    }
+}
+
+fn update_field_types(format_string: &FormatString, min_unused: usize) -> String {
+    format_string
+        .format_parts
+        .iter()
+        .map(|part| match part {
+            FormatPart::Literal(literal) => FormatPart::Literal(literal.to_string()),
+            FormatPart::Field {
+                field_name,
+                conversion_spec,
+                format_spec,
+            } => {
+                let new_field_name = FieldName::parse(field_name).unwrap(); // This should never fail because we parsed it before
+                let mut new_field_name_string = match new_field_name.field_type {
+                    FieldType::Auto => String::new(),
+                    FieldType::Index(i) => (i - min_unused).to_string(),
+                    FieldType::Keyword(keyword) => keyword,
+                };
+                for field_name_part in &new_field_name.parts {
+                    let field_name_part_string = match field_name_part {
+                        FieldNamePart::Attribute(attribute) => format!(".{attribute}"),
+                        FieldNamePart::Index(i) => format!("[{i}]"),
+                        FieldNamePart::StringIndex(s) => format!("[{s}]"),
+                    };
+                    new_field_name_string.push_str(&field_name_part_string);
+                }
+                let new_format_spec = FormatString::from_str(format_spec).unwrap(); // This should never fail because we parsed it before
+                let new_format_spec_string = update_field_types(&new_format_spec, min_unused);
+                FormatPart::Field {
+                    field_name: new_field_name_string,
+                    conversion_spec: *conversion_spec,
+                    format_spec: new_format_spec_string,
+                }
+            }
+        })
+        .map(unparse_format_part)
+        .collect()
+}
+
+/// Generate a [`Fix`] to remove unused positional arguments from a `format` call.
+pub fn remove_unused_positional_arguments_from_format_call(
+    unused_arguments: &[usize],
+    location: Range,
+    locator: &Locator,
+    stylist: &Stylist,
+    format_string: &FormatString,
+) -> Result<Fix> {
+    let module_text = locator.slice(location);
+    let mut tree = match_expression(module_text)?;
+    let call = match_call(&mut tree)?;
+
+    let mut index = 0;
+    call.args.retain(|_| {
+        index += 1;
+        !unused_arguments.contains(&(index - 1))
+    });
+
+    let mut min_unused_index = 0;
+    for index in unused_arguments {
+        if *index == min_unused_index {
+            min_unused_index += 1;
+        } else {
+            break;
+        }
+    }
+
+    let mut new_format_string;
+    if min_unused_index > 0 {
+        let func = match_attribute(&mut call.func)?;
+        let simple_string = match_simple_string(&mut func.value)?;
+        new_format_string = update_field_types(format_string, min_unused_index);
+        new_format_string = format!(r#""{new_format_string}""#);
+        simple_string.value = new_format_string.as_str();
+    }
 
     let mut state = CodegenState {
         default_newline: stylist.line_ending(),

crates/ruff/src/rules/pyflakes/format.rs

@@ -26,6 +26,7 @@ pub(crate) struct FormatSummary {
     pub indexes: Vec<usize>,
     pub keywords: Vec<String>,
     pub has_nested_parts: bool,
+    pub format_string: FormatString,
 }
 
 impl TryFrom<&str> for FormatSummary {
@@ -39,22 +40,22 @@ impl TryFrom<&str> for FormatSummary {
         let mut keywords = Vec::new();
         let mut has_nested_parts = false;
 
-        for format_part in format_string.format_parts {
+        for format_part in &format_string.format_parts {
             let FormatPart::Field {
                 field_name,
                 format_spec,
                 ..
             } = format_part else {
                 continue;
             };
-            let parsed = FieldName::parse(&field_name)?;
+            let parsed = FieldName::parse(field_name)?;
             match parsed.field_type {
                 FieldType::Auto => autos.push(autos.len()),
                 FieldType::Index(i) => indexes.push(i),
                 FieldType::Keyword(k) => keywords.push(k),
             };
 
-            let nested = FormatString::from_str(&format_spec)?;
+            let nested = FormatString::from_str(format_spec)?;
             for nested_part in nested.format_parts {
                 let FormatPart::Field { field_name, .. } = nested_part else {
                     continue;
@@ -74,6 +75,7 @@ impl TryFrom<&str> for FormatSummary {
             indexes,
             keywords,
             has_nested_parts,
+            format_string,
         })
     }
 }

crates/ruff/src/rules/pyflakes/rules/strings.rs

@@ -14,6 +14,7 @@ use crate::registry::AsRule;
 use super::super::cformat::CFormatSummary;
 use super::super::fixes::{
     remove_unused_format_arguments_from_dict, remove_unused_keyword_arguments_from_format_call,
+    remove_unused_positional_arguments_from_format_call,
 };
 use super::super::format::FormatSummary;
 
@@ -169,13 +170,19 @@ pub struct StringDotFormatExtraPositionalArguments {
     pub missing: Vec<String>,
 }
 
-impl Violation for StringDotFormatExtraPositionalArguments {
+impl AlwaysAutofixableViolation for StringDotFormatExtraPositionalArguments {
     #[derive_message_formats]
     fn message(&self) -> String {
         let StringDotFormatExtraPositionalArguments { missing } = self;
         let message = missing.join(", ");
         format!("`.format` call has unused arguments at position(s): {message}")
     }
+
+    fn autofix_title(&self) -> String {
+        let StringDotFormatExtraPositionalArguments { missing } = self;
+        let message = missing.join(", ");
+        format!("Remove extra positional arguments at position(s): {message}")
+    }
 }
 
 #[violation]
@@ -505,15 +512,30 @@ pub(crate) fn string_dot_format_extra_positional_arguments(
         return;
     }
 
-    checker.diagnostics.push(Diagnostic::new(
+    let mut diagnostic = Diagnostic::new(
         StringDotFormatExtraPositionalArguments {
             missing: missing
                 .iter()
                 .map(std::string::ToString::to_string)
                 .collect::<Vec<String>>(),
         },
         location,
-    ));
+    );
+    if checker.patch(diagnostic.kind.rule()) {
+        match remove_unused_positional_arguments_from_format_call(
+            &missing,
+            location,
+            checker.locator,
+            checker.stylist,
+            &summary.format_string,
+        ) {
+            Ok(fix) => {
+                diagnostic.amend(fix);
+            }
+            Err(e) => error!("Failed to remove unused positional arguments: {e}"),
+        }
+    }
+    checker.diagnostics.push(diagnostic);
 }
 
 /// F524