From e09159abfabb7d2eb94235ae812c16d8f5b24246 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 6 Sep 2023 08:59:43 +0200 Subject: [PATCH] Code review feedback --- crates/ruff_formatter/src/format_element.rs | 52 +++++++++++++------ .../src/format_element/document.rs | 2 +- crates/ruff_formatter/src/printer/mod.rs | 26 ++++++---- .../src/comments/format.rs | 10 ++-- 4 files changed, 60 insertions(+), 30 deletions(-) diff --git a/crates/ruff_formatter/src/format_element.rs b/crates/ruff_formatter/src/format_element.rs index bb26887bbf128..f5deac86c30d9 100644 --- a/crates/ruff_formatter/src/format_element.rs +++ b/crates/ruff_formatter/src/format_element.rs @@ -399,19 +399,39 @@ pub trait FormatElements { fn end_tag(&self, kind: TagKind) -> Option<&Tag>; } +/// New-type wrapper for a single-line text unicode width. +/// Mainly to prevent access to the inner value. +/// +/// ## Representation +/// +/// Represents the width by adding 1 to the actual width so that the width can be represented by a [`NonZeroU32`], +/// allowing [`TextWidth`] or [`Option`] fit in 4 bytes rather than 8. +/// +/// This means that 2^32 can not be precisely represented and instead has the same value as 2^32-1. +/// This imprecision shouldn't matter in practice because either text are longer than any configured line width +/// and thus, the text should break. +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub struct Width(NonZeroU32); + +impl Width { + pub(crate) const fn new(width: u32) -> Self { + Width(NonZeroU32::MIN.saturating_add(width)) + } + + pub const fn value(self) -> u32 { + self.0.get() - 1 + } +} + +/// The pre-computed unicode width of a text if it is a single-line text or a marker +/// that it is a multiline text if it contains a line feed. #[derive(Copy, Clone, Debug, Eq, PartialEq)] pub enum TextWidth { - Width(NonZeroU32), + Width(Width), Multiline, } impl TextWidth { - pub(crate) const fn new_width(width: u32) -> Self { - // SAFETY: 1 + x is guaranteed to be non zero - #[allow(unsafe_code)] - TextWidth::Width(unsafe { NonZeroU32::new_unchecked(width.saturating_add(1)) }) - } - pub fn from_text(text: &str, tab_width: TabWidth) -> TextWidth { let mut width = 0u32; @@ -425,12 +445,12 @@ impl TextWidth { width += char_width; } - Self::new_width(width) + Self::Width(Width::new(width)) } - pub const fn width(self) -> Option { + pub const fn width(self) -> Option { match self { - TextWidth::Width(width) => Some(width.get() - 1), + TextWidth::Width(width) => Some(width), TextWidth::Multiline => None, } } @@ -467,19 +487,21 @@ mod sizes { // be recomputed at a later point in time? // You reduced the size of a format element? Excellent work! + use super::{BestFittingVariants, Interned, TextWidth}; use static_assertions::assert_eq_size; assert_eq_size!(ruff_text_size::TextRange, [u8; 8]); - assert_eq_size!(crate::prelude::tag::VerbatimKind, [u8; 8]); - assert_eq_size!(crate::prelude::Interned, [u8; 16]); - assert_eq_size!(crate::format_element::BestFittingVariants, [u8; 16]); + assert_eq_size!(TextWidth, [u8; 4]); + assert_eq_size!(super::tag::VerbatimKind, [u8; 8]); + assert_eq_size!(Interned, [u8; 16]); + assert_eq_size!(BestFittingVariants, [u8; 16]); #[cfg(not(debug_assertions))] assert_eq_size!(crate::SourceCodeSlice, [u8; 8]); #[cfg(not(debug_assertions))] - assert_eq_size!(crate::format_element::Tag, [u8; 16]); + assert_eq_size!(super::Tag, [u8; 16]); #[cfg(not(debug_assertions))] - assert_eq_size!(crate::FormatElement, [u8; 24]); + assert_eq_size!(super::FormatElement, [u8; 24]); } diff --git a/crates/ruff_formatter/src/format_element/document.rs b/crates/ruff_formatter/src/format_element/document.rs index d085492786cf3..0ae0a333ea6fe 100644 --- a/crates/ruff_formatter/src/format_element/document.rs +++ b/crates/ruff_formatter/src/format_element/document.rs @@ -263,7 +263,7 @@ impl Format> for &[FormatElement] { let (text, text_width) = match element { #[allow(clippy::cast_possible_truncation)] FormatElement::Token { text } => { - (*text, TextWidth::new_width(text.len() as u32)) + (*text, TextWidth::Width(Width::new(text.len() as u32))) } FormatElement::Text { text, text_width } => { (text.as_ref(), *text_width) diff --git a/crates/ruff_formatter/src/printer/mod.rs b/crates/ruff_formatter/src/printer/mod.rs index 58ad92a266fca..3c3c7e970a35a 100644 --- a/crates/ruff_formatter/src/printer/mod.rs +++ b/crates/ruff_formatter/src/printer/mod.rs @@ -99,7 +99,7 @@ impl<'a> Printer<'a> { FormatElement::Text { text, text_width } => self.print_text( Text::Text { text, - width: *text_width, + text_width: *text_width, }, None, ), @@ -108,7 +108,7 @@ impl<'a> Printer<'a> { self.print_text( Text::Text { text, - width: *text_width, + text_width: *text_width, }, Some(slice.range()), ); @@ -407,10 +407,13 @@ impl<'a> Printer<'a> { self.state.buffer.push_str(token); self.state.line_width += token.len() as u32; } - Text::Text { text, width } => { + Text::Text { + text, + text_width: width, + } => { if let Some(width) = width.width() { self.state.buffer.push_str(text); - self.state.line_width += width; + self.state.line_width += width.value(); } else { for char in text.chars() { self.print_char(char); @@ -1107,7 +1110,7 @@ impl<'a, 'print> FitsMeasurer<'a, 'print> { return Ok(self.fits_text( Text::Text { text, - width: *text_width, + text_width: *text_width, }, args, )) @@ -1117,7 +1120,7 @@ impl<'a, 'print> FitsMeasurer<'a, 'print> { return Ok(self.fits_text( Text::Text { text, - width: *text_width, + text_width: *text_width, }, args, )); @@ -1338,9 +1341,9 @@ impl<'a, 'print> FitsMeasurer<'a, 'print> { Text::Token(token) => { self.state.line_width += token.len() as u32; } - Text::Text { text, width } => { - if let Some(width) = width.width() { - self.state.line_width += width; + Text::Text { text, text_width } => { + if let Some(width) = text_width.width() { + self.state.line_width += width.value(); } else { for c in text.chars() { let char_width = match c { @@ -1486,7 +1489,10 @@ enum Text<'a> { /// ASCII only text that contains no line breaks or tab characters. Token(&'a str), /// Arbitrary text. May contain `\n` line breaks, tab characters, or unicode characters. - Text { text: &'a str, width: TextWidth }, + Text { + text: &'a str, + text_width: TextWidth, + }, } #[cfg(test)] diff --git a/crates/ruff_python_formatter/src/comments/format.rs b/crates/ruff_python_formatter/src/comments/format.rs index 66117fb2b35b3..794f21a54a075 100644 --- a/crates/ruff_python_formatter/src/comments/format.rs +++ b/crates/ruff_python_formatter/src/comments/format.rs @@ -375,10 +375,12 @@ impl Format> for FormatTrailingEndOfLineComment<'_> { 0 } else { // Start with 2 because of the two leading spaces. - let width = TextWidth::from_text(&normalized_comment, f.options().tab_width()) - .width() - .expect("Expected comment not to contain any newlines") - + 2; + let width = 2u32.saturating_add( + TextWidth::from_text(&normalized_comment, f.options().tab_width()) + .width() + .expect("Expected comment not to contain any newlines") + .value(), + ); width };