Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions crates/ruff_annotate_snippets/src/renderer/display_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1183,6 +1183,21 @@ fn format_snippet<'m>(
let main_range = snippet.annotations.first().map(|x| x.range.start);
let origin = snippet.origin;
let need_empty_header = origin.is_some() || is_first;

let is_file_level = snippet.annotations.iter().any(|ann| ann.is_file_level);
if is_file_level {
assert!(
snippet.source.is_empty(),
"Non-empty file-level snippet that won't be rendered: {:?}",
snippet.source
);
let header = format_header(origin, main_range, &[], is_first);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

return DisplaySet {
display_lines: header.map_or_else(Vec::new, |header| vec![header]),
margin: Margin::new(0, 0, 0, 0, term_width, 0),
};
}

let mut body = format_body(
snippet,
need_empty_header,
Expand Down
7 changes: 7 additions & 0 deletions crates/ruff_annotate_snippets/src/snippet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,19 @@ pub struct Annotation<'a> {
pub(crate) range: Range<usize>,
pub(crate) label: Option<&'a str>,
pub(crate) level: Level,
pub(crate) is_file_level: bool,
}

impl<'a> Annotation<'a> {
pub fn label(mut self, label: &'a str) -> Self {
self.label = Some(label);
self
}

pub fn is_file_level(mut self, yes: bool) -> Self {
self.is_file_level = yes;
self
}
}

/// Types of annotations.
Expand Down Expand Up @@ -165,6 +171,7 @@ impl Level {
range: span,
label: None,
level: self,
is_file_level: false,
}
}
}
22 changes: 22 additions & 0 deletions crates/ruff_db/src/diagnostic/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,11 @@ pub struct Annotation {
is_primary: bool,
/// The diagnostic tags associated with this annotation.
tags: Vec<DiagnosticTag>,
/// Whether this annotation is a file-level or full-file annotation.
///
/// When set, rendering will only include the file's name and (optional) range. Everything else
/// is omitted, including any file snippet or message.
is_file_level: bool,
}

impl Annotation {
Expand All @@ -730,6 +735,7 @@ impl Annotation {
message: None,
is_primary: true,
tags: Vec::new(),
is_file_level: false,
}
}

Expand All @@ -746,6 +752,7 @@ impl Annotation {
message: None,
is_primary: false,
tags: Vec::new(),
is_file_level: false,
}
}

Expand Down Expand Up @@ -811,6 +818,21 @@ impl Annotation {
pub fn push_tag(&mut self, tag: DiagnosticTag) {
self.tags.push(tag);
}

/// Set whether or not this annotation is file-level.
///
/// File-level annotations are only rendered with their file name and range, if available. This
/// is intended for backwards compatibility with Ruff diagnostics, which historically used
/// `TextRange::default` to indicate a file-level diagnostic. In the new diagnostic model, a
/// [`Span`] with a range of `None` should be used instead, as mentioned in the `Span`
/// documentation.
///
/// TODO(brent) update this usage in Ruff and remove `is_file_level` entirely. See
/// <https://github.com/astral-sh/ruff/issues/19688>, especially my first comment, for more
/// details.
pub fn set_file_level(&mut self, yes: bool) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// 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 {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.is_file_level = yes;
}
}

/// Tags that can be associated with an annotation.
Expand Down
16 changes: 13 additions & 3 deletions crates/ruff_db/src/diagnostic/render.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ struct ResolvedAnnotation<'a> {
line_end: OneIndexed,
message: Option<&'a str>,
is_primary: bool,
is_file_level: bool,
}

impl<'a> ResolvedAnnotation<'a> {
Expand Down Expand Up @@ -432,6 +433,7 @@ impl<'a> ResolvedAnnotation<'a> {
line_end,
message: ann.get_message(),
is_primary: ann.is_primary,
is_file_level: ann.is_file_level,
})
}
}
Expand Down Expand Up @@ -653,6 +655,8 @@ struct RenderableAnnotation<'r> {
message: Option<&'r str>,
/// Whether this annotation is considered "primary" or not.
is_primary: bool,
/// Whether this annotation applies to an entire file, rather than a snippet within it.
is_file_level: bool,
}

impl<'r> RenderableAnnotation<'r> {
Expand All @@ -670,6 +674,7 @@ impl<'r> RenderableAnnotation<'r> {
range,
message: ann.message,
is_primary: ann.is_primary,
is_file_level: ann.is_file_level,
}
}

Expand All @@ -695,7 +700,7 @@ impl<'r> RenderableAnnotation<'r> {
if let Some(message) = self.message {
ann = ann.label(message);
}
ann
ann.is_file_level(self.is_file_level)
}
}

Expand Down Expand Up @@ -2551,7 +2556,12 @@ watermelon
/// of the corresponding line minus one. (The "minus one" is because
/// otherwise, the span will end where the next line begins, and this
/// confuses `ruff_annotate_snippets` as of 2025-03-13.)
fn span(&self, path: &str, line_offset_start: &str, line_offset_end: &str) -> Span {
pub(super) fn span(
&self,
path: &str,
line_offset_start: &str,
line_offset_end: &str,
) -> Span {
let span = self.path(path);

let file = span.expect_ty_file();
Expand All @@ -2574,7 +2584,7 @@ watermelon
}

/// Like `span`, but only attaches a file path.
fn path(&self, path: &str) -> Span {
pub(super) fn path(&self, path: &str) -> Span {
let file = system_path_to_file(&self.db, path).unwrap();
Span::from(file)
}
Expand Down
23 changes: 22 additions & 1 deletion crates/ruff_db/src/diagnostic/render/full.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
#[cfg(test)]
mod tests {
use ruff_diagnostics::Applicability;
use ruff_text_size::TextRange;

use crate::diagnostic::{
DiagnosticFormat, Severity,
Annotation, DiagnosticFormat, Severity,
render::tests::{TestEnvironment, create_diagnostics, create_syntax_error_diagnostics},
};

Expand Down Expand Up @@ -264,4 +265,24 @@ print()
|
");
}

/// For file-level diagnostics, we expect to see the header line with the diagnostic information
/// and the `-->` line with the file information but no lines of source code.
#[test]
fn file_level() {
let mut env = TestEnvironment::new();
env.add("example.py", "");
env.format(DiagnosticFormat::Full);

let mut diagnostic = env.err().build();
let span = env.path("example.py").with_range(TextRange::default());
let mut annotation = Annotation::primary(span);
annotation.set_file_level(true);
diagnostic.annotate(annotation);

insta::assert_snapshot!(env.render(&diagnostic), @r"
error[test-diagnostic]: main diagnostic message
--> example.py:1:1
");
}
}
10 changes: 9 additions & 1 deletion crates/ruff_linter/src/message/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,15 @@ where
);

let span = Span::from(file).with_range(range);
let annotation = Annotation::primary(span);
let mut annotation = Annotation::primary(span);
// The `0..0` range is used to highlight file-level diagnostics.
//
// TODO(brent) We should instead set this flag on annotations for individual lint rules that
// actually need it, but we need to be able to cache the new diagnostic model first. See
// https://github.com/astral-sh/ruff/issues/19688.
if range == TextRange::default() {
annotation.set_file_level(true);
}
diagnostic.annotate(annotation);

if let Some(suggestion) = suggestion {
Expand Down
Loading