Skip to content

feat(lsp): add vim.lsp.config and vim.lsp.enable#31031

Merged
lewis6991 merged 1 commit intoneovim:masterfrom
lewis6991:feat/lspsetup
Dec 10, 2024
Merged

feat(lsp): add vim.lsp.config and vim.lsp.enable#31031
lewis6991 merged 1 commit intoneovim:masterfrom
lewis6991:feat/lspsetup

Conversation

@lewis6991
Copy link
Member

@lewis6991 lewis6991 commented Nov 1, 2024

Problem

  • Setting up LSP clients require unnecessary boilerplate.
  • LSP configuration has no system.

Solution

Add vim.lsp.enable() and vim.lsp.config().

Design goals/requirements:

  • Default configuration of a server can be distributed across multiple sources.
    • And via RTP discovery.
  • Default configuration can be specified for all servers.
    • Can extend capabilities of all clients.
  • Configuration can be project specific.

Proposal:

  • Two new API's:
    • vim.lsp.config(name, cfg):
      • Used to define configurations for servers of name.
      • Can be used like a table or called as a function.
        • Use vim.lsp.config(name, cfg) to extend a configuration
        • Use vim.lsp.config[name] = cfg to define a configuration
      • Use vim.lsp.confg('*', cfg) to specify default config for all
        servers.
      • Examples:
        -- Set default root marker for all clients
        vim.lsp.config('*', {
          root_markers = { '.git' },
        })
        
        -- Set default configuration for clangd but don't enable it
        vim.lsp.config.clangd = {
          cmd = {
            'clangd',
            '--clang-tidy',
            '--background-index',
            '--offset-encoding=utf-8',
          },
          root_markers = { '.clangd', 'compile_commands.json' },
          filetypes = { 'c', 'cpp' },
        }
    • vim.lsp.enable(name)
      • Used to enable servers of name. Uses configuration defined via vim.lsp.config().
      • Examples:
        vim.lsp.enable('clangd')
  • Client configuration sources lsp/<name>.lua to resolve configurations.
    • Only done once per enabled configuration.
    • Redone if vim.lsp.config[name] is accessed (read or modified).

Notes:

  • Instead of adding vim.lsp.config(name, cfg) I considered
    vim.lsp.enable(name, cfg, config_only) where
    vim.lsp.enable(name, cfg, true) === vim.lsp.config(name, cfg).

    I decided against this since I felt a config_only field is slightly more confusing, harder to explain and less generally appealing than having vim.lsp.config. There is also some parallel to
    vim.diagnostic.enable and vim.diagnostic.config.

    If there is pushback to this, then I can remove that from this PR, and add it to a separate future PR for further consideration.

  • This PR doesn't provide a full solution for project specific config, but I'm pretty sure doesn't inhibit it. Though I think for most cases using things like workspace/didChangeConfiguration in on_attach or LspAttach should be sufficient.

  • This PR is not intended to be the final end goal of LSP configuration but just a medium term stepping stone that moves us forward in the right direction.

    Going forward I'd expect us to add some options into the native option system to complement (or replace) what is added here:

    • Buffer local options:

      • :setlocal lsp+=clangd
      • :setlocal root_marker+=compile_commands.json
    • LSP namespaced specific options:

      • :set lsp_servers.clangd.cmd=['clangd']
  • Continuation of feat(lsp): add vim.lsp.config #18506

Todo:

  • News
  • Documentation
  • Tests
  • Health check

