Sharing the language-server-protocol types

[Marwes]

In conjunction to releasing a new version of https://github.com/gluon-lang/gluon_language-server I was planning to also release a crate containing only the types used to communicate in the language-server-protocol https://github.com/Marwes/gluon_language-server/tree/source_map/vscode-languageserver-types.

I believe that at least for just these types there is little reason for duplicating the implementation effort (especially since there seems to be some inconsistencies in the protocol microsoft/language-server-protocol#87). Is there any interest in using that crate (or a crate like it) here as well? I'd be happy to move it to a separate repository as well if that seems easier to work with.

cc @bruno-medeiros

Related #35

[brendanzab]

If anything, it would be super nice to have a common repository for these types - I'm sure there are other folks who might be interested in building their own languages in Rust who might be able to benefit.

[bruno-medeiros]

Yeah it would be good to integrate this work. I'd be willing to use such crate in RustLSP, but there is one obstacle: I want the source to work with Racer (and other tools), which means Serde source generation has to occur into src directory itself, not the target dir, which tools don't understand. .

See for example: https://github.com/RustDT/RustLSP/blob/master/build.rs , src/lsp.IN.rs is generated into src/lsp.rs

(I think this might complicate the dual approach of also using serde_derive + nightly, but it should still be possible)

[Marwes]

... Serde source generation has to occur into src directory itself ...

Looks like there is an (old) issue open for handling include! racer-rust/racer#191. Ideally I think this should be solved by racer itself but it is hard to say how much work it would be to implement.

[nrc]

Yeah, it would be good to pull out the LSP crates.

@bruno-medeiros why use Serde source generation at all? Much better to use Serde derive which the compiler understands.

[Marwes]

@nrc Until macros 1.1 are stable using source generation is still necessary to support stable rust. That's about the only reason though.

[nrc]

@Marwes macros 1.1 are in FCP and are likely to be stabilised soon. Certainly well before the RLS moves off the nightly compiler. Given the inherent instability of Serde or other code gen even on stable compiler, I'd rather be on nightly for a few months with a good solution, than be stuck with a crappy solution.

[Marwes]

I guess that is fair. I do appreciate being able to build on stable but its hardly a deal breaker not to. Then again the cost of allowing for both modes is low as well.

[bruno-medeiros]

If macros 1.1 are going to be stabilized soon, I'm fine with using the nightly compiler. I'd also be okay with the dual approach - again if at least one of the modes can be made to work with Racer (this means not using include! . This might make it more tricky to use the dual approach tho.

But out of curiosity:

Given the inherent instability of Serde or other code gen even on stable compiler, I'd rather be on nightly for a few months with a good solution, than be stuck with a crappy solution.

why is serde_codegen a crappy solution? What is the inherent instability of Serde?

[nrc]

Serde is fine, but having to do codegen outside the compiler is not - you lose the benefit of decent error message, and it breaks a lot of tools support (including the RLS, and even Racer, it sounds like).

[bruno-medeiros]

@Marwes I was looking into this, it seems the dual approach can't be done and yet have it work with Racer, since the dual approach must use include!. So, would it be ok to change it to use nightly only? We can always revert back if it turns out to be a problem.

[bruno-medeiros]

@Marwes I would also want to merge in some changes from https://github.com/RustDT/RustLSP/blob/master/src/lsp.IN.rs - your VScode type seem more complete, but there is a few things I'd like to add.

[Marwes]

Extracted the crate into https://github.com/gluon-lang/languageserver-types. If anyone wants merge access, give me a ping.

[Marwes]

I would also want to merge in some changes from https://github.com/RustDT/RustLSP/blob/master/src/lsp.IN.rs - your VScode type seem more complete, but there is a few things I'd like to add.

Nice, yeah I am sure there are some stuff that aren't quite correct since there are parts aren't used yet and there are, as mentioned, some inconsistencies with nullable fields.

[bruno-medeiros]

@Marwes I sent a PR your way: gluon-lang/lsp-types#1. (I'd also accept a merge access). Check the full commit message for details. In short it updated to latest LSP protocol version, but also fixed some issues. For example you can't use #[serde(skip_serializing_if="Vec::is_empty")] on a field that is optional in the spec. It has to be an Option in Rust too, or deserialization will fail with a missing property error.

With this PR, RustLSP will be able to use the languageserver-types crate instead.

[bruno-medeiros]

@nrc You interested in a PR to make rustls use https://github.com/gluon-lang/languageserver-types? I can have a go at that.

[nrc]

@bruno-medeiros yeah, definitely. It might take some iteration to make it fit in, but I'm keen to have a more principled use of the LS protocol.

[Marwes]

For example you can't use #[serde(skip_serializing_if="Vec::is_empty")]

Yeah I just realized that this a few days ago when starting to implement visual codes debugserver protocol. As an alternative to using Option we could mark those fields with #[serde(default)] which will let deserialization succeed. As long as there should be no difference between an empty array or an undefined field I think that's a bit nicer.

[bruno-medeiros]

Yes, it would be possible to use #[serde(default)] to give a default if it's not present. However, there could potentially be a semantic difference in the protocol between null/missing vector and an empty one for some types. It's very, very unlikely - but you never know... So as a matter of principle and consistency, I'd rather keep the translation the same for all properties that are like that. (that is, always use Option)

Also, we can always add a getter associated method to the LSP type that returns an empty vector if it's a None. That should mitigate the issue, no?

[Marwes]

Figured that if one actually treats empty array and a missing field differently then that should be considered a bug in any case. If for some weird reason the correct behavior is to treat them differently then we could always change it to an Option at that point (ie prefer the nice API and only add Option if it's necessary).

Don't want to bikeshed any further so if you still prefer the Option then just go with that.

[bruno-medeiros]

I agree that a missing property should semantically be treated the same as a property with null value (in fact that's why I opened this: microsoft/language-server-protocol#87). But that is different from what I was talking above. I was talking about the difference between a null (or missing) list field vs. an empty list.

Don't want to bikeshed any further so if you still prefer the Option then just go with that.

Okay, let's go with Option then, thanks.

[Marwes]

But that is different from what I was talking above. I was talking about the difference between a null (or missing) list field vs. an empty list.

Meant the difference between a missing field and the empty list but I mistyped.