Skip to content

Commit

Permalink
Revert "lsp: Do not notify all language servers on file save" (zed-in…
Browse files Browse the repository at this point in the history
…dustries#19183)

Reverts zed-industries#17756. According to the existing
implementations of the LSP specification, namely
[Helix](https://github.com/helix-editor/helix/blob/a7651f5bf027ec98645d571ab05a685d97e1b772/helix-view/src/document.rs#L1038)
and, if I'm not wrong,
[VSCode](https://github.com/microsoft/vscode-languageserver-node/blob/main/client/src/common/textSynchronization.ts#L580),
`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](scalameta/metals-zed#28 (comment))
for example

Closes zed-industries#18636

Release Notes:

- N/A
  • Loading branch information
WeetHet authored Oct 16, 2024
1 parent 1286198 commit eddf70b
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 63 deletions.
16 changes: 0 additions & 16 deletions crates/project/src/lsp_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2900,27 +2900,11 @@ impl LspStore {
let file = File::from_dyn(buffer.read(cx).file())?;
let worktree_id = file.worktree_id(cx);
let abs_path = file.as_local()?.abs_path(cx);
let worktree_path = file.as_local()?.path();
let text_document = lsp::TextDocumentIdentifier {
uri: lsp::Url::from_file_path(abs_path).log_err()?,
};

let watched_paths_for_server = &self.as_local()?.language_server_watched_paths;
for server in self.language_servers_for_worktree(worktree_id) {
let should_notify = maybe!({
Some(
watched_paths_for_server
.get(&server.server_id())?
.read(cx)
.worktree_paths
.get(&worktree_id)?
.is_match(worktree_path),
)
})
.unwrap_or_default();
if !should_notify {
continue;
}
if let Some(include_text) = include_text(server.as_ref()) {
let text = if include_text {
Some(buffer.read(cx).text())
Expand Down
55 changes: 8 additions & 47 deletions crates/project/src/project_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,34 +385,6 @@ async fn test_managing_language_servers(cx: &mut gpui::TestAppContext) {

// A server is started up, and it is notified about Rust files.
let mut fake_rust_server = fake_rust_servers.next().await.unwrap();
fake_rust_server
.request::<lsp::request::RegisterCapability>(lsp::RegistrationParams {
registrations: vec![lsp::Registration {
id: Default::default(),
method: "workspace/didChangeWatchedFiles".to_string(),
register_options: serde_json::to_value(
lsp::DidChangeWatchedFilesRegistrationOptions {
watchers: vec![
lsp::FileSystemWatcher {
glob_pattern: lsp::GlobPattern::String(
"/the-root/Cargo.toml".to_string(),
),
kind: None,
},
lsp::FileSystemWatcher {
glob_pattern: lsp::GlobPattern::String(
"/the-root/*.rs".to_string(),
),
kind: None,
},
],
},
)
.ok(),
}],
})
.await
.unwrap();
assert_eq!(
fake_rust_server
.receive_notification::<lsp::notification::DidOpenTextDocument>()
Expand Down Expand Up @@ -460,24 +432,6 @@ async fn test_managing_language_servers(cx: &mut gpui::TestAppContext) {

// A json language server is started up and is only notified about the json buffer.
let mut fake_json_server = fake_json_servers.next().await.unwrap();
fake_json_server
.request::<lsp::request::RegisterCapability>(lsp::RegistrationParams {
registrations: vec![lsp::Registration {
id: Default::default(),
method: "workspace/didChangeWatchedFiles".to_string(),
register_options: serde_json::to_value(
lsp::DidChangeWatchedFilesRegistrationOptions {
watchers: vec![lsp::FileSystemWatcher {
glob_pattern: lsp::GlobPattern::String("/the-root/*.json".to_string()),
kind: None,
}],
},
)
.ok(),
}],
})
.await
.unwrap();
assert_eq!(
fake_json_server
.receive_notification::<lsp::notification::DidOpenTextDocument>()
Expand Down Expand Up @@ -528,7 +482,7 @@ async fn test_managing_language_servers(cx: &mut gpui::TestAppContext) {
)
);

// Save notifications are reported only to servers that signed up for a given extension.
// Save notifications are reported to all servers.
project
.update(cx, |project, cx| project.save_buffer(toml_buffer, cx))
.await
Expand All @@ -540,6 +494,13 @@ async fn test_managing_language_servers(cx: &mut gpui::TestAppContext) {
.text_document,
lsp::TextDocumentIdentifier::new(lsp::Url::from_file_path("/the-root/Cargo.toml").unwrap())
);
assert_eq!(
fake_json_server
.receive_notification::<lsp::notification::DidSaveTextDocument>()
.await
.text_document,
lsp::TextDocumentIdentifier::new(lsp::Url::from_file_path("/the-root/Cargo.toml").unwrap())
);

// Renames are reported only to servers matching the buffer's language.
fs.rename(
Expand Down

0 comments on commit eddf70b

Please sign in to comment.