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

Support rust-analyzer as Language Server #315

Closed
tweksteen opened this issue Jun 16, 2020 · 24 comments
Closed

Support rust-analyzer as Language Server #315

tweksteen opened this issue Jun 16, 2020 · 24 comments

Comments

@tweksteen
Copy link
Contributor

rust-analyzer is the evolution of RLS and has support for non-Cargo based projects.

Currently, corrosion requires to have RLS. RLS and rust-analyzer should be interoperable but corrosion relies on some static output of RLS (namely, rls --version).

@akurtakov
Copy link
Member

Do you know whether rust-analyzer can be installed via rustup?

@nmusatti
Copy link

No, according to the documentation: https://rust-analyzer.github.io/manual.html#installation it's not possible at the moment. You have to download a binary or wrap it up somehow.

@mickaelistria
Copy link
Contributor

Rustup installation is a must-have for proper integration in Corrosion. I suggest you open a request to rustup and/or rust-analyzer to work together and share the link here. It will be a pre-requisite.

@tweksteen
Copy link
Contributor Author

Thanks. This is tracked at rust-lang/rust#72978. (cc @matklad)

Assuming this lands soon, what needs to be done on the plugin side to fix the interaction with it? (One can already manually test using the current installation method).

@mickaelistria
Copy link
Contributor

Hopefully, a simple string replacement of rls by rust-analyzer in the various commands built by Corrosion should be enough ;)

@matklad
Copy link

matklad commented Jun 18, 2020

Rustup installation is a must-have for proper integration in Corrosion.

If the reason here is "we only want to support officially endorsed and distributed language servers", then it makes sense to wait for rustup integration (I'll estimate that as a couple of months). If the reason is "we assume the user has rustup", than I'd argue it is suboptimal user experience. The best "search path" for an IDE is, I think:

  • try to look the binary in $PATH
  • try to look in ~/.cargo/bin
  • fallback to installation via rustup, if that is avaiable
  • bail out and show a warning dialog.

@tweksteen
Copy link
Contributor Author

I agree. To echo Aleksey's reply, it might be worth being more explicit about your assumptions on the tools availability.

The "textbook" development workflow of installation via rustup, managing dependencies and building/testing via cargo does not apply everywhere, especially for heterogeneous code base. For instance, Android provides a prebuilt toolchain and uses an external build system. This shouldn't be the default supported case, but clarifying the assumptions would help making that scenario somewhat supported.

(Although this conversation would be better continued in another bug.)

@mickaelistria
Copy link
Contributor

Corrosion offers only 1 installation path, which is the default one for the language. Rustup also makes it very easy to ship Corrosion without risk of wrong IP stuff: basically, the IP and technical parts of installations and updates of the language server are externalized to Rustup and users actions, it's less responsibility and risk.
Corrosion cannot easily start shipping RLS nor Rust-Analyzer, it's simpler to provide 1 installation path, and that's better to provide the "official" path that some alien one.

However, the case of different installation is already handled: if you go to Window > Preference > Rust, you can already select "Rust Language Server Location > Other installation" to configure a different path for RLS or rust-analyzer.
You can also already use that to try rust-analyzer (just change the RLS path to the one of rust-analyzer and see what works and what doesn't), that would already be great to have feedback about just switching the LS that way.

When rust-analyzer is properly supported by the official installation path with rustup, then we'll just switch to it; unless some blocker issues are found.

@matklad
Copy link

matklad commented Jun 18, 2020

Also cc rust-lang/rust-analyzer#4604, which documents LSP extensions we use.

@tweksteen
Copy link
Contributor Author

You can also already use that to try rust-analyzer (just change the RLS path to the one of rust-analyzer and see what works and what doesn't), that would already be great to have feedback about just switching the LS that way.

That's right and I'm fine with this approach. This was actually what I mentioned in the 1st comment of this bug: corrosion relies on the output of rls --version, which fails for rust-analyzer.

@mickaelistria
Copy link
Contributor

This was actually what I mentioned in the 1st comment of this bug: corrosion relies on the output of rls --version, which fails for rust-analyzer.

If you want to provide a patch that allows more flexibility here (ie just warn in case output cannot be processed), that would be welcome; especially since we'll need it sometime later when moving to rust-analyzer as default.

@tweksteen
Copy link
Contributor Author

Simply fixing the regular expression was enough to get some integration working. I'll fill up other bugs for the specific issues. Thanks. (cc @werwurm)

@mickaelistria
Copy link
Contributor

Hi @tweksteen , did you identify further issues with rust-analyzer instead of RLS?
If not, then I believe the only thing we miss for a smooth transition would be ability to run rustup -c rust-analyzer

@mickaelistria
Copy link
Contributor

On rust-lang/rust-analyzer#4753 (comment) , @bjorn3 explained "There is already the rust-analyzer-preview rustup component." . Is it worth switching to it soon?

@tweksteen
Copy link
Contributor Author

@mickaelistria No blocking issue. I did have to create a separate config file for the language server, by setting the "RLS config" field. The documented default rls.conf was not picked up automatically. Otherwise, it somewhat works.

@mickaelistria
Copy link
Contributor

Ok, and do you think rust-analyzer is already better than RLS, and that switching to it as soon as it's in "stable" rustup toolchain would make Corrosion better?

@tweksteen
Copy link
Contributor Author

I wouldn't make such a call myself (as I have never used RLS). It is worth pointing out that upstream is officially supporting the move via RFC 2912. Therefore, it seems that it is just a matter of time until RLS is fully deprecated.

@mickaelistria
Copy link
Contributor

Is there anything left to do it when it come to allowing to use rust-analyzer; or are the current options good enough?
I agree we shouldn't switch from RLS to rust-analyzer before rust-analyzer is considered as stable. So if not further option is necessary for your case, we can close this issue.

@akurtakov
Copy link
Member

We need rustup release rust-lang/rustup#2408 so there is proper proxy for rust-analyzer. Once it's there we can provide ui to switch between rls and rust-analyzer.

@mickaelistria
Copy link
Contributor

@tweksteen I got some question about rust-analyzer support. Do you think you can document in Corrosion (with a PR) how one can configure Corrosion to use rust-analyzer instead of RLS?

@mickaelistria
Copy link
Contributor

I stumbled upon https://blogs.gnome.org/chergert/2021/03/04/rust-and-gnome-builder/ , there is apparently a flatpak that assumes rust-analyzer is stable. I'm wondering whether we should try to get Corrosion use rust-analyzer more aggressively; for example by testing against rust-analyzer instead of rls and by hacking the preference page to give more room to rust-analyzer.

@akurtakov
Copy link
Member

That would be very good approach IMHO.

@mickaelistria
Copy link
Contributor

Done with #357 . Snapshots promoting and prioritizing rust-analyzers should be available shortly.

@mickaelistria
Copy link
Contributor

Please try https://download.eclipse.org/corrosion/snapshots/products/ and create new tickets for further issues.

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

No branches or pull requests

5 participants