From a67cb8a6f1b4d96910dc4ffc2760f70df607e744 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 28 May 2026 12:49:23 -0400 Subject: [PATCH] Close pmacs-gpu phase A audit --- docs/pmacs-gpu-phase-a-audit.md | 95 ++++++++++++++++++ pmacs-gpu/src/main.rs | 7 +- src/inlay_hint.rs | 58 ++++++++++- src/lsp.rs | 15 ++- src/semantic_render.rs | 164 +++++++++++++++++++++++++++++--- tests/m4_acceptance.rs | 7 ++ 6 files changed, 325 insertions(+), 21 deletions(-) create mode 100644 docs/pmacs-gpu-phase-a-audit.md diff --git a/docs/pmacs-gpu-phase-a-audit.md b/docs/pmacs-gpu-phase-a-audit.md new file mode 100644 index 0000000..a536773 --- /dev/null +++ b/docs/pmacs-gpu-phase-a-audit.md @@ -0,0 +1,95 @@ +# pmacs-gpu Phase A audit + +Date: 2026-05-28 + +Phase A's framing criterion was not "the GUI is done." It was: +exercise the producer-arc wire shapes against a real consumer, absorb +small shape findings, and score the predicted finding categories. + +This audit covers the producer/consumer arc through Sessions 5-8: +`StyleSpans`, `Decorations`, `InlineAdornments`, +`FileStyleSummary`, `CursorByte`, and the pmacs-gpu consumer paths +that render or cache them. + +## Evidence + +| Session | Surface | Evidence | +|---|---|---| +| 5 | `Decorations` consumption | pmacs-gpu paints diagnostic severities as foreground overrides. Session validation surfaced stale diagnostic styling and byte-position replacement mistakes; the producer now resolves URI by `vp.buffer_id`, suppresses stale diagnostics, and full-resyncs style/deco families on CRDT generation transitions. | +| 6 | `InlineAdornments` consumption | pmacs-gpu inserts LSP inlay hints as zero-width virtual text. `m4_29_real_rust_analyzer_inlay_hints_via_auto_attach` covers rust-analyzer's strict range behavior; the GUI was manually validated with a Rust inlay-hints test file. | +| 7 | `FileStyleSummary` consumption | pmacs-gpu renders the right-side minimap from whole-file dominant line style and local rope structure. Manual validation pushed it from one-color-per-line bars to code-shaped strokes. | +| 8 | Temporal probe | Added a 1000-edit synthetic semantic-render probe that exercises CRDT generation transitions, stale inlay suppression, and fresh inlay rehydration. Also extended `didChange` stale marking to inlay hints. | + +The Session 8 automated probe is producer-level and deterministic: it +does not spawn rust-analyzer for all 1000 edits. Real-server coverage +for inlay production remains the PATH-gated rust-analyzer acceptance +test; the stress loop validates the temporal contract that LSP-derived +zero-width adornments must not render while their store is stale. + +## Findings + +| ID | Finding | Category | Classification | Resolution | +|---|---|---|---|---| +| A1 | Incremental `StyleSpans` / `Decorations` could leave cached byte ranges positioned against pre-edit text after CRDT edits. | Headless-test-blind-spot | Small | Track buffer generation and force `full=true` style/deco resync on generation transitions. | +| A2 | Diagnostics could render against stale post-edit text before the next `publishDiagnostics`. | Headless-test-blind-spot | Small | `DiagnosticStore` stale flag; `did_change_full` marks stale; `publishDiagnostics` clears via `set`; semantic producer suppresses stale diagnostics. | +| A3 | Multi-frontend semantic projection could resolve LSP stores through the editor's active buffer rather than the projected buffer. | Headless-test-blind-spot | Small | URI lookup now routes through `vp.buffer_id` for diagnostics, inlay hints, and LSP semantic tokens. | +| A4 | rust-analyzer rejected over-wide inlay-hint ranges, leaving `InlineAdornments` empty in normal Rust files. | Real-server-strictness | Small | Auto-pull inlay hints over the exact document end; guarded by `m4_29_real_rust_analyzer_inlay_hints_via_auto_attach`. | +| A5 | Inlay hints were not marked stale on `didChange`, so zero-width virtual text could stay anchored to pre-edit positions during sustained typing. | Temporal-interaction | Small | `InlayHintStore` now has stale flags; `did_change_full` marks stale; semantic render emits one empty `InlineAdornments` replacement to clear cached virtual text until fresh hints arrive. | +| A6 | `FileStyleSummary` trailing empty line needed an explicit convention. | Convention-vs-contract | Small | Producer test codifies "final newline creates a trailing empty summary line"; minimap consumes that shape directly. | +| A7 | Initial minimap rendering was visually valid but too coarse to be useful. | Consumer-projection-granularity | Small | pmacs-gpu derives indentation/length strokes from its local rope while using `FileStyleSummary` for color. | +| A8 | Selection/search/current-line backgrounds accumulate in consumer state but cannot render via glyph foreground attributes. | Quad-pipeline-needed | Structural | Deferred to the post-Phase-A wgpu quad pipeline. | + +## Predicted vs actual + +| Predicted bet | Result | Count | Notes | +|---|---:|---:|---| +| `StyleSpans` / `Decorations` dirty-segment edges at viewport boundaries | Surfaced | 3 | A1, A2, A3. The category was right and under-counted: stale state came from generation, freshness, and buffer identity. | +| `InlineAdornments` suppression flicker / edit-then-revert behavior | Surfaced | 1 | A5. Whole-set suppression is acceptable only when the backing store has freshness state. | +| `FileStyleSummary` trailing-empty-line behavior may need wire decision | Surfaced | 1 | A6. This is now a producer convention, not an open contract question. | +| `CursorByte` per-tick cadence may be wrong for a 60fps consumer | Did not surface as a Phase A fix | 0 | Current shape remains state-notification cadence. Cursor-derived backgrounds are deferred to the quad pipeline, so no new wire frequency was justified in Phase A. | +| `PresenceUpdate` peer color stability may need renderer-side identity discipline | Not exercised | 0 | pmacs-gpu does not yet consume peer cursors. This remains a future multi-frontend GUI surface, not a Phase A blocker. | + +Unpredicted categories that surfaced: + +| Category | Count | Findings | +|---|---:|---| +| Real-server-strictness | 1 | A4: rust-analyzer exact inlay range requirement. | +| Consumer-projection-granularity | 1 | A7: minimap needed local rope shape, not just per-line color. | +| Quad-pipeline-needed | 1 | A8: foreground-only glyph attributes cannot render background decoration kinds. | + +Score summary: + +- Predicted categories surfaced: 3 of 5. +- Predicted categories not fixed in Phase A: 2 of 5 (`CursorByte` + cadence, `PresenceUpdate` identity). +- Unpredicted categories surfaced: 3. +- Small findings absorbed in Phase A: 7. +- Structural findings deferred: 1. + +## Phase A close + +Phase A can close for the shipped producer-arc families: + +- `StyleSpans`: consumed by pmacs-gpu; generation/full-resync and stale + LSP token suppression are tested. +- `Decorations`: diagnostic foreground rendering works; stale + diagnostics are suppressed. Background kinds are stored but deferred + to the quad pipeline. +- `InlineAdornments`: inlay hints render as virtual text; real + rust-analyzer range behavior and edit-time stale clearing are tested. +- `FileStyleSummary`: minimap consumes the whole-file summary and uses + local rope shape for useful granularity. +- `CursorByte`: still delivered to semantic/CRDT sessions; no Phase A + contract change. + +Not closed by Phase A: + +- Background rectangles for `Selection`, `SearchMatch`, + `SearchMatchActive`, and `CurrentLine`. +- pmacs-gpu `PresenceUpdate` consumption. +- Producers for `BlockAdornments`, `FoldState`, and `ResourceOffer`. +- Soft-wrap intent beyond the current local frontend implementation. + +The next natural step is the wgpu quad pipeline, because it unlocks the +background-bearing decoration kinds already present in the wire and in +pmacs-gpu's cached decoration state. diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index 860b9c4..bbcb34f 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -667,8 +667,11 @@ impl State { // InlineAdornments use whole-set suppression rather // than dirty segments, so the same ownership rule // applies here: keep the last set until the producer - // sends a replacement. Session 8's temporal probe is - // where visible edit-flicker/staleness gets scored. + // sends a replacement. Session 8 closed the stale + // inlay case producer-side: `didChange` marks the + // inlay store stale, and the producer sends one empty + // replacement to clear cached virtual text until a + // fresh `textDocument/inlayHint` response arrives. let text = doc.get_text(LORO_TEXT_CONTAINER).to_string(); self.set_text(&text); None diff --git a/src/inlay_hint.rs b/src/inlay_hint.rs index 295bd7b..97a98e7 100644 --- a/src/inlay_hint.rs +++ b/src/inlay_hint.rs @@ -17,7 +17,7 @@ //! Lua reads [`InlayHintStore`] and surfaces hints; a render layer can //! subscribe to the same store when it lands. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::{Arc, Mutex}; use serde_json::Value; @@ -153,6 +153,7 @@ fn parse_hint(v: &Value) -> Option { #[derive(Default)] pub struct InlayHintStore { by_key: HashMap, + stale_uris: HashSet, } /// Key into [`InlayHintStore`]. @@ -184,12 +185,31 @@ impl InlayHintStore { /// Replace the response at `key`. pub fn set(&mut self, key: InlayHintKey, response: InlayHintResponse) { + 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 hint data for it. pub fn clear(&mut self, key: &InlayHintKey) { self.by_key.remove(key); + if !self.by_key.keys().any(|k| k.uri == key.uri) { + self.stale_uris.remove(&key.uri); + } + } + + /// Mark all inlay-hint entries for `uri` stale. Called when a + /// `textDocument/didChange` is sent so renderers do not paint + /// zero-width adornments at byte anchors from pre-edit text. + pub fn mark_stale(&mut self, uri: impl Into) { + self.stale_uris.insert(uri.into()); + } + + /// `true` iff `uri` has inlay-hint 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`. @@ -314,6 +334,40 @@ mod tests { assert!(s.get(&key).is_none()); } + #[test] + fn stale_flag_clears_on_set_and_final_clear() { + let mut s = InlayHintStore::new(); + let key = InlayHintKey::new("1", "file:///a"); + let response = InlayHintResponse { + hints: vec![InlayHint { + line: 0, + col: 0, + label: "h".into(), + kind: None, + padding_left: false, + padding_right: false, + tooltip: None, + }], + }; + + 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 inlay hints clear stale flag" + ); + + s.mark_stale("file:///a"); + s.clear(&key); + assert!( + !s.is_stale("file:///a"), + "clearing final hint entry clears stale flag" + ); + } + #[test] fn for_uri_filters_by_uri_and_picks_lowest_server() { let mk = |label: &str| InlayHintResponse { diff --git a/src/lsp.rs b/src/lsp.rs index c9a2f7d..a8de7e2 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -3044,11 +3044,12 @@ 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. + // T M11.8 / Session 8 — mark cached LSP-derived render + // families stale so semantic frontends suppress byte ranges + // anchored to pre-edit text until the server refreshes them. + // Diagnostics clear on `publishDiagnostics`, semantic tokens + // on `textDocument/semanticTokens`, and inlay hints on + // `textDocument/inlayHint`. self.diag_store .lock() .expect("diag store mutex poisoned") @@ -3057,6 +3058,10 @@ impl LspManager { .lock() .expect("semantic token store mutex poisoned") .mark_stale(uri.clone()); + self.inlay_hint_store + .lock() + .expect("inlay hint 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 929449e..80130b4 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -453,7 +453,10 @@ impl SemanticRenderState { /// Every hint is `AtOffset` (inlay hints are inline by definition) /// carrying `Text` with the (padding-applied) label and the default /// style — the instance has no inlay-specific theme face yet, and a -/// fabricated one would be dishonest. +/// fabricated one would be dishonest. Stale store entries are +/// suppressed the same way stale diagnostics / semantic tokens are: +/// a zero-width hint anchored to pre-edit text is still a byte range +/// bug, even though it has no source-byte width of its own. fn scoped_inline_adornments(state: &EditorState, vp: &DeclaredViewport) -> Vec { let core = state.core.borrow(); let Some(uri) = buffer_file_uri(&core, vp.buffer_id) else { @@ -462,6 +465,9 @@ fn scoped_inline_adornments(state: &EditorState, vp: &DeclaredViewport) -> Vec resp.hints.clone(), None => return Vec::new(), @@ -1741,11 +1747,23 @@ mod tests { // --- Step 3: InlineAdornments from the LSP inlay-hint store --- - /// Seed buffer text + path and an inlay-hint store entry. Keyed by - /// `(server, uri)`; `for_uri` picks the lowest server, so a fixed - /// `"1"` is fine. No LSP client needed — the inlay path never - /// consults `semantic_style_context` (cols are already byte - /// offsets, Step 0). + fn inlay_uri() -> String { + crate::lsp::path_to_file_uri(std::path::Path::new("/tmp/h.rs")) + } + + /// Seed the inlay-hint store. Keyed by `(server, uri)`; `for_uri` + /// picks the lowest server, so a fixed `"1"` is fine. No LSP + /// client needed — the inlay path never consults + /// `semantic_style_context` (cols are already byte offsets, Step 0). + fn set_inlay_store(state: &EditorState, uri: &str, hints: Vec) { + let store = state.lsp_manager.borrow().inlay_hint_store(); + store.lock().expect("inlay store").set( + crate::inlay_hint::InlayHintKey::new("1", uri), + crate::inlay_hint::InlayHintResponse { hints }, + ); + } + + /// Seed buffer text + path and an inlay-hint store entry. fn seed_inlay( state: &EditorState, buffer_id: BufferId, @@ -1765,12 +1783,7 @@ mod tests { .expect("seed buffer text"); core.set_buffer_path(buffer_id, Some(std::path::PathBuf::from("/tmp/h.rs"))); } - let uri = crate::lsp::path_to_file_uri(std::path::Path::new("/tmp/h.rs")); - let store = state.lsp_manager.borrow().inlay_hint_store(); - store.lock().expect("inlay store").set( - crate::inlay_hint::InlayHintKey::new("1", uri), - crate::inlay_hint::InlayHintResponse { hints }, - ); + set_inlay_store(state, &inlay_uri(), hints); } fn hint(line: u32, col: u32, label: &str) -> crate::inlay_hint::InlayHint { @@ -1845,6 +1858,133 @@ mod tests { } } + #[test] + fn inline_adornments_emit_empty_clear_while_inlay_store_stale() { + let state = empty_state(); + let mut s = local(); + let bid = active_buffer(&state); + seed_inlay(&state, bid, vec![hint(0, 5, ": i32")]); + s.set_viewport(bid, ByteRange { start: 0, end: 64 }, 0); + + assert!( + adornments_of(&s.render_frame(&state)).is_some(), + "first frame ships the adornments" + ); + + let uri = inlay_uri(); + let store = state.lsp_manager.borrow().inlay_hint_store(); + store.lock().expect("inlay store").mark_stale(uri.clone()); + + let clear = + adornments_of(&s.render_frame(&state)).expect("stale transition clears adornments"); + assert!( + clear.is_empty(), + "stale hints must clear the frontend's cached virtual text" + ); + assert!( + adornments_of(&s.render_frame(&state)).is_none(), + "unchanged stale-empty state is suppressed after the clear" + ); + + set_inlay_store(&state, &uri, vec![hint(0, 5, ": i32")]); + let refreshed = adornments_of(&s.render_frame(&state)).expect("fresh hints re-emit"); + assert_eq!(refreshed.len(), 1); + assert_eq!(refreshed[0].at, 5); + } + + #[cfg(feature = "crdt")] + #[test] + fn session8_temporal_probe_sustained_edits_clear_stale_inlays_until_refresh() { + let state = empty_state(); + let mut s = local(); + let bid = active_buffer(&state); + let path = std::path::PathBuf::from("/tmp/session8-inlay.txt"); + let uri = crate::lsp::path_to_file_uri(&path); + + { + let mut core = state.core.borrow_mut(); + let mut reg = core.registry.borrow_mut(); + let buf = reg.get_mut(bid).expect("active buffer"); + buf.apply_edit(crate::buffer::EditOp::Insert { + pos: 0, + bytes: b"let x = f();\n", + }) + .expect("seed buffer text"); + buf.upgrade_to_crdt(1).expect("upgrade to crdt"); + drop(reg); + core.set_buffer_path(bid, Some(path)); + } + + set_inlay_store(&state, &uri, vec![hint(0, 5, ": i32")]); + s.set_viewport( + bid, + ByteRange { + start: 0, + end: 4096, + }, + 0, + ); + assert!( + adornments_of(&s.render_frame(&state)).is_some(), + "baseline emits the fresh inlay hint" + ); + + let mut clear_frames = 0; + let mut full_style_frames = 0; + let mut full_deco_frames = 0; + for _ in 0..1000 { + { + let core = state.core.borrow(); + let mut reg = core.registry.borrow_mut(); + let buf = reg.get_mut(bid).expect("active buffer"); + buf.apply_edit(crate::buffer::EditOp::Insert { + pos: 0, + bytes: b"x", + }) + .expect("typing edit"); + } + let store = state.lsp_manager.borrow().inlay_hint_store(); + store.lock().expect("inlay store").mark_stale(uri.clone()); + + let msgs = s.render_frame(&state); + if let Some((full, _)) = style_segments(&msgs) + && full + { + full_style_frames += 1; + } + if let Some((full, _)) = decorations_of(&msgs) + && full + { + full_deco_frames += 1; + } + if let Some(items) = adornments_of(&msgs) { + assert!( + items.is_empty(), + "stale inlay hints must not render during sustained typing" + ); + clear_frames += 1; + } + } + + assert_eq!( + clear_frames, 1, + "first stale frame clears cached hints; later stale frames stay silent" + ); + assert_eq!( + full_style_frames, 1000, + "each CRDT generation transition forces a StyleSpans full resync" + ); + assert_eq!( + full_deco_frames, 1000, + "each CRDT generation transition forces a Decorations full resync" + ); + + set_inlay_store(&state, &uri, vec![hint(0, 1005, ": i32")]); + let refreshed = adornments_of(&s.render_frame(&state)).expect("fresh hints re-emit"); + assert_eq!(refreshed.len(), 1); + assert_eq!(refreshed[0].at, 1005); + } + // --- M1: FileStyleSummary (minimap producer, Open Q#2) --- fn summary_of(msgs: &[InstanceMessage]) -> Option<(u64, Vec