From 91832c8cd8de4743a5c8dad87005a67d9601d7e5 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 11 Jul 2023 13:20:02 +0300 Subject: [PATCH 1/2] Fix language servers improper restarts Language servers mixed `initialization_options` from hardcodes and user settings, fix that to ensure we restart servers on their settings changes only. --- crates/language/src/language.rs | 28 +++++++++++++++++++++++++++- crates/project/src/project.rs | 17 +++++------------ 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/crates/language/src/language.rs b/crates/language/src/language.rs index e8450344b8..d186bf630d 100644 --- a/crates/language/src/language.rs +++ b/crates/language/src/language.rs @@ -90,7 +90,8 @@ pub struct LanguageServerName(pub Arc); /// once at startup, and caches the results. pub struct CachedLspAdapter { pub name: LanguageServerName, - pub initialization_options: Option, + initialization_options: Option, + initialization_overrides: Mutex>, pub disk_based_diagnostic_sources: Vec, pub disk_based_diagnostics_progress_token: Option, pub language_ids: HashMap, @@ -109,6 +110,7 @@ impl CachedLspAdapter { Arc::new(CachedLspAdapter { name, initialization_options, + initialization_overrides: Mutex::new(None), disk_based_diagnostic_sources, disk_based_diagnostics_progress_token, language_ids, @@ -208,6 +210,30 @@ impl CachedLspAdapter { ) -> Option { self.adapter.label_for_symbol(name, kind, language).await } + + pub fn update_initialization_overrides(&self, new: Option<&Value>) -> bool { + let mut current = self.initialization_overrides.lock(); + if current.as_ref() != new { + *current = new.cloned(); + true + } else { + false + } + } + + pub fn initialization_options(&self) -> Option { + let initialization_options = self.initialization_options.as_ref(); + let override_options = self.initialization_overrides.lock().clone(); + match (initialization_options, override_options) { + (None, override_options) => override_options, + (initialization_options, None) => initialization_options.cloned(), + (Some(initialization_options), Some(override_options)) => { + let mut initialization_options = initialization_options.clone(); + merge_json_value_into(override_options, &mut initialization_options); + Some(initialization_options) + } + } + } } pub trait LspAdapterDelegate: Send + Sync { diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 81db0c7ed7..dc4c8852dd 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -78,8 +78,8 @@ use std::{ use terminals::Terminals; use text::Anchor; use util::{ - debug_panic, defer, http::HttpClient, merge_json_value_into, - paths::LOCAL_SETTINGS_RELATIVE_PATH, post_inc, ResultExt, TryFutureExt as _, + debug_panic, defer, http::HttpClient, paths::LOCAL_SETTINGS_RELATIVE_PATH, post_inc, ResultExt, + TryFutureExt as _, }; pub use fs::*; @@ -800,7 +800,7 @@ impl Project { .lsp .get(&adapter.name.0) .and_then(|s| s.initialization_options.as_ref()); - if adapter.initialization_options.as_ref() != new_lsp_settings { + if adapter.update_initialization_overrides(new_lsp_settings) { language_servers_to_restart.push((worktree, Arc::clone(language))); } } @@ -2545,20 +2545,13 @@ impl Project { let project_settings = settings::get::(cx); let lsp = project_settings.lsp.get(&adapter.name.0); let override_options = lsp.map(|s| s.initialization_options.clone()).flatten(); - - let mut initialization_options = adapter.initialization_options.clone(); - match (&mut initialization_options, override_options) { - (Some(initialization_options), Some(override_options)) => { - merge_json_value_into(override_options, initialization_options); - } - (None, override_options) => initialization_options = override_options, - _ => {} - } + adapter.update_initialization_overrides(override_options.as_ref()); let server_id = pending_server.server_id; let container_dir = pending_server.container_dir.clone(); let state = LanguageServerState::Starting({ let adapter = adapter.clone(); + let initialization_options = adapter.initialization_options(); let server_name = adapter.name.0.clone(); let languages = self.languages.clone(); let language = language.clone(); From 748e7af5a2c77e572474d836f9a4292dfd589780 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 11 Jul 2023 17:06:02 +0300 Subject: [PATCH 2/2] Add a test --- crates/editor/src/editor_tests.rs | 160 +++++++++++++++++++++++++- crates/editor/src/element.rs | 4 +- crates/editor/src/inlay_hint_cache.rs | 10 +- crates/language/src/language.rs | 6 + 4 files changed, 168 insertions(+), 12 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 9e726d6cc4..7b36287dca 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -22,7 +22,10 @@ use language::{ BracketPairConfig, FakeLspAdapter, LanguageConfig, LanguageRegistry, Point, }; use parking_lot::Mutex; +use project::project_settings::{LspSettings, ProjectSettings}; use project::FakeFs; +use std::sync::atomic; +use std::sync::atomic::AtomicUsize; use std::{cell::RefCell, future::Future, rc::Rc, time::Instant}; use unindent::Unindent; use util::{ @@ -1796,7 +1799,7 @@ async fn test_newline_comments(cx: &mut gpui::TestAppContext) { "}); } // Ensure that comment continuations can be disabled. - update_test_settings(cx, |settings| { + update_test_language_settings(cx, |settings| { settings.defaults.extend_comment_on_newline = Some(false); }); let mut cx = EditorTestContext::new(cx).await; @@ -4546,7 +4549,7 @@ async fn test_document_format_during_save(cx: &mut gpui::TestAppContext) { assert!(!cx.read(|cx| editor.is_dirty(cx))); // Set rust language override and assert overridden tabsize is sent to language server - update_test_settings(cx, |settings| { + update_test_language_settings(cx, |settings| { settings.languages.insert( "Rust".into(), LanguageSettingsContent { @@ -4660,7 +4663,7 @@ async fn test_range_format_during_save(cx: &mut gpui::TestAppContext) { assert!(!cx.read(|cx| editor.is_dirty(cx))); // Set rust language override and assert overridden tabsize is sent to language server - update_test_settings(cx, |settings| { + update_test_language_settings(cx, |settings| { settings.languages.insert( "Rust".into(), LanguageSettingsContent { @@ -7084,6 +7087,142 @@ async fn test_on_type_formatting_not_triggered(cx: &mut gpui::TestAppContext) { }); } +#[gpui::test] +async fn test_language_server_restart_due_to_settings_change(cx: &mut gpui::TestAppContext) { + init_test(cx, |_| {}); + + let language_name: Arc = "Rust".into(); + let mut language = Language::new( + LanguageConfig { + name: Arc::clone(&language_name), + path_suffixes: vec!["rs".to_string()], + ..Default::default() + }, + Some(tree_sitter_rust::language()), + ); + + let server_restarts = Arc::new(AtomicUsize::new(0)); + let closure_restarts = Arc::clone(&server_restarts); + let language_server_name = "test language server"; + let mut fake_servers = language + .set_fake_lsp_adapter(Arc::new(FakeLspAdapter { + name: language_server_name, + initialization_options: Some(json!({ + "testOptionValue": true + })), + initializer: Some(Box::new(move |fake_server| { + let task_restarts = Arc::clone(&closure_restarts); + fake_server.handle_request::(move |_, _| { + task_restarts.fetch_add(1, atomic::Ordering::Release); + futures::future::ready(Ok(())) + }); + })), + ..Default::default() + })) + .await; + + let fs = FakeFs::new(cx.background()); + fs.insert_tree( + "/a", + json!({ + "main.rs": "fn main() { let a = 5; }", + "other.rs": "// Test file", + }), + ) + .await; + let project = Project::test(fs, ["/a".as_ref()], cx).await; + project.update(cx, |project, _| project.languages().add(Arc::new(language))); + let (_, _workspace) = cx.add_window(|cx| Workspace::test_new(project.clone(), cx)); + let _buffer = project + .update(cx, |project, cx| { + project.open_local_buffer("/a/main.rs", cx) + }) + .await + .unwrap(); + let _fake_server = fake_servers.next().await.unwrap(); + update_test_language_settings(cx, |language_settings| { + language_settings.languages.insert( + Arc::clone(&language_name), + LanguageSettingsContent { + tab_size: NonZeroU32::new(8), + ..Default::default() + }, + ); + }); + cx.foreground().run_until_parked(); + assert_eq!( + server_restarts.load(atomic::Ordering::Acquire), + 0, + "Should not restart LSP server on an unrelated change" + ); + + update_test_project_settings(cx, |project_settings| { + project_settings.lsp.insert( + "Some other server name".into(), + LspSettings { + initialization_options: Some(json!({ + "some other init value": false + })), + }, + ); + }); + cx.foreground().run_until_parked(); + assert_eq!( + server_restarts.load(atomic::Ordering::Acquire), + 0, + "Should not restart LSP server on an unrelated LSP settings change" + ); + + update_test_project_settings(cx, |project_settings| { + project_settings.lsp.insert( + language_server_name.into(), + LspSettings { + initialization_options: Some(json!({ + "anotherInitValue": false + })), + }, + ); + }); + cx.foreground().run_until_parked(); + assert_eq!( + server_restarts.load(atomic::Ordering::Acquire), + 1, + "Should restart LSP server on a related LSP settings change" + ); + + update_test_project_settings(cx, |project_settings| { + project_settings.lsp.insert( + language_server_name.into(), + LspSettings { + initialization_options: Some(json!({ + "anotherInitValue": false + })), + }, + ); + }); + cx.foreground().run_until_parked(); + assert_eq!( + server_restarts.load(atomic::Ordering::Acquire), + 1, + "Should not restart LSP server on a related LSP settings change that is the same" + ); + + update_test_project_settings(cx, |project_settings| { + project_settings.lsp.insert( + language_server_name.into(), + LspSettings { + initialization_options: None, + }, + ); + }); + cx.foreground().run_until_parked(); + assert_eq!( + server_restarts.load(atomic::Ordering::Acquire), + 2, + "Should restart LSP server on another related LSP settings change" + ); +} + fn empty_range(row: usize, column: usize) -> Range { let point = DisplayPoint::new(row as u32, column as u32); point..point @@ -7203,7 +7342,7 @@ fn handle_copilot_completion_request( }); } -pub(crate) fn update_test_settings( +pub(crate) fn update_test_language_settings( cx: &mut TestAppContext, f: impl Fn(&mut AllLanguageSettingsContent), ) { @@ -7214,6 +7353,17 @@ pub(crate) fn update_test_settings( }); } +pub(crate) fn update_test_project_settings( + cx: &mut TestAppContext, + f: impl Fn(&mut ProjectSettings), +) { + cx.update(|cx| { + cx.update_global::(|store, cx| { + store.update_user_settings::(cx, f); + }); + }); +} + pub(crate) fn init_test(cx: &mut TestAppContext, f: fn(&mut AllLanguageSettingsContent)) { cx.foreground().forbid_parking(); @@ -7227,5 +7377,5 @@ pub(crate) fn init_test(cx: &mut TestAppContext, f: fn(&mut AllLanguageSettingsC crate::init(cx); }); - update_test_settings(cx, f); + update_test_language_settings(cx, f); } diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index fafbc33189..f0bae9533b 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -2916,7 +2916,7 @@ mod tests { use super::*; use crate::{ display_map::{BlockDisposition, BlockProperties}, - editor_tests::{init_test, update_test_settings}, + editor_tests::{init_test, update_test_language_settings}, Editor, MultiBuffer, }; use gpui::TestAppContext; @@ -3113,7 +3113,7 @@ mod tests { let resize_step = 10.0; let mut editor_width = 200.0; while editor_width <= 1000.0 { - update_test_settings(cx, |s| { + update_test_language_settings(cx, |s| { s.defaults.tab_size = NonZeroU32::new(tab_size); s.defaults.show_whitespaces = Some(ShowWhitespaceSetting::All); s.defaults.preferred_line_length = Some(editor_width as u32); diff --git a/crates/editor/src/inlay_hint_cache.rs b/crates/editor/src/inlay_hint_cache.rs index 70fb372504..52473f9971 100644 --- a/crates/editor/src/inlay_hint_cache.rs +++ b/crates/editor/src/inlay_hint_cache.rs @@ -847,7 +847,7 @@ mod tests { use text::Point; use workspace::Workspace; - use crate::editor_tests::update_test_settings; + use crate::editor_tests::update_test_language_settings; use super::*; @@ -1476,7 +1476,7 @@ mod tests { ), ] { edits_made += 1; - update_test_settings(cx, |settings| { + update_test_language_settings(cx, |settings| { settings.defaults.inlay_hints = Some(InlayHintSettings { enabled: true, show_type_hints: new_allowed_hint_kinds.contains(&Some(InlayHintKind::Type)), @@ -1520,7 +1520,7 @@ mod tests { edits_made += 1; let another_allowed_hint_kinds = HashSet::from_iter([Some(InlayHintKind::Type)]); - update_test_settings(cx, |settings| { + update_test_language_settings(cx, |settings| { settings.defaults.inlay_hints = Some(InlayHintSettings { enabled: false, show_type_hints: another_allowed_hint_kinds.contains(&Some(InlayHintKind::Type)), @@ -1577,7 +1577,7 @@ mod tests { let final_allowed_hint_kinds = HashSet::from_iter([Some(InlayHintKind::Parameter)]); edits_made += 1; - update_test_settings(cx, |settings| { + update_test_language_settings(cx, |settings| { settings.defaults.inlay_hints = Some(InlayHintSettings { enabled: true, show_type_hints: final_allowed_hint_kinds.contains(&Some(InlayHintKind::Type)), @@ -2269,7 +2269,7 @@ unedited (2nd) buffer should have the same hint"); crate::init(cx); }); - update_test_settings(cx, f); + update_test_language_settings(cx, f); } async fn prepare_test_objects( diff --git a/crates/language/src/language.rs b/crates/language/src/language.rs index d186bf630d..642f5469cd 100644 --- a/crates/language/src/language.rs +++ b/crates/language/src/language.rs @@ -453,6 +453,7 @@ fn deserialize_regex<'de, D: Deserializer<'de>>(d: D) -> Result, D #[cfg(any(test, feature = "test-support"))] pub struct FakeLspAdapter { pub name: &'static str, + pub initialization_options: Option, pub capabilities: lsp::ServerCapabilities, pub initializer: Option>, pub disk_based_diagnostics_progress_token: Option, @@ -1663,6 +1664,7 @@ impl Default for FakeLspAdapter { capabilities: lsp::LanguageServer::full_capabilities(), initializer: None, disk_based_diagnostics_progress_token: None, + initialization_options: None, disk_based_diagnostics_sources: Vec::new(), } } @@ -1712,6 +1714,10 @@ impl LspAdapter for Arc { async fn disk_based_diagnostics_progress_token(&self) -> Option { self.disk_based_diagnostics_progress_token.clone() } + + async fn initialization_options(&self) -> Option { + self.initialization_options.clone() + } } fn get_capture_indices(query: &Query, captures: &mut [(&str, &mut Option)]) {