diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index 3f61fac..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 @@ -478,33 +495,42 @@ end -- requesting delta without the capability would leave styling silently -- stale after the first edit. local function server_supports_semantic_delta(sid) - local ok, caps = pcall(pmacs.lsp.capabilities, sid) - if not ok or not caps then return false end - local p = caps.semanticTokensProvider + 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 only when the server NEGOTIATED it (full.delta == true) and a - -- prior resultId is held; /full otherwise. Never clear the store - -- first: a delta splices against the retained raw stream. + -- /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 server_supports_semantic_delta(rec.server) then + 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) @@ -1605,19 +1631,33 @@ function pmacs.lsp.semantic_tokens() return end -- Don't clear: a delta splices against the retained raw stream. - -- Delta only when negotiated (full.delta) — same rule as the - -- auto-pull path; a resultId alone does not imply delta support. + -- 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 server_supports_semantic_delta(rec.server) then + 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 ec5f98a..4f48f87 100644 --- a/src/bin/pmacs_fake_lsp.rs +++ b/src/bin/pmacs_fake_lsp.rs @@ -37,6 +37,10 @@ //! 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`, `full` +//! null) and rejects `semanticTokens/full` — per LSP, `full` and +//! `range` are optional, independent capabilities. //! * 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 @@ -179,6 +183,17 @@ fn main() { 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 == "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)); + } + } // 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 @@ -773,6 +788,20 @@ 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 == "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 diff --git a/src/daemon.rs b/src/daemon.rs index 51d74d4..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 @@ -2052,10 +2074,15 @@ fn handle_remote_crdt_op( 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 one codepoint (1–4 UTF-8 bytes) IS a typed - // character — Backspace/Delete/Undo produce deletes or larger - // shapes and stay chain-breaks. - if edit.range.start == edit.range.end && (1..=4).contains(&edit.inserted_len) { + // 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 @@ -2609,6 +2636,52 @@ mod tests { 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 .get(&FrontendId::LOCAL) 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/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 bf425d3..8d4dd16 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -4529,6 +4529,59 @@ 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 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.