diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index 9452fbf748..1d185f57b3 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -15,7 +15,7 @@ use red_knot_project::watch::ProjectWatcher; use red_knot_project::{watch, Db}; use red_knot_project::{ProjectDatabase, ProjectMetadata}; use red_knot_server::run_server; -use ruff_db::diagnostic::{DisplayDiagnosticConfig, OldDiagnosticTrait, Severity}; +use ruff_db::diagnostic::{Diagnostic, DisplayDiagnosticConfig, Severity}; use ruff_db::system::{OsSystem, SystemPath, SystemPathBuf}; use salsa::plumbing::ZalsaDatabase; @@ -288,7 +288,7 @@ impl MainLoop { let diagnostics_count = result.len(); for diagnostic in result { - writeln!(stdout, "{}", diagnostic.display(db, &display_config))?; + diagnostic.print(db, &display_config, &mut stdout)?; failed |= diagnostic.severity() >= min_error_severity; } @@ -359,7 +359,7 @@ enum MainLoopMessage { CheckWorkspace, CheckCompleted { /// The diagnostics that were found during the check. - result: Vec>, + result: Vec, revision: u64, }, ApplyChanges(Vec), diff --git a/crates/red_knot/tests/file_watching.rs b/crates/red_knot/tests/file_watching.rs index 7f5dd6331a..acdb7071c7 100644 --- a/crates/red_knot/tests/file_watching.rs +++ b/crates/red_knot/tests/file_watching.rs @@ -1125,11 +1125,11 @@ print(sys.last_exc, os.getegid()) assert_eq!(diagnostics.len(), 2); assert_eq!( - diagnostics[0].message(), + diagnostics[0].primary_message(), "Type `` has no attribute `last_exc`" ); assert_eq!( - diagnostics[1].message(), + diagnostics[1].primary_message(), "Type `` has no attribute `getegid`" ); diff --git a/crates/red_knot_project/src/db.rs b/crates/red_knot_project/src/db.rs index 1acbee6504..8f2b336b3f 100644 --- a/crates/red_knot_project/src/db.rs +++ b/crates/red_knot_project/src/db.rs @@ -6,7 +6,7 @@ use crate::{Project, ProjectMetadata}; use red_knot_ide::Db as IdeDb; use red_knot_python_semantic::lint::{LintRegistry, RuleSelection}; use red_knot_python_semantic::{Db as SemanticDb, Program}; -use ruff_db::diagnostic::OldDiagnosticTrait; +use ruff_db::diagnostic::Diagnostic; use ruff_db::files::{File, Files}; use ruff_db::system::System; use ruff_db::vendored::VendoredFileSystem; @@ -56,12 +56,12 @@ impl ProjectDatabase { } /// Checks all open files in the project and its dependencies. - pub fn check(&self) -> Result>, Cancelled> { + pub fn check(&self) -> Result, Cancelled> { self.with_db(|db| db.project().check(db)) } #[tracing::instrument(level = "debug", skip(self))] - pub fn check_file(&self, file: File) -> Result>, Cancelled> { + pub fn check_file(&self, file: File) -> Result, Cancelled> { self.with_db(|db| self.project().check_file(db, file)) } diff --git a/crates/red_knot_project/src/lib.rs b/crates/red_knot_project/src/lib.rs index 5ef9be3ca9..a0a7c531d7 100644 --- a/crates/red_knot_project/src/lib.rs +++ b/crates/red_knot_project/src/lib.rs @@ -9,7 +9,9 @@ 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::{DiagnosticId, OldDiagnosticTrait, OldParseDiagnostic, Severity, Span}; +use ruff_db::diagnostic::{ + create_parse_diagnostic, Annotation, Diagnostic, DiagnosticId, Severity, Span, +}; use ruff_db::files::File; use ruff_db::parsed::parsed_module; use ruff_db::source::{source_text, SourceTextError}; @@ -17,7 +19,6 @@ use ruff_db::system::{SystemPath, SystemPathBuf}; use rustc_hash::FxHashSet; use salsa::Durability; use salsa::Setter; -use std::borrow::Cow; use std::sync::Arc; use thiserror::Error; @@ -163,24 +164,27 @@ impl Project { } /// Checks all open files in the project and its dependencies. - pub(crate) fn check(self, db: &ProjectDatabase) -> Vec> { + pub(crate) fn check(self, db: &ProjectDatabase) -> Vec { let project_span = tracing::debug_span!("Project::check"); let _span = project_span.enter(); tracing::debug!("Checking project '{name}'", name = self.name(db)); - let mut diagnostics: Vec> = Vec::new(); - diagnostics.extend(self.settings_diagnostics(db).iter().map(|diagnostic| { - let diagnostic: Box = Box::new(diagnostic.clone()); - diagnostic - })); + let mut diagnostics: Vec = Vec::new(); + diagnostics.extend( + self.settings_diagnostics(db) + .iter() + .map(OptionDiagnostic::to_diagnostic), + ); let files = ProjectFiles::new(db, self); - diagnostics.extend(files.diagnostics().iter().cloned().map(|diagnostic| { - let diagnostic: Box = Box::new(diagnostic); - diagnostic - })); + diagnostics.extend( + files + .diagnostics() + .iter() + .map(IOErrorDiagnostic::to_diagnostic), + ); let result = Arc::new(std::sync::Mutex::new(diagnostics)); let inner_result = Arc::clone(&result); @@ -208,14 +212,11 @@ impl Project { Arc::into_inner(result).unwrap().into_inner().unwrap() } - pub(crate) fn check_file(self, db: &dyn Db, file: File) -> Vec> { + pub(crate) fn check_file(self, db: &dyn Db, file: File) -> Vec { let mut file_diagnostics: Vec<_> = self .settings_diagnostics(db) .iter() - .map(|diagnostic| { - let diagnostic: Box = Box::new(diagnostic.clone()); - diagnostic - }) + .map(OptionDiagnostic::to_diagnostic) .collect(); let check_diagnostics = check_file_impl(db, file); @@ -398,35 +399,36 @@ impl Project { } } -fn check_file_impl(db: &dyn Db, file: File) -> Vec> { - let mut diagnostics: Vec> = Vec::new(); +fn check_file_impl(db: &dyn Db, file: File) -> Vec { + let mut diagnostics: Vec = Vec::new(); // Abort checking if there are IO errors. let source = source_text(db.upcast(), file); if let Some(read_error) = source.read_error() { - diagnostics.push(Box::new(IOErrorDiagnostic { - file: Some(file), - error: read_error.clone().into(), - })); + diagnostics.push( + IOErrorDiagnostic { + file: Some(file), + error: read_error.clone().into(), + } + .to_diagnostic(), + ); return diagnostics; } let parsed = parsed_module(db.upcast(), file); - diagnostics.extend(parsed.errors().iter().map(|error| { - let diagnostic: Box = - Box::new(OldParseDiagnostic::new(file, error.clone())); - diagnostic - })); + diagnostics.extend( + parsed + .errors() + .iter() + .map(|error| create_parse_diagnostic(file, error)), + ); - diagnostics.extend(check_types(db.upcast(), file).iter().map(|diagnostic| { - let boxed: Box = Box::new(diagnostic.clone()); - boxed - })); + diagnostics.extend(check_types(db.upcast(), file).into_iter().cloned()); diagnostics.sort_unstable_by_key(|diagnostic| { diagnostic - .span() + .primary_span() .and_then(|span| span.range()) .unwrap_or_default() .start() @@ -494,21 +496,13 @@ pub struct IOErrorDiagnostic { error: IOErrorKind, } -impl OldDiagnosticTrait for IOErrorDiagnostic { - fn id(&self) -> DiagnosticId { - DiagnosticId::Io - } - - fn message(&self) -> Cow { - self.error.to_string().into() - } - - fn span(&self) -> Option { - self.file.map(Span::from) - } - - fn severity(&self) -> Severity { - Severity::Error +impl IOErrorDiagnostic { + fn to_diagnostic(&self) -> Diagnostic { + let mut diag = Diagnostic::new(DiagnosticId::Io, Severity::Error, &self.error); + if let Some(file) = self.file { + diag.annotate(Annotation::primary(Span::from(file))); + } + diag } } @@ -526,7 +520,6 @@ mod tests { use crate::db::tests::TestDb; use crate::{check_file_impl, ProjectMetadata}; use red_knot_python_semantic::types::check_types; - use ruff_db::diagnostic::OldDiagnosticTrait; use ruff_db::files::system_path_to_file; use ruff_db::source::source_text; use ruff_db::system::{DbWithTestSystem, DbWithWritableSystem as _, SystemPath, SystemPathBuf}; @@ -550,7 +543,7 @@ mod tests { assert_eq!( check_file_impl(&db, file) .into_iter() - .map(|diagnostic| diagnostic.message().into_owned()) + .map(|diagnostic| diagnostic.primary_message().to_string()) .collect::>(), vec!["Failed to read file: No such file or directory".to_string()] ); @@ -566,7 +559,7 @@ mod tests { assert_eq!( check_file_impl(&db, file) .into_iter() - .map(|diagnostic| diagnostic.message().into_owned()) + .map(|diagnostic| diagnostic.primary_message().to_string()) .collect::>(), vec![] as Vec ); diff --git a/crates/red_knot_project/src/metadata/options.rs b/crates/red_knot_project/src/metadata/options.rs index eaf19fe318..5506dceab1 100644 --- a/crates/red_knot_project/src/metadata/options.rs +++ b/crates/red_knot_project/src/metadata/options.rs @@ -2,14 +2,13 @@ use crate::metadata::value::{RangedValue, RelativePathBuf, ValueSource, ValueSou use crate::Db; use red_knot_python_semantic::lint::{GetLintError, Level, LintSource, RuleSelection}; use red_knot_python_semantic::{ProgramSettings, PythonPath, PythonPlatform, SearchPathSettings}; -use ruff_db::diagnostic::{DiagnosticFormat, DiagnosticId, OldDiagnosticTrait, Severity, Span}; +use ruff_db::diagnostic::{Annotation, Diagnostic, DiagnosticFormat, DiagnosticId, Severity, Span}; use ruff_db::files::system_path_to_file; use ruff_db::system::{System, SystemPath}; use ruff_macros::Combine; use ruff_python_ast::PythonVersion; use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; -use std::borrow::Cow; use std::fmt::Debug; use thiserror::Error; @@ -397,22 +396,14 @@ impl OptionDiagnostic { fn with_span(self, span: Option) -> Self { OptionDiagnostic { span, ..self } } -} -impl OldDiagnosticTrait for OptionDiagnostic { - fn id(&self) -> DiagnosticId { - self.id - } - - fn message(&self) -> Cow { - Cow::Borrowed(&self.message) - } - - fn span(&self) -> Option { - self.span.clone() - } - - fn severity(&self) -> Severity { - self.severity + pub(crate) fn to_diagnostic(&self) -> Diagnostic { + if let Some(ref span) = self.span { + let mut diag = Diagnostic::new(self.id, self.severity, ""); + diag.annotate(Annotation::primary(span.clone()).message(&self.message)); + diag + } else { + Diagnostic::new(self.id, self.severity, &self.message) + } } } diff --git a/crates/red_knot_python_semantic/src/suppression.rs b/crates/red_knot_python_semantic/src/suppression.rs index 576e66cb48..3a76a0b7c4 100644 --- a/crates/red_knot_python_semantic/src/suppression.rs +++ b/crates/red_knot_python_semantic/src/suppression.rs @@ -1,7 +1,7 @@ use crate::lint::{GetLintError, Level, LintMetadata, LintRegistry, LintStatus}; -use crate::types::{TypeCheckDiagnostic, TypeCheckDiagnostics}; +use crate::types::TypeCheckDiagnostics; use crate::{declare_lint, lint::LintId, Db}; -use ruff_db::diagnostic::DiagnosticId; +use ruff_db::diagnostic::{Annotation, Diagnostic, DiagnosticId, Span}; use ruff_db::{files::File, parsed::parsed_module, source::source_text}; use ruff_python_parser::TokenKind; use ruff_python_trivia::Cursor; @@ -319,14 +319,11 @@ impl<'a> CheckSuppressionsContext<'a> { return; }; - self.diagnostics.push(TypeCheckDiagnostic { - id: DiagnosticId::Lint(lint.name()), - message: message.to_string(), - range, - severity, - file: self.file, - secondary_messages: vec![], - }); + let id = DiagnosticId::Lint(lint.name()); + let mut diag = Diagnostic::new(id, severity, ""); + let span = Span::from(self.file).with_range(range); + diag.annotate(Annotation::primary(span).message(message)); + self.diagnostics.push(diag); } } diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index dc04d835a5..879ba7098a 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -15,7 +15,7 @@ use type_ordering::union_or_intersection_elements_ordering; pub(crate) use self::builder::{IntersectionBuilder, UnionBuilder}; pub(crate) use self::diagnostic::register_lints; -pub use self::diagnostic::{TypeCheckDiagnostic, TypeCheckDiagnostics}; +pub use self::diagnostic::TypeCheckDiagnostics; pub(crate) use self::display::TypeArrayDisplay; pub(crate) use self::infer::{ infer_deferred_types, infer_definition_types, infer_expression_type, infer_expression_types, diff --git a/crates/red_knot_python_semantic/src/types/context.rs b/crates/red_knot_python_semantic/src/types/context.rs index c5ed58dbc6..6119c86b8d 100644 --- a/crates/red_knot_python_semantic/src/types/context.rs +++ b/crates/red_knot_python_semantic/src/types/context.rs @@ -2,12 +2,14 @@ use std::fmt; use drop_bomb::DebugDropBomb; use ruff_db::{ - diagnostic::{DiagnosticId, OldSecondaryDiagnosticMessage, Severity}, + diagnostic::{ + Annotation, Diagnostic, DiagnosticId, OldSecondaryDiagnosticMessage, Severity, Span, + }, files::File, }; use ruff_text_size::{Ranged, TextRange}; -use super::{binding_type, Type, TypeCheckDiagnostic, TypeCheckDiagnostics}; +use super::{binding_type, Type, TypeCheckDiagnostics}; use crate::semantic_index::symbol::ScopeId; use crate::{ @@ -147,14 +149,13 @@ impl<'db> InferContext<'db> { // returns a rule selector for a given file that respects the package's settings, // any global pragma comments in the file, and any per-file-ignores. - self.diagnostics.borrow_mut().push(TypeCheckDiagnostic { - file: self.file, - id, - message: message.to_string(), - range: ranged.range(), - severity, - secondary_messages, - }); + let mut diag = Diagnostic::new(id, severity, ""); + for secondary_msg in &secondary_messages { + diag.sub(secondary_msg.to_sub_diagnostic()); + } + let span = Span::from(self.file).with_range(ranged.range()); + diag.annotate(Annotation::primary(span).message(message)); + self.diagnostics.borrow_mut().push(diag); } pub(super) fn set_in_no_type_check(&mut self, no_type_check: InNoTypeCheck) { diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs index 067f3d4f76..b3db81bf69 100644 --- a/crates/red_knot_python_semantic/src/types/diagnostic.rs +++ b/crates/red_knot_python_semantic/src/types/diagnostic.rs @@ -8,16 +8,11 @@ use crate::types::string_annotation::{ RAW_STRING_TYPE_ANNOTATION, }; use crate::types::{ClassLiteralType, KnownInstanceType, Type}; -use ruff_db::diagnostic::{ - DiagnosticId, OldDiagnosticTrait, OldSecondaryDiagnosticMessage, Severity, Span, -}; -use ruff_db::files::File; +use ruff_db::diagnostic::{Diagnostic, OldSecondaryDiagnosticMessage, Span}; use ruff_python_ast::{self as ast, AnyNodeRef}; -use ruff_text_size::{Ranged, TextRange}; +use ruff_text_size::Ranged; use rustc_hash::FxHashSet; -use std::borrow::Cow; use std::fmt::Formatter; -use std::sync::Arc; /// Registers all known type check lints. pub(crate) fn register_lints(registry: &mut LintRegistryBuilder) { @@ -899,52 +894,6 @@ declare_lint! { } } -#[derive(Debug, Eq, PartialEq, Clone)] -pub struct TypeCheckDiagnostic { - pub(crate) id: DiagnosticId, - pub(crate) message: String, - pub(crate) range: TextRange, - pub(crate) severity: Severity, - pub(crate) file: File, - pub(crate) secondary_messages: Vec, -} - -impl TypeCheckDiagnostic { - pub fn id(&self) -> DiagnosticId { - self.id - } - - pub fn message(&self) -> &str { - &self.message - } - - pub fn file(&self) -> File { - self.file - } -} - -impl OldDiagnosticTrait for TypeCheckDiagnostic { - fn id(&self) -> DiagnosticId { - self.id - } - - fn message(&self) -> Cow { - TypeCheckDiagnostic::message(self).into() - } - - fn span(&self) -> Option { - Some(Span::from(self.file).with_range(self.range)) - } - - fn secondary_messages(&self) -> &[OldSecondaryDiagnosticMessage] { - &self.secondary_messages - } - - fn severity(&self) -> Severity { - self.severity - } -} - /// A collection of type check diagnostics. /// /// The diagnostics are wrapped in an `Arc` because they need to be cloned multiple times @@ -954,13 +903,13 @@ impl OldDiagnosticTrait for TypeCheckDiagnostic { /// each Salsa-struct comes with an overhead. #[derive(Default, Eq, PartialEq)] pub struct TypeCheckDiagnostics { - diagnostics: Vec>, + diagnostics: Vec, used_suppressions: FxHashSet, } impl TypeCheckDiagnostics { - pub(crate) fn push(&mut self, diagnostic: TypeCheckDiagnostic) { - self.diagnostics.push(Arc::new(diagnostic)); + pub(crate) fn push(&mut self, diagnostic: Diagnostic) { + self.diagnostics.push(diagnostic); } pub(super) fn extend(&mut self, other: &TypeCheckDiagnostics) { @@ -985,7 +934,7 @@ impl TypeCheckDiagnostics { self.diagnostics.shrink_to_fit(); } - pub fn iter(&self) -> std::slice::Iter<'_, Arc> { + pub fn iter(&self) -> std::slice::Iter<'_, Diagnostic> { self.diagnostics.iter() } } @@ -997,8 +946,8 @@ impl std::fmt::Debug for TypeCheckDiagnostics { } impl IntoIterator for TypeCheckDiagnostics { - type Item = Arc; - type IntoIter = std::vec::IntoIter>; + type Item = Diagnostic; + type IntoIter = std::vec::IntoIter; fn into_iter(self) -> Self::IntoIter { self.diagnostics.into_iter() @@ -1006,8 +955,8 @@ impl IntoIterator for TypeCheckDiagnostics { } impl<'a> IntoIterator for &'a TypeCheckDiagnostics { - type Item = &'a Arc; - type IntoIter = std::slice::Iter<'a, std::sync::Arc>; + type Item = &'a Diagnostic; + type IntoIter = std::slice::Iter<'a, Diagnostic>; fn into_iter(self) -> Self::IntoIter { self.diagnostics.iter() diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 4ee4d8a55a..d121a17a81 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -7343,6 +7343,7 @@ mod tests { use crate::semantic_index::{global_scope, semantic_index, symbol_table, use_def_map}; use crate::symbol::global_symbol; use crate::types::check_types; + use ruff_db::diagnostic::Diagnostic; use ruff_db::files::{system_path_to_file, File}; use ruff_db::system::DbWithWritableSystem as _; use ruff_db::testing::{assert_function_query_was_not_run, assert_function_query_was_run}; @@ -7377,7 +7378,7 @@ mod tests { fn assert_diagnostic_messages(diagnostics: &TypeCheckDiagnostics, expected: &[&str]) { let messages: Vec<&str> = diagnostics .iter() - .map(|diagnostic| diagnostic.message()) + .map(Diagnostic::primary_message) .collect(); assert_eq!(&messages, expected); } 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 89012f259e..589a9806f3 100644 --- a/crates/red_knot_server/src/server/api/requests/diagnostic.rs +++ b/crates/red_knot_server/src/server/api/requests/diagnostic.rs @@ -72,10 +72,10 @@ fn compute_diagnostics(snapshot: &DocumentSnapshot, db: &ProjectDatabase) -> Vec fn to_lsp_diagnostic( db: &dyn Db, - diagnostic: &dyn ruff_db::diagnostic::OldDiagnosticTrait, + diagnostic: &ruff_db::diagnostic::Diagnostic, encoding: crate::PositionEncoding, ) -> Diagnostic { - let range = if let Some(span) = diagnostic.span() { + 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()); @@ -99,7 +99,7 @@ fn to_lsp_diagnostic( code: Some(NumberOrString::String(diagnostic.id().to_string())), code_description: None, source: Some("red-knot".into()), - message: diagnostic.message().into_owned(), + message: diagnostic.primary_message().to_string(), related_information: None, data: None, } diff --git a/crates/red_knot_test/Cargo.toml b/crates/red_knot_test/Cargo.toml index fee0d05141..06727647c9 100644 --- a/crates/red_knot_test/Cargo.toml +++ b/crates/red_knot_test/Cargo.toml @@ -13,7 +13,7 @@ license.workspace = true [dependencies] red_knot_python_semantic = { workspace = true, features = ["serde"] } red_knot_vendored = { workspace = true } -ruff_db = { workspace = true, features = ["testing"] } +ruff_db = { workspace = true, features = ["os", "testing"] } ruff_index = { workspace = true } ruff_notebook = { workspace = true } ruff_python_trivia = { workspace = true } diff --git a/crates/red_knot_test/src/diagnostic.rs b/crates/red_knot_test/src/diagnostic.rs index db3c2a77d2..f869f999ab 100644 --- a/crates/red_knot_test/src/diagnostic.rs +++ b/crates/red_knot_test/src/diagnostic.rs @@ -2,7 +2,7 @@ //! //! We don't assume that we will get the diagnostics in source order. -use ruff_db::diagnostic::OldDiagnosticTrait; +use ruff_db::diagnostic::Diagnostic; use ruff_source_file::{LineIndex, OneIndexed}; use std::ops::{Deref, Range}; @@ -12,21 +12,21 @@ use std::ops::{Deref, Range}; /// [`LineDiagnosticRange`] has one entry for each contiguous slice of the diagnostics vector /// containing diagnostics which all start on the same line. #[derive(Debug)] -pub(crate) struct SortedDiagnostics { - diagnostics: Vec, +pub(crate) struct SortedDiagnostics<'a> { + diagnostics: Vec<&'a Diagnostic>, line_ranges: Vec, } -impl SortedDiagnostics -where - T: OldDiagnosticTrait, -{ - pub(crate) fn new(diagnostics: impl IntoIterator, line_index: &LineIndex) -> Self { +impl<'a> SortedDiagnostics<'a> { + pub(crate) fn new( + diagnostics: impl IntoIterator, + line_index: &LineIndex, + ) -> Self { let mut diagnostics: Vec<_> = diagnostics .into_iter() .map(|diagnostic| DiagnosticWithLine { line_number: diagnostic - .span() + .primary_span() .and_then(|span| span.range()) .map_or(OneIndexed::from_zero_indexed(0), |range| { line_index.line_index(range.start()) @@ -76,7 +76,7 @@ where diags } - pub(crate) fn iter_lines(&self) -> LineDiagnosticsIterator { + pub(crate) fn iter_lines(&self) -> LineDiagnosticsIterator<'_> { LineDiagnosticsIterator { diagnostics: self.diagnostics.as_slice(), inner: self.line_ranges.iter(), @@ -92,16 +92,13 @@ struct LineDiagnosticRange { } /// Iterator to group sorted diagnostics by line. -pub(crate) struct LineDiagnosticsIterator<'a, T> { - diagnostics: &'a [T], +pub(crate) struct LineDiagnosticsIterator<'a> { + diagnostics: &'a [&'a Diagnostic], inner: std::slice::Iter<'a, LineDiagnosticRange>, } -impl<'a, T> Iterator for LineDiagnosticsIterator<'a, T> -where - T: OldDiagnosticTrait, -{ - type Item = LineDiagnostics<'a, T>; +impl<'a> Iterator for LineDiagnosticsIterator<'a> { + type Item = LineDiagnostics<'a>; fn next(&mut self) -> Option { let LineDiagnosticRange { @@ -115,20 +112,20 @@ where } } -impl std::iter::FusedIterator for LineDiagnosticsIterator<'_, T> where T: OldDiagnosticTrait {} +impl std::iter::FusedIterator for LineDiagnosticsIterator<'_> {} /// All diagnostics that start on a single line of source code in one embedded Python file. #[derive(Debug)] -pub(crate) struct LineDiagnostics<'a, T> { +pub(crate) struct LineDiagnostics<'a> { /// Line number on which these diagnostics start. pub(crate) line_number: OneIndexed, /// Diagnostics starting on this line. - pub(crate) diagnostics: &'a [T], + pub(crate) diagnostics: &'a [&'a Diagnostic], } -impl Deref for LineDiagnostics<'_, T> { - type Target = [T]; +impl<'a> Deref for LineDiagnostics<'a> { + type Target = [&'a Diagnostic]; fn deref(&self) -> &Self::Target { self.diagnostics @@ -136,22 +133,20 @@ impl Deref for LineDiagnostics<'_, T> { } #[derive(Debug)] -struct DiagnosticWithLine { +struct DiagnosticWithLine<'a> { line_number: OneIndexed, - diagnostic: T, + diagnostic: &'a Diagnostic, } #[cfg(test)] mod tests { use crate::db::Db; - use crate::diagnostic::OldDiagnosticTrait; - use ruff_db::diagnostic::{DiagnosticId, LintName, Severity, Span}; - use ruff_db::files::{system_path_to_file, File}; + use ruff_db::diagnostic::{Annotation, Diagnostic, DiagnosticId, LintName, Severity, Span}; + use ruff_db::files::system_path_to_file; use ruff_db::source::line_index; use ruff_db::system::DbWithWritableSystem as _; use ruff_source_file::OneIndexed; use ruff_text_size::{TextRange, TextSize}; - use std::borrow::Cow; #[test] fn sort_and_group() { @@ -168,10 +163,19 @@ mod tests { let diagnostics: Vec<_> = ranges .into_iter() - .map(|range| DummyDiagnostic { range, file }) + .map(|range| { + let mut diag = Diagnostic::new( + DiagnosticId::Lint(LintName::of("dummy")), + Severity::Error, + "dummy", + ); + let span = Span::from(file).with_range(range); + diag.annotate(Annotation::primary(span)); + diag + }) .collect(); - let sorted = super::SortedDiagnostics::new(diagnostics, &lines); + let sorted = super::SortedDiagnostics::new(diagnostics.iter(), &lines); let grouped = sorted.iter_lines().collect::>(); let [line1, line2] = &grouped[..] else { @@ -183,28 +187,4 @@ mod tests { assert_eq!(line2.line_number, OneIndexed::from_zero_indexed(1)); assert_eq!(line2.diagnostics.len(), 1); } - - #[derive(Debug)] - struct DummyDiagnostic { - range: TextRange, - file: File, - } - - impl OldDiagnosticTrait for DummyDiagnostic { - fn id(&self) -> DiagnosticId { - DiagnosticId::Lint(LintName::of("dummy")) - } - - fn message(&self) -> Cow { - "dummy".into() - } - - fn span(&self) -> Option { - Some(Span::from(self.file).with_range(self.range)) - } - - fn severity(&self) -> Severity { - Severity::Error - } - } } diff --git a/crates/red_knot_test/src/lib.rs b/crates/red_knot_test/src/lib.rs index ec95d14f0e..c1f1128271 100644 --- a/crates/red_knot_test/src/lib.rs +++ b/crates/red_knot_test/src/lib.rs @@ -6,14 +6,15 @@ use config::SystemKind; use parser as test_parser; use red_knot_python_semantic::types::check_types; use red_knot_python_semantic::{Program, ProgramSettings, PythonPath, SearchPathSettings}; -use ruff_db::diagnostic::{DisplayDiagnosticConfig, OldDiagnosticTrait, OldParseDiagnostic}; +use ruff_db::diagnostic::{create_parse_diagnostic, Diagnostic, DisplayDiagnosticConfig}; use ruff_db::files::{system_path_to_file, File}; 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_source_file::{LineIndex, OneIndexed}; -use std::fmt::Write; +use std::fmt::Write as _; +use std::io::Write as _; mod assertion; mod config; @@ -220,15 +221,10 @@ fn run_test( .filter_map(|test_file| { let parsed = parsed_module(db, test_file.file); - let mut diagnostics: Vec> = parsed + let mut diagnostics: Vec = parsed .errors() .iter() - .cloned() - .map(|error| { - let diagnostic: Box = - Box::new(OldParseDiagnostic::new(test_file.file, error)); - diagnostic - }) + .map(|error| create_parse_diagnostic(test_file.file, error)) .collect(); let type_diagnostics = match catch_unwind(|| check_types(db, test_file.file)) { @@ -258,19 +254,15 @@ fn run_test( }); } }; - diagnostics.extend(type_diagnostics.into_iter().map(|diagnostic| { - let diagnostic: Box = Box::new((*diagnostic).clone()); - diagnostic - })); + diagnostics.extend(type_diagnostics.into_iter().cloned()); - let failure = - match matcher::match_file(db, test_file.file, diagnostics.iter().map(|d| &**d)) { - Ok(()) => None, - Err(line_failures) => Some(FileFailures { - backtick_offsets: test_file.backtick_offsets, - by_line: line_failures, - }), - }; + let failure = match matcher::match_file(db, test_file.file, &diagnostics) { + Ok(()) => None, + Err(line_failures) => Some(FileFailures { + backtick_offsets: test_file.backtick_offsets, + by_line: line_failures, + }), + }; if test.should_snapshot_diagnostics() { snapshot_diagnostics.extend(diagnostics); } @@ -324,15 +316,15 @@ struct TestFile { backtick_offsets: Vec, } -fn create_diagnostic_snapshot( +fn create_diagnostic_snapshot( db: &mut db::Db, relative_fixture_path: &Utf8Path, test: &parser::MarkdownTest, - diagnostics: impl IntoIterator, + diagnostics: impl IntoIterator, ) -> String { let display_config = DisplayDiagnosticConfig::default().color(false); - let mut snapshot = String::new(); + let mut snapshot = Vec::new(); writeln!(snapshot).unwrap(); writeln!(snapshot, "---").unwrap(); writeln!(snapshot, "mdtest name: {}", test.name()).unwrap(); @@ -368,8 +360,8 @@ fn create_diagnostic_snapshot( writeln!(snapshot).unwrap(); } writeln!(snapshot, "```").unwrap(); - writeln!(snapshot, "{}", diag.display(db, &display_config)).unwrap(); + diag.print(db, &display_config, &mut snapshot).unwrap(); writeln!(snapshot, "```").unwrap(); } - snapshot + String::from_utf8(snapshot).unwrap() } diff --git a/crates/red_knot_test/src/matcher.rs b/crates/red_knot_test/src/matcher.rs index c38efefc4e..19184cecfe 100644 --- a/crates/red_knot_test/src/matcher.rs +++ b/crates/red_knot_test/src/matcher.rs @@ -1,10 +1,10 @@ -//! Match [`OldDiagnosticTrait`]s against assertions and produce test failure +//! Match [`Diagnostic`]s against assertions and produce test failure //! messages for any mismatches. use crate::assertion::{InlineFileAssertions, ParsedAssertion, UnparsedAssertion}; use crate::db::Db; use crate::diagnostic::SortedDiagnostics; use colored::Colorize; -use ruff_db::diagnostic::{DiagnosticAsStrError, DiagnosticId, OldDiagnosticTrait}; +use ruff_db::diagnostic::{Diagnostic, DiagnosticAsStrError, DiagnosticId}; use ruff_db::files::File; use ruff_db::source::{line_index, source_text, SourceText}; use ruff_source_file::{LineIndex, OneIndexed}; @@ -47,14 +47,11 @@ struct LineFailures { range: Range, } -pub(super) fn match_file( +pub(super) fn match_file( db: &Db, file: File, - diagnostics: impl IntoIterator, -) -> Result<(), FailuresByLine> -where - T: OldDiagnosticTrait, -{ + diagnostics: &[Diagnostic], +) -> Result<(), FailuresByLine> { // Parse assertions from comments in the file, and get diagnostics from the file; both // ordered by line number. let assertions = InlineFileAssertions::from_file(db, file); @@ -155,8 +152,8 @@ impl Unmatched for ParsedAssertion<'_> { } } -fn maybe_add_undefined_reveal_clarification( - diagnostic: &T, +fn maybe_add_undefined_reveal_clarification( + diagnostic: &Diagnostic, original: std::fmt::Arguments, ) -> String { if diagnostic.id().is_lint_named("undefined-reveal") { @@ -169,10 +166,7 @@ fn maybe_add_undefined_reveal_clarification( } } -impl Unmatched for T -where - T: OldDiagnosticTrait, -{ +impl Unmatched for &Diagnostic { fn unmatched(&self) -> String { let id = self.id(); let id = id.as_str().unwrap_or_else(|error| match error { @@ -181,15 +175,12 @@ where maybe_add_undefined_reveal_clarification( self, - format_args!(r#"[{id}] "{message}""#, message = self.message()), + format_args!(r#"[{id}] "{message}""#, message = self.primary_message()), ) } } -impl UnmatchedWithColumn for T -where - T: OldDiagnosticTrait, -{ +impl UnmatchedWithColumn for &Diagnostic { fn unmatched_with_column(&self, column: OneIndexed) -> String { let id = self.id(); let id = id.as_str().unwrap_or_else(|error| match error { @@ -198,7 +189,10 @@ where maybe_add_undefined_reveal_clarification( self, - format_args!(r#"{column} [{id}] "{message}""#, message = self.message()), + format_args!( + r#"{column} [{id}] "{message}""#, + message = self.primary_message() + ), ) } } @@ -226,21 +220,21 @@ impl Matcher { } } - /// Check a slice of [`OldDiagnosticTrait`]s against a slice of + /// Check a slice of [`Diagnostic`]s against a slice of /// [`UnparsedAssertion`]s. /// /// Return vector of [`Unmatched`] for any unmatched diagnostics or /// assertions. - fn match_line<'a, 'b, T: OldDiagnosticTrait + 'a>( + fn match_line<'a, 'b>( &self, - diagnostics: &'a [T], + diagnostics: &'a [&'a Diagnostic], assertions: &'a [UnparsedAssertion<'b>], ) -> Result<(), Vec> where 'b: 'a, { let mut failures = vec![]; - let mut unmatched: Vec<_> = diagnostics.iter().collect(); + let mut unmatched = diagnostics.to_vec(); for assertion in assertions { match assertion.parse() { Ok(assertion) => { @@ -263,9 +257,9 @@ impl Matcher { } } - fn column(&self, diagnostic: &T) -> OneIndexed { + fn column(&self, diagnostic: &Diagnostic) -> OneIndexed { diagnostic - .span() + .primary_span() .and_then(|span| span.range()) .map(|range| { self.line_index @@ -275,7 +269,7 @@ impl Matcher { .unwrap_or(OneIndexed::from_zero_indexed(0)) } - /// Check if `assertion` matches any [`OldDiagnosticTrait`]s in `unmatched`. + /// Check if `assertion` matches any [`Diagnostic`]s in `unmatched`. /// /// If so, return `true` and remove the matched diagnostics from `unmatched`. Otherwise, return /// `false`. @@ -285,11 +279,7 @@ impl Matcher { /// /// A `Revealed` assertion must match a revealed-type diagnostic, and may also match an /// undefined-reveal diagnostic, if present. - fn matches( - &self, - assertion: &ParsedAssertion, - unmatched: &mut Vec<&T>, - ) -> bool { + fn matches(&self, assertion: &ParsedAssertion, unmatched: &mut Vec<&Diagnostic>) -> bool { match assertion { ParsedAssertion::Error(error) => { let position = unmatched.iter().position(|diagnostic| { @@ -297,10 +287,10 @@ impl Matcher { !(diagnostic.id().is_lint_named(rule) || diagnostic.id().matches(rule)) }) && error .column - .is_none_or(|col| col == self.column(*diagnostic)) + .is_none_or(|col| col == self.column(diagnostic)) && error .message_contains - .is_none_or(|needle| diagnostic.message().contains(needle)) + .is_none_or(|needle| diagnostic.primary_message().contains(needle)) }); if let Some(position) = position { unmatched.swap_remove(position); @@ -319,7 +309,7 @@ impl Matcher { for (index, diagnostic) in unmatched.iter().enumerate() { if matched_revealed_type.is_none() && diagnostic.id() == DiagnosticId::RevealedType - && diagnostic.message() == expected_reveal_type_message + && diagnostic.primary_message() == expected_reveal_type_message { matched_revealed_type = Some(index); } else if matched_undefined_reveal.is_none() @@ -347,13 +337,12 @@ impl Matcher { #[cfg(test)] mod tests { use super::FailuresByLine; - use ruff_db::diagnostic::{DiagnosticId, OldDiagnosticTrait, Severity, Span}; + use ruff_db::diagnostic::{Annotation, Diagnostic, DiagnosticId, Severity, Span}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::system::DbWithWritableSystem as _; use ruff_python_trivia::textwrap::dedent; use ruff_source_file::OneIndexed; use ruff_text_size::TextRange; - use std::borrow::Cow; struct ExpectedDiagnostic { id: DiagnosticId, @@ -371,45 +360,17 @@ mod tests { } } - fn into_diagnostic(self, file: File) -> TestDiagnostic { - TestDiagnostic { - id: self.id, - message: self.message, - range: self.range, - file, - } - } - } - - #[derive(Debug)] - struct TestDiagnostic { - id: DiagnosticId, - message: &'static str, - range: TextRange, - file: File, - } - - impl OldDiagnosticTrait for TestDiagnostic { - fn id(&self) -> DiagnosticId { - self.id - } - - fn message(&self) -> Cow { - self.message.into() - } - - fn span(&self) -> Option { - Some(Span::from(self.file).with_range(self.range)) - } - - fn severity(&self) -> Severity { - Severity::Error + fn into_diagnostic(self, file: File) -> Diagnostic { + let mut diag = Diagnostic::new(self.id, Severity::Error, ""); + let span = Span::from(file).with_range(self.range); + diag.annotate(Annotation::primary(span).message(self.message)); + diag } } fn get_result( source: &str, - diagnostics: Vec, + expected_diagnostics: Vec, ) -> Result<(), FailuresByLine> { colored::control::set_override(false); @@ -417,13 +378,11 @@ mod tests { db.write_file("/src/test.py", source).unwrap(); let file = system_path_to_file(&db, "/src/test.py").unwrap(); - super::match_file( - &db, - file, - diagnostics - .into_iter() - .map(|diagnostic| diagnostic.into_diagnostic(file)), - ) + let diagnostics: Vec = expected_diagnostics + .into_iter() + .map(|diagnostic| diagnostic.into_diagnostic(file)) + .collect(); + super::match_file(&db, file, &diagnostics) } fn assert_fail(result: Result<(), FailuresByLine>, messages: &[(usize, &[&str])]) { diff --git a/crates/red_knot_wasm/src/lib.rs b/crates/red_knot_wasm/src/lib.rs index ab7a5476cc..1a179f9b46 100644 --- a/crates/red_knot_wasm/src/lib.rs +++ b/crates/red_knot_wasm/src/lib.rs @@ -7,7 +7,7 @@ use red_knot_project::watch::{ChangeEvent, ChangedKind, CreatedKind, DeletedKind use red_knot_project::ProjectMetadata; use red_knot_project::{Db, ProjectDatabase}; use red_knot_python_semantic::Program; -use ruff_db::diagnostic::{DisplayDiagnosticConfig, OldDiagnosticTrait}; +use ruff_db::diagnostic::{self, DisplayDiagnosticConfig}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::source::{line_index, source_text}; use ruff_db::system::walk_directory::WalkDirectoryBuilder; @@ -223,18 +223,18 @@ impl FileHandle { #[wasm_bindgen] pub struct Diagnostic { #[wasm_bindgen(readonly)] - inner: Box, + inner: diagnostic::Diagnostic, } #[wasm_bindgen] impl Diagnostic { - fn wrap(diagnostic: Box) -> Self { + fn wrap(diagnostic: diagnostic::Diagnostic) -> Self { Self { inner: diagnostic } } #[wasm_bindgen] pub fn message(&self) -> JsString { - JsString::from(&*self.inner.message()) + JsString::from(self.inner.primary_message()) } #[wasm_bindgen] @@ -250,13 +250,13 @@ impl Diagnostic { #[wasm_bindgen(js_name = "textRange")] pub fn text_range(&self) -> Option { self.inner - .span() + .primary_span() .and_then(|span| Some(TextRange::from(span.range()?))) } #[wasm_bindgen(js_name = "toRange")] pub fn to_range(&self, workspace: &Workspace) -> Option { - self.inner.span().and_then(|span| { + self.inner.primary_span().and_then(|span| { let line_index = line_index(workspace.db.upcast(), span.file()); let source = source_text(workspace.db.upcast(), span.file()); let text_range = span.range()?; @@ -273,10 +273,11 @@ impl Diagnostic { #[wasm_bindgen] pub fn display(&self, workspace: &Workspace) -> JsString { let config = DisplayDiagnosticConfig::default().color(false); + let mut buf = vec![]; self.inner - .display(workspace.db.upcast(), &config) - .to_string() - .into() + .print(workspace.db.upcast(), &config, &mut buf) + .unwrap(); + String::from_utf8(buf).unwrap().into() } } diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 73051e49c2..c8532316f6 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -1,7 +1,6 @@ #![allow(clippy::disallowed_names)] use ruff_benchmark::criterion; -use std::borrow::Cow; use std::ops::Range; use criterion::{criterion_group, criterion_main, BatchSize, Criterion}; @@ -13,7 +12,7 @@ use red_knot_project::metadata::value::RangedValue; use red_knot_project::watch::{ChangeEvent, ChangedKind}; use red_knot_project::{Db, ProjectDatabase, ProjectMetadata}; use ruff_benchmark::TestFile; -use ruff_db::diagnostic::{DiagnosticId, OldDiagnosticTrait, Severity}; +use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::source::source_text; use ruff_db::system::{MemoryFileSystem, SystemPath, SystemPathBuf, TestSystem}; @@ -56,7 +55,7 @@ type KeyDiagnosticFields = ( DiagnosticId, Option<&'static str>, Option>, - Cow<'static, str>, + &'static str, Severity, ); @@ -64,7 +63,7 @@ static EXPECTED_DIAGNOSTICS: &[KeyDiagnosticFields] = &[( DiagnosticId::lint("unused-ignore-comment"), Some("/src/tomllib/_parser.py"), Some(22299..22333), - Cow::Borrowed("Unused blanket `type: ignore` directive"), + "Unused blanket `type: ignore` directive", Severity::Warning, )]; @@ -193,21 +192,21 @@ fn benchmark_cold(criterion: &mut Criterion) { } #[track_caller] -fn assert_diagnostics(db: &dyn Db, diagnostics: &[Box]) { +fn assert_diagnostics(db: &dyn Db, diagnostics: &[Diagnostic]) { let normalized: Vec<_> = diagnostics .iter() .map(|diagnostic| { ( diagnostic.id(), diagnostic - .span() + .primary_span() .map(|span| span.file()) .map(|file| file.path(db).as_str()), diagnostic - .span() + .primary_span() .and_then(|span| span.range()) .map(Range::::from), - diagnostic.message(), + diagnostic.primary_message(), diagnostic.severity(), ) }) diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index 56d5445aca..4677933391 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -119,6 +119,54 @@ impl Diagnostic { let display = DisplayDiagnostic::new(&resolver, config, self); writeln!(wtr, "{display}") } + + /// Returns the identifier for this diagnostic. + pub fn id(&self) -> DiagnosticId { + self.inner.id + } + + /// Returns the primary message for this diagnostic. + /// + /// A diagnostic always has a message, but it may be empty. + pub fn primary_message(&self) -> &str { + if !self.inner.message.is_empty() { + return &self.inner.message; + } + // FIXME: As a special case, while we're migrating Red Knot + // to the new diagnostic data model, we'll look for a primary + // message from the primary annotation. This is because most + // Red Knot diagnostics are created with an empty diagnostic + // message and instead attach the message to the annotation. + // Fixing this will require touching basically every diagnostic + // in Red Knot, so we do it this way for now to match the old + // semantics. ---AG + self.primary_annotation() + .and_then(|ann| ann.message.as_deref()) + .unwrap_or_default() + } + + /// Returns the severity of this diagnostic. + /// + /// Note that this may be different than the severity of sub-diagnostics. + pub fn severity(&self) -> Severity { + self.inner.severity + } + + /// Returns the "primary" annotation of this diagnostic if one exists. + /// + /// When there are multiple primary annotation, then the first one that was + /// added to this diagnostic is returned. + pub fn primary_annotation(&self) -> Option<&Annotation> { + self.inner.annotations.iter().find(|ann| ann.is_primary) + } + + /// Returns the "primary" span of this diagnostic if one exists. + /// + /// When there are multiple primary spans, then the first one that was + /// added to this diagnostic is returned. + pub fn primary_span(&self) -> Option { + self.primary_annotation().map(|ann| ann.span.clone()) + } } #[derive(Debug, Clone, Eq, PartialEq)] @@ -564,3 +612,16 @@ pub enum DiagnosticFormat { /// This may use color when printing to a `tty`. Concise, } + +/// Creates a `Diagnostic` from a parse error. +/// +/// This should _probably_ be a method on `ruff_python_parser::ParseError`, but +/// at time of writing, `ruff_db` depends on `ruff_python_parser` instead of +/// the other way around. And since we want to do this conversion in a couple +/// places, it makes sense to centralize it _somewhere_. So it's here for now. +pub fn create_parse_diagnostic(file: File, err: &ruff_python_parser::ParseError) -> Diagnostic { + let mut diag = Diagnostic::new(DiagnosticId::InvalidSyntax, Severity::Error, ""); + let span = Span::from(file).with_range(err.location); + diag.annotate(Annotation::primary(span).message(&err.error)); + diag +}