Skip to content

Commit

Permalink
Preserve quotes in generated f-strings (#15794)
Browse files Browse the repository at this point in the history
## 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>
  • Loading branch information
ntBre and AlexWaygood authored Jan 29, 2025
1 parent d151ca8 commit 23c9884
Show file tree
Hide file tree
Showing 11 changed files with 197 additions and 116 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -194,3 +194,17 @@ def f():
z = not foo()
else:
z = other


# These two cases double as tests for f-string quote preservation. The first
# f-string should preserve its double quotes, and the second should preserve
# single quotes
if cond:
var = "str"
else:
var = f"{first}-{second}"

if cond:
var = "str"
else:
var = f'{first}-{second}'
12 changes: 7 additions & 5 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,11 +294,7 @@ impl<'a> Checker<'a> {

/// Create a [`Generator`] to generate source code based on the current AST state.
pub(crate) fn generator(&self) -> Generator {
Generator::new(
self.stylist.indentation(),
self.preferred_quote(),
self.stylist.line_ending(),
)
Generator::new(self.stylist.indentation(), self.stylist.line_ending())
}

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

/// Return the default f-string flags a generated `FString` node should use, given where we are
/// in the AST.
pub(crate) fn default_fstring_flags(&self) -> ast::FStringFlags {
ast::FStringFlags::empty().with_quote_style(self.preferred_quote())
}

/// Returns the appropriate quoting for f-string by reversing the one used outside of
/// the f-string.
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,3 +301,55 @@ SIM108.py:193:1: SIM108 [*] Use ternary operator `z = not foo() if foo() else ot
195 |-else:
196 |- z = other
193 |+z = not foo() if foo() else other
197 194 |
198 195 |
199 196 | # These two cases double as tests for f-string quote preservation. The first

SIM108.py:202:1: SIM108 [*] Use ternary operator `var = "str" if cond else f"{first}-{second}"` instead of `if`-`else`-block
|
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}"
| |_____________________________^ SIM108
206 |
207 | if cond:
|
= help: Replace `if`-`else`-block with `var = "str" if cond else f"{first}-{second}"`

Unsafe fix
199 199 | # These two cases double as tests for f-string quote preservation. The first
200 200 | # f-string should preserve its double quotes, and the second should preserve
201 201 | # single quotes
202 |-if cond:
203 |- var = "str"
204 |-else:
205 |- var = f"{first}-{second}"
202 |+var = "str" if cond else f"{first}-{second}"
206 203 |
207 204 | if cond:
208 205 | var = "str"

SIM108.py:207:1: SIM108 [*] Use ternary operator `var = "str" if cond else f'{first}-{second}'` instead of `if`-`else`-block
|
205 | var = f"{first}-{second}"
206 |
207 | / if cond:
208 | | var = "str"
209 | | else:
210 | | var = f'{first}-{second}'
| |_____________________________^ SIM108
|
= help: Replace `if`-`else`-block with `var = "str" if cond else f'{first}-{second}'`

Unsafe fix
204 204 | else:
205 205 | var = f"{first}-{second}"
206 206 |
207 |-if cond:
208 |- var = "str"
209 |-else:
210 |- var = f'{first}-{second}'
207 |+var = "str" if cond else f'{first}-{second}'
Original file line number Diff line number Diff line change
Expand Up @@ -301,3 +301,55 @@ SIM108.py:193:1: SIM108 [*] Use ternary operator `z = not foo() if foo() else ot
195 |-else:
196 |- z = other
193 |+z = not foo() if foo() else other
197 194 |
198 195 |
199 196 | # These two cases double as tests for f-string quote preservation. The first

SIM108.py:202:1: SIM108 [*] Use ternary operator `var = "str" if cond else f"{first}-{second}"` instead of `if`-`else`-block
|
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}"
| |_____________________________^ SIM108
206 |
207 | if cond:
|
= help: Replace `if`-`else`-block with `var = "str" if cond else f"{first}-{second}"`

Unsafe fix
199 199 | # These two cases double as tests for f-string quote preservation. The first
200 200 | # f-string should preserve its double quotes, and the second should preserve
201 201 | # single quotes
202 |-if cond:
203 |- var = "str"
204 |-else:
205 |- var = f"{first}-{second}"
202 |+var = "str" if cond else f"{first}-{second}"
206 203 |
207 204 | if cond:
208 205 | var = "str"

SIM108.py:207:1: SIM108 [*] Use ternary operator `var = "str" if cond else f'{first}-{second}'` instead of `if`-`else`-block
|
205 | var = f"{first}-{second}"
206 |
207 | / if cond:
208 | | var = "str"
209 | | else:
210 | | var = f'{first}-{second}'
| |_____________________________^ SIM108
|
= help: Replace `if`-`else`-block with `var = "str" if cond else f'{first}-{second}'`

Unsafe fix
204 204 | else:
205 205 | var = f"{first}-{second}"
206 206 |
207 |-if cond:
208 |- var = "str"
209 |-else:
210 |- var = f'{first}-{second}'
207 |+var = "str" if cond else f'{first}-{second}'
6 changes: 1 addition & 5 deletions crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,11 +363,7 @@ impl<'a> QuoteAnnotator<'a> {
let generator = Generator::from(self.stylist);
// we first generate the annotation with the inverse quote, so we can
// generate the string literal with the preferred quote
let subgenerator = Generator::new(
self.stylist.indentation(),
self.stylist.quote().opposite(),
self.stylist.line_ending(),
);
let subgenerator = Generator::new(self.stylist.indentation(), self.stylist.line_ending());
let annotation = subgenerator.expr(&expr_without_forward_references);
generator.expr(&Expr::from(ast::StringLiteral {
range: TextRange::default(),
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/flynt/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fn to_f_string_expression_element(inner: &Expr) -> ast::FStringElement {
/// Convert a string to a [`ast::FStringElement::Literal`].
pub(super) fn to_f_string_literal_element(s: &str) -> ast::FStringElement {
ast::FStringElement::Literal(ast::FStringLiteralElement {
value: s.to_string().into_boxed_str(),
value: Box::from(s),
range: TextRange::default(),
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ fn is_static_length(elts: &[Expr]) -> bool {
elts.iter().all(|e| !e.is_starred_expr())
}

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

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,7 @@ pub(crate) fn assert_with_print_message(checker: &mut Checker, stmt: &ast::StmtA
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
checker.generator().stmt(&Stmt::Assert(ast::StmtAssert {
test: stmt.test.clone(),
msg: print_arguments::to_expr(&call.arguments, checker.default_string_flags())
.map(Box::new),
msg: print_arguments::to_expr(&call.arguments, checker).map(Box::new),
range: TextRange::default(),
})),
// We have to replace the entire statement,
Expand Down Expand Up @@ -96,6 +95,8 @@ mod print_arguments {
};
use ruff_text_size::TextRange;

use crate::checkers::ast::Checker;

/// Converts an expression to a list of `FStringElement`s.
///
/// Three cases are handled:
Expand Down Expand Up @@ -222,6 +223,7 @@ mod print_arguments {
fn args_to_fstring_expr(
mut args: impl ExactSizeIterator<Item = Vec<FStringElement>>,
sep: impl ExactSizeIterator<Item = FStringElement>,
flags: FStringFlags,
) -> Option<Expr> {
// If there are no arguments, short-circuit and return `None`
let first_arg = args.next()?;
Expand All @@ -236,7 +238,7 @@ mod print_arguments {
Some(Expr::FString(ExprFString {
value: FStringValue::single(FString {
elements: FStringElements::from(fstring_elements),
flags: FStringFlags::default(),
flags,
range: TextRange::default(),
}),
range: TextRange::default(),
Expand All @@ -256,7 +258,7 @@ mod print_arguments {
/// - [`Some`]<[`Expr::StringLiteral`]> if all arguments including `sep` are string literals.
/// - [`Some`]<[`Expr::FString`]> if any of the arguments are not string literals.
/// - [`None`] if the `print` contains no positional arguments at all.
pub(super) fn to_expr(arguments: &Arguments, flags: StringLiteralFlags) -> Option<Expr> {
pub(super) fn to_expr(arguments: &Arguments, checker: &Checker) -> Option<Expr> {
// Convert the `sep` argument into `FStringElement`s
let sep = arguments
.find_keyword("sep")
Expand Down Expand Up @@ -286,7 +288,13 @@ mod print_arguments {

// Attempt to convert the `sep` and `args` arguments to a string literal,
// falling back to an f-string if the arguments are not all string literals.
args_to_string_literal_expr(args.iter(), sep.iter(), flags)
.or_else(|| args_to_fstring_expr(args.into_iter(), sep.into_iter()))
args_to_string_literal_expr(args.iter(), sep.iter(), checker.default_string_flags())
.or_else(|| {
args_to_fstring_expr(
args.into_iter(),
sep.into_iter(),
checker.default_fstring_flags(),
)
})
}
}
26 changes: 24 additions & 2 deletions crates/ruff_python_ast/src/nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1063,10 +1063,32 @@ bitflags! {

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

impl FStringFlags {
/// Construct a new [`FStringFlags`] with **no flags set**.
///
/// See [`FStringFlags::with_quote_style`], [`FStringFlags::with_triple_quotes`], and
/// [`FStringFlags::with_prefix`] for ways of setting the quote style (single or double),
/// enabling triple quotes, and adding prefixes (such as `r`), respectively.
///
/// See the documentation for [`FStringFlags`] for additional caveats on this constructor, and
/// situations in which alternative ways to construct this struct should be used, especially
/// when writing lint rules.
pub fn empty() -> Self {
Self(FStringFlagsInner::empty())
}

#[must_use]
pub fn with_quote_style(mut self, quote_style: Quote) -> Self {
self.0
Expand Down Expand Up @@ -2229,7 +2251,7 @@ impl From<AnyStringFlags> for FStringFlags {
value.prefix()
)
};
let new = FStringFlags::default()
let new = FStringFlags::empty()
.with_quote_style(value.quote_style())
.with_prefix(fstring_prefix);
if value.is_triple_quoted() {
Expand Down
Loading

0 comments on commit 23c9884

Please sign in to comment.