Better LSP conformance #5516

vsrs · 2020-07-24T11:26:08Z

At the moment rust-analyzer does not fully conform to the LSP. This PR fixes two LSP related issues:

rust-analyzer sends predefined server capabilities and does not take supplied client capabilities in mind.
rust-analyzer uses dynamic textDocument/didSave registration even if the client does not support it.

crates/rust-analyzer/src/caps.rs

kjeremy · 2020-07-24T13:38:19Z

crates/rust-analyzer/src/main_loop.rs

-            |_, _| (),
-        );
+        if self.config.client_caps.dynamic_watched_files {
+            let save_registration_options = lsp_types::TextDocumentSaveRegistrationOptions {


Why do we only register for didSave if the client is watching the files? Aren't they separate things?

If the client sets the workspace/didChangeWatchedFiles/dynamicRegistration capability to false it means that the server cannot use client/registerCapability for workspace/didChangeWatchedFiles, doesn't it?

I think so... but textDocument/didSave isn't controlled by workspace/didChangeWatchedFiles. I'm not sure of the relationship here.

Yes, you are absolutely right, this is my mistake. I cannot understand how I managed to confuse different capabilities.
I already asked to rollback the PR.

The spec is incredibly confusing around this sort of thing :( and you are correct in that we are not checking static/dynamic registrations correctly.

I think that this and the diagnostics bug I probably introduced a few days ago (and the call hierarchy issue I fixed awhile back) highlight the need for more precise tests on our LSP layer.

@olegtk I tried Preview5. Same problem (textDocument/didOpen goes before initialized):

C->S: {"jsonrpc":"2.0","id":2,"method":"initialize","params":{...}} S->C: {"jsonrpc":"2.0","id":2,"result":{...,"serverInfo":{"name":"rust-analyzer","version":"57b4ec4"}} C->S: {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{...}} C->S: {"jsonrpc":"2.0","method":"initialized","params":{}}

I've tried to catch textDocument/didOpen via ILanguageClientMiddleLayer, wait until the server is fully initialized (ILanguageClient.OnServerInitializedAsync called) and resend didOpen notification. It works, but right after textDocument/didOpen VS sends textDocument/documentSymbol (also before initialized) and I did not find a way to catch and delay this request (ILanguageClientMiddleLayer does not work).

Hmm, that's not a known issue... can you please point to your ILanguageClient implementation?

I don't have a fully functional ILanguageClient, just a quick sketch (enough to reproduce didOpen\initialized on my system):

public class ContentDefinition { public const string RustContentTypeName = "Rust"; [Export] [Name(RustContentTypeName)] [BaseDefinition(CodeRemoteContentDefinition.CodeRemoteContentTypeName)] internal static ContentTypeDefinition RustContentTypeDefinition { get; } [Export] [FileExtension(".rs")] [ContentType(RustContentTypeName)] internal static FileExtensionToContentTypeDefinition RustFileExtensionDefinition { get; } } [Export(typeof(ILanguageClient))] [ContentType(ContentDefinition.RustContentTypeName)] public class Client : ILanguageClient{ public string Name => "VSRust"; public IEnumerable<string> ConfigurationSections => null; public object InitializationOptions => null; public IEnumerable<string> FilesToWatch => null; public event AsyncEventHandler<EventArgs> StartAsync; public event AsyncEventHandler<EventArgs> StopAsync; public async Task<Connection> ActivateAsync(CancellationToken token) { await Task.Yield(); var info = new ProcessStartInfo { FileName = @"E:\Projects\Rust\rust-analyzer\target\debug\rust-analyzer.exe", Arguments = "", RedirectStandardInput = true, RedirectStandardOutput = true, UseShellExecute = false, CreateNoWindow = true }; info.Environment.Add("RA_WAIT_DBG", "1"); info.Environment.Add("RA_LOG", "1"); var process = new Process { StartInfo = info }; if( process.Start() ) { return new Connection(process.StandardOutput.BaseStream, process.StandardInput.BaseStream); } return null; } public async Task OnLoadedAsync() { var tmp = StartAsync; if( tmp != null ) { await tmp.InvokeAsync(this, EventArgs.Empty); } } public Task OnServerInitializedAsync() { return Task.CompletedTask; } public Task OnServerInitializeFailedAsync(Exception e) { return Task.CompletedTask; } }

@olegtk Perhaps it's not the best place to continue discussion. In case you need more details please drop me an email to [email protected].

Actually you are right, @vsrs, it's a bug in VS LSP client. It's tracked now internally. Thanks!

matklad · 2020-07-24T13:53:25Z

bors r+

bors · 2020-07-24T14:00:30Z

Build succeeded:

vsrs · 2020-07-24T14:17:35Z

bors r+

@matklad I'm sorry but looks like I messed up registerCapability calls for workspace/didChangeWatchedFiles and textDocument/didSave. Could you please rollback this PR entirely.

@kjeremy Thanks for noticing this!

This reverts commit 5b13c24, reversing changes made to c3defe2.

matklad · 2020-07-24T14:28:41Z

No worries, reverted!

vsrs · 2020-07-24T14:55:21Z

No worries, reverted!

Thanks.

joaotavora · 2021-01-10T18:23:12Z

Just noting this is an actual problem in joaotavora/eglot#526. Though I'm thinking of starting to support this dynamic registration anyway, maybe other clients suffer from the same problem.

By the way, what is the purpose of this dynamic registration? Is it to check for changes to Cargo.toml & friends?

cc rust-lang#5516 (comment)

matklad · 2021-01-10T19:55:31Z

yup, def our bug, fixed in #7241

We only use dynamic registration stuff for didSave (to reload cargo.toml) and watched files (to notice changes to .rs after branch switches and such). tbh, I don't understand why the dynamic infra exists in the first place -- this two cases could have been just a separate "watch these files" request, but instead they are handled via this nested RPC contraption :-(

{
    "jsonrpc":"2.0",
    "id":2,
*   "method":"client/registerCapability", 
    "params":{
        "registrations":[{
            "id":"workspace/didChangeWatchedFiles",
*           "method":"workspace/didChangeWatchedFiles",
            "registerOptions":{"watchers":[{"globPattern":"/home/matklad/tmp/hello/**/*.rs"}]}
        }]
    }
}

joaotavora · 2021-01-10T20:03:37Z

Thanks!

I don't know what you mean by "dynamic infra". Am I to understand that you are now sending a request for workspace/didChangeWatchedFiles? If so, that's how RLS does it (and my client Eglot supports that).

matklad · 2021-01-10T20:16:22Z

I don't know what you mean by "dynamic infra".

The whole "dynamic registration of capabilities" part of the lsp -- we don't use dynamic registration anywhere else, and I don't see why we would ever want to.

We have to use it here though, because there's no other way to enable these notifications.

Before #7241, we were sending these two registrations unconditionally (which violates the protocol). Now we only send client/registerCapability if the corresponding dynamic_registration property is set to true in client's capabilites.

joaotavora · 2021-01-10T20:40:09Z

The whole "dynamic registration of capabilities" part of the lsp -- we don't use dynamic registration anywhere else, and I don't see why we would ever want to.

Ah right, that makes sense. Yes, I agree with you. I also find that part of LSP kind of strange/over-engineered (tho I might be missing something).

We have to use it here though, because there's no other way to enable these notifications.

Right, and the way that RLS does it is to dynamically register the capability for the workspace/didChangeWatchedFiles method. It does this with suitable arguments, thereby asking the client to watch these file's changes in the file system.

Eglot supports that and reports that it supports dynamicRegistration for the didChangeWatchedFiles method specifically.

EDIT: my question is: rust-analyser now registering for didChangeWatchedFiles dynamically (versus registering for :textDocument/didSave)????

Now we only send client/registerCapability if the corresponding dynamic_registration property is set to true in client's capabilites.

Right. That is a correct fix, regardless of anything else.

matklad · 2021-01-12T10:00:26Z

I also find that part of LSP kind of strange/over-engineered (tho I might be missing something).

I am fairly confident that it is indeed strange, based both on my personal experience developing IDEs, and on studding the Dart protocol: https://htmlpreview.github.io/?https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/doc/api.html

cc rust-lang#5516 (comment)

vsrs added 3 commits July 24, 2020 13:13

Prepare server capabilities based on client ones.

594ce72

Send dynamic didSave only if the client supports

f195f87

Code formatting

57b4ec4

vsrs changed the title ~~Sc caps~~ Better LSP conformance Jul 24, 2020

matklad reviewed Jul 24, 2020

View reviewed changes

crates/rust-analyzer/src/caps.rs Outdated Show resolved Hide resolved

crates/rust-analyzer/src/caps.rs Outdated Show resolved Hide resolved

kjeremy reviewed Jul 24, 2020

View reviewed changes

crates/rust-analyzer/src/caps.rs Outdated Show resolved Hide resolved

kjeremy mentioned this pull request Jul 24, 2020

Is the client supposed to ignore unknown server caps? microsoft/language-server-protocol#1050

Closed

Always send experimental & semantic server caps.

a84dc44

kjeremy reviewed Jul 24, 2020

View reviewed changes

bors bot merged commit 5b13c24 into rust-lang:master Jul 24, 2020

matklad added a commit that referenced this pull request Jul 24, 2020

Revert "Merge #5516"

14a3a71

This reverts commit 5b13c24, reversing changes made to c3defe2.

PuercoPop mentioned this pull request Jan 10, 2021

[eglot] (warning) Server tried to register unsupported capability `textDocument/didSave' | with rust-analyzer joaotavora/eglot#526

Closed

matklad added a commit to matklad/rust-analyzer that referenced this pull request Jan 10, 2021

Honor client's dynamic registration caps

bb453ed

cc rust-lang#5516 (comment)

ivan770 pushed a commit to ivan770/rust-analyzer that referenced this pull request Jan 22, 2021

Honor client's dynamic registration caps

5536d23

cc rust-lang#5516 (comment)

Matthias-Fauconneau pushed a commit to Matthias-Fauconneau/rust-analyzer that referenced this pull request Feb 7, 2021

Honor client's dynamic registration caps

03c1241

cc rust-lang#5516 (comment)

clemenswasser mentioned this pull request May 4, 2021

Visual Studio integration #8729

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better LSP conformance #5516

Better LSP conformance #5516

vsrs commented Jul 24, 2020

kjeremy Jul 24, 2020

vsrs Jul 24, 2020

kjeremy Jul 24, 2020

vsrs Jul 24, 2020

kjeremy Jul 24, 2020

vsrs Jul 24, 2020

olegtk Jul 24, 2020

vsrs Jul 24, 2020

vsrs Jul 24, 2020

olegtk Jul 27, 2020

matklad commented Jul 24, 2020

bors bot commented Jul 24, 2020

vsrs commented Jul 24, 2020

matklad commented Jul 24, 2020

vsrs commented Jul 24, 2020

joaotavora commented Jan 10, 2021 •

edited

Loading

matklad commented Jan 10, 2021

joaotavora commented Jan 10, 2021

matklad commented Jan 10, 2021

joaotavora commented Jan 10, 2021 •

edited

Loading

matklad commented Jan 12, 2021

Better LSP conformance #5516

Better LSP conformance #5516

Conversation

vsrs commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Jul 24, 2020

bors bot commented Jul 24, 2020

vsrs commented Jul 24, 2020

matklad commented Jul 24, 2020

vsrs commented Jul 24, 2020

joaotavora commented Jan 10, 2021 • edited Loading

matklad commented Jan 10, 2021

joaotavora commented Jan 10, 2021

matklad commented Jan 10, 2021

joaotavora commented Jan 10, 2021 • edited Loading

matklad commented Jan 12, 2021

joaotavora commented Jan 10, 2021 •

edited

Loading

joaotavora commented Jan 10, 2021 •

edited

Loading