From 718b0cadf4398bf611bd76c982c64486e679d8e1 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Wed, 2 Apr 2025 10:38:24 -0400 Subject: [PATCH] ruff_db: switch diagnostic rendering over to `std::fmt::Display` It was already using this approach internally, so this is "just" a matter of rejiggering the public API of `Diagnostic`. We were previously writing directly to a `std::io::Write` since it was thought that this worked better with the linear typing fakery. Namely, it increased confidence that the diagnostic rendering was actually written somewhere useful, instead of just being converted to a string that could potentially get lost. For reasons discussed in #17130, the linear type fakery was removed. And so there is less of a reason to require a `std::io::Write` implementation for diagnostic rendering. Indeed, this would sometimes result in `unwrap()` calls when one wants to convert to a `String`. --- crates/red_knot/src/main.rs | 2 +- crates/red_knot_ide/src/goto.rs | 20 +++++------- crates/red_knot_test/src/lib.rs | 9 +++--- crates/red_knot_wasm/src/lib.rs | 7 ++--- crates/ruff_db/src/diagnostic/mod.rs | 27 ++++++++-------- crates/ruff_db/src/diagnostic/render.rs | 41 +++++++++++++++---------- 6 files changed, 53 insertions(+), 53 deletions(-) diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index 1d185f57b3..021a777611 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -288,7 +288,7 @@ impl MainLoop { let diagnostics_count = result.len(); for diagnostic in result { - diagnostic.print(db, &display_config, &mut stdout)?; + write!(stdout, "{}", diagnostic.display(db, &display_config))?; failed |= diagnostic.severity() >= min_error_severity; } diff --git a/crates/red_knot_ide/src/goto.rs b/crates/red_knot_ide/src/goto.rs index 3320fb4eb7..b747d3891f 100644 --- a/crates/red_knot_ide/src/goto.rs +++ b/crates/red_knot_ide/src/goto.rs @@ -241,6 +241,7 @@ pub(crate) fn find_goto_target(parsed: &ParsedModule, offset: TextSize) -> Optio #[cfg(test)] mod tests { + use std::fmt::Write; use crate::db::tests::TestDb; use crate::{goto_type_definition, NavigationTarget}; @@ -860,24 +861,19 @@ f(**kwargs) return "No type definitions found".to_string(); } - let mut buf = vec![]; + let mut buf = String::new(); let source = targets.range; + let config = DisplayDiagnosticConfig::default() + .color(false) + .format(DiagnosticFormat::Full); for target in &*targets { - GotoTypeDefinitionDiagnostic::new(source, target) - .into_diagnostic() - .print( - &self.db, - &DisplayDiagnosticConfig::default() - .color(false) - .format(DiagnosticFormat::Full), - &mut buf, - ) - .unwrap(); + let diag = GotoTypeDefinitionDiagnostic::new(source, target).into_diagnostic(); + write!(buf, "{}", diag.display(&self.db, &config)).unwrap(); } - String::from_utf8(buf).unwrap() + buf } } diff --git a/crates/red_knot_test/src/lib.rs b/crates/red_knot_test/src/lib.rs index c1f1128271..cb1bbc8819 100644 --- a/crates/red_knot_test/src/lib.rs +++ b/crates/red_knot_test/src/lib.rs @@ -13,8 +13,7 @@ use ruff_db::parsed::parsed_module; use ruff_db::system::{DbWithWritableSystem as _, SystemPath, SystemPathBuf}; use ruff_db::testing::{setup_logging, setup_logging_with_filter}; use ruff_source_file::{LineIndex, OneIndexed}; -use std::fmt::Write as _; -use std::io::Write as _; +use std::fmt::Write; mod assertion; mod config; @@ -324,7 +323,7 @@ fn create_diagnostic_snapshot( ) -> String { let display_config = DisplayDiagnosticConfig::default().color(false); - let mut snapshot = Vec::new(); + let mut snapshot = String::new(); writeln!(snapshot).unwrap(); writeln!(snapshot, "---").unwrap(); writeln!(snapshot, "mdtest name: {}", test.name()).unwrap(); @@ -360,8 +359,8 @@ fn create_diagnostic_snapshot( writeln!(snapshot).unwrap(); } writeln!(snapshot, "```").unwrap(); - diag.print(db, &display_config, &mut snapshot).unwrap(); + write!(snapshot, "{}", diag.display(db, &display_config)).unwrap(); writeln!(snapshot, "```").unwrap(); } - String::from_utf8(snapshot).unwrap() + snapshot } diff --git a/crates/red_knot_wasm/src/lib.rs b/crates/red_knot_wasm/src/lib.rs index 0766e8c4f6..b211f5d089 100644 --- a/crates/red_knot_wasm/src/lib.rs +++ b/crates/red_knot_wasm/src/lib.rs @@ -315,11 +315,10 @@ impl Diagnostic { #[wasm_bindgen] pub fn display(&self, workspace: &Workspace) -> JsString { let config = DisplayDiagnosticConfig::default().color(false); - let mut buf = vec![]; self.inner - .print(workspace.db.upcast(), &config, &mut buf) - .unwrap(); - String::from_utf8(buf).unwrap().into() + .display(workspace.db.upcast(), &config) + .to_string() + .into() } } diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index d839e2d553..b5701e7501 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -5,11 +5,12 @@ use thiserror::Error; use ruff_annotate_snippets::Level as AnnotateLevel; use ruff_text_size::TextRange; +pub use self::render::DisplayDiagnostic; pub use crate::diagnostic::old::OldSecondaryDiagnosticMessage; use crate::files::File; use crate::Db; -use self::render::{DisplayDiagnostic, FileResolver}; +use self::render::FileResolver; // This module should not be exported. We are planning to migrate off // the APIs in this module. @@ -100,22 +101,18 @@ impl Diagnostic { Arc::make_mut(&mut self.inner).subs.push(sub); } - /// Print this diagnostic to the given writer. + /// Return a `std::fmt::Display` implementation that renders this + /// diagnostic into a human readable format. /// - /// # Errors - /// - /// This is guaranteed to only return an error if the underlying - /// writer given returns an error. Otherwise, the formatting of - /// diagnostics themselves is infallible. - pub fn print( - &self, - db: &dyn Db, - config: &DisplayDiagnosticConfig, - mut wtr: impl std::io::Write, - ) -> std::io::Result<()> { + /// Note that this `Display` impl includes a trailing line terminator, so + /// callers should prefer using this with `write!` instead of `writeln!`. + pub fn display<'c, 'db, 'd>( + &'d self, + db: &'db dyn Db, + config: &'c DisplayDiagnosticConfig, + ) -> DisplayDiagnostic<'c, 'db, 'd> { let resolver = FileResolver::new(db); - let display = DisplayDiagnostic::new(&resolver, config, self); - writeln!(wtr, "{display}") + DisplayDiagnostic::new(resolver, config, self) } /// Returns the identifier for this diagnostic. diff --git a/crates/ruff_db/src/diagnostic/render.rs b/crates/ruff_db/src/diagnostic/render.rs index d6dfd8521d..d27a929c64 100644 --- a/crates/ruff_db/src/diagnostic/render.rs +++ b/crates/ruff_db/src/diagnostic/render.rs @@ -17,20 +17,31 @@ use super::{ Annotation, Diagnostic, DiagnosticFormat, DisplayDiagnosticConfig, Severity, SubDiagnostic, }; +/// A type that implements `std::fmt::Display` for diagnostic rendering. +/// +/// It is created via [`Diagnostic::display`]. +/// +/// The lifetime parameters are: +/// +/// * `'c` is the lifetime of the rendering configuration. +/// * `'r` is the lifetime of the resolver used to load the contents of `Span` +/// values. When using Salsa, this most commonly corresponds to the lifetime +/// of a Salsa `Db`. +/// * `'d` is the lifetime of the diagnostic being rendered. #[derive(Debug)] -pub(crate) struct DisplayDiagnostic<'a> { - config: &'a DisplayDiagnosticConfig, - resolver: &'a FileResolver<'a>, +pub struct DisplayDiagnostic<'c, 'r, 'd> { + config: &'c DisplayDiagnosticConfig, + resolver: FileResolver<'r>, annotate_renderer: AnnotateRenderer, - diag: &'a Diagnostic, + diag: &'d Diagnostic, } -impl<'a> DisplayDiagnostic<'a> { +impl<'c, 'r, 'd> DisplayDiagnostic<'c, 'r, 'd> { pub(crate) fn new( - resolver: &'a FileResolver<'a>, - config: &'a DisplayDiagnosticConfig, - diag: &'a Diagnostic, - ) -> DisplayDiagnostic<'a> { + resolver: FileResolver<'r>, + config: &'c DisplayDiagnosticConfig, + diag: &'d Diagnostic, + ) -> DisplayDiagnostic<'c, 'r, 'd> { let annotate_renderer = if config.color { AnnotateRenderer::styled() } else { @@ -45,7 +56,7 @@ impl<'a> DisplayDiagnostic<'a> { } } -impl std::fmt::Display for DisplayDiagnostic<'_> { +impl std::fmt::Display for DisplayDiagnostic<'_, '_, '_> { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { if matches!(self.config.format, DiagnosticFormat::Concise) { match self.diag.severity() { @@ -65,15 +76,15 @@ impl std::fmt::Display for DisplayDiagnostic<'_> { } write!(f, ":")?; } - return write!(f, " {message}", message = self.diag.primary_message()); + return writeln!(f, " {message}", message = self.diag.primary_message()); } - let resolved = Resolved::new(self.resolver, self.diag); + let resolved = Resolved::new(&self.resolver, self.diag); let renderable = resolved.to_renderable(self.config.context); for diag in renderable.diagnostics.iter() { writeln!(f, "{}", self.annotate_renderer.render(diag.to_annotate()))?; } - Ok(()) + writeln!(f) } } @@ -2130,9 +2141,7 @@ watermelon /// /// (This will set the "printed" flag on `Diagnostic`.) fn render(&self, diag: &Diagnostic) -> String { - let mut buf = vec![]; - diag.print(&self.db, &self.config, &mut buf).unwrap(); - String::from_utf8(buf).unwrap() + diag.display(&self.db, &self.config).to_string() } }