Stream: t-compiler/wg-rls-2.0

Topic: merging RLS and VS Code extension


matklad (Apr 28 2020 at 14:04, on Zulip):

Hi @Igor Matuszewski ! I feel like we should chat a bit about strategy for VS Code extension some time. I really like your proposal about using a single existing extensions for both.

I don't have time for a chat right now, but I am creating this stream as a placeholder, and for the case you already have some ideas do dump in async fashion :)

Igor Matuszewski (Apr 28 2020 at 14:05, on Zulip):

sounds great :thumbs_up:

Igor Matuszewski (Apr 28 2020 at 15:09, on Zulip):

So I think immediately we should re-brand the Rust (rls) extension to a regular "Rust support for Visual Studio Code" or similar (de-emphasize RLS)

Igor Matuszewski (Apr 28 2020 at 15:10, on Zulip):

I did clean some bits and pieces and tried to extract the core "client workspaces" (spawned LSPs and their rootUris) and project detection logic and abstract it away from the rest of the extension

Igor Matuszewski (Apr 28 2020 at 15:11, on Zulip):

so I'd say we're in a good position to start implementing the RA engine support

Igor Matuszewski (Apr 28 2020 at 15:11, on Zulip):

One thing that needs reworking is the configuration prefixes - currently we have rust-client.* for anything client specific and rust.* are automatically synced with corresponding plugins

Igor Matuszewski (Apr 28 2020 at 15:12, on Zulip):

so I think we'd have to change that logic and extract a common Rust and RLS-specific options or at least warn against using RLS-specific configurations (and vice-versa with RA)

matklad (Apr 30 2020 at 16:34, on Zulip):

@Igor Matuszewski let's continue here, for future reference

Igor Matuszewski (Apr 30 2020 at 16:35, on Zulip):

So I'm all for merging the plugins, definitely

matklad (Apr 30 2020 at 16:35, on Zulip):

So, I wouldn't actually hate moving vscode plugin to a separare repo, but I definitelly would strongly prefer it in tree

matklad (Apr 30 2020 at 16:35, on Zulip):

and I don't feel there are strong reasons not to do this.

Igor Matuszewski (Apr 30 2020 at 16:35, on Zulip):

heh, I just wanted to metion that I'm strongly in favour of having it dedicated :)

matklad (Apr 30 2020 at 16:35, on Zulip):

Like, even if it is in a separate repo under rust-lang, it's still blessed

matklad (Apr 30 2020 at 16:36, on Zulip):

But I guess that's probably not the most important thing -- it should be easey to move one way or anoter afterwards

Igor Matuszewski (Apr 30 2020 at 16:36, on Zulip):

for me, I think that coupling these two together encourages doing so even further, which results in rust-analyzer not being strictly LSP conformant and/or having some issues when trying to integrate with other editors

Igor Matuszewski (Apr 30 2020 at 16:37, on Zulip):

and it sorts of sends the message of 'extension for Rust Analyzer', which I believe should be "the official, go-to Rust support extension for Visual Studio Code"

Igor Matuszewski (Apr 30 2020 at 16:37, on Zulip):

so that people don't need to go into details that there's RLs or Rust Analyzer or anything else, just a good VSCode experience out-of-box

matklad (Apr 30 2020 at 16:38, on Zulip):

Yeah, agree that "marketing"-wise separate repo is better

Igor Matuszewski (Apr 30 2020 at 16:38, on Zulip):

we could have nightly features or prototypes for a development extension in-tree, but once they mature I imagine we should add support for new features as they are stabilized and added to the official extension

matklad (Apr 30 2020 at 16:39, on Zulip):

That I would think would be worst :D

Igor Matuszewski (Apr 30 2020 at 16:39, on Zulip):

let's not then :) didn't want to lose the development velocity bonus of keeping stuff in-tree

matklad (Apr 30 2020 at 16:39, on Zulip):

Like, coordinating two streams of development is painful :)

Igor Matuszewski (Apr 30 2020 at 16:40, on Zulip):

if anything, I think it promotes a cleaner separation of concerns and more well-though "API" boundaries

Igor Matuszewski (Apr 30 2020 at 16:40, on Zulip):

so it's not like it's all bad :stuck_out_tongue:

Igor Matuszewski (Apr 30 2020 at 16:40, on Zulip):

but agreed, it's more work overall

matklad (Apr 30 2020 at 16:40, on Zulip):

Not sure -- like, there's a process boundary anyway, you can't get any more separated then that

matklad (Apr 30 2020 at 16:41, on Zulip):

but let's probably pause this particular discussion -- as I've said, these seems easy to change as we go

matklad (Apr 30 2020 at 16:41, on Zulip):

Rather, how do you imagine the actual merge?

Igor Matuszewski (Apr 30 2020 at 16:41, on Zulip):

yes, but the urge to be compliant to that once implementation is easy to satify if all you're going to do is push 1 commit with changes here and there

Igor Matuszewski (Apr 30 2020 at 16:41, on Zulip):

err, misspoke but you get the idea :)

matklad (Apr 30 2020 at 16:42, on Zulip):

I sort of see two ways this can play out

