workspace: Make didChangeConfiguration use its parameter

[rherilier]

Hi,

Here is a patch to make workspace/didChangeConfiguration capable of changing the configuration settings as describe in the LSP spec.

any comment/feed-back are welcome

[MaskRay]

Do you have a language client that needs this?

[rherilier]

eglot (the Emacs' builtin LSP client) does.

It is far better compared to restarting ccls when doing a git rebase or switching between builds.

And it's the last point I have to solve to switch my Emacs config from lsp-mode to eglot :)

Thx for all you work on ccls.

[rherilier]

It seems your are waiting for a more rationale reason. Here I go:

I looked at 3 editors to see how they handle the LSP client part.

First, vscode (obviously) relies on workspace/didChangeConfiguration (w/dCC for short). It emits one right after initialize to send user-defined parameters (that point is not really explicit in specification but as Microsoft is the originator of LSP, we can consider vscode as an implementation reference). ccls works because the extension vscode-ccls restarts ccls instead of emitting a w/DCC.

Second, neovim expects to configure the LSP server with a w/dCC just like vscode. No forced restart available (and no reference to ccls it its code-base).

Last, Emacs (which has 2 LSP clients). The one I want to use (eglot) expects to send LSP server configuration through w/dCC. So, same default behavior as the 2 other editors. No forced restart available even if ccls is detected.

The other client I want to move from (lsp-mode) is design to rely on w/dCC too for all the language server it supports. But to make ccls works with it, an other package (emacs-ccls), which depends on lsp-mode, has to be used and it relies on restarting ccls too (and the user has to explicitly do the restart him/herself). Quite bothersome.

Then I looked at some LSP servers code.

All those with support for w/dCC can handle their configuration through this request.

Clangd can handle configuration through w/dCC too but its configuration capability is quite limited (and it does not even handle correctly its own option set)...

So, we have 4 LSP clients expecting the servers (independently of the programming language) will read their configuration from w/dCC. ccls is really usable by 2 because of the existing extension/package which does a restart (and only one is usable without having to write extra code).

So, my PR aims to bring ccls hot-reconfiguration support to any editor which does not have the kill'n'start that vscode-ccls and emacs-ccls have.

Obviously, updating files or symbolic links at the project's root is not really a durable solution as it implies to add unnecessary complexity inside or outside of the editors.

Best regards

[MaskRay]

Thanks for the explanation.

ccls is really usable by 2 because of the existing extension/package which does a restart (and only one is usable without having to write extra code).

Can you point me to the restart code in the 2 clients? What operations did you do to trigger restart?

When does neovim send didChangeConfiguration?

[rherilier]

Can you point me to the restart code in the 2 clients?

In vscode-ccls, look at src/serverContext.ts and search for onDidChangeConfiguration to find the function doing the restart (commands.executeCommand('ccls.restart');) and its use.

For Emacs with emacs-ccls, the users have to call lsp-mode's function lsp-workspace-restart (defined in lsp-mode.el) to make effective their new settings.

With my PR merged, emacs-ccls should simply use lsp-mode's function lsp--set-configuration to trigger the settings update (many other lsp-mode's back-ends use it if you search for an example).

What operations did you do to trigger restart?

As stated just above, the function to call is lsp-workspace-restart, and for record, the elisp code I had used looks like:

