Add null to return types

[MariaSolOs]

After microsoft/vscode-languageserver-node#1691 these methods are typed as potentially returning null. Updating the documentation to reflect this.

[mfussenegger]

This is a breaking change for clients. Is this really necessary?

[puremourning]

Hang on isn't this a breaking change?

[MariaSolOs]

This is a breaking change for clients. Is this really necessary?

I'm just trying to update the documentation to reflect the current state of the protocol.

[jwortmann]

I always thought that the documentation is the protocol.

In other words, the source of truth would be what is written on the website labeled as "Specification" under https://microsoft.github.io/language-server-protocol/specifications/specification-current, and not some implementation detail in a vscode-languageserver-node library.

That said, in the client implementation for Sublime Text we already have a workaround for exactly this use case (more precisely one part of the proposed changes), because apparently there is already a language server that violates the current specs for this return type.

Of course I don't know how other clients handle it, or whether they would crash if null is returned and they try to iterate the results directly.

[MariaSolOs]

I always thought that the documentation is the protocol.

In other words, the source of truth would be what is written on the website labeled as "Specification" under https://microsoft.github.io/language-server-protocol/specifications/specification-current, and not some implementation detail in a vscode-languageserver-node library.

I 200% agree with you here. However from my contribution experience in this repository I know that changes will often be made first to the VS Code LSP implementation and then the metamodel will be generated there and ported to this repo (see how these generator scripts are defined there). I don't like this order of operations, but I want to make sure that when the metamodel is updated in the future the documentation reflects what the types do so that other editors can align.

[puremourning]

This is a breaking change for clients. Is this really necessary?

I'm just trying to update the documentation to reflect the current state of the protocol.

The documentation, is the protocol.

[MariaSolOs]

This is a breaking change for clients. Is this really necessary?

I'm just trying to update the documentation to reflect the current state of the protocol.

The documentation, is the protocol.

Please read #2235 (comment). I'm on your side here.

[dbaeumer]

The specification is the truth.

The reason why I actually would like to change the specification here is consistency. The fact that the two requests don't allow to return null seems to be an oversight. All other requests that return an [] of things do allow to return null.

So I somehow assume that most clients will handle null in these cases as well.

[puremourning]

I for one explicitly check the spec every time to see if a value can be null. Because that requires extra code vs say an empty list.

As it happens I don't think this particular item is supported by my client so it's not such a big deal for me, but in general it's true.

[mfussenegger]

So I somehow assume that most clients will handle null in these cases as well.

They don't.

I'd also think that if there is a someType[] | null the specification should clarify if there is a semantic difference between null and empty list. And if there isn't - just don't do the | null. Representing information with types that can represent more states than necessary isn't useful.

[dbaeumer]

As said it is more about consistency. The protocol started to allow null in case of an empty array and I would prefer to keep it that way instead of saying it differs from case to case.

[MariaSolOs]

As said it is more about consistency. The protocol started to allow null in case of an empty array and I would prefer to keep it that way instead of saying it differs from case to case.

Is this documented in the specification?

These types are defined using TypeScript syntax though, and in TypeScript an empty array is different from null. There's actually an inconsistency between the specification and the expected implementation.