From 3938e1c9fe0cdc933a03dd5b6eb01e0912f186c1 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 7 Aug 2023 11:15:44 -0400 Subject: [PATCH] Fix format literals --- .../test/fixtures/pyupgrade/UP030_0.py | 27 ++ .../test/fixtures/pyupgrade/UP030_1.py | 14 + .../test/fixtures/pyupgrade/UP030_2.py | 28 -- .../src/checkers/ast/analyze/expression.rs | 2 +- crates/ruff/src/rules/pyupgrade/mod.rs | 1 - .../rules/pyupgrade/rules/format_literals.rs | 124 ++++++--- ...__rules__pyupgrade__tests__UP030_0.py.snap | 248 ++++++++++++++++++ ...__rules__pyupgrade__tests__UP030_2.py.snap | 32 --- 8 files changed, 374 insertions(+), 102 deletions(-) delete mode 100644 crates/ruff/resources/test/fixtures/pyupgrade/UP030_2.py diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP030_0.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP030_0.py index 35b85eb3cc3e32..37e3ba4d49d4f8 100644 --- a/crates/ruff/resources/test/fixtures/pyupgrade/UP030_0.py +++ b/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) diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP030_1.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP030_1.py index 85fcf311f5c81b..5cb750ece9c1c8 100644 --- a/crates/ruff/resources/test/fixtures/pyupgrade/UP030_1.py +++ b/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) diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP030_2.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP030_2.py deleted file mode 100644 index 30f2d0c8863dac..00000000000000 --- a/crates/ruff/resources/test/fixtures/pyupgrade/UP030_2.py +++ /dev/null @@ -1,28 +0,0 @@ -# These SHOULD change - -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) - -"{1}_{0}".format(*args) - -"{1}_{0}".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) diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index ae29ce56f552f8..e47b871f776a55 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -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( diff --git a/crates/ruff/src/rules/pyupgrade/mod.rs b/crates/ruff/src/rules/pyupgrade/mod.rs index 7ffeb2b1165ffc..9e2f633ba03b2d 100644 --- a/crates/ruff/src/rules/pyupgrade/mod.rs +++ b/crates/ruff/src/rules/pyupgrade/mod.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"))] diff --git a/crates/ruff/src/rules/pyupgrade/rules/format_literals.rs b/crates/ruff/src/rules/pyupgrade/rules/format_literals.rs index 1469cb7e74d95e..360e8b855fa52d 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/format_literals.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/format_literals.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 = @@ -118,26 +189,30 @@ fn generate_arguments<'a>(arguments: &[Arg<'a>], order: &'a [usize]) -> Result 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 { - 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); -} diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP030_0.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP030_0.py.snap index 393bdb502c78c7..a0b8e1a467227a 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP030_0.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP030_0.py.snap @@ -249,7 +249,255 @@ UP030_0.py:34:1: UP030 Use implicit references for positional format fields 33 | 34 | '{' '0}'.format(1) | ^^^^^^^^^^^^^^^^^^ UP030 +35 | +36 | args = list(range(10)) | = help: Remove explicit positional indices +UP030_0.py:39:1: UP030 [*] Use implicit references for positional format fields + | +37 | kwargs = {x: x for x in range(10)} +38 | +39 | "{0}".format(*args) + | ^^^^^^^^^^^^^^^^^^^ UP030 +40 | +41 | "{0}".format(**kwargs) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +36 36 | args = list(range(10)) +37 37 | kwargs = {x: x for x in range(10)} +38 38 | +39 |-"{0}".format(*args) + 39 |+"{}".format(*args) +40 40 | +41 41 | "{0}".format(**kwargs) +42 42 | + +UP030_0.py:41:1: UP030 [*] Use implicit references for positional format fields + | +39 | "{0}".format(*args) +40 | +41 | "{0}".format(**kwargs) + | ^^^^^^^^^^^^^^^^^^^^^^ UP030 +42 | +43 | "{0}_{1}".format(*args) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +38 38 | +39 39 | "{0}".format(*args) +40 40 | +41 |-"{0}".format(**kwargs) + 41 |+"{}".format(**kwargs) +42 42 | +43 43 | "{0}_{1}".format(*args) +44 44 | + +UP030_0.py:43:1: UP030 [*] Use implicit references for positional format fields + | +41 | "{0}".format(**kwargs) +42 | +43 | "{0}_{1}".format(*args) + | ^^^^^^^^^^^^^^^^^^^^^^^ UP030 +44 | +45 | "{0}_{1}".format(1, *args) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +40 40 | +41 41 | "{0}".format(**kwargs) +42 42 | +43 |-"{0}_{1}".format(*args) + 43 |+"{}_{}".format(*args) +44 44 | +45 45 | "{0}_{1}".format(1, *args) +46 46 | + +UP030_0.py:45:1: UP030 [*] Use implicit references for positional format fields + | +43 | "{0}_{1}".format(*args) +44 | +45 | "{0}_{1}".format(1, *args) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ UP030 +46 | +47 | "{0}_{1}".format(1, 2, *args) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +42 42 | +43 43 | "{0}_{1}".format(*args) +44 44 | +45 |-"{0}_{1}".format(1, *args) + 45 |+"{}_{}".format(1, *args) +46 46 | +47 47 | "{0}_{1}".format(1, 2, *args) +48 48 | + +UP030_0.py:47:1: UP030 [*] Use implicit references for positional format fields + | +45 | "{0}_{1}".format(1, *args) +46 | +47 | "{0}_{1}".format(1, 2, *args) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP030 +48 | +49 | "{0}_{1}".format(*args, 1, 2) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +44 44 | +45 45 | "{0}_{1}".format(1, *args) +46 46 | +47 |-"{0}_{1}".format(1, 2, *args) + 47 |+"{}_{}".format(1, 2, *args) +48 48 | +49 49 | "{0}_{1}".format(*args, 1, 2) +50 50 | + +UP030_0.py:49:1: UP030 [*] Use implicit references for positional format fields + | +47 | "{0}_{1}".format(1, 2, *args) +48 | +49 | "{0}_{1}".format(*args, 1, 2) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP030 +50 | +51 | "{0}_{1}_{2}".format(1, **kwargs) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +46 46 | +47 47 | "{0}_{1}".format(1, 2, *args) +48 48 | +49 |-"{0}_{1}".format(*args, 1, 2) + 49 |+"{}_{}".format(*args, 1, 2) +50 50 | +51 51 | "{0}_{1}_{2}".format(1, **kwargs) +52 52 | + +UP030_0.py:51:1: UP030 [*] Use implicit references for positional format fields + | +49 | "{0}_{1}".format(*args, 1, 2) +50 | +51 | "{0}_{1}_{2}".format(1, **kwargs) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP030 +52 | +53 | "{0}_{1}_{2}".format(1, 2, **kwargs) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +48 48 | +49 49 | "{0}_{1}".format(*args, 1, 2) +50 50 | +51 |-"{0}_{1}_{2}".format(1, **kwargs) + 51 |+"{}_{}_{}".format(1, **kwargs) +52 52 | +53 53 | "{0}_{1}_{2}".format(1, 2, **kwargs) +54 54 | + +UP030_0.py:53:1: UP030 [*] Use implicit references for positional format fields + | +51 | "{0}_{1}_{2}".format(1, **kwargs) +52 | +53 | "{0}_{1}_{2}".format(1, 2, **kwargs) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP030 +54 | +55 | "{0}_{1}_{2}".format(1, 2, 3, **kwargs) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +50 50 | +51 51 | "{0}_{1}_{2}".format(1, **kwargs) +52 52 | +53 |-"{0}_{1}_{2}".format(1, 2, **kwargs) + 53 |+"{}_{}_{}".format(1, 2, **kwargs) +54 54 | +55 55 | "{0}_{1}_{2}".format(1, 2, 3, **kwargs) +56 56 | + +UP030_0.py:55:1: UP030 [*] Use implicit references for positional format fields + | +53 | "{0}_{1}_{2}".format(1, 2, **kwargs) +54 | +55 | "{0}_{1}_{2}".format(1, 2, 3, **kwargs) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP030 +56 | +57 | "{0}_{1}_{2}".format(1, 2, 3, *args, **kwargs) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +52 52 | +53 53 | "{0}_{1}_{2}".format(1, 2, **kwargs) +54 54 | +55 |-"{0}_{1}_{2}".format(1, 2, 3, **kwargs) + 55 |+"{}_{}_{}".format(1, 2, 3, **kwargs) +56 56 | +57 57 | "{0}_{1}_{2}".format(1, 2, 3, *args, **kwargs) +58 58 | + +UP030_0.py:57:1: UP030 [*] Use implicit references for positional format fields + | +55 | "{0}_{1}_{2}".format(1, 2, 3, **kwargs) +56 | +57 | "{0}_{1}_{2}".format(1, 2, 3, *args, **kwargs) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP030 +58 | +59 | "{1}_{0}".format(1, 2, *args) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +54 54 | +55 55 | "{0}_{1}_{2}".format(1, 2, 3, **kwargs) +56 56 | +57 |-"{0}_{1}_{2}".format(1, 2, 3, *args, **kwargs) + 57 |+"{}_{}_{}".format(1, 2, 3, *args, **kwargs) +58 58 | +59 59 | "{1}_{0}".format(1, 2, *args) +60 60 | + +UP030_0.py:59:1: UP030 [*] Use implicit references for positional format fields + | +57 | "{0}_{1}_{2}".format(1, 2, 3, *args, **kwargs) +58 | +59 | "{1}_{0}".format(1, 2, *args) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP030 +60 | +61 | "{1}_{0}".format(1, 2) + | + = help: Remove explicit positional indices + +ℹ Suggested fix +56 56 | +57 57 | "{0}_{1}_{2}".format(1, 2, 3, *args, **kwargs) +58 58 | +59 |-"{1}_{0}".format(1, 2, *args) + 59 |+"{}_{}".format(2, 1, ) +60 60 | +61 61 | "{1}_{0}".format(1, 2) + +UP030_0.py:61:1: UP030 [*] Use implicit references for positional format fields + | +59 | "{1}_{0}".format(1, 2, *args) +60 | +61 | "{1}_{0}".format(1, 2) + | ^^^^^^^^^^^^^^^^^^^^^^ UP030 + | + = help: Remove explicit positional indices + +ℹ Suggested fix +58 58 | +59 59 | "{1}_{0}".format(1, 2, *args) +60 60 | +61 |-"{1}_{0}".format(1, 2) + 61 |+"{}_{}".format(2, 1) + diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP030_2.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP030_2.py.snap index 7a965691874954..17d9794a3f28d8 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP030_2.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP030_2.py.snap @@ -85,38 +85,6 @@ UP030_2.py:12:1: UP030 [*] Use implicit references for positional format fields 14 14 | "{1}_{0}".format(*args) 15 15 | -UP030_2.py:14:1: UP030 Use implicit references for positional format fields - | -12 | "{0}_{1}".format(1, *args) -13 | -14 | "{1}_{0}".format(*args) - | ^^^^^^^^^^^^^^^^^^^^^^^ UP030 -15 | -16 | "{1}_{0}".format(1, *args) - | - = help: Remove explicit positional indices - -UP030_2.py:16:1: UP030 [*] Use implicit references for positional format fields - | -14 | "{1}_{0}".format(*args) -15 | -16 | "{1}_{0}".format(1, *args) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ UP030 -17 | -18 | "{0}_{1}".format(1, 2, *args) - | - = help: Remove explicit positional indices - -ℹ Suggested fix -13 13 | -14 14 | "{1}_{0}".format(*args) -15 15 | -16 |-"{1}_{0}".format(1, *args) - 16 |+"{}_{}".format(*args, 1) -17 17 | -18 18 | "{0}_{1}".format(1, 2, *args) -19 19 | - UP030_2.py:18:1: UP030 [*] Use implicit references for positional format fields | 16 | "{1}_{0}".format(1, *args)