From 6e34f74c164a646f07f3fe9bed9492c3557a95d7 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Tue, 11 Feb 2025 14:55:12 -0500 Subject: [PATCH] add diagnostic `Span` (couples `File` and `TextRange`) (#16101) This essentially makes it impossible to construct a `Diagnostic` that has a `TextRange` but no `File`. This is meant to be a precursor to multi-span support. (Note that I consider this more of a prototyping-change and not necessarily what this is going to look like longer term.) Reviewers can probably review this PR as one big diff instead of commit-by-commit. --- crates/red_knot_project/src/lib.rs | 19 +-- .../red_knot_project/src/metadata/options.rs | 39 +++--- .../src/types/diagnostic.rs | 10 +- .../src/server/api/requests/diagnostic.rs | 10 +- crates/red_knot_test/src/diagnostic.rs | 13 +- crates/red_knot_test/src/matcher.rs | 13 +- crates/ruff_benchmark/benches/red_knot.rs | 10 +- crates/ruff_db/src/diagnostic.rs | 115 ++++++++++-------- 8 files changed, 115 insertions(+), 114 deletions(-) diff --git a/crates/red_knot_project/src/lib.rs b/crates/red_knot_project/src/lib.rs index 3d8ee4ae4ce17e..2126f36a841e97 100644 --- a/crates/red_knot_project/src/lib.rs +++ b/crates/red_knot_project/src/lib.rs @@ -8,14 +8,13 @@ pub use metadata::{ProjectDiscoveryError, ProjectMetadata}; use red_knot_python_semantic::lint::{LintRegistry, LintRegistryBuilder, RuleSelection}; use red_knot_python_semantic::register_lints; use red_knot_python_semantic::types::check_types; -use ruff_db::diagnostic::{Diagnostic, DiagnosticId, ParseDiagnostic, Severity}; +use ruff_db::diagnostic::{Diagnostic, DiagnosticId, ParseDiagnostic, Severity, Span}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::parsed::parsed_module; use ruff_db::source::{source_text, SourceTextError}; use ruff_db::system::walk_directory::WalkState; use ruff_db::system::{FileType, SystemPath}; use ruff_python_ast::PySourceType; -use ruff_text_size::TextRange; use rustc_hash::{FxBuildHasher, FxHashSet}; use salsa::Durability; use salsa::Setter; @@ -345,7 +344,13 @@ fn check_file_impl(db: &dyn Db, file: File) -> Vec> { boxed })); - diagnostics.sort_unstable_by_key(|diagnostic| diagnostic.range().unwrap_or_default().start()); + diagnostics.sort_unstable_by_key(|diagnostic| { + diagnostic + .span() + .and_then(|span| span.range()) + .unwrap_or_default() + .start() + }); diagnostics } @@ -458,12 +463,8 @@ impl Diagnostic for IOErrorDiagnostic { self.error.to_string().into() } - fn file(&self) -> Option { - Some(self.file) - } - - fn range(&self) -> Option { - None + fn span(&self) -> Option { + Some(Span::from(self.file)) } fn severity(&self) -> Severity { diff --git a/crates/red_knot_project/src/metadata/options.rs b/crates/red_knot_project/src/metadata/options.rs index 401dcf1233345c..09a167ca98ae0c 100644 --- a/crates/red_knot_project/src/metadata/options.rs +++ b/crates/red_knot_project/src/metadata/options.rs @@ -4,11 +4,10 @@ use red_knot_python_semantic::lint::{GetLintError, Level, LintSource, RuleSelect use red_knot_python_semantic::{ ProgramSettings, PythonPlatform, PythonVersion, SearchPathSettings, SitePackages, }; -use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity}; -use ruff_db::files::{system_path_to_file, File}; +use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity, Span}; +use ruff_db::files::system_path_to_file; use ruff_db::system::{System, SystemPath}; use ruff_macros::Combine; -use ruff_text_size::TextRange; use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; use std::borrow::Cow; @@ -189,7 +188,14 @@ impl Options { ), }; - diagnostics.push(diagnostic.with_file(file).with_range(rule_name.range())); + let span = file.map(Span::from).map(|span| { + if let Some(range) = rule_name.range() { + span.with_range(range) + } else { + span + } + }); + diagnostics.push(diagnostic.with_span(span)); } } } @@ -348,8 +354,7 @@ pub struct OptionDiagnostic { id: DiagnosticId, message: String, severity: Severity, - file: Option, - range: Option, + span: Option, } impl OptionDiagnostic { @@ -358,21 +363,13 @@ impl OptionDiagnostic { id, message, severity, - file: None, - range: None, + span: None, } } #[must_use] - fn with_file(mut self, file: Option) -> Self { - self.file = file; - self - } - - #[must_use] - fn with_range(mut self, range: Option) -> Self { - self.range = range; - self + fn with_span(self, span: Option) -> Self { + OptionDiagnostic { span, ..self } } } @@ -385,12 +382,8 @@ impl Diagnostic for OptionDiagnostic { Cow::Borrowed(&self.message) } - fn file(&self) -> Option { - self.file - } - - fn range(&self) -> Option { - self.range + fn span(&self) -> Option { + self.span.clone() } fn severity(&self) -> Severity { diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs index 99d07dd9f973a9..2f08b84f88bc09 100644 --- a/crates/red_knot_python_semantic/src/types/diagnostic.rs +++ b/crates/red_knot_python_semantic/src/types/diagnostic.rs @@ -8,7 +8,7 @@ use crate::types::string_annotation::{ }; use crate::types::{ClassLiteralType, KnownInstanceType, Type}; use crate::{declare_lint, Db}; -use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity}; +use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity, Span}; use ruff_db::files::File; use ruff_python_ast::{self as ast, AnyNodeRef}; use ruff_text_size::{Ranged, TextRange}; @@ -802,12 +802,8 @@ impl Diagnostic for TypeCheckDiagnostic { TypeCheckDiagnostic::message(self).into() } - fn file(&self) -> Option { - Some(TypeCheckDiagnostic::file(self)) - } - - fn range(&self) -> Option { - Some(Ranged::range(self)) + fn span(&self) -> Option { + Some(Span::from(self.file).with_range(self.range)) } fn severity(&self) -> Severity { diff --git a/crates/red_knot_server/src/server/api/requests/diagnostic.rs b/crates/red_knot_server/src/server/api/requests/diagnostic.rs index 968c05abe65e03..ba788817ec4c03 100644 --- a/crates/red_knot_server/src/server/api/requests/diagnostic.rs +++ b/crates/red_knot_server/src/server/api/requests/diagnostic.rs @@ -75,11 +75,13 @@ fn to_lsp_diagnostic( diagnostic: &dyn ruff_db::diagnostic::Diagnostic, encoding: crate::PositionEncoding, ) -> Diagnostic { - let range = if let (Some(file), Some(range)) = (diagnostic.file(), diagnostic.range()) { - let index = line_index(db.upcast(), file); - let source = source_text(db.upcast(), file); + let range = if let Some(span) = diagnostic.span() { + let index = line_index(db.upcast(), span.file()); + let source = source_text(db.upcast(), span.file()); - range.to_range(&source, &index, encoding) + span.range() + .map(|range| range.to_range(&source, &index, encoding)) + .unwrap_or_default() } else { Range::default() }; diff --git a/crates/red_knot_test/src/diagnostic.rs b/crates/red_knot_test/src/diagnostic.rs index bb5a099b50ef11..56ee51223cc8ba 100644 --- a/crates/red_knot_test/src/diagnostic.rs +++ b/crates/red_knot_test/src/diagnostic.rs @@ -26,7 +26,8 @@ where .into_iter() .map(|diagnostic| DiagnosticWithLine { line_number: diagnostic - .range() + .span() + .and_then(|span| span.range()) .map_or(OneIndexed::from_zero_indexed(0), |range| { line_index.line_index(range.start()) }), @@ -144,7 +145,7 @@ struct DiagnosticWithLine { mod tests { use crate::db::Db; use crate::diagnostic::Diagnostic; - use ruff_db::diagnostic::{DiagnosticId, LintName, Severity}; + use ruff_db::diagnostic::{DiagnosticId, LintName, Severity, Span}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::source::line_index; use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; @@ -198,12 +199,8 @@ mod tests { "dummy".into() } - fn file(&self) -> Option { - Some(self.file) - } - - fn range(&self) -> Option { - Some(self.range) + fn span(&self) -> Option { + Some(Span::from(self.file).with_range(self.range)) } fn severity(&self) -> Severity { diff --git a/crates/red_knot_test/src/matcher.rs b/crates/red_knot_test/src/matcher.rs index e7add5c50adb27..d350ec7c61c3ec 100644 --- a/crates/red_knot_test/src/matcher.rs +++ b/crates/red_knot_test/src/matcher.rs @@ -257,7 +257,8 @@ impl Matcher { fn column(&self, diagnostic: &T) -> OneIndexed { diagnostic - .range() + .span() + .and_then(|span| span.range()) .map(|range| { self.line_index .source_location(range.start(), &self.source) @@ -334,7 +335,7 @@ impl Matcher { #[cfg(test)] mod tests { use super::FailuresByLine; - use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity}; + use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity, Span}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; use ruff_python_trivia::textwrap::dedent; @@ -385,12 +386,8 @@ mod tests { self.message.into() } - fn file(&self) -> Option { - Some(self.file) - } - - fn range(&self) -> Option { - Some(self.range) + fn span(&self) -> Option { + Some(Span::from(self.file).with_range(self.range)) } fn severity(&self) -> Severity { diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 5c6362dc0ac0cb..87366b04dec1bf 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -229,8 +229,14 @@ fn assert_diagnostics(db: &dyn Db, diagnostics: &[Box]) { .map(|diagnostic| { ( diagnostic.id(), - diagnostic.file().map(|file| file.path(db).as_str()), - diagnostic.range().map(Range::::from), + diagnostic + .span() + .map(|span| span.file()) + .map(|file| file.path(db).as_str()), + diagnostic + .span() + .and_then(|span| span.range()) + .map(Range::::from), diagnostic.message(), diagnostic.severity(), ) diff --git a/crates/ruff_db/src/diagnostic.rs b/crates/ruff_db/src/diagnostic.rs index 4b3be76ab9f147..e342014b3f97e0 100644 --- a/crates/ruff_db/src/diagnostic.rs +++ b/crates/ruff_db/src/diagnostic.rs @@ -164,19 +164,11 @@ pub trait Diagnostic: Send + Sync + std::fmt::Debug { fn message(&self) -> Cow; - /// The file this diagnostic is associated with. - /// - /// File can be `None` for diagnostics that don't originate from a file. - /// For example: - /// * A diagnostic indicating that a directory couldn't be read. - /// * A diagnostic related to a CLI argument - fn file(&self) -> Option; - - /// The primary range of the diagnostic in `file`. + /// The primary span of the diagnostic. /// /// The range can be `None` if the diagnostic doesn't have a file /// or it applies to the entire file (e.g. the file should be executable but isn't). - fn range(&self) -> Option; + fn span(&self) -> Option; fn severity(&self) -> Severity; @@ -191,6 +183,47 @@ pub trait Diagnostic: Send + Sync + std::fmt::Debug { } } +/// A span represents the source of a diagnostic. +/// +/// 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)] +pub struct Span { + file: File, + range: Option, +} + +impl Span { + /// Returns the `File` attached to this `Span`. + pub fn file(&self) -> File { + self.file + } + + /// Returns the range, if available, attached to this `Span`. + /// + /// When there is no range, it is convention to assume that this `Span` + /// refers to the corresponding `File` as a whole. In some cases, consumers + /// of this API may use the range `0..0` to represent this case. + pub fn range(&self) -> Option { + self.range + } + + /// Returns a new `Span` with the given `range` attached to it. + pub fn with_range(self, range: TextRange) -> Span { + Span { + range: Some(range), + ..self + } + } +} + +impl From for Span { + fn from(file: File) -> Span { + Span { file, range: None } + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)] pub enum Severity { Info, @@ -236,8 +269,8 @@ impl std::fmt::Display for DisplayDiagnostic<'_> { let rendered = renderer.render(message); writeln!(f, "{rendered}") }; - match (self.diagnostic.file(), self.diagnostic.range()) { - (None, _) => { + match self.diagnostic.span() { + None => { // NOTE: This is pretty sub-optimal. It doesn't render well. We // really want a snippet, but without a `File`, we can't really // render anything. It looks like this case currently happens @@ -248,20 +281,20 @@ impl std::fmt::Display for DisplayDiagnostic<'_> { let msg = format!("{}: {}", self.diagnostic.id(), self.diagnostic.message()); render(f, level.title(&msg)) } - (Some(file), range) => { - let path = file.path(self.db).to_string(); - let source = source_text(self.db, file); + Some(span) => { + let path = span.file.path(self.db).to_string(); + let source = source_text(self.db, span.file); let title = self.diagnostic.id().to_string(); let message = self.diagnostic.message(); - let Some(range) = range else { + let Some(range) = span.range else { let snippet = Snippet::source(source.as_str()).origin(&path).line_start(1); return render(f, level.title(&title).snippet(snippet)); }; // The bits below are a simplified copy from // `crates/ruff_linter/src/message/text.rs`. - let index = line_index(self.db, file); + let index = line_index(self.db, span.file); let source_code = SourceCode::new(source.as_str(), &index); let content_start_index = source_code.line_index(range.start()); @@ -315,12 +348,8 @@ where (**self).message() } - fn file(&self) -> Option { - (**self).file() - } - - fn range(&self) -> Option { - (**self).range() + fn span(&self) -> Option { + (**self).span() } fn severity(&self) -> Severity { @@ -340,12 +369,8 @@ where (**self).message() } - fn file(&self) -> Option { - (**self).file() - } - - fn range(&self) -> Option { - (**self).range() + fn span(&self) -> Option { + (**self).span() } fn severity(&self) -> Severity { @@ -362,12 +387,8 @@ impl Diagnostic for Box { (**self).message() } - fn file(&self) -> Option { - (**self).file() - } - - fn range(&self) -> Option { - (**self).range() + fn span(&self) -> Option { + (**self).span() } fn severity(&self) -> Severity { @@ -384,12 +405,8 @@ impl Diagnostic for &'_ dyn Diagnostic { (**self).message() } - fn file(&self) -> Option { - (**self).file() - } - - fn range(&self) -> Option { - (**self).range() + fn span(&self) -> Option { + (**self).span() } fn severity(&self) -> Severity { @@ -406,12 +423,8 @@ impl Diagnostic for std::sync::Arc { (**self).message() } - fn file(&self) -> Option { - (**self).file() - } - - fn range(&self) -> Option { - (**self).range() + fn span(&self) -> Option { + (**self).span() } fn severity(&self) -> Severity { @@ -440,12 +453,8 @@ impl Diagnostic for ParseDiagnostic { self.error.error.to_string().into() } - fn file(&self) -> Option { - Some(self.file) - } - - fn range(&self) -> Option { - Some(self.error.location) + fn span(&self) -> Option { + Some(Span::from(self.file).with_range(self.error.location)) } fn severity(&self) -> Severity {