[ruff] Treat f-string interpolation as potential side effect in RUF019 (#24426)

anishgirianish · web-flow · commit 87a0f01cfd01 · 2026-04-09T09:36:17.000+02:00
## Summary Fixes #12953 F-string interpolation can call `__format__`/`__str__`/`__repr__`, which may have side effects. RUF019 was applying a safe auto-fix that collapsed two `__str__` calls into one, changing behavior. Added a tri-state `SideEffect` enum (`No`/`Maybe`/`Yes`) and a `side_effect()` function that reuses the existing `any_over_expr` traversal via a new `FnMut` variant (`any_over_expr_mut`), following the approach suggested by @ntBre in the other closed pr tagged in the issue In RUF019, `SideEffect::Maybe` (non-literal f-string interpolation) now produces an unsafe fix instead of a safe one. Literal interpolations like `f"{1}"` remain safe. ## Test Plan - Added f-string fixture cases to `RUF019.py` (non-literal → unsafe, literal → safe, no interpolation → safe). - `cargo nextest run -p ruff_linter` - Ecosystem check (stable + preview)
diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF019.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF019.py
@@ -30,3 +30,26 @@
         d ["key"]
 ):
     ...
+
+# https://github.com/astral-sh/ruff/issues/12953
+# F-string with non-literal interpolation — unsafe fix (may invoke __str__)
+class Formatter:
+    def __str__(self):
+        print("side effect!")
+        return "key"
+
+c = Formatter()
+if f"{c}" in d and d[f"{c}"]:
+    pass
+
+# F-string with only literal interpolation — safe fix
+if f"{1}" in d and d[f"{1}"]:
+    pass
+
+# Plain f-string without interpolation — safe fix
+if f"key" in d and d[f"key"]:
+    pass
+
+# Walrus operator is a side effect — should not emit
+if (k := "key") in d and d[(k := "key")]:
+    pass
diff --git a/crates/ruff_linter/src/rules/ruff/rules/unnecessary_key_check.rs b/crates/ruff_linter/src/rules/ruff/rules/unnecessary_key_check.rs
@@ -3,7 +3,7 @@ use ruff_python_ast::comparable::ComparableExpr;
 use ruff_python_ast::{self as ast, BoolOp, CmpOp, Expr};
 
 use ruff_macros::{ViolationMetadata, derive_message_formats};
-use ruff_python_ast::helpers::contains_effect;
+use ruff_python_ast::helpers::side_effect;
 use ruff_python_ast::token::parenthesized_range;
 use ruff_text_size::Ranged;
 
@@ -31,7 +31,8 @@ use crate::{AlwaysFixableViolation, Edit, Fix};
 /// ```
 ///
 /// ## Fix safety
-/// This rule's fix is marked as safe, unless the expression contains comments.
+/// This rule's fix is marked as safe, unless the expression contains comments
+/// or may have side effects.
 #[derive(ViolationMetadata)]
 #[violation_metadata(stable_since = "v0.2.0")]
 pub(crate) struct UnnecessaryKeyCheck;
@@ -101,19 +102,21 @@ pub(crate) fn unnecessary_key_check(checker: &Checker, expr: &Expr) {
         return;
     }
 
-    if contains_effect(obj_left, |id| checker.semantic().has_builtin_binding(id))
-        || contains_effect(key_left, |id| checker.semantic().has_builtin_binding(id))
-    {
+    let obj_effect = side_effect(obj_left, |id| checker.semantic().has_builtin_binding(id));
+    let key_effect = side_effect(key_left, |id| checker.semantic().has_builtin_binding(id));
+    let combined = obj_effect.merge(key_effect);
+    if combined.is_present() {
         return;
     }
 
     let mut diagnostic = checker.report_diagnostic(UnnecessaryKeyCheck, expr.range());
 
-    let applicability = if checker.comment_ranges().intersects(expr.range()) {
-        Applicability::Unsafe
-    } else {
-        Applicability::Safe
-    };
+    let applicability =
+        if !combined.is_absent() || checker.comment_ranges().intersects(expr.range()) {
+            Applicability::Unsafe
+        } else {
+            Applicability::Safe
+        };
 
     diagnostic.set_fix(Fix::applicable_edit(
         Edit::range_replacement(
diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF019_RUF019.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF019_RUF019.py.snap
@@ -136,4 +136,60 @@ help: Replace with `dict.get`
 28 +         d.get("key")
 29 | ):
 30 |     ...
+31 |
 note: This is an unsafe fix and may change runtime behavior
+
+RUF019 [*] Unnecessary key check before dictionary access
+  --> RUF019.py:42:4
+   |
+41 | c = Formatter()
+42 | if f"{c}" in d and d[f"{c}"]:
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^
+43 |     pass
+   |
+help: Replace with `dict.get`
+39 |         return "key"
+40 |
+41 | c = Formatter()
+   - if f"{c}" in d and d[f"{c}"]:
+42 + if d.get(f"{c}"):
+43 |     pass
+44 |
+45 | # F-string with only literal interpolation — safe fix
+note: This is an unsafe fix and may change runtime behavior
+
+RUF019 [*] Unnecessary key check before dictionary access
+  --> RUF019.py:46:4
+   |
+45 | # F-string with only literal interpolation — safe fix
+46 | if f"{1}" in d and d[f"{1}"]:
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^
+47 |     pass
+   |
+help: Replace with `dict.get`
+43 |     pass
+44 |
+45 | # F-string with only literal interpolation — safe fix
+   - if f"{1}" in d and d[f"{1}"]:
+46 + if d.get(f"{1}"):
+47 |     pass
+48 |
+49 | # Plain f-string without interpolation — safe fix
+
+RUF019 [*] Unnecessary key check before dictionary access
+  --> RUF019.py:50:4
+   |
+49 | # Plain f-string without interpolation — safe fix
+50 | if f"key" in d and d[f"key"]:
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^
+51 |     pass
+   |
+help: Replace with `dict.get`
+47 |     pass
+48 |
+49 | # Plain f-string without interpolation — safe fix
+   - if f"key" in d and d[f"key"]:
+50 + if d.get(f"key"):
+51 |     pass
+52 |
+53 | # Walrus operator is a side effect — should not emit
diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs
