[ty] Fix Salsa panic propagation (#24141)

## Summary

This PR fixes probably the most likely case why users saw
astral-sh/ty#1565 in their IDE.

I added handling to convert panics to diagnostics in
#17631 to `check_file_impl`.
However, this was before `check_file_impl` became a Salsa query. We have
to be a bit more careful, now that `check_file_impl` is a Salsa query.


1. We have to add an untracked read whenever we suppress a panic because
Salsa only carries over dependencies of queries that run to completion
and not of queries that panic (the assumption is that all queries
unwind). Adding an `untracked_read` ensures that the `check_file_impl`
reruns after every change. This is more often than necessary, but it is
the best we can do here without knowing the exact dependencies that were
collected up to when `check_file_impl`'s dependency panicked.
2. Suppressing all Salsa panics in `check_files_impl` is unlikely to be
what we want because it means the function still runs to completion even
when the query was cancelled. Instead, we want to propagate a
cancellation so that its `db` handle gets released as quickly as
possible to unblock any pending mutation. However, we do need some
special handling for Salsa's propagating panic to avoid regressing
#17631. For a query `A` running on
thread `a` that depends on query `B` running on thread `b`. If `B`
panics, Salsa throws the original panic on thread `b` but throws a
`Cancelled::PropagatingPanic` panic on thread `a`. Thread `b`'s panic is
the more useful one because it contains the actual panic information. We
already convert `b`'s panic to a `Diagnostic`, but we silently ignore
any `Cancelled::PropagatingPanic`. This PR also creates a propagating
panic to a diagnostic. While these panics don't contain any useful
information, they at least indicate to a user that `A` was only
partially checked. They should have a second diagnostic for `B` that
contains the full panic information (unless `B` is a query that didn't
run as part of `project.check`, e.g. `hover`).


## Test plan

I used @AlexWaygood's panda-stubs reproducer and I was no longer able to
reproduce the Salsa panic.

I plan on doing some CLI `--watch` testing tomorrow morning but this
should block code review.

Author: MichaReiser

crates/ruff_db/src/panic.rs

@@ -32,6 +32,10 @@ impl Payload {
 }
 
 impl PanicError {
+    pub fn resume_unwind(self) -> ! {
+        std::panic::resume_unwind(self.payload.0)
+    }
+
     pub fn to_diagnostic_message(&self, path: Option<impl std::fmt::Display>) -> String {
         use std::fmt::Write;
 

crates/ty_project/src/lib.rs

@@ -20,8 +20,7 @@ use ruff_db::parsed::parsed_module;
 use ruff_db::source::{SourceTextError, source_text};
 use ruff_db::system::{SystemPath, SystemPathBuf};
 use rustc_hash::FxHashSet;
-use salsa::Durability;
-use salsa::Setter;
+use salsa::{Database, Durability, Setter};
 use std::backtrace::BacktraceStatus;
 use std::collections::hash_set;
 use std::iter::FusedIterator;
@@ -319,6 +318,9 @@ impl Project {
                 for file in &files {
                     let db = db.clone();
                     let reporter = &*reporter;
+
+                    db.unwind_if_revision_cancelled();
+
                     scope.spawn(move |_| {
                         let check_file_span =
                             tracing::debug_span!(parent: project_span, "check_file", ?file);
@@ -646,10 +648,9 @@ pub(crate) fn check_file_impl(db: &dyn Db, file: File) -> Result<Box<[Diagnostic
     {
         let db = AssertUnwindSafe(db);
         match catch(&**db, file, || check_types(*db, file)) {
-            Ok(Some(type_check_diagnostics)) => {
+            Ok(type_check_diagnostics) => {
                 diagnostics.extend(type_check_diagnostics);
             }
-            Ok(None) => {}
             Err(diagnostic) => diagnostics.push(diagnostic),
         }
     }
@@ -749,16 +750,37 @@ enum IOErrorKind {
     SourceText(#[from] SourceTextError),
 }
 
-fn catch<F, R>(db: &dyn Db, file: File, f: F) -> Result<Option<R>, Diagnostic>
+fn catch<F, R>(db: &dyn Db, file: File, f: F) -> Result<R, Diagnostic>
 where
     F: FnOnce() -> R + UnwindSafe,
 {
-    match ruff_db::panic::catch_unwind(|| {
-        // Ignore salsa errors
-        salsa::Cancelled::catch(f).ok()
-    }) {
+    match ruff_db::panic::catch_unwind(f) {
         Ok(result) => Ok(result),
         Err(error) => {
+            match error.payload.downcast_ref::<salsa::Cancelled>() {
+                None => {
+                    // Add a diagnostic (by not early returning) for
+                    // any non Salsa panic (a bug in ty)
+                }
+                Some(salsa::Cancelled::PropagatedPanic) => {
+                    // Add a diagnostic for propagated Salsa panics. That is, query `A`
+                    // running on thread `a` depends on query `B` running on thread `b`
+                    // and query `B` panics. However, avoid adding such a diagnostic
+                    // if query `B` panicked because of a cancellation by calling
+                    // `unwind_if_revision_cancelled`.
+                    //
+                    // The propagated Salsa panic isn't very actionable for users,
+                    // but it can be useful to know that file A failed to type check
+                    // because file B panicked (both files will have a panic-diagnostic).
+                    db.unwind_if_revision_cancelled();
+                }
+
+                // For any pending write or local cancellation, resume the panic to abort the outer query.
+                Some(_) => {
+                    error.resume_unwind();
+                }
+            }
+
             let message = error.to_diagnostic_message(Some(file.path(db)));
             let mut diagnostic = Diagnostic::new(DiagnosticId::Panic, Severity::Fatal, message);
             diagnostic.add_bug_sub_diagnostics("%5Bpanic%5D");
@@ -790,6 +812,10 @@ where
                 });
             }
 
+            // Report an untracked read because Salsa didn't carry over
+            // the dependencies of any query called by `f` because it panicked.
+            db.report_untracked_read();
+
             Err(diagnostic)
         }
     }