From 886239480eb5343f2cf923af50498ba80b5648d2 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 21 May 2026 19:01:10 +0000 Subject: [PATCH] =?UTF-8?q?T=20M11.8=20=E2=80=94=20diag-store=20stale-flag?= =?UTF-8?q?=20closes=20LSP-re-analysis-gap=20surface=20(#48)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surfaced during session-5 manual validation as the final iteration of bet #1 from the framing pass: edits that shift byte positions left old diagnostic colors painted over post-edit text in both `pmacs-gpu` and (now-visible) the TUI. Persisted for the full LSP re-analysis window (100ms–5s). PR #47 fixed the StyleSpans side via generation-tracked full=true emission, but Decorations remained vulnerable: the producer's diff shipped old diagnostics from the diag store, whose entries were indexed at pre-edit byte positions until clangd republished. Fix: a per-URI `stale_uris` flag in `DiagnosticStore`. The LSP layer's `did_change_full` marks the URI stale right after sending the notification; the next `publishDiagnostics` absorb path's `set` clears it. The `semantic_render` producer reads `is_stale` and skips diagnostic emission entirely while stale. Effect: between an edit and clangd's next publish, the producer ships zero diagnostic decorations. Frontend's replace/merge clears old positions cleanly. Brief uncolored window (≤ LSP re-analysis latency) replaces the previous wrong-position-color persistence. The correct visual tradeoff: honest emptiness over deceptive staleness. Files changed: - `src/diag.rs` — `DiagnosticStore` gains `stale_uris: HashSet`; new `mark_stale` / `is_stale` API; `set` and `clear` reset the flag on the assumption that absorption / explicit removal mean the LSP has caught up. - `src/lsp.rs` — `LspManager::did_change_full` calls `diag_store.lock().mark_stale(uri)` after `send_notification`. - `src/semantic_render.rs` — `scoped_decorations` reads `is_stale` alongside `for_uri`; when stale, suppresses the diagnostic loop (selection and other non-diagnostic kinds still emit). Tests (all crdt-gated where they reference semantic_render): - `diag::tests::stale_flag_default_false` - `diag::tests::mark_stale_sets_flag` (per-URI scoping) - `diag::tests::set_clears_stale_flag` - `diag::tests::empty_set_clears_stale_flag_too` - `diag::tests::clear_drops_stale_flag` - `semantic_render::tests::diagnostics_suppressed_while_diag_store_stale` — assert no diagnostic kinds emit while stale; assert they re-emit after a fresh `set` clears the flag. Gates: cargo fmt + clippy (workspace, with/without `crdt`) clean; lib 1482 (+6) with crdt; 1319 (+6) without; m4 83; m11_5 2. This is approach (A) from the session-5 ask: track per-URI freshness relative to buffer edits, suppress emission until LSP catches up. Approach (B) — clangd's didChange/publishDiagnostics version matching — would be more precise but requires plumbing version tracking through the LspManager's document state, which is a larger change deferred. The stale-flag captures the same semantic ("any edit since last publish ⇒ stale") at a cheaper cost. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 --- src/diag.rs | 88 +++++++++++++++++++++++++++++++++++++++++- src/lsp.rs | 9 +++++ src/semantic_render.rs | 84 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 177 insertions(+), 4 deletions(-) diff --git a/src/diag.rs b/src/diag.rs index 4e29e63..e053fe0 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -167,9 +167,26 @@ impl Diagnostic { /// Per-URI diagnostic set. Mutated by [`crate::lsp::LspManager`] /// when handling `textDocument/publishDiagnostics`; read by /// [`DiagnosticView`] on every render. +/// +/// **T M11.8 — `stale_uris` tracking.** Per-URI flag set by +/// [`Self::mark_stale`] whenever the LSP layer ships a +/// `textDocument/didChange` for that URI; cleared by [`Self::set`] +/// when fresh diagnostics arrive. The semantic-frontend producer +/// reads [`Self::is_stale`] and suppresses decorations during the +/// window between an edit and clangd's republish, so a `pmacs-gpu` +/// (or any `semantic_render`) frontend never paints diagnostic +/// colors at byte positions that have since shifted under the +/// document. Closes the LSP-re-analysis-gap surface that bet #1 +/// in the session-4 framing pass predicted. #[derive(Default)] pub struct DiagnosticStore { by_uri: HashMap>, + /// URIs whose stored diagnostics are known to be out of date + /// because a `textDocument/didChange` was issued after the last + /// `publishDiagnostics` was absorbed. `Self::set` clears entries + /// here on the assumption that a fresh `publishDiagnostics` + /// corresponds to the latest sent version. + stale_uris: std::collections::HashSet, } impl DiagnosticStore { @@ -182,9 +199,14 @@ impl DiagnosticStore { /// Replace the diagnostic set for `uri`. Sorts by start position /// so [`Self::next_after`] / [`Self::previous_before`] don't /// have to re-sort on each query. + /// + /// Also clears the URI's stale flag (T M11.8) — fresh + /// diagnostics imply the LSP has caught up to the current + /// document state. pub fn set(&mut self, uri: impl Into, mut diags: Vec) { diags.sort_by(Diagnostic::compare_by_position); let uri = uri.into(); + self.stale_uris.remove(&uri); if diags.is_empty() { self.by_uri.remove(&uri); } else { @@ -192,9 +214,28 @@ impl DiagnosticStore { } } - /// Drop the diagnostics for `uri`. + /// Drop the diagnostics for `uri`. Also clears the stale flag — + /// no entry to be stale about. pub fn clear(&mut self, uri: &str) { self.by_uri.remove(uri); + self.stale_uris.remove(uri); + } + + /// Mark `uri`'s stored diagnostics as stale (T M11.8). Called + /// by the LSP layer on each `textDocument/didChange` so the + /// `semantic_render` producer can suppress emission during the + /// LSP-re-analysis gap. The next [`Self::set`] (or + /// [`Self::clear`]) clears the flag. + pub fn mark_stale(&mut self, uri: impl Into) { + self.stale_uris.insert(uri.into()); + } + + /// `true` iff the URI's stored diagnostics are stale (the + /// document has been edited since the last `publishDiagnostics` + /// absorption). T M11.8. + #[must_use] + pub fn is_stale(&self, uri: &str) -> bool { + self.stale_uris.contains(uri) } /// All diagnostics for `uri`, in start-position order. @@ -685,6 +726,51 @@ mod tests { assert_eq!(s.uris().count(), 0); } + // ---- T M11.8 stale-flag ---- + + #[test] + fn stale_flag_default_false() { + let s = DiagnosticStore::new(); + assert!(!s.is_stale("file:///a")); + } + + #[test] + fn mark_stale_sets_flag() { + let mut s = DiagnosticStore::new(); + s.mark_stale("file:///a"); + assert!(s.is_stale("file:///a")); + assert!(!s.is_stale("file:///b"), "stale flag is per-URI"); + } + + #[test] + fn set_clears_stale_flag() { + let mut s = DiagnosticStore::new(); + s.mark_stale("file:///a"); + assert!(s.is_stale("file:///a")); + s.set("file:///a", vec![diag(0, DiagnosticSeverity::Error, "x")]); + assert!(!s.is_stale("file:///a")); + } + + #[test] + fn empty_set_clears_stale_flag_too() { + // Empty diags after an edit means "LSP has caught up and + // there are no issues" — clear stale to allow rendering an + // empty decoration set. + let mut s = DiagnosticStore::new(); + s.mark_stale("file:///a"); + s.set("file:///a", Vec::new()); + assert!(!s.is_stale("file:///a")); + } + + #[test] + fn clear_drops_stale_flag() { + let mut s = DiagnosticStore::new(); + s.set("file:///a", vec![diag(0, DiagnosticSeverity::Error, "x")]); + s.mark_stale("file:///a"); + s.clear("file:///a"); + assert!(!s.is_stale("file:///a")); + } + #[test] fn severity_label_and_glyph_are_stable() { for (s, lbl, gl) in [ diff --git a/src/lsp.rs b/src/lsp.rs index 0ebc432..f778533 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -3044,6 +3044,15 @@ impl LspManager { let uri = uri.into(); let text = text.into(); self.documents.insert((sid, uri.clone()), text.clone()); + // T M11.8 — mark the diag-store entry stale so the + // semantic-frontend producer suppresses its emission until + // clangd's next `publishDiagnostics` re-establishes + // freshness via `set`. Closes the visible-stale-color + // window observed in session-5 manual validation. + self.diag_store + .lock() + .expect("diag 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 f41153a..2ade9e2 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -390,13 +390,21 @@ impl SemanticRenderState { // Lua LSP glue opened the document under. The URI is derived // from `vp.buffer_id` (see [`buffer_file_uri`]'s docs for why // not from `core.active_buffer_path()`). + // + // T M11.8 — skip emission while the store is stale (the + // document has been edited since the last `publishDiagnostics` + // absorption). Without this, the producer ships diagnostics + // whose byte positions point at pre-edit text — the visible + // wrong-position color artifact session-5 validation surfaced. + // The LSP layer's `did_change_full` marks the URI stale; the + // next `publishDiagnostics` absorbs and clears the flag. if let Some(uri) = buffer_file_uri(&core, vp.buffer_id) { - let diags = { + let (diags, is_stale) = { let store = state.lsp_manager.borrow().diag_store(); let guard = store.lock().expect("diag store mutex poisoned"); - guard.for_uri(&uri).to_vec() + (guard.for_uri(&uri).to_vec(), guard.is_stale(&uri)) }; - if !diags.is_empty() { + if !is_stale && !diags.is_empty() { let registry = core.registry.clone(); let reg = registry.borrow(); if let Ok(buf) = reg.get(vp.buffer_id) { @@ -1124,6 +1132,76 @@ mod tests { assert_eq!(decos[0].range, ByteRange { start: 4, end: 6 }); } + /// T M11.8 regression: when the diag store's entry for the URI + /// is marked stale (an edit has been issued since the last + /// `publishDiagnostics`), the producer must suppress diagnostic + /// emission so the frontend doesn't paint colors at pre-edit + /// byte positions over post-edit text. Closes the bet-#1 + /// surface that session-5 validation exposed. + #[test] + fn diagnostics_suppressed_while_diag_store_stale() { + let state = empty_state(); + let buffer_id = active_buffer(&state); + seed_diagnostic(&state, buffer_id); + + // Mark the diag store stale for the seeded URI. The producer + // should now emit zero diagnostic decorations. + let uri = crate::lsp::path_to_file_uri(std::path::Path::new("/tmp/m114.rs")); + state + .lsp_manager + .borrow() + .diag_store() + .lock() + .expect("diag store") + .mark_stale(uri.clone()); + + let mut s = local(); + s.set_viewport(buffer_id, ByteRange { start: 0, end: 64 }, 0); + let (_full, decos) = + decorations_of(&s.render_frame(&state)).expect("a Decorations message"); + assert!( + decos.iter().all(|d| !matches!( + d.kind, + DecorationKind::DiagnosticError + | DecorationKind::DiagnosticWarning + | DecorationKind::DiagnosticInfo + | DecorationKind::DiagnosticHint + )), + "stale diag store ⇒ no diagnostic decorations emitted; got {decos:?}" + ); + + // Once a fresh `set` clears the stale flag, the decoration + // re-appears on the next render frame. + state + .lsp_manager + .borrow() + .diag_store() + .lock() + .expect("diag store") + .set( + &uri, + vec![crate::diag::Diagnostic { + start_line: 1, + start_col: 0, + end_line: 1, + end_col: 2, + severity: crate::diag::DiagnosticSeverity::Warning, + message: "x".into(), + source: None, + code: None, + }], + ); + + let (_full, decos) = + decorations_of(&s.render_frame(&state)).expect("a Decorations message"); + assert!( + decos + .iter() + .any(|d| d.kind == DecorationKind::DiagnosticWarning), + "fresh set ⇒ stale flag cleared ⇒ decoration re-emitted; got {decos:?}" + ); + } + /// Regression: in a multi-frontend setup the editor's *active* /// buffer (set by `core.active_buffer_id()`, derived from the /// active frontend's view) can differ from the buffer a given