Skip to content

Commit

Permalink
Range formatting: Fix invalid syntax after parenthesizing expression (#…
Browse files Browse the repository at this point in the history 
…9751)
  • Loading branch information
@MichaReiser
MichaReiser committed Feb 2, 2024
1 parent 50bfbcf commit 4f7fb56
Show file tree
Hide file tree
Showing 24 changed files with 351 additions and 212 deletions.
19 changes: 6 additions & 13 deletions crates/ruff_formatter/src/builders.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,11 +308,8 @@ impl std::fmt::Debug for Token {
/// assert_eq!(printed.as_code(), r#""Hello 'Ruff'""#);
/// assert_eq!(printed.sourcemap(), [
/// SourceMarker { source: TextSize::new(0), dest: TextSize::new(0) },
/// SourceMarker { source: TextSize::new(0), dest: TextSize::new(7) },
/// SourceMarker { source: TextSize::new(8), dest: TextSize::new(7) },
/// SourceMarker { source: TextSize::new(8), dest: TextSize::new(13) },
/// SourceMarker { source: TextSize::new(14), dest: TextSize::new(13) },
/// SourceMarker { source: TextSize::new(14), dest: TextSize::new(14) },
/// SourceMarker { source: TextSize::new(20), dest: TextSize::new(14) },
/// ]);
///
Expand Down Expand Up @@ -340,29 +337,25 @@ impl<Context> Format<Context> for SourcePosition {
}
}

/// Creates a text from a dynamic string with its optional start-position in the source document.
/// Creates a text from a dynamic string.
///
/// This is done by allocating a new string internally.
pub fn text(text: &str, position: Option<TextSize>) -> Text {
pub fn text(text: &str) -> Text {
debug_assert_no_newlines(text);

Text { text, position }
Text { text }
}

#[derive(Eq, PartialEq)]
pub struct Text<'a> {
text: &'a str,
position: Option<TextSize>,
}

impl<Context> Format<Context> for Text<'_>
where
Context: FormatContext,
{
fn fmt(&self, f: &mut Formatter<Context>) -> FormatResult<()> {
if let Some(position) = self.position {
source_position(position).fmt(f)?;
}

f.write_element(FormatElement::Text {
text: self.text.to_string().into_boxed_str(),
text_width: TextWidth::from_text(self.text, f.options().indent_width()),
Expand Down Expand Up @@ -2292,7 +2285,7 @@ impl<Context, T> std::fmt::Debug for FormatWith<Context, T> {
/// let mut join = f.join_with(&separator);
///
/// for item in &self.items {
/// join.entry(&format_with(|f| write!(f, [text(item, None)])));
/// join.entry(&format_with(|f| write!(f, [text(item)])));
/// }
/// join.finish()
/// })),
Expand Down Expand Up @@ -2377,7 +2370,7 @@ where
/// let mut count = 0;
///
/// let value = format_once(|f| {
/// write!(f, [text(&std::format!("Formatted {count}."), None)])
/// write!(f, [text(&std::format!("Formatted {count}."))])
/// });
///
/// format!(SimpleFormatContext::default(), [value]).expect("Formatting once works fine");
Expand Down
52 changes: 17 additions & 35 deletions crates/ruff_formatter/src/format_element/document.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -346,10 +346,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
}

FormatElement::SourcePosition(position) => {
write!(
f,
[text(&std::format!("source_position({position:?})"), None)]
)?;
write!(f, [text(&std::format!("source_position({position:?})"))])?;
}

FormatElement::LineSuffixBoundary => {
Expand All @@ -360,7 +357,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
write!(f, [token("best_fitting(")])?;

if *mode != BestFittingMode::FirstLine {
write!(f, [text(&std::format!("mode: {mode:?}, "), None)])?;
write!(f, [text(&std::format!("mode: {mode:?}, "))])?;
}

write!(f, [token("[")])?;
Expand Down Expand Up @@ -392,17 +389,14 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
write!(
f,
[
text(&std::format!("<interned {index}>"), None),
text(&std::format!("<interned {index}>")),
space(),
&&**interned,
]
)?;
}
Some(reference) => {
write!(
f,
[text(&std::format!("<ref interned *{reference}>"), None)]
)?;
write!(f, [text(&std::format!("<ref interned *{reference}>"))])?;
}
}
}
Expand All @@ -421,7 +415,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
f,
[
token("<END_TAG_WITHOUT_START<"),
text(&std::format!("{:?}", tag.kind()), None),
text(&std::format!("{:?}", tag.kind())),
token(">>"),
]
)?;
Expand All @@ -436,9 +430,9 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
token(")"),
soft_line_break_or_space(),
token("ERROR<START_END_TAG_MISMATCH<start: "),
text(&std::format!("{start_kind:?}"), None),
text(&std::format!("{start_kind:?}")),
token(", end: "),
text(&std::format!("{:?}", tag.kind()), None),
text(&std::format!("{:?}", tag.kind())),
token(">>")
]
)?;
Expand Down Expand Up @@ -470,7 +464,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
f,
[
token("align("),
text(&count.to_string(), None),
text(&count.to_string()),
token(","),
space(),
]
Expand All @@ -482,7 +476,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
f,
[
token("line_suffix("),
text(&std::format!("{reserved_width:?}"), None),
text(&std::format!("{reserved_width:?}")),
token(","),
space(),
]
Expand All @@ -499,11 +493,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
if let Some(group_id) = group.id() {
write!(
f,
[
text(&std::format!("\"{group_id:?}\""), None),
token(","),
space(),
]
[text(&std::format!("\"{group_id:?}\"")), token(","), space(),]
)?;
}

