diff --git a/builtin/hooks/default.lua b/builtin/hooks/default.lua index 4fabfe9..69b04c3 100644 --- a/builtin/hooks/default.lua +++ b/builtin/hooks/default.lua @@ -16,6 +16,12 @@ -- format-on-save subscribe here. -- * editor.before-quit --- short-circuit. A callback may veto quit -- (e.g. "buffer modified --- save first?"). +-- * resource.renamed --- all-must-succeed (dired Stage 2a). Fired +-- after a successful rename, with (old, new) +-- canonical absolute paths. +-- * resource.deleted --- all-must-succeed (dired Stage 2a). Fired +-- after a successful delete, with the +-- canonical absolute path. -- -- These are *defined* here so user config can attach callbacks via -- pmacs.hook.add. Run sites are in Rust (after-load, after-edit) and in @@ -76,6 +82,32 @@ define { kind = "short-circuit", } +define { + name = "resource.renamed", + description = "Fired once per SUCCESSFUL filesystem rename, with the old " .. + "and new paths as canonical absolute strings. The core " .. + "reconciles what it can reach -- buffer paths and names, the " .. + "URI-keyed LSP stores, attached diagnostic overlays -- but a " .. + "package that keys its own state by path or URI is invisible " .. + "to that, so this hook is the mechanism that scales. It " .. + "carries PATHS rather than a rebind list precisely because " .. + "dired's listing buffers are pathless: a path-keyed consumer " .. + "must be able to reconcile from (old, new) alone. Does not " .. + "fire for a rename that failed or was cancelled.", + kind = "all-must-succeed", +} + +define { + name = "resource.deleted", + description = "Fired once per SUCCESSFUL filesystem delete, with the " .. + "canonical absolute path. Buffers on the path and beneath it " .. + "have already been reconciled: unmodified ones killed " .. + "through both removal phases, modified ones kept alive. " .. + "Subscribers drop their own path-keyed state. Does not fire " .. + "for a delete that failed or was cancelled.", + kind = "all-must-succeed", +} + define { name = "editor.before-quit", description = "Fired before the editor exits. Return false to veto.", diff --git a/builtin/runtime/fs.lua b/builtin/runtime/fs.lua index 39e3baa..35b25a2 100644 --- a/builtin/runtime/fs.lua +++ b/builtin/runtime/fs.lua @@ -163,6 +163,30 @@ end -- If a package needs at-most-one-pending semantics for mutations, -- it should serialize on the package side (await each op before -- dispatching the next). The fs primitive can't enforce that. +-- +-- **And that is a CORRECTNESS precondition, not only a cancellation +-- one (dired Stage 2a, Q#DR29).** A successful `rename` or `remove` +-- reconciles the editor's path owners in the main-thread drain — buffer +-- paths and names, the URI-keyed LSP state, the `resource.renamed` / +-- `resource.deleted` hooks. That reconciliation is deliberately +-- order-INDEPENDENT: the runtime drains the reply bus with `try_recv` +-- and establishes no execution token, so a worker can finish first and +-- be descheduled before sending, and reply order therefore does not +-- recover filesystem execution order. +-- +-- Independent mutations commute, so nothing is owed for them. But +-- **mutations whose source/target paths overlap must be serialized by +-- dispatching the next only after the previous handle settles.** There +-- is no static ordering rule that would substitute: rename `dir` -> +-- `newdir` racing delete `dir/child.txt` needs delete-then-rename if +-- the delete ran first on disk and rename-then-delete if the rename +-- did, and a fixed "deletes before renames" rule gets one of the two +-- wrong — the kill misses, the rename then rebinds the buffer onto a +-- path whose file is gone, and it survives pointing at nothing. +-- +-- A caller that ignores this owns the residue: a buffer left bound to a +-- stale path, or killed when it should have been rebound. Recoverable +-- and visible, not data loss — but real. function fs.rename(from, to) if type(from) ~= "string" then diff --git a/builtin/runtime/lsp.lua b/builtin/runtime/lsp.lua index f17827f..d23409b 100644 --- a/builtin/runtime/lsp.lua +++ b/builtin/runtime/lsp.lua @@ -1124,7 +1124,7 @@ pmacs.hook.add("buffer.after-edit", function() -- Stale suppression must stay keystroke-accurate even though the -- O(file) didChange send below is coalesced: render families -- anchored to pre-edit positions are hidden from this edit on. - pcall(pmacs.lsp._mark_document_stale, rec.uri) + pcall(pmacs.lsp._mark_document_stale, rec.server, rec.uri) -- Arc 1d: was this edit a typed character? The input-origin signal -- (see the trigger block below). local typed = pmacs.editor.this_command @@ -1437,19 +1437,44 @@ local function apply_workspace_edit(ops) end end if #plan == 0 then return 0, 0, 0 end - local origin = active_buffer_path() + -- G1 — capture the origin BUFFER, not its path. A path captured here + -- is a plain Lua local, and no amount of reconciliation can reach an + -- already-captured local: once the batch renames or deletes the active + -- file, that string names something that is no longer there. The + -- handle follows a rename for free, because the buffer is what moved. + -- + -- The framing's G1 described the failure as a "phantom buffer" created + -- by `resolve_target_buffer`'s NotFound arm. **That is not what + -- happens on this path, and the wrong explanation is recorded here + -- rather than left to be rediscovered.** `pmacs.buffer.find_or_open` + -- calls `crate::file_io::load_file` directly and maps the error, so a + -- missing path RAISES; the NotFound arm belongs to + -- `EditorCore::resolve_target_buffer`, which serves + -- `pmacs.window.display_file` and the startup/daemon target, not this + -- binding. The real defect is quieter: `restore_origin` runs under a + -- `pcall`, so the raise is swallowed and the user is left in whatever + -- buffer the last applied op made active. And when the old path DOES + -- still resolve -- a batch that deletes and then recreates it -- the + -- fallback silently opens a file the user asked to delete. + local origin_buf = pmacs.window.buffer() local edit_total, files, res_ops = 0, 0, 0 -- Plan items fully applied before a failure. Q#RD3 permits partial -- application, so this is what stops a caller claiming "nothing was -- mutated" when something was. local applied_ops = 0 - -- Return the user to where they invoked from — best-effort, since - -- that path may have just been renamed or deleted. Runs on the - -- FAILURE path too (Q#RD7): previously this ran only after a - -- successful loop, so a mid-batch refusal stranded the user in - -- whatever buffer the last applied op left active. + -- Return the user to where they invoked from. Runs on the FAILURE + -- path too (Q#RD7): previously this ran only after a successful loop, + -- so a mid-batch refusal stranded the user in whatever buffer the last + -- applied op left active. + -- + -- **No path fallback (G1).** If the origin buffer is gone — the batch + -- deleted its file and reconciliation killed it — restore NOTHING. + -- "Return the user somewhere plausible" is not worth re-opening a path + -- the batch just destroyed, and when that path has been recreated the + -- fallback would drop the user into a file they asked to delete. local function restore_origin() - if origin then pcall(pmacs.buffer.find_or_open, origin) end + if not origin_buf then return end + pcall(pmacs.window.switch_buffer, origin_buf) end for _, item in ipairs(plan) do local ok, err @@ -2882,3 +2907,191 @@ pmacs.command.define { pmacs.keymap.bind { scope = "global", sequence = "M-g n", command = "diag.next" } pmacs.keymap.bind { scope = "global", sequence = "M-g p", command = "diag.previous" } + +-- Resource reconciliation --------------------------------------------------- +-- +-- dired Stage 2a, §5. A rename or delete moves or destroys a path that +-- FOURTEEN URI-keyed store families, the `documents` mirror, the pending +-- response routes and the attached diagnostic overlays are all keyed by. +-- `EditorCore` reconciles the buffer's own path and name; these two +-- subscribers reconcile the LSP layer, which is buffer-keyed here +-- (`rec.uri` is cached per buffer and read at dozens of sites, so ONE +-- rebind reaches all of them) and URI-keyed in Rust. +-- +-- These subscribers are independent of every other `resource.renamed` +-- consumer by construction: this one touches URI-keyed state, dired's +-- touches its own handle table, and neither reads what the other wrote. +-- That matters because `all-must-succeed` does NOT abort the fan-out — +-- `run_all_must_succeed` collects each callback's error and continues — +-- so a subscriber may not rely on a raising peer to stop the sequence, +-- and the ordered teardown below is ordered INTERNALLY rather than by +-- registration. + +-- Every attachment whose document is `path` or lies beneath it, as +-- `{ key, rec, path }`. Resolved through `path_for_uri` and compared +-- with `paths_related`, so the comparison is component-aware and runs on +-- the same canonical form the buffer registry keys on. +local function attachments_under(path) + local out = {} + for key, rec in pairs(attachments) do + local rec_path = rec.uri and pmacs.lsp.path_for_uri(rec.uri) + if rec_path and paths_related(rec_path, path) then + out[#out + 1] = { key = key, rec = rec, path = rec_path } + end + end + return out +end + +-- How many attributed failures one status line spells out before +-- collapsing the rest into a count. +local RESOURCE_REPORT_LIMIT = 2 + +-- A failure collector for a reconciliation fan-out. +-- +-- **Why this exists rather than a bare `pcall` per step.** Every step +-- below is fallible for reasons outside this file's control -- a stale +-- server id makes `forget_uri` raise, a stopped server makes `did_close` +-- raise -- and an IGNORED `pcall` makes the hook callback RETURN +-- SUCCESSFULLY. `resource.renamed` and `resource.deleted` are +-- `all-must-succeed`, so the registry's error logger is the mechanism +-- that surfaces a failing subscriber; a callback that swallows its own +-- failures gives that logger nothing to log, and the concrete outcome is +-- silent: `forget_uri` fails, the callback carries on, and the old +-- stores, routes and `documents` entry stay live under a URI the editor +-- no longer holds. +-- +-- It must NOT abort the loop. One unreachable server must not leave +-- every other attachment unreconciled, so failures accumulate and are +-- raised once, after every attachment has been processed. +local function failure_sink(hook_name) + local sink = { hook = hook_name, items = {} } + + -- Run `fn(...)`, and on a raise record it attributed to `what`. + -- Returns `ok, value` like `pcall`, so a caller can branch. + function sink:step(what, fn, ...) + local ok, value = pcall(fn, ...) + if not ok then + self.items[#self.items + 1] = string.format("%s: %s", what, tostring(value)) + end + return ok, value + end + + -- Report everything collected, on BOTH channels, and raise. + -- + -- The raise is what the `all-must-succeed` logger needs in order to + -- write an attributed record to *errors*; the status line is what the + -- user actually sees, because stale LSP state looks like the editor + -- quietly breaking. `pmacs.error` is deliberately not used: it is + -- defined only by a test stub, so writing there would reproduce the + -- silence this replaces. + function sink:finish() + if #self.items == 0 then return end + local shown, n = {}, #self.items + for i = 1, math.min(n, RESOURCE_REPORT_LIMIT) do shown[i] = self.items[i] end + local summary = table.concat(shown, "; ") + if n > #shown then + summary = summary .. string.format("; and %d more", n - #shown) + end + pcall(pmacs.editor.set_status, + string.format("LSP %s: %d reconciliation failure%s -- %s", + self.hook, n, (n == 1 and "" or "s"), summary)) + error(string.format("%s: %s", self.hook, table.concat(self.items, "; ")), 0) + end + + return sink +end + +pmacs.hook.add("resource.renamed", function(old_path, new_path) + if type(old_path) ~= "string" or type(new_path) ~= "string" then return end + local sink = failure_sink("resource.renamed") + for _, hit in ipairs(attachments_under(old_path)) do + local key, rec, old_uri = hit.key, hit.rec, hit.rec.uri + -- The buffer's own path was rebound before this hook fired, so ask + -- it rather than reconstructing the tail ourselves. A buffer that + -- somehow lost its path (killed, unbound) cannot be re-opened, and + -- falls through to the teardown-only path below. Not routed through + -- the sink: a pathless buffer is a legitimate state here, not a + -- reconciliation failure. + local ok_path, new_buf_path = pcall(function() return rec.buffer:path() end) + local new_uri = (ok_path and new_buf_path) and file_uri_for(new_buf_path) or nil + + -- 1. Flush any pending didChange for the OLD uri, so the server is + -- not left holding an edit it can no longer attribute. + sink:step("flush didChange for " .. old_uri, flush_did_change_for, rec) + pending_did_change[key] = nil + + -- 2. didClose the old uri — this removes the open-document + -- registration and nothing else. + sink:step("didClose " .. old_uri, pmacs.lsp.did_close, rec.server, old_uri) + + -- 3. Purge the routes, drain their awaiters, and clear all fourteen + -- stores plus `documents` for the old key. Runs against the OLD + -- server, which matters when step 4 picks a different one. + -- A failure here is the one that most needs reporting: the + -- callback would otherwise continue with the old stores, routes + -- and `documents` entry all still live. + sink:step("forget_uri " .. old_uri, pmacs.lsp.forget_uri, rec.server, old_uri) + + if not new_uri then + attachments[key] = nil + styled_buffers[key] = nil + diag_viewed_buffers[key] = nil + else + -- 4. Re-run ensure_server. Server affinity keys on the detected + -- project root, so a rename ACROSS roots needs a different + -- server; a same-root rename reuses the existing one. + local ok_sid, sid = sink:step("ensure_server for " .. new_buf_path, + ensure_server, rec.language, new_buf_path) + if not (ok_sid and sid) then + attachments[key] = nil + styled_buffers[key] = nil + diag_viewed_buffers[key] = nil + else + -- 5. didOpen the new uri with the buffer's current text and a + -- fresh version. This also reclaims the tombstone for + -- exactly (server, new uri). + rec.server = sid + rec.uri = new_uri + rec.version = 1 + local ok_text, text = sink:step("read " .. new_uri, buffer_text, rec.buffer) + sink:step("didOpen " .. new_uri, pmacs.lsp.did_open, + sid, new_uri, rec.version, ok_text and text or "") + -- 6. Re-root the diagnostic overlays. `DiagnosticView.uri` is + -- set once at construction and is private, so this is the + -- only way to move it — and the sweep reaches PASSIVE + -- windows, which the attach path cannot, while preserving + -- each overlay's position in the composition order. + sink:step("re-root diagnostics to " .. new_uri, + pmacs.diag._rename_resource, old_uri, new_uri) + end + end + end + -- Raised only after EVERY attachment has been processed: one + -- unreachable server must not leave the rest unreconciled. + sink:finish() +end) + +pmacs.hook.add("resource.deleted", function(path) + if type(path) ~= "string" then return end + local sink = failure_sink("resource.deleted") + for _, hit in ipairs(attachments_under(path)) do + local key, rec = hit.key, hit.rec + -- No flush: the document is gone, and shipping a didChange for a + -- file the server can no longer read buys nothing. + pending_did_change[key] = nil + sink:step("didClose " .. rec.uri, pmacs.lsp.did_close, rec.server, rec.uri) + sink:step("forget_uri " .. rec.uri, pmacs.lsp.forget_uri, rec.server, rec.uri) + -- Drop the record unconditionally, INCLUDING after a failure above. + -- The buffer may be gone entirely (an unmodified visited file is + -- killed), in which case a retained record is a dangling handle that + -- `repull_for_attachments` would iterate; and a modified buffer kept + -- alive has no file to analyze until it is saved, which re-attaches + -- through the ordinary path. Keeping a record whose teardown failed + -- would be strictly worse than dropping it: the failure is reported + -- either way, and a retained one is re-swept every refresh. + attachments[key] = nil + styled_buffers[key] = nil + diag_viewed_buffers[key] = nil + end + sink:finish() +end) diff --git a/docs/active-work.md b/docs/active-work.md index bb94cc2..87592b4 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -655,6 +655,177 @@ has **no branch and no framing yet**. `git fetch githubsucks && git worktree add ../pmacs-rd-impl -b resource-op-delete-guard-impl githubsucks/resource-op-delete-guard-impl`. +## dired Stage 2a — rename/delete reconciliation — PR #196 OPEN, review round 1 closed + +- Portable branch: `githubsucks/dired-stage2-impl`, worktree + `../pmacs-dired-s2`. Implements **Stage 2a only** of the framing merged + as #171 (`docs/dired-stage2-framing.md` rev 9, §5/§6/§10 — the + substrate transaction, no dired surface). Position against `main`, as + pasted command output rather than a remembered constant: + + ``` + $ git merge-base HEAD githubsucks/main + e003b81cdd577140fc77330bd4578d3090696877 + ``` + + That base is the #190 merge, and #190 matters here specifically: + Stage 2a **adopts** its `delete_verdict` refusal rather than + reinventing one, and lifts its walk query out into + `editor_core::buffers_bound_under` so the guard and both + reconciliation seams cannot disagree about which buffers an operation + touches. **Re-measure the merge-base before relying on it** — + `main` has branch protection, all 12 checks must pass on the merging + head, and a conflicting PR builds no merge ref at all, so a green run + from before a move reads as current when it is not. **Re-measured after + round 1: `main` had not moved, so no integration was needed** — that is + a reading of the tree, not a standing fact. +- **What 2b and 2c still owe, stated so the split boundary is auditable.** + 2a ships **no user-visible surface at all** and no dired code: the + `dired_acceptance` count is deliberately unchanged at **25**, and a + moved count there would mean it touched something it should not have. + 2b owes the mark and operation layer (`m u U t d x D R w M`), + `pmacs.minibuffer.confirm` plus its `src/editor.rs` load-sequence line, + `pmacs.killring.push`, dired's own `resource.renamed` subscriber, and + acceptance 1–22, 33, 39–41. 2c owes `mkdir`/`copy`/`remove_dir_all`, + `JobKind` 12 → 15, `dired.recursive-deletes`, and acceptance 42–47. +- **The split boundary has not moved since rev 9.** It was re-checked + against this tree: #188 (generated-buffer immutability Stage 1) did not + convert dired's `paint`, so §3.1's coordination note is still an + obligation of that lane rather than a collision with this one, and + nothing in this diff touches `builtin/runtime/dired.lua`. +- **Two m4 rows were re-pinned, and that is a behaviour change to a + landed lane's assertions.** `rd9` and `rd14` pinned #190's deliberate + restraint on the `apply_resource_op` delete arm — descendants stay + orphaned, only the first of two duplicate path-bound buffers is + reconciled — and both doc comments gave the same reason: widening + would have routed N buffers through `remove_buffer_and_fire`, phase 2 + without phase 1, leaving up to N windows on removed ids. + `EditorCore::reconcile_delete` composes both phases, so the constraint + is discharged and the old assertions became the defect. Each row now + asserts BOTH directions — reconciled away **and** no window holding a + removed id — and each direction is bite-verified. +- **One framing claim is wrong and is corrected at the test, not + silently worked around.** §5's G1 says a stale captured path + "materializes a phantom" by reaching `resolve_target_buffer`'s + `NotFound` arm. It does not: `pmacs.buffer.find_or_open` calls + `crate::file_io::load_file` directly and maps the error, so a missing + path **raises**, and the `NotFound` arm belongs to + `resolve_target_buffer`, which serves `pmacs.window.display_file` and + the startup/daemon target rather than that binding. The defect is real + and smaller: the `pcall` swallows the raise, so the user is stranded + wherever the last applied op left them. Acceptance 34 is restructured + to bite on that (its plan edits another file first, which is what makes + the restore observable at all) and the correction is recorded in the + test's own doc comment. +- **Two bites were vacuous as the framing specified them, and both + reasons are worth keeping.** Item 28's *rename* row cannot pin the + walk's containment rule: `reconcile_rename` calls + `Path::strip_prefix` to rebuild a descendant's tail, and that is + component-aware too, so a string-prefix walk is silently corrected a + second time. The row moved to the **delete** side, where the walk's + verdict IS the kill list. Item 30's composition-order assertion was a + tautology: the LSP attach leaves `diagnostic` **last** in the stack, and + moving the last element to the end is a no-op, so a remove-and-re-push + was indistinguishable from an in-place mutation; the row now pushes one + more overlay after it and asserts that precondition explicitly. +- **23 acceptance criteria are bite-verified by executed mutation**, each + labelled `OK (assertion)` — none merely `OK (COMPILE)`, and none + vacuous. Items 25, 27, 28, 29 (both directions), 30 (both mutations), + 31, 31b (both gates), 31d (both halves), 34, 50 (both mutations), 51, + 52, 53b, 54, 55, plus the two re-pinned m4 rows in three + configurations. +- **Review round 1 found four defects; all four are fixed, and all four + were the same shape — a failure that left state wrong and told nobody.** + Worth keeping as one lesson rather than four bugs: every one of them + was a `pcall` or a discarded return value, and each *looked* like + defensive coding. + - **P1 — delete refusals were silent.** `reconcile_delete_and_fire` + returned `kept_modified` and `refused` and both production callers + discarded them, so a last-buffer refusal or the asynchronous + modified-buffer race left the file gone and the buffer still bound to + it — and the next `C-x C-s` recreates the deleted file. Reporting + moved **inside the shared seam**, for the same reason the + reconciliation lives there: a caller that has to remember to report + is a caller that will forget. Channel is `EditorCore::status`; + **not `pmacs.error`**, which is defined only by a test stub, so a + report there would have been the same silence. + - **P2 — the LSP subscribers swallowed their own reconciliation + failures.** Ignored `pcall`s made the callback return successfully, + so the `all-must-succeed` logger had nothing to log. A shared + failure sink now attributes each step and raises **after** the loop, + because a fix that aborts on the first failure would leave every + other attachment unreconciled — that wrong fix is itself a + bite-verified mutation. + - **P2 — `forget_uri` left purged requests live in the client.** It + dropped `pending_routes` and `pending_external` but not the ids + `send_request` puts in `LspClient.pending`, and recorded nothing in + `cancelled_rids`, so a server that never replies leaked the entry and + a late reply surfaced as a generic unrouted response. The per-rid + work is now extracted from `drain_cancelled_externals` as + `abandon_request` and **reused** rather than copied. + - **P2 — acceptance 35 was unpinned even after the G1 correction.** + With a plain delete the forbidden path fallback is unobservable: + `find_or_open` raises out of `load_file` and the `pcall` swallows it, + so both assertions passed with the fallback present. The plan now + deletes the origin's file **and recreates it**, which gives the + fallback something to open. The corrected G1 explanation also reached + the production comments, which still repeated the false + `resolve_target_buffer::NotFound` story — *a correction that stops at + the test comment has only half landed.* +- **One round-1 pin passed with its own bug restored, and the reason is + reusable.** Acceptance 53 asserted `contains("only.txt")` for the + buffer-name attribution — but the status line opens with + `deleted only.txt:`, the deleted path's **basename**, so stripping the + attribution changed nothing the assertion could see. Both halves now + assert the buffer's *own* name, which for a path-backed buffer is the + full path and which only the attribution can produce. **A pin written + to close a review finding is exactly the kind that passes with the bug + restored**, and the detector was running the bite rather than reading + the assertion. +- **31 bites now, all executed, every one labelled `OK (assertion)`** — + the original 23 plus 8 for round 1 (report call removed; refusal reason + unattributed; kept-modified name dropped; subscriber failures + swallowed; the wrong fix that aborts the loop; `forget_uri` skipping + `abandon_request`; and the forbidden path fallback restored, which must + fail acceptance 34 **and** 35 independently). +- Verification at this head, each gate run to its own file and its own + exit code checked (never through a pipe): `cargo fmt --check` clean; + `cargo clippy --workspace --all-targets -- -D warnings` clean; + `cargo test --lib` **1,876** passed / 3 ignored; `--lib --features + crdt` **2,061** / 4 ignored; the new + `resource_reconciliation_acceptance` **25** default and **25** crdt; + `dired_acceptance` **25** and **25** crdt, deliberately unmoved; the + frozen additivity gate `m8_1` **10** / `m8_2` **15** / `m8_3` **32**, + all unchanged; `m4_acceptance -- --skip basedpyright` **149** passed / + 3 ignored / 1 filtered; `lsp_multi_root_acceptance` **13**; + `lsp_dispatch_seams_acceptance` **15**; + `typed_edit_chain_acceptance` **13**; `journey_acceptance` **24** + (the ratchet floor, asserted as a count rather than a colour); + `gpu_invocation_acceptance` **15** crdt — **and that number is only + real with `pmacs` and `pmacs-gpu` built first**, which is the `a37` + trap in §5: the same command reported 12 failures before the build and + 15 passes after, so a red run there is not evidence of a regression + until the binaries exist; `PMACS_REQUIRE_GPU=1 cargo test -p + pmacs-gpu` **202**; isolated-`XDG_CONFIG_HOME` workspace sweep with + `--no-fail-fast` **3,559** passed across **104** suites, 19 ignored, 0 + failed; `git diff --check` clean. Every one of those was run as its own + step with its own exit status checked — never `cmd | tail` inside an + `&&` chain, which returns *tail's* status and has masked a real failure + in this repo before. +- **Ownership, per the framing's own warning.** §16 says 2a must not run + concurrently with **Journey Stage 1b**, because 1b's LSP + spawn-failure reporting lands in `builtin/runtime/lsp.lua`'s + attachment lifecycle and 1b's compile/binding half touches + `src/editor_core.rs` — the same two files 2a rewrites, where the + conflicts are semantic rather than textual so a clean `git merge` + proves nothing. **1b must not be started while this PR is open.** No + other lane in flight touches them: #188 is `dired.lua`/`buffer.rs` + generated-buffer writes, and the bottom-panel and CI lanes are + elsewhere. +- Recovery from a clean checkout: + `git fetch githubsucks && git worktree add ../pmacs-dired-s2 + -b dired-stage2-impl githubsucks/dired-stage2-impl`. + ## Generated-buffer immutability framing lane — PR #188 OPEN, PROPOSED - Portable branch: `githubsucks/generated-buffer-immutability`; worktree diff --git a/src/async_runtime.rs b/src/async_runtime.rs index 493d993..3620a32 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*` @@ -1749,6 +1860,126 @@ mod tests { } } + /// dired Stage 2a, acceptance 54 (controlled-bus layer). Allocate + /// two resource jobs **without dispatching workers**, inject their + /// successful replies in a chosen order, and assert + /// `TickOutcome.resources` reports exactly that order. + /// + /// This is the honest statement of what the runtime guarantees: + /// `tick` drains the reply bus with `try_recv` and establishes no + /// execution token, so what a consumer sees is bus-arrival order. + /// The test fails against sorting by job id or kind, and against any + /// claim that the order recovers dispatch or filesystem-execution + /// order — because the injection order here is *deliberately* the + /// reverse of the allocation order in the first case. + #[test] + fn tick_reports_resources_in_bus_arrival_order_not_allocation_order() { + fn run(reverse: bool) -> Vec { + let rt = AsyncRuntime::with_pool_size(1); + let (a, _) = rt.allocate_with_resource( + JobKind::FsRename, + None, + None, + Some(ResourceOp::Rename { + from: PathBuf::from("/tmp/a-from"), + to: PathBuf::from("/tmp/a-to"), + }), + ); + let (b, _) = rt.allocate_with_resource( + JobKind::FsRemove, + None, + None, + Some(ResourceOp::Remove { + path: PathBuf::from("/tmp/b-gone"), + }), + ); + let order = if reverse { [b, a] } else { [a, b] }; + for id in order { + rt.workers + .send( + ASYNC_REPLY_TOPIC, + &WorkerReply { + job_id: id, + kind: ReplyKind::FsUnit, + }, + ) + .expect("inject reply"); + } + let outcome = rt.tick(); + assert_eq!(outcome.settled.len(), 2, "both jobs settled"); + outcome.resources + } + + let a_first = ResourceOp::Rename { + from: PathBuf::from("/tmp/a-from"), + to: PathBuf::from("/tmp/a-to"), + }; + let b_first = ResourceOp::Remove { + path: PathBuf::from("/tmp/b-gone"), + }; + + assert_eq!( + run(true), + vec![b_first.clone(), a_first.clone()], + "B injected first must be reported first, even though A was \ + allocated first" + ); + assert_eq!( + run(false), + vec![a_first, b_first], + "and the reverse arrival order reverses the report" + ); + } + + /// A failed or cancelled mutation reconciles nothing, so it must not + /// appear in `resources` at all (acceptance 37's runtime half). + #[test] + fn a_failed_or_cancelled_resource_job_is_not_harvested() { + let rt = AsyncRuntime::with_pool_size(1); + let (failed, _) = rt.allocate_with_resource( + JobKind::FsRename, + None, + None, + Some(ResourceOp::Rename { + from: PathBuf::from("/tmp/nope"), + to: PathBuf::from("/tmp/also-nope"), + }), + ); + let (cancelled, _) = rt.allocate_with_resource( + JobKind::FsRemove, + None, + None, + Some(ResourceOp::Remove { + path: PathBuf::from("/tmp/never"), + }), + ); + rt.workers + .send( + ASYNC_REPLY_TOPIC, + &WorkerReply { + job_id: failed, + kind: ReplyKind::Error("ENOENT".to_owned()), + }, + ) + .expect("inject"); + rt.workers + .send( + ASYNC_REPLY_TOPIC, + &WorkerReply { + job_id: cancelled, + kind: ReplyKind::Cancelled, + }, + ) + .expect("inject"); + let outcome = rt.tick(); + assert_eq!(outcome.settled.len(), 2, "both settled"); + assert!( + outcome.resources.is_empty(), + "only Complete mutations are harvested; got {:?}", + outcome.resources + ); + } + #[test] fn dispatch_sum_completes_with_correct_value() { let rt = AsyncRuntime::with_pool_size(2); diff --git a/src/buffer.rs b/src/buffer.rs index a9c01e9..3ccb5e9 100644 --- a/src/buffer.rs +++ b/src/buffer.rs @@ -148,6 +148,26 @@ struct EditDescription { inserted_len: u64, } +/// Provenance of a [`Buffer`]'s name (dired Stage 2a, Q#DR30). +/// +/// A rename must move a name that merely *renders* the file's path and +/// must leave a name the user chose alone. String inspection cannot +/// tell those apart — a user may legitimately name a buffer with a +/// string that normalizes to its own path — so the fact is recorded at +/// the moment the name is written instead of being reconstructed +/// later. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum BufferNameOrigin { + /// A caller named this buffer: `Buffer::new`/`from_bytes`, an + /// ordinary [`Buffer::set_name`], or the `pmacs.buffer.set_name` + /// binding. A rename leaves the name alone. + Explicit, + /// The name was derived from the buffer's backing path by a + /// path-backed creation site, through + /// [`Buffer::set_path_derived_name`]. A rename rewrites it. + PathDerived, +} + /// The unit of editable content: rope + identity + views + undo. /// /// # Threading @@ -159,6 +179,14 @@ pub struct Buffer { id: BufferId, rope: Rope, name: String, + /// Where [`Self::name`] came from. Recorded rather than inferred, + /// because a path-backed buffer's name is **not** reliably its + /// path: `get_or_load_buffer` takes the name from the path *as + /// given* and normalizes only the stored `file_path`, so a + /// relative open is named `foo.rs` while its path is absolute. + /// Rename reconciliation asks this bit, never the string + /// (dired Stage 2a, Q#DR30). + name_origin: BufferNameOrigin, /// The buffer's single active major mode, if one has been selected. major_mode: Option, is_modified: bool, @@ -247,6 +275,10 @@ impl Buffer { id, rope, name: name.into(), + // Construction names a buffer explicitly. A path-backed + // creation site re-records provenance through + // `set_path_derived_name` right after binding the path. + name_origin: BufferNameOrigin::Explicit, major_mode: None, is_modified: false, read_only: false, @@ -449,9 +481,35 @@ impl Buffer { &self.name } - /// Set the buffer's name. Used by save-as and rename operations. + /// Set the buffer's name, recording it as **explicitly chosen** + /// ([`BufferNameOrigin::Explicit`]). + /// + /// This is the user-facing door — `pmacs.buffer.set_name` and + /// save-as go through it — and it is deliberately explicit even + /// when the string happens to denote the file: naming a buffer + /// `notes` for `${cwd}/notes` is still a naming operation, and a + /// later rename must not overwrite it. Path-backed creation sites + /// use [`Self::set_path_derived_name`] instead. pub fn set_name(&mut self, name: impl Into) { self.name = name.into(); + self.name_origin = BufferNameOrigin::Explicit; + } + + /// Set the buffer's name **and** record that it was derived from + /// the buffer's backing path ([`BufferNameOrigin::PathDerived`]). + /// + /// Every site that creates or re-binds a path-backed buffer uses + /// this door, including rename reconciliation itself — so a second + /// rename still follows the path. + pub fn set_path_derived_name(&mut self, name: impl Into) { + self.name = name.into(); + self.name_origin = BufferNameOrigin::PathDerived; + } + + /// Where this buffer's name came from (dired Stage 2a, Q#DR30). + #[must_use] + pub fn name_origin(&self) -> BufferNameOrigin { + self.name_origin } /// This buffer's active major mode, if any. diff --git a/src/diag.rs b/src/diag.rs index 88aa6cc..b81a4dc 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 @@ -489,6 +505,17 @@ impl DiagnosticView { } impl View for DiagnosticView { + /// Re-root this view when the buffer's file was renamed (dired + /// Stage 2a, §5). The URI field is private and `View` has no + /// downcast, so this hook is the only way an outside sweep can + /// reach it — and mutating in place preserves this overlay's + /// position in the window's composition order. + fn rename_resource(&mut self, old_uri: &str, new_uri: &str) { + if self.uri == old_uri { + new_uri.clone_into(&mut self.uri); + } + } + fn kind(&self) -> &'static str { "diagnostic" } @@ -745,6 +772,49 @@ mod tests { } } + /// dired Stage 2a §5, finding 4. `clear` *creates* an `epochs` + /// entry, because a consumer caching against the epoch has to see + /// that the diagnostics went away; nothing ever removes one. So a + /// `forget_uri` that called `clear` would leave a URI-keyed leak + /// behind in the one map nothing prunes — which is why the forget + /// path is its own store method. + #[test] + fn forget_drops_the_epoch_while_clear_deliberately_bumps_it() { + let mut store = DiagnosticStore::new(); + store.set( + "file:///a.rs", + vec![diag(0, DiagnosticSeverity::Error, "boom")], + ); + store.mark_stale("file:///a.rs"); + assert_eq!(store.epoch_for("file:///a.rs"), 1); + + store.clear("file:///a.rs"); + assert_eq!( + store.epoch_for("file:///a.rs"), + 2, + "clear announces the removal to epoch-keyed caches" + ); + + store.set( + "file:///a.rs", + vec![diag(0, DiagnosticSeverity::Error, "boom")], + ); + store.mark_stale("file:///a.rs"); + store.forget("file:///a.rs"); + assert!(store.for_uri("file:///a.rs").is_empty(), "diagnostics"); + assert!(!store.is_stale("file:///a.rs"), "stale flag"); + assert_eq!( + store.severity_counts_for("file:///a.rs"), + (0, 0, 0, 0), + "severity counts" + ); + assert_eq!( + store.epoch_for("file:///a.rs"), + 0, + "forget leaves no trace at all, epoch included" + ); + } + #[test] fn from_lsp_value_parses_minimal_diagnostic() { let v = json!({ diff --git a/src/editor_core.rs b/src/editor_core.rs index 661b767..9b8b5d3 100644 --- a/src/editor_core.rs +++ b/src/editor_core.rs @@ -938,11 +938,18 @@ impl EditorCore { } let normalized = normalize_buffer_path(path.to_path_buf()); let (bytes, meta) = crate::file_io::load_file(path)?; + // The name is the path **as given** — a relative open is named + // `foo.rs` while `file_path` below is absolute. Recording the + // provenance (Q#DR30) is what lets rename reconciliation move + // this name without having to guess from the string. let display_name = path.display().to_string(); let id = self .registry .borrow_mut() - .create_from_bytes(display_name, &bytes); + .create_from_bytes(display_name.clone(), &bytes); + if let Ok(b) = self.registry.borrow_mut().get_mut(id) { + b.set_path_derived_name(display_name); + } self.set_buffer_path(id, Some(normalized)); self.set_buffer_meta(id, Some(meta)); Ok((id, true)) @@ -1001,7 +1008,12 @@ impl EditorCore { }), Err(error) if error.kind() == std::io::ErrorKind::NotFound => { let display_path = path.display().to_string(); - let buffer_id = self.registry.borrow_mut().create(display_path); + let buffer_id = self.registry.borrow_mut().create(display_path.clone()); + // Path-backed creation site (Q#DR30): the name is the + // path, so a later rename may move it. + if let Ok(b) = self.registry.borrow_mut().get_mut(buffer_id) { + b.set_path_derived_name(display_path); + } self.set_buffer_path(buffer_id, Some(path.to_path_buf())); "[new file]".clone_into(&mut self.status); Ok(ResolvedTarget::Buffer { @@ -4828,6 +4840,205 @@ 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), + // Named, because the reason alone is not actionable: + // `kill_buffer`'s "cannot kill the last remaining + // buffer" says nothing about *which* buffer is now + // bound to a path whose file is gone, and that buffer's + // name is what the user needs in order to save it + // somewhere else. + Err(message) => { + let name = self + .registry + .borrow() + .get(id) + .map_or_else(|_| format!("{id:?}"), |b| b.name().to_owned()); + out.refused + .push((id, format!("buffer {name:?}: {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( @@ -5085,6 +5296,85 @@ 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..f5630b6 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)?; @@ -1640,15 +1699,35 @@ impl LspManager { ); } for rid in abandoned_rids { - self.pending_routes.remove(&(sid, rid)); - if let Some(client) = self.clients.get_mut(&sid) { - client.pending.remove(&rid); - client.cancelled_rids.insert(rid); - } - self.send_cancel_request(sid, rid); + self.abandon_request(sid, rid); } } + /// Abandon one in-flight request: drop its response route, drop the + /// client's `pending` entry, record the rid so a late reply is + /// dropped silently rather than surfacing as an unmatched response, + /// and ask the server to stop working on it. + /// + /// Extracted from [`Self::drain_cancelled_externals`] by dired Stage + /// 2a so [`Self::forget_uri`] reuses it instead of being a second, + /// incomplete copy. **All four steps are load-bearing together.** + /// Removing only the route and the awaiter — which is what + /// `forget_uri` originally did — leaves `client.pending` holding the + /// rid forever when the server never replies, and leaves + /// `cancelled_rids` without it, so a late reply arrives as a generic + /// unrouted response instead of being discarded. On a cross-root + /// rename that is worse than a leak: the old server keeps the entry + /// and no attachment drains it afterwards, so the entries + /// accumulate. + fn abandon_request(&mut self, sid: LspServerId, rid: u64) { + self.pending_routes.remove(&(sid, rid)); + if let Some(client) = self.clients.get_mut(&sid) { + client.pending.remove(&rid); + client.cancelled_rids.insert(rid); + } + self.send_cancel_request(sid, rid); + } + /// Send `$/cancelRequest { id }` to `sid`, best-effort. A server /// that is not accepting writes (stopped / crashed) is skipped by /// [`Self::send_notification`]'s state guard; the `Err` is @@ -2901,6 +2980,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 +3122,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 +3132,233 @@ 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. **Abandon every in-flight request scoped to this URI**, through + /// [`Self::abandon_request`] — the same path the per-tick + /// cancellation sweep uses, so the route, the client's `pending` + /// entry, the `cancelled_rids` record and `$/cancelRequest` all + /// happen together rather than only the first of the four. + /// `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. + #[allow( + clippy::too_many_lines, + reason = "the fourteen store families are a flat inventory; splitting it is how one of them gets forgotten, which is the defect this method exists to prevent" + )] + 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())); + + // Steps 2 and 3 — collect the rids this URI owns, settle their + // awaiters cancelled, then abandon each request through the + // SAME path the per-tick sweep uses + // ([`Self::abandon_request`]): route, `client.pending`, + // `cancelled_rids`, `$/cancelRequest`. Purging the route alone + // would leave the request live in the client and a late reply + // unrecognised. + 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 { + if let Some(p) = self.pending_external.remove(&(sid, *rid)) { + for a in &p.awaiters { + self.runtime.complete_external_cancelled(a.job_id); + } + } + self.abandon_request(sid, *rid); + } + + // 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 +3374,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 +3408,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 +3432,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") @@ -3764,3 +4104,721 @@ mod tests { assert_eq!(resolve_config_section(&s, Some("")), s); } } + +// --------------------------------------------------------------------------- +// dired Stage 2a — `forget_uri`, and the tombstone that gates the +// uncorrelated resurrection paths (§5, acceptance 31 / 31b / 31c / 31d). +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod resource_reconciliation_tests { + use super::*; + use crate::async_runtime::JobOutcome; + + /// A manager plus two live-enough clients. `/bin/cat` blocks on + /// stdin, so both stay in `Starting` for the whole test and every + /// notification is deferred rather than written — which is exactly + /// what these tests want: they assert on manager-owned state, not on + /// wire traffic. + fn manager_with_two_servers() -> (LspManager, LspServerId, LspServerId) { + use std::cell::RefCell; + use std::rc::Rc; + let sup = Rc::new(RefCell::new(crate::process::ProcessSupervisor::new())); + let runtime = Rc::new(crate::async_runtime::AsyncRuntime::with_pool_size(1)); + let mut mgr = LspManager::new(sup, runtime); + let mut spec_a = LspServerSpec::new("a", "rust", "/bin/cat"); + spec_a.restart = LspRestartPolicy::Never; + let mut spec_b = LspServerSpec::new("b", "rust", "/bin/cat"); + spec_b.restart = LspRestartPolicy::Never; + let a = mgr.spawn(spec_a).expect("spawn a"); + let b = mgr.spawn(spec_b).expect("spawn b"); + (mgr, a, b) + } + + fn publish(mgr: &mut LspManager, sid: LspServerId, uri: &str, message: &str) { + mgr.handle_notification( + sid, + "textDocument/publishDiagnostics".to_owned(), + json!({ + "uri": uri, + "diagnostics": [{ + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 1 }, + }, + "severity": 1, + "message": message, + }], + }), + Instant::now(), + ); + } + + fn diag_messages(mgr: &LspManager, uri: &str) -> Vec { + mgr.diag_store + .lock() + .expect("diag store") + .for_uri(uri) + .iter() + .map(|d| d.message.clone()) + .collect() + } + + /// Populate every one of the fourteen URI-keyed store families plus + /// the `documents` mirror for `(sid, uri)`. + fn populate_all_stores(mgr: &mut LspManager, sid: LspServerId, uri: &str) { + let server = sid.raw().to_string(); + publish(mgr, sid, uri, "a diagnostic"); + mgr.completion_store.lock().unwrap().set( + crate::completion::CompletionKey::new(server.clone(), uri), + crate::completion::CompletionResponse::from_lsp_value(&json!([{ "label": "x" }])), + ); + mgr.hover_store.lock().unwrap().set( + crate::hover::HoverKey::new(server.clone(), uri), + crate::hover::Hover::from_lsp_value(&json!({ "contents": "doc" })) + .expect("a hover payload with contents parses"), + ); + mgr.signature_store.lock().unwrap().set( + crate::signature::SignatureKey::new(server.clone(), uri), + crate::signature::SignatureHelp::from_lsp_value( + &json!({ "signatures": [{ "label": "f()" }] }), + ), + ); + mgr.definition_store.lock().unwrap().set( + crate::definition::DefinitionKey::new(server.clone(), uri), + crate::definition::DefinitionResponse::from_lsp_value(&json!({ + "uri": uri, + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + })), + ); + for kind in [ + crate::locations::LocationKind::References, + crate::locations::LocationKind::Declaration, + crate::locations::LocationKind::TypeDefinition, + crate::locations::LocationKind::Implementation, + ] { + mgr.locations_store.lock().unwrap().set( + crate::locations::LocationsKey::new(server.clone(), uri, kind), + crate::definition::DefinitionResponse::from_lsp_value(&json!({ + "uri": uri, + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + })), + ); + } + mgr.symbol_store.lock().unwrap().set( + crate::symbol::SymbolKey::document(server.clone(), uri), + crate::symbol::SymbolResponse::from_lsp_value( + &json!([{ + "name": "S", "kind": 5, + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + "selectionRange": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + }]), + uri, + ), + ); + mgr.document_highlight_store.lock().unwrap().set( + crate::document_highlight::DocumentHighlightKey::new(server.clone(), uri), + crate::document_highlight::DocumentHighlightResponse::from_lsp_value(&json!([{ + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 1 } }, + }])), + ); + mgr.formatting_store.lock().unwrap().set( + crate::formatting::FormattingKey::new(server.clone(), uri), + crate::formatting::FormattingResponse::from_lsp_value(&json!([{ + "range": { "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 } }, + "newText": "x", + }])), + ); + mgr.rename_store.lock().unwrap().set( + crate::rename::RenameKey::new(server.clone(), uri), + crate::rename::WorkspaceEditResponse::from_lsp_value(&json!({ "changes": {} })), + ); + mgr.prepare_rename_store.lock().unwrap().set( + crate::prepare_rename::PrepareRenameKey::new(server.clone(), uri), + crate::prepare_rename::PrepareRenameResponse::from_lsp_value(&json!({ + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 1 }, + })), + ); + mgr.code_action_store.lock().unwrap().set( + crate::code_action::CodeActionKey::new(server.clone(), uri), + crate::code_action::CodeActionResponse::from_lsp_value(&json!([{ "title": "fix" }])), + ); + mgr.inlay_hint_store.lock().unwrap().set( + crate::inlay_hint::InlayHintKey::new(server.clone(), uri), + crate::inlay_hint::InlayHintResponse::from_lsp_value(&json!([{ + "position": { "line": 0, "character": 0 }, + "label": ": i32", + }])), + ); + mgr.semantic_token_store.lock().unwrap().set( + crate::semantic_tokens::SemanticTokenKey::new(server, uri), + crate::semantic_tokens::SemanticTokensResponse::from_lsp_value(&json!({ + "data": [0, 0, 1, 0, 0], + })), + ); + mgr.documents + .insert((sid, uri.to_owned()), "text".to_owned()); + } + + /// Which of the fourteen families still hold an entry for + /// `(sid, uri)`, by name. An empty vector is the post-forget + /// expectation; naming the survivors is what makes a failure + /// actionable instead of "assert!(false)". + #[allow( + clippy::too_many_lines, + reason = "one probe per store family, mirroring the inventory under test" + )] + fn populated_families(mgr: &LspManager, sid: LspServerId, uri: &str) -> Vec<&'static str> { + let server = sid.raw().to_string(); + let mut out = Vec::new(); + if !diag_messages(mgr, uri).is_empty() { + out.push("diag"); + } + if mgr + .completion_store + .lock() + .unwrap() + .get(&crate::completion::CompletionKey::new(server.clone(), uri)) + .is_some() + { + out.push("completion"); + } + if mgr + .hover_store + .lock() + .unwrap() + .get(&crate::hover::HoverKey::new(server.clone(), uri)) + .is_some() + { + out.push("hover"); + } + if mgr + .signature_store + .lock() + .unwrap() + .get(&crate::signature::SignatureKey::new(server.clone(), uri)) + .is_some() + { + out.push("signature"); + } + if mgr + .definition_store + .lock() + .unwrap() + .get(&crate::definition::DefinitionKey::new(server.clone(), uri)) + .is_some() + { + out.push("definition"); + } + for (kind, label) in [ + (crate::locations::LocationKind::References, "references"), + (crate::locations::LocationKind::Declaration, "declaration"), + ( + crate::locations::LocationKind::TypeDefinition, + "typeDefinition", + ), + ( + crate::locations::LocationKind::Implementation, + "implementation", + ), + ] { + if mgr + .locations_store + .lock() + .unwrap() + .get(&crate::locations::LocationsKey::new( + server.clone(), + uri, + kind, + )) + .is_some() + { + out.push(label); + } + } + if mgr + .symbol_store + .lock() + .unwrap() + .get(&crate::symbol::SymbolKey::document(server.clone(), uri)) + .is_some() + { + out.push("symbol"); + } + if mgr + .document_highlight_store + .lock() + .unwrap() + .get(&crate::document_highlight::DocumentHighlightKey::new( + server.clone(), + uri, + )) + .is_some() + { + out.push("documentHighlight"); + } + if mgr + .formatting_store + .lock() + .unwrap() + .get(&crate::formatting::FormattingKey::new(server.clone(), uri)) + .is_some() + { + out.push("formatting"); + } + if mgr + .rename_store + .lock() + .unwrap() + .get(&crate::rename::RenameKey::new(server.clone(), uri)) + .is_some() + { + out.push("rename"); + } + if mgr + .prepare_rename_store + .lock() + .unwrap() + .get(&crate::prepare_rename::PrepareRenameKey::new( + server.clone(), + uri, + )) + .is_some() + { + out.push("prepareRename"); + } + if mgr + .code_action_store + .lock() + .unwrap() + .get(&crate::code_action::CodeActionKey::new(server.clone(), uri)) + .is_some() + { + out.push("codeAction"); + } + if mgr + .inlay_hint_store + .lock() + .unwrap() + .get(&crate::inlay_hint::InlayHintKey::new(server.clone(), uri)) + .is_some() + { + out.push("inlayHint"); + } + if mgr + .semantic_token_store + .lock() + .unwrap() + .get(&crate::semantic_tokens::SemanticTokenKey::new(server, uri)) + .is_some() + { + out.push("semanticTokens"); + } + out + } + + /// Acceptance 31, store half — every one of the fourteen families + /// plus `documents` loses its entry. The `populated_families` + /// precondition is what makes this bite: an assertion that the + /// stores are empty afterwards passes vacuously if nothing filled + /// them. + #[test] + fn forget_uri_clears_all_fourteen_store_families_and_the_document_mirror() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let uri = "file:///tmp/old.rs"; + populate_all_stores(&mut mgr, a, uri); + let before = populated_families(&mgr, a, uri); + assert_eq!( + before.len(), + 17, + "precondition: every family must hold an entry before the forget \ + (14 families, of which `locations` counts four kinds); got {before:?}" + ); + assert!(mgr.documents.contains_key(&(a, uri.to_owned()))); + + mgr.forget_uri(a, uri).expect("forget a known server"); + + let after = populated_families(&mgr, a, uri); + assert!( + after.is_empty(), + "these families survived the forget: {after:?}" + ); + assert!( + !mgr.documents.contains_key(&(a, uri.to_owned())), + "the `documents` mirror is what didChange diffs against, so a \ + stale entry under the old URI is a correctness problem" + ); + } + + /// Acceptance 31, route half, plus W3's exemption. A response + /// already in flight at rename time must not repopulate the old key + /// after the clear — and a `workspace/symbol` route, which carries a + /// query and no URI at all, must survive. + #[test] + fn forget_uri_purges_routes_for_the_uri_and_retains_workspace_symbol() { + let (mut mgr, a, b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + let other = "file:///tmp/other.rs"; + mgr.pending_routes.insert( + (a, 1), + ResponseRoute::Hover { + uri: old.to_owned(), + }, + ); + mgr.pending_routes.insert( + (a, 2), + ResponseRoute::Locations { + uri: old.to_owned(), + kind: crate::locations::LocationKind::References, + }, + ); + mgr.pending_routes.insert( + (a, 3), + ResponseRoute::WorkspaceSymbol { + query: "Widget".to_owned(), + }, + ); + mgr.pending_routes.insert( + (a, 4), + ResponseRoute::Hover { + uri: other.to_owned(), + }, + ); + // Same URI, different server: another server's in-flight work is + // not ours to cancel. + mgr.pending_routes.insert( + (b, 5), + ResponseRoute::Hover { + uri: old.to_owned(), + }, + ); + + mgr.forget_uri(a, old).expect("forget"); + + assert!(!mgr.pending_routes.contains_key(&(a, 1)), "hover for old"); + assert!( + !mgr.pending_routes.contains_key(&(a, 2)), + "locations for old" + ); + assert!( + mgr.pending_routes.contains_key(&(a, 3)), + "workspace/symbol carries no URI and is not scoped to any \ + document, so a rename does not invalidate it" + ); + assert!( + mgr.pending_routes.contains_key(&(a, 4)), + "an unrelated document's route must survive" + ); + assert!( + mgr.pending_routes.contains_key(&(b, 5)), + "another server's route for the same URI must survive" + ); + } + + /// Acceptance 31, drain half. `pending_external` holds the + /// `Handle:await()` side, and its own contract says it is + /// drained-cancelled wherever `pending_routes` is purged. Neither + /// existing sweep is URI-scoped, and the one with the similar name + /// (`drain_cancelled_externals`) removes only awaiters whose token + /// was flipped or which timed out — **a rename flips no token**, so + /// modelling on it would drain nothing and park the coroutine + /// forever. + #[test] + fn forget_uri_settles_the_awaiters_joined_to_the_purged_routes() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + let other = "file:///tmp/other.rs"; + let runtime = mgr.runtime.clone(); + + let mut register = |rid: u64, uri: &str| { + let (job_id, token) = runtime.register_external(JobKind::LspRequest, None); + mgr.pending_routes.insert( + (a, rid), + ResponseRoute::Hover { + uri: uri.to_owned(), + }, + ); + mgr.pending_external.insert( + (a, rid), + PendingExternal { + method: "textDocument/hover".to_owned(), + awaiters: vec![Awaiter { job_id, token }], + dispatched_at: Instant::now(), + }, + ); + job_id + }; + let doomed = register(1, old); + let survivor = register(2, other); + + // Nothing has settled yet: the drain, not the registration, is + // what must produce the outcome. + let _ = runtime.tick(); + assert!(!runtime.is_complete(doomed)); + assert!(!runtime.is_complete(survivor)); + + mgr.forget_uri(a, old).expect("forget"); + let _ = runtime.tick(); + + assert!( + matches!(runtime.take_result(doomed), Some(JobOutcome::Cancelled)), + "an awaiter parked on a route we just purged must wake cancelled" + ); + assert!( + !mgr.pending_external.contains_key(&(a, 1)), + "and its entry must be gone, not merely settled" + ); + assert!( + runtime.take_result(survivor).is_none(), + "an unrelated document's awaiter keeps waiting" + ); + assert!(mgr.pending_external.contains_key(&(a, 2))); + } + + /// Review round 1 — `forget_uri` must abandon the request in the + /// **client**, not only in the route table. + /// + /// `pending_routes` and `pending_external` are two of four places an + /// in-flight request lives. `LspClient.pending` (written by + /// `send_request`) and `cancelled_rids` are the other two, and + /// dropping only the first two leaves the entry live forever when the + /// server never replies, while a late reply arrives as a generic + /// unrouted response instead of being discarded. On a cross-root + /// rename the old server keeps those entries and no attachment + /// drains it afterwards, so they accumulate. + /// + /// Bite: fails against a `forget_uri` that purges routes and + /// awaiters without going through `abandon_request`. + #[test] + fn forget_uri_abandons_the_request_in_the_client_too() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + let other = "file:///tmp/other.rs"; + + for (rid, uri) in [(11u64, old), (12u64, other)] { + mgr.pending_routes.insert( + (a, rid), + ResponseRoute::Hover { + uri: uri.to_owned(), + }, + ); + let client = mgr.clients.get_mut(&a).expect("client a"); + client.pending.insert(rid, "textDocument/hover".to_owned()); + } + let client = mgr.clients.get(&a).expect("client a"); + assert!(client.pending.contains_key(&11), "precondition"); + assert!(client.pending.contains_key(&12), "precondition"); + assert!( + client.cancelled_rids.is_empty(), + "precondition: nothing abandoned yet" + ); + + mgr.forget_uri(a, old).expect("forget"); + + let client = mgr.clients.get(&a).expect("client a"); + assert!( + !client.pending.contains_key(&11), + "the purged request must leave `client.pending`, or it leaks \ + for the lifetime of a server that never replies" + ); + assert!( + client.cancelled_rids.contains(&11), + "and must be recorded, or a late reply surfaces as a generic \ + unrouted response instead of being dropped" + ); + assert!( + client.pending.contains_key(&12), + "an unrelated document's request must survive" + ); + assert!( + !client.cancelled_rids.contains(&12), + "and must not be marked abandoned" + ); + } + + /// Acceptance 31c — the error contract, both arms. The second is the + /// one that matters: the subscriber runs per attachment, an + /// attachment need not have any pending route or populated result, + /// and repeated cleanup after a partial teardown must stay safe. + #[test] + fn forget_uri_raises_for_an_unknown_server_and_succeeds_with_no_state() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let unknown = LspServerId::next(); + let err = mgr + .forget_uri(unknown, "file:///tmp/x.rs") + .expect_err("unknown server must raise, matching `forget`"); + assert!(err.contains("unknown server"), "{err}"); + + mgr.forget_uri(a, "file:///tmp/never-touched.rs").expect( + "a URI with no state under a known server is an \ + idempotent success, not an error", + ); + mgr.forget_uri(a, "file:///tmp/never-touched.rs") + .expect("and repeating it stays safe"); + } + + /// Acceptance 31b — the uncorrelated write. This notification + /// carries no request id, so the route purge cannot see it, and + /// `diag_store` has no correlated writers at all. The companion + /// assertion is that the tombstone does **not** over-reach: a + /// publish for a different, never-opened URI is still absorbed, + /// which is exactly what a `documents` membership gate would have + /// broken. + #[test] + fn a_late_publish_for_a_forgotten_uri_is_dropped_and_an_unopened_uri_is_not() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + publish(&mut mgr, a, old, "before"); + assert_eq!(diag_messages(&mgr, old), vec!["before".to_owned()]); + + mgr.forget_uri(a, old).expect("forget"); + assert!(diag_messages(&mgr, old).is_empty(), "cleared by the forget"); + + publish(&mut mgr, a, old, "late arrival"); + assert!( + diag_messages(&mgr, old).is_empty(), + "a publish naming a URI we explicitly forgot must be dropped" + ); + + // Servers legitimately publish for files the editor never + // opened — a crate-wide push naming a dependency. + let never_opened = "file:///tmp/dependency.rs"; + publish(&mut mgr, a, never_opened, "third-party"); + assert_eq!( + diag_messages(&mgr, never_opened), + vec!["third-party".to_owned()], + "the tombstone drops only what we forgot, never everything \ + outside `documents`" + ); + } + + /// Acceptance 31b, second gate. `mark_document_stale` creates URI + /// keys in three stores, so without its own check a forgotten URI + /// regains a stale flag in all three. + #[test] + fn mark_document_stale_cannot_flag_a_forgotten_pair_in_any_of_the_three_stores() { + let (mut mgr, a, _b) = manager_with_two_servers(); + let old = "file:///tmp/old.rs"; + mgr.forget_uri(a, old).expect("forget"); + + mgr.mark_document_stale(a, old); + + assert!(!mgr.diag_store.lock().unwrap().is_stale(old), "diagnostics"); + assert!( + !mgr.semantic_token_store.lock().unwrap().is_stale(old), + "semantic tokens" + ); + assert!( + !mgr.inlay_hint_store.lock().unwrap().is_stale(old), + "inlay hints" + ); + + // And it still works for a URI we did not forget, so the gate is + // the tombstone and not a blanket disable. + let live = "file:///tmp/live.rs"; + mgr.mark_document_stale(a, live); + assert!(mgr.diag_store.lock().unwrap().is_stale(live)); + } + + /// Acceptance 31d — identity and reclamation are exact. Tombstone + /// one URI under two servers; `did_open(A, uri)` clears only A's + /// pair, so an A write is admitted while a later B write is dropped. + #[test] + fn the_tombstone_is_keyed_by_the_exact_server_uri_pair() { + let (mut mgr, a, b) = manager_with_two_servers(); + let uri = "file:///tmp/shared.rs"; + mgr.forget_uri(a, uri).expect("forget under a"); + mgr.forget_uri(b, uri).expect("forget under b"); + assert!(mgr.is_forgotten(a, uri)); + assert!(mgr.is_forgotten(b, uri)); + + mgr.did_open(a, uri, 1, "text").expect("reopen under a"); + assert!( + !mgr.is_forgotten(a, uri), + "reopening is the editor saying it holds the URI again" + ); + assert!( + mgr.is_forgotten(b, uri), + "and it says nothing about another server's tombstone" + ); + + publish(&mut mgr, a, uri, "from A"); + assert_eq!( + diag_messages(&mgr, uri), + vec!["from A".to_owned()], + "A's write is admitted after A reopened" + ); + publish(&mut mgr, b, uri, "from B"); + assert_eq!( + diag_messages(&mgr, uri), + vec!["from A".to_owned()], + "B is still tombstoned for this URI, so its later write is \ + dropped and A's payload survives untouched" + ); + } + + /// Acceptance 31d — a restart generation flip drops every pair for + /// its own server and retains every other server's. + #[test] + fn start_generation_reclaims_only_the_flipped_servers_tombstones() { + let (mut mgr, a, b) = manager_with_two_servers(); + mgr.forget_uri(a, "file:///tmp/a1.rs").expect("forget"); + mgr.forget_uri(a, "file:///tmp/a2.rs").expect("forget"); + mgr.forget_uri(b, "file:///tmp/b1.rs").expect("forget"); + assert_eq!(mgr.forgotten_document_count(), 3); + + let mut client = mgr.clients.remove(&b).expect("client b"); + mgr.start_generation(b, &mut client).expect("restart b"); + mgr.clients.insert(b, client); + + assert!(mgr.is_forgotten(a, "file:///tmp/a1.rs")); + assert!(mgr.is_forgotten(a, "file:///tmp/a2.rs")); + assert!( + !mgr.is_forgotten(b, "file:///tmp/b1.rs"), + "B's generation is gone, so B's tombstones go with it" + ); + assert_eq!(mgr.forgotten_document_count(), 2); + } + + /// Acceptance 31d — terminal `forget` likewise, and the set is empty + /// once the only owning generation is torn down. This is what makes + /// the set reclaimed rather than a leak; it deliberately is **not** + /// size-bounded, because a capacity or LRU eviction would let an + /// arbitrarily late notification resurrect an evicted key. + #[test] + fn terminal_forget_reclaims_only_its_own_servers_tombstones() { + let (mut mgr, a, b) = manager_with_two_servers(); + mgr.forget_uri(a, "file:///tmp/a1.rs").expect("forget"); + mgr.forget_uri(b, "file:///tmp/b1.rs").expect("forget"); + assert_eq!(mgr.forgotten_document_count(), 2); + + if let Some(client) = mgr.clients.get_mut(&b) { + client.state = LspClientState::Stopped { + ended: Instant::now(), + }; + } + mgr.forget(b).expect("forget b"); + assert!(mgr.is_forgotten(a, "file:///tmp/a1.rs")); + assert!(!mgr.is_forgotten(b, "file:///tmp/b1.rs")); + assert_eq!(mgr.forgotten_document_count(), 1); + + if let Some(client) = mgr.clients.get_mut(&a) { + client.state = LspClientState::Stopped { + ended: Instant::now(), + }; + } + mgr.forget(a).expect("forget a"); + assert_eq!( + mgr.forgotten_document_count(), + 0, + "the set is empty once the owning generations are gone" + ); + } +} diff --git a/src/lua_bindings/diag.rs b/src/lua_bindings/diag.rs index c462f44..b9e632a 100644 --- a/src/lua_bindings/diag.rs +++ b/src/lua_bindings/diag.rs @@ -232,6 +232,32 @@ 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 d5a2b4b..a8a130b 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,223 @@ 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); + } + if let Ok(path_arg) = lua.create_string(normalized.as_os_str().as_encoded_bytes()) { + let mut args = mlua::MultiValue::new(); + args.push_back(mlua::Value::String(path_arg)); + run_hook_if_defined(lua, "resource.deleted", args); + } + // Reported AFTER the fan-out, deliberately: a subscriber may set its + // own status, and this message must be the last word because it is + // the data-loss-adjacent one. Unconditional, so a path that cannot + // cross into Lua still gets its refusal reported rather than losing + // both the hook and the report. + report_delete_reconcile(lua, &normalized, &outcome); + outcome +} + +/// Cap on how many buffer names one status line spells out before +/// collapsing the rest into a count. A directory delete can reach +/// dozens; a status line that scrolls off is a message nobody reads. +const DELETE_REPORT_NAMED_LIMIT: usize = 3; + +/// Render the buffers a delete could not reconcile, and put it on the +/// status channel. +/// +/// **Silence here is the defect this exists to close.** Both outcomes +/// leave a buffer alive and still bound to a path whose file is gone, so +/// the next `C-x C-s` recreates the file the user just deleted. That is +/// recoverable only if the user knows it happened: +/// +/// * `kept_modified` — a modified buffer, kept on purpose. On the +/// synchronous path #190 refuses before disk so this cannot arise, but +/// `pmacs.fs.remove` dispatches a worker, and a buffer modified in the +/// interval between the caller's check and the syscall reaches here. +/// * `refused` — could not be removed at all: the last remaining buffer +/// (`kill_buffer` refuses to empty the registry), or a buffer that was +/// mid-edit when the reconciliation ran. +/// +/// The channel is `EditorCore::status`, which is what +/// `pmacs.editor.set_status` writes. **Not `pmacs.error`** — that +/// channel is defined only by a test stub, so all fifteen of its guarded +/// call sites are dead, and a report written there would be exactly the +/// silence being fixed. +/// +/// Lives inside the shared seam rather than at its two call sites, for +/// the same reason the reconciliation does: a caller that has to +/// remember to report is a caller that will forget. The first version of +/// this function's callers both discarded the outcome. +fn report_delete_reconcile( + lua: &Lua, + path: &std::path::Path, + outcome: &crate::editor_core::DeleteReconcile, +) { + if outcome.kept_modified.is_empty() && outcome.refused.is_empty() { + return; + } + let name_of = |p: &std::path::Path| { + p.file_name() + .map_or_else(|| p.display().to_string(), |n| n.to_string_lossy().into()) + }; + let mut parts: Vec = Vec::new(); + if !outcome.kept_modified.is_empty() { + let n = outcome.kept_modified.len(); + let named: Vec<&str> = outcome + .kept_modified + .iter() + .take(DELETE_REPORT_NAMED_LIMIT) + .map(|(_, name)| name.as_str()) + .collect(); + parts.push(format!( + "{n} buffer{} with unsaved changes kept ({}{}) — saving {} will RECREATE the deleted file", + if n == 1 { "" } else { "s" }, + named.join(", "), + if n > named.len() { + format!(", and {} more", n - named.len()) + } else { + String::new() + }, + if n == 1 { "it" } else { "them" }, + )); + } + if !outcome.refused.is_empty() { + let n = outcome.refused.len(); + let named: Vec = outcome + .refused + .iter() + .take(DELETE_REPORT_NAMED_LIMIT) + .map(|(_, why)| why.clone()) + .collect(); + parts.push(format!( + "{n} buffer{} could not be closed ({}{})", + if n == 1 { "" } else { "s" }, + named.join("; "), + if n > named.len() { + format!("; and {} more", n - named.len()) + } else { + String::new() + }, + )); + } + let message = format!("deleted {}: {}", name_of(path), parts.join("; ")); + if let Some(core) = lua.app_data_ref::() { + core.borrow_mut().status = message; + } +} + +/// 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 +3432,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( @@ -3244,6 +3487,11 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result() { let mut core = core.borrow_mut(); core.switch_active_buffer(id) @@ -3307,6 +3555,10 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result() { let mut core = core.borrow_mut(); core.switch_active_buffer(id) @@ -3428,12 +3680,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 @@ -3490,18 +3748,19 @@ fn install_buffer_module(lua: &Lua, registry: &SharedRegistry) -> mlua::Result { return Err(mlua::Error::external(format!( @@ -7358,9 +7617,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) @@ -9992,11 +10257,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(()) })?, )?; @@ -10520,6 +10792,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( diff --git a/src/view.rs b/src/view.rs index 424fd9e..e3e4b6c 100644 --- a/src/view.rs +++ b/src/view.rs @@ -310,6 +310,20 @@ pub trait View { fn clone_for_split(&self) -> Option> { None } + + /// Retarget this overlay from `old_uri` to `new_uri` after a + /// resource rename (dired Stage 2a, §5). Default: no-op — a view + /// that renders nothing URI-keyed is unaffected. + /// + /// Mutates **in place**, so the overlay keeps its position in the + /// window's composition order. That is the reason this is a trait + /// hook rather than a remove-and-re-push at the call site: overlays + /// are an ordered `Vec` merged in sequence, and re-pushing would + /// move a diagnostic underline to the end of the stack. It is also + /// how *passive* windows are reached at all — the Lua attach path + /// (`pmacs.diag._attach_view`) can only touch the active window, + /// while the sweep that drives this walks every window. + fn rename_resource(&mut self, _old_uri: &str, _new_uri: &str) {} } // --------------------------------------------------------------------------- diff --git a/tests/m4_acceptance.rs b/tests/m4_acceptance.rs index 220ae99..64323e0 100644 --- a/tests/m4_acceptance.rs +++ b/tests/m4_acceptance.rs @@ -8549,17 +8549,26 @@ fn rd8_recursive_delete_refuses_for_a_modified_descendant() { assert!(tree.exists(), "including the directory itself"); } -/// Criterion 9 — a *clean* recursive delete leaves descendant buffers -/// orphaned, not removed. +/// Criterion 9 — a *clean* recursive delete reconciles descendant +/// buffers, through both removal phases. /// -/// This pin deliberately asserts today's imperfect behaviour. Widening -/// reconciliation to the tree would route N buffers through -/// `remove_buffer_and_fire` — phase 2 without phase 1 — promoting the -/// parked dangling-window defect from exact-path to tree-wide. +/// **Rewritten by dired Stage 2a** (`docs/dired-stage2-framing.md` §6, +/// Q#RD27 / acceptance 23). This row previously pinned the opposite — +/// that the descendant buffer stayed orphaned — and gave the reason: +/// widening reconciliation would have routed N buffers through +/// `remove_buffer_and_fire`, which is phase 2 *without* phase 1, so a +/// tree delete would have left up to N windows pointing at removed ids. +/// That constraint is discharged: `EditorCore::reconcile_delete` +/// composes the same two phases `pmacs.buffer.kill` composes, and the +/// delete arm routes through it. The old assertion is not merely +/// obsolete, it is now the defect — an orphaned buffer whose next +/// `C-x C-s` recreates a file the user deleted. /// -/// Bite: fails against an implementation that widens reconciliation. +/// Bite, both directions: fails against an exact-path reconciliation +/// (the descendant survives) **and** against a widening that skips +/// phase 1 (a window keeps a removed id). #[test] -fn rd9_clean_recursive_delete_leaves_descendants_orphaned() { +fn rd9_clean_recursive_delete_reconciles_descendants_through_both_phases() { let dir = tempfile::tempdir().expect("tempdir"); let tree = dir.path().join("tree"); std::fs::create_dir(&tree).expect("mkdir"); @@ -8568,6 +8577,13 @@ fn rd9_clean_recursive_delete_leaves_descendants_orphaned() { let mut state = pmacs::editor::EditorState::new(); rd_open(&mut state, "B", &inner); + // Display it, so the phase-1 window redirect has something to do. + state + .lua_host + .lua() + .load("pmacs.window.switch_buffer(B)") + .exec() + .expect("show the descendant"); let (ok, err) = rd_delete(&mut state, &tree, ", recursive = true"); assert!(ok, "a clean tree deletes: {err}"); @@ -8580,9 +8596,23 @@ fn rd9_clean_recursive_delete_leaves_descendants_orphaned() { .eval() .expect("validity probe"); assert!( - still, - "THE BITE: reconciliation stays exact-path, so the descendant \ - buffer is orphaned rather than removed" + !still, + "THE BITE: a buffer under a recursively deleted directory must be \ + reconciled away, not left bound to a path whose file is gone" + ); + + let core = state.core.borrow(); + let dangling: Vec<_> = core + .windows + .iter() + .filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id)) + .map(|(id, w)| (*id, w.buffer_id)) + .collect(); + assert!( + dangling.is_empty(), + "THE OTHER HALF: widening the reconciliation must not promote the \ + dangling-window defect from exact-path to tree-wide; dangling: \ + {dangling:?}" ); } @@ -8633,15 +8663,22 @@ fn rd10_absent_plus_ignore_does_not_destroy_a_modified_buffer() { assert_eq!(text, "unsavedcontent\n", "the unsaved edit survives"); } -/// Criterion 14 — clean duplicates: exactly one match reconciled. +/// Criterion 14 — clean duplicates: **every** match reconciled. /// -/// Bite: fails against an implementation that removes **all** matches. -/// It pins the reconciliation half of Q#RD10 and *only* that: with both -/// buffers clean there is no verdict difference between consulting one -/// match and consulting all, so this setup cannot see validation -/// breadth. Criterion 6 is what detects incomplete validation. +/// **Rewritten by dired Stage 2a** (§6, acceptance 23). This row +/// previously pinned "exactly one", which was Q#RD10's deliberate +/// restraint: removing them all would have routed N buffers through +/// `remove_buffer_and_fire` — phase 2 without phase 1 — so the second +/// duplicate was left alive rather than have its window dangle. +/// `reconcile_delete` composes both phases, so the restraint is gone and +/// the surviving duplicate is now the defect: it is bound to a path +/// whose file no longer exists, and `find_by_path` cannot even see it. +/// +/// Bite: fails against a first-match implementation (one duplicate +/// survives) and against a widening that skips phase 1 (a window keeps +/// a removed id). #[test] -fn rd14_clean_duplicates_reconcile_exactly_one() { +fn rd14_clean_duplicates_all_reconcile() { let dir = tempfile::tempdir().expect("tempdir"); let f = dir.path().join("twin.rs"); std::fs::write(&f, b"twin\n").expect("write"); @@ -8658,6 +8695,13 @@ fn rd14_clean_duplicates_reconcile_exactly_one() { .exec() .expect("two clean buffers on one path"); + state + .lua_host + .lua() + .load("pmacs.window.switch_buffer(SECOND)") + .exec() + .expect("show the second duplicate"); + let (ok, err) = rd_delete(&mut state, &f, ""); assert!(ok, "two clean duplicates must not block: {err}"); @@ -8668,9 +8712,23 @@ fn rd14_clean_duplicates_reconcile_exactly_one() { .eval() .expect("validity probe"); assert!( - first != second, - "THE BITE: exactly one duplicate is reconciled away, not both \ - and not neither (first={first}, second={second})" + !first && !second, + "THE BITE: both buffers bound to the deleted path must be \ + reconciled away; a survivor points at a file that is gone and is \ + invisible to `find_by_path` (first={first}, second={second})" + ); + + let core = state.core.borrow(); + let dangling: Vec<_> = core + .windows + .iter() + .filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id)) + .map(|(id, w)| (*id, w.buffer_id)) + .collect(); + assert!( + dangling.is_empty(), + "THE OTHER HALF: removing every match must not leave a window on \ + a removed id; dangling: {dangling:?}" ); } diff --git a/tests/resource_reconciliation_acceptance.rs b/tests/resource_reconciliation_acceptance.rs new file mode 100644 index 0000000..c9beba4 --- /dev/null +++ b/tests/resource_reconciliation_acceptance.rs @@ -0,0 +1,2045 @@ +//! dired Stage 2a acceptance — rename and delete reconciliation. +//! +//! `docs/dired-stage2-framing.md` §5, §6, §10; acceptance items 23–38 +//! and 50–55. +//! +//! **This suite contains no dired content.** Stage 2a ships no dired +//! surface at all: it is the substrate transaction that Stage 2b's `R`, +//! `D` and `x` then stand on, and it closes three defects on `main` +//! that need no dired to be worth fixing — the workspace-edit phantom +//! buffer, the raw first-match registry lookup both `apply_resource_op` +//! arms used, and the incomplete removal lifecycle. +//! +//! Two disciplines the framing forces on every row here: +//! +//! * **Drive the real entry point.** A reconciliation with no +//! production caller passes every direct-call test, so the rename +//! rows go through `pmacs.fs.rename` (worker-dispatched, harvested in +//! the drain) or through `pmacs.buffer.apply_resource_op` (synchronous, +//! main-thread), never through `EditorCore::reconcile_rename`. +//! * **Pump to quiescence, never to a frame count**, because every +//! mutation is worker-dispatched. + +use std::path::{Path, PathBuf}; +use std::time::{Duration, Instant}; + +use pmacs::editor::EditorState; + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +fn exec(state: &EditorState, source: &str) { + state + .lua_host + .lua() + .load(source.to_owned()) + .exec() + .unwrap_or_else(|e| panic!("lua exec failed: {e}\n--- source ---\n{source}")); +} + +fn eval(state: &EditorState, source: &str) -> T { + state + .lua_host + .lua() + .load(source.to_owned()) + .eval() + .unwrap_or_else(|e| panic!("lua eval failed: {e}\n--- source ---\n{source}")) +} + +/// Escape a path for embedding in a Lua double-quoted string. +fn lua_str(path: &Path) -> String { + path.display() + .to_string() + .replace('\\', "\\\\") + .replace('"', "\\\"") +} + +/// Pump the async runtime until `predicate` holds or the deadline +/// lapses. Quiescence, not a frame count: the whole point of items 24 +/// and 25 is that the reconciliation happens in the drain, and the drain +/// runs whenever a reply arrives. +fn pump_until bool>(state: &mut EditorState, what: &str, predicate: F) { + let deadline = Instant::now() + Duration::from_secs(5); + while !predicate(state) { + assert!(Instant::now() < deadline, "pump deadline exceeded: {what}"); + state.tick_async(); + std::thread::sleep(Duration::from_millis(2)); + } +} + +/// Pump a fixed number of times without any expectation. Used only to +/// give a dispatched job every chance to settle before asserting that +/// something did **not** happen. +fn pump_a_while(state: &mut EditorState) { + for _ in 0..80 { + state.tick_async(); + std::thread::sleep(Duration::from_millis(2)); + } +} + +/// A canonicalized temp directory. Canonicalized because the buffer +/// registry stores lexically-normalized absolute paths and macOS's +/// `/var` is a symlink to `/private/var`; without this the expected +/// paths below would differ from the stored ones by a symlink hop. +struct Fixture { + _dir: tempfile::TempDir, + root: PathBuf, +} + +impl Fixture { + fn new() -> Self { + let dir = tempfile::tempdir().unwrap(); + let root = std::fs::canonicalize(dir.path()).unwrap(); + Self { _dir: dir, root } + } + + fn write(&self, rel: &str, contents: &str) -> PathBuf { + let path = self.root.join(rel); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, contents).unwrap(); + path + } + + fn dir(&self, rel: &str) -> PathBuf { + let path = self.root.join(rel); + std::fs::create_dir_all(&path).unwrap(); + path + } + + fn at(&self, rel: &str) -> PathBuf { + self.root.join(rel) + } +} + +fn editor() -> EditorState { + let state = EditorState::new(); + // No language server may spawn from these fixtures. The LSP rows + // that DO want one configure it explicitly. + exec(&state, "pmacs.lsp.config = {}"); + state +} + +/// Open `path` into a buffer and return the Lua global name holding its +/// handle. Buffers are held on globals so a test can re-read their path +/// and name after the reconciliation moved them. +fn open_as(state: &EditorState, global: &str, path: &Path) { + exec( + state, + &format!( + "_G.{global} = pmacs.buffer.find_or_open(\"{}\")", + lua_str(path) + ), + ); +} + +fn buffer_path(state: &EditorState, global: &str) -> Option { + eval( + state, + &format!("local b = _G.{global}; return b and b:path() or nil"), + ) +} + +fn buffer_name(state: &EditorState, global: &str) -> Option { + eval( + state, + &format!("local b = _G.{global}; return b and b:name() or nil"), + ) +} + +fn status(state: &EditorState) -> String { + state.core.borrow().status.clone() +} + +fn buffer_is_valid(state: &EditorState, global: &str) -> bool { + eval( + state, + &format!("local b = _G.{global}; return (b ~= nil) and b:is_valid()"), + ) +} + +/// Dispatch a rename **without awaiting** the handle, then pump. +/// Fire-and-forget is the shape item 25 pins: the reconciliation must +/// not live at result consumption. +fn rename_fire_and_forget(state: &mut EditorState, from: &Path, to: &Path) { + exec( + state, + &format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(from), + lua_str(to) + ), + ); + pump_until(state, "rename lands on disk", |_| to.exists()); + // The rename landing on disk and the reply reaching the main thread + // are two events; pump past the first to reach the second. + pump_a_while(state); +} + +fn remove_fire_and_forget(state: &mut EditorState, path: &Path) { + exec(state, &format!("pmacs.fs.remove(\"{}\")", lua_str(path))); + pump_until(state, "remove lands on disk", |_| !path.exists()); + pump_a_while(state); +} + +// --------------------------------------------------------------------------- +// 25 — no-await rename +// --------------------------------------------------------------------------- + +/// Acceptance 25. Dispatch `pmacs.fs.rename`, **never take the +/// result**, pump: the open buffer's path has moved. +/// +/// Bite: fails if the reconciliation lives at result consumption +/// (`_take_result`) rather than in the drain, because nothing here ever +/// consumes the handle. +#[test] +fn acc25_a_never_awaited_rename_still_moves_the_open_buffers_path() { + let fx = Fixture::new(); + let old = fx.write("notes.txt", "hello\n"); + let new = fx.at("renamed.txt"); + let mut state = editor(); + open_as(&state, "B", &old); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(old.to_str().unwrap()) + ); + + rename_fire_and_forget(&mut state, &old, &new); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the buffer must follow a fire-and-forget rename" + ); +} + +// --------------------------------------------------------------------------- +// 26, 27, 28 — the walk +// --------------------------------------------------------------------------- + +/// Acceptance 26. A buffer open on `dir/child.txt` follows +/// `dir` → `newdir`. +#[test] +fn acc26_a_buffer_under_a_renamed_directory_follows_it() { + let fx = Fixture::new(); + fx.dir("tree"); + let child = fx.write("tree/child.txt", "x\n"); + let mut state = editor(); + open_as(&state, "B", &child); + + rename_fire_and_forget(&mut state, &fx.at("tree"), &fx.at("newtree")); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(fx.at("newtree/child.txt").to_str().unwrap()), + "a descendant keeps its relative tail under the new root" + ); +} + +/// Acceptance 27. **Every** match, not the first: two descendant +/// buffers under the renamed directory *and* two buffers visiting the +/// same exact path all move. +/// +/// Bite: fails against `find_by_path`'s first match. One child buffer +/// would not defeat a first-match implementation; this does, twice over +/// — and the duplicate-path pair is the case `find_by_path` cannot even +/// see, because it returns on the first hit. +#[test] +fn acc27_every_affected_buffer_moves_not_only_the_first() { + let fx = Fixture::new(); + fx.dir("tree"); + let one = fx.write("tree/one.txt", "1\n"); + let two = fx.write("tree/nested/two.txt", "2\n"); + let mut state = editor(); + open_as(&state, "ONE", &one); + open_as(&state, "TWO", &two); + // Two buffers on the SAME exact path. `pmacs.buffer.from_file` + // creates a fresh buffer without deduping, which is how a duplicate + // path binding is reachable from public Lua. + exec( + &state, + &format!("_G.DUP = pmacs.buffer.from_file(\"{}\")", lua_str(&one)), + ); + let dup_first: String = eval(&state, "return _G.ONE:path()"); + let dup_second: String = eval(&state, "return _G.DUP:path()"); + assert_eq!( + dup_first, dup_second, + "precondition: two distinct buffers bound to one path" + ); + + rename_fire_and_forget(&mut state, &fx.at("tree"), &fx.at("newtree")); + + assert_eq!( + buffer_path(&state, "ONE").as_deref(), + Some(fx.at("newtree/one.txt").to_str().unwrap()), + "first descendant" + ); + assert_eq!( + buffer_path(&state, "TWO").as_deref(), + Some(fx.at("newtree/nested/two.txt").to_str().unwrap()), + "second, more deeply nested descendant" + ); + assert_eq!( + buffer_path(&state, "DUP").as_deref(), + Some(fx.at("newtree/one.txt").to_str().unwrap()), + "the second buffer on the same path — invisible to a first-match \ + lookup, and left pointing at nothing by one" + ); +} + +/// Acceptance 28. Renaming `/…/foo` must not rebind a buffer on +/// `/…/foobar`. +/// +/// Bite: fails against a string `starts_with` instead of a +/// path-component prefix. +#[test] +fn acc28_a_false_string_prefix_is_not_a_path_prefix() { + let fx = Fixture::new(); + fx.dir("foo"); + let inside = fx.write("foo/a.txt", "in\n"); + let sibling = fx.write("foobar.txt", "out\n"); + let mut state = editor(); + open_as(&state, "IN", &inside); + open_as(&state, "OUT", &sibling); + + rename_fire_and_forget(&mut state, &fx.at("foo"), &fx.at("renamed")); + + assert_eq!( + buffer_path(&state, "IN").as_deref(), + Some(fx.at("renamed/a.txt").to_str().unwrap()), + "the real descendant moves" + ); + assert_eq!( + buffer_path(&state, "OUT").as_deref(), + Some(sibling.to_str().unwrap()), + "`foobar.txt` shares a string prefix with `foo` and is not under it" + ); +} + +/// Acceptance 28, delete side — **and this is the row that bites.** +/// +/// The rename row above cannot falsify a string-prefix walk on its own: +/// `reconcile_rename` calls `Path::strip_prefix` to rebuild the +/// descendant's tail, and that is component-aware too, so a false +/// prefix match is silently dropped a second time and the buffer stays +/// put. Deletion has no such second guard — the walk's verdict IS the +/// kill list — so the containment rule has to be pinned here. +/// +/// Bite: a string `starts_with` instead of `Path::starts_with` kills a +/// buffer on `foobar.txt` when `foo/` is deleted. +#[test] +fn acc28_delete_a_false_string_prefix_does_not_widen_the_kill_list() { + let fx = Fixture::new(); + fx.dir("foo"); + let inside = fx.write("foo/a.txt", "in\n"); + let sibling = fx.write("foobar.txt", "out\n"); + let state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "IN", &inside); + open_as(&state, "OUT", &sibling); + + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"delete\", \ + path = \"{}\", recursive = true }}", + lua_str(&fx.at("foo")) + ), + ); + + assert!(!fx.at("foo").exists(), "the directory is gone"); + assert!( + !buffer_is_valid(&state, "IN"), + "the real descendant is reconciled away" + ); + assert!( + buffer_is_valid(&state, "OUT"), + "`foobar.txt` shares a string prefix with `foo` and is not under \ + it — killing it destroys an unrelated buffer whose file still \ + exists" + ); + assert!( + sibling.exists(), + "and that file is indeed still on disk, which is what makes the \ + kill wrong rather than merely early" + ); +} + +// --------------------------------------------------------------------------- +// 29 — name provenance, both directions +// --------------------------------------------------------------------------- + +/// Acceptance 29(a). A buffer opened by a **relative** path is named +/// `foo.rs` while its stored path is absolute, and its name follows the +/// rename because its load site recorded `PathDerived`. +/// +/// Bite: rev 7's string-equality rule fails this — the name never +/// equalled the normalized path, so it would have been left stale while +/// insisting it was user-chosen. +#[test] +fn acc29a_a_relative_opens_name_follows_the_rename() { + let fx = Fixture::new(); + let old = fx.write("relative.txt", "x\n"); + let new = fx.at("moved.txt"); + let mut state = editor(); + // Open by a path whose *spelling* is not the stored path: a `.` + // component is folded by normalization but kept in the name, which + // reproduces the relative-open shape without depending on the + // process cwd. + let as_given = fx.at("./relative.txt"); + open_as(&state, "B", &as_given); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(as_given.to_str().unwrap()), + "precondition: the name is the path AS GIVEN, not the stored path" + ); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(old.to_str().unwrap()), + "precondition: only the stored path is normalized" + ); + + rename_fire_and_forget(&mut state, &old, &new); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()) + ); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the name follows because the load site recorded path provenance, \ + not because the old name happened to match the old path" + ); +} + +/// Acceptance 29(b). A name set explicitly through +/// `pmacs.buffer.set_name` survives the rename **even when that string +/// normalizes to the file's own stored path**. +/// +/// Bite: rev 8's path-equivalence heuristic fails this — the chosen +/// name normalizes to the exact stored path, so the heuristic would +/// overwrite it. +#[test] +fn acc29b_an_explicitly_set_name_survives_even_when_it_denotes_the_file() { + let fx = Fixture::new(); + let old = fx.write("notes", "x\n"); + let new = fx.at("notes-renamed"); + let mut state = editor(); + open_as(&state, "B", &old); + // The chosen name IS the file's absolute path. Under a + // path-equivalence rule this is indistinguishable from a + // path-derived name; under recorded provenance it is not. + exec( + &state, + &format!("pmacs.buffer.set_name(_G.B, \"{}\")", lua_str(&old)), + ); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(old.to_str().unwrap()), + "precondition: the explicit name normalizes to the stored path" + ); + + rename_fire_and_forget(&mut state, &old, &new); + + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the PATH always follows" + ); + assert_eq!( + buffer_name(&state, "B").as_deref(), + Some(old.to_str().unwrap()), + "and the explicitly chosen name does not, however much it looks \ + like a path-derived one" + ); +} + +// --------------------------------------------------------------------------- +// 36, 37 — the synchronous arm, and failure +// --------------------------------------------------------------------------- + +/// Acceptance 36. `apply_resource_op`'s rename finds a buffer whose +/// stored path is normalized but whose op names it **un-normalized**. +/// +/// Bite: fails against the raw `find_by_path(&from)` this arm used — +/// stored paths are normalized on write, so a raw lookup with a `.` +/// component in it misses the buffer entirely and the rename silently +/// reconciles nothing. +#[test] +fn acc36_the_synchronous_arm_matches_an_un_normalized_op_path() { + let fx = Fixture::new(); + let old = fx.write("sync.txt", "x\n"); + let new = fx.at("sync-moved.txt"); + let state = editor(); + open_as(&state, "B", &old); + + let unnormalized = fx.at("./sync.txt"); + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"rename\", \ + old_path = \"{}\", new_path = \"{}\" }}", + lua_str(&unnormalized), + lua_str(&new) + ), + ); + + assert!(new.exists(), "the rename happened on disk"); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "the buffer must be found even though the op spelled the source \ + path differently from the stored one" + ); +} + +/// Acceptance 37. A **failed** rename reconciles nothing — and fires no +/// hook. +#[test] +fn acc37_a_failed_rename_reconciles_nothing_and_fires_no_hook() { + let fx = Fixture::new(); + let present = fx.write("present.txt", "x\n"); + let missing = fx.at("does-not-exist.txt"); + let mut state = editor(); + open_as(&state, "B", &present); + exec( + &state, + "_G.FIRED = 0 + pmacs.hook.add('resource.renamed', function() _G.FIRED = _G.FIRED + 1 end)", + ); + + // Renaming a path that does not exist fails in the worker. + exec( + &state, + &format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(&missing), + lua_str(&fx.at("target.txt")) + ), + ); + pump_a_while(&mut state); + + let fired: i64 = eval(&state, "return _G.FIRED"); + assert_eq!(fired, 0, "a failed mutation reconciles nothing"); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(present.to_str().unwrap()), + "and no unrelated buffer moved" + ); +} + +// --------------------------------------------------------------------------- +// 50, 55 — the hooks +// --------------------------------------------------------------------------- + +/// Acceptance 50. `resource.renamed` fires **exactly once** per +/// successful rename, with `(old, new)` as normalized absolute paths, +/// and does not fire for a rename that failed. The symmetric assertion +/// for `resource.deleted` accompanies it. +/// +/// Bite: fails if the hook fires for a failed rename, or fires with the +/// un-normalized path a caller happened to spell. +#[test] +fn acc50_the_hooks_fire_once_with_normalized_paths() { + let fx = Fixture::new(); + let old = fx.write("hooked.txt", "x\n"); + let new = fx.at("hooked-moved.txt"); + let doomed = fx.write("doomed.txt", "y\n"); + let mut state = editor(); + exec( + &state, + "_G.RENAMES = {} + _G.DELETES = {} + pmacs.hook.add('resource.renamed', function(a, b) + _G.RENAMES[#_G.RENAMES + 1] = tostring(a) .. ' -> ' .. tostring(b) + end) + pmacs.hook.add('resource.deleted', function(p) + _G.DELETES[#_G.DELETES + 1] = tostring(p) + end)", + ); + + // Spell BOTH paths un-normalized, so the hook's arguments can only + // be canonical if the fire site normalizes them. + exec( + &state, + &format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(&fx.at("./hooked.txt")), + lua_str(&fx.at("./hooked-moved.txt")) + ), + ); + pump_until(&mut state, "rename hook", |s| { + let n: i64 = eval(s, "return #_G.RENAMES"); + n > 0 + }); + pump_a_while(&mut state); + + let renames: String = eval(&state, "return table.concat(_G.RENAMES, '|')"); + assert_eq!( + renames, + format!("{} -> {}", old.display(), new.display()), + "exactly one row, and both paths canonical — a path-keyed \ + subscriber needs the form the registry keys on" + ); + + exec( + &state, + &format!("pmacs.fs.remove(\"{}\")", lua_str(&fx.at("./doomed.txt"))), + ); + pump_until(&mut state, "delete hook", |s| { + let n: i64 = eval(s, "return #_G.DELETES"); + n > 0 + }); + pump_a_while(&mut state); + let deletes: String = eval(&state, "return table.concat(_G.DELETES, '|')"); + assert_eq!( + deletes, + doomed.display().to_string(), + "one row, canonical path" + ); +} + +/// Acceptance 55. Both hooks are `all-must-succeed`, not +/// short-circuit: with two subscribers registered and the **first one +/// raising**, the second still runs and the error is reported rather +/// than swallowed. +/// +/// Bite: fails against a `short-circuit` registration, where the first +/// subscriber's return would stop the fan-out and silently prevent every +/// later one from reconciling — which no test asserting only "the hook +/// fired" would catch. +#[test] +fn acc55_a_raising_subscriber_does_not_stop_the_fan_out() { + let fx = Fixture::new(); + let old = fx.write("fanout.txt", "x\n"); + let new = fx.at("fanout-moved.txt"); + let doomed = fx.write("fanout-doomed.txt", "y\n"); + let mut state = editor(); + exec( + &state, + "_G.SECOND_RAN = 0 + _G.SECOND_DELETED = 0 + pmacs.hook.add('resource.renamed', function() error('first subscriber explodes') end) + pmacs.hook.add('resource.renamed', function() _G.SECOND_RAN = _G.SECOND_RAN + 1 end) + pmacs.hook.add('resource.deleted', function() error('first subscriber explodes') end) + pmacs.hook.add('resource.deleted', function() _G.SECOND_DELETED = _G.SECOND_DELETED + 1 end)", + ); + + rename_fire_and_forget(&mut state, &old, &new); + let ran: i64 = eval(&state, "return _G.SECOND_RAN"); + assert_eq!( + ran, 1, + "`all-must-succeed` collects the first callback's error and \ + continues; a short-circuit registration would have stopped here" + ); + + remove_fire_and_forget(&mut state, &doomed); + let deleted: i64 = eval(&state, "return _G.SECOND_DELETED"); + assert_eq!(deleted, 1, "same for `resource.deleted`"); + + // The error is reported, not swallowed: the hook error log is the + // `*errors*` buffer. + let errors: String = eval( + &state, + "for _, b in ipairs(pmacs.buffer.list()) do + if b:name() == '*errors*' then return b:slice(0, b:len()) end + end + return ''", + ); + assert!( + errors.contains("first subscriber explodes"), + "the raising subscriber's error must be reported; *errors* held: \ + {errors:?}" + ); +} + +// --------------------------------------------------------------------------- +// 23, 24 — the delete lookups +// --------------------------------------------------------------------------- + +/// Acceptance 23. `apply_resource_op`'s delete reaches **descendants** +/// and a **second buffer on the same path** — the raw first-match lookup +/// replaced by the shared prefix-aware, normalizing query. +/// +/// (#190 owns the modified-buffer refusal; it refuses before disk, so by +/// the time this lane's reconciliation runs there is no modified buffer +/// on the synchronous path to spare. This row asserts the lookup fix.) +#[test] +fn acc23_the_synchronous_delete_reaches_descendants_and_duplicates() { + let fx = Fixture::new(); + fx.dir("tree/nested"); + let one = fx.write("tree/one.txt", "1\n"); + fx.write("tree/nested/two.txt", "2\n"); + let state = editor(); + open_as(&state, "ONE", &one); + open_as(&state, "TWO", &fx.at("tree/nested/two.txt")); + exec( + &state, + &format!("_G.DUP = pmacs.buffer.from_file(\"{}\")", lua_str(&one)), + ); + // Keep an unrelated buffer alive so the last-buffer refusal is not + // what this row measures. + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"delete\", \ + path = \"{}\", recursive = true }}", + lua_str(&fx.at("./tree")) + ), + ); + + assert!(!fx.at("tree").exists(), "the tree is gone from disk"); + assert!( + !buffer_is_valid(&state, "ONE"), + "a buffer directly under the deleted directory" + ); + assert!( + !buffer_is_valid(&state, "TWO"), + "a more deeply nested descendant" + ); + assert!( + !buffer_is_valid(&state, "DUP"), + "the second buffer on the same path — the one a first-match \ + lookup cannot see, which #190 deliberately left in place \ + because it had no two-phase kill to route it through" + ); + assert!(buffer_is_valid(&state, "KEEP"), "an unrelated buffer"); +} + +/// Acceptance 24. A **fire-and-forget** `pmacs.fs.remove` reconciles +/// too: never taking the handle still kills the unmodified buffer, which +/// is what makes the drain harvest the right seam rather than dired +/// firing the hook itself. +#[test] +fn acc24_a_never_awaited_remove_still_kills_the_unmodified_buffer() { + let fx = Fixture::new(); + let doomed = fx.write("gone.txt", "x\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + assert!(buffer_is_valid(&state, "B")); + + remove_fire_and_forget(&mut state, &doomed); + + assert!( + !buffer_is_valid(&state, "B"), + "the harvest must reconcile a delete no one awaited" + ); + assert!(buffer_is_valid(&state, "KEEP")); +} + +/// Acceptance 18's substrate half, and §6's modified case: a **modified** +/// buffer whose file is deleted out from under it keeps its contents. The +/// buffer half is the part of the promise that is robust, because it runs +/// at drain time against whatever state exists then. +#[test] +fn a_modified_buffer_survives_a_delete_with_its_contents() { + let fx = Fixture::new(); + let doomed = fx.write("dirty.txt", "original\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec(&state, "_G.B:insert(0, 'edited ')"); + let modified: bool = eval(&state, "return _G.B:is_modified()"); + assert!(modified, "precondition"); + + remove_fire_and_forget(&mut state, &doomed); + + assert!( + buffer_is_valid(&state, "B"), + "a modified buffer is kept alive deliberately, not killed" + ); + let contents: String = eval(&state, "return _G.B:slice(0, _G.B:len())"); + assert_eq!(contents, "edited original\n", "with its contents intact"); + assert!(!doomed.exists(), "while the file is gone"); +} + +// --------------------------------------------------------------------------- +// 51, 52, 53 — the removal lifecycle +// --------------------------------------------------------------------------- + +/// Acceptance 51. A killed buffer completes **both** removal phases: +/// after a delete reconciles, an `on_removed` callback registered for +/// that buffer has fired and its buffer-local keymap entries are gone. +/// +/// Bite: fails against an implementation that calls only +/// `EditorCore::kill_buffer`, which does no phase-2 cleanup at all. +/// +/// Note 51 and 52 are a matched pair and **neither alone is +/// sufficient** — each pre-existing removal path passes one and fails +/// the other, which is exactly why both phases had to be named. +#[test] +fn acc51_a_killed_buffer_completes_both_removal_phases() { + let fx = Fixture::new(); + let doomed = fx.write("phase2.txt", "x\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec( + &state, + "_G.ON_REMOVED = 0 + pmacs.buffer.on_removed(_G.B, function() _G.ON_REMOVED = _G.ON_REMOVED + 1 end) + pmacs.command.define { name = 'test.noop', description = 'x', fn = function() end } + pmacs.keymap.bind { scope = 'buffer', buffer = _G.B, + sequence = 'C-c C-1', command = 'test.noop' } + -- `pmacs.keymap.lookup` is deliberately raw-global, so the + -- buffer-scoped row is only visible through `list()`. + function _G.BOUND_ROWS() + local n = 0 + for _, e in ipairs(pmacs.keymap.list()) do + if e.command == 'test.noop' then n = n + 1 end + end + return n + end", + ); + let bound_before: i64 = eval(&state, "return _G.BOUND_ROWS()"); + assert_eq!( + bound_before, 1, + "precondition: the buffer-local binding exists" + ); + + remove_fire_and_forget(&mut state, &doomed); + + assert!(!buffer_is_valid(&state, "B"), "phase 1 removed the buffer"); + let fired: i64 = eval(&state, "return _G.ON_REMOVED"); + assert_eq!( + fired, 1, + "phase 2 must fire the registered on_removed callback; 0 means the \ + reconciliation called `EditorCore::kill_buffer` alone" + ); + let bound_after: i64 = eval(&state, "return _G.BOUND_ROWS()"); + assert_eq!( + bound_after, 0, + "phase 2 must purge the buffer-scoped keymap, so a later buffer \ + cannot inherit a dead one's bindings" + ); +} + +/// Acceptance 52. A window displaying the deleted buffer is +/// **redirected**, not left dangling: no window holds a removed id. +/// +/// Bite: fails against `remove_buffer_and_fire`, which is what +/// `apply_resource_op` used — phase 2 without phase 1, so +/// `BufferRegistry::remove` runs while every window showing the buffer +/// keeps pointing at the id it just dropped. +#[test] +fn acc52_a_window_showing_the_deleted_buffer_is_redirected() { + let fx = Fixture::new(); + let doomed = fx.write("shown.txt", "x\n"); + let state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + let doomed_id = state.core.borrow().active_buffer_id(); + assert!( + state + .core + .borrow() + .windows + .values() + .any(|w| w.buffer_id == doomed_id), + "precondition: a window shows the doomed buffer" + ); + + exec( + &state, + &format!( + "pmacs.buffer.apply_resource_op {{ kind = \"delete\", path = \"{}\" }}", + lua_str(&doomed) + ), + ); + + let core = state.core.borrow(); + assert!( + !core.registry.borrow().contains(doomed_id), + "the buffer was removed" + ); + let dangling: Vec<_> = core + .windows + .iter() + .filter(|(_, w)| !core.registry.borrow().contains(w.buffer_id)) + .map(|(id, w)| (*id, w.buffer_id)) + .collect(); + assert!( + dangling.is_empty(), + "no window may hold a removed buffer id; dangling: {dangling:?}" + ); + assert!( + !core.windows.values().any(|w| w.buffer_id == doomed_id), + "and specifically not the deleted one" + ); +} + +/// Acceptance 53. The last-buffer and mid-edit refusals are +/// **reported, not silent**, and neither aborts the reconciliation of +/// other buffers. +#[test] +fn acc53_the_last_buffer_refusal_keeps_the_buffer_and_the_rest_proceeds() { + // Half one: the file behind the only open buffer. `kill_buffer` + // refuses to remove the last remaining buffer, so the file goes and + // the buffer stays. + let fx = Fixture::new(); + let only = fx.write("only.txt", "x\n"); + let mut state = editor(); + // Drop every other buffer so the target really is the last one. + exec( + &state, + &format!( + "_G.ONLY = pmacs.buffer.find_or_open(\"{}\") + for _, b in ipairs(pmacs.buffer.list()) do + if tostring(b) ~= tostring(_G.ONLY) then + pcall(pmacs.buffer.kill, b) + end + end + return #pmacs.buffer.list()", + lua_str(&only) + ), + ); + let count: i64 = eval(&state, "return #pmacs.buffer.list()"); + assert_eq!(count, 1, "precondition: exactly one buffer is open"); + + remove_fire_and_forget(&mut state, &only); + assert!( + buffer_is_valid(&state, "ONLY"), + "the last remaining buffer cannot be killed, so it survives the \ + deletion of its file" + ); + // **Reported, not silent.** Survival alone is not the criterion: the + // buffer is still bound to a path whose file is gone, so the next + // `C-x C-s` recreates the file the user deleted. That is recoverable + // only if the user is told. + let said = status(&state); + assert!( + said.contains("could not be closed"), + "the refusal must reach the status channel; status was {said:?}" + ); + // Asserted as the buffer's OWN name, not as the basename. The + // message opens with `deleted only.txt:` — the *path* — so a + // `contains("only.txt")` check passes with the attribution stripped, + // which is exactly how this assertion was vacuous when first + // written. A path-backed buffer's name is the full path, and only + // the `buffer "…"` prefix can produce it. + let expect_named = format!("buffer {:?}", only.display().to_string()); + assert!( + said.contains(&expect_named), + "the refusal must name the buffer, because `cannot kill the last \ + remaining buffer` alone does not say WHICH buffer is now bound \ + to a deleted path; wanted {expect_named:?} in {said:?}" + ); + + // Half two: a directory of buffers where one refuses removal. The + // rest must still reconcile. + let fx2 = Fixture::new(); + fx2.dir("batch"); + let a = fx2.write("batch/a.txt", "a\n"); + let b = fx2.write("batch/b.txt", "b\n"); + let c = fx2.write("batch/c.txt", "c\n"); + let mut state2 = editor(); + open_as(&state2, "KEEP", &fx2.write("keep.txt", "k\n")); + open_as(&state2, "A", &a); + open_as(&state2, "B", &b); + open_as(&state2, "C", &c); + // B refuses: it is modified. + exec(&state2, "_G.B:insert(0, 'dirty ')"); + + remove_fire_and_forget(&mut state2, &fx2.at("batch/a.txt")); + remove_fire_and_forget(&mut state2, &fx2.at("batch/b.txt")); + remove_fire_and_forget(&mut state2, &fx2.at("batch/c.txt")); + + assert!(!buffer_is_valid(&state2, "A"), "A reconciled"); + assert!( + buffer_is_valid(&state2, "B"), + "B was kept because it is modified" + ); + assert!( + !buffer_is_valid(&state2, "C"), + "and C still reconciled afterwards — one refusal must not abort \ + the rest" + ); + // The kept-modified case reports too, and says what the consequence + // is. This is the asynchronous race the framing's H1 leaves open: + // #190 refuses before disk on the synchronous path, but + // `pmacs.fs.remove` dispatches a worker, so a buffer modified in the + // interval reaches the drain with its file already gone. + let said2 = status(&state2); + assert!( + said2.contains("unsaved changes kept"), + "a modified buffer kept alive over a deleted file must be \ + reported; status was {said2:?}" + ); + assert!( + said2.contains("RECREATE"), + "and the report must state the consequence — saving it puts the \ + deleted file back; status was {said2:?}" + ); + // Same discipline: the full path is the buffer's name, while the + // message's `deleted b.txt:` prefix is only the basename. + assert!( + said2.contains(&b.display().to_string()), + "the kept buffer must be named, and by its own name rather than \ + the deleted path's basename; status was {said2:?}" + ); +} + +// --------------------------------------------------------------------------- +// 53b — a mid-edit refusal leaves editor state UNCHANGED +// --------------------------------------------------------------------------- + +/// Acceptance 53b, all three assertions, **stated individually**. +/// +/// With the buffer `editing_in_progress`, displayed in an ordinary +/// window, shown in a side window, and present in `round_trip_buffers`, +/// a delete reconciling it must leave each of the following provably +/// untouched. Each fails independently against the same one-line bite — +/// removing the `editing_in_progress` preflight — which is the point: a +/// single compound assertion can pass on two of the three and hide the +/// third. +/// +/// | # | Assertion | What the missing preflight breaks | +/// |---|---|---| +/// | i | the ordinary window still shows the buffer, cursor/selection/`view_top` intact | `kill_buffer` redirects the window to the fallback before `BufferRegistry::remove` refuses | +/// | ii | the side window is still open and still shows the buffer | `remove_side_window` collapses it first | +/// | iii | the buffer is still in `round_trip_buffers` | `round_trip_buffers.remove` runs first — the **first** thing `kill_buffer` does, and the easiest to miss | +#[test] +#[allow( + clippy::too_many_lines, + reason = "three independent assertions, each with its own precondition; a compound check is exactly what this row exists to avoid" +)] +fn acc53b_a_mid_edit_refusal_leaves_window_side_and_round_trip_state_untouched() { + let fx = Fixture::new(); + let doomed = fx.write("midedit.txt", "0123456789\nsecond line\n"); + let mut state = editor(); + // A grid frontend's real frame size is its declaration, and a side + // window needs one before it can be placed. + state.sync_frame_geometry( + pmacs::protocol::FrontendId::LOCAL, + pmacs::cell::CellSize::new(24, 80), + ); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "B", &doomed); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + exec(&state, "pmacs.buffer.set_round_trip_input(_G.B, true)"); + + let doomed_id = state.core.borrow().active_buffer_id(); + let ordinary_window = state.core.borrow().active_window_id(); + // Seat a distinctive cursor + selection + scroll position on the + // ORDINARY window, so a redirect is detectable as more than "the + // window moved". + { + let mut core = state.core.borrow_mut(); + let win = core + .windows + .get_mut(&ordinary_window) + .expect("ordinary window"); + win.cursor = 4; + win.selection = Some(pmacs::window::Selection { anchor: 2 }); + win.view_top = 1; + } + + // A SIDE window showing the same buffer, so the collapse the + // preflight prevents has something to collapse. + exec( + &state, + "pmacs.window.display(_G.B, { side = \"bottom\", height = 4 })", + ); + let side_windows: Vec<_> = state + .core + .borrow() + .side_window_for(pmacs::protocol::FrontendId::LOCAL) + .into_iter() + .collect(); + assert!( + !side_windows.is_empty(), + "precondition: a side window exists" + ); + assert_eq!( + state + .core + .borrow() + .windows + .get(&side_windows[0]) + .expect("side window") + .buffer_id, + doomed_id, + "precondition: the side window shows the doomed buffer" + ); + assert_ne!( + side_windows[0], ordinary_window, + "precondition: the side window is a second window" + ); + assert!( + state.core.borrow().buffer_round_trips(doomed_id), + "precondition: the buffer round-trips input" + ); + + // Put the buffer mid-edit. `begin_edit` is the flag + // `BufferRegistry::remove` refuses on, and the whole point of the + // preflight is that the refusal arrives too late. + { + let core = state.core.borrow(); + let mut reg = core.registry.borrow_mut(); + reg.get_mut(doomed_id) + .expect("doomed buffer") + .begin_edit() + .expect("begin edit"); + } + + remove_fire_and_forget(&mut state, &doomed); + + let core = state.core.borrow(); + // (i) the ordinary window, with its seated state. + let win = core + .windows + .get(&ordinary_window) + .expect("the ordinary window still exists"); + assert_eq!( + win.buffer_id, doomed_id, + "(i) the ordinary window must still show the buffer" + ); + assert_eq!(win.cursor, 4, "(i) cursor"); + assert_eq!( + win.selection, + Some(pmacs::window::Selection { anchor: 2 }), + "(i) selection" + ); + assert_eq!(win.view_top, 1, "(i) view_top"); + + // (ii) the side window. + for side in &side_windows { + let side_win = core.windows.get(side).unwrap_or_else(|| { + panic!( + "(ii) side window {side:?} was collapsed by a kill that should never have started" + ) + }); + assert_eq!( + side_win.buffer_id, doomed_id, + "(ii) the side window must still show the buffer" + ); + } + + // (iii) round-trip membership. + assert!( + core.buffer_round_trips(doomed_id), + "(iii) the buffer must still round-trip input — this is the FIRST \ + thing `kill_buffer` drops and the easiest to miss" + ); + + assert!( + core.registry.borrow().contains(doomed_id), + "and the buffer itself is still in the registry" + ); + drop(core); + // And the refusal is REPORTED. Leaving state untouched is only half + // the contract: the file is gone, so a user who is not told keeps a + // buffer bound to a path that no longer exists. + let said = status(&state); + assert!( + said.contains("mid-edit") && said.contains("could not be closed"), + "a mid-edit refusal must reach the status channel; status was \ + {said:?}" + ); +} + +// --------------------------------------------------------------------------- +// 54 — independent mutations both reconcile, in either arrival order +// --------------------------------------------------------------------------- + +/// Acceptance 54, integration layer. Dispatch a rename and a delete on +/// **disjoint** paths, wait for both, and assert both registry effects +/// occurred. It fails against dropping or deduplicating one resource +/// kind. +/// +/// The disjoint end state is confidence coverage, **not** a claimed bite +/// against interdependent sequencing: disjoint paths necessarily +/// commute. The controlled-bus layer that does pin arrival order lives +/// in `src/async_runtime.rs`, and no test here pretends to pin an order +/// the mechanism does not establish. +#[test] +fn acc54_a_rename_and_a_delete_on_disjoint_paths_both_reconcile() { + for reverse in [false, true] { + let fx = Fixture::new(); + let renamed_from = fx.write("moves.txt", "m\n"); + let renamed_to = fx.at("moved.txt"); + let deleted = fx.write("goes.txt", "g\n"); + let mut state = editor(); + open_as(&state, "KEEP", &fx.write("keep.txt", "k\n")); + open_as(&state, "MOVES", &renamed_from); + open_as(&state, "GOES", &deleted); + + let rename = format!( + "pmacs.fs.rename(\"{}\", \"{}\")", + lua_str(&renamed_from), + lua_str(&renamed_to) + ); + let remove = format!("pmacs.fs.remove(\"{}\")", lua_str(&deleted)); + if reverse { + exec(&state, &remove); + exec(&state, &rename); + } else { + exec(&state, &rename); + exec(&state, &remove); + } + + pump_until(&mut state, "both mutations", |s| { + renamed_to.exists() && !deleted.exists() && !buffer_is_valid(s, "GOES") + }); + pump_a_while(&mut state); + + assert_eq!( + buffer_path(&state, "MOVES").as_deref(), + Some(renamed_to.to_str().unwrap()), + "the rename reconciled (dispatch order reversed: {reverse})" + ); + assert!( + !buffer_is_valid(&state, "GOES"), + "the delete reconciled (dispatch order reversed: {reverse})" + ); + } +} + +// --------------------------------------------------------------------------- +// The LSP-facing rows (30, 31c, 32, 34, 35) +// --------------------------------------------------------------------------- +// +// Driven against `pmacs_fake_lsp` so nothing here needs a real toolchain +// on PATH. The fake publishes two synthetic diagnostics (one Error at +// line 0, one Warning at line 2) on every `didOpen`, which is what makes +// "the new URI's diagnostics" observable at all. + +fn fake_lsp_path() -> String { + env!("CARGO_BIN_EXE_pmacs_fake_lsp").to_owned() +} + +/// Configure `language` to spawn the fake, and pin the project-marker +/// walk to `root` so a stray `.git` above the tempdir cannot silently +/// turn a markerless fixture into a detected one. +fn configure_fake(state: &EditorState, root: &Path, language: &str) { + exec( + state, + &format!( + "pmacs.project.set_search_boundary(\"{}\") + pmacs.lsp.config.{language} = {{ command = \"{}\" }}", + lua_str(root), + fake_lsp_path() + ), + ); +} + +/// Pump the real frame order — processes, LSP, async — until `predicate` +/// holds. All three are needed: the fake's frames arrive through the +/// supervisor, the manager parses them, and the rename settles on the +/// async bus. +fn settle_until bool>( + state: &mut EditorState, + what: &str, + predicate: F, +) { + let deadline = Instant::now() + Duration::from_secs(20); + while !predicate(state) { + assert!( + Instant::now() < deadline, + "settle deadline exceeded: {what}" + ); + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + std::thread::sleep(Duration::from_millis(5)); + } +} + +fn settle_a_while(state: &mut EditorState) { + for _ in 0..120 { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + std::thread::sleep(Duration::from_millis(3)); + } +} + +fn server_count(state: &EditorState) -> i64 { + eval(state, "return #pmacs.lsp.list()") +} + +/// `language|root_uri|state` per live server, sorted, so assertions do +/// not depend on spawn order. +fn server_rows(state: &EditorState) -> Vec { + let joined: String = eval( + state, + r#" + local out = {} + for _, s in ipairs(pmacs.lsp.list()) do + out[#out + 1] = table.concat({ + s.language_id or "", s.root_uri or "", + (s.state and s.state.kind) or "", + }, "|") + end + table.sort(out) + return table.concat(out, "\n") + "#, + ); + if joined.is_empty() { + Vec::new() + } else { + joined.lines().map(str::to_owned).collect() + } +} + +fn diag_count(state: &EditorState, uri: &str) -> i64 { + eval( + state, + &format!( + "local e, w, i, h = pmacs.diag.count(\"{uri}\") + return (e or 0) + (w or 0) + (i or 0) + (h or 0)" + ), + ) +} + +/// Mirror of `file_uri_for` in `builtin/runtime/lsp.lua`. Reimplemented +/// rather than imported, so the test states the expected encoding +/// independently of the code under test. +fn file_uri(path: &Path) -> String { + let mut out = String::from("file://"); + for byte in path.display().to_string().as_bytes() { + match byte { + b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'/' | b'-' | b'_' | b'.' | b'~' | b':' => { + out.push(*byte as char); + } + _ => { + use std::fmt::Write as _; + let _ = write!(out, "%{byte:02X}"); + } + } + } + out +} + +/// Every window's overlay kinds, in composition order, keyed by window. +fn overlay_kinds_per_window(state: &EditorState) -> Vec<(u64, Vec<&'static str>)> { + let core = state.core.borrow(); + let mut rows: Vec<(u64, Vec<&'static str>)> = core + .windows + .iter() + .map(|(id, w)| (id.raw(), w.overlay_kinds())) + .collect(); + rows.sort_by_key(|(id, _)| *id); + rows +} + +/// Count cells carrying a diagnostic **error** underline colour, per +/// window rect, by painting one real frame. +/// +/// This is the only per-window, view-level observation available: +/// `DiagnosticView.uri` is private and `View` has no downcast, so +/// asserting on the store would prove nothing about whether the overlay +/// was re-rooted. A view still pointing at the old URI renders nothing, +/// because `forget_uri` emptied that key. +fn error_underlines_per_window(state: &EditorState) -> Vec<(u64, usize)> { + use pmacs::cell::{Cell, CellGrid, CellSize, Color}; + use pmacs::protocol::FrontendId; + use pmacs::window::Rect; + + let size = CellSize::new(24, 80); + let mut cells = vec![Cell::default(); (size.rows * size.cols) as usize]; + let mut grid = CellGrid { + cells: &mut cells, + stride: size.cols, + size, + }; + pmacs::editor::paint_frame( + state, + FrontendId::LOCAL, + &std::collections::HashMap::new(), + &mut grid, + size, + ); + let placements = { + let core = state.core.borrow(); + let view = core.views.get(&FrontendId::LOCAL).expect("LOCAL view"); + let area = Rect::new(0, 0, size.rows - 1, size.cols); + let fixed = core.panel_fixed_rows(FrontendId::LOCAL, area.size.rows); + view.layout.compute(area, &fixed) + }; + let error = Color::Indexed(1); + let mut rows: Vec<(u64, usize)> = placements + .into_iter() + .map(|(win, rect)| { + let mut n = 0; + for row in rect.origin.row..rect.origin.row + rect.size.rows { + for col in rect.origin.col..rect.origin.col + rect.size.cols { + let idx = (row * size.cols + col) as usize; + if cells.get(idx).map(|c| c.style.underline_color) == Some(error) { + n += 1; + } + } + } + (win.raw(), n) + }) + .collect(); + rows.sort_by_key(|(id, _)| *id); + rows +} + +/// Acceptance 30. An attached LSP buffer with **diagnostics present +/// before** the rename, shown in **at least two windows**: afterwards +/// both windows render the **new** URI's diagnostics, the old URI's +/// store is empty, and each window's overlay keeps its **position in +/// the composition order**. +/// +/// Bite, two mutations: `rec.uri` updated without re-rooting the +/// diagnostic view (both windows then render nothing, because the old +/// key is empty); and a remove-and-re-push, which would pass a +/// one-window render test while moving the diagnostic overlay to the end +/// of the stack — caught by the composition-order assertion. +#[test] +fn acc30_diagnostics_re_root_in_every_window_and_keep_their_stack_position() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let old = fx.write( + "proj/src/main.rs", + "fn main() {}\n// second\n// third line here\n", + ); + let new = fx.at("proj/src/renamed.rs"); + let mut state = editor(); + state.sync_frame_geometry( + pmacs::protocol::FrontendId::LOCAL, + pmacs::cell::CellSize::new(24, 80), + ); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "B", &old); + + let old_uri = file_uri(&old); + let new_uri = file_uri(&new); + settle_until(&mut state, "diagnostics for the old URI", |s| { + diag_count(s, &old_uri) > 0 + }); + + // A second window showing the same buffer, with its own + // `DiagnosticView`. A split alone does not carry one — the view + // does not implement `clone_for_split` — so the switch hook is what + // attaches it, and that path only ever touches the ACTIVE window. + exec(&state, "pmacs.window.split_horizontal()"); + // `try_split_active` leaves focus where it was, so the switch hook — + // which can only reach the ACTIVE window — has to be given the new + // one explicitly. + exec(&state, "pmacs.window.focus_next()"); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + settle_a_while(&mut state); + + // Push one more overlay AFTER the diagnostic in each window. + // Without this the composition-order assertion below cannot bite: + // the LSP attach leaves `diagnostic` LAST in the stack, and moving + // the last element to the end is a no-op, so a remove-and-re-push + // would be indistinguishable from an in-place mutation. + // `_attach_highlight` uses `push_overlay` (no dedup), so a second + // call appends. + exec( + &state, + "pmacs.parse._attach_highlight(_G.B, pmacs.parse.buffer_language(_G.B))", + ); + exec(&state, "pmacs.window.focus_next()"); + exec( + &state, + "pmacs.parse._attach_highlight(_G.B, pmacs.parse.buffer_language(_G.B))", + ); + + let before_kinds = overlay_kinds_per_window(&state); + assert!( + before_kinds + .iter() + .all(|(_, kinds)| kinds.iter().position(|k| *k == "diagnostic") + < Some(kinds.len() - 1)), + "precondition: the diagnostic overlay must NOT be last, or \ + \"keeps its stack position\" is unfalsifiable; got {before_kinds:?}" + ); + let before_paint = error_underlines_per_window(&state); + assert_eq!( + before_paint.len(), + 2, + "precondition: two windows are placed; got {before_paint:?}" + ); + for (win, n) in &before_paint { + assert!( + *n > 0, + "precondition: window {win} must already paint diagnostic \ + underlines; got {before_paint:?} with overlays \ + {before_kinds:?}" + ); + } + let diag_positions_before: Vec<(u64, Option)> = before_kinds + .iter() + .map(|(w, kinds)| (*w, kinds.iter().position(|k| *k == "diagnostic"))) + .collect(); + assert!( + diag_positions_before.iter().all(|(_, p)| p.is_some()), + "precondition: every window carries a diagnostic overlay; got \ + {before_kinds:?}" + ); + + rename_fire_and_forget(&mut state, &old, &new); + settle_until(&mut state, "diagnostics for the new URI", |s| { + diag_count(s, &new_uri) > 0 + }); + settle_a_while(&mut state); + + assert_eq!( + diag_count(&state, &old_uri), + 0, + "the old URI's store must be empty" + ); + assert!( + diag_count(&state, &new_uri) > 0, + "and the new URI's must be populated" + ); + + let after_kinds = overlay_kinds_per_window(&state); + let diag_positions_after: Vec<(u64, Option)> = after_kinds + .iter() + .map(|(w, kinds)| (*w, kinds.iter().position(|k| *k == "diagnostic"))) + .collect(); + assert_eq!( + diag_positions_after, diag_positions_before, + "each window's diagnostic overlay must keep its position in the \ + composition order; a remove-and-re-push would move it to the end \ + ({before_kinds:?} -> {after_kinds:?})" + ); + + let after_paint = error_underlines_per_window(&state); + assert_eq!(after_paint.len(), 2, "still two windows: {after_paint:?}"); + for (win, n) in &after_paint { + assert!( + *n > 0, + "window {win} must render the NEW URI's diagnostics; 0 means \ + its overlay is still keyed under the old URI, whose store the \ + forget emptied ({after_paint:?})" + ); + } +} + +/// Acceptance 31c, at the Lua binding. Raises for an unknown server id; +/// **succeeds** for a URI with no state under a known server. +/// +/// The second arm is the one that matters: the `resource.renamed` +/// subscriber calls this per attachment, and an attachment need not have +/// any pending route or populated result store. An over-strict binding +/// would turn that ordinary idempotent case into an error inside a hook. +#[test] +fn acc31c_the_forget_uri_binding_raises_for_an_unknown_server_and_not_for_an_unknown_uri() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let file = fx.write("proj/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "B", &file); + settle_until(&mut state, "one live server", |s| server_count(s) == 1); + + // An unknown id has to be a real handle to a server the manager no + // longer holds: `LspServerIdLua` is opaque and cannot be forged from + // an integer, which is itself the binding's first line of defence. + let raised: String = eval( + &state, + "local sid + for _, row in ipairs(pmacs.lsp.list()) do sid = row.id end + assert(sid, 'no server to stale out') + _G.STALE = sid + pmacs.lsp.stop(sid) + return 'stopped'", + ); + assert_eq!(raised, "stopped"); + settle_until(&mut state, "the server is forgotten", |s| { + let gone: bool = eval( + s, + "local ok = pcall(pmacs.lsp.forget, _G.STALE) + return #pmacs.lsp.list() == 0", + ); + gone + }); + let raised: String = eval( + &state, + "local ok, err = pcall(pmacs.lsp.forget_uri, _G.STALE, 'file:///nope.rs') + if ok then return 'DID NOT RAISE' end + return tostring(err)", + ); + assert!( + raised.contains("unknown server"), + "an unknown server id must raise, matching `pmacs.lsp.forget`; got \ + {raised:?}" + ); + + // And the success arm, against a live server. + open_as( + &state, + "C", + &fx.write("proj/src/second.rs", "fn second() {}\n"), + ); + settle_until(&mut state, "a live server again", |s| server_count(s) == 1); + let ok: bool = eval( + &state, + "local sid + for _, row in ipairs(pmacs.lsp.list()) do sid = row.id end + local a = pcall(pmacs.lsp.forget_uri, sid, 'file:///never-opened.rs') + local b = pcall(pmacs.lsp.forget_uri, sid, 'file:///never-opened.rs') + return a and b", + ); + assert!( + ok, + "a URI with no state under a known server is an idempotent \ + success, and repeating it stays safe" + ); +} + +/// Acceptance 32. A rename **across project roots** re-runs +/// `ensure_server` and the buffer ends up attached to a **different** +/// server; a same-root rename reuses the existing one (#161's affinity +/// key is the detected project root). +#[test] +fn acc32_a_cross_root_rename_re_runs_ensure_server_and_a_same_root_one_reuses() { + // Same root first: renaming within one package must not spawn a + // second server. + let fx = Fixture::new(); + fx.write("a/Cargo.toml", "[package]\nname = \"a\"\n"); + let inside = fx.write("a/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "B", &inside); + settle_until(&mut state, "one initialized server", |s| { + server_rows(s) == vec![format!("rust|{}|initialized", file_uri(&fx.at("a")))] + }); + let root_a = file_uri(&fx.at("a")); + assert_eq!( + server_rows(&state), + vec![format!("rust|{root_a}|initialized")], + "precondition: one server, rooted at package a" + ); + + rename_fire_and_forget(&mut state, &inside, &fx.at("a/src/moved.rs")); + settle_a_while(&mut state); + assert_eq!( + server_count(&state), + 1, + "a same-root rename reuses the existing server: {:?}", + server_rows(&state) + ); + + // Now across roots: `b` is its own package, so its file needs its + // own server. + fx.write("b/Cargo.toml", "[package]\nname = \"b\"\n"); + std::fs::create_dir_all(fx.at("b/src")).unwrap(); + rename_fire_and_forget( + &mut state, + &fx.at("a/src/moved.rs"), + &fx.at("b/src/moved.rs"), + ); + settle_until(&mut state, "a second server for package b", |s| { + server_count(s) == 2 + }); + settle_a_while(&mut state); + + let root_b = file_uri(&fx.at("b")); + let rows = server_rows(&state); + assert!( + rows.iter().any(|r| r.contains(&root_b)), + "the cross-root rename must spawn a server rooted at package b; \ + rows were {rows:?}" + ); + assert_eq!( + rows.len(), + 2, + "exactly one new server, not one per reconciliation pass: {rows:?}" + ); +} + +/// Point the `rust` server at the `applyeditplan` fake carrying `plan`, +/// and hand back the sink the client's response to the server-initiated +/// `workspace/applyEdit` lands in. Must run before the first `.rs` file +/// is opened — that open is what launches the server. +fn plan_server(state: &EditorState, dir: &Path, plan: &serde_json::Value) -> PathBuf { + let plan_path = dir.join("plan.json"); + std::fs::write(&plan_path, serde_json::to_vec(plan).unwrap()).unwrap(); + let sink = dir.join("applyedit-response.json"); + exec( + state, + &format!( + "pmacs.lsp.config.rust = {{ + command = \"{}\", + env = {{ + PMACS_FAKE_LSP_MODE = 'applyeditplan', + PMACS_FAKE_LSP_EDIT_PLAN = '{}', + PMACS_FAKE_LSP_APPLYEDIT_SINK = '{}', + }}, + }}", + fake_lsp_path(), + plan_path.display(), + sink.display() + ), + ); + sink +} + +/// Ask the fake to deliver its planned `workspace/applyEdit`. Driven by +/// an `executeCommand` rather than fired at `initialized`, so the test +/// controls *when* the batch arrives — these fixtures depend on a +/// specific buffer being active first, and a server-timed request would +/// race that setup. +fn trigger_apply_edit(state: &EditorState) { + exec( + state, + "local sid + for _, row in ipairs(pmacs.lsp.list()) do + if row.state and row.state.kind == 'initialized' then sid = row.id end + end + assert(sid, 'no initialized server') + pmacs.lsp.request_execute_command(sid, 'pmacs.fake.applyEdit', {})", + ); +} + +fn wait_for_apply_response(state: &mut EditorState, sink: &Path) -> serde_json::Value { + let deadline = Instant::now() + Duration::from_secs(20); + loop { + state.tick_processes(); + state.tick_lsp(); + state.tick_async(); + if let Ok(raw) = std::fs::read(sink) + && let Ok(v) = serde_json::from_slice::(&raw) + { + assert!( + v.get("fakeError").is_none(), + "the fixture itself failed: {v:?}" + ); + return v; + } + assert!( + Instant::now() < deadline, + "the client never answered the server's workspace/applyEdit" + ); + std::thread::sleep(Duration::from_millis(10)); + } +} + +/// Acceptance 34. Renaming the **active** file through the full +/// `apply_workspace_edit` path returns the user to the **same buffer** +/// (now under its new path), and leaves no buffer bound to the obsolete +/// path. +/// +/// The plan deliberately edits *another* file first. Without that the +/// row cannot bite at all: the applier only has to restore the origin if +/// something moved the active buffer away, and a lone rename op does not. +/// +/// Bite: the applier restoring by path instead of by buffer handle. A +/// captured path no longer resolves after its own batch renamed it, so +/// `find_or_open` raises, the `pcall` swallows it, and the user is +/// stranded in whatever buffer the last applied op left active. No +/// reconciliation can reach a string a Lua local already captured. +/// +/// **One framing claim corrected here.** §5's G1 says the stale path +/// "materializes a phantom": that `find_or_open(origin)` reaches +/// `resolve_target_buffer`'s `NotFound` arm, which creates an empty +/// path-backed buffer and selects it. It does not. +/// `pmacs.buffer.find_or_open` (`src/lua_bindings/mod.rs`) calls +/// `crate::file_io::load_file` directly and maps the error, so a missing +/// path **raises**; the `NotFound` arm belongs to +/// `EditorCore::resolve_target_buffer`, which serves +/// `pmacs.window.display_file` and the startup/daemon target, not this +/// binding. The defect is real but smaller than G1 states — a silently +/// swallowed restore, not a fabricated file — and this row asserts the +/// half that is true. The no-buffer-at-the-old-path assertion is kept as +/// a cheap guard against a future fallback that *would* create one, and +/// is not the biting half. +#[test] +fn acc34_renaming_the_active_file_through_the_applier_returns_the_same_buffer() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let old = fx.write("proj/src/main.rs", "fn main() {}\n"); + let other = fx.write("proj/src/other.rs", "fn other() {}\n"); + let new = fx.at("proj/src/renamed.rs"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + let sink = plan_server( + &state, + &fx.root, + &serde_json::json!({ + "documentChanges": [ + { + // Moves the active buffer away, so the restore has + // something to undo. + "textDocument": { "uri": file_uri(&other), "version": 1 }, + "edits": [{ + "range": { + "start": { "line": 0, "character": 0 }, + "end": { "line": 0, "character": 0 }, + }, + "newText": "// touched\n", + }], + }, + { + "kind": "rename", + "oldUri": file_uri(&old), + "newUri": file_uri(&new), + }, + ], + }), + ); + open_as(&state, "B", &old); + settle_until(&mut state, "server initialized", |s| { + server_rows(s).iter().any(|r| r.ends_with("|initialized")) + }); + // The applier restores the buffer that was active when the batch + // began, so make that the file being renamed. + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + let active_before = state.core.borrow().active_buffer_id(); + + trigger_apply_edit(&state); + let response = wait_for_apply_response(&mut state, &sink); + assert_eq!( + response["result"]["applied"], true, + "the batch must apply: {response:?}" + ); + settle_a_while(&mut state); + + assert!(new.exists(), "the rename landed on disk"); + assert!(!old.exists()); + assert_eq!( + state.core.borrow().active_buffer_id(), + active_before, + "the user must be returned to the SAME buffer, now under its new \ + path — a path-based restore raises on the renamed-away path, the \ + pcall swallows it, and the user is left wherever the last applied \ + op put them" + ); + assert_eq!( + buffer_path(&state, "B").as_deref(), + Some(new.to_str().unwrap()), + "and that buffer's path followed the rename" + ); + + let stale: bool = eval( + &state, + &format!( + "for _, b in ipairs(pmacs.buffer.list()) do + if b:path() == \"{}\" then return true end + end + return false", + lua_str(&old) + ), + ); + assert!(!stale, "no buffer may remain bound to the obsolete path"); +} + +/// Acceptance 35. When the origin buffer is **gone** after the edit, the +/// applier restores **nothing** rather than falling back to the old +/// path. +/// +/// **The plan deletes the origin's file and then RECREATES it, and that +/// is what makes the row bite at all.** With a plain delete the forbidden +/// fallback is unobservable: `find_or_open` on a path that no longer +/// exists raises straight out of `file_io::load_file`, the surrounding +/// `pcall` swallows it, and nothing happens — so "no buffer at the old +/// path" and "the active buffer is live" both hold with the fallback +/// present. Recreating the path gives the fallback something to open, and +/// it is not a contrived shape: a `documentChanges` batch that deletes +/// and recreates a file is ordinary LSP refactoring output. +/// +/// Bite: the applier restoring by path instead of by buffer handle. The +/// handle is invalid (reconciliation killed the buffer) so a +/// handle-based restore does nothing; a path-based one loads the +/// recreated file into a NEW buffer and switches the user into it — +/// dropping them, silently, into a file they asked to delete. +#[test] +fn acc35_when_the_origin_buffer_is_gone_the_applier_restores_nothing() { + let fx = Fixture::new(); + fx.write("proj/Cargo.toml", "[package]\nname = \"p\"\n"); + let doomed = fx.write("proj/src/main.rs", "fn main() {}\n"); + let other = fx.write("proj/src/other.rs", "fn other() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + let sink = plan_server( + &state, + &fx.root, + &serde_json::json!({ + "documentChanges": [ + { + "kind": "delete", + "uri": file_uri(&doomed), + }, + { + // Recreates the path, so a path-based restore has a + // file to open and the fallback becomes observable. + "kind": "create", + "uri": file_uri(&doomed), + }, + ], + }), + ); + // `other` keeps the registry non-empty so the delete's kill is not + // refused for being the last buffer. + open_as(&state, "OTHER", &other); + open_as(&state, "B", &doomed); + settle_until(&mut state, "server initialized", |s| { + server_rows(s).iter().any(|r| r.ends_with("|initialized")) + }); + exec(&state, "pmacs.window.switch_buffer(_G.B)"); + + trigger_apply_edit(&state); + let response = wait_for_apply_response(&mut state, &sink); + assert_eq!( + response["result"]["applied"], true, + "the batch must apply: {response:?}" + ); + settle_a_while(&mut state); + + assert!( + doomed.exists(), + "precondition for the bite: the batch recreated the path, so a \ + path-based restore CAN open it" + ); + assert!( + !buffer_is_valid(&state, "B"), + "the origin buffer was reconciled away by the delete" + ); + + let reopened: bool = eval( + &state, + &format!( + "for _, b in ipairs(pmacs.buffer.list()) do + if b:path() == \"{}\" then return true end + end + return false", + lua_str(&doomed) + ), + ); + assert!( + !reopened, + "the applier must restore NOTHING. A path-based restore loads the \ + recreated file into a fresh buffer, which is the editor silently \ + re-opening a file the user asked to delete" + ); + + let active_path: Option = eval( + &state, + "local b = pmacs.window.buffer(); return b and b:path() or nil", + ); + assert_ne!( + active_path.as_deref(), + Some(doomed.to_str().unwrap()), + "and the user must not be sitting in it either" + ); + let active_valid = { + let core = state.core.borrow(); + let id = core.active_buffer_id(); + core.registry.borrow().contains(id) + }; + assert!( + active_valid, + "the window it left behind must still sit on a live buffer" + ); +} + +/// Review round 1 — a reconciliation failure inside the LSP subscriber +/// must be **reported and attributed**, and must not stop the remaining +/// attachments from reconciling. +/// +/// The scenario is the reviewer's: a **stale server id**. The attachment +/// record still names a server the manager has forgotten, so +/// `did_close` and `forget_uri` both raise. With ignored `pcall`s the +/// callback returned successfully, so the `all-must-succeed` hook logger +/// had nothing to log, and the old stores, routes and `documents` entry +/// stayed live under a URI the editor no longer held — silently. +/// +/// Two packages under one parent directory give two servers, and only +/// one is staled out, so the row can assert both halves at once: the +/// failure is surfaced, **and** the healthy attachment still moves. +/// +/// Bite: fails against ignored `pcall`s (nothing on either channel), and +/// against a fix that lets the first failure `error()` out of the loop +/// (the healthy attachment would never reconcile). +#[test] +fn a_subscriber_reconciliation_failure_is_reported_and_the_rest_still_reconcile() { + let fx = Fixture::new(); + fx.write("w/a/Cargo.toml", "[package]\nname = \"a\"\n"); + fx.write("w/b/Cargo.toml", "[package]\nname = \"b\"\n"); + let file_a = fx.write("w/a/src/main.rs", "fn main() {}\n"); + let file_b = fx.write("w/b/src/main.rs", "fn main() {}\n"); + let mut state = editor(); + configure_fake(&state, &fx.root, "rust"); + open_as(&state, "A", &file_a); + settle_until(&mut state, "server for package a", |s| server_count(s) == 1); + open_as(&state, "B", &file_b); + settle_until(&mut state, "server for package b", |s| server_count(s) == 2); + + // Stale out the server serving package `a` only: stop it, then forget + // it, leaving `attachments[a].server` naming a server the manager no + // longer holds. `forget_uri` raises for an unknown server id, which + // is exactly the failure mode under test. + let root_a = file_uri(&fx.at("w/a")); + exec( + &state, + &format!( + "local victim + for _, row in ipairs(pmacs.lsp.list()) do + if row.root_uri == \"{root_a}\" then victim = row.id end + end + assert(victim, 'no server rooted at package a') + pcall(pmacs.lsp.stop, victim) + _G.VICTIM = victim" + ), + ); + settle_until(&mut state, "the victim is forgotten", |s| { + let gone: bool = eval( + s, + "pcall(pmacs.lsp.forget, _G.VICTIM) + for _, row in ipairs(pmacs.lsp.list()) do + if row.id == _G.VICTIM then return false end + end + return true", + ); + gone + }); + exec(&state, "pmacs.editor.set_status('')"); + + // Rename the parent, so BOTH attachments are in the fan-out. + let new_uri_b = file_uri(&fx.at("w2/b/src/main.rs")); + rename_fire_and_forget(&mut state, &fx.at("w"), &fx.at("w2")); + settle_a_while(&mut state); + + // Half one: the failure is surfaced, on both channels, attributed to + // the operation that failed. + let said = status(&state); + assert!( + said.contains("resource.renamed") && said.contains("reconciliation failure"), + "the failure must reach the status channel; status was {said:?}" + ); + assert!( + said.contains("forget_uri"), + "and must name WHICH step failed — an unattributed count does not \ + tell anyone that the URI-keyed stores were left live; status was \ + {said:?}" + ); + + let errors: String = eval( + &state, + "for _, b in ipairs(pmacs.buffer.list()) do + if b:name() == '*errors*' then return b:slice(0, b:len()) end + end + return ''", + ); + assert!( + errors.contains("resource.renamed") && errors.contains("forget_uri"), + "the callback must RAISE, so the all-must-succeed hook logger has \ + something to record; *errors* held {errors:?}" + ); + + // Half two: the healthy attachment still reconciled. The fake + // republishes diagnostics on every `didOpen`, so diagnostics under + // the NEW uri prove the whole ordered teardown ran for package b + // after package a's failed. + settle_until(&mut state, "package b reattached at its new uri", |s| { + diag_count(s, &new_uri_b) > 0 + }); + assert!( + diag_count(&state, &new_uri_b) > 0, + "one unreachable server must not leave every other attachment \ + unreconciled — the raise has to come after the loop, not inside it" + ); +}