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

Feature request: add general support for the Language Server Protocol #517

Closed
Firehed opened this issue Apr 28, 2017 · 56 comments
Closed

Feature request: add general support for the Language Server Protocol #517

Firehed opened this issue Apr 28, 2017 · 56 comments

Comments

@Firehed
Copy link
Contributor

Firehed commented Apr 28, 2017

Many languages are starting to add support for the Language Server Protocol, which supports (among other things) getting diagnostic errors on files from the language server. I believe ale could leverage this to get pretty reusable support across many languages, and add support for new languages in the future with relatively little effort.

@w0rp
Copy link
Member

w0rp commented Apr 29, 2017

Yep, I had planned on doing that for TypeScript, perhaps. Supporting this probably won't be easy, but it'll be worth doing.

My short term plan for TypeScript is to add a linter which grabs results form tsuquyomi, when it's installed, which implements parts of the LSP for TypeScript.

@w0rp
Copy link
Member

w0rp commented Apr 29, 2017

https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md I recommend reading this, for anyone interested in implementing an LSP client. It took me a while to track that page down. That's the best specification I can find.

@Firehed
Copy link
Contributor Author

Firehed commented May 1, 2017

Just FYI, TypeScript (well, tsserver) doesn't appear to actually implement the protocol - although the implementation mechanics will likely be extremely similar.

Broadly, I think the process will look like:

  • Spawn server, if one is not already running (this should be the only language-specific thing)
  • client/registerCapability
  • Send textDocument/didOpen when a document opens
  • Send textDocument/didChange with the full buffer contents when a change occurs (it can be done as a series of diffs by the look of it, but that sounds very painful)
  • Send textDocument/didSave on save (may need only the above or this, depending on the user's ale settings)
  • Listen for textDocument/publishDiagnostics, and display any errors it indicates
  • Cleanup work as needed (didClose, stop server, etc)

I'm certainly not an expert on the protocol, but I think that's all that is necessary since a lot of the protocol is for refactoring, IDE hints, etc. Easier said than done, of course, but it may not be too horrible

@w0rp
Copy link
Member

w0rp commented Jun 8, 2017

I have created the branch tsserver which reports errors using tsserver. The API for tsserver is different from LSP servers, but parts of the logic are roughly the same. I'll experiment with it a bit before merging it into master.

Now I think it would be a good idea to try out a real LSP server for some other language.

@w0rp
Copy link
Member

w0rp commented Jun 13, 2017

I fixed the way the handling of diagnostic responses figured out which errors are for which buffers for TSServer and merged that into master now. Now others can try it out and I can fix whatever bugs come up.

@w0rp w0rp modified the milestones: Version 1.5, Version 1.4 Jun 14, 2017
@w0rp
Copy link
Member

w0rp commented Jun 14, 2017

I'm moving the milestone for actual language server support, as none of the servers listed in the table support diagnostics yet, which is what we really care about. The table is here: http://langserver.org/

@euclio
Copy link

euclio commented Jun 14, 2017 via email

@w0rp
Copy link
Member

w0rp commented Jun 14, 2017

Aha, thanks for the information. In that case, we can integrate with the Rust LSP server for 1.4.

@w0rp w0rp modified the milestones: Version 1.4, Version 1.5 Jun 14, 2017
@w0rp w0rp removed this from the Version 1.4 milestone Jun 27, 2017
@w0rp
Copy link
Member

w0rp commented Jun 27, 2017

I'm removing the release milestone. If someone can suggest a language server which works well and which doesn't require some complicated installation process, I can work on this.

@euclio
Copy link

euclio commented Jun 27, 2017

@w0rp did you have trouble installing the RLS?

@w0rp
Copy link
Member

w0rp commented Jun 27, 2017

Yeah, I don't know how Rust stuff works.

@w0rp
Copy link
Member

w0rp commented Jun 27, 2017

If I can write sudo apt install rls, then I'll work with it.

@euclio
Copy link

euclio commented Jun 27, 2017

Not a one-liner, but

curl https://sh.rustup.rs -sSf | sh
rustup update nightly
rustup component add rls --toolchain nightly
rustup component add rust-analysis --toolchain nightly
rustup component add rust-src --toolchain nightly

and

$ rls +nightly

should get you there. You also will need to set up a cargo project to actually use the server.

You could also try the Typescript or Go language servers from https://github.com/sourcegraph/, but I haven't used them personally.

@Firehed
Copy link
Contributor Author

Firehed commented Jun 27, 2017

@w0rp I'm experimenting with the PHP language server (I honestly have no idea how well it works) which I'd be happy to contribute if I can get the thing going. Shouldn't have a very complex installation process (it's just a composer require in an existing project, if you're familiar with the modern php tools).

However, after playing around a bit (read: starting with tsserver.vim and changing some names and paths) I get E605: Exception not caught: executable and executable_callback cannot be used when lsp == 'lsp'. Clearly the "start server" command requirements are different for TSServer than for other LSP (reasonable, given TSServer's not-quite-the-sameness), but the thing that actually starts the server seems to require either of those to be set. Is the implication here that ale expects the language server to already be running? If not, what's the correct way to configure the linter?

The relevant part of my (failing) linter definition is as follows:

call ale#linter#Define('php', {
\   'name': 'langserver',
\   'lsp': 'lsp',
\   'executable_callback': 'ale_linters#php#langserver#GetExecutable',
\   'callback': 'ale_linters#php#langserver#Handle',
\})