Future considerations

  • Consider allowing a table to be returned from lsp/*.lua.
  • Better accommodate project specific configurations.

@github-actions github-actions bot added the lsp label Nov 1, 2024
@lewis6991 lewis6991 added the needs:discussion Need comments/opinions from people familiar with this topic. label Nov 1, 2024
@clason

This comment was marked as resolved.

@lewis6991

This comment was marked as resolved.

@lewis6991
Copy link
Member Author

lewis6991 commented Nov 1, 2024

I'm happy to hear the sorts of stuff you want to configure. From my config there isn't much. From the work I've done on the handlers recently, there isn't a huge amount other than floating borders which can be done with something like vim.go.float_border.

Is there anything else we need to consider?

For reference this is all I have in my config:

local function with(f, config)
  return function(c)
    return f(vim.tbl_deep_extend('force', config, c or {}))
  end
end

vim.lsp.buf.signature_help = with(vim.lsp.buf.signature_help, {
  border = 'rounded',
  title_pos = 'left',
})

local have_cmp, cmp_nvim_lsp = pcall(require, 'cmp_nvim_lsp')
if have_cmp then
  local f = lsp.protocol.make_client_capabilities
  lsp.protocol.make_client_capabilities = function()
    return vim.tbl_deep_extend(
      'force',
      f(),
      cmp_nvim_lsp.default_capabilities()
    )
  end
end

Copy link
Member

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I've only mentioned this in chat, but I think it would be really great if a root_dir_marker ends up being a general project construct and not tied to lsp.

E.g. it could be a filetype option so one could use it with vim.filetype.get_option("lua", "root_dir_markers"). E.g. in nvim-lint this came up too and I've so far told people that they can query lsp.get_clients() and then use that root_dir. Would be nice if there were a more direct way to do that.

Other than that, this seems simple enough - I like it.

@lewis6991
Copy link
Member Author

Ah yes, I do recall that. At least with this, markers are optional so it leaves room to retrofit a buffer option in the future.

I can draft up a PR to at least see what such an option could look like.

@gpanders
Copy link
Member

gpanders commented Nov 3, 2024

How would this work if I wanted to configure my LSP client differently for a single project? I would want to have a "main" vim.lsp.setup() that sets the options that I will use as the default for most projects, but if I want to configure my LSP differently for just one project, how would I do that?

Since each call to vim.lsp.setup() creates a new FileType autocommand, calling vim.lsp.setup() in a .nvimrc file (with project-specific configuration) wouldn't work because that would end up creating two autocommands. This would create two clients (or perhaps the second autocommand would re-use the client from the first autocommand?).

I mentioned in chat a while ago that I've been using my own implementation of "user-friendly LSP configuration" that uses "passive discovery" (by finding files on the user's runtimepath) rather than "active registration" (i.e. explicitly calling vim.lsp.setup or similar). But frankly I'm happy to see any movement towards making LSP configuration easier so I'm supportive of this, if there's consensus among the team. I just want to make sure we're thinking about all possible use cases.

@lewis6991
Copy link
Member Author

lewis6991 commented Nov 3, 2024

How would this work if I wanted to configure my LSP client differently for a single project? I would want to have a "main" vim.lsp.setup() that sets the options that I will use as the default for most projects, but if I want to configure my LSP differently for just one project, how would I do that?

Fair question. What would you want to configure that isn't possible via vim.lsp.ClientConfig? Generally I lean towards providing my LSP configuration via editor agnostic methods, e.g. .luarc.json, .clangd, rather than implementing that with Nvim logic.

Another way would to to use something like:

        client.notify('workspace/didChangeConfiguration', { settings = client.settings })

in a LspAttach autocmd.

So I guess the details matter.

Note that currently the client name is used as a sort of key to prevent multiple clients attaching to the same buffer. I can't quite figure where the exact logic for this is, but the way I've been testing my multi-client handler changes is by calling lsp.setup multiple times but by providing an explicit client.name (e.g. luals and luals2). It's not an exact solution, but it might lead to something.

I mentioned in chat a while ago that I've been using my own implementation of "user-friendly LSP configuration" that uses "passive discovery" (by finding files on the user's runtimepath) rather than "active registration" (i.e. explicitly calling vim.lsp.setup or similar). But frankly I'm happy to see any movement towards making LSP configuration easier so I'm supportive of this, if there's consensus among the team. I just want to make sure we're thinking about all possible use cases.

I don't think this means we can't also have and rtp based approach as well. Just like we have FileType autocmds in addition to ftplugin/.

Does your rtp based approach have any significant differences to having something like plugin/luals.lua which just calls vim.lsp.setup?

@mfussenegger
Copy link
Member

mfussenegger commented Nov 3, 2024

I can't quite figure where the exact logic for this is

Do you mean

if reuse_client(client, config) then
?

Update: Nevermind, that's already there


I mentioned in chat a while ago that I've been using my own implementation of "user-friendly LSP configuration" that uses "passive discovery" (by finding files on the user's runtimepath) rather than "active registration"

Do you have a small example how that would look like?
There was also an issue lately in the lsp repo about servers creating a .lsp entry to make them discoverable. See microsoft/language-server-protocol#2051, which might be related?

@gpanders
Copy link
Member

gpanders commented Nov 3, 2024

What would you want to configure that isn't possible via vim.lsp.ClientConfig?

gopls doesn't have any sort of configuration file so all configuration has to be done through the settings field in the initialization request. One example of a project-specific configuration setting is the local setting, which doesn't make sense to use globally/for all projects. For one project at work, I also explicitly set the root directory for both gopls and rust-analyzer (rather than relying on root markers).

Maybe this is possible using only vim.lsp.ClientConfig though.

Does your rtp based approach have any significant differences to having something like plugin/luals.lua which just calls vim.lsp.setup?

I think the only significant difference is the fact that the rtp approach "merges" all of the files found in the rtp for a given server before calling vim.lsp.start. This allows a "hierarchy" of configuration settings (e.g. project-local -> plugin -> user).

Do you have a small example how that would look like?

The user creates Lua files under an lsp/ directory on the runtimepath. Examples: https://github.com/gpanders/dotfiles/tree/master/.config/nvim/lsp. The implementation itself is here (it's only about 100 lines of Lua).

This extends well to project-local configuration. A user can create a .nvimrc file which adds any directory to the runtimepath. In my case, I have set runtimepath+=$PWD/.nvim in .nvimrc and create overrides in e.g. .nvim/lsp/gopls.lua.

@przepompownia

This comment was marked as resolved.

@justinmk

This comment was marked as resolved.

@lewis6991

This comment was marked as resolved.

@justinmk

This comment was marked as resolved.

@lewis6991

This comment was marked as resolved.

@justinmk
Copy link
Member

justinmk commented Nov 4, 2024

Because a function for non server specific configuration would have a different signature, and I don't want to combine these two things into a single function.

"Non-server specific" means something like "global defaults"? I don't see why they would have different interfaces, in fact it would be strange if they did.

@lewis6991
Copy link
Member Author

lewis6991 commented Nov 4, 2024

"Non-server specific" means something like "global defaults"?

No it does not. It means things like border hovers, quickfix handling and settings that have nothing to do with a server. Basically defaults for configs that may pass to vim.lsp.buf which are designed to handle multiple servers/clients, and thus their configs are not specific to any one server/client.

@mfussenegger
Copy link
Member

mfussenegger commented Nov 5, 2024

lsp.enable() could be another naming option?

means things like border hovers, quickfix handling and settings that have nothing to do with a server. Basically defaults for configs that may pass to vim.lsp.buf which are designed to handle multiple servers/clients, and thus their configs are not specific to any one server/client.

If it's about global options, shouldn't they just use the option system? Relating to #31074 (comment)

And if it's for options that are client/server specific, putting them under the same lsp.config(setup) function would fit again?

@clason
Copy link
Member

clason commented Nov 5, 2024

I think a big part of "API pressure" here comes from the fact that we (still) don't have table options; there is (understandable) reticence to add a bucketful of new options for different LSP (and diagnostics and treesitter and...) aspects.

(And I like enable())

@lewis6991
Copy link
Member Author

If it's about global options, shouldn't they just use the option system? Relating to #31074 (comment)

Maybe, but that then puts the existence of vim.diagnostic.config under question.

And if it's for options that are client/server specific, putting them under the same lsp.config(setup) function would fit again?

Yes. I'm only suggesting we reservevim.lsp.config for config that doesn't fit in a specific client/server.

It might look something like this:

vim.lsp.config({
  hover = {
    silent = true
  },
  declaration = {
    reuse_win = true
  },
  signature_help = {
    silent = false
  },
  format = {
    filter = function(client) ... end
    timeout_ms = 100,
  },
  rename = {
    filter = function() ... end
  },
  references = {
    loclist = true,
  },
  code_action = {
    apply = true,
  }
})

Note this example includes options we already support. The schema for this config can be semi-autoamtically derived by coupling it with the functions in vim.lsp.buf.*.

@mfussenegger
Copy link
Member

mfussenegger commented Nov 5, 2024

but that then puts the existence of vim.diagnostic.config under question.

diagnostic.config has per-provider namespacing. Drawing that parallel would only match for per-client/server options.

Unless options started to support namespace indexing vim.o[ns].xy = ...

I think a big part of "API pressure" here comes from the fact that we (still) don't have table options

Fair, but I don't think the handler changes add much pressure to add these options now. Before one had to monkey patch handlers, now one could monkey patch the buf. functions. Not like one is worse than the other. (Not that I like that approach much)

It might look something like this [..]

Still unclear if all these aspects warrant global configuration.

  • silent: Can explore supporting the silent flag set via :verbose and keymap.set(... { silent }).
  • reuse_win: Maybe use 'switchbuf'
  • format filter: This seems client specific in a way and overlaps with changing capabilities to disable formatting on a client
  • rename filter: Same
  • references loclist: Keymap might be good enough?
  • code_action apply: I'd think this doesn't make for a good global in any case, as you likely want to filter the code-actions for a specific one. E.g. for organize imports or something. E.g. I have one keymap for all code-actions, one filtered for refactoring actions.

But assuming global options for these is warranted - if options supported tables nicely, and these don't need client/server namespacing - this could be vim.o.lsp = {} or vim.o.lsp_hover = { silent = true }


But independent of this aspect. I'd probably still favour enable here over config (or setup) because it feels like a more an active operation than vim.diagnostic.config and it mirrors the subsystem's enable functions (completion.enable, etc.)

@TravonteD

This comment has been minimized.

@lewis6991

This comment has been minimized.

@neovim neovim locked and limited conversation to collaborators Nov 18, 2024
@justinmk
Copy link
Member

justinmk commented Dec 6, 2024

@lithammer this is a formalization (and improvement) of the "lsp config" concept. nvim-lspconfig will define its configs using vim.lsp.config: neovim/nvim-lspconfig#3494 , but also in your personal config you will find that vim.lsp.config + vim.lsp.enable saves you some boilerplate.

@lewis6991 lewis6991 force-pushed the feat/lspsetup branch 4 times, most recently from e753e41 to 04e98d9 Compare December 6, 2024 16:56
pcall(chunk)
end
end
pcall(vim.cmd.runtime, { ('lsp/%s.lua'):format(name), bang = true })
Copy link
Member

@clason clason Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also nvim__get_runtime (doesn't support globs or modelines yet, hence not used extensively so far).

@lewis6991 lewis6991 force-pushed the feat/lspsetup branch 2 times, most recently from 90eda71 to 6cbfca0 Compare December 9, 2024 11:57
@clason
Copy link
Member

clason commented Dec 9, 2024

MacOS failures are consistent, unfortunately.

@lewis6991
Copy link
Member Author

lewis6991 commented Dec 9, 2024

I'd be really surprised if it was because of this change since I've pretty much only added new functions.

EDIT: actually there is one line of code that might have caused the failures.

@clason
Copy link
Member

clason commented Dec 9, 2024

Yep, and macOS is special since that's not where we disable file watching.

Design goals/requirements:
- Default configuration of a server can be distributed across multiple sources.
  - And via RTP discovery.
- Default configuration can be specified for all servers.
- Configuration _can_ be project specific.

Solution:

- Two new API's:
  - `vim.lsp.config(name, cfg)`:
    - Used to define default configurations for servers of name.
    - Can be used like a table or called as a function.
    - Use `vim.lsp.confg('*', cfg)` to specify default config for all
      servers.
  - `vim.lsp.enable(name)`
    - Used to enable servers of name. Uses configuration defined
    via `vim.lsp.config()`.
@lewis6991
Copy link
Member Author

@mfussenegger can you just double-check your happy with the changes to client.lua and the tests removed in lsp_spec.lua.

TLDR is that we now always use lsp.protocol.make_client_capabilities() as a base for capabilities. The consequence of this is that we can no longer remove tables from the default capabilities.

If that isn't acceptable, then maybe we can add an extra_capabilities field to avoid users having to pre-populate capabilities just to add new ones.

@mfussenegger
Copy link
Member

TLDR is that we now always use lsp.protocol.make_client_capabilities() as a base for capabilities. The consequence of this is that we can no longer remove tables from the default capabilities.

Sounds okay to me.

@lewis6991
Copy link
Member Author

Friendly reminder that any new features merged into master should be considered unstable until they are included in an official release, which will be 0.11. Until then, the API introduced with this PR is subject to changes.

If there are any unresolved issues raised with the design, then we may prefix the API with _ to mark as unstable for the official release.

@lewis6991 lewis6991 merged commit 3f1d09b into neovim:master Dec 10, 2024
@neovim neovim locked as resolved and limited conversation to collaborators Dec 10, 2024
@lewis6991 lewis6991 deleted the feat/lspsetup branch December 27, 2024 09:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

Comments