Tracking issue fo RFC2912, transition to rust-analyzer as our official LSP

[matklad]

Issues blocking providing rust-analyzer as an alternative

• rust-analyzer should mostly conform to LSP protocol
  • specifically replace adHoc "rust-analyzer.applySourceChange" with less adHoc SnippetTextEdit
• rust-analyzer should be packaged with rustup
• rust-lang.rust extension should work with both rust-analyzer and rls
  • all the code from editors/code in this repo should be moved to https://github.com/rust-lang/rls-vscode

Issues blocking switching to rust-analyzer as the main implementation

• rust-analyzer should fully conform to LSP protocol

Issues under discussion

• on the fly checking
• RAM usage
• startup time

[eminence]

Could someone briefly describe how RLS does on-the-fly checking? I know that rust-analyzer on-the-fly checking will be implemented in its own way, but knowing how RLS does it I think could be some useful background knowledge for this thread.

[bjorn3]

RLS acts as a custom rustc driver and uses an internal compiler api (FileLoader) to provide sources directly to the parser without reading from the disk. The main problem with this, it that it is tied to a specific rustc version.

[puremourning]

specifically, "rust-analyzer.applySourceChange" command should be opt-in

I don’t think opt-in is the way to go. Just implement these executeCommand for it like the protocol says. You can still extend the response for your vscode-only cursor position thing.

[bstaletic]

To expand on what @puremourning said, making it opt-in would leave out a lot of code actions for clients who don't explicitly opt-in. I strongly believe rust-analyzer can do better.

Instead of making rust-analyzer.applySourceChange opt-in, let's just (or "just") make rust-analyzer handle executeCommand properly. If a server sends a CodeAction object with a 'command' field, that server should be ready to receive the same command through executeCommand request. Then the server is supposed to make a series of applyEdit requests, to let the client consume the chosen Command. After all of the applyEdit requests were, well, applied and the client responded, the server needs to send a response to executeCommand, which can simply be null.

The reason I think this shouldn't be hard, is because rust-analyzer.applySourceChange is already a single WorkspaceEdit (with an extra field that can be ignored by the clients) which is exactly what should be in an applyEdit request.

In other words:

1. Client makes a textDocument/codeAction request.
2. rust-analyzer responds with a CodeAction, containing a rust-analyzer.applySourceChange command.
3. Clients would have a choice at this point.
   3.1. VSCode, for example, may continue to handle rust-analyzer.applySourceChange in a special way, as it needs fewer messages and thus provides better latency.
   3.2. Clients that prefer protocol conformance over latency may choose to send an executeCommand request. Further steps only care about these clients.
4. Client sends an executeCommand request, containing rust-analyzer.applySourceChange.
5. rust-analyzer, before sending a response to executeCommand sends an applyEdit response.
   5.1. This is basically just taking the command[ 'arguments' ][ 0 ] and putting it in { 'edit': <here> }
6. The client responds with { 'applied': bool }
7. rust-analyzer responds with anything to the executeCommand request.

This makes clients who want to respect the standard and clients who want to cater to rust-analyzer special cases happy.

[matklad]

Yup, sorry, that was bad wording on my part. I definitely intend to make all code-actions available to all client, with cursor positioning being a sort of "progressive enhancement". I think the plan outlined in microsoft/language-server-protocol#724 (comment) should work.

[bstaletic]

I tried to implement what I was talking about in my previous comment, but I got stuck. First of all, tell me if you want me to create a separate thread for this.

I added the ExecuteCommand capability and am having troubles writing handlers::handle_execute_command. The problem is that, for step 5, I need:

1. Create a server->client request, before returning from the handler.
2. Block until the client has responded to the request.
3. Finally respond to workspace/executeCommand.

This is what I have written, in all its broken glory:

pub fn handle_execute_command(
    world: WorldSnapshot,
    params: req::ExecuteCommandParams,
) -> Result<Option<req::WorkspaceEdit>> {
    if params.command == "rust-analyzer.applySourceChange" {
        let arg = params.arguments[0];
        let edit : req::WorkspaceEdit = serde_json::from_value(arg).unwrap();
        if client_supports_apply_edits {
        let request_id = loop_state.next_request_id();
        let request = request_new::<req::ApplyWorkspaceEdit>(
            request_id.clone(),
            req::ApplyWorkspaceEditParams { edit: edit },
        );
        msg_sender.send(request.into())?;
        wait_for_response(); // Completely made up function name.
        return Ok(None)
        } else {
            // For clients who do not support `ApplyWorkspaceEdit` requests
            return Ok(Some(edit))
        }
    }
    return Ok(None)
}

