Skip to content

fix: switch to gen-lsp-types #2494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ribru17 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: switch to gen-lsp-types #2494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 28 additions & 29 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ typstyle-core = { version = "=0.14.0", default-features = false }
# LSP
crossbeam-channel = "0.5.15"
dapts = "0.0.6"
lsp-types = { version = "=0.95.0", features = ["proposed"] }
lsp-types = { package = "gen-lsp-types", version = "0.5.0", features = ["url"] }

# CLI
clap = { version = "4.5", features = ["derive", "env", "unicode"] }
Expand Down
5 changes: 3 additions & 2 deletions crates/sync-lsp/src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use std::sync::atomic::AtomicU32;
use std::sync::{Arc, Weak};

use futures::future::MaybeDone;
use lsp_types::{LspNotificationMethod, LspRequestMethod};
use parking_lot::Mutex;
use serde::Serialize;
use serde_json::{Value as JsonValue, from_value};
Expand Down Expand Up @@ -614,8 +615,8 @@ type RawHandler<S, T> = fn(srv: &mut S, args: T) -> ScheduleResult;
type BoxPureHandler<S, T> = Box<dyn Fn(&mut S, T) -> LspResult<()>>;
type BoxHandler<S, T> = Box<dyn Fn(&mut S, T) -> SchedulableResponse<JsonValue>>;
type ExecuteCmdMap<S> = HashMap<&'static str, BoxHandler<S, Vec<JsonValue>>>;
type RegularCmdMap<S> = HashMap<&'static str, BoxHandler<S, JsonValue>>;
type NotifyCmdMap<S> = HashMap<&'static str, BoxPureHandler<S, JsonValue>>;
type RegularCmdMap<S> = HashMap<LspRequestMethod, BoxHandler<S, JsonValue>>;
type NotifyCmdMap<S> = HashMap<LspNotificationMethod, BoxPureHandler<S, JsonValue>>;
type ResourceMap<S> = HashMap<ImmutPath, BoxHandler<S, Vec<JsonValue>>>;
type MayInitBoxHandler<A, S, T> =
Box<dyn for<'a> Fn(ServiceState<'a, A, S>, &LspClient, T) -> anyhow::Result<()>>;
Expand Down
11 changes: 6 additions & 5 deletions crates/sync-lsp/src/server/dap_srv.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ where
mut self,
handler: RawHandler<Args::S, JsonValue>,
) -> Self {
self.req_handlers.insert(R::COMMAND, Box::new(handler));
self.req_handlers
.insert(R::COMMAND.into(), Box::new(handler));
self
}

