Skip to content
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.

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

Make handling requests/notifications nicer #21

Open
MinusGix opened this issue Sep 9, 2023 · 0 comments
Open

Make handling requests/notifications nicer #21

MinusGix opened this issue Sep 9, 2023 · 0 comments

Comments

@MinusGix
Copy link
Member

MinusGix commented Sep 9, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant