Fix format literals

Author: charliermarsh

crates/ruff/resources/test/fixtures/pyupgrade/UP030_0.py

@@ -32,3 +32,30 @@
 )
 
 '{' '0}'.format(1)
+
+args = list(range(10))
+kwargs = {x: x for x in range(10)}
+
+"{0}".format(*args)
+
+"{0}".format(**kwargs)
+
+"{0}_{1}".format(*args)
+
+"{0}_{1}".format(1, *args)
+
+"{0}_{1}".format(1, 2, *args)
+
+"{0}_{1}".format(*args, 1, 2)
+
+"{0}_{1}_{2}".format(1, **kwargs)
+
+"{0}_{1}_{2}".format(1, 2, **kwargs)
+
+"{0}_{1}_{2}".format(1, 2, 3, **kwargs)
+
+"{0}_{1}_{2}".format(1, 2, 3, *args, **kwargs)
+
+"{1}_{0}".format(1, 2, *args)
+
+"{1}_{0}".format(1, 2)

crates/ruff/resources/test/fixtures/pyupgrade/UP030_1.py

@@ -15,3 +15,17 @@
 print(f"{0}".format(1))
 
 ''.format(1)
+
+'{1} {0}'.format(*args)
+
+"{1}_{0}".format(*args, 1)
+
+"{1}_{0}".format(*args, 1, 2)
+
+"{1}_{0}".format(1, **kwargs)
+
+"{1}_{0}".format(1, foo=2)
+
+"{1}_{0}".format(1, 2, **kwargs)
+
+"{1}_{0}".format(1, 2, foo=3, bar=4)

crates/ruff/resources/test/fixtures/pyupgrade/UP030_2.py

@@ -410,7 +410,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
                                         );
                                     }
                                     if checker.enabled(Rule::FormatLiterals) {
-                                        pyupgrade::rules::format_literals(checker, &summary, expr);
+                                        pyupgrade::rules::format_literals(checker, &summary, call);
                                     }
                                     if checker.enabled(Rule::FString) {
                                         pyupgrade::rules::f_strings(

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

@@ -30,7 +30,6 @@ mod tests {
     #[test_case(Rule::FString, Path::new("UP032_2.py"))]
     #[test_case(Rule::FormatLiterals, Path::new("UP030_0.py"))]
     #[test_case(Rule::FormatLiterals, Path::new("UP030_1.py"))]
-    #[test_case(Rule::FormatLiterals, Path::new("UP030_2.py"))]
     #[test_case(Rule::LRUCacheWithMaxsizeNone, Path::new("UP033_0.py"))]
     #[test_case(Rule::LRUCacheWithMaxsizeNone, Path::new("UP033_1.py"))]
     #[test_case(Rule::LRUCacheWithoutParameters, Path::new("UP011.py"))]

crates/ruff/src/rules/pyupgrade/mod.rs

@@ -2,7 +2,7 @@ use anyhow::{anyhow, Result};
 use libcst_native::{Arg, Expression};
 use once_cell::sync::Lazy;
 use regex::Regex;
-use ruff_python_ast::{Expr, Ranged};
+use ruff_python_ast::{self as ast, Expr, Ranged};
 
 use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
 use ruff_macros::{derive_message_formats, violation};
@@ -55,6 +55,77 @@ impl Violation for FormatLiterals {
     }
 }
 
+/// UP030
+pub(crate) fn format_literals(
+    checker: &mut Checker,
+    summary: &FormatSummary,
+    call: &ast::ExprCall,
+) {
+    // The format we expect is, e.g.: `"{0} {1}".format(...)`
+    if summary.has_nested_parts {
+        return;
+    }
+    if !summary.keywords.is_empty() {
+        return;
+    }
+    if !summary.autos.is_empty() {
+        return;
+    }
+    if summary.indices.is_empty() {
+        return;
+    }
+    if (0..summary.indices.len()).any(|index| !summary.indices.contains(&index)) {
+        return;
+    }
+
+    // If the positional indices aren't sequential (e.g., `"{1} {0}".format(1, 2)`), then we
+    // need to reorder the function arguments; so we need to ensure that the function
+    // arguments aren't splatted (e.g., `"{1} {0}".format(*foo)`), that there are a sufficient
+    // number of them, etc.
+    let arguments = if is_sequential(&summary.indices) {
+        Arguments::Preserve
+    } else {
+        // Ex) `"{1} {0}".format(foo=1, bar=2)`
+        if !call.arguments.keywords.is_empty() {
+            return;
+        }
+
+        // Ex) `"{1} {0}".format(foo)`
+        if call.arguments.args.len() < summary.indices.len() {
+            return;
+        }
+
+        // Ex) `"{1} {0}".format(*foo)`
+        if call
+            .arguments
+            .args
+            .iter()
+            .take(summary.indices.len())
+            .any(Expr::is_starred_expr)
+        {
+            return;
+        }
+
+        Arguments::Reorder(&summary.indices)
+    };
+
+    let mut diagnostic = Diagnostic::new(FormatLiterals, call.range());
+    if checker.patch(diagnostic.kind.rule()) {
+        diagnostic.try_set_fix(|| {
+            Ok(Fix::suggested(Edit::range_replacement(
+                generate_call(call, arguments, checker.locator(), checker.stylist())?,
+                call.range(),
+            )))
+        });
+    }
+    checker.diagnostics.push(diagnostic);
+}
+
+/// Returns true if the indices are sequential.
+fn is_sequential(indices: &[usize]) -> bool {
+    indices.iter().enumerate().all(|(idx, value)| idx == *value)
+}
+
 // An opening curly brace, followed by any integer, followed by any text,
 // followed by a closing brace.
 static FORMAT_SPECIFIER: Lazy<Regex> =
@@ -118,26 +189,30 @@ fn generate_arguments<'a>(arguments: &[Arg<'a>], order: &'a [usize]) -> Result<V
     Ok(new_arguments)
 }
 
