diff --git a/Cargo.lock b/Cargo.lock index 768c871516..72a90b18ea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2886,6 +2886,7 @@ dependencies = [ "schemars", "serde", "serde_json", + "similar", "tempfile", "thiserror 2.0.12", "tracing", diff --git a/crates/ruff_db/Cargo.toml b/crates/ruff_db/Cargo.toml index f3df7fac70..40eb42ec8f 100644 --- a/crates/ruff_db/Cargo.toml +++ b/crates/ruff_db/Cargo.toml @@ -40,6 +40,7 @@ salsa = { workspace = true } schemars = { workspace = true, optional = true } serde = { workspace = true, optional = true } serde_json = { workspace = true, optional = true } +similar = { workspace = true } thiserror = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true, optional = true } diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index 53b4247adc..db6befc9aa 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -1294,6 +1294,10 @@ pub struct DisplayDiagnosticConfig { hide_severity: bool, /// Whether to show the availability of a fix in a diagnostic. show_fix_status: bool, + /// Whether to show the diff for an available fix after the main diagnostic. + /// + /// This currently only applies to `DiagnosticFormat::Full`. + show_fix_diff: bool, /// The lowest applicability that should be shown when reporting diagnostics. fix_applicability: Applicability, } @@ -1341,6 +1345,14 @@ impl DisplayDiagnosticConfig { } } + /// Whether to show a diff for an available fix after the main diagnostic. + pub fn show_fix_diff(self, yes: bool) -> DisplayDiagnosticConfig { + DisplayDiagnosticConfig { + show_fix_diff: yes, + ..self + } + } + /// Set the lowest fix applicability that should be shown. /// /// In other words, an applicability of `Safe` (the default) would suppress showing fixes or fix @@ -1364,6 +1376,7 @@ impl Default for DisplayDiagnosticConfig { preview: false, hide_severity: false, show_fix_status: false, + show_fix_diff: false, fix_applicability: Applicability::Safe, } } diff --git a/crates/ruff_db/src/diagnostic/render/full.rs b/crates/ruff_db/src/diagnostic/render/full.rs index 0eee73e543..951b812562 100644 --- a/crates/ruff_db/src/diagnostic/render/full.rs +++ b/crates/ruff_db/src/diagnostic/render/full.rs @@ -1,7 +1,17 @@ +use std::borrow::Cow; +use std::num::NonZeroUsize; + +use anstyle::Style; +use similar::{ChangeTag, TextDiff}; + use ruff_annotate_snippets::Renderer as AnnotateRenderer; +use ruff_diagnostics::{Applicability, Fix}; +use ruff_source_file::OneIndexed; +use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::diagnostic::render::{FileResolver, Resolved}; -use crate::diagnostic::{Diagnostic, DisplayDiagnosticConfig, stylesheet::DiagnosticStylesheet}; +use crate::diagnostic::stylesheet::{DiagnosticStylesheet, fmt_styled}; +use crate::diagnostic::{Diagnostic, DiagnosticSource, DisplayDiagnosticConfig}; pub(super) struct FullRenderer<'a> { resolver: &'a dyn FileResolver, @@ -48,12 +58,199 @@ impl<'a> FullRenderer<'a> { writeln!(f, "{}", renderer.render(diag.to_annotate()))?; } writeln!(f)?; + + if self.config.show_fix_diff { + if let Some(diff) = Diff::from_diagnostic(diag, &stylesheet, self.resolver) { + writeln!(f, "{diff}")?; + } + } } Ok(()) } } +/// Renders a diff that shows the code fixes. +/// +/// The implementation isn't fully fledged out and only used by tests. Before using in production, try +/// * Improve layout +/// * Replace tabs with spaces for a consistent experience across terminals +/// * Replace zero-width whitespaces +/// * Print a simpler diff if only a single line has changed +/// * Compute the diff from the `Edit` because diff calculation is expensive. +struct Diff<'a> { + fix: &'a Fix, + diagnostic_source: DiagnosticSource, + stylesheet: &'a DiagnosticStylesheet, +} + +impl<'a> Diff<'a> { + fn from_diagnostic( + diagnostic: &'a Diagnostic, + stylesheet: &'a DiagnosticStylesheet, + resolver: &'a dyn FileResolver, + ) -> Option> { + Some(Diff { + fix: diagnostic.fix()?, + diagnostic_source: diagnostic + .primary_span_ref()? + .file + .diagnostic_source(resolver), + stylesheet, + }) + } +} + +impl std::fmt::Display for Diff<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let source_code = self.diagnostic_source.as_source_code(); + let source_text = source_code.text(); + + // TODO(dhruvmanila): Add support for Notebook cells once it's user-facing + let mut output = String::with_capacity(source_text.len()); + let mut last_end = TextSize::default(); + + for edit in self.fix.edits() { + output.push_str(source_code.slice(TextRange::new(last_end, edit.start()))); + output.push_str(edit.content().unwrap_or_default()); + last_end = edit.end(); + } + + output.push_str(&source_text[usize::from(last_end)..]); + + let diff = TextDiff::from_lines(source_text, &output); + + let message = match self.fix.applicability() { + // TODO(zanieb): Adjust this messaging once it's user-facing + Applicability::Safe => "Safe fix", + Applicability::Unsafe => "Unsafe fix", + Applicability::DisplayOnly => "Display-only fix", + }; + + // TODO(brent) `stylesheet.separator` is cyan rather than blue, as we had before. I think + // we're getting rid of this soon anyway, so I didn't think it was worth adding another + // style to the stylesheet temporarily. The color doesn't appear at all in the snapshot + // tests, which is the only place these are currently used. + writeln!(f, "ℹ {}", fmt_styled(message, self.stylesheet.separator))?; + + let (largest_old, largest_new) = diff + .ops() + .last() + .map(|op| (op.old_range().start, op.new_range().start)) + .unwrap_or_default(); + + let digit_with = OneIndexed::from_zero_indexed(largest_new.max(largest_old)).digits(); + + for (idx, group) in diff.grouped_ops(3).iter().enumerate() { + if idx > 0 { + writeln!(f, "{:-^1$}", "-", 80)?; + } + for op in group { + for change in diff.iter_inline_changes(op) { + let sign = match change.tag() { + ChangeTag::Delete => "-", + ChangeTag::Insert => "+", + ChangeTag::Equal => " ", + }; + + let line_style = LineStyle::from(change.tag(), self.stylesheet); + + let old_index = change.old_index().map(OneIndexed::from_zero_indexed); + let new_index = change.new_index().map(OneIndexed::from_zero_indexed); + + write!( + f, + "{} {} |{}", + Line { + index: old_index, + width: digit_with + }, + Line { + index: new_index, + width: digit_with + }, + fmt_styled(line_style.apply_to(sign), self.stylesheet.emphasis), + )?; + + for (emphasized, value) in change.iter_strings_lossy() { + let value = show_nonprinting(&value); + if emphasized { + write!( + f, + "{}", + fmt_styled(line_style.apply_to(&value), self.stylesheet.underline) + )?; + } else { + write!(f, "{}", line_style.apply_to(&value))?; + } + } + if change.missing_newline() { + writeln!(f)?; + } + } + } + } + + Ok(()) + } +} + +struct LineStyle { + style: Style, +} + +impl LineStyle { + fn apply_to(&self, input: &str) -> impl std::fmt::Display { + fmt_styled(input, self.style) + } + + fn from(value: ChangeTag, stylesheet: &DiagnosticStylesheet) -> LineStyle { + match value { + ChangeTag::Equal => LineStyle { + style: stylesheet.none, + }, + ChangeTag::Delete => LineStyle { + style: stylesheet.deletion, + }, + ChangeTag::Insert => LineStyle { + style: stylesheet.insertion, + }, + } + } +} + +struct Line { + index: Option, + width: NonZeroUsize, +} + +impl std::fmt::Display for Line { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self.index { + None => { + for _ in 0..self.width.get() { + f.write_str(" ")?; + } + Ok(()) + } + Some(idx) => write!(f, "{: Cow<'_, str> { + if s.find(['\x07', '\x08', '\x1b', '\x7f']).is_some() { + Cow::Owned( + s.replace('\x07', "␇") + .replace('\x08', "␈") + .replace('\x1b', "␛") + .replace('\x7f', "␡"), + ) + } else { + Cow::Borrowed(s) + } +} + #[cfg(test)] mod tests { use ruff_diagnostics::Applicability; diff --git a/crates/ruff_db/src/diagnostic/stylesheet.rs b/crates/ruff_db/src/diagnostic/stylesheet.rs index 50be7ee41c..bba985ead3 100644 --- a/crates/ruff_db/src/diagnostic/stylesheet.rs +++ b/crates/ruff_db/src/diagnostic/stylesheet.rs @@ -40,9 +40,12 @@ pub struct DiagnosticStylesheet { pub(crate) help: Style, pub(crate) line_no: Style, pub(crate) emphasis: Style, + pub(crate) underline: Style, pub(crate) none: Style, pub(crate) separator: Style, pub(crate) secondary_code: Style, + pub(crate) insertion: Style, + pub(crate) deletion: Style, } impl Default for DiagnosticStylesheet { @@ -63,9 +66,12 @@ impl DiagnosticStylesheet { help: AnsiColor::BrightCyan.on_default().effects(Effects::BOLD), line_no: bright_blue.effects(Effects::BOLD), emphasis: Style::new().effects(Effects::BOLD), + underline: Style::new().effects(Effects::UNDERLINE), none: Style::new(), separator: AnsiColor::Cyan.on_default(), secondary_code: AnsiColor::Red.on_default().effects(Effects::BOLD), + insertion: AnsiColor::Green.on_default(), + deletion: AnsiColor::Red.on_default(), } } @@ -78,9 +84,12 @@ impl DiagnosticStylesheet { help: Style::new(), line_no: Style::new(), emphasis: Style::new(), + underline: Style::new(), none: Style::new(), separator: Style::new(), secondary_code: Style::new(), + insertion: Style::new(), + deletion: Style::new(), } } } diff --git a/crates/ruff_linter/src/message/diff.rs b/crates/ruff_linter/src/message/diff.rs deleted file mode 100644 index 437fc0a830..0000000000 --- a/crates/ruff_linter/src/message/diff.rs +++ /dev/null @@ -1,202 +0,0 @@ -use std::fmt::{Display, Formatter}; -use std::num::NonZeroUsize; - -use colored::{Color, ColoredString, Colorize, Styles}; -use similar::{ChangeTag, TextDiff}; - -use ruff_db::diagnostic::Diagnostic; -use ruff_source_file::{OneIndexed, SourceFile}; -use ruff_text_size::{Ranged, TextRange, TextSize}; - -use crate::text_helpers::ShowNonprinting; -use crate::{Applicability, Fix}; - -/// Renders a diff that shows the code fixes. -/// -/// The implementation isn't fully fledged out and only used by tests. Before using in production, try -/// * Improve layout -/// * Replace tabs with spaces for a consistent experience across terminals -/// * Replace zero-width whitespaces -/// * Print a simpler diff if only a single line has changed -/// * Compute the diff from the [`Edit`] because diff calculation is expensive. -pub(super) struct Diff<'a> { - fix: &'a Fix, - source_code: &'a SourceFile, -} - -impl<'a> Diff<'a> { - pub(crate) fn from_message(message: &'a Diagnostic) -> Option> { - message.fix().map(|fix| Diff { - source_code: message.expect_ruff_source_file(), - fix, - }) - } -} - -impl Display for Diff<'_> { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - // TODO(dhruvmanila): Add support for Notebook cells once it's user-facing - let mut output = String::with_capacity(self.source_code.source_text().len()); - let mut last_end = TextSize::default(); - - for edit in self.fix.edits() { - output.push_str( - self.source_code - .slice(TextRange::new(last_end, edit.start())), - ); - output.push_str(edit.content().unwrap_or_default()); - last_end = edit.end(); - } - - output.push_str(&self.source_code.source_text()[usize::from(last_end)..]); - - let diff = TextDiff::from_lines(self.source_code.source_text(), &output); - - let message = match self.fix.applicability() { - // TODO(zanieb): Adjust this messaging once it's user-facing - Applicability::Safe => "Safe fix", - Applicability::Unsafe => "Unsafe fix", - Applicability::DisplayOnly => "Display-only fix", - }; - writeln!(f, "ℹ {}", message.blue())?; - - let (largest_old, largest_new) = diff - .ops() - .last() - .map(|op| (op.old_range().start, op.new_range().start)) - .unwrap_or_default(); - - let digit_with = - calculate_print_width(OneIndexed::from_zero_indexed(largest_new.max(largest_old))); - - for (idx, group) in diff.grouped_ops(3).iter().enumerate() { - if idx > 0 { - writeln!(f, "{:-^1$}", "-", 80)?; - } - for op in group { - for change in diff.iter_inline_changes(op) { - let sign = match change.tag() { - ChangeTag::Delete => "-", - ChangeTag::Insert => "+", - ChangeTag::Equal => " ", - }; - - let line_style = LineStyle::from(change.tag()); - - let old_index = change.old_index().map(OneIndexed::from_zero_indexed); - let new_index = change.new_index().map(OneIndexed::from_zero_indexed); - - write!( - f, - "{} {} |{}", - Line { - index: old_index, - width: digit_with - }, - Line { - index: new_index, - width: digit_with - }, - line_style.apply_to(sign).bold() - )?; - - for (emphasized, value) in change.iter_strings_lossy() { - let value = value.show_nonprinting(); - if emphasized { - write!(f, "{}", line_style.apply_to(&value).underline().on_black())?; - } else { - write!(f, "{}", line_style.apply_to(&value))?; - } - } - if change.missing_newline() { - writeln!(f)?; - } - } - } - } - - Ok(()) - } -} - -struct LineStyle { - fgcolor: Option, - style: Option, -} - -impl LineStyle { - fn apply_to(&self, input: &str) -> ColoredString { - let mut colored = ColoredString::from(input); - if let Some(color) = self.fgcolor { - colored = colored.color(color); - } - - if let Some(style) = self.style { - match style { - Styles::Clear => colored.clear(), - Styles::Bold => colored.bold(), - Styles::Dimmed => colored.dimmed(), - Styles::Underline => colored.underline(), - Styles::Reversed => colored.reversed(), - Styles::Italic => colored.italic(), - Styles::Blink => colored.blink(), - Styles::Hidden => colored.hidden(), - Styles::Strikethrough => colored.strikethrough(), - } - } else { - colored - } - } -} - -impl From for LineStyle { - fn from(value: ChangeTag) -> Self { - match value { - ChangeTag::Equal => LineStyle { - fgcolor: None, - style: Some(Styles::Dimmed), - }, - ChangeTag::Delete => LineStyle { - fgcolor: Some(Color::Red), - style: None, - }, - ChangeTag::Insert => LineStyle { - fgcolor: Some(Color::Green), - style: None, - }, - } - } -} - -struct Line { - index: Option, - width: NonZeroUsize, -} - -impl Display for Line { - fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { - match self.index { - None => { - for _ in 0..self.width.get() { - f.write_str(" ")?; - } - Ok(()) - } - Some(idx) => write!(f, "{: NonZeroUsize { - const TEN: OneIndexed = OneIndexed::from_zero_indexed(9); - - let mut width = OneIndexed::ONE; - - while value >= TEN { - value = OneIndexed::new(value.get() / 10).unwrap_or(OneIndexed::MIN); - width = width.checked_add(1).unwrap(); - } - - width -} diff --git a/crates/ruff_linter/src/message/grouped.rs b/crates/ruff_linter/src/message/grouped.rs index 0a7de26b2d..1733d94b2b 100644 --- a/crates/ruff_linter/src/message/grouped.rs +++ b/crates/ruff_linter/src/message/grouped.rs @@ -10,7 +10,6 @@ use ruff_notebook::NotebookIndex; use ruff_source_file::{LineColumn, OneIndexed}; use crate::fs::relativize_path; -use crate::message::diff::calculate_print_width; use crate::message::{Emitter, EmitterContext}; use crate::settings::types::UnsafeFixes; @@ -53,8 +52,8 @@ impl Emitter for GroupedEmitter { max_column_length = max_column_length.max(message.start_location.column); } - let row_length = calculate_print_width(max_row_length); - let column_length = calculate_print_width(max_column_length); + let row_length = max_row_length.digits(); + let column_length = max_column_length.digits(); // Print the filename. writeln!(writer, "{}:", relativize_path(&*filename).underline())?; @@ -131,8 +130,7 @@ impl Display for DisplayGroupedMessage<'_> { write!( f, " {row_padding}", - row_padding = " " - .repeat(self.row_length.get() - calculate_print_width(start_location.line).get()) + row_padding = " ".repeat(self.row_length.get() - start_location.line.digits().get()) )?; // Check if we're working on a jupyter notebook and translate positions with cell accordingly @@ -159,9 +157,8 @@ impl Display for DisplayGroupedMessage<'_> { f, "{row}{sep}{col}{col_padding} {code_and_body}", sep = ":".cyan(), - col_padding = " ".repeat( - self.column_length.get() - calculate_print_width(start_location.column).get() - ), + col_padding = + " ".repeat(self.column_length.get() - start_location.column.digits().get()), code_and_body = RuleCodeAndBody { message, show_fix_status: self.show_fix_status, diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index b6328c686b..1918575a2b 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -21,7 +21,6 @@ pub use text::TextEmitter; use crate::Fix; use crate::registry::Rule; -mod diff; mod github; mod gitlab; mod grouped; diff --git a/crates/ruff_linter/src/message/text.rs b/crates/ruff_linter/src/message/text.rs index 610a7a750c..1e47c7e258 100644 --- a/crates/ruff_linter/src/message/text.rs +++ b/crates/ruff_linter/src/message/text.rs @@ -1,23 +1,19 @@ use std::io::Write; -use ruff_db::diagnostic::{Diagnostic, DiagnosticFormat, DisplayDiagnosticConfig}; +use ruff_db::diagnostic::{ + Diagnostic, DiagnosticFormat, DisplayDiagnosticConfig, DisplayDiagnostics, +}; -use crate::message::diff::Diff; use crate::message::{Emitter, EmitterContext}; use crate::settings::types::UnsafeFixes; pub struct TextEmitter { - /// Whether to show the diff of a fix, for diagnostics that have a fix. - /// - /// Note that this is not currently exposed in the CLI (#7352) and is only used in tests. - show_fix_diff: bool, config: DisplayDiagnosticConfig, } impl Default for TextEmitter { fn default() -> Self { Self { - show_fix_diff: false, config: DisplayDiagnosticConfig::default() .format(DiagnosticFormat::Concise) .hide_severity(true) @@ -35,7 +31,7 @@ impl TextEmitter { #[must_use] pub fn with_show_fix_diff(mut self, show_fix_diff: bool) -> Self { - self.show_fix_diff = show_fix_diff; + self.config = self.config.show_fix_diff(show_fix_diff); self } @@ -77,15 +73,11 @@ impl Emitter for TextEmitter { diagnostics: &[Diagnostic], context: &EmitterContext, ) -> anyhow::Result<()> { - for message in diagnostics { - write!(writer, "{}", message.display(context, &self.config))?; - - if self.show_fix_diff { - if let Some(diff) = Diff::from_message(message) { - writeln!(writer, "{diff}")?; - } - } - } + write!( + writer, + "{}", + DisplayDiagnostics::new(context, &self.config, diagnostics) + )?; Ok(()) } diff --git a/crates/ruff_source_file/src/line_index.rs b/crates/ruff_source_file/src/line_index.rs index 1f04aef240..4adb9d17b7 100644 --- a/crates/ruff_source_file/src/line_index.rs +++ b/crates/ruff_source_file/src/line_index.rs @@ -622,6 +622,33 @@ impl OneIndexed { pub fn checked_sub(self, rhs: Self) -> Option { self.0.get().checked_sub(rhs.get()).and_then(Self::new) } + + /// Calculate the number of digits in `self`. + /// + /// This is primarily intended for computing the length of the string representation for + /// formatted printing. + /// + /// # Examples + /// + /// ``` + /// use ruff_source_file::OneIndexed; + /// + /// let one = OneIndexed::new(1).unwrap(); + /// assert_eq!(one.digits().get(), 1); + /// + /// let hundred = OneIndexed::new(100).unwrap(); + /// assert_eq!(hundred.digits().get(), 3); + /// + /// let thousand = OneIndexed::new(1000).unwrap(); + /// assert_eq!(thousand.digits().get(), 4); + /// ``` + pub const fn digits(self) -> NonZeroUsize { + // Safety: the 1+ ensures this is always non-zero, and + // `usize::MAX.ilog10()` << `usize::MAX`, so the result is always safe + // to cast to a usize, even though it's returned as a u32 + // (u64::MAX.ilog10() is 19). + NonZeroUsize::new(1 + self.0.get().ilog10() as usize).unwrap() + } } impl Default for OneIndexed { diff --git a/crates/ty_server/src/server/api/requests/completion.rs b/crates/ty_server/src/server/api/requests/completion.rs index ee59532f9c..214c04582f 100644 --- a/crates/ty_server/src/server/api/requests/completion.rs +++ b/crates/ty_server/src/server/api/requests/completion.rs @@ -6,6 +6,7 @@ use lsp_types::{ CompletionItem, CompletionItemKind, CompletionParams, CompletionResponse, Documentation, Url, }; use ruff_db::source::{line_index, source_text}; +use ruff_source_file::OneIndexed; use ty_ide::completion; use ty_project::ProjectDatabase; use ty_python_semantic::CompletionKind; @@ -59,7 +60,8 @@ impl BackgroundDocumentRequestHandler for CompletionRequestHandler { return Ok(None); } - let max_index_len = completions.len().saturating_sub(1).to_string().len(); + // Safety: we just checked that completions is not empty. + let max_index_len = OneIndexed::new(completions.len()).unwrap().digits().get(); let items: Vec = completions .into_iter() .enumerate()