ebkalderon / tower-lsp

Language Server Protocol implementation written in Rust
Apache License 2.0
1.01k stars 57 forks source link

Allow custom methods to override default method signature #393

Open sdankel opened 1 year ago

sdankel commented 1 year ago

There are some cases where we want to implement features that are not yet added to the LSP spec. For example, for hover requests, some editors like VSCode support extra actions in the hover docs.

Unfortunately, it's not possible to just write a custom_method with the new response type. When I try this for textDocument/hover, I get:

[Error - 3:18:21 PM] Request textDocument/hover failed.
  Message: Method not found
  Code: -32601 

Even with the same method signature, I get this error. It seems to me that any custom methods should override the default error implementation, regardless of their method signature (or at least, regardless of the contents of the response type). This would give users a lot more control and flexibility with this framework.

Here's exactly what I'm trying to do:

    let (service, socket) = LspService::build(Backend::new)
        .custom_method("textDocument/hover", Backend::hover)
        .finish();

// My custom response
pub struct MyHover {
    #[serde(flatten)]
    pub hover: lsp_types::Hover,
    #[serde(skip_serializing_if = "Vec::is_empty")]
    pub actions: Vec<CommandLinkGroup>,
    // ... any other fields I want
}

impl Backend {
    pub async fn hover_req(&self, params: HoverParams) -> jsonrpc::Result<Option<MyHover>> {
        /// implementation...
    }
}

@ebkalderon What do you think?

sdankel commented 1 year ago

I found a workaround for this particular example, but I still think this would be useful to have. Happy to submit a PR if you agree.

ebkalderon commented 1 year ago

I think this might be useful in certain cases! Although this would technically make the server not compliant with the LSP specification, and so not a recommended practice, I don't see a reason to explicitly disallow this. I think the core design of the library would likely need to shift from the current LanguageServer trait based design, though. Thanks for the suggestion! Also, I'm glad you managed to find a workaround for your specific use case in the meantime.