-/// Returns true if the indices are sequential.
-fn is_sequential(indices: &[usize]) -> bool {
-    indices.iter().enumerate().all(|(idx, value)| idx == *value)
+#[derive(Debug, Copy, Clone)]
+enum Arguments<'a> {
+    /// Preserve the arguments to the `.format(...)` call.
+    Preserve,
+    /// Reorder the arguments to the `.format(...)` call, based on the given
+    /// indices.
+    Reorder(&'a [usize]),
 }
 
 /// Returns the corrected function call.
 fn generate_call(
-    expr: &Expr,
-    correct_order: &[usize],
+    call: &ast::ExprCall,
+    arguments: Arguments,
     locator: &Locator,
     stylist: &Stylist,
 ) -> Result<String> {
-    let content = locator.slice(expr.range());
+    let content = locator.slice(call.range());
     let parenthesized_content = format!("({content})");
     let mut expression = match_expression(&parenthesized_content)?;
 
     // Fix the call arguments.
     let call = match_call_mut(&mut expression)?;
-    if !is_sequential(correct_order) {
-        call.args = generate_arguments(&call.args, correct_order)?;
+    if let Arguments::Reorder(order) = arguments {
+        call.args = generate_arguments(&call.args, order)?;
     }
 
     // Fix the string itself.
@@ -157,34 +232,3 @@ fn generate_call(
 
     Ok(output)
 }
-
-/// UP030
-pub(crate) fn format_literals(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) {
-    // The format we expect is, e.g.: `"{0} {1}".format(...)`
-    if summary.has_nested_parts {
-        return;
-    }
-    if !summary.keywords.is_empty() {
-        return;
-    }
-    if !summary.autos.is_empty() {
-        return;
-    }
-    if summary.indices.is_empty() {
-        return;
-    }
-    if (0..summary.indices.len()).any(|index| !summary.indices.contains(&index)) {
-        return;
-    }
-
-    let mut diagnostic = Diagnostic::new(FormatLiterals, expr.range());
-    if checker.patch(diagnostic.kind.rule()) {
-        diagnostic.try_set_fix(|| {
-            Ok(Fix::suggested(Edit::range_replacement(
-                generate_call(expr, &summary.indices, checker.locator(), checker.stylist())?,
-                expr.range(),
-            )))
-        });
-    }
-    checker.diagnostics.push(diagnostic);
-}