Codify the methods and their arguments and result types

[Marwes]

This crate could provide a minimal encoding of the methods and notifications in the protocol.

pub trait Request {
    type Param;
    type Result;
    // Has to be a method as associated constants are unstable
    fn method() -> &'static str;
}

pub trait Notification {
    type Param;
    fn method() -> &'static str;
}

https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#showmessage-request

pub struct ShowMessageRequest;
impl Request for ShowMessageRequest {
    type Param = ShowMessageRequestParams;
    type Result = MessageActionItem;
    fn method() -> &'static str { "window/showMessageRequest" }
}

https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#showmessage-notification

pub struct ShowMessageNotification;
impl Notification for ShowMessageNotification {
    type Param = ShowMessageParams;
    fn method() -> &'static str { "window/showMessage" }
}

[Marwes]

I probably won't implement this myself right now as I already have the methods I need working.

cc @nrc Would this be useful for rls?

[Xanewok]

That'd be great! However it'd be very convenient to have same kind of structs for requests/notifications as there are for params wrt (de)serialization. It'd make the communication a lot more straightforward and easier to use. While I think exposing this as traits is a great idea, how would that mesh together with the struct (de)serialization?

@Marwes I think it would, as there was an early plan to clean up a bit RLS custom Request/Notification bits and build it mimicking approach taken here for the message params.

[Xanewok]

I'll try and have a go at it in spare time and see how it goes.

[Marwes]

@Xanewok Definitely add a pair of macros to avoid some of the repeition as well decl_request decl_notification or something.

While I think exposing this as traits is a great idea, how would that mesh together with the struct (de)serialization?

What do you mean?

[Marwes]

That'd be great! However it'd be very convenient to have same kind of structs for requests/notifications as there are for params wrt (de)serialization.

The structs set as Param and Result are still the same types as they always are. This just gives a way to abstract over requests and notifications.

I was thinking of adding something like this for my own implementation of a language server but this issue is intentionally made to be unaware of how it is actually used.

struct RequestImpl<Func, Tag : Request> {
     handler: Func,
    _marker: PhantomData<Tag>
}

impl RequestImpl<Func, Tag>
where
    Func: FnMut(Tag::Param) -> Result<Tag::Result, Error>
    Tag: Request
{
     fn handle(...) { ... }
}

[nrc]

There is also https://github.com/RustDT/RustLSP, which I think does something similar to this.

[nrc]

I think the general idea here is good. It would be great to be more disciplined about how the RLS deals with messages and such in the protocol. If we could type check more stuff, that would be even better. I think it is kind of complex to get a good design here. It would be worth experimenting with a language server (like the RLS) to make sure any design here is ergonomic to use and efficient (i.e., doesn't have tonnes of virtual dispatch). Currently the RLS uses a pretty complex system of macros, and I don't think it is very readable, although it is concise.

[Marwes]

However it'd be very convenient to have same kind of structs for requests/notifications as there are for params wrt (de)serialization.

Do you mean that each impl of say Request should be implemented on the parameters of the method? That is fine where it is possible, but there are some methods which do not take a unique parameter https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#completion-request https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#goto-definition-request.

There is also https://github.com/RustDT/RustLSP, which I think does something similar to this.

RustLSP has a big trait specifying each method https://github.com/RustDT/RustLSP/blob/e39ed06b14d711edfa912ce5e24e73ca0921f848/src/lsp.rs#L117-L152. Personally I feel like this trait does a bit to much. It assumes all methods can be implemented and only offers one way of implementing the RPC.

The proposed way to do it here should (as much as possible) be agnostic to how RPC are implemented.

I think it is kind of complex to get a good design here.

Yep, which is why I think we should only link method name to its parameters and return type. How method calls actually are implemented should be implemented outside this crate.

[bruno-medeiros]

There is also https://github.com/RustDT/RustLSP, which I think does something similar to this.

Yes, unfortunately I wasn't able to update it to version 3.0 of LSP (started a full-time job in the meanwhile, I don't think I will be able to work on it anymore)

[bruno-medeiros]

RustLSP has a big trait specifying each method https://github.com/RustDT/RustLSP/blob/e39ed06b14d711edfa912ce5e24e73ca0921f848/src/lsp.rs#L117-L152. Personally I feel like this trait does a bit to much. It assumes all methods can be implemented and only offers one way of implementing the RPC.

