Skip to content

Commit 4f7fb56

Browse files
MichaReiserMichaReiser
authored
Range formatting: Fix invalid syntax after parenthesizing expression (#9751)
1 parent 50bfbcf commit 4f7fb56

24 files changed

Lines changed: 351 additions & 212 deletions

File tree

‎crates/ruff_formatter/src/builders.rs‎

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -308,11 +308,8 @@ impl std::fmt::Debug for Token {
308308
/// assert_eq!(printed.as_code(), r#""Hello 'Ruff'""#);
309309
/// assert_eq!(printed.sourcemap(), [
310310
/// SourceMarker { source: TextSize::new(0), dest: TextSize::new(0) },
311-
/// SourceMarker { source: TextSize::new(0), dest: TextSize::new(7) },
312311
/// SourceMarker { source: TextSize::new(8), dest: TextSize::new(7) },
313-
/// SourceMarker { source: TextSize::new(8), dest: TextSize::new(13) },
314312
/// SourceMarker { source: TextSize::new(14), dest: TextSize::new(13) },
315-
/// SourceMarker { source: TextSize::new(14), dest: TextSize::new(14) },
316313
/// SourceMarker { source: TextSize::new(20), dest: TextSize::new(14) },
317314
/// ]);
318315
///
@@ -340,29 +337,25 @@ impl<Context> Format<Context> for SourcePosition {
340337
}
341338
}
342339

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

348-
Text { text, position }
346+
Text { text }
349347
}
350348

351349
#[derive(Eq, PartialEq)]
352350
pub struct Text<'a> {
353351
text: &'a str,
354-
position: Option<TextSize>,
355352
}
356353

357354
impl<Context> Format<Context> for Text<'_>
358355
where
359356
Context: FormatContext,
360357
{
361358
fn fmt(&self, f: &mut Formatter<Context>) -> FormatResult<()> {
362-
if let Some(position) = self.position {
363-
source_position(position).fmt(f)?;
364-
}
365-
366359
f.write_element(FormatElement::Text {
367360
text: self.text.to_string().into_boxed_str(),
368361
text_width: TextWidth::from_text(self.text, f.options().indent_width()),
@@ -2292,7 +2285,7 @@ impl<Context, T> std::fmt::Debug for FormatWith<Context, T> {
22922285
/// let mut join = f.join_with(&separator);
22932286
///
22942287
/// for item in &self.items {
2295-
/// join.entry(&format_with(|f| write!(f, [text(item, None)])));
2288+
/// join.entry(&format_with(|f| write!(f, [text(item)])));
22962289
/// }
22972290
/// join.finish()
22982291
/// })),
@@ -2377,7 +2370,7 @@ where
23772370
/// let mut count = 0;
23782371
///
23792372
/// let value = format_once(|f| {
2380-
/// write!(f, [text(&std::format!("Formatted {count}."), None)])
2373+
/// write!(f, [text(&std::format!("Formatted {count}."))])
23812374
/// });
23822375
///
23832376
/// format!(SimpleFormatContext::default(), [value]).expect("Formatting once works fine");

‎crates/ruff_formatter/src/format_element/document.rs‎

Lines changed: 17 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -346,10 +346,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
346346
}
347347

348348
FormatElement::SourcePosition(position) => {
349-
write!(
350-
f,
351-
[text(&std::format!("source_position({position:?})"), None)]
352-
)?;
349+
write!(f, [text(&std::format!("source_position({position:?})"))])?;
353350
}
354351

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

362359
if *mode != BestFittingMode::FirstLine {
363-
write!(f, [text(&std::format!("mode: {mode:?}, "), None)])?;
360+
write!(f, [text(&std::format!("mode: {mode:?}, "))])?;
364361
}
365362

366363
write!(f, [token("[")])?;
@@ -392,17 +389,14 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
392389
write!(
393390
f,
394391
[
395-
text(&std::format!("<interned {index}>"), None),
392+
text(&std::format!("<interned {index}>")),
396393
space(),
397394
&&**interned,
398395
]
399396
)?;
400397
}
401398
Some(reference) => {
402-
write!(
403-
f,
404-
[text(&std::format!("<ref interned *{reference}>"), None)]
405-
)?;
399+
write!(f, [text(&std::format!("<ref interned *{reference}>"))])?;
406400
}
407401
}
408402
}
@@ -421,7 +415,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
421415
f,
422416
[
423417
token("<END_TAG_WITHOUT_START<"),
424-
text(&std::format!("{:?}", tag.kind()), None),
418+
text(&std::format!("{:?}", tag.kind())),
425419
token(">>"),
426420
]
427421
)?;
@@ -436,9 +430,9 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
436430
token(")"),
437431
soft_line_break_or_space(),
438432
token("ERROR<START_END_TAG_MISMATCH<start: "),
439-
text(&std::format!("{start_kind:?}"), None),
433+
text(&std::format!("{start_kind:?}")),
440434
token(", end: "),
441-
text(&std::format!("{:?}", tag.kind()), None),
435+
text(&std::format!("{:?}", tag.kind())),
442436
token(">>")
443437
]
444438
)?;
@@ -470,7 +464,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
470464
f,
471465
[
472466
token("align("),
473-
text(&count.to_string(), None),
467+
text(&count.to_string()),
474468
token(","),
475469
space(),
476470
]
@@ -482,7 +476,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
482476
f,
483477
[
484478
token("line_suffix("),
485-
text(&std::format!("{reserved_width:?}"), None),
479+
text(&std::format!("{reserved_width:?}")),
486480
token(","),
487481
space(),
488482
]
@@ -499,11 +493,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
499493
if let Some(group_id) = group.id() {
500494
write!(
501495
f,
502-
[
503-
text(&std::format!("\"{group_id:?}\""), None),
504-
token(","),
505-
space(),
506-
]
496+
[text(&std::format!("\"{group_id:?}\"")), token(","), space(),]
507497
)?;
508498
}
509499

