diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 3e9a40d..70b520e 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -452,11 +452,28 @@ function pull_inlay_hints_quiet(rec) end) end -local function server_supports_semantic_tokens(sid) +-- LSP defines `semanticTokensProvider.full` and `.range` as optional, +-- INDEPENDENT capabilities: a provider may be range-only, and sending +-- it /full gets a rejection the pull path would swallow. Gate each +-- request kind on its own capability. +local function semantic_provider(sid) local ok, caps = pcall(pmacs.lsp.capabilities, sid) - if not ok or not caps then return false end + if not ok or not caps then return nil end local p = caps.semanticTokensProvider - return p ~= nil and p ~= false + if p == nil or p == false then return nil end + return p +end + +local function server_supports_semantic_full(sid) + local p = semantic_provider(sid) + if type(p) ~= "table" then return false end + return p.full == true or type(p.full) == "table" +end + +local function server_supports_semantic_range(sid) + local p = semantic_provider(sid) + if type(p) ~= "table" then return false end + return p.range == true or type(p.range) == "table" end -- Arc 1c. Semantic tokens are pull-model, exactly like inlay hints: the @@ -470,24 +487,50 @@ end -- -- Assigns the forward-declared local above (a fresh `local function` -- here would shadow it, leaving `flush_did_change`'s upvalue nil). +-- Whether the server negotiated DELTA semantic-token support: +-- `semanticTokensProvider.full` must be a table with `delta == true`. +-- Holding a `resultId` does NOT imply delta capability — servers may +-- return one from /full regardless — and a conforming full-only server +-- rejects /full/delta. The pull path swallows request errors, so +-- requesting delta without the capability would leave styling silently +-- stale after the first edit. +local function server_supports_semantic_delta(sid) + local p = semantic_provider(sid) + if type(p) ~= "table" then return false end + return type(p.full) == "table" and p.full.delta == true +end + function pull_semantic_tokens_quiet(rec) if not rec or not server_is_initialized(rec.server) then return end - if not server_supports_semantic_tokens(rec.server) then return end + local has_full = server_supports_semantic_full(rec.server) + local has_range = server_supports_semantic_range(rec.server) + if not has_full and not has_range then return end -- The server must see the current text before computing token -- positions against it. A no-op when called from `flush_did_change` -- itself (the pending entry is removed before the send). flush_did_change_for(rec) - -- Delta when we hold a `resultId` (the server only returns one when it - -- supports delta), full otherwise --- matching `pmacs.lsp - -- .semantic_tokens()`. Never clear the store first: a delta splices - -- against the retained raw stream. - local prev = pmacs.semantic_tokens.result_id(rec.server, rec.uri) + -- /full when negotiated (delta only when full.delta == true and a + -- prior resultId is held); a RANGE-ONLY provider gets a + -- whole-document /range request instead — never an unsupported + -- /full. Never clear the store first: a delta splices against the + -- retained raw stream. + local prev = nil + if has_full and server_supports_semantic_delta(rec.server) then + prev = pmacs.semantic_tokens.result_id(rec.server, rec.uri) + end + local end_line, end_col + if not has_full then + end_line, end_col = document_end_position(buffer_text(rec.buffer)) + end pmacs.async(function() pcall(function() if prev then pmacs.lsp.request_semantic_tokens_delta(rec.server, rec.uri, prev):await() - else + elseif has_full then pmacs.lsp.request_semantic_tokens(rec.server, rec.uri):await() + else + pmacs.lsp.request_semantic_tokens_range( + rec.server, rec.uri, 0, 0, end_line, end_col):await() end end) end) @@ -640,18 +683,31 @@ end) -- Arc 1d: signature-help auto-trigger ---------------------------------- -- --- `buffer.after-edit` carries no payload, so a *typed character* is --- reconstructed from state exactly the way `completion.lua` does (Q#C9): --- same buffer, cursor advanced by exactly one byte. Paste, undo, kill, --- and remote CRDT edits produce any other delta and never auto-trigger. --- (Trigger characters are ASCII, so a one-byte advance is sound.) -local last_typed = { key = nil, cursor = nil } +-- A *typed character* is recognized by the input-origin signal, not by +-- cursor-delta inference: inside `buffer.after-edit`, +-- `pmacs.editor.this_command() == "buffer.self-insert"` names an edit +-- produced by typing — on either frontend (the daemon classifies +-- single-codepoint optimistic inserts the same way), per-frontend (no +-- cross-frontend misclassification), with no prior-edit snapshot (the +-- first character typed in a buffer triggers). Paste, undo, kill, +-- pointer, and every other input leave `this_command` as something +-- else — a one-byte paste of "(" can never trigger. +-- The last full UTF-8 codepoint ending at `cursor`, as a string. LSP +-- trigger characters are strings, not ASCII bytes, so this must be +-- codepoint-aware: read up to 4 bytes back and take the suffix from +-- the last non-continuation byte. local function char_before(buf, cursor) if cursor <= 0 then return nil end - local ok, s = pcall(function() return buf:slice(cursor - 1, cursor) end) - if not ok or type(s) ~= "string" or #s ~= 1 then return nil end - return s + local from = cursor - 4 + if from < 0 then from = 0 end + local ok, s = pcall(function() return buf:slice(from, cursor) end) + if not ok or type(s) ~= "string" or #s == 0 then return nil end + for i = #s, 1, -1 do + local b = s:byte(i) + if b < 0x80 or b >= 0xC0 then return s:sub(i) end + end + return nil end -- The set of characters that should (re)open signature help, as the @@ -710,13 +766,10 @@ pmacs.hook.add("buffer.after-edit", function() -- O(file) didChange send below is coalesced: render families -- anchored to pre-edit positions are hidden from this edit on. pcall(pmacs.lsp._mark_document_stale, rec.uri) - -- Arc 1d: did the user just type a signature trigger character? - -- Recorded before the early-outs below so the snapshot stays accurate - -- for the *next* edit even when this one doesn't trigger. - local cursor = pmacs.editor.cursor() - local prev_key, prev_cursor = last_typed.key, last_typed.cursor - last_typed.key, last_typed.cursor = key, cursor - local typed_one = key == prev_key and prev_cursor and cursor - prev_cursor == 1 + -- Arc 1d: was this edit a typed character? The input-origin signal + -- (see the trigger block below). + local typed = pmacs.editor.this_command + and pmacs.editor.this_command() == "buffer.self-insert" local now = pmacs.editor.monotonic_ms() local pending = pending_did_change[key] if pending and pending.rec == rec then @@ -726,8 +779,8 @@ pmacs.hook.add("buffer.after-edit", function() end -- Fire *after* queuing the pending didChange: `signature_help_quiet` -- flushes it, so the server sees the character we are asking about. - if not typed_one then return end - local ch = char_before(buf, cursor) + if not typed then return end + local ch = char_before(buf, pmacs.editor.cursor()) if not ch then return end local triggers = signature_trigger_chars(rec.server) if not (triggers and triggers[ch]) then return end @@ -1578,14 +1631,33 @@ function pmacs.lsp.semantic_tokens() return end -- Don't clear: a delta splices against the retained raw stream. - local prev = pmacs.semantic_tokens.result_id(rec.server, rec.uri) + -- Same gating as the auto-pull path: /full only when negotiated + -- (delta only under full.delta); a range-only provider gets a + -- whole-document /range request. + local has_full = server_supports_semantic_full(rec.server) + local has_range = server_supports_semantic_range(rec.server) + if not has_full and not has_range then + pmacs.editor.set_status("LSP: server has no semantic-token support") + return + end + local prev = nil + if has_full and server_supports_semantic_delta(rec.server) then + prev = pmacs.semantic_tokens.result_id(rec.server, rec.uri) + end + local end_line, end_col + if not has_full then + end_line, end_col = document_end_position(buffer_text(rec.buffer)) + end pmacs.async(function() local ok, err = pcall(function() if prev then pmacs.lsp.request_semantic_tokens_delta( rec.server, rec.uri, prev):await() - else + elseif has_full then pmacs.lsp.request_semantic_tokens(rec.server, rec.uri):await() + else + pmacs.lsp.request_semantic_tokens_range( + rec.server, rec.uri, 0, 0, end_line, end_col):await() end end) if not ok then diff --git a/src/bin/pmacs_fake_lsp.rs b/src/bin/pmacs_fake_lsp.rs index 3b8d837..9be1cd0 100644 --- a/src/bin/pmacs_fake_lsp.rs +++ b/src/bin/pmacs_fake_lsp.rs @@ -32,6 +32,19 @@ //! `rootUri` received in `initialize` to the file named by //! `PMACS_FAKE_LSP_ROOT_SINK`, so a test can assert the //! auto-attach path derives the project root from the opened file. +//! * If launched with `PMACS_FAKE_LSP_MODE=fullonly`: advertises a +//! full-only `semanticTokensProvider` (`"full": true`, no delta +//! member) and rejects `semanticTokens/full/delta` with a JSON-RPC +//! error — a conforming full-only server, for testing that the +//! client never requests delta without the negotiated capability. +//! * If launched with `PMACS_FAKE_LSP_MODE=rangeonly`: advertises a +//! range-only `semanticTokensProvider` (`"range": true`, no `full`) +//! and rejects `semanticTokens/full` — per LSP, `full` and `range` +//! are optional, independent capabilities. +//! * If launched with `PMACS_FAKE_LSP_MODE=rangeonly16`: `rangeonly` +//! plus UTF-16 position encoding, with strict UTF-16 bounds +//! validation on `/range` — rejects a client that sent raw byte +//! columns for non-ASCII text. //! * If launched with `PMACS_FAKE_LSP_MODE=sighelp`: additionally //! advertises `signatureHelpProvider` with `(` / `,` triggers, so a //! test can drive the Arc 1d auto-trigger. Every other mode omits the @@ -54,6 +67,8 @@ fn main() { let mut stdout = io::stdout().lock(); let mut crashed_after_init = false; let mut open_docs: HashMap = HashMap::new(); + // `fullonly` observability: counts /full responses (rid-1, rid-2…). + let mut full_count: u32 = 0; loop { let body = match read_frame(&mut stdin) { Ok(Some(b)) => b, @@ -138,7 +153,12 @@ fn main() { "tokenTypes": ["namespace", "function", "variable"], "tokenModifiers": ["declaration", "readonly"] }, - "full": true + // The default mode implements /full/delta, so + // it truthfully NEGOTIATES delta. Clients may + // only send /full/delta when `full` is + // `{ "delta": true }`; a bare `true` (the + // `fullonly` override below) is full-only. + "full": { "delta": true } } }, "serverInfo": { "name": "pmacs-fake-lsp", "version": "0.1.0" } @@ -158,13 +178,43 @@ fn main() { resp["result"]["capabilities"]["renameProvider"] = serde_json::json!({ "prepareProvider": true }); } + // `fullonly`: a conforming FULL-ONLY semantic-token + // server — advertises `"full": true` (no delta member) + // and REJECTS /full/delta below. Exercises the client + // rule that a stored resultId alone must never cause a + // delta request. + if mode == "fullonly" { + resp["result"]["capabilities"]["semanticTokensProvider"]["full"] = + serde_json::Value::from(true); + } + // `rangeonly`: LSP allows a provider to advertise + // `range` WITHOUT `full` — the /full arm below rejects + // in this mode, so a client that ignores the split gets + // a visible failure instead of silent staleness. + if mode.starts_with("rangeonly") { + let p = &mut resp["result"]["capabilities"]["semanticTokensProvider"]; + if let Some(obj) = p.as_object_mut() { + obj.remove("full"); + obj.insert("range".into(), serde_json::Value::from(true)); + } + } + // `rangeonly16` additionally negotiates UTF-16, so the + // /range arm can validate that the client converted its + // byte columns to UTF-16 code units. + if mode == "rangeonly16" { + resp["result"]["capabilities"]["positionEncoding"] = + serde_json::Value::from("utf-16"); + } // Arc 1d: advertise signature help only in `sighelp`, so // every other mode keeps the no-auto-trigger path (the // `textDocument/signatureHelp` arm below still answers // the manual `M-x lsp.signature-help` in any mode). if mode == "sighelp" { + // "«" (U+00AB, 2 UTF-8 bytes) exercises the rule + // that LSP trigger characters are strings, not + // ASCII bytes. resp["result"]["capabilities"]["signatureHelpProvider"] = serde_json::json!({ - "triggerCharacters": ["("], + "triggerCharacters": ["(", "\u{ab}"], "retriggerCharacters": [","] }); } @@ -749,6 +799,37 @@ fn main() { write_frame(&mut stdout, &resp); } ("textDocument/semanticTokens/full", Some(idv)) => { + // `rangeonly`: a range-only provider rejects /full — + // the client should have sent a range request. + if mode.starts_with("rangeonly") { + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "error": { + "code": -32601, + "message": "semanticTokens/full not supported" + } + }); + write_frame(&mut stdout, &resp); + continue; + } + // `fullonly`: bump the resultId per request so a test + // can observe WHICH pull refreshed the store — a + // repull that wrongly went to /full/delta is rejected + // and leaves the previous rid in place. + if mode == "fullonly" { + full_count += 1; + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "result": { + "resultId": format!("rid-{full_count}"), + "data": [0, 0, 4, 1, 1, 0, 5, 3, 2, 0, 2, 2, 7, 0, 2] + } + }); + write_frame(&mut stdout, &resp); + continue; + } // T M4.5: relative-encoded `data`. Three tokens: // [0,0,4,1,1] line 0 col 0 len 4, function, decl // [0,5,3,2,0] same line col 5 len 3, variable @@ -764,6 +845,27 @@ fn main() { write_frame(&mut stdout, &resp); } ("textDocument/semanticTokens/range", Some(idv)) => { + // `rangeonly16`: strict bounds validation in UTF-16 + // units. A client that sent raw byte columns for + // non-ASCII text overshoots the last line's UTF-16 + // length and is rejected — the fixture for the + // outbound-position conversion. + if mode == "rangeonly16" + && let Ok(sink) = std::env::var("PMACS_FAKE_RANGE_SINK") + { + let _ = std::fs::write(&sink, format!("{params}")); + } + if mode == "rangeonly16" + && let Some(message) = utf16_range_error(¶ms, &open_docs) + { + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "error": { "code": -32602, "message": message } + }); + write_frame(&mut stdout, &resp); + continue; + } // T M4.5: same shape as /full, scoped to a range. // One token: line 1 col 0 len 3, variable. let resp = serde_json::json!({ @@ -774,6 +876,22 @@ fn main() { write_frame(&mut stdout, &resp); } ("textDocument/semanticTokens/full/delta", Some(idv)) => { + // `fullonly`: a conforming full-only server rejects a + // delta request outright — the client should never have + // sent it (capabilities advertised `"full": true` with + // no delta member). + if mode == "fullonly" { + let resp = serde_json::json!({ + "jsonrpc": "2.0", + "id": idv, + "error": { + "code": -32601, + "message": "semanticTokens/full/delta not supported" + } + }); + write_frame(&mut stdout, &resp); + continue; + } // T M4.5: a `SemanticTokensDelta` over the /full data // `[0,0,4,1,1, 0,5,3,2,0, 2,2,7,0,2]` — replace the // last 5-int group (idx 10..15) with [3,0,9,1,0], so @@ -967,6 +1085,47 @@ fn document_end_position(text: &str) -> (u64, u64) { (line, col) } +/// `rangeonly16` bounds validation: the request's end position must not +/// exceed the document end measured in UTF-16 code units (the +/// negotiated encoding). Byte-column overshoot on non-ASCII text is +/// exactly the client bug this catches. +fn utf16_range_error( + params: &serde_json::Value, + open_docs: &HashMap, +) -> Option { + // Fail-CLOSED: a fixture that silently skips validation on an + // unexpected state (missing uri / unrecorded doc) reads as a pass. + let Some(uri) = params + .get("textDocument") + .and_then(|t| t.get("uri")) + .and_then(serde_json::Value::as_str) + else { + return Some("range request carried no textDocument.uri".into()); + }; + let Some(text) = open_docs.get(uri) else { + return Some(format!("no didOpen text recorded for {uri}")); + }; + let (mut line, mut col) = (0u64, 0u64); + for ch in text.chars() { + if ch == '\n' { + line += 1; + col = 0; + } else { + col += ch.len_utf16() as u64; + } + } + let end = params.get("range")?.get("end")?; + let end_line = end.get("line")?.as_u64()?; + let end_col = end.get("character")?.as_u64()?; + if end_line > line || (end_line == line && end_col > col) { + Some(format!( + "invalid utf-16 range end {end_line}:{end_col}; document ends at {line}:{col}" + )) + } else { + None + } +} + fn inlay_range_error( params: &serde_json::Value, open_docs: &HashMap, diff --git a/src/daemon.rs b/src/daemon.rs index 15342de..0a87188 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1978,6 +1978,28 @@ fn handle_inbound_paste( }); } +/// True when `edit` inserted exactly one UTF-8 codepoint: the leading +/// byte's sequence length equals `inserted_len` (kill ring review +/// round 4 — the typed-character classification for optimistic edits). +#[cfg(feature = "crdt")] +fn is_single_codepoint_insert(edit: &crate::rope::Edit) -> bool { + let len = edit.inserted_len; + if !(1..=4).contains(&len) { + return false; + } + let mut first = [0u8; 1]; + edit.new_rope + .slice(edit.range.start, edit.range.start + 1, &mut first); + let expected = match first[0] { + b if b < 0x80 => 1, + b if b < 0xC0 => return false, // bare continuation byte + b if b < 0xE0 => 2, + b if b < 0xF0 => 3, + _ => 4, + }; + expected == len +} + /// T M10.10 (post-audit) — apply a *pre-validated* /// `FrontendEvent::CrdtOp`. Identity, capability, and scope checks /// happen upstream in `validate_remote_crdt_op`; this function trusts @@ -2008,11 +2030,16 @@ fn handle_remote_crdt_op( buffer_id: crate::buffer::BufferId, op: crate::rope::CrdtOp, ) { - // Kill ring Q#KR2: an optimistic edit is non-command input — GPU - // typing, Enter/Tab, Backspace/Delete all arrive here without ever - // touching dispatch_key. It must break the source frontend's - // command chain, or `C-k x C-k` on the GPU would append across the - // typed character. + // Kill ring Q#KR2: an optimistic edit arrives here without ever + // touching dispatch_key, so the source's command boundary must be + // updated — or `C-k x C-k` on the GPU would append across the typed + // character. Break first (covers every early-return path); a + // successful apply refines this below: a single-codepoint insert is + // re-classified as `buffer.self-insert`, giving typed characters the + // same boundary on both frontends. That keeps kill-chain semantics + // identical (self-insert is not a kill) while making `this_command` + // a usable input-origin signal for typed-char consumers (signature + // help; the completion popup can migrate later). editor.core.borrow_mut().break_command_chain(source); // Effect 1: apply to buffer's CRDT + rope. Capture the Edit // (or `None` for an op that imported cleanly but produced no @@ -2045,6 +2072,19 @@ fn handle_remote_crdt_op( // notify but the op still needs broadcasting (F17). if let Some(edit) = edit_opt.as_ref() { let mut core = editor.core.borrow_mut(); + // The input-origin refinement promised above. The optimistic + // layer emits exactly one op per keystroke, so an empty-range + // insert of EXACTLY ONE codepoint is a typed character — + // Backspace/Delete/Undo produce deletes or larger shapes and + // stay chain-breaks. Decoding the inserted bytes (they are in + // the post-edit rope) rather than trusting `inserted_len` + // alone: a 2-byte insert of "a(" is two ASCII codepoints and + // must NOT classify as typing (review round 4 — it would + // spuriously auto-trigger signature help). Exact provenance on + // the wire op is the named deferred general fix. + if edit.range.start == edit.range.end && is_single_codepoint_insert(edit) { + core.rotate_command(source, "buffer.self-insert"); + } // Transient status messages clear on user input. The Key path // gets this from `dispatch_key`'s entry clear; the optimistic // path routes plain typing here instead, and since v15 ships @@ -2511,13 +2551,15 @@ mod tests { ); } - /// Kill ring Q#KR2 — an optimistic edit is non-command input: GPU - /// typing arrives here without touching dispatch_key, so it must - /// break the source frontend's command chain or `C-k x C-k` on the - /// GPU would append across the typed character. + /// Kill ring Q#KR2 — GPU typing arrives here without touching + /// dispatch_key, so it must update the source frontend's command + /// boundary or `C-k x C-k` on the GPU would append across the typed + /// character. A single-codepoint insert classifies as + /// `buffer.self-insert` (the input-origin signal for signature + /// help); anything else breaks the chain outright. #[cfg(feature = "crdt")] #[test] - fn handle_remote_crdt_op_breaks_the_source_command_chain() { + fn handle_remote_crdt_op_classifies_typed_input_and_ends_kill_chains() { use crate::editor::EditorState; use crate::protocol::FrontendId; @@ -2573,11 +2615,72 @@ mod tests { ); let core = editor.core.borrow(); - assert!( + // A single-codepoint optimistic insert classifies as a typed + // character: the boundary rotates to buffer.self-insert (the + // input-origin signal), which — not being a kill command — + // still breaks the kill chain exactly like the TUI typed-char + // path. + assert_eq!( core.command_history .get(&source) - .is_none_or(|b| b.this.is_none()), - "the optimistic edit must break the source's chain" + .and_then(|b| b.this.as_deref()), + Some("buffer.self-insert"), + "a typed optimistic insert classifies as self-insert" + ); + assert_eq!( + core.command_history + .get(&source) + .and_then(|b| b.last.as_deref()), + None, + "the pre-existing kill chain is gone (break-then-classify): a \ + following kill reads last = self-insert after its own rotation \ + and never appends" + ); + drop(core); + + // A TWO-codepoint insert ("a(") must NOT classify as typing + // (review round 4): its 2-byte length satisfies a naive 1-4 + // predicate, but decoding shows two ASCII codepoints — a typed + // key never produces that, and classifying it would let a + // multi-char op spuriously auto-trigger signature help. + editor + .core + .borrow_mut() + .rotate_command(source, "edit.kill-line"); + let snapshot_bytes = { + let core = editor.core.borrow(); + let reg = core.registry.borrow(); + reg.get(buffer_id) + .expect("buffer") + .crdt_state() + .expect("crdt-backed") + .export_snapshot() + .expect("export snapshot") + }; + let peer2 = loro::LoroDoc::new(); + peer2.set_peer_id(7).expect("set peer id"); + peer2.import(&snapshot_bytes).expect("import snapshot"); + let v_before = peer2.oplog_vv(); + peer2.get_text("body").insert(0, "a(").expect("peer insert"); + let op_bytes = peer2 + .export(loro::ExportMode::updates(&v_before)) + .expect("export op"); + handle_remote_crdt_op( + &mut editor, + source, + buffer_id, + crate::rope::CrdtOp { + peer_id: 7, + bytes: op_bytes, + }, + ); + let core = editor.core.borrow(); + assert_eq!( + core.command_history + .get(&source) + .and_then(|b| b.this.as_deref()), + None, + "a multi-codepoint insert breaks the chain instead of classifying as typing" ); assert_eq!( core.command_history diff --git a/src/editor.rs b/src/editor.rs index 2b946f7..b9bdf03 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -930,6 +930,17 @@ impl EditorState { CompletionPopupKey::Prev => self.core.borrow_mut().completion_popup_step(-1), CompletionPopupKey::Dismiss => self.core.borrow_mut().completion_popup_close(), CompletionPopupKey::Accept => { + // Accepting a completion is its own command boundary + // (review round 4): without this stamp, `this_command` + // could still read "buffer.self-insert" from the typing + // that raised the popup, and the after-edit fired below + // would let a candidate ending in "(" spuriously + // auto-trigger signature help. + { + let mut core = self.core.borrow_mut(); + let fid = core.active_frontend; + core.rotate_command(fid, "completion.accept"); + } let pre_revision = self.active_buffer_revision(); self.core.borrow_mut().completion_popup_accept(); if pre_revision != self.active_buffer_revision() { diff --git a/src/editor_core.rs b/src/editor_core.rs index 6eb50d0..4dcccf9 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -2104,6 +2104,21 @@ impl EditorCore { .as_deref() } + /// The active frontend's *current* command — Emacs's `this-command`. + /// Inside a `buffer.after-edit` hook this names the command that + /// produced the edit, which is the **input-origin signal**: + /// `"buffer.self-insert"` means the edit was a typed character + /// (keybound or optimistic), while a paste / pointer / unbound input + /// left it `None`. Per-frontend, so two attached frontends never + /// misclassify each other's input. + #[must_use] + pub fn this_command(&self) -> Option<&str> { + self.command_history + .get(&self.active_frontend)? + .this + .as_deref() + } + /// Copy the active region into the clipboard slot and queue an /// outbound OS-clipboard publish to the originating frontend. /// Returns `false` (a no-op) when there is no region. diff --git a/src/lsp.rs b/src/lsp.rs index e777c98..9849b3e 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -2003,11 +2003,16 @@ impl LspManager { diagnostics: &[Value], ) -> Result { let uri = uri.into(); + // Same encoding rule as every outbound range: columns are pmacs + // byte offsets and must convert to the negotiated units — a + // UTF-16 server receiving byte columns resolves the wrong range + // for non-ASCII lines (same bug class as the semantic-token + // range fix in this change). let params = json!({ "textDocument": { "uri": uri.clone() }, "range": { - "start": { "line": start_line, "character": start_col }, - "end": { "line": end_line, "character": end_col }, + "start": self.outbound_position(sid, &uri, start_line, start_col), + "end": self.outbound_position(sid, &uri, end_line, end_col), }, "context": { "diagnostics": diagnostics }, }); @@ -2038,7 +2043,7 @@ impl LspManager { "textDocument": { "uri": uri.clone() }, "range": { "start": self.outbound_position(sid, &uri, start_line, start_col), - "end": self.outbound_position(sid, &uri, end_line, end_col), + "end": self.outbound_position(sid, &uri, end_line, end_col), }, }); let req_id = self.send_request(sid, "textDocument/inlayHint", params)?; @@ -2081,11 +2086,17 @@ impl LspManager { end_col: u32, ) -> Result { let uri = uri.into(); + // Columns arrive as pmacs byte offsets; convert per the + // negotiated position encoding, exactly like the inlay-hint + // range. A UTF-16 server receiving raw byte columns gets an + // invalid end character for non-ASCII text ("é" is two bytes + // but one UTF-16 unit) and may reject the request — fatal for + // a range-only provider whose ONLY pull path this is. let params = json!({ "textDocument": { "uri": uri.clone() }, "range": { - "start": { "line": start_line, "character": start_col }, - "end": { "line": end_line, "character": end_col }, + "start": self.outbound_position(sid, &uri, start_line, start_col), + "end": self.outbound_position(sid, &uri, end_line, end_col), }, }); let req_id = self.send_request(sid, "textDocument/semanticTokens/range", params)?; diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 17ed4fa..dcdab4b 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -10999,6 +10999,18 @@ fn install_session(editor: &Table, lua: &Lua, core: &SharedCore) -> mlua::Result lua.create_function(move |_, ()| Ok(cc.borrow().last_command().map(str::to_owned)))?, )?; } + { + // this_command(): the command currently executing for the + // active frontend — the input-origin signal. Inside + // `buffer.after-edit`, "buffer.self-insert" means the edit was + // a typed character; nil means a non-command input (paste, + // pointer gesture, optimistic delete/undo). Per-frontend. + let cc = core.clone(); + editor.set( + "this_command", + lua.create_function(move |_, ()| Ok(cc.borrow().this_command().map(str::to_owned)))?, + )?; + } { // view_top(): the active window's first visible source line. // The saveplace getter (Arc 3) — pairs with set_view_top so a diff --git a/tests/completion_popup_acceptance.rs b/tests/completion_popup_acceptance.rs index c2ffeaf..f15a705 100644 --- a/tests/completion_popup_acceptance.rs +++ b/tests/completion_popup_acceptance.rs @@ -72,6 +72,23 @@ fn typing_opens_popup_and_tab_accepts() { 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"); + + // Kill ring review round 4: accepting a completion is its own + // command boundary. Without the stamp, this_command would still + // read "buffer.self-insert" from the typing that raised the popup, + // and a candidate ending in "(" would spuriously auto-trigger + // signature help from the accept's after-edit. + let this: Option = s + .lua_host + .lua() + .load("return pmacs.editor.this_command()") + .eval() + .unwrap(); + assert_eq!( + this.as_deref(), + Some("completion.accept"), + "accept stamps its own boundary, not the typing's self-insert" + ); } /// C-n moves the highlight before RET accepts, so the second diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 1f43d54..bb8b77f 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -4451,15 +4451,13 @@ fn arc1d_signature_help_auto_triggers_on_trigger_char() { end)()"; assert!(pump_lua_flag(&mut state, initialized, 5), "server init"); - // The first keystroke only seeds the typed-char snapshot; the second - // is the trigger. (A trigger char cannot fire off the very first edit - // in a buffer, which is correct: there is no prior cursor to compare.) - for c in ['f', '('] { - state.dispatch_key( - pmacs::protocol::FrontendId::LOCAL, - KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE), - ); - } + // The very FIRST character typed in the buffer is the trigger: the + // input-origin signal (this_command == buffer.self-insert) needs no + // prior-edit snapshot, so there is no warm-up keystroke. + state.dispatch_key( + pmacs::protocol::FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('('), KeyModifiers::NONE), + ); let deadline = Instant::now() + Duration::from_secs(5); let mut saw = false; while Instant::now() < deadline { @@ -4531,6 +4529,321 @@ fn arc1d_signature_help_does_not_trigger_on_ordinary_typing() { } } +/// Arc 1c review fix — a RANGE-ONLY provider (LSP: `full` and `range` +/// are optional, independent capabilities). The client must serve it a +/// whole-document /range request, never /full — the fake rejects /full +/// outright, so a client ignoring the split gets an empty store and +/// this test fails. +#[test] +fn arc1c_range_only_server_is_served_range_requests() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("a.rs"); + std::fs::write(&a_path, b"fn a() {}\n\n").expect("write a"); + let a_disp = a_path.display().to_string(); + + let mut state = EditorState::new(); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'rangeonly' }}, + }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + + // The rangeonly fake's /range reply carries one token; its + // presence proves the auto-pull went through the range path. + let has_tokens = format!( + "(function() \ + local sid \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then sid=r.id end \ + end \ + if not sid then return false end \ + local t = pmacs.semantic_tokens.tokens(sid, 'file://{a_disp}') \ + return t ~= nil and #t > 0 \ + end)()" + ); + assert!( + pump_lua_flag(&mut state, &has_tokens, 5), + "a range-only provider must be served a whole-document /range request" + ); +} + +/// Arc 1c review fix — a range-only server that negotiated UTF-16. +/// The whole-document range's columns are derived from pmacs byte +/// offsets; they must go through `outbound_position` like every other +/// outbound position. The last line ends in non-ASCII ("é" = 2 bytes, +/// 1 UTF-16 unit), and the fake validates the end bound strictly in +/// UTF-16 units — raw byte columns overshoot and are rejected, leaving +/// the store empty and this test failing. +#[test] +fn arc1c_range_only_utf16_server_gets_converted_bounds() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("a.rs"); + std::fs::write(&a_path, "fn a() {}\nlet x = \u{e9}\u{e9};".as_bytes()).expect("write a"); + let a_disp = a_path.display().to_string(); + + let mut state = EditorState::new(); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'rangeonly16' }}, + }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + + let has_tokens = format!( + "(function() \ + local sid \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then sid=r.id end \ + end \ + if not sid then return false end \ + local t = pmacs.semantic_tokens.tokens(sid, 'file://{a_disp}') \ + return t ~= nil and #t > 0 \ + end)()" + ); + assert!( + pump_lua_flag(&mut state, &has_tokens, 5), + "a UTF-16 range-only server must receive converted (not byte) columns" + ); +} + +/// Arc 1d — a server-declared NON-ASCII trigger character works. LSP +/// trigger characters are strings; the fake declares "«" (2 UTF-8 +/// bytes), and the codepoint-aware `char_before` must match it. +#[test] +fn arc1d_signature_help_triggers_on_non_ascii_trigger_char() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("a.rs"); + std::fs::write(&a_path, b"\n").expect("write a"); + let a_disp = a_path.display().to_string(); + + let mut state = EditorState::new(); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'sighelp' }}, + }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + let initialized = "(function() \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then return true end \ + end \ + return false \ + end)()"; + assert!(pump_lua_flag(&mut state, initialized, 5), "server init"); + + state.dispatch_key( + pmacs::protocol::FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('\u{ab}'), KeyModifiers::NONE), + ); + let deadline = Instant::now() + Duration::from_secs(5); + let mut saw = false; + while Instant::now() < deadline { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + if state.core.borrow().status.contains("fn echo(") { + saw = true; + break; + } + } + assert!(saw, "a non-ASCII trigger character must auto-trigger"); +} + +/// Arc 1d — an edit that is NOT a typed character never triggers, even +/// when it inserts exactly one trigger byte. The input-origin signal +/// (`this_command`) distinguishes it; a cursor-delta heuristic could +/// not (a one-byte programmatic insert of `(` looks identical). +#[test] +fn arc1d_signature_help_ignores_non_typed_edits() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("a.rs"); + std::fs::write(&a_path, b"\n").expect("write a"); + let a_disp = a_path.display().to_string(); + + let mut state = EditorState::new(); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'sighelp' }}, + }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + let initialized = "(function() \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then return true end \ + end \ + return false \ + end)()"; + assert!(pump_lua_flag(&mut state, initialized, 5), "server init"); + + // A movement command stamps this_command = cursor.*; then a + // programmatic one-byte insert of "(" fires after-edit. Under the + // old cursor-delta heuristic this was indistinguishable from + // typing. + state.dispatch_key( + pmacs::protocol::FrontendId::LOCAL, + KeyEvent::new(KeyCode::Down, KeyModifiers::NONE), + ); + state + .lua_host + .lua() + .load( + "pmacs.window.buffer():insert(pmacs.editor.cursor(), '(') \n\ + pmacs.hook.run('buffer.after-edit')", + ) + .exec() + .expect("programmatic insert"); + let deadline = Instant::now() + Duration::from_secs(2); + while Instant::now() < deadline { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + assert!( + !state.core.borrow().status.contains("fn echo("), + "a non-typed one-byte '(' insert must not trigger signature help" + ); + } +} + +/// Arc 1c review fix — a conforming FULL-ONLY server (advertises +/// `"full": true`, rejects /full/delta). Holding a resultId from the +/// first /full pull must NOT cause a delta request: the repull after an +/// edit goes to /full again and the store refreshes. Before the fix, +/// the delta request was rejected, the error swallowed, and semantic +/// styling stayed silently stale after the first edit. +#[test] +fn arc1c_full_only_server_repulls_via_full_not_delta() { + use pmacs::editor::EditorState; + + let dir = tempfile::tempdir().expect("tempdir"); + let a_path = dir.path().join("a.rs"); + std::fs::write(&a_path, b"fn a() {}\n\n").expect("write a"); + let a_disp = a_path.display().to_string(); + + let mut state = EditorState::new(); + let fake = fake_lsp_path(); + state + .lua_host + .lua() + .load(format!( + "pmacs.lsp.config.rust = {{ + command = '{fake}', + env = {{ PMACS_FAKE_LSP_MODE = 'fullonly' }}, + }}" + )) + .exec() + .expect("override rust config"); + state + .lua_host + .lua() + .load(format!("pmacs.buffer.find_or_open('{a_disp}')")) + .exec() + .expect("open a.rs"); + + let has_tokens = format!( + "(function() \ + local sid \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then sid=r.id end \ + end \ + if not sid then return false end \ + local t = pmacs.semantic_tokens.tokens(sid, 'file://{a_disp}') \ + return t ~= nil and #t > 0 \ + end)()" + ); + assert!( + pump_lua_flag(&mut state, &has_tokens, 5), + "attach /full pull" + ); + + // The fullonly fake bumps its resultId per /full response, so the + // store's rid says WHICH pull refreshed it. After the attach pull it + // is rid-1; the post-edit repull must advance it via /full. A repull + // that wrongly went to /full/delta (the pre-fix behavior: a stored + // resultId alone triggered delta) is rejected by the server, the + // error swallowed, and the rid stays rid-1 — silently stale. + let rid_is = |n: u32| { + format!( + "(function() \ + local sid \ + for _,r in ipairs(pmacs.lsp.list()) do \ + if r.state and r.state.kind=='initialized' then sid=r.id end \ + end \ + if not sid then return false end \ + return pmacs.semantic_tokens.result_id(sid, 'file://{a_disp}') == 'rid-{n}' \ + end)()" + ) + }; + assert!( + pump_lua_flag(&mut state, &rid_is(1), 5), + "attach pull is rid-1" + ); + state.dispatch_key( + pmacs::protocol::FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::NONE), + ); + assert!( + pump_lua_flag(&mut state, &rid_is(2), 5), + "a full-only server's repull must refresh via /full, not stale-out on a rejected delta" + ); +} + /// T M4.5 — `textDocument/semanticTokens/range` through the Lua /// surface. Same decode path as `/full`, scoped to a range; the /// fake returns one token.