Expand All @@ -54,7 +55,7 @@ where
handler: fn(&mut Args::S, R::Arguments) -> ScheduleResult,
) -> Self {
self.req_handlers.insert(
R::COMMAND,
R::COMMAND.into(),
Box::new(move |s, req| handler(s, from_json(req)?)),
);
self
Expand All @@ -66,7 +67,7 @@ where
handler: AsyncHandler<Args::S, R::Arguments, R::Response>,
) -> Self {
self.req_handlers.insert(
R::COMMAND,
R::COMMAND.into(),
Box::new(move |s, req| erased_response(handler(s, from_json(req)?))),
);
self
Expand Down Expand Up @@ -222,7 +223,7 @@ where

let is_disconnect = method == dapts::request::Disconnect::COMMAND;

let Some(handler) = self.requests.get(method) else {
let Some(handler) = self.requests.get(&method.to_owned().into()) else {
Copy link
Copy Markdown
Owner

@Myriad-Dreamin Myriad-Dreamin May 10, 2026

Choose a reason for hiding this comment

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

I'm not pretty in flavor of converting string enums into LspRequestMethod or LspNotificationMethod everywhere. And this case is even worse, for a DAP server, every "DapCommandMethod" and "DapEventMethod" are converted to LspRequestMethod::Custom and LspNotificationMethod::Custom and requires extra allocations.
Context: LSP and DAP both share a same base protocol.

What's your idea?

Copy link
Copy Markdown
Author

@ribru17 ribru17 May 13, 2026

Choose a reason for hiding this comment

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

Good point... maybe it's better to remove the typing guarantee and keep these keys as &str. This pain point will be largely mitigated by implementing the suggestion from my previous comment, but the Custom dap request/notification variants will still be present since the LSP metamodel is unaware of the DAP specification.

log::warn!("unhandled dap request: {method}");
break 'serve_req just_result(Err(method_not_found()));
};
Expand Down Expand Up @@ -251,7 +252,7 @@ where
event,
body,
}: dap::Event| {
let Some(handler) = self.notifications.get(event.as_str()) else {
let Some(handler) = self.notifications.get(&event.clone().into()) else {
log::warn!("unhandled event: {event}");
return Ok(());
};
Expand Down
37 changes: 20 additions & 17 deletions crates/sync-lsp/src/server/lsp_srv.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use lsp_types::{notification::Notification as Notif, request::Request as Req, *};
use lsp_types::{Notification as Notif, Request as Req, *};

use super::*;

Expand Down Expand Up @@ -27,7 +27,7 @@ impl LspClient {
) {
let mut req_queue = self.req_queue.lock();
let request = req_queue.outgoing.register(
R::METHOD.to_owned(),
R::METHOD.to_string(),
params,
Box::new(|s, resp| handler(s, resp.try_into().unwrap())),
);
Expand All @@ -42,7 +42,7 @@ impl LspClient {

/// Sends a typed notification to the client.
pub fn send_notification<N: Notif>(&self, params: &N::Params) {
self.send_notification_(lsp::Notification::new(N::METHOD.to_owned(), params));
self.send_notification_(lsp::Notification::new(N::METHOD.to_string(), params));
}

/// Sends an untyped notification to the client.
Expand Down Expand Up @@ -199,7 +199,7 @@ where
// }

while let Ok(msg) = inbox.recv() {
const EXIT_METHOD: &str = notification::Exit::METHOD;
const EXIT_METHOD: LspNotificationMethod = ExitNotification::METHOD;
let loop_start = tinymist_std::time::now();
match msg {
Evt(event) => {
Expand Down Expand Up @@ -228,7 +228,7 @@ where
self.client.handle.spawn(fut);
}
Msg(LspMessage::Notification(not)) => {
let is_exit = not.method == EXIT_METHOD;
let is_exit = not.method == EXIT_METHOD.as_str();
let track_id = self
.next_not_id
.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
Expand Down Expand Up @@ -294,8 +294,8 @@ where
/// Registers and handles a request. This should only be called once per
/// incoming request.
pub fn on_lsp_request(&mut self, method: &str, params: JsonValue) -> ScheduleResult {
match (&mut self.state, method) {
(State::Uninitialized(args), request::Initialize::METHOD) => {
match (&mut self.state, LspRequestMethod::from(method.to_owned())) {
(State::Uninitialized(args), InitializeRequest::METHOD) => {
// todo: what will happen if the request cannot be deserialized?
let params = serde_json::from_value::<Args::I>(params);
match params {
Expand All @@ -311,15 +311,15 @@ where
(State::Uninitialized(..) | State::Initializing(..), _) => {
just_result(Err(not_initialized()))
}
(_, request::Initialize::METHOD) => {
(_, InitializeRequest::METHOD) => {
just_result(Err(invalid_request("server is already initialized")))
}
// todo: generalize this
(State::Ready(..), request::ExecuteCommand::METHOD) => self.on_execute_command(params),
(State::Ready(..), ExecuteCommandRequest::METHOD) => self.on_execute_command(params),
(State::Ready(s), method) => 'serve_req: {
let is_shutdown = method == request::Shutdown::METHOD;
let is_shutdown = method == ShutdownRequest::METHOD;

let Some(handler) = self.requests.get(method) else {
let Some(handler) = self.requests.get(&method) else {
log::warn!("unhandled lsp request: {method}");
break 'serve_req just_result(Err(method_not_found()));
};
Expand Down Expand Up @@ -351,29 +351,32 @@ where

// todo: generalize this
if command == "tinymist.getResources" {
self.get_resources(arguments)
self.get_resources(arguments.unwrap_or_default())
} else {
let Some(handler) = self.commands.get(command.as_str()) else {
log::error!("asked to execute unknown command: {command}");
return Err(method_not_found());
};
handler(s, arguments)
handler(s, arguments.unwrap_or_default())
}
}

/// Handles an incoming notification.
pub fn on_notification(&mut self, method: &str, params: JsonValue) -> LspResult<()> {
let handle = |s, method: &str, params: JsonValue| {
let Some(handler) = self.notifications.get(method) else {
let Some(handler) = self.notifications.get(&method.to_owned().into()) else {
log::warn!("unhandled notification: {method}");
Copy link
Copy Markdown
Owner

@Myriad-Dreamin Myriad-Dreamin May 10, 2026

Choose a reason for hiding this comment

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

Nit: every log of request/notification method introduces an extra allocation. while it doesn't matter much, but I feel it is kind of imperfect:

impl fmt::Display for LspNotificationMethod {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        let s: String = self.clone().into(); // a clone
        write!(f, "{s}")
    }
}

Copy link
Copy Markdown
Author

@ribru17 ribru17 May 13, 2026

Choose a reason for hiding this comment

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

Good point. I think the best thing to do is to move from a custom variant of Cow<'static, str> to just &str, where the reference lifetime is a parameterized 'a. That way, the enum becomes Copy and can be constructed from any &str without any allocations, while still allowing exhaustive enum-style pattern matching. What do you think?

return Ok(());
};

handler(s, params)
};

match (&mut self.state, method) {
(state, notification::Initialized::METHOD) => {
match (
&mut self.state,
LspNotificationMethod::from(method.to_owned()),
) {
(state, InitializedNotification::METHOD) => {
let mut s = State::ShuttingDown;
std::mem::swap(state, &mut s);
match s {
Expand All @@ -394,7 +397,7 @@ where
};
handle(s, method, params)
}
(State::Ready(state), method) => handle(state, method, params),
(State::Ready(state), method) => handle(state, method.as_str(), params),
// todo: whether it is safe to ignore notifications
(State::Uninitialized(..) | State::Initializing(..), method) => {
log::warn!("server is not ready yet, while received notification {method}");
Expand Down
Loading