dense-analysis / ale

Check syntax in Vim/Neovim asynchronously and fix files, with Language Server Protocol (LSP) support
BSD 2-Clause "Simplified" License
13.56k stars 1.44k forks source link

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

Closed Firehed closed 7 years ago

Firehed commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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:

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

Rust definitely supports diagnostics. Seems the table is outdated.

On Wed, Jun 14, 2017, 5:07 AM w0rp notifications@github.com wrote:

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/

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/w0rp/ale/issues/517#issuecomment-308371101, or mute the thread https://github.com/notifications/unsubscribe-auth/ABTxFvJpV0Oguics-GpRprcQSHbJOhEtks5sD6KzgaJpZM4NL4jT .

w0rp commented 7 years ago

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

w0rp commented 7 years ago

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 commented 7 years ago

@w0rp did you have trouble installing the RLS?

w0rp commented 7 years ago

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

w0rp commented 7 years ago

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

euclio commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

The idea is to find some language server which is easy to work with, and use that to actually finish implementing support for this.

Firehed commented 7 years ago

Seems very reasonable. I got the PHP LSP linked above working in a fairly straightforward way outside of ale, which does the stdin/stdout thing. It specifically required initialize followed by textDocument/didOpen, but the protocol doesn't make it too clear to me if that's always the case or just what worked here.

I have no idea if that's more or less difficult than RLS above, but I'm more than happy to provide code samples, JSON bodies, or whatever else you find helpful. It's ~100loc most of which is just proc_open/fread/fwrite stuff.

w0rp commented 7 years ago

Sure, go for it.

Firehed commented 7 years ago

I cleaned it up a bit so hopefully it's actually possible to follow now - Firehed/php-lsp-testing. I don't know how much it will actually help in practice.

More importantly, I have it "sort of working": https://gist.github.com/Firehed/3f6828072a208600703417bf2d888dfd. The configuration is done horrendously (or not at all) and it mostly writes debug into to files, but it successfully starts the server, sends initialize, and receives the inbound messages.

I did bypass some of the validation to get some LSP stuff firing in any way, making a new callback in ale#engine#Invoke that works similarly to the TSServer one. Initial findings:

This particular LS automatically published diagnostic results for all project source files after the initialize message, and I added in some didOpen and didChange events since they seem especially relevant for Ale. In all cases, I'd receive something like this:

{"method":"textDocument\/publishDiagnostics","params":{"uri":"\/Users\/firehed\/dev\/php-lsp-testing\/goodfile.php","diagnostics":[]}}

