[`ruff`] Fix false positives and negatives in `RUF010` by LaBatata101 · Pull Request #18690 · astral-sh/ruff

LaBatata101 · 2025-06-15T23:25:35Z

Summary

The fix is suppressed if the f-string has debug text or call expression arguments contains a starred expression, ex:

f"{ascii(1)=}"
f"{ascii(*arg)}"

Fixes #16325

Test Plan

Add regression tests

LaBatata101 · 2025-06-15T23:26:41Z

I don't get why the rule is not being trigged for the tests I added when ran with cargo test. But it's fine in a standalone file.

f"{str({})}"

f"{ascii({} | {})}"

import builtins

f"{builtins.repr(1)}"

f"{ascii(1)=}"

f"{ascii(lambda: 1)}"

f"{ascii(x := 2)}"

Running with cargo run -p ruff -- check sample.py --preview --no-cache --select RUF010 give me these diagnostics

sample2.py:1:4: RUF010 [*] Use explicit conversion flag
  |
1 | f"{str({})}"
  |    ^^^^^^^ RUF010
2 |
3 | f"{ascii({} | {})}"
  |
  = help: Replace with conversion flag

sample2.py:3:4: RUF010 [*] Use explicit conversion flag
  |
1 | f"{str({})}"
2 |
3 | f"{ascii({} | {})}"
  |    ^^^^^^^^^^^^^^ RUF010
4 |
5 | import builtins
  |
  = help: Replace with conversion flag

sample2.py:7:4: RUF010 [*] Use explicit conversion flag
  |
5 | import builtins
6 |
7 | f"{builtins.repr(1)}"
  |    ^^^^^^^^^^^^^^^^ RUF010
8 |
9 | f"{ascii(1)=}"
  |
  = help: Replace with conversion flag

sample2.py:9:4: RUF010 Use explicit conversion flag
   |
 7 | f"{builtins.repr(1)}"
 8 |
 9 | f"{ascii(1)=}"
   |    ^^^^^^^^ RUF010
10 |
11 | f"{ascii(lambda: 1)}"
   |
   = help: Replace with conversion flag

sample2.py:11:4: RUF010 [*] Use explicit conversion flag
   |
 9 | f"{ascii(1)=}"
10 |
11 | f"{ascii(lambda: 1)}"
   |    ^^^^^^^^^^^^^^^^ RUF010
12 |
13 | f"{ascii(x := 2)}"
   |
   = help: Replace with conversion flag

sample2.py:13:4: RUF010 [*] Use explicit conversion flag
   |
11 | f"{ascii(lambda: 1)}"
12 |
13 | f"{ascii(x := 2)}"
   |    ^^^^^^^^^^^^^ RUF010
   |
   = help: Replace with conversion flag

Found 6 errors.
[*] 5 fixable with the `--fix` option.

But in the tests it only flags for the f"{str({})}" and f"{builtins.repr(1)}".

chirizxc · 2025-06-16T08:23:40Z

I don't get why the rule is not being trigged for the tests I added when ran with cargo test. But it's fine in a standalone file.

i think you forgot to add a new snapshot

github-actions · 2025-06-16T14:13:57Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

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

MichaReiser · 2025-06-23T08:28:53Z

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

+    let contains_curly_brace = checker
+        .tokens()
+        .in_range(arg.range())


I don't think this is correct. E.g we don't want to add a space for an expression like:

1 if b({ "key": "test" }) else 10

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

MichaReiser · 2025-06-24T08:40:57Z

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

+        if contains_brace(&call.args[0].value) {
+            formatted_string_expression.whitespace_before_expression = space();
+        }
+
+        formatted_string_expression.expression = if needs_paren(&call.args[0].value) {
+            call.args[0]
+                .value
+                .clone()
+                .with_parens(LeftParen::default(), RightParen::default())
+        } else {
+            call.args[0].value.clone()
+        };
+


Can't we move this into the map arm and make it based on Ruff's input AST?

I think we can't do that without reparsing the f-string expression, because in the map arm we only have the generated string of the fstring CST node.

but can't we inspect the f_string variable?

We can, but I don't see how it is going to help. In the map we already have the built fix string, and in this case we don't want to parenthesize the entire f-string, just a part of it.

That's what I had in mind:

