go-to-definition spawns new LSP server with root at definition

[jonhoo]

Description

When you use go-to-definition on a symbol that takes you outside of the current project (such as one defined in another crate in the Rust world), nvim-lspconfig will spawn a new LSP server (at least for rust-analyzer) with the current working directory set to the directory of the project that holds the symbol definition, rather than the current nvim working directory. This, in turn, causes issues when the working directory has implications for the LSP server's configuration (or the configurations of the tools it in turn invokes).

It's not entirely clear to me whether this a bug with neovim's lsp support, with nvim-lspconfig, with rust-analyzer, or with LSP more broadly, but I suspect it's a nvim-lspconfig issue as VSCode for example handles this correctly.

I also believe this is related to (but not quite the same as) #804.

I think the right solution here is to either find a way to continue using the same LSP server instance, or to spawn the new LSP server instances in :pwd, and then point it at the project directory for the definition. That way it'll still pick up the right configuration.

Neovim version

NVIM v0.7.2
Build type: Debug
LuaJIT 2.1.0-beta3
Compilation: /usr/bin/clang -DNVIM_TS_HAS_SET_MATCH_LIMIT -DNVIM_TS_HAS_SET_ALLOCATOR -g -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wshadow -Wconversion -Wdouble-promotion -Wmissing-noreturn -Wmissing-format-attribute -Wmissing-prototypes -Wimplicit-fallthrough -Wvla -fstack-protector-strong -fno-common -fdiagnostics-color=auto -DINCLUDE_GENERATED_DECLARATIONS -D_GNU_SOURCE -DNVIM_MSGPACK_HAS_FLOAT32 -DNVIM_UNIBI_HAS_VAR_FROM -DMIN_LOG_LEVEL=1

Features: +acl +iconv +tui
See ":help feature-compile"

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "/usr/local/share/nvim"

Run :checkhealth for more info

Nvim-lspconfig version

d346335

Operating system and version

Amazon Linux 2 (aarch64; 5.4 kernel)

Affected language servers

rust-analyzer, though likely also others

Steps to reproduce

For example, consider two Rust projects where one depends on the other, and both require nightly Rust, but only the consumer has a Rustup toolchain override file:

$ cargo new --lib needs-nightly
$ cargo new --lib consumer-of-needs-nightly
$ echo 'needs-nightly = { path = "../needs-nightly" }' >> consumer-of-needs-nightly/Cargo.toml
$ echo -e 'nightly' > consumer-of-needs-nightly/^C
$ echo '#![feature(allocator_api)]' > needs-nightly/src/lib.rs
$ echo 'pub struct Foo;' >> needs-nightly/src/lib.rs
$ echo 'use needs_nightly::Foo;' > consumer-of-needs-nightly/src/lib.rs
$ cd consumer-of-needs-nightly/ && rustup override set nightly
$ cargo check
...
    Finished dev [unoptimized + debuginfo] target(s) in 0.15s
$ nvim src/lib.rs

This is a pretty common scenario — consider if needs-nightly isn't actually a local project, but instead is depended on using a version number, and so its definition is actually somewhere in ~/.cargo/src/cache/whatever. The version that's there is unlikely to have a rust-toolchain.toml file.

Actual behavior

In the opened neovim session, all LSP features work just fine. However, if you "go to definition" on Foo in the use line, notice that the #![feature(allocator_api)] gets highlighted with the error

#![feature(allocator_api)]     _ `#![feature]` may not be used on the stable release channel

Despite :pwd having a rust-toolchain.toml that specifies nightly. There are many other ways for this to manifest, such as if the current directory has a .cargo/config.toml that defines other (required) rustflags, new Cargo registries, etc., that can even cause the LSP server to fail to start entirely.

(the relevant second LSP server launch is here)

Expected behavior

After go-to-definition, the configuration from :pwd should continue to apply, meaning rust-toolchain.toml should still be respected, so no errors should be shown in needs-nightly.

Minimal config

local on_windows = vim.loop.os_uname().version:match 'Windows'

