joaotavora / eglot

A client for Language Server Protocol servers
GNU General Public License v3.0
2.26k stars 200 forks source link

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

Closed narendraj9 closed 3 years ago

narendraj9 commented 4 years ago

The following warning appears when rust-analyzer LSP server starts up with eglot. It seems that eglot assumes that the server cannot send a notification of type textDocument/didSave.

[client-notification] Thu Aug 27 23:21:00 2020:
(:jsonrpc "2.0" :method "workspace/didChangeConfiguration" :params
          (:settings nil))
[server-request] (id:0) Thu Aug 27 23:21:00 2020:
(:jsonrpc "2.0" :id 0 :method "client/registerCapability" :params
          (:registrations
           [(:id "textDocument/didSave" :method "textDocument/didSave" :registerOptions
                 (:documentSelector
                  [(:pattern "**/*.rs")
                   (:pattern "**/Cargo.toml")
                   (:pattern "**/Cargo.lock")]
                  :includeText :json-false))]))
[client-reply] (id:0) Thu Aug 27 23:21:00 2020:
(:jsonrpc "2.0" :id 0 :result nil)
[server-request] (id:1) Thu Aug 27 23:21:00 2020:
(:jsonrpc "2.0" :id 1 :method "workspace/configuration" :params
          (:items
           [(:section "rust-analyzer")]))
[client-reply] (id:1) Thu Aug 27 23:21:00 2020:
(:jsonrpc "2.0" :id 1 :result
          [nil])
[server-notification] Thu Aug 27 23:21:15 2020:
(:jsonrpc "2.0" :method "textDocument/publishDiagnostics" :params
          (:diagnostics
           []
           :uri "file:///home/narendraj9/code/rust-analyzer/xtask/src/main.rs" :version 0))
rodelrod commented 3 years ago

Does anyone have a workaround for this? I can't use eglot on rust because of this (most important functionality seems to be inaccessible)/

joaotavora commented 3 years ago

Thanks for pinging. Eglot is short on hands-on-deck and I didn't notice so many "thumbs up" going up there. I'll see what I can do.

joaotavora commented 3 years ago

In this case, what's missing is the dynamic registration of capabilities, i.e. the possibility to write the eglot--capabilities slot of the server object based on these notifications. I don't know if the server should be sending these notifications since Eglot, the client, doesn't report that it supports "dynamic registration" in the first place. But I'm not 100% on this.

Anyway, it's not very hard to add this feature, on just has to know where to stop. For example, here, the dynamic registration of the didSave capability has some fancy registerOptions:

(:jsonrpc "2.0" :id 0 :method "client/registerCapability" :params
          (:registrations
           [(:id "textDocument/didSave" :method "textDocument/didSave" :registerOptions
                 (:documentSelector
                  [(:pattern "**/*.rs")
                   (:pattern "**/Cargo.toml")
                   (:pattern "**/Cargo.lock")]
                  :includeText :json-false))]))

Given this notification, Eglot should indeed register that the server understands "textDocument/didSave", but ignore that it only wnats "didSave" for those files or that :includeText is false. I believe Eglot will work fine without it, and we can always add support for it later.

EDIT: the above reasoning is mostly wrong. Eglot is free to state that it does not support dynamic registration of this capability, and it already states support for its capabilities in the client initialize request.

joaotavora commented 3 years ago

This is likely on the rust analyzer side. It should not be sending this request for a dynamic registration of textDocument/didSave because Eglot is telling it explicitly that it does not support dynamic registration of such types. This is easy to see in this transcript

