[ruff] Treat panics as fatal diagnostics, sort panics last (#20258)

- Convert panics to diagnostics with id `Panic`, severity `Fatal`, and
the error as the diagnostic message, annotated with a `Span` with empty
code block and no range.
- Updates the post-linting message diagnostic handling to track the
maximum severity seen, and then prints the "report a bug in ruff"
message only if the max severity was `Fatal`

This depends on the sorting changes since it creates diagnostics with no
range specified.

Author: amyreese

crates/ruff/src/commands/check.rs

@@ -6,12 +6,14 @@ use std::time::Instant;
 use anyhow::Result;
 use colored::Colorize;
 use ignore::Error;
-use log::{debug, error, warn};
+use log::{debug, warn};
 #[cfg(not(target_family = "wasm"))]
 use rayon::prelude::*;
 use rustc_hash::FxHashMap;
 
-use ruff_db::diagnostic::Diagnostic;
+use ruff_db::diagnostic::{
+    Annotation, Diagnostic, DiagnosticId, Span, SubDiagnostic, SubDiagnosticSeverity,
+};
 use ruff_db::panic::catch_unwind;
 use ruff_linter::package::PackageRoot;
 use ruff_linter::registry::Rule;
@@ -193,21 +195,24 @@ fn lint_path(
     match result {
         Ok(inner) => inner,
         Err(error) => {
-            let message = r"This indicates a bug in Ruff. If you could open an issue at:
-
-    https://github.com/astral-sh/ruff/issues/new?title=%5BLinter%20panic%5D
-
-...with the relevant file contents, the `pyproject.toml` settings, and the following stack trace, we'd be very appreciative!
-";
-
-            error!(
-                "{}{}{} {message}\n{error}",
-                "Panicked while linting ".bold(),
-                fs::relativize_path(path).bold(),
-                ":".bold()
+            let message = match error.payload.as_str() {
+                Some(summary) => format!("Fatal error while linting: {summary}"),
+                _ => "Fatal error while linting".to_owned(),
+            };
+            let mut diagnostic = Diagnostic::new(
+                DiagnosticId::Panic,
+                ruff_db::diagnostic::Severity::Fatal,
+                message,
             );
-
-            Ok(Diagnostics::default())
+            let span = Span::from(SourceFileBuilder::new(path.to_string_lossy(), "").finish());
+            let mut annotation = Annotation::primary(span);
+            annotation.set_file_level(true);
+            diagnostic.annotate(annotation);
+            diagnostic.sub(SubDiagnostic::new(
+                SubDiagnosticSeverity::Info,
+                format!("{error}"),
+            ));
+            Ok(Diagnostics::new(vec![diagnostic], FxHashMap::default()))
         }
     }
 }

crates/ruff/src/lib.rs

@@ -9,10 +9,11 @@ use std::sync::mpsc::channel;
 use anyhow::Result;
 use clap::CommandFactory;
 use colored::Colorize;
-use log::warn;
+use log::{error, warn};
 use notify::{RecursiveMode, Watcher, recommended_watcher};
 
 use args::{GlobalConfigArgs, ServerCommand};
+use ruff_db::diagnostic::{Diagnostic, Severity};
 use ruff_linter::logging::{LogLevel, set_up_logging};
 use ruff_linter::settings::flags::FixMode;
 use ruff_linter::settings::types::OutputFormat;
@@ -444,6 +445,27 @@ pub fn check(args: CheckCommand, global_options: GlobalConfigArgs) -> Result<Exi
         }
 
         if !cli.exit_zero {
+            let max_severity = diagnostics
+                .inner
+                .iter()
+                .map(Diagnostic::severity)
+                .max()
+                .unwrap_or(Severity::Info);
+            if max_severity.is_fatal() {
+                // When a panic/fatal error is reported, prompt the user to open an issue on github.
+                // Diagnostics with severity `fatal` will be sorted to the bottom, and printing the
+                // message here instead of attaching it to the diagnostic ensures that we only print
+                // it once instead of repeating it for each diagnostic. Prints to stderr to prevent
+                // the message from being captured by tools parsing the normal output.
+                let message = "Panic during linting indicates a bug in Ruff. If you could open an issue at:
+
+https://github.com/astral-sh/ruff/issues/new?title=%5BLinter%20panic%5D
+
+...with the relevant file contents, the `pyproject.toml` settings, and the stack trace above, we'd be very appreciative!
+";
+                error!("{message}");
+                return Ok(ExitStatus::Error);
+            }
             if cli.diff {
                 // If we're printing a diff, we always want to exit non-zero if there are
                 // any fixable violations (since we've printed the diff, but not applied the

crates/ruff/tests/lint.rs

@@ -5891,3 +5891,113 @@ fn show_fixes_in_full_output_with_preview_enabled() {
     ",
     );
 }
+
+#[test]
+fn rule_panic_mixed_results_concise() -> Result<()> {
+    let tempdir = TempDir::new()?;
+
+    // Create python files
+    let file_a_path = tempdir.path().join("normal.py");
+    let file_b_path = tempdir.path().join("panic.py");
+    fs::write(&file_a_path, b"import os")?;
+    fs::write(&file_b_path, b"print('hello, world!')")?;
+
+    insta::with_settings!({
+        filters => vec![
+            (tempdir_filter(&tempdir).as_str(), "[TMP]/"),
+            (r"\\", r"/"),
+        ]
+    }, {
+    assert_cmd_snapshot!(
+        Command::new(get_cargo_bin(BIN_NAME))
+            .args(["check", "--select", "RUF9", "--preview", "--output-format=concise", "--no-cache"])
+            .args([file_a_path, file_b_path]),
+        @r"
+    success: false
+    exit_code: 2
+    ----- stdout -----
+    [TMP]/normal.py:1:1: RUF900 Hey this is a stable test rule.
+    [TMP]/normal.py:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
+    [TMP]/normal.py:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
+    [TMP]/normal.py:1:1: RUF903 Hey this is a stable test rule with a display only fix.
+    [TMP]/normal.py:1:1: RUF911 Hey this is a preview test rule.
+    [TMP]/normal.py:1:1: RUF950 Hey this is a test rule that was redirected from another.
+    [TMP]/panic.py: panic: Fatal error while linting: This is a fake panic for testing.
+    Found 7 errors.
+    [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
+
+    ----- stderr -----
+    error: Panic during linting indicates a bug in Ruff. If you could open an issue at:
+
+    https://github.com/astral-sh/ruff/issues/new?title=%5BLinter%20panic%5D
+
+    ...with the relevant file contents, the `pyproject.toml` settings, and the stack trace above, we'd be very appreciative!
+    ");
+    });
+    Ok(())
+}
+
+#[test]
+fn rule_panic_mixed_results_full() -> Result<()> {
+    let tempdir = TempDir::new()?;
+
+    // Create python files
+    let file_a_path = tempdir.path().join("normal.py");
+    let file_b_path = tempdir.path().join("panic.py");
+    fs::write(&file_a_path, b"import os")?;
+    fs::write(&file_b_path, b"print('hello, world!')")?;
+
+    insta::with_settings!({
+        filters => vec![
+            (tempdir_filter(&tempdir).as_str(), "[TMP]/"),
+            (r"\\", r"/"),
+        ]
+    }, {
+    assert_cmd_snapshot!(
+        Command::new(get_cargo_bin(BIN_NAME))
+            .args(["check", "--select", "RUF9", "--preview", "--output-format=full", "--no-cache"])
+            .args([file_a_path, file_b_path]),
+        @r"
+    success: false
+    exit_code: 2
+    ----- stdout -----
+    RUF900 Hey this is a stable test rule.
+    --> [TMP]/normal.py:1:1
+
+    RUF901 [*] Hey this is a stable test rule with a safe fix.
+    --> [TMP]/normal.py:1:1
+    1 + # fix from stable-test-rule-safe-fix
+    2 | import os
+
+    RUF902 Hey this is a stable test rule with an unsafe fix.
+    --> [TMP]/normal.py:1:1
+
+    RUF903 Hey this is a stable test rule with a display only fix.
+    --> [TMP]/normal.py:1:1
+
+    RUF911 Hey this is a preview test rule.
+    --> [TMP]/normal.py:1:1
+
+    RUF950 Hey this is a test rule that was redirected from another.
+    --> [TMP]/normal.py:1:1
+
+    panic: Fatal error while linting: This is a fake panic for testing.
+    --> [TMP]/panic.py:1:1
+    info: panicked at crates/ruff_linter/src/rules/ruff/rules/test_rules.rs:511:9:
+    This is a fake panic for testing.
+    run with `RUST_BACKTRACE=1` environment variable to display a backtrace
+
+
+    Found 7 errors.
+    [*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).
+
+    ----- stderr -----
+    error: Panic during linting indicates a bug in Ruff. If you could open an issue at:
+
+    https://github.com/astral-sh/ruff/issues/new?title=%5BLinter%20panic%5D
+
+    ...with the relevant file contents, the `pyproject.toml` settings, and the stack trace above, we'd be very appreciative!
+    ");
+    });
+    Ok(())
+}

crates/ruff_db/src/diagnostic/mod.rs

@@ -499,10 +499,12 @@ impl Diagnostic {
     /// Panics if either diagnostic has no primary span, or if its file is not a `SourceFile`.
     pub fn ruff_start_ordering(&self, other: &Self) -> std::cmp::Ordering {
         let a = (
+            self.severity().is_fatal(),
             self.expect_ruff_source_file(),
             self.range().map(|r| r.start()),
         );
         let b = (
+            other.severity().is_fatal(),
             other.expect_ruff_source_file(),
             other.range().map(|r| r.start()),
         );

crates/ruff_linter/src/checkers/ast/mod.rs

@@ -3266,6 +3266,11 @@ impl<'a> LintContext<'a> {
     pub(crate) const fn settings(&self) -> &LinterSettings {
         self.settings
     }
+
+    #[cfg(any(feature = "test-rules", test))]
+    pub(crate) const fn source_file(&self) -> &SourceFile {
+        &self.source_file
+    }
 }
 
 /// An abstraction for mutating a diagnostic.

crates/ruff_linter/src/codes.rs

@@ -1080,6 +1080,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Ruff, "950") => (RuleGroup::Stable, rules::ruff::rules::RedirectedToTestRule),
         #[cfg(any(feature = "test-rules", test))]
         (Ruff, "960") => (RuleGroup::Removed, rules::ruff::rules::RedirectedFromPrefixTestRule),
+        #[cfg(any(feature = "test-rules", test))]
+        (Ruff, "990") => (RuleGroup::Preview, rules::ruff::rules::PanicyTestRule),
 
 
         // flake8-django

crates/ruff_linter/src/linter.rs

@@ -319,6 +319,9 @@ pub fn check_path(
                         &context,
                     );
                 }
+                Rule::PanicyTestRule => {
+                    test_rules::PanicyTestRule::diagnostic(locator, comment_ranges, &context);
+                }
                 _ => unreachable!("All test rules must have an implementation"),
             }
         }

crates/ruff_linter/src/rules/ruff/rules/test_rules.rs

@@ -46,6 +46,7 @@ pub(crate) const TEST_RULES: &[Rule] = &[
     Rule::RedirectedFromTestRule,
     Rule::RedirectedToTestRule,
     Rule::RedirectedFromPrefixTestRule,
+    Rule::PanicyTestRule,
 ];
 
 pub(crate) trait TestRule {
@@ -477,3 +478,39 @@ impl TestRule for RedirectedFromPrefixTestRule {
         );
     }
 }
+
+/// # What it does
+/// Fake rule for testing panics.
+///
+/// # Why is this bad?
+/// Panics are bad.
+///
+/// # Example
+/// ```python
+/// foo
+/// ```
+///
+/// Use instead:
+/// ```python
+/// bar
+/// ```
+#[derive(ViolationMetadata)]
+pub(crate) struct PanicyTestRule;
+
+impl Violation for PanicyTestRule {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::None;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        "If you see this, maybe panic!".to_string()
+    }
+}
+
+impl TestRule for PanicyTestRule {
+    fn diagnostic(_locator: &Locator, _comment_ranges: &CommentRanges, context: &LintContext) {
+        assert!(
+            !context.source_file().name().ends_with("panic.py"),
+            "This is a fake panic for testing."
+        );
+    }
+}