diff --git a/crates/ruff_formatter/src/buffer.rs b/crates/ruff_formatter/src/buffer.rs index 94b09df039..4137fbe58e 100644 --- a/crates/ruff_formatter/src/buffer.rs +++ b/crates/ruff_formatter/src/buffer.rs @@ -1,10 +1,11 @@ use super::{write, Arguments, FormatElement}; use crate::format_element::Interned; -use crate::prelude::LineMode; +use crate::prelude::{LineMode, Tag}; use crate::{FormatResult, FormatState}; use rustc_hash::FxHashMap; use std::any::{Any, TypeId}; use std::fmt::Debug; +use std::num::NonZeroUsize; use std::ops::{Deref, DerefMut}; /// A trait for writing or formatting into [`FormatElement`]-accepting buffers or streams. @@ -294,10 +295,11 @@ where } } -/// A Buffer that removes any soft line breaks. +/// A Buffer that removes any soft line breaks or [`if_group_breaks`](crate::builders::if_group_breaks) elements. /// /// - 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) +/// - Removes [`if_group_breaks`](crate::builders::if_group_breaks) elements. /// /// # Examples /// @@ -350,6 +352,8 @@ pub struct RemoveSoftLinesBuffer<'a, Context> { /// It's fine to not snapshot the cache. The worst that can happen is that it holds on interned elements /// that are now unused. But there's little harm in that and the cache is cleaned when dropping the buffer. interned_cache: FxHashMap, + + state: RemoveSoftLineBreaksState, } impl<'a, Context> RemoveSoftLinesBuffer<'a, Context> { @@ -357,6 +361,7 @@ impl<'a, Context> RemoveSoftLinesBuffer<'a, Context> { pub fn new(inner: &'a mut dyn Buffer) -> Self { Self { inner, + state: RemoveSoftLineBreaksState::default(), interned_cache: FxHashMap::default(), } } @@ -375,6 +380,8 @@ fn clean_interned( if let Some(cleaned) = interned_cache.get(interned) { cleaned.clone() } else { + let mut state = RemoveSoftLineBreaksState::default(); + // Find the first soft line break element or interned element that must be changed let result = interned .iter() @@ -382,8 +389,9 @@ fn clean_interned( .find_map(|(index, element)| match element { FormatElement::Line(LineMode::Soft | LineMode::SoftOrSpace) => { let mut cleaned = Vec::new(); - cleaned.extend_from_slice(&interned[..index]); - Some((cleaned, &interned[index..])) + let (before, after) = interned.split_at(index); + cleaned.extend_from_slice(before); + Some((cleaned, &after[1..])) } FormatElement::Interned(inner) => { let cleaned_inner = clean_interned(inner, interned_cache); @@ -398,19 +406,33 @@ fn clean_interned( } } - _ => None, + element => { + if state.should_drop(element) { + let mut cleaned = Vec::new(); + let (before, after) = interned.split_at(index); + cleaned.extend_from_slice(before); + Some((cleaned, &after[1..])) + } else { + None + } + } }); let result = match result { // Copy the whole interned buffer so that becomes possible to change the necessary elements. Some((mut cleaned, rest)) => { for element in rest { + if state.should_drop(element) { + continue; + } + let element = match element { FormatElement::Line(LineMode::Soft) => continue, FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space, FormatElement::Interned(interned) => { FormatElement::Interned(clean_interned(interned, interned_cache)) } + element => element.clone(), }; cleaned.push(element); @@ -431,12 +453,17 @@ impl Buffer for RemoveSoftLinesBuffer<'_, Context> { type Context = Context; fn write_element(&mut self, element: FormatElement) { + if self.state.should_drop(&element) { + return; + } + let element = match element { FormatElement::Line(LineMode::Soft) => return, FormatElement::Line(LineMode::SoftOrSpace) => FormatElement::Space, FormatElement::Interned(interned) => { FormatElement::Interned(self.clean_interned(&interned)) } + element => element, }; @@ -456,14 +483,77 @@ impl Buffer for RemoveSoftLinesBuffer<'_, Context> { } fn snapshot(&self) -> BufferSnapshot { - self.inner.snapshot() + BufferSnapshot::Any(Box::new(RemoveSoftLinebreaksSnapshot { + inner: self.inner.snapshot(), + state: self.state, + })) } fn restore_snapshot(&mut self, snapshot: BufferSnapshot) { - self.inner.restore_snapshot(snapshot); + let RemoveSoftLinebreaksSnapshot { inner, state } = snapshot.unwrap_any(); + self.inner.restore_snapshot(inner); + self.state = state; } } +#[derive(Copy, Clone, Debug, Default)] +enum RemoveSoftLineBreaksState { + #[default] + Default, + InIfGroupBreaks { + conditional_content_level: NonZeroUsize, + }, +} + +impl RemoveSoftLineBreaksState { + fn should_drop(&mut self, element: &FormatElement) -> bool { + match self { + Self::Default => { + // Entered the start of an `if_group_breaks` + if let FormatElement::Tag(Tag::StartConditionalContent(condition)) = element { + if condition.mode.is_expanded() { + *self = Self::InIfGroupBreaks { + conditional_content_level: NonZeroUsize::new(1).unwrap(), + }; + return true; + } + } + + false + } + Self::InIfGroupBreaks { + conditional_content_level, + } => { + match element { + // A nested `if_group_breaks` or `if_group_fits` + FormatElement::Tag(Tag::StartConditionalContent(_)) => { + *conditional_content_level = conditional_content_level.saturating_add(1); + } + // The end of an `if_group_breaks` or `if_group_fits`. + FormatElement::Tag(Tag::EndConditionalContent) => { + if let Some(level) = NonZeroUsize::new(conditional_content_level.get() - 1) + { + *conditional_content_level = level; + } else { + // Found the end tag of the initial `if_group_breaks`. Skip this element but retain + // the elements coming after + *self = RemoveSoftLineBreaksState::Default; + } + } + _ => {} + } + + true + } + } + } +} + +struct RemoveSoftLinebreaksSnapshot { + inner: BufferSnapshot, + state: RemoveSoftLineBreaksState, +} + pub trait BufferExtensions: Buffer + Sized { /// Returns a new buffer that calls the passed inspector for every element that gets written to the output #[must_use] diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py index 3c895ee85a..e61467cfea 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py @@ -346,3 +346,6 @@ _ = ( f'This string uses double quotes in an expression {"it's a quote"}' f'This f-string does not use any quotes.' ) + +# Regression test for https://github.com/astral-sh/ruff/issues/14487 +f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" diff --git a/crates/ruff_python_formatter/src/builders.rs b/crates/ruff_python_formatter/src/builders.rs index 3d49904926..45b74ed8b5 100644 --- a/crates/ruff_python_formatter/src/builders.rs +++ b/crates/ruff_python_formatter/src/builders.rs @@ -1,7 +1,7 @@ use ruff_formatter::{write, Argument, Arguments}; use ruff_text_size::{Ranged, TextRange, TextSize}; -use crate::context::{FStringState, NodeLevel, WithNodeLevel}; +use crate::context::{NodeLevel, WithNodeLevel}; use crate::other::commas::has_magic_trailing_comma; use crate::prelude::*; @@ -206,16 +206,6 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { pub(crate) fn finish(&mut self) -> FormatResult<()> { self.result.and_then(|()| { - // If the formatter is inside an f-string expression element, and the layout - // is flat, then we don't need to add a trailing comma. - if let FStringState::InsideExpressionElement(context) = - self.fmt.context().f_string_state() - { - if !context.can_contain_line_breaks() { - return Ok(()); - } - } - if let Some(last_end) = self.entries.position() { let magic_trailing_comma = has_magic_trailing_comma( TextRange::new(last_end, self.sequence_end), diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring.py.snap index 407dcccf3f..e00cd01460 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring.py.snap @@ -353,6 +353,9 @@ _ = ( f'This string uses double quotes in an expression {"it's a quote"}' f'This f-string does not use any quotes.' ) + +# Regression test for https://github.com/astral-sh/ruff/issues/14487 +f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" ``` ## Outputs @@ -728,6 +731,9 @@ _ = ( f"This string uses double quotes in an expression {"it's a quote"}" f"This f-string does not use any quotes." ) + +# Regression test for https://github.com/astral-sh/ruff/issues/14487 +f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" ``` @@ -1091,6 +1097,9 @@ _ = ( f'This string uses double quotes in an expression {"it's a quote"}' f'This f-string does not use any quotes.' ) + +# Regression test for https://github.com/astral-sh/ruff/issues/14487 +f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" ``` @@ -1444,7 +1453,7 @@ _ = ( # comment 27 # comment 28 } woah {x}" -@@ -318,27 +330,27 @@ +@@ -318,29 +330,29 @@ if indent2: foo = f"""hello world hello { @@ -1490,4 +1499,6 @@ _ = ( + f"This string uses double quotes in an expression {"it's a quote"}" + f"This f-string does not use any quotes." ) + + # Regression test for https://github.com/astral-sh/ruff/issues/14487 ```