lsp: Exclude dynamic settings from LanguageServerSeed identity (#47376)

Shuhei Kadowaki created 2 months ago

`LanguageServerSeed` is used as a key to identify language servers.
Previously, it included the entire `LspSettings`, which meant that
changing `lsp.<server>.settings` (dynamic configuration) would cause the
server to restart unnecessarily.

Dynamic settings can be updated via LSP's
`workspace/didChangeConfiguration` notification without requiring a
server restart. Only `binary` and `initialization_options` should be
part of the server identity, as changes to these genuinely require
restarting the server.

This is a follow-up fix to #35270 which introduced `LanguageServerSeed`
but inadvertently included dynamic settings in the server identity
(although I remember that this dynamic settings reflection stopped
working pretty recently, so there might be other commits besides #35270
that changed the behavior of `LanguageServerSeed`)

Closes #ISSUE

Release Notes:

- Fixed language servers unnecessarily restarting when changing
`lsp.<server>.settings` configuration. Dynamic settings are now properly
updated via `workspace/didChangeConfiguration` without requiring a
server restart.

Change summary

crates/project/src/lsp_store.rs                 | 27 ++++++++++++++++--
crates/project/src/manifest_tree/server_tree.rs | 12 ++++---
crates/project/src/project_settings.rs          |  1 
3 files changed, 31 insertions(+), 9 deletions(-)

Detailed changes

crates/project/src/lsp_store.rs

@@ -36,7 +36,7 @@ use crate::{
         ManifestTree,
     },
     prettier_store::{self, PrettierStore, PrettierStoreEvent},
-    project_settings::{LspSettings, ProjectSettings},
+    project_settings::{BinarySettings, LspSettings, ProjectSettings},
     toolchain_store::{LocalToolchainStore, ToolchainStoreEvent},
     trusted_worktrees::{PathTrust, TrustedWorktrees, TrustedWorktreesEvent},
     worktree_store::{WorktreeStore, WorktreeStoreEvent},
@@ -226,12 +226,22 @@ struct UnifiedLanguageServer {
     project_roots: HashSet<Arc<RelPath>>,
 }
 
+/// Settings that affect language server identity.
+///
+/// Dynamic settings (`LspSettings::settings`) are excluded because they can be
+/// updated via `workspace/didChangeConfiguration` without restarting the server.
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
+struct LanguageServerSeedSettings {
+    binary: Option<BinarySettings>,
+    initialization_options: Option<serde_json::Value>,
+}
+
 #[derive(Clone, Debug, Hash, PartialEq, Eq)]
 struct LanguageServerSeed {
     worktree_id: WorktreeId,
     name: LanguageServerName,
     toolchain: Option<Toolchain>,
-    settings: Arc<LspSettings>,
+    settings: LanguageServerSeedSettings,
 }
 
 #[derive(Debug)]
@@ -332,7 +342,10 @@ impl LocalLspStore {
         let key = LanguageServerSeed {
             worktree_id: worktree_handle.read(cx).id(),
             name: disposition.server_name.clone(),
-            settings: disposition.settings.clone(),
+            settings: LanguageServerSeedSettings {
+                binary: disposition.settings.binary.clone(),
+                initialization_options: disposition.settings.initialization_options.clone(),
+            },
             toolchain: disposition.toolchain.clone(),
         };
         if let Some(state) = self.language_server_ids.get_mut(&key) {
@@ -5052,7 +5065,13 @@ impl LspStore {
                             let key = LanguageServerSeed {
                                 worktree_id,
                                 name: disposition.server_name.clone(),
-                                settings: disposition.settings.clone(),
+                                settings: LanguageServerSeedSettings {
+                                    binary: disposition.settings.binary.clone(),
+                                    initialization_options: disposition
+                                        .settings
+                                        .initialization_options
+                                        .clone(),
+                                },
                                 toolchain: local.toolchain_store.read(cx).active_toolchain(
                                     path.worktree_id,
                                     &path.path,

crates/project/src/manifest_tree/server_tree.rs

@@ -421,11 +421,13 @@ impl ServerTreeRebase {
                     .and_then(|worktree_nodes| worktree_nodes.roots.get(&disposition.path.path))
                     .and_then(|roots| roots.get(&disposition.server_name))
                     .filter(|(old_node, _)| {
-                        (&disposition.toolchain, &disposition.settings)
-                            == (
-                                &old_node.disposition.toolchain,
-                                &old_node.disposition.settings,
-                            )
+                        // Only compare settings that require server restart.
+                        // Dynamic settings (settings.settings) can be updated via DidChangeConfiguration
+                        // without restarting the server.
+                        disposition.toolchain == old_node.disposition.toolchain
+                            && disposition.settings.binary == old_node.disposition.settings.binary
+                            && disposition.settings.initialization_options
+                                == old_node.disposition.settings.initialization_options
                     })
                 else {
                     return Some(node);

crates/project/src/project_settings.rs

@@ -17,6 +17,7 @@ use rpc::{
 };
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
+pub use settings::BinarySettings;
 pub use settings::DirenvSettings;
 pub use settings::LspSettings;
 use settings::{