@w0rp
Copy link
Member

w0rp commented Jun 27, 2017

Support for language servers hasn't been fully implemented yet. The only thing that has is tsserver, which works differently.

I realised more recently that both connections to network sockets and reading and writing to and from stdio and stdout will need to be supported, as the protocol doesn't specify how to actually connect to the servers, so that part is basically a free-for-all.

@w0rp
Copy link
Member

w0rp commented Aug 1, 2017

Sending didChange many times is okay. It's better to fire it more often than needed, than less than needed. As with tsserver, I just say that the entire document has changed every time error-checking is done. That's also the way that diagnostics are requested. For tsserver, there's a distinct message for asking for diagnostics, but for LSP the diagnostics are returned in response to a document changing. So the easiest way to get diagnostics again is to just say that the document has changed.

@w0rp
Copy link
Member

w0rp commented Aug 1, 2017

I may or may not have fixed that issue with getting the original diagnostics back again. I was sending the initialize message more than once in some cases, and that might have been the cause of that issue. I'll try it out later.

@Firehed
Copy link
Contributor Author

Firehed commented Aug 1, 2017

Played around with the Rust LS a bit today as well (as much as I can for how infrequently I use the language). Managed to break the thing to where it was spewing a bunch of stuff to stderr, which I think may be closed to moving files around underneath it (I don't think moving lib.rs to main.rs is generally accepted as a good idea).

That aside, it worked well, quickly, and with no configuration once I actually had RLS installed (ignoring telling Ale to use it). Far more useful than the PHP one. From a practical standpoint, I wish the -- INSERT -- didn't override Ale's message, but I can probably screw around with my prompt to fix that.

I think sending textDocument/didClose may help somewhat, but I'm also not sure what's the best way to handle underlying files being removed from the buffer (e.g. :!mv % newdest.ext or :!rm %) - I'd think a didClose and don't send any messages to the server if a buffer doesn't have an underlying file. I think most IDEs avoid this problem entirely by always operating on an actual file.

@w0rp
Copy link
Member

w0rp commented Aug 1, 2017

set noshowmode will disable the -- INSERT -- message. I set that myself in vimrc, and then I show the mode with a single character in my statusline with lightline.

@w0rp
Copy link
Member

w0rp commented Aug 1, 2017

I think the textDocument/didClose message can be implemented without too much trouble. A function call can be added to the function for cleaning up when a buffer is removed which will send the textDoument/didClose message, and remove the buffer from the List of open documents, so the textDocument/didOpen message can be sent later then the file is opened again.

@Firehed
Copy link
Contributor Author

Firehed commented Aug 1, 2017

Sounds good. I don't know the exact semantics of file vs buffer especially with multiple splits or tabs, but as long as it only fires didClose when the last split for a file closes, I think that will work.

@w0rp
Copy link
Member

w0rp commented Aug 1, 2017

I can explain that. Splitting the window shows the same buffer in two windows, and tabs contain windows. ALE cleans up a buffer before it is unloaded with BufUnload, which only happens when it's no longer being used anywhere.

rsrchboy added a commit to rsrchboy/ale that referenced this issue Aug 2, 2017
* upstream/master:
  Cover the SaveEvent function with a test
  dense-analysis#734 - Use the buffer number from the events for entering buffers and saving buffers for checking buffers
  dense-analysis#734 - Do not clear file linter results when no buffers are run
  Add stylelint fixer
  Cover special LSP initialize response handling with Vader tests
  dense-analysis#517 - Get the Rust language server working in a basic way
  When servers never send an initialize response, but instead just publish diagnostics straight away, handle that as an initialize response
  Add some error message handling for LSP, for test purposes
  Fix some bugs so the PHP language server will show errors at least once
@w0rp
Copy link
Member

w0rp commented Aug 2, 2017

The PHP language server seems to work pretty well now. It looks like the problem was sending the initialize message twice. I'll enable it now, and start documenting everything so far.

w0rp added a commit that referenced this issue Aug 2, 2017
@w0rp
Copy link
Member

w0rp commented Aug 2, 2017

I just pushed a commit which renames the Rust LSP linter to 'rls', so update your configuration appropriately. LSP linters had to be all named 'langserver', but now they can be named anything. I have covered both sets of the LSP linter callbacks with a few Vader tests now.

rsrchboy added a commit to rsrchboy/ale that referenced this issue Aug 3, 2017
* upstream/master:
  Cover the Rust LSP with tests, allow LSP linters to be named anything, and rename the Rust LSP linter to `rls`
  Use g: for the PHP Vader tests
  Cover the PHP language server functions with Vader tests
  dense-analysis#517 Enable the PHP language server
  Fix indentation and a typo in the gometalinter documentation
  Rubocop: Show cop name
  added gometalinter docs - Fix for dense-analysis#816
@prabirshrestha
Copy link

I'm also interested in getting the language server protocol working with ale.

Currently vim-lsp is still work in progress so need to use dev branch.

Plug 'prabirshrestha/asyncomplete.vim'
Plug 'prabirshrestha/async.vim'
Plug 'prabirshrestha/vim-lsp', { 'branch': 'dev' }
Plug 'prabirshrestha/asyncomplete-lsp.vim'

if executable('pyls')
    " pip install python-language-server
    au User lsp_setup call lsp#register_server({
        \ 'name': 'pyls',
        \ 'cmd': {server_info->['pyls']},
        \ 'whitelist': ['python'],
        \ })
endif

If ale has public apis it can listen to all the LSP notifications from the language servers and send it to ale using lsp#register_notifications. This allows vim-lsp to control all the server process as well as proper didOpen, didChange, didClose events which means only one instance of the server.

function! s:on_notification(server_name, data) abort
    echom a:server_name . json_encode(a:data['response'])
    " use ale apis and add diagnostics message if a:data is not error and is of type diagnostics
endfunction

au User lsp_setup call lsp#register_notifications('ale', function('s:on_notification'))

@w0rp
Copy link
Member

w0rp commented Aug 6, 2017

I don't think depending on other plugins will work. LSP is too new to write stable client for it, and you can't specify dependences for Vim projects like you can with Node or Python projects. The torrent of pull requests would slow down development, and minor changes to the plugin would result in the integration breaking often.

@OliverUv
Copy link

OliverUv commented Aug 7, 2017

You could potentially use it as a library, included via git submodules. I believe this is how https://github.com/vim-jp/vital.vim is used

@w0rp
Copy link
Member

w0rp commented Aug 7, 2017

The LSP implementation in ALE is much further along already, and PHP and Rust support are ready to release.

@w0rp
Copy link
Member

w0rp commented Aug 7, 2017

git submodules don't really work as a solution for adding plugin dependencies. You can't have two different versions of the same plugin with the same autoload function names, so you'd have to never have two different plugins with the same dependencies. You can't wrap functions in a namespace either, as the full autoload function name is written into the name of every function.