local function join_paths(...)
  local path_sep = on_windows and '\\' or '/'
  local result = table.concat({ ... }, path_sep)
  return result
end

vim.cmd [[set runtimepath=$VIMRUNTIME]]

local temp_dir = vim.loop.os_getenv 'TEMP' or '/tmp'

vim.cmd('set packpath=' .. join_paths(temp_dir, 'nvim', 'site'))

local package_root = join_paths(temp_dir, 'nvim', 'site', 'pack')
local lspconfig_path = join_paths(package_root, 'test', 'start', 'nvim-lspconfig')

if vim.fn.isdirectory(lspconfig_path) ~= 1 then
  vim.fn.system { 'git', 'clone', 'https://github.com/neovim/nvim-lspconfig', lspconfig_path }
end

vim.lsp.set_log_level 'trace'
require('vim.lsp.log').set_format_func(vim.inspect)
local nvim_lsp = require 'lspconfig'
local on_attach = function(_, bufnr)
  local function buf_set_option(...)
    vim.api.nvim_buf_set_option(bufnr, ...)
  end

  buf_set_option('omnifunc', 'v:lua.vim.lsp.omnifunc')

  -- Mappings.
  local opts = { buffer = bufnr, noremap = true, silent = true }
  vim.keymap.set('n', 'gD', vim.lsp.buf.declaration, opts)
  vim.keymap.set('n', 'gd', vim.lsp.buf.definition, opts)
  vim.keymap.set('n', 'K', vim.lsp.buf.hover, opts)
  vim.keymap.set('n', 'gi', vim.lsp.buf.implementation, opts)
  vim.keymap.set('n', '<C-k>', vim.lsp.buf.signature_help, opts)
  vim.keymap.set('n', '<space>wa', vim.lsp.buf.add_workspace_folder, opts)
  vim.keymap.set('n', '<space>wr', vim.lsp.buf.remove_workspace_folder, opts)
  vim.keymap.set('n', '<space>wl', function()
    print(vim.inspect(vim.lsp.buf.list_workspace_folders()))
  end, opts)
  vim.keymap.set('n', '<space>D', vim.lsp.buf.type_definition, opts)
  vim.keymap.set('n', '<space>rn', vim.lsp.buf.rename, opts)
  vim.keymap.set('n', 'gr', vim.lsp.buf.references, opts)
  vim.keymap.set('n', '<space>e', vim.diagnostic.open_float, opts)
  vim.keymap.set('n', '[d', vim.diagnostic.goto_prev, opts)
  vim.keymap.set('n', ']d', vim.diagnostic.goto_next, opts)
  vim.keymap.set('n', '<space>q', vim.diagnostic.setloclist, opts)
end

-- Add the server that troubles you here
local name = 'rust_analyzer'
local cmd = { 'rust-analyzer' }
if not name then
  print 'You have not defined a server name, please edit minimal_init.lua'
end
if not nvim_lsp[name].document_config.default_config.cmd and not cmd then
  print [[You have not defined a server default cmd for a server
    that requires it please edit minimal_init.lua]]
end

nvim_lsp[name].setup {
  cmd = cmd,
  on_attach = on_attach,
}

print [[You can find your log at $HOME/.cache/nvim/lsp.log. Please paste in a github issue under a details tag as described in the issue template.]]

LSP log

https://gist.github.com/jonhoo/a4d400ee5d14e153904395e3eb12d497

[sigmaSd]

Hello @jonhoo

This doesn't seem to need nightly to trigger, you can use two crates that depend on each other and just jumping between them will spawn a new lsp server which is incorrect (vscode is the reference indeed)

The problem boils down to https://github.com/neovim/nvim-lspconfig/blob/master/lua/lspconfig/configs.lua#L227 and https://github.com/neovim/nvim-lspconfig/blob/master/lua/lspconfig/util.lua#L288 when we see a buffer that doesn't have a client attached to it, we spawn a new lsp server, the correct approach is to send 'textDocument/didOpen (which is vim.lsp_attach) instead

