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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Language Server Protocol #8

Closed
sebakri opened this issue Sep 26, 2017 · 15 comments
Closed

Language Server Protocol #8

sebakri opened this issue Sep 26, 2017 · 15 comments

Comments

@sebakri
Copy link

sebakri commented Sep 26, 2017

Hi,
FIRST: you really do a great job 馃

Do you plan to support LSP (https://github.com/Microsoft/language-server-protocol)
I think it would be great if the server could speak LSP :-)
Maybe it does already and I just didn't find it in code. This wouldn't be the first time ;-)

@castwide
Copy link
Owner

Thanks!

I've looked into the possibility of LSP implementation, and it's a definite candidate for a future version. One consideration: LSP supports a lot of features that the Solargraph gem historically leaves to other tools, such as validation and document formatting. We need to determine which features to integrate and how to handle it.

@ypresto
Copy link

ypresto commented Nov 20, 2017

There is another (actively developed) language server project but it seems lacking type inference.
https://github.com/mtsmfm/language_server-ruby
Perhaps this library could be integrated to that server.

(NOTE: Owner of that lib also published LSP server core.)
https://github.com/mtsmfm/language_server-protocol-ruby

@castwide
Copy link
Owner

I've started work on LSP support. The first version will use stdio for client connections.

@castwide
Copy link
Owner

I'm pivoting to socket connections for the first version of LSP. The performance on stdio dropped with the addition of linting because it depends on a spawned rubocop process, and it would hang while the parent was reading from stdin. The socket server is able to run rubocop in the background.

Work in progress is on the language-server branch. I expect to add helpers to solargraph-utils to make it easy to use with Node.js in general and vscode-languageclient in particular.

@castwide
Copy link
Owner

To run the socket server, use solargraph socket (default port is 7658).

@castwide
Copy link
Owner

castwide commented Apr 3, 2018

The language-server branch is merged into master. Version 0.18.0 of the gem and version 0.14.0 of the VSCode extension will use LSP.

Documentation is in progress at https://github.com/castwide/solargraph/blob/master/LANGUAGE_SERVER.md.

@castwide
Copy link
Owner

castwide commented Apr 5, 2018

Support for LSP is published in gem version 0.18.0.

@castwide
Copy link
Owner

Version 0.19.0 is published. The VSCode extension uses Microsoft's language client for integration, and at least one person has gotten it to work with Sublime.

@keegancsmith
Copy link

Hello @castwide awesome work. We took a look at experimentally integrating solargraph into https://sourcegraph.com/ via your new LSP support: sourcegraph/lsp-adapter#19

I came across two issues, I'll just mention them here but can file individual issues:

I see above you mention requiring TCP. I'm interested if solargraph would work with multiple workspaces (a workspace per TCP connection) once the CWD issue is solved? Also interested in why server mode fixes rubocop?

I'm not a ruby programmer, but anything we (Sourcegraph) can do to help solve that? This seems like the most promising ruby LS we have tried out.

@castwide
Copy link
Owner

Thanks, @keegancsmith!

  • Good to know about the didOpen notification. That might resolve a recent issue with the VSCode extension as well.
  • Solargraph sets the workspace root as the CWD so the process can use whatever Ruby environment is configured for the project using .ruby-version, gemsets, bundler, etc. This is especially necessary for environments using Ruby installation managers like rvm and rbenv. There's a related discussion at Question: setup with multi Ruby install聽vscode-solargraph#12. It should be possible to remove that assumption from the server, however. It's mostly a function of the client that starts the process.

A workspace per TCP connection should be feasible.

When I experimented with a stdio transport, child RuboCop processes would block I/O while they were running, so any other requests and notifications would be frozen until RuboCop exited. I'm open to adding a stdio transport, but that issue would need to be resolved.

I appreciate any help you'd like to provide. Please feel free to open new issues or PRs.

@castwide
Copy link
Owner

castwide commented May 1, 2018

The master branch includes a Library update that allows mapping of files without a didOpen notification. It'll be published in version 0.21.0.

@keegancsmith
Copy link

The master branch includes a Library update that allows mapping of files without a didOpen notification. It'll be published in version 0.21.0.

great!

Solargraph sets the workspace root as the CWD so the process can use whatever Ruby environment is configured for the project using .ruby-version, gemsets, bundler, etc. This is especially necessary for environments using Ruby installation managers like rvm and rbenv. There's a related discussion at castwide/vscode-solargraph#12. It should be possible to remove that assumption from the server, however. It's mostly a function of the client that starts the process.

Yeah, I can image a very simple fix just being Dir.chdir on the workspace root.

A workspace per TCP connection should be feasible.

It's not really that important TBH. It's useful if there are resources that can be shared between workspaces, but otherwise a subprocess per workspace is quite a useful thing to do.

child RuboCop processes would block I/O while they were running, so any other requests and notifications would be frozen until RuboCop exited.

Could that be an issue with however you do subprocesses? Maybe you are sending your stdin to the subprocess? I find it interesting that somehow using server fixed it, but yeah I am likely missing something due to lack of ruby experience.

I appreciate any help you'd like to provide. Please feel free to open new issues or PRs.

Thanks for the feedback!

@castwide
Copy link
Owner

castwide commented May 2, 2018

Yeah, I can image a very simple fix just being Dir.chdir on the workspace root.

Something like that might work for most cases, but rvm/rbenv, gemsets, bundles, etc. add a lot of moving parts that require the process to modify the environment at startup. Using the workspace as the CWD was the easiest way to resolve the most common issues. There might be other solutions, but it's been an ongoing concern.

Could that be an issue with however you do subprocesses? Maybe you are sending your stdin to the subprocess? I find it interesting that somehow using server fixed it, but yeah I am likely missing something due to lack of ruby experience.

I tried a few different methods, but the subprocess always blocked stdio, even when I started it in a separate thread and used Open3 to pipe the streams. Using a TCP socket for I/O bypassed the problem, so I went with that for the first transport implementation. There could very well be some other solution I missed, though.

@castwide
Copy link
Owner

castwide commented May 7, 2018

Gem version 0.21.0 is published with correct handling of files without the didOpen notification.

@castwide
Copy link
Owner

LSP is now the standard integration method.

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

No branches or pull requests

4 participants