From 9718958c4c358ee1607e808b8b66006d4c8c735c Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 26 May 2026 11:15:23 -0400 Subject: [PATCH] Fix stale TUI styling after edits --- builtin/runtime/syntax.lua | 55 +++++++++++++++++++++++ src/diag.rs | 49 ++++++++++++++++++++ src/highlight.rs | 91 +++++++++++++++++++++++++++++++++++++ src/lsp.rs | 4 ++ src/semantic_render.rs | 3 ++ src/semantic_tokens.rs | 61 ++++++++++++++++++++++++- tests/m4_acceptance.rs | 92 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 353 insertions(+), 2 deletions(-) diff --git a/builtin/runtime/syntax.lua b/builtin/runtime/syntax.lua index 62d1718..f28e012 100644 --- a/builtin/runtime/syntax.lua +++ b/builtin/runtime/syntax.lua @@ -17,6 +17,11 @@ -- when `pmacs.parse._dispatch` is called below; drained by the tick -- step. local pending_parse_jobs = {} +local parse_job_buffer_keys = {} +local inflight_parse_by_buffer = {} +local parse_buffer_by_key = {} +local parse_lang_by_buffer = {} +local reparse_requested_by_buffer = {} local raw_dispatch = pmacs.parse._dispatch @@ -24,8 +29,18 @@ local raw_dispatch = pmacs.parse._dispatch -- pending set. Calls into `_dispatch` from outside this file (e.g. -- a hand-rolled user script) get the same tracking for free. function pmacs.parse._dispatch(buf, lang) + local key = tostring(buf) + parse_buffer_by_key[key] = buf + parse_lang_by_buffer[key] = lang + local inflight = inflight_parse_by_buffer[key] + if inflight then + reparse_requested_by_buffer[key] = true + return inflight + end local job_id = raw_dispatch(buf, lang) pending_parse_jobs[job_id] = true + parse_job_buffer_keys[job_id] = key + inflight_parse_by_buffer[key] = job_id return job_id end @@ -68,6 +83,40 @@ pmacs.hook.add("buffer.after-load", function() end end) +local function reparse_active_buffer_after_edit() + local buf = pmacs.window.buffer() + if not buf then return end + if not pmacs.parse._has_view(buf) then return end + local pending = pmacs.parse._pending_edits(buf) + if not pending or pending == 0 then return end + local path = buf:name() + if not path then return end + local lang = pmacs.parse.language_for_path(path) + if not lang then return end + pmacs.parse._dispatch(buf, lang) +end + +pmacs.hook.add("buffer.after-edit", function() + -- `ParseView:on_edit` records incremental edits synchronously, but + -- highlight overlays only see new spans after a fresh parse settles. + local ok, err = pcall(reparse_active_buffer_after_edit) + if not ok and pmacs.error then + pmacs.error("syntax.after-edit: " .. tostring(err)) + end +end) + +local function dispatch_follow_up_if_dirty(key) + local buf = parse_buffer_by_key[key] + local lang = parse_lang_by_buffer[key] + if not buf or not lang then return end + local pending = pmacs.parse._pending_edits(buf) + local requested = reparse_requested_by_buffer[key] + reparse_requested_by_buffer[key] = nil + if requested or (pending and pending > 0) then + pmacs.parse._dispatch(buf, lang) + end +end + -- After-tick step: any parse job that has settled gets its bundle -- installed into the buffer's view (and its pending entry drained). -- Extension hook on top of the async runtime's tick rather than a @@ -78,8 +127,14 @@ pmacs._async.tick = function(...) local ret = prior_tick(...) for job_id in pairs(pending_parse_jobs) do if pmacs._async._is_complete(job_id) then + local key = parse_job_buffer_keys[job_id] pmacs.parse._install_settled(job_id) pending_parse_jobs[job_id] = nil + parse_job_buffer_keys[job_id] = nil + if key and inflight_parse_by_buffer[key] == job_id then + inflight_parse_by_buffer[key] = nil + dispatch_follow_up_if_dirty(key) + end end end return ret diff --git a/src/diag.rs b/src/diag.rs index 29548db..3dfe2bf 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -421,6 +421,9 @@ impl View for DiagnosticView { // doesn't today, but the discipline is cheap). let diags: Vec = { let guard = self.store.lock().expect("diag store mutex poisoned"); + if guard.is_stale(&self.uri) { + return; + } guard.for_uri(&self.uri).to_vec() }; if diags.is_empty() { @@ -796,4 +799,50 @@ mod tests { let view = DiagnosticView::new("file:///a", store); assert_eq!(view.kind(), "diagnostic"); } + + #[test] + fn diagnostic_view_suppresses_stale_store_entries() { + use crate::cell::{Cell, CellSize, UnderlineStyle}; + + let store = make_shared_store(); + { + let mut guard = store.lock().expect("diag store"); + guard.set( + "file:///a", + vec![diag(0, DiagnosticSeverity::Error, "stale")], + ); + guard.mark_stale("file:///a"); + } + + let mut buf = Buffer::new(crate::buffer::BufferId::next(), "test.c"); + buf.apply_edit(crate::buffer::EditOp::Insert { + pos: 0, + bytes: b"hello\n", + }) + .expect("seed buffer"); + + let mut view = DiagnosticView::new("file:///a", store); + let mut backing = vec![Cell::default(); 10]; + let mut grid = CellGrid { + cells: &mut backing, + stride: 10, + size: CellSize::new(1, 10), + }; + view.render( + &buf, + Viewport { + buffer_start: 0, + buffer_end: buf.len(), + cell_origin: CellCoord::new(0, 0), + cell_size: CellSize::new(1, 10), + }, + &mut grid, + ); + + assert_eq!( + grid.get(CellCoord::new(0, 0)).style.underline, + UnderlineStyle::None, + "stale diagnostics must not underline shifted TUI bytes" + ); + } } diff --git a/src/highlight.rs b/src/highlight.rs index a995a45..3f06e89 100644 --- a/src/highlight.rs +++ b/src/highlight.rs @@ -531,6 +531,9 @@ impl View for LspStyleView { }; let store = mgr.semantic_token_store(); let guard = store.lock().expect("semantic-token store mutex poisoned"); + if guard.is_stale(&uri) { + return; + } let Some((_, resp)) = guard.for_uri(&uri) else { return; }; @@ -878,6 +881,94 @@ mod tests { ); } + #[test] + fn lsp_style_view_suppresses_stale_semantic_tokens() { + use crate::cell::{Cell, CellSize}; + use crate::editor::EditorState; + use crate::lsp::PositionEncoding; + use crate::semantic_tokens::{SemanticToken, SemanticTokenKey, SemanticTokensResponse}; + + let state = EditorState::new(); + let buffer_id = state.core.borrow().active_window().buffer_id; + { + let mut core = state.core.borrow_mut(); + core.registry + .clone() + .borrow_mut() + .get_mut(buffer_id) + .expect("active buffer") + .apply_edit(crate::buffer::EditOp::Insert { + pos: 0, + bytes: b"foo\n", + }) + .expect("seed"); + core.set_buffer_path(buffer_id, Some(std::path::PathBuf::from("/tmp/x.c"))); + } + state.syntax_registry.theme().lock().expect("theme").insert( + "function", + Style { + bold: true, + ..Style::default() + }, + ); + let sid = state + .lsp_manager + .borrow_mut() + .insert_initialized_test_client( + serde_json::json!({ + "semanticTokensProvider": { + "legend": { "tokenTypes": ["function"], "tokenModifiers": [] } + } + }), + PositionEncoding::Utf16, + ); + let active_path = state.core.borrow().active_buffer_path().expect("path set"); + let uri = crate::lsp::path_to_file_uri(&active_path); + { + let mgr = state.lsp_manager.borrow(); + let store = mgr.semantic_token_store(); + let mut guard = store.lock().expect("store"); + guard.set( + SemanticTokenKey::new(sid.raw().to_string(), uri.clone()), + SemanticTokensResponse { + tokens: vec![SemanticToken { + line: 0, + start: 0, + length: 3, + token_type: 0, + token_modifiers: 0, + }], + result_id: None, + raw: Vec::new(), + }, + ); + guard.mark_stale(uri); + } + + let mut view = LspStyleView::new(state.lsp_manager.clone(), state.syntax_registry.theme()); + let mut backing: Vec = vec![Cell::default(); 20]; + let mut grid = CellGrid { + cells: &mut backing, + stride: 20, + size: CellSize::new(1, 20), + }; + let viewport = Viewport { + buffer_start: 0, + buffer_end: u64::MAX, + cell_origin: CellCoord::new(0, 0), + cell_size: CellSize::new(1, 20), + }; + let registry = state.core.borrow().registry.clone(); + let reg = registry.borrow(); + let buf = reg.get(buffer_id).expect("buffer"); + view.render(buf, viewport, &mut grid); + + assert!( + !grid.get(CellCoord::new(0, 0)).style.bold, + "stale semantic tokens must not paint over current syntax/text" + ); + } + #[test] #[allow(clippy::too_many_lines)] // Scripted fixture; the headline // test next door is similarly diff --git a/src/lsp.rs b/src/lsp.rs index f778533..6143e2f 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -3053,6 +3053,10 @@ impl LspManager { .lock() .expect("diag store mutex poisoned") .mark_stale(uri.clone()); + self.semantic_token_store + .lock() + .expect("semantic token store mutex poisoned") + .mark_stale(uri.clone()); let params = json!({ "textDocument": { "uri": uri, diff --git a/src/semantic_render.rs b/src/semantic_render.rs index 2ade9e2..929449e 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -770,6 +770,9 @@ fn lsp_scoped_style_spans(state: &EditorState, vp: &DeclaredViewport) -> Vec resp.tokens.clone(), None => return Vec::new(), diff --git a/src/semantic_tokens.rs b/src/semantic_tokens.rs index cd9dfb5..257e2ec 100644 --- a/src/semantic_tokens.rs +++ b/src/semantic_tokens.rs @@ -25,7 +25,7 @@ //! priority vs. tree-sitter) is a separate rendering milestone; like //! the other LSP features, nothing here paints — Lua reads the store. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::{Arc, Mutex}; use serde_json::Value; @@ -219,6 +219,10 @@ impl SemanticTokensLegend { #[derive(Default)] pub struct SemanticTokenStore { by_key: HashMap, + /// URIs whose stored semantic tokens are known to be stale + /// because the document changed after the last full/delta token + /// response was absorbed. + stale_uris: HashSet, } /// Key into [`SemanticTokenStore`]. @@ -250,12 +254,31 @@ impl SemanticTokenStore { /// Replace the response at `key`. pub fn set(&mut self, key: SemanticTokenKey, response: SemanticTokensResponse) { + self.stale_uris.remove(&key.uri); self.by_key.insert(key, response); } - /// Drop the entry at `key`. + /// Drop the entry at `key`. Also clears the stale flag for that + /// URI when no other server has token data for it. pub fn clear(&mut self, key: &SemanticTokenKey) { self.by_key.remove(key); + if !self.by_key.keys().any(|k| k.uri == key.uri) { + self.stale_uris.remove(&key.uri); + } + } + + /// Mark all semantic-token entries for `uri` stale. Called when + /// a `textDocument/didChange` is sent so renderers do not paint + /// byte ranges from a pre-edit token set. + pub fn mark_stale(&mut self, uri: impl Into) { + self.stale_uris.insert(uri.into()); + } + + /// `true` iff `uri` has semantic-token data that should not be + /// rendered against the current buffer text. + #[must_use] + pub fn is_stale(&self, uri: &str) -> bool { + self.stale_uris.contains(uri) } /// Look up the entry at `key`. @@ -425,6 +448,40 @@ mod tests { assert!(s.get(&key).is_none()); } + #[test] + fn stale_flag_clears_on_set_and_final_clear() { + let mut s = SemanticTokenStore::new(); + let key = SemanticTokenKey::new("1", "file:///a"); + let response = SemanticTokensResponse { + tokens: vec![SemanticToken { + line: 0, + start: 0, + length: 1, + token_type: 0, + token_modifiers: 0, + }], + result_id: None, + raw: vec![0, 0, 1, 0, 0], + }; + + s.set(key.clone(), response.clone()); + s.mark_stale("file:///a"); + assert!(s.is_stale("file:///a")); + + s.set(key.clone(), response); + assert!( + !s.is_stale("file:///a"), + "fresh semantic tokens clear stale flag" + ); + + s.mark_stale("file:///a"); + s.clear(&key); + assert!( + !s.is_stale("file:///a"), + "clearing final token entry clears stale flag" + ); + } + #[test] fn for_uri_filters_by_uri_and_picks_lowest_server() { let mk = |tok_type: u32| SemanticTokensResponse { diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 868b29a..5f5d154 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -87,8 +87,10 @@ use std::fmt::Write as _; use std::sync::Arc; use std::time::{Duration, Instant}; +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; use pmacs::async_runtime::{AsyncRuntime, JobOutcome, JobResult}; use pmacs::buffer::{Buffer, BufferId, EditOp}; +use pmacs::protocol::FrontendId; use pmacs::syntax::{self, ParseRequest, ParseView}; /// Synthetic Rust source: `n` lines of plausible-but-trivial code. @@ -330,6 +332,20 @@ fn current_tree_language(state: &pmacs::editor::EditorState) -> Option { lua.load(chunk).eval::>().ok().flatten() } +/// Returns the source snapshot attached to the active buffer's most +/// recently installed parse tree. +fn current_tree_text(state: &pmacs::editor::EditorState) -> Option { + let lua = state.lua_host.lua(); + let chunk = r" + local buf = pmacs.window.buffer() + if not buf then return nil end + local tree = pmacs.parse.tree(buf) + if not tree then return nil end + return tree:text() + "; + lua.load(chunk).eval::>().ok().flatten() +} + /// Opening a `.rs` file produces a parse tree whose root is a /// `source_file` and whose language label is `rust`. The dispatch /// runs through the `buffer.after-load` hook installed by @@ -625,6 +641,75 @@ fn m4_3_highlight_updates_within_one_frame_after_parse() { ); } +/// Regression for task #25: an edit delivered through the normal +/// key-dispatch path must trigger the syntax runtime to dispatch a +/// fresh parse. Without the `buffer.after-edit` hook in +/// `builtin/runtime/syntax.lua`, `ParseView:on_edit` accumulates a +/// pending edit but `pmacs.parse.tree(buf)` remains the old source +/// indefinitely, which leaves syntax colors pinned to stale byte +/// offsets in both frontends. +#[test] +fn m4_3_key_edit_reparses_active_buffer_without_manual_dispatch() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("after_edit.rs"); + std::fs::write(&path, b"fn main() {}\n").expect("write"); + + let mut state = open_and_wait_for_parse(path); + assert_eq!(current_tree_text(&state).as_deref(), Some("fn main() {}\n")); + + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('x'), KeyModifiers::NONE), + ); + + let pending_after_key: Option = state + .lua_host + .lua() + .load( + r" + local buf = pmacs.window.buffer() + return pmacs.parse._pending_edits(buf) + ", + ) + .eval() + .expect("pending edit count"); + assert_eq!( + pending_after_key, + Some(0), + "after-edit syntax hook should dispatch and drain pending parse edits" + ); + + state.dispatch_key( + FrontendId::LOCAL, + KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE), + ); + + let pending_after_second_key: Option = state + .lua_host + .lua() + .load( + r" + local buf = pmacs.window.buffer() + return pmacs.parse._pending_edits(buf) + ", + ) + .eval() + .expect("pending edit count after queued edit"); + assert_eq!( + pending_after_second_key, + Some(1), + "edits made while a parse is in flight should wait for a follow-up parse" + ); + + pump_async(&mut state, |s| { + current_tree_text(s).as_deref() == Some("xyfn main() {}\n") + }); + assert_eq!( + current_tree_text(&state).as_deref(), + Some("xyfn main() {}\n") + ); +} + /// A Lua-set theme observably changes the rendered cells: replacing /// the default theme with one that maps `keyword` to a distinctive /// foreground color produces cells in that color where keywords @@ -1295,6 +1380,13 @@ fn m4_5_did_change_notifications_go_out_after_edits() { .did_change_full(sid, uri, v, text) .expect("didChange"); } + { + let store = mgr.borrow().semantic_token_store(); + assert!( + store.lock().expect("semantic token store").is_stale(uri), + "didChange must mark semantic tokens stale so stale TUI LSP styles are suppressed" + ); + } // The fake LSP replies with a `pmacs/echo` notification per // didOpen/didChange (5 total). let evs = drain_lsp_until(&sup, &mgr, sid, Duration::from_secs(5), |evs| {