vscode-rust icon indicating copy to clipboard operation
vscode-rust copied to clipboard

🔥 merge with rust-analyzer extension 🔥

Open matklad opened this issue 4 years ago • 17 comments

So, as of today rust-analyzer is available via rustup, which probably is a good place to start actively meting TypeScript code bases.

This is the meta issue to discuss this.

I don't have a particular plan in mind, but here are some thoughts:

Location:

The unified extension should live in this repository. Long term, I still moderately strongly believe that a monorepo makes more sense long term, but:

  • it's not that important, and many people think that a separate repo makes more sense
  • while we support both rls and rust-analyzer, it certainly makes more sense to use a separate repo.

Leadership:

Who would be primarily "in charge" of unification effort and subsequent maintenance of unified codebase? Are there any volunteers?

Implementation:

I see roughly two ways to implement the big picture:

  • "dispatch" between two extensions -- have rls_activate/deactivate and ra_activate/deactivate functions, and call one of them from the top-level entry point. This way, the code of extensions themselves is mostly unmodified, there's just a single top-level shim added
  • "proper" merge into unified code bases, where different parts potentially know about each other

matklad avatar Jul 08 '20 13:07 matklad

Since I started the merging effort, I'll gladly bring this over the finish line.

With regards to the implementation, I'd lean towards the "proper" approach that was started in https://github.com/rust-lang/rls-vscode/pull/793. From what I can tell, people have been using it with more or less a success modulo some missing pieces/customizations that were not supported here. We can keep going and incrementally merge them into one.

Location

As long as the location of the extension goes, I'd say this should be live in the same place as other Rust editor extensions, even if we decide to support support only rust-analyzer at some point.

There's nothing inherently rust-analyzer about it and so I believe this better separates the concerns, so to speak. If there ever will be built-in LSP in rustc itself (as a possible path of migration) we'll be faced with the same dilemma whether to move.

Additionally, in retrospect I believe that having both the editor extension and the LSP together made custom extensions a lot more tempting, which in turn blessed a single editor and made the support in other editors somewhat lacking (e.g. see vim support thread and the initiative to cut down on extensions in rust-analyzer).

As such, I'd be in favor of keeping them separate long-term and rather finding more ways to facilitate the development on both tools, which I understand is the primary motivation for keeping them together in a monorepo.

:bike: :house:

I think it'd be good to rename the extension to something like vscode-rust to be more in line with the rest of the language extension and since it's not going to be rls-vscode for much longer. What do you think about it?

Xanewok avatar Jul 09 '20 10:07 Xanewok

Since I started the merging effort, I'll gladly bring this over the finish line.

Awesome, thank you!

I think it'd be good to rename the extension to something like vscode-rust to be more in line with the rest of the language extension and since it's not going to be rls-vscode for much longer. What do you think about it?

The extension is already called rust I believe. But +1 on renaming the repo to vscode-rust!

With regards to the implementation, I'd lean towards the "proper" approach

Makes sense! I think technically we might start with using some git magic to move editors/code directory from rust-analyzer into this repository (or we can forget the history and just-copy-paste, I am fine with both). I think it would be useful to have to code-bases as is in the same repo at some point, as migration via moving code from one folder to another would be more effective at not loosing important bits of functionality.

We might want to stir existing rust-analyzer extension in place some, to make the later grafting easier. One specific issue I think we need to solve before hand is distribution. Let me make a separate comment about that ...

matklad avatar Jul 09 '20 10:07 matklad

I think technically we might start with using some git magic to move editors/code directory from rust-analyzer into this repository

git subtree?

bjorn3 avatar Jul 09 '20 11:07 bjorn3

So, at the moment, rust-analyzer's binary is distributed via GitHub release, and we take advantage of the fact that the vscode extension and the server are packaged into a single release. Basically, every week we release a new version of vscode extension, and it than downloads the binary for the same tag. So, we effectively re-use vscode auto-update flow to update the binary itself.

I think, at least for transition period, unified extension should have an ability to auto-update rust-analyzer from GitHub releases. It's pretty crucial that we are able to ship incremental ~~breakages~~ improvements. But this auto-update can be done by polling (after asking for user's consent) github releases. We already have logic for this in place, for the nightly channel.

Long term, we should switch to primarily (only) supporting rustup.

matklad avatar Jul 09 '20 11:07 matklad

@bjorn3 more like git filter-branch I believe

matklad avatar Jul 09 '20 11:07 matklad

Long term, we should switch to primarily (only) supporting rustup.

TBH, I quite like the current setup because you can still update rust-analyzer without the full toolchain.

For nightly users, rustup update nightly invalidates every cache and ~~incremental build~~ build directory, so it's not worth updating every day.

lnicola avatar Jul 09 '20 11:07 lnicola

For nightly users, rustup update nightly invalidates every cache and incremental build, so it's not worth updating every day.

I would assume that the extension would run rustup run --toolchain nightly rust-analyzer --, so that the user don't have to switch manually. Though, we need to make sure that that would use the correct (stable) cargo check and cargo metadata.

matklad avatar Jul 09 '20 11:07 matklad

Read that as "for those who use nightly as their default toolchain".

lnicola avatar Jul 09 '20 11:07 lnicola

FWIW the repository is now renamed: https://github.com/rust-lang/vscode-rust.

Xanewok avatar Jul 12 '20 21:07 Xanewok

Nice! I think we are ready to „freeze“ vscode extension in rust-analyzer‘s repo after tomorrow’s release. I‘ll try to transfer the git history to this repo tomorrow

matklad avatar Jul 12 '20 22:07 matklad

PR is up: https://github.com/rust-lang/vscode-rust/pull/816

TIL: git-filter-repo is the new git filter-branch

matklad avatar Jul 13 '20 09:07 matklad

Long term, we should switch to primarily (only) supporting rustup.

That would be rather unfortunate. My Linux distro (Fedora) does a really good job at keeping the Rust toolchain up to date via its package manager, and I much prefer my distro's package manager to handle software installs than anything else.

I've a strong preference for the current settings for selectively disabling rustup to be maintained, and having the extension periodically poll GitHub for more recent rust-analyzer releases.

mattburgess avatar Jul 23 '20 21:07 mattburgess

I have opinions on this, but I also think it’s way to early to discuss any specifics here.

matklad avatar Jul 23 '20 22:07 matklad

I wonder if it wouldn't be better to rename the current extension and keep it for "stable" users (or, alternately, publish rust-analyzer under a new name) than to merge the extensions or try to support both servers with a single one.

lnicola avatar Oct 15 '20 16:10 lnicola

So what is the current status with this merge? It currently seems like this extension is abandoned. If the merge has been given up, we should update the extension to make it clear that this is deprecated and new users should install the rust-analyzer extension instead of this one, also see #927.

MarkDDR avatar Oct 03 '21 11:10 MarkDDR

See https://github.com/rust-analyzer/rust-analyzer/issues/4224#issuecomment-929008874 for the current status.

matklad avatar Oct 03 '21 13:10 matklad

Update: rust-analyzer has moved to rust-lang.rust-analyzer. We will probably not merge the two extensions, but instead deprecate the old one (RLS / rust-lang.rust), so that existing users (who might prefer RLS) will not see any changes.

lnicola avatar May 19 '22 08:05 lnicola