Remove destructive fixes for F523 (#4883)

crates/ruff/src/cst/matchers.rs

@@ -3,7 +3,7 @@ use libcst_native::{
     Arg, Attribute, Call, Comparison, CompoundStatement, Dict, Expression, FormattedString,
     FormattedStringContent, FormattedStringExpression, FunctionDef, GeneratorExp, If, Import,
     ImportAlias, ImportFrom, ImportNames, IndentedBlock, Lambda, ListComp, Module, Name,
-    SimpleString, SmallStatement, Statement, Suite, Tuple, With,
+    SmallStatement, Statement, Suite, Tuple, With,
 };
 
 pub(crate) fn match_module(module_text: &str) -> Result<Module> {
@@ -109,16 +109,6 @@ pub(crate) fn match_attribute<'a, 'b>(
     }
 }
 
-pub(crate) 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")
-    }
-}
-
 pub(crate) fn match_formatted_string<'a, 'b>(
     expression: &'a mut Expression<'b>,
 ) -> Result<&'a mut FormattedString<'b>> {

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

@@ -1,20 +1,15 @@
-use anyhow::{anyhow, bail, Ok, Result};
+use anyhow::{bail, Ok, Result};
 use libcst_native::{DictElement, Expression};
 use ruff_text_size::TextRange;
-use rustpython_format::{
-    FieldName, FieldNamePart, FieldType, FormatPart, FormatString, FromTemplate,
-};
 use rustpython_parser::ast::{Excepthandler, Expr, Ranged};
 use rustpython_parser::{lexer, Mode, Tok};
 
-use crate::autofix::codemods::CodegenStylist;
 use ruff_diagnostics::Edit;
 use ruff_python_ast::source_code::{Locator, Stylist};
-use ruff_python_ast::str::{leading_quote, raw_contents, trailing_quote};
+use ruff_python_ast::str::raw_contents;
 
-use crate::cst::matchers::{
-    match_attribute, match_call_mut, match_dict, match_expression, match_simple_string,
-};
+use crate::autofix::codemods::CodegenStylist;
+use crate::cst::matchers::{match_call_mut, match_dict, match_expression};
 
 /// Generate a [`Edit`] to remove unused keys from format dict.
 pub(crate) fn remove_unused_format_arguments_from_dict(
@@ -60,132 +55,25 @@ pub(crate) fn remove_unused_keyword_arguments_from_format_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, index_map: &[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,
-            } => {
-                // SAFETY: We've already parsed this string before.
-                let new_field_name = FieldName::parse(field_name).unwrap();
-                let mut new_field_name_string = match new_field_name.field_type {
-                    FieldType::Auto => String::new(),
-                    FieldType::Index(i) => index_map.get(i).unwrap_or(&i).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);
-                }
-
-                // SAFETY: We've already parsed this string before.
-                let new_format_spec = FormatString::from_str(format_spec).unwrap();
-                let new_format_spec_string = update_field_types(&new_format_spec, index_map);
-                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 [`Edit`] to remove unused positional arguments from a `format` call.
 pub(crate) fn remove_unused_positional_arguments_from_format_call(
     unused_arguments: &[usize],
     location: TextRange,
     locator: &Locator,
     stylist: &Stylist,
-    format_string: &FormatString,
 ) -> Result<Edit> {
     let module_text = locator.slice(location);
     let mut tree = match_expression(module_text)?;
     let call = match_call_mut(&mut tree)?;
 
-    // Remove any unused arguments, and generate a map from previous index to new index.
+    // Remove any unused arguments.
     let mut index = 0;
-    let mut offset = 0;
-    let mut index_map = Vec::with_capacity(call.args.len());
     call.args.retain(|_| {
-        index_map.push(index - offset);
         let is_unused = unused_arguments.contains(&index);
         index += 1;
-        if is_unused {
-            offset += 1;
-        }
         !is_unused
     });
 
-    // If we removed an argument, we may need to rewrite the positional themselves.
-    // Ex) `"{1}{2}".format(a, b, c)` to `"{0}{1}".format(b, c)`
-    let rewrite_arguments = index_map
-        .iter()
-        .enumerate()
-        .filter(|&(prev_index, _)| !unused_arguments.contains(&prev_index))
-        .any(|(prev_index, &new_index)| prev_index != new_index);
-
-    let new_format_string;
-    if rewrite_arguments {
-        // Extract the format string verbatim.
-        let func = match_attribute(&mut call.func)?;
-        let simple_string = match_simple_string(&mut func.value)?;
-
-        // Extract existing quotes from the format string.
-        let leading_quote = leading_quote(simple_string.value).ok_or_else(|| {
-            anyhow!(
-                "Could not find leading quote for format string: {}",
-                simple_string.value
-            )
-        })?;
-        let trailing_quote = trailing_quote(simple_string.value).ok_or_else(|| {
-            anyhow!(
-                "Could not find trailing quote for format string: {}",
-                simple_string.value
-            )
-        })?;
-
-        // Update the format string, preserving the quotes.
-        new_format_string = format!(
-            "{}{}{}",
-            leading_quote,
-            update_field_types(format_string, &index_map),
-            trailing_quote
-        );
-
-        simple_string.value = new_format_string.as_str();
-    }
-
     Ok(Edit::range_replacement(
         tree.codegen_stylist(stylist),
         location,

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

@@ -26,7 +26,6 @@ pub(crate) struct FormatSummary {
     pub(crate) indices: Vec<usize>,
     pub(crate) keywords: Vec<String>,
     pub(crate) has_nested_parts: bool,
-    pub(crate) format_string: FormatString,
 }
 
 impl TryFrom<&str> for FormatSummary {
@@ -75,7 +74,6 @@ impl TryFrom<&str> for FormatSummary {
             indices,
             keywords,
             has_nested_parts,
-            format_string,
         })
     }
 }

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

@@ -4,7 +4,7 @@ use ruff_text_size::TextRange;
 use rustc_hash::FxHashSet;
 use rustpython_parser::ast::{self, Constant, Expr, Identifier, Keyword};
 
-use ruff_diagnostics::{AlwaysAutofixableViolation, AutofixKind, Diagnostic, Violation};
+use ruff_diagnostics::{AlwaysAutofixableViolation, AutofixKind, Diagnostic, Fix, Violation};
 use ruff_macros::{derive_message_formats, violation};
 
 use crate::checkers::ast::Checker;
@@ -604,14 +604,14 @@ pub(crate) fn percent_format_extra_named_arguments(
         location,
     );
     if checker.patch(diagnostic.kind.rule()) {
-        #[allow(deprecated)]
-        diagnostic.try_set_fix_from_edit(|| {
-            remove_unused_format_arguments_from_dict(
+        diagnostic.try_set_fix(|| {
+            let edit = remove_unused_format_arguments_from_dict(
                 &missing,
                 right,
                 checker.locator,
                 checker.stylist,
-            )
+            )?;
+            Ok(Fix::automatic(edit))
         });
     }
     checker.diagnostics.push(diagnostic);
@@ -770,14 +770,14 @@ pub(crate) fn string_dot_format_extra_named_arguments(
         location,
     );
     if checker.patch(diagnostic.kind.rule()) {
-        #[allow(deprecated)]
-        diagnostic.try_set_fix_from_edit(|| {
-            remove_unused_keyword_arguments_from_format_call(
+        diagnostic.try_set_fix(|| {
+            let edit = remove_unused_keyword_arguments_from_format_call(
                 &missing,
                 location,
                 checker.locator,
                 checker.stylist,
-            )
+            )?;
+            Ok(Fix::automatic(edit))
         });
     }
     checker.diagnostics.push(diagnostic);
@@ -815,16 +815,42 @@ pub(crate) fn string_dot_format_extra_positional_arguments(
         location,
     );
     if checker.patch(diagnostic.kind.rule()) {
-        #[allow(deprecated)]
-        diagnostic.try_set_fix_from_edit(|| {
-            remove_unused_positional_arguments_from_format_call(
-                &missing,
-                location,
-                checker.locator,
-                checker.stylist,
-                &summary.format_string,
-            )
-        });
+        // We can only fix if the positional arguments we're removing don't require re-indexing
+        // the format string itself. For example, we can't fix `"{1}{2}".format(0, 1, 2)"`, since
+        // this requires changing the format string to `"{0}{1}"`. But we can fix
+        // `"{0}{1}".format(0, 1, 2)`, since this only requires modifying the call arguments.
+        fn is_contiguous_from_end<T>(indexes: &[usize], target: &[T]) -> bool {
+            if indexes.is_empty() {
+                return true;
+            }
+
+            let mut expected_index = target.len() - 1;
+            for &index in indexes.iter().rev() {
+                if index != expected_index {
+                    return false;
+                }
+
+                if expected_index == 0 {
+                    break;
+                }
+
+                expected_index -= 1;
+            }
+
+            true
+        }
+
+        if is_contiguous_from_end(&missing, args) {
+            diagnostic.try_set_fix(|| {
+                let edit = remove_unused_positional_arguments_from_format_call(
+                    &missing,
+                    location,
+                    checker.locator,
+                    checker.stylist,
+                )?;
+                Ok(Fix::automatic(edit))
+            });
+        }
     }
     checker.diagnostics.push(diagnostic);
 }

crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F504_F504.py.snap

@@ -12,7 +12,7 @@ F504.py:3:1: F504 [*] `%`-format string has unused named argument(s): b
   |
   = help: Remove extra named arguments: b
 
-ℹ Suggested fix
+ℹ Fix
 1 1 | # Ruff has no way of knowing if the following are F505s
 2 2 | a = "wrong"
 3   |-"%(a)s %(c)s" % {a: "?", "b": "!"}  # F504 ("b" not used)
@@ -31,7 +31,7 @@ F504.py:8:1: F504 [*] `%`-format string has unused named argument(s): b
    |
    = help: Remove extra named arguments: b
 
-ℹ Suggested fix
+ℹ Fix
 5 5 | hidden = {"a": "!"}
 6 6 | "%(a)s %(c)s" % {"x": 1, **hidden}  # Ok (cannot see through splat)
 7 7 | 
@@ -47,7 +47,7 @@ F504.py:9:1: F504 [*] `%`-format string has unused named argument(s): b
    |
    = help: Remove extra named arguments: b
 
-ℹ Suggested fix
+ℹ Fix
 6 6 | "%(a)s %(c)s" % {"x": 1, **hidden}  # Ok (cannot see through splat)
 7 7 | 
 8 8 | "%(a)s" % {"a": 1, r"b": "!"}  # F504 ("b" not used)

crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F504_F50x.py.snap

@@ -12,7 +12,7 @@ F50x.py:8:1: F504 [*] `%`-format string has unused named argument(s): baz
    |
    = help: Remove extra named arguments: baz
 
-ℹ Suggested fix
+ℹ Fix
 5 5 | '%s %s' % (1,)  # F507
 6 6 | '%s %s' % (1, 2, 3)  # F507
 7 7 | '%(bar)s' % {}  # F505

crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F522_F522.py.snap

@@ -10,7 +10,7 @@ F522.py:1:1: F522 [*] `.format` call has unused named argument(s): bar
   |
   = help: Remove extra named arguments: bar
 
-ℹ Suggested fix
+ℹ Fix
 1   |-"{}".format(1, bar=2)  # F522
   1 |+"{}".format(1, )  # F522
 2 2 | "{bar}{}".format(1, bar=2, spam=3)  # F522
@@ -27,7 +27,7 @@ F522.py:2:1: F522 [*] `.format` call has unused named argument(s): spam
   |
   = help: Remove extra named arguments: spam
 
-ℹ Suggested fix
+ℹ Fix
 1 1 | "{}".format(1, bar=2)  # F522
 2   |-"{bar}{}".format(1, bar=2, spam=3)  # F522
   2 |+"{bar}{}".format(1, bar=2, )  # F522
@@ -43,7 +43,7 @@ F522.py:4:1: F522 [*] `.format` call has unused named argument(s): eggs, ham
   |
   = help: Remove extra named arguments: eggs, ham
 
-ℹ Suggested fix
+ℹ Fix
 1 1 | "{}".format(1, bar=2)  # F522
 2 2 | "{bar}{}".format(1, bar=2, spam=3)  # F522
 3 3 | "{bar:{spam}}".format(bar=2, spam=3)  # No issues