Skip to content

Commit

Permalink
Fix unnecessary space around power op in overlong f-string expressions
Browse files Browse the repository at this point in the history
  • Loading branch information
MichaReiser committed Nov 20, 2024
1 parent 942d6ee commit 2bd63c3
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 5 deletions.
108 changes: 104 additions & 4 deletions crates/ruff_formatter/src/buffer.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
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};
Expand Down Expand Up @@ -294,10 +294,11 @@ where
}
}

/// A Buffer that removes any soft line breaks.
/// A Buffer that removes any soft line breaks or [`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` elements.
///
/// # Examples
///
Expand Down Expand Up @@ -350,13 +351,16 @@ 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<Interned, Interned>,

state: RemoveSoftLineBreaksState,
}

impl<'a, Context> RemoveSoftLinesBuffer<'a, Context> {
/// Creates a new buffer that removes the soft line breaks before writing them into `buffer`.
pub fn new(inner: &'a mut dyn Buffer<Context = Context>) -> Self {
Self {
inner,
state: RemoveSoftLineBreaksState::default(),
interned_cache: FxHashMap::default(),
}
}
Expand All @@ -375,15 +379,27 @@ 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()
.enumerate()
.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::Tag(Tag::StartConditionalContent(condition))
if condition.mode.is_expanded() =>
{
state.increment_conditional_content();
let mut cleaned = Vec::new();
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);
Expand All @@ -405,12 +421,23 @@ fn clean_interned(
// 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))
}
FormatElement::Tag(Tag::StartConditionalContent(condition))
if condition.mode.is_expanded() =>
{
state.increment_conditional_content();
continue;
}

element => element.clone(),
};
cleaned.push(element);
Expand All @@ -431,12 +458,23 @@ impl<Context> 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))
}
FormatElement::Tag(Tag::StartConditionalContent(condition))
if condition.mode.is_expanded() =>
{
self.state.increment_conditional_content();
return;
}

element => element,
};

Expand Down Expand Up @@ -464,6 +502,68 @@ impl<Context> Buffer for RemoveSoftLinesBuffer<'_, Context> {
}
}

#[derive(Copy, Clone, Debug, Default)]
enum RemoveSoftLineBreaksState {
#[default]
Default,
InIfGroupBreaks {
conditional_content_level: usize,
},
}

impl RemoveSoftLineBreaksState {
fn should_drop(&mut self, element: &FormatElement) -> bool {
match self {
Self::Default => false,
Self::InIfGroupBreaks { .. } => {
match element {
FormatElement::Tag(Tag::StartConditionalContent(_)) => {
self.increment_conditional_content();
}
FormatElement::Tag(Tag::EndConditionalContent) => {
self.decrement_conditional_content();
}
_ => {}
}

true
}
}
}

fn decrement_conditional_content(&mut self) -> bool {
match self {
Self::InIfGroupBreaks {
conditional_content_level,
} => {
if *conditional_content_level == 0 {
*self = RemoveSoftLineBreaksState::Default;
true
} else {
*conditional_content_level -= 1;
false
}
}
Self::Default => false,
}
}

fn increment_conditional_content(&mut self) {
match self {
Self::InIfGroupBreaks {
conditional_content_level,
} => {
*conditional_content_level += 1;
}
Self::Default => {
*self = RemoveSoftLineBreaksState::InIfGroupBreaks {
conditional_content_level: 0,
};
}
}
}
}

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]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
```
Expand Down Expand Up @@ -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"
```
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
```

0 comments on commit 2bd63c3

Please sign in to comment.