Igor Matuszewski (Apr 30 2020 at 16:42, on Zulip):

I didn't read the source code for the rust-analyzer extension yet, which I think I should before commenting more on that

matklad (Apr 30 2020 at 16:42, on Zulip):

tbh, I haven't read source of rls-vscode for a while :D)

Igor Matuszewski (Apr 30 2020 at 16:43, on Zulip):

I don't think there's pressure to merge them right away - I'd focus on getting the support for Rust Analyzer first in the rls-vscode, so that we can technically progress with what's outlined in the RFC

matklad (Apr 30 2020 at 16:43, on Zulip):

Yeah, that also would work

Igor Matuszewski (Apr 30 2020 at 16:44, on Zulip):

as we go, we should add more and more feature parity but also document and identify the LSP extensions

matklad (Apr 30 2020 at 16:44, on Zulip):

On rust-analyzer side, I think it makese sense to first fix LSP-complience internally, and only then move to officially suggesting users to try

Igor Matuszewski (Apr 30 2020 at 16:44, on Zulip):

how did you see the process?

matklad (Apr 30 2020 at 16:45, on Zulip):

Let me think, I haven't considered adding preliminary support directly to rls-vscode

matklad (Apr 30 2020 at 16:46, on Zulip):

Yeah, I would imagine that eventually we'd just do one big-bang merge. Like, it's only 4k lines of code total, don't think we need any kind of complicated staged rollout procedure or what not

matklad (Apr 30 2020 at 16:47, on Zulip):

(as an aside, I considered implementing the extension in a single TS file, to get rid of the bundler, and to create a pressure to not add custom TS things)

matklad (Apr 30 2020 at 16:47, on Zulip):

But initially it indeed seems a good idea to add rust-analyzer mode to existing extension, which would basically call rust-analyzer binary instead of rls.

matklad (Apr 30 2020 at 16:48, on Zulip):

Maybe having just a config switch for this is enough? And showing the message to the user with the installation instructions.

matklad (Apr 30 2020 at 16:48, on Zulip):

We can also copy-paste the code for downloading the binary form GitHub releases, but it is somewhat messy

Igor Matuszewski (Apr 30 2020 at 16:48, on Zulip):

Well, maybe

Igor Matuszewski (Apr 30 2020 at 16:48, on Zulip):

It sort of is nice to just click "Now use Rust Analyzer" and stuff is being done for you

matklad (Apr 30 2020 at 16:49, on Zulip):

Alternatively, we can first put rust-analyzer into rustup, and than installation becomes rustup component add

Igor Matuszewski (Apr 30 2020 at 16:49, on Zulip):

I very much like that, coupled with the task execution introspection - users see what's happening so they can follow along and understand

matklad (Apr 30 2020 at 16:49, on Zulip):

what is "task exection introspection"?

Igor Matuszewski (Apr 30 2020 at 16:49, on Zulip):

yes, rustup component add rust-analyzer would be definitely the easiest to solve from the extension's perspective

Igor Matuszewski (Apr 30 2020 at 16:50, on Zulip):

so to install RLS, we need to install rust-src, rust-analysis and rls components - we execute the tasks as if they were provided by the user

Igor Matuszewski (Apr 30 2020 at 16:50, on Zulip):

and they can see the output and the progress of what's happening, e.g. rustup component add XYZ --toolchain ABC

Igor Matuszewski (Apr 30 2020 at 16:51, on Zulip):

I think that's both convenient and informative, so the users don't think it's some black magic machinery they don't have any influence over

matklad (Apr 30 2020 at 16:51, on Zulip):

Hm, yep, that's nice UX!

matklad (Apr 30 2020 at 16:51, on Zulip):

So yeah, I think it'll be useful for you to talke a look at this code: https://github.com/rust-analyzer/rust-analyzer/blob/fec1e7c8e10e1c592642fac0c497cd57bd3f003c/editors/code/src/main.ts#L171-L243

Igor Matuszewski (Apr 30 2020 at 16:52, on Zulip):

yep, thanks!

matklad (Apr 30 2020 at 16:52, on Zulip):

(and probably https://github.com/rust-analyzer/rust-analyzer/blob/master/editors/code/src/net.ts as well, which handled the networking)

matklad (Apr 30 2020 at 16:53, on Zulip):

That's the integration with the current release process. I think coping just that to rust-vscode would be the best way to get the ball rolling

matklad (Apr 30 2020 at 16:53, on Zulip):

definitelly faster than pusing stuff to rustup

Igor Matuszewski (Apr 30 2020 at 18:48, on Zulip):

yep, definitely

Igor Matuszewski (Apr 30 2020 at 18:48, on Zulip):

I'll copy over the relevant bits

Igor Matuszewski (May 06 2020 at 00:04, on Zulip):

There's a WIP that downloads and install rust analyzer if "rust-client.engine" (also WIP) is set to rust-analyzer: https://github.com/rust-lang/rls-vscode/tree/support-rust-analyzer

Igor Matuszewski (May 06 2020 at 00:04, on Zulip):

It's rough around the edges and lacks some bits like automatic rust-src component installation but it's getting there =)

Igor Matuszewski (May 06 2020 at 00:04, on Zulip):