(progn (setq ccls-initialization-options
             `(:compilationDatabaseDirectory ,current-build-dir))
       (lsp-restart-workspace))

When does neovim send didChangeConfiguration?

It happens in runtime/lua/vim/lsp/client.lua around the line 585 and is in a callback for the LSP request initialize. The code excerpt is:

if next(self.settings) then
  self:notify(ms.workspace_didChangeConfiguration, { settings = self.settings })
end

As I never programmed in lua and don't use neovim, I don't know how to consider the file protocol.lua. Asking the neovim's team would be faster if you want to go further ;)

Best regards

[rherilier]

I realized that my PR is missing an important point: if a workspace is reindexed, the diagnostic of any of its working files should be updated and sent to the client.

As I don't know how github will handle a new commit after the one you added in the PR, here is a commit (from a testing branch): rherilier@458e0e4

So, if you are fine with these changes, do you mind if I merge your changes on my side before doing a force-pushed with the aforementioned changes in a second commit? Or all in one commit if your prefer.

[MaskRay]

Feel free to force push to this PR and drop my commit after the change is merged into yours. I don't mind:)

I am still not clear why the client sends didChangeConfiguration. Is that because you have a hook that changes initialization options only after initialize? Why?

With the new didChangeConfiguration, ccls/reload is probably no longer useful.

It happens in runtime/lua/vim/lsp/client.lua around the line 585 and is in a callback for the LSP request initialize. The code excerpt is:

I have seen if next(self.settings) then inside rpc.request('initialize', initialize_params, function(init_err, result) and I don't understand the point. The initialize request contains initialization options. Why is didChangeConfiguration needed?
Perhaps related to the server request workspace/configuration? However, that does not seem to be needed inside initialize.

As I don't know how github will handle a new commit after the one you added in the PR, here is a commit (from a testing branch): rherilier@458e0e4

scheduleDiag pushes new items to diag_tasks, which could be redundant when initialization options do not really change.
However, I can see that it is necessary when command line options are changed and require re-parsing the working files. This is ok.

If we do change main_OnIndexed, place the db parameter before SourceManager to be similar to the parameter order in indexer.cc.

[rherilier]

Doh! I just notice there is another big issue: the index stops working unless the LSP client sent a textDocument/didOpen...

[rherilier]

Feel free to force push to this PR and drop my commit after the change is merged into yours. I don't mind:)

thx

I am still not clear why the client sends didChangeConfiguration. Is that because you have a hook that changes initialization options only after initialize? Why?

Your guess is right: I make my Emacs config send a didChangeConfiguration in various cases:

• when enabling my "development mode" which configures many things (build/test/packaging/whatever commands, project navigation and... ccls),
• when switching between builds (debug/relwithdebinfo/release) while debugging;
• when switching between git branches.

It can be needed too while rebasing or bissecting.

With the new didChangeConfiguration, ccls/reload is probably no longer useful.

ccls/reload clears the vfs and the db while the original didChangeConfiguration only forces the re-indexing. So I'm sure about it but you're well placed to tell it :)

But it gives me an hint to investigate the issue from my previous message. thx twice ;)

I have seen... However, that does not seem to be needed inside initialize.

The commit neovim/neovim@d5c489c seems to imply there are LSP servers which does/did not read the configuration from the initialize request... And remember my first long message: vsvode send initialize+didChangeConfiguration. I think all clients simply use it as a reference (even if it is redundant).

... which could be redundant when initialization options do not really change.

the lone way to know if it is redundant or not is to compare the outputs of cc -E .... $FILE with the old and new compilation options... And the diagnostic updates are fortunately only for the working files, not the whole project.

If we do change main_OnIndexed, place the db parameter before SourceManager to be similar to the parameter order in indexer.cc.

I'll fix it!

Best regards

[rherilier]

ccls/reload seems effectively becoming useless but what about replacing its uses in editors' plugins with a workspace/didChangeConfiguration where only whitelist and blacklist are set to keep the old behavior as it is?

Can you tell me why a cache is not automatically updated by bool indexer_Parse(...) when it detects that a cache is invalid?

As this function is quite complex, I'm not sure where to trigger the update of all invalid caches... Do you have an idea?

And the commit from my test branch is completely irrelevant regarding to the non-updated invalid cache entries.

Best regards

[rherilier]

EDIT: Doh! I forget cache entries are not accessible in sema_manager.cc but are in pipeline.cc... so, please, ignore what I wrote /o\

I forgot my proposal to trigger the invalid caches update:

1. add a method SemaManager::updateInvalidCaches(const std::string &root) to trigger updates of invalid cache entries which are related to the workspace folder root;
2. modify messages/workspace.cc as follows:

diff --git a/src/messages/workspace.cc b/src/messages/workspace.cc
index 2e58fe9e..61cbac01 100644
--- a/src/messages/workspace.cc
+++ b/src/messages/workspace.cc
@@ -48,6 +48,9 @@ void MessageHandler::workspace_didChangeConfiguration(JsonReader &reader) {
     project->load(folder);
   project->index(wfiles, RequestId());
 
+  for (auto &[folder, _] : g_config->workspaceFolders)
+    manager->updateInvalidCaches(folder);
+
   manager->clear();
 }