If an LS can't effectively process the request method, it can just implement the trait function by completing the response with an error saying the request is not supported, so I don't see this as an issue. (see for example: https://github.com/RustDT/MockLS/blob/master/src/mock_ls.rs)

Also, what do you mean "only offers one way of implementing the RPC"? You can have different trait implementations... I guess you can't control how the Params object gets deserialized though. That might be a minor performance limitation?

[Marwes]

If an LS can't effectively process the request method, it can just implement the trait function by completing the response with an error saying the request is not supported, so I don't see this as an issue. (see for example: https://github.com/RustDT/MockLS/blob/master/src/mock_ls.rs)

Sure, but by not using a big trait we don't even have to add those dummy methods. I just want this as bare bones as possible to avoid encoding any sort of assumption about how requests are handled.

Also, what do you mean "only offers one way of implementing the RPC"? You can have different trait implementations... I guess you can't control how the Params object gets deserialized though. That might be a minor performance limitation?

There isn't an easy way to handle requests with https://github.com/alexcrichton/futures-rs for instance. I don't doubt it is possible, but bending that abstraction to accommodate futures is less ideal than just having a direction abstraction on top futures (and if one does not want to use futures then another abstraction should work equally well).

[Xanewok]

I think it's more important to provide efficient and ergonomic data structures and related type logic here wrt protocol, while leaving more freedom for the consumer how exactly message/error handling will be implemented using the types in this crate.

I tried to extend @Marwes's solution a bit: https://gist.github.com/Xanewok/5b062485a98c1312481e1790772ae477
Thoughts/feedback on this? This is only a general direction in which it could go.
With this I'd say it's quite ergonomic and separates handling logic. If we're going with static dispatch approach, we could later introduce a union type for message types (like RLS does), which would probably allow for straightforward deserialization.

[Marwes]

@Xanewok

fn id(&self) -> &ls_types::NumberOrString;
fn params(&self) -> &Self::Param;

These methods conceptually exists for all requests and are also the same for any json rpc protocol so they aren't unique to the language server. For instance, I use the core crate from https://github.com/paritytech/jsonrpc to receive forward requests to the correct callback which means I don't need to bother with the object containing the id field at all.

Tweaking your example a bit https://gist.github.com/Marwes/93a5d9530226e5404493000d653db979 to remove a bit of boilerplate.

[Marwes]

With this I'd say it's quite ergonomic and separates handling logic. If we're going with static dispatch approach, we could later introduce a union type for message types (like RLS does), which would probably allow for straightforward deserialization.

Is there any benefit to introducing an enum as an intermediate stage? Instead of dispatching to the handler immediately?

[Xanewok]

Oh, I see it now, I'm sorry. Wasn't aware of the core crate you mentioned and I thought the end goal would be to implement all the JSON-RPC types LSP uses here...

Looking at what this crate provides, the only thing that would be to add here is to specify which exacatly methods/notifications are supported via the JSON-RPC and LSP (ShowMessage, Completion etc...) and for those, binding params type to the result type? (So traits like https://paritytech.github.io/jsonrpc/jsonrpc_core/trait.RpcMethodSimple.html, but parametrized with a similar Trait you showed previously with associated Params and Result types?)

[nrc]

Has to be a method as associated constants are unstable

I would just use an assoc const - stable is overrated

but there are some methods which do not take a unique parameter

Could we just use unions or enums for those?

[Marwes]

Oh, I see it now, I'm sorry. Wasn't aware of the core crate you mentioned and I thought the end goal would be to implement all the JSON-RPC types LSP uses here...

:) I could have been clearer. But yeah, I think the JSON-RPC part can be implemented separately as there both other protocols using JSON-RPC and multiple approaches of handling requests.

(So traits like https://paritytech.github.io/jsonrpc/jsonrpc_core/trait.RpcMethodSimple.html, but parametrized with a similar Trait you showed previously with associated Params and Result types?)

I guess that is what I really meant, though as you can see, even that crate has two traits defining an RPC method https://paritytech.github.io/jsonrpc/jsonrpc_core/trait.RpcMethodSync.html (Future vs Result) and I thought it best to leave that choice out of this library.

I do have a trait like that in gluon's language server https://github.com/gluon-lang/gluon_language-server/blob/23a3eb23e5434bc78567804bfc6ea54d7b761aac/src/rpc.rs#L29-L37. But each method I define using it has the method name, parameter and return type defined in an ad-hoc way which I'd rather avoid. I know that my initial proposal will work for what I had in mind at least but perhaps there is a more general way of doing it.

I am not against defining something larger than my initial proposal, I just figured it would be difficult to agree on a design which works everwhere.

I would just use an assoc const - stable is overrated

Maybe both, and hide the associated constant behind a feature? If a macro is used to define these things there shouldn't be much extra work.

Footnote

I want to turn this https://github.com/gluon-lang/gluon_language-server/blob/23a3eb23e5434bc78567804bfc6ea54d7b761aac/src/rpc.rs#L75-L118 into something like.

impl<P, T> RpcMethodSimple for ServerCommand<T>
where
    T: Request,
    P: FnMut(T::Param) -> BoxFuture<T::Result, Error>
{
    fn call(&self, param: Params) -> BoxFuture<Value, Error> {
        let value = match param {
            Params::Map(map) => Value::Object(map),
            Params::Array(arr) => Value::Array(arr),
            Params::None => Value::Null,
        };
        let err = match from_value(value.clone()) {
            Ok(value) => {
                return (self.handler)(value)
            }
            Err(err) => err,
        };
        let data = self.0.invalid_params();
        futures::failed(Error {
            code: ErrorCode::InvalidParams,
            message: format!("Invalid params: {}", err),
            data: data.as_ref()
                .map(|v| to_value(v).expect("error data could not be serialized")),
        }).boxed()
    }
}

fn register_command<T>(command: T) where T: Request + RpcMethodSimple {
    // extract the method name with T::method() to register a handler
}

fn main() {
     let command = ServerCommand::<CompletionRequest, >::new(
        // param is now inferred to be `<CompletionRequest as Request>::Param`
        // and the result type to be `<CompletionRequest as Request>::Result`
        |param| ...
    );
     
     register_command(command);
}