Skip to content

Commit

Permalink
add diagnostic Span (couples File and TextRange) (#16101)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
BurntSushi authored Feb 11, 2025
1 parent 9c17931 commit 6e34f74
Show file tree
Hide file tree
Showing 8 changed files with 115 additions and 114 deletions.
19 changes: 10 additions & 9 deletions crates/red_knot_project/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -345,7 +344,13 @@ fn check_file_impl(db: &dyn Db, file: File) -> Vec<Box<dyn Diagnostic>> {
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
}
Expand Down Expand Up @@ -458,12 +463,8 @@ impl Diagnostic for IOErrorDiagnostic {
self.error.to_string().into()
}

fn file(&self) -> Option<File> {
Some(self.file)
}

fn range(&self) -> Option<TextRange> {
None
fn span(&self) -> Option<Span> {
Some(Span::from(self.file))
}

fn severity(&self) -> Severity {
Expand Down
39 changes: 16 additions & 23 deletions crates/red_knot_project/src/metadata/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
}
}
}
Expand Down Expand Up @@ -348,8 +354,7 @@ pub struct OptionDiagnostic {
id: DiagnosticId,
message: String,
severity: Severity,
file: Option<File>,
range: Option<TextRange>,
span: Option<Span>,
}

impl OptionDiagnostic {
Expand All @@ -358,21 +363,13 @@ impl OptionDiagnostic {
id,
message,
severity,
file: None,
range: None,
span: None,
}
}

#[must_use]
fn with_file(mut self, file: Option<File>) -> Self {
self.file = file;
self
}

#[must_use]
fn with_range(mut self, range: Option<TextRange>) -> Self {
self.range = range;
self
fn with_span(self, span: Option<Span>) -> Self {
OptionDiagnostic { span, ..self }
}
}

Expand All @@ -385,12 +382,8 @@ impl Diagnostic for OptionDiagnostic {
Cow::Borrowed(&self.message)
}

fn file(&self) -> Option<File> {
self.file
}

fn range(&self) -> Option<TextRange> {
self.range
fn span(&self) -> Option<Span> {
self.span.clone()
}

fn severity(&self) -> Severity {
Expand Down
10 changes: 3 additions & 7 deletions crates/red_knot_python_semantic/src/types/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -802,12 +802,8 @@ impl Diagnostic for TypeCheckDiagnostic {
TypeCheckDiagnostic::message(self).into()
}

fn file(&self) -> Option<File> {
Some(TypeCheckDiagnostic::file(self))
}

fn range(&self) -> Option<TextRange> {
Some(Ranged::range(self))
fn span(&self) -> Option<Span> {
Some(Span::from(self.file).with_range(self.range))
}

fn severity(&self) -> Severity {
Expand Down
10 changes: 6 additions & 4 deletions crates/red_knot_server/src/server/api/requests/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
};
Expand Down
13 changes: 5 additions & 8 deletions crates/red_knot_test/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}),
Expand Down Expand Up @@ -144,7 +145,7 @@ struct DiagnosticWithLine<T> {
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};
Expand Down Expand Up @@ -198,12 +199,8 @@ mod tests {
"dummy".into()
}

fn file(&self) -> Option<File> {
Some(self.file)
}

fn range(&self) -> Option<TextRange> {
Some(self.range)
fn span(&self) -> Option<Span> {
Some(Span::from(self.file).with_range(self.range))
}

fn severity(&self) -> Severity {
Expand Down
13 changes: 5 additions & 8 deletions crates/red_knot_test/src/matcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,8 @@ impl Matcher {

fn column<T: Diagnostic>(&self, diagnostic: &T) -> OneIndexed {
diagnostic
.range()
.span()
.and_then(|span| span.range())
.map(|range| {
self.line_index
.source_location(range.start(), &self.source)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -385,12 +386,8 @@ mod tests {
self.message.into()
}

fn file(&self) -> Option<File> {
Some(self.file)
}

fn range(&self) -> Option<TextRange> {
Some(self.range)
fn span(&self) -> Option<Span> {
Some(Span::from(self.file).with_range(self.range))
}

fn severity(&self) -> Severity {
Expand Down
10 changes: 8 additions & 2 deletions crates/ruff_benchmark/benches/red_knot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,14 @@ fn assert_diagnostics(db: &dyn Db, diagnostics: &[Box<dyn Diagnostic>]) {
.map(|diagnostic| {
(
diagnostic.id(),
diagnostic.file().map(|file| file.path(db).as_str()),
diagnostic.range().map(Range::<usize>::from),
diagnostic
.span()
.map(|span| span.file())
.map(|file| file.path(db).as_str()),
diagnostic
.span()
.and_then(|span| span.range())
.map(Range::<usize>::from),
diagnostic.message(),
diagnostic.severity(),
)
Expand Down
Loading

0 comments on commit 6e34f74

Please sign in to comment.