Rust Analyzer is not notified of changes to Cargo.toml

[lelgenio]

One of the most annoying things about writing rust with kak-lsp is that Rust Analyzer does not reload the list of dependencies when they are added to Cargo.toml.

How to reproduce my issue:

• Create new rust project: cargo new my_test_proj
• Open main.rs with kakoune: cd my_test_proj; kak ./src/main.rs
• Add some dependency (either cargo add or manually editing Cargo.toml): cargo add rand
• Try to use new dependency
• Notice it's not suggested on completion, and you cannot go to definitions
• Notice information from cargo check does appear as inline errors/warnings

Who should notify who?

Doing some research on the topic I found a claim by a RA developer that the burden of notifying the language server (RA) of changes to Cargo.toml was on the language client (kak-lsp in our case). I tried to find that comment again but was not successful, I think it was in a discussion about sublime text.

But looking at it now, it seems RA was supposed to do this itself, and just work, and well, It does not: https://github.com/rust-lang/rust-analyzer/pull/9426

A curious workaround

Adding toml files to the list of filetypes for rust-analyzer allows it to pick up on changes to Cargo.toml, but it also tries to check against rust syntax, so not usable.

 [language.rust]
-filetypes = ["rust"]
+filetypes = ["rust", "toml"]
 roots = ["rust-toolchain.toml", "rust-toolchain", "Cargo.toml"]
 command = "rust-analyzer"

[krobelus]

Very cool stuff. Looks like we can support this by supporting dynamic registration of textDocument/didSave. rust-analyzer registers to receive this notification for **/*.rs and **/Cargo.toml. When enabling lsp global (lsp-enable), the behavior makes sense to me, we just need to forward Cargo.toml. I'm not sure how this should interact when using lsp-enable-window. If a user only runs lsp-enable-window for Rust files, should we still activate a textDocument/didSave notifications for Cargo.toml? I guess so. This will be ugly but doable.

Obviously, another workaround is to restart rust-analyzer. I have mappings to enable/disable lsp so restarting is somewhat of a bad habit of mine

[sclu1034]

The textDocument/didSave notification would only help with manually editing Cargo.toml. In my usage, most changes come via cargo add or cargo update (which changes Cargo.lock instead), and those would still require RA to be able to watch those files.

[krobelus]

Looks like the VSCode watches for external changes to Cargo.toml (even if that file is never opened in VSCode), so it works with cargo add.

So what LSP wants us to do is create watchers for **/*.Cargo.toml and translate change events into textDocument/didSave. We can use notify, perhaps including it in the controller process's event loop.

[sclu1034]

It's not just Cargo.toml. cargo update changes Cargo.lock, and should be handled just as well. RA waits for notifications on both.

And I believe workspace/didChangeWatchedFiles is the correct notification for those file watchers that aren't triggered by the user hitting "Save".

[krobelus]

Good point. When I turn on the capability for didChangeWatchedFiles, rust-analyzer asks for both, see below.

I guess we're supposed to send workspace/didChangeWatchedFiles only for external modifications and textDocument/didSave for whenever the user runs :write. If that's true, this is quite an awkward split. Due to kak-lsp's architecture, it takes us extra work to

• on save, not send a redundant workspace/didChangeWatchedFiles
  • but still when saving Cargo.toml do send textDocument/didSave (even when lsp-enable-window is used)

My initial draft will just always send workspace/didChangeWatchedFiles. I'm sure we can find a clever way to suppress redundant notifications.

{
  "jsonrpc": "2.0",
  "id": 0,
  "method": "client/registerCapability",
  "params": {
    "registrations": [
      {
        "id": "textDocument/didSave",
        "method": "textDocument/didSave",
        "registerOptions": {
          "includeText": false,
          "documentSelector": [
            {
              "pattern": "**/*.rs"
            },
            {
              "pattern": "**/Cargo.toml"
            },
            {
              "pattern": "**/Cargo.lock"
            }
          ]
        }
      }
    ]
  }
}
{
  "jsonrpc": "2.0",
  "id": 3,
  "method": "client/registerCapability",
  "params": {
    "registrations": [
      {
        "id": "workspace/didChangeWatchedFiles",
        "method": "workspace/didChangeWatchedFiles",
        "registerOptions": {
          "watchers": [
            {
              "globPattern": "/home/johannes/git/kak-lsp/**/*.rs"
            },
            {
              "globPattern": "/home/johannes/git/kak-lsp/**/Cargo.toml"
            },
            {
              "globPattern": "/home/johannes/git/kak-lsp/**/Cargo.lock"
            }
          ]
        }
      }
    ]
  }
}

