fix: compare config.cmd/cmd_env to check if new client should be spawned

[jedrzejboczar]

I am using on_new_config to determine the command used to spawn clangd depending on root_dir (inside docker or on host). With the recent addition of multi-workspace support, now the first spawned clangd will be serving all files, even if for some I would like a separate clangd inside docker container.

This PR attempts to solve the problem by comparing more configuration keys when checking if a client from cache can be used. I added comparison of cmd, cmd_env, cmd_cwd (if not nil) and settings (deep comparison).

I am not sure if this is the correct way of solving the issue, but it seems to work well with my setup.

I noticed one issue: LspInfo shows root_dir correctly only for the server that is attached to the current buffer. For example, when I have server A for buffer 1 and server B for buffer 2, then LspInfo shows Running in single file mode. for A when called from buffer 2, and for B if called from buffer 1, so it seems that root_dir is picked depending on current buffer. But I think this is only for LspInfo as lsp features seem to work well.

[glepnir]

it's not in lsp spec. why do you think it's not problem of server. the language server should implement the workspacefolder.

[jedrzejboczar]

The problem is that I need to run separate clangd servers in different docker containers. Let's say I have files under 3 projects: /projectA, /projectB and /projectC. A is running on host, but for B and C I want to use clangd in docker container (to have correct environment, dependencies, etc.). I would have the following:

root directory:  /projectA
cmd:             clangd 

root directory:  /projectB
cmd:             docker run --interactive --rm --user root --workdir /root/workdir --volume /projectB:/root/workdir my-clangd-image clangd --path-mappings=/projectB=/root/workdir

root directory:  /projectC
cmd:             docker run --interactive --rm --user root --workdir /root/workdir --volume /projectC:/root/workdir my-clangd-image clangd --path-mappings=/projectC=/root/workdir

There is no way for clangd from projectB to work correctly for projectC by switching workspaceFolder. They have different filesystems with different dependencies installed.

[glepnir]

clangd doesn't rely heavily on the workspace folder, but finds projects by looking for compilation databases relative to source files. It already supports multiple compilation databases, e.g. if you open files in multiple directories, each with compile_commands.json.

[jedrzejboczar]

I know, but I am compiling the projects for different architectures, e.g. projectA is for host system, while projectB e.g. for ARM embbedded microcontroller. In docker I have arm-none-eabi-gcc the toolchain installed and I'm bulding the C project inside docker. If not running clangd in docker container than it won't correctly pick up the paths (as compile_commands.json contains paths starting with /root/workdir from docker) and also won't see correct gcc toolchain.

[jedrzejboczar]

But regardless of my concrete setup with clangd, some people might want to do something equivalent - to start the language server with different command line arguments depending on project (e.g. some feature is only configurable from cmdline args). In such case, changing workspaceFolder is not enough - the server should be respawned with new cmd.

[glepnir]

the cmd can be a function type in vim.lsp.start

[jedrzejboczar]

This is currently checked in this part:

    cmd = eq.type_dispatch { -- string[] | (function(dispatchers: table): table) (see vim.lsp.start_client)
      ['function'] = function() return true end, -- for now don't check the function case, just assume equal
      table = eq.list(eq.builtin),
    },