If you want to depend on other plugins, then they have to be installed alongside your plugin, and all other plugins have to share the same version of that other plugin. That puts controlling the version you depend on beyond your control.

Vim's plugin ecosystem is very much inferior to something like NPM.

@w0rp
Copy link
Member

w0rp commented Aug 7, 2017

I'll close this now, as basic LSP integration is done. We can worry about TCP connections later, as there's currently no way to make TCP connections natively in stable versions of NeoVim that I know of, but there is in Vim 8.

I don't think closing and re-opening documents is essential at the moment. I'll open an issue for handling that. It probably won't be incredibly difficult to implement.

@w0rp w0rp closed this as completed Aug 7, 2017
rsrchboy added a commit to rsrchboy/ale that referenced this issue Aug 9, 2017
* upstream/master:
  Fix dense-analysis#468 - Add a cool down period for when things go wrong
  Document the extra optional argument for ale#Queue
  Simplify some comparisons
  Ban use of ==# or ==? in the codebase, and prefer is# or is? instead
  Fix dense-analysis#833 - Do not open windows on save when the option is off
  Fix dense-analysis#271 - Add the ability to open the quickfix or loclist windows only after saving a file
  Fix the resetting of selections and annotate it
  added missing visual reselection after quick/location list update (dense-analysis#788)
  dense-analysis#517 - Document arguments for defining LSP linters
  Document the PHP langserver integration
  Document the rls linter
  The default for ale_rust_cargo_use_check was wrong in the documentation
  Fix dense-analysis#823 - Write Windows files with CRLF
  Stop writing a test file in real world usage in some cases
@nickspoons
Copy link

This is looking great. I'm still wondering though, as jez asked in an earlier comment, what the ultimate goal is? Is ALE eventually going to support navigating to symbol definitions and listing references?

If not, (and this was also mentioned by prabirshrestha above), there will always be a need for some other LSP plugin to be running its own version of the server alongside ALE, won't there?

For example, I am currently using the ALE tsserver linter together with tsuquyomi for typescript, which works great but does mean that there are 2 instances of the server running. I also hope to use the OmniSharp LSP server for C# when it is ready, and that will result in the same situation.

So is it not an option to open an ALE API, allowing other plugins to pass in their diagnostics? That would allow the external plugin (whether it was a generic LSP plugin or a specific plugin like tsuquyomi) to maintain their servers, and just let ALE display the results - the external plugin would then depend on ALE, not the other way around. I think (?) this is what @prabirshrestha was getting at.

@w0rp
Copy link
Member

w0rp commented Aug 19, 2017

Vim plugins for new protocols like LSP cannot work together in such a manner, because you can't configure requirements for dependencies like you can with Python's PIP. I think any attempt to get one plugin to use another plugin's LSP client will result in failure.

Until LSP has been seriously adopted in many languages, the specification will continue to evolve, and the clients will evolve with the specification. If the specification eventually becomes stable enough that it will be possible to develop clients without breaking API changes, then it will be possible for two plugins to reliably work together in this manner.

Until it becomes possible to have one plugin depend on another, each plugin will have to connect to an LSP separately. This can be undesirable, so ALE is forced to implement other features like completion.

@w0rp
Copy link
Member

w0rp commented Aug 19, 2017

The best way to get some perspective on this is to try and implement something like this, and then discuss it after having tried to implement it.

@nickspoons
Copy link

I suppose I was thinking that ALE could define the diagnostic format it expects and let any external plugins conform to that - the external plugin is then responsible for any formatting that might be required from the LSP server to make it fit to ALE's needs. This way ALE still doesn't need to know or worry about the plugin, so no dependency issues from this end.

I had imagined this being something like a matter of calling ale#linter#define(...) but passing in a callback method to the external plugin instead of an executable.

However, as you quite fairly note, I haven't tried implementing something like this and I don't understand all of the implications.

@prabirshrestha
Copy link

While the LSP protocol itself has been stable and is versioned to avoid breaking changes, many of the server implementations are not stable. But until and unless we don't push for it I don't think it will ever be. People will continue to use standalone linters since it works the best for them.

I was sort of thinking an api like this would be good. I have not read the ale code nor written linters so not sure if this would suffice. add takes in a list of messages and any one include the ale core could take advantage of it. It may also have to be extended to have custom user data so that we can perform code actions.

ale#messages#clear({ 'buf': bufnr('%') })

ale#messages#add([{
    \ 'buf': bufnr('%'),
    \ 'severity': 'error',
    \ 'message': 'message',
    \ 'range': {
    \   'start': { 'line': 1, 'col': 1 },
    \   'end': { 'line': 1, 'col': 10 },
    \ },
    \ }])

