ALS fails after jumping into library with multiple or zero gpr files

[TamaMcGlinn]

Description

While in my project, ALS works just fine, assuming I have a single .gpr file.
But when I jump through a reference to some library, I get a new ALS instance which fails because it is given the root directory of the library, which contains multiple or zero .gpr files.

Neovim version

NVIM v0.6.0-dev+560-g1fdbd29df
Build type: RelWithDebInfo
LuaJIT 2.1.0-beta3
Compilation: /usr/bin/gcc-11 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 -DNVIM_TS_HAS_SET_MATCH_LIMIT -O2 -g -Og -g -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wshadow -Wconversion -Wmissing-prototypes -Wimplicit-fallthrough -Wvla -fstack-protector-strong -fno-common -fdiagnostics-color=always -DINCLUDE_GENERATED_DECLARATIONS -D_GNU_SOURCE -DNVIM_MSGPACK_HAS_FLOAT32 -DNVIM_UNIBI_HAS_VAR_FROM -DMIN_LOG_LEVEL=3 -I/home/runner/work/neovim/neovim/build/config -I/home/runner/work/neovim/neovim/src -I/home/runner/work/neovim/neovim/.deps/usr/include -I/usr/include -I/home/runner/work/neovim/neovim/build/src/nvim/auto -I/home/runner/work/neovim/neovim/build/include
Compiled by runner@fv-az32-619

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

system vimrc file: "$VIM/sysinit.vim"
fall-back for $VIM: "
/home/runner/work/neovim/neovim/build/nvim.AppDir/usr/share/nvim"

Nvim-lspconfig version

c510964

Operating system and version

Ubuntu 21.04

Affected language servers

als

Steps to reproduce

1. Open Ada file (e.g. src/mkproj.adb in mkproj)
2. Go to definition on something in the gnat runtime, e.g. Ada.Environment_Variables on line 8 of above.

Actual behavior

ALS spits out this error: 'LSP[als] More than one .gpr found. Note: you can configure a project through the ada.projectFile setting' and also doesn't work. For example, I can't jump to definitions / references in the library file now.

Expected behavior

Expected behaviour is to not throw an error and for ALS to work inside the library file as well.

One way to get that behaviour is to set the root dir to /, as in PR1678. This causes the lspconfig to reuse the ALS instance for the project, which already chose the correct gpr file. To chose a different project, you have to restart neovim, or use this gpr selector plugin.

But I assume you have a different fix in mind, since you closed the PR.

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 install_path = join_paths(package_root, 'packer', 'start', 'packer.nvim')
local compile_path = join_paths(install_path, 'plugin', 'packer_compiled.lua')

local function load_plugins()
  require('packer').startup {
    {
      'wbthomason/packer.nvim',
      'neovim/nvim-lspconfig',
    },
    config = {
      package_root = package_root,
      compile_path = compile_path,
    },
  }
end

_G.load_config = function()
  vim.lsp.set_log_level 'trace'
  if vim.fn.has 'nvim-0.5.1' == 1 then
    require('vim.lsp.log').set_format_func(vim.inspect)
  end
  local nvim_lsp = require 'lspconfig'
  local on_attach = function(_, bufnr)
    local function buf_set_keymap(...)
      vim.api.nvim_buf_set_keymap(bufnr, ...)
    end
    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 = { noremap = true, silent = true }
    buf_set_keymap('n', 'gD', '<Cmd>lua vim.lsp.buf.declaration()<CR>', opts)
    buf_set_keymap('n', 'gd', '<Cmd>lua vim.lsp.buf.definition()<CR>', opts)
    buf_set_keymap('n', 'K', '<Cmd>lua vim.lsp.buf.hover()<CR>', opts)
    buf_set_keymap('n', 'gi', '<cmd>lua vim.lsp.buf.implementation()<CR>', opts)
    buf_set_keymap('n', '<C-k>', '<cmd>lua vim.lsp.buf.signature_help()<CR>', opts)
    buf_set_keymap('n', '<space>wa', '<cmd>lua vim.lsp.buf.add_workspace_folder()<CR>', opts)
    buf_set_keymap('n', '<space>wr', '<cmd>lua vim.lsp.buf.remove_workspace_folder()<CR>', opts)
    buf_set_keymap('n', '<space>wl', '<cmd>lua print(vim.inspect(vim.lsp.buf.list_workspace_folders()))<CR>', opts)
    buf_set_keymap('n', '<space>D', '<cmd>lua vim.lsp.buf.type_definition()<CR>', opts)
    buf_set_keymap('n', '<space>rn', '<cmd>lua vim.lsp.buf.rename()<CR>', opts)
    buf_set_keymap('n', 'gr', '<cmd>lua vim.lsp.buf.references()<CR>', opts)
    buf_set_keymap('n', '<space>e', '<cmd>lua vim.lsp.diagnostic.show_line_diagnostics()<CR>', opts)
    buf_set_keymap('n', '[d', '<cmd>lua vim.lsp.diagnostic.goto_prev()<CR>', opts)
    buf_set_keymap('n', ']d', '<cmd>lua vim.lsp.diagnostic.goto_next()<CR>', opts)
    buf_set_keymap('n', '<space>q', '<cmd>lua vim.lsp.diagnostic.set_loclist()<CR>', opts)
  end

  -- Add the server that troubles you here
  local name = 'als'
  nvim_lsp[name].setup {
    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.]]
end

if vim.fn.isdirectory(install_path) == 0 then
  vim.fn.system { 'git', 'clone', 'https://github.com/wbthomason/packer.nvim', install_path }
  load_plugins()
  require('packer').sync()
  vim.cmd [[autocmd User PackerComplete ++once lua load_config()]]
else
  load_plugins()
  require('packer').sync()
  _G.load_config()
end

LSP log

right, but I gave a minimal config

[mjlbach]

But I assume you have a different fix in mind, since you closed the PR.

To clarify, I did not close the PR. You edited documentation files manually which triggered the github action which closes PRs that don't comply with our PR guidelines.

One way to get that behaviour is to set the root dir to /, as in PR1678.

This also incorrectly sends the filesystem root as workspaceFolders to ALS, and is not cross-platform compatible.

[TamaMcGlinn]

Okay, apologies for the misunderstanding. However, I did not edit any documentation files; when I submitted the PR a commit was automatically added on top which edited documentation to match what I had changed in als.vim. The only edit I did was to set the root dir to / and to display the active gpr file. I will review the PR guidelines for next time.

Do you have any other idea how to change the als configuration to never automatically start a new ALS instance? Sending '/' is a real hack of course.

Perhaps I can modify lua/lspconfig/configs.lua line 102 to ask the config for reuse_existing_server which defaults to false and then set that to true for als? Would that be acceptable?

[mjlbach]

However, I did not edit any documentation files; when I submitted the PR a commit was automatically added on top which edited documentation to match what I had changed in als.vim.

That's because you developed on master, we explicitly request developing on feature branches here: https://github.com/neovim/nvim-lspconfig/blob/master/CONTRIBUTING.md#pull-requests-prs.

If you develop on master, it will run the docgen action on your master branch.

Perhaps I can modify lua/lspconfig/configs.lua line 102 to ask the config for reuse_existing_server which defaults to false and then set that to true for als? Would that be acceptable?

If you are proposing special-casing ALS in configs.lua, then no.

Do you have any other idea how to change the als configuration to never automatically start a new ALS instance?

I think it's extremely difficult to solve correctly without project local configuration. I would suggest bypassing lspconfig, and using start_client directly for your usecase.

[TamaMcGlinn]

I am not proposing special-casing ALS in configs.lua. After adding an option for this, any language server will be able to set reuse_existing_server to true to get this behaviour of not starting new language servers. I never suggested adding code like if languageserver == als in configs.lua.

You keep saying 'do this in your config' or 'for your usecase' but I am just trying to fix the als configuration for everyone. Look at the reproduction steps; this is one of the first things someone will do who (attempts to) use als in neovim using lspconfig - it is not at all specific to any project of mine.

[mjlbach]

You keep saying 'do this in your config' or 'for your usecase'

That's because you keep suggesting fixes/changes to lspconfig that seem extremely overfit to ALS/your particular situation (such as proposing a PR which would effectively break ALS support on Windows). I would rather solve the general case elegantly than bolt something on to lspconfig to address your immediate problem.

Here are some concrete suggestions:

• Ensure single_file_support is disabled and special case the root pattern in ALS to ignore system libraries to prevent ALS from starting for system libraries. This would match the behavior of vscode.
• Use project local configuration to pre-configure the workspaceFolders, including those of the system libraries.

[TamaMcGlinn]

