lsp: Do not notify all language servers on file save

[osiewicz]

This is not an ideal solution to https://github.com/fasterthanlime/zed-diags-readme, but current status quo is not great either; we were just going through all of the language servers and notifying them, whereas we should ideally do it based on a glob.
/cc @fasterthanlime

Release Notes:

• N/A

[fasterthanlime]

I do not fully understand this change nor exactly how the integration works, but I want to emphasize that rust analyzer should absolutely know about the change in the markdown file in this instance.

The markdown file does contain rust code blocks that rust analyzer needs to know about I’m assuming.

[osiewicz]

It's a bit more subtle; I don't think we spawn a separate language server instance in this case (for . md file) and the existing Rust instance does not care about Markdown files, as it does not register for watching of .md files.

This is a can of worms. ૮꒰꩜ ᯅ ꩜;꒱ა

[fasterthanlime]

Mhhh, I see.

for the record the original error I saw in the logs was something like “got diagnostics for path/to/README.md but this path isn’t in VFS” or something.

when making the repro I got a different error (the r-a panic)

[Veykril]

The panic in r-a happens because it receives an invalid path for the on save handler, it only registers rust and toml files for it but zed incorrectly gives it other files, violating the LSP (see #17376). We fixed panic behavior in r-a (it now logs an error) but this still needs fixing in zed.

[fasterthanlime]

but zed incorrectly gives it other files

Okay but there is Rust code in the markdown file.

The markdown file is included as the “doc” attribute of the .rs file, and there’s runnable doc tests in there, hence the diagnostics etc.

Shouldn’t r-a care about this markdown file?

[osiewicz]

I think there are 3 issues at play:

1. Zed sends out file save notifications on paths RA didn't ask for.
2. RA does not notice that a change in include_str'd file should be causing a rebuild of the project.
3. Zed doesn't support completions in Rust snippets in .md files.

I think 1. is fully on Zed, no question asked; I'd say 2 is on RA side. Whether .md file contains Rust code or not seems orthogonal to the issue?
3. seems like a joint effort; RA shouldn't care about parsing markdown files and what-not. I imagine Zed should treat each code snippet in a .md file as a separate (or maybe not?) unnamed buffer and work with that. For it to happen though, RA needs to support unnamed buffers in the first place.

[Veykril]

but zed incorrectly gives it other files

Okay but there is Rust code in the markdown file.

The markdown file is included as the “doc” attribute of the .rs file, and there’s runnable doc tests in there, hence the diagnostics etc.

Shouldn’t r-a care about this markdown file?

Two things:

1. r-a is not able to expand macro invocations in builtin attributes right now -> Support macro expansion inside attribute rust-lang/rust-analyzer#8092
2. r-a's VFS does not currently handle files other than .rs and .toml files, so even if 1) worked it would still not include the readme -> include!ing files without .rs suffix does not work rust-lang/rust-analyzer#10178

Shouldn’t r-a care about this markdown file?

So to answer, yes, r-a should kind of care (in that it should track the content of the file changing), but it won't ever care about markdown files being saved. (there is a different notification for file updates that would be relevant).

[filipwiech]

@osiewicz While the topic of Rust snippets in Markdown files seems more complex and would require further design and work, the changes in this PR still appear to make sense and be generally better than the current state of things. Would it make sense to go ahead and merge it, to avoid the most obvious issues and to improve the situation for all language servers (fix LSP violation)? 🙂

[osiewicz]

@filipwiech yeah agree, we should not creep the scope; I think the solution in this PR might actually undernotify in certain cases (e.g. I'm pretty sure it won't notify RA about changes to .toml files), so I'll hack on it some more on the plane today.

[filipwiech]

@osiewicz Sorry to bother you, but it appears that this PR has broken support for Scala projects (with Scala extension, https://github.com/scalameta/metals-zed). After saving files with the Scala source code in the properly imported project LSP server doesn't get notified about the changes. AFAIK Metals (the LSP provider) does support the didChangeWatchedFiles client capability, so not sure what's going on here, maybe the watchers are not being correctly registered. 🙁

EDIT:
From the official documentation it seems that Metals registers only some special nice-to-have globs as part of the watchers, see here for more details: https://scalameta.org/metals/docs/integrations/new-editor#workspacedidchangewatchedfiles. So the "normal" source code files are still expected to be sent by the editor, even though they are not explicitly included in the registered paths. 🙁

Maybe the previous approach using the language_servers_for_buffer could still be combined with the paths watched by the server, as a fallback? 🤔