From 415342e5bbd39ec0084ccd7615a42ea1bfd41e93 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 20 Jun 2023 19:19:36 -0400 Subject: [PATCH] Revert change to `RUF010` to remove unnecessary `str` calls This reverts commit aba073a7911bdc1bb08dc4bcb0feeb5448535db0. --- .../resources/test/fixtures/ruff/RUF010.py | 9 - .../explicit_f_string_type_conversion.rs | 213 ++++-------------- ..._rules__ruff__tests__RUF010_RUF010.py.snap | 122 +--------- 3 files changed, 49 insertions(+), 295 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF010.py b/crates/ruff/resources/test/fixtures/ruff/RUF010.py index 6416b79c9e3d3..77e459c21496a 100644 --- a/crates/ruff/resources/test/fixtures/ruff/RUF010.py +++ b/crates/ruff/resources/test/fixtures/ruff/RUF010.py @@ -34,12 +34,3 @@ def ascii(arg): " intermediary content " f" that flows {repr(obj)} of type {type(obj)}.{additional_message}" # RUF010 ) - - -f"{str(bla)}" # RUF010 - -f"{str(bla):20}" # RUF010 - -f"{bla!s}" # RUF010 - -f"{bla!s:20}" # OK diff --git a/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs b/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs index 513843feffe86..b87135e0bd887 100644 --- a/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs +++ b/crates/ruff/src/rules/ruff/rules/explicit_f_string_type_conversion.rs @@ -6,7 +6,6 @@ use rustpython_parser::ast::{self, Expr, Ranged}; use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::prelude::ConversionFlag; use ruff_python_ast::source_code::{Locator, Stylist}; use crate::autofix::codemods::CodegenStylist; @@ -22,62 +21,36 @@ use crate::registry::AsRule; /// f-strings support dedicated conversion flags for these types, which are /// more succinct and idiomatic. /// -/// In the case of `str()`, it's also redundant, since `str()` is the default -/// conversion. +/// Note that, in many cases, calling `str()` within an f-string is +/// unnecessary and can be removed entirely, as the value will be converted +/// to a string automatically, the notable exception being for classes that +/// implement a custom `__format__` method. /// /// ## Example /// ```python /// a = "some string" -/// f"{str(a)}" /// f"{repr(a)}" /// ``` /// /// Use instead: /// ```python /// a = "some string" -/// f"{a}" /// f"{a!r}" /// ``` #[violation] -pub struct ExplicitFStringTypeConversion { - operation: Operation, -} +pub struct ExplicitFStringTypeConversion; impl AlwaysAutofixableViolation for ExplicitFStringTypeConversion { #[derive_message_formats] fn message(&self) -> String { - let ExplicitFStringTypeConversion { operation } = self; - match operation { - Operation::ConvertCallToConversionFlag => { - format!("Use explicit conversion flag") - } - Operation::RemoveCall => format!("Remove unnecessary `str` conversion"), - Operation::RemoveConversionFlag => format!("Remove unnecessary conversion flag"), - } + format!("Use explicit conversion flag") } fn autofix_title(&self) -> String { - let ExplicitFStringTypeConversion { operation } = self; - match operation { - Operation::ConvertCallToConversionFlag => { - format!("Replace with conversion flag") - } - Operation::RemoveCall => format!("Remove `str` call"), - Operation::RemoveConversionFlag => format!("Remove conversion flag"), - } + "Replace with conversion flag".to_string() } } -#[derive(Debug, PartialEq, Eq)] -enum Operation { - /// Ex) Convert `f"{repr(bla)}"` to `f"{bla!r}"` - ConvertCallToConversionFlag, - /// Ex) Convert `f"{bla!s}"` to `f"{bla}"` - RemoveConversionFlag, - /// Ex) Convert `f"{str(bla)}"` to `f"{bla}"` - RemoveCall, -} - /// RUF010 pub(crate) fn explicit_f_string_type_conversion( checker: &mut Checker, @@ -96,156 +69,50 @@ pub(crate) fn explicit_f_string_type_conversion( .enumerate() { let ast::ExprFormattedValue { - value, - conversion, - format_spec, - range: _, + value, conversion, .. } = formatted_value; - match conversion { - ConversionFlag::Ascii | ConversionFlag::Repr => { - // Nothing to do. - continue; - } - ConversionFlag::Str => { - // Skip if there's a format spec. - if format_spec.is_some() { - continue; - } - - // Remove the conversion flag entirely. - // Ex) `f"{bla!s}"` - let mut diagnostic = Diagnostic::new( - ExplicitFStringTypeConversion { - operation: Operation::RemoveConversionFlag, - }, - value.range(), - ); - if checker.patch(diagnostic.kind.rule()) { - diagnostic.try_set_fix(|| { - remove_conversion_flag(expr, index, checker.locator, checker.stylist) - }); - } - checker.diagnostics.push(diagnostic); - } - ConversionFlag::None => { - // Replace with the appropriate conversion flag. - let Expr::Call(ast::ExprCall { - func, - args, - keywords, - .. - }) = value.as_ref() else { - continue; - }; + // Skip if there's already a conversion flag. + if !conversion.is_none() { + continue; + } - // Can't be a conversion otherwise. - if args.len() != 1 || !keywords.is_empty() { - continue; - } + let Expr::Call(ast::ExprCall { + func, + args, + keywords, + .. + }) = value.as_ref() else { + continue; + }; + + // Can't be a conversion otherwise. + if args.len() != 1 || !keywords.is_empty() { + continue; + } - let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { - continue; - }; + let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() else { + continue; + }; - if !matches!(id.as_str(), "str" | "repr" | "ascii") { - continue; - }; + if !matches!(id.as_str(), "str" | "repr" | "ascii") { + continue; + }; - if !checker.semantic().is_builtin(id) { - continue; - } + if !checker.semantic().is_builtin(id) { + continue; + } - if id == "str" && format_spec.is_none() { - // Ex) `f"{str(bla)}"` - let mut diagnostic = Diagnostic::new( - ExplicitFStringTypeConversion { - operation: Operation::RemoveCall, - }, - value.range(), - ); - if checker.patch(diagnostic.kind.rule()) { - diagnostic.try_set_fix(|| { - remove_conversion_call(expr, index, checker.locator, checker.stylist) - }); - } - checker.diagnostics.push(diagnostic); - } else { - // Ex) `f"{repr(bla)}"` - let mut diagnostic = Diagnostic::new( - ExplicitFStringTypeConversion { - operation: Operation::ConvertCallToConversionFlag, - }, - value.range(), - ); - if checker.patch(diagnostic.kind.rule()) { - diagnostic.try_set_fix(|| { - convert_call_to_conversion_flag( - expr, - index, - checker.locator, - checker.stylist, - ) - }); - } - checker.diagnostics.push(diagnostic); - } - } + let mut diagnostic = Diagnostic::new(ExplicitFStringTypeConversion, value.range()); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.try_set_fix(|| { + convert_call_to_conversion_flag(expr, index, checker.locator, checker.stylist) + }); } + checker.diagnostics.push(diagnostic); } } -/// Generate a [`Fix`] to remove a conversion flag from a formatted expression. -fn remove_conversion_flag( - expr: &Expr, - index: usize, - locator: &Locator, - stylist: &Stylist, -) -> Result { - // Parenthesize the expression, to support implicit concatenation. - let range = expr.range(); - let content = locator.slice(range); - let parenthesized_content = format!("({content})"); - let mut expression = match_expression(&parenthesized_content)?; - - // Replace the formatted call expression at `index` with a conversion flag. - let formatted_string_expression = match_part(index, &mut expression)?; - formatted_string_expression.conversion = None; - - // Remove the parentheses (first and last characters). - let mut content = expression.codegen_stylist(stylist); - content.remove(0); - content.pop(); - - Ok(Fix::automatic(Edit::range_replacement(content, range))) -} - -/// Generate a [`Fix`] to remove a call from a formatted expression. -fn remove_conversion_call( - expr: &Expr, - index: usize, - locator: &Locator, - stylist: &Stylist, -) -> Result { - // Parenthesize the expression, to support implicit concatenation. - let range = expr.range(); - let content = locator.slice(range); - let parenthesized_content = format!("({content})"); - let mut expression = match_expression(&parenthesized_content)?; - - // Replace the formatted call expression at `index` with a conversion flag. - let formatted_string_expression = match_part(index, &mut expression)?; - let call = match_call_mut(&mut formatted_string_expression.expression)?; - formatted_string_expression.expression = call.args[0].value.clone(); - - // Remove the parentheses (first and last characters). - let mut content = expression.codegen_stylist(stylist); - content.remove(0); - content.pop(); - - Ok(Fix::automatic(Edit::range_replacement(content, range))) -} - /// Generate a [`Fix`] to replace an explicit type conversion with a conversion flag. fn convert_call_to_conversion_flag( expr: &Expr, diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF010_RUF010.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF010_RUF010.py.snap index 2f0133295c7fe..ffbb12608b6f9 100644 --- a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF010_RUF010.py.snap +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF010_RUF010.py.snap @@ -1,21 +1,21 @@ --- source: crates/ruff/src/rules/ruff/mod.rs --- -RUF010.py:9:4: RUF010 [*] Remove unnecessary `str` conversion +RUF010.py:9:4: RUF010 [*] Use explicit conversion flag | 9 | f"{str(bla)}, {repr(bla)}, {ascii(bla)}" # RUF010 | ^^^^^^^^ RUF010 10 | 11 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 | - = help: Remove `str` call + = help: Replace with conversion flag ℹ Fix 6 6 | pass 7 7 | 8 8 | 9 |-f"{str(bla)}, {repr(bla)}, {ascii(bla)}" # RUF010 - 9 |+f"{bla}, {repr(bla)}, {ascii(bla)}" # RUF010 + 9 |+f"{bla!s}, {repr(bla)}, {ascii(bla)}" # RUF010 10 10 | 11 11 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 12 12 | @@ -58,7 +58,7 @@ RUF010.py:9:29: RUF010 [*] Use explicit conversion flag 11 11 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 12 12 | -RUF010.py:11:4: RUF010 [*] Remove unnecessary `str` conversion +RUF010.py:11:4: RUF010 [*] Use explicit conversion flag | 9 | f"{str(bla)}, {repr(bla)}, {ascii(bla)}" # RUF010 10 | @@ -67,14 +67,14 @@ RUF010.py:11:4: RUF010 [*] Remove unnecessary `str` conversion 12 | 13 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 | - = help: Remove `str` call + = help: Replace with conversion flag ℹ Fix 8 8 | 9 9 | f"{str(bla)}, {repr(bla)}, {ascii(bla)}" # RUF010 10 10 | 11 |-f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 - 11 |+f"{d['a']}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 + 11 |+f"{d['a']!s}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 12 12 | 13 13 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 14 14 | @@ -121,7 +121,7 @@ RUF010.py:11:35: RUF010 [*] Use explicit conversion flag 13 13 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 14 14 | -RUF010.py:13:5: RUF010 [*] Remove unnecessary `str` conversion +RUF010.py:13:5: RUF010 [*] Use explicit conversion flag | 11 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 12 | @@ -130,14 +130,14 @@ RUF010.py:13:5: RUF010 [*] Remove unnecessary `str` conversion 14 | 15 | f"{bla!s}, {(repr(bla))}, {(ascii(bla))}" # RUF010 | - = help: Remove `str` call + = help: Replace with conversion flag ℹ Fix 10 10 | 11 11 | f"{str(d['a'])}, {repr(d['b'])}, {ascii(d['c'])}" # RUF010 12 12 | 13 |-f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 - 13 |+f"{bla}, {(repr(bla))}, {(ascii(bla))}" # RUF010 + 13 |+f"{bla!s}, {(repr(bla))}, {(ascii(bla))}" # RUF010 14 14 | 15 15 | f"{bla!s}, {(repr(bla))}, {(ascii(bla))}" # RUF010 16 16 | @@ -184,27 +184,6 @@ RUF010.py:13:34: RUF010 [*] Use explicit conversion flag 15 15 | f"{bla!s}, {(repr(bla))}, {(ascii(bla))}" # RUF010 16 16 | -RUF010.py:15:4: RUF010 [*] Remove unnecessary conversion flag - | -13 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 -14 | -15 | f"{bla!s}, {(repr(bla))}, {(ascii(bla))}" # RUF010 - | ^^^ RUF010 -16 | -17 | f"{foo(bla)}" # OK - | - = help: Remove conversion flag - -ℹ Fix -12 12 | -13 13 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 -14 14 | -15 |-f"{bla!s}, {(repr(bla))}, {(ascii(bla))}" # RUF010 - 15 |+f"{bla}, {(repr(bla))}, {(ascii(bla))}" # RUF010 -16 16 | -17 17 | f"{foo(bla)}" # OK -18 18 | - RUF010.py:15:14: RUF010 [*] Use explicit conversion flag | 13 | f"{(str(bla))}, {(repr(bla))}, {(ascii(bla))}" # RUF010 @@ -247,27 +226,6 @@ RUF010.py:15:29: RUF010 [*] Use explicit conversion flag 17 17 | f"{foo(bla)}" # OK 18 18 | -RUF010.py:21:4: RUF010 [*] Remove unnecessary conversion flag - | -19 | f"{str(bla, 'ascii')}, {str(bla, encoding='cp1255')}" # OK -20 | -21 | f"{bla!s} {[]!r} {'bar'!a}" # OK - | ^^^ RUF010 -22 | -23 | "Not an f-string {str(bla)}, {repr(bla)}, {ascii(bla)}" # OK - | - = help: Remove conversion flag - -ℹ Fix -18 18 | -19 19 | f"{str(bla, 'ascii')}, {str(bla, encoding='cp1255')}" # OK -20 20 | -21 |-f"{bla!s} {[]!r} {'bar'!a}" # OK - 21 |+f"{bla} {[]!r} {'bar'!a}" # OK -22 22 | -23 23 | "Not an f-string {str(bla)}, {repr(bla)}, {ascii(bla)}" # OK -24 24 | - RUF010.py:35:20: RUF010 [*] Use explicit conversion flag | 33 | f"Member of tuple mismatches type at index {i}. Expected {of_shape_i}. Got " @@ -285,67 +243,5 @@ RUF010.py:35:20: RUF010 [*] Use explicit conversion flag 35 |- f" that flows {repr(obj)} of type {type(obj)}.{additional_message}" # RUF010 35 |+ f" that flows {obj!r} of type {type(obj)}.{additional_message}" # RUF010 36 36 | ) -37 37 | -38 38 | - -RUF010.py:39:4: RUF010 [*] Remove unnecessary `str` conversion - | -39 | f"{str(bla)}" # RUF010 - | ^^^^^^^^ RUF010 -40 | -41 | f"{str(bla):20}" # RUF010 - | - = help: Remove `str` call - -ℹ Fix -36 36 | ) -37 37 | -38 38 | -39 |-f"{str(bla)}" # RUF010 - 39 |+f"{bla}" # RUF010 -40 40 | -41 41 | f"{str(bla):20}" # RUF010 -42 42 | - -RUF010.py:41:4: RUF010 [*] Use explicit conversion flag - | -39 | f"{str(bla)}" # RUF010 -40 | -41 | f"{str(bla):20}" # RUF010 - | ^^^^^^^^ RUF010 -42 | -43 | f"{bla!s}" # RUF010 - | - = help: Replace with conversion flag - -ℹ Fix -38 38 | -39 39 | f"{str(bla)}" # RUF010 -40 40 | -41 |-f"{str(bla):20}" # RUF010 - 41 |+f"{bla!s:20}" # RUF010 -42 42 | -43 43 | f"{bla!s}" # RUF010 -44 44 | - -RUF010.py:43:4: RUF010 [*] Remove unnecessary conversion flag - | -41 | f"{str(bla):20}" # RUF010 -42 | -43 | f"{bla!s}" # RUF010 - | ^^^ RUF010 -44 | -45 | f"{bla!s:20}" # OK - | - = help: Remove conversion flag - -ℹ Fix -40 40 | -41 41 | f"{str(bla):20}" # RUF010 -42 42 | -43 |-f"{bla!s}" # RUF010 - 43 |+f"{bla}" # RUF010 -44 44 | -45 45 | f"{bla!s:20}" # OK