feat(als): remove als from server_configurations

[TamaMcGlinn]

This PR was closed august 21st because we needed to wait for 0.2.0 but I see now that 1.0.0 is released, so we can remove this deprecated language server now.

Moved ALS config into own repo. See issue #1683
regarding problems with the current config.
lspconfig is not intended to provide functional
lsp configs, but rather a framework for other
plugins to implement those.
For more about that, see the discussion on
#1693

The new ALS LSP config plugin is
https://github.com/TamaMcGlinn/nvim-lspconfig-ada

[github-actions]

Note that server_configurations.md or server_configurations.txt will be regenerated by the docgen CI process. Edit the Lua source file instead. For details on generating documentation, see: https://github.com/neovim/nvim-lspconfig/blob/master/CONTRIBUTING.md#generating-docs

[justinmk]

lspconfig is not intended to provide functional lsp configs, but rather a framework for other plugins to implement those.

FWIW, let me clarify: lspconfig is intended to be pure "data"; and the builtin Nvim LSP client (vim.lsp) is the "framework".

I've been thinking about this more, and I have relaxed on the idea of putting command wrappers in lspconfig, but we just need to make sure that those are minimal, and we don't end up building bespoke plugins for each config. Part of this is tracked in neovim/neovim#28329

[TamaMcGlinn]

I'm just happy to now be able to maintain the Ada LSP settings in a separate plugin. If someone disagrees with my way of setting up ALS, they can just fork that plugin, or fork it and change the name so that people can have both installed and switch between them easily. It seems a sensible way to do all the configs in my opinion.

[glacambre]

I think this change was a mistake. As far as I can tell, there was nothing wrong with the existing als configuration. nvim-lspconfig-ada may be technically superior (I wouldn't know, I prefer to use my own scripting solution on top of what used to be in nvim-lspconfig), but there was no reason to remove the als definition from nvim-lspconfig: as far as I could tell, the als' presence in nvim-lspconfig would not prevent nvim-lspconfig-ada from being used.

There is a high cost to installing plugins, especially in terms of trust. nvim-lspconfig is a trusted source, as it belongs to the neovim organisation. There was value in having a standard and trusted alternative available to users, even if it provided less things out of the box.

This opinion isn't only my own, this is feedback I got from colleagues at $WORK too (and in fact only decided to share after I discovered I wasn't alone feeling this way).

I think neovim's user experienced would be improved if some rule was established about not removing working server configurations from nvim-lspconfig.

Now, for those who found this issue and are looking to go back to how things were with minimal effort, you can add the following to your configuration:

local configs = require("lspconfig.configs")
configs.ada_language_server = {
  default_config = {
    cmd = { 'ada_language_server' },
    filetypes = { 'ada' },
    root_dir = l.util.root_pattern('Makefile', '.git', '*.gpr', '*.adc'),
  },
}

And then, use require("lspconfig").ada_language_server.setup{ ... } the same way you used to do als.setup{ ... }.

[justinmk]

There is a high cost to installing plugins, especially in terms of trust. nvim-lspconfig is a trusted source, as it belongs to the neovim organisation. There was value in having a standard and trusted alternative available to users, even if it provided less things out of the box.

Good point. I agree it should be restored. This is partly my fault, because I gave mixed signals about the purpose of nvim-lspconfig. My latest comment at #1937 (comment) hopefully removes some confusion.

[TamaMcGlinn]

As far as I can tell, there was nothing wrong with the existing als configuration.

What about issue 1683 then?

