From f294942ef56dc3735a9bea392df17f97a6a50244 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 29 Jul 2026 18:01:45 -0400 Subject: [PATCH] wip(stage2a): the reconciliation transaction and the URI teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One shared walk query (`buffers_bound_under`), lifted out of #190's `delete_verdict` so the guard and both reconciliation seams cannot disagree about which buffers an operation touches: every buffer, both sides normalized, component-aware containment. `EditorCore::reconcile_rename` moves the stored path and — only for a `PathDerived` name — the buffer name. `EditorCore::reconcile_delete` composes the same two removal phases `pmacs.buffer.kill` composes, preflighting `editing_in_progress` because a `ConcurrentEdit` refusal arrives after `kill_buffer` has already moved windows. Phase 2 stays with the caller; `EditorCore` gains no Lua handle. `AsyncRuntime::tick` now returns a `TickOutcome` carrying the settled ids plus the successful resource mutations, in bus-arrival order, which is documented as not being execution order. `PendingJob.resource` retains the paths the dispatchers move into the worker closure. `LspManager::forget_uri` purges the routes carrying a URI, drains the awaiters joined to them on the rid, and clears all fourteen stores plus `documents`. A generation-scoped exact-pair tombstone gates the two uncorrelated writers that can otherwise resurrect what it cleared: `publishDiagnostics` and `mark_document_stale`, which now takes a server id. `ResponseRoute::scoped_uri` is the one variant list, with `uri()` delegating to it. New Lua surface: `pmacs.buffer.set_name`, `pmacs.lsp.forget_uri`, `pmacs.diag._rename_resource`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Lv428Fth9LRtffwJSsqH7T --- src/async_runtime.rs | 127 ++++++++++++++- src/diag.rs | 16 ++ src/editor_core.rs | 262 ++++++++++++++++++++++++++++++ src/lsp.rs | 332 +++++++++++++++++++++++++++++++++++++-- src/lua_bindings/diag.rs | 25 +++ src/lua_bindings/mod.rs | 264 +++++++++++++++++++++++++++---- 6 files changed, 975 insertions(+), 51 deletions(-) diff --git a/src/async_runtime.rs b/src/async_runtime.rs index 493d993..551458f 100644 --- a/src/async_runtime.rs +++ b/src/async_runtime.rs @@ -391,6 +391,70 @@ struct PendingJob { /// When the job was registered. Used to compute "age" in the /// `*workers*` buffer. dispatched_at: Instant, + /// The filesystem mutation this job performs, retained so the + /// main-thread drain can reconcile the editor's path owners once + /// the syscall lands (dired Stage 2a, §5). + /// + /// The paths have to live here because the dispatchers **move** + /// them into the worker closure and nothing else retains them, and + /// because the reply is undifferentiated — rename and remove both + /// settle as `ReplyKind::FsUnit`, so a drain cannot key on the + /// reply and must key on the pending job. + /// + /// One enum field rather than a pair of `Option`s: two would admit + /// a both-`Some` state that cannot occur, which every consumer + /// would then have to rule out by hand. `COHERENCE.md` §9 is why + /// this is a field on the job and not a side map — the parse + /// job→buffer link already lives in a side map and §9 names that as + /// the defect. + resource: Option, +} + +/// A settled filesystem mutation, with the paths the worker consumed +/// (dired Stage 2a, §5). +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum ResourceOp { + /// A successful `rename(from, to)`. + Rename { + /// Source path, as the caller spelled it. + from: PathBuf, + /// Destination path, as the caller spelled it. + to: PathBuf, + }, + /// A successful `remove(path)`. + Remove { + /// The path that was removed. + path: PathBuf, + }, +} + +/// What one [`AsyncRuntime::tick`] observed. +/// +/// Settle identity and resource metadata come out of **one** +/// transaction — the post-drain loop already borrows `pending` to +/// record completions — so a consumer cannot see a settle without its +/// resource, or the reverse. +#[derive(Clone, Debug, Default)] +pub struct TickOutcome { + /// Ids that transitioned from `Running` to a terminal state during + /// this tick. The Lua runtime resumes coroutines parked on these. + pub settled: Vec, + /// Successful resource mutations, **in bus-arrival order. This is + /// not filesystem execution order.** + /// + /// [`AsyncRuntime::tick`] drains the reply bus with `try_recv` and + /// the runtime establishes no execution token, so a worker can + /// complete, be descheduled before sending, and have a later + /// mutation's reply arrive first. A consumer that reads "in settle + /// order" and infers causality is wrong; reconciliation is + /// deliberately order-independent (Q#DR29), and the primitive's + /// contract is that a caller with overlapping source/target paths + /// serializes by awaiting each op before dispatching the next. + /// + /// Carries **only** jobs that settled + /// [`PendingState::Complete`] — a failed or cancelled mutation + /// reconciles nothing and fires no hook. + pub resources: Vec, } /// Snapshot of a job's terminal state, returned by @@ -684,6 +748,18 @@ impl AsyncRuntime { kind: JobKind, supersede_key: Option<&str>, stream: Option, + ) -> (JobId, CancellationToken) { + self.allocate_with_resource(kind, supersede_key, stream, None) + } + + /// [`Self::allocate`], plus the filesystem mutation this job + /// performs. Only the two mutating fs dispatchers pass `resource`. + fn allocate_with_resource( + &self, + kind: JobKind, + supersede_key: Option<&str>, + stream: Option, + resource: Option, ) -> (JobId, CancellationToken) { let id = self.next_job_id.fetch_add(1, Ordering::Relaxed); let cancel = CancellationToken::new(); @@ -711,6 +787,7 @@ impl AsyncRuntime { max_batch: stream.unwrap_or(0), kind, dispatched_at: Instant::now(), + resource, }, ); (id, cancel) @@ -869,7 +946,17 @@ impl AsyncRuntime { /// Dispatch a `rename(from, to)` job. Settles to /// [`JobResult::Unit`] on success. T M8.1. pub fn dispatch_fs_rename(&self, from: PathBuf, to: PathBuf, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::FsRename, supersede, None); + // The closure below MOVES both paths; the pending entry is the + // only thing that still knows them when the reply lands. + let (id, cancel) = self.allocate_with_resource( + JobKind::FsRename, + supersede, + None, + Some(ResourceOp::Rename { + from: from.clone(), + to: to.clone(), + }), + ); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_rename(&cancel, &from, &to); @@ -891,7 +978,12 @@ impl AsyncRuntime { /// Dispatch a `remove(path)` job. T M8.1. pub fn dispatch_fs_remove(&self, path: PathBuf, supersede: Option<&str>) -> JobId { - let (id, cancel) = self.allocate(JobKind::FsRemove, supersede, None); + let (id, cancel) = self.allocate_with_resource( + JobKind::FsRemove, + supersede, + None, + Some(ResourceOp::Remove { path: path.clone() }), + ); let bus = self.workers.clone(); self.pool.dispatch(move |_pool| { let kind = run_fs_remove(&cancel, &path); @@ -996,11 +1088,16 @@ impl AsyncRuntime { } } - /// Drain every queued reply on the main-thread bus, update - /// pending entries, and return the list of ids that *transitioned - /// from Running to a terminal state* during this tick. The Lua - /// runtime resumes coroutines parked on these ids. - pub fn tick(&self) -> Vec { + /// Drain every queued reply on the main-thread bus, update pending + /// entries, and report what settled. + /// + /// [`TickOutcome::settled`] is the ids that transitioned from + /// `Running` to a terminal state during this tick — the Lua runtime + /// resumes coroutines parked on these. + /// [`TickOutcome::resources`] is the filesystem mutations among them + /// that **succeeded**, in **bus-arrival order** (see the field's + /// own documentation: that is not execution order). + pub fn tick(&self) -> TickOutcome { let mut newly_settled = Vec::new(); while let Ok(env) = self.main.try_recv() { let Ok(reply): Result = self.main.decode(&env) else { @@ -1071,6 +1168,7 @@ impl AsyncRuntime { // a successor that came in mid-flight will have overwritten // the entry already, and that successor's pending lifetime // is what owns the slot now. + let mut resources = Vec::new(); if !newly_settled.is_empty() { let pending = self.pending.borrow(); let mut sup = self.supersede.borrow_mut(); @@ -1078,6 +1176,16 @@ impl AsyncRuntime { let now = Instant::now(); for id in &newly_settled { if let Some(job) = pending.get(id) { + // The harvest (§5): one more read in a loop that + // already borrows `pending` and reads `job.kind`, + // so settle identity and resource metadata come out + // of one transaction. Gated on `Complete` — a + // failed or cancelled mutation reconciles nothing. + if let Some(resource) = &job.resource + && matches!(job.state, PendingState::Complete(_)) + { + resources.push(resource.clone()); + } if let Some(key) = &job.supersede_key && sup.get(key) == Some(id) { @@ -1106,7 +1214,10 @@ impl AsyncRuntime { completed.pop_back(); } } - newly_settled + TickOutcome { + settled: newly_settled, + resources, + } } /// Snapshot the runtime's job tables for the `*workers*` diff --git a/src/diag.rs b/src/diag.rs index ba67450..f773321 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -266,6 +266,22 @@ impl DiagnosticStore { *self.epochs.entry(uri.to_owned()).or_insert(0) += 1; } + /// Drop **every** trace of `uri`, epoch included (dired Stage 2a, + /// §5 finding 4). + /// + /// Distinct from [`Self::clear`] on purpose: `clear` *creates* an + /// `epochs` entry (`or_insert(0) += 1`) because a consumer caching + /// against the epoch must observe that the diagnostics went away. + /// Forgetting is the opposite intent — the editor no longer holds + /// this URI at all — so leaving the counter behind would be a + /// URI-keyed leak in the one map nothing else prunes. + pub fn forget(&mut self, uri: &str) { + self.by_uri.remove(uri); + self.severity_counts.remove(uri); + self.stale_uris.remove(uri); + self.epochs.remove(uri); + } + /// Monotonic per-URI change counter: how many times `set` / /// `clear` ran for this URI. `0` for a URI never written. /// Consumers cache against this to detect republishes that no diff --git a/src/editor_core.rs b/src/editor_core.rs index 56ccfbf..732e19a 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -4840,6 +4840,191 @@ impl EditorCore { .map_err(|e| e.to_string()) } + /// Rebind every buffer affected by a successful rename of `old` to + /// `new` (dired Stage 2a, Q#DR14). Returns one + /// [`RenameRebind`] per buffer moved. + /// + /// A rename is a **transaction across path owners**, not a field + /// update. This method owns the two owners that live in the buffer: + /// the stored path and — subject to the provenance rule below — the + /// name. Everything else keyed by the path (URI-keyed LSP stores, + /// diagnostic overlays, dired's pathless handles, a package's own + /// URI table) reconciles off the `resource.renamed` hook that the + /// caller fires, because no buffer-keyed rebind can reach them. + /// + /// Both rename paths call this — the drain harvest for + /// `pmacs.fs.rename` and `apply_resource_op`'s rename arm — so the + /// two cannot drift apart. + /// + /// # The name + /// + /// The name is rewritten only for a buffer whose name is + /// [`crate::buffer::BufferNameOrigin::PathDerived`]. String + /// inspection cannot substitute for that bit in either direction: a + /// relative open is named `foo.rs` (so an equality test leaves it + /// stale), and a user may name a buffer with a string that + /// normalizes to its own path (so a path-equivalence test + /// overwrites a chosen name). When it does fire, the new name is + /// the **normalized** new path — a buffer opened relatively + /// therefore acquires an absolute name, because no buffer records + /// which base its name was relative to. Reconciliation re-records + /// `PathDerived`, so a second rename still follows. + pub fn reconcile_rename(&mut self, old: &Path, new: &Path) -> Vec { + let old_n = normalize_buffer_path(old.to_path_buf()); + let new_n = normalize_buffer_path(new.to_path_buf()); + // A directory rename moves its whole subtree by construction, + // so descendants are always in scope here. + let affected = { + let reg = self.registry.borrow(); + buffers_bound_under(®, &old_n, true) + }; + let mut rebinds = Vec::with_capacity(affected.len()); + for (id, bound) in affected { + // Rebuild the path under the new root. An exact match maps + // to `new` itself; a descendant keeps its relative tail. + let target = if bound == old_n { + new_n.clone() + } else { + match bound.strip_prefix(&old_n) { + Ok(tail) => new_n.join(tail), + // Unreachable: `buffers_bound_under` matched on + // exactly this prefix. Skip rather than guess. + Err(_) => continue, + } + }; + let name_followed = { + let mut reg = self.registry.borrow_mut(); + let Ok(buf) = reg.get_mut(id) else { continue }; + buf.set_file_path(Some(target.clone())); + // The file behind this buffer moved, so metadata + // captured against the old path no longer describes + // it. Clearing is what `set_buffer_path`'s callers do + // via `set_buffer_meta`; leaving it would make + // external-change detection compare against a stat of + // a path that is gone. + buf.set_file_meta(None); + if buf.name_origin() == crate::buffer::BufferNameOrigin::PathDerived { + buf.set_path_derived_name(target.display().to_string()); + true + } else { + false + } + }; + rebinds.push(RenameRebind { + buffer_id: id, + old_path: bound, + new_path: target, + name_followed, + }); + } + rebinds + } + + /// Reconcile the buffers a successful delete of `path` orphaned + /// (dired Stage 2a, Q#DR18). + /// + /// Walks the whole registry by normalized equality **or** + /// component-aware prefix, so descendants of a deleted directory + /// are included and a second buffer on one path is not missed. + /// Descendants are unconditionally in scope here, unlike in + /// `delete_verdict`: a recursive delete destroyed them, and a + /// non-recursive one only succeeds on an *empty* directory, so a + /// buffer still bound underneath it was already an orphan. + /// + /// Policy, per buffer: + /// + /// * **modified** — kept alive and reported. The buffer keeps its + /// contents; only the file is gone. This is the half of the + /// promise that is robust, because it runs at drain time against + /// whatever state exists then. + /// * **mid-edit** — skipped entirely and reported in `refused`, + /// **preflighted** rather than discovered. A refusal from + /// `BufferRegistry::remove` is *not* inert: by the time it + /// returns `ConcurrentEdit`, [`Self::kill_buffer`] has already + /// dropped the id from `round_trip_buffers`, closed any side + /// window showing the buffer, and redirected every remaining + /// window onto a fallback with cursor, selection, overlays and + /// scroll position reset. The preflight is *sound*, not merely + /// cheap: phase 1 is entirely `EditorCore`, which holds no Lua + /// handle, so nothing between the check and the removal can + /// re-enter Lua and begin an edit. + /// * otherwise — killed through the full phase 1 above. + /// + /// Neither refusal aborts the rest: a directory delete reaching + /// twelve descendants must not stop at the one that is mid-edit. + /// + /// # Phase 2 is the caller's + /// + /// Buffer removal is two phases and the only place they are + /// composed today is a Lua binding (`pmacs.buffer.kill`). Phase 2 — + /// buffer-scoped keymaps, buffer-local config, folds, and the + /// registered `on_removed` callbacks — lives in `lua_bindings` and + /// needs `&Lua`, so this returns [`DeleteReconcile::killed`] and + /// its caller runs phase 2 over those ids. `EditorCore` does not + /// gain a Lua handle. + pub fn reconcile_delete(&mut self, path: &Path) -> DeleteReconcile { + let affected = { + let reg = self.registry.borrow(); + buffers_bound_under(®, path, true) + }; + let mut out = DeleteReconcile::default(); + for (id, _bound) in affected { + let preflight = { + let reg = self.registry.borrow(); + let Ok(buf) = reg.get(id) else { continue }; + let name = buf.name().to_owned(); + if buf.is_modified() { + Some(Err((true, name))) + } else if buf.editing_in_progress() { + Some(Err((false, name))) + } else { + Some(Ok(())) + } + }; + match preflight { + Some(Ok(())) => {} + Some(Err((true, name))) => { + out.kept_modified.push((id, name)); + continue; + } + Some(Err((false, name))) => { + out.refused.push(( + id, + format!("buffer {name:?} is mid-edit; finish the edit first"), + )); + continue; + } + None => continue, + } + match self.kill_buffer(id) { + Ok(()) => out.killed.push(id), + Err(message) => out.refused.push((id, message)), + } + } + out + } + + /// Re-root every URI-keyed overlay in **every** window from + /// `old_uri` to `new_uri` (dired Stage 2a, §5). + /// + /// The traversal mirrors overlay disposal's + /// (`lua_bindings`'s `retain` over `overlay_identity`), with the + /// `retain` replaced by [`View::rename_resource`]. That reaches + /// passive windows as well as the active one — which the Lua attach + /// path cannot, since `pmacs.diag._attach_view` takes the active + /// window and errors otherwise — and preserves composition order, + /// because nothing is removed or re-pushed. + /// + /// A window that never received the overlay still has none; + /// renaming cannot re-root an overlay that was never attached. + pub fn rename_resource_in_views(&mut self, old_uri: &str, new_uri: &str) { + for win in self.windows.values_mut() { + for overlay in &mut win.overlays { + overlay.rename_resource(old_uri, new_uri); + } + } + } + /// Switch one frontend's active window to a different buffer, allocating /// a fresh [`TextView`] for it without changing global active state. pub fn switch_active_buffer_for( @@ -5097,6 +5282,83 @@ fn backward_word(buf: &Buffer, mut pos: Position) -> Position { pos } +/// Every path-bound buffer an operation on `target` affects, paired +/// with its **normalized** stored path (dired Stage 2a; the shared walk +/// query #190 introduced for `delete_verdict`, lifted so rename +/// reconciliation and delete reconciliation cannot drift from it). +/// +/// Three properties, each of which a naive lookup gets wrong: +/// +/// * It scans **every** buffer. +/// [`crate::buffer_registry::BufferRegistry::find_by_path`] is +/// first-match-only, and duplicate path-bound buffers are reachable +/// from public Lua via `pmacs.buffer.from_file` — so a first match +/// can hide a second buffer on the same path, which then survives +/// pointing at a path that no longer exists. +/// * Both sides are normalized. Stored paths are normalized on write +/// (`set_buffer_path`) while an op names its target however the +/// caller spelled it, so a raw comparison misses the match entirely. +/// * Containment is **component-aware** ([`Path::starts_with`]), never +/// a string prefix: `/foo` is not an ancestor of `/foobar`. +/// +/// `include_descendants` is the caller's decision because the two +/// consumers legitimately differ. A delete *guard* scopes descendants +/// to `recursive` (#190: a non-recursive delete destroys nothing +/// beneath the target, so a buffer under it must not refuse the op), +/// whereas a **rename** always moves its whole subtree and a +/// post-delete reconciliation is looking at a directory that is +/// already gone. +pub fn buffers_bound_under( + reg: &crate::buffer_registry::BufferRegistry, + target: &Path, + include_descendants: bool, +) -> Vec<(BufferId, PathBuf)> { + let target = normalize_buffer_path(target.to_path_buf()); + let mut out = Vec::new(); + for id in reg.ids() { + let Ok(buf) = reg.get(*id) else { continue }; + let Some(bound) = buf.file_path() else { continue }; + let bound = normalize_buffer_path(bound.to_path_buf()); + if bound == target || (include_descendants && bound.starts_with(&target)) { + out.push((*id, bound)); + } + } + out +} + +/// One buffer moved by [`EditorCore::reconcile_rename`]. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct RenameRebind { + /// The buffer that moved. + pub buffer_id: BufferId, + /// Its normalized path before the rename. + pub old_path: PathBuf, + /// Its normalized path after the rename. + pub new_path: PathBuf, + /// Whether the buffer's **name** followed the path, per + /// [`crate::buffer::BufferNameOrigin`]. Reported rather than + /// inferred so a consumer does not have to re-derive the + /// provenance rule. + pub name_followed: bool, +} + +/// Outcome of [`EditorCore::reconcile_delete`]. +/// +/// Three lists rather than two, because "kept on purpose" and "could +/// not be removed" are different events: collapsing them makes a +/// failure look like a policy decision. +#[derive(Clone, Debug, Default)] +pub struct DeleteReconcile { + /// Buffers whose phase 1 (core-side removal) completed. The + /// caller **must** run phase 2 (`after_buffer_removed`) over + /// these — `EditorCore` holds no Lua handle. + pub killed: Vec, + /// Modified buffers kept alive deliberately, with their names. + pub kept_modified: Vec<(BufferId, String)>, + /// Buffers that could not be removed, with the reason. + pub refused: Vec<(BufferId, String)>, +} + /// Normalize a buffer path to an absolute, lexically-clean form: /// /// 1. expand a leading `~` / `~/…` against `$HOME`, diff --git a/src/lsp.rs b/src/lsp.rs index 1eb8024..2d78f4a 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -821,6 +821,37 @@ pub struct LspManager { /// per-server [`crate::lsp_status::LspStatus`] for the modeline / /// `*lsp*` buffer. status_tracker: crate::lsp_status::LspStatusTracker, + /// dired Stage 2a §5 — exact `(server, uri)` pairs this editor + /// **explicitly forgot**, so a later *uncorrelated* write cannot + /// resurrect them. + /// + /// [`Self::forget_uri`] purges `pending_routes` and drains their + /// awaiters, which covers every write that is matched to a request + /// id. It cannot cover the writers that never go near a route, and + /// there are two that create state: + /// `textDocument/publishDiagnostics`, absorbed unconditionally — + /// and note that `diag_store` has **zero** correlated writers, so + /// the one store the purge most needs to protect is the one it + /// cannot help at all — and [`Self::mark_document_stale`], which + /// creates URI keys in three stores. + /// + /// Deliberately not the cheaper membership gate ("absorb only if + /// `(sid, uri)` is in `documents`"): servers legitimately publish + /// diagnostics for files the editor never opened — a crate-wide + /// push naming a dependency — and a membership gate drops every + /// one. A tombstone drops only what we forgot. `handle_response` + /// already uses this shape for late arrivals + /// (`client.cancelled_rids`); this is the same pattern with a + /// `(server, URI)` key instead of a request id. + /// + /// **Reclaimed and generation-scoped, not size-bounded.** + /// `did_open(sid, uri)` clears that exact pair; + /// [`Self::start_generation`] and [`Self::forget`] remove every pair + /// for their server and retain every other server's. A capacity or + /// LRU eviction would let an arbitrarily late notification + /// resurrect an evicted key, which is the whole failure this gate + /// exists to stop. + forgotten_documents: std::collections::HashSet<(LspServerId, String)>, /// T M4.9: `(project_root, language_id)` → server id. Drives the /// "LSP runs per-project, not per-buffer" invariant. Roots are /// stored as [`PathBuf`] so callers don't have to canonicalise @@ -901,9 +932,21 @@ enum ResponseRoute { } impl ResponseRoute { - /// The document URI this route targets — the key for the position - /// codec's document/encoding lookup. - fn uri(&self) -> &str { + /// The document URI this route is **scoped to**, if any (dired + /// Stage 2a, §5). + /// + /// Fourteen of the fifteen variants carry a `uri`. The fifteenth, + /// `WorkspaceSymbol`, carries a **query** and no URI at all — its + /// own comment explains that the query stands in for the doc URI in + /// the supersede key — so it answers `None`, and + /// [`LspManager::forget_uri`]'s purge retains it: a + /// workspace-symbol query is not scoped to any document and a + /// rename does not invalidate it. + /// + /// Exhaustive on purpose. A new URI-bearing variant must not + /// silently default to "not scoped", which would leave an in-flight + /// response able to repopulate a forgotten key. + fn scoped_uri(&self) -> Option<&str> { match self { ResponseRoute::Completion { uri } | ResponseRoute::Hover { uri } @@ -918,14 +961,25 @@ impl ResponseRoute { | ResponseRoute::SemanticTokensDelta { uri } | ResponseRoute::Locations { uri, .. } | ResponseRoute::DocumentSymbol { uri } - | ResponseRoute::DocumentHighlight { uri } => uri, - // workspace/symbol results span arbitrary files we have - // not cached — no doc to convert against, so the inbound - // codec must pass coordinates through untouched (same - // non-destructive rule as cross-file definition). - ResponseRoute::WorkspaceSymbol { .. } => "", + | ResponseRoute::DocumentHighlight { uri } => Some(uri), + ResponseRoute::WorkspaceSymbol { .. } => None, } } + + /// The document URI this route targets — the key for the position + /// codec's document/encoding lookup. + /// + /// Delegates to [`Self::scoped_uri`] so the variant list exists + /// once: two near-identical matches over fifteen variants is how + /// one of them ends up missing a variant the other has. + /// `workspace/symbol` results span arbitrary files we have not + /// cached, so there is no doc to convert against and the inbound + /// codec must pass coordinates through untouched (the same + /// non-destructive rule as cross-file definition) — which the empty + /// string already expressed. + fn uri(&self) -> &str { + self.scoped_uri().unwrap_or("") + } } /// One Lua-visible awaiter bound to an in-flight LSP request. Mirrors @@ -1021,6 +1075,7 @@ impl LspManager { semantic_token_store: crate::semantic_tokens::make_shared_store(), pending_routes: HashMap::new(), status_tracker: crate::lsp_status::LspStatusTracker::new(), + forgotten_documents: std::collections::HashSet::new(), project_servers: HashMap::new(), } } @@ -1329,6 +1384,10 @@ impl LspManager { // T M4.5 Option B: drop cached docs; the fresh server gets a // new `did_open` from the editor's reattach path. self.documents.retain(|(s, _), _| *s != id); + // dired Stage 2a §5 — the tombstone is generation-scoped: this + // generation's forgotten pairs go, every other server's stay. + // The reattach path re-`did_open`s whatever it still holds. + self.forgotten_documents.retain(|(s, _)| *s != id); client.state = LspClientState::Starting; let proc_spec = client.spec.to_process_spec(); let pid = self.supervisor.borrow_mut().spawn(proc_spec)?; @@ -2901,6 +2960,18 @@ impl LspManager { let Some(uri) = params.get("uri").and_then(Value::as_str).map(str::to_owned) else { return; }; + // dired Stage 2a §5 — the uncorrelated-write gate. This + // notification carries no request id, so `forget_uri`'s route + // purge cannot see it, and `diag_store` has no correlated + // writers at all: without this check a late publish for a + // renamed-away URI silently reinstates the state we just + // forgot. The gate is the exact `(server, uri)` pair, which is + // available here even though `DiagnosticStore.by_uri` is keyed + // by URI alone — so provenance is retained for selective + // teardown without changing the store's key. + if self.forgotten_documents.contains(&(sid, uri.clone())) { + return; + } // T M4.5 Option B: byte-normalise diagnostic ranges before the // store parses them, so the gutter renders correct spans on // non-ASCII lines. @@ -3031,6 +3102,9 @@ impl LspManager { // between exit and forget. Idempotent. self.drain_external_cancelled(sid); self.documents.retain(|(s, _), _| *s != sid); + // dired Stage 2a §5 — terminal removal drops every tombstone + // this server owned; other servers' pairs are retained. + self.forgotten_documents.retain(|(s, _)| *s != sid); self.status_tracker.forget(sid); // T M4.9: drop the project scoping so the next // ensure_server_for_project call spawns a fresh server. @@ -3038,6 +3112,223 @@ impl LspManager { Ok(()) } + /// Drop **every** trace of `uri` under `sid` (dired Stage 2a, §5). + /// + /// One manager-level method rather than fourteen call sites at the + /// Lua layer, because fourteen call sites is how one gets + /// forgotten. Four ordered steps: + /// + /// 1. **Tombstone `(sid, uri)` first**, before clearing anything. + /// Main-thread execution already makes the rest atomic with + /// respect to another manager tick, but putting the gate first + /// means every later call observes the forgotten state even if a + /// future refactor introduces an early return. + /// 2. **Purge `pending_routes`** whose route carries this URI. + /// `WorkspaceSymbol` is retained unconditionally: it carries no + /// URI at all — its query stands in for the doc URI in the + /// supersede key — and a workspace-symbol query is not scoped to + /// any document, so a rename does not invalidate it. Clearing + /// the stores *without* this purge is a race that reintroduces + /// exactly the state it removed: a response already in flight + /// routes on arrival and repopulates the old key after the clear. + /// 3. **Drain-cancel their awaiters.** `pending_external` holds the + /// `Handle:await()` side, and its contract is explicit that it is + /// drained-cancelled wherever `pending_routes` is purged. Neither + /// existing sweep is URI-scoped — both range over `sid` — so this + /// joins route to awaiter on the `rid`, which is the only index + /// between them. The model is + /// [`Self::drain_external_cancelled`], which is *unconditional*; + /// modelling on `drain_cancelled_externals` instead would drain + /// nothing, because it removes only awaiters whose cancellation + /// token was flipped or which outlived the request timeout, and + /// **a rename flips no token** — leaving any coroutine awaiting + /// against the old URI parked forever. + /// 4. **Clear all fourteen stores plus `documents`.** Two keys are + /// irregular: `locations_store` is *kind*-keyed, so all four + /// kinds must go, and `symbol_store` is *scope*-keyed and holds + /// workspace symbols too, so only the document-scoped entry is + /// dropped — the same asymmetry that makes `WorkspaceSymbol` + /// route-exempt above. Diagnostics go through + /// [`crate::diag::DiagnosticStore::forget`], not `clear`: `clear` + /// *increments* the epoch it is meant to forget. + /// + /// Takes the **old** URI, so calling it after `did_open` of the new + /// one is safe and order-independent. + /// + /// Note there is **no precedent to copy for the store half**: + /// neither server-scoped teardown clears the fourteen result stores. + /// `start_generation` clears deferred notifications, routes, + /// documents and externals; `forget` clears routes, documents, + /// externals, the status tracker and project scoping. Whether stale + /// results should survive a restart is a separate pre-existing + /// question, and this method deliberately does not answer it. + /// + /// # Errors + /// + /// Unknown `sid`, matching [`Self::forget`]'s behaviour for the same + /// input. A URI with **no** state under a known server is an + /// idempotent **success**: the caller runs per attachment, an + /// attachment need not have any pending route or populated result + /// store, and cleanup can be repeated after an earlier partial + /// teardown. + pub fn forget_uri(&mut self, sid: LspServerId, uri: &str) -> Result<(), String> { + if !self.clients.contains_key(&sid) { + return Err(format!("unknown server: {sid}")); + } + // Step 1 — the gate, first. + self.forgotten_documents.insert((sid, uri.to_owned())); + + // Step 2 — collect the rids this URI owns, then purge. + let doomed_rids: Vec = self + .pending_routes + .iter() + .filter(|((s, _), route)| *s == sid && route.scoped_uri() == Some(uri)) + .map(|((_, rid), _)| *rid) + .collect(); + for rid in &doomed_rids { + self.pending_routes.remove(&(sid, *rid)); + } + + // Step 3 — settle the awaiters joined to those rids cancelled. + for rid in &doomed_rids { + if let Some(p) = self.pending_external.remove(&(sid, *rid)) { + for a in &p.awaiters { + self.runtime.complete_external_cancelled(a.job_id); + } + } + } + + // Step 4 — the fourteen stores plus `documents`. + let server_key = sid.raw().to_string(); + self.diag_store + .lock() + .expect("diag store mutex poisoned") + .forget(uri); + self.completion_store + .lock() + .expect("completion store mutex poisoned") + .clear(&crate::completion::CompletionKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.hover_store + .lock() + .expect("hover store mutex poisoned") + .clear(&crate::hover::HoverKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.signature_store + .lock() + .expect("signature store mutex poisoned") + .clear(&crate::signature::SignatureKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.definition_store + .lock() + .expect("definition store mutex poisoned") + .clear(&crate::definition::DefinitionKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + { + let mut guard = self + .locations_store + .lock() + .expect("locations store mutex poisoned"); + // Kind-keyed: all four have to go. + for kind in [ + crate::locations::LocationKind::References, + crate::locations::LocationKind::Declaration, + crate::locations::LocationKind::TypeDefinition, + crate::locations::LocationKind::Implementation, + ] { + guard.clear(&crate::locations::LocationsKey { + server: server_key.clone(), + uri: uri.to_owned(), + kind, + }); + } + } + self.symbol_store + .lock() + .expect("symbol store mutex poisoned") + // Scope-keyed, and the store also holds workspace symbols: + // only the document-scoped entry is dropped. + .clear(&crate::symbol::SymbolKey { + server: server_key.clone(), + scope: crate::symbol::SymbolScope::Document(uri.to_owned()), + }); + self.document_highlight_store + .lock() + .expect("document highlight store mutex poisoned") + .clear(&crate::document_highlight::DocumentHighlightKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.formatting_store + .lock() + .expect("formatting store mutex poisoned") + .clear(&crate::formatting::FormattingKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.rename_store + .lock() + .expect("rename store mutex poisoned") + .clear(&crate::rename::RenameKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.prepare_rename_store + .lock() + .expect("prepare rename store mutex poisoned") + .clear(&crate::prepare_rename::PrepareRenameKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.code_action_store + .lock() + .expect("code action store mutex poisoned") + .clear(&crate::code_action::CodeActionKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.inlay_hint_store + .lock() + .expect("inlay hint store mutex poisoned") + .clear(&crate::inlay_hint::InlayHintKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.semantic_token_store + .lock() + .expect("semantic token store mutex poisoned") + .clear(&crate::semantic_tokens::SemanticTokenKey { + server: server_key.clone(), + uri: uri.to_owned(), + }); + self.documents.remove(&(sid, uri.to_owned())); + Ok(()) + } + + /// Whether `(sid, uri)` is currently tombstoned (dired Stage 2a). + /// Read surface for tests; production code consults the set + /// directly at its two gates. + #[must_use] + pub fn is_forgotten(&self, sid: LspServerId, uri: &str) -> bool { + self.forgotten_documents.contains(&(sid, uri.to_owned())) + } + + /// How many `(server, uri)` pairs are tombstoned. Read surface for + /// the reclamation tests — the set must not grow without bound + /// across teardowns. + #[must_use] + pub fn forgotten_document_count(&self) -> usize { + self.forgotten_documents.len() + } + /// Convenience: send `textDocument/didOpen` to `sid`. pub fn did_open( &mut self, @@ -3053,6 +3344,12 @@ impl LspManager { .ok_or_else(|| format!("unknown server: {sid}"))?; let uri = uri.into(); let text = text.into(); + // dired Stage 2a §5 — reclaim the tombstone for THIS exact pair + // and no other. Reopening the document is the editor saying it + // holds the URI again, so a later publish or stale-mark for it + // must be admitted; another server's tombstone for the same URI + // is untouched. + self.forgotten_documents.remove(&(sid, uri.clone())); // T M4.5 Option B: mirror the document so the position codec // can convert per-line between the server's `character` units // and pmacs byte offsets. @@ -3081,7 +3378,7 @@ impl LspManager { let uri = uri.into(); let text = text.into(); self.documents.insert((sid, uri.clone()), text.clone()); - self.mark_document_stale(&uri); + self.mark_document_stale(sid, &uri); let params = json!({ "textDocument": { "uri": uri, @@ -3105,7 +3402,20 @@ impl LspManager { /// mark staleness at *edit* time even while the (full-document, /// O(file)) didChange notification itself is debounced — per-edit /// staleness is what keeps stale-position artifacts off screen. - pub fn mark_document_stale(&self, uri: &str) { + /// + /// **Takes `sid` since dired Stage 2a.** It previously took no + /// server id while *creating* URI keys in three stores for every + /// server at once, which made it the second uncorrelated writer able + /// to resurrect a forgotten URI — and made an exact tombstone + /// impossible. Every caller already owns the attachment's server id, + /// so the parameter costs nothing. + pub fn mark_document_stale(&self, sid: LspServerId, uri: &str) { + // The second uncorrelated-write gate (§5 finding 2). Returns + // before touching any of the three stores, so a forgotten URI + // cannot regain a stale flag either. + if self.forgotten_documents.contains(&(sid, uri.to_owned())) { + return; + } self.diag_store .lock() .expect("diag store mutex poisoned") diff --git a/src/lua_bindings/diag.rs b/src/lua_bindings/diag.rs index c462f44..338dfc3 100644 --- a/src/lua_bindings/diag.rs +++ b/src/lua_bindings/diag.rs @@ -232,6 +232,31 @@ pub fn install_diag( )?; } + // dired Stage 2a §5 step 6 — re-root every attached + // `DiagnosticView` from `old_uri` to `new_uri` after a rename. + // + // `DiagnosticView.uri` is set once at construction and is private, + // and `View` has no downcast, so nothing outside `diag.rs` can + // reach it; the `View::rename_resource` hook is the seam. The sweep + // walks EVERY window, which is what `_attach_view` above cannot do + // — it takes the active window and errors otherwise — so a passive + // split that already holds the overlay is re-rooted too. It mutates + // in place, so each overlay keeps its position in the window's + // composition order; a remove-and-re-push would move the underline + // to the end of the stack and pass a one-window test anyway. + { + diag_mod.set( + "_rename_resource", + lua.create_function(move |lua, (old_uri, new_uri): (String, String)| { + let Some(core) = lua.app_data_ref::() else { + return Ok(false); + }; + core.borrow_mut().rename_resource_in_views(&old_uri, &new_uri); + Ok(true) + })?, + )?; + } + pmacs.set("diag", diag_mod)?; Ok(()) } diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index bbdb7fa..212e962 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1671,16 +1671,11 @@ fn delete_verdict( } }; - let target = crate::editor_core::normalize_buffer_path(path.to_path_buf()); - for id in reg.ids() { - let Ok(buf) = reg.get(*id) else { continue }; - let Some(bound) = buf.file_path() else { - continue; - }; - let bound = crate::editor_core::normalize_buffer_path(bound.to_path_buf()); - if bound != target && !(scan_descendants && bound.starts_with(&target)) { - continue; - } + // The shared walk (dired Stage 2a): one enumeration, so this guard + // and the two reconciliation seams cannot disagree about which + // buffers an operation on `path` touches. + for (id, _bound) in crate::editor_core::buffers_bound_under(reg, path, scan_descendants) { + let Ok(buf) = reg.get(id) else { continue }; // "Modified" is `Buffer::is_modified()`. No new notion of // dirtiness, and a *clean* open buffer is deliberately not // guarded — refusing there would fail legitimate deletes for @@ -1714,6 +1709,131 @@ fn delete_verdict( DeleteVerdict::Clear } +/// Reconcile a successful rename and fire `resource.renamed` (dired +/// Stage 2a, §5). +/// +/// Both rename paths land here — the drain harvest for +/// `pmacs.fs.rename` and `apply_resource_op`'s rename arm — so the two +/// can no longer drift, which is how the raw-lookup trap survived being +/// "fixed" once already. +/// +/// The hook carries the **paths**, normalized absolute, not the rebind +/// list: dired's buffers are pathless, so a path-keyed consumer must be +/// able to reconcile from `(old, new)` alone. And the Rust side is +/// structurally incapable of being complete — any package may key state +/// by URI in its own module table and the LSP manager will never know — +/// so the hook is the mechanism that scales, not a convenience. +/// +/// Returns the rebinds, for a caller that wants to report. +fn reconcile_rename_and_fire( + lua: &Lua, + from: &std::path::Path, + to: &std::path::Path, +) -> Vec { + let (rebinds, old_n, new_n) = { + let Some(core) = lua.app_data_ref::() else { + return Vec::new(); + }; + let mut core = core.borrow_mut(); + let rebinds = core.reconcile_rename(from, to); + ( + rebinds, + crate::editor_core::normalize_buffer_path(from.to_path_buf()), + crate::editor_core::normalize_buffer_path(to.to_path_buf()), + ) + }; + // The borrow is released before re-entering Lua: subscribers call + // back into the core (dired reverts a listing, the LSP subscriber + // re-attaches), and a live borrow would panic. + let mut args = mlua::MultiValue::new(); + args.push_back(mlua::Value::String( + match lua.create_string(old_n.as_os_str().as_encoded_bytes()) { + Ok(s) => s, + Err(_) => return rebinds, + }, + )); + args.push_back(mlua::Value::String( + match lua.create_string(new_n.as_os_str().as_encoded_bytes()) { + Ok(s) => s, + Err(_) => return rebinds, + }, + )); + run_hook_if_defined(lua, "resource.renamed", args); + rebinds +} + +/// Reconcile a successful delete and fire `resource.deleted` (dired +/// Stage 2a, §6). +/// +/// Composes the **same two removal phases** `pmacs.buffer.kill` +/// composes. Phase 1 (`EditorCore::reconcile_delete`) closes side +/// windows showing a doomed buffer, redirects every other window to a +/// fallback, and removes the id from the registry; phase 2 — +/// buffer-scoped keymaps, buffer-local config, folds, and the +/// registered `on_removed` callbacks — runs here, because it needs +/// `&Lua` and `EditorCore` has no Lua handle. +/// +/// `apply_resource_op`'s delete arm previously ran +/// `remove_buffer_and_fire`, i.e. phase 2 **without** phase 1, leaving +/// any window displaying that buffer pointing at a removed id. Routing +/// both paths through here is what makes that go away as a property of +/// the seam rather than as a separate patch. +fn reconcile_delete_and_fire( + lua: &Lua, + path: &std::path::Path, +) -> crate::editor_core::DeleteReconcile { + let (outcome, normalized) = { + let Some(core) = lua.app_data_ref::() else { + return crate::editor_core::DeleteReconcile::default(); + }; + let mut core = core.borrow_mut(); + let outcome = core.reconcile_delete(path); + ( + outcome, + crate::editor_core::normalize_buffer_path(path.to_path_buf()), + ) + }; + // Phase 2, over exactly the ids phase 1 removed. + for id in &outcome.killed { + after_buffer_removed(lua, *id); + } + let mut args = mlua::MultiValue::new(); + args.push_back(mlua::Value::String( + match lua.create_string(normalized.as_os_str().as_encoded_bytes()) { + Ok(s) => s, + Err(_) => return outcome, + }, + )); + run_hook_if_defined(lua, "resource.deleted", args); + outcome +} + +/// Drive [`crate::async_runtime::TickOutcome::resources`] through +/// reconciliation, one settled mutation at a time (dired Stage 2a, +/// Q#DR29). +/// +/// **Each settled mutation reconciles on its own, and nothing here +/// depends on the relative order of two mutations that were in flight +/// simultaneously** — `resources` is bus-arrival order and the runtime +/// establishes no execution token. That is safe rather than merely +/// honest: independent mutations commute, and the primitive's contract +/// (`builtin/runtime/fs.lua`) requires a caller with overlapping +/// source/target paths to serialize by awaiting each op before +/// dispatching the next. +fn reconcile_settled_resources(lua: &Lua, resources: &[crate::async_runtime::ResourceOp]) { + use crate::async_runtime::ResourceOp; + for op in resources { + match op { + ResourceOp::Rename { from, to } => { + reconcile_rename_and_fire(lua, from, to); + } + ResourceOp::Remove { path } => { + reconcile_delete_and_fire(lua, path); + } + } + } +} + fn remove_buffer_and_fire(lua: &Lua, registry: &SharedRegistry, id: BufferId) -> mlua::Result<()> { registry .borrow_mut() @@ -3220,6 +3340,37 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result*` buffer can follow a renamed + // directory is for dired's own `resource.renamed` subscriber to + // rename it. The alternative — kill and recreate under the new + // name — loses window placement, the cursor, the read-only + // intercept, round-trip input and the major mode, each of which + // would have to be re-established in the right order. + // + // Uniqueness stays the CALLER's job, matching the Rust setter; + // dired reuses its existing `<2>`-variant uniquifier. + // + // This records `BufferNameOrigin::Explicit` (Q#DR30): it is a + // naming operation even when the string happens to denote the + // file, so a later rename must not overwrite it. + let reg = registry.clone(); + buffer.set( + "set_name", + lua.create_function(move |_, (id, name): (BufferIdLua, String)| { + reg.borrow_mut() + .get_mut(id.0) + .map_err(mlua::Error::external)? + .set_name(name); + Ok(()) + })?, + )?; + } + { let reg = registry.clone(); buffer.set( @@ -3437,12 +3588,18 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result() - { - core.borrow_mut().set_buffer_path(id, Some(to.clone())); - } + // dired Stage 2a: the raw, first-match, + // un-normalized `find_by_path` lookup this arm + // used is replaced by the shared transaction. + // Three defects went with it — stored paths are + // normalized on write while the op names its + // target raw, so the lookup could miss the + // buffer entirely; a directory rename has many + // affected buffers by construction and only the + // first moved; and the buffer's *name* stayed + // stale, so the statusline and buffer list kept + // the old filename. + reconcile_rename_and_fire(lua, &from, &to); } "delete" => { // Four ordered phases (Q#RD2): stat/no-op @@ -3499,18 +3656,19 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result { return Err(mlua::Error::external(format!( @@ -7367,9 +7525,15 @@ pub fn install_async( async_mod.set( "_tick", lua.create_function(move |lua, ()| { - let ids = rt.tick(); - let t = lua.create_table_with_capacity(ids.len(), 0)?; - for (i, id) in ids.into_iter().enumerate() { + let outcome = rt.tick(); + // Reconcile BEFORE the settled ids reach Lua. The Lua + // runtime resumes parked coroutines from the table this + // returns, so a coroutine that renamed and then + // inspects a buffer would otherwise see pre-rename + // state. Ordering here is by construction, not by luck. + reconcile_settled_resources(lua, &outcome.resources); + let t = lua.create_table_with_capacity(outcome.settled.len(), 0)?; + for (i, id) in outcome.settled.into_iter().enumerate() { t.set(i + 1, id)?; } Ok(t) @@ -10001,11 +10165,18 @@ pub fn install_lsp( // `builtin/runtime/lsp.lua` calls this per edit so stale // suppression stays keystroke-accurate while the O(file) // full-document notification is coalesced. + // + // **Takes the server id since dired Stage 2a.** It previously + // took the URI alone while creating URI keys in three stores + // for every server at once, which made it the second + // uncorrelated writer able to resurrect a URI `forget_uri` had + // just cleared. The sole production caller already holds + // `rec.server`. let m = manager.clone(); lsp_mod.set( "_mark_document_stale", - lua.create_function(move |_, uri: String| { - m.borrow().mark_document_stale(&uri); + lua.create_function(move |_, (id, uri): (LspServerIdLua, String)| { + m.borrow().mark_document_stale(id.0, &uri); Ok(()) })?, )?; @@ -10529,6 +10700,35 @@ pub fn install_lsp( )?; } + { + // dired Stage 2a §5 — the per-document teardown the + // `resource.renamed` subscriber needs. Modelled on `forget` + // above: a closure over the shared manager that calls through + // and maps the error with `mlua::Error::external`. + // + // Error contract: **raises** for an unknown server id, matching + // `forget`'s behaviour for the same input, and **succeeds + // silently** when the URI has no state under a known server. + // The second arm is the one that matters — the subscriber runs + // per attachment, an attachment need not have any pending route + // or populated result store, and cleanup can be repeated after + // an earlier partial teardown. An over-strict binding would turn + // that ordinary idempotent case into an error inside a hook. + // + // Takes the **old** URI, so calling it after `did_open` of the + // new one is safe and order-independent. + let m = manager.clone(); + lsp_mod.set( + "forget_uri", + lua.create_function(move |_, (id, uri): (LspServerIdLua, String)| { + m.borrow_mut() + .forget_uri(id.0, &uri) + .map_err(mlua::Error::external)?; + Ok(()) + })?, + )?; + } + { let m = manager.clone(); lsp_mod.set(