Non-standard values in ServerCapabilities

[nemethf]

trafficstars

According to the LSP specification documentHighlightProvider in ServerCapabilities defined as:

documentHighlightProvider?: boolean;

However, the server sends "documentHighlightProvider":null. There are other values in the server capabilities that are null but should be boolean and therefore violate the specification.

Can you, please, change these to send false? Thank you.

[felixfbecker]

null and undefined are supposed to be equivalent in LSP, see this old thread: https://github.com/microsoft/vscode-languageserver-node/issues/86#issuecomment-245612694

[felixfbecker]

The specification was also written when TypeScript actually didn’t have a null type

[nemethf]

@joaotavora and I are a bit confused. @felixfbecker, can you look at the second half of https://github.com/microsoft/language-server-protocol/issues/830 as well? To me, it seems the two statements of @dbaeumer are contradictory. "we do treat undefined and null 'equal' in the protocol" vs. "{"completionProvider": null} will not register a completion provider and is even invalid since null is not listed as a valid type."

[dbaeumer]

This are two things: the spec and the implementation of the vscode node server running on an older TypeScript version.

• The specification clearly says that {"completionProvider": null} is not valid since null is not listed as valid type for completionProvider.
• The vscode server implementation. In the past TS had not support for strict null. So although null and undefined were not implicitly listed the were valid values. So the server tried its best to treat them according to the spec. Newer version of the server are using the TS strict mode so the compiler will now warn if null is assigned to values which don't support null.

The one issue is in language server protocol (https://github.com/microsoft/language-server-protocol/issues/830) and the other in the VS Code Node implementation (https://github.com/microsoft/vscode-languageserver-node/issues/86#issuecomment-245612694)

I hope this clarifies it.

[joaotavora]

I hope this clarifies it.

More or less: I've never used vscode things, I just code to the spec. Do I have to change my client because of vscode stuff? If the answer is "no", do you think I should change my client?

[felixfbecker]

@dbaeumer thanks, that does clarify it, although I still think that is contradictory to the old statement and still makes it a lot more cumbersome to implement the protocol in languages that have no concept of "undefined" properties.

we do treat undefined and null 'equal' in the protocol. I even think that I state somewhere that you should send null not undefined since undefined doesn't exist in other languages.

Is there any benefit to requiring this vs making optional properties also accept null?

[felixfbecker]

@dbaeumer I actually took a look at the proposed PR to ignore/remove all null properties, but looking at the spec, if what you say is true and LSP cares about the difference between null and undefined, we have a problem in PHP (and presumably almost every other language). In the spec there are multiple places where null is allowed but not undefined, and sometimes null is even given special meaning compared to undefined. (Search for "null" in https://microsoft.github.io/language-server-protocol/specifications/specification-3-14/)

How do you suggest to resolve these issues then? My proposal would be to make it clear in the spec that null is allowed and the same as undefined.

[dbaeumer]

Undefined is (besides at one place in a comment at that should be fixed) not used as a type since it is very JavaScript specific. In general even in TypeScript / JavaScript there is a difference between:

interface X {
  foo: number | undefined;
}

and

interface X {
  foo?: number;
}

The first one always has a property foo where as the second one doesn't. JSON doesn't support undefined as a value for a property. So the first can't be expressed in JSON.

If the spec wants to express the absence of a value although the property is not optional it is using null because undefined is very special to JavaScript. So the spec uses two concepts:

• optional properties
• null as a valid type.

So the only problematic things are if a property is optional and can carry null. I try hard to stay away from this and there are only two place in the spec that allow this. They are:

	/**
	 * The rootPath of the workspace. Is null
	 * if no folder is open.
	 *
	 * @deprecated in favour of rootUri.
	 */
	rootPath?: string | null;

	/**
	 * The workspace folders configured in the client when the server starts.
	 * This property is only available if the client supports workspace folders.
	 * It can be `null` if the client supports workspace folders but none are
	 * configured.
	 *
	 * Since 3.6.0
	 */
	workspaceFolders?: WorkspaceFolder[] | null;

which are both in the initialize request and I tried to document what their meaning is. At the end for the two null === absence of the property and the properties are optional since they got added in a later version.

I also want to stay away from explaining how language should implement the absence of a property. I think this can be handled differently in every language. Some might implement this using special setter / getter other might use null and yet other might have something comparable to undefined. So in PHP you could use null for optional properties (except of the above two case) and when serializing to JSON ignore the property.

Unless we change the spec (which technically would be breaking) on the wire a property marked as optional should not be represented as null.

[felixfbecker]

Sorry for being unclear - I used undefined interchangeably with "absent"/"optional".

So going back to the original issue, for a property like documentHighlightProvider?: boolean; a language server that defines this property MUST pass false for it if it doesn't support it, while null is not valid.

[dbaeumer]

Yes, from a spec point of view null is currently not valid. Adding it would be breaking and we need to ensure it is worth the swirl to do this.