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.)
This commit is contained in:
Andrew Gallant 2025-04-10 14:26:43 -04:00 committed by Andrew Gallant
parent 0caf09f5c3
commit bd5b8a9ec6
2 changed files with 112 additions and 19 deletions

View file

@ -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<str>,
message: DiagnosticMessage,
annotations: Vec<Annotation>,
subs: Vec<SubDiagnostic>,
}
@ -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<str>,
message: DiagnosticMessage,
annotations: Vec<Annotation>,
}
@ -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<Box<str>>,
message: Option<DiagnosticMessage>,
/// 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<str>`.
/// 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<str>);
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<String> for DiagnosticMessage {
fn from(s: String) -> DiagnosticMessage {
DiagnosticMessage(s.into())
}
}
impl From<Box<str>> for DiagnosticMessage {
fn from(s: Box<str>) -> 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<T: std::fmt::Display> 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

View file

@ -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,
}
}