@@ -524,11 +514,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
524514
if let Some(group_id) = id {
525515
write!(
526516
f,
527-
[
528-
text(&std::format!("\"{group_id:?}\""), None),
529-
token(","),
530-
space(),
531-
]
517+
[text(&std::format!("\"{group_id:?}\"")), token(","), space(),]
532518
)?;
533519
}
534520
}
@@ -561,7 +547,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
561547
f,
562548
[
563549
token("indent_if_group_breaks("),
564-
text(&std::format!("\"{id:?}\""), None),
550+
text(&std::format!("\"{id:?}\"")),
565551
token(","),
566552
space(),
567553
]
@@ -581,11 +567,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
581567
if let Some(group_id) = condition.group_id {
582568
write!(
583569
f,
584-
[
585-
text(&std::format!("\"{group_id:?}\""), None),
586-
token(","),
587-
space(),
588-
]
570+
[text(&std::format!("\"{group_id:?}\"")), token(","), space()]
589571
)?;
590572
}
591573
}
@@ -595,7 +577,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
595577
f,
596578
[
597579
token("label("),
598-
text(&std::format!("\"{label_id:?}\""), None),
580+
text(&std::format!("\"{label_id:?}\"")),
599581
token(","),
600582
space(),
601583
]
@@ -664,7 +646,7 @@ impl Format<IrFormatContext<'_>> for &[FormatElement] {
664646
ContentArrayEnd,
665647
token(")"),
666648
soft_line_break_or_space(),
667-
text(&std::format!("<START_WITHOUT_END<{top:?}>>"), None),
649+
text(&std::format!("<START_WITHOUT_END<{top:?}>>")),
668650
]
669651
)?;
670652
}
@@ -807,7 +789,7 @@ impl Format<IrFormatContext<'_>> for Condition {
807789
f,
808790
[
809791
token("if_group_fits_on_line("),
810-
text(&std::format!("\"{id:?}\""), None),
792+
text(&std::format!("\"{id:?}\"")),
811793
token(")")
812794
]
813795
),
@@ -816,7 +798,7 @@ impl Format<IrFormatContext<'_>> for Condition {
816798
f,
817799
[
818800
token("if_group_breaks("),
819-
text(&std::format!("\"{id:?}\""), None),
801+
text(&std::format!("\"{id:?}\"")),
820802
token(")")
821803
]
822804
),

