search

felixfbecker / php-language-server

PHP Implementation of the VS Code Language Server Protocol 🆚↔🖥
ISC License
1.14k stars 185 forks source link

Non-standard values in ServerCapabilities #758

Open nemethf opened 4 years ago

nemethf commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

nemethf commented 4 years ago

@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 commented 4 years ago

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

capture

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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:

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 commented 4 years ago

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 commented 4 years ago

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.