Fix fstring formatting removing overlong implicit concatenated string in expression part (#14811)

## Summary

Fixes https://github.com/astral-sh/ruff/issues/14778


The formatter incorrectly removed the inner implicitly concatenated
string for following single-line f-string:

```py
f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 'a' if True else ""}"

# formatted
f"{ if True else ''}"
```

This happened because I changed the `RemoveSoftlinesBuffer` in
https://github.com/astral-sh/ruff/pull/14489 to remove any content
wrapped in `if_group_breaks`. After all, it emulates an *all flat*
layout. This works fine when `if_group_breaks` is only used to **add**
content if the gorup breaks. It doesn't work if the same content is
rendered differently depending on if the group fits using
`if_group_breaks` and `if_groups_fits` because the enclosing `group`
might still *break* if the entire content exceeds the line-length limit.

This PR fixes this by unwrapping any `if_group_fits` content by removing
the `if_group_fits` start and end tags.


## Test Plan

added test
This commit is contained in:
Micha Reiser 2024-12-06 13:01:04 +01:00 committed by GitHub
parent 39623f8d40
commit 1559c73fcd
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 47 additions and 15 deletions

View file

@ -299,7 +299,8 @@ where
/// ///
/// - Removes [`lines`](FormatElement::Line) with the mode [`Soft`](LineMode::Soft). /// - Removes [`lines`](FormatElement::Line) with the mode [`Soft`](LineMode::Soft).
/// - Replaces [`lines`](FormatElement::Line) with the mode [`Soft`](LineMode::SoftOrSpace) with a [`Space`](FormatElement::Space) /// - Replaces [`lines`](FormatElement::Line) with the mode [`Soft`](LineMode::SoftOrSpace) with a [`Space`](FormatElement::Space)
/// - Removes [`if_group_breaks`](crate::builders::if_group_breaks) elements. /// - Removes [`if_group_breaks`](crate::builders::if_group_breaks) and all its content.
/// - Unwraps the content of [`if_group_fits_on_line`](crate::builders::if_group_fits_on_line) elements (but retains it).
/// ///
/// # Examples /// # Examples
/// ///
@ -387,7 +388,7 @@ fn clean_interned(
.iter() .iter()
.enumerate() .enumerate()
.find_map(|(index, element)| match element { .find_map(|(index, element)| match element {
FormatElement::Line(LineMode::Soft | LineMode::SoftOrSpace) => { FormatElement::Line(LineMode::SoftOrSpace) => {
let mut cleaned = Vec::new(); let mut cleaned = Vec::new();
let (before, after) = interned.split_at(index); let (before, after) = interned.split_at(index);
cleaned.extend_from_slice(before); cleaned.extend_from_slice(before);
@ -427,7 +428,6 @@ fn clean_interned(
} }
let element = match element { let element = match element {
FormatElement::Line(LineMode::Soft) => continue,
FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space, FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space,
FormatElement::Interned(interned) => { FormatElement::Interned(interned) => {
FormatElement::Interned(clean_interned(interned, interned_cache)) FormatElement::Interned(clean_interned(interned, interned_cache))
@ -458,7 +458,6 @@ impl<Context> Buffer for RemoveSoftLinesBuffer<'_, Context> {
} }
let element = match element { let element = match element {
FormatElement::Line(LineMode::Soft) => return,
FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space, FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space,
FormatElement::Interned(interned) => { FormatElement::Interned(interned) => {
FormatElement::Interned(self.clean_interned(&interned)) FormatElement::Interned(self.clean_interned(&interned))
@ -508,28 +507,34 @@ enum RemoveSoftLineBreaksState {
impl RemoveSoftLineBreaksState { impl RemoveSoftLineBreaksState {
fn should_drop(&mut self, element: &FormatElement) -> bool { fn should_drop(&mut self, element: &FormatElement) -> bool {
match self { match self {
Self::Default => { Self::Default => match element {
// Entered the start of an `if_group_breaks` FormatElement::Line(LineMode::Soft) => true,
if let FormatElement::Tag(Tag::StartConditionalContent(condition)) = element {
// Entered the start of an `if_group_breaks` or `if_group_fits`
// For `if_group_breaks`: Remove the start and end tag and all content in between.
// For `if_group_fits_on_line`: Unwrap the content. This is important because the enclosing group
// might still *expand* if the content exceeds the line width limit, in which case the
// `if_group_fits_on_line` content would be removed.
FormatElement::Tag(Tag::StartConditionalContent(condition)) => {
if condition.mode.is_expanded() { if condition.mode.is_expanded() {
*self = Self::InIfGroupBreaks { *self = Self::InIfGroupBreaks {
conditional_content_level: NonZeroUsize::new(1).unwrap(), conditional_content_level: NonZeroUsize::new(1).unwrap(),
}; };
return true;
} }
true
} }
FormatElement::Tag(Tag::EndConditionalContent) => true,
false _ => false,
} },
Self::InIfGroupBreaks { Self::InIfGroupBreaks {
conditional_content_level, conditional_content_level,
} => { } => {
match element { match element {
// A nested `if_group_breaks` or `if_group_fits` // A nested `if_group_breaks` or `if_group_fits_on_line`
FormatElement::Tag(Tag::StartConditionalContent(_)) => { FormatElement::Tag(Tag::StartConditionalContent(_)) => {
*conditional_content_level = conditional_content_level.saturating_add(1); *conditional_content_level = conditional_content_level.saturating_add(1);
} }
// The end of an `if_group_breaks` or `if_group_fits`. // The end of an `if_group_breaks` or `if_group_fits_on_line`.
FormatElement::Tag(Tag::EndConditionalContent) => { FormatElement::Tag(Tag::EndConditionalContent) => {
if let Some(level) = NonZeroUsize::new(conditional_content_level.get() - 1) if let Some(level) = NonZeroUsize::new(conditional_content_level.get() - 1)
{ {

View file

@ -665,6 +665,10 @@ _ = (
# Regression test for https://github.com/astral-sh/ruff/issues/14487 # Regression test for https://github.com/astral-sh/ruff/issues/14487
f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc"
# Regression test for https://github.com/astral-sh/ruff/issues/14778
f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 'a' if True else ""}"
f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 'a' if True else ""}"
# Quotes reuse # Quotes reuse
f"{'a'}" f"{'a'}"

View file

@ -1,6 +1,7 @@
--- ---
source: crates/ruff_python_formatter/tests/fixtures.rs source: crates/ruff_python_formatter/tests/fixtures.rs
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py
snapshot_kind: text
--- ---
## Input ## Input
```python ```python
@ -671,6 +672,10 @@ _ = (
# Regression test for https://github.com/astral-sh/ruff/issues/14487 # Regression test for https://github.com/astral-sh/ruff/issues/14487
f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc"
# Regression test for https://github.com/astral-sh/ruff/issues/14778
f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 'a' if True else ""}"
f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 'a' if True else ""}"
# Quotes reuse # Quotes reuse
f"{'a'}" f"{'a'}"
@ -1441,6 +1446,10 @@ _ = (
# Regression test for https://github.com/astral-sh/ruff/issues/14487 # Regression test for https://github.com/astral-sh/ruff/issues/14487
f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc"
# Regression test for https://github.com/astral-sh/ruff/issues/14778
f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' if True else ''}"
f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' if True else ''}"
# Quotes reuse # Quotes reuse
f"{'a'}" f"{'a'}"
@ -2163,6 +2172,10 @@ _ = (
# Regression test for https://github.com/astral-sh/ruff/issues/14487 # Regression test for https://github.com/astral-sh/ruff/issues/14487
f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc"
# Regression test for https://github.com/astral-sh/ruff/issues/14778
f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 'a' if True else ""}"
f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 'a' if True else ""}"
# Quotes reuse # Quotes reuse
f"{'a'}" f"{'a'}"
@ -2993,7 +3006,7 @@ f'{1=: {'ab"cd"'}}' # It's okay if the quotes are in an expression part.
) )
# Indentation # Indentation
@@ -632,29 +680,29 @@ @@ -632,37 +680,37 @@
if indent2: if indent2:
foo = f"""hello world foo = f"""hello world
hello { hello {
@ -3041,7 +3054,17 @@ f'{1=: {'ab"cd"'}}' # It's okay if the quotes are in an expression part.
) )
# Regression test for https://github.com/astral-sh/ruff/issues/14487 # Regression test for https://github.com/astral-sh/ruff/issues/14487
@@ -678,18 +726,18 @@ f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc"
# Regression test for https://github.com/astral-sh/ruff/issues/14778
-f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 'a' if True else ""}"
-f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' 'a' if True else ""}"
+f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' if True else ''}"
+f"{'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' if True else ''}"
# Quotes reuse
f"{'a'}"
@@ -682,18 +730,18 @@
f'{1: hy "user"}' f'{1: hy "user"}'
f'{1:hy "user"}' f'{1:hy "user"}'
f'{1: abcd "{1}" }' f'{1: abcd "{1}" }'