From bd5b8a9ec6a1877c6d457c1f2e52e57ea37679de Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Thu, 10 Apr 2025 14:26:43 -0400 Subject: [PATCH] ruff_db: tweak APIs accepting a diagnostic message This expands the set of types accepted for diagnostic messages. Instead of only `std::fmt::Display` impls, we now *also* accept a concrete `DiagnosticMessage`. This will be useful to avoid unnecessary copies of every single diagnostic message created via `InferContext::report_lint`. (I'll call out how this helps in a subsequent commit.) --- crates/ruff_db/src/diagnostic/mod.rs | 125 +++++++++++++++++++++--- crates/ruff_db/src/diagnostic/render.rs | 6 +- 2 files changed, 112 insertions(+), 19 deletions(-) diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index 99e478a1d8..c638dde8e1 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -45,16 +45,21 @@ impl Diagnostic { /// describes. Stated differently, if only one thing from a diagnostic can /// be shown to an end user in a particular context, it is the primary /// message. + /// + /// # Types implementing `IntoDiagnosticMessage` + /// + /// Callers can pass anything that implements `std::fmt::Display` + /// directly. If callers want or need to avoid cloning the diagnostic + /// message, then they can also pass a `DiagnosticMessage` directly. pub fn new<'a>( id: DiagnosticId, severity: Severity, - message: impl std::fmt::Display + 'a, + message: impl IntoDiagnosticMessage + 'a, ) -> Diagnostic { - let message = message.to_string().into_boxed_str(); let inner = Arc::new(DiagnosticInner { id, severity, - message, + message: message.into_diagnostic_message(), annotations: vec![], subs: vec![], }); @@ -83,7 +88,13 @@ impl Diagnostic { /// message is about a function call being invalid, a useful "info" /// sub-diagnostic could show the function definition (or only the relevant /// parts of it). - pub fn info<'a>(&mut self, message: impl std::fmt::Display + 'a) { + /// + /// # Types implementing `IntoDiagnosticMessage` + /// + /// Callers can pass anything that implements `std::fmt::Display` + /// directly. If callers want or need to avoid cloning the diagnostic + /// message, then they can also pass a `DiagnosticMessage` directly. + pub fn info<'a>(&mut self, message: impl IntoDiagnosticMessage + 'a) { self.sub(SubDiagnostic::new(Severity::Info, message)); } @@ -126,8 +137,8 @@ impl Diagnostic { /// didn't distinguish between messages on the entire diagnostic and /// messages attached to a particular span.) pub fn primary_message(&self) -> &str { - if !self.inner.message.is_empty() { - return &self.inner.message; + if !self.inner.message.as_str().is_empty() { + return self.inner.message.as_str(); } // FIXME: As a special case, while we're migrating Red Knot // to the new diagnostic data model, we'll look for a primary @@ -167,7 +178,7 @@ impl Diagnostic { /// cases, just converting it to a string (or printing it) will do what /// you want. pub fn concise_message(&self) -> ConciseMessage { - let main = &self.inner.message; + let main = self.inner.message.as_str(); let annotation = self .primary_annotation() .and_then(|ann| ann.get_message()) @@ -208,7 +219,7 @@ impl Diagnostic { struct DiagnosticInner { id: DiagnosticId, severity: Severity, - message: Box, + message: DiagnosticMessage, annotations: Vec, subs: Vec, } @@ -241,11 +252,16 @@ impl SubDiagnostic { /// describes. Stated differently, if only one thing from a diagnostic can /// be shown to an end user in a particular context, it is the primary /// message. - pub fn new<'a>(severity: Severity, message: impl std::fmt::Display + 'a) -> SubDiagnostic { - let message = message.to_string().into_boxed_str(); + /// + /// # Types implementing `IntoDiagnosticMessage` + /// + /// Callers can pass anything that implements `std::fmt::Display` + /// directly. If callers want or need to avoid cloning the diagnostic + /// message, then they can also pass a `DiagnosticMessage` directly. + pub fn new<'a>(severity: Severity, message: impl IntoDiagnosticMessage + 'a) -> SubDiagnostic { let inner = Box::new(SubDiagnosticInner { severity, - message, + message: message.into_diagnostic_message(), annotations: vec![], }); SubDiagnostic { inner } @@ -270,7 +286,7 @@ impl SubDiagnostic { #[derive(Debug, Clone, Eq, PartialEq)] struct SubDiagnosticInner { severity: Severity, - message: Box, + message: DiagnosticMessage, annotations: Vec, } @@ -304,7 +320,7 @@ pub struct Annotation { /// /// When present, rendering will include this message in the output and /// draw a line between the highlighted span and the message. - message: Option>, + message: Option, /// Whether this annotation is "primary" or not. When it isn't primary, an /// annotation is said to be "secondary." is_primary: bool, @@ -351,14 +367,20 @@ impl Annotation { /// /// When a message is attached to an annotation, then it will be associated /// with the highlighted span in some way during rendering. - pub fn message<'a>(self, message: impl std::fmt::Display + 'a) -> Annotation { - let message = Some(message.to_string().into_boxed_str()); + /// + /// # Types implementing `IntoDiagnosticMessage` + /// + /// Callers can pass anything that implements `std::fmt::Display` + /// directly. If callers want or need to avoid cloning the diagnostic + /// message, then they can also pass a `DiagnosticMessage` directly. + pub fn message<'a>(self, message: impl IntoDiagnosticMessage + 'a) -> Annotation { + let message = Some(message.into_diagnostic_message()); Annotation { message, ..self } } /// Returns the message attached to this annotation, if one exists. pub fn get_message(&self) -> Option<&str> { - self.message.as_deref() + self.message.as_ref().map(|m| m.as_str()) } } @@ -698,6 +720,77 @@ impl std::fmt::Display for ConciseMessage<'_> { } } +/// A diagnostic message string. +/// +/// This is, for all intents and purposes, equivalent to a `Box`. +/// But it does not implement `std::fmt::Display`. Indeed, that it its +/// entire reason for existence. It provides a way to pass a string +/// directly into diagnostic methods that accept messages without copying +/// that string. This works via the `IntoDiagnosticMessage` trait. +/// +/// In most cases, callers shouldn't need to use this. Instead, there is +/// a blanket trait implementation for `IntoDiagnosticMessage` for +/// anything that implements `std::fmt::Display`. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct DiagnosticMessage(Box); + +impl DiagnosticMessage { + /// Returns this message as a borrowed string. + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl From<&str> for DiagnosticMessage { + fn from(s: &str) -> DiagnosticMessage { + DiagnosticMessage(s.into()) + } +} + +impl From for DiagnosticMessage { + fn from(s: String) -> DiagnosticMessage { + DiagnosticMessage(s.into()) + } +} + +impl From> for DiagnosticMessage { + fn from(s: Box) -> DiagnosticMessage { + DiagnosticMessage(s) + } +} + +impl IntoDiagnosticMessage for DiagnosticMessage { + fn into_diagnostic_message(self) -> DiagnosticMessage { + self + } +} + +/// A trait for values that can be converted into a diagnostic message. +/// +/// Users of the diagnostic API can largely think of this trait as effectively +/// equivalent to `std::fmt::Display`. Indeed, everything that implements +/// `Display` also implements this trait. That means wherever this trait is +/// accepted, you can use things like `format_args!`. +/// +/// The purpose of this trait is to provide a means to give arguments _other_ +/// than `std::fmt::Display` trait implementations. Or rather, to permit +/// the diagnostic API to treat them differently. For example, this lets +/// callers wrap a string in a `DiagnosticMessage` and provide it directly +/// to any of the diagnostic APIs that accept a message. This will move the +/// string and avoid any unnecessary copies. (If we instead required only +/// `std::fmt::Display`, then this would potentially result in a copy via the +/// `ToString` trait implementation.) +pub trait IntoDiagnosticMessage { + fn into_diagnostic_message(self) -> DiagnosticMessage; +} + +/// Every `IntoDiagnosticMessage` is accepted, so to is `std::fmt::Display`. +impl IntoDiagnosticMessage for T { + fn into_diagnostic_message(self) -> DiagnosticMessage { + DiagnosticMessage::from(self.to_string()) + } +} + /// Creates a `Diagnostic` from a parse error. /// /// This should _probably_ be a method on `ruff_python_parser::ParseError`, but diff --git a/crates/ruff_db/src/diagnostic/render.rs b/crates/ruff_db/src/diagnostic/render.rs index a2dd0cf76a..111af97b6d 100644 --- a/crates/ruff_db/src/diagnostic/render.rs +++ b/crates/ruff_db/src/diagnostic/render.rs @@ -159,7 +159,7 @@ impl<'a> ResolvedDiagnostic<'a> { ResolvedAnnotation::new(path, &input, ann) }) .collect(); - let message = if diag.inner.message.is_empty() { + let message = if diag.inner.message.as_str().is_empty() { diag.inner.id.to_string() } else { // TODO: See the comment on `Renderable::id` for @@ -168,7 +168,7 @@ impl<'a> ResolvedDiagnostic<'a> { format!( "{id}: {message}", id = diag.inner.id, - message = diag.inner.message + message = diag.inner.message.as_str(), ) }; ResolvedDiagnostic { @@ -195,7 +195,7 @@ impl<'a> ResolvedDiagnostic<'a> { .collect(); ResolvedDiagnostic { severity: diag.inner.severity, - message: diag.inner.message.to_string(), + message: diag.inner.message.as_str().to_string(), annotations, } }