Language-server-protocol: Discourange server-initiated changes to the client's state

[matklad]

Philosophical issue split off #612.

Currently, we have one server-initiated request that changes client state: workspace/applyEdit. #612 suggests adding a second one, workspace/showTextDocument.

I'd like to argue that we should not add workspace/showTextDocument, and that we should deprecate/discourage use of workspace/applyEdit :-) There are three reasons to do this: UX predictability, implementation reliability, existence of better solutions for the problems solved by server-initiated changes.

From UX perspective, I think it is important to maintain that any change to user-visible state originates directly from user's actions. If something happens, it should happen because user pushed a button, not because server decided that it's the right time.

From implementation perspective, it is important to maintain causality links between user actions and state changes. Consider, for example, an "transmogrify" code action, which is implemented in two ways. In the first implementation, "transmogrify" directly contains WorkspaceEdit. In the second, lazy, implementation, it contains command, which is executed on the server and causes it to send workspace/applyEdit. In the first case, the client can show "undo transmogrify" in the undo UI, because it knows that the edit is a result of applying code action. In the second case, there's no lamportian link between "user applied code action" and "server send workspace edit", so we can't show nice UI without additional encoding. The second implementation issue is that it's hard to make server initiated edits consistent with the client state: when a server initiates the edit, it does not know the client's current state. When the client accepts the edit, it doesn't know the state the server was in when it sent the edit (see #584. I believe workspace/applyEdit is the single case where we can't guarantee 100% agreement on the global state, due to the fact that document changes are notifications and not requests).

Finally, I think workspace/applyEdit was added originally to support "lazy" code actions, such that the server can calculate the required edit only when the action is applied, and not merely a list of available actions is requested. I think this is an overengineered solution to the problem. Currently, the server first provides a name of command (which should be executed on the server itself), then it defines workspace/executeCommand to handle that command, and there it should make use of server-initiated requests to make stuff actually happen. But there's only single server-initiated request that does stuff: workspace/applyEdit. So this setup is more-or-less isomorphic to a simpler setup with an additional resolveCodeAction request from client to server, which returns a WorkspaceEdit directly.

It may be argued that "we can add more powerful server-initiated requests, like #612", but we can just extend capabilities of WorkspaceEdit, like how we've added file system operations to it recently. For example, adding "openDocuments: Uri[]" field to WorkspaceEdit I think should help with #612.

Action items here:

• decide what is the LSP guideline here: do we want server-initiated state changes?
• if we don't want them, than resolveCodeAction request should be added, applyWorkspaceEdit should be discouraged, and we should work with other issues requesting more server-initiated commands to figure out how we can make them client-initiated.

[rasika]

Following reply depends on my observations regarding the LSP protocol and VSCode implementation. Please correct me If I am wrong in anywhere.

I am not sure the scope of server-initiated client state changes; If we label proposed "workspace/showTextDocument" in #612 as such I assume "textDocument/publishDiagnostics" does the same.

Suppose you are changing a text in a text document. Now "Editor Client" will send the text changes into the LSServer as a notification. With the document change now LSServer need to re-compile the source document inorder to obtain code diagnostics. Meanwhile the "Editor Client" (adhering synchronous communication only) can't wait indefinetly while the LSServer compile and reply back with the code diagnostics. Thus, publishing back code-diagnostics happens asynchrnously as a JSON notification.

[Editor Client] -----------[didChange]---------------> [LSServer]
...
[Editor Client] <--------[publishDiagnostics]--------- [LSServer]

Now if we analyze above example; From the LSP protocol implementation POV the protocol has zero or minimum relationship between these two notifications. There's nothing that prevents some Editor client sending multiple "didChange" notifications more frequently(aka. flooding). However, LSServer implementation can decide on compiling and sending code diagnostics only for the last "didChange" notification(aka. server-side debouncing).

If we consider regarding causality; now nobody really knows, for which [didChange] notification with, which [publishDiagnostics] notification LSServer has replied. EoD what really matters for me is both [Editor Client] and [LSServer] shares the same state regarding the resources.

Further I would argue that server-initiated state changes are un-avoidable. I assume by definition "server-initiated state changes" are the modifications for the resources sent from server-to-client. For exmaple; there can be code actions that adds code templates into the text-document. Moreover, recently I needed to generate test templates for a given source code through the Code Actions. For all these, server-initiated state changes / notifications were needed to notify client to update the resource / text-document.

Now, coming back to my problem; supporting "workspace/showTextDocument" is yet another notification that LSServer requests "Editor Client" to open a specific Location. Originally proposed as a notification since the server does not need to wait for the client's respond. Again, this is related to the test-code generation. If a particular code action execution is required to modify / create a test file; there should be a way for the server to tell the client "Hey, I created a test file; go and open my file". Hope this clears my requirement for the "workspace/showTextDocument" as well.

[matklad]

Now if we analyze above example; From the LSP protocol implementation POV the protocol has zero or minimum relationship between these two notifications.

An excellent example, thanks! I've missed it when discussing #584. Indeed, because [didChange] is a notification and not a request, the client does not know, upon receiving [publishDiagnostics], which notifications are acknowledged by the server, and so the client can't, for example, adjust diagnostics spans based on typing.

