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

[RFC] Built-in LSP Support #6856

Closed
wants to merge 63 commits into from
Closed

[RFC] Built-in LSP Support #6856

wants to merge 63 commits into from

Conversation

tjdevries
Copy link
Contributor

@tjdevries tjdevries commented Jun 7, 2017

So, here's the very beginnings of LSP support in neovim.

It can currently, start a server, say that it has opened the file and request references from the server. It loads the references using setloclist.

Here's my vision (or at least a rough draft of it). It's too late for me right now to clean more of it up and I'm too excited not to finally at least put something as a WIP PR :)

Feedback welcome and appreciated.

Goals:

Registering callbacks

Should be something along the lines of

function lsp#request(request_name, arguments, use_default_callback, optional_callback)

as well as a lua function that has the same signature.

Where:

  • request_name
    • The name of the request that you want, i.e. 'textDocument/references'
    • This is specified in microsofts protocol.
  • arguments
    • If you want to hardcode in a value, pass in the values that you would like, the rest will be filled in with the default values
    • These default values would be grabbed programmatically, i.e. for Position, we would use line('.') and col('.')
    • This allows the functions to be tested or used outside of the LSP context
      • For example if you want to highlight all the references of something, but it's not the item under the cursor, you could pass in the location of that item to the function
  • use_default_callback
    • A boolean value to determine whether or not to use the default callback value
  • optional_callback
    • We will provide a default callback that will handle anything within the TUI
    • This will also use built-in neovim items (such as popupmnu, loclist, quickfix, etc.) so that if those are externalized by a GUI, they don't even have to do anything special.
    • Behavior could be to simply override the callback or to add it to a list of callbacks to call
      • Might want to use a list so that multiple hosts/clients of neovim could grab onto these events.
      • Probably don't want neovim to process the events though if the host/client override.
      • I'd just start with overriding to start with, since it is simple.

I imagine optional callbacks essentially allowing an even more extensive API, one that doesn't require Neovim explicit knowledge. So, to embed neovim in another editor, you could try and do lots of the information transfer with LSP callbacks.

From nvim
:call lsp#request('textDocument/references', [], v:false, custom_callback)

From remote
nvim.call_function('lsp#request', ['textDocument/references', [], false, custom_callback])

Creating default callbacks

function lsp#handle_response(request_name, response)
-> function lsp#response#textDocument.references(response)
-> function lsp#response#textDocument.hover(response)

Where:

  • response:
    • The response as specified by the protocol.
    • These should be made available so that people can use these as well to do more complicated tasks easily.
    • For example, could use the hover callback to create a simple hover pane that other hosts/clients/GUIs would understand.

@bfredl
Copy link
Member

bfredl commented Jun 7, 2017

Nice! I hope to improve jobs/channels in lua #6844 (comment) to the extent this can just use them and don't need to use libuv directly.

@tjdevries
Copy link
Contributor Author

@bfredl that sounds great! That would certainly ease some of the configuration for the client. I will try and watch out for that.

function! s:get_client() abort
if g:nvim_lsp_client == -1
lua << EOF
local client = require('runtime.lua.lsp.client')
Copy link
Contributor

Choose a reason for hiding this comment

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

This dedented block looks ugly, do not use <<EOF. Everything which does not fit into one line (lua require('lsp.client').get_client(), or, better, just return luaeval("require('lsp.client').get_client()") (move :if to lua to not split the logic)) should be in a required function.

Also should be require('lsp.client'), I do not know why #6789 still is not merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I have removed many of them and rebased, so I can use the updates from #6789 . I will clean up the code as I go along. Still brainstorming a lot of ideas.


function! lsp#request#textdocument_references()
lua << EOF
print('TODO')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

