Skip to content

Commit bc52b71

Browse files
[pyupgrade] Avoid converting format calls with more kinds of side effects (UP032) (#25484)
## Summary Fixes part of #15874: UP032 unsoundly converted str.format calls that repeat a side-effecting argument. The check only caught calls, so a repeated d[k] or walrus slipped through. Switch it to contains_effect. ## Test Plan New mdtest at crates/ruff_linter/resources/mdtest/pyupgrade/f-string.md. --------- Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
1 parent 8aaa13e commit bc52b71

2 files changed

Lines changed: 57 additions & 5 deletions

File tree

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
# `f-string` (`UP032`)
2+
3+
```toml
4+
lint.select = ["UP032"]
5+
```
6+
7+
## Repeated arguments with side effects
8+
9+
A `str.format` argument is evaluated once, but an f-string re-evaluates it on every interpolation. So
10+
when an argument is used more than once and can have a side effect, the call is left unconverted to
11+
avoid running that side effect twice. This used to cover only call expressions; it now covers any
12+
side-effecting expression, such as a subscript or a walrus binding. Even a builtin call like `list()`
13+
counts, since the f-string would construct a new object on each interpolation.
14+
15+
```py
16+
def foo(): ...
17+
18+
19+
d = {}
20+
21+
"{x} {x}".format(x=foo())
22+
"{x} {x}".format(x=d["k"])
23+
"{x} {x}".format(x=(y := 1))
24+
"{x.append} {x.append}".format(x=list())
25+
```
26+
27+
## A single use is still converted
28+
29+
When the argument is interpolated once, the side effect runs once either way, so the fix is safe.
30+
31+
```py
32+
def foo(): ...
33+
34+
35+
"{x}".format(x=foo()) # snapshot: f-string
36+
```
37+
38+
```snapshot
39+
error[UP032]: Use f-string instead of `format` call
40+
--> src/mdtest_snippet.py:4:1
41+
|
42+
4 | "{x}".format(x=foo()) # snapshot: f-string
43+
| ^^^^^^^^^^^^^^^^^^^^^
44+
|
45+
help: Convert to f-string
46+
1 | def foo(): ...
47+
2 |
48+
3 |
49+
- "{x}".format(x=foo()) # snapshot: f-string
50+
4 + f"{foo()}" # snapshot: f-string
51+
```

crates/ruff_linter/src/rules/pyupgrade/rules/f_strings.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use anyhow::{Context, Result};
44
use rustc_hash::{FxHashMap, FxHashSet};
55

66
use ruff_macros::{ViolationMetadata, derive_message_formats};
7-
use ruff_python_ast::helpers::any_over_expr;
7+
use ruff_python_ast::helpers::contains_effect;
88
use ruff_python_ast::str::{leading_quote, trailing_quote};
99
use ruff_python_ast::token::TokenKind;
1010
use ruff_python_ast::{self as ast, Expr, Keyword, StringFlags};
@@ -327,10 +327,11 @@ impl FStringConversion {
327327
// string, we can't convert the format string to an f-string. For example,
328328
// converting `"{x} {x}".format(x=foo())` would result in `f"{foo()} {foo()}"`,
329329
// which would call `foo()` twice.
330-
if !seen.insert(specifier) {
331-
if any_over_expr(arg, &Expr::is_call_expr) {
332-
return Ok(Self::SideEffects);
333-
}
330+
//
331+
// This is also true for builtins, so we don't treat them as special when
332+
// checking for effects here.
333+
if !seen.insert(specifier) && contains_effect(arg, |_| false) {
334+
return Ok(Self::SideEffects);
334335
}
335336

336337
converted.push_str(&formatted_expr(

0 commit comments

Comments
 (0)