Simplify protocol with regards to optional fields vs nullable fields.

[bruno-medeiros]

From my understanding, in the protocol, a field marked as "?" is optional. If it's not marked as optional, the key must be present, however, the value can still be null. This duality is confusing and makes it harder to undertand the protocol. Let's take InitializeParams as an example:

interface InitializeParams {
    /**
     * The process Id of the parent process that started
     * the server.
     */
    processId: number;

    /**
     * The rootPath of the workspace. Is null
     * if no folder is open.
     */
    rootPath: string;

    /**
     * User provided initialization options.
     */
    initializationOptions?: any;

    /**
     * The capabilities provided by the client (editor)
     */
    capabilities: ClientCapabilities;
}

We can clearly see that the the initializationOptions key is optional, but rootPath and processId are not. However, rootPath can be null, as the documentation specifies. But what about processId? Can it be null? It's not clear from the spec. I'd imagine it shouldn't be null, if it's not explicitly mentioned in the documentation. Yet I've tried out LSP4J and it sends processId as null... Who is right?

I propose that any field that can be null should be marked as optional (with "?"). Such that effectively, LSP states there is no semantic difference between a field that is missing (no key at all), and a field with key present, but with null value. This would eliminate ambiguities such as the example above.

This semantic distinction is even more important for languages such as Rust which don't have nullable types. (I'm writing an LSP SDK for Rust, and just realized this ambiguity will be a major pain)

[akosyakov]

@bruno-medeiros agree with you, the protocol should not make use of undefined values but stick to null.

I won't use LSP4J but VS Code to answer questions about the protocol since it is kind of a reference implementation.

Concerning processId, it can be null, consider cases when the language server is used locally or deployed as a web app. If you have a look at VS Code they also assume that. Another thing why do you actually need processId? Looking at the code you get an answer: the server should track whether the parent process is alive and exit, more to it if shutdownhas been called before it should exit with success otherwise with error.

Unfortunately currently the specification does not reflect effects of input parameters as well as legality and effects of previous calls on a current call's result. The best one can do is to test the server against VS Code and the client against let say the TS server, since the protocol was initially defined for them.

[bruno-medeiros]

Yeah, I just made processId optional in my implementation, not really fussed about it. It was just the first field I stumbled upon with this ambiguity. The problem is now I'm looking at all the the other fields in the protocol that are not marked as optional, and wondering if they can actually be null or not. 😑

I could test it out with VS Code, but as far as I know there is no simple way to test out a custom LSP language server, at least for someone who is not familiar with Typescript and VS Code extension development (and I'm not familiar at all with any of those). And even if I test with VS Code - the official LSP implementation - there is the risk of some other LSP implementation interpreting things in a different way.

[akosyakov]

@bruno-medeiros I think we just have to improve the spec for such properties.

Regarding to testing with VS Code. You can have a look at this simple extension. It is probably a bit outdated but it should be straightforward to upgrade it. It starts a java process with Xtext LS: https://github.com/akosyakov/ride-vsclient/blob/master/src/extension.ts#L18. You can change this line to start a process with your LS.

[dbaeumer]

I will keep it open since there is indeed a mismatch between properties being null and undefined ones. We should make this better. In general I tend now to not make any properties optional (undefined) since undefined is very JS specific and the protocol is for more than one implementation language

[bruno-medeiros]

Yeah, another example where it's not clear a property is optional or not is TextDocumentItem's languageId (or even version). There are sure to be others.

At the moment, I'm erroring on the cautional side, and try only to require a property if is strictly necessary for the underlying operation.

[dbaeumer]

Think about this again I am not sure if making all optional properties null is the right thing to do. We use JSON RPC and having a missing property in there is OK. The corresponding clients and server implemented in a different programming can then decide how they best map a missing attribute to their programing language.

There is on exception to this: error and result values send in the JSON-RPC responses can not be missing by JSON RPC definition. I made a pass a while ago to clearly specify where null is a valid result or error value. I used the TS or type to make this clear even for properties (e.g string | null).

I will close the issue since I think what we have in terms of the specification is clear enough right now (Please note that some properties instead of being optional are declared using | null. This was done for properties that we required in 1.0 but it did make sense to relax this).