lapce / lapce/lapce-plugin-rust

Make handling requests/notifications nicer

Open
#21 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
31
Forks
12
PR merge metrics
No merged PRs in 30d

Description

Currently LapcePlugin is defined as:

#[allow(unused_variables)]
pub trait LapcePlugin {
    fn handle_request(&mut self, id: u64, method: String, params: Value) {}
    fn handle_notification(&mut self, method: String, params: Value) {}
}

This is a bit eh in that it 'forces' the caller to do a big match statement over the method and then do the deserialization themselves. Ideally we would lessen that friction by default.

My initial thoughts are to have something vaguely like:
(I spell out types to make it more clear, but a lot of the callback types could be inferred)

// T is state.
// Q: A shorter/better name for `ResponseSender`?
type RequestHandler<T, V> = Box<dyn Fn(&mut T, V, ResponseSender)>;
type NotifHandler<T, V> = Box<dyn Fn(&mut T, V)>;
struct MsgHandler<T> {
    request_handlers: HashMap<String, RequestHandler<T, serde_json::Value>>,
    notif_handlers: HashMap<String, NotifHandler<T, serde_json::Value>>
}


impl<T> MsgHandler<T> {
    pub fn new() -> MsgHandler { /* ... */ }

    // Deserialize bound is a bit trickier in reality, but basically this
    // Q: The error `_` should probably be serde_json's deserialize error type but perhaps it should be more general?
    pub fn on_request<V: Deserialize, R: Serialize>(mut self, method: impl Into<String>, cb: impl Fn(&mut T, Result<V, _>, ResponseSender) + 'static) -> Self {
        // The outline of how request handlers work is that you're supposed to call the `send` function on `ResponseSender`` with your data.
        //   (We can't have it simply be returned from the function, because sometimes you only want to run LSP commands *after* you respond, like in initialize)
        let cb = Box::new(move |state: &mut T, data: serde_json::Value, resp: ResponseSender| {
            let data: Result<V, _> = serde_json::from_value(data);
            cb(state, data, resp);
        });
        self.request_handlers.insert(method.into(), cb);
    }

    pub fn on_notif<V: Deserialize>(mut self, method: impl Into<String>, cb: impl Fn(&mut T, Result<V, _>) + 'static) -> Self {
        let cb = Box::new(move |state: &mut T, data: serde_json::Value| {
            let data: Result<V, _> = serde_json::from_value(data);
            cb(state, data);
        });
        self.notif_handlers.insert(method.into(), cb);
    }

    /// Listen for the [`Initialize`] request.
    /// ... common docs here so that plugin authors don't have to read the lsp spec ...
    pub fn on_initialize(mut self, cb: impl Fn(&mut T, Result<InitializeParams, _>, ResponseSender) + 'static) -> Self {
        self.on_request(Initialize::Method, cb)
    }

    // ...
    // Various common functions.
    // We might not want to implement functions for literally every random LSP feature. Just common ones.
}

pub struct ResponseSender {
    // ... whatever is needed to send a response to the request ...
    // We could do FUN generic PhantomData to make so the closure can only send values of the allowed response type. I think that would be nice without adding complexity.

    // Q: we can log a warning if you didn't send a response/err back? Not sure if that's desirable in general.
    // sent_response: bool,
}
// ... obvious response sender impl ...

This would allow some common way of implementing a LSP startup to be like:

MsgHandler::new()
    .on_initialize(|state: &mut State, res: Result<InitializeParams, RpcError>, resp: ResponseSender| {
        resp.send(InitializeResult {
            // ...
        });

        PLUGIN_RPC.start_lsp(
            // ...
        );
    });

There's now two questions:

  • How do we say that we're using a specific message handler? There's no main function that the plugin can nicely 'start' in.
  • Do we want to allow dynamically registering handlers?
    • I lean towards yes, though do we alert the client editor about there being nothing handling the method?
      • RA logs warnings if it receives methods it doesn't handle. Maybe we should just automatically send out a command to log that, to encourage the plugin author to handle it nicely?

I'd say that what we can do is make the register_plugin have an alternate sort if you specify a 'handler creation function',

register_plugin!(State; handler: handler);

fn handler() -> MsgHandler<State> {
    MsgHandler::new()
        .on_initialize(|state: &mut State, res: Result<InitializeParams, RpcError>, resp: ResponseSender| {
            resp.send(InitializeResult {
                // ...
            });

            PLUGIN_RPC.start_lsp(
                // ...
            );
        })
}

We also introduce a new trait that acts as trait which LapcePlugin requires.

// ... inside lapce-plugin-rust
pub trait Handler {
    fn handle_request(&mut self, id: u64, method: String, params: Value);

    fn handle_notification(&mut self, method: String, params: Value);
}

pub trait LapcePlugin: Handler {}

The register_plugin trait would simply implement handler automatically.

// Example possible output of register plugin when specifying a handler function.
thread_local! {
    static STATE: std::cell::RefCell<State> = std::cell::RefCell::new(Default::default());
    static HANDLER: OnceCell<RefCell<MsgHandler<State>>> = OnceCell::new();
}

fn main() {}

pub fn handle_rpc() {
    // ... typical handle_rpc ...
}

impl Handler for State {
    fn handle_request(&mut self, id: u64, method: String, params: Value) {
        STATE.with(move |state| {
            HANDLER.get_or_init(|| RefCell::new(handler())).borrow_mut().handle_request(state, id, method, params)
        })
    }

    fn handle_notification(&mut self, method: String, params: Value) {
        STATE.with(move |state| {
            HANDLER.get_or_init(|| RefCell::new(handler())).borrow_mut().handle_notification(state, method, params)
        })
    }
}

This would allow the existing plugin implementations to just automatically work without any changes, and would allow swapping it out for a completely custom handler if desired.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading the existing LapcePlugin and register_plugin interfaces, then compare them with the proposed MsgHandler, ResponseSender, and Handler designs in this issue. The work is done when the request and notification handling API, handler registration approach, and compatibility with existing plugin implementations have been agreed and implemented.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
api, developer-experience, tooling
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.