The first sounds like a good approach. System libraries are installed in whatever the user has in the GPR_PROJECT_PATH env variable, so I guess I can read that in the root_dir function in als.

However, I'm not sure what you mean by 'special case the root pattern'. It would work if root_dir then searched for existing language servers and returned the root dir of the first one it encounters in this case, but that's quite a workaround.

[mjlbach]

It would work if root_dir then searched for existing language servers and returned the root dir of the first one it encounters in this case

I'm suggesting the opposite. Return False if the pattern matches a system library, so that lspconfig does not try to start (or attach) a language server for system Ada files.

[TamaMcGlinn]

But I want it to attach, otherwise you get no language server for system Ada files. The language server already running in your projects works fine for the system ada files.

[justinmk]

Thanks for your report. The configs here are best-effort:

It is hoped that these configurations serve as a "source of truth", but they are strictly best effort. If something doesn't work, these configs are useful as a starting point, which you can adjust to fit your environment.

• If you have an improvement or fix, please send a PR!
• If you have a specific question we'll try to answer it (keeping in mind the configs in this repo are best-effort and unsupported).
• If you found a bug in the core lsp module, the best way to get it fixed is to describe steps to reproduce it without the particular LSP server and other factors particular to your environment. Or better, by adding a failing test case to lsp_spec.lua, which has code to setup a fake LSP server to simulate various scenarios.

[TamaMcGlinn]

I supplied two PRs (1693 and 1678) that address this issue, but they were rejected because (1) mjlbach said he wanted "to keep the scope of lspconfig limited", and (2) I implemented a fix for linux only, which is easily fixable but (1) still stands so there's no point right now.

I did actually create a separate plugin for choosing which GPR file to use, and this ended up obviating the 'root directory set to /' PR 1678, and thereby also fixing this particular issue with jumping into a library.

Just to emphasize; if you just assemble a hello world example using the most common current open source Ada compiler, and try to jump into the print statement, you already run into this problem. ALS is pretty much unusable in its current state in nvim-lspconfig.

I would happy to redo 1678 in a way that is cross-platform compatible, so that ALS becomes useable without the separate lsp-gpr-selector plugin, but in this current climate it looks like that would be rejected anyway, just based on it being for Ada.

[TamaMcGlinn]

@justinmk I would appreciate it if you closed the issue if and only if the issue is fixed. This bug is still reproducible on the most recent nvim-lspconfig master branch.

[lithammer]

GH-1678 is definitely an odd solution though (i.e. setting / as root). Most (all?) other language servers don't have this problem. Would it be better to look at improving the behaviour of ALS instead?

[TamaMcGlinn]

I agree that 1678 is an odd solution. However, the problem is very clearly in lspconfig, not als, because the problem relates to when to start up als in the first place, and what to pass as the .gpr file to that language server. Hence, there cannot be any solution inside als itself.

Most language servers don't have the feature available, that you can search for system library usages within a particular project - because they do not have this concept of a project file that is understood by the language server. So instead, most language servers rely on 'single-file mode' which supports far less, even though it does work.

In my opinion, the proper solution is to add a new feature to lspconfig, allowing a server to set a boolean that says 'always re-use the existing languageserver instance'. This would be a feature added to all language-servers, even if als is the first to use it.

Since a complaint was made about special-casing lspconfig for als, I did it using the hack of setting the workspace to /, to include the whole system, so that it connects the new file to the existing als instance. For windows, I think the empty string would work (because paths there start with a driveletter).

[TamaMcGlinn]

You may not approve of my proposed solutions, but that doesn't magically fix the issue. This issue is still present. Please re-open.

[justinmk]

Status

#1683 (comment) :

Here are some concrete suggestions:

• Ensure single_file_support is disabled and special case the root pattern in ALS to ignore system libraries to prevent ALS from starting for system libraries. This would match the behavior of vscode.
• Use project local configuration to pre-configure the workspaceFolders, including those of the system libraries.

[TamaMcGlinn]

Eh, not quite; this will be fixed when the removal is complete. Just deprecating als doesn't do it.

[glepnir]

deprecate is a mark as remove in next version.

[TamaMcGlinn]

That is correct. So in the next version, this bug will be fixed. Right now, it's still open. If this is how you normally do things, that's fine, good to know.

Do you want me to re-submit a PR for that version, deleting als? My prior PR of course conflicts on rebase but it's easy to fix / make another.