Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LSP final candidate #11336

Merged
merged 308 commits into from Nov 13, 2019
Merged

LSP final candidate #11336

merged 308 commits into from Nov 13, 2019

Conversation

@norcalli
Copy link
Contributor

norcalli commented Nov 5, 2019

There are a few outstanding questions left, like:

  • What interface should we expose to the users?
  • How should we handle protocol extensions/custom protocols?
    For example, clangd supports passing offsetEncoding in capabilities, but this is a protocol extension, so the code must be done in lua if someone wants to pass it (this is optional, however. utf-16 works just fine).
vim.lsp.add_filetype_config {
  name = "clangd";
  filetype = {"c", "cpp"};
  cmd = "clangd -background-index";
  capabilities = {
    offsetEncoding = {"utf-8", "utf-16"};
  };
  on_init = vim.schedule_wrap(function(client, result)
    local offset_encoding = result.offsetEncoding
    if offset_encoding then
      print(offset_encoding)
      client.offset_encoding = offset_encoding
    end
  end)
}
  • When should I schedule_wrap things for the user? Is there a downside to always doing it? It seems like sometimes, the error reporting can suffer a little bit due to schedule_wrap.

Current viml example interface:

call lsp#set_log_level("debug")

call lsp#add_filetype_config({
  \ 'name': 'lua',
  \ 'filetype': ['lua'],
  \ 'cmd': ['/home/ashkan/works/3rd/lua-language-server/run.sh'],
  \ })

call lsp#add_filetype_config({
  \ 'name': 'js',
  \ 'filetype': ['javascript'],
  \ 'cmd': [$JAVASCRIPT_LANGUAGE_SERVER_DIRECTORY.'/lib/language-server-stdio.js'],
  \ })

call lsp#add_filetype_config({
  \ 'name': 'lua2',
  \ 'filetype': 'lua',
  \ 'cmd': './run.sh',
  \ 'cmd_cwd': '/home/ashkan/works/3rd/lua-language-server/',
  \ })

Each server must have a unique name to identify them. There's a way to define configs from viml, but lua may be necessary for advanced configuations.

If one didn't want to use lsp.add_config and wanted more granular control than filetype, they could use lsp.start_client and then lsp.attach_to_buffer.

I ended up rewriting about 90% of the existing PR. I tried to reduce the modules further, but I think this is as good as it gets. It could still be done, however, but putting protocol into util or lsp.

tjdevries and others added 30 commits Oct 17, 2018
Language Server Protocol defines two message types which are Request Message and Notification Message from client.
Now, built-in lsp client is implemented only Request Message.
So I impletent Notification Message.
I want to let lsp lib simplify at first.
So I remove pre and post autocmd hook until needed them.
…ntation
Now, we provide builtin callbacks instead of default callbacks.
So I remove default property from CallbakcObject
`table ~= {}` always returns false.
And I think is_empty function should be for table, so if is_empty receive another type instead of table, it throws error.
I think `next(table)` is better way that check whether table is empty or not.
…type callbacks.
I think client complex more than necessary.
So I remove autocmds module until we really need.
for i = 1, #line_diags - 1 do
table.insert(virt_texts, {"", severity_highlights[line_diags[i].severity]})
end
local last = line_diags[#line_diags]

This comment has been minimized.

Copy link
@bfredl

bfredl Nov 13, 2019

Member

is the spec guaranteeing that a diagnostic with highest severity comes last? (just wondering, we can improve this later)

This comment has been minimized.

Copy link
@norcalli

norcalli Nov 13, 2019

Author Contributor

Nope. Any delivery order, and I did consider the same thing. I'll slap a [TODO] here so we can Ctrl-F.

-- See https://github.com/palantir/python-language-server/commit/cfd6675bc10d5e8dbc50fc50f90e4a37b7178821#diff-f68667852a14e9f761f6ebf07ba02fc8 for an example of pyls handling both.
--]=]
elseif true or text_document_did_change == protocol.TextDocumentSyncKind.Full then
changes = full_changes(client)

This comment has been minimized.

Copy link
@bfredl

bfredl Nov 13, 2019

Member

TODO for later: we should throttle full buffer updates in some situation like global change.

end
end

local function update_tagstack()

This comment has been minimized.

Copy link
@bfredl

bfredl Nov 13, 2019

Member

TODO: this doesn't seem specific. Perhaps there should be a single-call API function to emulate tagstack?

This comment has been minimized.

Copy link
@norcalli

norcalli Nov 13, 2019

Author Contributor

That was a leftover from the last PR, so tbh I'm not exactly sure what it does at all.

This comment has been minimized.

Copy link
@justinmk

justinmk Nov 14, 2019

Member

Now that 'tagfunc' is available this might take the shape of #11280

return
end
for client_id in pairs(client_ids) do
-- This is unlikely to happen. Could only potentially happen in a race

This comment has been minimized.

Copy link
@bfredl

bfredl Nov 13, 2019

Member

TODO: reconsider this. I think it can happen if callback() invokes event processing which closes the client.

end
local _ = log.debug() and log.debug(log_prefix, "client.request", client_id, method, params, callback)
-- TODO keep these checks or just let it go anyway?
if (not client.resolved_capabilities.hover and method == 'textDocument/hover')

This comment has been minimized.

Copy link
@bfredl

bfredl Nov 13, 2019

Member

TODO: we should make this more systematic, like a map from capability names to method names or something.

@@ -0,0 +1,45 @@
function! lsp#add_filetype_config(config) abort

This comment has been minimized.

Copy link
@bfredl

