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

amyreese · 2025-09-05T00:08:28Z

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.

github-actions · 2025-09-05T18:58:34Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

ntBre

Thanks!

I still think it could make sense to try to follow ty's diagnostic model a bit more closely, as I mentioned on #20252. Did you have any luck trying to add a test showing the message? That might be a nice way to play around with different message formats. Let me know if you want me to take a look, it sounded like it might be trickier than I expected in #20252 (comment).

I also don't think we can land this without auditing our usage of Diagnostic::expect_range more fully. I took another look at its references just now, and I think this call chain would cause a panic in diagnostic rendering:

ruff/crates/ruff_linter/src/message/github.rs

Lines 22 to 24 in 16d674e

    
           let source_location = diagnostic.expect_ruff_start_location(); 
        
           let filename = diagnostic.expect_ruff_filename(); 
        
           let location = if context.is_notebook(&filename) {

ruff/crates/ruff_db/src/diagnostic/mod.rs

Line 459 in 16d674e

pub fn expect_ruff_start_location(&self) -> LineColumn {

If we try to render a new panic diagnostic with the GitHub output format, it will call Diagnostic::expect_ruff_start_location, which calls Diagnostic::expect_range.

Feel free to spin this off into a separate PR, but I'd feel better introducing diagnostics without ranges if we can remove Diagnostic::expect_range entirely first.

I tested this out locally to make sure I wasn't off base:

 > ./target/debug/ruff check --no-cache tmp/foo.py
panic: Panicked while linting tmp/foo.py:
panicked at crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs:430:17:
brent's panic
run with `RUST_BACKTRACE=1` environment variable to display a backtrace

--> tmp/foo.py:1:1
 |
 |

Found 1 error.
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!

> ./target/debug/ruff check --no-cache tmp/foo.py --output-format github

error: Ruff crashed. If you could open an issue at:

    https://github.com/astral-sh/ruff/issues/new?title=%5BPanic%5D

...quoting the executed command, along with the relevant file contents and `pyproject.toml` settings, we'd be very appreciative!


thread 'main' panicked at crates/ruff_db/src/diagnostic/mod.rs:499:22:
Expected a range for the primary span
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

amyreese · 2025-09-06T00:18:15Z

Check-pointed where I got to with trying to get a test rule — will look at teasing out the expect_range parts on Monday.

- 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` - Updates the diagnostic sorting to sort panics to the bottom - Adds a new test rule and test case to exercise panics in lint rules This depends on the sorting changes since it creates diagnostics with no range specified.

MichaReiser

This looks great to me but I'll defer to @ntBre to do the final review as he's more up to date with what you discussed

MichaReiser · 2025-09-15T13:48:38Z

crates/ruff/tests/lint.rs

+    [TMP]/panic.py: panic: Panicked while linting [TMP]/panic.py:
+    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
+


We try to avoid multiline content in diagnostic messages because it can break rendering for some output formats (as seen here).

You can see how we accomplish this in ty here:

ruff/crates/ty_project/src/lib.rs

Lines 669 to 746 in b3cc733

use std::fmt::Write;

let mut message = String::new();

message.push_str("Panicked");

if let Some(location) = error.location {

let _ = write!(&mut message, " at {location}");

}

let _ = write!(

&mut message,

" when checking `{file}`",

file = file.path(db)

);

if let Some(payload) = error.payload.as_str() {

let _ = write!(&mut message, ": `{payload}`");

}

let mut diagnostic = Diagnostic::new(DiagnosticId::Panic, Severity::Fatal, message);

diagnostic.sub(SubDiagnostic::new(

SubDiagnosticSeverity::Info,

"This indicates a bug in ty.",

));

let report_message = "If you could open an issue at https://github.com/astral-sh/ty/issues/new?title=%5Bpanic%5D, we'd be very appreciative!";

diagnostic.sub(SubDiagnostic::new(

SubDiagnosticSeverity::Info,

report_message,

));

diagnostic.sub(SubDiagnostic::new(

SubDiagnosticSeverity::Info,

format!(

"Platform: {os} {arch}",

os = std::env::consts::OS,

arch = std::env::consts::ARCH

),

));

if let Some(version) = ruff_db::program_version() {

diagnostic.sub(SubDiagnostic::new(

SubDiagnosticSeverity::Info,

format!("Version: {version}"),

));

}

diagnostic.sub(SubDiagnostic::new(

SubDiagnosticSeverity::Info,

format!(

"Args: {args:?}",

args = std::env::args().collect::<Vec<_>>()

),

));

if let Some(backtrace) = error.backtrace {

match backtrace.status() {

BacktraceStatus::Disabled => {

diagnostic.sub(SubDiagnostic::new(

SubDiagnosticSeverity::Info,

"run with `RUST_BACKTRACE=1` environment variable to show the full backtrace information",

));

}

BacktraceStatus::Captured => {

diagnostic.sub(SubDiagnostic::new(

SubDiagnosticSeverity::Info,

format!("Backtrace:\n{backtrace}"),

));

}

_ => {}

}

}

if let Some(backtrace) = error.salsa_backtrace {

salsa::attach(db, || {

diagnostic.sub(SubDiagnostic::new(

SubDiagnosticSeverity::Info,

backtrace.to_string(),

));

});

}

The downside of ty's approach is that we only show the "Please open an issue" part if a user uses output-format="full" (or any other format that renders sub diagnostics).

I don't feel strongly about this but I thought it was worth mentioning in case this wasn't an intentional design decision.

The PR looks great to me overall too! I think this is the main thing I'd find interesting to try out (plus the full severity sort). It looks like we can also clean up the message a bit since the diagnostic infrastructure already adds the filename automatically. Something like:

[TMP]/panic.py: panic: Panicked while linting

instead of the current

[TMP]/panic.py: panic: Panicked while linting [TMP]/panic.py:

panic: Panicked also seems a bit redundant, but I think panic: while linting would also be awkward.

Moved the full error/backtrace into a subdiagnostic, and made the main diagnostic message a single line like fatal error while linting: <panic summary>. I've left the "please open an issue" as a separate log to stderr, so that it would appear even in output formats like concise, but presumably shouldn't affect ability to process output on stdout.

The alternate is moving that message into a second subdiagnostic, but then it would be repeated for every single panic rather than just once at the end, and would also end up being hidden in concise output (or we make the main message more verbose? 😐).

As-is, in concise and full output format, it now looks like:

amethyst@lunatone ~/workspace/ruff amy/panic-diagnostics+ » cargo run -p ruff -- check crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC25*.py --no-cache --preview --select ASYNC --output-format=concise Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.10s Running `target/debug/ruff check crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC250.py crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC251.py --no-cache --preview --select ASYNC --output-format=concise` crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC251.py:6:5: ASYNC251 Async functions should not call `time.sleep` crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC250.py: panic: Fatal error while linting: fake panic Found 2 errors. 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! > [2] amethyst@lunatone ~/workspace/ruff amy/panic-diagnostics+ » cargo run -p ruff -- check crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC25*.py --no-cache --preview --select ASYNC --output-format=full Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.10s Running `target/debug/ruff check crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC250.py crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC251.py --no-cache --preview --select ASYNC --output-format=full` ASYNC251 Async functions should not call `time.sleep` --> crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC251.py:6:5 | 5 | async def func(): 6 | time.sleep(1) # ASYNC251 | ^^^^^^^^^^ | panic: Fatal error while linting: fake panic --> crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC250.py:1:1 | | info: panicked at crates/ruff_linter/src/rules/flake8_async/rules/blocking_input.rs:50:13: fake panic run with `RUST_BACKTRACE=1` environment variable to display a backtrace Found 2 errors. 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! > [2]

MichaReiser · 2025-09-15T13:49:29Z

crates/ruff/tests/lint.rs

+    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!
+    ");


I think I sort of prefer the old rendering where this message was next to the panic-diagnostic. As a user, it's now less clear to me what I have to report (imagine a few 100 diagnostics).

Part of the change was making sure that panics always sort to the bottom of the diagnostics, so other than being on stderr, to an interactive user it should always render "next to" the actual panic messages, which should always render after all of the non-fatal diagnostics.

Oh I see, it might be worth adding an inline comment explaining that this is the intended behavior as it wasn't obvious to me.

MichaReiser · 2025-09-15T13:51:04Z

crates/ruff_db/src/diagnostic/mod.rs

    /// 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(),


We could consider comparing by severity here (in reverse order)

As a user I still prefer having them sorted by file/lineno, because that lets me visually go through the list without jumping back and forth between different files or different parts of a single file. Eg, all of the lints for a single function or class would always be together in the final output.

But because panics trigger Ruff to discard all other diagnostics for a file, there's currently nothing for them to be grouped with, so I think it's reasonable to sort those at the end so that it's more obvious to the user.

As a user I still prefer having them sorted by file/lineno, because that lets me visually go through the list without jumping back and forth between different files or different parts of a single file. Eg, all of the lints for a single function or class would always be together in the final output.

That makes sense and is consistent with what we have in ty:

ruff/crates/ruff_db/src/diagnostic/mod.rs

Lines 534 to 565 in b6a740f

// We sort diagnostics in a way that keeps them in source order

// and grouped by file. After that, we fall back to severity

// (with fatal messages sorting before info messages) and then

// finally the diagnostic ID.

fn cmp(&self, other: &Self) -> std::cmp::Ordering {

if let (Some(span1), Some(span2)) = (

self.diagnostic.primary_span(),

other.diagnostic.primary_span(),

) {

let order = span1.file().path(&self.db).cmp(span2.file().path(&self.db));

if order.is_ne() {

return order;

}

if let (Some(range1), Some(range2)) = (span1.range(), span2.range()) {

let order = range1.start().cmp(&range2.start());

if order.is_ne() {

return order;

}

}

}

// Reverse so that, e.g., Fatal sorts before Info.

let order = self

.diagnostic

.severity()

.cmp(&other.diagnostic.severity())

.reverse();

if order.is_ne() {

return order;

}

self.diagnostic.id().cmp(&other.diagnostic.id())

}

The only difference is that ty only compares by severity if the file doesn't have a range and fatal errors come first rather than last and ty prints an extra warning as part of the summary if there were any panics (to avoid users missing them).

I don't have a strong preference towards either approach and using is_fatal here seems fine after reading your reasoning.

MichaReiser · 2025-09-15T13:52:24Z

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

+/// bar
+/// ```
+#[derive(ViolationMetadata)]
+pub(crate) struct PanicyTestRule;


Nice way of testing this (we don't have this in ty)

MichaReiser

This looks good to me

ntBre

Looks good to me too! Just one small suggestion about the diagnostic

ntBre · 2025-09-16T14:35:17Z

crates/ruff/src/commands/check.rs

-
-            Ok(Diagnostics::default())
+            let span = Span::from(SourceFileBuilder::new(path.to_string_lossy(), "").finish());
+            diagnostic.annotate(Annotation::primary(span));


I think we might want to call set_file_level(true) on this Annotation to avoid the empty snippet (| lines) in the diagnostic from your example:

--> crates/ruff_linter/resources/test/fixtures/flake8_async/ASYNC250.py:1:1 | | info: panicked at crates/ruff_linter/src/rules/flake8_async/rules/blocking_input.rs:50:13:

If you're interested in removing the need for set_file_level, this is listed as a potential improvement in your welcome document:

This task is related to the one above. Ruff currently uses an empty text range at position 0 (start of the file) for diagnostics that apply to the entire file. We need the range for suppression comments to work, but we don’t want to render a code frame for those. Brent added a flag for annotations (the diagnostic system renders code snippets for each annotation) so that they can be marked as file-level, in which case the code snippets aren’t rendered....

amyreese force-pushed the amy/diagnostics-sorting branch from 27ab7ae to 568f553 Compare September 5, 2025 18:41

amyreese requested review from MichaReiser, carljm, dcreager and sharkdp as code owners September 5, 2025 18:41

amyreese force-pushed the amy/panic-diagnostics branch from 271d6af to abf6b7c Compare September 5, 2025 18:47

ntBre reviewed Sep 5, 2025

View reviewed changes

amyreese force-pushed the amy/diagnostics-sorting branch from 568f553 to 55d36c3 Compare September 5, 2025 20:31

carljm removed their request for review September 5, 2025 21:31

Base automatically changed from amy/diagnostics-sorting to main September 5, 2025 22:23

amyreese force-pushed the amy/panic-diagnostics branch from abf6b7c to d40e9c6 Compare September 6, 2025 00:15

amyreese force-pushed the amy/panic-diagnostics branch from d40e9c6 to ef147de Compare September 11, 2025 14:58

amyreese force-pushed the amy/panic-diagnostics branch from ef147de to edf05f2 Compare September 12, 2025 22:32

amyreese changed the title ~~Better handling for panics during linting~~ [ruff] Better handling for panics during linting Sep 12, 2025

amyreese added the diagnostics Related to reporting of diagnostics. label Sep 12, 2025

amyreese force-pushed the amy/panic-diagnostics branch 2 times, most recently from cc59819 to 870acc7 Compare September 12, 2025 23:36

MichaReiser reviewed Sep 15, 2025

View reviewed changes

amyreese force-pushed the amy/panic-diagnostics branch from 870acc7 to df9f66e Compare September 15, 2025 23:20

MichaReiser approved these changes Sep 16, 2025

View reviewed changes

ntBre approved these changes Sep 16, 2025

View reviewed changes

amyreese force-pushed the amy/panic-diagnostics branch 2 times, most recently from e704155 to e7b39c3 Compare September 16, 2025 17:46

amyreese added the cli Related to the command-line interface label Sep 16, 2025

amyreese changed the title ~~[ruff] Better handling for panics during linting~~ [ruff] Treat panics as fatal diagnostics, sort panics last Sep 16, 2025

amyreese merged commit 8a027b0 into main Sep 16, 2025
36 checks passed

amyreese deleted the amy/panic-diagnostics branch September 16, 2025 18:33

BrewTestBot mentioned this pull request Sep 18, 2025

ruff 0.13.1 Homebrew/homebrew-core#244826

Merged

	let source_location = diagnostic.expect_ruff_start_location();
	let filename = diagnostic.expect_ruff_filename();
	let location = if context.is_notebook(&filename) {

	use std::fmt::Write;
	let mut message = String::new();
	message.push_str("Panicked");

	if let Some(location) = error.location {
	let _ = write!(&mut message, " at {location}");
	}

	let _ = write!(
	&mut message,
	" when checking `{file}`",
	file = file.path(db)
	);

	if let Some(payload) = error.payload.as_str() {
	let _ = write!(&mut message, ": `{payload}`");
	}

	let mut diagnostic = Diagnostic::new(DiagnosticId::Panic, Severity::Fatal, message);
	diagnostic.sub(SubDiagnostic::new(
	SubDiagnosticSeverity::Info,
	"This indicates a bug in ty.",
	));

	let report_message = "If you could open an issue at https://github.com/astral-sh/ty/issues/new?title=%5Bpanic%5D, we'd be very appreciative!";
	diagnostic.sub(SubDiagnostic::new(
	SubDiagnosticSeverity::Info,
	report_message,
	));
	diagnostic.sub(SubDiagnostic::new(
	SubDiagnosticSeverity::Info,
	format!(
	"Platform: {os} {arch}",
	os = std::env::consts::OS,
	arch = std::env::consts::ARCH
	),
	));
	if let Some(version) = ruff_db::program_version() {
	diagnostic.sub(SubDiagnostic::new(
	SubDiagnosticSeverity::Info,
	format!("Version: {version}"),
	));
	}

	diagnostic.sub(SubDiagnostic::new(
	SubDiagnosticSeverity::Info,
	format!(
	"Args: {args:?}",
	args = std::env::args().collect::<Vec<_>>()
	),
	));

	if let Some(backtrace) = error.backtrace {
	match backtrace.status() {
	BacktraceStatus::Disabled => {
	diagnostic.sub(SubDiagnostic::new(
	SubDiagnosticSeverity::Info,
	"run with `RUST_BACKTRACE=1` environment variable to show the full backtrace information",
	));
	}
	BacktraceStatus::Captured => {
	diagnostic.sub(SubDiagnostic::new(
	SubDiagnosticSeverity::Info,
	format!("Backtrace:\n{backtrace}"),
	));
	}
	_ => {}
	}
	}

	if let Some(backtrace) = error.salsa_backtrace {
	salsa::attach(db, \|\| {
	diagnostic.sub(SubDiagnostic::new(
	SubDiagnosticSeverity::Info,
	backtrace.to_string(),
	));
	});
	}

	// We sort diagnostics in a way that keeps them in source order
	// and grouped by file. After that, we fall back to severity
	// (with fatal messages sorting before info messages) and then
	// finally the diagnostic ID.
	fn cmp(&self, other: &Self) -> std::cmp::Ordering {
	if let (Some(span1), Some(span2)) = (
	self.diagnostic.primary_span(),
	other.diagnostic.primary_span(),
	) {
	let order = span1.file().path(&self.db).cmp(span2.file().path(&self.db));
	if order.is_ne() {
	return order;
	}

	if let (Some(range1), Some(range2)) = (span1.range(), span2.range()) {
	let order = range1.start().cmp(&range2.start());
	if order.is_ne() {
	return order;
	}
	}
	}
	// Reverse so that, e.g., Fatal sorts before Info.
	let order = self
	.diagnostic
	.severity()
	.cmp(&other.diagnostic.severity())
	.reverse();
	if order.is_ne() {
	return order;
	}
	self.diagnostic.id().cmp(&other.diagnostic.id())
	}

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

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

Uh oh!

Conversation

amyreese commented Sep 5, 2025

Uh oh!

github-actions bot commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

amyreese commented Sep 6, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Sep 5, 2025 •

edited

Loading

`ruff-ecosystem` results