(:jsonrpc "2.0" :id 1 :method "initialize" :params
          (:processId 191483 :rootPath "/home/capitaomorte/Source/Rust/" :rootUri "file:///home/capitaomorte/Source/Rust/" :initializationOptions #s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8125 data
                                                                                                                                                                ())
                      :capabilities
                      (:workspace
                       (:applyEdit t :executeCommand
                                   (:dynamicRegistration :json-false)
                                   :workspaceEdit
                                   (:documentChanges :json-false)
                                   :didChangeWatchedFiles
                                   (:dynamicRegistration t)
                                   :symbol
                                   (:dynamicRegistration :json-false)
                                   :configuration t)
                       :textDocument
                       (:synchronization
                        (:dynamicRegistration :json-false :willSave t :willSaveWaitUntil t :didSave t)

dynamicRegistration of client sub-features in the :synchronization group is explicitly set to json-false which will translate to false in the actual JSON. Conversation. Thus, an issue should be opened with the rust-analyzer folks (and maybe linked back to this comment).

In the meantime, I will try to come up with a workaround

joaotavora commented 3 years ago

OK so finally I tested with. I did this by installing a minimal Rust toolkit and the rust-analyser binary. I'm trying with a very simple project:

❯ tree hello
hello
|-- src/
|   `-- main.rs
|-- Cargo.lock
`-- Cargo.toml

1 directory, 3 files

Then I visit the src/main.rs and type C-u M-x eglot RET rust-analyser RET. It does print the warning, which I've found to be mostly benign (more about that later).

It seems to work fine. Therefore I don't understand the problems reported in this issue exactly. Is it just that people are annoyed by seeing the warning. Someone (@rodelrod ), seems to say that this inhibits important functionality.

However, if I do this before a commit that I've recentlyu pushed to Eglot, nothing interesting will happen. This is because Eglot was incorrectly having some RLS (the other RustLang server) kicking in with the C-u M-x eglot use case. @rodelrod were you using C-u M-x???

Anyway, if you don't have this commit (which will be available shortly), you need to explicitly:

(add-to-list 'eglot-server-programs '(rust-mode . ("rust-analyzer")))

Somewhere in your .emacs, but that is something you'll already be doing probably if you're thinking of switching to this server.

joaotavora commented 3 years ago

It does print the warning, which I've found to be mostly benign (more about that later).

Why did I say this? Well, my theory is that rust-analyser is using the dynamic registration of the textDocument/didSave capability to achieve the exact same objective that the other RustLang server (rls ) sought to achieve with the dynamic registration of the textDocument/didChangeWatchedFiles capability. That objective is to be notified of changes not only to .rs files (which are set to rust-mode in Emacs) but also the accompanying Cargo.toml files (which are not managed by that mode).

Thus I believe it might make sense to put this feature into Eglot, allowing dynamic registration of textDocument/didSave with the specific parameters requested by rust-analyser. It might even make sense, probably, to make rust-analyser the default Rust server for Eglot.

Anyway, I stress that this is a feature, and not a bug. It's a very interesting feature, but, if anything, the bug exists in rust-analysers's side, which shouldn't be sending this "illegal" dynamic registration.

rodelrod commented 3 years ago

@joaotavora Thanks for looking into this. I have to apologize for being misleading. Given your analysis and some more time that I've spent on this issue, I believe that at the time of my comment I was misattributing my problems to this warning.

To give you some context, several things were broken in my system at that moment. I knew very little about LSP and just wanted to quickly set up a Rust environment while learning the very basics of the language. I'm on Doom Emacs and just adding the (rust +lsp) didn't result in a working environment so I rushed into a doom upgrade that broke multiple unrelated things and launched me into a multi-hour rabbit hole.

By the point I was researching this issue, the situation was that flycheck was not signaling any syntax checking warnings or errors although I was seeing them being communicated by rust-analyzer in the eglot buffer. Since I was expecting these errors to show up every time I saved the file — and not knowing much about LSP — I wrongly assumed that the registration warning meant that eglot was not receiving the error messages that were being sent. .

Now I believe that the problem is in the interaction between flycheck and eglot or Doom's default setup of these packages.

  1. When I first open a file in a Rust project, flycheck is activated by default with the eglot checker, I do not see any errors either in the buffer or in the flycheck error list.
  2. If I change the file, save, disable flycheck and re-enable flycheck, I start seeing the errors in the buffer, but they still do not show in the flycheck error list.
  3. If I select the rustic-clippy checker, disable flycheck and re-enable flycheck (changing the file not required), I see the errors both in the buffer and in the error list
  4. If I disable flycheck, enable flymake and change the buffer (saving to the file not required), I see errors both in the buffer and in the error list.

Is flymake recommended over flycheck to use with eglot? Should I take this problem up with Doom?

joaotavora commented 3 years ago

launched me into a multi-hour rabbit hole.

Definitely know that feeling.

Is flymake recommended

Slightly so, yes. Though it you have Flycheck configured correctly to use a special flymake "checker" that adapts the content of Eglot's Flymake "backend", then it should be fine. No only in theory, but in practice as well. See https://github.com/joaotavora/eglot/issues/42#issuecomment-405655356 (and stick to that comment if you can, cause the rest of the issue is pretty nasty).

But clearly, Eglot is built to work with "nothing but Emacs" (or rather "nothing but Emacs master" or "nothing but Emacs non-master, and some packages developed in Emacs master".). So, with Flymake being part of Emacs, I design Eglot targetting Flymake.

Should I take this problem up with Doom?

Maybe, but I must say I also feel for Doom's maintainer.

Anyway, it seems you want to use LSP for some Rust-checking things and rustic-clippy for others. Even if it means selecting either one or the other, i recommend you stick to one tool only. My personal tentency is generally "stay as close to Emacs as possible", which normally equates to "have as little code running as possible". But if this "rustic" thing works for you, then never mind Eglot.

PuercoPop commented 3 years ago

Just a side-note

the bug exists in rust-analysers's side, which shouldn't be sending this "illegal" dynamic registration.

It did it. But it should be fixed now as a PR addressing it was merged ~6 months ago. See https://github.com/rust-analyzer/rust-analyzer/pull/5516

joaotavora commented 3 years ago

But it should be fixed now as a PR addressing it was merged ~6 months ago. See rust-analyzer/rust-analyzer#5516

Skimming that discussion, seems like that PR was reverted for whatever reason.

joaotavora commented 3 years ago

Seems the bug in fixed on rust-analyzer side

rodelrod commented 3 years ago

Anyway, it seems you want to use LSP for some Rust-checking things and rustic-clippy for others.

Please don't overestimate my sophistication on this topic :smile:. Before this, I'd never heard of rustic or rust-mode and couldn't tell flymake from flycheck; so I don't really care which one is doing the work as long as Emacs tells me when I mess up my Rust code. I'm just starting with the language and that tends to help! rustic-clippy is just one of the many things I tried in my quest for a working setup.

You (correctly) closed this issue as it refers to rust-analyzer. Thanks for moving things on that front!

As for my eglot/flycheck problem, I found — both through the Doom discord and reading through the flycheck issue that you mention — that this is a known issue in Doom that has been discussed lately. I don't grasp all the details but they have pinged you for an opinion on a draft PR to fix the issue.

I'll figure out how to override flycheck with flymake in Doom while waiting for that PR to progress.

joaotavora commented 3 years ago

I mess up my Rust code. I'm just starting with the language and that tends to help!

To tell you the truth, the Eglot project came to being when I was starting on Rust myself, specifically with that goal. Then I figured I had to rewrite Flymake for a proper Eglot client. Then Eglot became a thing of its own and I never really learned Rust. (so you were talking about rabbit holes earlier... hehehe)

If you want to get some progress, my view is that you either make sure that Doom's defaults are sane for you or that you switch away from Doom into vanilla Emacs, spend some time learning its shortcuts (it takes a week or so), and don't install many other packages.

Of course many people hold the opposite view and are scared/turned off by vanilla Emacs's apparent lack of bells and whistles. But in my experience it does make things nicer. And at any rate I applaud Doom's effort to be less bloaty than, say Spacemacs. But it's still a heavy layer of configuration that I cannot unravel (i don't have time to).

I'll figure out how to override flycheck with flymake in Doom while waiting for that PR to progress.

It's strange that Flycheck needs to be overridden because Flycheck is the external Emacs package, whereas Flymake is in Emacs. But I don't grasp Flymake details.

They have pinged me on the issue but I cannot answer, since it's limited to project collaborators, and I'm not one of them. :shrug: