Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 53 additions & 8 deletions crates/ty_server/src/server/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,23 @@ pub(super) fn request(req: server::Request) -> Task {
let id = req.id.clone();

match req.method.as_str() {
requests::DocumentDiagnosticRequestHandler::METHOD => background_request_task::<
requests::DocumentDiagnosticRequestHandler::METHOD => background_document_request_task::<
requests::DocumentDiagnosticRequestHandler,
>(
req, BackgroundSchedule::Worker
),
requests::GotoTypeDefinitionRequestHandler::METHOD => background_request_task::<
requests::GotoTypeDefinitionRequestHandler::METHOD => background_document_request_task::<
requests::GotoTypeDefinitionRequestHandler,
>(
req, BackgroundSchedule::Worker
),
requests::HoverRequestHandler::METHOD => background_request_task::<
requests::HoverRequestHandler::METHOD => background_document_request_task::<
requests::HoverRequestHandler,
>(req, BackgroundSchedule::Worker),
requests::InlayHintRequestHandler::METHOD => background_request_task::<
requests::InlayHintRequestHandler::METHOD => background_document_request_task::<
requests::InlayHintRequestHandler,
>(req, BackgroundSchedule::Worker),
requests::CompletionRequestHandler::METHOD => background_request_task::<
requests::CompletionRequestHandler::METHOD => background_document_request_task::<
requests::CompletionRequestHandler,
>(
req, BackgroundSchedule::LatencySensitive
Expand Down Expand Up @@ -135,7 +135,52 @@ where
}))
}

fn background_request_task<R: traits::BackgroundDocumentRequestHandler>(
#[expect(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment saying why this code is kept around even though it isn't used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split the workspace diagnostics work into two PRs to ease up the review process. This is going to be used in the follow-up PR (#18939). Sorry, I should've highlighted this as a review comment!

fn background_request_task<R: traits::BackgroundRequestHandler>(
req: server::Request,
schedule: BackgroundSchedule,
) -> Result<Task>
where
<<R as RequestHandler>::RequestType as Request>::Params: UnwindSafe,
{
let retry = R::RETRY_ON_CANCELLATION.then(|| req.clone());
let (id, params) = cast_request::<R>(req)?;

Ok(Task::background(schedule, move |session: &Session| {
let cancellation_token = session
.request_queue()
.incoming()
.cancellation_token(&id)
.expect("request should have been tested for cancellation before scheduling");

let snapshot = session.take_workspace_snapshot();

Box::new(move |client| {
let _span = tracing::debug_span!("request", %id, method = R::METHOD).entered();

// Test again if the request was cancelled since it was scheduled on the background task
// and, if so, return early
if cancellation_token.is_cancelled() {
tracing::trace!(
"Ignoring request id={id} method={} because it was cancelled",
R::METHOD
);

// We don't need to send a response here because the `cancel` notification
// handler already responded with a message.
return;
}

let result = ruff_db::panic::catch_unwind(|| R::run(snapshot, client, params));

if let Some(response) = request_result_to_response::<R>(&id, client, result, retry) {
respond::<R>(&id, response, client);
}
})
}))
}

fn background_document_request_task<R: traits::BackgroundDocumentRequestHandler>(
req: server::Request,
schedule: BackgroundSchedule,
) -> Result<Task>
Expand Down Expand Up @@ -168,7 +213,7 @@ where
};

let Some(snapshot) = session.take_snapshot(url) else {
tracing::warn!("Ignoring request because snapshot for path `{path:?}` doesn't exist.");
tracing::warn!("Ignoring request because snapshot for path `{path:?}` doesn't exist");
return Box::new(|_| {});
};

Expand Down Expand Up @@ -209,7 +254,7 @@ fn request_result_to_response<R>(
request: Option<lsp_server::Request>,
) -> Option<Result<<<R as RequestHandler>::RequestType as Request>::Result>>
where
R: traits::BackgroundDocumentRequestHandler,
R: traits::RetriableRequestHandler,
{
match result {
Ok(response) => Some(response),
Expand Down
10 changes: 7 additions & 3 deletions crates/ty_server/src/server/api/requests/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ use ty_project::ProjectDatabase;

use crate::DocumentSnapshot;
use crate::document::PositionExt;
use crate::server::api::traits::{BackgroundDocumentRequestHandler, RequestHandler};
use crate::server::api::traits::{
BackgroundDocumentRequestHandler, RequestHandler, RetriableRequestHandler,
};
use crate::session::client::Client;

pub(crate) struct CompletionRequestHandler;
Expand All @@ -18,8 +20,6 @@ impl RequestHandler for CompletionRequestHandler {
}

impl BackgroundDocumentRequestHandler for CompletionRequestHandler {
const RETRY_ON_CANCELLATION: bool = true;

fn document_url(params: &CompletionParams) -> Cow<Url> {
Cow::Borrowed(&params.text_document_position.text_document.uri)
}
Expand Down Expand Up @@ -65,3 +65,7 @@ impl BackgroundDocumentRequestHandler for CompletionRequestHandler {
Ok(Some(response))
}
}

impl RetriableRequestHandler for CompletionRequestHandler {
const RETRY_ON_CANCELLATION: bool = true;
}
6 changes: 5 additions & 1 deletion crates/ty_server/src/server/api/requests/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ use lsp_types::{

use crate::server::Result;
use crate::server::api::diagnostics::{Diagnostics, compute_diagnostics};
use crate::server::api::traits::{BackgroundDocumentRequestHandler, RequestHandler};
use crate::server::api::traits::{
BackgroundDocumentRequestHandler, RequestHandler, RetriableRequestHandler,
};
use crate::session::DocumentSnapshot;
use crate::session::client::Client;
use ty_project::ProjectDatabase;
Expand Down Expand Up @@ -43,7 +45,9 @@ impl BackgroundDocumentRequestHandler for DocumentDiagnosticRequestHandler {
}),
))
}
}

impl RetriableRequestHandler for DocumentDiagnosticRequestHandler {
fn salsa_cancellation_error() -> lsp_server::ResponseError {
lsp_server::ResponseError {
code: lsp_server::ErrorCode::ServerCancelled as i32,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ use ty_project::ProjectDatabase;

use crate::DocumentSnapshot;
use crate::document::{PositionExt, ToLink};
use crate::server::api::traits::{BackgroundDocumentRequestHandler, RequestHandler};
use crate::server::api::traits::{
BackgroundDocumentRequestHandler, RequestHandler, RetriableRequestHandler,
};
use crate::session::client::Client;

pub(crate) struct GotoTypeDefinitionRequestHandler;
Expand Down Expand Up @@ -70,3 +72,5 @@ impl BackgroundDocumentRequestHandler for GotoTypeDefinitionRequestHandler {
}
}
}

impl RetriableRequestHandler for GotoTypeDefinitionRequestHandler {}
6 changes: 5 additions & 1 deletion crates/ty_server/src/server/api/requests/hover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ use std::borrow::Cow;

use crate::DocumentSnapshot;
use crate::document::{PositionExt, ToRangeExt};
use crate::server::api::traits::{BackgroundDocumentRequestHandler, RequestHandler};
use crate::server::api::traits::{
BackgroundDocumentRequestHandler, RequestHandler, RetriableRequestHandler,
};
use crate::session::client::Client;
use lsp_types::request::HoverRequest;
use lsp_types::{HoverContents, HoverParams, MarkupContent, Url};
Expand Down Expand Up @@ -73,3 +75,5 @@ impl BackgroundDocumentRequestHandler for HoverRequestHandler {
}))
}
}

impl RetriableRequestHandler for HoverRequestHandler {}
6 changes: 5 additions & 1 deletion crates/ty_server/src/server/api/requests/inlay_hints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ use std::borrow::Cow;

use crate::DocumentSnapshot;
use crate::document::{RangeExt, TextSizeExt};
use crate::server::api::traits::{BackgroundDocumentRequestHandler, RequestHandler};
use crate::server::api::traits::{
BackgroundDocumentRequestHandler, RequestHandler, RetriableRequestHandler,
};
use crate::session::client::Client;
use lsp_types::request::InlayHintRequest;
use lsp_types::{InlayHintParams, Url};
Expand Down Expand Up @@ -64,3 +66,5 @@ impl BackgroundDocumentRequestHandler for InlayHintRequestHandler {
Ok(Some(inlay_hints))
}
}

impl RetriableRequestHandler for InlayHintRequestHandler {}
36 changes: 25 additions & 11 deletions crates/ty_server/src/server/api/traits.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! A stateful LSP implementation that calls into the ty API.

use crate::session::client::Client;
use crate::session::{DocumentSnapshot, Session};
use crate::session::{DocumentSnapshot, Session, WorkspaceSnapshot};

use lsp_types::notification::Notification as LSPNotification;
use lsp_types::request::Request;
Expand All @@ -25,11 +25,24 @@ pub(super) trait SyncRequestHandler: RequestHandler {
) -> super::Result<<<Self as RequestHandler>::RequestType as Request>::Result>;
}

