Skip to content

Better LSP conformance #5516

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 24, 2020
Merged

Better LSP conformance #5516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
594ce72
Prepare server capabilities based on client ones.
vsrs Jul 20, 2020
f195f87
Send dynamic didSave only if the client supports
vsrs Jul 24, 2020
57b4ec4
Code formatting
vsrs Jul 24, 2020
a84dc44
Always send experimental & semantic server caps.
vsrs Jul 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/rust-analyzer/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ pub struct ClientCapsConfig {
pub hover_actions: bool,
pub status_notification: bool,
pub signature_help_label_offsets: bool,
pub dynamic_watched_files: bool,
}

impl Config {
Expand Down Expand Up @@ -290,6 +291,13 @@ impl Config {
}

pub fn update_caps(&mut self, caps: &ClientCapabilities) {
if let Some(ws_caps) = caps.workspace.as_ref() {
if let Some(did_change_watched_files) = ws_caps.did_change_watched_files.as_ref() {
self.client_caps.dynamic_watched_files =
did_change_watched_files.dynamic_registration.unwrap_or(false);
}
}

if let Some(doc_caps) = caps.text_document.as_ref() {
if let Some(value) = doc_caps.definition.as_ref().and_then(|it| it.link_support) {
self.client_caps.location_link = value;
Expand Down
67 changes: 35 additions & 32 deletions crates/rust-analyzer/src/main_loop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,38 +106,41 @@ impl GlobalState {
);
};

let save_registration_options = lsp_types::TextDocumentSaveRegistrationOptions {
include_text: Some(false),
text_document_registration_options: lsp_types::TextDocumentRegistrationOptions {
document_selector: Some(vec![
lsp_types::DocumentFilter {
language: None,
scheme: None,
pattern: Some("**/*.rs".into()),
},
lsp_types::DocumentFilter {
language: None,
scheme: None,
pattern: Some("**/Cargo.toml".into()),
},
lsp_types::DocumentFilter {
language: None,
scheme: None,
pattern: Some("**/Cargo.lock".into()),
},
]),
},
};

let registration = lsp_types::Registration {
id: "textDocument/didSave".to_string(),
method: "textDocument/didSave".to_string(),
register_options: Some(serde_json::to_value(save_registration_options).unwrap()),
};
self.send_request::<lsp_types::request::RegisterCapability>(
lsp_types::RegistrationParams { registrations: vec![registration] },
|_, _| (),
);
if self.config.client_caps.dynamic_watched_files {
let save_registration_options = lsp_types::TextDocumentSaveRegistrationOptions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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].

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

include_text: Some(false),
text_document_registration_options: lsp_types::TextDocumentRegistrationOptions {
document_selector: Some(vec![
lsp_types::DocumentFilter {
language: None,
scheme: None,
pattern: Some("**/*.rs".into()),
},
lsp_types::DocumentFilter {
language: None,
scheme: None,
pattern: Some("**/Cargo.toml".into()),
},
lsp_types::DocumentFilter {
language: None,
scheme: None,
pattern: Some("**/Cargo.lock".into()),
},
]),
},
};

let registration = lsp_types::Registration {
id: "textDocument/didSave".to_string(),
method: "textDocument/didSave".to_string(),
register_options: Some(serde_json::to_value(save_registration_options).unwrap()),
};

self.send_request::<lsp_types::request::RegisterCapability>(
lsp_types::RegistrationParams { registrations: vec![registration] },
|_, _| (),
);
}

self.fetch_workspaces();

Expand Down