[krobelus]

This should be fixed in the file-watch branch which also fixes similar scenarios like when a source file is added or renamed. Sadly, watching a large directory can be expensive. We add the watches in a background thread so it shouldn't impact the user too much. However it's still ugly and inefficient.

I wonder if we should just not watch files ignored by Git (via the ignore crate). That might remedy most problems. It's wrong in some cases - if the user wants to edit a temporary crate that is gitignored - but those scenarios should be rare enough?

I believe VSCode has a config option that's a list of paths to exclude from search and probably also change-watching. In practice this usually ends up containing the same information is .gitignore. Kakoune has an option ignored_files but it's not used for excluding node_modules and such.

kak-lsp shouldn't make creative decisions in general but this one might be worth it, let's see.

[sclu1034]

For the duplicate notifications, it might be enough to employ a simple debounce per path. I.e. whenever either notification is sent, store the path in a ttl_cache. When sending a notification, check the cache first.

For the directory watching:

.gitignore (or any other XXXignore, because none are specifically intended for Rust/Cargo) should not be honored. It is a perfectly valid use case to have a source file that is ignored, but still relevant to the build. E.g. a secrets.rs for which a dummy version was committed, then ignored to not leak secrets. Or I might have a directory of generated sources I want to ignore in source control and my searches (.fdignore) but still handled by tooling. We could add our own ignore config option, but that might not be needed.

I'd assume that the vast majority of projects use the common directory layout, so we could limit our globs to those directories. We could also parse Cargo.toml for target auto-discovery settings and for additional targets and their paths. Or we could add an include config option for users that do have a non-standard layout.

[krobelus]

Or I might have a directory of generated sources

fair enough

For the duplicate notifications, it might be enough to employ a simple debounce per path. I.e. whenever either notification is sent, store the path in a ttl_cache. When sending a notification, check the cache first.

If I save a file in my editor, there's probably a race between the editor hook (lsp-did-save) and the inotify watch. I imagine we want to not send workspace/didChangeWatchedFiles when we send textDocument/didChange. This is not specified in LSP. I guess we can check what VSCode does. Because of the race, this is awkward to implement. However it probably doesn't matter because

1. the two notifications are probably functionally equivalent, so it doesn't matter which one we end up sending
2. we can add a small delay to watched files. Technically not a fireproof solution but good enough in practice

I'd assume that the vast majority of projects use the common directory layout, so we could limit our globs to those directories. We could also parse Cargo.toml for target auto-discovery settings and for additional targets and their paths.

Performance issues are probably rare for Rust projects since Rust workspaces tend to be reasonably sized (?). Anyway I agree that a good way to reduce the notification traffic is to look closer at the build configuration. However we should not have Rust-specific workarounds in kak-lsp (except in kak-lsp.toml). Ideally rust-analyzer takes care of this. They could give use more narrow directory cones to watch, for example /path/to/project/src/**.rs instead of /path/to/project/**.rs.

Or we could add an include config option for users that do have a non-standard layout.

Yeah we could add this and set it to ["src", "benches", ...] by default for Rust.

I was more worried about non-rust projects but it looks like most language servers do not request workspace/didChangeWatchedFiles notifications.