diff --git a/builtin/runtime/completion.lua b/builtin/runtime/completion.lua index fa480b8..89cfa7f 100644 --- a/builtin/runtime/completion.lua +++ b/builtin/runtime/completion.lua @@ -176,7 +176,15 @@ pmacs.hook.add("buffer.after-edit", function() close_popup() return end - if not publish(buf, session.anchor, prefix, "incomplete", nil) then + if publish(buf, session.anchor, prefix, "incomplete", nil) then + -- isIncomplete contract: a partial server response must be + -- re-queried as the user keeps typing, not merely re-filtered. + local rec = pmacs.lsp.active_attachment() + if rec then + local ok, incomplete = pcall(pmacs.completion.is_incomplete, rec.server, rec.uri) + if ok and incomplete then request_lsp_then_refresh() end + end + else session = nil -- narrowed to nothing: the session is over end return @@ -187,9 +195,14 @@ pmacs.hook.add("buffer.after-edit", function() if key ~= prev_key or not prev_cursor or cursor - prev_cursor ~= 1 then return end local prefix = word_prefix_before(buf, cursor) if #prefix >= MIN_PREFIX then - if publish(buf, cursor - #prefix, prefix, "invoked", nil) then - request_lsp_then_refresh() + -- Fire the LSP request even when the synchronous providers came + -- up empty: for an LSP-only word (no dabbrev/snippet/index hit, + -- cold store) the popup materializes when the response lands --- + -- the same pending-session shape as the trigger-char path. + if not publish(buf, cursor - #prefix, prefix, "invoked", nil) then + session = { key = key, anchor = cursor - #prefix, pending = true } end + request_lsp_then_refresh() return end if #prefix == 0 then diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 61cbef5..cf36fa8 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -545,16 +545,22 @@ function pmacs.lsp.active_attachment() end -- Flushing variant for request-issuing callers outside this file --- (Q#C8): resolves (or attaches) the active buffer's server AND --- flushes any debounced didChange first, so the server answers the --- caller's request against the current text --- exactly what every --- interactive command in this file gets from the local --- `attached_for_active`. The in-buffer completion driver --- (builtin/runtime/completion.lua) calls this before --- textDocument/completion; a non-flushing peek would hand the server --- stale text after a typing burst. +-- (Q#C8): when the active buffer already has a server attached, +-- flush any debounced didChange first and return the record, so the +-- caller's request is answered against the current text. Unlike the +-- local `attached_for_active`, this NEVER triggers an attach: the +-- in-buffer completion driver calls it on ordinary typing, and +-- spawning language servers as a typing side effect is wrong (and, +-- concretely, wedged the m4 suite with per-keystroke spawn attempts +-- across parallel tests). Attachment remains buffer-open policy. function pmacs.lsp.attachment_for_request() - return attached_for_active() + local buf = pmacs.window.buffer() + if not buf then return nil end + local key = tostring(buf) + local rec = attachments[key] + if not rec then return nil end + flush_did_change(key) + return rec end -- Hooks -------------------------------------------------------------------- diff --git a/docs/in-buffer-completion-framing.md b/docs/in-buffer-completion-framing.md index 5d110df..d667df7 100644 --- a/docs/in-buffer-completion-framing.md +++ b/docs/in-buffer-completion-framing.md @@ -277,6 +277,42 @@ and kill-ring are the named next table-stakes items). shows misfires, the fix is the named hook-payload PR, not heuristic patching. +## As-built notes — phase 1 (PR #92) + +Landed close to the framing; the user's TUI validation pass surfaced +five findings, all addressed in-branch: + +1. **LSP-only words never queried the server** — the auto-open path + fired `request_completion` only when the synchronous providers had + already produced rows. Now an empty sweep leaves a *pending* + session (the trigger-char shape) and the request always fires; + `isIncomplete` responses re-request on further typing via + `pmacs.completion.is_incomplete`. Corollary found while fixing: + `attachment_for_request` must flush-if-attached but **never + attach** — the first cut wrapped `attached_for_active`, which + spawns a server on demand, i.e. per-keystroke spawn attempts in + every unattached buffer (wedged the parallel m4 suite). + Attachment stays buffer-open policy. +2. **Strict URI scoping** — the built-in LSP provider now returns + *nothing* without `ctx.uri` (the framing's "legacy global drain + when absent" allowed unattached/scratch buffers to show another + file's cached completions). +3. **Pending prefixes own the keyboard** — `Action::Pending` (`C-x + ...`) dismisses the popup, and the popup shadow is additionally + guarded on `dispatcher.pending().is_empty()`, so a sequence's + continuation and its `C-g` abort reach the dispatcher. +4. **Window-scoped sessions** — `CompletionPopupState.window_id` + (stamped by `completion_popup_open`; Lua never sees it): only the + owning window's overlay paints (same-buffer splits each carry a + persistent overlay), and a focus change invalidates the session. +5. The worker-pool teardown fix (signal-only `EditorState::drop`) + rode along in the PR — unrelated to completion, surfaced by + running the m4 gate. + +Also caught by the new acceptance suite pre-validation: +`install_completion` rebuilt `pmacs.completion` and clobbered the +popup bindings — all `pmacs.completion` installers now merge. + ## Deferred (named, not silently dropped) - Snippet tabstops/placeholders (v1 inserts bodies literally). diff --git a/src/completion.rs b/src/completion.rs index 046d8cf..ee152e8 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -487,6 +487,14 @@ pub struct CompletionPopupState { /// Buffer the popup targets. The session closes the moment the /// active buffer differs (Q#C3 validation). pub buffer_id: BufferId, + /// Window the popup belongs to. Stamped by + /// [`crate::editor_core::EditorCore::completion_popup_open`] + /// (Lua publishers don't know window identity), `None` only + /// before that stamp. Two splits showing the same buffer each + /// carry a persistent overlay --- without this, both would paint + /// the popup; with it, only the owning window's overlay renders, + /// and a focus change closes the session (Q#C3). + pub window_id: Option, /// Byte offset where the typed prefix starts. For a /// trigger-character session (e.g. right after `.`) the prefix is /// empty and `anchor` equals the cursor. @@ -519,6 +527,7 @@ impl CompletionPopupState { } Some(Self { buffer_id, + window_id: None, anchor, prefix, candidates, @@ -637,13 +646,19 @@ pub(crate) fn popup_window(n: usize, selected: usize, max: usize) -> (usize, usi /// the popup would overflow the window's right edge. pub struct CompletionView { popup: SharedCompletionPopup, + /// The window this overlay instance is attached to. Overlays + /// persist per window; only the one matching the session's + /// `window_id` paints, so same-buffer splits don't each show + /// the popup. + window_id: crate::window::WindowId, } impl CompletionView { - /// Build a view reading `popup`. + /// Build a view reading `popup`, rendering only when the open + /// session belongs to `window_id`. #[must_use] - pub fn new(popup: SharedCompletionPopup) -> Self { - Self { popup } + pub fn new(popup: SharedCompletionPopup, window_id: crate::window::WindowId) -> Self { + Self { popup, window_id } } } @@ -826,6 +841,9 @@ impl View for CompletionView { let Some(popup) = guard.as_ref() else { return; }; + if popup.window_id != Some(self.window_id) { + return; // the session belongs to another window + } if popup.buffer_id != buf.id() { return; // this window shows a different buffer } diff --git a/src/completion_framework.rs b/src/completion_framework.rs index a9761b0..dbb183d 100644 --- a/src/completion_framework.rs +++ b/src/completion_framework.rs @@ -624,15 +624,18 @@ fn project_kind_to_completion_kind(k: &crate::project_index::SymbolKind) -> Comp /// the LSP completion store. The framework does **not** drive a /// fresh `textDocument/completion` request --- that's the editor's /// job; we just read whatever the async pipeline has produced so -/// far. With `ctx.uri` set (Q#C8 scoping, the popup driver's path) -/// only that document's entries surface --- across all servers keyed -/// to it --- so a popup never shows another buffer's candidates. -/// Without a URI the legacy global drain across every cached -/// `(server_id, uri)` key applies. The registry's dedup collapses -/// identical entries; the prefix score ranks them. +/// far. Strictly scoped to `ctx.uri` (Q#C8): only that document's +/// entries surface, across all servers keyed to it. **No URI → no +/// LSP candidates** --- an unattached/scratch buffer must never show +/// another file's cached completions (the original global drain did +/// exactly that). The registry's dedup collapses identical entries; +/// the prefix score ranks them. #[must_use] pub fn lsp_completion_provider(lsp: crate::lsp::SharedLspManager) -> ProviderFn { Box::new(move |ctx: &CompletionContext| -> Vec { + let Some(uri) = ctx.uri.clone() else { + return Vec::new(); + }; let store_handle = { let mgr = lsp.borrow(); mgr.completion_store() @@ -641,11 +644,7 @@ pub fn lsp_completion_provider(lsp: crate::lsp::SharedLspManager) -> ProviderFn return Vec::new(); }; let mut out: Vec = Vec::new(); - let keys: Vec<_> = store - .keys() - .filter(|k| ctx.uri.as_ref().is_none_or(|uri| k.uri == *uri)) - .cloned() - .collect(); + let keys: Vec<_> = store.keys().filter(|k| k.uri == uri).cloned().collect(); for key in keys { for item in store.items(&key) { out.push(item.clone()); diff --git a/src/editor.rs b/src/editor.rs index 88bdac7..6fb720e 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -578,8 +578,13 @@ impl EditorState { // below, so typing keeps self-inserting and motion keys keep // moving. The post-dispatch validation at the bottom of this // function closes the session when a fallen-through key breaks - // the anchor/prefix invariant. + // the anchor/prefix invariant. A pending multi-key prefix owns + // the keyboard: while one is in flight (`C-x ...`) the popup + // must not steal its continuation or its `C-g` abort --- and + // the Pending arm below closes the popup anyway, so this guard + // only covers the same-dispatch race. if self.core.borrow().completion_popup_is_open() + && self.dispatcher.pending().is_empty() && let Some(key) = CompletionPopupKey::from_chord(chord) { self.dispatch_completion_key(key); @@ -616,7 +621,12 @@ impl EditorState { } Action::Pending { .. } => { // The pending prefix is rendered from - // `dispatcher.pending()`; no command runs yet. + // `dispatcher.pending()`; no command runs yet. Starting + // a command sequence dismisses the completion popup: + // leaving it open would route the sequence's `C-g` + // abort (and its continuation chords) into the popup's + // shadow instead of the dispatcher. + self.core.borrow_mut().completion_popup_close(); } Action::Unbound { sequence } => match printable_char(&sequence) { Some(ch) => { diff --git a/src/editor_core.rs b/src/editor_core.rs index 9de7c18..a394222 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -1842,7 +1842,11 @@ impl EditorCore { /// Emptiness is enforced upstream: /// [`crate::completion::CompletionPopupState::new`] refuses to build /// a candidate-less session. - pub fn completion_popup_open(&mut self, state: crate::completion::CompletionPopupState) { + pub fn completion_popup_open(&mut self, mut state: crate::completion::CompletionPopupState) { + // Stamp the owning window (Lua publishers don't know window + // identity): only that window's overlay paints the popup, and + // a focus change invalidates the session. + state.window_id = Some(self.active_window_id()); *self .completion_popup .lock() @@ -1883,14 +1887,17 @@ impl EditorCore { /// this the session is stale, not a prefix. const MAX_PREFIX_BYTES: u64 = 512; - let (buffer_id, anchor) = { + let (buffer_id, window_id, anchor) = { let guard = self .completion_popup .lock() .expect("completion popup poisoned"); let p = guard.as_ref()?; - (p.buffer_id, p.anchor) + (p.buffer_id, p.window_id, p.anchor) }; + if window_id != Some(self.active_window_id()) { + return None; // focus moved to another window/split + } if self.active_buffer_id() != buffer_id { return None; } @@ -1979,9 +1986,10 @@ impl EditorCore { /// renders nothing while the popup is closed. fn ensure_completion_overlay(&mut self) { let popup = self.completion_popup.clone(); + let wid = self.active_window_id(); let win = self.active_window_mut(); if !win.overlay_kinds().contains(&"completion-popup") { - win.push_overlay(Box::new(crate::completion::CompletionView::new(popup))); + win.push_overlay(Box::new(crate::completion::CompletionView::new(popup, wid))); } } @@ -3382,4 +3390,24 @@ mod tests { assert_eq!(text_of(&s), "he world\n", "buffer untouched"); assert!(!s.completion_popup_is_open(), "stale accept still closes"); } + + #[test] + fn completion_popup_validate_closes_on_window_focus_change() { + // Two splits on the SAME buffer: the session is window-scoped, + // so moving focus (buffer unchanged!) must invalidate it --- + // this is also what keeps the persistent overlay in the other + // split from painting a popup it doesn't own. + let mut s = from_bytes(b"he world\n"); + s.split_active(Orientation::Horizontal, true); + s.active_window_mut().cursor = 2; + open_popup(&mut s, 0, "he", "hello"); + s.completion_popup_validate(); + assert!(s.completion_popup_is_open(), "session holds in its window"); + s.focus_next(); + s.completion_popup_validate(); + assert!( + !s.completion_popup_is_open(), + "focus change closes the session even with the same buffer" + ); + } } diff --git a/tests/completion_popup_acceptance.rs b/tests/completion_popup_acceptance.rs index 774f204..c2ffeaf 100644 --- a/tests/completion_popup_acceptance.rs +++ b/tests/completion_popup_acceptance.rs @@ -201,10 +201,68 @@ fn at_point_command_opens_below_threshold() { assert_eq!(text, "hello_world hello_world"); } -/// The Q#C8 URI scoping: with `ctx.uri` set, the LSP provider only -/// surfaces the matching document's cached items; without it, the -/// legacy global drain applies. Driven through the Lua collect -/// surface against a hand-seeded store. +/// A multi-key prefix owns the keyboard: starting `C-x` while the +/// popup is open dismisses it, so the sequence's continuation (and a +/// `C-g` abort) reaches the dispatcher instead of the popup shadow. +#[test] +fn pending_prefix_dismisses_popup_and_keeps_dispatcher_control() { + let mut s = EditorState::new(); + type_str(&mut s, "hello_world he"); + let (_, visible, _) = probe(&s); + assert!(visible); + + // C-x starts a prefix: the popup must close immediately... + s.dispatch_key( + FrontendId::LOCAL, + key(KeyCode::Char('x'), KeyModifiers::CONTROL), + ); + let (text, visible, _) = probe(&s); + assert_eq!(text, "hello_world he", "the prefix key edits nothing"); + assert!(!visible, "a pending prefix dismisses the popup"); + + // ...so this C-g aborts the prefix (not a popup), and typing + // afterwards self-inserts normally. + s.dispatch_key( + FrontendId::LOCAL, + key(KeyCode::Char('g'), KeyModifiers::CONTROL), + ); + type_str(&mut s, "x"); + let (text, _, _) = probe(&s); + assert_eq!( + text, "hello_world hex", + "after the aborted prefix, keys dispatch normally" + ); +} + +/// LSP-only words must still query the server: when the synchronous +/// providers return nothing at auto-open, a pending session is left +/// behind and the request fires anyway (previously the request was +/// gated on the popup having opened, so an empty dabbrev/snippet/ +/// index sweep meant the server was never asked). +#[test] +fn empty_sync_sweep_still_leaves_a_pending_session() { + let mut s = EditorState::new(); + // A buffer whose only word is the one being typed: dabbrev is + // structurally empty, no LSP attached. The popup cannot open... + type_str(&mut s, "qz"); + let (_, visible, _) = probe(&s); + assert!(!visible, "nothing to show without providers"); + // ...but the driver's session mirror must be pending rather than + // absent, so a (mocked) late LSP arrival could materialize it. + // With no attachment at all the request path no-ops; what we can + // assert end-to-end is that the state machine stays consistent: + // further typing neither crashes nor opens a bogus popup. + type_str(&mut s, "q"); + let (text, visible, _) = probe(&s); + assert_eq!(text, "qzq"); + assert!(!visible); +} + +/// The Q#C8 URI plumbing: `ctx.uri` reaches Lua providers as the +/// ninth positional arg, so a URI-aware provider can scope itself +/// (the BUILT-IN LSP provider is stricter still: no uri → no rows). +/// Driven through the Lua collect surface with an emulating provider +/// because the real LSP store is Rust-side. #[test] fn collect_scopes_lsp_candidates_to_ctx_uri() { let s = EditorState::new(); diff --git a/tests/worker_shutdown_acceptance.rs b/tests/worker_shutdown_acceptance.rs index 116aae4..3893a20 100644 --- a/tests/worker_shutdown_acceptance.rs +++ b/tests/worker_shutdown_acceptance.rs @@ -1,17 +1,25 @@ //! Worker-pool teardown regression: dropping an `EditorState` must //! release its worker threads even though the `Rc` is //! trapped in Lua-VM reference cycles and never reaches refcount -//! zero (`EditorState::drop` → `AsyncRuntime::shutdown_workers`). +//! zero (`EditorState::drop` → `AsyncRuntime::shutdown_workers`, +//! signal-only --- a join here deadlocks against workers blocked +//! handing replies to the main thread). //! //! Before the fix, every `EditorState` ever constructed leaked a //! full `cores - 1` worker pool: the m4 acceptance suite (54 editor- //! building tests) accumulated 1000+ live threads, each waking every //! 100ms. +//! +//! The thread-count probe and the idempotence check share ONE test +//! function: they both build `EditorState`s, and as separate tests +//! libtest may run them concurrently, polluting the /proc-based +//! baseline on high-core machines. use pmacs::editor::EditorState; -/// Thread-count probe via /proc; Linux-only (macOS CI skips --- the -/// leak and the fix are platform-independent, the *probe* isn't). +/// Thread-count probe via /proc; Linux-only (macOS CI runs the +/// non-Linux variant below --- the leak and the fix are platform- +/// independent, the *probe* isn't). #[cfg(target_os = "linux")] fn live_threads() -> usize { std::fs::read_dir("/proc/self/task").map_or(0, std::iter::Iterator::count) @@ -19,14 +27,14 @@ fn live_threads() -> usize { #[cfg(target_os = "linux")] #[test] -fn editor_state_drop_releases_worker_threads() { +fn editor_state_drop_releases_workers_and_shutdown_is_idempotent() { let baseline = live_threads(); for _ in 0..3 { let s = EditorState::new(); drop(s); } - // Joins are synchronous in drop; the small sleep only covers - // detached per-process reaper threads finishing up. + // Signal-only shutdown: parked workers exit within their 100ms + // park timeout; give them a beat. std::thread::sleep(std::time::Duration::from_millis(300)); let after = live_threads(); assert!( @@ -34,10 +42,17 @@ fn editor_state_drop_releases_worker_threads() { "worker threads leak across EditorState drop: \ baseline {baseline}, after 3 create/drop cycles {after}" ); + + // Idempotence: explicit shutdown twice, then drop runs it a + // third time --- none may hang or panic. + let s = EditorState::new(); + s.async_runtime.shutdown_workers(); + s.async_runtime.shutdown_workers(); + drop(s); } -/// Platform-independent variant: the pool reports itself dead after -/// an explicit shutdown, and shutdown is idempotent. +/// Platform-independent idempotence check for hosts without /proc. +#[cfg(not(target_os = "linux"))] #[test] fn explicit_shutdown_is_idempotent() { let s = EditorState::new();