LSP: Validate responses against client capabilities

[kjeremy]

There are a number of places where responses from the LSP should be altered based on client and server support. Responses for things like textDocument/rename that return a WorkspaceEdit are a good example of this:

export interface WorkspaceEdit {
	/**
	 * Holds changes to existing resources.
	 */
	changes?: { [uri: string]: TextEdit[]; };
        /**
	 * Depending on the client capability `workspace.workspaceEdit.resourceOperations` document changes
	 * are either an array of `TextDocumentEdit`s to express changes to n different text documents
	 * where each text document edit addresses a specific version of a text document. Or it can contain
	 * above `TextDocumentEdit`s mixed with create, rename and delete file / folder operations.
	 *
	 * Whether a client supports versioned document edits is expressed via
	 * `workspace.workspaceEdit.documentChanges` client capability.
	 *
	 * If a client neither supports `documentChanges` nor `workspace.workspaceEdit.resourceOperations` then
	 * only plain `TextEdit`s using the `changes` property are supported.
	 */
	documentChanges?: (TextDocumentEdit[] | (TextDocumentEdit | CreateFile | RenameFile | DeleteFile)[]);
}

[matklad]

For the time being, I'd rather focus on supporting only the latest version of the LSP and fixing the clients :)

[kjeremy]

Sounds good. I just wanted to throw it out there.

[DJMcNab]

When we do care about this, I think we can implement this in our ConvWith/TryConvWith impls, if we teach the ServerWorld about client capabilities.

[ZoeyR]

I don't think its as simple as supporting the latest version of the LSP. According to the official LSP spec if you don't support the capabilities then you are de-facto not supporting the latest version of the LSP. The LSP specification is designed to be backwards compatible and supporting client capabilities is part of all 3.0 versions of the specification.

This is also problematic for text editors with a slower release cycle than VSCode. Since VSCode defines the specification they will always have the latest features implemented. For other editors (such as VS) they will always be behind in the spec.

Edit: its also worth noting that some features cannot be implemented by some clients. And by not supporting capabilities you are unable to run on those clients ever.

[matklad]

That's true indeed! To elaborate a bit, we are in the explicitly experimental phase, so protocol conformance is a non-goal (at the moment). Instead, we want to try as may things as fast as possible, so we are cutting corners, and we do use custom extensions to the protocol.

For example, our syntax highgliting works only in VS Code (and is a horrible hack), assists are supported only in VS Code and Emacs (b/c we extended the protocol to position the cursor), and the same goes for extend selection (which we've up streamed to the protocol)

[ZoeyR]

Fair enough :) I only noticed this because I was attempting to get this working in Visual Studio and it doesn't have all the capabilities of VSCode.

[matklad]

Curious, what exactly is missing? Perhaps its easy enough to add?

[ZoeyR]

Well so the VS implementation of LSP is closed source and tied to the VS release cycle, which is quite slow compared to VSCode. Here are the list of things missing in VS I've noticed so far:

• Code action literals, VS uses the old Command[] response type.
• Completion snippets, VS does not support completion snippets (and possibly never will be able to)
• Something is broken with workspace symbols, although this may just be a bug on us not expecting null in certain scenarios.

[matklad]

I meant easy to fix on our side :) Neither of those is trivial, but removing snippets should be easy enough (iirc vim doesn’t like snippets as well). Code actions are tricky, they are implemented with client-side commad. Workspace symbols should probably just work? We don’t do anything special there, and I think the protocol there hasn’t changed for a while?

For document symbols, we use new hierarchical format, which I think won’t work as well? Changing it to a flat one shouldn’t be too difficult, although less important than completions.

[kjeremy]

Originally my idea for solving this was be to hold onto the client capabilities sent during Initialize (or derive a configuration struct from that) and then use it as a Context in conv.rs to convert between analysis results and replies. I think that that could get us most of the way there.

[ZoeyR]

There are some code actions that get sent over when the client requests it. It shouldn't be too hard to fix this if we can get the client capabilities since the code action literals that are sent contain the command.

[kjeremy]

The InitializeParams come through here as params and you can get the ClientCapabilities from that.

From ClientCapabilities you can check for text_document (type TextDocumentClientCapabilities) which should cover the cases above I think.

From there you would need to maybe teach ServerWorld about those (maybe just hold onto the capabilities directly?) and then I think implement ConvWith for the return types from analysis with an Output type of the server reply and make the necessary changes in the handle_* functions.

[bstaletic]

For the record, ycmd doesn't handle snippets and actually asserts so completion in ycmd with rust-analyzer is completely broken.

[joshtriplett]

As an initial step, would it make sense to look at the client capabilities provided during initial connection, and error out if the capabilities don't include things that we assume the client supports?

We can then incrementally reduce that set of "required capabilities" as rust-analyzer starts to take them into account (for instance, not providing snippets if the client doesn't advertise support for them).

[kjeremy]

We could send a log or trace message back to the user so they know functionality may be broken (hopefully they would file an issue). Some of the client capabilities are used more as hints (semantic token types are an example of this) to the server and probably wouldn't require any additional checking.

…

On Wed, Apr 22, 2020, 8:56 PM Josh Triplett ***@***.***> wrote: As an initial step, would it make sense to look at the client capabilities provided during initial connection, and error out if the capabilities don't include things that we assume the client supports? We can then incrementally reduce that set of "required capabilities" as rust-analyzer starts to take them into account (for instance, not providing snippets if the client doesn't advertise support for them). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#144 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBACREPNVR6BNZVBKAJQQ3RN6G2RANCNFSM4F54CSWA> .

[matklad]

As an initial step, would it make sense to look at the client capabilities provided during initial connection, and error out if the capabilities don't include things that we assume the client supports?

I strongly believe that "erroring out" is wrong behavior for an IDE most of the times. It should function on best-effort basis and never die and thus the best language for IDE development is Erlang.

I don't think we need any incremental steps here -- we just (and here I think the word just is actually justtified) need to go through all methods in handlers.rs and make sure to simplify our responses according to client's capabilities.

The good start would be the completion snippets issue.

[mishazharov]

It appears that this also affect Emacs' Eglot LSP client: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=63433