diff --git a/crates/ruff_benchmark/benches/ty.rs b/crates/ruff_benchmark/benches/ty.rs index bd5fdaa026b5b..c36e120274082 100644 --- a/crates/ruff_benchmark/benches/ty.rs +++ b/crates/ruff_benchmark/benches/ty.rs @@ -131,7 +131,7 @@ fn benchmark_incremental(criterion: &mut Criterion) { fn setup() -> Case { let case = setup_tomllib_case(); - let result: Vec<_> = case.db.check().unwrap(); + let result: Vec<_> = case.db.check(); assert_diagnostics(&case.db, &result, EXPECTED_TOMLLIB_DIAGNOSTICS); @@ -159,7 +159,7 @@ fn benchmark_incremental(criterion: &mut Criterion) { None, ); - let result = db.check().unwrap(); + let result = db.check(); assert_eq!(result.len(), EXPECTED_TOMLLIB_DIAGNOSTICS.len()); } @@ -179,7 +179,7 @@ fn benchmark_cold(criterion: &mut Criterion) { setup_tomllib_case, |case| { let Case { db, .. } = case; - let result: Vec<_> = db.check().unwrap(); + let result: Vec<_> = db.check(); assert_diagnostics(db, &result, EXPECTED_TOMLLIB_DIAGNOSTICS); }, @@ -293,7 +293,7 @@ fn benchmark_many_string_assignments(criterion: &mut Criterion) { }, |case| { let Case { db, .. } = case; - let result = db.check().unwrap(); + let result = db.check(); assert_eq!(result.len(), 0); }, BatchSize::SmallInput, @@ -339,7 +339,7 @@ fn benchmark_many_tuple_assignments(criterion: &mut Criterion) { }, |case| { let Case { db, .. } = case; - let result = db.check().unwrap(); + let result = db.check(); assert_eq!(result.len(), 0); }, BatchSize::SmallInput, diff --git a/crates/ruff_db/src/panic.rs b/crates/ruff_db/src/panic.rs index 67ebf940dc3b9..a6b67f1f1de0b 100644 --- a/crates/ruff_db/src/panic.rs +++ b/crates/ruff_db/src/panic.rs @@ -1,3 +1,4 @@ +use std::any::Any; use std::backtrace::BacktraceStatus; use std::cell::Cell; use std::panic::Location; @@ -24,17 +25,25 @@ impl Payload { None } } + + pub fn downcast_ref(&self) -> Option<&R> { + self.0.downcast_ref::() + } } impl std::fmt::Display for PanicError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - writeln!(f, "panicked at")?; + write!(f, "panicked at")?; if let Some(location) = &self.location { write!(f, " {location}")?; } if let Some(payload) = self.payload.as_str() { write!(f, ":\n{payload}")?; } + if let Some(query_trace) = self.salsa_backtrace.as_ref() { + let _ = writeln!(f, "{query_trace}"); + } + if let Some(backtrace) = &self.backtrace { match backtrace.status() { BacktraceStatus::Disabled => { @@ -49,6 +58,7 @@ impl std::fmt::Display for PanicError { _ => {} } } + Ok(()) } } diff --git a/crates/ty/src/lib.rs b/crates/ty/src/lib.rs index 443f61602eb36..ec66b60ca57d3 100644 --- a/crates/ty/src/lib.rs +++ b/crates/ty/src/lib.rs @@ -243,12 +243,14 @@ impl MainLoop { MainLoopMessage::CheckWorkspace => { let db = db.clone(); let sender = self.sender.clone(); - let mut reporter = R::default(); // Spawn a new task that checks the project. This needs to be done in a separate thread // to prevent blocking the main loop here. rayon::spawn(move || { - match db.check_with_reporter(&mut reporter) { + match salsa::Cancelled::catch(|| { + let mut reporter = R::default(); + db.check_with_reporter(&mut reporter) + }) { Ok(result) => { // Send the result back to the main loop for printing. sender diff --git a/crates/ty/tests/file_watching.rs b/crates/ty/tests/file_watching.rs index c8f3a186b0040..848117bf51583 100644 --- a/crates/ty/tests/file_watching.rs +++ b/crates/ty/tests/file_watching.rs @@ -1135,7 +1135,7 @@ print(sys.last_exc, os.getegid()) Ok(()) })?; - let diagnostics = case.db.check().context("Failed to check project.")?; + let diagnostics = case.db.check(); assert_eq!(diagnostics.len(), 2); assert_eq!( @@ -1160,7 +1160,7 @@ print(sys.last_exc, os.getegid()) }) .expect("Search path settings to be valid"); - let diagnostics = case.db.check().context("Failed to check project.")?; + let diagnostics = case.db.check(); assert!(diagnostics.is_empty()); Ok(()) @@ -1763,10 +1763,7 @@ fn changes_to_user_configuration() -> anyhow::Result<()> { let foo = case .system_file(case.project_path("foo.py")) .expect("foo.py to exist"); - let diagnostics = case - .db() - .check_file(foo) - .context("Failed to check project.")?; + let diagnostics = case.db().check_file(foo); assert!( diagnostics.is_empty(), @@ -1786,10 +1783,7 @@ fn changes_to_user_configuration() -> anyhow::Result<()> { case.apply_changes(changes); - let diagnostics = case - .db() - .check_file(foo) - .context("Failed to check project.")?; + let diagnostics = case.db().check_file(foo); assert!( diagnostics.len() == 1, diff --git a/crates/ty_project/src/db.rs b/crates/ty_project/src/db.rs index 14fb3ae24471a..9af4e7e50e80f 100644 --- a/crates/ty_project/src/db.rs +++ b/crates/ty_project/src/db.rs @@ -8,8 +8,8 @@ use ruff_db::files::{File, Files}; use ruff_db::system::System; use ruff_db::vendored::VendoredFileSystem; use ruff_db::{Db as SourceDb, Upcast}; +use salsa::Event; use salsa::plumbing::ZalsaDatabase; -use salsa::{Cancelled, Event}; use ty_ide::Db as IdeDb; use ty_python_semantic::lint::{LintRegistry, RuleSelection}; use ty_python_semantic::{Db as SemanticDb, Program}; @@ -76,24 +76,21 @@ impl ProjectDatabase { } /// Checks all open files in the project and its dependencies. - pub fn check(&self) -> Result, Cancelled> { + pub fn check(&self) -> Vec { let mut reporter = DummyReporter; let reporter = AssertUnwindSafe(&mut reporter as &mut dyn Reporter); - self.with_db(|db| db.project().check(db, reporter)) + self.project().check(self, reporter) } /// Checks all open files in the project and its dependencies, using the given reporter. - pub fn check_with_reporter( - &self, - reporter: &mut dyn Reporter, - ) -> Result, Cancelled> { + pub fn check_with_reporter(&self, reporter: &mut dyn Reporter) -> Vec { let reporter = AssertUnwindSafe(reporter); - self.with_db(|db| db.project().check(db, reporter)) + self.project().check(self, reporter) } #[tracing::instrument(level = "debug", skip(self))] - pub fn check_file(&self, file: File) -> Result, Cancelled> { - self.with_db(|db| self.project().check_file(db, file)) + pub fn check_file(&self, file: File) -> Vec { + self.project().check_file(self, file) } /// Returns a mutable reference to the system. @@ -107,13 +104,6 @@ impl ProjectDatabase { Arc::get_mut(&mut self.system) .expect("ref count should be 1 because `zalsa_mut` drops all other DB references.") } - - pub(crate) fn with_db(&self, f: F) -> Result - where - F: FnOnce(&ProjectDatabase) -> T + std::panic::UnwindSafe, - { - Cancelled::catch(|| f(self)) - } } impl Upcast for ProjectDatabase { diff --git a/crates/ty_server/src/server.rs b/crates/ty_server/src/server.rs index 36694eb2dafb1..bb030ad212fb3 100644 --- a/crates/ty_server/src/server.rs +++ b/crates/ty_server/src/server.rs @@ -1,10 +1,5 @@ //! Scheduling, I/O, and API endpoints. -use std::num::NonZeroUsize; -// The new PanicInfoHook name requires MSRV >= 1.82 -#[expect(deprecated)] -use std::panic::PanicInfo; - use lsp_server::Message; use lsp_types::{ ClientCapabilities, DiagnosticOptions, DiagnosticServerCapabilities, @@ -13,6 +8,8 @@ use lsp_types::{ TextDocumentSyncCapability, TextDocumentSyncKind, TextDocumentSyncOptions, TypeDefinitionProviderCapability, Url, }; +use std::num::NonZeroUsize; +use std::panic::PanicHookInfo; use self::connection::{Connection, ConnectionInitializer}; use self::schedule::event_loop_thread; @@ -125,9 +122,7 @@ impl Server { } pub(crate) fn run(self) -> crate::Result<()> { - // The new PanicInfoHook name requires MSRV >= 1.82 - #[expect(deprecated)] - type PanicHook = Box) + 'static + Sync + Send>; + type PanicHook = Box) + 'static + Sync + Send>; struct RestorePanicHook { hook: Option, } diff --git a/crates/ty_server/src/server/api.rs b/crates/ty_server/src/server/api.rs index a9dd9da1d0272..2b215004683bb 100644 --- a/crates/ty_server/src/server/api.rs +++ b/crates/ty_server/src/server/api.rs @@ -3,42 +3,41 @@ use crate::session::Session; use crate::system::{AnySystemPath, url_to_any_system_path}; use anyhow::anyhow; use lsp_server as server; +use lsp_server::RequestId; use lsp_types::notification::Notification; +use ruff_db::panic::PanicError; +use std::panic::UnwindSafe; mod diagnostics; mod notifications; mod requests; mod traits; -use notifications as notification; -use requests as request; - use self::traits::{NotificationHandler, RequestHandler}; - use super::{Result, client::Responder, schedule::BackgroundSchedule}; pub(super) fn request<'a>(req: server::Request) -> Task<'a> { let id = req.id.clone(); match req.method.as_str() { - request::DocumentDiagnosticRequestHandler::METHOD => background_request_task::< - request::DocumentDiagnosticRequestHandler, + requests::DocumentDiagnosticRequestHandler::METHOD => background_request_task::< + requests::DocumentDiagnosticRequestHandler, >( req, BackgroundSchedule::Worker ), - request::GotoTypeDefinitionRequestHandler::METHOD => background_request_task::< - request::GotoTypeDefinitionRequestHandler, + requests::GotoTypeDefinitionRequestHandler::METHOD => background_request_task::< + requests::GotoTypeDefinitionRequestHandler, >( req, BackgroundSchedule::Worker ), - request::HoverRequestHandler::METHOD => { - background_request_task::(req, BackgroundSchedule::Worker) - } - request::InlayHintRequestHandler::METHOD => background_request_task::< - request::InlayHintRequestHandler, + requests::HoverRequestHandler::METHOD => background_request_task::< + requests::HoverRequestHandler, + >(req, BackgroundSchedule::Worker), + requests::InlayHintRequestHandler::METHOD => background_request_task::< + requests::InlayHintRequestHandler, >(req, BackgroundSchedule::Worker), - request::CompletionRequestHandler::METHOD => background_request_task::< - request::CompletionRequestHandler, + requests::CompletionRequestHandler::METHOD => background_request_task::< + requests::CompletionRequestHandler, >( req, BackgroundSchedule::LatencySensitive ), @@ -64,23 +63,23 @@ pub(super) fn request<'a>(req: server::Request) -> Task<'a> { pub(super) fn notification<'a>(notif: server::Notification) -> Task<'a> { match notif.method.as_str() { - notification::DidCloseTextDocumentHandler::METHOD => { - local_notification_task::(notif) + notifications::DidCloseTextDocumentHandler::METHOD => { + local_notification_task::(notif) } - notification::DidOpenTextDocumentHandler::METHOD => { - local_notification_task::(notif) + notifications::DidOpenTextDocumentHandler::METHOD => { + local_notification_task::(notif) } - notification::DidChangeTextDocumentHandler::METHOD => { - local_notification_task::(notif) + notifications::DidChangeTextDocumentHandler::METHOD => { + local_notification_task::(notif) } - notification::DidOpenNotebookHandler::METHOD => { - local_notification_task::(notif) + notifications::DidOpenNotebookHandler::METHOD => { + local_notification_task::(notif) } - notification::DidCloseNotebookHandler::METHOD => { - local_notification_task::(notif) + notifications::DidCloseNotebookHandler::METHOD => { + local_notification_task::(notif) } - notification::DidChangeWatchedFiles::METHOD => { - local_notification_task::(notif) + notifications::DidChangeWatchedFiles::METHOD => { + local_notification_task::(notif) } lsp_types::notification::SetTrace::METHOD => { tracing::trace!("Ignoring `setTrace` notification"); @@ -103,23 +102,25 @@ pub(super) fn notification<'a>(notif: server::Notification) -> Task<'a> { fn _local_request_task<'a, R: traits::SyncRequestHandler>( req: server::Request, -) -> super::Result> { +) -> super::Result> +where + <::RequestType as lsp_types::request::Request>::Params: UnwindSafe, +{ let (id, params) = cast_request::(req)?; Ok(Task::local(|session, notifier, requester, responder| { - let _span = tracing::trace_span!("request", %id, method = R::METHOD).entered(); + let _span = tracing::debug_span!("request", %id, method = R::METHOD).entered(); let result = R::run(session, notifier, requester, params); respond::(id, result, &responder); })) } -// TODO(micha): Calls to `db` could panic if the db gets mutated while this task is running. -// We should either wrap `R::run_with_snapshot` with a salsa catch cancellation handler or -// use `SemanticModel` instead of passing `db` which uses a Result for all it's methods -// that propagate cancellations. fn background_request_task<'a, R: traits::BackgroundDocumentRequestHandler>( req: server::Request, schedule: BackgroundSchedule, -) -> super::Result> { +) -> super::Result> +where + <::RequestType as lsp_types::request::Request>::Params: UnwindSafe, +{ let (id, params) = cast_request::(req)?; Ok(Task::background(schedule, move |session: &Session| { let url = R::document_url(¶ms).into_owned(); @@ -128,6 +129,7 @@ fn background_request_task<'a, R: traits::BackgroundDocumentRequestHandler>( tracing::warn!("Ignoring request for invalid `{url}`"); return Box::new(|_, _| {}); }; + let db = match &path { AnySystemPath::System(path) => match session.project_db_for_path(path.as_std_path()) { Some(db) => db.clone(), @@ -142,19 +144,55 @@ fn background_request_task<'a, R: traits::BackgroundDocumentRequestHandler>( }; Box::new(move |notifier, responder| { - let _span = tracing::trace_span!("request", %id, method = R::METHOD).entered(); - let result = R::run_with_snapshot(&db, snapshot, notifier, params); - respond::(id, result, &responder); + let _span = tracing::debug_span!("request", %id, method = R::METHOD).entered(); + let result = ruff_db::panic::catch_unwind(|| { + R::run_with_snapshot(&db, snapshot, notifier, params) + }); + + if let Some(response) = request_result_to_response(&id, &responder, result) { + respond::(id, response, &responder); + } }) })) } +fn request_result_to_response( + id: &RequestId, + responder: &Responder, + result: std::result::Result, PanicError>, +) -> Option> { + match result { + Ok(response) => Some(response), + Err(error) => { + if error.payload.downcast_ref::().is_some() { + // Request was cancelled by Salsa. TODO: Retry + respond_silent_error( + id.clone(), + responder, + Error { + code: lsp_server::ErrorCode::ContentModified, + error: anyhow!("content modified"), + }, + ); + None + } else { + let message = format!("request handler {error}"); + + Some(Err(Error { + code: lsp_server::ErrorCode::InternalError, + error: anyhow!(message), + })) + } + } + } +} + fn local_notification_task<'a, N: traits::SyncNotificationHandler>( notif: server::Notification, ) -> super::Result> { let (id, params) = cast_notification::(notif)?; Ok(Task::local(move |session, notifier, requester, _| { - let _span = tracing::trace_span!("notification", method = N::METHOD).entered(); + let _span = tracing::debug_span!("notification", method = N::METHOD).entered(); if let Err(err) = N::run(session, notifier, requester, params) { tracing::error!("An error occurred while running {id}: {err}"); show_err_msg!("ty encountered a problem. Check the logs for more details."); @@ -163,10 +201,15 @@ fn local_notification_task<'a, N: traits::SyncNotificationHandler>( } #[expect(dead_code)] -fn background_notification_thread<'a, N: traits::BackgroundDocumentNotificationHandler>( +fn background_notification_thread<'a, N>( req: server::Notification, schedule: BackgroundSchedule, -) -> super::Result> { +) -> super::Result> +where + N: traits::BackgroundDocumentNotificationHandler, + <::NotificationType as lsp_types::notification::Notification>::Params: + UnwindSafe, +{ let (id, params) = cast_notification::(req)?; Ok(Task::background(schedule, move |session: &Session| { let url = N::document_url(¶ms); @@ -177,8 +220,20 @@ fn background_notification_thread<'a, N: traits::BackgroundDocumentNotificationH return Box::new(|_, _| {}); }; Box::new(move |notifier, _| { - let _span = tracing::trace_span!("notification", method = N::METHOD).entered(); - if let Err(err) = N::run_with_snapshot(snapshot, notifier, params) { + let _span = tracing::debug_span!("notification", method = N::METHOD).entered(); + + let result = match ruff_db::panic::catch_unwind(|| { + N::run_with_snapshot(snapshot, notifier, params) + }) { + Ok(result) => result, + Err(panic) => { + tracing::error!("An error occurred while running {id}: {panic}"); + show_err_msg!("ty encountered a panic. Check the logs for more details."); + return; + } + }; + + if let Err(err) = result { tracing::error!("An error occurred while running {id}: {err}"); show_err_msg!("ty encountered a problem. Check the logs for more details."); } @@ -198,6 +253,7 @@ fn cast_request( )> where Req: traits::RequestHandler, + <::RequestType as lsp_types::request::Request>::Params: UnwindSafe, { request .extract(Req::METHOD) @@ -232,6 +288,14 @@ fn respond( } } +/// Sends back an error response to the server using a [`Responder`] without showing a warning +/// to the user. +fn respond_silent_error(id: server::RequestId, responder: &Responder, error: Error) { + if let Err(err) = responder.respond::<()>(id, Err(error)) { + tracing::error!("Failed to send response: {err}"); + } +} + /// Tries to cast a serialized request from the server into /// a parameter type for a specific request handler. fn cast_notification( @@ -240,7 +304,9 @@ fn cast_notification( ( &'static str, <::NotificationType as lsp_types::notification::Notification>::Params, -)> where N: traits::NotificationHandler{ + )> where + N: traits::NotificationHandler, +{ Ok(( N::METHOD, notification diff --git a/crates/ty_server/src/server/api/diagnostics.rs b/crates/ty_server/src/server/api/diagnostics.rs index 367f65b96ee2a..b0cdb9a7b7d3c 100644 --- a/crates/ty_server/src/server/api/diagnostics.rs +++ b/crates/ty_server/src/server/api/diagnostics.rs @@ -41,13 +41,7 @@ pub(super) fn compute_diagnostics( return vec![]; }; - let diagnostics = match db.check_file(file) { - Ok(diagnostics) => diagnostics, - Err(cancelled) => { - tracing::info!("Diagnostics computation {cancelled}"); - return vec![]; - } - }; + let diagnostics = db.check_file(file); diagnostics .as_slice() diff --git a/crates/ty_wasm/src/lib.rs b/crates/ty_wasm/src/lib.rs index 4ce30936e65de..839442852644c 100644 --- a/crates/ty_wasm/src/lib.rs +++ b/crates/ty_wasm/src/lib.rs @@ -187,14 +187,14 @@ impl Workspace { /// Checks a single file. #[wasm_bindgen(js_name = "checkFile")] pub fn check_file(&self, file_id: &FileHandle) -> Result, Error> { - let result = self.db.check_file(file_id.file).map_err(into_error)?; + let result = self.db.check_file(file_id.file); Ok(result.into_iter().map(Diagnostic::wrap).collect()) } /// Checks all open files pub fn check(&self) -> Result, Error> { - let result = self.db.check().map_err(into_error)?; + let result = self.db.check(); Ok(result.into_iter().map(Diagnostic::wrap).collect()) }