diff --git a/Cargo.lock b/Cargo.lock index d325171eec..bbe15c5493 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2814,6 +2814,7 @@ dependencies = [ "regex", "ruff_annotate_snippets", "ruff_cache", + "ruff_db", "ruff_diagnostics", "ruff_macros", "ruff_notebook", diff --git a/crates/ruff/src/cache.rs b/crates/ruff/src/cache.rs index e30c461a12..127f6292b2 100644 --- a/crates/ruff/src/cache.rs +++ b/crates/ruff/src/cache.rs @@ -439,7 +439,7 @@ impl LintCacheData { .map(|msg| { // Make sure that all message use the same source file. assert_eq!( - &msg.file, + msg.file, messages.first().unwrap().source_file(), "message uses a different source file" ); diff --git a/crates/ruff/src/diagnostics.rs b/crates/ruff/src/diagnostics.rs index fc462de30f..4610eca5c4 100644 --- a/crates/ruff/src/diagnostics.rs +++ b/crates/ruff/src/diagnostics.rs @@ -15,7 +15,7 @@ use rustc_hash::FxHashMap; use ruff_diagnostics::Diagnostic; use ruff_linter::codes::Rule; use ruff_linter::linter::{lint_fix, lint_only, FixTable, FixerResult, LinterResult, ParseSource}; -use ruff_linter::message::{Message, SyntaxErrorMessage}; +use ruff_linter::message::Message; use ruff_linter::package::PackageRoot; use ruff_linter::pyproject_toml::lint_pyproject_toml; use ruff_linter::settings::types::UnsafeFixes; @@ -102,11 +102,7 @@ impl Diagnostics { let name = path.map_or_else(|| "-".into(), Path::to_string_lossy); let dummy = SourceFileBuilder::new(name, "").finish(); Self::new( - vec![Message::SyntaxError(SyntaxErrorMessage { - message: err.to_string(), - range: TextRange::default(), - file: dummy, - })], + vec![Message::syntax_error(err, TextRange::default(), dummy)], FxHashMap::default(), ) } diff --git a/crates/ruff_benchmark/benches/ty.rs b/crates/ruff_benchmark/benches/ty.rs index 028a3e8563..83c58e5e82 100644 --- a/crates/ruff_benchmark/benches/ty.rs +++ b/crates/ruff_benchmark/benches/ty.rs @@ -197,7 +197,7 @@ fn assert_diagnostics(db: &dyn Db, diagnostics: &[Diagnostic], expected: &[KeyDi diagnostic.id(), diagnostic .primary_span() - .map(|span| span.file()) + .map(|span| span.expect_ty_file()) .map(|file| file.path(db).as_str()), diagnostic .primary_span() diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index b3b82486b3..2f61c8a15d 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -1,15 +1,15 @@ use std::{fmt::Formatter, sync::Arc}; +use render::{FileResolver, Input}; +use ruff_source_file::{SourceCode, SourceFile}; use thiserror::Error; use ruff_annotate_snippets::Level as AnnotateLevel; use ruff_text_size::{Ranged, TextRange}; pub use self::render::DisplayDiagnostic; -use crate::files::File; -use crate::Db; +use crate::{files::File, Db}; -use self::render::FileResolver; mod render; mod stylesheet; @@ -115,10 +115,9 @@ impl Diagnostic { /// callers should prefer using this with `write!` instead of `writeln!`. pub fn display<'a>( &'a self, - db: &'a dyn Db, + resolver: &'a dyn FileResolver, config: &'a DisplayDiagnosticConfig, ) -> DisplayDiagnostic<'a> { - let resolver = FileResolver::new(db); DisplayDiagnostic::new(resolver, config, self) } @@ -233,6 +232,16 @@ impl Diagnostic { self.primary_annotation().map(|ann| ann.tags.as_slice()) } + /// Returns the "primary" span of this diagnostic, panicking if it does not exist. + /// + /// This should typically only be used when working with diagnostics in Ruff, where diagnostics + /// are currently required to have a primary span. + /// + /// See [`Diagnostic::primary_span`] for more details. + pub fn expect_primary_span(&self) -> Span { + self.primary_span().expect("Expected a primary span") + } + /// Returns a key that can be used to sort two diagnostics into the canonical order /// in which they should appear when rendered. pub fn rendering_sort_key<'a>(&'a self, db: &'a dyn Db) -> impl Ord + 'a { @@ -267,11 +276,7 @@ impl Ord for RenderingSortKey<'_> { self.diagnostic.primary_span(), other.diagnostic.primary_span(), ) { - let order = span1 - .file() - .path(self.db) - .as_str() - .cmp(span2.file().path(self.db).as_str()); + let order = span1.file().path(&self.db).cmp(span2.file().path(&self.db)); if order.is_ne() { return order; } @@ -643,6 +648,10 @@ impl DiagnosticId { DiagnosticId::UnknownRule => "unknown-rule", }) } + + pub fn is_invalid_syntax(&self) -> bool { + matches!(self, Self::InvalidSyntax) + } } #[derive(Copy, Clone, Debug, Eq, PartialEq, Error)] @@ -668,6 +677,62 @@ impl std::fmt::Display for DiagnosticId { } } +/// A unified file representation for both ruff and ty. +/// +/// Such a representation is needed for rendering [`Diagnostic`]s that can optionally contain +/// [`Annotation`]s with [`Span`]s that need to refer to the text of a file. However, ty and ruff +/// use very different file types: a `Copy`-able salsa-interned [`File`], and a heavier-weight +/// [`SourceFile`], respectively. +/// +/// This enum presents a unified interface to these two types for the sake of creating [`Span`]s and +/// emitting diagnostics from both ty and ruff. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum UnifiedFile { + Ty(File), + Ruff(SourceFile), +} + +impl UnifiedFile { + pub fn path<'a>(&'a self, resolver: &'a dyn FileResolver) -> &'a str { + match self { + UnifiedFile::Ty(file) => resolver.path(*file), + UnifiedFile::Ruff(file) => file.name(), + } + } + + fn diagnostic_source(&self, resolver: &dyn FileResolver) -> DiagnosticSource { + match self { + UnifiedFile::Ty(file) => DiagnosticSource::Ty(resolver.input(*file)), + UnifiedFile::Ruff(file) => DiagnosticSource::Ruff(file.clone()), + } + } +} + +/// A unified wrapper for types that can be converted to a [`SourceCode`]. +/// +/// As with [`UnifiedFile`], ruff and ty use slightly different representations for source code. +/// [`DiagnosticSource`] wraps both of these and provides the single +/// [`DiagnosticSource::as_source_code`] method to produce a [`SourceCode`] with the appropriate +/// lifetimes. +/// +/// See [`UnifiedFile::diagnostic_source`] for a way to obtain a [`DiagnosticSource`] from a file +/// and [`FileResolver`]. +#[derive(Clone, Debug)] +enum DiagnosticSource { + Ty(Input), + Ruff(SourceFile), +} + +impl DiagnosticSource { + /// Returns this input as a `SourceCode` for convenient querying. + fn as_source_code(&self) -> SourceCode { + match self { + DiagnosticSource::Ty(input) => SourceCode::new(input.text.as_str(), &input.line_index), + DiagnosticSource::Ruff(source) => SourceCode::new(source.source_text(), source.index()), + } + } +} + /// A span represents the source of a diagnostic. /// /// It consists of a `File` and an optional range into that file. When the @@ -675,14 +740,14 @@ impl std::fmt::Display for DiagnosticId { /// the entire file. For example, when the file should be executable but isn't. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Span { - file: File, + file: UnifiedFile, range: Option, } impl Span { - /// Returns the `File` attached to this `Span`. - pub fn file(&self) -> File { - self.file + /// Returns the `UnifiedFile` attached to this `Span`. + pub fn file(&self) -> &UnifiedFile { + &self.file } /// Returns the range, if available, attached to this `Span`. @@ -703,10 +768,38 @@ impl Span { pub fn with_optional_range(self, range: Option) -> Span { Span { range, ..self } } + + /// Returns the [`File`] attached to this [`Span`]. + /// + /// Panics if the file is a [`UnifiedFile::Ruff`] instead of a [`UnifiedFile::Ty`]. + pub fn expect_ty_file(&self) -> File { + match self.file { + UnifiedFile::Ty(file) => file, + UnifiedFile::Ruff(_) => panic!("Expected a ty `File`, found a ruff `SourceFile`"), + } + } + + /// Returns the [`SourceFile`] attached to this [`Span`]. + /// + /// Panics if the file is a [`UnifiedFile::Ty`] instead of a [`UnifiedFile::Ruff`]. + pub fn expect_ruff_file(&self) -> &SourceFile { + match &self.file { + UnifiedFile::Ty(_) => panic!("Expected a ruff `SourceFile`, found a ty `File`"), + UnifiedFile::Ruff(file) => file, + } + } } impl From for Span { fn from(file: File) -> Span { + let file = UnifiedFile::Ty(file); + Span { file, range: None } + } +} + +impl From for Span { + fn from(file: SourceFile) -> Self { + let file = UnifiedFile::Ruff(file); Span { file, range: None } } } diff --git a/crates/ruff_db/src/diagnostic/render.rs b/crates/ruff_db/src/diagnostic/render.rs index 1676347f33..c02565c684 100644 --- a/crates/ruff_db/src/diagnostic/render.rs +++ b/crates/ruff_db/src/diagnostic/render.rs @@ -16,7 +16,8 @@ use crate::{ }; use super::{ - Annotation, Diagnostic, DiagnosticFormat, DisplayDiagnosticConfig, Severity, SubDiagnostic, + Annotation, Diagnostic, DiagnosticFormat, DiagnosticSource, DisplayDiagnosticConfig, Severity, + SubDiagnostic, }; /// A type that implements `std::fmt::Display` for diagnostic rendering. @@ -30,17 +31,16 @@ use super::{ /// values. When using Salsa, this most commonly corresponds to the lifetime /// of a Salsa `Db`. /// * The lifetime of the diagnostic being rendered. -#[derive(Debug)] pub struct DisplayDiagnostic<'a> { config: &'a DisplayDiagnosticConfig, - resolver: FileResolver<'a>, + resolver: &'a dyn FileResolver, annotate_renderer: AnnotateRenderer, diag: &'a Diagnostic, } impl<'a> DisplayDiagnostic<'a> { pub(crate) fn new( - resolver: FileResolver<'a>, + resolver: &'a dyn FileResolver, config: &'a DisplayDiagnosticConfig, diag: &'a Diagnostic, ) -> DisplayDiagnostic<'a> { @@ -86,11 +86,13 @@ impl std::fmt::Display for DisplayDiagnostic<'_> { write!( f, " {path}", - path = fmt_styled(self.resolver.path(span.file()), stylesheet.emphasis) + path = fmt_styled(span.file().path(self.resolver), stylesheet.emphasis) )?; if let Some(range) = span.range() { - let input = self.resolver.input(span.file()); - let start = input.as_source_code().line_column(range.start()); + let diagnostic_source = span.file().diagnostic_source(self.resolver); + let start = diagnostic_source + .as_source_code() + .line_column(range.start()); write!( f, @@ -115,7 +117,7 @@ impl std::fmt::Display for DisplayDiagnostic<'_> { .emphasis(stylesheet.emphasis) .none(stylesheet.none); - let resolved = Resolved::new(&self.resolver, self.diag); + let resolved = Resolved::new(self.resolver, self.diag); let renderable = resolved.to_renderable(self.config.context); for diag in renderable.diagnostics.iter() { writeln!(f, "{}", renderer.render(diag.to_annotate()))?; @@ -144,7 +146,7 @@ struct Resolved<'a> { impl<'a> Resolved<'a> { /// Creates a new resolved set of diagnostics. - fn new(resolver: &FileResolver<'a>, diag: &'a Diagnostic) -> Resolved<'a> { + fn new(resolver: &'a dyn FileResolver, diag: &'a Diagnostic) -> Resolved<'a> { let mut diagnostics = vec![]; diagnostics.push(ResolvedDiagnostic::from_diagnostic(resolver, diag)); for sub in &diag.inner.subs { @@ -182,7 +184,7 @@ struct ResolvedDiagnostic<'a> { impl<'a> ResolvedDiagnostic<'a> { /// Resolve a single diagnostic. fn from_diagnostic( - resolver: &FileResolver<'a>, + resolver: &'a dyn FileResolver, diag: &'a Diagnostic, ) -> ResolvedDiagnostic<'a> { let annotations: Vec<_> = diag @@ -190,9 +192,9 @@ impl<'a> ResolvedDiagnostic<'a> { .annotations .iter() .filter_map(|ann| { - let path = resolver.path(ann.span.file); - let input = resolver.input(ann.span.file); - ResolvedAnnotation::new(path, &input, ann) + let path = ann.span.file.path(resolver); + let diagnostic_source = ann.span.file.diagnostic_source(resolver); + ResolvedAnnotation::new(path, &diagnostic_source, ann) }) .collect(); let message = if diag.inner.message.as_str().is_empty() { @@ -216,7 +218,7 @@ impl<'a> ResolvedDiagnostic<'a> { /// Resolve a single sub-diagnostic. fn from_sub_diagnostic( - resolver: &FileResolver<'a>, + resolver: &'a dyn FileResolver, diag: &'a SubDiagnostic, ) -> ResolvedDiagnostic<'a> { let annotations: Vec<_> = diag @@ -224,9 +226,9 @@ impl<'a> ResolvedDiagnostic<'a> { .annotations .iter() .filter_map(|ann| { - let path = resolver.path(ann.span.file); - let input = resolver.input(ann.span.file); - ResolvedAnnotation::new(path, &input, ann) + let path = ann.span.file.path(resolver); + let diagnostic_source = ann.span.file.diagnostic_source(resolver); + ResolvedAnnotation::new(path, &diagnostic_source, ann) }) .collect(); ResolvedDiagnostic { @@ -259,10 +261,18 @@ impl<'a> ResolvedDiagnostic<'a> { continue; }; - let prev_context_ends = - context_after(&prev.input.as_source_code(), context, prev.line_end).get(); - let this_context_begins = - context_before(&ann.input.as_source_code(), context, ann.line_start).get(); + let prev_context_ends = context_after( + &prev.diagnostic_source.as_source_code(), + context, + prev.line_end, + ) + .get(); + let this_context_begins = context_before( + &ann.diagnostic_source.as_source_code(), + context, + ann.line_start, + ) + .get(); // The boundary case here is when `prev_context_ends` // is exactly one less than `this_context_begins`. In // that case, the context windows are adajcent and we @@ -304,7 +314,7 @@ impl<'a> ResolvedDiagnostic<'a> { #[derive(Debug)] struct ResolvedAnnotation<'a> { path: &'a str, - input: Input, + diagnostic_source: DiagnosticSource, range: TextRange, line_start: OneIndexed, line_end: OneIndexed, @@ -318,8 +328,12 @@ impl<'a> ResolvedAnnotation<'a> { /// `path` is the path of the file that this annotation points to. /// /// `input` is the contents of the file that this annotation points to. - fn new(path: &'a str, input: &Input, ann: &'a Annotation) -> Option> { - let source = input.as_source_code(); + fn new( + path: &'a str, + diagnostic_source: &DiagnosticSource, + ann: &'a Annotation, + ) -> Option> { + let source = diagnostic_source.as_source_code(); let (range, line_start, line_end) = match (ann.span.range(), ann.message.is_some()) { // An annotation with no range AND no message is probably(?) // meaningless, but we should try to render it anyway. @@ -345,7 +359,7 @@ impl<'a> ResolvedAnnotation<'a> { }; Some(ResolvedAnnotation { path, - input: input.clone(), + diagnostic_source: diagnostic_source.clone(), range, line_start, line_end, @@ -510,8 +524,8 @@ impl<'r> RenderableSnippet<'r> { !anns.is_empty(), "creating a renderable snippet requires a non-zero number of annotations", ); - let input = &anns[0].input; - let source = input.as_source_code(); + let diagnostic_source = &anns[0].diagnostic_source; + let source = diagnostic_source.as_source_code(); let has_primary = anns.iter().any(|ann| ann.is_primary); let line_start = context_before( @@ -527,7 +541,7 @@ impl<'r> RenderableSnippet<'r> { let snippet_start = source.line_start(line_start); let snippet_end = source.line_end(line_end); - let snippet = input + let snippet = diagnostic_source .as_source_code() .slice(TextRange::new(snippet_start, snippet_end)); @@ -613,7 +627,7 @@ impl<'r> RenderableAnnotation<'r> { } } -/// A type that facilitates the retrieval of source code from a `Span`. +/// A trait that facilitates the retrieval of source code from a `Span`. /// /// At present, this is tightly coupled with a Salsa database. In the future, /// it is intended for this resolver to become an abstraction providing a @@ -628,36 +642,24 @@ impl<'r> RenderableAnnotation<'r> { /// callers will need to pass in a different "resolver" for turning `Span`s /// into actual file paths/contents. The infrastructure for this isn't fully in /// place, but this type serves to demarcate the intended abstraction boundary. -pub(crate) struct FileResolver<'a> { - db: &'a dyn Db, -} - -impl<'a> FileResolver<'a> { - /// Creates a new resolver from a Salsa database. - pub(crate) fn new(db: &'a dyn Db) -> FileResolver<'a> { - FileResolver { db } - } - +pub trait FileResolver { /// Returns the path associated with the file given. - fn path(&self, file: File) -> &'a str { - relativize_path( - self.db.system().current_directory(), - file.path(self.db).as_str(), - ) - } + fn path(&self, file: File) -> &str; /// Returns the input contents associated with the file given. - fn input(&self, file: File) -> Input { - Input { - text: source_text(self.db, file), - line_index: line_index(self.db, file), - } - } + fn input(&self, file: File) -> Input; } -impl std::fmt::Debug for FileResolver<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - write!(f, "") +impl FileResolver for &dyn Db { + fn path(&self, file: File) -> &str { + relativize_path(self.system().current_directory(), file.path(*self).as_str()) + } + + fn input(&self, file: File) -> Input { + Input { + text: source_text(*self, file), + line_index: line_index(*self, file), + } } } @@ -667,16 +669,9 @@ impl std::fmt::Debug for FileResolver<'_> { /// This contains the actual content of that input as well as a /// line index for efficiently querying its contents. #[derive(Clone, Debug)] -struct Input { - text: SourceText, - line_index: LineIndex, -} - -impl Input { - /// Returns this input as a `SourceCode` for convenient querying. - fn as_source_code(&self) -> SourceCode<'_, '_> { - SourceCode::new(self.text.as_str(), &self.line_index) - } +pub struct Input { + pub(crate) text: SourceText, + pub(crate) line_index: LineIndex, } /// Returns the line number accounting for the given `len` @@ -730,6 +725,7 @@ mod tests { use crate::files::system_path_to_file; use crate::system::{DbWithWritableSystem, SystemPath}; use crate::tests::TestDb; + use crate::Upcast; use super::*; @@ -2174,8 +2170,9 @@ watermelon fn span(&self, path: &str, line_offset_start: &str, line_offset_end: &str) -> Span { let span = self.path(path); - let text = source_text(&self.db, span.file()); - let line_index = line_index(&self.db, span.file()); + let file = span.expect_ty_file(); + let text = source_text(&self.db, file); + let line_index = line_index(&self.db, file); let source = SourceCode::new(text.as_str(), &line_index); let (line_start, offset_start) = parse_line_offset(line_offset_start); @@ -2237,7 +2234,7 @@ watermelon /// /// (This will set the "printed" flag on `Diagnostic`.) fn render(&self, diag: &Diagnostic) -> String { - diag.display(&self.db, &self.config).to_string() + diag.display(&self.db.upcast(), &self.config).to_string() } } diff --git a/crates/ruff_db/src/lib.rs b/crates/ruff_db/src/lib.rs index cdb3071553..c970a99d12 100644 --- a/crates/ruff_db/src/lib.rs +++ b/crates/ruff_db/src/lib.rs @@ -67,7 +67,7 @@ mod tests { use crate::system::TestSystem; use crate::system::{DbWithTestSystem, System}; use crate::vendored::VendoredFileSystem; - use crate::Db; + use crate::{Db, Upcast}; type Events = Arc>>; @@ -140,6 +140,15 @@ mod tests { } } + impl Upcast for TestDb { + fn upcast(&self) -> &(dyn Db + 'static) { + self + } + fn upcast_mut(&mut self) -> &mut (dyn Db + 'static) { + self + } + } + impl DbWithTestSystem for TestDb { fn test_system(&self) -> &TestSystem { &self.system diff --git a/crates/ruff_linter/Cargo.toml b/crates/ruff_linter/Cargo.toml index 2e48fbde19..0c204714a4 100644 --- a/crates/ruff_linter/Cargo.toml +++ b/crates/ruff_linter/Cargo.toml @@ -15,6 +15,7 @@ license = { workspace = true } [dependencies] ruff_annotate_snippets = { workspace = true } ruff_cache = { workspace = true } +ruff_db = { workspace = true } ruff_diagnostics = { workspace = true, features = ["serde"] } ruff_notebook = { workspace = true } ruff_macros = { workspace = true } diff --git a/crates/ruff_linter/src/message/azure.rs b/crates/ruff_linter/src/message/azure.rs index 727a427f9b..bb20d710f4 100644 --- a/crates/ruff_linter/src/message/azure.rs +++ b/crates/ruff_linter/src/message/azure.rs @@ -17,7 +17,7 @@ impl Emitter for AzureEmitter { context: &EmitterContext, ) -> anyhow::Result<()> { for message in messages { - let location = if context.is_notebook(message.filename()) { + let location = if context.is_notebook(&message.filename()) { // We can't give a reasonable location for the structured formats, // so we show one that's clearly a fallback LineColumn::default() diff --git a/crates/ruff_linter/src/message/diff.rs b/crates/ruff_linter/src/message/diff.rs index 0e56578e17..c0358d0b08 100644 --- a/crates/ruff_linter/src/message/diff.rs +++ b/crates/ruff_linter/src/message/diff.rs @@ -22,7 +22,7 @@ use crate::text_helpers::ShowNonprinting; /// * Compute the diff from the [`Edit`] because diff calculation is expensive. pub(super) struct Diff<'a> { fix: &'a Fix, - source_code: &'a SourceFile, + source_code: SourceFile, } impl<'a> Diff<'a> { diff --git a/crates/ruff_linter/src/message/github.rs b/crates/ruff_linter/src/message/github.rs index a038b17704..acff9e4c68 100644 --- a/crates/ruff_linter/src/message/github.rs +++ b/crates/ruff_linter/src/message/github.rs @@ -19,7 +19,7 @@ impl Emitter for GithubEmitter { ) -> anyhow::Result<()> { for message in messages { let source_location = message.compute_start_location(); - let location = if context.is_notebook(message.filename()) { + let location = if context.is_notebook(&message.filename()) { // We can't give a reasonable location for the structured formats, // so we show one that's clearly a fallback LineColumn::default() @@ -43,7 +43,7 @@ impl Emitter for GithubEmitter { write!( writer, "{path}:{row}:{column}:", - path = relativize_path(message.filename()), + path = relativize_path(&*message.filename()), row = location.line, column = location.column, )?; diff --git a/crates/ruff_linter/src/message/gitlab.rs b/crates/ruff_linter/src/message/gitlab.rs index 6d03a7c803..0ea0af43cf 100644 --- a/crates/ruff_linter/src/message/gitlab.rs +++ b/crates/ruff_linter/src/message/gitlab.rs @@ -62,7 +62,7 @@ impl Serialize for SerializedMessages<'_> { let start_location = message.compute_start_location(); let end_location = message.compute_end_location(); - let lines = if self.context.is_notebook(message.filename()) { + let lines = if self.context.is_notebook(&message.filename()) { // We can't give a reasonable location for the structured formats, // so we show one that's clearly a fallback json!({ @@ -77,8 +77,8 @@ impl Serialize for SerializedMessages<'_> { }; let path = self.project_dir.as_ref().map_or_else( - || relativize_path(message.filename()), - |project_dir| relativize_path_to(message.filename(), project_dir), + || relativize_path(&*message.filename()), + |project_dir| relativize_path_to(&*message.filename(), project_dir), ); let mut message_fingerprint = fingerprint(message, &path, 0); diff --git a/crates/ruff_linter/src/message/grouped.rs b/crates/ruff_linter/src/message/grouped.rs index b60830c961..c21198f89c 100644 --- a/crates/ruff_linter/src/message/grouped.rs +++ b/crates/ruff_linter/src/message/grouped.rs @@ -65,7 +65,7 @@ impl Emitter for GroupedEmitter { let column_length = calculate_print_width(max_column_length); // Print the filename. - writeln!(writer, "{}:", relativize_path(filename).underline())?; + writeln!(writer, "{}:", relativize_path(&*filename).underline())?; // Print each message. for message in messages { @@ -73,7 +73,7 @@ impl Emitter for GroupedEmitter { writer, "{}", DisplayGroupedMessage { - notebook_index: context.notebook_index(message.filename()), + notebook_index: context.notebook_index(&message.filename()), message, show_fix_status: self.show_fix_status, unsafe_fixes: self.unsafe_fixes, diff --git a/crates/ruff_linter/src/message/json.rs b/crates/ruff_linter/src/message/json.rs index b8d91c7db3..21b1d6d6ad 100644 --- a/crates/ruff_linter/src/message/json.rs +++ b/crates/ruff_linter/src/message/json.rs @@ -49,8 +49,9 @@ impl Serialize for ExpandedMessages<'_> { } pub(crate) fn message_to_json_value(message: &Message, context: &EmitterContext) -> Value { - let source_code = message.source_file().to_source_code(); - let notebook_index = context.notebook_index(message.filename()); + let source_file = message.source_file(); + let source_code = source_file.to_source_code(); + let notebook_index = context.notebook_index(&message.filename()); let fix = message.fix().map(|fix| { json!({ diff --git a/crates/ruff_linter/src/message/junit.rs b/crates/ruff_linter/src/message/junit.rs index 38edfdfca8..44f1fbe7c1 100644 --- a/crates/ruff_linter/src/message/junit.rs +++ b/crates/ruff_linter/src/message/junit.rs @@ -32,7 +32,7 @@ impl Emitter for JunitEmitter { report.add_test_suite(test_suite); } else { for (filename, messages) in group_messages_by_filename(messages) { - let mut test_suite = TestSuite::new(filename); + let mut test_suite = TestSuite::new(&filename); test_suite .extra .insert(XmlString::new("package"), XmlString::new("org.ruff")); @@ -44,7 +44,7 @@ impl Emitter for JunitEmitter { } = message; let mut status = TestCaseStatus::non_success(NonSuccessKind::Failure); status.set_message(message.body()); - let location = if context.is_notebook(message.filename()) { + let location = if context.is_notebook(&message.filename()) { // We can't give a reasonable location for the structured formats, // so we show one that's clearly a fallback LineColumn::default() @@ -66,7 +66,7 @@ impl Emitter for JunitEmitter { }, status, ); - let file_path = Path::new(filename); + let file_path = Path::new(&*filename); let file_stem = file_path.file_stem().unwrap().to_str().unwrap(); let classname = file_path.parent().unwrap().join(file_stem); case.set_classname(classname.to_str().unwrap()); diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index a3146ed93d..9ca16bc2be 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -1,8 +1,10 @@ +use std::borrow::Cow; use std::cmp::Ordering; use std::collections::BTreeMap; use std::io::Write; use std::ops::Deref; +use ruff_db::diagnostic::{self as db, Annotation, DiagnosticId, Severity, Span}; use ruff_python_parser::semantic_errors::SemanticSyntaxError; use rustc_hash::FxHashMap; @@ -45,7 +47,7 @@ mod text; #[derive(Clone, Debug, PartialEq, Eq)] pub enum Message { Diagnostic(DiagnosticMessage), - SyntaxError(SyntaxErrorMessage), + SyntaxError(db::Diagnostic), } /// A diagnostic message corresponding to a rule violation. @@ -59,14 +61,6 @@ pub struct DiagnosticMessage { pub noqa_offset: TextSize, } -/// A syntax error message raised by the parser. -#[derive(Clone, Debug, PartialEq, Eq)] -pub struct SyntaxErrorMessage { - pub message: String, - pub range: TextRange, - pub file: SourceFile, -} - #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] pub enum MessageKind { Diagnostic(Rule), @@ -83,6 +77,17 @@ impl MessageKind { } impl Message { + pub fn syntax_error( + message: impl std::fmt::Display, + range: TextRange, + file: SourceFile, + ) -> Message { + let mut diag = db::Diagnostic::new(DiagnosticId::InvalidSyntax, Severity::Error, ""); + let span = Span::from(file).with_range(range); + diag.annotate(Annotation::primary(span).message(message)); + Self::SyntaxError(diag) + } + /// Create a [`Message`] from the given [`Diagnostic`] corresponding to a rule violation. pub fn from_diagnostic( diagnostic: Diagnostic, @@ -114,14 +119,14 @@ impl Message { .next() .map_or(TextSize::new(0), TextLen::text_len); - Message::SyntaxError(SyntaxErrorMessage { - message: format!( + Message::syntax_error( + format_args!( "SyntaxError: {}", DisplayParseErrorType::new(&parse_error.error) ), - range: TextRange::at(parse_error.location.start(), len), + TextRange::at(parse_error.location.start(), len), file, - }) + ) } /// Create a [`Message`] from the given [`UnsupportedSyntaxError`]. @@ -129,11 +134,11 @@ impl Message { unsupported_syntax_error: &UnsupportedSyntaxError, file: SourceFile, ) -> Message { - Message::SyntaxError(SyntaxErrorMessage { - message: format!("SyntaxError: {unsupported_syntax_error}"), - range: unsupported_syntax_error.range, + Message::syntax_error( + format_args!("SyntaxError: {unsupported_syntax_error}"), + unsupported_syntax_error.range, file, - }) + ) } /// Create a [`Message`] from the given [`SemanticSyntaxError`]. @@ -141,11 +146,11 @@ impl Message { semantic_syntax_error: &SemanticSyntaxError, file: SourceFile, ) -> Message { - Message::SyntaxError(SyntaxErrorMessage { - message: format!("SyntaxError: {semantic_syntax_error}"), - range: semantic_syntax_error.range, + Message::syntax_error( + format_args!("SyntaxError: {semantic_syntax_error}"), + semantic_syntax_error.range, file, - }) + ) } pub const fn as_diagnostic_message(&self) -> Option<&DiagnosticMessage> { @@ -168,8 +173,11 @@ impl Message { } /// Returns `true` if `self` is a syntax error message. - pub const fn is_syntax_error(&self) -> bool { - matches!(self, Message::SyntaxError(_)) + pub fn is_syntax_error(&self) -> bool { + match self { + Message::Diagnostic(_) => false, + Message::SyntaxError(diag) => diag.id().is_invalid_syntax(), + } } /// Returns a message kind. @@ -192,7 +200,11 @@ impl Message { pub fn body(&self) -> &str { match self { Message::Diagnostic(m) => &m.kind.body, - Message::SyntaxError(m) => &m.message, + Message::SyntaxError(m) => m + .primary_annotation() + .expect("Expected a primary annotation for a ruff diagnostic") + .get_message() + .expect("Expected a message for a ruff diagnostic"), } } @@ -234,27 +246,47 @@ impl Message { } /// Returns the filename for the message. - pub fn filename(&self) -> &str { - self.source_file().name() + pub fn filename(&self) -> Cow<'_, str> { + match self { + Message::Diagnostic(m) => Cow::Borrowed(m.file.name()), + Message::SyntaxError(diag) => Cow::Owned( + diag.expect_primary_span() + .expect_ruff_file() + .name() + .to_string(), + ), + } } /// Computes the start source location for the message. pub fn compute_start_location(&self) -> LineColumn { - self.source_file() - .to_source_code() - .line_column(self.start()) + match self { + Message::Diagnostic(m) => m.file.to_source_code().line_column(m.range.start()), + Message::SyntaxError(diag) => diag + .expect_primary_span() + .expect_ruff_file() + .to_source_code() + .line_column(self.start()), + } } /// Computes the end source location for the message. pub fn compute_end_location(&self) -> LineColumn { - self.source_file().to_source_code().line_column(self.end()) + match self { + Message::Diagnostic(m) => m.file.to_source_code().line_column(m.range.end()), + Message::SyntaxError(diag) => diag + .expect_primary_span() + .expect_ruff_file() + .to_source_code() + .line_column(self.end()), + } } /// Returns the [`SourceFile`] which the message belongs to. - pub fn source_file(&self) -> &SourceFile { + pub fn source_file(&self) -> SourceFile { match self { - Message::Diagnostic(m) => &m.file, - Message::SyntaxError(m) => &m.file, + Message::Diagnostic(m) => m.file.clone(), + Message::SyntaxError(m) => m.expect_primary_span().expect_ruff_file().clone(), } } } @@ -275,7 +307,10 @@ impl Ranged for Message { fn range(&self) -> TextRange { match self { Message::Diagnostic(m) => m.range, - Message::SyntaxError(m) => m.range, + Message::SyntaxError(m) => m + .expect_primary_span() + .range() + .expect("Expected range for ruff span"), } } } @@ -293,11 +328,11 @@ impl Deref for MessageWithLocation<'_> { } } -fn group_messages_by_filename(messages: &[Message]) -> BTreeMap<&str, Vec> { +fn group_messages_by_filename(messages: &[Message]) -> BTreeMap> { let mut grouped_messages = BTreeMap::default(); for message in messages { grouped_messages - .entry(message.filename()) + .entry(message.filename().to_string()) .or_insert_with(Vec::new) .push(MessageWithLocation { message, diff --git a/crates/ruff_linter/src/message/pylint.rs b/crates/ruff_linter/src/message/pylint.rs index 91d79326a7..24c79c0bf2 100644 --- a/crates/ruff_linter/src/message/pylint.rs +++ b/crates/ruff_linter/src/message/pylint.rs @@ -18,7 +18,7 @@ impl Emitter for PylintEmitter { context: &EmitterContext, ) -> anyhow::Result<()> { for message in messages { - let row = if context.is_notebook(message.filename()) { + let row = if context.is_notebook(&message.filename()) { // We can't give a reasonable location for the structured formats, // so we show one that's clearly a fallback OneIndexed::from_zero_indexed(0) @@ -39,7 +39,7 @@ impl Emitter for PylintEmitter { writeln!( writer, "{path}:{row}: {body}", - path = relativize_path(message.filename()), + path = relativize_path(&*message.filename()), )?; } diff --git a/crates/ruff_linter/src/message/rdjson.rs b/crates/ruff_linter/src/message/rdjson.rs index 26c3e1d091..6a323cc56e 100644 --- a/crates/ruff_linter/src/message/rdjson.rs +++ b/crates/ruff_linter/src/message/rdjson.rs @@ -57,7 +57,8 @@ impl Serialize for ExpandedMessages<'_> { } fn message_to_rdjson_value(message: &Message) -> Value { - let source_code = message.source_file().to_source_code(); + let source_file = message.source_file(); + let source_code = source_file.to_source_code(); let start_location = source_code.line_column(message.start()); let end_location = source_code.line_column(message.end()); diff --git a/crates/ruff_linter/src/message/sarif.rs b/crates/ruff_linter/src/message/sarif.rs index 12dfd3a1a5..d6ad7e0115 100644 --- a/crates/ruff_linter/src/message/sarif.rs +++ b/crates/ruff_linter/src/message/sarif.rs @@ -121,7 +121,7 @@ impl SarifResult { fn from_message(message: &Message) -> Result { let start_location = message.compute_start_location(); let end_location = message.compute_end_location(); - let path = normalize_path(message.filename()); + let path = normalize_path(&*message.filename()); Ok(Self { rule: message.rule(), level: "error".to_string(), @@ -141,7 +141,7 @@ impl SarifResult { fn from_message(message: &Message) -> Result { let start_location = message.compute_start_location(); let end_location = message.compute_end_location(); - let path = normalize_path(message.filename()); + let path = normalize_path(&*message.filename()); Ok(Self { rule: message.rule(), level: "error".to_string(), diff --git a/crates/ruff_linter/src/message/text.rs b/crates/ruff_linter/src/message/text.rs index f1ede8c621..53896b651c 100644 --- a/crates/ruff_linter/src/message/text.rs +++ b/crates/ruff_linter/src/message/text.rs @@ -73,12 +73,12 @@ impl Emitter for TextEmitter { write!( writer, "{path}{sep}", - path = relativize_path(message.filename()).bold(), + path = relativize_path(&*message.filename()).bold(), sep = ":".cyan(), )?; let start_location = message.compute_start_location(); - let notebook_index = context.notebook_index(message.filename()); + let notebook_index = context.notebook_index(&message.filename()); // Check if we're working on a jupyter notebook and translate positions with cell accordingly let diagnostic_location = if let Some(notebook_index) = notebook_index { @@ -191,7 +191,8 @@ impl Display for MessageCodeFrame<'_> { Vec::new() }; - let source_code = self.message.source_file().to_source_code(); + let source_file = self.message.source_file(); + let source_code = source_file.to_source_code(); let content_start_index = source_code.line_index(self.message.start()); let mut start_index = content_start_index.saturating_sub(2); diff --git a/crates/ruff_server/src/lint.rs b/crates/ruff_server/src/lint.rs index 5281c49040..670b43cb75 100644 --- a/crates/ruff_server/src/lint.rs +++ b/crates/ruff_server/src/lint.rs @@ -14,7 +14,7 @@ use ruff_linter::{ directives::{extract_directives, Flags}, generate_noqa_edits, linter::check_path, - message::{DiagnosticMessage, Message, SyntaxErrorMessage}, + message::{DiagnosticMessage, Message}, package::PackageRoot, packaging::detect_package_root, registry::AsRule, @@ -173,10 +173,10 @@ pub(crate) fn check( locator.to_index(), encoding, )), - Message::SyntaxError(syntax_error_message) => { + Message::SyntaxError(_) => { if show_syntax_errors { Some(syntax_error_to_lsp_diagnostic( - syntax_error_message, + &message, &source_kind, locator.to_index(), encoding, @@ -322,7 +322,7 @@ fn to_lsp_diagnostic( } fn syntax_error_to_lsp_diagnostic( - syntax_error: SyntaxErrorMessage, + syntax_error: &Message, source_kind: &SourceKind, index: &LineIndex, encoding: PositionEncoding, @@ -331,7 +331,7 @@ fn syntax_error_to_lsp_diagnostic( let cell: usize; if let Some(notebook_index) = source_kind.as_ipy_notebook().map(Notebook::index) { - NotebookRange { cell, range } = syntax_error.range.to_notebook_range( + NotebookRange { cell, range } = syntax_error.range().to_notebook_range( source_kind.source_code(), index, notebook_index, @@ -340,7 +340,7 @@ fn syntax_error_to_lsp_diagnostic( } else { cell = usize::default(); range = syntax_error - .range + .range() .to_range(source_kind.source_code(), index, encoding); } @@ -353,7 +353,7 @@ fn syntax_error_to_lsp_diagnostic( code: None, code_description: None, source: Some(DIAGNOSTIC_NAME.into()), - message: syntax_error.message, + message: syntax_error.body().to_string(), related_information: None, data: None, }, diff --git a/crates/ruff_source_file/src/lib.rs b/crates/ruff_source_file/src/lib.rs index 51bd8852a1..bdf3d45573 100644 --- a/crates/ruff_source_file/src/lib.rs +++ b/crates/ruff_source_file/src/lib.rs @@ -195,7 +195,7 @@ impl SourceFile { } } - fn index(&self) -> &LineIndex { + pub fn index(&self) -> &LineIndex { self.inner .line_index .get_or_init(|| LineIndex::from_source_text(self.source_text())) diff --git a/crates/ruff_wasm/src/lib.rs b/crates/ruff_wasm/src/lib.rs index e93f2aa187..dc9c20323b 100644 --- a/crates/ruff_wasm/src/lib.rs +++ b/crates/ruff_wasm/src/lib.rs @@ -1,7 +1,7 @@ use std::path::Path; use js_sys::Error; -use ruff_linter::message::{DiagnosticMessage, Message, SyntaxErrorMessage}; +use ruff_linter::message::{DiagnosticMessage, Message}; use ruff_linter::settings::types::PythonVersion; use serde::{Deserialize, Serialize}; use wasm_bindgen::prelude::*; @@ -230,15 +230,13 @@ impl Workspace { .collect(), }), }, - Message::SyntaxError(SyntaxErrorMessage { message, range, .. }) => { - ExpandedMessage { - code: None, - message, - start_location: source_code.line_column(range.start()).into(), - end_location: source_code.line_column(range.end()).into(), - fix: None, - } - } + Message::SyntaxError(_) => ExpandedMessage { + code: None, + message: message.body().to_string(), + start_location: source_code.line_column(message.range().start()).into(), + end_location: source_code.line_column(message.range().end()).into(), + fix: None, + }, }) .collect(); diff --git a/crates/ty/src/main.rs b/crates/ty/src/main.rs index 96ed01c97a..2f5e31358b 100644 --- a/crates/ty/src/main.rs +++ b/crates/ty/src/main.rs @@ -14,6 +14,7 @@ use rayon::ThreadPoolBuilder; use ruff_db::diagnostic::{Diagnostic, DisplayDiagnosticConfig, Severity}; use ruff_db::max_parallelism; use ruff_db::system::{OsSystem, SystemPath, SystemPathBuf}; +use ruff_db::Upcast; use salsa::plumbing::ZalsaDatabase; use ty_project::metadata::options::Options; use ty_project::watch::ProjectWatcher; @@ -298,7 +299,11 @@ impl MainLoop { let diagnostics_count = result.len(); for diagnostic in result { - write!(stdout, "{}", diagnostic.display(db, &display_config))?; + write!( + stdout, + "{}", + diagnostic.display(&db.upcast(), &display_config) + )?; max_severity = max_severity.max(diagnostic.severity()); } diff --git a/crates/ty_ide/src/hover.rs b/crates/ty_ide/src/hover.rs index df00a0ccb8..7ddd4212c4 100644 --- a/crates/ty_ide/src/hover.rs +++ b/crates/ty_ide/src/hover.rs @@ -136,6 +136,7 @@ mod tests { Annotation, Diagnostic, DiagnosticFormat, DiagnosticId, DisplayDiagnosticConfig, LintName, Severity, Span, }; + use ruff_db::Upcast; use ruff_text_size::{Ranged, TextRange}; #[test] @@ -773,7 +774,7 @@ mod tests { .message("Cursor offset"), ); - write!(buf, "{}", diagnostic.display(&self.db, &config)).unwrap(); + write!(buf, "{}", diagnostic.display(&self.db.upcast(), &config)).unwrap(); buf } diff --git a/crates/ty_ide/src/lib.rs b/crates/ty_ide/src/lib.rs index fa06058e5e..5e49f1da5d 100644 --- a/crates/ty_ide/src/lib.rs +++ b/crates/ty_ide/src/lib.rs @@ -204,6 +204,7 @@ mod tests { use ruff_db::diagnostic::{Diagnostic, DiagnosticFormat, DisplayDiagnosticConfig}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::system::{DbWithWritableSystem, SystemPath, SystemPathBuf}; + use ruff_db::Upcast; use ruff_python_ast::PythonVersion; use ruff_text_size::TextSize; use ty_python_semantic::{ @@ -285,7 +286,7 @@ mod tests { .format(DiagnosticFormat::Full); for diagnostic in diagnostics { let diag = diagnostic.into_diagnostic(); - write!(buf, "{}", diag.display(&self.db, &config)).unwrap(); + write!(buf, "{}", diag.display(&self.db.upcast(), &config)).unwrap(); } buf diff --git a/crates/ty_project/src/db.rs b/crates/ty_project/src/db.rs index 05cb69f066..b656f9d337 100644 --- a/crates/ty_project/src/db.rs +++ b/crates/ty_project/src/db.rs @@ -126,6 +126,16 @@ impl Upcast for ProjectDatabase { } } +impl Upcast for ProjectDatabase { + fn upcast(&self) -> &(dyn Db + 'static) { + self + } + + fn upcast_mut(&mut self) -> &mut (dyn Db + 'static) { + self + } +} + #[salsa::db] impl IdeDb for ProjectDatabase {} diff --git a/crates/ty_python_semantic/src/types/context.rs b/crates/ty_python_semantic/src/types/context.rs index 6e0b52acee..454f8ce7d7 100644 --- a/crates/ty_python_semantic/src/types/context.rs +++ b/crates/ty_python_semantic/src/types/context.rs @@ -521,7 +521,7 @@ impl Drop for DiagnosticGuard<'_, '_> { }; let expected_file = self.ctx.file(); - let got_file = ann.get_span().file(); + let got_file = ann.get_span().expect_ty_file(); assert_eq!( expected_file, got_file, diff --git a/crates/ty_server/src/server/api/requests/diagnostic.rs b/crates/ty_server/src/server/api/requests/diagnostic.rs index 201d5db91b..ea4648efb1 100644 --- a/crates/ty_server/src/server/api/requests/diagnostic.rs +++ b/crates/ty_server/src/server/api/requests/diagnostic.rs @@ -76,8 +76,9 @@ fn to_lsp_diagnostic( encoding: crate::PositionEncoding, ) -> Diagnostic { let range = if let Some(span) = diagnostic.primary_span() { - let index = line_index(db.upcast(), span.file()); - let source = source_text(db.upcast(), span.file()); + let file = span.expect_ty_file(); + let index = line_index(db.upcast(), file); + let source = source_text(db.upcast(), file); span.range() .map(|range| range.to_lsp_range(&source, &index, encoding)) diff --git a/crates/ty_test/src/lib.rs b/crates/ty_test/src/lib.rs index e0838249f2..43e6b14108 100644 --- a/crates/ty_test/src/lib.rs +++ b/crates/ty_test/src/lib.rs @@ -13,6 +13,7 @@ use ruff_db::panic::catch_unwind; use ruff_db::parsed::parsed_module; use ruff_db::system::{DbWithWritableSystem as _, SystemPath, SystemPathBuf}; use ruff_db::testing::{setup_logging, setup_logging_with_filter}; +use ruff_db::Upcast; use ruff_source_file::{LineIndex, OneIndexed}; use std::backtrace::BacktraceStatus; use std::fmt::Write; @@ -464,7 +465,7 @@ fn create_diagnostic_snapshot( writeln!(snapshot).unwrap(); } writeln!(snapshot, "```").unwrap(); - write!(snapshot, "{}", diag.display(db, &display_config)).unwrap(); + write!(snapshot, "{}", diag.display(&db.upcast(), &display_config)).unwrap(); writeln!(snapshot, "```").unwrap(); } snapshot diff --git a/crates/ty_wasm/src/lib.rs b/crates/ty_wasm/src/lib.rs index 53969603ac..9ed86299e8 100644 --- a/crates/ty_wasm/src/lib.rs +++ b/crates/ty_wasm/src/lib.rs @@ -373,7 +373,7 @@ impl Diagnostic { self.inner.primary_span().and_then(|span| { Some(Range::from_file_range( &workspace.db, - FileRange::new(span.file(), span.range()?), + FileRange::new(span.expect_ty_file(), span.range()?), workspace.position_encoding, )) }) @@ -383,7 +383,7 @@ impl Diagnostic { pub fn display(&self, workspace: &Workspace) -> JsString { let config = DisplayDiagnosticConfig::default().color(false); self.inner - .display(workspace.db.upcast(), &config) + .display(&workspace.db.upcast(), &config) .to_string() .into() }