@@ -0,0 +1,383 @@
--
-- json.lua
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just use vim.api.nvim_call_function('json_…'? There already is a JSON parser in project, do not need to add a second one. If some features are missing they can be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to use the API to do it, since some of the json parsing might be done in a different thread I think. Unless it can work in another thread?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are using luv to fork out different Neovim threads? I do not think this is a good idea, most of the code does not expect this and making sure that different shared resources (memory, file descriptors except for a small subset, etc) are not accessed through spawned threads is close to impossible. Generally: until there are Neovim own functions for creating threads or fork()ing don’t do this in core plugins, it risks producing nasty heizenbugs.


As to the question, though not exactly API, but JSON decoder could be easily edited to work with lua natively: most of code is parsing there, just need to abstract away creation of values (I would personally go with creating jsondecode.c.h macros-parametrized file for this purpose, should also be possible to do abstraction via struct with function references). Encoder is a separate issue, the “easy” way is to not touch it at all, but create duplicate lua -> VimL conversion functions which are exactly like existing ones, but do not put allocated lists and dictionaries into M&S GC double-linked list. In both cases you will spend more time creating the tests then adjusting the code.

Copy link
Member

Choose a reason for hiding this comment

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

Luv won't spawn threads, but it can run callbacks at times which are not considered vimL-safe, though as long as GC won't be invoked in such a time (potentially, rooting might be incomplete), json_decode shouldn't be a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bfredl Container allocation functions are not reentrant because they alter a linked list in a number of steps. So it is not safe to call callbacks at random times even without GC run (how, BTW? I do not know how uv is going to perform async read at unsafe time without either using SIGALARM (should probably mess up with our timers), or a separate thread, or a separate process), unless you edit C code of the parser to work with lua containers.

Why not use existing jobs in any case, this is going to be safe for sure? Just have a lambda which immediately calls lua.

Copy link
Member

Choose a reason for hiding this comment

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

Not random times, just times the event loop is invoked. As previously this could be done in a way that queues all vimL until later, such spots could potentially be vimL GC unsafe. Can container allocation invoke GC or does that only happen at states?

The plan is indeed to improve jobs so they can be used here.


" TODO: Automatically open file if not opened already
function! LSP_References() abort
lua << EOF
Copy link
Contributor

Choose a reason for hiding this comment

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

lua lsp.client_references(), indented:

  1. Do not use ugly <<EOF blocks.
  2. All globals must be namespaced. This is not the only plugin using lua. I personally would prefer a single global per plugin or no globals at all (use lua require("…").func()).

Copy link
Contributor

Choose a reason for hiding this comment

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

Just like on #6789, I would say that it is vastly preferable if we put some of these finer points in the help docs, where people are likely to find them.

endfunction

function! LSP_DidOpen() abort
lua << EOF
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

" TODO(tjdevries): Make sure this works correctly
" TODO(tjdevries): Figure out how to call a passed callback
function! LSP_Request(request, params, callback) abort
return luaeval('client_request(_A.request, _A.args)', {request: request, args: params})
Copy link
Contributor

Choose a reason for hiding this comment

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

Same (globals).

@@ -0,0 +1,28 @@
" TODO(tjdevries): Move this to autoload?

execute('luafile ' . expand('<sfile>:h') . '/../lua/lsp/plugin.lua')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be something like lua lsp = require('lsp.plugin').setup(). (Needs #6789 or you adding a path to package.path.)

@@ -0,0 +1,38 @@
local helpers = require('test.functional.helpers')(after_each)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this should be test/functional/plugin/lsp/…: other runtime files are tested in plugin unless these are runtime files used from core in a special fashion (like providers which have corresponding C code). Applies to all test files here.

vim = vim or {}

util.echom = function(message)
vim.api.nvim_command('echom "' .. tostring(message) .. '"')
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove this function and use print(). And you have escaping issues here in any case.


function client:_on_error(level, err_message)
if type(level) ~= 'number' then
util.echom('we seem to have a not number' .. util.tostring(level))
Copy link
Contributor

Choose a reason for hiding this comment

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

Message should start with a capital letter.


execute('luafile ' . expand('<sfile>:h') . '/../lua/lsp/plugin.lua')

function! LSP_Start() abort
Copy link
Contributor

@wsdjeg wsdjeg Jun 8, 2017

Choose a reason for hiding this comment

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

@tjdevries hi, I think maybe some people like autoload function more. for example

function! lsp#start() abort

and I have read most of vim's runtime file, the file in runtime/plugin/ do not definde func with capital letter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed them to autoload. Thanks :)

@marvim marvim added the WIP label Jun 18, 2017
@@ -0,0 +1,304 @@
local uv = require('luv')
Copy link
Contributor

Choose a reason for hiding this comment

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

Is luv already included by default?

For me, inside of nvim:

:lua print(package.searchpath('luv', package.cpath))
/home/aktau/github/neovim/neovim/.deps/usr/lib/lua/5.1/luv.so
:lua print(package.cpath)
./?.so;/usr/local/lib/lua/5.1/?.so;/home/aktau/github/neovim/neovim/.deps/usr/lib/lua/5.1/?.so;/usr/local/lib/
lua/5.1/loadall.so

Or:

$ nvim -u NONE +'lua io.stderr:write(package.cpath)' +'lua os.exit()'
./?.so;/usr/local/lib/lua/5.1/?.so;/home/aktau/github/neovim/neovim/.deps/usr/lib/lua/5.1/?.so;/usr/local/lib/lua/5.1/loadall.so%                                                                                                            

It appears my build folder is in my package.cpath, this is the default package path from the LuaJIT compiled into nvim, but it can be overridden if the user sets their own path. This should be made much more robust. It is also not guaranteed that a user has the neovim .deps folder on their machine. We should attempt to not rely on optional libraries (especially those that need to be compiled, like luv).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hoping to not have to use luv soon. Once we get the jobs update (which I assume will happen before I finish this :) ) then I will use nvim's included job functionality, rather than a hacked together interpretation.

@tjdevries
Copy link
Contributor Author

@ZyX-I Is there a preferred way to get rid of the luacheck errors about referencing "global vim" without declaring it?

Should I just set local vim = vim or {}? Or change my luacheck config?

@justinmk
Copy link
Member

Instead of callbacks why not publish RPC notifications?

@bfredl
Copy link
Member

bfredl commented Jun 28, 2017

Then we need to be able to subscribe to them from vimL and Lua. Which we probably want anyway.

@ZyX-I
Copy link
Contributor

ZyX-I commented Jun 28, 2017

@tjdevries Tell luacheck that there is global vim. CMakeLists.txt already does just that for vim.lua.

@tjdevries
Copy link
Contributor Author

tjdevries commented Jun 28, 2017

@justinmk I think RPC notifications would work as well. Originally I was thinking of callbacks because I thought GUIs/plugins/etc. might want to add their own callback or remove the default callback to extend behavior. But I think RPC would work well for that too.

I never got around to implementing an async rpcrequest though. So whenever I send an LSP request, it would block, correct? Or are you saying after a request message is sent back, do rpcnotify(0, 'lsp/textDocument/references', ...)?

@justinmk
Copy link
Member

If the callbacks aren't needed then my suggestion is moot. My suggestion is only relevant under that assumption.

@euclio
Copy link
Contributor

euclio commented Aug 16, 2017

What's the status of this?

@tjdevries
Copy link
Contributor Author

I'm waiting on a few PRs to get merged to ease development of this.

I'm also working on a few different projects right now, so I haven't had a lot of time to work on this. I would recommend using one of the other plugins that provides the functionality for now.

I'm still planning on finishing this though :)

@balta2ar
Copy link

I'm waiting on a few PRs to get merged to ease development of this

For those who are curious, could you name them, please?

@tjdevries
Copy link
Contributor Author

There's some work being planned for job_control to be added into Lua, being able to cancel stuck loops in lua, and perhaps more lua integration with the vim.api module.

Primarily the job_control items, since that could effect the structure of the support within neovim.

@justinmk justinmk added the lsp label Aug 22, 2017
@jvican
Copy link

jvican commented Sep 28, 2017

Could we set up a BountySource for this to motivate @tjdevries get this through the finish line? I would be happy to donate some money to have native LSP support in neovim.

@blueyed
Copy link
Contributor

blueyed commented Sep 28, 2017

How does it compare to https://github.com/autozimu/LanguageClient-neovim?
(listed on http://langserver.org/#implementations-client also)

@tjdevries
Copy link
Contributor Author

Hi everyone 😄

Sorry for the long pause in dev, been working on some personal projects and practicing lua and lua within nvim.

@blueyed It currently compares to the language client as not as many features and its broken 😄. The idea however is that if this ships with neovim, people will be able to contribute more effectively, since it will be an "official" implementation. Also, it won't require any dependencies (so you won't have to install python and set up neovim-python) as the Lua will all run with whatever neovim ships with. I have lots of other ideas, but I should really finish the basic implementation first :)

