From 21d12b486bc03ded037d5248368c1a465eac09e7 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 7 Jul 2026 11:24:46 -0400 Subject: [PATCH 1/6] docs: in-buffer completion popup framing (Arc 1a) Framing for wiring the dormant M4.7/M4.11 completion system to real UI in both frontends. Q#C1-C9 decisions: Lua driver, core-owned session (search-store pattern), partial dispatcher shadow with post-dispatch validation, MenuView-style CompletionView rework, protocol v15 CompletionPopup message (phase 2), explicit GPU control-key predicate, validated accept, LSP scoping + flush-before-request, and the single-char trigger heuristic. Incorporates the 2026-07-07 review findings (LSP store scoping, score filtering, didChange flush, motion closure, GPU ESC routing, hook payload policy, context_for nit). Co-Authored-By: Claude Fable 5 --- docs/in-buffer-completion-framing.md | 290 +++++++++++++++++++++++++++ 1 file changed, 290 insertions(+) create mode 100644 docs/in-buffer-completion-framing.md diff --git a/docs/in-buffer-completion-framing.md b/docs/in-buffer-completion-framing.md new file mode 100644 index 0000000..5d110df --- /dev/null +++ b/docs/in-buffer-completion-framing.md @@ -0,0 +1,290 @@ +# In-buffer completion popup — framing (Arc 1a) + +pmacs has a complete completion system that nothing drives. M4.7 shipped +the LSP completion store + `CompletionView` popup (`src/completion.rs`); +M4.11 shipped the provider framework — registry, scoring, dedup, and +four providers (lsp=100, snippets=80, project_symbols=60, dabbrev=20 by +priority; `src/completion_framework.rs`); the async LSP request path +(`LspManager::request_completion`, `src/lsp.rs:1665`) works and is +exercised by m4 acceptance tests. But there is no keybinding, no +typed-char trigger, `CompletionView` is never instantiated, and no GPU +wire message exists. This arc wires it end-to-end in both frontends: +type, see candidates, TAB/RET accept. + +Roadmap context: `docs/roadmap-2026-07.md` Arc 1a. Closest wire +precedent: `docs/gpu-minibuffer-framing.md` (the +`SearchPrompt`/`MenuPrompt`/`MinibufferPrompt` family). Tightened +2026-07-07 after a code review pass against the current tree +(findings folded into Q#C1/C3/C6/C7/C8/C9 below). + +## What already exists (verified) + +- **Framework**: `CompletionRegistry::collect(ctx)` is synchronous — + filters enabled providers by priority, scores against `ctx.prefix` + (exact=1000 / prefix=600 / word-boundary=300 / substring=100), dedups + by `(label, insert_text)`, sorts (`completion_framework.rs:292-333`). + `Rc>` — main-thread only. Instantiated at startup + (`editor.rs:243`) and exposed as `pmacs.completion.{register,collect, + context_for,snippets.*}` — never called by builtin Lua. +- **The LSP provider does not request, and is globally scoped** — it + drains **every** cached `(server_id, uri)` entry in the store, + ignoring context (`completion_framework.rs:617-645`), so unmodified + it can surface stale candidates from other buffers/servers. Fresh + requests are the editor's job: `pmacs.lsp._request_completion_raw` → + awaitable handle (`builtin/runtime/lsp.lua:585-597`), response + absorbed into the store with positions already normalized to bytes + (`lsp.rs:2630-2647`). +- **`collect` does not filter non-matches** — `score_match` returns −1 + for a candidate that misses the prefix, but `collect` keeps it and + merely sorts it last (`completion_framework.rs:292-333`); a no-hit + prefix would still yield a popup full of unrelated rows. +- **The public attachment accessor does not flush** — interactive LSP + commands resolve via the *local* `attached_for_active()`, which runs + `flush_did_change` first so the server answers against current text + (`builtin/runtime/lsp.lua:520-533`); the public + `pmacs.lsp.active_attachment()` is deliberately side-effect-free and + skips the flush (`lsp.lua:541-545`). +- **`buffer.after-edit` carries no payload** — both edit paths fire it + with empty args (`editor.rs:575`, `daemon.rs:1923`); a driver cannot + see what changed, only that something did. +- **Trigger characters**: `CompletionTriggers::from_capabilities` + + `should_fire(ch)` per server (`completion.rs:406-454`), Lua-exposed. + No prefix-extraction helper exists anywhere — the driver must derive + the word before the cursor itself. +- **`CompletionView`** is a cell-owning popup (kind glyph + label + + detail, reverse-video selection, width ≤ 40, CJK-aware) — but it + reads the raw LSP store, not framework candidates, and the TUI + overlay loop hands every overlay the full window viewport + (`editor.rs:1667-1669`); nothing positions a sub-rect popup today. + `MenuView` is the precedent for a self-positioning cell popup. +- **Dispatcher shadows** already exist for minibuffer + (`editor.rs:520`), menu (`:500`), isearch (`:511`). + +## Decisions + +### Q#C1 — The driver is builtin Lua, core stays a library + +A new `builtin/runtime/completion.lua`: derives the prefix (word chars +`[A-Za-z0-9_]` before the cursor), decides *when* to open (Q#C9), +fires the LSP request through a flushing accessor (Q#C8), runs +`pmacs.completion.collect`, **filters to `score >= 0`** (collect +returns non-matches sorted last; Lua receives `score`, so the driver +drops them — a Rust-side filter inside `collect` would change M4.11 +semantics/tests and is left as a follow-up), and publishes the session +(Q#C2) only when at least one row survives. Mirrors how `lsp.lua` +drives every other LSP surface. Refresh-on-typing rides the same +after-edit hook while the session is open; the session closes when the +prefix dies (Q#C3's validation, `C-g`, or an edit that kills the +word). Nit rolled into phase 1: `pmacs.completion.context_for` cannot +express a `Char` trigger (`lua_bindings/mod.rs:9964` maps only +incomplete-vs-invoked) — either extend it or have the driver pass raw +ctx tables (already supported). + +### Q#C2 — Core-owned session state, the search-store pattern + +A frontend-agnostic `CompletionPopup` session on `EditorCore` (like +`menu: SharedMenu`): `{buffer_id, anchor_byte, prefix_len, candidates, +selected, total}`. Lua publishes into it; both frontends render from +it; keys resolve against it daemon-side. Control is daemon-owned even +though rendering is frontend-local — the Q#UX1 lesson, applied from +day one this time. Candidates carry `{label, kind, detail, +insert_text}` (framework `CompletionCandidate` projected down). + +### Q#C3 — Key capture: a partial dispatcher shadow, not buffer-local binds + +While the session is open, `dispatch_key` routes **only** +`TAB / RET / C-n / C-p / Up / Down / ESC / C-g` to +`dispatch_completion_key` (accept / accept / next / prev / next / prev +/ close / close); *everything else falls through* to normal dispatch, +so printable keys keep self-inserting and motion keys work. This is +the fourth member of the existing shadow family and avoids the +bind/unbind lifecycle discipline that transient buffer-local keymaps +would need. Considered and rejected: minibuffer-style full shadow +(typing must insert); `add_intercept` (sees edits only, never nav +chords); buffer-local binds (workable — the `*buffer-list*` idiom — +but the teardown hazard buys nothing here). + +**Session validity is enforced in core, post-dispatch — not by hooks.** +Cursor motion fires no hook (`buffer.after-edit` is edits-only, +`editor.rs:573`), so "close when the cursor leaves the word" cannot be +Lua-driven. After any dispatched action while the session is open, the +dispatcher runs a validation step: the session survives only if the +active buffer still matches `buffer_id`, the cursor sits inside +`[anchor .. anchor + current-word-end]`, and the prefix re-derived +from the buffer still starts at `anchor`. Anything else — motion off +the word, buffer switch, window change, undo that rewrote the region — +closes the session. This also covers remote/CRDT edits landing under +the popup: the next dispatch (or the producer tick) revalidates +against the shifted text. + +### Q#C4 — TUI popup: rework `CompletionView` on the MenuView pattern + +`CompletionView` switches its data source to the Q#C2 session and +becomes self-positioning like `MenuView`: compute its own sub-rect +anchored to the row below the cursor (above when near the window +bottom), clamped to the window and offset past the gutter +(`viewport.gutter_w`), painting ≤ `POPUP_VISIBLE` rows. Attached once +per window like `MenuView`, self-suppressing when the session is +closed or belongs to another buffer. + +### Q#C5 — Wire: `InstanceMessage::CompletionPopup`, protocol v15 + +Additive over v14; `SUPPORTED = [6..15]`, daemon-gated `>= 15`, +produced by `semantic_render::completion_popup_msg` with the family's +cached-compare suppression: + +``` +InstanceMessage::CompletionPopup { + buffer_id: BufferId, + anchor: Option, // byte offset of the prefix start; None = closed + prefix_len: u32, // bytes of typed prefix (frontend may embolden) + rows: Vec, // windowed slice (≤ POPUP_VISIBLE = 10) + selected: Option, // within `rows` + total: u32, +} +CompletionRow { label: String, kind: u8, detail: Option } +``` + +Byte anchor, not pixels — the GPU already maps byte→glyph rect for the +caret and presence washes; first byte-anchored popup on the wire +(menu anchors at the click pixel locally). Rows are display-only; +accept is a daemon round-trip, so `insert_text` never ships. + +### Q#C6 — GPU input while the popup is open: an explicit key predicate + +The existing gates are all-or-nothing (`daemon_intercepts_keys`, +`pmacs-gpu/src/main.rs:1913`; `dispatch_idle`) — reusing them would +round-trip *typing* too, killing optimistic latency exactly when the +user is mid-word. Instead the GPU gets an explicit +`is_completion_control_key(key, mods)` predicate, checked **before** +the optimistic-insert path and before local key handling, active only +while `CompletionLocal.is_some()`: exactly +`TAB / RET / ESC / C-g / C-n / C-p / Up / Down` forward as +`FrontendEvent::Key` round-trips into `dispatch_completion_key`; +every other key stays on its normal path (plain chars optimistic, +chords per the usual forwarding rules). Three keys need the gate +specifically because their default handling is wrong under a popup: +RET and TAB are optimistic-eligible today (they reduce to plain +inserts, `main.rs:5300`-area), and **ESC is a hardcoded local +exception** (`main.rs:1423`) that would be swallowed and never reach +the daemon — it must be conditionally forwarded while the popup is +open. Rendering: a `CompletionLocal` mirror + a dropdown layer cloned +from the `mb_dropdown_*` functions, anchored at the anchor byte's +glyph rect instead of the status band, clamped to the window (the +F-007 fit logic). + +### Q#C7 — Accept semantics: validate, then replace + +Accept **re-validates before touching the buffer**: the active buffer, +cursor, anchor, and the prefix currently in the text must all still +match the session (the Q#C3 invariant, re-checked at the moment of +accept — a remote edit or race can invalidate between frames). On +mismatch, accept is a no-op that closes the session. On match, it +replaces `[anchor .. cursor]` with the candidate's +`effective_insert_text()` through the normal command/edit layer (one +undo entry; fires `buffer.after-edit`, so LSP `didChange` and styling +refresh ride existing machinery), then closes the session. Snippet +bodies insert literally in v1 — no tabstop engine (deferred). + +### Q#C8 — LSP scoping + freshness: flush first, scope to the attachment + +Two correctness rules, then the UX: + +- **Flush before requesting.** `didChange` is debounced; a request + issued without flushing answers against stale text. The driver must + not use the non-flushing `pmacs.lsp.active_attachment()` — `lsp.lua` + grows a public flushing accessor (wrapping the local + `attached_for_active()`, `lsp.lua:520-533`) that the driver calls + before `request_completion`, exactly as every interactive LSP + command already does internally. +- **Scope candidates to the attachment.** The built-in LSP provider + drains the whole store across all `(server, uri)` keys. Fix in the + framework: `CompletionContext` gains `uri: Option` (the + driver fills it from the attachment record) and the provider reads + only that URI's entries. Fallback if the context change proves + awkward: disable the built-in LSP provider for the driver path and + have Lua merge `pmacs.completion.items(rec.server, rec.uri)` itself + — either way, no cross-buffer candidates. + +UX: publish immediately from the synchronous providers +(dabbrev/snippets/project-symbols feel instant), fire the LSP request, +and re-collect + re-publish when the response lands +(`handle:on_complete`). `isIncomplete` responses re-request on further +typing. No spinner; the popup just gets better a beat later. + +### Q#C9 — Trigger policy without edit metadata + +`buffer.after-edit` fires with no payload from both edit paths +(`editor.rs:575`, `daemon.rs:1923`), so the driver cannot see *what* +changed — it must reconstruct intent from state. Policy (v1): + +- The driver keeps a per-invocation snapshot `{buffer, revision, + cursor}`. +- **Open** on after-edit only when: the active buffer is unchanged, + the cursor sits at the end of a word with prefix ≥ 2, **and** the + cursor advanced by exactly one codepoint since the last snapshot + (the single-char-typing signature) — or the char before the cursor + is a server trigger character (`should_fire`). +- **Paste, undo/redo, kill, and remote edits never auto-open** (their + cursor delta ≠ 1 or the region signature doesn't match); `C-M-i` + covers the deliberate cases. +- While **open**, any after-edit re-derives the prefix from the buffer + and refreshes or closes — no metadata needed, the text is the truth. + +Named alternative, deliberately not taken now: giving +`buffer.after-edit` an edit payload (buffer id, kind, range). Variadic +Lua handlers would tolerate the new args, but the PR #52 revert +history says hook-signature changes must not ride along in a feature +bundle — if the heuristic proves flaky, the payload becomes its own +small, separately-validated PR. + +## Phasing (each phase independently green + user-validated) + +1. **Core + TUI, no wire change.** Session store + dispatcher shadow + with post-dispatch validation (Q#C3) + `CompletionView` rework + + `builtin/runtime/completion.lua` driver (Q#C1/C9 policy, score ≥ 0 + filter) + validated accept (Q#C7) + `completion.at-point` — plus + the small Rust/Lua seams the review surfaced: + `CompletionContext.uri` + LSP-provider scoping, the flushing + attachment accessor, and the `context_for` char-trigger fix + (Q#C8/C1). Validate in the standalone TUI and a TUI-attached + daemon. +2. **Wire + GPU.** Protocol v15, producer + daemon gate, GPU + `CompletionLocal` + dropdown layer + the RET/TAB optimistic gate. +3. **Polish + docs.** `isIncomplete` re-query, per-server + trigger-chars, as-built notes folded into this doc. + +Arc 2 interleave points: after phase 1 and after phase 2 (query-replace +and kill-ring are the named next table-stakes items). + +## Categorical bets (score at close) + +1. **The prompt-family pattern generalizes a fourth time** — producer / + cached-compare / gate / local-mirror drop in without core surgery. + Risk concentrates in the one new element: byte-anchored positioning. +2. **Synchronous `collect` is fast enough per keystroke.** dabbrev + scans the whole buffer text per call — O(buffer) on every refresh. + Bet: fine at typical file sizes; a cap/debounce is the fallback, + not a redesign. +3. **RET/TAB/ESC routing** — some path will both insert and accept + (or neither), or ESC will die at the GPU's local handler, until the + Q#C6 predicate is exactly right. Predicted highest-likelihood + finding. +4. **Popup placement edge cases** — bottom-of-window flip, gutter + offset, narrow windows. Clamping bugs, MenuView-class. +5. **The Q#C9 single-char heuristic holds.** Paste/undo/kill/remote + edits stay quiet and ordinary typing always opens. If validation + shows misfires, the fix is the named hook-payload PR, not + heuristic patching. + +## Deferred (named, not silently dropped) + +- Snippet tabstops/placeholders (v1 inserts bodies literally). +- `completionItem/resolve` (lazy documentation/detail) and + `additionalTextEdits` (auto-import) — needs a resolve round-trip on + selection change. +- A documentation panel beside the popup (company-style doc buffer). +- Fuzzy matching beyond the current prefix/word-boundary/substring + scorer. +- TUI/GPU visual unification (GPU leads, as with the minibuffer). +- Minibuffer-style persisted ranking / frequency weighting. From 361cc542c79cf9be810c0859697995cf87da1317 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 7 Jul 2026 11:42:30 -0400 Subject: [PATCH 2/6] feat(completion): popup session store, self-positioning view, dispatcher shadow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Q#C2: CompletionPopupState (buffer + byte anchor + prefix + candidates + selection) behind SharedCompletionPopup on EditorCore — the completion twin of SharedMenu. Q#C4: CompletionView reworked from the dormant M4.7 store-keyed full-viewport painter into a self-positioning overlay (MenuView model): windows candidates around the selection, maps the byte anchor to a screen cell via the diag-view walk, places below the anchor row (flips above when nothing fits), self-suppresses when closed or on a foreign buffer. Q#C3: dispatch_key gains a PARTIAL shadow — only TAB/RET/C-n/C-p/Up/Down/Esc/C-g intercept while the popup is open; everything else falls through so typing keeps self-inserting, with post-dispatch validation (active buffer, cursor at/after anchor, word bytes between) closing broken sessions after the after-edit hook has had its chance to refresh. Q#C7: accept re-validates at the moment of accept and applies a single Replace (one undo step; empty-prefix trigger sessions degrade to Insert), firing buffer.after-edit through the existing revision check. Framing: docs/in-buffer-completion-framing.md. Lua driver + bindings follow in this branch. Co-Authored-By: Claude Fable 5 --- src/completion.rs | 508 +++++++++++++++++++++++++++++++++++++-------- src/editor.rs | 107 ++++++++++ src/editor_core.rs | 310 +++++++++++++++++++++++++++ 3 files changed, 838 insertions(+), 87 deletions(-) diff --git a/src/completion.rs b/src/completion.rs index 50318f9..046d8cf 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -30,8 +30,9 @@ use std::sync::{Arc, Mutex}; use serde_json::Value; use unicode_width::UnicodeWidthChar; -use crate::buffer::Buffer; -use crate::cell::{Cell, CellCoord, CellGrid, Color, Glyph, Style}; +use crate::buffer::{Buffer, BufferId}; +use crate::cell::{CellCoord, CellGrid, Color, Glyph, Style}; +use crate::rope::Position; use crate::view::{View, Viewport}; // --------------------------------------------------------------------------- @@ -453,6 +454,114 @@ impl CompletionTriggers { } } +// --------------------------------------------------------------------------- +// In-buffer completion popup session (Arc 1a, Q#C2) +// --------------------------------------------------------------------------- + +/// One row of the in-buffer completion popup: a projection of a +/// [`crate::completion_framework::CompletionCandidate`] carrying only +/// what rendering and the accept path need. `insert_text` is already +/// resolved (label fallback applied) so accept never re-derives it. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct PopupCandidate { + /// Display label. + pub label: String, + /// Item kind (drives the glyph column). + pub kind: CompletionItemKind, + /// Optional one-line detail rendered after the label. + pub detail: Option, + /// Text that replaces `[anchor .. cursor]` on accept. + pub insert_text: String, +} + +/// Live state of the in-buffer completion popup (Q#C2). Frontend- +/// agnostic, mirroring [`crate::menu::MenuState`]: the Lua driver +/// publishes into it, the TUI [`CompletionView`] overlay renders from +/// it, the dispatcher's completion shadow navigates/accepts against +/// it, and (phase 2) the semantic producer ships it to the GPU. +/// +/// Unlike the menu's cell anchor, `anchor` is a **byte offset** (the +/// prefix start) --- each frontend maps byte → screen position itself, +/// so the instance never learns a pixel. +pub struct CompletionPopupState { + /// Buffer the popup targets. The session closes the moment the + /// active buffer differs (Q#C3 validation). + pub buffer_id: BufferId, + /// 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. + pub anchor: Position, + /// The prefix as of the last publish (refresh keeps it current). + pub prefix: String, + /// Candidates, best-first. The driver has already scored, dropped + /// non-matches, and capped. + pub candidates: Vec, + /// Highlighted row index into `candidates`. + pub selected: usize, + /// Full candidate count before any cap the driver applied. + pub total: usize, +} + +impl CompletionPopupState { + /// Build a session. Returns `None` when `candidates` is empty --- + /// an empty popup never opens (the driver enforces this too; this + /// is the belt to its suspenders). + #[must_use] + pub fn new( + buffer_id: BufferId, + anchor: Position, + prefix: String, + candidates: Vec, + total: usize, + ) -> Option { + if candidates.is_empty() { + return None; + } + Some(Self { + buffer_id, + anchor, + prefix, + candidates, + selected: 0, + total, + }) + } + + /// Move the highlight by `delta`, wrapping at the ends. + #[allow( + clippy::cast_possible_wrap, + reason = "candidate indices are bounded by Vec::len() which fits in isize on every supported target" + )] + pub fn step(&mut self, delta: isize) { + let len = self.candidates.len() as isize; + if len == 0 { + return; + } + let mut next = (self.selected as isize + delta) % len; + if next < 0 { + next += len; + } + self.selected = next as usize; + } + + /// The highlighted candidate. + #[must_use] + pub fn selected_candidate(&self) -> Option<&PopupCandidate> { + self.candidates.get(self.selected) + } +} + +/// Shared handle to the open popup (`None` when closed). Held by +/// [`crate::editor_core::EditorCore`] and read by [`CompletionView`], +/// the completion twin of [`crate::menu::SharedMenu`]. +pub type SharedCompletionPopup = Arc>>; + +/// A fresh, closed shared popup. +#[must_use] +pub fn make_shared_popup() -> SharedCompletionPopup { + Arc::new(Mutex::new(None)) +} + // --------------------------------------------------------------------------- // View // --------------------------------------------------------------------------- @@ -462,6 +571,18 @@ impl CompletionTriggers { /// rust-analyzer reply. const DEFAULT_POPUP_WIDTH: u32 = 40; +/// Rows the popup shows at once; when more candidates are live the +/// visible slice windows around the selection (mirroring the +/// minibuffer dropdown's `MB_VISIBLE` cap). +const POPUP_MAX_ROWS: u32 = 10; + +/// Minimum popup width in cells (glyph column + a readable label). +const POPUP_MIN_WIDTH: u32 = 12; + +/// Tab-stop width in display columns, matching [`crate::diag`] / +/// [`crate::text_view`]. +const TAB_WIDTH: u32 = 8; + /// Style for the currently-selected row (reverse video so it pops on /// any base palette). fn selected_style() -> Style { @@ -475,111 +596,266 @@ fn selected_style() -> Style { fn kind_style() -> Style { Style { fg: Color::Indexed(8), + bg: Color::Indexed(236), ..Style::default() } } -/// Popup view that draws the completion list at its viewport's -/// origin. The viewport's `cell_size` bounds the popup; the host -/// (Lua) decides where to put it. +/// Popup background (non-selected rows) --- the same dim fill as the +/// context menu, so the popup reads as a floating surface over the +/// buffer text it occludes. +fn popup_style() -> Style { + Style { + fg: Color::Indexed(252), + bg: Color::Indexed(236), + ..Style::default() + } +} + +/// The visible slice of `n` candidates windowed around `selected`: +/// returns `(start, len)`. Mirrors the minibuffer dropdown's centered +/// window so the highlight stays in view as the user cycles. +#[must_use] +pub(crate) fn popup_window(n: usize, selected: usize, max: usize) -> (usize, usize) { + if n <= max { + return (0, n); + } + let half = max / 2; + let start = selected.saturating_sub(half).min(n - max); + (start, max) +} + +/// Self-positioning popup overlay for the in-buffer completion session +/// (Q#C4). Persistent on the active window once attached (deduped by +/// [`View::kind`]); renders nothing while the popup is closed or the +/// window shows a different buffer, mirroring [`crate::menu::MenuView`]'s +/// self-suppressing model. Owns every cell inside the popup rectangle. /// -/// Unlike [`crate::diag::DiagnosticView`], this view does **not** -/// compose over a buffer's text --- it owns every cell inside its -/// viewport. The `_buf` parameter is unused. +/// Placement: the row *below* the anchor's screen row, with as many +/// rows as fit; when nothing fits below, it flips *above* the anchor. +/// The left edge sits at the anchor's display column, shifted left when +/// the popup would overflow the window's right edge. pub struct CompletionView { - key: CompletionKey, - store: SharedCompletionStore, + popup: SharedCompletionPopup, } impl CompletionView { - /// Construct a popup view for `key` against `store`. + /// Build a view reading `popup`. #[must_use] - pub fn new(key: CompletionKey, store: SharedCompletionStore) -> Self { - Self { key, store } + pub fn new(popup: SharedCompletionPopup) -> Self { + Self { popup } + } +} + +/// Display column of `byte_end` within `line_bytes` (tab-aware, +/// UTF-8-aware). The completion twin of the diagnostic underline's +/// column resolution. +fn display_col_for_byte(line_bytes: &[u8], byte_end: u32) -> u32 { + let end = (byte_end as usize).min(line_bytes.len()); + let text = String::from_utf8_lossy(&line_bytes[..end]); + let mut col = 0u32; + for ch in text.chars() { + if ch == '\t' { + col += TAB_WIDTH - (col % TAB_WIDTH); + } else { + col += char_display_width(ch); + } + } + col +} + +/// Resolved popup rectangle, in window-relative cells. +struct PopupRect { + /// First popup row, relative to the viewport's top. + top: u32, + /// Left edge, relative to the viewport's left. + left: u32, + /// Popup width in cells. + width: u32, + /// Rows actually shown (≤ the windowed candidate count). + shown: u32, +} + +/// Map the popup's byte anchor to a clamped on-screen rectangle: +/// below the anchor row when at least one row fits, flipped above +/// otherwise; left edge at the anchor's display column, shifted back +/// from the right margin. `None` when the anchor is scrolled out of +/// the viewport or nothing fits. +fn resolve_popup_rect( + buf: &Buffer, + viewport: Viewport, + anchor: Position, + rows: &[PopupCandidate], +) -> Option { + // Anchor byte → (screen row, display col), the diag-view walk. + let source: Vec = { + let mut bytes = vec![0u8; buf.len() as usize]; + if !bytes.is_empty() { + buf.snapshot_rope().slice(0, buf.len(), &mut bytes); + } + bytes + }; + let anchor = (anchor as usize).min(source.len()) as u32; + let line_offsets = crate::diag::compute_line_offsets(&source); + let start_line = crate::diag::line_at_offset(&line_offsets, viewport.buffer_start as u32); + let anchor_line = crate::diag::line_at_offset(&line_offsets, anchor); + if anchor_line < start_line { + return None; // anchor scrolled above the viewport + } + let anchor_row = anchor_line - start_line; + let max_rows = viewport.cell_size.rows; + let max_cols = viewport.cell_size.cols; + if anchor_row >= max_rows || max_cols == 0 { + return None; // anchor scrolled below the viewport + } + let line_start = line_offsets[anchor_line as usize]; + let line_end = line_offsets + .get(anchor_line as usize + 1) + .copied() + .unwrap_or(source.len() as u32); + let line_bytes = &source[line_start as usize..line_end as usize]; + let anchor_col = display_col_for_byte(line_bytes, anchor - line_start); + + // Vertical placement: below the anchor row when at least one row + // fits, else flipped above. + let want_rows = rows.len() as u32; + let below = max_rows - anchor_row - 1; + let (top, shown) = if below > 0 { + (anchor_row + 1, want_rows.min(below)) + } else { + let shown = want_rows.min(anchor_row); + (anchor_row - shown, shown) + }; + if shown == 0 { + return None; } - /// The key this view is keyed under. - #[must_use] - pub fn key(&self) -> &CompletionKey { - &self.key + // Width: glyph column + widest visible "label detail", clamped to + // the window; left edge shifts back from the right margin. + let widest = rows + .iter() + .map(|c| { + let detail = c.detail.as_deref().map_or(0, |d| d.chars().count() + 2); + (c.label.chars().count() + detail) as u32 + }) + .max() + .unwrap_or(0); + let width = (widest + 3) + .clamp(POPUP_MIN_WIDTH, DEFAULT_POPUP_WIDTH) + .min(max_cols); + let left = anchor_col.min(max_cols - width); + Some(PopupRect { + top, + left, + width, + shown, + }) +} + +/// Paint one popup row (background fill, kind glyph, label + detail) +/// at absolute row `r`, columns `[abs_left .. abs_left + width)`. +fn paint_popup_row( + cells: &mut CellGrid<'_>, + item: &PopupCandidate, + r: u32, + abs_left: u32, + width: u32, + selected: bool, +) { + let row_style = if selected { + selected_style() + } else { + popup_style() + }; + // Paint the whole row's background first so the selected row's + // reverse video covers the trailing whitespace. + for c in 0..width { + let cell = cells.at(CellCoord::new(r, abs_left + c)); + cell.glyph = Glyph::Char(' '); + cell.style = row_style; + cell.attachment = None; + } + // Column 0: kind glyph. + let kind_cell = cells.at(CellCoord::new(r, abs_left)); + kind_cell.glyph = Glyph::Char(item.kind.glyph()); + kind_cell.style = if selected { + selected_style() + } else { + kind_style() + }; + // Columns 2..: label, optionally followed by the detail. + let mut text = String::with_capacity(item.label.len() + 4); + text.push_str(&item.label); + if let Some(detail) = item.detail.as_deref() { + text.push_str(" "); + text.push_str(detail); + } + let mut col: u32 = 2; + for ch in text.chars() { + if col >= width { + break; + } + let cw = char_display_width(ch); + if cw == 0 { + continue; + } + let cell = cells.at(CellCoord::new(r, abs_left + col)); + cell.glyph = Glyph::Char(ch); + cell.style = row_style; + cell.attachment = None; + col += 1; + if cw == 2 && col < width { + let cont = cells.at(CellCoord::new(r, abs_left + col)); + cont.glyph = Glyph::Continuation; + cont.style = row_style; + cont.attachment = None; + col += 1; + } } } impl View for CompletionView { - fn render(&mut self, _buf: &Buffer, viewport: Viewport, cells: &mut CellGrid<'_>) { - let (items, selected) = { - let guard = self.store.lock().expect("completion store poisoned"); - let items = guard.items(&self.key).to_vec(); - (items, guard.selected(&self.key)) - }; + fn kind(&self) -> &'static str { + "completion-popup" + } - let max_rows = viewport.cell_size.rows; - let max_cols = viewport.cell_size.cols.max(1); - let origin = viewport.cell_origin; - let popup_cols = max_cols.min(DEFAULT_POPUP_WIDTH); - - // Clear the popup region. - for r in 0..max_rows { - for c in 0..max_cols { - *cells.at(CellCoord::new(origin.row + r, origin.col + c)) = Cell::default(); + fn render(&mut self, buf: &Buffer, viewport: Viewport, cells: &mut CellGrid<'_>) { + // Snapshot under the lock, then drop it before touching the rope. + let (anchor, rows_data, selected_in_window): (Position, Vec, usize) = { + let guard = self.popup.lock().expect("completion popup poisoned"); + let Some(popup) = guard.as_ref() else { + return; + }; + if popup.buffer_id != buf.id() { + return; // this window shows a different buffer } - } - if items.is_empty() { + let (start, len) = popup_window( + popup.candidates.len(), + popup.selected, + POPUP_MAX_ROWS as usize, + ); + ( + popup.anchor, + popup.candidates[start..start + len].to_vec(), + popup.selected - start, + ) + }; + if rows_data.is_empty() { return; } - - for row in 0..max_rows.min(items.len() as u32) { - let item = &items[row as usize]; - let row_style = if row as usize == selected { - selected_style() - } else { - Style::default() - }; - // Paint the whole row's background first so the selected - // row's reverse video covers the trailing whitespace. - for c in 0..popup_cols { - let cell = cells.at(CellCoord::new(origin.row + row, origin.col + c)); - cell.glyph = Glyph::Char(' '); - cell.style = row_style; - cell.attachment = None; - } - // Column 0: kind glyph. - let kind_cell = cells.at(CellCoord::new(origin.row + row, origin.col)); - kind_cell.glyph = Glyph::Char(item.kind.glyph()); - kind_cell.style = if row as usize == selected { - selected_style() - } else { - kind_style() - }; - // Columns 2..: label, optionally followed by " : detail". - let mut text = String::with_capacity(item.label.len() + 4); - text.push_str(&item.label); - if let Some(detail) = item.detail.as_deref() { - text.push_str(" "); - text.push_str(detail); - } - let mut col: u32 = 2; - for ch in text.chars() { - if col >= popup_cols { - break; - } - let width = char_display_width(ch); - if width == 0 { - continue; - } - let cell = cells.at(CellCoord::new(origin.row + row, origin.col + col)); - cell.glyph = Glyph::Char(ch); - cell.style = row_style; - cell.attachment = None; - col += 1; - if width == 2 && col < popup_cols { - let cont = cells.at(CellCoord::new(origin.row + row, origin.col + col)); - cont.glyph = Glyph::Continuation; - cont.style = row_style; - cont.attachment = None; - col += 1; - } - } + let Some(rect) = resolve_popup_rect(buf, viewport, anchor, &rows_data) else { + return; + }; + let origin = viewport.cell_origin; + for (i, item) in rows_data.iter().take(rect.shown as usize).enumerate() { + paint_popup_row( + cells, + item, + origin.row + rect.top + i as u32, + origin.col + rect.left, + rect.width, + i == selected_in_window, + ); } } } @@ -746,4 +1022,62 @@ mod tests { assert!(r.items.is_empty()); assert!(!r.is_incomplete); } + + // ---- popup session (Arc 1a, Q#C2) --------------------------------------- + + fn cand(label: &str) -> PopupCandidate { + PopupCandidate { + label: label.to_owned(), + kind: CompletionItemKind::Text, + detail: None, + insert_text: label.to_owned(), + } + } + + #[test] + fn popup_state_refuses_empty_candidates() { + assert!( + CompletionPopupState::new(BufferId::from_raw(1), 0, String::new(), vec![], 0).is_none() + ); + } + + #[test] + fn popup_state_step_wraps_both_directions() { + let mut p = CompletionPopupState::new( + BufferId::from_raw(1), + 0, + "ab".into(), + vec![cand("a"), cand("b"), cand("c")], + 3, + ) + .unwrap(); + assert_eq!(p.selected, 0); + p.step(1); + assert_eq!(p.selected, 1); + p.step(4); // 1 + 4 = 5, mod 3 = 2 + assert_eq!(p.selected, 2); + p.step(1); // wraps to the top + assert_eq!(p.selected, 0); + p.step(-1); // wraps to the bottom + assert_eq!(p.selected, 2); + assert_eq!(p.selected_candidate().unwrap().label, "c"); + } + + #[test] + fn popup_window_keeps_selection_visible() { + // Fits: identity window. + assert_eq!(popup_window(3, 0, 10), (0, 3)); + // Overflow: centers on the selection… + assert_eq!(popup_window(30, 15, 10), (10, 10)); + // …clamps at the top… + assert_eq!(popup_window(30, 0, 10), (0, 10)); + assert_eq!(popup_window(30, 2, 10), (0, 10)); + // …and at the bottom. + assert_eq!(popup_window(30, 29, 10), (20, 10)); + // The selected index always falls inside the window. + for sel in 0..30 { + let (start, len) = popup_window(30, sel, 10); + assert!(sel >= start && sel < start + len, "sel {sel} escaped"); + } + } } diff --git a/src/editor.rs b/src/editor.rs index 95d4388..4967a2e 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -493,6 +493,19 @@ impl EditorState { core.active_frontend = frontend_id; } + // Modal surfaces beat the completion popup (Q#C3): if a menu / + // search / minibuffer opened while the popup was up, close the + // popup before the modal shadow swallows this key --- otherwise + // it would linger, rendered but unreachable. + { + let mut core = self.core.borrow_mut(); + if core.completion_popup_is_open() + && (core.menu_is_open() || core.search_active() || core.minibuffer.is_active()) + { + core.completion_popup_close(); + } + } + // Context-menu interception (Q#CM1): while a menu is open every // key drives it (navigate / invoke / dismiss), shadowing the // global keymap like search and the minibuffer. Same shared path @@ -522,6 +535,21 @@ impl EditorState { return; } + // In-buffer completion popup (Q#C3): a PARTIAL shadow, the + // fourth member of the family above. Only the popup-control + // chords (TAB / RET / C-n / C-p / Up / Down / Esc / C-g) are + // intercepted; every other key falls through to normal dispatch + // 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. + if self.core.borrow().completion_popup_is_open() + && let Some(key) = CompletionPopupKey::from_chord(chord) + { + self.dispatch_completion_key(key); + return; + } + // Buffer-scope keybindings need the active buffer id passed // through the dispatcher (otherwise `keymap_stack::resolve` // skips the buffer-local map entirely and every "scope = @@ -575,6 +603,13 @@ impl EditorState { self.lua_host .run_hook("buffer.after-edit", mlua::MultiValue::new()); } + + // Q#C3 post-dispatch validation, deliberately AFTER the + // after-edit hook: the Lua driver may have just refreshed (or + // re-anchored) the popup for this very edit, and validation + // must judge the fresh session, not the stale one. A closed + // popup makes this a single mutex peek. + self.core.borrow_mut().completion_popup_validate(); } /// Active buffer's edit revision, or `None` if the registry no @@ -691,6 +726,28 @@ impl EditorState { } } + /// Drive the open completion popup from an intercepted control + /// chord (Q#C3). Accept (Q#C7) re-validates inside + /// [`EditorCore::completion_popup_accept`] and applies a single + /// Replace edit; when that edit lands, `buffer.after-edit` fires + /// here exactly as it does on the normal dispatch path, so LSP + /// `didChange` and styling refresh ride the existing machinery. + fn dispatch_completion_key(&mut self, key: CompletionPopupKey) { + match key { + CompletionPopupKey::Next => self.core.borrow_mut().completion_popup_step(1), + CompletionPopupKey::Prev => self.core.borrow_mut().completion_popup_step(-1), + CompletionPopupKey::Dismiss => self.core.borrow_mut().completion_popup_close(), + CompletionPopupKey::Accept => { + let pre_revision = self.active_buffer_revision(); + self.core.borrow_mut().completion_popup_accept(); + if pre_revision != self.active_buffer_revision() { + self.lua_host + .run_hook("buffer.after-edit", mlua::MultiValue::new()); + } + } + } + } + /// Drive an open context menu from a keystroke (Q#CM1). fn dispatch_menu_key(&mut self, chord: Chord) { match MenuKey::from_chord(chord) { @@ -1558,6 +1615,56 @@ impl MenuKey { } } +/// Keys intercepted while the in-buffer completion popup is open +/// (Q#C3). Unlike [`SearchKey`] / [`MenuKey`] this is a **partial** +/// shadow: `from_chord` returns `None` for every chord outside the +/// popup-control set, and the dispatcher lets those fall through to +/// normal dispatch --- printable keys keep self-inserting, motion keys +/// keep moving (the post-dispatch validation then decides whether the +/// session survives). The same decode runs in both frontends via the +/// daemon's `FrontendEvent::Key` round-trip. +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +enum CompletionPopupKey { + /// Highlight the next candidate (Down / C-n). + Next, + /// Highlight the previous candidate (Up / C-p). + Prev, + /// Accept the highlighted candidate (TAB / RET). + Accept, + /// Close the popup without accepting (Esc / C-g). + Dismiss, +} + +impl CompletionPopupKey { + /// Decode `chord` into a popup action, or `None` when the chord is + /// not popup control and must fall through to normal dispatch. + fn from_chord(chord: Chord) -> Option { + let ctrl = chord.modifiers.contains(KeyModifiers::CONTROL); + let alt = chord.modifiers.contains(KeyModifiers::ALT); + if !ctrl && !alt { + return match chord.code { + KeyCode::Down => Some(Self::Next), + KeyCode::Up => Some(Self::Prev), + KeyCode::Tab | KeyCode::Enter => Some(Self::Accept), + KeyCode::Esc => Some(Self::Dismiss), + _ => None, + }; + } + if ctrl + && !alt + && let KeyCode::Char(c) = chord.code + { + return match c { + 'n' => Some(Self::Next), + 'p' => Some(Self::Prev), + 'g' => Some(Self::Dismiss), + _ => None, + }; + } + None + } +} + /// Paint one full frame into `grid` and return the desired terminal /// cursor position. /// diff --git a/src/editor_core.rs b/src/editor_core.rs index 021748d..9de7c18 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -187,6 +187,12 @@ pub struct EditorCore { /// from the same state the dispatch path mutates — the menu twin of /// `search_store`. pub menu: crate::menu::SharedMenu, + /// Open in-buffer completion popup (Arc 1a, Q#C2), or `None` when + /// closed. Shared `Arc` so the TUI + /// [`crate::completion::CompletionView`] overlay renders from the + /// same state the dispatch path navigates and the Lua driver + /// publishes into — the completion twin of `menu`. + pub completion_popup: crate::completion::SharedCompletionPopup, } impl EditorCore { @@ -226,6 +232,7 @@ impl EditorCore { clipboard_slot: Vec::new(), pending_clipboard: None, menu: crate::menu::make_shared_menu(), + completion_popup: crate::completion::make_shared_popup(), } } @@ -1818,6 +1825,166 @@ impl EditorCore { } } + // ---- in-buffer completion popup (Arc 1a, Q#C2/Q#C3) -------------------- + + /// True while the in-buffer completion popup is open. + #[must_use] + pub fn completion_popup_is_open(&self) -> bool { + self.completion_popup + .lock() + .expect("completion popup poisoned") + .is_some() + } + + /// Open (or replace) the completion popup session. Attaches the + /// self-suppressing [`crate::completion::CompletionView`] overlay to + /// the active window on first use (deduped by kind, like the menu). + /// 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) { + *self + .completion_popup + .lock() + .expect("completion popup poisoned") = Some(state); + self.ensure_completion_overlay(); + } + + /// Close the popup (the overlay then self-suppresses). + pub fn completion_popup_close(&mut self) { + *self + .completion_popup + .lock() + .expect("completion popup poisoned") = None; + } + + /// Move the popup highlight by `delta` (wrapping). + pub fn completion_popup_step(&mut self, delta: isize) { + if let Some(p) = self + .completion_popup + .lock() + .expect("completion popup poisoned") + .as_mut() + { + p.step(delta); + } + } + + /// Q#C3 session invariant: the popup only survives while the + /// active buffer still matches, the cursor sits at or after the + /// anchor, and every byte between them is a word byte (`[A-Za-z0-9_]` + /// --- the same ASCII word definition the Lua driver uses). A + /// trigger-character session (empty prefix, `cursor == anchor`) + /// holds trivially. Returns the `(anchor, cursor)` pair while the + /// invariant holds. + #[must_use] + fn completion_session_holds(&self) -> Option<(Position, Position)> { + /// Longest byte run still plausibly a completion prefix; past + /// this the session is stale, not a prefix. + const MAX_PREFIX_BYTES: u64 = 512; + + let (buffer_id, anchor) = { + let guard = self + .completion_popup + .lock() + .expect("completion popup poisoned"); + let p = guard.as_ref()?; + (p.buffer_id, p.anchor) + }; + if self.active_buffer_id() != buffer_id { + return None; + } + let cursor = self.active_window().cursor; + if cursor < anchor || cursor - anchor > MAX_PREFIX_BYTES { + return None; + } + let reg = self.registry.borrow(); + let buffer = reg.get(buffer_id).ok()?; + if cursor > buffer.len() { + return None; + } + let mut bytes = vec![0u8; (cursor - anchor) as usize]; + if !bytes.is_empty() { + buffer.snapshot_rope().slice(anchor, cursor, &mut bytes); + } + bytes + .iter() + .all(|b| b.is_ascii_alphanumeric() || *b == b'_') + .then_some((anchor, cursor)) + } + + /// Q#C3 post-dispatch validation: close the popup unless the + /// session invariant still holds. Called by the dispatcher after + /// every fallen-through key (motion, edits, buffer switches) and + /// cheap enough to call unconditionally --- a closed popup is a + /// single mutex peek. + pub fn completion_popup_validate(&mut self) { + if self.completion_popup_is_open() && self.completion_session_holds().is_none() { + self.completion_popup_close(); + } + } + + /// Q#C7 accept: re-validate the session at the moment of accept, + /// close the popup, and --- only when the invariant still holds --- + /// replace `[anchor .. cursor]` with the highlighted candidate's + /// insert text as a **single** edit (one undo step, mirroring + /// [`Self::insert_char_over_region`]). Returns `true` iff the + /// buffer was edited (the dispatcher fires `buffer.after-edit` + /// off that signal). + pub fn completion_popup_accept(&mut self) -> bool { + let holds = self.completion_session_holds(); + let snap = { + let guard = self + .completion_popup + .lock() + .expect("completion popup poisoned"); + guard + .as_ref() + .and_then(|p| p.selected_candidate().map(|c| c.insert_text.clone())) + }; + self.completion_popup_close(); + let (Some((anchor, cursor)), Some(text)) = (holds, snap) else { + return false; + }; + self.active_window_mut().goal_col = None; + // An empty range degenerates to a plain insert (the + // trigger-character case, where nothing was typed yet). + let result = if cursor > anchor { + self.apply_active_edit(EditOp::Replace { + range: Range { + start: anchor, + end: cursor, + }, + bytes: text.as_bytes(), + }) + } else { + self.apply_active_edit(EditOp::Insert { + pos: anchor, + bytes: text.as_bytes(), + }) + }; + if let Err(e) = result { + self.status = format!("completion accept failed: {e}"); + return false; + } + let aw = self.active_window_mut(); + aw.cursor = anchor + text.len() as u64; + aw.selection = None; + true + } + + /// Ensure the active window carries a + /// [`crate::completion::CompletionView`] overlay (deduped by kind). + /// The view reads the shared popup, so one instance suffices; it + /// renders nothing while the popup is closed. + fn ensure_completion_overlay(&mut self) { + let popup = self.completion_popup.clone(); + let win = self.active_window_mut(); + if !win.overlay_kinds().contains(&"completion-popup") { + win.push_overlay(Box::new(crate::completion::CompletionView::new(popup))); + } + } + /// Safely remove `buffer_id` from the registry. Any window that /// was displaying it is redirected to a fallback buffer (`*scratch*`, /// created on demand) so window state never refers to a missing id. @@ -3072,4 +3239,147 @@ mod tests { assert!(!s.search_is_regex()); assert_eq!(s.search_match_summary().1, 1); } + + // ---- in-buffer completion popup (Arc 1a) -------------------------------- + + fn text_of(s: &EditorCore) -> String { + let id = s.active_buffer_id(); + let reg = s.registry.borrow(); + let buf = reg.get(id).expect("active buffer present"); + let mut bytes = vec![0u8; buf.len() as usize]; + if !bytes.is_empty() { + buf.snapshot_rope().slice(0, buf.len(), &mut bytes); + } + String::from_utf8(bytes).expect("test buffers are UTF-8") + } + + fn open_popup(s: &mut EditorCore, anchor: u64, prefix: &str, insert_text: &str) { + let state = crate::completion::CompletionPopupState::new( + s.active_buffer_id(), + anchor, + prefix.to_owned(), + vec![crate::completion::PopupCandidate { + label: insert_text.to_owned(), + kind: crate::completion::CompletionItemKind::Text, + detail: None, + insert_text: insert_text.to_owned(), + }], + 1, + ) + .expect("non-empty candidate list"); + s.completion_popup_open(state); + } + + #[test] + fn completion_popup_open_attaches_self_suppressing_overlay() { + let mut s = from_bytes(b"he\n"); + s.active_window_mut().cursor = 2; + open_popup(&mut s, 0, "he", "hello"); + assert!(s.completion_popup_is_open()); + assert!( + s.active_window() + .overlay_kinds() + .contains(&"completion-popup") + ); + // Re-opening dedups the overlay by kind. + open_popup(&mut s, 0, "he", "hello"); + let kinds = s.active_window().overlay_kinds(); + assert_eq!( + kinds.iter().filter(|k| **k == "completion-popup").count(), + 1 + ); + } + + #[test] + fn completion_popup_validate_survives_word_growth_and_empty_prefix() { + let mut s = from_bytes(b"he world\n"); + s.active_window_mut().cursor = 2; + open_popup(&mut s, 0, "he", "hello"); + s.completion_popup_validate(); + assert!(s.completion_popup_is_open(), "prefix `he` holds"); + // Typing extends the word: still valid. + s.insert_char('l'); + s.completion_popup_validate(); + assert!(s.completion_popup_is_open(), "prefix `hel` holds"); + // Trigger-char shape (cursor == anchor, empty prefix) holds too. + s.completion_popup_close(); + s.active_window_mut().cursor = 2; + open_popup(&mut s, 2, "", "llo"); + s.completion_popup_validate(); + assert!(s.completion_popup_is_open(), "empty prefix at anchor holds"); + } + + #[test] + fn completion_popup_validate_closes_when_invariant_breaks() { + let mut s = from_bytes(b"he world\n"); + s.active_window_mut().cursor = 2; + open_popup(&mut s, 0, "he", "hello"); + // Cursor moved past the word: `[anchor..cursor]` spans a space. + s.active_window_mut().cursor = 4; + s.completion_popup_validate(); + assert!(!s.completion_popup_is_open(), "non-word bytes close it"); + + // Cursor moved before the anchor. + s.active_window_mut().cursor = 2; + open_popup(&mut s, 2, "", "x"); + s.active_window_mut().cursor = 1; + s.completion_popup_validate(); + assert!(!s.completion_popup_is_open(), "cursor < anchor closes it"); + + // Session bound to a buffer that is not the active one. + let other = s.registry.borrow_mut().create("*other*"); + let state = crate::completion::CompletionPopupState::new( + other, + 0, + String::new(), + vec![crate::completion::PopupCandidate { + label: "x".into(), + kind: crate::completion::CompletionItemKind::Text, + detail: None, + insert_text: "x".into(), + }], + 1, + ) + .unwrap(); + s.completion_popup_open(state); + s.completion_popup_validate(); + assert!(!s.completion_popup_is_open(), "wrong buffer closes it"); + } + + #[test] + fn completion_popup_accept_replaces_prefix_as_one_undo_step() { + let mut s = from_bytes(b"he and more\n"); + s.active_window_mut().cursor = 2; + open_popup(&mut s, 0, "he", "hello_world"); + assert!(s.completion_popup_accept()); + assert_eq!(text_of(&s), "hello_world and more\n"); + assert_eq!(s.active_window().cursor, 11); + assert!(!s.completion_popup_is_open(), "accept closes the popup"); + // Q#C7: the replace is a single edit — one undo restores the + // original text (not an intermediate delete-then-insert state). + s.undo(); + assert_eq!(text_of(&s), "he and more\n"); + } + + #[test] + fn completion_popup_accept_empty_prefix_inserts_at_anchor() { + let mut s = from_bytes(b"x.\n"); + s.active_window_mut().cursor = 2; + open_popup(&mut s, 2, "", "method"); + assert!(s.completion_popup_accept()); + assert_eq!(text_of(&s), "x.method\n"); + assert_eq!(s.active_window().cursor, 8); + } + + #[test] + fn completion_popup_accept_is_noop_when_session_stale() { + let mut s = from_bytes(b"he world\n"); + s.active_window_mut().cursor = 2; + open_popup(&mut s, 0, "he", "hello"); + // Simulate a race: the cursor left the word before accept ran. + s.active_window_mut().cursor = 5; + assert!(!s.completion_popup_accept()); + assert_eq!(text_of(&s), "he world\n", "buffer untouched"); + assert!(!s.completion_popup_is_open(), "stale accept still closes"); + } } From 53d771a935fd0e89c22a73d9d4b4e44b7cebeb06 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 7 Jul 2026 12:09:25 -0400 Subject: [PATCH 3/6] feat(completion): Lua driver, popup bindings, LSP scoping + flush seams Q#C1/C9: builtin/runtime/completion.lua reconstructs typing intent from state (buffer.after-edit has no payload): a {buffer, cursor} snapshot recognizes the single-byte-advance typing signature, so paste/undo/kill/remote edits never auto-open; prefix >= 2 opens off the synchronous providers, server trigger chars open a pending session that materializes when the LSP answer lands; refresh-on-typing re-derives the prefix from the text; a core-closed popup suppresses reopen off the same edit (the accept case). completion.at-point on C-M-i covers deliberate invocation; the driver filters collect() to score >= 0 (collect keeps non-matches, merely sorted last). Q#C8: CompletionContext gains uri; the built-in LSP provider scopes to it (legacy global drain only when absent); Lua providers get uri as a trailing ninth positional arg; context_for can now express char triggers + uri. pmacs.lsp.attachment_for_request() exposes the flushing accessor (attached_for_active) so completion requests answer against current text, not the debounced didChange backlog. Q#C2 write path: pmacs.completion.popup_show/popup_hide/popup_visible publish into the core session (kind tags shared with collect() rows, so driver code passes rows straight through). Co-Authored-By: Claude Fable 5 --- builtin/runtime/completion.lua | 228 +++++++++++++++++++++++++++++++++ builtin/runtime/lsp.lua | 13 ++ src/completion_framework.rs | 22 +++- src/editor.rs | 10 ++ src/lua.rs | 1 + src/lua_bindings/mod.rs | 126 ++++++++++++++++-- 6 files changed, 388 insertions(+), 12 deletions(-) create mode 100644 builtin/runtime/completion.lua diff --git a/builtin/runtime/completion.lua b/builtin/runtime/completion.lua new file mode 100644 index 0000000..fa480b8 --- /dev/null +++ b/builtin/runtime/completion.lua @@ -0,0 +1,228 @@ +-- completion.lua --- in-buffer completion popup driver (Arc 1a). +-- +-- Wires the M4.11 provider framework (`pmacs.completion.collect`) and +-- the M4.7 LSP request path to the core's popup session +-- (`pmacs.completion.popup_show/hide`, Q#C2). The dispatcher owns +-- navigation and accept (Q#C3/Q#C7); this file decides WHEN the popup +-- opens, WHAT it shows, and keeps it fresh as the user types. +-- +-- Trigger policy (Q#C9): `buffer.after-edit` carries no payload, so +-- intent is reconstructed from state. A snapshot of {buffer, cursor} +-- from the previous invocation recognizes the single-char typing +-- signature (cursor advanced exactly one byte --- word and LSP +-- trigger characters are all ASCII); paste, undo, kill, and remote +-- edits (any other delta) never auto-open. `C-M-i` +-- (`completion.at-point`) covers deliberate invocation. +-- +-- Framing: docs/in-buffer-completion-framing.md. + +local MIN_PREFIX = 2 -- typed word length before the popup auto-opens +local MAX_ROWS = 64 -- cap on candidates published to the session + +-- Snapshot of the previous after-edit invocation (Q#C9). +local last = { key = nil, cursor = nil } + +-- Driver-side mirror of the session we opened: { key, anchor, +-- pending }. `popup_visible()` is the truth about the popup --- the +-- core closes it independently (validation, accept, dismiss, a modal +-- opening) --- so the mirror only remembers the anchor and detects +-- "the core closed it since we last looked", which doubles as the +-- reopen-after-accept suppressor. `pending = true` marks a session +-- whose popup hasn't opened yet (awaiting the LSP response). +local session = nil + +local function word_prefix_before(buf, cursor) + local start = cursor - 64 + if start < 0 then start = 0 end + local ok, chunk = pcall(function() return buf:slice(start, cursor) end) + if not ok or type(chunk) ~= "string" then return "" end + return chunk:match("[%w_]*$") or "" +end + +local function char_before(buf, cursor) + if cursor < 1 then return nil end + local ok, ch = pcall(function() return buf:slice(cursor - 1, cursor) end) + if ok and type(ch) == "string" and #ch == 1 then return ch end + return nil +end + +local function close_popup() + session = nil + if pmacs.completion.popup_visible() then pmacs.completion.popup_hide() end +end + +-- Collect through the framework, drop non-matches (collect keeps +-- negative-score rows, merely sorted last --- Q#C1), cap, and shape +-- rows for popup_show. Returns the rows plus the uncapped match count. +local function collect_rows(buf, prefix, trigger, trigger_char) + local rec = pmacs.lsp.active_attachment() -- peek: uri/language only + local ok_text, text = pcall(function() return buf:slice(0, buf:len()) end) + if not ok_text or type(text) ~= "string" then return {}, 0 end + local ctx = { + prefix = prefix, + line = pmacs.editor.cursor_line(), + col = pmacs.editor.cursor_col(), + buffer_text = text, + language = rec and rec.language or nil, + uri = rec and rec.uri or nil, -- Q#C8: scope URI-keyed providers + trigger = trigger, + trigger_char = trigger_char, + } + local ok, cands = pcall(pmacs.completion.collect, ctx) + if not ok or type(cands) ~= "table" then return {}, 0 end + local rows, total = {}, 0 + for _, c in ipairs(cands) do + if (c.score or -1) >= 0 then + total = total + 1 + if #rows < MAX_ROWS then + rows[#rows + 1] = { + label = c.label, + kind = c.kind, + detail = c.detail, + insert_text = c.insert_text, + } + end + end + end + return rows, total +end + +-- Re-collect and show the session at `anchor`. On zero matches the +-- popup hides; the caller decides whether the session survives as +-- `pending` (initial trigger-char / at-point opens awaiting the LSP) +-- or dies (a refresh that narrowed to nothing). Returns true when the +-- popup is showing afterwards. +local function publish(buf, anchor, prefix, trigger, trigger_char) + local rows, total = collect_rows(buf, prefix, trigger, trigger_char) + if #rows == 0 then + if pmacs.completion.popup_visible() then pmacs.completion.popup_hide() end + return false + end + session = { key = tostring(buf), anchor = anchor } + pmacs.completion.popup_show { + buffer = buf, + anchor = anchor, + prefix = prefix, + total = total, + candidates = rows, + } + return true +end + +-- Q#C8 "show fast, refresh on arrival": fire textDocument/completion +-- through the FLUSHING accessor (the server must see current text), +-- then re-publish when the response lands --- if the session is still +-- anchored where it was when the request left. +local function request_lsp_then_refresh() + local rec = pmacs.lsp.attachment_for_request() + if not rec or not session then return end + local line = pmacs.editor.cursor_line() + local col = pmacs.editor.cursor_col() + local anchor_at_request = session.anchor + local key_at_request = session.key + pmacs.async(function() + local ok = pcall(function() + pmacs.lsp.request_completion(rec.server, rec.uri, line, col):await() + end) + if not ok or not session then return end + if session.key ~= key_at_request or session.anchor ~= anchor_at_request then return end + local buf = pmacs.window.buffer() + if not buf or tostring(buf) ~= session.key then return end + local cursor = pmacs.editor.cursor() + local prefix = word_prefix_before(buf, cursor) + if cursor - #prefix ~= session.anchor then return end + if not publish(buf, session.anchor, prefix, "incomplete", nil) and session.pending then + -- Still nothing, even with the server's answer: the pending + -- session is dead. + session = nil + end + end) +end + +pmacs.hook.add("buffer.after-edit", function() + local buf = pmacs.window.buffer() + if not buf then + close_popup() + last.key, last.cursor = nil, nil + return + end + local key = tostring(buf) + local cursor = pmacs.editor.cursor() + local prev_key, prev_cursor = last.key, last.cursor + last.key, last.cursor = key, cursor + + local visible = pmacs.completion.popup_visible() + + if session and not session.pending and not visible then + -- The core closed the popup since we opened it (accept, dismiss, + -- validation, or a modal). Drop the mirror and do NOT reopen off + -- this same edit --- this is what stops an accept's own + -- after-edit from instantly re-raising the popup it just closed. + session = nil + return + end + + if visible and session then + -- Refresh the open session from the text. A prefix that no longer + -- reaches back to the anchor means the word died; close (the + -- core's post-dispatch validation independently enforces the same + -- invariant). + if key ~= session.key then + close_popup() + return + end + local prefix = word_prefix_before(buf, cursor) + if cursor < session.anchor or cursor - #prefix ~= session.anchor then + close_popup() + return + end + if not publish(buf, session.anchor, prefix, "incomplete", nil) then + session = nil -- narrowed to nothing: the session is over + end + return + end + + -- Popup closed: the Q#C9 auto-open policy. Same buffer, cursor + -- advanced by exactly one byte since the previous edit. + 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() + end + return + end + if #prefix == 0 then + -- Maybe a server trigger character (`.`, `:`, ...): a pending + -- session anchored at the cursor, opening when candidates arrive. + local ch = char_before(buf, cursor) + local rec = pmacs.lsp.active_attachment() + if not (ch and rec) then return end + local ok, fires = pcall(pmacs.completion.should_fire, rec.server, ch) + if not (ok and fires) then return end + if not publish(buf, cursor, "", "char", ch) then + session = { key = key, anchor = cursor, pending = true } + end + request_lsp_then_refresh() + end +end) + +local function completion_at_point() + local buf = pmacs.window.buffer() + if not buf then return end + local cursor = pmacs.editor.cursor() + local prefix = word_prefix_before(buf, cursor) + local anchor = cursor - #prefix + if not publish(buf, anchor, prefix, "invoked", nil) then + session = { key = tostring(buf), anchor = anchor, pending = true } + end + request_lsp_then_refresh() +end + +pmacs.command.define { + name = "completion.at-point", + description = "Open the in-buffer completion popup at the cursor.", + fn = completion_at_point, +} + +pmacs.keymap.bind { scope = "global", sequence = "C-M-i", command = "completion.at-point" } diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 032f3db..61cbef5 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -544,6 +544,19 @@ function pmacs.lsp.active_attachment() return attachments[tostring(buf)] 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. +function pmacs.lsp.attachment_for_request() + return attached_for_active() +end + -- Hooks -------------------------------------------------------------------- pmacs.hook.add("buffer.after-load", function() diff --git a/src/completion_framework.rs b/src/completion_framework.rs index c16f6f3..a9761b0 100644 --- a/src/completion_framework.rs +++ b/src/completion_framework.rs @@ -119,6 +119,11 @@ pub struct CompletionContext { pub project_root: Option, /// What kicked off the request. pub trigger: CompletionTrigger, + /// Document URI of the buffer being completed (Q#C8 scoping). + /// When set, URI-keyed providers (LSP) surface only this + /// document's entries; when `None` they fall back to the legacy + /// global drain across every cached key. + pub uri: Option, } impl CompletionContext { @@ -133,6 +138,7 @@ impl CompletionContext { language: None, project_root: None, trigger: CompletionTrigger::Invoked, + uri: None, } } } @@ -618,11 +624,15 @@ 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, across every cached `(server_id, uri)` key. The registry's -/// dedup collapses identical entries; the prefix score ranks them. +/// 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. #[must_use] pub fn lsp_completion_provider(lsp: crate::lsp::SharedLspManager) -> ProviderFn { - Box::new(move |_ctx: &CompletionContext| -> Vec { + Box::new(move |ctx: &CompletionContext| -> Vec { let store_handle = { let mgr = lsp.borrow(); mgr.completion_store() @@ -631,7 +641,11 @@ pub fn lsp_completion_provider(lsp: crate::lsp::SharedLspManager) -> ProviderFn return Vec::new(); }; let mut out: Vec = Vec::new(); - let keys: Vec<_> = store.keys().cloned().collect(); + let keys: Vec<_> = store + .keys() + .filter(|k| ctx.uri.as_ref().is_none_or(|uri| 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 4967a2e..c913191 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -256,6 +256,16 @@ impl EditorState { include_str!("../builtin/runtime/lsp.lua"), ) .expect("load lsp builtin chunk"); + // Arc 1a: the in-buffer completion popup driver. Loaded after + // lsp.lua because it drives `pmacs.lsp.request_completion` / + // `pmacs.lsp.attachment_for_request` and after the framework + // install above because it calls `pmacs.completion.collect`. + lua_host + .eval( + Some("@pmacs/builtin/runtime/completion.lua"), + include_str!("../builtin/runtime/completion.lua"), + ) + .expect("load completion builtin chunk"); // T M7.11 bundled-package bootstrap. Through M7.10 the REPL // was loaded directly via `eval(include_str!(...))`; the // M7.11 deliverable migrates it to the package system so it diff --git a/src/lua.rs b/src/lua.rs index 91221fc..da6f5fd 100644 --- a/src/lua.rs +++ b/src/lua.rs @@ -239,6 +239,7 @@ impl LuaHost { /// or loading the builtin chunks. pub fn attach_editor(&mut self, core: &SharedCore) -> mlua::Result<()> { lua_bindings::install_editor(&self.lua, core)?; + lua_bindings::install_completion_popup(&self.lua, core)?; self.core = Some(core.clone()); // Hooks first: command bodies in default.lua reference them. self.load_builtin( diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 523acc6..f70f2e9 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -9638,7 +9638,7 @@ fn lua_table_to_completion_item(t: &Table) -> mlua::Result mlua::Result { - let t = lua.create_table_with_capacity(0, 7)?; + let t = lua.create_table_with_capacity(0, 8)?; t.set("prefix", ctx.prefix.as_str())?; t.set("line", ctx.line)?; t.set("col", ctx.col)?; @@ -9649,6 +9649,9 @@ fn ctx_to_lua(lua: &Lua, ctx: &CompletionContext) -> mlua::Result
{ if let Some(p) = &ctx.project_root { t.set("project_root", p.display().to_string())?; } + if let Some(u) = &ctx.uri { + t.set("uri", u.as_str())?; + } let (trigger_tag, trigger_char): (&'static str, Option) = match ctx.trigger { CompletionTrigger::Invoked => ("invoked", None), CompletionTrigger::Char(c) => ("char", Some(c.to_string())), @@ -9677,6 +9680,7 @@ fn lua_table_to_ctx(t: &Table) -> CompletionContext { Some("incomplete") => CompletionTrigger::Incomplete, _ => CompletionTrigger::Invoked, }; + let uri: Option = t.get::>("uri").ok().flatten(); CompletionContext { prefix, line, @@ -9685,6 +9689,7 @@ fn lua_table_to_ctx(t: &Table) -> CompletionContext { language, project_root: project_root.map(std::path::PathBuf::from), trigger, + uri, } } @@ -9949,11 +9954,22 @@ pub fn install_completion_framework( // ctx_to_lua exposed as `pmacs.completion.context_for(...)` // for callers that need to construct a context table from // primitives. Convenience only --- callers can build their - // own. + // own. Trailing optionals: a "char" trigger needs its + // `trigger_char` (Q#C1 nit --- the helper previously could + // not express `CompletionTrigger::Char` at all), and `uri` + // scopes URI-keyed providers (Q#C8). m.set( "context_for", lua.create_function(move |lua, args: ContextForArgs| { - let (prefix, line, col, buffer_text, language, project_root, trigger) = args; + let (prefix, line, col, buffer_text, language, project_root, trigger, ch, uri) = + args; + let trigger = match trigger.as_deref() { + Some("incomplete") => CompletionTrigger::Incomplete, + Some("char") => ch + .and_then(|s| s.chars().next()) + .map_or(CompletionTrigger::Invoked, CompletionTrigger::Char), + _ => CompletionTrigger::Invoked, + }; let ctx = CompletionContext { prefix, line: line.unwrap_or(0), @@ -9961,11 +9977,8 @@ pub fn install_completion_framework( buffer_text: Rc::from(buffer_text.unwrap_or_default()), language, project_root: project_root.map(std::path::PathBuf::from), - trigger: if trigger.as_deref() == Some("incomplete") { - CompletionTrigger::Incomplete - } else { - CompletionTrigger::Invoked - }, + trigger, + uri, }; ctx_to_lua(lua, &ctx) })?, @@ -10007,6 +10020,97 @@ pub fn make_completion_framework( Ok((registry, snippets)) } +/// Install the in-buffer completion popup surface (Arc 1a, Q#C2) into +/// `pmacs.completion`: `popup_show{...}` publishes a session into the +/// core's shared popup (the Lua driver's write path), `popup_hide()` +/// closes it, `popup_visible()` peeks. Separate from +/// [`install_completion_framework`] because these need the +/// [`SharedCore`], which only exists once the editor attaches. +pub fn install_completion_popup(lua: &Lua, core: &SharedCore) -> mlua::Result<()> { + let pmacs: Table = lua.globals().get("pmacs")?; + let m: Table = match pmacs.get::>("completion")? { + Some(t) => t, + None => lua.create_table()?, + }; + + { + // popup_show{ buffer, anchor, prefix?, total?, candidates = { + // { label, kind?, detail?, insert_text? }, ... } } -> bool + // + // Returns false (popup left closed) for an empty candidate + // list. `kind` uses the same string tags as + // `pmacs.completion.collect` rows, so driver code can pass + // collect() output straight through. + let cc = core.clone(); + m.set( + "popup_show", + lua.create_function(move |_, spec: Table| { + let buffer: BufferIdLua = spec.get("buffer")?; + let anchor: u64 = spec.get("anchor")?; + let prefix: String = spec + .get::>("prefix") + .ok() + .flatten() + .unwrap_or_default(); + let rows: Table = spec.get("candidates")?; + let mut candidates = Vec::new(); + for row in rows.sequence_values::
() { + let row = row?; + let label: String = row.get("label")?; + let kind_tag: Option = row.get::>("kind").ok().flatten(); + let kind = kind_tag.as_deref().map_or( + crate::completion::CompletionItemKind::Text, + completion_kind_from_tag, + ); + let detail: Option = row.get::>("detail").ok().flatten(); + let insert_text: Option = + row.get::>("insert_text").ok().flatten(); + candidates.push(crate::completion::PopupCandidate { + insert_text: insert_text.unwrap_or_else(|| label.clone()), + label, + kind, + detail, + }); + } + let total: usize = spec + .get::>("total") + .ok() + .flatten() + .unwrap_or(candidates.len()); + let Some(state) = crate::completion::CompletionPopupState::new( + buffer.0, anchor, prefix, candidates, total, + ) else { + return Ok(false); + }; + cc.borrow_mut().completion_popup_open(state); + Ok(true) + })?, + )?; + } + + { + let cc = core.clone(); + m.set( + "popup_hide", + lua.create_function(move |_, ()| { + cc.borrow_mut().completion_popup_close(); + Ok(()) + })?, + )?; + } + + { + let cc = core.clone(); + m.set( + "popup_visible", + lua.create_function(move |_, ()| Ok(cc.borrow().completion_popup_is_open()))?, + )?; + } + + pmacs.set("completion", m)?; + Ok(()) +} + /// Argument tuple passed to a Lua-registered completion provider. /// Positional rather than table-based because the provider closure /// has no `&Lua` to build a table with at call time. @@ -10019,6 +10123,7 @@ type LuaProviderArgs = ( Option, String, Option, + Option, ); /// Argument tuple for `pmacs.completion.context_for`: positional @@ -10031,6 +10136,8 @@ type ContextForArgs = ( Option, Option, Option, + Option, + Option, ); /// mlua doesn't accept arbitrary Rust types as call arguments @@ -10053,6 +10160,9 @@ fn lua_compat_ctx_args(ctx: &CompletionContext) -> LuaProviderArgs { ctx.project_root.as_ref().map(|p| p.display().to_string()), trigger_tag.to_owned(), trigger_char, + // Trailing addition (Q#C8): existing Lua providers that + // ignore the ninth positional arg are unaffected. + ctx.uri.clone(), ) } From c01cfd1e938f4816529a8ea80f3e190c635b8b36 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 7 Jul 2026 12:49:47 -0400 Subject: [PATCH 4/6] test(completion): popup acceptance suite; fix pmacs.completion table clobber Seven end-to-end tests through dispatch_key: dabbrev auto-open + TAB accept, C-n/RET second-candidate accept, Esc dismiss with fall-through typing (and no same-edit reopen), Home-breaks-anchor validation close, yank-shaped edits never auto-open (Q#C9), C-M-i below the threshold, and ctx.uri scoping through the Lua provider surface. The suite immediately caught a real wiring bug: install_completion (M4.7, runs at make_lsp_manager time) built pmacs.completion with a fresh lua.create_table(), clobbering the popup bindings installed at editor-attach time --- popup_visible was nil at runtime and the driver hook errored silently into *errors* on every edit. It now merges into the existing table, the same idiom as install_completion_framework, so installer order no longer matters. Co-Authored-By: Claude Fable 5 --- src/lua_bindings/mod.rs | 9 +- tests/completion_popup_acceptance.rs | 249 +++++++++++++++++++++++++++ 2 files changed, 257 insertions(+), 1 deletion(-) create mode 100644 tests/completion_popup_acceptance.rs diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index f70f2e9..0961a9e 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -8165,7 +8165,14 @@ fn completion_item_to_lua(lua: &Lua, item: &CompletionItem) -> mlua::Result mlua::Result<()> { let pmacs: Table = lua.globals().get("pmacs")?; - let m = lua.create_table()?; + // Merge into an existing `pmacs.completion` (the popup surface + // installs at editor-attach time, before the LSP manager exists) + // instead of clobbering it --- the same idiom as + // `install_completion_framework`. + let m: Table = match pmacs.get::>("completion")? { + Some(t) => t, + None => lua.create_table()?, + }; { let mgr = manager.clone(); diff --git a/tests/completion_popup_acceptance.rs b/tests/completion_popup_acceptance.rs new file mode 100644 index 0000000..6243c25 --- /dev/null +++ b/tests/completion_popup_acceptance.rs @@ -0,0 +1,249 @@ +//! In-buffer completion popup acceptance (Arc 1a) --- the phase-1 +//! TUI/core path end-to-end: the Lua driver's Q#C9 auto-open policy, +//! the Q#C3 partial dispatcher shadow, and Q#C7 validated accept, all +//! driven through `dispatch_key` exactly as a terminal user would. +//! +//! The LSP provider's request path is covered by the m4_5 fake-LSP +//! suite; these tests run hermetic (dabbrev + custom Lua providers) +//! so no server binary is needed. +//! +//! Framing: docs/in-buffer-completion-framing.md. + +use crossterm::event::{KeyCode, KeyEvent, KeyEventKind, KeyEventState, KeyModifiers}; +use pmacs::editor::EditorState; +use pmacs::protocol::FrontendId; + +fn key(code: KeyCode, mods: KeyModifiers) -> KeyEvent { + KeyEvent { + code, + modifiers: mods, + kind: KeyEventKind::Press, + state: KeyEventState::empty(), + } +} + +fn type_str(s: &mut EditorState, text: &str) { + for ch in text.chars() { + s.dispatch_key( + FrontendId::LOCAL, + key(KeyCode::Char(ch), KeyModifiers::NONE), + ); + } +} + +/// `(buffer text, popup visible?, cursor)` probed through the Lua +/// surface --- the same introspection a user-facing script would use. +fn probe(s: &EditorState) -> (String, bool, i64) { + s.lua_host + .lua() + .load( + " + local b = pmacs.window.buffer() + local text = b:slice(0, b:len()) + return text, pmacs.completion.popup_visible(), pmacs.editor.cursor() + ", + ) + .eval() + .expect("probe buffer/popup state") +} + +/// Typing a two-char prefix of an existing buffer word auto-opens the +/// popup off dabbrev (Q#C9 single-char signature), and TAB accepts: +/// the prefix is replaced by the candidate in one step. +#[test] +fn typing_opens_popup_and_tab_accepts() { + let mut s = EditorState::new(); + type_str(&mut s, "hello_world "); + let (_, visible, _) = probe(&s); + assert!( + !visible, + "no popup while typing the only word in the buffer" + ); + + type_str(&mut s, "he"); + let (_, visible, _) = probe(&s); + assert!( + visible, + "prefix `he` with dabbrev match `hello_world` opens" + ); + + s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Tab, KeyModifiers::NONE)); + let (text, visible, cursor) = probe(&s); + assert_eq!(text, "hello_world hello_world", "TAB replaces the prefix"); + assert!(!visible, "accept closes the popup"); + assert_eq!(cursor, 23, "cursor lands just past the inserted text"); +} + +/// C-n moves the highlight before RET accepts, so the second +/// candidate wins. Uses a custom provider for a deterministic order. +#[test] +fn ctrl_n_navigates_then_ret_accepts_second_candidate() { + let mut s = EditorState::new(); + s.lua_host + .lua() + .load( + " + pmacs.completion.register({ + name = 'test_src', + priority = 200, + fn = function() + return { + { label = 'aardvark', kind = 'text' }, + { label = 'aardwolf', kind = 'text' }, + } + end, + }) + ", + ) + .exec() + .expect("register test provider"); + + type_str(&mut s, "aa"); + let (_, visible, _) = probe(&s); + assert!(visible, "custom provider matches the `aa` prefix"); + + s.dispatch_key( + FrontendId::LOCAL, + key(KeyCode::Char('n'), KeyModifiers::CONTROL), + ); + s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Enter, KeyModifiers::NONE)); + let (text, visible, _) = probe(&s); + assert_eq!(text, "aardwolf", "C-n selected the second candidate"); + assert!(!visible); +} + +/// Esc dismisses without touching the buffer, and the next printable +/// key self-inserts normally (the shadow is partial, not modal). +#[test] +fn esc_dismisses_and_typing_falls_through() { + let mut s = EditorState::new(); + type_str(&mut s, "hello_world he"); + let (_, visible, _) = probe(&s); + assert!(visible); + + s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Esc, KeyModifiers::NONE)); + let (text, visible, _) = probe(&s); + assert_eq!(text, "hello_world he", "Esc leaves the buffer untouched"); + assert!(!visible, "Esc dismisses"); + + type_str(&mut s, "x"); + let (text, visible, _) = probe(&s); + assert_eq!(text, "hello_world hex", "typing after Esc self-inserts"); + assert!(!visible, "the dismissed popup does not reopen off that key"); +} + +/// Motion that breaks the anchor invariant closes the popup via the +/// post-dispatch validation (Q#C3): Home moves the cursor before the +/// anchor. +#[test] +fn motion_before_anchor_closes_popup() { + let mut s = EditorState::new(); + type_str(&mut s, "hello_world he"); + let (_, visible, _) = probe(&s); + assert!(visible); + + s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Home, KeyModifiers::NONE)); + let (text, visible, _) = probe(&s); + assert_eq!(text, "hello_world he"); + assert!(!visible, "cursor before the anchor invalidates the session"); +} + +/// A multi-byte edit (kill-ring yank) never auto-opens the popup --- +/// the Q#C9 single-char signature rejects paste-shaped deltas. +#[test] +fn yank_shaped_edit_does_not_auto_open() { + let mut s = EditorState::new(); + type_str(&mut s, "hello_world hello"); + // Select the trailing word and cut it (C-w): the popup that was + // open over `hello` closes as its word dies. + for _ in 0..5 { + s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Left, KeyModifiers::SHIFT)); + } + s.dispatch_key( + FrontendId::LOCAL, + key(KeyCode::Char('w'), KeyModifiers::CONTROL), + ); + let (text, visible, _) = probe(&s); + assert_eq!(text, "hello_world "); + assert!(!visible, "cutting the word closes the popup"); + + // Yank it back: one edit, five bytes --- not a typing signature. + s.dispatch_key( + FrontendId::LOCAL, + key(KeyCode::Char('y'), KeyModifiers::CONTROL), + ); + let (text, visible, _) = probe(&s); + assert_eq!(text, "hello_world hello", "yank restored the word"); + assert!(!visible, "a 5-byte edit must not auto-open the popup"); +} + +/// `completion.at-point` (C-M-i) opens deliberately, even below the +/// auto-open prefix threshold. +#[test] +fn at_point_command_opens_below_threshold() { + let mut s = EditorState::new(); + type_str(&mut s, "hello_world h"); + let (_, visible, _) = probe(&s); + assert!(!visible, "a 1-char prefix stays below the auto-open bar"); + + s.dispatch_key( + FrontendId::LOCAL, + key( + KeyCode::Char('i'), + KeyModifiers::CONTROL | KeyModifiers::ALT, + ), + ); + let (_, visible, _) = probe(&s); + assert!(visible, "C-M-i opens the popup on demand"); + + s.dispatch_key(FrontendId::LOCAL, key(KeyCode::Tab, KeyModifiers::NONE)); + let (text, _, _) = probe(&s); + 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. +#[test] +fn collect_scopes_lsp_candidates_to_ctx_uri() { + let s = EditorState::new(); + let (scoped, unscoped): (u64, u64) = s + .lua_host + .lua() + .load( + " + -- Seed the M4.7 LSP store for two URIs through a custom + -- provider is not possible (the store is Rust-side), so + -- emulate the shape: a URI-aware provider that mirrors + -- what the built-in LSP provider does with ctx.uri. + pmacs.completion.register({ + name = 'uri_aware', + priority = 150, + fn = function(prefix, line, col, text, lang, root, trig, tchar, uri) + local by_uri = { + ['file:///a.rs'] = { { label = 'alpha_from_a' } }, + ['file:///b.rs'] = { { label = 'alpha_from_b' } }, + } + if uri then + return by_uri[uri] or {} + end + local all = {} + for _, items in pairs(by_uri) do + for _, it in ipairs(items) do all[#all + 1] = it end + end + return all + end, + }) + local scoped = pmacs.completion.collect({ + prefix = 'alpha', uri = 'file:///a.rs', + }) + local unscoped = pmacs.completion.collect({ prefix = 'alpha' }) + return #scoped, #unscoped + ", + ) + .eval() + .expect("collect with and without uri"); + assert_eq!(scoped, 1, "ctx.uri reaches Lua providers (9th arg)"); + assert_eq!(unscoped, 2, "no uri → all documents"); +} From a1f5b1ffd7211c29f4fa1a75266f9c198e8a82ff Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 7 Jul 2026 12:54:33 -0400 Subject: [PATCH 5/6] fix(worker): deterministic pool teardown; EditorState::drop signals workers Every EditorState leaked its entire worker pool (cores-1 threads, each waking every 100ms): the Rc is cloned into dozens of Lua closures, and registries those closures capture store mlua::Function values -- reference cycles through the Lua VM that keep the Rc from ever reaching zero, so WorkerPool::Drop never ran. Harmless for one editor per process; in the m4 acceptance suite (54 editor-building tests) it accumulated 1000+ live threads (observed: 60 complete pmacs-worker-0..14 pools at once) plus ~9k spurious wakeups/second. Fix: WorkerPool::signal_shutdown() -- set the shutdown flag + wake the parkers from a shared reference; parked workers exit within their 100ms park timeout. Exposed as AsyncRuntime::shutdown_workers(), called from a new impl Drop for EditorState. Deliberately signal-only, NO join on the drop path: a worker can be blocked publishing its reply onto the bus only the main thread drains, and a first cut that joined in Drop deadlocked the m4 suite at teardown (fake-LSP tests wedged 2h+). WorkerPool::shutdown() (signal + join) remains for owners with no bus consumer to deadlock against; Drop delegates to it as before. Measured on the m4 suite: peak live threads 1026 -> 122. Regression: tests/worker_shutdown_acceptance.rs (thread-count probe is /proc-based, Linux-gated; the idempotence test runs everywhere). Co-Authored-By: Claude Fable 5 --- src/async_runtime.rs | 14 +++++ src/editor.rs | 26 ++++++++++ src/worker.rs | 77 ++++++++++++++++++++++------ tests/completion_popup_acceptance.rs | 2 +- tests/worker_shutdown_acceptance.rs | 47 +++++++++++++++++ 5 files changed, 149 insertions(+), 17 deletions(-) create mode 100644 tests/worker_shutdown_acceptance.rs diff --git a/src/async_runtime.rs b/src/async_runtime.rs index cad4235..94ae001 100644 --- a/src/async_runtime.rs +++ b/src/async_runtime.rs @@ -602,6 +602,20 @@ impl AsyncRuntime { Self::with_pool(WorkerPool::new(size)) } + /// Signal the worker-pool threads to exit (see + /// [`crate::worker::WorkerPool::signal_shutdown`]). Idempotent, + /// non-blocking. Called from [`crate::editor::EditorState`]'s + /// `Drop`: the runtime's `Rc` is captured into Lua-VM reference + /// cycles and never reaches a zero refcount, so without this + /// explicit teardown every editor instance leaks its whole pool + /// --- one leaked pool per test in a suite that builds real + /// editors. Signal-only (no join): a worker blocked handing its + /// reply to the main thread must not deadlock the main thread's + /// drop. + pub fn shutdown_workers(&self) { + self.pool.signal_shutdown(); + } + /// Returns the number of in-flight or settled-but-not-yet-taken /// pending entries. #[must_use] diff --git a/src/editor.rs b/src/editor.rs index c913191..88bdac7 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -102,6 +102,32 @@ pub struct EditorState { mouse_click: Option, } +impl Drop for EditorState { + /// Tear down the worker-pool threads. + /// + /// The `Rc` is cloned into dozens of Lua closures + /// (`pmacs.workers`, LSP request wrappers, ...), and several of + /// the registries those closures capture themselves store + /// `mlua::Function` values --- reference cycles through the Lua + /// VM that keep the `Rc` from ever reaching zero. Harmless for a + /// single editor per process (the OS reclaims at exit), but a + /// test binary that builds one `EditorState` per test would leak + /// one full worker pool (`cores - 1` threads, each waking every + /// 100ms) per test --- observed as 1000+ live threads in the m4 + /// acceptance suite. Dropping the editor reaches the pool through + /// its own `Rc` clone and signals the threads down regardless of + /// the cycle; parked workers exit within their 100ms wakeup. + /// + /// Signal-only, NO join: a worker can be blocked publishing its + /// reply onto the bus that this (main) thread drains --- joining + /// here deadlocked the m4 suite at teardown for hours. A worker + /// stuck mid-handoff stays alive (bounded by its job), which is + /// still a ~15x improvement over leaking every pool whole. + fn drop(&mut self) { + self.async_runtime.shutdown_workers(); + } +} + #[derive(Copy, Clone)] struct MouseClickState { frontend_id: FrontendId, diff --git a/src/worker.rs b/src/worker.rs index 03a41a4..ade0a2d 100644 --- a/src/worker.rs +++ b/src/worker.rs @@ -175,7 +175,17 @@ impl PoolShared { /// thread). pub struct WorkerPool { shared: Arc, - workers: Vec>, + /// Join handles, drained exactly once by [`Self::shutdown`] + /// (directly or via `Drop`). Behind a `Mutex` so shutdown works + /// from a shared reference: the pool's owner is typically an + /// `Rc` cloned into Lua closures, and those clones + /// form VM reference cycles that keep the `Rc` from ever + /// reaching zero --- an embedder that merely *drops* its handle + /// would leak every worker thread. `EditorState::drop` calls + /// `shutdown()` explicitly instead. + workers: Mutex>>, + /// Thread count at construction (stable across shutdown). + size: usize, } impl WorkerPool { @@ -194,7 +204,7 @@ impl WorkerPool { next_id: AtomicU64::new(0), parker: (Mutex::new(()), Condvar::new()), }); - let workers = local_queues + let workers: Vec> = local_queues .into_iter() .enumerate() .map(|(idx, local)| { @@ -205,7 +215,11 @@ impl WorkerPool { .expect("spawn worker thread") }) .collect(); - Self { shared, workers } + Self { + shared, + size, + workers: Mutex::new(workers), + } } /// Build a pool sized at `available_parallelism - 1`, with a @@ -218,10 +232,48 @@ impl WorkerPool { Self::new(cores.saturating_sub(1)) } - /// Number of worker threads owned by this pool. + /// Number of worker threads this pool was built with. #[must_use] pub fn size(&self) -> usize { - self.workers.len() + self.size + } + + /// Signal every worker to exit, without joining. Idle (parked) + /// workers observe the flag within their 100ms park timeout and + /// return; a worker mid-job exits when its job finishes. Queued + /// jobs that haven't been picked up are dropped without running; + /// jobs dispatched *after* the signal are never picked up. + /// + /// Exists as an explicit method (not just `Drop`) because the + /// pool's owning `Rc` is captured into Lua-VM + /// reference cycles and may never be reclaimed --- callers that + /// know the editor is going away (`EditorState::drop`) signal the + /// threads down regardless. + /// + /// Deliberately does NOT join: a worker can be blocked publishing + /// its reply onto the message bus that only the *main thread* + /// drains, so a main-thread join here is a deadlock (observed as + /// the m4 acceptance suite wedging for hours at teardown). Callers + /// that own the whole world and want the join use + /// [`Self::shutdown`] (or just drop the pool). + pub fn signal_shutdown(&self) { + self.shared.shutdown.store(true, Ordering::Release); + self.shared.notify_all(); + } + + /// [`Self::signal_shutdown`] plus a join of every worker thread. + /// Idempotent: the second call finds no handles and returns + /// immediately. Only safe where no worker can be blocked on the + /// caller's own thread (see `signal_shutdown`); `Drop` uses it + /// because a pool being dropped has no live bus consumer to + /// deadlock against in the bare-pool case. + pub fn shutdown(&self) { + self.signal_shutdown(); + let handles: Vec> = + std::mem::take(&mut *self.workers.lock().expect("worker pool mutex poisoned")); + for handle in handles { + let _ = handle.join(); + } } /// Submit `work` to be run on a worker. Returns a [`JobHandle`] @@ -263,18 +315,11 @@ impl WorkerPool { } impl Drop for WorkerPool { - /// Shutdown semantics: dropping the pool signals every worker - /// to exit at its next idle wakeup and joins them. Running - /// jobs run to completion (or to their own cancellation - /// check); queued jobs that haven't been picked up are dropped - /// without running. Tests and callers that want explicit - /// shutdown just `drop(pool)`. + /// Dropping the pool is an implicit [`Self::shutdown`]: every + /// worker is signalled to exit at its next idle wakeup and + /// joined. No-op when `shutdown` already ran. fn drop(&mut self) { - self.shared.shutdown.store(true, Ordering::Release); - self.shared.notify_all(); - for handle in self.workers.drain(..) { - let _ = handle.join(); - } + self.shutdown(); } } diff --git a/tests/completion_popup_acceptance.rs b/tests/completion_popup_acceptance.rs index 6243c25..774f204 100644 --- a/tests/completion_popup_acceptance.rs +++ b/tests/completion_popup_acceptance.rs @@ -3,7 +3,7 @@ //! the Q#C3 partial dispatcher shadow, and Q#C7 validated accept, all //! driven through `dispatch_key` exactly as a terminal user would. //! -//! The LSP provider's request path is covered by the m4_5 fake-LSP +//! The LSP provider's request path is covered by the `m4_5` fake-LSP //! suite; these tests run hermetic (dabbrev + custom Lua providers) //! so no server binary is needed. //! diff --git a/tests/worker_shutdown_acceptance.rs b/tests/worker_shutdown_acceptance.rs new file mode 100644 index 0000000..116aae4 --- /dev/null +++ b/tests/worker_shutdown_acceptance.rs @@ -0,0 +1,47 @@ +//! 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`). +//! +//! 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. + +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). +#[cfg(target_os = "linux")] +fn live_threads() -> usize { + std::fs::read_dir("/proc/self/task").map_or(0, std::iter::Iterator::count) +} + +#[cfg(target_os = "linux")] +#[test] +fn editor_state_drop_releases_worker_threads() { + 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. + std::thread::sleep(std::time::Duration::from_millis(300)); + let after = live_threads(); + assert!( + after <= baseline + 2, + "worker threads leak across EditorState drop: \ + baseline {baseline}, after 3 create/drop cycles {after}" + ); +} + +/// Platform-independent variant: the pool reports itself dead after +/// an explicit shutdown, and shutdown is idempotent. +#[test] +fn explicit_shutdown_is_idempotent() { + let s = EditorState::new(); + s.async_runtime.shutdown_workers(); + s.async_runtime.shutdown_workers(); // second call must not hang or panic + drop(s); // drop runs shutdown a third time +} From 61a31b3ad488c2a5762ca37d2ffec8426e0c5418 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 7 Jul 2026 16:54:14 -0400 Subject: [PATCH 6/6] fix(completion): address TUI-validation findings (LSP query gaps, scoping, prefix keys, window scope) Five findings from the manual validation pass, all in-branch: 1. LSP-only words never queried the server: the auto-open path fired request_completion only when the sync providers already produced rows. An empty sweep now leaves a pending session and the request always fires; isIncomplete responses re-request on further typing. Corollary: attachment_for_request now flushes-if-attached but NEVER attaches -- the first cut wrapped attached_for_active, which spawns servers on demand, i.e. per-keystroke spawn attempts in unattached buffers (wedged the parallel m4 suite; serial ran 3x slower). Attachment stays buffer-open policy. 2. Cross-buffer LSP leak: the built-in provider's no-uri fallback was the legacy global store drain, so scratch/unattached buffers could show another file's cached completions. Strict now: no uri, no rows. 3. Pending prefixes own the keyboard: Action::Pending (C-x ...) dismisses the popup and the popup shadow is guarded on an empty dispatcher prefix, so the sequence's continuation and its C-g abort reach the dispatcher instead of the popup. 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. Flaky worker test: the /proc thread-count probe and the idempotence check both build EditorStates and could run concurrently, polluting the baseline; merged into one test (non-Linux keeps a portable idempotence variant). Regression tests for 1-4; framing doc gains the as-built notes. Co-Authored-By: Claude Fable 5 --- builtin/runtime/completion.lua | 19 ++++++-- builtin/runtime/lsp.lua | 24 ++++++---- docs/in-buffer-completion-framing.md | 36 +++++++++++++++ src/completion.rs | 24 ++++++++-- src/completion_framework.rs | 21 +++++---- src/editor.rs | 14 +++++- src/editor_core.rs | 36 +++++++++++++-- tests/completion_popup_acceptance.rs | 66 ++++++++++++++++++++++++++-- tests/worker_shutdown_acceptance.rs | 31 +++++++++---- 9 files changed, 227 insertions(+), 44 deletions(-) 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();