So currently we're not handling function, we just assume they are equal (so this would just use workspace folders). I am not sure if anywhere in lspconfig cmd is actually a function, but it seems like it's not something that is used often (please correct me if I'm wrong)?

[justinmk]

thanks for the help. but this is far too much code. nvim-lspconfig should not be complicated, it's just a collection of configs.

is the comparison/merge code something that is needed in the Nvim Lua stdlib? for reference:

• https://github.com/neovim/neovim/blob/3b7548504344d72b0228e1bd29a7f28df813a3bd/runtime/lua/vim/shared.lua#L256
• https://github.com/neovim/neovim/blob/3b7548504344d72b0228e1bd29a7f28df813a3bd/runtime/lua/vim/shared.lua#L335
  • does vim.deep_equal() need an "override" capability so that the caller can pass a custom function?

is there some missing LSP feature that is needed in Nvim LSP core (not lspconfig)?

[jedrzejboczar]

Thanks for suggestion, didn't know about vim.deep_equal so was partially re-implementing it. I've refactored the code to use deep_equal - I needed to handle two special cases, but the code is much simpler now.

I don't think this is something related to LSP core as from what I understand core doesn't support multiple workspaceFolders on single client, while lspconfig does. But currently lspconfig is not checking that the new client configuration differs in such a way that it requires spawning new server process (because e.g. cmd is different) instead of reusing the same process and changing workspaceFolder.

[justinmk]

core as from what I understand core doesn't support multiple workspaceFolders on single client, while lspconfig does

well, is that an intentional design constraint of vim.lsp (cc @mfussenegger )? I don't see any (obvious) discussions: https://github.com/neovim/neovim/search?p=2&q=multiple+workspaceFolders&type=issues

in general, adding features into nvim-lspconfig is not the right approach. if a feature is missing in core Nvim LSP, we need to ask why. creating a parallel universe in nvim-lspconfig complicates everything.

nvim-lspconfig should be just a collection of configs for vim.lsp. It is not intended to add missing features to vim.lsp.

[glepnir]

#2418 maybe can fix this.

[jedrzejboczar]

Well, to be fair I don't really know much about the workspaceFolders feature. This PR is more like "regression fix", as my current setup stopped working after recent changes to lspconfig.

I think #2418 is just a temporary fix. Yes, it would fix my current problem because it would disable handling of workspaceFolders for clangd. But I don't think it is a general fix - sooner or later someone will run into the same issue.

---

To give some more context, here is some (simplified) info about my setup (and why there is a problem):

• In my LSP setup I have a custom on_new_config handler
• on_new_config is meant to be run by lspconfig before spawning language server, so that user can set config dynamically
• My handler checks root_dir and searches upwards for a file dir .. '/.nvim/lsp.json'
• This JSON may look somewhat like this (there are more docker/clangd arguments but they are omitted for clarity):

{
    "clangd": {
        "cmd": [
            "docker",
            "run",
            "my-image",
            "clangd",
            "--path-mappings=host/dir=container/dir"
        ]
    }
}

• on_new_config reads the file and applies updates to client configuration changing client.confg.cmd
• Now lspconfig spawns the server and adds it to it's per-root_dir server manger

This way I can have project local clangd configurations. But recent changes to lspconfig resulted in lspconfig trying to use the same server for multiple root_dirs by changing workspaceFolder at runtime. This of course is not valid approach when the servers are run with different cmd. This PR fixes the problem by comparing cmd and some other keys in client config to determine whether it is possible to re-use given server process (as currently lspconfig only compares config.name).

So if any other user uses on_new_config to determine cmd they would have the same problem regardless of language server they use and #2418 won't fix this if their server supports workspaceFolders (as #2418 only fixes problem for servers that don't support workspaceFolders.

---

Regarding LSP functionality in Neovim core: as far as I understand it lspconfig contains a list of server configurations and also provides maneger that keeps track of LSP-to-root_dir mapping etc. However if that's already implemented in Neovim core, then I have to revisit my LSP configuration as I would prefer using lspconfig only as a "list of configurations".

Also, having some generalized "project local settings" functionality in Neovim core would be great, but I know it's a big thing. With the recent addition of vim.secure.truct I might revisit my config to maybe get rid of JSONs and use lua (but I am not sure if it would simplify anything due to the callback-based nature of spawning language servers).

[mfussenegger]

well, is that an intentional design constraint of vim.lsp (cc @mfussenegger )? I don't see any (obvious) discussions: https://github.com/neovim/neovim/search?p=2&q=multiple+workspaceFolders&type=issues

Neovim core supports multiple workspace folders, but it's up to the user to manage it.
E.g. vim.lsp.start by default has a reuse_client implementation that checks the client name and root_dir. A user could provide a custom function for different heuristics and set workspace_folders in the configuration accordingly.

What we could do in core is to add some further convenience to auto-extend the workspace folders if reuse_client returns true and the new configuration has a different root_dir or workspace_folders then existing clients. But providing this custom reuse_client function would still be up to the user (or some config project like lspconfig), because although servers should support workspaceFolders, not every server supports having a workspace split over many folders and instead will use the first and ignore all others.