/// A request handler that can be run on a background thread.
pub(super) trait BackgroundDocumentRequestHandler: RequestHandler {
/// Whether this request be retried if it was cancelled due to a modification to the Salsa database.
pub(super) trait RetriableRequestHandler: RequestHandler {
/// Whether this request can be cancelled if the Salsa database is modified.
const RETRY_ON_CANCELLATION: bool = false;

/// The error to return if the request was cancelled due to a modification to the Salsa database.
fn salsa_cancellation_error() -> lsp_server::ResponseError {
lsp_server::ResponseError {
code: lsp_server::ErrorCode::ContentModified as i32,
message: "content modified".to_string(),
data: None,
}
}
}

/// A request handler that can be run on a background thread.
///
/// This handler is specific to requests that operate on a single document.
pub(super) trait BackgroundDocumentRequestHandler: RetriableRequestHandler {
fn document_url(
params: &<<Self as RequestHandler>::RequestType as Request>::Params,
) -> std::borrow::Cow<lsp_types::Url>;
Expand All @@ -40,14 +53,15 @@ pub(super) trait BackgroundDocumentRequestHandler: RequestHandler {
client: &Client,
params: <<Self as RequestHandler>::RequestType as Request>::Params,
) -> super::Result<<<Self as RequestHandler>::RequestType as Request>::Result>;
}

fn salsa_cancellation_error() -> lsp_server::ResponseError {
lsp_server::ResponseError {
code: lsp_server::ErrorCode::ContentModified as i32,
message: "content modified".to_string(),
data: None,
}
}
/// A request handler that can be run on a background thread.
pub(super) trait BackgroundRequestHandler: RetriableRequestHandler {
fn run(
snapshot: WorkspaceSnapshot,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should call this SessionSnapshot instead. It's unfortunate that the LSP specification made such a mess of the term workspace (which can refer to an open folder, or all open foldlers).

client: &Client,
params: <<Self as RequestHandler>::RequestType as Request>::Params,
) -> super::Result<<<Self as RequestHandler>::RequestType as Request>::Result>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I'd find helpful here is some prose (perhaps on the module doc) that briefly explains the traits here and how they're connected.

If you think this code is going to be heavily refactored soon, then it might not make sense to add docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely! Most of this stuff is currently in my and Micha's mind but it would be useful to have it written down. I'll prioritize it soon as there are now more people who'll be working on the server.

}

/// A supertrait for any server notification handler.
Expand Down
31 changes: 31 additions & 0 deletions crates/ty_server/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use std::collections::{BTreeMap, VecDeque};
use std::ops::{Deref, DerefMut};
use std::panic::AssertUnwindSafe;
use std::sync::Arc;

use anyhow::{Context, anyhow};
Expand Down Expand Up @@ -223,6 +224,14 @@ impl Session {
self.index().key_from_url(url)
}

pub(crate) fn take_workspace_snapshot(&self) -> WorkspaceSnapshot {
WorkspaceSnapshot {
projects: AssertUnwindSafe(self.projects.values().cloned().collect()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest removing the AssertUnwindSafe here and insteaed wrap the entire snapshot in crates/ty_server/src/server/api.rs in an AssertUnwindSafe and add a comment saying that it's safe to move it across unwind boundaries because the value isn't used after unwinding.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean that the snapshot parameter type of BackgroundRequestHandler::run would need to be AssertUnwindSafe<SessionSnapshot>. Or, are you suggesting to wrap the AssertUnwindSafe on the entire closure that is the first argument to catch_unwind?

Copy link
Member Author

@dhruvmanila dhruvmanila Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The former doesn't seems to work as the compiler still errors stating that some reference may not be safe to be transferred across the catch_unwind boundary.

Edit: Nevermind, it works.

index: self.index.clone().unwrap(),
position_encoding: self.position_encoding,
}
}

pub(crate) fn initialize_workspaces(&mut self, workspace_settings: Vec<(Url, ClientOptions)>) {
assert!(!self.workspaces.all_initialized());

Expand Down Expand Up @@ -453,6 +462,28 @@ impl DocumentSnapshot {
}
}

/// An immutable snapshot of the current state of [`Session`].
pub(crate) struct WorkspaceSnapshot {
projects: AssertUnwindSafe<Vec<ProjectDatabase>>,
index: Arc<index::Index>,
position_encoding: PositionEncoding,
}

#[expect(dead_code)]
impl WorkspaceSnapshot {
pub(crate) fn projects(&self) -> &[ProjectDatabase] {
&self.projects
}

pub(crate) fn index(&self) -> &index::Index {
&self.index
}

pub(crate) fn position_encoding(&self) -> PositionEncoding {
self.position_encoding
}
}

#[derive(Debug, Default)]
pub(crate) struct Workspaces {
workspaces: BTreeMap<Url, Workspace>,
Expand Down
Loading