Skip to content

Commit

Permalink
Remove empty strings when converting to f-string (UP032) (#11524)
Browse files Browse the repository at this point in the history
## Summary

This PR brings back the functionality to remove empty strings when
converting to an f-string in `UP032`.

For context, #8712 added this
functionality to remove _trailing_ empty strings but it got removed in
#8697 possibly unexpectedly so.

There's one difference which is that this PR will remove _any_ empty
strings and not just trailing ones. For example,

```diff
--- /Users/dhruv/playground/ruff/src/UP032.py
+++ /Users/dhruv/playground/ruff/src/UP032.py
@@ -1,7 +1,5 @@
 (
-    "{a}"
-    ""
-    "{b}"
-    ""
-).format(a=1, b=1)
+    f"{1}"
+    f"{1}"
+)
```

## Test Plan

Run `cargo insta test` and update the snapshots.
  • Loading branch information
dhruvmanila authored May 27, 2024
1 parent 5dcde88 commit 9200dfc
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 58 deletions.
62 changes: 29 additions & 33 deletions crates/ruff_linter/src/rules/pyupgrade/rules/f_strings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,10 @@ fn formatted_expr<'a>(expr: &Expr, context: FormatContext, locator: &Locator<'a>

#[derive(Debug, Clone)]
enum FStringConversion {
/// The format string only contains literal parts.
Literal,
/// The format string only contains literal parts and is empty.
EmptyLiteral,
/// The format string only contains literal parts and is non-empty.
NonEmptyLiteral,
/// The format call uses arguments with side effects which are repeated within the
/// format string. For example: `"{x} {x}".format(x=foo())`.
SideEffects,
Expand Down Expand Up @@ -263,7 +265,7 @@ impl FStringConversion {

// If the format string is empty, it doesn't need to be converted.
if contents.is_empty() {
return Ok(Self::Literal);
return Ok(Self::EmptyLiteral);
}

// Parse the format string.
Expand All @@ -275,7 +277,7 @@ impl FStringConversion {
.iter()
.all(|part| matches!(part, FormatPart::Literal(..)))
{
return Ok(Self::Literal);
return Ok(Self::NonEmptyLiteral);
}

let mut converted = String::with_capacity(contents.len());
Expand Down Expand Up @@ -406,7 +408,7 @@ pub(crate) fn f_strings(checker: &mut Checker, call: &ast::ExprCall, summary: &F
return;
};

let mut patches: Vec<(TextRange, String)> = vec![];
let mut patches: Vec<(TextRange, FStringConversion)> = vec![];
let mut lex = lexer::lex_starts_at(
checker.locator().slice(call.func.range()),
Mode::Expression,
Expand All @@ -431,18 +433,14 @@ pub(crate) fn f_strings(checker: &mut Checker, call: &ast::ExprCall, summary: &F
}
Some((Tok::String { .. }, range)) => {
match FStringConversion::try_convert(range, &mut summary, checker.locator()) {
Ok(FStringConversion::Convert(fstring)) => patches.push((range, fstring)),
// Convert escaped curly brackets e.g. `{{` to `{` in literal string parts
Ok(FStringConversion::Literal) => patches.push((
range,
curly_unescape(checker.locator().slice(range)).to_string(),
)),
// If the format string contains side effects that would need to be repeated,
// we can't convert it to an f-string.
Ok(FStringConversion::SideEffects) => return,
// If any of the segments fail to convert, then we can't convert the entire
// expression.
Err(_) => return,
// Otherwise, push the conversion to be processed later.
Ok(conversion) => patches.push((range, conversion)),
}
}
Some(_) => continue,
Expand All @@ -455,30 +453,28 @@ pub(crate) fn f_strings(checker: &mut Checker, call: &ast::ExprCall, summary: &F

let mut contents = String::with_capacity(checker.locator().slice(call).len());
let mut prev_end = call.start();
for (range, fstring) in patches {
contents.push_str(
checker
.locator()
.slice(TextRange::new(prev_end, range.start())),
);
contents.push_str(&fstring);
prev_end = range.end();
}

// If the remainder is non-empty, add it to the contents.
let rest = checker.locator().slice(TextRange::new(prev_end, end));
if !lexer::lex_starts_at(rest, Mode::Expression, prev_end)
.flatten()
.all(|(token, _)| match token {
Tok::Comment(_) | Tok::Newline | Tok::NonLogicalNewline | Tok::Indent | Tok::Dedent => {
true
for (range, conversion) in patches {
let fstring = match conversion {
FStringConversion::Convert(fstring) => Some(fstring),
FStringConversion::EmptyLiteral => None,
FStringConversion::NonEmptyLiteral => {
// Convert escaped curly brackets e.g. `{{` to `{` in literal string parts
Some(curly_unescape(checker.locator().slice(range)).to_string())
}
Tok::String { value, .. } => value.is_empty(),
_ => false,
})
{
contents.push_str(rest);
// We handled this in the previous loop.
FStringConversion::SideEffects => unreachable!(),
};
if let Some(fstring) = fstring {
contents.push_str(
checker
.locator()
.slice(TextRange::new(prev_end, range.start())),
);
contents.push_str(&fstring);
}
prev_end = range.end();
}
contents.push_str(checker.locator().slice(TextRange::new(prev_end, end)));

// If necessary, add a space between any leading keyword (`return`, `yield`, `assert`, etc.)
// and the string. For example, `return"foo"` is valid, but `returnf"foo"` is not.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -767,16 +767,16 @@ UP032_0.py:86:1: UP032 [*] Use f-string instead of `format` call
85 85 |
86 86 | (
87 |- "{a}"
87 |+ f"{1}"
88 88 | ""
88 |- ""
89 |- "{b}"
89 |+ f"{1}"
90 90 | ""
90 |- ""
91 |-).format(a=1, b=1)
91 |+)
92 92 |
93 93 | (
94 94 | (
87 |+ f"{1}"
88 |+ f"{1}"
89 |+)
92 90 |
93 91 | (
94 92 | (

UP032_0.py:94:5: UP032 [*] Use f-string instead of `format` call
|
Expand Down Expand Up @@ -1089,11 +1089,10 @@ UP032_0.py:212:18: UP032 [*] Use f-string instead of `format` call
211 211 | # When fixing, trim the trailing empty string.
212 |-raise ValueError("Conflicting configuration dicts: {!r} {!r}"
213 |- "".format(new_dict, d))
212 |+raise ValueError(f"Conflicting configuration dicts: {new_dict!r} {d!r}"
213 |+ "")
214 214 |
215 215 | # When fixing, trim the trailing empty string.
216 216 | raise ValueError("Conflicting configuration dicts: {!r} {!r}"
212 |+raise ValueError(f"Conflicting configuration dicts: {new_dict!r} {d!r}")
214 213 |
215 214 | # When fixing, trim the trailing empty string.
216 215 | raise ValueError("Conflicting configuration dicts: {!r} {!r}"

UP032_0.py:216:18: UP032 [*] Use f-string instead of `format` call
|
Expand All @@ -1113,10 +1112,11 @@ UP032_0.py:216:18: UP032 [*] Use f-string instead of `format` call
215 215 | # When fixing, trim the trailing empty string.
216 |-raise ValueError("Conflicting configuration dicts: {!r} {!r}"
217 |- .format(new_dict, d))
216 |+raise ValueError(f"Conflicting configuration dicts: {new_dict!r} {d!r}")
218 217 |
219 218 | raise ValueError(
220 219 | "Conflicting configuration dicts: {!r} {!r}"
216 |+raise ValueError(f"Conflicting configuration dicts: {new_dict!r} {d!r}"
217 |+ )
218 218 |
219 219 | raise ValueError(
220 220 | "Conflicting configuration dicts: {!r} {!r}"

UP032_0.py:220:5: UP032 [*] Use f-string instead of `format` call
|
Expand All @@ -1136,10 +1136,9 @@ UP032_0.py:220:5: UP032 [*] Use f-string instead of `format` call
220 |- "Conflicting configuration dicts: {!r} {!r}"
221 |- "".format(new_dict, d)
220 |+ f"Conflicting configuration dicts: {new_dict!r} {d!r}"
221 |+ ""
222 222 | )
223 223 |
224 224 | raise ValueError(
222 221 | )
223 222 |
224 223 | raise ValueError(

UP032_0.py:225:5: UP032 [*] Use f-string instead of `format` call
|
Expand All @@ -1160,10 +1159,9 @@ UP032_0.py:225:5: UP032 [*] Use f-string instead of `format` call
225 |- "Conflicting configuration dicts: {!r} {!r}"
226 |- "".format(new_dict, d)
225 |+ f"Conflicting configuration dicts: {new_dict!r} {d!r}"
226 |+ ""
227 227 |
228 228 | )
229 229 |
227 226 |
228 227 | )
229 228 |

UP032_0.py:231:1: UP032 [*] Use f-string instead of `format` call
|
Expand Down

0 comments on commit 9200dfc

Please sign in to comment.