Use salsa accumulators for diagnostics

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"] }

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<Box<dyn Diagnostic>>,
+        result: Vec<CompileDiagnostic>,
         revision: u64,
     },
     ApplyChanges(Vec<watch::ChangeEvent>),

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<Self>,
         files: Files,

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<Unpack<'a>>,
     },
     AnnAssign(&'a ast::StmtAnnAssign),

crates/red_knot_python_semantic/src/semantic_index/definition.rs

@@ -211,7 +211,6 @@ pub(crate) struct AssignmentDefinitionNodeRef<'a> {
     pub(crate) unpack: Option<Unpack<'a>>,
     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<ast::Expr>,
     name: AstNodeRef<ast::ExprName>,
-    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)]

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,19 +2208,21 @@ 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<Type<'db>, NotCallableError<'db>> {
         match self {
             Self::Callable { return_ty } => Ok(*return_ty),
             Self::RevealType {
                 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
             }
         }