bfredl Nov 13, 2019

Member

TODO: reconsider this layer (entire file) when we have v:lua.

@bfredl bfredl merged commit 00dc12c into neovim:master Nov 13, 2019
6 checks passed
6 checks passed
builds.sr.ht: freebsd.yml builds.sr.ht job completed successfully
Details
builds.sr.ht: openbsd.yml builds.sr.ht job completed successfully
Details
codecov/patch 86.09% of diff hit (target 77.61%)
Details
codecov/project 77.62% (+0.01%) compared to db436d5
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@norcalli norcalli deleted the norcalli:lsp-review branch Nov 13, 2019
@norcalli

This comment has been minimized.

Copy link
Contributor Author

norcalli commented Nov 13, 2019

For future travelers here, I'm trying to build an interface for common configurations so that we can have something similar to coc's servers except all in one place here https://github.com/norcalli/nvim-common-lsp. Help me get your favorite server to have an easy to use interface.

Also thanks to @h-michael for keeping the old PR alive, updating it, and answering my questions, and @tjdevries for the initial PR :) Without them, this wouldn't have been possible.

@h-michael

This comment has been minimized.

Copy link
Contributor

h-michael commented Nov 14, 2019

@norcalli When I got up in the morning, there was a present! Thank you for the great achievement.
@tjdevries This time came because you suggested, started the discussion, and started the implementation.
@neovim/core Thanks for adding and reviewing many important features needed for this.

@tbodt

This comment has been minimized.

Copy link

tbodt commented Nov 14, 2019

I have an alternate idea for configuration:

lsp#add_config('clangd', {'cmd': 'clangd'})
autocmd FileType c,cpp lsp#set_server('clangd')

This has the huge (to me) advantage of allowing you to define project-specific language server configurations, such as this in a .lvimrc:

lsp#add_config('special clangd', {'cmd': ['~/project/prebuilt/bin/clangd', '-log=verbose']})
autocmd FileType c,cpp lsp#set_server('special clangd')

Does this sound like a good idea? I'd be happy to write a PR for it.

@jrop

This comment has been minimized.

Copy link

jrop commented Nov 14, 2019

@tbodt

project-specific

This would be pretty sweet, actually

@norcalli

This comment has been minimized.

Copy link
Contributor Author

norcalli commented Nov 14, 2019

@tbodt I'm guessing your comment is in response to the add_filetype_config interface, which, I agree, I was never a fan of. It felt too low value for something that a user could easily do themselves with just a little bit of Lua, but I guess the thought was to provide something that anyone could use. The interface that I prefer is the start_client and buf_attach_client pair. With those, you can do everything you want.

So, for instance, your suggestion, while better than the add_filetype_config interface, could easily be achieved with:

clangd_base_config = {
  cmd = 'clangd';
}
clangd_clients = {}

vim.api.nvim_command [[autocmd FileType c,cpp lua clangd_clients[vim.lsp.start_client(vim.tbl_extend("error", clangd_base_config, { root_dir = vim.fn.expand("%:h") }))] = true]]

Or something literally anything. The point is that the amount of code required is very minimal if you use Lua, whereas I found all the vimL interfaces difficult to design for because they are limited.

However, as to your point of per-project configuration, if you look at the docs at https://github.com/norcalli/nvim-common-lsp#gopls, you'll see how you can do that without needing something like .lvimrc. You use the root_dir function to signal what is or isn't a new project based on its root directory. This is very similar to how coc.nvim does things, except that instead of using settings, I use a function, which will always be my preferred approach as it means that users can compose existing interfaces without us needing to add many options that do almost the same thing (like "rootPatterns" and "ignoreRootPatterns" in coc.nvim). Again, this isn't a vimL interface, but, to be fair, I'm a Lua expert, not a vimL one.

So I think I would remove all the "utility" interfaces I've added and defer to the Lua interface start_client/buf_attach_client as the default and allow others/the community to decide what a good vimL interface should be.

@tbodt

This comment has been minimized.

Copy link

tbodt commented Nov 14, 2019

Agreed that start_client/attach_buf_client can do all things. I'm not a big fan of Lua over VimL though, mainly because my entire vimrc is VimL and it feels weird to have two languages. I'll try and think of a good VimL interface for this. The root_dir interface from common-lsp is pretty sweet, maybe I'll come up with a VimL design based on that.

@justinmk justinmk added this to the 0.5 milestone Nov 14, 2019
@jonhoo

This comment has been minimized.

Copy link

jonhoo commented Nov 14, 2019

🎉

It would be really handy to have a "quick-start" guide for this feature. Just how to get set up with the basics working!

@mcepl

This comment has been minimized.

Copy link
Contributor

mcepl commented Nov 14, 2019

tada

It would be really handy to have a "quick-start" guide for this feature. Just how to get set up with the basics working!

:he lsp.txt has something, but yes this and other questions are being worked on #11389.

@andymass

This comment has been minimized.

Copy link

andymass commented Nov 14, 2019

Does this handle utf-16 properly?

@clason

This comment has been minimized.

Copy link
Contributor

clason commented Nov 14, 2019

@andymass not yet; there are still some issues with missing byte--char conversions, but it's on top of the list mentioned in the issue mcepl mentioned.

@norcalli

This comment has been minimized.

Copy link
Contributor Author

norcalli commented Nov 14, 2019

@andymass yeah, that's first on my agenda for today. It should be a transparent fix which makes any existing callbacks just work.

h-michael added a commit to h-michael/deoplete-lsp that referenced this pull request Nov 16, 2019
neovim/neovim#11336 is merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.