[ruff] Fix false positives and negatives in RUF010 (#18690)

crates/ruff_linter/resources/test/fixtures/ruff/RUF010.py

@@ -36,5 +36,19 @@ f"{ascii(bla)}"  # OK
 )
 
 
-# OK
+# https://github.com/astral-sh/ruff/issues/16325
 f"{str({})}"
+
+f"{str({} | {})}"
+
+import builtins
+
+f"{builtins.repr(1)}"
+
+f"{repr(1)=}"
+
+f"{repr(lambda: 1)}"
+
+f"{repr(x := 2)}"
+
+f"{str(object=3)}"

crates/ruff_linter/src/cst/matchers.rs

@@ -3,8 +3,8 @@ use anyhow::{Result, bail};
 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,
+    ImportAlias, ImportFrom, ImportNames, IndentedBlock, Lambda, ListComp, Module, SmallStatement,
-    SmallStatement, Statement, Suite, Tuple, With,
+    Statement, Suite, Tuple, With,
 };
 use ruff_python_codegen::Stylist;
 
@@ -104,14 +104,6 @@ pub(crate) fn match_attribute<'a, 'b>(
     }
 }
 
-pub(crate) fn match_name<'a, 'b>(expression: &'a Expression<'b>) -> Result<&'a Name<'b>> {
-    if let Expression::Name(name) = expression {
-        Ok(name)
-    } else {
-        bail!("Expected Expression::Name")
-    }
-}
-
 pub(crate) fn match_arg<'a, 'b>(call: &'a Call<'b>) -> Result<&'a Arg<'b>> {
     if let Some(arg) = call.args.first() {
         Ok(arg)

crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs

@@ -1,17 +1,19 @@
-use anyhow::{Result, bail};
+use std::fmt::Display;
 
+use anyhow::Result;
+
+use libcst_native::{LeftParen, ParenthesizedNode, RightParen};
 use ruff_macros::{ViolationMetadata, derive_message_formats};
-use ruff_python_ast::{self as ast, Arguments, Expr};
+use ruff_python_ast::{self as ast, Expr, OperatorPrecedence};
-use ruff_python_codegen::Stylist;
+use ruff_python_parser::TokenKind;
 use ruff_text_size::Ranged;
 
-use crate::Locator;
 use crate::checkers::ast::Checker;
+use crate::cst::helpers::space;
 use crate::cst::matchers::{
-    match_call_mut, match_formatted_string, match_formatted_string_expression, match_name,
+    match_call_mut, match_formatted_string, match_formatted_string_expression, transform_expression,
-    transform_expression,
 };
-use crate::{AlwaysFixableViolation, Edit, Fix};
+use crate::{Edit, Fix, FixAvailability, Violation};
 
 /// ## What it does
 /// Checks for uses of `str()`, `repr()`, and `ascii()` as explicit type
@@ -40,14 +42,16 @@ use crate::{AlwaysFixableViolation, Edit, Fix};
 #[derive(ViolationMetadata)]
 pub(crate) struct ExplicitFStringTypeConversion;
 
-impl AlwaysFixableViolation for ExplicitFStringTypeConversion {
+impl Violation for ExplicitFStringTypeConversion {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
+
     #[derive_message_formats]
     fn message(&self) -> String {
         "Use explicit conversion flag".to_string()
     }
 
-    fn fix_title(&self) -> String {
+    fn fix_title(&self) -> Option<String> {
-        "Replace with conversion flag".to_string()
+        Some("Replace with conversion flag".to_string())
     }
 }
 
@@ -68,84 +72,142 @@ pub(crate) fn explicit_f_string_type_conversion(checker: &Checker, f_string: &as
             continue;
         }
 
-        let Expr::Call(ast::ExprCall {
+        let Expr::Call(call) = expression.as_ref() else {
-            func,
+            continue;
-            arguments:
+        };
-                Arguments {
+
-                    args,
+        let Some(conversion) = checker
-                    keywords,
+            .semantic()
-                    range: _,
+            .resolve_builtin_symbol(&call.func)
-                    node_index: _,
+            .and_then(Conversion::from_str)
-                },
-            ..
-        }) = expression.as_ref()
         else {
             continue;
         };
+        let arg = match conversion {
+            // Handles the cases: `f"{str(object=arg)}"` and `f"{str(arg)}"`
+            Conversion::Str if call.arguments.len() == 1 => {
+                let Some(arg) = call.arguments.find_argument_value("object", 0) else {
+                    continue;
+                };
+                arg
+            }
+            Conversion::Str | Conversion::Repr | Conversion::Ascii => {
+                // Can't be a conversion otherwise.
+                if !call.arguments.keywords.is_empty() {
+                    continue;
+                }
 
-        // Can't be a conversion otherwise.
+                // Can't be a conversion otherwise.
-        if !keywords.is_empty() {
+                let [arg] = call.arguments.args.as_ref() else {
-            continue;
+                    continue;
-        }
+                };
-
+                arg
-        // Can't be a conversion otherwise.
+            }
-        let [arg] = &**args else {
-            continue;
         };
 
-        // Avoid attempting to rewrite, e.g., `f"{str({})}"`; the curly braces are problematic.
+        // Suppress lint for starred expressions.
-        if matches!(
+        if arg.is_starred_expr() {
-            arg,
+            return;
-            Expr::Dict(_) | Expr::Set(_) | Expr::DictComp(_) | Expr::SetComp(_)
-        ) {
-            continue;
-        }
-
-        if !checker
-            .semantic()
-            .resolve_builtin_symbol(func)
-            .is_some_and(|builtin| matches!(builtin, "str" | "repr" | "ascii"))
-        {
-            continue;
         }
 
         let mut diagnostic =
             checker.report_diagnostic(ExplicitFStringTypeConversion, expression.range());
+
+        // Don't support fixing f-string with debug text.
+        if element
+            .as_interpolation()
+            .is_some_and(|interpolation| interpolation.debug_text.is_some())
+        {
+            return;
+        }
+
         diagnostic.try_set_fix(|| {
-            convert_call_to_conversion_flag(f_string, index, checker.locator(), checker.stylist())
+            convert_call_to_conversion_flag(checker, conversion, f_string, index, arg)
         });
     }
 }
 
 /// Generate a [`Fix`] to replace an explicit type conversion with a conversion flag.
 fn convert_call_to_conversion_flag(
+    checker: &Checker,
+    conversion: Conversion,
     f_string: &ast::FString,
     index: usize,
-    locator: &Locator,
+    arg: &Expr,
-    stylist: &Stylist,
 ) -> Result<Fix> {
-    let source_code = locator.slice(f_string);
+    let source_code = checker.locator().slice(f_string);
-    transform_expression(source_code, stylist, |mut expression| {
+    transform_expression(source_code, checker.stylist(), |mut expression| {
         let formatted_string = match_formatted_string(&mut expression)?;
         // Replace the formatted call expression at `index` with a conversion flag.
         let formatted_string_expression =
             match_formatted_string_expression(&mut formatted_string.parts[index])?;
         let call = match_call_mut(&mut formatted_string_expression.expression)?;
-        let name = match_name(&call.func)?;
+
-        match name.value {
+        formatted_string_expression.conversion = Some(conversion.as_str());
-            "str" => {
+
-                formatted_string_expression.conversion = Some("s");
+        if starts_with_brace(checker, arg) {
-            }
+            formatted_string_expression.whitespace_before_expression = space();
-            "repr" => {
-                formatted_string_expression.conversion = Some("r");
-            }
-            "ascii" => {
-                formatted_string_expression.conversion = Some("a");
-            }
-            _ => bail!("Unexpected function call: `{:?}`", name.value),
         }
-        formatted_string_expression.expression = call.args[0].value.clone();
+
+        formatted_string_expression.expression = if needs_paren(OperatorPrecedence::from_expr(arg))
+        {
+            call.args[0]
+                .value
+                .clone()
+                .with_parens(LeftParen::default(), RightParen::default())
+        } else {
+            call.args[0].value.clone()
+        };
+
         Ok(expression)
     })
     .map(|output| Fix::safe_edit(Edit::range_replacement(output, f_string.range())))
 }
+
+fn starts_with_brace(checker: &Checker, arg: &Expr) -> bool {
+    checker
+        .tokens()
+        .in_range(arg.range())
+        .iter()
+        // Skip the trivia tokens
+        .find(|token| !token.kind().is_trivia())
+        .is_some_and(|token| matches!(token.kind(), TokenKind::Lbrace))
+}
+
+fn needs_paren(precedence: OperatorPrecedence) -> bool {
+    precedence <= OperatorPrecedence::Lambda
+}
+
+/// Represents the three built-in Python conversion functions that can be replaced
+/// with f-string conversion flags.
+#[derive(Copy, Clone)]
+enum Conversion {
+    Ascii,
+    Str,
+    Repr,
+}
+
+impl Conversion {
+    fn from_str(value: &str) -> Option<Self> {
+        Some(match value {
+            "ascii" => Self::Ascii,
+            "str" => Self::Str,
+            "repr" => Self::Repr,
+            _ => return None,
+        })
+    }
+
+    fn as_str(self) -> &'static str {
+        match self {
+            Conversion::Ascii => "a",
+            Conversion::Str => "s",
+            Conversion::Repr => "r",
+        }
+    }
+}
+
+impl Display for Conversion {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.as_str())
+    }
+}

crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF010_RUF010.py.snap

@@ -245,3 +245,133 @@ RUF010.py:35:20: RUF010 [*] Use explicit conversion flag
 36 36 | )
 37 37 | 
 38 38 | 
+
+RUF010.py:40:4: RUF010 [*] Use explicit conversion flag
+   |
+39 | # https://github.com/astral-sh/ruff/issues/16325
+40 | f"{str({})}"
+   |    ^^^^^^^ RUF010
+41 |
+42 | f"{str({} | {})}"
+   |
+   = help: Replace with conversion flag
+
+ℹ Safe fix
+37 37 | 
+38 38 | 
+39 39 | # https://github.com/astral-sh/ruff/issues/16325
+40    |-f"{str({})}"
+   40 |+f"{ {}!s}"
+41 41 | 
+42 42 | f"{str({} | {})}"
+43 43 | 
+
+RUF010.py:42:4: RUF010 [*] Use explicit conversion flag
+   |
+40 | f"{str({})}"
+41 |
+42 | f"{str({} | {})}"
+   |    ^^^^^^^^^^^^ RUF010
+43 |
+44 | import builtins
+   |
+   = help: Replace with conversion flag
+
+ℹ Safe fix
+39 39 | # https://github.com/astral-sh/ruff/issues/16325
+40 40 | f"{str({})}"
+41 41 | 
+42    |-f"{str({} | {})}"
+   42 |+f"{ {} | {}!s}"
+43 43 | 
+44 44 | import builtins
+45 45 | 
+
+RUF010.py:46:4: RUF010 [*] Use explicit conversion flag
+   |
+44 | import builtins
+45 |
+46 | f"{builtins.repr(1)}"
+   |    ^^^^^^^^^^^^^^^^ RUF010
+47 |
+48 | f"{repr(1)=}"
+   |
+   = help: Replace with conversion flag
+
+ℹ Safe fix
+43 43 | 
+44 44 | import builtins
+45 45 | 
+46    |-f"{builtins.repr(1)}"
+   46 |+f"{1!r}"
+47 47 | 
+48 48 | f"{repr(1)=}"
+49 49 | 
+
+RUF010.py:48:4: RUF010 Use explicit conversion flag
+   |
+46 | f"{builtins.repr(1)}"
+47 |
+48 | f"{repr(1)=}"
+   |    ^^^^^^^ RUF010
+49 |
+50 | f"{repr(lambda: 1)}"
+   |
+   = help: Replace with conversion flag
+
+RUF010.py:50:4: RUF010 [*] Use explicit conversion flag
+   |
+48 | f"{repr(1)=}"
+49 |
+50 | f"{repr(lambda: 1)}"
+   |    ^^^^^^^^^^^^^^^ RUF010
+51 |
+52 | f"{repr(x := 2)}"
+   |
+   = help: Replace with conversion flag
+
+ℹ Safe fix
+47 47 | 
+48 48 | f"{repr(1)=}"
+49 49 | 
+50    |-f"{repr(lambda: 1)}"
+   50 |+f"{(lambda: 1)!r}"
+51 51 | 
+52 52 | f"{repr(x := 2)}"
+53 53 | 
+
+RUF010.py:52:4: RUF010 [*] Use explicit conversion flag
+   |
+50 | f"{repr(lambda: 1)}"
+51 |
+52 | f"{repr(x := 2)}"
+   |    ^^^^^^^^^^^^ RUF010
+53 |
+54 | f"{str(object=3)}"
+   |
+   = help: Replace with conversion flag
+
+ℹ Safe fix
+49 49 | 
+50 50 | f"{repr(lambda: 1)}"
+51 51 | 
+52    |-f"{repr(x := 2)}"
+   52 |+f"{(x := 2)!r}"
+53 53 | 
+54 54 | f"{str(object=3)}"
+
+RUF010.py:54:4: RUF010 [*] Use explicit conversion flag
+   |
+52 | f"{repr(x := 2)}"
+53 |
+54 | f"{str(object=3)}"
+   |    ^^^^^^^^^^^^^ RUF010
+   |
+   = help: Replace with conversion flag
+
+ℹ Safe fix
+51 51 | 
+52 52 | f"{repr(x := 2)}"
+53 53 | 
+54    |-f"{str(object=3)}"
+   54 |+f"{3!s}"