I think if this is all correct, the fix is to to detect here https://github.com/neovim/nvim-lspconfig/blob/master/lua/lspconfig/util.lua#L288 if there is already an active lsp of the same language we don't need to spawn a new one

[sigmaSd]

The traceback for second start is like this (using vim.debug.traceback())

"stack traceback:
\t/usr/share/nvim/runtime/lua/vim/lsp/rpc.lua:660: in function 'start'
\t/usr/share/nvim/runtime/lua/vim/lsp.lua:1178: in function 'start_client'
\t.../pack/packer/start/nvim-lspconfig/lua/lspconfig/util.lua:288: in function 'add'
\t...ck/packer/start/nvim-lspconfig/lua/lspconfig/configs.lua:248: in function 'try_add'
\t...ck/packer/start/nvim-lspconfig/lua/lspconfig/configs.lua:76: in function <...ck/packer/start/nvim-lspconfig/lua/lspconfig/configs.lua:75>
\t[C]: in function 'nvim_cmd'
\t/usr/share/nvim/runtime/filetype.lua:23: in function </usr/share/nvim/runtime/filetype.lua:22>
\t[C]: in function 'nvim_buf_call'
\t/usr/share/nvim/runtime/filetype.lua:22: in function </usr/share/nvim/runtime/filetype.lua:11>
\t[C]: in function 'nvim_set_current_buf'
\t/usr/share/nvim/runtime/lua/vim/lsp/util.lua:1092: in function 'jump_to_location'
\t/usr/share/nvim/runtime/lua/vim/lsp/handlers.lua:359: in function 'handler'\n\t/usr/share/nvim/runtime/lua/vim/lsp.lua:1383: in function ''\n\tvim/_editor.lua: in function <vim/_editor.lua:0>"

[sigmaSd]

I patched the code and indeed seems to fix the origianl issue, so for summery

• this should become spawn a new server if there is not already an active one for the same filetype https://github.com/neovim/nvim-lspconfig/blob/master/lua/lspconfig/configs.lua#L226
• we can add a check here maybe, to check if clients have client for the same filetype then just return that client_id https://github.com/neovim/nvim-lspconfig/blob/master/lua/lspconfig/util.lua#L252

I'm not sure exactly how to check and where for the filetype though

[yioneko]

This might be related: #842.

Personally I think the design of root_dir is totally a mistake as it's out of spec and might result in confusion like this issue. And I use this workaround for setup of all the servers:

require('lspconfig')[name].setup {
   root_dir = function()
      return vim.loop.cwd()
   end
}

This will ensure that no multiple instances of the server will be spawned.

[glepnir]

Just simply looked at the code of emacs lsp-bridge lsp-mode and coc.nvim about the root dir, there are some problems in the design of the root dir of lspconfig. But I haven't thought of a good refactoring solution yet

[glepnir]

I removed the root_dir on the core. Re-made only the server initialization parameters. Use the current cwd. When the server starts successfully, add a unique identifier, which is generated by cwd.

1. The root dir does not meet the lsp standard
2. The root dir is only a basis for us to detect whether there is an existing client
3. Finding rootdir needs to use a lot of checks which is a bit wasteful of time.

[yioneko]

Just for reference, here is how coc.nvim deals with workspace folders. It seems that at least coc.nvim also uses detection mechanism like root_dir, but much smarter to test cwd first.

The root cause, I think, should be the lack of support of workspace folders feature at core.

• vscode handles multiple workspace folders by requiring user to define a special config file at root
• sublime does the same

And neovim doesn't have such feature currently. Or more specifically, we do not have such a concept like "workspace" or "project" in editor. I'm not familiar with core and its roadmap, so I cannot dig on it further here. I always launch nvim at the root of the project, thus cwd is already the best approximation to "workspace" for me. I guess this is not the case for everyone, and directly using cwd should not be always safe.

[glepnir]

agree. I checked the emacs lsp-bridge lsp-mode and vim yegappan/lsp . I think we need rethinking about of root_dir .