Hide empty snippets for full-file diagnostics #19653

ntBre · 2025-07-30T23:24:47Z

Summary

This is the other commit I wanted to spin off from #19415, currently stacked on #19644.

This PR suppresses blank snippets for empty ranges at the very beginning of a file, and for empty ranges in non-existent files. Ruff includes empty ranges for IO errors, for example.

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

Lines 100 to 110 in f4e93b6

    
           // The `0..0` range is used to highlight file-level diagnostics. 
        
           if message.expect_range() != TextRange::default() { 
        
               writeln!( 
        
                   writer, 
        
                   "{}", 
        
                   MessageCodeFrame { 
        
                       message, 
        
                       notebook_index 
        
                   } 
        
               )?; 
        
           }

The diagnostics now look like this (new snapshot test):

error[test-diagnostic]: main diagnostic message
--> example.py:1:1

Instead of ¹

error[test-diagnostic]: main diagnostic message
--> example.py:1:1
 |
 |

Test Plan

A new ruff_db test showing the expected output format

This doesn't correspond precisely to the example in the PR because of some details of the diagnostic builder helper methods in ruff_db, but you can see another example in the current version of the summary in Move full diagnostic rendering to ruff_db #19415. ↩

codspeed-hq · 2025-07-30T23:35:40Z

CodSpeed Instrumentation Performance Report

Merging #19653 will not alter performance

_{Comparing brent/full-file-diagnostics (2cfad84) with main (2db4e5d)}

Summary

✅ 42 untouched benchmarks

github-actions · 2025-07-30T23:37:10Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser

Thank you. The approach looks good to me but I'm unsure about the ruff integration

MichaReiser · 2025-08-01T08:19:37Z

crates/ruff_annotate_snippets/src/renderer/display_list.rs

+
+    let is_file_level = snippet.annotations.iter().any(|ann| ann.is_file_level);
+    if is_file_level {
+        let header = format_header(origin, main_range, &[], is_first);


Nit: Would it make sense to add an assertion that the snippet has no body (e.g no associated message?)

Sure, some kind of assertion makes sense to me. I think we can assert that the snippet source is empty, I'll just have to rework the test to avoid using the builder, which expects a non-empty snippet. It looks like Ruff's real diagnostics that will go through this path should already have empty snippets. We could also check that the annotation labels are empty (the text after ^^^), but I think those are always empty for us right now.

I think RenderableSnippet::new might try to expand the snippet to the usual number of context lines, unless we also check is_file_level there. We might have to remove or update the assertion when running this in #19415, but I'll add it for now. It'll be a good reminder to revisit it.

Oh, maybe that's in line with your suggestion about applying this per-rule. I guess we shouldn't render one of these except in cases where the snippet isn't available, like an io-error!

MichaReiser · 2025-08-01T08:20:15Z

crates/ruff_db/src/diagnostic/mod.rs

+    /// When set, rendering will not include a file snippet but will still have the file's name and
+    /// (optional) range.


We should mention that this also omits the annotation's message. Or we could phrase it differently and say that everything other than the file name and range are omitted.

MichaReiser · 2025-08-01T08:23:14Z

crates/ruff_db/src/diagnostic/mod.rs

        self.tags.push(tag);
    }
+
+    pub fn set_file_level(&mut self, yes: bool) {


Can we document the meaning of a file level annotation here too

Good idea, I added a note here about Span too, referring to this section of the Span docs:

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

Lines 1080 to 1084 in e7e7b7b

/// It consists of a `File` and an optional range into that file. When the

/// range isn't present, it semantically implies that the diagnostic refers to

/// the entire file. For example, when the file should be executable but isn't.

#[derive(Debug, Clone, PartialEq, Eq, Hash, get_size2::GetSize)]

pub struct Span {

Should this get a TODO that it ought to be removed in favor of Ruff diagnostics being updated to use a None span?

Ah that's a good point. I added an explicit TODO in the code here and also wrote some notes from looking into the options today: #19688 (comment). In short, I thought we would handle this with a None range on the span, but I think you're right that we should omit the span entirely instead.

It might be a bit tangential to the main issue in #19688, but at least it's recorded somewhere, and I can spin it off later if needed.

Yeah this at least gives us a possible path toward eventually merging back with upstream.

crates/ruff_linter/src/message/mod.rs

MichaReiser · 2025-08-01T08:28:05Z

crates/ruff_linter/src/message/mod.rs

+    let mut annotation = Annotation::primary(span);
+    // The `0..0` range is used to highlight file-level diagnostics.
+    if range == TextRange::default() {
+        annotation.set_file_level(true);
+    }


Would it be much more work to instead change the rules that add file level diagnostics rather than doing this "global" override?

(the rule would have to mutate the Diagnostic after reporting it with report_diagnostic).

The reason I'd prefer this is that it makes it more apparent where we rely on this behavior and it doesn't prevent other rules to use an empty range (e.g. what if we had a rule that enforces module level docstrings. That rule would probably ask you to insert the docstring at 0:0.

Ah that's a great idea! We actually do have rules like you're describing. I think at least one import rule suggests adding an import at the top of the file (probably a __future__ import rule). It might be easier to handle that in #19415 as I'm going through the snapshots, but I can revert this for now.

You're right about the syntax errors too, I don't think we have any file-level ones. That was just for consistency with the current version.

On second thought, we might want to hold off on this until we can cache the Annotations. I don't think set_file_level will be cached currently.

On second thought, we might want to hold off on this until we can cache the Annotations. I don't think set_file_level will be cached currently.

I'm not sure I understand what you mean by caching.

(also discussed on Discord, just recording here for future reference)

For caching, I just mean in ruff's diagnostic cache. I think we would lose the is_file_level field on the annotation when converting to a CacheMessage

I'll drop the syntax error check because that shouldn't affect any real diagnostics and open an issue to follow up on this after a caching PR.

Let's maybe add a TODO here too

MichaReiser · 2025-08-01T15:30:06Z

crates/ruff_linter/src/message/mod.rs

+    let mut annotation = Annotation::primary(span);
+    // The `0..0` range is used to highlight file-level diagnostics.
+    if range == TextRange::default() {
+        annotation.set_file_level(true);
+    }


Let's maybe add a TODO here too

BurntSushi · 2025-08-04T15:26:54Z

crates/ruff_db/src/diagnostic/mod.rs

        self.tags.push(tag);
    }
+
+    pub fn set_file_level(&mut self, yes: bool) {


Should this get a TODO that it ought to be removed in favor of Ruff diagnostics being updated to use a None span?

Summary -- Fixes a snapshot test failure I saw in #19653 locally and in Windows CI by padding the hex ID to 16 digits to match the regex in `filter_result_id`. https://github.com/astral-sh/ruff/blob/78e5fe0a51fcbb1456d54c65b4632f60c268e389/crates/ty_server/tests/e2e/pull_diagnostics.rs#L380-L384 Test Plan -- I applied this to the branch from #19653 locally and saw that the tests now pass. I couldn't reproduce this failure directly on `main` or this branch, though.

Summary -- This isn't really polished for review yet, but this is the other commit I wanted to spin off from #19415. This suppresses empty snippets for empty ranges at the very beginning of a file, and for empty ranges in non-existent files. Ruff includes empty ranges for IO errors, for example. Test Plan -- TODO add a `ruff_db` test with an example

We do need a single character because the diagnostic builder does some subtraction on the range, but conceptually this can be a completely empty or non-existent file, as can be the case in Ruff.

also construct the test span manually to avoid some non-empty snippet assumptions in `TestEnvironment::span`

we don't currently have any file-level syntax errors, so this wasn't having any effect

ntBre added internal An internal refactor or improvement diagnostics Related to reporting of diagnostics. labels Jul 30, 2025

ntBre force-pushed the brent/hide-severity branch from 1e1bd62 to 1cdb26c Compare July 31, 2025 16:23

ntBre force-pushed the brent/full-file-diagnostics branch from 8a027bb to f4e93b6 Compare July 31, 2025 20:34

ntBre changed the title ~~[WIP] Hide empty snippets for full-file diagnostics~~ Hide empty snippets for full-file diagnostics Jul 31, 2025

ntBre marked this pull request as ready for review July 31, 2025 20:46

ntBre requested review from AlexWaygood, BurntSushi, MichaReiser, carljm, dcreager and sharkdp as code owners July 31, 2025 20:46

ntBre removed request for AlexWaygood, carljm, dcreager and sharkdp July 31, 2025 20:46

ntBre mentioned this pull request Jul 31, 2025

Move full diagnostic rendering to ruff_db #19415

Merged

MichaReiser reviewed Aug 1, 2025

View reviewed changes

This was referenced Aug 1, 2025

Set Annotation::is_file_level in affected lint rules #19688

Open

Use new Diagnostic functionality for Ruff rules #19690

Closed

MichaReiser approved these changes Aug 1, 2025

View reviewed changes

BurntSushi approved these changes Aug 4, 2025

View reviewed changes

Base automatically changed from brent/hide-severity to main August 5, 2025 13:56

ntBre force-pushed the brent/full-file-diagnostics branch from 01c8ac8 to 4603fc8 Compare August 5, 2025 14:24

ntBre mentioned this pull request Aug 5, 2025

Use fixed hash width for ty_server diagnostics #19766

Merged

ntBre added 8 commits August 5, 2025 10:55

remove irrelevant code from test case

864c056

We do need a single character because the diagnostic builder does some subtraction on the range, but conceptually this can be a completely empty or non-existent file, as can be the case in Ruff.

clarify comment

c35d34f

assert that the snippet is empty before rendering only the header

c64a7e5

also construct the test span manually to avoid some non-empty snippet assumptions in `TestEnvironment::span`

document Annotation::set_file_level

739b35b

revert file-level settings for syntax errors

1e6c99f

we don't currently have any file-level syntax errors, so this wasn't having any effect

add todo

b59ddc7

add explicit todo around removing is_file_level

f3e0e2a

hyperlink

2cfad84

ntBre force-pushed the brent/full-file-diagnostics branch from 4603fc8 to 2cfad84 Compare August 5, 2025 14:56

ntBre merged commit b324ae1 into main Aug 5, 2025
35 checks passed

ntBre deleted the brent/full-file-diagnostics branch August 5, 2025 15:20

	// The `0..0` range is used to highlight file-level diagnostics.
	if message.expect_range() != TextRange::default() {
	writeln!(
	writer,
	"{}",
	MessageCodeFrame {
	message,
	notebook_index
	}
	)?;
	}

		/// When set, rendering will not include a file snippet but will still have the file's name and
		/// (optional) range.

	/// It consists of a `File` and an optional range into that file. When the
	/// range isn't present, it semantically implies that the diagnostic refers to
	/// the entire file. For example, when the file should be executable but isn't.
	#[derive(Debug, Clone, PartialEq, Eq, Hash, get_size2::GetSize)]
	pub struct Span {

Hide empty snippets for full-file diagnostics #19653

Hide empty snippets for full-file diagnostics #19653

Uh oh!

Conversation

ntBre commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Footnotes

Uh oh!

codspeed-hq bot commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #19653 will not alter performance

Summary

Uh oh!

github-actions bot commented Jul 30, 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!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ntBre commented Jul 30, 2025 •

edited

Loading

codspeed-hq bot commented Jul 30, 2025 •

edited

Loading

github-actions bot commented Jul 30, 2025 •

edited

Loading

`ruff-ecosystem` results