Revert "lsp: Do not notify all language servers on file save"

[WeetHet]

Reverts #17756. According to the existing implementations of the LSP specification, namely Helix and, if I'm not wrong, VSCode, textDocument/didSave has nothing to do with the watched files and should be sent to the language servers connected to the buffers even if the files are not watched by those. As the LSP spec doesn't say anything about didSave being related to the watched files, and the reference implementation in VSCode seemingly does not filter the notifications according to those, it seems like this is an incorrect interpretation of the specification

This also causes issues with language servers. See Metals issue for example

Closes #18636

Release Notes:

• N/A

[zed-industries-bot]

	Messages
📖	This PR includes links to the following GitHub Issues: #scalameta/metals-zed#28 If this PR aims to close an issue, please include a Closes #ISSUE line at the top of the PR body.

Generated by 🚫 dangerJS against aa5a59d

[WeetHet]

@osiewicz, no rush, but if you could tell me what the opinion of the team on this matter is, it would give me an idea of what to do next. I'm currently diving into Scala, and using another editor after Zed doesn't feel great 😔

[osiewicz]

Hey, I am a bit busy right now, but I've marked myself as a reviewer as I was involved in both the original PR and the followup discussion.
From my POV, I don't mind either implementation, but do keep in mind that the original PR was intended to fix our integration with rust-analyzer; therefore, I don't think solving this issue is going to be as simple as reverting a PR. Basically, you either break Scala support or Rust support; one of them has to adjust (I don't think we should be special-casing our behavior depending on the server we're working on).

[tgodzik]

Wasn't the Rust Analyzer actually fixed for that case? rust-lang/rust-analyzer@772acef

I will also try to reuse client file watching for .scala files once I have time to look at it, but since the assumptions are not really a LSP standard I am not sure if this will fix things with 100% certainty.

[osiewicz]

@tgodzik yeah well, they've silenced the error so that it doesn't bring down the server. Still, it would be good (IMHO) to estabilish a common ground per this post.

[tgodzik]

Wait, actually Rust Analyzer is doing the right thing, it's using https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocumentRegistrationOptions which has nothing to do with https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeWatchedFiles

There actually is the specific option for didSave and if it's null then everything should be sent

[osiewicz]

@tgodzik sweet, that settles it then. That means that Zed is the the one changing. I'll work up a PR to support filtering based on document selectors, as it seems like we don't have that at all right now.
Thank you for your persistence. I hope I didn't come across as too much of a PITA. :)

[tgodzik]

I was mostly surprised, I haven't figured out the actual problem earlier. I did spend a lot of time in that spec already 😅 It made a lot of sense to have an option to declare supported files and it turns out there is!

[WeetHet]

I was mostly surprised, I haven't figured out the actual problem earlier. I did spend a lot of time in that spec already 😅 It made a lot of sense to have an option to declare supported files and it turns out there is!

Yeah, I somehow missed it too, even though I've seen it and had it in the back of my mind but for some reason the discussion in the LSP repo made me think otherwise

[osiewicz]

@WeetHet since today is a release day and RA should not error too much after this revert, I'm going to merge your PR and work on a follow-up separately. Thanks!