Great work, thanks!

matklad (May 06 2020 at 08:02, on Zulip):

Nice!

matklad (May 06 2020 at 08:04, on Zulip):

I wonder what specific next step should we pursue....

Igor Matuszewski (May 06 2020 at 09:31, on Zulip):

I need to make the end-to-end installation experience pleasant and complete, along with further making internal bits more engine-agnostic

Igor Matuszewski (May 06 2020 at 09:39, on Zulip):

@matklad there is one thing though that's making the current integration suboptimal. If I had to guess, Rust Analyzer listens to workspace folder changes and wants to be a single LSP server, whereas we went with the single server per project directory approach, to minimize the overhead so to speak

Igor Matuszewski (May 06 2020 at 09:40, on Zulip):

is my guess correct? If so, what do you say about intercepting and ignoring the workspace change requests and keep the fine-grained approach we have currently?

Igor Matuszewski (May 06 2020 at 09:40, on Zulip):

there's also the fact that rust-analyzer aims to be more build system agnostic, whereas our project layout logic in the extension uses Cargo.toml as its basis for project discovery and where to spawn the RLS instance

matklad (May 06 2020 at 09:49, on Zulip):

I feel pretty strongly that a single server instance is a better approach

matklad (May 06 2020 at 09:50, on Zulip):

rust-analyzer can (in theory more than in practice though) support arbitrary complex projects layout. For example, it can support several interdependent workspaces, and different workspaces can share sysroot crates

matklad (May 06 2020 at 09:51, on Zulip):

There's some discussion about this in this PR: https://github.com/rust-analyzer/rust-analyzer/pull/3987

Igor Matuszewski (May 06 2020 at 11:52, on Zulip):

didn't dig deeper into that but just want to add that Jannick was also kind enough to contribute to the existing workspace support in the RLS extension

Igor Matuszewski (May 06 2020 at 11:54, on Zulip):

and I'm not disagreeing on the single server instance approach, just to be clear

Igor Matuszewski (May 06 2020 at 11:54, on Zulip):

however I do find the argument of laziness/reduced overhead sound, especially in terms of possibly big monorepos

Igor Matuszewski (May 06 2020 at 11:55, on Zulip):

so I'm not sure how rust-analyzer works internally as of now, but if I were to initialize it in a monorepo will it try to, let's say, analyze project A even if I only opened/edited files for project B (which may not be related to project A)?

Igor Matuszewski (May 06 2020 at 11:57, on Zulip):

/me digs into that PR

Igor Matuszewski (May 06 2020 at 12:36, on Zulip):

Jesus christ I hate Zulip UX, sorry for the topic spam

matklad (May 06 2020 at 13:02, on Zulip):

So this is somewhat in the air internally, but generally you "initialize" rust-analyzer with a set of projects. The "initialize" logic is a piece that I'd expect we tweak quite a bit in the future, but at the moment it is "discover Cargo.toml's in workspace folders up to depth 1". One thing we should definitelly do is to change this to "look at the explicit set of Cargo.toml in the config", which should cover "I want to activate rust-analyzer only for subset of open projects"

Igor Matuszewski (May 06 2020 at 13:18, on Zulip):

Does this mean that it doesn't respect root{Path, Uri} and rather looks for the in the configuration?

matklad (May 06 2020 at 13:19, on Zulip):

So, at the moment, we don't look at config

matklad (May 06 2020 at 13:20, on Zulip):

But I expect that we should add feature where, if config contains cargoTomls = ["foo/Cargo.toml", "bar/Cargo.toml"], we should use that

matklad (May 06 2020 at 13:20, on Zulip):

That is, I think it's important to nail down both:

1) auto discovery
2) explicit config

and that probably auto-discovery should desugar internally to the same thing as explicit config

Igor Matuszewski (May 11 2020 at 18:03, on Zulip):

Here's a first pass at supporting Rust Analyzer in a user-friendly way - it'll install rust-src for you, you can specify a releaseTag that can be downloaded for you (and upgraded/changed later if you want) or you can specify your own binary.

Igor Matuszewski (May 11 2020 at 18:03, on Zulip):

https://github.com/rust-lang/rls-vscode/pull/793

Igor Matuszewski (May 11 2020 at 18:04, on Zulip):

It also supports passing rust-analyzer-specific options although they're not yet typed out in package.json for the user

Igor Matuszewski (May 11 2020 at 18:05, on Zulip):

I'd appreciate taking a look <3 If there won't be anything critical, I'd like to do that in a follow-up PR though

Coenen Benjamin (May 11 2020 at 18:11, on Zulip):

I checked the PR and I'm wondering for how long we will use the "experimental" wording when we talk about rust-analyzer ? Is RFC a goal to not use "experimental" anymore ?

Igor Matuszewski (May 12 2020 at 06:07, on Zulip):

I meant to emphasize that rust-analyzer support in rls-vscode is experimental in itself

Igor Matuszewski (May 12 2020 at 06:08, on Zulip):

That’s because wcustom commands like applySourceChange are not implemented, somewhat limiting the practical capabilities of the tool

Last update: Oct 28 2020 at 18:15UTC