Things I need:

1. client_supports_apply_edits: basically, client capabilities, in order to be 100% conforming.
2. loop_state.next_request_id(), request_new and msg_sender, in order to send the request. I'm not sure if I can/should just import main_loop::request_new in handlers.rs.
3. Some way to block until I get a response for the request I just made.

[matklad]

@bstaletic please note that we probably we'll change the precise interface here soon (I plan to overhaul this stuff next week, but it might spill longer than that), so it probably doesn't make sense to invest too much time into this right now. I'll take a closer look tomorrow, but my first thought is that I am not too excited about using server -> client applyEdit command, as it's impossible to ensure consistency with it, and it looses causality: microsoft/language-server-protocol#663 (comment)

Regarding your approach, the current architecture doesn't really allow to block until a part of protocol is executed --- we kind of just have a giant explicit state machine in the main loop, and handle all events manually. I'd probably just reply to the command right away, without waiting for the client to acknowledge applyWorkspaceEdit -- the client doesn't really have a way to piece the puzzle together and realise that applyWorkspaceEdit is connected to the executeCommand.

[puremourning]

the client doesn't really have a way to piece the puzzle together and realise that applyWorkspaceEdit is connected to the executeCommand.

yet, it must. in order for clients to behave sensibly (causality as you put it), we must assume that any applyEdit received twix'd an executeCommand and its reply are associated with that command.

Indeed that's exactly what we do, blocking the user while waiting for the command reply and collecting edits. This works as far as we can tell for all servers so far.

[Xanewok]

We can somewhat tick the blocking issue box:

rust-lang.rust extension should work with both rust-analyzer and rls

Implementation

Also posted it in the RFC thread: rust-lang/rfcs#2912 (comment)

It doesn't provide exact feature parity since it doesn't work yet with messages such as rust-analyzer.applySourceChange and other custom extensions that are not LSP-proposed.

[matklad]

#4494 implements code action snippet capability.

[matklad]

I've transitioned all code actions to use progressive enhancement, so code actions in next release should just work with any any editor which implements codeActionLiteralSupport. We do not implement command-based actions, as those require custom client-side plumbing or round-about workspace/applyEdit. This is conforming to the protocol (we just don't emit code actions without codeActionLiteralSupport) and i think should be fine in practice: if client supports workspace/applyEdit, it shouldnt't be problematic to support suppling edit directly with code-actions. I'll mark some comments in this thread as resolved.

I've also started documenting all our protocol extensions in this document.

At this point, although we are not fully confirming to the protocol yet, I believe we've solved all major practical conformance issues.

[matklad]

Next step, shipping with rustup, hit an unexpected snag:

rust-lang/rust#72978 (comment)

We require too recent glibc (2.9) for file watching. Specifically, this function is not available on Rust CI builders: https://github.com/hannobraun/inotify-sys/blob/155435810601d422ccec540d5eaf90728d60ff15/src/lib.rs#L618.

I'll look into this by making file watching optional or some such, but that's going to be a big change, so it's unlikely to land very soon. I am curious if anyone can come up with some horrible cute linker trick to work-around this. Can we somehow poly-fill this symbol or avoid using it altogether?

cc @hannobraun just in case :-)

[hannobraun]

Please correct me if I'm wrong, but it looks like ra_vfs depends on notify, which depends on inotify, which depends on inotify-sys. This would mean, the problem is this call: https://github.com/hannobraun/inotify/blob/23f7ec656b8dbf573783b70d38c9988537aebf3f/src/inotify.rs#L101

If inotify_init1 is indeed the only problem here, then this should be straight-forward to fix: Call inotify_init instead, then set the flags in a second call with fcntl (example). notify depends on inotify 0.7, so it would be necessary to also backport this change and do another 0.7.x release.

I will try to look into this soon, but if anyone wants to do the legwork here (making and testing the change), this would be highly welcome.