(although sometimes the uri had a file:// prefix)

w0rp commented 7 years ago

Cool, thanks! I'll take a look and I'll try and implement LSP support for finding problems via stdio over the next few days. Support for TCP connections can be done later on.

Firehed commented 7 years ago

I've tweaked the diff a bit, it now successfully echos diagnostic messages to the console without requiring you to fully mess around with running the LSP server. Just chmod +x fake.php and it will just emit a publishDiagnostics event every few seconds.

https://gist.github.com/Firehed/73a9045a37b75c1629405be97a0758f8

Not all of the didOpen/didChange handling is present/correct, but I think the high level concept is getting reasonably close.

w0rp commented 7 years ago

I got automatic completion to work with tsserver. Again, there are many differences, but some of the basics are the same with LSP. I put that in a completion branch I might merge later, after I test it at work for a little while. It works like so:

  1. Watch for changes in insert mode timers and delays, like with linting.
  2. When the timer is run, and the cursor position hasn't changed, look for LSP linters, and send completion commands to the server for the first linter found.
  3. Get a response with completions with details. (Really two responses for tsserver.)
  4. If the cursor position still hasn't changed, then temporarily replace omnifunc with an ALE function, and then start omni completion which then calls that function, parses the response, and displays the results.

With the above, it's possible to automatically suggest completions while you type without blocking editing too much.

I did this mostly because I wanted to see if I could do it, and because it was fun.

w0rp commented 7 years ago

I got as far as getting the PHP language server to return this.

{"error":{"code":-32601,"message":"Method textDocument/didChange is not implemented","data":null},"id":null,"jsonrpc":"2.0"}

I dunno if that means that it can't be used to do what we want or not.

Firehed commented 7 years ago

I was originally getting the same with textDocument/didOpen - it turned out that I was sending the event before the server had finished processing (and responding to) the initialize message. Looking at the source of the PHP LS indicates the textDocument message parser is set up in initialize, and that error is emitted when any textDocument/* message is received before that happens.

I did a dirty hack to avoid this, but I think a more stable implementation needs track the request and response IDs for, at least, initialize and textDocument/didOpen. I used dumb autoincrement IDs, but the spec says strings are allowed so another option is to encode project- and file-specific data in there (e.g. "id": "initialize:~user/projectRoot" or "id":"didOpen:path/to/file.ext") but that may be unnecessary/wasteful.

Something like this, I think:

Other servers may handle this better, but clearly it's not safe to rely on this behavior. I wish the LSP spec was clearer in this regard, but oh well.

Also, the autocomplete stuff sounds awesome! The language-specific plugins I've used (php and rust, primarily) have been generally, uh, slow and not wonderful. I'd be super excited to see that (and possibly other LSP features, seeing that we've come this far) land or be something that other plugins can hook into.

w0rp commented 7 years ago

I wish they had defined the LSP spec so that the server couldn't handle a request from a client until after it had finished sending a response. Then you'd be able to just send 3 requests in the right order, and not have to worry about waiting until responses have been handled.

I also wish they made this version number thing optional. I can't think of many scenarios where it would be an advantage to identify each and every change to a file if undo messages also have to increment the numbers.

I'll see if I can delay sending the other messages until the responses come back.

Firehed commented 7 years ago

Yeah, I agree it seems a bit silly, since it forces editors to do a probably-unnecessary amount of state tracking. I think the intent was that you'd use the cancel message on an undo, but seeing that publishDiagnostics isn't technically a response to any specific message, that model doesn't work too well.

jez commented 7 years ago

Just stumbled upon this thread; seems like LSP support is in the pretty early stages. Which perhaps makes it a good time for this question: what's the ultimate goal for ALE + LSP?

ALE is fundamentally a "checking" plugin. If we look at this through the lens of LSP, ALE currently only deals with what LSP would call "Diagnostics," which is their word for error and warning messages.

However, LSP is larger than just diagnostics: they also support type inspection, code completion, reference navigation, and more. With that in mind, what might make sense is to find an existing plugin for handling LSP, and integrate ALE into it.

For example: LanguageClient-neovim is an LSP plugin for Neovim. Its goal is to support all LSP features. One way it does this is by fetching out to external plugins (like deoplete or nvim-completion-manager) for individual tasks.

ALE is already good at tasks like navigating the quickfix/loc lists (:ALENext, :ALEPrevious) and enabling or disabling diagnostics (:ALEToggle). These are features that a full-fledged LSP-client would have to re-implement.

It might be the case that we should be looking to integrate ALE into LSP plugins, not LSP into the ALE plugin. Of course, I'm new here, and it's ultimately up to you what you do with this plugin 😉 It just seemed like we could benefit from a discussion of what the goal is.

For reference: two popular plugins for working with LSP in Neovim:

w0rp commented 7 years ago

The goal is to first implement language diagnostics so language server errors can be checked alongside running executables. Then after that, I'll consider adding completion support, which I already have working for tsserver, and I'll probably support that after I find that it works well enough for me to use it for work.

I'm not really interested in depending on other plugins too much, as there's little to no support for managing plugin versions for Vim plugins, or dependency trees, like PIP, NPM, etc. I'm willing to pay the cost of implementing LSP support separately, as I think it will be far less than the cost of trying to integrate with another plugin for this.

The plugins linked above don't seem to have Vim 8 support, so they are no-go for me.

w0rp commented 7 years ago

I'll work on getting LSP support via stdio working eventually. I stopped working on it for now so I could focus on fixing bugs and working on other projects. I don't think it's too far off. tsserver support should work already without any configuration required.

w0rp commented 7 years ago

I just merged the code for LSP stdio support, but it's not working yet. I want to get some other people trying it out with the existing tsserver integration, and I want to develop and changes for tsserver in such a way that will be easier to add in LSP support later.

w0rp commented 7 years ago

I managed to get the PHP language server to send diagnostics for a file, and then those diagnostics are displayed as errors, but it doesn't quite work yet. For some reason, the PHP language server seems to send the correct diagnostics for a file you are editing, if you keep sending the textDocument/didChange message, but also keeps sending the original diagnostics back to you as well. I don't know why this is happening, if there is a bug in the PHP language server, or what.

If you want to debug this and try and fix it yourself, I recommend doing the following.

Run ALE with the latest commit and Vim 8, and add the following line to your vimrc file:

call ch_logfile(expand('~/channel.log'), 'w')

Then remove the finish line in ale_linters/php/langserver.vim , which is used to disable the PHP language server client for now. You should be able to see 100% of the messages going back and forth between Vim and the language server.

w0rp commented 7 years ago

I have noted some very strange behaviour from the PHP language server.

  1. The PHP language server outputs a very large number of log messages, and there doesn't appear to be a way to tell the server not to send those log messages.
  2. The PHP language server never sends an initialize response. It just starts publishing diagnostics straight away, and you have to figure out which project might have been initialized from those messages.
  3. The PHP language server immediately starts publishing diagnostics on files you never told the language server you opened.
w0rp commented 7 years ago

I got the Rust language server to work in a basic way. The installation notes for rls from @euclio were very helpful. Thank you for your help @euclio. You can try it out now by using let g:ale_linters = {'rust': ['langserver']} or similar. I have barely tested it yet, I just got it to start reporting errors as I edited a buffer, and it didn't blow up.

Firehed commented 7 years ago

This is awesome! I am super excited, and will be happy to help test things out.

I observed similar behavior with the PHP LSP when prototyping - upon calling initialize, it seems to index the whole project (and send the messages) regardless of what files are open. I expect that other implementations, especially for dynamic languages, may do similar things since they are also going to support code-completion and not just diagnostics, although going without the spam would be nice. I did see you opened an issue in the project

For some reason, the PHP language server seems to send the correct diagnostics for a file you are editing, if you keep sending the textDocument/didChange message, but also keeps sending the original diagnostics back to you as well.

I'm not experiencing this issue, but I did observe that opening a file for the first time cause Ale to publish the changes twice:

 59.455754 : looking for messages on channels
 61.176643 SEND on 0: 'Content-Length: 205

{"id":null,"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///Users/firehed/dev/php-lsp-testing/bad.php","version":4,"languageId":"php","text":"<?php\ninval<<<<id;"}}}'
 61.176985 SEND on 0: 'Content-Length: 209

{"id":null,"jsonrpc":"2.0","method":"textDocument/didChange","params":{"contentChanges":[{"text":"<?php\ninval<<<<id;"}],"textDocument":{"uri":"file:///Users/firehed/dev/php-lsp-testing/bad.php","version":5}}}'
 61.182626 SEND on 0: 'Content-Length: 209

{"id":null,"jsonrpc":"2.0","method":"textDocument/didChange","params":{"contentChanges":[{"text":"<?php\ninval<<<<id;"}],"textDocument":{"uri":"file:///Users/firehed/dev/php-lsp-testing/bad.php","version":6}}}'
 61.188844 : looking for messages on channels
 61.242529 RECV on 0: 'Content-Type: application/vscode-jsonrpc; charset=utf8
Content-Length: 304

{"method":"textDocument\/publishDiagnostics","params":{"uri":"file:\/\/\/Users\/firehed\/dev\/php-lsp-testing\/bad.php","diagnostics":[{"range":{"start":{"line":1,"character":7},"end":{"line":1,"character":7}},"severity":1,"code":null,"source":"php","message":"'Expression' expected."}]},"jsonrpc":"2.0"}Content-Type: application/vscode-jsonrpc; charset=utf8
Content-Length: 41

{"result":null,"id":null,"jsonrpc":"2.0"}Content-Type: application/vscode-jsonrpc; charset=utf8
Content-Length: 304

{"method":"textDocument\/publishDiagnostics","params":{"uri":"file:\/\/\/Users\/firehed\/dev\/php-lsp-testing\/bad.php","diagnostics":[{"range":{"start":{"line":1,"character":7},"end":{"line":1,"character":7}},"severity":1,"code":null,"source":"php","message":"'Expression' expected."}]},"jsonrpc":"2.0"}Content-Type: application/vscode-jsonrpc; charset=utf8
Content-Length: 41

{"result":null,"id":null,"jsonrpc":"2.0"}Content-Type: application/vscode-jsonrpc; charset=utf8
Content-Length: 304

{"method":"textDocument\/publishDiagnostics","params":{"uri":"file:\/\/\/Users\/firehed\/dev\/php-lsp-testing\/bad.php","diagnostics":[{"range":{"start":{"line":1,"character":7},"end":{"line":1,"character":7}},"severity":1,"code":null,"source":"php","message":"'Expression' expected."}]},"jsonrpc":"2.0"}Content-Type: application/vscode-jsonrpc; charset=utf8
Content-Length: 41

{"result":null,"id":null,"jsonrpc":"2.0"}'
 61.242539 on 0: Invoking channel callback <SNR>108_VimOutputCallback
 65.193765 : looking for messages on channels

This was achieved by opening a good file, waiting for that absurd spewing of indexing spam to finish, and then :vsp bad.php. I did not make any changes to the file, just opened it in a new split.

Opening the same bad file again in another split (e.g. |good|bad|bad|) sometimes fired another didChange event, despite no changes occurring. Not sure what's up there.

This was with PHP LS @ 7ce2284

Realistically, I think that any PHP LS is going to be fairly limited in what it can do for diagnostics (this one doesn't seem to handle much more than what the existing php -l linter does) just due to the nature of the language. I'll get the Rust one set up as well and see how that goes.

w0rp commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.

w0rp commented 7 years ago

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 commented 7 years ago

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.

prabirshrestha commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

w0rp commented 7 years ago

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 commented 7 years ago

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.

nickspoons commented 7 years ago

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.