Here is an example how I implemented autocomplete of lsp. https://github.com/prabirshrestha/asyncomplete-lsp.vim/blob/cc168c5a0873dc902d9e1f55032e776a2fcd8d69/plugin/asyncomplete.vim. vim-lsp was designed to not include any autocomplete plugins and let users choose their own autocomplete plugins that works for them. It has necessary hooks, lsp_server_init and lsp_server_exited events which allows other plugins to register when a server is initialized or exited. It allows other plugins to send requests using lsp#send_request or receive server notifications using lsp#register_notifications. In the end it would look something like this.

function! s:on_notification(server_name, data) abort
    if s:is_diagnostics_notification(a:data)
        call ale#messages#clear()
        call ale#messages#add(s:convert_lsp_diagnostics_to_ale(a:data))
    endif
endfunction

lsp#register_notifications('*', function('s:on_notification'))

There are also other things like textDocument/didChange that needs to be handled.

@w0rp
Copy link
Member

w0rp commented Aug 19, 2017

The idea of having an LSP plugin send errors to ALE is a better one, but there a a number of problems which need to be solved, and some of which might not be possible to solve. The Socratic method applies here. There are many questions to ask.

If a separate plugin decides to send a list of problems to ALE at any time, how will ALE then control disabling linting for a file? Should ALE start ignoring messages being sent to it from other plugins? How will ALE implement re-enabling linting? Should there be a hook for telling other plugins that they should resend their list of items back to ALE again?

Should other plugins be required to implement a hook for when ALE requests a buffer to be checked? What if a user doesn't want ALE to check a file when it is opened, but another plugin thinks it does? What if the options for ALE change? Should they be required to call a function for checking if linting is enabled for a file? How would you go about disabling particular linters?

Should LSP linters be implemented in other plugins in the ale_linters directory like other linters? Linters that are defined with Define are designed to be started by ALE, but diagnostic results in general LSP plugins could arrive at any time. ALE is actually the one asking for a list of problems. Would another plugin implement a callback which is used to return a List of items to ALE through another function call to ALE?

There are many questions to ask.

My recommendation is to think about this, identify legitimate problems with how ALE works now, propose a workable solution which makes sense, and open a pull request for that solution. We could talk about this for weeks, but that would largely be a waste of time. Write some code, and get myself and others to look at it.

@dense-analysis dense-analysis locked and limited conversation to collaborators Aug 19, 2017
@w0rp
Copy link
Member

w0rp commented Aug 19, 2017

ALE has since implemented full support for Language Server Protocol itself, and there is no need for additional plugins. Servers can be started automatically, in some cases with zero configuration, and you don't pay for the features you don't use.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants