Move fix suggestion to subdiagnostic (#19464)

Summary
--

This PR tweaks Ruff's internal usage of the new diagnostic model to more
closely
match the intended use, as I understand it. Specifically, it moves the
fix/help
suggestion from the primary annotation's message to a subdiagnostic. In
turn, it
adds the secondary/noqa code as the new primary annotation message. As
shown in
the new `ruff_db` tests, this more closely mirrors Ruff's current
diagnostic
output.

I also added `Severity::Help` to render the fix suggestion with a
`help:` prefix
instead of `info:`.

These changes don't have any external impact now but should help a bit
with #19415.

Test Plan
--

New full output format tests in `ruff_db`

Rendered Diagnostics
--

Full diagnostic output from `annotate-snippets` in this PR:

``` 
error[unused-import]: `os` imported but unused
  --> fib.py:1:8
   |
 1 | import os
   |        ^^
   |
 help: Remove unused import: `os`
```

Current Ruff output for the same code:

```
fib.py:1:8: F401 [*] `os` imported but unused
  |
1 | import os
  |        ^^ F401
  |
  = help: Remove unused import: `os`
```

Proposed final output after #19415:

``` 
F401 [*] `os` imported but unused
  --> fib.py:1:8
   |
 1 | import os
   |        ^^
   |
 help: Remove unused import: `os`
```

These are slightly updated from
#19464 (comment)
below to remove the extra noqa codes in the primary annotation messages
for the first and third cases.

Author: ntBre

crates/ruff/src/cache.rs

@@ -454,7 +454,7 @@ impl LintCacheData {
                 CacheMessage {
                     rule,
                     body: msg.body().to_string(),
-                    suggestion: msg.suggestion().map(ToString::to_string),
+                    suggestion: msg.first_help_text().map(ToString::to_string),
                     range: msg.expect_range(),
                     parent: msg.parent(),
                     fix: msg.fix().cloned(),

crates/ruff_db/src/diagnostic/mod.rs

@@ -122,7 +122,14 @@ impl Diagnostic {
     /// 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));
+        self.sub(SubDiagnostic::new(SubDiagnosticSeverity::Info, message));
+    }
+
+    /// Adds a "help" sub-diagnostic with the given message.
+    ///
+    /// See the closely related [`Diagnostic::info`] method for more details.
+    pub fn help<'a>(&mut self, message: impl IntoDiagnosticMessage + 'a) {
+        self.sub(SubDiagnostic::new(SubDiagnosticSeverity::Help, message));
     }
 
     /// Adds a "sub" diagnostic to this diagnostic.
@@ -377,9 +384,15 @@ impl Diagnostic {
         self.primary_message()
     }
 
-    /// Returns the fix suggestion for the violation.
-    pub fn suggestion(&self) -> Option<&str> {
-        self.primary_annotation()?.get_message()
+    /// Returns the message of the first sub-diagnostic with a `Help` severity.
+    ///
+    /// Note that this is used as the fix title/suggestion for some of Ruff's output formats, but in
+    /// general this is not the guaranteed meaning of such a message.
+    pub fn first_help_text(&self) -> Option<&str> {
+        self.sub_diagnostics()
+            .iter()
+            .find(|sub| matches!(sub.inner.severity, SubDiagnosticSeverity::Help))
+            .map(|sub| sub.inner.message.as_str())
     }
 
     /// Returns the URL for the rule documentation, if it exists.
@@ -565,7 +578,10 @@ impl SubDiagnostic {
     /// 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 {
+    pub fn new<'a>(
+        severity: SubDiagnosticSeverity,
+        message: impl IntoDiagnosticMessage + 'a,
+    ) -> SubDiagnostic {
         let inner = Box::new(SubDiagnosticInner {
             severity,
             message: message.into_diagnostic_message(),
@@ -643,7 +659,7 @@ impl SubDiagnostic {
 
 #[derive(Debug, Clone, Eq, PartialEq, get_size2::GetSize)]
 struct SubDiagnosticInner {
-    severity: Severity,
+    severity: SubDiagnosticSeverity,
     message: DiagnosticMessage,
     annotations: Vec<Annotation>,
 }
@@ -1170,6 +1186,30 @@ impl Severity {
     }
 }
 
+/// Like [`Severity`] but exclusively for sub-diagnostics.
+///
+/// This supports an additional `Help` severity that may not be needed in main diagnostics.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, get_size2::GetSize)]
+pub enum SubDiagnosticSeverity {
+    Help,
+    Info,
+    Warning,
+    Error,
+    Fatal,
+}
+
+impl SubDiagnosticSeverity {
+    fn to_annotate(self) -> AnnotateLevel {
+        match self {
+            SubDiagnosticSeverity::Help => AnnotateLevel::Help,
+            SubDiagnosticSeverity::Info => AnnotateLevel::Info,
+            SubDiagnosticSeverity::Warning => AnnotateLevel::Warning,
+            SubDiagnosticSeverity::Error => AnnotateLevel::Error,
+            SubDiagnosticSeverity::Fatal => AnnotateLevel::Error,
+        }
+    }
+}
+
 /// Configuration for rendering diagnostics.
 #[derive(Clone, Debug)]
 pub struct DisplayDiagnosticConfig {

crates/ruff_db/src/diagnostic/render.rs

@@ -26,6 +26,7 @@ use azure::AzureRenderer;
 use pylint::PylintRenderer;
 
 mod azure;
+mod full;
 #[cfg(feature = "serde")]
 mod json;
 #[cfg(feature = "serde")]
@@ -256,7 +257,7 @@ impl<'a> Resolved<'a> {
 /// both.)
 #[derive(Debug)]
 struct ResolvedDiagnostic<'a> {
-    severity: Severity,
+    level: AnnotateLevel,
     id: Option<String>,
     message: String,
     annotations: Vec<ResolvedAnnotation<'a>>,
@@ -281,7 +282,7 @@ impl<'a> ResolvedDiagnostic<'a> {
         let id = Some(diag.inner.id.to_string());
         let message = diag.inner.message.as_str().to_string();
         ResolvedDiagnostic {
-            severity: diag.inner.severity,
+            level: diag.inner.severity.to_annotate(),
             id,
             message,
             annotations,
@@ -304,7 +305,7 @@ impl<'a> ResolvedDiagnostic<'a> {
             })
             .collect();
         ResolvedDiagnostic {
-            severity: diag.inner.severity,
+            level: diag.inner.severity.to_annotate(),
             id: None,
             message: diag.inner.message.as_str().to_string(),
             annotations,
@@ -371,7 +372,7 @@ impl<'a> ResolvedDiagnostic<'a> {
         snippets_by_input
             .sort_by(|snips1, snips2| snips1.has_primary.cmp(&snips2.has_primary).reverse());
         RenderableDiagnostic {
-            severity: self.severity,
+            level: self.level,
             id: self.id.as_deref(),
             message: &self.message,
             snippets_by_input,
@@ -459,7 +460,7 @@ struct Renderable<'r> {
 #[derive(Debug)]
 struct RenderableDiagnostic<'r> {
     /// The severity of the diagnostic.
-    severity: Severity,
+    level: AnnotateLevel,
     /// The ID of the diagnostic. The ID can usually be used on the CLI or in a
     /// config file to change the severity of a lint.
     ///
@@ -478,15 +479,14 @@ struct RenderableDiagnostic<'r> {
 impl RenderableDiagnostic<'_> {
     /// Convert this to an "annotate" snippet.
     fn to_annotate(&self) -> AnnotateMessage<'_> {
-        let level = self.severity.to_annotate();
         let snippets = self.snippets_by_input.iter().flat_map(|snippets| {
             let path = snippets.path;
             snippets
                 .snippets
                 .iter()
                 .map(|snippet| snippet.to_annotate(path))
         });
-        let mut message = level.title(self.message);
+        let mut message = self.level.title(self.message);
         if let Some(id) = self.id {
             message = message.id(id);
         }
@@ -864,7 +864,10 @@ mod tests {
 
     use ruff_diagnostics::{Edit, Fix};
 
-    use crate::diagnostic::{Annotation, DiagnosticId, SecondaryCode, Severity, Span};
+    use crate::diagnostic::{
+        Annotation, DiagnosticId, IntoDiagnosticMessage, SecondaryCode, Severity, Span,
+        SubDiagnosticSeverity,
+    };
     use crate::files::system_path_to_file;
     use crate::system::{DbWithWritableSystem, SystemPath};
     use crate::tests::TestDb;
@@ -1548,7 +1551,7 @@ watermelon
 
         let mut diag = env.err().primary("animals", "3", "3", "").build();
         diag.sub(
-            env.sub_builder(Severity::Info, "this is a helpful note")
+            env.sub_builder(SubDiagnosticSeverity::Info, "this is a helpful note")
                 .build(),
         );
         insta::assert_snapshot!(
@@ -1577,15 +1580,15 @@ watermelon
 
         let mut diag = env.err().primary("animals", "3", "3", "").build();
         diag.sub(
-            env.sub_builder(Severity::Info, "this is a helpful note")
+            env.sub_builder(SubDiagnosticSeverity::Info, "this is a helpful note")
                 .build(),
         );
         diag.sub(
-            env.sub_builder(Severity::Info, "another helpful note")
+            env.sub_builder(SubDiagnosticSeverity::Info, "another helpful note")
                 .build(),
         );
         diag.sub(
-            env.sub_builder(Severity::Info, "and another helpful note")
+            env.sub_builder(SubDiagnosticSeverity::Info, "and another helpful note")
                 .build(),
         );
         insta::assert_snapshot!(
@@ -2370,7 +2373,7 @@ watermelon
         /// sub-diagnostic with "error" severity and canned values for
         /// its identifier and message.
         fn sub_warn(&mut self) -> SubDiagnosticBuilder<'_> {
-            self.sub_builder(Severity::Warning, "sub-diagnostic message")
+            self.sub_builder(SubDiagnosticSeverity::Warning, "sub-diagnostic message")
         }
 
         /// Returns a builder for tersely constructing diagnostics.
@@ -2391,7 +2394,11 @@ watermelon
         }
 
         /// Returns a builder for tersely constructing sub-diagnostics.
-        fn sub_builder(&mut self, severity: Severity, message: &str) -> SubDiagnosticBuilder<'_> {
+        fn sub_builder(
+            &mut self,
+            severity: SubDiagnosticSeverity,
+            message: &str,
+        ) -> SubDiagnosticBuilder<'_> {
             let subdiag = SubDiagnostic::new(severity, message);
             SubDiagnosticBuilder { env: self, subdiag }
         }
@@ -2494,6 +2501,12 @@ watermelon
             self.diag.set_noqa_offset(noqa_offset);
             self
         }
+
+        /// Adds a "help" sub-diagnostic with the given message.
+        fn help(mut self, message: impl IntoDiagnosticMessage) -> DiagnosticBuilder<'e> {
+            self.diag.help(message);
+            self
+        }
     }
 
     /// A helper builder for tersely populating a `SubDiagnostic`.
@@ -2600,7 +2613,8 @@ def fibonacci(n):
 
         let diagnostics = vec![
             env.builder("unused-import", Severity::Error, "`os` imported but unused")
-                .primary("fib.py", "1:7", "1:9", "Remove unused import: `os`")
+                .primary("fib.py", "1:7", "1:9", "")
+                .help("Remove unused import: `os`")
                 .secondary_code("F401")
                 .fix(Fix::unsafe_edit(Edit::range_deletion(TextRange::new(
                     TextSize::from(0),
@@ -2613,12 +2627,8 @@ def fibonacci(n):
                 Severity::Error,
                 "Local variable `x` is assigned to but never used",
             )
-            .primary(
-                "fib.py",
-                "6:4",
-                "6:5",
-                "Remove assignment to unused variable `x`",
-            )
+            .primary("fib.py", "6:4", "6:5", "")
+            .help("Remove assignment to unused variable `x`")
             .secondary_code("F841")
             .fix(Fix::unsafe_edit(Edit::deletion(
                 TextSize::from(94),
@@ -2720,7 +2730,8 @@ if call(foo
 
         let diagnostics = vec![
             env.builder("unused-import", Severity::Error, "`os` imported but unused")
-                .primary("notebook.ipynb", "2:7", "2:9", "Remove unused import: `os`")
+                .primary("notebook.ipynb", "2:7", "2:9", "")
+                .help("Remove unused import: `os`")
                 .secondary_code("F401")
                 .fix(Fix::safe_edit(Edit::range_deletion(TextRange::new(
                     TextSize::from(9),
@@ -2733,12 +2744,8 @@ if call(foo
                 Severity::Error,
                 "`math` imported but unused",
             )
-            .primary(
-                "notebook.ipynb",
-                "4:7",
-                "4:11",
-                "Remove unused import: `math`",
-            )
+            .primary("notebook.ipynb", "4:7", "4:11", "")
+            .help("Remove unused import: `math`")
             .secondary_code("F401")
             .fix(Fix::safe_edit(Edit::range_deletion(TextRange::new(
                 TextSize::from(28),
@@ -2751,12 +2758,8 @@ if call(foo
                 Severity::Error,
                 "Local variable `x` is assigned to but never used",
             )
-            .primary(
-                "notebook.ipynb",
-                "10:4",
-                "10:5",
-                "Remove assignment to unused variable `x`",
-            )
+            .primary("notebook.ipynb", "10:4", "10:5", "")
+            .help("Remove assignment to unused variable `x`")
             .secondary_code("F841")
             .fix(Fix::unsafe_edit(Edit::range_deletion(TextRange::new(
                 TextSize::from(94),

crates/ruff_db/src/diagnostic/render/full.rs

@@ -0,0 +1,66 @@
+#[cfg(test)]
+mod tests {
+    use crate::diagnostic::{
+        DiagnosticFormat,
+        render::tests::{create_diagnostics, create_syntax_error_diagnostics},
+    };
+
+    #[test]
+    fn output() {
+        let (env, diagnostics) = create_diagnostics(DiagnosticFormat::Full);
+        insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r#"
+        error[unused-import]: `os` imported but unused
+         --> fib.py:1:8
+          |
+        1 | import os
+          |        ^^
+          |
+        help: Remove unused import: `os`
+
+        error[unused-variable]: Local variable `x` is assigned to but never used
+         --> fib.py:6:5
+          |
+        4 | def fibonacci(n):
+        5 |     """Compute the nth number in the Fibonacci sequence."""
+        6 |     x = 1
+          |     ^
+        7 |     if n == 0:
+        8 |         return 0
+          |
+        help: Remove assignment to unused variable `x`
+
+        error[undefined-name]: Undefined name `a`
+         --> undef.py:1:4
+          |
+        1 | if a == 1: pass
+          |    ^
+          |
+        "#);
+    }
+
+    #[test]
+    fn syntax_errors() {
+        let (env, diagnostics) = create_syntax_error_diagnostics(DiagnosticFormat::Full);
+        insta::assert_snapshot!(env.render_diagnostics(&diagnostics), @r"
+        error[invalid-syntax]: SyntaxError: Expected one or more symbol names after import
+         --> syntax_errors.py:1:15
+          |
+        1 | from os import
+          |               ^
+        2 |
+        3 | if call(foo
+          |
+
+        error[invalid-syntax]: SyntaxError: Expected ')', found newline
+         --> syntax_errors.py:3:12
+          |
+        1 | from os import
+        2 |
+        3 | if call(foo
+          |            ^
+        4 |     def bar():
+        5 |         pass
+          |
+        ");
+    }
+}

crates/ruff_db/src/diagnostic/render/json.rs

@@ -87,7 +87,7 @@ pub(super) fn diagnostic_to_json<'a>(
 
     let fix = diagnostic.fix().map(|fix| JsonFix {
         applicability: fix.applicability(),
-        message: diagnostic.suggestion(),
+        message: diagnostic.first_help_text(),
         edits: ExpandedEdits {
             edits: fix.edits(),
             notebook_index,

crates/ruff_linter/src/message/mod.rs

@@ -75,11 +75,12 @@ where
     );
 
     let span = Span::from(file).with_range(range);
-    let mut annotation = Annotation::primary(span);
+    let annotation = Annotation::primary(span);
+    diagnostic.annotate(annotation);
+
     if let Some(suggestion) = suggestion {
-        annotation = annotation.message(suggestion);
+        diagnostic.help(suggestion);
     }
-    diagnostic.annotate(annotation);
 
     if let Some(fix) = fix {
         diagnostic.set_fix(fix);