From d95fc3442f29ccaecab270a914d3bf728be563cd Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:18:55 +0800 Subject: [PATCH] fix(lsp): owner-only sync, stale-edit checks, request timeouts, re-enable - Only the buffer that owns a document may use its server. The same file open in another window used to send its own text as didChange on F12, rename, references or signature help, swapping the document under the owner. LspStore::context now takes the requester and refuses a non-owner quietly, before anything is sent. - apply_workspace_edit checks every open buffer an edit touches against a baseline: the texts when a rename was asked for or the code-action menu was filled, or the text the server last heard for its own workspace/applyEdit (which then answers applied: false). A buffer typed in, opened or closed since means nothing is applied. - A request made after the server's output closed fails at once instead of waiting forever. Requests time out after 10 s (initialize after 60 s, shutdown after 2 s) and cancel themselves on the server. - Turning editor_lsp back on asks every window for its open files again, which starts their servers. Windows register how to be asked; closed ones are forgotten. --- src/ui/lsp/client.rs | 107 +++++++++++++-- src/ui/lsp/editor.rs | 18 ++- src/ui/lsp/locations.rs | 10 +- src/ui/lsp/mod.rs | 251 +++++++++++++++++++++++++++++----- src/ui/lsp/providers.rs | 37 ++++- src/ui/lsp/signature.rs | 7 +- src/ui/lsp/tests.rs | 280 ++++++++++++++++++++++++++++++++++++++ src/ui/settings/editor.rs | 5 +- 8 files changed, 656 insertions(+), 59 deletions(-) diff --git a/src/ui/lsp/client.rs b/src/ui/lsp/client.rs index 922cf189..f409a7e0 100644 --- a/src/ui/lsp/client.rs +++ b/src/ui/lsp/client.rs @@ -16,6 +16,7 @@ use std::path::Path; use std::process::{Child, Command, Stdio}; use std::sync::atomic::{AtomicI64, Ordering}; use std::sync::{Arc, Mutex, Weak}; +use std::time::Duration; use anyhow::{Context as _, anyhow}; use serde_json::Value; @@ -23,7 +24,18 @@ use serde_json::Value; use super::rpc::{self, FrameReader, Incoming, RequestId, ResponseError}; type Reply = smol::channel::Sender>; -type Pending = Arc>>; +/// Requests waiting for an answer. `None` once the server's output has +/// closed: nothing will ever be answered again, and a request made after +/// that fails at once instead of waiting forever. +type Pending = Arc>>>; + +/// How long an ordinary request waits for its answer. Everything the editor +/// asks is interactive — someone is waiting on a completion, a jump, a +/// rename — and a server that has not answered in this long is stuck. +pub(crate) const REQUEST_TIMEOUT: Duration = Duration::from_secs(10); + +/// `initialize` may index before answering; give it longer. +const INITIALIZE_TIMEOUT: Duration = Duration::from_secs(60); /// Something the server did that nobody asked for. #[derive(Debug)] @@ -121,7 +133,7 @@ impl LspClient { } })?; - let pending: Pending = Arc::default(); + let pending: Pending = Arc::new(Mutex::new(Some(HashMap::new()))); let (events_tx, events) = smol::channel::unbounded(); { let pending = pending.clone(); @@ -145,7 +157,11 @@ impl LspClient { }; let event = match message { Incoming::Response { id, result } => { - let reply = pending.lock().unwrap().remove(&id); + let reply = pending + .lock() + .unwrap() + .as_mut() + .and_then(|waiting| waiting.remove(&id)); if let Some(reply) = reply { let _ = reply.try_send(result); } @@ -167,7 +183,7 @@ impl LspClient { // Every request still waiting will never be answered: // dropping the senders fails them rather than leaving // them hanging. - pending.lock().unwrap().clear(); + pending.lock().unwrap().take(); let _ = events_tx.send_blocking(ServerEvent::Exited); })?; } @@ -211,7 +227,17 @@ impl LspClient { method: &str, params: Value, ) -> impl std::future::Future> + use<> { - self.request_inner(method, params, true) + self.request_inner(method, params, true, REQUEST_TIMEOUT) + } + + /// [`Self::request_raw`] with its own time limit. + pub(crate) fn request_raw_within( + self: &Arc, + method: &str, + params: Value, + timeout: Duration, + ) -> impl std::future::Future> + use<> { + self.request_inner(method, params, true, timeout) } /// `initialize`, which is the one request that goes past the gate. @@ -219,7 +245,7 @@ impl LspClient { self: &Arc, params: Value, ) -> impl std::future::Future> + use<> { - self.request_inner("initialize", params, false) + self.request_inner("initialize", params, false, INITIALIZE_TIMEOUT) } fn request_inner( @@ -227,15 +253,27 @@ impl LspClient { method: &str, params: Value, gated: bool, + timeout: Duration, ) -> impl std::future::Future> + use<> { let id = self.next_id.fetch_add(1, Ordering::Relaxed); let (tx, rx) = smol::channel::bounded(1); - self.pending.lock().unwrap().insert(id, tx); - self.send(rpc::encode(&rpc::request(id, method, params)), gated); + // Checked and registered under one lock: the reader closes the map + // under the same lock, so a request either makes it in before the + // server went away (and is failed with the rest) or sees it gone. + let registered = match self.pending.lock().unwrap().as_mut() { + Some(waiting) => { + waiting.insert(id, tx); + true + } + None => false, + }; + if registered { + self.send(rpc::encode(&rpc::request(id, method, params)), gated); + } let guard = CancelOnDrop { client: Arc::downgrade(self), id, - done: false, + done: !registered, }; let method = method.to_owned(); async move { @@ -243,7 +281,19 @@ impl LspClient { // capturing only `guard.done` would drop the guard — and cancel // the request — before it was ever sent an answer. let mut guard = guard; - let answer = rx.recv().await; + if !registered { + return Err(anyhow!("{method}: the language server has exited")); + } + let answer = smol::future::or(async { Some(rx.recv().await) }, async { + smol::Timer::after(timeout).await; + None + }) + .await; + // A timeout leaves `done` unset, so dropping the guard tells the + // server to stop working on it. + let Some(answer) = answer else { + return Err(anyhow!("{method}: no answer in {}s", timeout.as_secs_f32())); + }; guard.done = true; match answer { Ok(Ok(value)) => Ok(value), @@ -332,7 +382,13 @@ impl Drop for CancelOnDrop { let Some(client) = self.client.upgrade() else { return; }; - if client.pending.lock().unwrap().remove(&self.id).is_some() { + let removed = client + .pending + .lock() + .unwrap() + .as_mut() + .and_then(|waiting| waiting.remove(&self.id)); + if removed.is_some() { client.notify_raw("$/cancelRequest", serde_json::json!({ "id": self.id })); } } @@ -477,6 +533,35 @@ mod tests { )); } + #[test] + fn a_request_after_the_server_exited_fails_at_once() { + let (client, events, server) = connect(); + client.open_gate(); + drop(server); + assert!(matches!( + smol::block_on(events.recv()).unwrap(), + ServerEvent::Exited + )); + // No timeout involved: a limit of an hour would hang the test. + let late = + client.request_raw_within("textDocument/hover", Value::Null, Duration::from_secs(3600)); + let err = smol::block_on(late).unwrap_err(); + assert!(format!("{err:#}").contains("exited"), "{err:#}"); + } + + #[test] + fn a_request_nobody_answers_times_out_and_is_cancelled() { + let (client, _events, mut server) = connect(); + client.open_gate(); + let asked = client.request_raw_within("slow", Value::Null, Duration::from_millis(50)); + let sent = server.next(); + let err = smol::block_on(asked).unwrap_err(); + assert!(format!("{err:#}").contains("no answer"), "{err:#}"); + let cancel = server.next(); + assert_eq!(cancel["method"], "$/cancelRequest"); + assert_eq!(cancel["params"]["id"], sent["id"]); + } + #[test] fn dropping_an_unanswered_request_cancels_it() { let (client, _events, mut server) = connect(); diff --git a/src/ui/lsp/editor.rs b/src/ui/lsp/editor.rs index 26d5a355..3d501322 100644 --- a/src/ui/lsp/editor.rs +++ b/src/ui/lsp/editor.rs @@ -3,6 +3,7 @@ //! definition, format, rename), and the status bar's word on the server. use std::path::PathBuf; +use std::rc::Rc; use gpui::prelude::*; use gpui::{AnyElement, Context, Entity, WeakEntity, Window, div, px}; @@ -49,6 +50,14 @@ impl Tty7App { } else { Vec::new() }; + let app = cx.entity().downgrade(); + LspStore::register_window( + cx.entity_id(), + Rc::new(move |cx: &mut gpui::App| { + app.update(cx, |app, cx| app.lsp_sync_buffers(cx)).is_ok() + }), + cx, + ); LspStore::sync_window(cx.entity_id(), buffers, cx); } @@ -84,6 +93,7 @@ impl Tty7App { let text = f.input.read(cx).text().clone(); let doc = LspStore::context( &f.path, + f.input.entity_id(), Freshen::Text(&text), Some(window.window_handle()), cx, @@ -269,12 +279,16 @@ impl Tty7App { let text = f.input.read(cx).text().clone(); let Some(doc) = LspStore::context( &f.path, + f.input.entity_id(), Freshen::Text(&text), Some(window.window_handle()), cx, ) else { return; }; + // Every open buffer as the server sees it now; the answer is applied + // only if none of those it touches has changed by then. + let baseline = LspStore::snapshot(cx); let old_name = text .word_range(offset) .map(|r| text.slice(r).to_string()) @@ -293,7 +307,9 @@ impl Tty7App { cx.spawn_in(window, async move |_, cx| { let result = request.await; let _ = cx.update(|window, cx| match result { - Ok(Some(edit)) => super::apply_workspace_edit(edit, encoding, cx), + Ok(Some(edit)) => { + super::apply_workspace_edit(edit, encoding, Some(baseline), cx); + } Ok(None) => {} Err(e) => { let context = t_fmt(L10nKey::LspRenameFailed, &[("name", &old_name)]); diff --git a/src/ui/lsp/locations.rs b/src/ui/lsp/locations.rs index b931cdd3..1b391ad5 100644 --- a/src/ui/lsp/locations.rs +++ b/src/ui/lsp/locations.rs @@ -158,6 +158,7 @@ impl Tty7App { }; let Some(doc) = LspStore::context( &f.path, + f.input.entity_id(), Freshen::Text(&text), Some(window.window_handle()), cx, @@ -206,7 +207,8 @@ impl Tty7App { let Some(f) = self.editor_active_local_file() else { return; }; - let Some(doc) = LspStore::context(&f.path, Freshen::Skip, None, cx) else { + let Some(doc) = LspStore::context(&f.path, f.input.entity_id(), Freshen::Skip, None, cx) + else { return; }; if doc.caps.workspace_symbol_provider.is_none() { @@ -215,6 +217,7 @@ impl Tty7App { let app = cx.entity().downgrade(); let handle = window.window_handle(); let path = f.path.clone(); + let requester = f.input.entity_id(); let live: crate::ui::search::LiveQuery = std::rc::Rc::new(move |query: &str, cx: &mut App| { let (app, path, query) = (app.clone(), path.clone(), query.to_owned()); @@ -223,7 +226,7 @@ impl Tty7App { cx.defer(move |cx| { let _ = handle.update(cx, |_, window, cx| { let _ = app.update(cx, |app, cx| { - app.lsp_workspace_query(&path, query, window, cx) + app.lsp_workspace_query(&path, requester, query, window, cx) }); }); }); @@ -238,6 +241,7 @@ impl Tty7App { fn lsp_workspace_query( &mut self, path: &Path, + requester: gpui::EntityId, query: String, window: &mut Window, cx: &mut Context, @@ -252,7 +256,7 @@ impl Tty7App { } return; } - let Some(doc) = LspStore::context(path, Freshen::Skip, None, cx) else { + let Some(doc) = LspStore::context(path, requester, Freshen::Skip, None, cx) else { return; }; let client = doc.client.clone(); diff --git a/src/ui/lsp/mod.rs b/src/ui/lsp/mod.rs index 81dbbc05..f848647b 100644 --- a/src/ui/lsp/mod.rs +++ b/src/ui/lsp/mod.rs @@ -170,9 +170,16 @@ pub(crate) struct LspStore { docs: HashMap, missing: HashMap<&'static str, Instant>, next_generation: u64, + /// Each window editor's way to offer its buffers again, for when the + /// setting is turned back on. Returns `false` once the window is gone. + windows: HashMap, + /// `editor_lsp` as last seen, to tell a turn-on from any other change. + enabled: bool, _subscriptions: Vec, } +pub(crate) type Resync = Rc bool>; + impl Global for LspStore {} fn try_store_mut(cx: &mut App) -> Option<&mut LspStore> { @@ -199,15 +206,17 @@ impl LspStore { } let quit = cx.on_app_quit(|cx| LspStore::on_quit(cx)); let config = cx.observe_global::(|cx| { - if !cx.global::().editor_lsp - && cx - .try_global::() - .is_some_and(|s| !s.docs.is_empty()) - { - cx.update_global::(|store, cx| store.close_all(cx)); + let enabled = cx.global::().editor_lsp; + if cx.try_global::().is_none() { + return; } + cx.update_global::(|store, cx| store.set_enabled(enabled, cx)); }); + let enabled = cx + .try_global::() + .is_some_and(|c| c.editor_lsp); cx.set_global(LspStore { + enabled, _subscriptions: vec![quit, config], ..Default::default() }); @@ -220,6 +229,48 @@ impl LspStore { // ---- The editor's side ---- + /// Remembers how a window's editor offers its buffers, so turning the + /// setting back on can ask every window for them again. + pub(crate) fn register_window(app_id: EntityId, resync: Resync, cx: &mut App) { + Self::update(cx, |store, _| { + store.windows.entry(app_id).or_insert(resync); + }); + } + + /// The `editor_lsp` setting, as it changes. Off closes every document + /// and lets the servers go; on asks each window for its open buffers, + /// which starts them again — deferred, since the setting is changed from + /// inside some window's own update. + fn set_enabled(&mut self, enabled: bool, cx: &mut App) { + let was = std::mem::replace(&mut self.enabled, enabled); + if !enabled { + if !self.docs.is_empty() || !self.servers.is_empty() { + self.close_all(cx); + } + return; + } + if was { + return; + } + let windows: Vec<(EntityId, Resync)> = self + .windows + .iter() + .map(|(id, r)| (*id, r.clone())) + .collect(); + cx.defer(move |cx| { + let gone: Vec = windows + .into_iter() + .filter(|(_, resync)| !resync(cx)) + .map(|(id, _)| id) + .collect(); + if let Some(store) = try_store_mut(cx) { + for id in gone { + store.windows.remove(&id); + } + } + }); + } + /// Brings the documents one window's editor owns in line with the /// buffers it has: opens the new ones, closes the ones that went away or /// moved, and leaves the rest alone. Cheap when nothing changed. @@ -373,34 +424,57 @@ impl LspStore { /// The server behind `path`, with any edit still waiting sent first (see /// [`Freshen`]) so the request that follows sees the text the caller sees. + /// + /// Only the buffer that owns the document may ask (`requester` is its + /// `InputState`'s id). The same file open in a second window is a second + /// buffer whose text can differ from the owner's; letting it send that + /// text would swap the document under the owner, and letting it ask + /// against the owner's text would answer with positions for a text it + /// does not hold. So a non-owner is refused, quietly: that buffer simply + /// has no language features until the owner closes and it takes over. pub(crate) fn context( path: &Path, + requester: EntityId, sync: Freshen<'_>, window: Option, cx: &mut App, ) -> Option { cx.try_global::()?; Self::update(cx, |store, _| { - match sync { - Freshen::Text(text) => store.flush_text(path, text), - Freshen::Skip => {} - } - let doc = store.docs.get_mut(path)?; - if window.is_some() { - doc.window = window; - } - let server = store.servers.get(&doc.server)?; - if server.phase != Phase::Running { - return None; - } - Some(DocContext { - client: server.client.clone()?, - uri: doc.uri.clone(), - encoding: server.encoding, - caps: server.caps.clone(), - diagnostics: doc.diagnostics.clone(), - root: doc.server.root.clone(), - }) + store.context_for(path, requester, sync, window) + }) + } + + fn context_for( + &mut self, + path: &Path, + requester: EntityId, + sync: Freshen<'_>, + window: Option, + ) -> Option { + // Checked before anything is sent: a non-owner must never sync. + if self.docs.get(path)?.owner_id != requester { + return None; + } + match sync { + Freshen::Text(text) => self.flush_text(path, text), + Freshen::Skip => {} + } + let doc = self.docs.get_mut(path)?; + if window.is_some() { + doc.window = window; + } + let server = self.servers.get(&doc.server)?; + if server.phase != Phase::Running { + return None; + } + Some(DocContext { + client: server.client.clone()?, + uri: doc.uri.clone(), + encoding: server.encoding, + caps: server.caps.clone(), + diagnostics: doc.diagnostics.clone(), + root: doc.server.root.clone(), }) } @@ -849,8 +923,23 @@ impl LspStore { match serde_json::from_value::(params) { Ok(params) => { - apply_workspace_edit(params.edit, encoding, cx); - Ok(json!({ "applied": true })) + // Measured against what this server last + // heard: a buffer typed in since would take + // the edit at the wrong places. + let per_file = workspace_edit_files(params.edit); + match self.stale_target(&per_file, None, cx) { + Some(path) => Ok(json!({ + "applied": false, + "failureReason": format!( + "{} changed since the server last saw it", + path.display() + ), + })), + None => { + spawn_apply(per_file, encoding, cx); + Ok(json!({ "applied": true })) + } + } } Err(e) => { Ok(json!({ "applied": false, "failureReason": e.to_string() })) @@ -1018,13 +1107,10 @@ impl LspStore { let key = key.clone(); cx.spawn(async move |cx| { let executor = cx.background_executor().clone(); - let answered = client.request_raw("shutdown", Value::Null); - let timeout = executor.timer(Duration::from_secs(2)); - smol::future::or(async { answered.await.map(|_| ()).ok() }, async { - timeout.await; - None - }) - .await; + // Two seconds to say it is done; it is told to exit either way. + let _ = client + .request_raw_within("shutdown", Value::Null, Duration::from_secs(2)) + .await; client.notify_raw("exit", Value::Null); for _ in 0..20 { if client.reap_if_exited() { @@ -1189,12 +1275,105 @@ fn initialize_params(root: &Path, spec: &ServerSpec) -> Value { /// does, straight to disk otherwise. Run on a later turn of the event loop, /// never inside the update that asked for it — a buffer's own window may be /// the one being updated. +/// +/// `baseline` is what every open document held when the request that +/// produced the edit went out ([`LspStore::snapshot`]). If any buffer the +/// edit touches holds something else now — typed in, opened or closed since +/// — nothing at all is applied: half a rename is worse than none. Without a +/// baseline, the text the server last heard is the measure. Returns whether +/// the edit was applied. pub(crate) fn apply_workspace_edit( edit: lsp_types::WorkspaceEdit, encoding: Encoding, + baseline: Option>, cx: &mut App, -) { +) -> bool { let per_file = workspace_edit_files(edit); + let stale = cx + .try_global::() + .and_then(|store| store.stale_target(&per_file, baseline.as_ref(), cx)); + if let Some(path) = stale { + log::info!( + "lsp: not applying an edit: {} changed since it was asked for", + path.display() + ); + return false; + } + spawn_apply(per_file, encoding, cx); + true +} + +/// Whether every text in `now` is the one in `baseline`, for the files an +/// edit touches: the first that is not. A file open in only one of the two +/// was opened or closed in between, which counts as changed. +pub(crate) fn first_stale( + targets: &[PathBuf], + now: &HashMap, + baseline: &HashMap, +) -> Option { + targets + .iter() + .find(|path| match (now.get(*path), baseline.get(*path)) { + (Some(now), Some(then)) => now != then, + (None, None) => false, + _ => true, + }) + .cloned() +} + +impl LspStore { + /// What every open document's buffer holds right now — the baseline a + /// request that answers with edits is checked against. + pub(crate) fn snapshot(cx: &App) -> HashMap { + let Some(store) = cx.try_global::() else { + return HashMap::new(); + }; + store + .docs + .iter() + .filter_map(|(path, doc)| { + Some((path.clone(), doc.owner.upgrade()?.read(cx).text().clone())) + }) + .collect() + } + + fn stale_target( + &self, + per_file: &[(Uri, Vec)], + baseline: Option<&HashMap>, + cx: &App, + ) -> Option { + let targets: Vec = per_file + .iter() + .filter_map(|(u, _)| uri_to_path(u)) + .collect(); + let open = |path: &PathBuf| { + let doc = self.docs.get(path)?; + Some((doc, doc.owner.upgrade()?)) + }; + let now: HashMap = targets + .iter() + .filter_map(|p| Some((p.clone(), open(p)?.1.read(cx).text().clone()))) + .collect(); + let server_heard: HashMap; + let baseline = match baseline { + Some(baseline) => baseline, + None => { + server_heard = targets + .iter() + .filter_map(|p| { + let (doc, _) = open(p)?; + Some((p.clone(), doc.sent.clone().filter(|_| !doc.dirty)?)) + }) + .collect(); + &server_heard + } + }; + first_stale(&targets, &now, baseline) + } +} + +fn spawn_apply(per_file: Vec<(Uri, Vec)>, encoding: Encoding, cx: &mut App) { cx.spawn(async move |cx| { for (uri, edits) in per_file { let Some(path) = uri_to_path(&uri) else { diff --git a/src/ui/lsp/providers.rs b/src/ui/lsp/providers.rs index ed8aa4c0..fe19bf72 100644 --- a/src/ui/lsp/providers.rs +++ b/src/ui/lsp/providers.rs @@ -34,7 +34,13 @@ use crate::ui::app::Tty7App; const MAX_COMPLETIONS: usize = 300; pub(crate) struct LspProvider { + /// Every open buffer's text when the code-action menu was last filled: + /// an action picked from it is applied only if what it edits is + /// unchanged since. + actions_baseline: std::cell::RefCell>>, path: PathBuf, + /// The buffer this provider serves — always the document's owner. + input: gpui::EntityId, app: WeakEntity, } @@ -45,7 +51,9 @@ pub(super) fn install( cx: &mut App, ) { let provider = Rc::new(LspProvider { + actions_baseline: Default::default(), path: path.to_path_buf(), + input: input.entity_id(), app, }); input.update(cx, |state, cx| { @@ -69,7 +77,13 @@ pub(super) fn uninstall(input: &Entity, cx: &mut App) { impl LspProvider { fn context(&self, sync: Freshen<'_>, window: &Window, cx: &mut App) -> Option { - LspStore::context(&self.path, sync, Some(window.window_handle()), cx) + LspStore::context( + &self.path, + self.input, + sync, + Some(window.window_handle()), + cx, + ) } } @@ -143,7 +157,7 @@ impl CompletionProvider for LspProvider { ) -> Task> { // Resolving asks nothing of the text, which the caller may be in the // middle of changing: nothing is sent first. - let Some(doc) = LspStore::context(&self.path, Freshen::Skip, None, cx) else { + let Some(doc) = LspStore::context(&self.path, self.input, Freshen::Skip, None, cx) else { return Task::ready(Ok(item)); }; let resolvable = doc @@ -367,7 +381,7 @@ impl DefinitionProvider for LspProvider { return false; }; let position = location.target_selection_range.start; - let encoding = LspStore::context(&self.path, Freshen::Skip, None, cx) + let encoding = LspStore::context(&self.path, self.input, Freshen::Skip, None, cx) .map(|d| d.encoding) .unwrap_or_default(); let app = self.app.clone(); @@ -440,6 +454,7 @@ impl CodeActionProvider for LspProvider { if doc.caps.code_action_provider.is_none() { return not_available(); } + *self.actions_baseline.borrow_mut() = Some(LspStore::snapshot(cx)); let lsp_range = convert::offsets_to_lsp_range(&text, range, doc.encoding); // The diagnostics the range touches, which is what turns a server's // quick fixes on. @@ -501,6 +516,14 @@ impl CodeActionProvider for LspProvider { Some(lsp_types::CodeActionProviderCapability::Options(o)) if o.resolve_provider == Some(true) ); + // Taken when the menu was filled, which is also when an action that + // came with its edit was computed. Without one there is nothing to + // vouch for the open buffers, and an edit to any of them is refused. + let baseline = self + .actions_baseline + .borrow_mut() + .take() + .unwrap_or_default(); let client = doc.client.clone(); let encoding = doc.encoding; window.spawn(cx, async move |cx| { @@ -513,7 +536,13 @@ impl CodeActionProvider for LspProvider { action }; if let Some(edit) = action.edit { - cx.update(|_, cx| super::apply_workspace_edit(edit, encoding, cx))?; + let applied = cx.update(|_, cx| { + super::apply_workspace_edit(edit, encoding, Some(baseline), cx) + })?; + if !applied { + // Its command would act on the same stale ground. + return Ok(()); + } } if let Some(command) = action.command { // Whatever this changes comes back as `workspace/applyEdit`. diff --git a/src/ui/lsp/signature.rs b/src/ui/lsp/signature.rs index 4319f69f..9034d110 100644 --- a/src/ui/lsp/signature.rs +++ b/src/ui/lsp/signature.rs @@ -113,7 +113,8 @@ impl Tty7App { }; // Looked at without sending the edit: most keystrokes stop here, and // the server hears about them on the usual debounce. - let Some(doc) = LspStore::context(&f.path, Freshen::Skip, None, cx) else { + let Some(doc) = LspStore::context(&f.path, f.input.entity_id(), Freshen::Skip, None, cx) + else { return; }; let Some(options) = doc.caps.signature_help_provider.as_ref() else { @@ -127,7 +128,9 @@ impl Tty7App { if !is_trigger && !visible { return; } - let Some(doc) = LspStore::context(&f.path, Freshen::Text(&text), None, cx) else { + let Some(doc) = + LspStore::context(&f.path, f.input.entity_id(), Freshen::Text(&text), None, cx) + else { return; }; let context = lsp_types::SignatureHelpContext { diff --git a/src/ui/lsp/tests.rs b/src/ui/lsp/tests.rs index abad6237..4e5ac966 100644 --- a/src/ui/lsp/tests.rs +++ b/src/ui/lsp/tests.rs @@ -260,3 +260,283 @@ fn lsp_smoke_a_real_server_publishes_diagnostics_and_shuts_down() { client.kill(); assert!(exited, "clangd exits on `exit`"); } + +/// A store with one running server on the far end of two pipes, and one +/// document owned by `owner`. Returns the path and what the server reads. +mod harness { + use std::io::Read as _; + + use gpui::{Entity, TestAppContext, WindowHandle}; + use gpui_component::input::InputState; + + use super::super::*; + use crate::ui::lsp::rpc::FrameReader; + + pub(super) struct FakeServer { + frames: FrameReader, + from_client: std::io::PipeReader, + _to_client: std::io::PipeWriter, + } + + impl FakeServer { + pub(super) fn next(&mut self) -> Value { + let mut chunk = [0u8; 4096]; + loop { + if let Some(body) = self.frames.next_frame() { + return serde_json::from_slice(&body).unwrap(); + } + let n = self.from_client.read(&mut chunk).unwrap(); + assert!(n > 0, "the client closed its end"); + self.frames.push(&chunk[..n]); + } + } + } + + pub(super) struct Setup { + pub(super) window: WindowHandle, + pub(super) owner: Entity, + pub(super) other: Entity, + pub(super) path: PathBuf, + pub(super) server: FakeServer, + } + + pub(super) fn setup(cx: &mut TestAppContext) -> Setup { + cx.update(|cx| { + gpui_component::init(cx); + cx.set_global(crate::core::config::Config::default()); + }); + let window = cx.add_window(|window, cx| { + InputState::new(window, cx) + .code_editor("rust") + .multi_line(true) + .default_value("fn foo() {}\n") + }); + let owner = window.root(cx).unwrap(); + let other = window + .update(cx, |_, window, cx| { + cx.new(|cx| { + InputState::new(window, cx) + .code_editor("rust") + .multi_line(true) + .default_value("fn foo() { /* the other window's edit */ }\n") + }) + }) + .unwrap(); + let path = PathBuf::from("/tmp/tty7-lsp-test/src/lib.rs"); + let (client_reads, server_writes) = std::io::pipe().unwrap(); + let (server_reads, client_writes) = std::io::pipe().unwrap(); + let (client, _events) = + LspClient::from_streams("fake", client_reads, client_writes).unwrap(); + client.open_gate(); + let key = ServerKey { + name: "rust-analyzer", + root: PathBuf::from("/tmp/tty7-lsp-test"), + }; + cx.update(|cx| { + LspStore::ensure(cx); + let text = owner.read(cx).text().clone(); + let store = cx.global_mut::(); + store.servers.insert( + key.clone(), + Server { + spec: servers::spec_for_language("rust").unwrap(), + label: "fake".into(), + generation: 1, + phase: Phase::Running, + client: Some(client), + caps: Rc::new(lsp_types::ServerCapabilities::default()), + encoding: Encoding::Utf16, + crashes: Vec::new(), + idle_seq: 0, + _tasks: Vec::new(), + }, + ); + store.docs.insert( + path.clone(), + Doc { + uri: path_to_uri(&path).unwrap(), + language_id: "rust", + server: key, + version: 0, + owner: owner.downgrade(), + owner_id: owner.entity_id(), + app_id: owner.entity_id(), + app: WeakEntity::new_invalid(), + window: Some(window.into()), + sent: Some(text), + dirty: false, + change_seq: 0, + symbols_seq: 0, + diagnostics: Vec::new(), + problems: (0, 0), + }, + ); + }); + Setup { + window, + owner, + other, + path, + server: FakeServer { + frames: FrameReader::default(), + from_client: server_reads, + _to_client: server_writes, + }, + } + } +} + +/// The same file open in a second window is a second buffer, maybe with +/// other text. Its requests are refused before anything is sent, so the +/// server's copy stays the owner's. +#[gpui::test] +fn a_buffer_that_does_not_own_the_document_never_syncs(cx: &mut gpui::TestAppContext) { + let mut s = harness::setup(cx); + cx.update(|cx| { + let other_text = s.other.read(cx).text().clone(); + let refused = LspStore::context( + &s.path, + s.other.entity_id(), + Freshen::Text(&other_text), + None, + cx, + ); + assert!(refused.is_none(), "a non-owner gets no server"); + + let owner_text = gpui_component::Rope::from("fn foo() { owner(); }\n"); + let allowed = LspStore::context( + &s.path, + s.owner.entity_id(), + Freshen::Text(&owner_text), + None, + cx, + ); + assert!(allowed.is_some()); + }); + // The first thing the server hears is the owner's change: the other + // buffer's text never went out. + let change = s.server.next(); + assert_eq!(change["method"], "textDocument/didChange"); + assert_eq!( + change["params"]["contentChanges"][0]["text"], + "fn foo() { owner(); }\n" + ); +} + +#[test] +fn an_edit_is_stale_when_a_file_it_touches_changed_opened_or_closed() { + let rope = gpui_component::Rope::from; + let (a, b, c) = ( + PathBuf::from("/a"), + PathBuf::from("/b"), + PathBuf::from("/c"), + ); + let baseline = HashMap::from([(a.clone(), rope("x")), (b.clone(), rope("y"))]); + let targets = vec![a.clone(), c.clone()]; + // `a` unchanged, `c` on disk then and now. + let now = HashMap::from([(a.clone(), rope("x")), (b.clone(), rope("typed"))]); + assert_eq!( + first_stale(&targets, &now, &baseline), + None, + "only targets count" + ); + // `a` typed in. + let now = HashMap::from([(a.clone(), rope("x!"))]); + assert_eq!(first_stale(&targets, &now, &baseline), Some(a.clone())); + // `c` opened since. + let now = HashMap::from([(a.clone(), rope("x")), (c.clone(), rope("z"))]); + assert_eq!(first_stale(&targets, &now, &baseline), Some(c.clone())); + // `a` closed since. + assert_eq!(first_stale(&targets, &HashMap::new(), &baseline), Some(a)); +} + +/// A rename or code action's edit, answered against a text the buffer has +/// since moved on from, is dropped whole; answered against the text it +/// still holds, it is applied. +#[gpui::test] +fn a_workspace_edit_for_a_changed_buffer_is_not_applied(cx: &mut gpui::TestAppContext) { + let s = harness::setup(cx); + let rename = |cx: &mut gpui::App, baseline| { + let mut changes = HashMap::new(); + changes.insert( + path_to_uri(&s.path).unwrap(), + vec![TextEdit { + range: Range::new(Position::new(0, 3), Position::new(0, 6)), + new_text: "bar".into(), + }], + ); + let edit = WorkspaceEdit { + changes: Some(changes), + ..Default::default() + }; + apply_workspace_edit(edit, Encoding::Utf16, Some(baseline), cx) + }; + + let baseline = cx.update(|cx| LspStore::snapshot(cx)); + // Typed in after the request went out. + s.window + .update(cx, |state, window, cx| { + state.set_value("fn foo() { typed }\n", window, cx) + }) + .unwrap(); + assert!(!cx.update(|cx| rename(cx, baseline))); + cx.run_until_parked(); + assert_eq!( + cx.update(|cx| s.owner.read(cx).text().to_string()), + "fn foo() { typed }\n" + ); + + let baseline = cx.update(|cx| LspStore::snapshot(cx)); + assert!(cx.update(|cx| rename(cx, baseline))); + cx.run_until_parked(); + assert_eq!( + cx.update(|cx| s.owner.read(cx).text().to_string()), + "fn bar() { typed }\n" + ); +} + +/// Turning the setting back on asks every window for its open files again, +/// which is what starts their servers. +#[gpui::test] +fn turning_lsp_back_on_offers_every_window_again(cx: &mut gpui::TestAppContext) { + use std::cell::Cell; + cx.update(|cx| cx.set_global(crate::core::config::Config::default())); + let asked = Rc::new(Cell::new(0)); + let window_gone = Rc::new(Cell::new(false)); + cx.update(|cx| { + let asked = asked.clone(); + LspStore::register_window( + EntityId::from(1u64), + Rc::new(move |_| { + asked.set(asked.get() + 1); + true + }), + cx, + ); + let gone = window_gone.clone(); + LspStore::register_window( + EntityId::from(2u64), + Rc::new(move |_| { + gone.set(true); + false + }), + cx, + ); + }); + let set = |cx: &mut gpui::TestAppContext, on: bool| { + cx.update(|cx| { + cx.update_global::(|c, _| c.editor_lsp = on) + }); + cx.run_until_parked(); + }; + set(cx, false); + assert_eq!(asked.get(), 0, "turning it off asks nobody"); + set(cx, true); + assert_eq!(asked.get(), 1); + assert!(window_gone.get()); + // A closed window is forgotten; one still open is asked on every turn-on. + set(cx, false); + set(cx, true); + assert_eq!(asked.get(), 2); + assert_eq!(cx.update(|cx| cx.global::().windows.len()), 1); +} diff --git a/src/ui/settings/editor.rs b/src/ui/settings/editor.rs index 2764c340..bcb49057 100644 --- a/src/ui/settings/editor.rs +++ b/src/ui/settings/editor.rs @@ -61,8 +61,9 @@ impl Tty7App { ) } - /// Off closes every document's server (`LspStore` watches the config); - /// on lets the next frame open them again. + /// `LspStore` watches the config: off closes every document and lets + /// its server go; on asks every window for the files it has open, which + /// starts their servers again. pub(crate) fn set_editor_lsp(&mut self, on: bool, cx: &mut Context) { self.update_config(cx, |cfg| cfg.editor_lsp = on); cx.notify();