Subject: [PATCH] Rename `knot_benchmark` to `ty_benchmark` --- Index: crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs b/crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs --- a/crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs (revision 13921af61d95c86f1cddfbad51471c0654b72162) +++ b/crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs (date 1750839908748) @@ -122,7 +122,7 @@ } diagnostic.try_set_fix(|| { - convert_call_to_conversion_flag(checker, conversion, f_string, index, element) + convert_call_to_conversion_flag(checker, conversion, f_string, index, arg) }); } } @@ -133,7 +133,7 @@ conversion: Conversion, f_string: &ast::FString, index: usize, - element: &InterpolatedStringElement, + arg: &Expr, ) -> Result<Fix> { let source_code = checker.locator().slice(f_string); transform_expression(source_code, checker.stylist(), |mut expression| { @@ -145,7 +145,7 @@ formatted_string_expression.conversion = Some(conversion.as_str()); - if contains_brace(checker, element) { + if starts_with_lbrace(checker, arg) { formatted_string_expression.whitespace_before_expression = space(); } @@ -163,26 +163,19 @@ .map(|output| Fix::safe_edit(Edit::range_replacement(output, f_string.range()))) } -fn contains_brace(checker: &Checker, element: &InterpolatedStringElement) -> bool { - let Some(interpolation) = element.as_interpolation() else { - return false; - }; - let Some(call) = interpolation.expression.as_call_expr() else { - return false; - }; - +fn starts_with_lbrace(checker: &Checker, arg: &Expr) -> bool { checker .tokens() - .after(call.arguments.start()) + .in_range(arg.range()) .iter() // Skip the trivia tokens and the `(` from the arguments .find(|token| !token.kind().is_trivia() && token.kind() != TokenKind::Lpar) .is_some_and(|token| matches!(token.kind(), TokenKind::Lbrace)) } -fn needs_paren(expr: &Expression) -> bool { - matches!(expr, Expression::Lambda(_) | Expression::NamedExpr(_)) -} +// fn needs_paren(expr: &Expr) -> bool { +// matches!(expr, Expr::Lambda(_) | Expr::Named(_)) +// } /// Represents the three built-in Python conversion functions that can be replaced /// with f-string conversion flags.

MichaReiser · 2025-06-25T08:23:14Z

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

+fn needs_paren(expr: &Expression) -> bool {
+    matches!(expr, Expression::Lambda(_) | Expression::NamedExpr(_))
+}


We should add tests demonstrating this behavior. I think this is also another place where we could use OperatorPrecedence instead of listing all expressions where any precedence lower or equal to lambda require parenthesizing.

We should add tests demonstrating this behavior.

We already have it, here:

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

Lines 50 to 52 in 13921af

f"{ascii(lambda: 1)}"

f"{ascii(x := 2)}"

It might then be unnecessary? Because I don't see any failing tests if I change the code to:

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(); // };

Oh, the diagnostic was not triggering for those cases in the test file because the ascii binding is being shadowed. I didn't notice that, thanks for pointing that out! Should be fixed now.

MichaReiser · 2025-06-25T08:26:00Z

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

+        if contains_brace(&call.args[0].value) {
+            formatted_string_expression.whitespace_before_expression = space();
+        }
+
+        formatted_string_expression.expression = if needs_paren(&call.args[0].value) {
+            call.args[0]
+                .value
+                .clone()
+                .with_parens(LeftParen::default(), RightParen::default())
+        } else {
+            call.args[0].value.clone()
+        };
+


That's what I had in mind:

Subject: [PATCH] Rename `knot_benchmark` to `ty_benchmark` --- Index: crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs b/crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs --- a/crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs (revision 13921af61d95c86f1cddfbad51471c0654b72162) +++ b/crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs (date 1750839908748) @@ -122,7 +122,7 @@ } diagnostic.try_set_fix(|| { - convert_call_to_conversion_flag(checker, conversion, f_string, index, element) + convert_call_to_conversion_flag(checker, conversion, f_string, index, arg) }); } } @@ -133,7 +133,7 @@ conversion: Conversion, f_string: &ast::FString, index: usize, - element: &InterpolatedStringElement, + arg: &Expr, ) -> Result<Fix> { let source_code = checker.locator().slice(f_string); transform_expression(source_code, checker.stylist(), |mut expression| { @@ -145,7 +145,7 @@ formatted_string_expression.conversion = Some(conversion.as_str()); - if contains_brace(checker, element) { + if starts_with_lbrace(checker, arg) { formatted_string_expression.whitespace_before_expression = space(); } @@ -163,26 +163,19 @@ .map(|output| Fix::safe_edit(Edit::range_replacement(output, f_string.range()))) } -fn contains_brace(checker: &Checker, element: &InterpolatedStringElement) -> bool { - let Some(interpolation) = element.as_interpolation() else { - return false; - }; - let Some(call) = interpolation.expression.as_call_expr() else { - return false; - }; - +fn starts_with_lbrace(checker: &Checker, arg: &Expr) -> bool { checker .tokens() - .after(call.arguments.start()) + .in_range(arg.range()) .iter() // Skip the trivia tokens and the `(` from the arguments .find(|token| !token.kind().is_trivia() && token.kind() != TokenKind::Lpar) .is_some_and(|token| matches!(token.kind(), TokenKind::Lbrace)) } -fn needs_paren(expr: &Expression) -> bool { - matches!(expr, Expression::Lambda(_) | Expression::NamedExpr(_)) -} +// fn needs_paren(expr: &Expr) -> bool { +// matches!(expr, Expr::Lambda(_) | Expr::Named(_)) +// } /// Represents the three built-in Python conversion functions that can be replaced /// with f-string conversion flags.

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

MichaReiser

Thank you. This is great!

* main: [ty] Add builtins to completions derived from scope (#18982) [ty] Don't add incorrect subdiagnostic for unresolved reference (#18487) [ty] Simplify `KnownClass::check_call()` and `KnownFunction::check_call()` (#18981) [ty] Add micro-benchmark for #711 (#18979) [`flake8-annotations`] Make `ANN401` example error out-of-the-box (#18974) [`flake8-async`] Make `ASYNC110` example error out-of-the-box (#18975) [pandas]: Fix issue on `non pandas` dataframe `in-place` usage (PD002) (#18963) [`pylint`] Fix `PLC0415` example (#18970) [ty] Add environment variable to dump Salsa memory usage stats (#18928) [`pylint`] Fix `PLW0108` autofix introducing a syntax error when the lambda's body contains an assignment expression (#18678) Bump 0.12.1 (#18969) [`FastAPI`] Add fix safety section to `FAST002` (#18940) [ty] Add regression test for leading tab mis-alignment in diagnostic rendering (#18965) [ty] Resolve python environment in `Options::to_program_settings` (#18960) [`ruff`] Fix false positives and negatives in `RUF010` (#18690) [ty] Fix rendering of long lines that are indented with tabs [ty] Add regression test for diagnostic rendering panic [ty] Move venv and conda env discovery to `SearchPath::from_settings` (#18938)

[ruff] Fix false positives and negatives in RUF010

eb39e30

Suppress fix for starred expressions

21edcd0

LaBatata101 marked this pull request as ready for review June 16, 2025 14:06

LaBatata101 added 2 commits June 16, 2025 18:21

Fix false negative

9090ea3

fix typo

9c2bce2

MichaReiser added the fixes Related to suggested fixes for violations label Jun 17, 2025

MichaReiser reviewed Jun 20, 2025

View reviewed changes

Address feedback

0af2362

LaBatata101 requested a review from MichaReiser June 20, 2025 14:46

MichaReiser reviewed Jun 23, 2025

View reviewed changes

Update code

568f4f4

MichaReiser reviewed Jun 24, 2025

View reviewed changes

crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Jun 24, 2025

View reviewed changes

crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Jun 24, 2025

View reviewed changes

Refactor code

13921af

LaBatata101 requested a review from MichaReiser June 24, 2025 14:18

MichaReiser reviewed Jun 25, 2025

View reviewed changes

Update code

37dc947

LaBatata101 requested a review from MichaReiser June 25, 2025 22:13

MichaReiser reviewed Jun 26, 2025

View reviewed changes

crates/ruff_linter/src/rules/ruff/rules/explicit_f_string_type_conversion.rs Show resolved Hide resolved

LaBatata101 added 2 commits June 26, 2025 11:01

Fix tests

e8ecbb6

Remove unnecessary check

a1b5012

MichaReiser approved these changes Jun 26, 2025

View reviewed changes

MichaReiser added rule Implementing or modifying a lint rule bug Something isn't working and removed fixes Related to suggested fixes for violations labels Jun 26, 2025

MichaReiser merged commit d006976 into astral-sh:main Jun 26, 2025
36 checks passed

LaBatata101 deleted the fix-RUF010 branch June 26, 2025 20:27

BrewTestBot mentioned this pull request Jun 26, 2025

ruff 0.12.1 Homebrew/homebrew-core#228300

Merged

dscorbett mentioned this pull request Sep 22, 2025

RUF010 diagnostic is unhelpful for an interpolation with debug text #20519

Closed

Comments

Conversation

LaBatata101 commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

LaBatata101 commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chirizxc commented Jun 16, 2025

Uh oh!

github-actions bot commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaBatata101 Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaBatata101 Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaBatata101 Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LaBatata101 commented Jun 15, 2025 •

edited

Loading

LaBatata101 commented Jun 15, 2025 •

edited

Loading

github-actions bot commented Jun 16, 2025 •

edited

Loading

`ruff-ecosystem` results

LaBatata101 Jun 25, 2025 •

edited

Loading

LaBatata101 Jun 25, 2025 •

edited

Loading

LaBatata101 Jun 26, 2025 •

edited

Loading