From 35f3815087071f26901c3914142eee57557fb10e Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 3 Dec 2024 18:21:42 +0100 Subject: [PATCH] Use salsa accumulators for diagnostics --- Cargo.lock | 9 +- Cargo.toml | 2 +- crates/red_knot/src/main.rs | 6 +- crates/red_knot_python_semantic/src/db.rs | 1 + .../src/semantic_index/builder.rs | 27 +- .../src/semantic_index/definition.rs | 9 - crates/red_knot_python_semantic/src/types.rs | 56 ++- .../src/types/diagnostic.rs | 422 ++++++++---------- .../src/types/infer.rs | 313 +++++++------ .../src/types/string_annotation.rs | 23 +- .../src/types/unpacker.rs | 20 +- crates/red_knot_server/src/server/api.rs | 6 +- crates/red_knot_test/src/db.rs | 1 + crates/red_knot_test/src/lib.rs | 24 +- crates/red_knot_wasm/src/lib.rs | 1 - crates/red_knot_workspace/Cargo.toml | 1 - crates/red_knot_workspace/src/db.rs | 22 +- crates/red_knot_workspace/src/workspace.rs | 176 ++++---- crates/ruff/src/commands/analyze_graph.rs | 4 +- crates/ruff_benchmark/benches/red_knot.rs | 4 +- crates/ruff_db/Cargo.toml | 1 - crates/ruff_db/src/diagnostic.rs | 42 ++ crates/ruff_db/src/files.rs | 9 +- crates/ruff_db/src/lib.rs | 2 +- crates/ruff_db/src/parsed.rs | 9 +- crates/ruff_db/src/source.rs | 84 +++- crates/ruff_graph/src/db.rs | 12 +- 27 files changed, 668 insertions(+), 618 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 105f961029ea2..fb7092fdafdbf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2403,7 +2403,6 @@ dependencies = [ "ruff_cache", "ruff_db", "ruff_python_ast", - "ruff_text_size", "rustc-hash 2.1.0", "salsa", "serde", @@ -2630,7 +2629,6 @@ dependencies = [ "salsa", "serde", "tempfile", - "thiserror 2.0.3", "tracing", "tracing-subscriber", "tracing-tree", @@ -3189,7 +3187,7 @@ checksum = "e86697c916019a8588c99b5fac3cead74ec0b4b819707a682fd4d23fa0ce1ba1" [[package]] name = "salsa" version = "0.18.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=254c749b02cde2fd29852a7463a33e800b771758#254c749b02cde2fd29852a7463a33e800b771758" +source = "git+https://github.com/salsa-rs/salsa.git?rev=e68679b3a9c2b5cfd8eab92de89edf4073b03601#e68679b3a9c2b5cfd8eab92de89edf4073b03601" dependencies = [ "append-only-vec", "arc-swap", @@ -3199,6 +3197,7 @@ dependencies = [ "indexmap", "lazy_static", "parking_lot", + "rayon", "rustc-hash 2.1.0", "salsa-macro-rules", "salsa-macros", @@ -3209,12 +3208,12 @@ dependencies = [ [[package]] name = "salsa-macro-rules" version = "0.1.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=254c749b02cde2fd29852a7463a33e800b771758#254c749b02cde2fd29852a7463a33e800b771758" +source = "git+https://github.com/salsa-rs/salsa.git?rev=e68679b3a9c2b5cfd8eab92de89edf4073b03601#e68679b3a9c2b5cfd8eab92de89edf4073b03601" [[package]] name = "salsa-macros" version = "0.18.0" -source = "git+https://github.com/salsa-rs/salsa.git?rev=254c749b02cde2fd29852a7463a33e800b771758#254c749b02cde2fd29852a7463a33e800b771758" +source = "git+https://github.com/salsa-rs/salsa.git?rev=e68679b3a9c2b5cfd8eab92de89edf4073b03601#e68679b3a9c2b5cfd8eab92de89edf4073b03601" dependencies = [ "heck", "proc-macro2", diff --git a/Cargo.toml b/Cargo.toml index 8931a9a4fb211..eee179803b155 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -118,7 +118,7 @@ rand = { version = "0.8.5" } rayon = { version = "1.10.0" } regex = { version = "1.10.2" } rustc-hash = { version = "2.0.0" } -salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "254c749b02cde2fd29852a7463a33e800b771758" } +salsa = { git = "https://github.com/salsa-rs/salsa.git", rev = "e68679b3a9c2b5cfd8eab92de89edf4073b03601" } schemars = { version = "0.8.16" } seahash = { version = "4.1.0" } serde = { version = "1.0.197", features = ["derive"] } diff --git a/crates/red_knot/src/main.rs b/crates/red_knot/src/main.rs index 173652746cb34..6efd0411b78f1 100644 --- a/crates/red_knot/src/main.rs +++ b/crates/red_knot/src/main.rs @@ -12,7 +12,7 @@ use red_knot_workspace::watch; use red_knot_workspace::watch::WorkspaceWatcher; use red_knot_workspace::workspace::settings::Configuration; use red_knot_workspace::workspace::WorkspaceMetadata; -use ruff_db::diagnostic::Diagnostic; +use ruff_db::diagnostic::CompileDiagnostic; use ruff_db::system::{OsSystem, System, SystemPath, SystemPathBuf}; use salsa::plumbing::ZalsaDatabase; use target_version::TargetVersion; @@ -297,7 +297,7 @@ impl MainLoop { while let Ok(message) = self.receiver.recv() { match message { MainLoopMessage::CheckWorkspace => { - let db = db.snapshot(); + let db = db.clone(); let sender = self.sender.clone(); // Spawn a new task that checks the workspace. This needs to be done in a separate thread @@ -380,7 +380,7 @@ impl MainLoopCancellationToken { enum MainLoopMessage { CheckWorkspace, CheckCompleted { - result: Vec>, + result: Vec, revision: u64, }, ApplyChanges(Vec), diff --git a/crates/red_knot_python_semantic/src/db.rs b/crates/red_knot_python_semantic/src/db.rs index 92f8619babc85..d46a9991f3566 100644 --- a/crates/red_knot_python_semantic/src/db.rs +++ b/crates/red_knot_python_semantic/src/db.rs @@ -19,6 +19,7 @@ pub(crate) mod tests { use super::Db; #[salsa::db] + #[derive(Clone)] pub(crate) struct TestDb { storage: salsa::Storage, files: Files, diff --git a/crates/red_knot_python_semantic/src/semantic_index/builder.rs b/crates/red_knot_python_semantic/src/semantic_index/builder.rs index 68258c9759153..aef69fa7b861e 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -314,10 +314,6 @@ impl<'db> SemanticIndexBuilder<'db> { self.current_assignments.last().copied() } - fn current_assignment_mut(&mut self) -> Option<&mut CurrentAssignment<'db>> { - self.current_assignments.last_mut() - } - fn add_pattern_constraint( &mut self, subject: &ast::Expr, @@ -686,7 +682,7 @@ where ast::Expr::List(_) | ast::Expr::Tuple(_) => { Some(CurrentAssignment::Assign { node, - first: true, + unpack: Some(Unpack::new( self.db, self.file, @@ -700,11 +696,9 @@ where )), }) } - ast::Expr::Name(_) => Some(CurrentAssignment::Assign { - node, - unpack: None, - first: false, - }), + ast::Expr::Name(_) => { + Some(CurrentAssignment::Assign { node, unpack: None }) + } _ => None, }; @@ -1082,18 +1076,13 @@ where if is_definition { match self.current_assignment() { - Some(CurrentAssignment::Assign { - node, - first, - unpack, - }) => { + Some(CurrentAssignment::Assign { node, unpack }) => { self.add_definition( symbol, AssignmentDefinitionNodeRef { unpack, value: &node.value, name: name_node, - first, }, ); } @@ -1144,11 +1133,6 @@ where } } - if let Some(CurrentAssignment::Assign { first, .. }) = self.current_assignment_mut() - { - *first = false; - } - walk_expr(self, expr); } ast::Expr::Named(node) => { @@ -1355,7 +1339,6 @@ where enum CurrentAssignment<'a> { Assign { node: &'a ast::StmtAssign, - first: bool, unpack: Option>, }, AnnAssign(&'a ast::StmtAnnAssign), diff --git a/crates/red_knot_python_semantic/src/semantic_index/definition.rs b/crates/red_knot_python_semantic/src/semantic_index/definition.rs index 82f8cf7557199..c5651839b0a9c 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/definition.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/definition.rs @@ -211,7 +211,6 @@ pub(crate) struct AssignmentDefinitionNodeRef<'a> { pub(crate) unpack: Option>, pub(crate) value: &'a ast::Expr, pub(crate) name: &'a ast::ExprName, - pub(crate) first: bool, } #[derive(Copy, Clone, Debug)] @@ -282,12 +281,10 @@ impl<'db> DefinitionNodeRef<'db> { unpack, value, name, - first, }) => DefinitionKind::Assignment(AssignmentDefinitionKind { target: TargetKind::from(unpack), value: AstNodeRef::new(parsed.clone(), value), name: AstNodeRef::new(parsed, name), - first, }), DefinitionNodeRef::AnnotatedAssignment(assign) => { DefinitionKind::AnnotatedAssignment(AstNodeRef::new(parsed, assign)) @@ -374,7 +371,6 @@ impl<'db> DefinitionNodeRef<'db> { value: _, unpack: _, name, - first: _, }) => name.into(), Self::AnnotatedAssignment(node) => node.into(), Self::AugmentedAssignment(node) => node.into(), @@ -591,7 +587,6 @@ pub struct AssignmentDefinitionKind<'db> { target: TargetKind<'db>, value: AstNodeRef, name: AstNodeRef, - first: bool, } impl<'db> AssignmentDefinitionKind<'db> { @@ -606,10 +601,6 @@ impl<'db> AssignmentDefinitionKind<'db> { pub(crate) fn name(&self) -> &ast::ExprName { self.name.node() } - - pub(crate) fn is_first(&self) -> bool { - self.first - } } #[derive(Clone, Debug)] diff --git a/crates/red_knot_python_semantic/src/types.rs b/crates/red_knot_python_semantic/src/types.rs index c3cb22bda42af..a3af5c05fd4d6 100644 --- a/crates/red_knot_python_semantic/src/types.rs +++ b/crates/red_knot_python_semantic/src/types.rs @@ -7,7 +7,6 @@ use ruff_db::files::File; use ruff_python_ast as ast; pub(crate) use self::builder::{IntersectionBuilder, UnionBuilder}; -pub use self::diagnostic::{TypeCheckDiagnostic, TypeCheckDiagnostics}; pub(crate) use self::display::TypeArrayDisplay; pub(crate) use self::infer::{ infer_deferred_types, infer_definition_types, infer_expression_types, infer_scope_types, @@ -25,7 +24,9 @@ use crate::stdlib::{ builtins_symbol, core_module_symbol, typing_extensions_symbol, CoreStdlibModule, }; use crate::symbol::{Boundness, Symbol}; -use crate::types::diagnostic::TypeCheckDiagnosticsBuilder; +use crate::types::diagnostic::{ + report_not_iterable, report_not_iterable_possibly_unbound, report_type_diagnostic, +}; use crate::types::mro::{ClassBase, Mro, MroError, MroIterator}; use crate::types::narrow::narrowing_constraint; use crate::{Db, FxOrderSet, Module, Program}; @@ -43,21 +44,17 @@ mod unpacker; #[cfg(test)] mod property_tests; -#[salsa::tracked(return_ref)] -pub fn check_types(db: &dyn Db, file: File) -> TypeCheckDiagnostics { +#[salsa::tracked] +pub fn check_types(db: &dyn Db, file: File) { let _span = tracing::trace_span!("check_types", file=?file.path(db)).entered(); tracing::debug!("Checking file '{path}'", path = file.path(db)); let index = semantic_index(db, file); - let mut diagnostics = TypeCheckDiagnostics::default(); for scope_id in index.scope_ids() { - let result = infer_scope_types(db, scope_id); - diagnostics.extend(result.diagnostics()); + let _ = infer_scope_types(db, scope_id); } - - diagnostics } /// Infer the public type of a symbol (its type as seen from outside its scope). @@ -2131,19 +2128,21 @@ impl<'db> CallOutcome<'db> { } /// Get the return type of the call, emitting default diagnostics if needed. - fn unwrap_with_diagnostic<'a>( + fn unwrap_with_diagnostic( &self, db: &'db dyn Db, + file: File, node: ast::AnyNodeRef, - diagnostics: &'a mut TypeCheckDiagnosticsBuilder<'db>, ) -> Type<'db> { - match self.return_ty_result(db, node, diagnostics) { + match self.return_ty_result(db, file, node) { Ok(return_ty) => return_ty, Err(NotCallableError::Type { not_callable_ty, return_ty, }) => { - diagnostics.add( + report_type_diagnostic( + db, + file, node, "call-non-callable", format_args!( @@ -2158,7 +2157,9 @@ impl<'db> CallOutcome<'db> { called_ty, return_ty, }) => { - diagnostics.add( + report_type_diagnostic( + db, + file, node, "call-non-callable", format_args!( @@ -2174,7 +2175,9 @@ impl<'db> CallOutcome<'db> { called_ty, return_ty, }) => { - diagnostics.add( + report_type_diagnostic( + db, + file, node, "call-non-callable", format_args!( @@ -2189,7 +2192,9 @@ impl<'db> CallOutcome<'db> { callable_ty: called_ty, return_ty, }) => { - diagnostics.add( + report_type_diagnostic( + db, + file, node, "call-non-callable", format_args!( @@ -2203,11 +2208,11 @@ impl<'db> CallOutcome<'db> { } /// Get the return type of the call as a result. - fn return_ty_result<'a>( + fn return_ty_result( &self, db: &'db dyn Db, + file: File, node: ast::AnyNodeRef, - diagnostics: &'a mut TypeCheckDiagnosticsBuilder<'db>, ) -> Result, NotCallableError<'db>> { match self { Self::Callable { return_ty } => Ok(*return_ty), @@ -2215,7 +2220,9 @@ impl<'db> CallOutcome<'db> { return_ty, revealed_ty, } => { - diagnostics.add( + report_type_diagnostic( + db, + file, node, "revealed-type", format_args!("Revealed type is `{}`", revealed_ty.display(db)), @@ -2254,10 +2261,10 @@ impl<'db> CallOutcome<'db> { *return_ty } else { revealed = true; - outcome.unwrap_with_diagnostic(db, node, diagnostics) + outcome.unwrap_with_diagnostic(db, file, node) } } - _ => outcome.unwrap_with_diagnostic(db, node, diagnostics), + _ => outcome.unwrap_with_diagnostic(db, file, node), }; union_builder = union_builder.add(return_ty); } @@ -2372,20 +2379,21 @@ enum IterationOutcome<'db> { impl<'db> IterationOutcome<'db> { fn unwrap_with_diagnostic( self, + db: &dyn Db, + file: File, iterable_node: ast::AnyNodeRef, - diagnostics: &mut TypeCheckDiagnosticsBuilder<'db>, ) -> Type<'db> { match self { Self::Iterable { element_ty } => element_ty, Self::NotIterable { not_iterable_ty } => { - diagnostics.add_not_iterable(iterable_node, not_iterable_ty); + report_not_iterable(db, file, iterable_node, not_iterable_ty); Type::Unknown } Self::PossiblyUnboundDunderIter { iterable_ty, element_ty, } => { - diagnostics.add_not_iterable_possibly_unbound(iterable_node, iterable_ty); + report_not_iterable_possibly_unbound(db, file, iterable_node, iterable_ty); element_ty } } diff --git a/crates/red_knot_python_semantic/src/types/diagnostic.rs b/crates/red_knot_python_semantic/src/types/diagnostic.rs index 36b87247b9926..84db3c4a59b5d 100644 --- a/crates/red_knot_python_semantic/src/types/diagnostic.rs +++ b/crates/red_knot_python_semantic/src/types/diagnostic.rs @@ -1,16 +1,12 @@ use crate::types::{ClassLiteralType, Type}; use crate::Db; -use ruff_db::diagnostic::{Diagnostic, Severity}; +use ruff_db::diagnostic::{CompileDiagnostic, Diagnostic, Severity}; use ruff_db::files::File; use ruff_python_ast::{self as ast, AnyNodeRef}; use ruff_text_size::{Ranged, TextRange}; use std::borrow::Cow; -use std::fmt::Formatter; -use std::ops::Deref; -use std::sync::Arc; - #[derive(Debug, Eq, PartialEq, Clone)] -pub struct TypeCheckDiagnostic { +pub(crate) struct TypeCheckDiagnostic { // TODO: Don't use string keys for rules pub(super) rule: String, pub(super) message: String, @@ -19,15 +15,15 @@ pub struct TypeCheckDiagnostic { } impl TypeCheckDiagnostic { - pub fn rule(&self) -> &str { + pub(crate) fn rule(&self) -> &str { &self.rule } - pub fn message(&self) -> &str { + pub(crate) fn message(&self) -> &str { &self.message } - pub fn file(&self) -> File { + pub(crate) fn file(&self) -> File { self.file } } @@ -60,263 +56,209 @@ impl Ranged for TypeCheckDiagnostic { } } -/// A collection of type check diagnostics. -/// -/// The diagnostics are wrapped in an `Arc` because they need to be cloned multiple times -/// when going from `infer_expression` to `check_file`. We could consider -/// making [`TypeCheckDiagnostic`] a Salsa struct to have them Arena-allocated (once the Tables refactor is done). -/// Using Salsa struct does have the downside that it leaks the Salsa dependency into diagnostics and -/// each Salsa-struct comes with an overhead. -#[derive(Default, Eq, PartialEq)] -pub struct TypeCheckDiagnostics { - inner: Vec>, +/// Emit a diagnostic declaring that the object represented by `node` is not iterable +pub(super) fn report_not_iterable( + db: &dyn Db, + file: File, + node: AnyNodeRef, + not_iterable_ty: Type, +) { + report_type_diagnostic( + db, + file, + node, + "not-iterable", + format_args!( + "Object of type `{}` is not iterable", + not_iterable_ty.display(db) + ), + ); } -impl TypeCheckDiagnostics { - pub(super) fn push(&mut self, diagnostic: TypeCheckDiagnostic) { - self.inner.push(Arc::new(diagnostic)); - } - - pub(crate) fn shrink_to_fit(&mut self) { - self.inner.shrink_to_fit(); - } +/// Emit a diagnostic declaring that the object represented by `node` is not iterable +/// because its `__iter__` method is possibly unbound. +pub(super) fn report_not_iterable_possibly_unbound( + db: &dyn Db, + file: File, + node: AnyNodeRef, + element_ty: Type, +) { + report_type_diagnostic( + db, + file, + node, + "not-iterable", + format_args!( + "Object of type `{}` is not iterable because its `__iter__` method is possibly unbound", + element_ty.display(db) + ), + ); } -impl Extend for TypeCheckDiagnostics { - fn extend>(&mut self, iter: T) { - self.inner.extend(iter.into_iter().map(std::sync::Arc::new)); - } +/// Emit a diagnostic declaring that an index is out of bounds for a tuple. +pub(super) fn report_index_out_of_bounds( + db: &dyn Db, + file: File, + kind: &'static str, + node: AnyNodeRef, + tuple_ty: Type, + length: usize, + index: i64, +) { + report_type_diagnostic( + db, + file, + node, + "index-out-of-bounds", + format_args!( + "Index {index} is out of bounds for {kind} `{}` with length {length}", + tuple_ty.display(db) + ), + ); } -impl Extend> for TypeCheckDiagnostics { - fn extend>>(&mut self, iter: T) { - self.inner.extend(iter); - } +/// Emit a diagnostic declaring that a type does not support subscripting. +pub(super) fn report_non_subscriptable( + db: &dyn Db, + file: File, + node: AnyNodeRef, + non_subscriptable_ty: Type, + method: &str, +) { + report_type_diagnostic( + db, + file, + node, + "non-subscriptable", + format_args!( + "Cannot subscript object of type `{}` with no `{method}` method", + non_subscriptable_ty.display(db) + ), + ); } -impl<'a> Extend<&'a std::sync::Arc> for TypeCheckDiagnostics { - fn extend>>(&mut self, iter: T) { - self.inner - .extend(iter.into_iter().map(std::sync::Arc::clone)); - } +pub(super) fn report_unresolved_module<'a>( + db: &dyn Db, + file: File, + import_node: impl Into>, + level: u32, + module: Option<&str>, +) { + report_type_diagnostic( + db, + file, + import_node.into(), + "unresolved-import", + format_args!( + "Cannot resolve import `{}{}`", + ".".repeat(level as usize), + module.unwrap_or_default() + ), + ); } -impl std::fmt::Debug for TypeCheckDiagnostics { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - self.inner.fmt(f) - } +pub(super) fn report_slice_step_size_zero(db: &dyn Db, file: File, node: AnyNodeRef) { + report_type_diagnostic( + db, + file, + node, + "zero-stepsize-in-slice", + format_args!("Slice step size can not be zero"), + ); } -impl Deref for TypeCheckDiagnostics { - type Target = [std::sync::Arc]; - - fn deref(&self) -> &Self::Target { - &self.inner +pub(super) fn report_invalid_assignment( + db: &dyn Db, + file: File, + node: AnyNodeRef, + declared_ty: Type, + assigned_ty: Type, +) { + match declared_ty { + Type::ClassLiteral(ClassLiteralType { class }) => { + report_type_diagnostic(db, file, node, "invalid-assignment", format_args!( + "Implicit shadowing of class `{}`; annotate to make it explicit if this is intentional", + class.name(db))); + } + Type::FunctionLiteral(function) => { + report_type_diagnostic(db, file, node, "invalid-assignment", format_args!( + "Implicit shadowing of function `{}`; annotate to make it explicit if this is intentional", + function.name(db))); + } + _ => { + report_type_diagnostic( + db, + file, + node, + "invalid-assignment", + format_args!( + "Object of type `{}` is not assignable to `{}`", + assigned_ty.display(db), + declared_ty.display(db), + ), + ); + } } } -impl IntoIterator for TypeCheckDiagnostics { - type Item = Arc; - type IntoIter = std::vec::IntoIter>; - - fn into_iter(self) -> Self::IntoIter { - self.inner.into_iter() - } +pub(super) fn report_possibly_unresolved_reference( + db: &dyn Db, + file: File, + expr_name_node: &ast::ExprName, +) { + let ast::ExprName { id, .. } = expr_name_node; + + report_type_diagnostic( + db, + file, + expr_name_node.into(), + "possibly-unresolved-reference", + format_args!("Name `{id}` used when possibly not defined"), + ); } -impl<'a> IntoIterator for &'a TypeCheckDiagnostics { - type Item = &'a Arc; - type IntoIter = std::slice::Iter<'a, std::sync::Arc>; +pub(super) fn report_unresolved_reference(db: &dyn Db, file: File, expr_name_node: &ast::ExprName) { + let ast::ExprName { id, .. } = expr_name_node; - fn into_iter(self) -> Self::IntoIter { - self.inner.iter() - } + report_type_diagnostic( + db, + file, + expr_name_node.into(), + "unresolved-reference", + format_args!("Name `{id}` used when not defined"), + ); } -pub(super) struct TypeCheckDiagnosticsBuilder<'db> { - db: &'db dyn Db, - file: File, - diagnostics: TypeCheckDiagnostics, +/// Returns `true` if any diagnostic is enabled for this file. +pub(crate) fn is_any_diagnostic_enabled(db: &dyn Db, file: File) -> bool { + db.is_file_open(file) } -impl<'db> TypeCheckDiagnosticsBuilder<'db> { - pub(super) fn new(db: &'db dyn Db, file: File) -> Self { - Self { - db, - file, - diagnostics: TypeCheckDiagnostics::default(), - } - } - - /// Emit a diagnostic declaring that the object represented by `node` is not iterable - pub(super) fn add_not_iterable(&mut self, node: AnyNodeRef, not_iterable_ty: Type<'db>) { - self.add( - node, - "not-iterable", - format_args!( - "Object of type `{}` is not iterable", - not_iterable_ty.display(self.db) - ), - ); - } - - /// Emit a diagnostic declaring that the object represented by `node` is not iterable - /// because its `__iter__` method is possibly unbound. - pub(super) fn add_not_iterable_possibly_unbound( - &mut self, - node: AnyNodeRef, - element_ty: Type<'db>, - ) { - self.add( - node, - "not-iterable", - format_args!( - "Object of type `{}` is not iterable because its `__iter__` method is possibly unbound", - element_ty.display(self.db) - ), - ); - } - - /// Emit a diagnostic declaring that an index is out of bounds for a tuple. - pub(super) fn add_index_out_of_bounds( - &mut self, - kind: &'static str, - node: AnyNodeRef, - tuple_ty: Type<'db>, - length: usize, - index: i64, - ) { - self.add( - node, - "index-out-of-bounds", - format_args!( - "Index {index} is out of bounds for {kind} `{}` with length {length}", - tuple_ty.display(self.db) - ), - ); - } - - /// Emit a diagnostic declaring that a type does not support subscripting. - pub(super) fn add_non_subscriptable( - &mut self, - node: AnyNodeRef, - non_subscriptable_ty: Type<'db>, - method: &str, - ) { - self.add( - node, - "non-subscriptable", - format_args!( - "Cannot subscript object of type `{}` with no `{method}` method", - non_subscriptable_ty.display(self.db) - ), - ); - } - - pub(super) fn add_unresolved_module( - &mut self, - import_node: impl Into>, - level: u32, - module: Option<&str>, - ) { - self.add( - import_node.into(), - "unresolved-import", - format_args!( - "Cannot resolve import `{}{}`", - ".".repeat(level as usize), - module.unwrap_or_default() - ), - ); - } - - pub(super) fn add_slice_step_size_zero(&mut self, node: AnyNodeRef) { - self.add( - node, - "zero-stepsize-in-slice", - format_args!("Slice step size can not be zero"), - ); - } - - pub(super) fn add_invalid_assignment( - &mut self, - node: AnyNodeRef, - declared_ty: Type<'db>, - assigned_ty: Type<'db>, - ) { - match declared_ty { - Type::ClassLiteral(ClassLiteralType { class }) => { - self.add(node, "invalid-assignment", format_args!( - "Implicit shadowing of class `{}`; annotate to make it explicit if this is intentional", - class.name(self.db))); - } - Type::FunctionLiteral(function) => { - self.add(node, "invalid-assignment", format_args!( - "Implicit shadowing of function `{}`; annotate to make it explicit if this is intentional", - function.name(self.db))); - } - _ => { - self.add( - node, - "invalid-assignment", - format_args!( - "Object of type `{}` is not assignable to `{}`", - assigned_ty.display(self.db), - declared_ty.display(self.db), - ), - ); - } - } - } - - pub(super) fn add_possibly_unresolved_reference(&mut self, expr_name_node: &ast::ExprName) { - let ast::ExprName { id, .. } = expr_name_node; - - self.add( - expr_name_node.into(), - "possibly-unresolved-reference", - format_args!("Name `{id}` used when possibly not defined"), - ); - } - - pub(super) fn add_unresolved_reference(&mut self, expr_name_node: &ast::ExprName) { - let ast::ExprName { id, .. } = expr_name_node; - - self.add( - expr_name_node.into(), - "unresolved-reference", - format_args!("Name `{id}` used when not defined"), - ); +/// Reports a diagnostic for the given node and file if diagnostic reporting is enabled for this file. +pub(crate) fn report_type_diagnostic( + db: &dyn Db, + file: File, + node: AnyNodeRef, + rule: &str, + message: std::fmt::Arguments, +) { + if !is_any_diagnostic_enabled(db, file) { + return; } - /// Adds a new diagnostic. - /// - /// The diagnostic does not get added if the rule isn't enabled for this file. - pub(super) fn add(&mut self, node: AnyNodeRef, rule: &str, message: std::fmt::Arguments) { - if !self.db.is_file_open(self.file) { - return; - } - - // TODO: Don't emit the diagnostic if: - // * The enclosing node contains any syntax errors - // * The rule is disabled for this file. We probably want to introduce a new query that - // 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. + // TODO: Don't emit the diagnostic if: + // * The enclosing node contains any syntax errors + // * The rule is disabled for this file. We probably want to introduce a new query that + // 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.push(TypeCheckDiagnostic { - file: self.file, + CompileDiagnostic::report( + db.upcast(), + TypeCheckDiagnostic { + file, rule: rule.to_string(), message: message.to_string(), range: node.range(), - }); - } - - pub(super) fn extend(&mut self, diagnostics: &TypeCheckDiagnostics) { - self.diagnostics.extend(diagnostics); - } - - pub(super) fn finish(mut self) -> TypeCheckDiagnostics { - self.diagnostics.shrink_to_fit(); - self.diagnostics - } + }, + ); } diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 9471a6fb1172d..6af8163595879 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -48,7 +48,11 @@ use crate::semantic_index::semantic_index; use crate::semantic_index::symbol::{NodeWithScopeKind, NodeWithScopeRef, ScopeId}; use crate::semantic_index::SemanticIndex; use crate::stdlib::builtins_module_scope; -use crate::types::diagnostic::{TypeCheckDiagnostics, TypeCheckDiagnosticsBuilder}; +use crate::types::diagnostic::{ + report_index_out_of_bounds, report_invalid_assignment, report_non_subscriptable, + report_possibly_unresolved_reference, report_slice_step_size_zero, report_type_diagnostic, + report_unresolved_module, report_unresolved_reference, +}; use crate::types::mro::MroErrorKind; use crate::types::unpacker::{UnpackResult, Unpacker}; use crate::types::{ @@ -216,9 +220,6 @@ pub(crate) struct TypeInference<'db> { /// The definitions that are deferred. deferred: FxHashSet>, - /// The diagnostics for this region. - diagnostics: TypeCheckDiagnostics, - /// The scope belong to this region. scope: ScopeId<'db>, } @@ -230,7 +231,7 @@ impl<'db> TypeInference<'db> { bindings: FxHashMap::default(), declarations: FxHashMap::default(), deferred: FxHashSet::default(), - diagnostics: TypeCheckDiagnostics::default(), + scope, } } @@ -254,15 +255,10 @@ impl<'db> TypeInference<'db> { self.declarations[&definition] } - pub(crate) fn diagnostics(&self) -> &TypeCheckDiagnostics { - &self.diagnostics - } - fn shrink_to_fit(&mut self) { self.expressions.shrink_to_fit(); self.bindings.shrink_to_fit(); self.declarations.shrink_to_fit(); - self.diagnostics.shrink_to_fit(); self.deferred.shrink_to_fit(); } } @@ -341,8 +337,6 @@ pub(super) struct TypeInferenceBuilder<'db> { /// expression could be deferred if the file has `from __future__ import annotations` import or /// is a stub file but we're still in a non-deferred region. deferred_state: DeferredExpressionState, - - diagnostics: TypeCheckDiagnosticsBuilder<'db>, } impl<'db> TypeInferenceBuilder<'db> { @@ -373,7 +367,6 @@ impl<'db> TypeInferenceBuilder<'db> { file, deferred_state: DeferredExpressionState::None, types: TypeInference::empty(scope), - diagnostics: TypeCheckDiagnosticsBuilder::new(db, file), } } @@ -386,7 +379,6 @@ impl<'db> TypeInferenceBuilder<'db> { .extend(inference.declarations.iter()); self.types.expressions.extend(inference.expressions.iter()); self.types.deferred.extend(inference.deferred.iter()); - self.diagnostics.extend(&inference.diagnostics); } fn scope(&self) -> ScopeId<'db> { @@ -501,7 +493,9 @@ impl<'db> TypeInferenceBuilder<'db> { for (class, class_node) in class_definitions { // (1) Check that the class does not have a cyclic definition if class.is_cyclically_defined(self.db) { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, class_node.into(), "cyclic-class-def", format_args!( @@ -521,7 +515,9 @@ impl<'db> TypeInferenceBuilder<'db> { MroErrorKind::DuplicateBases(duplicates) => { let base_nodes = class_node.bases(); for (index, duplicate) in duplicates { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, (&base_nodes[*index]).into(), "duplicate-base", format_args!("Duplicate base class `{}`", duplicate.name(self.db)), @@ -531,7 +527,9 @@ impl<'db> TypeInferenceBuilder<'db> { MroErrorKind::InvalidBases(bases) => { let base_nodes = class_node.bases(); for (index, base_ty) in bases { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, (&base_nodes[*index]).into(), "invalid-base", format_args!( @@ -541,7 +539,9 @@ impl<'db> TypeInferenceBuilder<'db> { ); } } - MroErrorKind::UnresolvableMro { bases_list } => self.diagnostics.add( + MroErrorKind::UnresolvableMro { bases_list } => report_type_diagnostic( + self.db, + self.file, class_node.into(), "inconsistent-mro", format_args!( @@ -571,7 +571,9 @@ impl<'db> TypeInferenceBuilder<'db> { } => { let node = class_node.into(); if *candidate1_is_base_class { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, node, "conflicting-metaclass", format_args!( @@ -586,7 +588,9 @@ impl<'db> TypeInferenceBuilder<'db> { ) ); } else { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, node, "conflicting-metaclass", format_args!( @@ -732,7 +736,9 @@ impl<'db> TypeInferenceBuilder<'db> { _ => return, }; - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, expr.into(), "division-by-zero", format_args!( @@ -757,7 +763,9 @@ impl<'db> TypeInferenceBuilder<'db> { // TODO point out the conflicting declarations in the diagnostic? let symbol_table = self.index.symbol_table(binding.file_scope(self.db)); let symbol_name = symbol_table.symbol(binding.symbol(self.db)).name(); - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, node, "conflicting-declarations", format_args!( @@ -769,8 +777,7 @@ impl<'db> TypeInferenceBuilder<'db> { }, ); if !bound_ty.is_assignable_to(self.db, declared_ty) { - self.diagnostics - .add_invalid_assignment(node, declared_ty, bound_ty); + report_invalid_assignment(self.db, self.file, node, declared_ty, bound_ty); // allow declarations to override inference in case of invalid assignment bound_ty = declared_ty; }; @@ -787,7 +794,9 @@ impl<'db> TypeInferenceBuilder<'db> { let ty = if inferred_ty.is_assignable_to(self.db, ty) { ty } else { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, node, "invalid-declaration", format_args!( @@ -813,8 +822,7 @@ impl<'db> TypeInferenceBuilder<'db> { let inferred_ty = if inferred_ty.is_assignable_to(self.db, declared_ty) { inferred_ty } else { - self.diagnostics - .add_invalid_assignment(node, declared_ty, inferred_ty); + report_invalid_assignment(self.db, self.file, node, declared_ty, inferred_ty); // if the assignment is invalid, fall back to assuming the annotation is correct declared_ty }; @@ -1297,7 +1305,9 @@ impl<'db> TypeInferenceBuilder<'db> { // TODO: Make use of Protocols when we support it (the manager be assignable to `contextlib.AbstractContextManager`). match (enter, exit) { (Symbol::Unbound, Symbol::Unbound) => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1308,7 +1318,9 @@ impl<'db> TypeInferenceBuilder<'db> { Type::Unknown } (Symbol::Unbound, _) => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1320,7 +1332,9 @@ impl<'db> TypeInferenceBuilder<'db> { } (Symbol::Type(enter_ty, enter_boundness), exit) => { if enter_boundness == Boundness::PossiblyUnbound { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1332,9 +1346,11 @@ impl<'db> TypeInferenceBuilder<'db> { let target_ty = enter_ty .call(self.db, &[context_expression_ty]) - .return_ty_result(self.db, context_expression.into(), &mut self.diagnostics) + .return_ty_result(self.db, self.file, context_expression.into(), ) .unwrap_or_else(|err| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!(" @@ -1346,7 +1362,9 @@ impl<'db> TypeInferenceBuilder<'db> { match exit { Symbol::Unbound => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1359,7 +1377,9 @@ impl<'db> TypeInferenceBuilder<'db> { // TODO: Use the `exit_ty` to determine if any raised exception is suppressed. if exit_boundness == Boundness::PossiblyUnbound { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1379,14 +1399,12 @@ impl<'db> TypeInferenceBuilder<'db> { Type::none(self.db), ], ) - .return_ty_result( - self.db, - context_expression.into(), - &mut self.diagnostics, - ) + .return_ty_result(self.db, self.file, context_expression.into()) .is_err() { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, context_expression.into(), "invalid-context-manager", format_args!( @@ -1466,7 +1484,9 @@ impl<'db> TypeInferenceBuilder<'db> { let bound_or_constraint = match bound.as_deref() { Some(expr @ ast::Expr::Tuple(ast::ExprTuple { elts, .. })) => { if elts.len() < 2 { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, expr.into(), "invalid-typevar-constraints", format_args!("TypeVar must have at least two constrained types"), @@ -1663,12 +1683,6 @@ impl<'db> TypeInferenceBuilder<'db> { let target_ty = match assignment.target() { TargetKind::Sequence(unpack) => { let unpacked = infer_unpack_types(self.db, unpack); - // Only copy the diagnostics if this is the first assignment to avoid duplicating the - // unpack assignments. - if assignment.is_first() { - self.diagnostics.extend(unpacked.diagnostics()); - } - unpacked.get(name_ast_id).unwrap_or(Type::Unknown) } TargetKind::Name => value_ty, @@ -1777,12 +1791,14 @@ impl<'db> TypeInferenceBuilder<'db> { let call = class_member.call(self.db, &[target_type, value_type]); let augmented_return_ty = match call.return_ty_result( self.db, + self.file, AnyNodeRef::StmtAugAssign(assignment), - &mut self.diagnostics, ) { Ok(t) => t, Err(e) => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, assignment.into(), "unsupported-operator", format_args!( @@ -1803,7 +1819,9 @@ impl<'db> TypeInferenceBuilder<'db> { let binary_return_ty = self.infer_binary_expression_type(left_ty, right_ty, op) .unwrap_or_else(|| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, assignment.into(), "unsupported-operator", format_args!( @@ -1832,7 +1850,9 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_binary_expression_type(left_ty, right_ty, op) .unwrap_or_else(|| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, assignment.into(), "unsupported-operator", format_args!( @@ -1919,7 +1939,7 @@ impl<'db> TypeInferenceBuilder<'db> { } else { iterable_ty .iterate(self.db) - .unwrap_with_diagnostic(iterable.into(), &mut self.diagnostics) + .unwrap_with_diagnostic(self.db, self.file, iterable.into()) }; self.store_expression_type(target, loop_var_value_ty); @@ -1958,7 +1978,7 @@ impl<'db> TypeInferenceBuilder<'db> { if let Some(module) = self.module_ty_from_name(&module_name) { module } else { - self.diagnostics.add_unresolved_module(alias, 0, Some(name)); + report_unresolved_module(self.db, self.file, alias, 0, Some(name)); Type::Unknown } } else { @@ -2086,7 +2106,9 @@ impl<'db> TypeInferenceBuilder<'db> { match module_ty.member(self.db, &ast::name::Name::new(&name.id)) { Symbol::Type(ty, boundness) => { if boundness == Boundness::PossiblyUnbound { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, AnyNodeRef::Alias(alias), "possibly-unbound-import", format_args!("Member `{name}` of module `{module_name}` is possibly unbound",), @@ -2096,7 +2118,9 @@ impl<'db> TypeInferenceBuilder<'db> { ty } Symbol::Unbound => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, AnyNodeRef::Alias(alias), "unresolved-import", format_args!("Module `{module_name}` has no member `{name}`",), @@ -2105,8 +2129,7 @@ impl<'db> TypeInferenceBuilder<'db> { } } } else { - self.diagnostics - .add_unresolved_module(import_from, *level, module); + report_unresolved_module(self.db, self.file, import_from, *level, module); Type::Unknown } } @@ -2120,8 +2143,8 @@ impl<'db> TypeInferenceBuilder<'db> { "Relative module resolution `{}` failed: too many leading dots", format_import_from_module(*level, module), ); - self.diagnostics - .add_unresolved_module(import_from, *level, module); + + report_unresolved_module(self.db, self.file, import_from, *level, module); Type::Unknown } Err(ModuleNameResolutionError::UnknownCurrentModule) => { @@ -2130,8 +2153,8 @@ impl<'db> TypeInferenceBuilder<'db> { format_import_from_module(*level, module), self.file.path(self.db) ); - self.diagnostics - .add_unresolved_module(import_from, *level, module); + + report_unresolved_module(self.db, self.file, import_from, *level, module); Type::Unknown } }; @@ -2596,7 +2619,7 @@ impl<'db> TypeInferenceBuilder<'db> { } else { iterable_ty .iterate(self.db) - .unwrap_with_diagnostic(iterable.into(), &mut self.diagnostics) + .unwrap_with_diagnostic(self.db, self.file, iterable.into()) }; self.types.expressions.insert( @@ -2697,7 +2720,7 @@ impl<'db> TypeInferenceBuilder<'db> { let function_type = self.infer_expression(func); function_type .call(self.db, arg_types.as_slice()) - .unwrap_with_diagnostic(self.db, func.as_ref().into(), &mut self.diagnostics) + .unwrap_with_diagnostic(self.db, self.file, func.as_ref().into()) } fn infer_starred_expression(&mut self, starred: &ast::ExprStarred) -> Type<'db> { @@ -2708,9 +2731,11 @@ impl<'db> TypeInferenceBuilder<'db> { } = starred; let iterable_ty = self.infer_expression(value); - iterable_ty - .iterate(self.db) - .unwrap_with_diagnostic(value.as_ref().into(), &mut self.diagnostics); + iterable_ty.iterate(self.db).unwrap_with_diagnostic( + self.db, + self.file, + value.as_ref().into(), + ); // TODO todo_type!() @@ -2729,9 +2754,11 @@ impl<'db> TypeInferenceBuilder<'db> { let ast::ExprYieldFrom { range: _, value } = yield_from; let iterable_ty = self.infer_expression(value); - iterable_ty - .iterate(self.db) - .unwrap_with_diagnostic(value.as_ref().into(), &mut self.diagnostics); + iterable_ty.iterate(self.db).unwrap_with_diagnostic( + self.db, + self.file, + value.as_ref().into(), + ); // TODO get type from `ReturnType` of generator todo_type!() @@ -2803,7 +2830,9 @@ impl<'db> TypeInferenceBuilder<'db> { { let mut builtins_symbol = builtins_symbol(self.db, name); if builtins_symbol.is_unbound() && name == "reveal_type" { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, name_node.into(), "undefined-reveal", format_args!( @@ -2858,7 +2887,7 @@ impl<'db> TypeInferenceBuilder<'db> { match self.lookup_name(name) { Symbol::Type(looked_up_ty, looked_up_boundness) => { if looked_up_boundness == Boundness::PossiblyUnbound { - self.diagnostics.add_possibly_unresolved_reference(name); + report_possibly_unresolved_reference(self.db, self.file, name); } bindings_ty @@ -2867,9 +2896,9 @@ impl<'db> TypeInferenceBuilder<'db> { } Symbol::Unbound => { if bindings_ty.is_some() { - self.diagnostics.add_possibly_unresolved_reference(name); + report_possibly_unresolved_reference(self.db, self.file, name); } else { - self.diagnostics.add_unresolved_reference(name); + report_unresolved_reference(self.db, self.file, name); } bindings_ty.unwrap_or(Type::Unknown) } @@ -2900,7 +2929,9 @@ impl<'db> TypeInferenceBuilder<'db> { match value_ty.member(self.db, &attr.id) { Symbol::Type(member_ty, boundness) => { if boundness == Boundness::PossiblyUnbound { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, attribute.into(), "possibly-unbound-attribute", format_args!( @@ -2914,7 +2945,9 @@ impl<'db> TypeInferenceBuilder<'db> { member_ty } Symbol::Unbound => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, attribute.into(), "unresolved-attribute", format_args!( @@ -2988,14 +3021,13 @@ impl<'db> TypeInferenceBuilder<'db> { { let call = class_member.call(self.db, &[operand_type]); - match call.return_ty_result( - self.db, - AnyNodeRef::ExprUnaryOp(unary), - &mut self.diagnostics, - ) { + match call.return_ty_result(self.db, self.file, AnyNodeRef::ExprUnaryOp(unary)) + { Ok(t) => t, Err(e) => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, unary.into(), "unsupported-operator", format_args!( @@ -3007,7 +3039,9 @@ impl<'db> TypeInferenceBuilder<'db> { } } } else { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, unary.into(), "unsupported-operator", format_args!( @@ -3048,7 +3082,9 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_binary_expression_type(left_ty, right_ty, *op) .unwrap_or_else(|| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, binary.into(), "unsupported-operator", format_args!( @@ -3358,7 +3394,9 @@ impl<'db> TypeInferenceBuilder<'db> { self.infer_binary_type_comparison(left_ty, *op, right_ty) .unwrap_or_else(|error| { // Handle unsupported operators (diagnostic, `bool`/`Unknown` outcome) - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, AnyNodeRef::ExprCompare(compare), "unsupported-operator", format_args!( @@ -3904,7 +3942,9 @@ impl<'db> TypeInferenceBuilder<'db> { .py_index(i32::try_from(int).expect("checked in branch arm")) .copied() .unwrap_or_else(|_| { - self.diagnostics.add_index_out_of_bounds( + report_index_out_of_bounds( + self.db, + self.file, "tuple", value_node.into(), value_ty, @@ -3923,7 +3963,7 @@ impl<'db> TypeInferenceBuilder<'db> { let new_elements: Vec<_> = new_elements.copied().collect(); Type::tuple(self.db, &new_elements) } else { - self.diagnostics.add_slice_step_size_zero(value_node.into()); + report_slice_step_size_zero(self.db, self.file, value_node.into()); Type::Unknown } } @@ -3937,7 +3977,9 @@ impl<'db> TypeInferenceBuilder<'db> { .py_index(i32::try_from(int).expect("checked in branch arm")) .map(|ch| Type::string_literal(self.db, &ch.to_string())) .unwrap_or_else(|_| { - self.diagnostics.add_index_out_of_bounds( + report_index_out_of_bounds( + self.db, + self.file, "string", value_node.into(), value_ty, @@ -3957,7 +3999,7 @@ impl<'db> TypeInferenceBuilder<'db> { let literal: String = new_chars.collect(); Type::string_literal(self.db, &literal) } else { - self.diagnostics.add_slice_step_size_zero(value_node.into()); + report_slice_step_size_zero(self.db, self.file, value_node.into()); Type::Unknown }; result @@ -3972,7 +4014,9 @@ impl<'db> TypeInferenceBuilder<'db> { .py_index(i32::try_from(int).expect("checked in branch arm")) .map(|byte| Type::bytes_literal(self.db, &[*byte])) .unwrap_or_else(|_| { - self.diagnostics.add_index_out_of_bounds( + report_index_out_of_bounds( + self.db, + self.file, "bytes literal", value_node.into(), value_ty, @@ -3991,7 +4035,7 @@ impl<'db> TypeInferenceBuilder<'db> { let new_bytes: Vec = new_bytes.copied().collect(); Type::bytes_literal(self.db, &new_bytes) } else { - self.diagnostics.add_slice_step_size_zero(value_node.into()); + report_slice_step_size_zero(self.db, self.file, value_node.into()); Type::Unknown } } @@ -4015,7 +4059,9 @@ impl<'db> TypeInferenceBuilder<'db> { Symbol::Unbound => {} Symbol::Type(dunder_getitem_method, boundness) => { if boundness == Boundness::PossiblyUnbound { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, value_node.into(), "call-possibly-unbound-method", format_args!( @@ -4027,9 +4073,11 @@ impl<'db> TypeInferenceBuilder<'db> { return dunder_getitem_method .call(self.db, &[slice_ty]) - .return_ty_result(self.db, value_node.into(), &mut self.diagnostics) + .return_ty_result(self.db, self.file, value_node.into()) .unwrap_or_else(|err| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, value_node.into(), "call-non-callable", format_args!( @@ -4059,7 +4107,9 @@ impl<'db> TypeInferenceBuilder<'db> { Symbol::Unbound => {} Symbol::Type(ty, boundness) => { if boundness == Boundness::PossiblyUnbound { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, value_node.into(), "call-possibly-unbound-method", format_args!( @@ -4071,9 +4121,11 @@ impl<'db> TypeInferenceBuilder<'db> { return ty .call(self.db, &[slice_ty]) - .return_ty_result(self.db, value_node.into(), &mut self.diagnostics) + .return_ty_result(self.db, self.file, value_node.into()) .unwrap_or_else(|err| { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, value_node.into(), "call-non-callable", format_args!( @@ -4092,13 +4144,17 @@ impl<'db> TypeInferenceBuilder<'db> { return KnownClass::GenericAlias.to_instance(self.db); } - self.diagnostics.add_non_subscriptable( + report_non_subscriptable( + self.db, + self.file, value_node.into(), value_ty, "__class_getitem__", ); } else { - self.diagnostics.add_non_subscriptable( + report_non_subscriptable( + self.db, + self.file, value_node.into(), value_ty, "__getitem__", @@ -4170,7 +4226,6 @@ impl<'db> TypeInferenceBuilder<'db> { pub(super) fn finish(mut self) -> TypeInference<'db> { self.infer_region(); - self.types.diagnostics = self.diagnostics.finish(); self.types.shrink_to_fit(); self.types } @@ -4215,7 +4270,9 @@ impl<'db> TypeInferenceBuilder<'db> { ast::Expr::Starred(starred) => self.infer_starred_expression(starred), ast::Expr::BytesLiteral(bytes) => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, bytes.into(), "annotation-byte-string", format_args!("Type expressions cannot use bytes literal"), @@ -4224,7 +4281,9 @@ impl<'db> TypeInferenceBuilder<'db> { } ast::Expr::FString(fstring) => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, fstring.into(), "annotation-f-string", format_args!("Type expressions cannot use f-strings"), @@ -4253,10 +4312,7 @@ impl<'db> TypeInferenceBuilder<'db> { DeferredExpressionState::InStringAnnotation, ) } - Err(diagnostics) => { - self.diagnostics.extend(&diagnostics); - Type::Unknown - } + Err(()) => Type::Unknown, } } } @@ -4476,10 +4532,7 @@ impl<'db> TypeInferenceBuilder<'db> { DeferredExpressionState::InStringAnnotation, ) } - Err(diagnostics) => { - self.diagnostics.extend(&diagnostics); - Type::Unknown - } + Err(()) => Type::Unknown, } } @@ -4596,7 +4649,9 @@ impl<'db> TypeInferenceBuilder<'db> { Ok(ty) => ty, Err(nodes) => { for node in nodes { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, node.into(), "invalid-literal-parameter", format_args!( @@ -4632,7 +4687,9 @@ impl<'db> TypeInferenceBuilder<'db> { todo_type!("generic type alias") } KnownInstanceType::NoReturn | KnownInstanceType::Never => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, subscript.into(), "invalid-type-parameter", format_args!( @@ -4643,7 +4700,9 @@ impl<'db> TypeInferenceBuilder<'db> { Type::Unknown } KnownInstanceType::LiteralString => { - self.diagnostics.add( + report_type_diagnostic( + self.db, + self.file, subscript.into(), "invalid-type-parameter", format_args!( @@ -5038,8 +5097,6 @@ fn perform_membership_test_comparison<'db>( #[cfg(test)] mod tests { - use anyhow::Context; - use crate::db::tests::TestDb; use crate::program::{Program, SearchPathSettings}; use crate::python_version::PythonVersion; @@ -5048,6 +5105,8 @@ mod tests { use crate::semantic_index::{global_scope, semantic_index, symbol_table, use_def_map}; use crate::types::check_types; use crate::{HasTy, ProgramSettings, SemanticModel}; + use anyhow::Context; + use ruff_db::diagnostic::{CompileDiagnostic, Diagnostic}; use ruff_db::files::{system_path_to_file, File}; use ruff_db::parsed::parsed_module; use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; @@ -5149,22 +5208,18 @@ mod tests { } #[track_caller] - fn assert_diagnostic_messages(diagnostics: &TypeCheckDiagnostics, expected: &[&str]) { - let messages: Vec<&str> = diagnostics + fn assert_file_diagnostics(db: &TestDb, filename: &str, expected: &[&str]) { + let file = system_path_to_file(db, filename).unwrap(); + + let diagnostics = check_types::accumulated::(db, file); + + let messages: Vec<_> = diagnostics .iter() .map(|diagnostic| diagnostic.message()) .collect(); assert_eq!(&messages, expected); } - #[track_caller] - fn assert_file_diagnostics(db: &TestDb, filename: &str, expected: &[&str]) { - let file = system_path_to_file(db, filename).unwrap(); - let diagnostics = check_types(db, file); - - assert_diagnostic_messages(diagnostics, expected); - } - #[test] fn from_import_with_no_module_name() -> anyhow::Result<()> { // This test checks that invalid syntax in a `StmtImportFrom` node @@ -5949,7 +6004,15 @@ mod tests { assert!(z.is_unbound()); // (There is a diagnostic for invalid syntax that's emitted, but it's not listed by `assert_file_diagnostics`) - assert_file_diagnostics(&db, "src/a.py", &["Name `z` used when not defined"]); + assert_file_diagnostics( + &db, + "src/a.py", + &[ + "Expected an identifier, but found a keyword 'in' that cannot be used here", + "Expected 'in', found name", + "Name `z` used when not defined", + ], + ); Ok(()) } diff --git a/crates/red_knot_python_semantic/src/types/string_annotation.rs b/crates/red_knot_python_semantic/src/types/string_annotation.rs index b4801f4401225..42264125f2503 100644 --- a/crates/red_knot_python_semantic/src/types/string_annotation.rs +++ b/crates/red_knot_python_semantic/src/types/string_annotation.rs @@ -5,10 +5,10 @@ use ruff_python_ast::{self as ast, ModExpression, StringFlags}; use ruff_python_parser::{parse_expression_range, Parsed}; use ruff_text_size::Ranged; -use crate::types::diagnostic::{TypeCheckDiagnostics, TypeCheckDiagnosticsBuilder}; +use crate::types::diagnostic::report_type_diagnostic; use crate::Db; -type AnnotationParseResult = Result, TypeCheckDiagnostics>; +type AnnotationParseResult = Result, ()>; /// Parses the given expression as a string annotation. pub(crate) fn parse_string_annotation( @@ -20,12 +20,13 @@ pub(crate) fn parse_string_annotation( let source = source_text(db.upcast(), file); let node_text = &source[string_expr.range()]; - let mut diagnostics = TypeCheckDiagnosticsBuilder::new(db, file); if let [string_literal] = string_expr.value.as_slice() { let prefix = string_literal.flags.prefix(); if prefix.is_raw() { - diagnostics.add( + report_type_diagnostic( + db, + file, string_literal.into(), "annotation-raw-string", format_args!("Type expressions cannot use raw string literal"), @@ -49,7 +50,9 @@ pub(crate) fn parse_string_annotation( // ``` match parse_expression_range(source.as_str(), range_excluding_quotes) { Ok(parsed) => return Ok(parsed), - Err(parse_error) => diagnostics.add( + Err(parse_error) => report_type_diagnostic( + db, + file, string_literal.into(), "forward-annotation-syntax-error", format_args!("Syntax error in forward annotation: {}", parse_error.error), @@ -58,7 +61,9 @@ pub(crate) fn parse_string_annotation( } else { // The raw contents of the string doesn't match the parsed content. This could be the // case for annotations that contain escape sequences. - diagnostics.add( + report_type_diagnostic( + db, + file, string_expr.into(), "annotation-escape-character", format_args!("Type expressions cannot contain escape characters"), @@ -66,12 +71,14 @@ pub(crate) fn parse_string_annotation( } } else { // String is implicitly concatenated. - diagnostics.add( + report_type_diagnostic( + db, + file, string_expr.into(), "annotation-implicit-concat", format_args!("Type expressions cannot span multiple string literals"), ); } - Err(diagnostics.finish()) + Err(()) } diff --git a/crates/red_knot_python_semantic/src/types/unpacker.rs b/crates/red_knot_python_semantic/src/types/unpacker.rs index dc96fac252af5..d077baadb786d 100644 --- a/crates/red_knot_python_semantic/src/types/unpacker.rs +++ b/crates/red_knot_python_semantic/src/types/unpacker.rs @@ -6,22 +6,22 @@ use rustc_hash::FxHashMap; use crate::semantic_index::ast_ids::{HasScopedExpressionId, ScopedExpressionId}; use crate::semantic_index::symbol::ScopeId; -use crate::types::{todo_type, Type, TypeCheckDiagnostics, TypeCheckDiagnosticsBuilder}; +use crate::types::{todo_type, Type}; use crate::Db; /// Unpacks the value expression type to their respective targets. pub(crate) struct Unpacker<'db> { db: &'db dyn Db, + file: File, targets: FxHashMap>, - diagnostics: TypeCheckDiagnosticsBuilder<'db>, } impl<'db> Unpacker<'db> { pub(crate) fn new(db: &'db dyn Db, file: File) -> Self { Self { db, + file, targets: FxHashMap::default(), - diagnostics: TypeCheckDiagnosticsBuilder::new(db, file), } } @@ -103,9 +103,11 @@ impl<'db> Unpacker<'db> { let value_ty = if value_ty.is_literal_string() { Type::LiteralString } else { - value_ty - .iterate(self.db) - .unwrap_with_diagnostic(AnyNodeRef::from(target), &mut self.diagnostics) + value_ty.iterate(self.db).unwrap_with_diagnostic( + self.db, + self.file, + AnyNodeRef::from(target), + ) }; for element in elts { self.unpack(element, value_ty, scope); @@ -119,7 +121,6 @@ impl<'db> Unpacker<'db> { pub(crate) fn finish(mut self) -> UnpackResult<'db> { self.targets.shrink_to_fit(); UnpackResult { - diagnostics: self.diagnostics.finish(), targets: self.targets, } } @@ -128,15 +129,10 @@ impl<'db> Unpacker<'db> { #[derive(Debug, Default, PartialEq, Eq)] pub(crate) struct UnpackResult<'db> { targets: FxHashMap>, - diagnostics: TypeCheckDiagnostics, } impl<'db> UnpackResult<'db> { pub(crate) fn get(&self, expr_id: ScopedExpressionId) -> Option> { self.targets.get(&expr_id).copied() } - - pub(crate) fn diagnostics(&self) -> &TypeCheckDiagnostics { - &self.diagnostics - } } diff --git a/crates/red_knot_server/src/server/api.rs b/crates/red_knot_server/src/server/api.rs index ebef3472d1565..be1aca8637dcd 100644 --- a/crates/red_knot_server/src/server/api.rs +++ b/crates/red_knot_server/src/server/api.rs @@ -91,11 +91,11 @@ fn background_request_task<'a, R: traits::BackgroundDocumentRequestHandler>( let db = match path { AnySystemPath::System(path) => { match session.workspace_db_for_path(path.as_std_path()) { - Some(db) => db.snapshot(), - None => session.default_workspace_db().snapshot(), + Some(db) => db.clone(), + None => session.default_workspace_db().clone(), } } - AnySystemPath::SystemVirtual(_) => session.default_workspace_db().snapshot(), + AnySystemPath::SystemVirtual(_) => session.default_workspace_db().clone(), }; let Some(snapshot) = session.take_snapshot(url) else { diff --git a/crates/red_knot_test/src/db.rs b/crates/red_knot_test/src/db.rs index 3cbd2eccb7efd..8a8f3416c46ce 100644 --- a/crates/red_knot_test/src/db.rs +++ b/crates/red_knot_test/src/db.rs @@ -7,6 +7,7 @@ use ruff_db::vendored::VendoredFileSystem; use ruff_db::{Db as SourceDb, Upcast}; #[salsa::db] +#[derive(Clone)] pub(crate) struct Db { workspace_root: SystemPathBuf, storage: salsa::Storage, diff --git a/crates/red_knot_test/src/lib.rs b/crates/red_knot_test/src/lib.rs index 23957a9dea814..1caa1852c4a83 100644 --- a/crates/red_knot_test/src/lib.rs +++ b/crates/red_knot_test/src/lib.rs @@ -2,9 +2,8 @@ use camino::Utf8Path; use colored::Colorize; use parser as test_parser; use red_knot_python_semantic::types::check_types; -use ruff_db::diagnostic::{Diagnostic, ParseDiagnostic}; +use ruff_db::diagnostic::{CompileDiagnostic, Diagnostic}; use ruff_db::files::{system_path_to_file, File, Files}; -use ruff_db::parsed::parsed_module; use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; use ruff_source_file::LineIndex; use ruff_text_size::TextSize; @@ -102,24 +101,9 @@ fn run_test(db: &mut db::Db, test: &parser::MarkdownTest) -> Result<(), Failures let failures: Failures = test_files .into_iter() .filter_map(|test_file| { - let parsed = parsed_module(db, test_file.file); - - let mut diagnostics: Vec> = parsed - .errors() - .iter() - .cloned() - .map(|error| { - let diagnostic: Box = - Box::new(ParseDiagnostic::new(test_file.file, error)); - diagnostic - }) - .collect(); - - let type_diagnostics = check_types(db, test_file.file); - diagnostics.extend(type_diagnostics.into_iter().map(|diagnostic| { - let diagnostic: Box = Box::new((*diagnostic).clone()); - diagnostic - })); + let mut diagnostics = check_types::accumulated::(db, test_file.file); + // Filter out diagnostics that are not related to the current file. + diagnostics.retain(|diagnostic| diagnostic.file() == test_file.file); match matcher::match_file(db, test_file.file, diagnostics) { Ok(()) => None, diff --git a/crates/red_knot_wasm/src/lib.rs b/crates/red_knot_wasm/src/lib.rs index d9f7514509d74..59124fc0162ef 100644 --- a/crates/red_knot_wasm/src/lib.rs +++ b/crates/red_knot_wasm/src/lib.rs @@ -6,7 +6,6 @@ use wasm_bindgen::prelude::*; use red_knot_workspace::db::{Db, RootDatabase}; use red_knot_workspace::workspace::settings::Configuration; use red_knot_workspace::workspace::WorkspaceMetadata; -use ruff_db::diagnostic::Diagnostic; use ruff_db::files::{system_path_to_file, File}; use ruff_db::system::walk_directory::WalkDirectoryBuilder; use ruff_db::system::{ diff --git a/crates/red_knot_workspace/Cargo.toml b/crates/red_knot_workspace/Cargo.toml index c91906d982d8c..88d575a8b5d5c 100644 --- a/crates/red_knot_workspace/Cargo.toml +++ b/crates/red_knot_workspace/Cargo.toml @@ -17,7 +17,6 @@ red_knot_python_semantic = { workspace = true } ruff_cache = { workspace = true } ruff_db = { workspace = true, features = ["os", "cache", "serde"] } ruff_python_ast = { workspace = true, features = ["serde"] } -ruff_text_size = { workspace = true } red_knot_vendored = { workspace = true } anyhow = { workspace = true } diff --git a/crates/red_knot_workspace/src/db.rs b/crates/red_knot_workspace/src/db.rs index 780eb9be81239..c5b0682050b00 100644 --- a/crates/red_knot_workspace/src/db.rs +++ b/crates/red_knot_workspace/src/db.rs @@ -4,9 +4,9 @@ use std::sync::Arc; use salsa::plumbing::ZalsaDatabase; use salsa::{Cancelled, Event}; -use crate::workspace::{check_file, Workspace, WorkspaceMetadata}; +use crate::workspace::{Workspace, WorkspaceMetadata}; use red_knot_python_semantic::{Db as SemanticDb, Program}; -use ruff_db::diagnostic::Diagnostic; +use ruff_db::diagnostic::CompileDiagnostic; use ruff_db::files::{File, Files}; use ruff_db::system::System; use ruff_db::vendored::VendoredFileSystem; @@ -20,6 +20,7 @@ pub trait Db: SemanticDb + Upcast { } #[salsa::db] +#[derive(Clone)] pub struct RootDatabase { workspace: Option, storage: salsa::Storage, @@ -48,14 +49,14 @@ impl RootDatabase { } /// Checks all open files in the workspace and its dependencies. - pub fn check(&self) -> Result>, Cancelled> { + pub fn check(&self) -> Result, Cancelled> { self.with_db(|db| db.workspace().check(db)) } - pub fn check_file(&self, file: File) -> Result>, Cancelled> { + pub fn check_file(&self, file: File) -> Result, Cancelled> { let _span = tracing::debug_span!("check_file", file=%file.path(self)).entered(); - self.with_db(|db| check_file(db, file)) + self.with_db(|db| db.workspace().check_file(self, file)) } /// Returns a mutable reference to the system. @@ -75,16 +76,6 @@ impl RootDatabase { { Cancelled::catch(|| f(self)) } - - #[must_use] - pub fn snapshot(&self) -> Self { - Self { - workspace: self.workspace, - storage: self.storage.clone(), - files: self.files.snapshot(), - system: Arc::clone(&self.system), - } - } } impl Upcast for RootDatabase { @@ -172,6 +163,7 @@ pub(crate) mod tests { use crate::workspace::{Workspace, WorkspaceMetadata}; #[salsa::db] + #[derive(Clone)] pub(crate) struct TestDb { storage: salsa::Storage, events: std::sync::Arc>>, diff --git a/crates/red_knot_workspace/src/workspace.rs b/crates/red_knot_workspace/src/workspace.rs index 780f552f7158b..db1c4f3f445a3 100644 --- a/crates/red_knot_workspace/src/workspace.rs +++ b/crates/red_knot_workspace/src/workspace.rs @@ -1,24 +1,20 @@ #![allow(clippy::ref_option)] -use crate::db::Db; -use crate::db::RootDatabase; +use crate::db::{Db, RootDatabase}; use crate::workspace::files::{Index, Indexed, IndexedIter, PackageFiles}; pub use metadata::{PackageMetadata, WorkspaceDiscoveryError, WorkspaceMetadata}; use red_knot_python_semantic::types::check_types; use red_knot_python_semantic::SearchPathSettings; -use ruff_db::diagnostic::{Diagnostic, ParseDiagnostic, Severity}; -use ruff_db::parsed::parsed_module; -use ruff_db::source::{source_text, SourceTextError}; +use ruff_db::diagnostic::{CompileDiagnostic, Diagnostic}; +use ruff_db::source::source_text; use ruff_db::system::FileType; use ruff_db::{ files::{system_path_to_file, File}, system::{walk_directory::WalkState, SystemPath, SystemPathBuf}, }; use ruff_python_ast::{name::Name, PySourceType}; -use ruff_text_size::TextRange; use rustc_hash::{FxBuildHasher, FxHashSet}; use salsa::{Durability, Setter as _}; -use std::borrow::Cow; use std::iter::FusedIterator; use std::{collections::BTreeMap, sync::Arc}; @@ -109,6 +105,7 @@ pub struct Package { // TODO: Add the loaded settings. } +#[salsa::tracked] impl Workspace { pub fn from_metadata(db: &dyn Db, metadata: WorkspaceMetadata) -> Self { let mut packages = BTreeMap::new(); @@ -186,35 +183,45 @@ impl Workspace { } /// Checks all open files in the workspace and its dependencies. - pub fn check(self, db: &RootDatabase) -> Vec> { - let workspace_span = tracing::debug_span!("check_workspace"); - let _span = workspace_span.enter(); - - tracing::debug!("Checking workspace"); - let files = WorkspaceFiles::new(db, self); - let result = Arc::new(std::sync::Mutex::new(Vec::new())); - let inner_result = Arc::clone(&result); - - let db = db.snapshot(); - let workspace_span = workspace_span.clone(); - - rayon::scope(move |scope| { - for file in &files { - let result = inner_result.clone(); - let db = db.snapshot(); - let workspace_span = workspace_span.clone(); - - scope.spawn(move |_| { - let check_file_span = tracing::debug_span!(parent: &workspace_span, "check_file", file=%file.path(&db)); - let _entered = check_file_span.entered(); - - let file_diagnostics = check_file(&db, file); - result.lock().unwrap().extend(file_diagnostics); - }); - } + pub fn check(self, db: &RootDatabase) -> Vec { + check_workspace_par(db, self); + + let mut diagnostics: Vec = + check_workspace_sync::accumulated::(db, self); + + diagnostics.sort_unstable_by(|a, b| { + let a_file = a.file(); + let b_file = b.file(); + + a_file.cmp(&b_file).then_with(|| { + a_file + .path(db) + .as_str() + .cmp(b_file.path(db).as_str()) + .then_with(|| { + a.range() + .unwrap_or_default() + .start() + .cmp(&b.range().unwrap_or_default().start()) + }) + }) }); - Arc::into_inner(result).unwrap().into_inner().unwrap() + diagnostics + } + + pub fn check_file(self, db: &dyn Db, file: File) -> Vec { + let mut diagnostics: Vec = + check_file::accumulated::(db, file); + + // Salsa returns all diagnostics that were created while checking this file, + // including diagnostics from dependencies. Remove diagnostics that don't belong to this file. + diagnostics.retain(|diagnostic| diagnostic.file() == file); + + diagnostics + .sort_unstable_by_key(|diagnostic| diagnostic.range().unwrap_or_default().start()); + + diagnostics } /// Opens a file in the workspace. @@ -376,33 +383,53 @@ impl Package { } } -pub(super) fn check_file(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); +/// Checks all workspace files in parallel. +/// +/// This function should be merged with [`check_workspace_sync`] and use `salsa::par_map` once parallel-salsa is more stable. +/// It is only necessary today because `check_workspace_sync::accumulated::(db, self)` +/// only passes a `&dyn Db` but we need a `&RootDatabase` to be able to clone the database for each thread. +fn check_workspace_par(db: &RootDatabase, workspace: Workspace) { + let workspace_span = tracing::debug_span!("check_workspace"); + let _span = workspace_span.enter(); + + tracing::debug!("Checking workspace"); + let files = WorkspaceFiles::new(db, workspace); + + let db = db.clone(); + let workspace_span = workspace_span.clone(); + + rayon::scope(move |scope| { + for file in &files { + let db = db.clone(); + let workspace_span = workspace_span.clone(); + scope.spawn(move |_| { + let check_file_span = tracing::debug_span!(parent: &workspace_span, "check_file", file=%file.path(&db)); + let _entered = check_file_span.entered(); + + check_file(&db, file); + }); + } + }); +} - if let Some(read_error) = source.read_error() { - diagnostics.push(Box::new(IOErrorDiagnostic { - file, - error: read_error.clone(), - })); - return diagnostics; +#[salsa::tracked] +fn check_workspace_sync(db: &dyn Db, workspace: Workspace) { + for file in &WorkspaceFiles::new(db, workspace) { + check_file(db, file); } +} - let parsed = parsed_module(db.upcast(), file); - diagnostics.extend(parsed.errors().iter().map(|error| { - let diagnostic: Box = Box::new(ParseDiagnostic::new(file, error.clone())); - diagnostic - })); - - diagnostics.extend(check_types(db.upcast(), file).iter().map(|diagnostic| { - let boxed: Box = Box::new(diagnostic.clone()); - boxed - })); +#[salsa::tracked] +pub(super) fn check_file(db: &dyn Db, file: File) { + // Abort checking if there are IO errors. + let source = source_text(db.upcast(), file); - diagnostics.sort_unstable_by_key(|diagnostic| diagnostic.range().unwrap_or_default().start()); + // Skip over files that failed to read. + if source.has_read_error() { + return; + } - diagnostics + check_types(db.upcast(), file); } fn discover_package_files(db: &dyn Db, package: Package) -> FxHashSet { @@ -526,34 +553,6 @@ impl Iterator for WorkspaceFilesIter<'_> { } } -#[derive(Debug)] -pub struct IOErrorDiagnostic { - file: File, - error: SourceTextError, -} - -impl Diagnostic for IOErrorDiagnostic { - fn rule(&self) -> &str { - "io" - } - - fn message(&self) -> Cow { - self.error.to_string().into() - } - - fn file(&self) -> File { - self.file - } - - fn range(&self) -> Option { - None - } - - fn severity(&self) -> Severity { - Severity::Error - } -} - #[derive(Debug, Eq, PartialEq, Clone)] pub struct PackageTree(BTreeMap); @@ -612,7 +611,8 @@ impl FusedIterator for PackageTreeIter<'_> {} #[cfg(test)] mod tests { use crate::db::tests::TestDb; - use crate::workspace::{check_file, WorkspaceMetadata}; + use crate::db::Db; + use crate::workspace::WorkspaceMetadata; use red_knot_python_semantic::types::check_types; use ruff_db::diagnostic::Diagnostic; use ruff_db::files::system_path_to_file; @@ -637,7 +637,8 @@ mod tests { assert_eq!(source_text(&db, file).as_str(), ""); assert_eq!( - check_file(&db, file) + db.workspace() + .check_file(&db, file) .into_iter() .map(|diagnostic| diagnostic.message().into_owned()) .collect::>(), @@ -653,7 +654,8 @@ mod tests { assert_eq!(source_text(&db, file).as_str(), ""); assert_eq!( - check_file(&db, file) + db.workspace() + .check_file(&db, file) .into_iter() .map(|diagnostic| diagnostic.message().into_owned()) .collect::>(), diff --git a/crates/ruff/src/commands/analyze_graph.rs b/crates/ruff/src/commands/analyze_graph.rs index f8a0684d428a7..0eb36cea6bf68 100644 --- a/crates/ruff/src/commands/analyze_graph.rs +++ b/crates/ruff/src/commands/analyze_graph.rs @@ -81,7 +81,7 @@ pub(crate) fn analyze_graph( // Collect and resolve the imports for each file. let result = Arc::new(Mutex::new(Vec::new())); let inner_result = Arc::clone(&result); - let db = db.snapshot(); + let db = db.clone(); rayon::scope(move |scope| { for resolved_file in paths { @@ -137,7 +137,7 @@ pub(crate) fn analyze_graph( continue; }; - let db = db.snapshot(); + let db = db.clone(); let glob_resolver = glob_resolver.clone(); let root = root.clone(); let result = inner_result.clone(); diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 60e9845f0e143..3d5fdb0de894a 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -8,7 +8,7 @@ use red_knot_workspace::workspace::settings::Configuration; use red_knot_workspace::workspace::WorkspaceMetadata; use ruff_benchmark::criterion::{criterion_group, criterion_main, BatchSize, Criterion}; use ruff_benchmark::TestFile; -use ruff_db::diagnostic::Diagnostic; +use ruff_db::diagnostic::CompileDiagnostic; use ruff_db::files::{system_path_to_file, File}; use ruff_db::source::source_text; use ruff_db::system::{MemoryFileSystem, SystemPath, SystemPathBuf, TestSystem}; @@ -178,7 +178,7 @@ fn benchmark_cold(criterion: &mut Criterion) { } #[track_caller] -fn assert_diagnostics(db: &dyn Db, diagnostics: Vec>) { +fn assert_diagnostics(db: &dyn Db, diagnostics: Vec) { let normalized: Vec<_> = diagnostics .into_iter() .map(|diagnostic| { diff --git a/crates/ruff_db/Cargo.toml b/crates/ruff_db/Cargo.toml index cef0d11f8f301..31fd926383036 100644 --- a/crates/ruff_db/Cargo.toml +++ b/crates/ruff_db/Cargo.toml @@ -30,7 +30,6 @@ matchit = { workspace = true } salsa = { workspace = true } serde = { workspace = true, optional = true } path-slash = { workspace = true } -thiserror = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true, optional = true } tracing-tree = { workspace = true, optional = true } diff --git a/crates/ruff_db/src/diagnostic.rs b/crates/ruff_db/src/diagnostic.rs index ed2836e705b25..7d9b791a421f1 100644 --- a/crates/ruff_db/src/diagnostic.rs +++ b/crates/ruff_db/src/diagnostic.rs @@ -5,6 +5,7 @@ use crate::{ }; use ruff_python_parser::ParseError; use ruff_text_size::TextRange; +use salsa::Accumulator as _; use std::borrow::Cow; pub trait Diagnostic: Send + Sync + std::fmt::Debug { @@ -73,6 +74,47 @@ impl std::fmt::Display for DisplayDiagnostic<'_> { } } +#[salsa::accumulator] +pub struct CompileDiagnostic(std::sync::Arc); + +impl CompileDiagnostic { + pub fn report(db: &dyn Db, diagnostic: T) + where + T: Diagnostic + 'static, + { + Self(std::sync::Arc::new(diagnostic)).accumulate(db); + } + + pub fn display<'a>(&'a self, db: &'a dyn Db) -> DisplayDiagnostic<'a> { + DisplayDiagnostic { + db, + diagnostic: &*self.0, + } + } +} + +impl Diagnostic for CompileDiagnostic { + fn rule(&self) -> &str { + self.0.rule() + } + + fn message(&self) -> Cow { + self.0.message() + } + + fn file(&self) -> File { + self.0.file() + } + + fn range(&self) -> Option { + self.0.range() + } + + fn severity(&self) -> Severity { + self.0.severity() + } +} + impl Diagnostic for Box where T: Diagnostic, diff --git a/crates/ruff_db/src/files.rs b/crates/ruff_db/src/files.rs index ed257efc135ee..d4354957aa523 100644 --- a/crates/ruff_db/src/files.rs +++ b/crates/ruff_db/src/files.rs @@ -48,7 +48,7 @@ pub fn vendored_path_to_file( } /// Lookup table that maps [file paths](`FilePath`) to salsa interned [`File`] instances. -#[derive(Default)] +#[derive(Default, Clone)] pub struct Files { inner: Arc, } @@ -253,13 +253,6 @@ impl Files { root.set_revision(db).to(FileRevision::now()); } } - - #[must_use] - pub fn snapshot(&self) -> Self { - Self { - inner: Arc::clone(&self.inner), - } - } } impl std::fmt::Debug for Files { diff --git a/crates/ruff_db/src/lib.rs b/crates/ruff_db/src/lib.rs index adccefb7c43c1..0ccfee7ad9567 100644 --- a/crates/ruff_db/src/lib.rs +++ b/crates/ruff_db/src/lib.rs @@ -48,7 +48,7 @@ mod tests { /// /// Uses an in memory filesystem and it stubs out the vendored files by default. #[salsa::db] - #[derive(Default)] + #[derive(Default, Clone)] pub(crate) struct TestDb { storage: salsa::Storage, files: Files, diff --git a/crates/ruff_db/src/parsed.rs b/crates/ruff_db/src/parsed.rs index e93d5e55178c2..13b8d6fab064c 100644 --- a/crates/ruff_db/src/parsed.rs +++ b/crates/ruff_db/src/parsed.rs @@ -5,6 +5,7 @@ use std::sync::Arc; use ruff_python_ast::{ModModule, PySourceType}; use ruff_python_parser::{parse_unchecked_source, Parsed}; +use crate::diagnostic::{CompileDiagnostic, ParseDiagnostic}; use crate::files::{File, FilePath}; use crate::source::source_text; use crate::Db; @@ -37,7 +38,13 @@ pub fn parsed_module(db: &dyn Db, file: File) -> ParsedModule { .map_or(PySourceType::Python, PySourceType::from_extension), }; - ParsedModule::new(parse_unchecked_source(&source, ty)) + let parsed = parse_unchecked_source(&source, ty); + + for error in parsed.errors() { + CompileDiagnostic::report(db, ParseDiagnostic::new(file, error.clone())); + } + + ParsedModule::new(parsed) } /// Cheap cloneable wrapper around the parsed module. diff --git a/crates/ruff_db/src/source.rs b/crates/ruff_db/src/source.rs index 115b274c7a991..13c76e1764bcd 100644 --- a/crates/ruff_db/src/source.rs +++ b/crates/ruff_db/src/source.rs @@ -1,28 +1,39 @@ +use std::borrow::Cow; use std::ops::Deref; use std::sync::Arc; use countme::Count; -use ruff_notebook::Notebook; -use ruff_python_ast::PySourceType; -use ruff_source_file::LineIndex; - +use crate::diagnostic::{CompileDiagnostic, Diagnostic, Severity}; use crate::files::{File, FilePath}; use crate::Db; +use ruff_notebook::{Notebook, NotebookError}; +use ruff_python_ast::PySourceType; +use ruff_source_file::LineIndex; +use ruff_text_size::TextRange; /// Reads the source text of a python text file (must be valid UTF8) or notebook. #[salsa::tracked] pub fn source_text(db: &dyn Db, file: File) -> SourceText { let path = file.path(db); let _span = tracing::trace_span!("source_text", file = %path).entered(); - let mut read_error = None; + let mut has_read_error = false; let kind = if is_notebook(file.path(db)) { file.read_to_notebook(db) .unwrap_or_else(|error| { tracing::debug!("Failed to read notebook '{path}': {error}"); - read_error = Some(SourceTextError::FailedToReadNotebook(error.to_string())); + CompileDiagnostic::report( + db, + SourceTextDiagnostic { + error: SourceTextError::FailedToReadNotebook(error), + file, + }, + ); + + has_read_error = true; + Notebook::empty() }) .into() @@ -30,8 +41,16 @@ pub fn source_text(db: &dyn Db, file: File) -> SourceText { file.read_to_string(db) .unwrap_or_else(|error| { tracing::debug!("Failed to read file '{path}': {error}"); + CompileDiagnostic::report( + db, + SourceTextDiagnostic { + error: SourceTextError::FailedToReadFile(error), + file, + }, + ); + + has_read_error = true; - read_error = Some(SourceTextError::FailedToReadFile(error.to_string())); String::new() }) .into() @@ -40,7 +59,7 @@ pub fn source_text(db: &dyn Db, file: File) -> SourceText { SourceText { inner: Arc::new(SourceTextInner { kind, - read_error, + has_read_error, count: Count::new(), }), } @@ -93,8 +112,8 @@ impl SourceText { } /// Returns `true` if there was an error when reading the content of the file. - pub fn read_error(&self) -> Option<&SourceTextError> { - self.inner.read_error.as_ref() + pub fn has_read_error(&self) -> bool { + self.inner.has_read_error } } @@ -127,7 +146,7 @@ impl std::fmt::Debug for SourceText { struct SourceTextInner { count: Count, kind: SourceTextKind, - read_error: Option, + has_read_error: bool, } #[derive(Eq, PartialEq)] @@ -148,12 +167,45 @@ impl From for SourceTextKind { } } -#[derive(Debug, thiserror::Error, PartialEq, Eq, Clone)] +#[derive(Debug)] +pub(crate) struct SourceTextDiagnostic { + error: SourceTextError, + file: File, +} + +impl Diagnostic for SourceTextDiagnostic { + fn rule(&self) -> &str { + "io-error" + } + + fn message(&self) -> Cow { + match &self.error { + SourceTextError::FailedToReadNotebook(notebook_error) => { + format!("Failed to read notebook: {notebook_error}").into() + } + SourceTextError::FailedToReadFile(error) => { + format!("Failed to read file: {error}").into() + } + } + } + + fn file(&self) -> File { + self.file + } + + fn range(&self) -> Option { + None + } + + fn severity(&self) -> Severity { + Severity::Error + } +} + +#[derive(Debug)] pub enum SourceTextError { - #[error("Failed to read notebook: {0}`")] - FailedToReadNotebook(String), - #[error("Failed to read file: {0}")] - FailedToReadFile(String), + FailedToReadNotebook(NotebookError), + FailedToReadFile(std::io::Error), } /// Computes the [`LineIndex`] for `file`. diff --git a/crates/ruff_graph/src/db.rs b/crates/ruff_graph/src/db.rs index 39f74081bc83d..cba140e55926f 100644 --- a/crates/ruff_graph/src/db.rs +++ b/crates/ruff_graph/src/db.rs @@ -14,7 +14,7 @@ static EMPTY_VENDORED: std::sync::LazyLock = std::sync::Lazy }); #[salsa::db] -#[derive(Default)] +#[derive(Default, Clone)] pub struct ModuleDb { storage: salsa::Storage, files: Files, @@ -52,16 +52,6 @@ impl ModuleDb { Ok(db) } - - /// Create a snapshot of the current database. - #[must_use] - pub fn snapshot(&self) -> Self { - Self { - storage: self.storage.clone(), - system: self.system.clone(), - files: self.files.snapshot(), - } - } } impl Upcast for ModuleDb {