I agree that the old als configuration was useable - as long as you happened to not want to jump into any package outside the current project directory. My change is really just a minor fix, not deserving of being a whole new plugin. If there was an option to simply fix the existing config, I would do that, but I tried that and was told that neovim is intended to go the route of VSCode, with separate plugins per language, so that the maintenance burden is only on those who actually know how the language server works - and I can see the point @mjlbach and @justinmk are making on that (see discussion in #1693).

However, I also agree that it's quite messy having these as separate plugins. Just look at this PR - switching to the new method is going to break for some users as they will need to update nvim-lspconfig to the latest version whenever updating nvim-lspconfig-ada; this problem is going to keep cropping up, because any language server plugins built on top of nvim-lspconfig have a dependency on that version of nvim-lspconfig. For now all I can do is periodically update to the latest version, make sure everything works, and pin that version of nvim-lspconfig in my plugin's readme.

@justinmk what's your view on this dependency problem and how it should be resolved, in general for nvim-lspconfig-based plugins? If indeed my fix had simply been made in-place, your later refactoring would of course have also updated the als plugin.

[justinmk]

what's your view on this dependency problem and how it should be resolved

Are you referring to potentially breaking changes in nvim-lspconfig? If so, we plan to avoid those. Things like #3330 will be paired with a compatibility shim in the future.

[TamaMcGlinn]

Yes. A compatibility shim would indeed have avoided the issue. At the moment, since Ada is the only plugin that you're not allowing us to maintain inside nvim-lspconfig, it is also the only one affected by this compatibility problem. So I quite understand if things will break like that in future, despite best efforts at maintaining backward compatibility - out of sight = out of mind.

Are you now proposing to include the latest version of https://github.com/TamaMcGlinn/nvim-lspconfig-ada, and allow us to maintain it like the other plugins? That would be great!

Or, the complete opposite, would you welcome a PR that removes all the other plugins as well, since they should be in separate repositories? I just want the Ada plugin to work the same as the others, it doesn't matter to me which way you want to go.

I just hope you're not proposing to include the old and broken als config - that would immediately break things for all users who have installed nvim-lspconfig-ada, as the lsp config name is the same, "als".

[TamaMcGlinn]

@glacambre

(I wouldn't know, I prefer to use my own scripting solution on top of what used to be in nvim-lspconfig)

How do you know you prefer your scripting solution if you haven't tried mine? Did you run into the same or different problems, that you needed to code around? Anyway, please share your solution so that we can collectively decide what is best.

[justinmk]

I just hope you're not proposing to include the old and broken als config

I have no idea about the details here and don't have time to deep dive. If what @glacambre requested seems reasonable then that is what I'm in favor of.

[glacambre]

What about issue 1683 then?

It's probably because the ALS has been improved since you opened that issue, but can't reproduce the problem you describe in the OP (errors when navigating to runtime sources). Furthermore, I believe I've never encountered this issue because I either use the ALS on projects that have extremely simple project structures (and so the ALS works by default) or projects where there are multiple project files and the one to use cannot be decided by a generic mechanism (I instead hardcoded my choices through scripting).

At the moment, since Ada is the only plugin that you're not allowing us to maintain inside nvim-lspconfig

It's not a matter of allowing or not allowing a language server in nvim-lspconfig, it's a matter of allowing code vs data declarations. The data declarations of the ALS are fine and allowed.

I just hope you're not proposing to include the old and broken als config - that would immediately break things for all users who have installed nvim-lspconfig-ada, as the lsp config name is the same, "als".

If a runtime path conflict is the only thing to worry about, then I wonder if we could find a way to ensure that nvim-lspconfig's als.lua is always found last. If not, the configuration could be re-introduced as ada_language_server.lua.

How do you know you prefer your scripting solution if you haven't tried mine?

I prefer my solution because my solution requires less trust.

please share your solution

It's not very generalizable, basically I do:

local function als_on_init(client)
       local result = nil
       local path = client.workspace_folders[1].uri:sub(8)
       if vim.fn.filereadable(path .. "/gnat.gpr") ~= 0 then
               result = path .. "/gnat.gpr"
       elseif vim.fn.filereadable(path .. "/gnat2scil.gpr") ~= 0 then
               result = path .. "/gnat2scil.gpr"
       elseif ... then
          ...
       end
       if result ~= nil then
               client.config.settings.ada = { projectFile = result, log_level = 0 } 
               client.notify("workspace/didChangeConfiguration", client.config.settings)
       end
       return true
end
l.ada_language_server.setup{
       on_init = als_on_init;
       settings = {};
}

[TamaMcGlinn]

Thanks for sharing. Looks like you are basically hardcoding in your config a set hierarchy of projectfiles, to which you add all the projects you need. This works as long as you only ever need one particular projectfile inside each project - in order to switch projectfiles within one project you need to modify & reload your config, and restart als, which would be tedious. Often Ada projects have many library projects which are never selected, and one main project, in which case indeed you can hardcode it in various ways.

However, you could still consider using nvim-lsp-gpr-selector instead; it asks you on startup which projectfile to use whenever there is ambiguity, and also allows you to fuzzy search for project files, in case there's many. It then remembers your choice for next time, so it should give you strictly better behaviour than your script. In case there's anything missing in your workflow after switching, please open an issue and it can be fixed.

It's 109 lines of lua, so if trust is an issue then just review, fork and pin one version of it in your config, e.g.

Plug 'glacambre/nvim-lsp-gpr-selector', { 'branch': 'reviewed_version' }

If what @glacambre requested seems reasonable then that is what I'm in favor of.

It's not entirely clear to me which version @glacambre is proposing - the mistake he points to seems to be that nvim-lspconfig no longer works at all for Ada, and I agree that's not desirable. I also agree with @glacambre that backwards compatibility is important. Hence, I would like those who have made the step to use nvim-lspconfig-ada to not end up with a broken config after some version is added again in the default nvim-lspconfig.

If a runtime path conflict is the only thing to worry about, then I wonder if we could find a way to ensure that nvim-lspconfig's als.lua is always found last. If not, the configuration could be re-introduced as ada_language_server.lua.

I think changing the filename and also the config name is necessary.

[TamaMcGlinn]

@glacambre I concur that the original issue #1683 no longer occurs, using the config you provided (minus the errant "l." - undefined global, please edit).

I still do like showing the active GPR file though, especially since a similar issue still does happen if you don't select which project file to use and there are multiple in the root ("No project was loaded, because more than one project file has been found in the root directory. Please change configuration to point to a correct project file").

Try opening src/two.adb in test_multigpr without gprselector installed. However, I can see that the urgency of my modified config is indeed now greatly reduced - the only difference is showing the active project file, or the helpful GPR project: Ambiguous (error)" to point toward the problem when doing :LspInfo.

As long as it comes back with a different name and filename so that nvim-lspconfig-ada continues to work, I approve.

But as for your statement that Ada isn't being special-cased, but simply must be data-only like the other configs, that's a novel idea as it wasn't suggested at all in my pullrequest. And indeed, searching the other configs, I found 143 others that include functions. A few examples are below. Rather, @justinmk's stated reason we can't have this feature for Ada is:

Maintaining customizations for hundreds of configs is not sustainable and not in the purview of nvim-lspconfig.

In other words, he doesn't use Ada and doesn't want to maintain it, so split it off into a separate plugin, and that's really how all the configs should be done eventually. Fine. I maintained a fork of nvim-lspconfig for a year first, but it got too annoying having to explain to colleagues why it was broken each time I rebased my changes, so I took the advice and made it a separate plugin.

gopls.lua

    root_dir = function(fname)
      -- see: https://github.com/neovim/nvim-lspconfig/issues/804
      if not mod_cache then
        local result = async.run_command { 'go', 'env', 'GOMODCACHE' }
        if result and result[1] then
          mod_cache = vim.trim(result[1])
        else
          mod_cache = vim.fn.system 'go env GOMODCACHE'
        end
      end
      if mod_cache and fname:sub(1, #mod_cache) == mod_cache then
        local clients = util.get_lsp_clients { name = 'gopls' }
        if #clients > 0 then
          return clients[#clients].config.root_dir
        end
      end
      return util.root_pattern('go.work', 'go.mod', '.git')(fname)
    end,

vala_ls.lua:

    root_dir = function(fname)
      local root = util.search_ancestors(fname, meson_matcher)
      return root or util.find_git_ancestor(fname)
    end,

[justinmk]

Rather, @justinmk's stated reason we can't have this feature for Ada is:

Maintaining customizations for hundreds of configs is not sustainable and not in the purview of nvim-lspconfig.

That still holds in general, but simple commands are in-scope (though we have work to do to provide a common Nvim core features so that commands defined in lspconfig can be minimal). Customizing lspinfo is not part of that, and creates a web of entangled workarounds that we don't want. #3375

LspInfo is now simply a checkhealth report, I'm not sure if that changes anything for your particular case.

[TamaMcGlinn]

LspInfo is now simply a checkhealth report, I'm not sure if that changes anything for your particular case.

No, it's still crucial to know whether a GPR project file was unambiguously set. It's non-obvious and quite fatal to not set it when this is ambiguous. You can't really call it a checkhealth for ALS if it's not allowed to report this setting.

Does it become okay if I reduce the code in lspinfo to a one-liner? For example, not searching for a gpr file but just reporting if it was explicitly set? Or is any customization of lspinfo disallowed, such as the 16 lines of code you can find in hls.lua?

[justinmk]

Does it become okay if I reduce the code in lspinfo to a one-liner?

well, can we generalize this in LspInfo itself, instead of adding per-config overrides? Does the lsp server report this field in any of its responses? #3376

[glacambre]

well, can we generalize this in LspInfo itself

Could a new field be added to language server configurations listing additional workspace/executeCommand requests that would be nice to perform on :LspInfo? Then the als' configuration could have als-project-file in that field, and Neovim could display the result of that request as a string in LspInfo's buffer.

[justinmk]

That definitely seems preferable. It touches on my naïve idea in neovim/neovim#28329

Added to #3376