Expand All @@ -524,11 +514,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
if let Some(group_id) = id {
write!(
f,
[
text(&std::format!("\"{group_id:?}\""), None),
token(","),
space(),
]
[text(&std::format!("\"{group_id:?}\"")), token(","), space(),]
)?;
}
}
Expand Down Expand Up @@ -561,7 +547,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
f,
[
token("indent_if_group_breaks("),
text(&std::format!("\"{id:?}\""), None),
text(&std::format!("\"{id:?}\"")),
token(","),
space(),
]
Expand All @@ -581,11 +567,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
if let Some(group_id) = condition.group_id {
write!(
f,
[
text(&std::format!("\"{group_id:?}\""), None),
token(","),
space(),
]
[text(&std::format!("\"{group_id:?}\"")), token(","), space()]
)?;
}
}
Expand All @@ -595,7 +577,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
f,
[
token("label("),
text(&std::format!("\"{label_id:?}\""), None),
text(&std::format!("\"{label_id:?}\"")),
token(","),
space(),
]
Expand Down Expand Up @@ -664,7 +646,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
ContentArrayEnd,
token(")"),
soft_line_break_or_space(),
text(&std::format!("<START_WITHOUT_END<{top:?}>>"), None),
text(&std::format!("<START_WITHOUT_END<{top:?}>>")),
]
)?;
}
Expand Down Expand Up @@ -807,7 +789,7 @@ impl Format<IrFormatContext<'_>> for Condition {
f,
[
token("if_group_fits_on_line("),
text(&std::format!("\"{id:?}\""), None),
text(&std::format!("\"{id:?}\"")),
token(")")
]
),
Expand All @@ -816,7 +798,7 @@ impl Format<IrFormatContext<'_>> for Condition {
f,
[
token("if_group_breaks("),
text(&std::format!("\"{id:?}\""), None),
text(&std::format!("\"{id:?}\"")),
token(")")
]
),
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_formatter/src/format_extensions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub trait MemoizeFormat<Context> {
/// let value = self.value.get();
/// self.value.set(value + 1);
///
/// write!(f, [text(&std::format!("Formatted {value} times."), None)])
/// write!(f, [text(&std::format!("Formatted {value} times."))])
/// }
/// }
///
Expand Down Expand Up @@ -110,7 +110,7 @@ where
/// write!(f, [
/// token("Count:"),
/// space(),
/// text(&std::format!("{current}"), None),
/// text(&std::format!("{current}")),
/// hard_line_break()
/// ])?;
///
Expand Down
66 changes: 52 additions & 14 deletions crates/ruff_formatter/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use std::marker::PhantomData;
use std::num::{NonZeroU16, NonZeroU8, TryFromIntError};

use crate::format_element::document::Document;
use crate::printer::{Printer, PrinterOptions, SourceMapGeneration};
use crate::printer::{Printer, PrinterOptions};
pub use arguments::{Argument, Arguments};
pub use buffer::{
Buffer, BufferExtensions, BufferSnapshot, Inspect, RemoveSoftLinesBuffer, VecBuffer,
Expand Down Expand Up @@ -269,7 +269,6 @@ impl FormatOptions for SimpleFormatOptions {
line_width: self.line_width,
indent_style: self.indent_style,
indent_width: self.indent_width,
source_map_generation: SourceMapGeneration::Enabled,
..PrinterOptions::default()
}
}
Expand Down Expand Up @@ -433,28 +432,40 @@ impl Printed {
std::mem::take(&mut self.verbatim_ranges)
}

/// Slices the formatted code to the sub-slices that covers the passed `source_range`.
/// Slices the formatted code to the sub-slices that covers the passed `source_range` in `source`.
///
/// The implementation uses the source map generated during formatting to find the closest range
/// in the formatted document that covers `source_range` or more. The returned slice
/// matches the `source_range` exactly (except indent, see below) if the formatter emits [`FormatElement::SourcePosition`] for
/// the range's offsets.
///
/// ## Indentation
/// The indentation before `source_range.start` is replaced with the indentation returned by the formatter
/// to fix up incorrectly intended code.
///
/// Returns the entire document if the source map is empty.
///
/// # Panics
/// If `source_range` points to offsets that are not in the bounds of `source`.
#[must_use]
pub fn slice_range(self, source_range: TextRange) -> PrintedRange {
pub fn slice_range(self, source_range: TextRange, source: &str) -> PrintedRange {
let mut start_marker: Option<SourceMarker> = None;
let mut end_marker: Option<SourceMarker> = None;

// Note: The printer can generate multiple source map entries for the same source position.
// For example if you have:
// * token("a + b")
// * `source_position(276)`
// * `token(")")`
// * `source_position(276)`
// * `token("def")`
// * `token("foo")`
// * `source_position(284)`
// The printer uses the source position 276 for both the tokens `def` and `foo` because that's the only position it knows of.
// * `hard_line_break`
// The printer uses the source position 276 for both the tokens `)` and the `\n` because
// there were multiple `source_position` entries in the IR with the same offset.
// This can happen if multiple nodes start or end at the same position. A common example
// for this are expressions and expression statement that always end at the same offset.
//
// Warning: Source markers are often emitted sorted by their source position but it's not guaranteed.
// Warning: Source markers are often emitted sorted by their source position but it's not guaranteed
// and depends on the emitted `IR`.
// They are only guaranteed to be sorted in increasing order by their destination position.
for marker in self.sourcemap {
// Take the closest start marker, but skip over start_markers that have the same start.
Expand All @@ -471,17 +482,44 @@ impl Printed {
}
}

let start = start_marker.map(|marker| marker.dest).unwrap_or_default();
let end = end_marker.map_or_else(|| self.code.text_len(), |marker| marker.dest);
let code_range = TextRange::new(start, end);
let (source_start, formatted_start) = start_marker
.map(|marker| (marker.source, marker.dest))
.unwrap_or_default();

let (source_end, formatted_end) = end_marker
.map_or((source.text_len(), self.code.text_len()), |marker| {
(marker.source, marker.dest)
});

let source_range = TextRange::new(source_start, source_end);
let formatted_range = TextRange::new(formatted_start, formatted_end);

// Extend both ranges to include the indentation
let source_range = extend_range_to_include_indent(source_range, source);
let formatted_range = extend_range_to_include_indent(formatted_range, &self.code);

PrintedRange {
code: self.code[code_range].to_string(),
code: self.code[formatted_range].to_string(),
source_range,
}
}
}

/// Extends `range` backwards (by reducing `range.start`) to include any directly preceding whitespace (`\t` or ` `).
///
/// # Panics
/// If `range.start` is out of `source`'s bounds.
fn extend_range_to_include_indent(range: TextRange, source: &str) -> TextRange {
let whitespace_len: TextSize = source[..usize::from(range.start())]
.chars()
.rev()
.take_while(|c| matches!(c, ' ' | '\t'))
.map(TextLen::text_len)
.sum();

TextRange::new(range.start() - whitespace_len, range.end())
}

#[derive(Debug, Clone, Eq, PartialEq)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
Expand Down Expand Up @@ -537,7 +575,7 @@ pub type FormatResult<F> = Result<F, FormatError>;
/// impl Format<SimpleFormatContext> for Paragraph {
/// fn fmt(&self, f: &mut Formatter<SimpleFormatContext>) -> FormatResult<()> {
/// write!(f, [
/// text(&self.0, None),
/// text(&self.0),
/// hard_line_break(),
/// ])
/// }
Expand Down

0 comments on commit 4f7fb56

Please sign in to comment.