I imagine there will still be other plugins around that integrate the core features built here to interact with other plugins. I can imagine sources for deoplete that use the builtin lsp#request(...) to get completion options. Maybe we will also implement some async api calls to interface with lsp as well. As I said, lots of ideas.

@jvican Not sure how that would work. If you're interested in using LSP in the mean-time, I would really recommend the implementation @blueyed linked. That way you could use most of the up-side of LSP and not have to spend money :)

@KillTheMule
Copy link
Contributor

I'm currently using https://github.com/autozimu/LanguageClient-neovim with the RLS. If you're at a point where you'd want someone to test this, let me know :)

@tjdevries
Copy link
Contributor Author

@KillTheMule I'll let you know :D Thanks for the offer.

@@ -0,0 +1,117 @@
local lsp_util = require('runtime.lua.lsp.util')
Copy link
Member

Choose a reason for hiding this comment

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

drive-by comment: runtime.lua.lsp.util looks too nested, possibly a sign of over-architecture. Hyper-categorization is bad enough, but sub-categories and sub-sub-categories are rarely needed.

I think we already have too many subdirectories in Nvim source. We should try to keep things relatively flat.

Copy link
Member

@justinmk justinmk Dec 28, 2017

Choose a reason for hiding this comment

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

In fact the LSP support should be one big file, with truly common utilities in one separate file (call it foo.lua, doesn't matter: again, one big file, we can nitpick over naming/placement later). "Common" means "can be used by any other Lua code, not just LSP".

Copy link
Member

Choose a reason for hiding this comment

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

The most important thing to avoid is shared state. The line count of a file doesn't mean anything as long as there is minimal or no shared global or pseudo-global state.

Copy link
Member

@bfredl bfredl Dec 28, 2017

Choose a reason for hiding this comment

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

I think it looks too nested only because it's wrong: it should be require('lsp.util'). That wouldn't seem too bad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it should just be lsp.util, but I couldn't get the tests working at first without the runtime.lua, so I'll fix that soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am looking to try and consolidate as much as possible :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@justinmk Should be fixed now. It's just lsp.util or lsp.callbacks, just like any other lua plugin.

@andreypopp
Copy link
Contributor

Maybe neovim could only expose Lua API for LSP? Seeing how much effort is being put in user space to support LSP it would be unfair to discard it (and especially given that such effort has value both for vim and neovim users). That way neovim would provide backing API on which underlying user space implementations could rely when run on neovim.

@justinmk
Copy link
Member

justinmk commented Jan 23, 2019

vim-lsp also uses autoload/lsp.vim

Maybe neovim could only expose Lua API for LSP?

Somewhere in the comment cloud above, we discussed minimizing and, ideally, eliminating the VimL wrapper for this, so lsp.vim definitely isn't a hard requirement. It can be renamed for now, and revisited later.

@mcepl
Copy link
Contributor

mcepl commented Jan 23, 2019

Hi. The branch uses autoload/lsp.vim as for Vim script API.
But it conflicts with vim-lsp.

I understand, you were first, but if this gets built-in to Neovim, then I am afraid it should get a precedence, shouldn't it?

@bfredl
Copy link
Member

bfredl commented Jan 23, 2019

@mcepl but as said above this will be trivially solved by renaming the file.

Alternatively, as I might already have said in the hidden 170 comments, we should change the init.vim definitions to be in the form let g:lsp_some_good_var_name = { server definitons }, so no autoload calls will be needed from vimL.

@mcepl
Copy link
Contributor

mcepl commented Jan 23, 2019

@bfredl certainly, and it is not a big deal. Actually, eliminating autoload seems to me like a good idea.

@mcepl
Copy link
Contributor

mcepl commented Feb 15, 2019

It is extremely easy to add support for renaming of symbols (thank you!):

function! LSPRename()
    let s:newName = input('Enter new name: ', expand('<cword>'))
    call lsp#request_async('textDocument/rename',
        \ {'newName': s:newName})
endfunction

au FileType lua,sh,c,python
    \ noremap <silent> <buffer> <F2> :call LSPRename()<CR>

but there is a bug somewhere. Whenever I run this command, I get one more EOL in the end of the file.

@Piping

This comment has been minimized.

@KillTheMule
Copy link
Contributor

@tjdevries I've made nvim into a fake languageserver and used it for a test here. I'm planing to extend this a bit, seems to be a good way to test this.

Generally, though, what's you're plan, do you think you'll find time to work on this? I offer my help, up to the point of bringing it over the finish line.

@Piping

This comment has been minimized.

@mcepl

This comment has been minimized.

@Piping

This comment has been minimized.

@tjdevries
Copy link
Contributor Author

@KillTheMule I have my last day of work at my current company on Friday and then have a week off after that. If I don't make any progress during that week, it'd be great if you could take this PR to the finish line for me. I'd be happy to hop on gitter more often if you need to bounce ideas around or try and figure out what I was doing during parts of the PR.

Thanks for following up and offering your help.

@KillTheMule
Copy link
Contributor

That's good to hear. Let me throw out some questions that occured to me during writing the integration test. Feel free to discard any of my thoughts, but I think I should at least mention them :) Talking on gitter would be fine, too :)

  1. Right now, you're sending Notifications as Requests. The spec has them different, in that at Notification does not have an id field, and needs no response. Are you planning to make a difference here? That would also make my life easier for the integration test :D From reports, things seem to be working, so servers don't seem to mind, but I guess going with the spec would be a good thing.
  2. Sync requests don't return the response, but use callbacks. I found that pretty unintuitive, and I think it brings quite a bit of procedure to clients using this code. Wouldn't it make sense to simply return the response? Async requests would of course still need a callback...
  3. Sync requests are async requests with a (busy?) waiting loop. You're waiting 100ms in each step... isn't that much too much?
  4. From my pov, this PR provides two things: An lsp-lib that can be used for clients, and a simple client. Would it make sense to separate those more? I found it a bit surprising that potential other clients have to unregister the default handlers, for example. Also, I see a lot of code that already handles things in a specific way, which befits a client, but not a lib. Not sure how important that is, but I'd separate those things :)
  5. I did not yet dive into that, but generally I found that user autocommands are inadequate because they can't carry arguments, i.e. you can't distinguish from which buffer they originated. Might that pose a problem when someone has several clients running? Or do you have a way to transport that information?
  6. Not a question, but please consider doing this.

@mcepl
Copy link
Contributor

mcepl commented Jun 9, 2019

@tjdevries @KillTheMule ping? This LSP client is accused of being a cause of #10167.

@KillTheMule
Copy link
Contributor

Hey, my latest info was that tj was working on a larger refactoring, but I haven't heard from him since. I'm pressed for time these days, so I did not push this.

@h-michael
Copy link
Contributor

h-michael commented Jun 11, 2019

@KillTheMule @tjdevries Can I take over this PR?

@bfredl
Copy link
Member

bfredl commented Jun 11, 2019

@h-michael Sure, open a new PR with your changes.

@KillTheMule
Copy link
Contributor

@h-michael Sure, open a new PR with your changes.

Definitely. I'll keep track and try to help out, let me know if there's something particular you'd like me to do. In particular, I could extend my "nvim as a fake language server" approach for testing :)

@h-michael h-michael mentioned this pull request Jun 14, 2019
20 tasks
@h-michael
Copy link
Contributor

I open the PR #10222. And I would like to discuss where to set the fish line. :)

@mcepl
Copy link
Contributor

mcepl commented Sep 16, 2019

Shouldn’t this PR be closed? The story is no longer here.

@justinmk justinmk closed this Sep 22, 2019
@neovim neovim locked as resolved and limited conversation to collaborators Sep 22, 2019
@justinmk
Copy link
Member

Resolution/Summary

Continued at #10222 .


Locked to keep the summary visible. You can always chat or open a ticket if you have new information/topics to discuss.

@dundargoc dundargoc removed the request for review from ZyX-I October 4, 2022 11:58
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.

None yet