Skip to content

Commit 23c9884

Browse files
ntBreAlexWaygood
ntBre
and
AlexWaygood
authored
Preserve quotes in generated f-strings (#15794)
## Summary This is another follow-up to #15726 and #15778, extending the quote-preserving behavior to f-strings and deleting the now-unused `Generator::quote` field. ## Details I also made one unrelated change to `rules/flynt/helpers.rs` to remove a `to_string` call for making a `Box<str>` and tweaked some arguments to some of the `Generator::unparse_f_string` methods to make the code easier to follow, in my opinion. Happy to revert especially the latter of these if needed. Unfortunately this still does not fix the issue in #9660, which appears to be more of an escaping issue than a quote-preservation issue. After #15726, the result is now `a = f'# {"".join([])}' if 1 else ""` instead of `a = f"# {''.join([])}" if 1 else ""` (single quotes on the outside now), but we still don't have the desired behavior of double quotes everywhere on Python 3.12+. I added a test for this but split it off into another branch since it ended up being unaddressed here, but my `dbg!` statements showed the correct preferred quotes going into [`UnicodeEscape::with_preferred_quote`](https://github.com/astral-sh/ruff/blob/main/crates/ruff_python_literal/src/escape.rs#L54). ## Test Plan Existing rule and `Generator` tests. --------- Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
1 parent d151ca8 commit 23c9884

11 files changed

Lines changed: 197 additions & 116 deletions

File tree

‎crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM108.py‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,3 +194,17 @@ def f():
194194
z = not foo()
195195
else:
196196
z = other
197+
198+
199+
# These two cases double as tests for f-string quote preservation. The first
200+
# f-string should preserve its double quotes, and the second should preserve
201+
# single quotes
202+
if cond:
203+
var = "str"
204+
else:
205+
var = f"{first}-{second}"
206+
207+
if cond:
208+
var = "str"
209+
else:
210+
var = f'{first}-{second}'

‎crates/ruff_linter/src/checkers/ast/mod.rs‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -294,11 +294,7 @@ impl<'a> Checker<'a> {
294294

295295
/// Create a [`Generator`] to generate source code based on the current AST state.
296296
pub(crate) fn generator(&self) -> Generator {
297-
Generator::new(
298-
self.stylist.indentation(),
299-
self.preferred_quote(),
300-
self.stylist.line_ending(),
301-
)
297+
Generator::new(self.stylist.indentation(), self.stylist.line_ending())
302298
}
303299

304300
/// Return the preferred quote for a generated `StringLiteral` node, given where we are in the
@@ -319,6 +315,12 @@ impl<'a> Checker<'a> {
319315
ast::BytesLiteralFlags::empty().with_quote_style(self.preferred_quote())
320316
}
321317

318+
/// Return the default f-string flags a generated `FString` node should use, given where we are
319+
/// in the AST.
320+
pub(crate) fn default_fstring_flags(&self) -> ast::FStringFlags {
321+
ast::FStringFlags::empty().with_quote_style(self.preferred_quote())
322+
}
323+
322324
/// Returns the appropriate quoting for f-string by reversing the one used outside of
323325
/// the f-string.
324326
///

‎crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM108_SIM108.py.snap‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,3 +301,55 @@ SIM108.py:193:1: SIM108 [*] Use ternary operator `z = not foo() if foo() else ot
301301
195 |-else:
302302
196 |- z = other
303303
193 |+z = not foo() if foo() else other
304+
197 194 |
305+
198 195 |
306+
199 196 | # These two cases double as tests for f-string quote preservation. The first
307+
308+
SIM108.py:202:1: SIM108 [*] Use ternary operator `var = "str" if cond else f"{first}-{second}"` instead of `if`-`else`-block
309+
|
310+
200 | # f-string should preserve its double quotes, and the second should preserve
311+
201 | # single quotes
312+
202 | / if cond:
313+
203 | | var = "str"
314+
204 | | else:
315+
205 | | var = f"{first}-{second}"
316+
| |_____________________________^ SIM108
317+
206 |
318+
207 | if cond:
319+
|
320+
= help: Replace `if`-`else`-block with `var = "str" if cond else f"{first}-{second}"`
321+
322+
ℹ Unsafe fix
323+
199 199 | # These two cases double as tests for f-string quote preservation. The first
324+
200 200 | # f-string should preserve its double quotes, and the second should preserve
325+
201 201 | # single quotes
326+
202 |-if cond:
327+
203 |- var = "str"
328+
204 |-else:
329+
205 |- var = f"{first}-{second}"
330+
202 |+var = "str" if cond else f"{first}-{second}"
331+
206 203 |
332+
207 204 | if cond:
333+
208 205 | var = "str"
334+
335+
SIM108.py:207:1: SIM108 [*] Use ternary operator `var = "str" if cond else f'{first}-{second}'` instead of `if`-`else`-block
336+
|
337+
205 | var = f"{first}-{second}"
338+
206 |
339+
207 | / if cond:
340+
208 | | var = "str"
341+
209 | | else:
342+
210 | | var = f'{first}-{second}'
343+
| |_____________________________^ SIM108
344+
|
345+
= help: Replace `if`-`else`-block with `var = "str" if cond else f'{first}-{second}'`
346+
347+
ℹ Unsafe fix
348+
204 204 | else:
349+
205 205 | var = f"{first}-{second}"
350+
206 206 |
351+
207 |-if cond:
352+
208 |- var = "str"
353+
209 |-else:
354+
210 |- var = f'{first}-{second}'
355+
207 |+var = "str" if cond else f'{first}-{second}'

‎crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM108_SIM108.py.snap‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,3 +301,55 @@ SIM108.py:193:1: SIM108 [*] Use ternary operator `z = not foo() if foo() else ot
301301
195 |-else:
302302
196 |- z = other
303303
193 |+z = not foo() if foo() else other
304+
197 194 |
305+
198 195 |
306+
199 196 | # These two cases double as tests for f-string quote preservation. The first
307+
308+
SIM108.py:202:1: SIM108 [*] Use ternary operator `var = "str" if cond else f"{first}-{second}"` instead of `if`-`else`-block
309+
|
310+
200 | # f-string should preserve its double quotes, and the second should preserve
311+
201 | # single quotes
312+
202 | / if cond:
313+
203 | | var = "str"
314+
204 | | else:
315+
205 | | var = f"{first}-{second}"
316+
| |_____________________________^ SIM108
317+
206 |
318+
207 | if cond:
319+
|
320+
= help: Replace `if`-`else`-block with `var = "str" if cond else f"{first}-{second}"`
321+
322+
ℹ Unsafe fix
323+
199 199 | # These two cases double as tests for f-string quote preservation. The first
324+
200 200 | # f-string should preserve its double quotes, and the second should preserve
325+
201 201 | # single quotes
326+
202 |-if cond:
327+
203 |- var = "str"
328+
204 |-else:
329+
205 |- var = f"{first}-{second}"
330+
202 |+var = "str" if cond else f"{first}-{second}"
331+
206 203 |
332+
207 204 | if cond:
333+
208 205 | var = "str"
334+
335+
SIM108.py:207:1: SIM108 [*] Use ternary operator `var = "str" if cond else f'{first}-{second}'` instead of `if`-`else`-block
336+
|
337+
205 | var = f"{first}-{second}"
338+
206 |
339+
207 | / if cond:
340+
208 | | var = "str"
341+
209 | | else:
342+
210 | | var = f'{first}-{second}'
343+
| |_____________________________^ SIM108
344+
|
345+
= help: Replace `if`-`else`-block with `var = "str" if cond else f'{first}-{second}'`
346+
347+
ℹ Unsafe fix
348+
204 204 | else:
349+
205 205 | var = f"{first}-{second}"
350+
206 206 |
351+
207 |-if cond:
352+
208 |- var = "str"
353+
209 |-else:
354+
210 |- var = f'{first}-{second}'
355+
207 |+var = "str" if cond else f'{first}-{second}'

‎crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -363,11 +363,7 @@ impl<'a> QuoteAnnotator<'a> {
363363
let generator = Generator::from(self.stylist);
364364
// we first generate the annotation with the inverse quote, so we can
365365
// generate the string literal with the preferred quote
366-
let subgenerator = Generator::new(
367-
self.stylist.indentation(),
368-
self.stylist.quote().opposite(),
369-
self.stylist.line_ending(),
370-
);
366+
let subgenerator = Generator::new(self.stylist.indentation(), self.stylist.line_ending());
371367
let annotation = subgenerator.expr(&expr_without_forward_references);
372368
generator.expr(&Expr::from(ast::StringLiteral {
373369
range: TextRange::default(),

‎crates/ruff_linter/src/rules/flynt/helpers.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ fn to_f_string_expression_element(inner: &Expr) -> ast::FStringElement {
1515
/// Convert a string to a [`ast::FStringElement::Literal`].
1616
pub(super) fn to_f_string_literal_element(s: &str) -> ast::FStringElement {
1717
ast::FStringElement::Literal(ast::FStringLiteralElement {
18-
value: s.to_string().into_boxed_str(),
18+
value: Box::from(s),
1919
range: TextRange::default(),
2020
})
2121
}

‎crates/ruff_linter/src/rules/flynt/rules/static_join_to_fstring.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,8 @@ fn is_static_length(elts: &[Expr]) -> bool {
6060
elts.iter().all(|e| !e.is_starred_expr())
6161
}
6262

63-
fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option<Expr> {
63+
/// Build an f-string consisting of `joinees` joined by `joiner` with `flags`.
64+
fn build_fstring(joiner: &str, joinees: &[Expr], flags: FStringFlags) -> Option<Expr> {
6465
// If all elements are string constants, join them into a single string.
6566
if joinees.iter().all(Expr::is_string_literal_expr) {
6667
let mut flags = None;
@@ -104,7 +105,7 @@ fn build_fstring(joiner: &str, joinees: &[Expr]) -> Option<Expr> {
104105
let node = ast::FString {
105106
elements: f_string_elements.into(),
106107
range: TextRange::default(),
107-
flags: FStringFlags::default(),
108+
flags,
108109
};
109110
Some(node.into())
110111
}
@@ -137,7 +138,7 @@ pub(crate) fn static_join_to_fstring(checker: &mut Checker, expr: &Expr, joiner:
137138

138139
// Try to build the fstring (internally checks whether e.g. the elements are
139140
// convertible to f-string elements).
140-
let Some(new_expr) = build_fstring(joiner, joinees) else {
141+
let Some(new_expr) = build_fstring(joiner, joinees, checker.default_fstring_flags()) else {
141142
return;
142143
};
143144

‎crates/ruff_linter/src/rules/ruff/rules/assert_with_print_message.rs‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,7 @@ pub(crate) fn assert_with_print_message(checker: &mut Checker, stmt: &ast::StmtA
6666
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
6767
checker.generator().stmt(&Stmt::Assert(ast::StmtAssert {
6868
test: stmt.test.clone(),
69-
msg: print_arguments::to_expr(&call.arguments, checker.default_string_flags())
70-
.map(Box::new),
69+
msg: print_arguments::to_expr(&call.arguments, checker).map(Box::new),
7170
range: TextRange::default(),
7271
})),
7372
// We have to replace the entire statement,
@@ -96,6 +95,8 @@ mod print_arguments {
9695
};
9796
use ruff_text_size::TextRange;
9897

98+
use crate::checkers::ast::Checker;
99+
99100
/// Converts an expression to a list of `FStringElement`s.
100101
///
101102
/// Three cases are handled:
@@ -222,6 +223,7 @@ mod print_arguments {
222223
fn args_to_fstring_expr(
223224
mut args: impl ExactSizeIterator<Item = Vec<FStringElement>>,
224225
sep: impl ExactSizeIterator<Item = FStringElement>,
226+
flags: FStringFlags,
225227
) -> Option<Expr> {
226228
// If there are no arguments, short-circuit and return `None`
227229
let first_arg = args.next()?;
@@ -236,7 +238,7 @@ mod print_arguments {
236238
Some(Expr::FString(ExprFString {
237239
value: FStringValue::single(FString {
238240
elements: FStringElements::from(fstring_elements),
239-
flags: FStringFlags::default(),
241+
flags,
240242
range: TextRange::default(),
241243
}),
242244
range: TextRange::default(),
@@ -256,7 +258,7 @@ mod print_arguments {
256258
/// - [`Some`]<[`Expr::StringLiteral`]> if all arguments including `sep` are string literals.
257259
/// - [`Some`]<[`Expr::FString`]> if any of the arguments are not string literals.
258260
/// - [`None`] if the `print` contains no positional arguments at all.
259-
pub(super) fn to_expr(arguments: &Arguments, flags: StringLiteralFlags) -> Option<Expr> {
261+
pub(super) fn to_expr(arguments: &Arguments, checker: &Checker) -> Option<Expr> {
260262
// Convert the `sep` argument into `FStringElement`s
261263
let sep = arguments
262264
.find_keyword("sep")
@@ -286,7 +288,13 @@ mod print_arguments {
286288

287289
// Attempt to convert the `sep` and `args` arguments to a string literal,
288290
// falling back to an f-string if the arguments are not all string literals.
289-
args_to_string_literal_expr(args.iter(), sep.iter(), flags)
290-
.or_else(|| args_to_fstring_expr(args.into_iter(), sep.into_iter()))
291+
args_to_string_literal_expr(args.iter(), sep.iter(), checker.default_string_flags())
292+
.or_else(|| {
293+
args_to_fstring_expr(
294+
args.into_iter(),
295+
sep.into_iter(),
296+
checker.default_fstring_flags(),
297+
)
298+
})
291299
}
292300
}

‎crates/ruff_python_ast/src/nodes.rs‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1063,10 +1063,32 @@ bitflags! {
10631063

10641064
/// Flags that can be queried to obtain information
10651065
/// regarding the prefixes and quotes used for an f-string.
1066-
#[derive(Default, Copy, Clone, Eq, PartialEq, Hash)]
1066+
///
1067+
/// ## Notes on usage
1068+
///
1069+
/// If you're using a `Generator` from the `ruff_python_codegen` crate to generate a lint-rule fix
1070+
/// from an existing f-string literal, consider passing along the [`FString::flags`] field. If you
1071+
/// don't have an existing literal but have a `Checker` from the `ruff_linter` crate available,
1072+
/// consider using `Checker::default_fstring_flags` to create instances of this struct; this method
1073+
/// will properly handle nested f-strings. For usage that doesn't fit into one of these categories,
1074+
/// the public constructor [`FStringFlags::empty`] can be used.
1075+
#[derive(Copy, Clone, Eq, PartialEq, Hash)]
10671076
pub struct FStringFlags(FStringFlagsInner);
10681077

10691078
impl FStringFlags {
1079+
/// Construct a new [`FStringFlags`] with **no flags set**.
1080+
///
1081+
/// See [`FStringFlags::with_quote_style`], [`FStringFlags::with_triple_quotes`], and
1082+
/// [`FStringFlags::with_prefix`] for ways of setting the quote style (single or double),
1083+
/// enabling triple quotes, and adding prefixes (such as `r`), respectively.
1084+
///
1085+
/// See the documentation for [`FStringFlags`] for additional caveats on this constructor, and
1086+
/// situations in which alternative ways to construct this struct should be used, especially
1087+
/// when writing lint rules.
1088+
pub fn empty() -> Self {
1089+
Self(FStringFlagsInner::empty())
1090+
}
1091+
10701092
#[must_use]
10711093
pub fn with_quote_style(mut self, quote_style: Quote) -> Self {
10721094
self.0
@@ -2229,7 +2251,7 @@ impl From<AnyStringFlags> for FStringFlags {
22292251
value.prefix()
22302252
)
22312253
};
2232-
let new = FStringFlags::default()
2254+
let new = FStringFlags::empty()
22332255
.with_quote_style(value.quote_style())
22342256
.with_prefix(fstring_prefix);
22352257
if value.is_triple_quoted() {

0 commit comments

Comments
 (0)