[matklad]

accidentally send the comment, continuing.

I think this even leads to a user-visible issue: today in VS Code, if I define a problem matcher for a task and launch task, I got error highlighting. However, if I add/remove lines from the editor, without re-running the task, highlighted ranges are not modified, meaning that wrong tokens are highlighted.

To be able to correctly adjust diagnostic ranges for language-server generated diagnostics, VS Code needs to know which notifications were acknowledged before publishDiagnostics notification.

[matklad]

Further I would argue that server-initiated state changes are un-avoidable. I assume by definition "server-initiated state changes" are the modifications for the resources sent from server-to-client.

I'd say that if CodeAction contains a WorkspaceEdit, then this edit is no "server-initiated". It's true that the edit is computed on the server, but it happens when client explicitly requests code actions (so, client knows which notifications were sent to the server when it was computing the edit), and it's the client who actually applies the edit. That is, client knows the state of the world both when edit is computed and when it is applied.

In contrast, if code action contains a command, and server applies edit by sending workspace/applyEdit as a reaction to the command, client has no idea about what was the cause of the edit, and so it doesn't know the state of the world when the edit was generated, and can't compare it with the current state of the world.

That is, "server-initiated change" refers only to workspace/applyEdit request. Edits which are sent as responses to completions/code actions are safe.

[dbaeumer]

The model the LSP uses to ensure both client and servers can check if they talk about the same state is as follows:

• files that are open are owned by the client (e.g. the client holds the truth) / files that are closed are owned by the storage (e.g. the file system holds the truth)
• the client guarantees that he send everything in order. It doesn't for example asking for code complete before sending all changes. The changes contain a version number of the document as given by the client.
• if a server initiates a change on the client (being it by a code action result or by applyEdit) it needs to provide the version number of the document it wants to change.

We do know that we need to improve this in two ways:

• support a version number in diagnostics so that editors which have support for keeping old states can readjust the position (https://github.com/Microsoft/language-server-protocol/issues/201)
• currently null can be used as a version number if the document is closed. We should allow using hashes as well to handle the case that the document got opened while the request was sent but still has the same content.

That for me means that 'showTextDocument` should have a version number as well and that the location can only be treated as valid if the version numbers match (unless the client has means to adjsut the location).

In general I don't see the UX argument. I do agree that if a server does this it should be related to a user action (which IMO will be the case). The pure reason that this is async shouldn't have an influence on this. I am pretty sure that the API of most editors allow me to open a document and it would be easy to implement something that every 10 seconds opens a random document. And still editors provide these APIs.

All this being said I would not add notification support if I could start fresh. So I am in for discourage them but I think it is not worth to break a lot of servers and clients to simply remove them. It is under our control to not add more notifications.

[matklad]

it is not worth to break a lot of servers and clients to simply remove them.

That is unquestionable, everything should be opt-in.

support a version number in diagnostics

Similarly to discussion in #584, it will provide weaker than possible guarantee. If client knew which notifications had been acknowledged by the server when the server computed the diagnostic, it would be possible to say if the current set of diagnostics corresponds to the latest global state. It would also be a "protocol-level consistency" which would work out of the box for every request (as opposed to opt-in to version numbers in every notification).

I am pretty sure that the API of most editors allow me to open a document and it would be easy to implement something that every 10 seconds opens a random document. And still editors provide these APIs.

Agree that having powerful open "you can do anything" APIs is useful, but it's also helpful to handle common case via more closed API. Specifically, for a task "create and open the file" I would expect a single protocol request/type which can be used everywhere where it is possible to create files, as opposed to writing custom code to send an additional requst on the server everywhere I am creating files. Similarly, for code actions I would prefer to define a single "resolve code action edit" requset, rather than define a custom server command which sends requests to the client.

[dbaeumer]

I am not against having a create file change that also supports open the file or having a workspace edit that support this. This makes total sense to me.

I am still the opinion whether a client tracks a state or a server provides a state results in the same complexity and expressiveness. I think the difference in thinking is that you have a global state on the client enforced by the acknowledgement of changes whereas the current LSP model has a single state per document. If you want a single state per model on the client then it will become more complicated as well since you need to track per document which requests got acknowledged. I do agree that a a global state with the current model is complicated as well since we would need to send all version numbers for all open documents the server knows of even if some of them are nor modified (this could today even be done my sending a text edit for those documents but without any change)

When it comes to diagnostics I even think that the optimistic model is good enough and that the assumption that the document state and the provided diagnostics converge at some point is fair (it even has user benefits to apply them and not to drop them).

In Eclipse (which BTW I worked on for many years) we started with a global state/lock approach and then turned it into a fine grained resource based state / lock model since we noticed that too many results that would have still made sense to be applied got dropped or deferred by the global state model.

[matklad]

That makes a lot of sense, thanks!

My takeaways are

• we shouldn't worry too much about strict global consistency, but, if there'll be significant changes to notifications about changes (like making the semantics of edits to match the rest of the protocol), we might consider changing them to requests

• adding a separate "showTextDocument" request is good, but, if extending WorkspaceEdit works for the current use-case, it might be preferable (at least, it will be more convenient for my implementation of the server)