‎crates/ruff_formatter/src/format_extensions.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ pub trait MemoizeFormat<Context> {
3232
/// let value = self.value.get();
3333
/// self.value.set(value + 1);
3434
///
35-
/// write!(f, [text(&std::format!("Formatted {value} times."), None)])
35+
/// write!(f, [text(&std::format!("Formatted {value} times."))])
3636
/// }
3737
/// }
3838
///
@@ -110,7 +110,7 @@ where
110110
/// write!(f, [
111111
/// token("Count:"),
112112
/// space(),
113-
/// text(&std::format!("{current}"), None),
113+
/// text(&std::format!("{current}")),
114114
/// hard_line_break()
115115
/// ])?;
116116
///

‎crates/ruff_formatter/src/lib.rs‎

Lines changed: 52 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ use std::marker::PhantomData;
4141
use std::num::{NonZeroU16, NonZeroU8, TryFromIntError};
4242

4343
use crate::format_element::document::Document;
44-
use crate::printer::{Printer, PrinterOptions, SourceMapGeneration};
44+
use crate::printer::{Printer, PrinterOptions};
4545
pub use arguments::{Argument, Arguments};
4646
pub use buffer::{
4747
Buffer, BufferExtensions, BufferSnapshot, Inspect, RemoveSoftLinesBuffer, VecBuffer,
@@ -269,7 +269,6 @@ impl FormatOptions for SimpleFormatOptions {
269269
line_width: self.line_width,
270270
indent_style: self.indent_style,
271271
indent_width: self.indent_width,
272-
source_map_generation: SourceMapGeneration::Enabled,
273272
..PrinterOptions::default()
274273
}
275274
}
@@ -433,28 +432,40 @@ impl Printed {
433432
std::mem::take(&mut self.verbatim_ranges)
434433
}
435434

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

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

474-
let start = start_marker.map(|marker| marker.dest).unwrap_or_default();
475-
let end = end_marker.map_or_else(|| self.code.text_len(), |marker| marker.dest);
476-
let code_range = TextRange::new(start, end);
485+
let (source_start, formatted_start) = start_marker
486+
.map(|marker| (marker.source, marker.dest))
487+
.unwrap_or_default();
488+
489+
let (source_end, formatted_end) = end_marker
490+
.map_or((source.text_len(), self.code.text_len()), |marker| {
491+
(marker.source, marker.dest)
492+
});
493+
494+
let source_range = TextRange::new(source_start, source_end);
495+
let formatted_range = TextRange::new(formatted_start, formatted_end);
496+
497+
// Extend both ranges to include the indentation
498+
let source_range = extend_range_to_include_indent(source_range, source);
499+
let formatted_range = extend_range_to_include_indent(formatted_range, &self.code);
477500

478501
PrintedRange {
479-
code: self.code[code_range].to_string(),
502+
code: self.code[formatted_range].to_string(),
480503
source_range,
481504
}
482505
}
483506
}
484507

508+
/// Extends `range` backwards (by reducing `range.start`) to include any directly preceding whitespace (`\t` or ` `).
509+
///
510+
/// # Panics
511+
/// If `range.start` is out of `source`'s bounds.
512+
fn extend_range_to_include_indent(range: TextRange, source: &str) -> TextRange {
513+
let whitespace_len: TextSize = source[..usize::from(range.start())]
514+
.chars()
515+
.rev()
516+
.take_while(|c| matches!(c, ' ' | '\t'))
517+
.map(TextLen::text_len)
518+
.sum();
519+
520+
TextRange::new(range.start() - whitespace_len, range.end())
521+
}
522+
485523
#[derive(Debug, Clone, Eq, PartialEq)]
486524
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
487525
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
@@ -537,7 +575,7 @@ pub type FormatResult<F> = Result<F, FormatError>;
537575
/// impl Format<SimpleFormatContext> for Paragraph {
538576
/// fn fmt(&self, f: &mut Formatter<SimpleFormatContext>) -> FormatResult<()> {
539577
/// write!(f, [
540-
/// text(&self.0, None),
578+
/// text(&self.0),
541579
/// hard_line_break(),
542580
/// ])
543581
/// }

0 commit comments

Comments
 (0)