From 9691dd4e9f3bd220d18a190448c96295ad06c231 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 23 Jul 2026 13:42:20 -0400 Subject: [PATCH] fix(fold): address PR #142 review round 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - **Finding 1 (bug):** a delete starting exactly at a fold's `end` removed the `\n` that `end` names — the last hidden line's terminator — but the strictly-after arm (`os >= e`) kept the fold, leaving a mid-line end. `translate`'s after-arm is now `os > e || (os == e && old_len == 0)` so a pure insert at `e` still stays outside while a delete at `e` falls to the drop arm, symmetric with the head side. New unit test `delete_starting_at_tail_boundary_drops_fold` (bite-verified). - **Finding 2:** `pmacs.buffer.kill` didn't clean the fold registry. Added `FoldRegistry::forget_buffer(id)` (id-keyed; the view died with the buffer) and wired it into `after_buffer_removed`, mirroring the keymap/config cleanup; the registry is now stashed as Lua app-data. `forget(&mut Buffer)` is clarified as the revert/reload reset. - **Finding 3:** `fold.close-all` now moves the invoking point to the head when it closes a fold around it (Q#FD3); the data-API `fold` exemption (programmatic, no invoking point) is named in the module doc. - Nits: dropped the dead `!(both empty)` conjunct in `fold_state_msg`; replaced the trivial fresh-registry assert; added coverage for the stale-tree refuse via a fold command, the read-only-buffer rejection (Q#FD11), and unfold normalizing an arbitrary range. Gates green: fmt, clippy --workspace --all-targets, --lib (1786), --features crdt (1962), folding_acceptance (24), m4 (skip basedpyright), required-GPU, git diff --check. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Q5BkezMppbpCgGAYk2ftxV --- src/fold.rs | 40 +++++++++++++++-- src/lua_bindings/fold.rs | 31 ++++++++----- src/lua_bindings/mod.rs | 3 ++ src/semantic_render.rs | 6 +-- tests/folding_acceptance.rs | 89 +++++++++++++++++++++++++++++++++++-- 5 files changed, 148 insertions(+), 21 deletions(-) diff --git a/src/fold.rs b/src/fold.rs index 3772b34..7e0140e 100644 --- a/src/fold.rs +++ b/src/fold.rs @@ -231,8 +231,12 @@ impl FoldStore { start: shift(s), end: shift(e), }) - } else if os >= e { - // Strictly after the fold (an insert at exactly `e` too). + } else if os > e || (os == e && old_len == 0) { + // Strictly after the fold, OR a pure insert at exactly `e` + // (left outside). A *delete* starting at `e` removes the + // `\n` that `e` names — the terminator of the last hidden + // line — so it destroys the tail and falls through to the + // drop arm below, symmetric with the head side. Some(ByteRange { start: s, end: e }) } else if os > s && oe < e { // Strictly inside the interior — the fold still hides a @@ -348,14 +352,24 @@ impl FoldRegistry { } /// Drop the buffer's store and detach its translator view — the - /// content-replacement (revert/reload) and buffer-close reset. Named - /// bytes no longer exist, so revalidation is not attempted (Q#FD8). + /// content-replacement (revert/reload) reset, where the buffer survives + /// but its bytes are replaced wholesale, so the view must come off too + /// (a later fold re-attaches a fresh one). Named bytes no longer exist, + /// so revalidation is not attempted (Q#FD8, framing acceptance 8). pub fn forget(&self, buffer: &mut Buffer) { if let Some(entry) = self.stores.borrow_mut().remove(&buffer.id()) { buffer.detach_view(entry.view); } } + /// Drop the store for a buffer that has already been removed (the + /// `pmacs.buffer.kill` path). The buffer — and its attached translator + /// view — is gone, so only the map entry needs clearing; there is no + /// view to detach. + pub fn forget_buffer(&self, buf: BufferId) { + self.stores.borrow_mut().remove(&buf); + } + /// Unfold every fold in `buf` containing `p`. The pre-edit hook the /// six `EditorCore` edit primitives call; a no-op when the buffer has /// no store (hence no folds). Returns the count unfolded. @@ -732,6 +746,24 @@ mod tests { assert!(store.is_empty()); } + #[test] + fn delete_starting_at_tail_boundary_drops_fold() { + // A delete beginning exactly at `end` removes the `\n` that `end` + // names (the last hidden line's terminator), destroying the tail — + // it must drop, not survive with a mid-line end. Mirror of + // `insert_at_tail_boundary_leaves_fold_untouched` for a delete. + let mut store = FoldStore::new(); + store.insert(r(10, 30)); + store.translate(&edit(Range::new(30, 31), 0)); // delete one byte at `end` + assert!(store.is_empty()); + + // Same class: a delete starting at the boundary and extending past. + let mut store = FoldStore::new(); + store.insert(r(10, 30)); + store.translate(&edit(Range::new(30, 45), 0)); + assert!(store.is_empty()); + } + #[test] fn containment_is_start_exclusive_end_inclusive() { let store = { diff --git a/src/lua_bindings/fold.rs b/src/lua_bindings/fold.rs index b79c2c7..0d4664c 100644 --- a/src/lua_bindings/fold.rs +++ b/src/lua_bindings/fold.rs @@ -23,8 +23,11 @@ //! Fold *creation* refuses against an absent or stale parse tree (Q#FD10) //! and validates the buffer kind / UTF-8 boundaries / >= 1-hidden-line //! rule (Q#FD11); a rejection reports on the status line and returns -//! `false`. Folding a range containing the invoking frontend's point moves -//! that point to the head line (Q#FD3). +//! `false`. The **interactive** commands (`toggle`/`close`/`cycle`/ +//! `close_all`) move the invoking frontend's point to the head line when +//! they fold a range around it (Q#FD3); the **data-API** `fold` is +//! deliberately exempt — a programmatic caller names an explicit buffer +//! and range with no invoking point to relocate. use std::sync::{Arc, Mutex}; @@ -45,6 +48,10 @@ use crate::syntax::{ParseTreeBundle, SharedSyntaxRegistry}; mirroring install_config; splitting fragments the wiring" )] pub fn install_fold(lua: &Lua, fold_registry: &SharedFoldRegistry) -> mlua::Result<()> { + // Also stash the registry as app-data so the buffer-remove cleanup + // (`after_buffer_removed`) can drop a killed buffer's store, mirroring + // the keymap/config registries. + lua.set_app_data(fold_registry.clone()); let fold_mod = lua.create_table()?; // ---- data API --------------------------------------------------------- @@ -99,8 +106,7 @@ pub fn install_fold(lua: &Lua, fold_registry: &SharedFoldRegistry) -> mlua::Resu return Ok(true); } if let Ok(Some(bytes)) = document_bytes(lua, id) - && let Some(normalized) = - fold::normalize_arbitrary_range(&bytes, requested) + && let Some(normalized) = fold::normalize_arbitrary_range(&bytes, requested) { return Ok(lock(&store).remove(normalized)); } @@ -242,14 +248,17 @@ pub fn install_fold(lua: &Lua, fold_registry: &SharedFoldRegistry) -> mlua::Resu }; let targets = fold::top_level_fold_targets(&bundle); let store = store_for(lua, ®, id)?; - let mut s = lock(&store); - let mut n = 0i64; - for t in targets { - if s.insert(t) { - n += 1; - } + let inserted: Vec = { + let mut s = lock(&store); + targets.into_iter().filter(|t| s.insert(*t)).collect() + }; + // close-all is interactive: if a newly-closed top-level + // fold contains the invoking point, move it to the head + // (Q#FD3). At most one top-level fold can contain it. + for r in &inserted { + maybe_move_point(lua, id, *r); } - Ok(n) + Ok(i64::try_from(inserted.len()).unwrap_or(i64::MAX)) })?, )?; } diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 02dd286..7e84899 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -1466,6 +1466,9 @@ fn after_buffer_removed(lua: &Lua, id: BufferId) { if let Some(config) = lua.app_data_ref::() { config.borrow_mut().remove_buffer(id); } + if let Some(folds) = lua.app_data_ref::() { + folds.forget_buffer(id); + } let callbacks = match lua.app_data_ref::() { Some(callbacks) => callbacks.take(id), None => Vec::new(), diff --git a/src/semantic_render.rs b/src/semantic_render.rs index 25e7fea..65750d3 100644 --- a/src/semantic_render.rs +++ b/src/semantic_render.rs @@ -1434,9 +1434,9 @@ impl SemanticRenderState { let should_emit = match self.last_folds.get(&buffer_id) { // First sight: speak only if there is a fold to show. None => !folds.is_empty(), - // A real change; `empty → empty` conveys nothing and is - // suppressed, `non-empty → empty` is a change worth sending. - Some(prev) => *prev != folds && !(folds.is_empty() && prev.is_empty()), + // Any change: `empty → empty` is byte-identical and suppressed, + // `non-empty → empty` differs and emits one clearing frame. + Some(prev) => *prev != folds, }; if !should_emit { return None; diff --git a/tests/folding_acceptance.rs b/tests/folding_acceptance.rs index 1e882c5..3f9977e 100644 --- a/tests/folding_acceptance.rs +++ b/tests/folding_acceptance.rs @@ -13,8 +13,7 @@ use std::sync::Arc; use pmacs::buffer::{Buffer, BufferId, EditOp}; use pmacs::editor::EditorState; use pmacs::fold::{ - self, CycleOutcome, FoldStore, close_at, cycle_at, fold_target_at, open_at, - top_level_fold_targets, + CycleOutcome, FoldStore, close_at, cycle_at, fold_target_at, open_at, top_level_fold_targets, }; use pmacs::protocol::ByteRange; use pmacs::syntax::{ParseTreeBundle, ParseView, SyntaxRegistry, run_parse}; @@ -487,5 +486,89 @@ fn forget_drops_store_and_detaches_view() { s.fold_registry.forget(reg.get_mut(id).unwrap()); } assert!(s.fold_registry.store(id).is_none(), "the store is dropped"); - assert!(fold::make_shared_fold_registry().folds(id).is_empty()); + assert!( + s.fold_registry.folds(id).is_empty(), + "no folds survive the drop" + ); +} + +#[test] +fn forget_buffer_drops_store_on_kill() { + // The id-keyed reset the pmacs.buffer.kill path uses: the buffer (and + // its attached view) is gone, so only the map entry is cleared. + let s = EditorState::new(); + let id = active_id(&s); + insert_into(&s, id, "line0\nline1\nline2\n"); + { + let core = s.core.borrow(); + let mut reg = core.registry.borrow_mut(); + s.fold_registry + .store_or_attach(reg.get_mut(id).unwrap()) + .lock() + .unwrap() + .insert(ByteRange { start: 5, end: 11 }); + } + assert!(s.fold_registry.store(id).is_some()); + s.fold_registry.forget_buffer(id); + assert!(s.fold_registry.store(id).is_none()); +} + +#[test] +fn stale_parse_tree_refuses_fold() { + // Q#FD10: an edit after the parse leaves `pending_edit_count() > 0`, + // so a fold command refuses (the settled coordinates are stale). + let s = EditorState::new(); + let id = active_id(&s); + let src = "fn foo() {\n let x = 1;\n let y = 2;\n}\n"; + insert_into(&s, id, src); + install_rust_parse(&s, id); + // A further edit accumulates a pending edit on the attached parse view. + insert_into(&s, id, "// stale\n"); + s.core.borrow_mut().set_cursor_byte(20); + + exec(&s, "pmacs.command.invoke('fold.close')"); + let n: i64 = eval(&s, "return #pmacs.fold.folds(pmacs.window.buffer())"); + assert_eq!(n, 0, "a stale parse tree refuses fold creation"); +} + +#[test] +fn read_only_buffer_is_rejected() { + // Q#FD11's "normal document buffer" guard: terminals are read-only, so + // a read-only buffer is not foldable. + let s = EditorState::new(); + exec( + &s, + "b = pmacs.buffer.from_bytes('ro.rs', 'aaa\\nbbb\\nccc\\n')", + ); + let id = { + let core = s.core.borrow(); + let reg = core.registry.borrow(); + reg.find_by_name("ro.rs").expect("buffer") + }; + { + let core = s.core.borrow(); + let mut reg = core.registry.borrow_mut(); + reg.get_mut(id).unwrap().set_read_only(true); + } + let ok: bool = eval(&s, "return pmacs.fold.fold(b, { start = 3, ['end'] = 7 })"); + assert!(!ok, "a read-only buffer is rejected (Q#FD11)"); +} + +#[test] +fn unfold_normalizes_an_arbitrary_range_to_a_stored_fold() { + let s = EditorState::new(); + exec( + &s, + "b = pmacs.buffer.from_bytes('doc.rs', 'aaa\\nbbb\\nccc\\nddd\\n')", + ); + let folded: bool = eval(&s, "return pmacs.fold.fold(b, { start = 3, ['end'] = 11 })"); + assert!(folded); + // A *different* input range that normalizes to the same stored fold. + let unfolded: bool = eval( + &s, + "return pmacs.fold.unfold(b, { start = 1, ['end'] = 10 })", + ); + assert!(unfolded, "unfold normalizes an arbitrary range"); + let n: i64 = eval(&s, "return #pmacs.fold.folds(b)"); + assert_eq!(n, 0); }