From 3bde76126afc4cc01d369472d1f0909d24a86b76 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 21 Jul 2026 13:50:34 -0400 Subject: [PATCH 1/4] docs(config): framing for the config registry (revision 2) The cross-cutting substrate ranked first on the side-quest north star. A third registry beside CommandRegistry and HookRegistry, with two scopes (global and buffer-local), a closed scalar value vocabulary, post-commit listeners, and no wire surface. Ground truth established by scout at 7bc0c61, including three findings that shaped the design: * Tab width is five constants across two crates with two different values -- four TAB_WIDTH = 8 in the daemon, 4 in the GPU minimap, and no tab expansion at all on the GPU main text path. It is a rendering-parity bug, not a config gap, so it is deferred to stage 2 rather than made the proving adopter. * The mode system is unwired: every editor KeymapStack::resolve passes an empty active-modes slice. A mode scope would ship a knob that silently never fires. * load_user_config and set_init_complete both run inside EditorState::new under one cfg(not(test)) block, so the StartupOnly freeze point covers the daemon and local entry points uniformly -- and never runs in --lib test builds. Revision 2 records review round 1 (F1-F11). The load-bearing fix is F1: an override is now always stored, with only value_epoch and listener dispatch keyed on effective-value change. The prior "equal-value set is a true no-op" reading meant an equal-valued buffer-local override stored nothing, so a later global set would flip the very buffer the user had pinned -- silently voiding the per-buffer toggle this arc exists to deliver. Also carried by revision 2: listener dispatch semantics pinned across scopes, GC-collected listeners dropped (no MetaMethod::Gc precedent exists), migration wrappers keep their legacy coercion, StartupOnly x set_local rejected at define time, string-list dropped, describe's `local` field renamed buffer_local (Lua keyword), and the direct-remove leak corrected to permanent-but-non-aliasing since BufferIds are never reused. No implementation. Framing only. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/config-registry-framing.md | 904 ++++++++++++++++++++++++++++++++ 1 file changed, 904 insertions(+) create mode 100644 docs/config-registry-framing.md diff --git a/docs/config-registry-framing.md b/docs/config-registry-framing.md new file mode 100644 index 0000000..476d25a --- /dev/null +++ b/docs/config-registry-framing.md @@ -0,0 +1,904 @@ +# Config registry — framing (cross-cutting substrate) + +pmacs has no unified configuration surface. Every setting that exists +today invented its own shape: a getter-when-nil function here, a raw +mutable Lua table there, a window field, a Rust preference struct with +its own epoch counter and wire channel. Nothing is discoverable, nothing +is validated centrally, and there is no way at all to say "this setting, +but only in this buffer." + +That gap is the named blocker on five separate backlog items — the +per-buffer auto-pair toggle, language-aware indent, per-language comment +padding, per-project compile commands, and the tab-width duplication — +and `docs/side-quest-backlog.md` ranks it first on the north star for +exactly that reason. + +This framing proposes a third registry alongside `CommandRegistry` and +`HookRegistry`, built to the rules those two already enforce, with two +scopes (global and buffer-local) and no wire surface. + +Backlog: `docs/side-quest-backlog.md` — "Cross-cutting substrate", +north-star item 1. Not a numbered roadmap arc; it is the substrate the +roadmap keeps tripping over. + +**Revision 1 — 2026-07-21.** Supersedes a withdrawn same-day draft. +Ideas carried forward from it, credited where they land: the +post-commit listener API with disposable handles (Q#CR6), `Live` vs +`StartupOnly` mutability (Q#CR10), owned-value/deep-copy storage +(Q#CR3), the two-epoch counters (Q#CR2), strict raw-table spec +validation (Q#CR3), and the LuaJIT-vs-lua54 integer-exactness +requirement (acceptance 6). Not carried forward, with reasons inline: +global-only scoping (Q#CR4), `editor.tab_width` as the proving adopter +(Q#CR13), snake_case names (Q#CR9), and Lua bindings inside +`lua_bindings/mod.rs` (Q#CR2). + +**Revision 2 — 2026-07-21, review round 1.** Findings F1–F11. +- **F1 (the one that mattered):** "equal-value set is a true no-op" + contradicted `is_set`, and under the naive reading an equal-valued + buffer-local override stored nothing — so a later global set would + flip the very buffer the user had pinned, silently breaking the + flagship feature. Overrides are now **always stored**; only + `value_epoch` and listener dispatch key on effective-value change + (Q#CR2, Q#CR4, acceptance 11). +- **F2:** listener dispatch semantics pinned across scopes (Q#CR6). +- **F3:** GC-collected listeners dropped — no `MetaMethod::Gc` exists + anywhere in `mod.rs` (verified: zero matches), the compile-mode + precedent is explicit-dispose only, and GC timing differs across the + two Lua backends (Q#CR6). +- **F4:** the migration wrappers keep their legacy coercion (Q#CR8). +- **F5:** `StartupOnly` × `set_local` resolved — the combination is + rejected at define time (Q#CR10). +- **F6:** `string-list` dropped from the stage-1 vocabulary (Q#CR3). +- **F7:** `describe`'s `local` field renamed `buffer_local` — `local` + is a Lua keyword (Q#CR11). +- **F8:** the direct-remove leak is permanent-but-bounded with no + aliasing hazard, and now has an acceptance item (Q#CR5). +- **F9:** `get(name)` with no buffer resolves the global chain only + (Q#CR4). +- **F10:** builtin defines moved to their owning modules (Q#CR14). +- **F11:** all stale line anchors corrected against `7bc0c61`. +- **Bet 6 confirmed and retired** into ground truth, with a correction + to this doc's own "Init ordering" claim. + +--- + +## Ground truth (as of `7bc0c61`) + +### `src/config.rs` is not a config registry + +Despite the name, `src/config.rs` (274 lines) is the `init.lua` *loader* +and nothing else: XDG config-dir resolution (`user_config_dir`, +`resolve_config_dir`), a `package.path` prepend so a user's config can +span files (`install_package_path`), and a non-fatal eval +(`load_user_config_at` — missing file, unreadable file, parse error and +runtime error are all survivable by contract). It stores no settings and +knows no setting names. The name is taken; that is a naming problem for +the new module, not a design one. + +### The settings zoo — nine shapes, none of them shared + +Every one of these is a real user-facing configuration surface at +`7bc0c61`. They agree on nothing: + +| Surface | Shape | Storage | Validated? | +| --- | --- | --- | --- | +| `pmacs.async_config.frame_target_ms(ms)` (`async.lua:458`) | getter-when-nil fn | module-local upvalue | ad-hoc | +| `pmacs.async_config.default_max_batch(n)` (`async.lua:468`) | getter-when-nil fn | module-local upvalue | ad-hoc | +| `pmacs.autosave.interval_ms(ms)` (`autosave.lua:42`) | getter-when-nil fn | module-local upvalue | hand-rolled floor + NaN check | +| `pmacs.killring.max(n)` (`killring.lua:65`) | getter-when-nil fn | module-local upvalue | ad-hoc | +| `pmacs.editops.trim_on_save(on)` (`editops.lua:844`) | getter-when-nil fn | module-local upvalue | none | +| `pmacs.autosave.enable` (`autosave.lua:34`), `pmacs.recentf.enable` (`recentf.lua:21`), `pmacs.saveplace.enable` (`saveplace.lua:21`), `pmacs.session.desktop_mode` (`desktop.lua:19`) | boolean fn | module-local upvalue | `on ~= false` | +| `pmacs.lsp.config`, `pmacs.lsp.filetypes`, `pmacs.comment.strings`, `pmacs.pair.sets` (`pair.lua:40`) | raw mutable table | plain Lua | at read time, per-consumer | +| `pmacs.parse.shebangs` / `.filenames` / `.injection_aliases` (`syntax.lua:55`) | write-through proxy | Rust registry behind a proxy | at write | +| `pmacs.theme.set/merge`, `pmacs.gpu.set_font` (`font_pref.rs`), `pmacs.window.set_line_numbers` (`mod.rs:11254`), `pmacs.statusline.register` (`statusline.rs`) | Rust binding | Rust struct + epoch + wire channel | at the binding | + +Consequences worth stating plainly: no enumeration (you cannot ask what +is configurable), no `describe`, no type discipline, no change +notification, and **no scoping** — every entry above is global except +`line_numbers`, which is window-local because a window happened to be +the convenient place to hang it. + +### Two registries already do this correctly + +`src/command.rs` (320 lines) and `src/hook.rs` (627 lines) are the +precedent, and they agree with each other: + +- `HashMap` by name plus a `Vec` insertion order for + stable listing. +- **R42** — a non-empty `description` is mandatory at define time + (`CommandError::MissingDescription`, `HookError::MissingDescription`). +- **R50** — spec-table keys are checked against a closed set, so a typo + is an error, not a silent no-op (`UnknownField`, carrying the + supported-key list in the message). +- Duplicate names are rejected, never silently overwritten — "silent + overwrite makes refactoring bugs invisible" (`command.rs:12`). +- `SourceLocation { file, line }` captured from Lua debug info at + registration, surfaced verbatim by `pmacs.describe.*`. +- Both live behind `Rc>` as Lua app data + (`SharedCommandRegistry`, `SharedHookRegistry`, `mod.rs:2265-2271`). +- `HookRegistry::snapshot` exists specifically so the caller can drop + the registry borrow before running user code that may re-enter. + +A settings registry that does not look like these two would be the odd +one out for no reason. + +### Scoping: what exists, what does not + +- **Window-local** exists exactly once, ad-hoc: + `core.active_window_mut().line_numbers` (`mod.rs:11254`). +- **Buffer-keyed side tables** are established: + `BufferRemoveCallbacks` is a `HashMap>` + (`mod.rs:133-176`); `KeymapStack` keys per-buffer maps the same way. +- **`Buffer` has no property bag.** The struct (`buffer.rs:158`) is + rope, name, revision, views, marks, undo/redo, path, file meta, the + in-flight-edit flag, and optional CRDT state. There is nowhere to put + a setting, and it is the type carrying undo and CRDT invariants. +- **Buffer death has one choke point**, `after_buffer_removed` + (`mod.rs:1458`), which already drops that buffer's keymaps before + firing remove callbacks. Three call sites route through it + (`mod.rs:3033`, `3123`, `5014`). +- **`BufferId`s are never reused** — allocated from a global counter and + documented unique (`buffer_registry.rs:82-84`). Any leaked per-buffer + state is permanent-but-bounded and can never alias onto a future + buffer. +- **The mode system is unwired.** Of 33 `.resolve(` sites in `src/`, + every `KeymapStack::resolve` in the editor passes `&[]` for + `active_modes`; the only non-empty callers are two unit tests inside + `keymap_stack.rs`. Mode-scoped anything is unavailable today. +- **Project detection exists**: `pmacs.project.detect(path)` → + `{root, kind}` (`mod.rs:10360`). +- **Language resolution exists**: `buffer_language` (`lsp.lua:471`), + exported as `pmacs.lsp.buffer_language` (`lsp.lua:494`), added by + auto-pairing (#110). + +### Init ordering, and the freeze point (corrected in revision 2) + +Revision 1 said `init.lua` runs "from `load_user_config` on the real +entry points only." **That was wrong** — it described `install_state_dirs` +(`editor.rs:551`), the Arc 3 pattern, not user config. The truth: + +Builtin runtime chunks are evaluated inside `EditorState::new()` +(`editor.rs:194-408`, the linear bootstrap). At the tail of the *same* +function, under a single `#[cfg(not(test))]` block (`editor.rs:465-476`): + +```rust +#[cfg(not(test))] +{ + crate::config::load_user_config(&mut lua_host); + lua_host.set_init_complete(); +} +``` + +Three consequences the design depends on: + +1. **Builtins define, the user sets, the flag freezes** — one ordered + sequence, no new machinery needed. +2. Because it lives in `new()` itself, the freeze covers the daemon + entry (`daemon.rs:468`) and the local one uniformly. This retires + revision 1's bet 6: the `StartupOnly` freeze point exists and is + `InitCompleteFlag` (`mod.rs:2270`), consumed by `require_init_phase` + (`mod.rs:663`). +3. **In `--lib` test builds neither line runs.** User config is never + loaded and the flag never flips. Any acceptance test asserting + post-freeze behavior must flip it explicitly, the way + `mod.rs:14655-14690` already does; otherwise it passes vacuously. + +### Both Lua backends ship + +`Cargo.toml:75-77` — `default = ["luajit"]`, with `lua54` a supported, +mutually exclusive fallback (audit F-002 pins the +`--no-default-features --features lua54` build). LuaJIT is Lua 5.1 +semantics and has no native integer subtype; lua54 does. Any numeric +validation must therefore behave identically under both feature +selections and cannot rely on `math.type`. The same caution kills two +other tempting designs: GC-timed listener lifetime (Q#CR6) and `#` on a +holey array (Q#CR3). + +### No GC-cleanup precedent exists + +`mod.rs` contains **zero** `MetaMethod::Gc` implementations. The only +`dispose` binding is the explicit one at `mod.rs:1867` (the compile-mode +overlay handle). Resource lifetime in this codebase is +explicit-dispose, never finalizer-driven. + +### Tab width — the motivating example is worse than advertised + +The backlog calls this "the five hardcoded tab-width sites." The scout +found five sites across **two crates with two different values**: + +- `const TAB_WIDTH: u32 = 8` in `src/text_view.rs:35`, + `src/highlight.rs:321`, `src/diag.rs:393`, `src/completion.rs:594` — + four independent copies of the same constant and the same + `col += TAB_WIDTH - (col % TAB_WIDTH)` expansion. +- `pmacs-gpu/src/main.rs:6728-6733` — `advance_minimap_col` expands a + tab to **4**, not 8. + +And the GPU's main text path expands nothing at all: buffer text reaches +the frontend as raw bytes via `BufferSnapshot`/`CrdtOp`, so a literal +`\t` is handed to glyphon and shaped by the font. (The only two `'\t'` +sites in `pmacs-gpu/src/main.rs` are both in the minimap.) Tab rendering +is already inconsistent between the two frontends *before* any setting +exists. This is a rendering-parity bug wearing a config-shaped hat, and +it drives Q#CR13. + +--- + +## Non-goals + +No settings GUI. No second config-file format. No persistence outside +`init.lua`. No per-project trust or loading policy. No automatic +migration of every existing setter. No protocol messages and no +frontend-local pixel/font settings. No filesystem watching or hot reload +of `init.lua`. + +--- + +## Decisions + +### Q#CR1 — Scope: substrate, two scopes, three consumers, no wire + +Stage 1 delivers the registry, the Lua surface, buffer-local scoping, +discovery, and exactly three first consumers (Q#CR8). It does **not** +deliver persistence, a `customize` UI, a settings panel, tab width, +per-language or per-project scopes, or any protocol change. One feature, +one branch, one PR. + +The deliverable that justifies the PR on its own: the named backlog item +"per-buffer auto-pair toggle (config-registry-blocked)" stops being +blocked. + +### Q#CR2 — `src/config_registry.rs`: a third registry, Rust-owned + +New module `src/config_registry.rs` — not `config.rs`, which the +`init.lua` loader owns. Shape mirrors `hook.rs`: + +```rust +pub struct ConfigDefinition { + pub name: String, + pub description: String, // R42, mandatory + pub kind: ConfigKind, + pub default: ConfigValue, + pub mutability: ConfigMutability, // Live | StartupOnly + pub source: SourceLocation, +} + +pub struct ConfigListener { + id: u64, // generation-safe, never reused + name: String, + body: Function, + source: SourceLocation, +} + +pub struct ConfigRegistry { + by_name: HashMap, + order: Vec, + global: HashMap, // overrides only + locals: HashMap>, + listeners: Vec, // registration order + next_listener_id: u64, + frozen: bool, // Q#CR10 + definition_epoch: u64, + value_epoch: u64, +} +``` + +**Override storage versus epoch advancement are two different questions +(F1).** Revision 1 conflated them and broke the flagship feature. The +rule: + +- **An override is always stored**, even when it equals the value it + shadows. `set` and `set_local` unconditionally record an entry, which + is what makes `is_set` meaningful (Q#CR4) and what makes a + buffer-local *pin* actually pin. +- **`value_epoch` advances, and listeners fire, only when an effective + value changes.** Storing an override equal to the current effective + value is observationally silent but not structurally absent. + +The failure this prevents: with `editing.auto-pair` globally `true`, a +user calls `set_local(buf, "editing.auto-pair", true)` to pin that +buffer, then later `set("editing.auto-pair", false)` globally. Under +"true no-op" the local was never stored and the pinned buffer flips — +the pin silently never existed. Acceptance 11 pins this, bite-verified. + +`definition_epoch` advances when a key is defined; `value_epoch` gives +future render-path consumers a single `u64` to gate on, mirroring the +split syntax/face epochs from #120 (Q#TH6). + +`ConfigError` carries `EmptyName`, `MissingDescription` (R42), +`DuplicateName`, `NotFound`, `UnknownField` (R50, listing supported +keys), `TypeMismatch { name, expected, got }`, `OutOfRange`, +`NotAChoice { name, got, choices }`, `StartupOnlyLocal` (Q#CR10), and +`StartupOnlyAfterFreeze`. Behind +`SharedConfigRegistry = Rc>` as Lua app data, +set beside the other five at `mod.rs:2265-2271`. + +**Rust-owned, not a Lua table**, for two reasons. Rust consumers must +read a setting without borrowing Lua (the tab-width sites in Q#CR13 are +the eventual proof). And duplicate/typo/type rejection must be enforced +somewhere a user's `init.lua` cannot bypass — the argument +`command.rs:12` already makes. + +**Bindings go in a new `src/lua_bindings/config.rs`, not in `mod.rs`.** +The withdrawn draft proposed an "owned section of +`src/lua_bindings/mod.rs`"; that file is 15,594 lines, the paused F-016 +split lives there, and the concurrent vterm lane names it as its own +overlap file. A submodule reduces the shared-file footprint to one `mod` +line plus one `install_config(...)` call and removes almost all of the +inter-lane conflict surface. + +### Q#CR3 — Closed value vocabulary, owned data, strict specs + +``` +boolean | integer | number | string | enum +``` + +`string-list` is **dropped** from stage 1 (F6). No stage-1 adopter wants +it, and "copied densely from a 1-based array" does not say what happens +to a table with holes — `#` on a holey table returns an arbitrary +border, with no guarantee LuaJIT and lua54 choose the same one. +Admitting it means either explicit hole-rejection logic or a +cross-backend nondeterminism vector that acceptance 6 would have to +chase. It returns with its first real adopter and an explicit hole rule. + +`ConfigValue` is owned Rust data. Lua tables, functions and userdata are +never stored. Metadata returned to Lua (`describe`, `list`) is a fresh +table each call, never a handle onto registry state. Definition specs +are strict raw tables: unknown fields, missing fields, +metatable-provided values, wrong types, non-finite bounds, inverted +ranges, duplicate enum choices, and a default that violates its own +contract all reject **before** any mutation. Defaults pass the same +validator as user values. + +Numbers must be finite; integers must be exact — checked by value, not +by `math.type`, per the two-backend ground truth. `integer` and `number` +carry optional `min`/`max`. + +Tables as *values* stay out. Table-valued configuration already has a +working home — `pmacs.lsp.config`, `pmacs.pair.sets`, +`pmacs.comment.strings`, the write-through proxies — and admitting them +means answering deep-equality, merge-vs-replace, per-key validation and +per-key notification. That is a second arc. + +### Q#CR4 — Two scopes: global and buffer-local. Language and project are patterns, not scopes + +This is the framing's load-bearing decision and the main departure from +the withdrawn draft, which was global-only. Global-only would not +unblock a single one of the five backlog items the registry exists to +unblock — four of the five are inherently per-buffer or per-language. + +```lua +pmacs.config.define { name = …, description = …, type = …, default = … } +pmacs.config.set(name, value) -- global override +pmacs.config.set_local(buf, name, value) -- buffer-local override +pmacs.config.get(name [, buf]) -- see resolution below +pmacs.config.reset(name [, buf]) -- drop exactly one layer +pmacs.config.is_set(name [, buf]) -- override present, not value ≠ default +``` + +**Resolution, pinned (F9).** `get(name, buf)` resolves +buffer-local → global → default. `get(name)` with **no buffer argument +resolves the global chain only** (global override → default) and never +consults an implicitly-active buffer. The signature is the contract; +there is no hidden ambient buffer. A consumer that wants buffer-aware +behavior must pass the buffer, and the acceptance list pins that +forgetting it yields the global value rather than a surprise. + +`reset(name)` drops the global override; `reset(name, buf)` drops only +that buffer's local layer and re-exposes the global. `is_set` reports +override *presence* at the queried layer, which is well-defined +precisely because Q#CR2 always stores overrides. + +Per-language and per-project behavior is achieved the way Emacs achieves +it: a callback on `buffer.after-load` reads +`pmacs.lsp.buffer_language(buf)` or `pmacs.project.detect(buf:path())` +and calls `set_local`. The registry never learns what a language or a +project is. + +The claim, stated so it can be falsified: **all five backlog-blocked +features are expressible as a hook that sets buffer-locals.** +Language-aware indent, per-language comment padding and the per-buffer +auto-pair toggle are buffer-local by nature. Per-project compile +commands are a `set_local` keyed on the detected root. Tab width is +buffer-local (Q#CR13 defers it for an unrelated reason). + +Mode-scoped settings are not offered because they cannot work: every +editor `KeymapStack::resolve` passes `&[]`. Offering a mode scope on an +unwired mode system would ship a knob that silently never fires. + +### Q#CR5 — Buffer-local storage: registry-owned, purged at the existing choke point + +`locals: HashMap>` inside the +registry, following `BufferRemoveCallbacks` (`mod.rs:133`) rather than +adding a field to `Buffer` — the buffer struct carries undo and CRDT +invariants and has no property bag, and config does not belong in it. + +Purge rides `after_buffer_removed` (`mod.rs:1458`), one line beside the +keymap purge already there. No new hook is defined for this, and the +purge does **not** fire listeners (Q#CR6): the buffer is gone, so there +is no effective value for anyone to observe. + +Honest limitation, corrected in revision 2 (F8): `BufferRegistry::remove` +can be called directly without going through `remove_buffer_and_fire` +(`editor.rs:5527` does so in a test). Such a path leaks that buffer's +locals **permanently** — not "until the `BufferId` is reused", because +`BufferId`s come from a global counter and are never reused +(`buffer_registry.rs:82-84`). The leak is therefore bounded and can +never alias onto a future buffer, which makes it a memory footnote +rather than a correctness hazard. Stage 1 pins the contract with +acceptance 13 rather than restructuring `BufferRegistry`. + +### Q#CR6 — Change delivery: post-commit listeners, borrow released + +Carried forward from the withdrawn draft, whose design here is better +than a plain hook and matches the compile-mode `handle:dispose()` +precedent (`mod.rs:1867`). The registry never runs arbitrary Lua while +mutably borrowed. + +```lua +local handle = pmacs.config.on_change('editing.auto-pair', function(new, old, buf) + -- receives already-validated owned values +end) +handle:dispose() -- idempotent, generation-safe +``` + +Set/reset flow: resolve and validate the candidate with no mutation → +**store the override unconditionally** (Q#CR2) → advance `value_epoch` +iff the effective value changed → **release the registry borrow** → +if the effective value changed, invoke listeners in registration order +with copied values → log a failing listener to the normal Lua error sink +and continue the rest. + +**Dispatch semantics, pinned (F2):** + +- **(a)** A global `set` that changes the global effective value fires + once with `buf = nil`. +- **(b)** It does **not** additionally fire per-buffer. A buffer holding + its own override is shadowed — its effective value did not change — so + a listener that cares about a specific buffer must re-resolve with + `get(name, buf)`. The notification says "the global changed", not + "every buffer changed". +- **(c)** The `after_buffer_removed` purge (Q#CR5) fires nothing. +- **(d)** `on_change` on an undefined name raises `NotFound`, matching + the define-before-set posture of Q#CR10. + +None of the three stage-1 adopters consumes `on_change` — `pair.lua` +reads at insert time, `editops.lua` at save, `autosave.lua` per tick — +so the acceptance tests are the only exercise these semantics get. That +is precisely why the contract is written out here rather than left to +the first consumer to discover. + +**Listeners persist until explicitly disposed (F3).** Revision 1 said a +garbage-collected handle stops firing; that is dropped. There is no +`MetaMethod::Gc` anywhere in `mod.rs`, the compile-mode precedent is +explicit-dispose only, and GC timing differs between LuaJIT and lua54 — +importing finalizer nondeterminism would cut directly against the +cross-backend exactness this framing demands elsewhere. A user who +registers `on_change` and drops the handle keeps the listener; that is +the same bargain `pmacs.hook.add` already makes. + +A listener error does not roll the value back: earlier listeners may +already have applied side effects, and rollback would create two sources +of truth. Re-entrant `set`/`reset` is permitted after borrow release and +produces a later notification epoch; a per-dispatch recursion bound +stops an accidental listener cycle from hanging the editor. Listener ids +are never reused, so a stale handle can never dispose a newer listener. + +The borrow-release step is the one to bite-verify — +`HookRegistry::snapshot` exists for exactly this reason, and the +statusline arc's three-phase borrow-released transaction is the recent +precedent for getting it wrong being expensive. + +### Q#CR7 — No wire surface; protocol stays at v18 + +Every stage-1 setting is read daemon-side, in Lua. Nothing new goes on +the wire and `SUPPORTED` is untouched. + +Settings whose consumer lives in a frontend already have their own +authoritative facts channel — `ThemeFacts` (v16), `FontFacts` (v17), +`StatuslineSegments` (v18), `LineNumbers` (v13/v14). That +one-channel-per-concern shape is deliberate, with an +authoritative-per-attachment contract and a snapshot/baseline reset +contract behind it (#120 rounds 2-5); a generic "config facts" channel +would have to re-derive all of that and would fit worse than what those +four already do. The registry is not a transport. + +### Q#CR8 — Three first consumers, chosen to prove three shapes + +| Setting | Type | Proves | Status | +| --- | --- | --- | --- | +| `editing.auto-pair` | boolean | buffer-local resolution | **new** — unblocks the named backlog item | +| `editing.trim-on-save` | boolean | migration behind a stable API | migrates `editops.lua:844` | +| `autosave.interval-ms` | integer, min 1000 | validation + live re-read each tick | migrates `autosave.lua:42` | + +All three are consumed entirely in Lua, on the daemon, which is what +makes Q#CR7 true. The withdrawn draft deferred adopter selection to +implementation time; naming them now is what lets the acceptance list +below be written before any code exists. + +**No existing surface is removed**, and the wrappers keep their legacy +coercion (F4). This is the subtle part of the migration. Today +`pmacs.autosave.interval_ms(1500.7)` succeeds and floors to `1500` +(`autosave.lua:48`), and `pmacs.editops.trim_on_save("yes")` sets true +(`on ~= false`, `editops.lua:846`). A *thin* wrapper over a strict +`config.set` would reject both, because Q#CR3's `integer` demands +exactness and `boolean` demands a real boolean. So the wrappers coerce +first — `math.floor` / `~= false` — and then call `set` with an +already-conforming value. The registry stays strict; the legacy API +stays lenient; acceptance 27-28 pin both directions on inputs no current +test covers. + +Richer feature-specific APIs are explicitly not deleted: +`pmacs.gpu.set_font` remains the font preference API until a separately +framed migration decides how a daemon-global preference and +frontend-local resolution interact. + +`pair.lua` reads `editing.auto-pair` in its insertion predicate. It +loads before `lsp.lua` by an existing ordering contract (Q#AP7, +`editor.rs:319` and `:325`), so the config surface must be installed +before `pair.lua` (Q#CR14). + +### Q#CR9 — Names are dotted and kebab-cased + +`editing.auto-pair`, `autosave.interval-ms`, and — when it arrives — +`editor.tab-width`. Lowercase ASCII segments, total length bounded at +128 bytes. + +Segment grammar, tightened in revision 2: `[a-z][a-z0-9]*(-[a-z0-9]+)*`. +Revision 1's `[a-z][a-z0-9_-]*` admitted `auto-` and `a--b`; this form +forbids a trailing hyphen and a doubled hyphen while accepting every +name we actually want. + +The withdrawn draft used snake_case (`editor.tab_width`). Kebab matches +the two registries that already exist — `buffer.before-save`, +`buffer.after-switch`, `buffer.self-insert`, `buffer.kill-this` — and +setting *names* are strings in the registry vocabulary, not Lua +identifiers. Lua field names stay snake_case as they are today. + +### Q#CR10 — Define before set; `Live` vs `StartupOnly` + +`pmacs.config.set` on an undefined name raises `NotFound`, exactly as +`pmacs.hook.add` does. Silent acceptance of an unknown name is how typos +become permanent mysteries. + +Definitions are immutable after first registration, except that a +byte-for-byte identical redefinition succeeds (idempotent reload); a +conflicting redefinition fails and leaves the original exactly as it +was. + +`mutability` is `Live` or `StartupOnly`. `StartupOnly` keys accept +writes while user config is loading and freeze when `set_init_complete` +runs at the tail of `EditorState::new()` (`editor.rs:465-476`); a later +write returns an error naming the key and the policy. This generalizes +the posture `require_init_phase` (`mod.rs:663`) already hard-codes for +`pmacs.attach`, and gives it a declarative home. + +**`StartupOnly` and `set_local` are mutually exclusive (F5).** +Buffer-locals are set at runtime, from `buffer.after-load` hooks that +fire long after the freeze — so a `StartupOnly` key could never carry a +buffer-local override, and a `set_local` against one would be dead code +that looks live. `define` rejects the combination outright with +`StartupOnlyLocal` rather than shipping a knob whose two halves +contradict each other. + +The ordering that makes define-before-set safe is the corrected sequence +in ground truth: builtins define during `EditorState::new()`, user +config runs, then the freeze — all inside the same function. Packages +loading after init cannot be pre-configured from `init.lua` — the +existing v0.1 posture, not a new limitation. Staging pending sets for +later-defined names is deferred. + +### Q#CR11 — Discovery: `describe` + `list` + `M-x describe-setting` + +`pmacs.config.describe(name [, buf])` returns a fresh table with `name`, +`description`, `type`, `default`, `choices`, bounds, `mutability`, +`value`, `global`, `buffer_local`, and `source`. + +**`buffer_local`, not `local` (F7)** — `local` is a Lua keyword, so +`info.local` is a syntax error and every consumer would be forced to +write `info["local"]`. The field is present only when a buffer argument +is given and that buffer holds an override. + +`pmacs.config.list()` returns fresh metadata tables, deterministic by +key. Neither ever exposes an internal table or a listener function. + +`M-x describe-setting` renders through `src/help.rs`, following +`render_hook` / `format_hook_text` (`help.rs:160`, `:170`) so the +`*help*` buffer, its link spans and its view-rebuild path work +unchanged. The withdrawn draft had `describe`/`list` as Lua-only; wiring +the `*help*` surface is what makes the registry discoverable to a user +rather than to a script. + +A `list-settings` listview panel is deferred — the machinery exists from +Arc 1b, but it is UI scope on a substrate PR. + +### Q#CR12 — No persistence in stage 1 + +Emacs's `custom-file` problem — the init file says one thing, the +persisted state file says another, and the user cannot tell which won — +is a genuine design hazard deserving its own framing round. The +`$XDG_STATE_HOME/pmacs` machinery from Arc 3 exists +(`install_state_dirs`, `editor.rs:551`), so this is a question about +what we want, not about plumbing. + +### Q#CR13 — Tab width is stage 2, and the reason is not config + +Tab width is the backlog's headline example, and the withdrawn draft +used `editor.tab_width` as its running example and candidate adopter. It +is deliberately out of stage 1 here. + +Per the ground truth: the daemon has four `TAB_WIDTH = 8` constants, the +GPU minimap expands tabs to 4, and the GPU's main text path does not +expand tabs at all. So `editor.tab-width` cannot be honored on the GPU +by defining a setting. It needs frontend tab expansion, a decision about +whether the value crosses the wire or the frontend reads its own, and a +rendering-parity fix that stands on its own merits. Note that the +withdrawn draft's own non-goals excluded protocol messages and +frontend-local settings — which its headline example required. Stage 1 +resolves that contradiction by deferring the example, not the non-goal. + +Stage 2: unify the four daemon constants into a resolved value threaded +through the display-column functions as a parameter — they are pure +functions today and should stay pure — then answer the GPU question. + +### Q#CR14 — Surface in `config.lua`; each module defines its own keys + +`builtin/runtime/config.lua` holds the friendly Lua surface only, loaded +in `EditorState::new()` immediately after `fs.lua` (`editor.rs:206`) and +before every module that defines or reads a setting — in particular +before `pair.lua` (`editor.rs:319`), whose own load-before-`lsp.lua` +contract (`editor.rs:325`) is unaffected. + +**Builtin `define` calls live with their owning modules (F10)**, not +centralized in `config.lua`. Revision 1 centralized them, which would +have pointed every builtin setting's `SourceLocation` at `config.lua` — +weakening exactly what acceptance 9 celebrates — and coupled +`config.lua` to autosave's floor, pair's default and editops' semantics. +The hook precedent is owner-defines: `builtin/hooks/default.lua` defines +hooks, but each module owns its own behavior. So `pair.lua` defines +`editing.auto-pair`, `editops.lua` defines `editing.trim-on-save`, and +`autosave.lua` defines `autosave.interval-ms`. Ordering still works — +every builtin chunk precedes `init.lua` regardless. + +The Rust `install_config` call sits with the other installs in +`attach_editor`. Total footprint in the two files the vterm lane also +touches: one install call plus one chunk load in `editor.rs`, one `mod` +line plus one call in `lua_bindings/mod.rs`. + +### Q#CR15 — Threading and hot paths + +Main-thread `Rc>`, matching the syntax, statusline, command +and hook registries. Reads return borrowed or copied data internally and +allocate only when crossing into Lua metadata. Render hot paths cache a +typed value or gate on `value_epoch`; they must never build a Lua table +or do a string lookup per cell or per frame. + +No worker-thread mutation. Workers receive copied settings in job specs, +so a live change affects future jobs only, unless the owning feature +explicitly cancels and restarts work. + +--- + +## Bets + +1. **Global + buffer-local is sufficient.** Falsified if any of the five + backlog-blocked features cannot be expressed as a hook that calls + `set_local`. The most likely falsifier is per-project compile + commands, where the natural key is a project root — if a + project-scoped value must survive with no buffer open, this fails and + a third scope is needed. +2. **No protocol change is needed.** Falsified if any stage-1 consumer + turns out to have a frontend-side reader. Checked against all three: + `pair.lua`, `editops.lua`, `autosave.lua` all run daemon-side. +3. **Scalars are sufficient for stage 1.** Falsified if a first consumer + wants a table- or list-valued setting. The three chosen adopters are + two booleans and an integer, so this is near-certain for stage 1 and + says nothing about stage 2. +4. **`after_buffer_removed` catches every buffer death that matters.** + Falsified by a production path that removes a buffer from the + registry without it. The known exception is test-only today + (`editor.rs:5527`), and per F8 its blast radius is a bounded, + non-aliasing leak. +5. **Migrating two settings behind unchanged public functions is + invisible to users.** Falsified if any observable behavior of + `trim_on_save` / `interval_ms` changes — including the coercion + behavior on non-conforming inputs, which is why F4's pins exist. + +Revision 1's bet 6 (a real `StartupOnly` freeze point) is **confirmed** +and retired into ground truth: `set_init_complete` at +`editor.rs:465-476`, inside `EditorState::new()`, covering both entry +points. + +--- + +## Deferred (named) + +- **Persistence** and the `custom-file` split-brain question (Q#CR12). +- **Tab width and the GPU tab-expansion parity fix** — stage 2 + (Q#CR13), including the GPU minimap's divergent width of 4. +- **`string-list`** (F6) — returns with its first adopter and an + explicit hole-rejection rule. +- **Per-language and per-project first-class scopes**, if bet 1 falls. +- **Mode scope** — blocked on wiring the mode system (every editor + `resolve` passes `&[]`); a cross-cutting backlog item in its own right. +- **Window-local scope** — `line_numbers` is the existing precedent and + the first migration if a second window-local setting appears. +- **Table-valued settings** (Q#CR3), and with them any migration of + `pmacs.lsp.config`, `pmacs.pair.sets`, `pmacs.comment.strings` or the + `pmacs.parse.*` write-through proxies. +- **Migrating the remaining scalar surfaces** — `async_config` (×2), + `killring.max`, `autosave.enable`, `recentf.enable`, + `saveplace.enable`, `session.desktop_mode`. +- **Migrating `pmacs.gpu.set_font`** — needs its own framing for the + daemon-preference / frontend-resolution split (Q#CR8). +- **Deprecating the getter-when-nil functions** once migration + completes; stage 1 keeps every one of them, coercion included. +- **Pending-set staging** for names defined after `init.lua` runs + (Q#CR10), which would also give packages a configuration story. +- **`M-x list-settings`** as a listview panel (Q#CR11). +- **A `customize`-style editing UI**, and a `:set`-style minibuffer + command. + +--- + +## Acceptance + +Registry semantics (unit, `src/config_registry.rs`): + +1. Valid definitions round-trip every value kind and produce a fresh + metadata table. +2. `define` rejects an empty name, a missing or whitespace-only + description (R42), a duplicate name, an unknown spec key (R50), a + metatable-provided field, non-finite bounds, an inverted range, + duplicate enum choices, and a default violating its own contract — + each **without** adding a definition or advancing either epoch, and + each with a message naming the offending field. +3. `define` rejects the name grammar's edge cases: trailing hyphen + (`auto-`), doubled hyphen (`a--b`), empty segment, leading digit, and + over-length (Q#CR9). +4. An identical redefinition is idempotent; a conflicting redefinition + leaves the original exact. +5. Values and definitions are deep-copied from Lua — mutating the + caller's table afterwards cannot alter registry state. +6. Integer and number finite/boundary cases are exact under **both** + `--features luajit` (default) and + `--no-default-features --features lua54`. +7. `list` is deterministic by key; `describe`/`list` never expose an + internal mutable table or a listener function. +8. `names()` / `list()` order is stable across ≥3 defines. +9. `SourceLocation` is captured from the *defining module's* chunk and + renders `file:line` — `editing.auto-pair` reports `pair.lua`, not + `config.lua` (Q#CR14). + +Scoping, storage and buffer-local lifecycle: + +10. `get(name, buf)` resolves buffer-local → global → default; + `reset(name, buf)` drops only the local layer and re-exposes the + global; `reset(name)` drops only the global. +11. **An equal-valued override is still stored (F1).** With the global + value `true`, `set_local(buf, name, true)` then + `set(name, false)` leaves `get(name, buf)` `true` and + `get(name)` `false` — **bite-verified**: the test fails against a + "true no-op" implementation that declines to store it. +12. A buffer-local set on buffer A does not change the value seen in + buffer B; `is_set` reports override presence per layer, including + for an equal-valued override. +13. A buffer-local value is dropped when the buffer is removed through + `remove_buffer_and_fire`; removing a buffer directly via + `BufferRegistry::remove` leaves the entry, and the test asserts that + documented limitation rather than a fix (Q#CR5, F8). +14. `get(name)` with no buffer argument returns the global chain result + even when the active buffer holds a different local override (F9). +15. `value_epoch` advances only on effective-value change; storing an + equal-valued override advances neither epoch. + +Listeners (Q#CR6): + +16. Callbacks run after borrow release, in registration order, with + copied `(new, old, buf)` values — **bite-verified**: the test fails + with the borrow held. +17. A global `set` fires once with `buf = nil` (a), and does **not** + fire for a buffer whose own override shadows the change (b). +18. The `after_buffer_removed` locals purge fires no listener (c). +19. `on_change` on an undefined name raises `NotFound` (d). +20. One raising listener is logged once and does not block later + listeners; the committed value stays authoritative. +21. A re-entrant `set` from inside a listener creates a later ordered + epoch without a `RefCell` panic; a recursive cycle hits the bounded + error rather than hanging. +22. Dispose is idempotent and id-generation-safe; a stale handle never + disposes a newer listener. A dropped-but-undisposed handle keeps + firing (F3) — the inverse of revision 1's claim, pinned so the + behavior cannot silently regress to GC-dependence. + +Startup and mutability: + +23. `StartupOnly` keys accept writes before the freeze and reject after, + with a message naming the key and the policy. **The test flips + `InitCompleteFlag` explicitly** (`mod.rs:14655-14690` is the + pattern) — in `--lib` builds `set_init_complete` never runs, so a + test that omits this passes vacuously. +24. `define` rejects `mutability = 'startup'` combined with any + `set_local` attempt, via `StartupOnlyLocal` (F5). +25. A failing `init.lua` preserves prior successful sets and still + starts the editor; missing `init.lua`, broken `init.lua` and + `require` package-path behavior are unchanged. +26. `config.lua` is installed before `pair.lua`, and `pair.lua` still + loads before `lsp.lua` (the Q#AP7 contract is untouched). + +Adopters: + +27. `pmacs.editops.trim_on_save(true)` and + `pmacs.config.set("editing.trim-on-save", true)` are interchangeable + in both directions — each observes the other's writes — and + `trim_on_save("yes")` still sets true (F4). +28. `pmacs.autosave.interval_ms(1500.7)` still returns `1500`, and + `interval_ms(500)` still raises on the floor — now enforced by the + registry validator behind the wrapper's coercion. **Bite-verified** + against the removed hand-rolled check (F4). +29. `editing.auto-pair` false globally suppresses pairing; false + buffer-locally suppresses it in that buffer only, with pairing still + active in a second buffer of the same language. +30. The autosave tick observes a mid-session interval change without a + restart (the existing live-re-read contract, re-pinned). +31. Neither adopter performs a per-frame or per-cell Lua lookup, and + both preserve their previous default behavior exactly. + +Discovery: + +32. `pmacs.config.describe` returns every documented field, with + `buffer_local` absent when no buffer-local override exists and the + field name usable without bracket syntax (F7). +33. `M-x describe-setting` renders into `*help*` with the source + location present. + +Gates: the full standing suite per `CLAUDE.md`, plus the new acceptance +suite, plus `--no-default-features --features lua54` for item 6. + +--- + +## Resolved review questions (round 1) + +1. **Scope set** — accept global + buffer-local. Bet 1's named falsifier + (a project value surviving with no buffer open) is real and correctly + deferred. +2. **`string-list`** — dropped (F6). Returns with an adopter. +3. **`Live`/`StartupOnly`** — accepted; freeze point confirmed at + `editor.rs:465-476`, covering both entry points. Test-build caveat + (acceptance 23) and the `set_local` exclusion (F5) carried into the + doc. +4. **No-rollback listener errors** — accepted; matches the hook + log-and-continue posture. +5. **Three adopters, `set_font` untouched** — accepted, with F4's + coercion pins added. +6. **Defer tab width** — accepted; the parity mess is verified real and + the withdrawn draft's contradiction correctly diagnosed. +7. **Kebab-case** — accepted, with the grammar tightened to + `[a-z][a-z0-9]*(-[a-z0-9]+)*` to forbid `auto-` and `a--b`. + +Open for round 2: nothing blocking. F10 (owner-defines) was adopted as +recommended; if the lead prefers centralized defines for reviewability, +acceptance 9 is the item that changes. + +--- + +## Lane coordination (config registry ↔ vterm) + +Both lanes are active concurrently in separate worktrees. Assignment: + +| File | Owner | Other lane | +| --- | --- | --- | +| `src/editor.rs` runtime-load block | **config** — its chunk must load before consumers | vterm appends its chunk at the tail | +| `src/lua_bindings/mod.rs` | **neither** — config adds `lua_bindings/config.rs`, vterm adds its own submodule; each takes one `mod` line + one install call | keep both footprints to one line | +| `src/lib.rs` module exports | one line each | trivial rebase conflict, resolve in favor of both | +| `src/ansi.rs` | **vterm** | config never touches it | +| `docs/agent-handoff.md`, `docs/active-work.md` | whichever merges second rewrites its §1 entry post-rebase | do not co-edit | + +Neither feature imports the other's types. Recommended merge order: +config registry first (smaller, substrate, heaviest footprint in the +shared files), then vterm rebases. Per the standing constraint, vterm +keeps terminal settings hard-wired in its early stages and adopts +`pmacs.config` only after both contracts land, so neither arc is a +prerequisite for the other. + +Implementation branch, after framing approval: `config-registry`, cut +from then-current canonical `main`, never from a vterm branch. From 6844262495f59439bf880596e305d325a726f6fc Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 21 Jul 2026 16:05:26 -0400 Subject: [PATCH 2/4] feat(config): typed configuration registry with buffer-local scope A third registry beside CommandRegistry and HookRegistry, per docs/config-registry-framing.md. Unblocks the per-buffer auto-pair toggle, the first of the five backlog items the missing config surface was gating. Substrate (src/config_registry.rs): * ConfigRegistry keyed by name with definition order preserved, R42 mandatory descriptions, R50 typo detection, duplicate rejection, and SourceLocation provenance -- the command/hook vocabulary. * Closed scalar kinds: boolean, integer, number, string, enum. Owned Rust values; Lua tables, functions and userdata are never stored. Integer exactness is checked by value, never math.type, so the luajit and lua54 builds agree. * Two scopes. get(name, buf) resolves buffer-local -> global -> default; get(name) with no buffer resolves the global chain only and never consults an ambient buffer. Buffer-locals live in a registry-owned side table purged at after_buffer_removed, beside the keymap purge already there. * An override is ALWAYS stored, even when equal to the value it shadows; only value_epoch and listener dispatch key on effective change. Without this a buffer pinned to the current value stores nothing and a later global set flips it -- the pin silently never existed. equal_valued_local_override_is_still_stored_and_shields_buffer fails against the naive reading. Bindings (src/lua_bindings/config.rs): * define/get/set/set_local/reset/is_set/describe/list/on_change. Spec tables are read raw, so neither an unknown key nor a metatable-provided value can smuggle a field in. * Listeners commit inside the borrow, snapshot, drop the borrow, and only then re-enter Lua -- verified by holding the borrow and watching the test panic with "RefCell already borrowed". A raising listener is logged without blocking later ones or rolling back, and a depth bound turns an accidental cycle into a pointed error. Listeners persist until explicitly disposed; there is no Gc path, matching the rest of the codebase. * StartupOnly freezes off the existing InitCompleteFlag at write time, so this arc adds no editor.rs call at all. Adopters, each defining its own key so SourceLocation names the owning module: editing.auto-pair (pair.lua, read per-buffer against the typed edit's SOURCE buffer), editing.trim-on-save (editops.lua), autosave.interval-ms (autosave.lua). No public function is removed or deprecated, and both migration wrappers keep their legacy coercion -- trim_on_save("yes") still enables, interval_ms(1500.7) still floors to 1500 -- coercing before handing the strict registry a conforming value. M-x describe-setting renders into *help*, modeled on describe-command. Framing revision 3 records four defects implementation found in the document itself: acceptance 30 and 31 contradicted each other; the planned builtin/runtime/config.lua had nothing to hold and would have broken the source-location contract had it held the one helper it might have; F5 asked define to police a call it cannot see, moved to set_local; and list() ordering was underspecified. No protocol change; SUPPORTED stays [6..18]. No wire surface. Zero changes to src/editor.rs. Gates: fmt, clippy -D warnings, --lib (1683), --lib --features crdt (1857), the new config_registry_acceptance (13) plus auto_pair (45), editops (72), autosave (29) and m9_6 (25), m4 --skip basedpyright (114), PMACS_REQUIRE_GPU=1 pmacs-gpu (109), the lua54 backend build, and the full workspace sweep (2795 tests, exit 0). git diff --check clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- builtin/commands/default.lua | 67 + builtin/runtime/autosave.lua | 38 +- builtin/runtime/editops.lua | 23 +- builtin/runtime/pair.lua | 20 + docs/config-registry-framing.md | 129 +- src/config_registry.rs | 2124 +++++++++++++++++++++++++++ src/lib.rs | 1 + src/lua_bindings/config.rs | 1460 ++++++++++++++++++ src/lua_bindings/mod.rs | 8 + tests/config_registry_acceptance.rs | 415 ++++++ 10 files changed, 4251 insertions(+), 34 deletions(-) create mode 100644 src/config_registry.rs create mode 100644 src/lua_bindings/config.rs create mode 100644 tests/config_registry_acceptance.rs diff --git a/builtin/commands/default.lua b/builtin/commands/default.lua index 7b761b9..04c49fe 100644 --- a/builtin/commands/default.lua +++ b/builtin/commands/default.lua @@ -1169,3 +1169,70 @@ cmd { name = "editor.describe-command", end, } end } + +-- describe-setting (config registry, acceptance 33) ------------------------- +-- +-- The configuration registry's discovery surface. `pmacs.config.describe` +-- returns the metadata table at the Rust layer; this is the interactive +-- way in, modeled on `editor.describe-command` directly above and sharing +-- its `*help*` buffer handling. +-- +-- The prompt takes free text: `pmacs.minibuffer.read`'s `source` is a +-- fixed vocabulary ("commands", "buffers") resolved in Rust, and adding a +-- settings source means touching the minibuffer candidate machinery, +-- which this arc deliberately stays out of. `pmacs.config.list()` is the +-- programmatic way to enumerate names meanwhile; a completion source (and +-- an M-x list-settings panel) are named deferrals in the framing. + +local function describe_setting_lines(name, info) + -- Header block mirrors help.rs's `format_hook_text`: aligned label + -- column, then a blank line, then the description as prose. + local lines = { + "Setting: " .. name, + " Type: " .. tostring(info.type), + " Default: " .. tostring(info.default), + " Value: " .. tostring(info.value), + " Mutability: " .. tostring(info.mutability), + " Source: " .. tostring(info.source), + } + if info.min ~= nil then lines[#lines + 1] = " Min: " .. tostring(info.min) end + if info.max ~= nil then lines[#lines + 1] = " Max: " .. tostring(info.max) end + if type(info.choices) == "table" and #info.choices > 0 then + lines[#lines + 1] = " Choices: " .. table.concat(info.choices, ", ") + end + lines[#lines + 1] = "" + local desc = info.description + if type(desc) ~= "string" or desc == "" then desc = "(no description)" end + lines[#lines + 1] = desc + lines[#lines + 1] = "" + -- Overrides. `global` is the global-chain resolution regardless of the + -- buffer argument, so "same as default" is a real, distinguishable + -- state from "overridden to the same value" only via is_set — which is + -- why this reports the resolved values rather than claiming presence. + lines[#lines + 1] = "Global value: " .. tostring(info.global) + if info.buffer_local ~= nil then + lines[#lines + 1] = "Buffer-local override: " .. tostring(info.buffer_local) + end + return lines +end + +cmd { name = "editor.describe-setting", + description = "Prompt for a setting name and render its definition in *help*.", + fn = function() + pmacs.minibuffer.read { + prompt = "Describe setting: ", + history = "command", + on_accept = function(name) + if name == nil or name == "" then return end + -- An undefined name raises NotFound rather than returning nil + -- (the define-before-set posture, Q#CR10), so this must pcall. + local buf = pmacs.window.buffer() + local ok, info = pcall(pmacs.config.describe, name, buf) + if not ok or type(info) ~= "table" then + pmacs.editor.set_status("describe-setting: no such setting: " .. name) + return + end + show_help_text(table.concat(describe_setting_lines(name, info), "\n")) + end, + } + end } diff --git a/builtin/runtime/autosave.lua b/builtin/runtime/autosave.lua index 1728693..0d816c6 100644 --- a/builtin/runtime/autosave.lua +++ b/builtin/runtime/autosave.lua @@ -24,7 +24,20 @@ pmacs.autosave = pmacs.autosave or {} local DEFAULT_INTERVAL_MS = 30000 -- Emacs's auto-save-timeout local MIN_INTERVAL_MS = 1000 -- each sweep fsyncs; don't storm -local interval = DEFAULT_INTERVAL_MS +-- Migrated to `autosave.interval-ms` (Q#CR8's third adopter: integer, +-- validated, re-read live). The tick below re-reads the registry every +-- frame — see its comment — so storage moves there instead of a +-- module-local, but the wrapper's shape and its lenient coercion (F4) +-- stay exactly as they were. +pmacs.config.define { + name = "autosave.interval-ms", + description = "Milliseconds between periodic autosave sweeps.", + type = "integer", + default = DEFAULT_INTERVAL_MS, + min = MIN_INTERVAL_MS, + mutability = "live", +} + local enabled = true local last_sweep_ms = nil -- Report on the first tick (the startup scan), and after every load. @@ -38,14 +51,20 @@ end -- interval_ms([ms]) --- getter when `ms` is nil, else a validated setter. -- Shape follows `pmacs.async_config.frame_target_ms`. The tick re-reads --- this every frame, so a change takes effect immediately -- no restart. +-- the registry every frame, so a change -- through this wrapper OR a +-- direct `pmacs.config.set` -- takes effect immediately, no restart. +-- Legacy coercion stays lenient (F4): floor a fractional `ms` FIRST, +-- then hand the registry an already-conforming integer, since +-- `pmacs.config.set` itself demands exactness. The floor-then-min error +-- message and threshold are unchanged. function pmacs.autosave.interval_ms(ms) - if ms == nil then return interval end + if ms == nil then return pmacs.config.get("autosave.interval-ms") end if type(ms) ~= "number" or ms ~= ms or ms < MIN_INTERVAL_MS then error("pmacs.autosave.interval_ms: expected a number >= " .. MIN_INTERVAL_MS) end - interval = math.floor(ms) - return interval + local floored = math.floor(ms) + pmacs.config.set("autosave.interval-ms", floored) + return floored end -- sweep() --- force a pass now. Returns (written, blocked, conflicted). @@ -113,9 +132,10 @@ end -- The cadence (Q#AS2). `process.after-tick` fires every frame -- and the -- run loops tick on a frame *timeout*, not only on input, so this keeps --- running while the editor is idle. Costs one clock read + a compare per --- frame, and parks no worker thread (a long `workers.sleep` would hold --- one of only `available_parallelism - 1` pool threads). +-- running while the editor is idle. Costs one clock read, one registry +-- get (a borrowed/copied scalar, no Lua table built -- Q#CR15) and a +-- compare per frame, and parks no worker thread (a long `workers.sleep` +-- would hold one of only `available_parallelism - 1` pool threads). pmacs.hook.add("process.after-tick", function() if needs_report then needs_report = false @@ -127,7 +147,7 @@ pmacs.hook.add("process.after-tick", function() last_sweep_ms = now return end - if now - last_sweep_ms >= interval then + if now - last_sweep_ms >= pmacs.config.get("autosave.interval-ms") then last_sweep_ms = now sweep_reporting() end diff --git a/builtin/runtime/editops.lua b/builtin/runtime/editops.lua index 13fc1a6..f78b677 100644 --- a/builtin/runtime/editops.lua +++ b/builtin/runtime/editops.lua @@ -840,11 +840,24 @@ pmacs.command.define { -- Opt-in trim-on-save. Getter when nil (the killring.max shape); -- default OFF — rewriting bytes on save is a policy, not a default. -local trim_enabled = false +-- Migrated to `editing.trim-on-save` (Q#CR8's second adopter) behind +-- this unchanged signature. Legacy coercion stays lenient (F4): anything +-- but a literal `false` turns it on, same as the boolean-enable shape +-- shared by autosave.enable/recentf.enable/saveplace.enable — coerce +-- FIRST, then hand the registry an already-conforming boolean, since +-- `pmacs.config.set` itself is strict. +pmacs.config.define { + name = "editing.trim-on-save", + description = "Delete trailing whitespace from every line before a save.", + type = "boolean", + default = false, + mutability = "live", +} function pmacs.editops.trim_on_save(on) - if on == nil then return trim_enabled end - trim_enabled = (on ~= false) - return trim_enabled + if on == nil then return pmacs.config.get("editing.trim-on-save") end + local enabled = (on ~= false) + pmacs.config.set("editing.trim-on-save", enabled) + return enabled end -- Registered at load time (gated inside) so it runs BEFORE @@ -861,7 +874,7 @@ pmacs.hook.add("buffer.before-save", function() -- either way). Both reports are pcall'd — a broken reporting -- channel must not resurrect the veto. local ok, err = pcall(function() - if trim_enabled then + if pmacs.config.get("editing.trim-on-save") then trim_active("delete-trailing-whitespace (on save)") end end) diff --git a/builtin/runtime/pair.lua b/builtin/runtime/pair.lua index 1fe0e4e..6d014d4 100644 --- a/builtin/runtime/pair.lua +++ b/builtin/runtime/pair.lua @@ -37,6 +37,20 @@ local ed = pmacs.editor -- daemon-peer op, so its undo is cross-peer-degraded (documented -- limitation; the general fix is chronological cross-peer undo -- arbitration, named substrate work). + +-- Per-buffer on/off switch (Q#CR8's flagship adopter). Read against the +-- SOURCE buffer of the typed edit, never the currently active one — see +-- the hook body below, which resolves it the same way `set_for` resolves +-- the buffer's pair set (round 2, finding 2): `rec.buffer`, not +-- `pmacs.window.buffer()`. +pmacs.config.define { + name = "editing.auto-pair", + description = "Automatically insert (and skip over) the closing half of a typed pair.", + type = "boolean", + default = true, + mutability = "live", +} + pmacs.pair.sets = { default = { "()", "[]", "{}", '""' }, python = { "()", "[]", "{}", '""', "''" }, @@ -198,6 +212,12 @@ pmacs.hook.add("buffer.after-edit", function() if not rec then return end if not (ed.this_command and ed.this_command() == "buffer.self-insert") then return end + -- The master switch, per-buffer (Q#CR4): the SOURCE buffer of the + -- typed edit, resolved buffer-local -> global -> default(true). A + -- second buffer of the same language is untouched by a buffer-local + -- override here (acceptance 29). + if not pmacs.config.get("editing.auto-pair", rec.buffer) then return end + local buf = pmacs.window.buffer() if not buf then return end diff --git a/docs/config-registry-framing.md b/docs/config-registry-framing.md index 476d25a..a6173b7 100644 --- a/docs/config-registry-framing.md +++ b/docs/config-registry-framing.md @@ -60,6 +60,29 @@ global-only scoping (Q#CR4), `editor.tab_width` as the proving adopter - **Bet 6 confirmed and retired** into ground truth, with a correction to this doc's own "Init ordering" claim. +**Revision 3 — 2026-07-21, implementation round 1.** Two corrections +found while building the adopters, both against this document rather +than against the code. +- **Acceptance 30 vs 31 contradicted each other** — 31 forbade a + "per-frame lookup" that 30 required. Q#CR15's prohibition is about + render hot paths (per cell, per line, table construction), not one + O(1) scalar `get` per tick. Item 31 reworded. +- **`builtin/runtime/config.lua` does not exist** (Q#CR14). `pmacs.config` + installs from Rust before any runtime chunk, exactly as `pmacs.command` + and `pmacs.hook` do, so the chunk had nothing to hold — and the one + helper it might have held would have broken acceptance 9 by capturing + every builtin define's `SourceLocation` at `config.lua`. Acceptance 26 + rewritten from a now-vacuous claim. Net effect: this arc touches + `src/editor.rs` zero times. +- **F5's rejection point was unimplementable as written** (Q#CR10). + Revision 2 had `define` rejecting `StartupOnly` + `set_local`, but + `define` cannot know about a future call and `StartupOnly` is legal + alone. Moved to `set_local`, where the combination actually manifests. + Acceptance 24 rewritten. +- The registry's `frozen` flag is driven from the existing + `InitCompleteFlag` at write time rather than by a new `editor.rs` + call, which is what keeps the zero-touch claim above true. + --- ## Ground truth (as of `7bc0c61`) @@ -576,9 +599,22 @@ the posture `require_init_phase` (`mod.rs:663`) already hard-codes for Buffer-locals are set at runtime, from `buffer.after-load` hooks that fire long after the freeze — so a `StartupOnly` key could never carry a buffer-local override, and a `set_local` against one would be dead code -that looks live. `define` rejects the combination outright with -`StartupOnlyLocal` rather than shipping a knob whose two halves -contradict each other. +that looks live. + +**Corrected in revision 3.** Revision 2 said "`define` rejects the +combination outright", which is not implementable: `define` cannot know +about a future `set_local`, and `StartupOnly` is perfectly legal on its +own. The rejection lives where the combination actually manifests — +**`set_local` returns `StartupOnlyLocal` when the named definition is +`StartupOnly`**, unconditionally and independent of freeze state, so the +error is the same before and after startup rather than changing shape +mid-session. + +`reset(name, None)` after the freeze is also refused for a `StartupOnly` +key: dropping a frozen override would let the default silently reassert +itself post-freeze, which is the same hazard `set` is blocked for. +Buffer-local `reset` needs no such check, since such a key can never +hold a local override in the first place. The ordering that makes define-before-set safe is the corrected sequence in ground truth: builtins define during `EditorState::new()`, user @@ -598,8 +634,18 @@ later-defined names is deferred. write `info["local"]`. The field is present only when a buffer argument is given and that buffer holds an override. -`pmacs.config.list()` returns fresh metadata tables, deterministic by -key. Neither ever exposes an internal table or a listener function. +`pmacs.config.list()` returns fresh metadata tables in **definition +order**, matching `names()` and the command/hook registries' "stable +listing" rule. Neither `describe` nor `list` ever exposes an internal +table or a listener function. + +Revision 3 clarification: revision 2 said "deterministic by key", which +implementation read as possibly meaning sorted-by-name. The requirement +is *determinism* — never `HashMap` iteration order — and definition +order satisfies it while staying consistent with the two sibling +registries. A UI that wants alphabetical sorts at the presentation +layer; the substrate does not decide that. Bounds are exposed as flat +`min` / `max` fields, not a nested `bounds` table. `M-x describe-setting` renders through `src/help.rs`, following `render_hook` / `format_hook_text` (`help.rs:160`, `:170`) so the @@ -640,13 +686,39 @@ Stage 2: unify the four daemon constants into a resolved value threaded through the display-column functions as a parameter — they are pure functions today and should stay pure — then answer the GPU question. -### Q#CR14 — Surface in `config.lua`; each module defines its own keys +### Q#CR14 — No runtime chunk; each module defines its own keys -`builtin/runtime/config.lua` holds the friendly Lua surface only, loaded -in `EditorState::new()` immediately after `fs.lua` (`editor.rs:206`) and -before every module that defines or reads a setting — in particular -before `pair.lua` (`editor.rs:319`), whose own load-before-`lsp.lua` -contract (`editor.rs:325`) is unaffected. +**Revised in revision 3.** Revision 2 put a "friendly Lua surface" in +`builtin/runtime/config.lua`, loaded after `fs.lua`. Implementation +established there is nothing for that file to hold, so **it does not +exist**. + +Two facts kill it. First, `pmacs.config` is installed entirely from +Rust by `attach_editor`, which runs before *any* `builtin/runtime/*.lua` +chunk is evaluated in `EditorState::new()` — so the bindings are already +the friendly surface, with no raw underscore layer needing a Lua wrapper +the way `pmacs._async` needs `async.lua`. `pmacs.command` and +`pmacs.hook` are the precedent: both are pure-Rust surfaces with no +runtime chunk. Second, the one helper such a file might plausibly hold — +a shared `define`-wrapping convenience — would actively break acceptance +9, because `SourceLocation` is captured from Lua debug info at the +`define` call site, so routing every builtin define through a helper in +`config.lua` would make every builtin setting report `config.lua` as its +source instead of its owning module. + +The module documentation that revision 2 assigned to that chunk lives in +the `src/lua_bindings/config.rs` module header instead. + +Builtin `define` calls live with their owning modules (F10), not +centralized. `pair.lua` defines `editing.auto-pair`, `editops.lua` +defines `editing.trim-on-save`, `autosave.lua` defines +`autosave.interval-ms`. `pair.lua`'s load-before-`lsp.lua` contract +(`editor.rs:319`, `:325`) is untouched, and because the bindings install +ahead of every chunk, no load-order constraint is added by this arc at +all. + +Consequence worth noting for the concurrent vterm lane: this arc now +touches `src/editor.rs` **zero** times. **Builtin `define` calls live with their owning modules (F10)**, not centralized in `config.lua`. Revision 1 centralized them, which would @@ -762,8 +834,9 @@ Registry semantics (unit, `src/config_registry.rs`): 6. Integer and number finite/boundary cases are exact under **both** `--features luajit` (default) and `--no-default-features --features lua54`. -7. `list` is deterministic by key; `describe`/`list` never expose an - internal mutable table or a listener function. +7. `list` returns definition order (never `HashMap` order); + `describe`/`list` never expose an internal mutable table or a + listener function, and each call returns a fresh table. 8. `names()` / `list()` order is stable across ≥3 defines. 9. `SourceLocation` is captured from the *defining module's* chunk and renders `file:line` — `editing.auto-pair` reports `pair.lua`, not @@ -817,13 +890,20 @@ Startup and mutability: `InitCompleteFlag` explicitly** (`mod.rs:14655-14690` is the pattern) — in `--lib` builds `set_init_complete` never runs, so a test that omits this passes vacuously. -24. `define` rejects `mutability = 'startup'` combined with any - `set_local` attempt, via `StartupOnlyLocal` (F5). +24. `set_local` against a `StartupOnly` definition returns + `StartupOnlyLocal` both before and after the freeze (F5, corrected + in revision 3 — `define` cannot police a future call). A + `StartupOnly` global `reset` after the freeze is refused for the + same reason `set` is. 25. A failing `init.lua` preserves prior successful sets and still starts the editor; missing `init.lua`, broken `init.lua` and `require` package-path behavior are unchanged. -26. `config.lua` is installed before `pair.lua`, and `pair.lua` still - loads before `lsp.lua` (the Q#AP7 contract is untouched). +26. The `pmacs.config` table is populated before the first + `builtin/runtime/*.lua` chunk evaluates — a builtin module's + top-level `define` call succeeds — and `pair.lua` still loads before + `lsp.lua` (the Q#AP7 contract is untouched). Revision 3: this + replaces "`config.lua` is installed before `pair.lua`", which became + vacuous when that chunk was removed (Q#CR14). Adopters: @@ -839,9 +919,18 @@ Adopters: buffer-locally suppresses it in that buffer only, with pairing still active in a second buffer of the same language. 30. The autosave tick observes a mid-session interval change without a - restart (the existing live-re-read contract, re-pinned). -31. Neither adopter performs a per-frame or per-cell Lua lookup, and - both preserve their previous default behavior exactly. + restart (the existing live-re-read contract, re-pinned), whether the + change arrived through the wrapper or through a direct + `pmacs.config.set`. +31. No adopter builds a Lua table or performs a string-keyed lookup + **per cell or per rendered line**, and all three preserve their + previous default behavior exactly. Revision 3 note: this item and + item 30 read as contradictory in revision 2 — 30 *requires* the + autosave tick to re-read every frame while 31 forbade a "per-frame + lookup". Q#CR15's prohibition targets render hot paths, not a single + O(1) scalar `get` once per tick, which is exactly what Q#CR8 asks + autosave to prove. One scalar `get` per tick is explicitly + conforming. Discovery: diff --git a/src/config_registry.rs b/src/config_registry.rs new file mode 100644 index 0000000..964dbd2 --- /dev/null +++ b/src/config_registry.rs @@ -0,0 +1,2124 @@ +// config_registry.rs --- Typed, two-scope, introspectable configuration registry. + +//! Configuration settings. +//! +//! Per `docs/config-registry-framing.md` (revision 2), every editor +//! setting is a named, typed [`ConfigDefinition`] registered in a +//! [`ConfigRegistry`], mirroring [`crate::command::CommandRegistry`] +//! and [`crate::hook::HookRegistry`] in shape and error vocabulary: +//! R42 (mandatory description), duplicate-rejection-over-silent- +//! overwrite, and a [`SourceLocation`] captured at definition time. +//! R50 (unknown-field rejection) is enforced by the Lua bindings lane +//! for this registry, since it is the layer that sees the raw spec +//! table --- see [`ConfigError::UnknownField`]. +//! +//! # Scopes +//! +//! Exactly two: a global override and, per [`BufferId`], a +//! buffer-local override. [`ConfigRegistry::get`] resolves +//! buffer-local -> global -> default when given a buffer, and +//! global -> default only when given `None` --- there is no ambient +//! "current buffer" at this layer (Q#CR4, F9). +//! +//! # Storage versus change notification (F1) +//! +//! These are two different questions, and conflating them breaks the +//! flagship buffer-local-pin feature: +//! +//! * [`ConfigRegistry::set`] and [`ConfigRegistry::set_local`] +//! **always store** the override, even when it is equal to the +//! value it shadows. +//! * The value epoch advances, and a listener dispatch should happen, +//! **only when the effective value changes** --- reported back via +//! [`ConfigChange::changed`]. +//! +//! Concretely: with `editing.auto-pair` globally `true`, pinning one +//! buffer via `set_local(buf, "editing.auto-pair", true)` must still +//! record an override for that buffer, so that a later +//! `set("editing.auto-pair", false)` does not silently flip the +//! pinned buffer. `equal_valued_local_override_is_still_stored_and_shields_buffer` +//! in the tests below is written to fail against a "true no-op" +//! implementation that declines to store the equal-valued override. +//! +//! # The value-validation seam +//! +//! This module owns *value-level* validation: a [`ConfigValue`] +//! against a [`ConfigKind`] (type match, bounds, enum membership, +//! string emptiness, number finiteness). The Lua bindings lane owns +//! *Lua-level* validation: converting a raw Lua value into a +//! [`ConfigValue`], strict spec-table parsing, and R50 rejection. +//! [`ConfigRegistry::validate`] is the seam --- it lets the bindings +//! lane check a candidate value against an already-registered +//! definition without committing anything, and [`ConfigRegistry::set`] +//! / [`ConfigRegistry::set_local`] call the same path internally, so +//! there is exactly one place the check can drift. For the one +//! numeric-exactness question that must be identical across both Lua +//! backends (`LuaJIT` numbers are always `f64`; Lua 5.4 numbers may +//! carry a native integer subtype), [`ConfigValue::int_from_f64`] +//! gives the bindings lane a by-value exactness check that never +//! consults `math.type`. +//! +//! # Listeners +//! +//! [`ConfigRegistry::on_change`] registers a callback against a +//! setting name and returns a generation-safe `u64` id; +//! [`ConfigRegistry::dispose`] removes it, idempotently. This registry +//! never invokes a listener itself --- [`ConfigRegistry::snapshot`] +//! hands the caller an owned `Vec` so it can drop the +//! registry borrow before re-entering Lua, exactly as +//! [`crate::hook::HookRegistry::snapshot`] does. Listeners persist +//! until explicitly disposed; there is no GC-timed lifetime (Q#CR6, +//! F3) --- a dropped-but-undisposed handle keeps firing. +//! +//! # Mutability and the startup freeze +//! +//! A [`ConfigMutability::StartupOnly`] key accepts writes until +//! [`ConfigRegistry::freeze`] is called (at `set_init_complete` time) +//! and rejects them after, via [`ConfigError::StartupOnlyAfterFreeze`]. +//! `StartupOnly` and [`ConfigRegistry::set_local`] are mutually +//! exclusive: a buffer-local write against a `StartupOnly` key is +//! always rejected with [`ConfigError::StartupOnlyLocal`], independent +//! of freeze state, because buffer-locals are only ever set at +//! runtime, long after any freeze (Q#CR10, F5). +//! +//! # Buffer-local lifecycle +//! +//! [`ConfigRegistry::remove_buffer`] drops a buffer's entire local +//! map. It fires no listener (Q#CR6 (c)): the buffer is gone, so there +//! is no effective value left for anyone to observe. Callers that skip +//! this (a direct buffer-registry removal bypassing the normal +//! choke point) leak that buffer's locals permanently but harmlessly, +//! since `BufferId`s are never reused (Q#CR5, F8) --- this module does +//! not and cannot fix that; it only owns the happy path. +//! +//! # Threading +//! +//! Single-threaded, like the command / hook registries. Lives behind +//! `Rc>` as Lua app data. + +use std::collections::{HashMap, HashSet}; + +use mlua::Function; +use thiserror::Error; + +use crate::buffer::BufferId; +use crate::command::SourceLocation; + +// --------------------------------------------------------------------------- +// Value vocabulary +// --------------------------------------------------------------------------- + +/// The closed value-kind vocabulary (Q#CR3). Deliberately has no +/// list/table variant --- `string-list` was dropped from stage 1 +/// (framing F6) and table-valued settings are a deferred arc. +#[derive(Clone, Debug, PartialEq)] +pub enum ConfigKind { + /// A `true`/`false` value. + Boolean, + /// A signed 64-bit integer, with optional inclusive bounds. + Integer { + /// Inclusive lower bound, if any. + min: Option, + /// Inclusive upper bound, if any. + max: Option, + }, + /// A finite `f64`, with optional inclusive bounds. Bounds must + /// themselves be finite (checked at `define` time). + Number { + /// Inclusive lower bound, if any. + min: Option, + /// Inclusive upper bound, if any. + max: Option, + }, + /// A UTF-8 string. + String { + /// Whether an empty string is a valid value. + allow_empty: bool, + }, + /// A closed set of string choices. Values are stored as + /// [`ConfigValue::Str`] and validated against `choices`. + Enum { + /// The valid choices. Must be non-empty and duplicate-free + /// (checked at `define` time). + choices: Vec, + }, +} + +impl ConfigKind { + /// Stable type-name string used in error messages. Matches + /// [`ConfigValue::type_name`]'s vocabulary except for `Enum`, + /// whose values are physically strings but whose *kind* is more + /// useful to name than its storage representation. + #[must_use] + pub fn type_name(&self) -> &'static str { + match self { + Self::Boolean => "boolean", + Self::Integer { .. } => "integer", + Self::Number { .. } => "number", + Self::String { .. } => "string", + Self::Enum { .. } => "enum", + } + } + + /// Validate that the kind's own constraints are internally + /// consistent, independent of any candidate value: finite bounds, + /// `min <= max`, and (for `Enum`) duplicate-free choices. Called + /// once at `define` time, before the default is checked against + /// the kind. + fn validate_self(&self, name: &str) -> Result<(), ConfigError> { + match self { + Self::Boolean | Self::String { .. } => Ok(()), + Self::Integer { min, max } => { + if let (Some(lo), Some(hi)) = (min, max) + && lo > hi + { + return Err(ConfigError::OutOfRange { + name: name.to_owned(), + detail: format!("minimum {lo} exceeds maximum {hi}"), + }); + } + Ok(()) + } + Self::Number { min, max } => { + for bound in [*min, *max].into_iter().flatten() { + if !bound.is_finite() { + return Err(ConfigError::NonFiniteNumber { + name: name.to_owned(), + value: bound, + }); + } + } + if let (Some(lo), Some(hi)) = (min, max) + && lo > hi + { + return Err(ConfigError::OutOfRange { + name: name.to_owned(), + detail: format!("minimum {lo} exceeds maximum {hi}"), + }); + } + Ok(()) + } + Self::Enum { choices } => { + // Rejected directly rather than left to the default's + // own `NotAChoice` failure: an empty choice list is a + // malformed *definition*, and reporting it as "the + // default is not one of []" sends the author looking at + // the wrong field. + if choices.is_empty() { + return Err(ConfigError::EmptyChoices { + name: name.to_owned(), + }); + } + let mut seen = HashSet::new(); + for choice in choices { + if !seen.insert(choice.as_str()) { + return Err(ConfigError::DuplicateChoice { + name: name.to_owned(), + choice: choice.clone(), + }); + } + } + Ok(()) + } + } + } + + /// Validate `value` against this kind: type match, numeric bounds, + /// enum membership, string emptiness, and number finiteness. Pure + /// value-vs-kind logic with no knowledge of scope --- resolving + /// which layer a value lives in is [`ConfigRegistry`]'s job. This + /// is the half of validation this module owns (see the module + /// doc's "value-validation seam" section); the Lua bindings lane + /// calls it (indirectly, via [`ConfigRegistry::validate`]) after + /// converting a raw Lua value into a [`ConfigValue`]. + pub fn validate(&self, name: &str, value: &ConfigValue) -> Result<(), ConfigError> { + match (self, value) { + (Self::Boolean, ConfigValue::Bool(_)) => Ok(()), + (Self::Integer { min, max }, ConfigValue::Int(v)) => { + if let Some(lo) = min + && v < lo + { + return Err(ConfigError::OutOfRange { + name: name.to_owned(), + detail: format!("{v} is below the minimum {lo}"), + }); + } + if let Some(hi) = max + && v > hi + { + return Err(ConfigError::OutOfRange { + name: name.to_owned(), + detail: format!("{v} is above the maximum {hi}"), + }); + } + Ok(()) + } + (Self::Number { min, max }, ConfigValue::Num(v)) => { + if !v.is_finite() { + return Err(ConfigError::NonFiniteNumber { + name: name.to_owned(), + value: *v, + }); + } + if let Some(lo) = min + && v < lo + { + return Err(ConfigError::OutOfRange { + name: name.to_owned(), + detail: format!("{v} is below the minimum {lo}"), + }); + } + if let Some(hi) = max + && v > hi + { + return Err(ConfigError::OutOfRange { + name: name.to_owned(), + detail: format!("{v} is above the maximum {hi}"), + }); + } + Ok(()) + } + (Self::String { allow_empty }, ConfigValue::Str(s)) => { + if !allow_empty && s.is_empty() { + return Err(ConfigError::EmptyString { + name: name.to_owned(), + }); + } + Ok(()) + } + (Self::Enum { choices }, ConfigValue::Str(s)) => { + if choices.iter().any(|c| c == s) { + Ok(()) + } else { + Err(ConfigError::NotAChoice { + name: name.to_owned(), + got: s.clone(), + choices: choices.clone(), + }) + } + } + _ => Err(ConfigError::TypeMismatch { + name: name.to_owned(), + expected: self.type_name(), + got: value.type_name(), + }), + } + } +} + +/// An owned configuration value. Lua tables, functions and userdata +/// are never stored --- only these four scalars (Q#CR3). +#[derive(Clone, Debug, PartialEq)] +pub enum ConfigValue { + /// A boolean value. + Bool(bool), + /// A signed 64-bit integer value. + Int(i64), + /// A finite `f64` value. + Num(f64), + /// A string value. Also used for `Enum`-kind values. + Str(String), +} + +impl ConfigValue { + /// Stable type-name string used in error messages. Matches + /// [`ConfigKind::type_name`]'s vocabulary, except an `Enum` + /// definition's values report as `"string"` here since that is + /// their physical representation. + #[must_use] + pub fn type_name(&self) -> &'static str { + match self { + Self::Bool(_) => "boolean", + Self::Int(_) => "integer", + Self::Num(_) => "number", + Self::Str(_) => "string", + } + } + + /// Construct a [`Self::Int`] from an `f64`, checking exactness + /// **by value**. This is the seam the Lua bindings lane needs for + /// cross-backend-identical integer handling: `LuaJIT` numbers are + /// always `f64` (Lua 5.1 has no integer subtype), Lua 5.4 numbers + /// may carry a native integer subtype --- and this function never + /// looks at which backend produced `v` or what `math.type` would + /// say, only at the numeric value itself, so both backends agree + /// byte-for-byte. + /// + /// # Errors + /// + /// [`ConfigError::NonFiniteNumber`] if `v` is `NaN` or infinite; + /// [`ConfigError::NonIntegral`] if `v` has a fractional part or + /// falls outside the range exactly representable as an `i64`. + pub fn int_from_f64(name: &str, v: f64) -> Result { + if !v.is_finite() { + return Err(ConfigError::NonFiniteNumber { + name: name.to_owned(), + value: v, + }); + } + if v.fract() != 0.0 || v < i64::MIN as f64 || v > i64::MAX as f64 { + return Err(ConfigError::NonIntegral { + name: name.to_owned(), + value: v, + }); + } + Ok(Self::Int(v as i64)) + } +} + +/// When a definition's value may be written (Q#CR10). +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ConfigMutability { + /// Writable at any time, including after the startup freeze. + Live, + /// Writable only while user config is loading. A write after + /// [`ConfigRegistry::freeze`] returns + /// [`ConfigError::StartupOnlyAfterFreeze`]. Mutually exclusive + /// with [`ConfigRegistry::set_local`], which always returns + /// [`ConfigError::StartupOnlyLocal`] for a key of this mutability + /// (F5): buffer-locals are only ever set at runtime, long after + /// any freeze. + StartupOnly, +} + +// --------------------------------------------------------------------------- +// Definitions and listeners +// --------------------------------------------------------------------------- + +/// A registered configuration setting. +#[derive(Clone, Debug)] +pub struct ConfigDefinition { + /// Unique, dotted, kebab-cased name (e.g. `editing.auto-pair`). + pub name: String, + /// One-line human-readable description (R42, mandatory, + /// non-empty after trim). + pub description: String, + /// The value's type and constraints. + pub kind: ConfigKind, + /// The value used when no override applies at any scope. + pub default: ConfigValue, + /// When the value may be written. + pub mutability: ConfigMutability, + /// Where the call to `pmacs.config.define` originated. + pub source: SourceLocation, +} + +/// One callback registered against a setting name via +/// [`ConfigRegistry::on_change`]. +/// +/// Cloning is cheap: `String`s clone trivially and `mlua::Function` is +/// reference-counted internally. +#[derive(Clone)] +pub struct ConfigListener { + /// Generation-safe id, never reused. Returned by `on_change` and + /// consumed by [`ConfigRegistry::dispose`]. + pub id: u64, + /// The setting name this listener watches. + pub name: String, + /// The Lua callback body, invoked as `function(new, old, buf)`. + pub body: Function, + /// Where the call to `pmacs.config.on_change` originated. + pub source: SourceLocation, +} + +/// Outcome of a [`ConfigRegistry::set`], [`ConfigRegistry::set_local`] +/// or [`ConfigRegistry::reset`] call: the effective value immediately +/// before and after, and whether it actually changed (F1). The +/// override itself is always stored (or, for `reset`, dropped) +/// regardless of `changed` --- only epoch advancement and listener +/// dispatch key on it. A caller dispatching `on_change` uses `new`/ +/// `old` directly as the listener's `(new, old, buf)` arguments. +#[derive(Clone, Debug, PartialEq)] +pub struct ConfigChange { + /// `true` iff the effective value differs from before the call. + pub changed: bool, + /// The effective value immediately before this call. + pub old: ConfigValue, + /// The effective value immediately after this call. Equal to + /// `old` when `changed` is `false`. + pub new: ConfigValue, +} + +// --------------------------------------------------------------------------- +// Errors +// --------------------------------------------------------------------------- + +/// Errors raised by the config registry. +#[derive(Debug, Error)] +pub enum ConfigError { + /// `pmacs.config.define` was called with no name. + #[error("config name must be non-empty")] + EmptyName, + + /// Q#CR9: the name failed the dotted, kebab-cased grammar + /// (`[a-z][a-z0-9]*(-[a-z0-9]+)*` per dot-separated segment) or + /// exceeded the 128-byte length bound. + #[error("config name \"{name}\" is invalid: {reason}")] + InvalidName { + /// The offending name. + name: String, + /// Which grammar rule it broke. + reason: String, + }, + + /// R42: `define` was called without a description, or with one + /// that's empty after trimming. + #[error("config \"{name}\" requires a non-empty description (R42)")] + MissingDescription { + /// The offending config name. + name: String, + }, + + /// A config with this name is already defined with a different + /// specification. A byte-for-byte identical redefinition is + /// idempotent and does **not** raise this (Q#CR10). + #[error( + "config \"{name}\" is already defined with a different specification (refusing to redefine)" + )] + DuplicateName { + /// The offending config name. + name: String, + }, + + /// `get`/`set`/`set_local`/`reset`/`is_set`/`on_change` referenced + /// a name that has not been `define`d (Q#CR10: define-before-use). + #[error("config \"{name}\" is not defined")] + NotFound { + /// The offending config name. + name: String, + }, + + /// A value's runtime type does not match its definition's + /// [`ConfigKind`]. + #[error("config \"{name}\" expects a {expected} value, got {got}")] + TypeMismatch { + /// The offending config name. + name: String, + /// The kind's declared type name. + expected: &'static str, + /// The candidate value's type name. + got: &'static str, + }, + + /// An integer or number value fell outside its definition's + /// declared `min`/`max`, or the bounds themselves are inverted + /// (`min > max`) at define time. + #[error("config \"{name}\" is out of range: {detail}")] + OutOfRange { + /// The offending config name. + name: String, + /// A human-readable description of which bound was broken. + detail: String, + }, + + /// An enum-kind value is not one of its definition's `choices`. + #[error("config \"{name}\" value \"{got}\" is not one of: {choices:?}")] + NotAChoice { + /// The offending config name. + name: String, + /// The rejected value. + got: String, + /// The definition's full choice list. + choices: Vec, + }, + + /// An enum-kind definition listed the same choice twice. Not part + /// of the caller-given error vocabulary; added because the + /// framing's acceptance list (item 2) explicitly requires + /// rejecting this at define time and none of the other variants + /// name it precisely. + #[error("config \"{name}\" enum choices contain a duplicate: \"{choice}\"")] + DuplicateChoice { + /// The offending config name. + name: String, + /// The choice that appeared more than once. + choice: String, + }, + + /// An enum-kind definition listed no choices at all. Rejected + /// directly so the author is pointed at `choices`, rather than + /// indirectly via the default failing `NotAChoice` against an empty + /// list. + #[error("config \"{name}\" is an enum with no choices; `choices` must be non-empty")] + EmptyChoices { + /// The offending config name. + name: String, + }, + + /// A string-kind value was empty and the definition's + /// `allow_empty` is `false`. + #[error("config \"{name}\" does not allow an empty string")] + EmptyString { + /// The offending config name. + name: String, + }, + + /// A number-kind value, or a number-kind bound, was not finite + /// (`NaN` or infinite). + #[error("config \"{name}\" requires a finite number, got {value}")] + NonFiniteNumber { + /// The offending config name. + name: String, + /// The non-finite value. + value: f64, + }, + + /// An integer-kind candidate did not represent an exact integer + /// **by value** --- see [`ConfigValue::int_from_f64`]. + #[error("config \"{name}\" requires an exact integer, got {value}")] + NonIntegral { + /// The offending config name. + name: String, + /// The non-integral (or out-of-`i64`-range) candidate. + value: f64, + }, + + /// Q#CR10/F5: `set_local` targeted a + /// [`ConfigMutability::StartupOnly`] definition. Always rejected, + /// independent of freeze state. + #[error("config \"{name}\" is startup-only and cannot carry a buffer-local override")] + StartupOnlyLocal { + /// The offending config name. + name: String, + }, + + /// Q#CR10: `set`/`reset` targeted a + /// [`ConfigMutability::StartupOnly`] definition after + /// [`ConfigRegistry::freeze`] was called. + #[error("config \"{name}\" is startup-only and cannot be written after startup completes")] + StartupOnlyAfterFreeze { + /// The offending config name. + name: String, + }, + + /// R50: the Lua bindings lane found a key in a raw spec table that + /// this registry doesn't know about (or the key's value came from + /// a metatable rather than the table itself). Raised by the + /// bindings lane, not by this module directly --- `supported` is + /// the lane's own field list, pre-joined into the message the way + /// [`crate::hook::HookError::UnknownField`] hardcodes its own. + #[error("unknown field `{field}` in config spec; supported: {supported}")] + UnknownField { + /// The offending key. + field: String, + /// The bindings lane's supported-field list. + supported: String, + }, +} + +// --------------------------------------------------------------------------- +// Name grammar +// --------------------------------------------------------------------------- + +/// Maximum byte length of a config name (Q#CR9). +const MAX_NAME_LEN: usize = 128; + +/// Validate the dotted, kebab-cased name grammar (Q#CR9): each +/// dot-separated segment matches `[a-z][a-z0-9]*(-[a-z0-9]+)*`, ASCII +/// only, at most [`MAX_NAME_LEN`] bytes total. Deliberately rejects a +/// trailing hyphen, a doubled hyphen, an empty segment, and a leading +/// digit. +fn validate_name(name: &str) -> Result<(), ConfigError> { + if name.is_empty() { + return Err(ConfigError::EmptyName); + } + if name.len() > MAX_NAME_LEN { + return Err(ConfigError::InvalidName { + name: name.to_owned(), + reason: format!("exceeds the {MAX_NAME_LEN}-byte length limit"), + }); + } + for segment in name.split('.') { + if let Err(reason) = validate_segment(segment) { + return Err(ConfigError::InvalidName { + name: name.to_owned(), + reason: reason.to_owned(), + }); + } + } + Ok(()) +} + +/// Validate one dot-separated segment against +/// `[a-z][a-z0-9]*(-[a-z0-9]+)*`. +fn validate_segment(segment: &str) -> Result<(), &'static str> { + let mut chars = segment.chars(); + let Some(first) = chars.next() else { + return Err("contains an empty segment"); + }; + if !first.is_ascii_lowercase() { + return Err("each segment must start with a lowercase letter"); + } + let mut prev_hyphen = false; + let mut trailing_hyphen = false; + for c in chars { + if c == '-' { + if prev_hyphen { + return Err("contains a doubled hyphen"); + } + prev_hyphen = true; + trailing_hyphen = true; + } else if c.is_ascii_lowercase() || c.is_ascii_digit() { + prev_hyphen = false; + trailing_hyphen = false; + } else { + return Err("contains a character outside a-z, 0-9, and hyphen"); + } + } + if trailing_hyphen { + return Err("ends with a trailing hyphen"); + } + Ok(()) +} + +// --------------------------------------------------------------------------- +// ConfigRegistry +// --------------------------------------------------------------------------- + +/// Registry of named, typed configuration settings with global and +/// buffer-local override scopes. +/// +/// Construction goes through [`Self::new`] / [`Self::default`]. Define +/// via [`Self::define`]; read via [`Self::get`]; write via +/// [`Self::set`] / [`Self::set_local`]; drop an override via +/// [`Self::reset`]. See the module doc for the storage-versus-epoch +/// split (F1) and the resolution rules (F9). +#[derive(Default)] +pub struct ConfigRegistry { + by_name: HashMap, + /// Definition order, for stable listing. + order: Vec, + /// Global overrides only. Absence means "fall through to + /// default", not "value is the default". + global: HashMap, + /// Per-buffer overrides only, keyed the same way as `global`. + locals: HashMap>, + /// Registered `on_change` listeners, in registration order. + listeners: Vec, + next_listener_id: u64, + frozen: bool, + definition_epoch: u64, + value_epoch: u64, +} + +impl ConfigRegistry { + /// An empty registry. + #[must_use] + pub fn new() -> Self { + Self::default() + } + + // -- definitions --------------------------------------------------- + + /// Define a new setting. + /// + /// Validates, in order: the name grammar (Q#CR9), R42 (non-empty + /// description), the kind's internal consistency (finite bounds, + /// `min <= max`, duplicate-free enum choices), and the default + /// against its own kind. A byte-for-byte identical redefinition + /// (same description, kind, default and mutability) succeeds as a + /// no-op, supporting idempotent config reload; a conflicting + /// redefinition fails with [`ConfigError::DuplicateName`] and + /// leaves the original definition --- including its overrides and + /// its source location --- exactly as it was. + /// + /// # Errors + /// + /// See the variants above; nothing is mutated and neither epoch + /// advances on any error path. + pub fn define( + &mut self, + name: String, + description: String, + kind: ConfigKind, + default: ConfigValue, + mutability: ConfigMutability, + source: SourceLocation, + ) -> Result<(), ConfigError> { + validate_name(&name)?; + if description.trim().is_empty() { + return Err(ConfigError::MissingDescription { name }); + } + kind.validate_self(&name)?; + kind.validate(&name, &default)?; + + if let Some(existing) = self.by_name.get(&name) { + if existing.description == description + && existing.kind == kind + && existing.default == default + && existing.mutability == mutability + { + return Ok(()); + } + return Err(ConfigError::DuplicateName { name }); + } + + self.order.push(name.clone()); + self.by_name.insert( + name.clone(), + ConfigDefinition { + name, + description, + kind, + default, + mutability, + source, + }, + ); + self.definition_epoch = self.definition_epoch.saturating_add(1); + Ok(()) + } + + fn definition(&self, name: &str) -> Result<&ConfigDefinition, ConfigError> { + self.by_name.get(name).ok_or_else(|| ConfigError::NotFound { + name: name.to_owned(), + }) + } + + /// Look up a definition by name. + #[must_use] + pub fn get_definition(&self, name: &str) -> Option<&ConfigDefinition> { + self.by_name.get(name) + } + + /// True iff `name` is defined. + #[must_use] + pub fn contains(&self, name: &str) -> bool { + self.by_name.contains_key(name) + } + + /// Names in definition order. + #[must_use] + pub fn names(&self) -> &[String] { + &self.order + } + + /// Number of defined settings. + #[must_use] + pub fn len(&self) -> usize { + self.by_name.len() + } + + /// True iff no settings are defined. + #[must_use] + pub fn is_empty(&self) -> bool { + self.by_name.is_empty() + } + + /// Validate `value` against `name`'s definition without storing + /// anything. The seam the Lua bindings lane uses once it has + /// converted a raw Lua value into a [`ConfigValue`]: call this to + /// get a properly-vocabularied [`ConfigError`] before deciding + /// whether to call [`Self::set`] / [`Self::set_local`] --- though + /// calling `set`/`set_local` directly is also fine, since they run + /// the identical check internally. + /// + /// # Errors + /// + /// [`ConfigError::NotFound`] if `name` is undefined; otherwise + /// whatever [`ConfigKind::validate`] returns. + pub fn validate(&self, name: &str, value: &ConfigValue) -> Result<(), ConfigError> { + let def = self.definition(name)?; + def.kind.validate(name, value) + } + + // -- resolution ------------------------------------------------------ + + /// Resolve the effective value of `name`. + /// + /// With `buf`, resolution is buffer-local -> global -> default. + /// With `None`, resolution is global -> default **only** --- there + /// is no ambient "current buffer" at this layer (Q#CR4, F9): a + /// caller that wants buffer-aware behavior must pass the buffer. + /// + /// # Errors + /// + /// [`ConfigError::NotFound`] if `name` is undefined. + pub fn get(&self, name: &str, buf: Option) -> Result<&ConfigValue, ConfigError> { + let def = self.definition(name)?; + if let Some(id) = buf + && let Some(v) = self.locals.get(&id).and_then(|m| m.get(name)) + { + return Ok(v); + } + Ok(self.global.get(name).unwrap_or(&def.default)) + } + + /// True iff an override is present for `name` at the queried + /// layer: the buffer-local layer if `buf` is given, the global + /// layer otherwise. Well-defined precisely because overrides are + /// always stored (F1) --- including one equal to the value it + /// shadows. + /// + /// # Errors + /// + /// [`ConfigError::NotFound`] if `name` is undefined. + pub fn is_set(&self, name: &str, buf: Option) -> Result { + self.definition(name)?; + Ok(match buf { + Some(id) => self.locals.get(&id).is_some_and(|m| m.contains_key(name)), + None => self.global.contains_key(name), + }) + } + + /// The global override for `name`, if one is stored. `None` means + /// "falls through to default", distinct from an override that + /// happens to equal the default. + #[must_use] + pub fn global_override(&self, name: &str) -> Option<&ConfigValue> { + self.global.get(name) + } + + /// The buffer-local override for `name` on `buf`, if one is + /// stored. + #[must_use] + pub fn local_override(&self, name: &str, buf: BufferId) -> Option<&ConfigValue> { + self.locals.get(&buf).and_then(|m| m.get(name)) + } + + // -- writes ------------------------------------------------------------ + + /// Set the global override for `name`. + /// + /// The override is stored unconditionally, even if `value` equals + /// the value it shadows (F1). The returned [`ConfigChange`] tells + /// the caller whether the *effective* global value actually + /// changed; the value epoch advances iff it did. + /// + /// # Errors + /// + /// [`ConfigError::NotFound`] if `name` is undefined; + /// [`ConfigError::StartupOnlyAfterFreeze`] if the definition is + /// [`ConfigMutability::StartupOnly`] and [`Self::freeze`] has + /// already been called; otherwise whatever [`Self::validate`] + /// returns for `value`. + pub fn set(&mut self, name: &str, value: ConfigValue) -> Result { + let (mutability, default) = { + let def = self.definition(name)?; + (def.mutability, def.default.clone()) + }; + if mutability == ConfigMutability::StartupOnly && self.frozen { + return Err(ConfigError::StartupOnlyAfterFreeze { + name: name.to_owned(), + }); + } + self.validate(name, &value)?; + + let old = self.global.get(name).cloned().unwrap_or(default); + let new = value.clone(); + let changed = old != new; + self.global.insert(name.to_owned(), value); + if changed { + self.value_epoch = self.value_epoch.saturating_add(1); + } + Ok(ConfigChange { changed, old, new }) + } + + /// Set the buffer-local override for `name` on `buf`. + /// + /// The override is stored unconditionally, even if `value` equals + /// the value it shadows --- this is the storage half of F1's fix: + /// a buffer pinned to the current global value must stay pinned + /// when the global value later changes. The returned + /// [`ConfigChange`] reports whether `buf`'s effective value + /// changed; the value epoch advances iff it did. + /// + /// # Errors + /// + /// [`ConfigError::NotFound`] if `name` is undefined; + /// [`ConfigError::StartupOnlyLocal`] if the definition is + /// [`ConfigMutability::StartupOnly`] (always, independent of + /// freeze state); otherwise whatever [`Self::validate`] returns + /// for `value`. + pub fn set_local( + &mut self, + buf: BufferId, + name: &str, + value: ConfigValue, + ) -> Result { + let (mutability, default) = { + let def = self.definition(name)?; + (def.mutability, def.default.clone()) + }; + if mutability == ConfigMutability::StartupOnly { + return Err(ConfigError::StartupOnlyLocal { + name: name.to_owned(), + }); + } + self.validate(name, &value)?; + + let global_effective = self.global.get(name).cloned().unwrap_or(default); + let old = self + .locals + .get(&buf) + .and_then(|m| m.get(name)) + .cloned() + .unwrap_or(global_effective); + let new = value.clone(); + let changed = old != new; + self.locals + .entry(buf) + .or_default() + .insert(name.to_owned(), value); + if changed { + self.value_epoch = self.value_epoch.saturating_add(1); + } + Ok(ConfigChange { changed, old, new }) + } + + /// Drop exactly one override layer for `name`. + /// + /// With `buf`, drops only that buffer's local override and + /// re-exposes the global chain. With `None`, drops the global + /// override and re-exposes the default. The value epoch advances + /// iff the effective value actually changes as a result. + /// + /// # Errors + /// + /// [`ConfigError::NotFound`] if `name` is undefined; + /// [`ConfigError::StartupOnlyAfterFreeze`] if resetting the + /// *global* layer of a [`ConfigMutability::StartupOnly`] + /// definition after [`Self::freeze`]. + pub fn reset( + &mut self, + name: &str, + buf: Option, + ) -> Result { + let (mutability, default) = { + let def = self.definition(name)?; + (def.mutability, def.default.clone()) + }; + if let Some(id) = buf { + let global_effective = self.global.get(name).cloned().unwrap_or(default); + let old = self + .locals + .get(&id) + .and_then(|m| m.get(name)) + .cloned() + .unwrap_or_else(|| global_effective.clone()); + if let Some(m) = self.locals.get_mut(&id) { + m.remove(name); + } + let new = global_effective; + let changed = old != new; + if changed { + self.value_epoch = self.value_epoch.saturating_add(1); + } + return Ok(ConfigChange { changed, old, new }); + } + + if mutability == ConfigMutability::StartupOnly && self.frozen { + return Err(ConfigError::StartupOnlyAfterFreeze { + name: name.to_owned(), + }); + } + let old = self + .global + .get(name) + .cloned() + .unwrap_or_else(|| default.clone()); + self.global.remove(name); + let changed = old != default; + if changed { + self.value_epoch = self.value_epoch.saturating_add(1); + } + Ok(ConfigChange { + changed, + old, + new: default, + }) + } + + /// Drop `id`'s entire buffer-local override map. Called from the + /// existing `after_buffer_removed` choke point (Q#CR5). Fires no + /// listener (Q#CR6 (c)): the buffer is gone, so there is no + /// effective value left to observe. Does not advance the value + /// epoch either, for the same reason. + pub fn remove_buffer(&mut self, id: BufferId) { + self.locals.remove(&id); + } + + // -- listeners ----------------------------------------------------- + + /// Register `body` to run on future effective-value changes to + /// `name`. Returns a generation-safe id, never reused, for later + /// [`Self::dispose`]. + /// + /// # Errors + /// + /// [`ConfigError::NotFound`] if `name` is undefined (Q#CR6 (d)). + pub fn on_change( + &mut self, + name: &str, + body: Function, + source: SourceLocation, + ) -> Result { + self.definition(name)?; + let id = self.next_listener_id; + self.next_listener_id = self.next_listener_id.saturating_add(1); + self.listeners.push(ConfigListener { + id, + name: name.to_owned(), + body, + source, + }); + Ok(id) + } + + /// Remove a listener by id. Idempotent: disposing an id twice, or + /// an id that was never issued, is a no-op. Generation-safe: since + /// ids are never reused, a stale handle can never dispose a newer + /// listener that happens to reuse its slot. + pub fn dispose(&mut self, id: u64) { + self.listeners.retain(|l| l.id != id); + } + + /// Snapshot the listeners registered against `name`, in + /// registration order, so the caller can drop the registry borrow + /// before invoking Lua (which may re-enter the registry). This + /// registry never runs a listener itself --- mirrors + /// [`crate::hook::HookRegistry::snapshot`] exactly. + #[must_use] + pub fn snapshot(&self, name: &str) -> Vec { + self.listeners + .iter() + .filter(|l| l.name == name) + .cloned() + .collect() + } + + // -- startup freeze -------------------------------------------------- + + /// Flip the startup freeze. One-way in practice: called once, at + /// `set_init_complete` time, from the tail of `EditorState::new()`. + /// After this, a write to a [`ConfigMutability::StartupOnly`] + /// key's global layer returns + /// [`ConfigError::StartupOnlyAfterFreeze`]. + pub fn freeze(&mut self) { + self.frozen = true; + } + + /// True iff [`Self::freeze`] has been called. + #[must_use] + pub fn is_frozen(&self) -> bool { + self.frozen + } + + // -- epochs ------------------------------------------------------------ + + /// Advances by one on every successful [`Self::define`] of a new + /// name (not on an idempotent redefinition, which mutates + /// nothing). + #[must_use] + pub fn definition_epoch(&self) -> u64 { + self.definition_epoch + } + + /// Advances by one on every write that changes an *effective* + /// value --- never on a write that only stores an equal-valued + /// override (F1, acceptance 15). + #[must_use] + pub fn value_epoch(&self) -> u64 { + self.value_epoch + } +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + + fn src(line: i32) -> SourceLocation { + SourceLocation { + file: "test.lua".into(), + line, + } + } + + fn define_bool( + r: &mut ConfigRegistry, + name: &str, + default: bool, + mutability: ConfigMutability, + ) { + r.define( + name.into(), + "a boolean setting".into(), + ConfigKind::Boolean, + ConfigValue::Bool(default), + mutability, + src(1), + ) + .unwrap(); + } + + // ---- acceptance 1: round-trip every kind ------------------------------- + + #[test] + fn define_then_get_round_trips_every_kind() { + let mut r = ConfigRegistry::new(); + r.define( + "editing.auto-pair".into(), + "Pair brackets on insert.".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(1), + ) + .unwrap(); + r.define( + "autosave.interval-ms".into(), + "Autosave interval.".into(), + ConfigKind::Integer { + min: Some(1000), + max: None, + }, + ConfigValue::Int(30_000), + ConfigMutability::Live, + src(2), + ) + .unwrap(); + r.define( + "editing.fill-column".into(), + "Preferred wrap column.".into(), + ConfigKind::Number { + min: Some(1.0), + max: Some(1000.0), + }, + ConfigValue::Num(80.0), + ConfigMutability::Live, + src(3), + ) + .unwrap(); + r.define( + "editing.comment-prefix".into(), + "Comment prefix string.".into(), + ConfigKind::String { allow_empty: true }, + ConfigValue::Str(String::new()), + ConfigMutability::Live, + src(4), + ) + .unwrap(); + r.define( + "editing.line-ending".into(), + "Line ending style.".into(), + ConfigKind::Enum { + choices: vec!["lf".into(), "crlf".into()], + }, + ConfigValue::Str("lf".into()), + ConfigMutability::Live, + src(5), + ) + .unwrap(); + + assert_eq!( + r.get("editing.auto-pair", None).unwrap(), + &ConfigValue::Bool(true) + ); + assert_eq!( + r.get("autosave.interval-ms", None).unwrap(), + &ConfigValue::Int(30_000) + ); + assert_eq!( + r.get("editing.fill-column", None).unwrap(), + &ConfigValue::Num(80.0) + ); + assert_eq!( + r.get("editing.comment-prefix", None).unwrap(), + &ConfigValue::Str(String::new()) + ); + assert_eq!( + r.get("editing.line-ending", None).unwrap(), + &ConfigValue::Str("lf".into()) + ); + } + + // ---- acceptance 2: define-time rejections ------------------------------ + + #[test] + fn define_rejects_empty_name() { + let mut r = ConfigRegistry::new(); + let err = r + .define( + String::new(), + "x".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::EmptyName)); + assert_eq!(r.len(), 0); + assert_eq!(r.definition_epoch(), 0); + } + + #[test] + fn define_rejects_whitespace_only_description() { + let mut r = ConfigRegistry::new(); + let err = r + .define( + "editing.x".into(), + " \n\t ".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::MissingDescription { name } if name == "editing.x")); + assert_eq!(r.len(), 0); + } + + #[test] + fn define_rejects_conflicting_duplicate_without_mutating() { + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + let epoch_before = r.definition_epoch(); + + let err = r + .define( + "editing.x".into(), + "a different description".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(9), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::DuplicateName { name } if name == "editing.x")); + assert_eq!( + r.definition_epoch(), + epoch_before, + "no epoch advance on rejection" + ); + assert_eq!( + r.get_definition("editing.x").unwrap().description, + "a boolean setting" + ); + } + + #[test] + fn define_rejects_non_finite_bounds() { + let mut r = ConfigRegistry::new(); + let err = r + .define( + "editing.x".into(), + "x".into(), + ConfigKind::Number { + min: Some(f64::NAN), + max: None, + }, + ConfigValue::Num(1.0), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::NonFiniteNumber { name, .. } if name == "editing.x")); + assert_eq!(r.len(), 0); + } + + #[test] + fn define_rejects_inverted_range() { + let mut r = ConfigRegistry::new(); + let err = r + .define( + "editing.x".into(), + "x".into(), + ConfigKind::Integer { + min: Some(10), + max: Some(5), + }, + ConfigValue::Int(7), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::OutOfRange { name, .. } if name == "editing.x")); + assert_eq!(r.len(), 0); + } + + #[test] + fn define_rejects_an_enum_with_no_choices_pointedly() { + // Without the direct check this is still rejected, but only + // indirectly: the default fails `NotAChoice` against an empty + // list, which names the wrong field. Assert the pointed error + // so a regression to the indirect path is visible. + let mut r = ConfigRegistry::new(); + let err = r + .define( + "editing.x".into(), + "x".into(), + ConfigKind::Enum { choices: vec![] }, + ConfigValue::Str("a".into()), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!( + matches!(err, ConfigError::EmptyChoices { name } if name == "editing.x"), + "an empty choice list must be reported as such, not as a bad default" + ); + assert_eq!(r.len(), 0, "a rejected define registers nothing"); + } + + #[test] + fn define_rejects_duplicate_enum_choices() { + let mut r = ConfigRegistry::new(); + let err = r + .define( + "editing.x".into(), + "x".into(), + ConfigKind::Enum { + choices: vec!["a".into(), "b".into(), "a".into()], + }, + ConfigValue::Str("a".into()), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!( + matches!(err, ConfigError::DuplicateChoice { name, choice } if name == "editing.x" && choice == "a") + ); + assert_eq!(r.len(), 0); + } + + #[test] + fn define_rejects_default_violating_its_own_contract() { + let mut r = ConfigRegistry::new(); + let err = r + .define( + "editing.x".into(), + "x".into(), + ConfigKind::Integer { + min: Some(1000), + max: None, + }, + ConfigValue::Int(500), // below its own minimum + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::OutOfRange { name, .. } if name == "editing.x")); + assert_eq!(r.len(), 0); + + let err2 = r + .define( + "editing.y".into(), + "y".into(), + ConfigKind::Enum { + choices: vec!["lf".into(), "crlf".into()], + }, + ConfigValue::Str("cr".into()), // not a choice + ConfigMutability::Live, + src(2), + ) + .unwrap_err(); + assert!(matches!(err2, ConfigError::NotAChoice { name, .. } if name == "editing.y")); + } + + // ---- acceptance 3: name grammar edge cases ----------------------------- + + #[test] + fn define_rejects_trailing_hyphen() { + let mut r = ConfigRegistry::new(); + let err = r + .define( + "auto-".into(), + "x".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::InvalidName { .. })); + } + + #[test] + fn define_rejects_doubled_hyphen() { + let mut r = ConfigRegistry::new(); + let err = r + .define( + "a--b".into(), + "x".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::InvalidName { .. })); + } + + #[test] + fn define_rejects_empty_segment() { + let mut r = ConfigRegistry::new(); + let err = r + .define( + "editing..auto-pair".into(), + "x".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::InvalidName { .. })); + } + + #[test] + fn define_rejects_leading_digit() { + let mut r = ConfigRegistry::new(); + let err = r + .define( + "editing.1auto".into(), + "x".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::InvalidName { .. })); + } + + #[test] + fn define_rejects_overlength_name() { + let mut r = ConfigRegistry::new(); + let long = "a".repeat(129); + let err = r + .define( + long, + "x".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(1), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::InvalidName { .. })); + } + + #[test] + fn define_accepts_well_formed_kebab_dotted_names() { + let mut r = ConfigRegistry::new(); + for name in [ + "a", + "editing.auto-pair", + "autosave.interval-ms", + "a.b.c-d-e", + ] { + define_bool(&mut r, name, true, ConfigMutability::Live); + } + assert_eq!(r.len(), 4); + } + + // ---- acceptance 4: idempotent vs conflicting redefinition -------------- + + #[test] + fn identical_redefinition_is_idempotent_and_keeps_original_source() { + let mut r = ConfigRegistry::new(); + r.define( + "editing.x".into(), + "desc".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(10), + ) + .unwrap(); + // Pin an override so we can prove it survives too. + r.set("editing.x", ConfigValue::Bool(false)).unwrap(); + let epoch_before = r.definition_epoch(); + + r.define( + "editing.x".into(), + "desc".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(99), // different call site: same reload, different line + ) + .unwrap(); + + assert_eq!(r.len(), 1); + assert_eq!( + r.definition_epoch(), + epoch_before, + "idempotent reload adds nothing" + ); + assert_eq!(r.get_definition("editing.x").unwrap().source.line, 10); + assert_eq!( + r.get("editing.x", None).unwrap(), + &ConfigValue::Bool(false), + "the override survives an idempotent reload" + ); + } + + #[test] + fn conflicting_redefinition_leaves_original_exact() { + let mut r = ConfigRegistry::new(); + r.define( + "editing.x".into(), + "original".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(10), + ) + .unwrap(); + + let err = r + .define( + "editing.x".into(), + "changed".into(), + ConfigKind::Boolean, + ConfigValue::Bool(false), // different default + ConfigMutability::Live, + src(20), + ) + .unwrap_err(); + assert!(matches!(err, ConfigError::DuplicateName { .. })); + + let def = r.get_definition("editing.x").unwrap(); + assert_eq!(def.description, "original"); + assert_eq!(def.default, ConfigValue::Bool(true)); + assert_eq!(def.source.line, 10); + } + + // ---- acceptance 8: stable ordering -------------------------------------- + + #[test] + fn names_are_stable_across_at_least_three_defines() { + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "c", true, ConfigMutability::Live); + define_bool(&mut r, "a", true, ConfigMutability::Live); + define_bool(&mut r, "b", true, ConfigMutability::Live); + assert_eq!(r.names(), &["c".to_owned(), "a".into(), "b".into()]); + } + + // ---- acceptance 9 (bonus): source captured verbatim --------------------- + + #[test] + fn source_location_is_captured_from_the_defining_module() { + let mut r = ConfigRegistry::new(); + r.define( + "editing.auto-pair".into(), + "x".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + SourceLocation { + file: "pair.lua".into(), + line: 12, + }, + ) + .unwrap(); + assert_eq!( + r.get_definition("editing.auto-pair") + .unwrap() + .source + .render(), + "pair.lua:12" + ); + } + + // ---- acceptance 10 / 14: resolution and F9 ------------------------------ + + #[test] + fn get_resolves_local_then_global_then_default() { + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + let buf = BufferId::next(); + + assert_eq!( + r.get("editing.x", Some(buf)).unwrap(), + &ConfigValue::Bool(true) + ); + + r.set("editing.x", ConfigValue::Bool(false)).unwrap(); + assert_eq!( + r.get("editing.x", Some(buf)).unwrap(), + &ConfigValue::Bool(false) + ); + + r.set_local(buf, "editing.x", ConfigValue::Bool(true)) + .unwrap(); + assert_eq!( + r.get("editing.x", Some(buf)).unwrap(), + &ConfigValue::Bool(true) + ); + + r.reset("editing.x", Some(buf)).unwrap(); + assert_eq!( + r.get("editing.x", Some(buf)).unwrap(), + &ConfigValue::Bool(false) + ); + + r.reset("editing.x", None).unwrap(); + assert_eq!( + r.get("editing.x", Some(buf)).unwrap(), + &ConfigValue::Bool(true) + ); + } + + #[test] + fn get_with_no_buffer_ignores_any_local_override() { + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + let buf = BufferId::next(); + r.set_local(buf, "editing.x", ConfigValue::Bool(false)) + .unwrap(); + + // F9: get(name) with no buffer never consults an "active" + // buffer. It must see the global chain only, even though a + // buffer somewhere holds a different local override. + assert_eq!(r.get("editing.x", None).unwrap(), &ConfigValue::Bool(true)); + } + + // ---- acceptance 11: THE F1 test ----------------------------------------- + + #[test] + fn equal_valued_local_override_is_still_stored_and_shields_buffer() { + // This is the bite-verified regression test for framing F1. + // + // Scenario: a setting defaults to `true`. A buffer is pinned + // to `true` via set_local -- at the moment of pinning, the + // local override is *equal* to the effective global value, so + // an implementation that treats "equal-valued set" as a true + // no-op would store nothing for that buffer. Then the global + // value flips to `false`. If the local override was never + // stored, the "pinned" buffer silently flips too -- the pin + // never existed. The fix (Q#CR2/F1): overrides are *always* + // stored, even when they equal the value they shadow; only + // the value epoch and listener dispatch key on effective + // change. This test fails against the naive "true no-op" + // implementation and passes against the always-store one. + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.auto-pair", true, ConfigMutability::Live); + let buf = BufferId::next(); + + // Global effective value is `true` (the default; no override + // yet). Pin the buffer to that same value. + let pin = r + .set_local(buf, "editing.auto-pair", ConfigValue::Bool(true)) + .unwrap(); + assert!( + !pin.changed, + "pinning to the current value is observationally silent" + ); + assert!( + r.is_set("editing.auto-pair", Some(buf)).unwrap(), + "but the override IS stored -- is_set must see it" + ); + + // Now flip the global value. + let flip = r + .set("editing.auto-pair", ConfigValue::Bool(false)) + .unwrap(); + assert!(flip.changed); + + // The pinned buffer must NOT have flipped. + assert_eq!( + r.get("editing.auto-pair", Some(buf)).unwrap(), + &ConfigValue::Bool(true), + "F1: the buffer-local pin must survive a later global change" + ); + // The global (no-buffer) chain must reflect the flip. + assert_eq!( + r.get("editing.auto-pair", None).unwrap(), + &ConfigValue::Bool(false) + ); + } + + // ---- acceptance 12: per-buffer isolation + is_set ----------------------- + + #[test] + fn set_local_on_one_buffer_does_not_affect_another() { + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + let a = BufferId::next(); + let b = BufferId::next(); + + r.set_local(a, "editing.x", ConfigValue::Bool(false)) + .unwrap(); + + assert_eq!( + r.get("editing.x", Some(a)).unwrap(), + &ConfigValue::Bool(false) + ); + assert_eq!( + r.get("editing.x", Some(b)).unwrap(), + &ConfigValue::Bool(true) + ); + assert!(r.is_set("editing.x", Some(a)).unwrap()); + assert!(!r.is_set("editing.x", Some(b)).unwrap()); + } + + #[test] + fn is_set_reports_presence_even_for_an_equal_valued_override() { + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + assert!(!r.is_set("editing.x", None).unwrap()); + r.set("editing.x", ConfigValue::Bool(true)).unwrap(); // equal to default + assert!(r.is_set("editing.x", None).unwrap()); + } + + // ---- acceptance 13: buffer-local lifecycle ------------------------------- + + #[test] + fn remove_buffer_drops_its_local_overrides() { + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + let buf = BufferId::next(); + r.set_local(buf, "editing.x", ConfigValue::Bool(false)) + .unwrap(); + assert!(r.is_set("editing.x", Some(buf)).unwrap()); + + r.remove_buffer(buf); + + assert!(!r.is_set("editing.x", Some(buf)).unwrap()); + assert_eq!( + r.get("editing.x", Some(buf)).unwrap(), + &ConfigValue::Bool(true) + ); + } + + #[test] + fn locals_persist_until_remove_buffer_is_explicitly_called() { + // Documents the Q#CR5/F8 contract this module owns: without an + // explicit remove_buffer call (which in production rides + // after_buffer_removed), a buffer's locals are never purged. + // A BufferRegistry::remove bypass that skips that choke point + // is out of this file's scope (it lives in editor.rs), but the + // half of the contract this registry is responsible for -- + // "no automatic purge exists" -- is directly testable here. + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + let buf = BufferId::next(); + r.set_local(buf, "editing.x", ConfigValue::Bool(false)) + .unwrap(); + // ... time passes, nothing calls remove_buffer ... + assert!( + r.is_set("editing.x", Some(buf)).unwrap(), + "leak is permanent until purged" + ); + } + + // ---- acceptance 15: epoch discipline ------------------------------------- + + #[test] + fn value_epoch_advances_only_on_effective_change() { + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + let epoch0 = r.value_epoch(); + + let noop = r.set("editing.x", ConfigValue::Bool(true)).unwrap(); // equal to default + assert!(!noop.changed); + assert_eq!( + r.value_epoch(), + epoch0, + "equal-valued override advances no epoch" + ); + + let real = r.set("editing.x", ConfigValue::Bool(false)).unwrap(); + assert!(real.changed); + assert_eq!(r.value_epoch(), epoch0 + 1); + + let buf = BufferId::next(); + let local_noop = r + .set_local(buf, "editing.x", ConfigValue::Bool(false)) + .unwrap(); // equal to current global + assert!(!local_noop.changed); + assert_eq!(r.value_epoch(), epoch0 + 1, "still no advance"); + + let local_real = r + .set_local(buf, "editing.x", ConfigValue::Bool(true)) + .unwrap(); + assert!(local_real.changed); + assert_eq!(r.value_epoch(), epoch0 + 2); + } + + #[test] + fn definition_epoch_advances_only_on_new_definitions() { + let mut r = ConfigRegistry::new(); + assert_eq!(r.definition_epoch(), 0); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + assert_eq!(r.definition_epoch(), 1); + // Idempotent redefinition: no advance. + r.define( + "editing.x".into(), + "a boolean setting".into(), + ConfigKind::Boolean, + ConfigValue::Bool(true), + ConfigMutability::Live, + src(2), + ) + .unwrap(); + assert_eq!(r.definition_epoch(), 1); + } + + // ---- the validate() seam -------------------------------------------------- + + #[test] + fn validate_checks_without_committing() { + let mut r = ConfigRegistry::new(); + r.define( + "autosave.interval-ms".into(), + "x".into(), + ConfigKind::Integer { + min: Some(1000), + max: None, + }, + ConfigValue::Int(30_000), + ConfigMutability::Live, + src(1), + ) + .unwrap(); + + let err = r + .validate("autosave.interval-ms", &ConfigValue::Int(500)) + .unwrap_err(); + assert!(matches!(err, ConfigError::OutOfRange { .. })); + // Nothing was mutated by the failed validation. + assert!(!r.is_set("autosave.interval-ms", None).unwrap()); + assert_eq!(r.value_epoch(), 0); + + assert!( + r.validate("autosave.interval-ms", &ConfigValue::Int(5000)) + .is_ok() + ); + } + + #[test] + fn validate_on_undefined_name_is_not_found() { + let r = ConfigRegistry::new(); + let err = r.validate("nope", &ConfigValue::Bool(true)).unwrap_err(); + assert!(matches!(err, ConfigError::NotFound { name } if name == "nope")); + } + + // ---- type mismatch and per-kind validation --------------------------------- + + #[test] + fn set_rejects_wrong_type() { + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + let err = r.set("editing.x", ConfigValue::Int(1)).unwrap_err(); + assert!(matches!( + err, + ConfigError::TypeMismatch { + expected: "boolean", + got: "integer", + .. + } + )); + } + + #[test] + fn string_kind_rejects_empty_unless_allowed() { + let mut r = ConfigRegistry::new(); + r.define( + "editing.x".into(), + "x".into(), + ConfigKind::String { allow_empty: false }, + ConfigValue::Str("nonempty".into()), + ConfigMutability::Live, + src(1), + ) + .unwrap(); + let err = r + .set("editing.x", ConfigValue::Str(String::new())) + .unwrap_err(); + assert!(matches!(err, ConfigError::EmptyString { .. })); + } + + #[test] + fn enum_kind_rejects_non_choice() { + let mut r = ConfigRegistry::new(); + r.define( + "editing.x".into(), + "x".into(), + ConfigKind::Enum { + choices: vec!["lf".into(), "crlf".into()], + }, + ConfigValue::Str("lf".into()), + ConfigMutability::Live, + src(1), + ) + .unwrap(); + let err = r + .set("editing.x", ConfigValue::Str("cr".into())) + .unwrap_err(); + assert!(matches!(err, ConfigError::NotAChoice { got, .. } if got == "cr")); + } + + // ---- listeners: registration, snapshot, dispose ----------------------- + + #[test] + fn on_change_on_undefined_name_raises_not_found() { + let lua = mlua::Lua::new(); + let mut r = ConfigRegistry::new(); + let f = lua.create_function(|_, ()| Ok(())).unwrap(); + let err = r.on_change("nope", f, src(1)).unwrap_err(); + assert!(matches!(err, ConfigError::NotFound { name } if name == "nope")); + } + + #[test] + fn snapshot_returns_owned_listeners_in_registration_order_and_survives_registry_drop() { + let lua = mlua::Lua::new(); + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + for line in 10..13 { + let f = lua.create_function(|_, ()| Ok(())).unwrap(); + r.on_change("editing.x", f, src(line)).unwrap(); + } + let snap = r.snapshot("editing.x"); + assert_eq!(snap.len(), 3); + assert_eq!( + snap.iter().map(|l| l.source.line).collect::>(), + vec![10, 11, 12] + ); + + // Bite-verify borrow release: the registry itself can be + // dropped and the snapshot's Lua functions are still callable + // -- proving the caller genuinely does not need to hold the + // registry borrow while invoking them. + drop(r); + for l in &snap { + l.body.call::<()>(()).unwrap(); + } + } + + #[test] + fn dispose_is_idempotent_and_id_generation_safe() { + let lua = mlua::Lua::new(); + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + let f1 = lua.create_function(|_, ()| Ok(())).unwrap(); + let id1 = r.on_change("editing.x", f1, src(1)).unwrap(); + + r.dispose(id1); + assert!(r.snapshot("editing.x").is_empty()); + r.dispose(id1); // idempotent: no panic, no effect + assert!(r.snapshot("editing.x").is_empty()); + + let f2 = lua.create_function(|_, ()| Ok(())).unwrap(); + let id2 = r.on_change("editing.x", f2, src(2)).unwrap(); + assert_ne!(id1, id2); + + // A stale id must never dispose a newer listener. + r.dispose(id1); + assert_eq!(r.snapshot("editing.x").len(), 1); + assert_eq!(r.snapshot("editing.x")[0].id, id2); + } + + #[test] + fn global_set_does_not_change_a_shadowed_buffers_effective_value() { + // Q#CR6 (b): a global `set` fires only the global-scoped + // notification; a buffer holding its own override is + // shadowed, so its effective value must not move. + let mut r = ConfigRegistry::new(); + define_bool(&mut r, "editing.x", true, ConfigMutability::Live); + let buf = BufferId::next(); + r.set_local(buf, "editing.x", ConfigValue::Bool(true)) + .unwrap(); + + let change = r.set("editing.x", ConfigValue::Bool(false)).unwrap(); + assert!(change.changed, "the global effective value did change"); + assert_eq!( + r.get("editing.x", Some(buf)).unwrap(), + &ConfigValue::Bool(true), + "the shadowed buffer's effective value must not move" + ); + } + + // ---- mutability and the startup freeze --------------------------------- + + #[test] + fn startup_only_key_accepts_writes_before_freeze_and_rejects_after() { + let mut r = ConfigRegistry::new(); + define_bool( + &mut r, + "lsp.root-markers", + true, + ConfigMutability::StartupOnly, + ); + + // NOTE: in --lib test builds, set_init_complete never runs and + // the freeze flag never flips on its own -- this test flips it + // explicitly, the way mod.rs's own acceptance tests do, so it + // does not pass vacuously. + assert!(r.set("lsp.root-markers", ConfigValue::Bool(false)).is_ok()); + + r.freeze(); + assert!(r.is_frozen()); + + let err = r + .set("lsp.root-markers", ConfigValue::Bool(true)) + .unwrap_err(); + assert!( + matches!(err, ConfigError::StartupOnlyAfterFreeze { name } if name == "lsp.root-markers") + ); + + let err2 = r.reset("lsp.root-markers", None).unwrap_err(); + assert!(matches!(err2, ConfigError::StartupOnlyAfterFreeze { .. })); + } + + #[test] + fn startup_only_key_always_rejects_set_local() { + let mut r = ConfigRegistry::new(); + define_bool( + &mut r, + "lsp.root-markers", + true, + ConfigMutability::StartupOnly, + ); + let buf = BufferId::next(); + + // Rejected even before freeze: the combination is banned + // outright (F5), not just after startup completes. + assert!(!r.is_frozen()); + let err = r + .set_local(buf, "lsp.root-markers", ConfigValue::Bool(false)) + .unwrap_err(); + assert!( + matches!(err, ConfigError::StartupOnlyLocal { name } if name == "lsp.root-markers") + ); + } + + // ---- ConfigValue::int_from_f64 (acceptance 6's pure-Rust half) -------- + + #[test] + fn int_from_f64_accepts_exact_values() { + assert_eq!( + ConfigValue::int_from_f64("x", 3.0).unwrap(), + ConfigValue::Int(3) + ); + assert_eq!( + ConfigValue::int_from_f64("x", -1500.0).unwrap(), + ConfigValue::Int(-1500) + ); + assert_eq!( + ConfigValue::int_from_f64("x", 0.0).unwrap(), + ConfigValue::Int(0) + ); + } + + #[test] + fn int_from_f64_rejects_fractional_values() { + let err = ConfigValue::int_from_f64("x", 1500.7).unwrap_err(); + assert!(matches!(err, ConfigError::NonIntegral { .. })); + } + + #[test] + fn int_from_f64_rejects_non_finite_values() { + for v in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + let err = ConfigValue::int_from_f64("x", v).unwrap_err(); + assert!(matches!(err, ConfigError::NonFiniteNumber { .. })); + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 33e9338..4f283f5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -55,6 +55,7 @@ pub mod command; pub mod completion; pub mod completion_framework; pub mod config; +pub mod config_registry; // T M10.2: CRDT-backed buffer state. Feature-gated so v0.1 builds // carry zero overhead — the `loro` dependency isn't pulled in, no // field on the Buffer struct layout, no branch on `apply_edit`. diff --git a/src/lua_bindings/config.rs b/src/lua_bindings/config.rs new file mode 100644 index 0000000..43e0423 --- /dev/null +++ b/src/lua_bindings/config.rs @@ -0,0 +1,1460 @@ +// lua_bindings/config.rs --- pmacs.config: the configuration registry surface. + +//! `pmacs.config.*` --- the Lua surface over [`crate::config_registry`]. +//! Per `docs/config-registry-framing.md`: two scopes, no wire surface, +//! and no runtime chunk (Q#CR14) --- `pmacs.config` is installed here, +//! entirely from Rust, before any `builtin/runtime/*.lua` chunk +//! evaluates, exactly like `pmacs.command` and `pmacs.hook` have no +//! Lua-side companion file either. +//! +//! ```lua +//! pmacs.config.define { name = ..., description = ..., type = ..., +//! default = ..., min = ..., max = ..., +//! choices = ..., allow_empty = ..., +//! mutability = ... } +//! pmacs.config.get(name [, buf]) +//! pmacs.config.set(name, value) +//! pmacs.config.set_local(buf, name, value) +//! pmacs.config.reset(name [, buf]) +//! pmacs.config.is_set(name [, buf]) +//! pmacs.config.describe(name [, buf]) +//! pmacs.config.list() +//! local handle = pmacs.config.on_change(name, function(new, old, buf) ... end) +//! handle:dispose() +//! ``` +//! +//! # Two scopes, no ambient buffer (Q#CR4, F9) +//! +//! `set` writes the global override; `set_local` writes one buffer's +//! override. `get(name, buf)` resolves buffer-local -> global -> +//! default. **`get(name)` with no buffer argument resolves the global +//! chain only** --- global override, then default --- and never +//! consults an "active" buffer. There is no ambient buffer at this +//! layer; a caller that wants buffer-aware behavior must pass one. +//! `describe` and `list` follow the identical rule for their `value` +//! field. +//! +//! # Define before set; owner-defines, not a runtime chunk (Q#CR10, Q#CR14) +//! +//! Every name must be `define`d before `get`/`set`/`set_local`/ +//! `reset`/`is_set`/`describe`/`on_change` touches it --- an undefined +//! name raises `NotFound`, the same posture `pmacs.hook.add` already +//! has. Definitions live with the module that owns the setting, not in +//! a shared helper: `pair.lua` defines `editing.auto-pair`, +//! `editops.lua` defines `editing.trim-on-save`, `autosave.lua` +//! defines `autosave.interval-ms`. [`SourceLocation`] is captured from +//! Lua debug info at the `define` call site (see [`caller_source`]), +//! so a shared wrapper would point every builtin setting's reported +//! source at the wrapper instead of its owner --- which is exactly why +//! no such wrapper exists here. +//! +//! # Strict specs, lenient wrappers (Q#CR3, Q#CR8) +//! +//! `type` is one of `boolean | integer | number | string | enum` --- +//! no list/table type in stage 1. `define`'s spec table is read with +//! **raw** access only (typo-detection via [`Table::pairs`], field +//! reads via [`Table::raw_get`]), so neither an unknown key nor a +//! metatable-provided value can smuggle a field in (R50). Unknown +//! keys, a missing-or-whitespace-only `description` (R42), and a +//! `default` that violates its own `min`/`max`/`choices` are all +//! rejected before anything is registered. `set`/`set_local` enforce +//! the identical type and bounds on every write --- this module +//! converts a raw [`Value`] into a [`ConfigValue`] and lets +//! [`ConfigRegistry`] own the value-level check (see that module's +//! "value-validation seam"). +//! +//! That strictness is deliberate and is not this module's to relax for +//! any particular adopter: `pmacs.editops.trim_on_save("yes")` and +//! `pmacs.autosave.interval_ms(1500.7)` keep their legacy coercion +//! (flooring, `~= false`) in their own builtin files, calling `set` +//! only with an already-conforming value. This registry never coerces. +//! +//! # Integer exactness, by value, never `math.type` (acceptance 6) +//! +//! `LuaJIT` (Lua 5.1 semantics) never produces `Value::Integer` --- every +//! `LuaJIT` number arrives as `Value::Number(f64)`. Lua 5.4 produces +//! `Value::Integer(i64)` for integer literals, which are already exact +//! and read straight through with no float round-trip (round-tripping +//! a large `i64` through `f64` can silently change its value). +//! `Value::Number` on either backend goes through +//! [`ConfigValue::int_from_f64`], which checks exactness by the +//! numeric value alone. Neither backend's arm inspects which backend +//! produced the value or what `math.type` would say. +//! +//! # Listener dispatch (Q#CR6) +//! +//! `set`/`set_local`/`reset` commit inside the registry borrow, then +//! (iff the effective value changed) drop that borrow completely +//! before invoking any listener body --- see [`dispatch_config_listeners`]. +//! A raising listener is logged and does not stop later listeners or +//! roll back the committed value. [`ConfigDispatchDepth`] bounds +//! re-entrant dispatch so an accidental listener cycle raises a +//! pointed error instead of recursing forever. Listeners persist until +//! explicitly disposed (F3): there is no `MetaMethod::Gc` anywhere in +//! this module, matching the rest of the codebase's explicit-dispose +//! posture. +//! +//! # The startup freeze, without touching `editor.rs` +//! +//! [`ConfigMutability::StartupOnly`] enforcement is entirely +//! [`ConfigRegistry`]'s job (`StartupOnlyAfterFreeze`); this module's +//! only responsibility is calling [`ConfigRegistry::freeze`] at the +//! right moment. `set` and `reset` do so lazily: if +//! [`super::InitCompleteFlag`] reports the init phase complete and the +//! registry isn't frozen yet, they freeze it before proceeding. That +//! keeps the single source of truth for "init is done" in the flag +//! `EditorState::new` already flips, with no new call added to +//! `editor.rs`. + +use std::cell::{Cell, RefCell}; +use std::rc::Rc; + +use mlua::{Function, Lua, Table, UserData, UserDataMethods, Value}; +use thiserror::Error; + +use super::{BufferIdLua, InitCompleteFlag, SharedRegistry, caller_source, require_string_key}; +use crate::buffer::EditOp; +use crate::command::SourceLocation; +use crate::config_registry::{ + ConfigChange, ConfigError, ConfigKind, ConfigMutability, ConfigRegistry, ConfigValue, +}; + +/// Shared, single-threaded handle to the configuration registry. +/// Lives behind `Rc>` as Lua app data, exactly like the +/// command and hook registries (`mod.rs:2265-2271`). +pub type SharedConfigRegistry = Rc>; + +/// Recursion bound for re-entrant listener dispatch (Q#CR6). A +/// listener is free to call `pmacs.config.set`/`set_local`/`reset` +/// re-entrantly --- the registry borrow is always released before any +/// listener body runs (see [`dispatch_config_listeners`]) --- but an +/// accidental cycle (A's listener sets B, B's listener sets A, ...) +/// must not hang the editor. Chosen generously above any legitimate +/// re-entrant chain a real adopter would produce. +const MAX_DISPATCH_DEPTH: u32 = 32; + +/// Per-VM re-entrant listener-dispatch depth counter. Newtype app data +/// (mirrors [`InitCompleteFlag`]'s `Rc>` shape) so +/// `set`/`set_local`/`reset` dispatch calls made from arbitrarily +/// nested re-entrant `pmacs.config.*` calls all see the same counter. +#[derive(Clone)] +struct ConfigDispatchDepth(Rc>); + +impl ConfigDispatchDepth { + fn new() -> Self { + Self(Rc::new(Cell::new(0))) + } +} + +/// RAII guard: increments on construction, decrements on drop --- +/// including on an early `?` return from a failed Lua value +/// conversion between the increment and the dispatch loop --- so the +/// counter can never get stuck above zero. +struct DispatchDepthGuard(Rc>); + +impl Drop for DispatchDepthGuard { + fn drop(&mut self) { + self.0.set(self.0.get().saturating_sub(1)); + } +} + +/// Errors raised by this Lua-boundary module itself, distinct from +/// [`ConfigError`]: these describe a failure to make sense of a raw +/// Lua value (an unrecognized `type =` tag, a missing one, a listener +/// cycle) rather than a failure of already-typed config data. See the +/// module doc's "value-validation seam" cross-reference. +#[derive(Debug, Error)] +enum ConfigBindingError { + /// `define` was called without a `type` field. + #[error( + "config spec requires a \"type\" field (one of boolean, integer, number, string, enum)" + )] + MissingType, + + /// `define`'s `type` field wasn't one of the closed vocabulary. + #[error("unknown config type `{got}`; expected one of: boolean, integer, number, string, enum")] + UnknownType { + /// The offending type tag. + got: String, + }, + + /// `define`'s `type = "enum"` had no (or an empty) `choices` table. + #[error("config \"{name}\" (type = \"enum\") requires a `choices` table of strings")] + MissingChoices { + /// The config name being defined. + name: String, + }, + + /// `define`'s `mutability` field wasn't `"live"` or `"startup"`. + #[error("unknown config mutability `{got}`; expected one of: live, startup")] + UnknownMutability { + /// The offending mutability tag. + got: String, + }, + + /// Re-entrant listener dispatch exceeded [`MAX_DISPATCH_DEPTH`] --- + /// almost certainly an accidental listener cycle. + #[error( + "config listener dispatch exceeded the recursion bound ({max}); a listener cycle is likely" + )] + ListenerCycle { + /// The bound that was exceeded. + max: u32, + }, +} + +// --------------------------------------------------------------------------- +// Lua Value <-> ConfigValue conversion +// --------------------------------------------------------------------------- + +fn type_mismatch(name: &str, expected: &'static str, got: &Value) -> mlua::Error { + mlua::Error::external(ConfigError::TypeMismatch { + name: name.to_owned(), + expected, + got: got.type_name(), + }) +} + +/// Convert a raw Lua number into an exact `i64`. See the module doc's +/// "integer exactness" section: `Value::Integer` (lua54 only) is +/// already exact and is never round-tripped through `f64`; +/// `Value::Number` (both backends) goes through +/// [`ConfigValue::int_from_f64`]. +fn lua_exact_i64(name: &str, value: Value) -> mlua::Result { + match value { + Value::Integer(i) => Ok(i), + Value::Number(f) => match ConfigValue::int_from_f64(name, f) { + Ok(ConfigValue::Int(i)) => Ok(i), + Ok(_) => unreachable!("int_from_f64 always returns ConfigValue::Int"), + Err(e) => Err(mlua::Error::external(e)), + }, + other => Err(type_mismatch(name, "integer", &other)), + } +} + +fn lua_to_f64(name: &str, value: Value) -> mlua::Result { + match value { + Value::Integer(i) => Ok(i as f64), + Value::Number(f) => Ok(f), + other => Err(type_mismatch(name, "number", &other)), + } +} + +/// Convert a raw Lua value into a [`ConfigValue`] under an +/// already-known [`ConfigKind`]. Used for both `define`'s `default` +/// field and `set`/`set_local`'s value argument --- the one converter +/// both paths share, so there is exactly one place Lua-level type +/// coercion can drift (mirrors [`ConfigRegistry::validate`] being the +/// single value-level seam on the Rust side). +fn lua_to_config_value(name: &str, kind: &ConfigKind, value: Value) -> mlua::Result { + match kind { + ConfigKind::Boolean => match value { + Value::Boolean(b) => Ok(ConfigValue::Bool(b)), + other => Err(type_mismatch(name, "boolean", &other)), + }, + ConfigKind::Integer { .. } => Ok(ConfigValue::Int(lua_exact_i64(name, value)?)), + ConfigKind::Number { .. } => Ok(ConfigValue::Num(lua_to_f64(name, value)?)), + ConfigKind::String { .. } => match value { + Value::String(s) => Ok(ConfigValue::Str(s.to_str()?.to_owned())), + other => Err(type_mismatch(name, "string", &other)), + }, + ConfigKind::Enum { .. } => match value { + Value::String(s) => Ok(ConfigValue::Str(s.to_str()?.to_owned())), + other => Err(type_mismatch(name, "enum", &other)), + }, + } +} + +fn config_value_to_lua(lua: &Lua, v: &ConfigValue) -> mlua::Result { + Ok(match v { + ConfigValue::Bool(b) => Value::Boolean(*b), + ConfigValue::Int(i) => Value::Integer(*i), + ConfigValue::Num(f) => Value::Number(*f), + ConfigValue::Str(s) => Value::String(lua.create_string(s)?), + }) +} + +// --------------------------------------------------------------------------- +// define(): strict raw-table spec parsing +// --------------------------------------------------------------------------- + +/// The closed set of keys `pmacs.config.define {...}` accepts. Checked +/// with raw table access (R50) --- see [`check_unknown_fields`]. +const DEFINE_SPEC_FIELDS: &[&str] = &[ + "name", + "description", + "type", + "default", + "min", + "max", + "choices", + "allow_empty", + "mutability", +]; + +/// R50 typo-detection: every key actually present in the raw table +/// (via [`Table::pairs`], which --- like [`Table::raw_get`] below --- +/// never invokes `__pairs`/`__index`) must be in `allowed`, or the +/// spec is rejected naming the offender and the supported-key list. +/// This alone doesn't stop a metatable `__index` from answering a +/// `raw_get` for a key the table itself never had; that's why every +/// field read below uses `raw_get`, not `get`, too. +fn check_unknown_fields(spec: &Table, allowed: &[&str]) -> mlua::Result<()> { + for pair in spec.clone().pairs::() { + let (k, _) = pair?; + let key = require_string_key(k)?; + if !allowed.contains(&key.as_str()) { + return Err(mlua::Error::external(ConfigError::UnknownField { + field: key, + supported: allowed.join(", "), + })); + } + } + Ok(()) +} + +fn read_bound_i64(spec: &Table, field: &'static str, name: &str) -> mlua::Result> { + match spec.raw_get::(field)? { + Value::Nil => Ok(None), + other => Ok(Some(lua_exact_i64(name, other)?)), + } +} + +fn read_bound_f64(spec: &Table, field: &'static str, name: &str) -> mlua::Result> { + match spec.raw_get::(field)? { + Value::Nil => Ok(None), + other => Ok(Some(lua_to_f64(name, other)?)), + } +} + +/// Read `choices` as a raw sequence of Lua strings --- no numeric +/// coercion (unlike `Table::sequence_values::()`, which would +/// silently stringify a numeric entry): a non-string choice is a +/// definition bug, not a value to paper over. +fn read_choices(spec: &Table, name: &str) -> mlua::Result> { + let Some(t) = spec.raw_get::>("choices")? else { + return Err(mlua::Error::external(ConfigBindingError::MissingChoices { + name: name.to_owned(), + })); + }; + let mut choices = Vec::new(); + for v in t.sequence_values::() { + match v? { + Value::String(s) => choices.push(s.to_str()?.to_owned()), + other => return Err(type_mismatch(name, "enum", &other)), + } + } + Ok(choices) +} + +fn parse_kind_and_default(spec: &Table, name: &str) -> mlua::Result<(ConfigKind, ConfigValue)> { + let type_str: Option = spec.raw_get("type")?; + let type_str = + type_str.ok_or_else(|| mlua::Error::external(ConfigBindingError::MissingType))?; + + let kind = match type_str.as_str() { + "boolean" => ConfigKind::Boolean, + "integer" => ConfigKind::Integer { + min: read_bound_i64(spec, "min", name)?, + max: read_bound_i64(spec, "max", name)?, + }, + "number" => ConfigKind::Number { + min: read_bound_f64(spec, "min", name)?, + max: read_bound_f64(spec, "max", name)?, + }, + "string" => ConfigKind::String { + allow_empty: spec + .raw_get::>("allow_empty")? + .unwrap_or(false), + }, + "enum" => ConfigKind::Enum { + choices: read_choices(spec, name)?, + }, + other => { + return Err(mlua::Error::external(ConfigBindingError::UnknownType { + got: other.to_owned(), + })); + } + }; + + let raw_default: Value = spec.raw_get("default")?; + let default = lua_to_config_value(name, &kind, raw_default)?; + Ok((kind, default)) +} + +fn parse_mutability(spec: &Table) -> mlua::Result { + match spec.raw_get::>("mutability")?.as_deref() { + None | Some("live") => Ok(ConfigMutability::Live), + Some("startup") => Ok(ConfigMutability::StartupOnly), + Some(other) => Err(mlua::Error::external( + ConfigBindingError::UnknownMutability { + got: other.to_owned(), + }, + )), + } +} + +// --------------------------------------------------------------------------- +// Listener dispatch (Q#CR6) +// --------------------------------------------------------------------------- + +/// Snapshot `name`'s listeners, drop the registry borrow, then invoke +/// each in registration order with copied `(new, old, buf)` values. +/// +/// The borrow-release is the whole point (Q#CR6): by the time any +/// listener body runs, `reg.borrow()` from *within* that body (e.g. a +/// re-entrant `pmacs.config.set`) sees no outstanding borrow from us. +/// A raising listener is logged to the `*errors*` buffer and does not +/// stop later listeners or affect the already-committed value. +/// [`MAX_DISPATCH_DEPTH`] bounds re-entrant dispatch depth. +fn dispatch_config_listeners( + lua: &Lua, + reg: &SharedConfigRegistry, + name: &str, + change: &ConfigChange, + buf: Option, +) -> mlua::Result<()> { + let listeners = reg.borrow().snapshot(name); + if listeners.is_empty() { + return Ok(()); + } + + let depth_cell = lua + .app_data_ref::() + .expect("ConfigDispatchDepth installed by install_config") + .0 + .clone(); + let depth = depth_cell.get(); + if depth >= MAX_DISPATCH_DEPTH { + return Err(mlua::Error::external(ConfigBindingError::ListenerCycle { + max: MAX_DISPATCH_DEPTH, + })); + } + depth_cell.set(depth + 1); + let _guard = DispatchDepthGuard(depth_cell); + + let new_lua = config_value_to_lua(lua, &change.new)?; + let old_lua = config_value_to_lua(lua, &change.old)?; + for listener in listeners { + if let Err(err) = listener + .body + .call::<()>((new_lua.clone(), old_lua.clone(), buf)) + { + log_config_listener_error(lua, &listener.source, &err); + } + } + Ok(()) +} + +/// Append a one-line entry to the `*errors*` buffer naming the +/// listener's source. Mirrors `log_hook_error` / `log_buffer_removed_error` +/// in `mod.rs`; a no-op (rather than a panic) if the buffer registry +/// app data isn't installed, matching those precedents. +fn log_config_listener_error(lua: &Lua, source: &SourceLocation, err: &mlua::Error) { + let line = format!( + "[config] on_change listener at {} raised: {err}\n", + source.render() + ); + let result = { + let Some(app) = lua.app_data_ref::() else { + return; + }; + let mut reg = app.borrow_mut(); + let id = match reg.find_by_name(crate::lua::ERRORS_BUFFER_NAME) { + Some(id) => id, + None => reg.create(crate::lua::ERRORS_BUFFER_NAME), + }; + let Ok(buf) = reg.get_mut(id) else { + return; + }; + let pos = buf.len(); + let edit = buf + .apply_edit(EditOp::Insert { + pos, + bytes: line.as_bytes(), + }) + .ok(); + edit.map(|e| (id, e)) + }; + if let Some((id, edit)) = result { + super::notify_buffer_edit_to_windows(lua, id, &edit); + } +} + +/// If the init phase has completed and the registry isn't frozen yet, +/// freeze it now. Called from `set`/`reset` only (see the module doc's +/// "startup freeze" section) --- `set_local` never consults the frozen +/// flag at all, so freezing ahead of it would change nothing. +fn maybe_freeze_after_init(lua: &Lua, reg: &SharedConfigRegistry) { + let complete = lua + .app_data_ref::() + .is_some_and(|f| f.is_complete()); + if complete { + let mut r = reg.borrow_mut(); + if !r.is_frozen() { + r.freeze(); + } + } +} + +// --------------------------------------------------------------------------- +// describe() / list() +// --------------------------------------------------------------------------- + +/// Build a fresh descriptor table for `name`, shared by `describe` and +/// `list`. Never returns a handle onto registry state (Q#CR3): every +/// field is copied out. +fn describe_one( + lua: &Lua, + reg: &ConfigRegistry, + name: &str, + buf: Option, +) -> mlua::Result { + let def = reg.get_definition(name).ok_or_else(|| { + mlua::Error::external(ConfigError::NotFound { + name: name.to_owned(), + }) + })?; + + let t = lua.create_table()?; + t.set("name", def.name.clone())?; + t.set("description", def.description.clone())?; + t.set("type", def.kind.type_name())?; + t.set("default", config_value_to_lua(lua, &def.default)?)?; + match &def.kind { + ConfigKind::Boolean => {} + ConfigKind::Integer { min, max } => { + if let Some(v) = min { + t.set("min", *v)?; + } + if let Some(v) = max { + t.set("max", *v)?; + } + } + ConfigKind::Number { min, max } => { + if let Some(v) = min { + t.set("min", *v)?; + } + if let Some(v) = max { + t.set("max", *v)?; + } + } + ConfigKind::String { allow_empty } => { + t.set("allow_empty", *allow_empty)?; + } + ConfigKind::Enum { choices } => { + let choices_t = lua.create_table_with_capacity(choices.len(), 0)?; + for (i, c) in choices.iter().enumerate() { + choices_t.set(i + 1, c.as_str())?; + } + t.set("choices", choices_t)?; + } + } + t.set( + "mutability", + match def.mutability { + ConfigMutability::Live => "live", + ConfigMutability::StartupOnly => "startup", + }, + )?; + + // `value` follows the same buf-argument contract as `get` (F9): + // with `buf`, buffer-local -> global -> default; with `None`, the + // global chain only. `global` is always the global-chain + // resolution, independent of `buf`, so a caller can always see + // "what would this be with no override at all on my buffer." + let value = reg + .get(name, buf.map(BufferIdLua::id)) + .map_err(mlua::Error::external)?; + t.set("value", config_value_to_lua(lua, value)?)?; + let global = reg.get(name, None).map_err(mlua::Error::external)?; + t.set("global", config_value_to_lua(lua, global)?)?; + + // F7: `buffer_local`, never `local` (a Lua keyword). Present only + // when a buffer was given AND that buffer holds an override. + if let Some(b) = buf + && let Some(v) = reg.local_override(name, b.id()) + { + t.set("buffer_local", config_value_to_lua(lua, v)?)?; + } + + t.set("source", def.source.render())?; + Ok(t) +} + +// --------------------------------------------------------------------------- +// on_change() handle +// --------------------------------------------------------------------------- + +/// Userdata handle returned by `pmacs.config.on_change`. Models the +/// `dispose` binding at `mod.rs:1867` (the compile-mode style-overlay +/// handle): explicit, idempotent teardown, no `MetaMethod::Gc` --- +/// `ConfigRegistry::dispose` is itself idempotent and generation-safe +/// by listener id, so this wrapper adds no extra state of its own. A +/// dropped-but-never-disposed handle keeps firing (F3): there is +/// nothing here that would stop it. +struct ConfigListenerHandleLua { + id: u64, + registry: SharedConfigRegistry, +} + +impl UserData for ConfigListenerHandleLua { + fn add_methods>(methods: &mut M) { + methods.add_method("dispose", |_, this, ()| { + this.registry.borrow_mut().dispose(this.id); + Ok(()) + }); + } +} + +// --------------------------------------------------------------------------- +// install +// --------------------------------------------------------------------------- + +/// Install `pmacs.config.*` over `registry`. +/// +/// Called from [`super::install`] while the `pmacs` table is being +/// built, so `pmacs.config` exists before any `builtin/runtime/*.lua` +/// chunk evaluates (Q#CR14) --- the same ordering guarantee +/// `pmacs.command` and `pmacs.hook` already have. +#[allow( + clippy::too_many_lines, + reason = "linear list of raw bindings; splitting fragments a coherent surface" +)] +pub fn install_config(lua: &Lua, registry: &SharedConfigRegistry) -> mlua::Result
{ + lua.set_app_data(ConfigDispatchDepth::new()); + let config_mod = lua.create_table()?; + + { + let reg = registry.clone(); + config_mod.set( + "define", + lua.create_function(move |lua, spec: Table| -> mlua::Result<()> { + check_unknown_fields(&spec, DEFINE_SPEC_FIELDS)?; + let name: String = spec.raw_get::>("name")?.unwrap_or_default(); + let description: String = spec + .raw_get::>("description")? + .unwrap_or_default(); + let (kind, default) = parse_kind_and_default(&spec, &name)?; + let mutability = parse_mutability(&spec)?; + reg.borrow_mut() + .define( + name, + description, + kind, + default, + mutability, + caller_source(lua, 2), + ) + .map_err(mlua::Error::external)?; + Ok(()) + })?, + )?; + } + + { + let reg = registry.clone(); + config_mod.set( + "get", + lua.create_function(move |lua, (name, buf): (String, Option)| { + let r = reg.borrow(); + let v = r + .get(&name, buf.map(BufferIdLua::id)) + .map_err(mlua::Error::external)?; + config_value_to_lua(lua, v) + })?, + )?; + } + + { + let reg = registry.clone(); + config_mod.set( + "is_set", + lua.create_function(move |_, (name, buf): (String, Option)| { + reg.borrow() + .is_set(&name, buf.map(BufferIdLua::id)) + .map_err(mlua::Error::external) + })?, + )?; + } + + { + let reg = registry.clone(); + config_mod.set( + "set", + lua.create_function( + move |lua, (name, value): (String, Value)| -> mlua::Result<()> { + maybe_freeze_after_init(lua, ®); + let kind = { + let r = reg.borrow(); + r.get_definition(&name) + .ok_or_else(|| { + mlua::Error::external(ConfigError::NotFound { name: name.clone() }) + })? + .kind + .clone() + }; + let cv = lua_to_config_value(&name, &kind, value)?; + let change = reg + .borrow_mut() + .set(&name, cv) + .map_err(mlua::Error::external)?; + if change.changed { + dispatch_config_listeners(lua, ®, &name, &change, None)?; + } + Ok(()) + }, + )?, + )?; + } + + { + let reg = registry.clone(); + config_mod.set( + "set_local", + lua.create_function( + move |lua, (buf, name, value): (BufferIdLua, String, Value)| -> mlua::Result<()> { + let kind = { + let r = reg.borrow(); + r.get_definition(&name) + .ok_or_else(|| { + mlua::Error::external(ConfigError::NotFound { name: name.clone() }) + })? + .kind + .clone() + }; + let cv = lua_to_config_value(&name, &kind, value)?; + let change = reg + .borrow_mut() + .set_local(buf.id(), &name, cv) + .map_err(mlua::Error::external)?; + if change.changed { + dispatch_config_listeners(lua, ®, &name, &change, Some(buf))?; + } + Ok(()) + }, + )?, + )?; + } + + { + let reg = registry.clone(); + config_mod.set( + "reset", + lua.create_function( + move |lua, (name, buf): (String, Option)| -> mlua::Result<()> { + maybe_freeze_after_init(lua, ®); + let change = reg + .borrow_mut() + .reset(&name, buf.map(BufferIdLua::id)) + .map_err(mlua::Error::external)?; + if change.changed { + dispatch_config_listeners(lua, ®, &name, &change, buf)?; + } + Ok(()) + }, + )?, + )?; + } + + { + let reg = registry.clone(); + config_mod.set( + "describe", + lua.create_function(move |lua, (name, buf): (String, Option)| { + describe_one(lua, ®.borrow(), &name, buf) + })?, + )?; + } + + { + let reg = registry.clone(); + config_mod.set( + "list", + lua.create_function(move |lua, ()| { + let r = reg.borrow(); + let out = lua.create_table_with_capacity(r.names().len(), 0)?; + for (i, name) in r.names().iter().enumerate() { + out.set(i + 1, describe_one(lua, &r, name, None)?)?; + } + Ok(out) + })?, + )?; + } + + { + let reg = registry.clone(); + config_mod.set( + "on_change", + lua.create_function(move |lua, (name, body): (String, Function)| { + let id = reg + .borrow_mut() + .on_change(&name, body, caller_source(lua, 2)) + .map_err(mlua::Error::external)?; + Ok(ConfigListenerHandleLua { + id, + registry: reg.clone(), + }) + })?, + )?; + } + + Ok(config_mod) +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +#[cfg(test)] +mod tests { + use super::*; + use crate::buffer::BufferId; + + fn fresh() -> (Lua, SharedConfigRegistry) { + let lua = Lua::new(); + let registry: SharedConfigRegistry = Rc::new(RefCell::new(ConfigRegistry::new())); + let config_mod = install_config(&lua, ®istry).expect("install_config"); + let pmacs = lua.create_table().expect("pmacs table"); + pmacs.set("config", config_mod).expect("pmacs.config"); + lua.globals().set("pmacs", pmacs).expect("globals"); + (lua, registry) + } + + /// Execute a chunk purely for side effects; discards any return. + fn run(lua: &Lua, src: &str) -> mlua::Result<()> { + lua.load(src).exec() + } + + /// Evaluate a chunk and convert its return value(s) to `T`. + fn eval(lua: &Lua, src: &str) -> mlua::Result { + lua.load(src).eval::() + } + + /// Like [`run`], but under an explicit chunk name --- for + /// exercising `caller_source`'s capture of a "real" file location. + fn run_named(lua: &Lua, name: &str, src: &str) -> mlua::Result<()> { + lua.load(src).set_name(name).exec() + } + + // ---- acceptance 1: round-trip every kind, via Lua ---------------------- + + #[test] + fn define_then_get_round_trips_every_kind_via_lua() { + let (lua, _reg) = fresh(); + run( + &lua, + r#" + pmacs.config.define{ name="editing.auto-pair", description="d", type="boolean", default=true } + pmacs.config.define{ name="autosave.interval-ms", description="d", type="integer", default=30000, min=1000 } + pmacs.config.define{ name="editing.fill-column", description="d", type="number", default=80.0, min=1.0, max=1000.0 } + pmacs.config.define{ name="editing.comment-prefix", description="d", type="string", default="", allow_empty=true } + pmacs.config.define{ name="editing.line-ending", description="d", type="enum", default="lf", choices={"lf","crlf"} } + "#, + ) + .unwrap(); + + assert!(eval::(&lua, "return pmacs.config.get('editing.auto-pair')").unwrap()); + assert_eq!( + eval::(&lua, "return pmacs.config.get('autosave.interval-ms')").unwrap(), + 30_000 + ); + assert!( + (eval::(&lua, "return pmacs.config.get('editing.fill-column')").unwrap() - 80.0) + .abs() + < f64::EPSILON + ); + assert_eq!( + eval::(&lua, "return pmacs.config.get('editing.comment-prefix')").unwrap(), + "" + ); + assert_eq!( + eval::(&lua, "return pmacs.config.get('editing.line-ending')").unwrap(), + "lf" + ); + } + + // ---- acceptance 2: R50 unknown field + metatable smuggling ------------- + + #[test] + fn define_rejects_unknown_field_naming_offender_and_supported_keys() { + let (lua, _reg) = fresh(); + let err = run( + &lua, + r#"pmacs.config.define{ name="x.y", description="d", type="boolean", default=true, typo_field=1 }"#, + ) + .unwrap_err(); + let msg = err.to_string(); + assert!(msg.contains("typo_field"), "{msg}"); + assert!(msg.contains("supported"), "{msg}"); + assert!(msg.contains("name"), "{msg}"); + } + + #[test] + fn define_rejects_missing_or_whitespace_description() { + let (lua, _reg) = fresh(); + let err = run( + &lua, + r#"pmacs.config.define{ name="x.y", description=" ", type="boolean", default=true }"#, + ) + .unwrap_err(); + assert!(err.to_string().contains("R42"), "{err}"); + } + + #[test] + fn define_ignores_metatable_provided_default_via_raw_access() { + // R50's harder half: a spec table whose OWN keys never include + // `default` at all -- a metatable `__index` answers for it + // instead. Raw access must not pick that up: the define must + // fail as though `default` were absent (nil), not silently + // succeed with the metatable-smuggled value. + let (lua, _reg) = fresh(); + let err = run( + &lua, + r#" + local spec = setmetatable({ name="x.y", description="d", type="boolean" }, { + __index = function(_, k) if k == "default" then return true end end, + }) + pmacs.config.define(spec) + "#, + ) + .unwrap_err(); + assert!( + err.to_string().contains("boolean"), + "expected a boolean-vs-nil type mismatch, got: {err}" + ); + // And the definition must not have been registered. + let names: i64 = eval(&lua, "return #pmacs.config.list()").unwrap(); + assert_eq!( + names, 0, + "the rejected define must not have registered anything" + ); + } + + // ---- acceptance 9: SourceLocation from the DEFINING module -------------- + + #[test] + fn source_location_is_captured_from_the_defining_chunk_not_a_helper() { + let (lua, _reg) = fresh(); + run_named( + &lua, + "@pmacs/builtin/runtime/pair.lua", + "\n\npmacs.config.define{ name='editing.auto-pair', description='d', type='boolean', default=true }\n", + ) + .expect("define from a named chunk"); + + let source: String = eval( + &lua, + "return pmacs.config.describe('editing.auto-pair').source", + ) + .unwrap(); + assert!( + source.starts_with("pmacs/builtin/runtime/pair.lua:"), + "expected the defining chunk's own path, got {source:?}" + ); + assert!( + source.ends_with(":3"), + "expected line 3 (the call sits after two blank lines), got {source:?}" + ); + } + + // ---- F9: get(name) with no buffer resolves the global chain only ------- + + #[test] + fn get_with_no_buffer_ignores_any_local_override() { + let (lua, reg) = fresh(); + run( + &lua, + r#"pmacs.config.define{ name="editing.x", description="d", type="boolean", default=true }"#, + ) + .unwrap(); + let buf = BufferId::next(); + reg.borrow_mut() + .set_local(buf, "editing.x", ConfigValue::Bool(false)) + .unwrap(); + + lua.globals().set("__buf", BufferIdLua(buf)).unwrap(); + assert!( + eval::(&lua, "return pmacs.config.get('editing.x')").unwrap(), + "no-buffer get must see the global chain only" + ); + assert!(!eval::(&lua, "return pmacs.config.get('editing.x', __buf)").unwrap()); + } + + // ---- listeners: the borrow-release bite test ---------------------------- + + #[test] + fn listener_runs_after_borrow_release_and_can_reentrantly_write() { + // Bite-verified: if `dispatch_config_listeners` still held a + // `RefCell` borrow on the registry while calling the listener + // body, the listener's own `pmacs.config.set` below would hit + // a `BorrowMutError` panic. No panic, plus observing the + // second setting's new value, is the proof the borrow was + // released before Lua ran. + let (lua, _reg) = fresh(); + run( + &lua, + r#" + pmacs.config.define{ name="a", description="d", type="boolean", default=true } + pmacs.config.define{ name="b", description="d", type="boolean", default=true } + pmacs.config.on_change('a', function(new, old, buf) + pmacs.config.set('b', false) + end) + pmacs.config.set('a', false) + "#, + ) + .unwrap(); + + assert!( + !eval::(&lua, "return pmacs.config.get('b')").unwrap(), + "the re-entrant set from inside the listener must have committed" + ); + } + + // ---- Q#CR6 (a)/(b): fires once with buf=nil, not for a shadowed buffer - + + #[test] + fn global_set_fires_once_with_nil_buf_and_not_for_a_shadowed_buffer() { + let (lua, reg) = fresh(); + run( + &lua, + r#"pmacs.config.define{ name="editing.x", description="d", type="boolean", default=true }"#, + ) + .unwrap(); + let buf = BufferId::next(); + reg.borrow_mut() + .set_local(buf, "editing.x", ConfigValue::Bool(true)) + .unwrap(); + + run( + &lua, + r" + calls = {} + pmacs.config.on_change('editing.x', function(new, old, buf) + table.insert(calls, { new = new, buf = buf }) + end) + pmacs.config.set('editing.x', false) + ", + ) + .unwrap(); + + assert_eq!( + eval::(&lua, "return #calls").unwrap(), + 1, + "must fire exactly once, not once per buffer" + ); + assert!( + eval::(&lua, "return calls[1].buf == nil").unwrap(), + "global set must report buf = nil" + ); + + // The shadowed buffer's effective value must not have moved. + assert_eq!( + reg.borrow().get("editing.x", Some(buf)).unwrap(), + &ConfigValue::Bool(true) + ); + } + + // ---- Q#CR6 (c): remove_buffer purges without firing -------------------- + + #[test] + fn remove_buffer_purges_locals_without_firing_a_listener() { + let (lua, reg) = fresh(); + run( + &lua, + r#" + pmacs.config.define{ name="editing.x", description="d", type="boolean", default=true } + fire_count = 0 + pmacs.config.on_change('editing.x', function() fire_count = fire_count + 1 end) + "#, + ) + .unwrap(); + let buf = BufferId::next(); + + // set_local DOES fire (the buffer's effective value changes) -- + // establishes a nonzero baseline so the next assertion is + // meaningful (not just "still zero because nothing happened"). + reg.borrow_mut() + .set_local(buf, "editing.x", ConfigValue::Bool(false)) + .unwrap(); + // set_local was driven directly through the core (not the Lua + // binding above), so it did not dispatch our Lua listener -- + // that's fine; this test is about remove_buffer specifically. + let before: i64 = eval(&lua, "return fire_count").unwrap(); + + reg.borrow_mut().remove_buffer(buf); + let after: i64 = eval(&lua, "return fire_count").unwrap(); + assert_eq!(before, after, "remove_buffer must never fire a listener"); + } + + // ---- Q#CR6 (d): on_change on an undefined name raises NotFound --------- + + #[test] + fn on_change_on_undefined_name_raises_not_found() { + let (lua, _reg) = fresh(); + let err = run(&lua, "pmacs.config.on_change('nope', function() end)").unwrap_err(); + assert!(err.to_string().contains("not defined"), "{err}"); + } + + // ---- one raising listener is logged and does not block later ones ------ + + #[test] + fn one_raising_listener_does_not_block_later_listeners_or_the_value() { + let (lua, _reg) = fresh(); + run( + &lua, + r#" + pmacs.config.define{ name="editing.x", description="d", type="boolean", default=true } + second_ran = false + pmacs.config.on_change('editing.x', function() error("boom") end) + pmacs.config.on_change('editing.x', function() second_ran = true end) + pmacs.config.set('editing.x', false) + "#, + ) + .unwrap(); + + assert!( + eval::(&lua, "return second_ran").unwrap(), + "a raising listener must not block later ones" + ); + assert!( + !eval::(&lua, "return pmacs.config.get('editing.x')").unwrap(), + "the committed value must stay authoritative despite the raise" + ); + } + + // ---- recursion bound: an accidental listener cycle is stopped ---------- + + #[test] + fn listener_cycle_hits_the_recursion_bound_instead_of_hanging() { + let (lua, _reg) = fresh(); + run( + &lua, + r#" + pmacs.config.define{ name="a", description="d", type="integer", default=0 } + pmacs.config.define{ name="b", description="d", type="integer", default=0 } + n = 0 + pmacs.config.on_change('a', function(new) + n = n + 1 + pmacs.config.set('b', new) + end) + pmacs.config.on_change('b', function(new) + n = n + 1 + pmacs.config.set('a', new + 1) + end) + "#, + ) + .unwrap(); + + // The top-level call itself must return successfully -- each + // nested listener failure is logged and absorbed by the level + // above, so the cycle unwinds cleanly rather than raising all + // the way out. + run(&lua, "pmacs.config.set('a', 1)").expect("top-level call must not raise"); + + let n: i64 = eval(&lua, "return n").unwrap(); + assert_eq!( + n, + i64::from(MAX_DISPATCH_DEPTH), + "exactly MAX_DISPATCH_DEPTH listener invocations run before the bound stops the cycle" + ); + } + + // ---- dispose: idempotent, generation-safe, no GC dependency (F3, 22) --- + + #[test] + fn dispose_is_idempotent_and_a_dropped_undisposed_handle_keeps_firing() { + let (lua, _reg) = fresh(); + run( + &lua, + r#" + pmacs.config.define{ name="editing.x", description="d", type="boolean", default=true } + fires = 0 + do + local h = pmacs.config.on_change('editing.x', function() fires = fires + 1 end) + h:dispose() + h:dispose() -- idempotent: no error + end + collectgarbage("collect") + pmacs.config.set('editing.x', false) + "#, + ) + .unwrap(); + assert_eq!( + eval::(&lua, "return fires").unwrap(), + 0, + "disposed listener must not fire" + ); + + run( + &lua, + r#" + pmacs.config.define{ name="editing.y", description="d", type="boolean", default=true } + fires2 = 0 + do + local h2 = pmacs.config.on_change('editing.y', function() fires2 = fires2 + 1 end) + end + collectgarbage("collect") + pmacs.config.set('editing.y', false) + "#, + ) + .unwrap(); + assert_eq!( + eval::(&lua, "return fires2").unwrap(), + 1, + "a dropped-but-undisposed handle must keep firing (F3)" + ); + } + + #[test] + fn dispose_is_generation_safe_a_stale_id_never_disposes_a_newer_listener() { + let (lua, reg) = fresh(); + run( + &lua, + r#"pmacs.config.define{ name="editing.x", description="d", type="boolean", default=true }"#, + ) + .unwrap(); + run( + &lua, + r" + first_fires = 0 + h1 = pmacs.config.on_change('editing.x', function() first_fires = first_fires + 1 end) + h1:dispose() + second_fires = 0 + h2 = pmacs.config.on_change('editing.x', function() second_fires = second_fires + 1 end) + h1:dispose() -- stale: must not touch h2's listener + ", + ) + .unwrap(); + assert_eq!(reg.borrow().snapshot("editing.x").len(), 1); + run(&lua, "pmacs.config.set('editing.x', false)").unwrap(); + assert_eq!(eval::(&lua, "return first_fires").unwrap(), 0); + assert_eq!(eval::(&lua, "return second_fires").unwrap(), 1); + } + + // ---- startup freeze, lazy via InitCompleteFlag -------------------------- + + #[test] + fn set_after_init_complete_freezes_and_rejects_startup_only_writes() { + let (lua, _reg) = fresh(); + let flag = InitCompleteFlag::new(); + lua.set_app_data(flag.clone()); + run( + &lua, + r#"pmacs.config.define{ name="lsp.root-markers", description="d", type="boolean", default=true, mutability="startup" }"#, + ) + .unwrap(); + + // Before init completes: writable. + run(&lua, "pmacs.config.set('lsp.root-markers', false)").unwrap(); + + flag.set_complete(); + let err = run(&lua, "pmacs.config.set('lsp.root-markers', true)").unwrap_err(); + assert!(err.to_string().contains("startup-only"), "{err}"); + } + + #[test] + fn define_startup_only_always_rejects_set_local() { + let (lua, _reg) = fresh(); + run( + &lua, + r#"pmacs.config.define{ name="lsp.root-markers", description="d", type="boolean", default=true, mutability="startup" }"#, + ) + .unwrap(); + let buf = BufferId::next(); + lua.globals().set("__buf", BufferIdLua(buf)).unwrap(); + let err = run( + &lua, + "pmacs.config.set_local(__buf, 'lsp.root-markers', false)", + ) + .unwrap_err(); + assert!(err.to_string().contains("buffer-local"), "{err}"); + } + + // ---- describe(): field shape, buffer_local naming (F7), fresh table ---- + + #[test] + fn describe_has_buffer_local_field_only_when_a_buffer_override_exists() { + let (lua, reg) = fresh(); + run( + &lua, + r#"pmacs.config.define{ name="editing.x", description="d", type="boolean", default=true }"#, + ) + .unwrap(); + let buf = BufferId::next(); + lua.globals().set("__buf", BufferIdLua(buf)).unwrap(); + + assert!( + eval::( + &lua, + "return pmacs.config.describe('editing.x', __buf).buffer_local == nil" + ) + .unwrap(), + "buffer_local must be absent with no override" + ); + + reg.borrow_mut() + .set_local(buf, "editing.x", ConfigValue::Bool(false)) + .unwrap(); + assert!( + eval::( + &lua, + "return pmacs.config.describe('editing.x', __buf).buffer_local == false" + ) + .unwrap(), + "buffer_local must reflect the stored override" + ); + + // Every documented field is present, and describe() returns a + // FRESH table each call (mutating one call's result cannot + // affect the next). + assert!( + eval::( + &lua, + r#" + local info = pmacs.config.describe('editing.x', __buf) + info.name = "mutated" + local info2 = pmacs.config.describe('editing.x', __buf) + return info2.name == 'editing.x' + and info2.description == 'd' + and info2.type == 'boolean' + and info2.default == true + and info2.mutability == 'live' + and info2.value == false + and info2.global == true + and type(info2.source) == 'string' + "# + ) + .unwrap() + ); + } + + #[test] + fn list_is_stable_and_returns_full_descriptor_tables() { + let (lua, _reg) = fresh(); + run( + &lua, + r#" + pmacs.config.define{ name="c", description="d", type="boolean", default=true } + pmacs.config.define{ name="a", description="d", type="boolean", default=true } + pmacs.config.define{ name="b", description="d", type="boolean", default=true } + "#, + ) + .unwrap(); + + let check = r" + local l = pmacs.config.list() + return #l == 3 and l[1].name == 'c' and l[2].name == 'a' and l[3].name == 'b' + and l[1].description == 'd' + "; + assert!( + eval::(&lua, check).unwrap(), + "list() must be stable across repeated calls" + ); + assert!(eval::(&lua, check).unwrap(), "and on a second call"); + } + + // ---- acceptance 6 (generic across luajit/lua54): exactness by value ---- + + #[test] + fn integer_exactness_is_checked_by_value_not_math_type() { + let (lua, _reg) = fresh(); + run( + &lua, + r#"pmacs.config.define{ name="n", description="d", type="integer", default=0 }"#, + ) + .unwrap(); + run(&lua, "pmacs.config.set('n', 1500)").unwrap(); + assert_eq!( + eval::(&lua, "return pmacs.config.get('n')").unwrap(), + 1500 + ); + + let err = run(&lua, "pmacs.config.set('n', 1500.7)").unwrap_err(); + assert!(err.to_string().contains("exact integer"), "{err}"); + + // A whole-numbered float is accepted exactly (matters most + // under LuaJIT, where this is the ONLY way an integer literal + // ever arrives -- Lua 5.1 has no integer subtype). + run(&lua, "pmacs.config.set('n', 42.0)").unwrap(); + assert_eq!( + eval::(&lua, "return pmacs.config.get('n')").unwrap(), + 42 + ); + + // Out-of-i64-range / non-finite floats are rejected too. + let err2 = run(&lua, "pmacs.config.set('n', 1/0)").unwrap_err(); + assert!(err2.to_string().contains("finite"), "{err2}"); + } + + #[test] + fn is_set_reports_override_presence_and_reset_drops_exactly_one_layer() { + let (lua, reg) = fresh(); + run( + &lua, + r#"pmacs.config.define{ name="editing.x", description="d", type="boolean", default=true }"#, + ) + .unwrap(); + let buf = BufferId::next(); + lua.globals().set("__buf", BufferIdLua(buf)).unwrap(); + + assert!(!eval::(&lua, "return pmacs.config.is_set('editing.x')").unwrap()); + run(&lua, "pmacs.config.set('editing.x', true)").unwrap(); // equal to default (F1) + assert!( + eval::(&lua, "return pmacs.config.is_set('editing.x')").unwrap(), + "an equal-valued override is still stored (F1)" + ); + + run(&lua, "pmacs.config.set_local(__buf, 'editing.x', false)").unwrap(); + assert!(eval::(&lua, "return pmacs.config.is_set('editing.x', __buf)").unwrap()); + + run(&lua, "pmacs.config.reset('editing.x', __buf)").unwrap(); + assert!(!eval::(&lua, "return pmacs.config.is_set('editing.x', __buf)").unwrap()); + assert!( + eval::(&lua, "return pmacs.config.is_set('editing.x')").unwrap(), + "reset(name, buf) drops only the local layer" + ); + + run(&lua, "pmacs.config.reset('editing.x')").unwrap(); + assert!(!eval::(&lua, "return pmacs.config.is_set('editing.x')").unwrap()); + drop(reg); + } + + // ---- define-before-use (Q#CR10) ----------------------------------------- + + #[test] + fn ops_on_an_undefined_name_raise_not_found() { + let (lua, _reg) = fresh(); + for src in [ + "pmacs.config.get('nope')", + "pmacs.config.set('nope', true)", + "pmacs.config.is_set('nope')", + "pmacs.config.describe('nope')", + "pmacs.config.reset('nope')", + ] { + let err = run(&lua, src).unwrap_err(); + assert!(err.to_string().contains("not defined"), "{src}: {err}"); + } + } + + // ---- enum / bounds validation surfaces through the Lua boundary -------- + + #[test] + fn enum_and_bounds_validation_surfaces_through_lua() { + let (lua, _reg) = fresh(); + run( + &lua, + r#" + pmacs.config.define{ name="editing.eol", description="d", type="enum", default="lf", choices={"lf","crlf"} } + pmacs.config.define{ name="autosave.interval-ms", description="d", type="integer", default=30000, min=1000 } + "#, + ) + .unwrap(); + + let err = run(&lua, "pmacs.config.set('editing.eol', 'cr')").unwrap_err(); + assert!(err.to_string().contains("cr"), "{err}"); + + let err2 = run(&lua, "pmacs.config.set('autosave.interval-ms', 500)").unwrap_err(); + assert!(err2.to_string().contains("range"), "{err2}"); + } +} diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 9af5b5b..fd104ef 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -82,6 +82,7 @@ use crate::workers_buffer; // beyond call seams. Public entry points a domain owns are re-exported here // so external `crate::lua_bindings::` paths (and in-file uses) stay // stable. +mod config; mod diag; mod index; mod mcp; @@ -1459,6 +1460,9 @@ fn after_buffer_removed(lua: &Lua, id: BufferId) { if let Some(keymaps) = lua.app_data_ref::() { keymaps.borrow_mut().remove_buffer(id); } + if let Some(config) = lua.app_data_ref::() { + config.borrow_mut().remove_buffer(id); + } let callbacks = match lua.app_data_ref::() { Some(callbacks) => callbacks.take(id), None => Vec::new(), @@ -2277,6 +2281,9 @@ pub fn install( lua.set_app_data(BufferRemoveCallbacks::new()); let statusline = Rc::new(RefCell::new(StatuslineRegistry::new())); lua.set_app_data(statusline.clone()); + let config_registry: config::SharedConfigRegistry = + Rc::new(RefCell::new(crate::config_registry::ConfigRegistry::new())); + lua.set_app_data(config_registry.clone()); let pmacs = lua.create_table()?; pmacs.set("buffer", install_buffer_module(lua, registry)?)?; @@ -2285,6 +2292,7 @@ pub fn install( pmacs.set("menu", install_menu_module(lua, menus)?)?; pmacs.set("hook", install_hook_module(lua, hooks)?)?; pmacs.set("statusline", install_statusline_module(lua, &statusline)?)?; + pmacs.set("config", config::install_config(lua, &config_registry)?)?; // Wall-clock millis (since UNIX epoch). Used by builtin runtime // chunks for timeout loops; `os.clock()` only counts CPU time and // is a poor fit for "wait until something arrives over I/O". diff --git a/tests/config_registry_acceptance.rs b/tests/config_registry_acceptance.rs new file mode 100644 index 0000000..b5e7a74 --- /dev/null +++ b/tests/config_registry_acceptance.rs @@ -0,0 +1,415 @@ +//! Config-registry acceptance (docs/config-registry-framing.md). +//! +//! The registry's own semantics are unit-tested in +//! `src/config_registry.rs` (value/scope/epoch/listener behavior) and +//! `src/lua_bindings/config.rs` (the Lua boundary). This suite covers +//! only what those cannot reach: the three adopters wired into a real +//! `EditorState`, the owner-defines source-location contract observed +//! after actual chunk load, and `M-x describe-setting` rendering +//! through the real minibuffer. +//! +//! Framing acceptance items covered here: 9, 19, 26, 27, 28, 29, 30, 33. +//! +//! Pairing is exercised by DISPATCHING keys, never +//! `pmacs.command.invoke` — pair.lua reacts to `buffer.after-edit` +//! with a typed-edit record that only real dispatch produces, so an +//! invoke-driven test would pass vacuously against a broken gate. + +use std::path::PathBuf; +use std::sync::atomic::{AtomicUsize, Ordering}; + +use crossterm::event::{KeyCode, KeyEvent, KeyEventKind, KeyEventState, KeyModifiers}; +use pmacs::editor::EditorState; +use pmacs::lua_bindings::StateDir; +use pmacs::protocol::FrontendId; + +// --------------------------------------------------------------------------- +// Harness (mirrors tests/auto_pair_acceptance.rs) +// --------------------------------------------------------------------------- + +fn exec(s: &EditorState, src: &str) { + s.lua_host.lua().load(src.to_string()).exec().unwrap(); +} + +fn eval(s: &EditorState, src: &str) -> T { + s.lua_host.lua().load(src.to_string()).eval().unwrap() +} + +fn fresh_state_dir() -> PathBuf { + static SEQ: AtomicUsize = AtomicUsize::new(0); + let dir = std::env::temp_dir().join(format!( + "pmacs-configreg-{}-{}", + std::process::id(), + SEQ.fetch_add(1, Ordering::Relaxed) + )); + std::fs::create_dir_all(&dir).unwrap(); + dir +} + +fn editor(state_dir: &std::path::Path) -> EditorState { + let s = EditorState::new(); + s.lua_host.lua().remove_app_data::(); + s.lua_host + .lua() + .set_app_data(StateDir(state_dir.to_path_buf())); + // Language DETECTION must work; server SPAWNING must not (rust and + // python carry default configs and the after-load hook would spawn + // real servers). + exec(&s, "pmacs.lsp.config = {}"); + s +} + +fn write_file(dir: &std::path::Path, name: &str, body: &str) -> String { + let p = dir.join(name); + std::fs::write(&p, body).unwrap(); + p.display().to_string() +} + +fn key(code: KeyCode, mods: KeyModifiers) -> KeyEvent { + KeyEvent { + code, + modifiers: mods, + kind: KeyEventKind::Press, + state: KeyEventState::NONE, + } +} + +fn type_str(s: &mut EditorState, text: &str) { + for ch in text.chars() { + s.dispatch_key( + FrontendId::LOCAL, + key(KeyCode::Char(ch), KeyModifiers::NONE), + ); + } +} + +fn press(s: &mut EditorState, code: KeyCode) { + s.dispatch_key(FrontendId::LOCAL, key(code, KeyModifiers::NONE)); +} + +fn active_text(s: &EditorState) -> String { + let b: mlua::String = eval( + s, + "local b = pmacs.window.buffer(); return b:slice(0, b:len())", + ); + String::from_utf8_lossy(&b.as_bytes()).into_owned() +} + +/// `show_help_text` ends by switching the window to `*help*`, so after +/// a describe command the active buffer IS the help buffer. Asserting +/// through the active buffer also proves the switch happened, which a +/// direct by-name lookup would silently tolerate skipping. +fn help_text(s: &EditorState) -> String { + let name: String = eval(s, "return pmacs.window.buffer():name()"); + assert_eq!(name, "*help*", "the describe command must display *help*"); + active_text(s) +} + +// --------------------------------------------------------------------------- +// Item 26 / 9 — owner-defines, observed after real chunk load +// --------------------------------------------------------------------------- + +#[test] +fn builtin_defines_succeed_at_chunk_load_and_report_their_owning_module() { + let s = editor(&fresh_state_dir()); + + // Item 26: the define calls at the top of pair.lua / editops.lua / + // autosave.lua ran during EditorState::new, which is only possible + // if pmacs.config was populated before the first runtime chunk. + for name in [ + "editing.auto-pair", + "editing.trim-on-save", + "autosave.interval-ms", + ] { + let known: bool = eval( + &s, + &format!("return pmacs.config.describe({name:?}) ~= nil"), + ); + assert!(known, "{name} must be defined by its owning module"); + } + + // Item 9: each definition's SourceLocation points at the module + // that owns the setting, not at a shared helper. This is exactly + // what a centralized define table in a config.lua would have + // broken (framing Q#CR14). + let pair_src: String = eval( + &s, + "return pmacs.config.describe('editing.auto-pair').source", + ); + assert!( + pair_src.contains("pair.lua"), + "editing.auto-pair must report pair.lua as its source, got {pair_src:?}" + ); + let trim_src: String = eval( + &s, + "return pmacs.config.describe('editing.trim-on-save').source", + ); + assert!( + trim_src.contains("editops.lua"), + "editing.trim-on-save must report editops.lua, got {trim_src:?}" + ); + let auto_src: String = eval( + &s, + "return pmacs.config.describe('autosave.interval-ms').source", + ); + assert!( + auto_src.contains("autosave.lua"), + "autosave.interval-ms must report autosave.lua, got {auto_src:?}" + ); +} + +// --------------------------------------------------------------------------- +// Item 29 — the flagship: per-buffer auto-pair, the feature this arc exists for +// --------------------------------------------------------------------------- + +#[test] +fn auto_pair_off_buffer_locally_suppresses_only_that_buffer() { + let dir = fresh_state_dir(); + let mut s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + let b = write_file(&dir, "b.rs", ""); + + // Two buffers of the SAME language — so a language-keyed + // implementation could not pass this test. The handle is stashed in + // a Lua global because buffer handles are userdata, not ids. + exec(&s, &format!("BUF_A = pmacs.buffer.find_or_open({a:?})")); + exec(&s, &format!("pmacs.buffer.find_or_open({b:?})")); + + // Turn pairing off in buffer A only. + exec( + &s, + "pmacs.config.set_local(BUF_A, 'editing.auto-pair', false)", + ); + + // B (untouched) still pairs. + exec(&s, "pmacs.editor.goto_byte(0)"); + type_str(&mut s, "("); + assert_eq!( + active_text(&s), + "()", + "a buffer with no local override still pairs" + ); + + // A (overridden) does not. + exec(&s, &format!("pmacs.buffer.find_or_open({a:?})")); + exec(&s, "pmacs.editor.goto_byte(0)"); + type_str(&mut s, "("); + assert_eq!( + active_text(&s), + "(", + "the buffer-local override must suppress pairing in THIS buffer" + ); +} + +#[test] +fn auto_pair_off_globally_suppresses_everywhere() { + let dir = fresh_state_dir(); + let mut s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("pmacs.buffer.find_or_open({a:?})")); + exec(&s, "pmacs.config.set('editing.auto-pair', false)"); + exec(&s, "pmacs.editor.goto_byte(0)"); + type_str(&mut s, "("); + assert_eq!(active_text(&s), "(", "a global false suppresses pairing"); +} + +#[test] +fn auto_pair_defaults_on_so_the_migration_changed_no_default() { + let dir = fresh_state_dir(); + let mut s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("pmacs.buffer.find_or_open({a:?})")); + exec(&s, "pmacs.editor.goto_byte(0)"); + type_str(&mut s, "("); + assert_eq!(active_text(&s), "()", "pairing is on by default"); +} + +// --------------------------------------------------------------------------- +// Items 27 / 28 — the migration wrappers keep their legacy coercion (F4) +// --------------------------------------------------------------------------- + +#[test] +fn trim_on_save_wrapper_and_registry_are_interchangeable_both_ways() { + let s = editor(&fresh_state_dir()); + + // Wrapper write observed by the registry. + exec(&s, "pmacs.editops.trim_on_save(true)"); + let via_registry: bool = eval(&s, "return pmacs.config.get('editing.trim-on-save')"); + assert!(via_registry, "the wrapper's write must reach the registry"); + + // Registry write observed by the wrapper. + exec(&s, "pmacs.config.set('editing.trim-on-save', false)"); + let via_wrapper: bool = eval(&s, "return pmacs.editops.trim_on_save()"); + assert!( + !via_wrapper, + "the registry's write must be visible through the wrapper" + ); +} + +#[test] +fn trim_on_save_keeps_its_lenient_truthiness() { + // F4: the registry is strict (a real boolean or nothing), but this + // legacy setter has always accepted anything that is not literally + // `false`. A thin wrapper over a strict `set` would raise here. + let s = editor(&fresh_state_dir()); + exec(&s, "pmacs.editops.trim_on_save('yes')"); + let on: bool = eval(&s, "return pmacs.config.get('editing.trim-on-save')"); + assert!(on, "a non-false argument must still turn trimming on"); + + exec(&s, "pmacs.editops.trim_on_save(false)"); + let off: bool = eval(&s, "return pmacs.config.get('editing.trim-on-save')"); + assert!(!off, "a literal false must still turn it off"); +} + +#[test] +fn interval_ms_keeps_flooring_a_fractional_argument() { + // F4 again: `integer` demands exactness, so the wrapper must floor + // BEFORE handing the value over. Pre-migration this returned 1500. + let s = editor(&fresh_state_dir()); + let got: i64 = eval(&s, "return pmacs.autosave.interval_ms(1500.7)"); + assert_eq!(got, 1500, "a fractional interval floors, it does not raise"); + let stored: i64 = eval(&s, "return pmacs.config.get('autosave.interval-ms')"); + assert_eq!(stored, 1500, "and the floored value is what was stored"); +} + +#[test] +fn interval_ms_still_raises_below_the_floor() { + let s = editor(&fresh_state_dir()); + let raised: bool = eval( + &s, + "local ok = pcall(pmacs.autosave.interval_ms, 500); return not ok", + ); + assert!(raised, "sub-floor intervals must still raise"); + let unchanged: i64 = eval(&s, "return pmacs.autosave.interval_ms()"); + assert_eq!(unchanged, 30000, "a rejected set leaves the value alone"); +} + +// --------------------------------------------------------------------------- +// Item 30 — a direct registry write is what the tick will read +// --------------------------------------------------------------------------- + +#[test] +fn interval_change_through_the_registry_is_visible_to_the_cadence_reader() { + // The tick re-reads `pmacs.config.get("autosave.interval-ms")` every + // frame rather than a module-local, so a mid-session change through + // EITHER path applies without a restart. Asserting through the + // wrapper's getter proves the module-local is really gone: a stale + // upvalue would still report 30000 here. + let s = editor(&fresh_state_dir()); + exec(&s, "pmacs.config.set('autosave.interval-ms', 5000)"); + let seen: i64 = eval(&s, "return pmacs.autosave.interval_ms()"); + assert_eq!( + seen, 5000, + "a direct registry write must be what the cadence reads" + ); +} + +// --------------------------------------------------------------------------- +// Item 19 — user config runs after builtins define, so a set in init.lua lands +// --------------------------------------------------------------------------- + +#[test] +fn a_set_in_user_config_position_is_observed_by_the_consumer() { + // EditorState::new does not load user config in test builds, so + // this drives the same ORDER explicitly: every builtin has defined, + // and a user-config-shaped `set` now runs against those names and + // is observed by the adopter that owns each one. + let dir = fresh_state_dir(); + let mut s = editor(&dir); + exec( + &s, + r#" + pmacs.config.set("editing.auto-pair", false) + pmacs.config.set("editing.trim-on-save", true) + pmacs.config.set("autosave.interval-ms", 9000) + "#, + ); + assert!( + eval::(&s, "return pmacs.editops.trim_on_save()"), + "editops observes the user-config write" + ); + assert_eq!( + eval::(&s, "return pmacs.autosave.interval_ms()"), + 9000, + "autosave observes the user-config write" + ); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("pmacs.buffer.find_or_open({a:?})")); + exec(&s, "pmacs.editor.goto_byte(0)"); + type_str(&mut s, "("); + assert_eq!( + active_text(&s), + "(", + "pair.lua observes the user-config write" + ); +} + +// --------------------------------------------------------------------------- +// Item 33 — M-x describe-setting renders into *help* +// --------------------------------------------------------------------------- + +#[test] +fn describe_setting_renders_into_help_with_the_source_location() { + let s0 = editor(&fresh_state_dir()); + let mut s = s0; + exec(&s, "pmacs.command.invoke('editor.describe-setting')"); + type_str(&mut s, "editing.auto-pair"); + press(&mut s, KeyCode::Enter); + + let text = help_text(&s); + assert!( + text.contains("Setting: editing.auto-pair"), + "*help* must carry the setting header, got {text:?}" + ); + assert!( + text.contains("pair.lua"), + "the rendered source location must name the owning module, got {text:?}" + ); + assert!( + text.contains("Type:") && text.contains("boolean"), + "the type must be rendered, got {text:?}" + ); + assert!( + text.contains("Mutability:") && text.contains("live"), + "mutability must be rendered, got {text:?}" + ); +} + +#[test] +fn describe_setting_reports_an_unknown_name_in_the_status_line() { + // `describe` RAISES NotFound for an undefined name rather than + // returning nil (define-before-set, Q#CR10), so the command must + // pcall — without it this dispatch would surface a Lua traceback. + let s0 = editor(&fresh_state_dir()); + let mut s = s0; + exec(&s, "pmacs.command.invoke('editor.describe-setting')"); + type_str(&mut s, "editing.no-such-setting"); + press(&mut s, KeyCode::Enter); + + let status = s.core.borrow().status.clone(); + assert!( + format!("{status:?}").contains("no such setting"), + "an unknown name must report cleanly, got {status:?}" + ); +} + +#[test] +fn describe_setting_shows_a_buffer_local_override_when_one_exists() { + let dir = fresh_state_dir(); + let mut s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("pmacs.buffer.find_or_open({a:?})")); + exec( + &s, + "pmacs.config.set_local(pmacs.window.buffer(), 'editing.auto-pair', false)", + ); + exec(&s, "pmacs.command.invoke('editor.describe-setting')"); + type_str(&mut s, "editing.auto-pair"); + press(&mut s, KeyCode::Enter); + + let text = help_text(&s); + assert!( + text.contains("Buffer-local override:"), + "an existing buffer-local override must be reported, got {text:?}" + ); +} From 82ae2fb5ca0d5710b249619453ff45342f0b910a Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 21 Jul 2026 16:24:44 -0400 Subject: [PATCH 3/4] fix(config): reject the saturating i64 upper boundary in int_from_f64 Review round 1, finding 1. `i64::MAX as f64` rounds UP to 2^63 = 9223372036854775808.0, one greater than `i64::MAX`. The guard used `>`, so a value of exactly 2^63 passed validation and `v as i64` then saturated it to 9223372036854775807 --- the registry silently stored a different number than the caller wrote. The bounds are asymmetric on purpose, and the fix must not be applied to both ends: `i64::MIN as f64` IS exactly -2^63 and round-trips, so the lower comparison stays `<`. Tightening it in sympathy would wrongly reject a legitimate value. Three tests: the boundary is rejected (fails against the `>` form, bite-verified), `i64::MIN` is still accepted, and the largest representable integer below the boundary (2^63 - 1024) still converts. Also from review round 1, finding 2, which does NOT reproduce: the concern was that because `set_local` never calls `maybe_freeze_after_init`, a `set_local` as the first post-init operation would defer the lazy freeze and let a later `StartupOnly` write slip through. It cannot --- `set` and `reset` call `maybe_freeze_after_init` as their first statement, ahead of the definition lookup and the mutator, so the freeze always lands before the check within the same call. No code change; the ordering is now pinned by a test that drives exactly that sequence (post-init `set_local` on a Live key, then a `StartupOnly` write) and asserts the write is still refused and that the refused set is itself what triggered the freeze. Gates: fmt, clippy -D warnings, --lib (1687), --lib --features crdt (1861), lua54 backend, config_registry_acceptance (13), and the full workspace sweep (2799 tests, exit 0). git diff --check clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/config_registry.rs | 49 +++++++++++++++++++++++++++++++++- src/lua_bindings/config.rs | 54 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) diff --git a/src/config_registry.rs b/src/config_registry.rs index 964dbd2..f089c6a 100644 --- a/src/config_registry.rs +++ b/src/config_registry.rs @@ -356,7 +356,13 @@ impl ConfigValue { value: v, }); } - if v.fract() != 0.0 || v < i64::MIN as f64 || v > i64::MAX as f64 { + // The bounds are deliberately ASYMMETRIC. `i64::MIN as f64` is + // -2^63, which round-trips exactly, so `<` correctly admits it. + // `i64::MAX as f64` rounds UP to 2^63 --- one more than + // `i64::MAX` --- so a `>` here would admit exactly 2^63 and then + // `v as i64` would saturate it to `i64::MAX`, silently storing a + // different number than the caller wrote. `>=` rejects it. + if v.fract() != 0.0 || v < i64::MIN as f64 || v >= i64::MAX as f64 { return Err(ConfigError::NonIntegral { name: name.to_owned(), value: v, @@ -2121,4 +2127,45 @@ mod tests { assert!(matches!(err, ConfigError::NonFiniteNumber { .. })); } } + + #[test] + fn int_from_f64_rejects_the_saturating_upper_boundary() { + // Review round 1, finding 1. `i64::MAX as f64` rounds UP to + // 2^63 = 9223372036854775808.0, one more than i64::MAX. With a + // `>` guard this value passes validation and `v as i64` then + // SATURATES to 9223372036854775807 --- the registry silently + // stores a different number than the caller asked for. This + // test fails against the `>` form. + let boundary = i64::MAX as f64; + let err = ConfigValue::int_from_f64("x", boundary).unwrap_err(); + assert!( + matches!(err, ConfigError::NonIntegral { .. }), + "2^63 must be rejected, not saturated to i64::MAX" + ); + // Anything beyond it too. + assert!(ConfigValue::int_from_f64("x", boundary * 2.0).is_err()); + } + + #[test] + fn int_from_f64_accepts_the_exact_lower_boundary() { + // The bounds are asymmetric on purpose: unlike the upper one, + // `i64::MIN as f64` IS exactly i64::MIN and round-trips, so + // tightening the lower comparison to `<=` alongside the upper + // `>=` would wrongly reject a legitimate value. + let lo = i64::MIN as f64; + assert_eq!( + ConfigValue::int_from_f64("x", lo).unwrap(), + ConfigValue::Int(i64::MIN), + "i64::MIN is representable and must still be accepted" + ); + } + + #[test] + fn int_from_f64_accepts_the_largest_representable_integer_below_the_boundary() { + // The next f64 below 2^63 is 2^63 - 1024, which is a valid i64. + // Pins that the `>=` fix did not over-reject the top of range. + let below = (i64::MAX as f64) - 1024.0; + let got = ConfigValue::int_from_f64("x", below).unwrap(); + assert_eq!(got, ConfigValue::Int(9_223_372_036_854_774_784)); + } } diff --git a/src/lua_bindings/config.rs b/src/lua_bindings/config.rs index 43e0423..960576e 100644 --- a/src/lua_bindings/config.rs +++ b/src/lua_bindings/config.rs @@ -1253,6 +1253,60 @@ mod tests { assert!(err.to_string().contains("startup-only"), "{err}"); } + #[test] + fn a_set_local_first_does_not_open_a_startup_only_write_window() { + // Review round 1, finding 2. `set_local` does not call + // `maybe_freeze_after_init`, so the concern was that a + // `set_local` as the first post-init operation defers the freeze + // and lets a subsequent `StartupOnly` write slip through. + // + // It cannot: `set` and `reset` call `maybe_freeze_after_init` as + // their FIRST statement, before the definition lookup and before + // the mutator runs, so the freeze always lands ahead of the + // check in the very same call. This test drives that exact + // ordering -- post-init `set_local` on a Live key, then a + // `StartupOnly` write -- and asserts the write is still refused. + let (lua, reg) = fresh(); + let flag = InitCompleteFlag::new(); + lua.set_app_data(flag.clone()); + run( + &lua, + r#"pmacs.config.define{ name="editing.live-one", description="d", type="boolean", default=true }"#, + ) + .unwrap(); + run( + &lua, + r#"pmacs.config.define{ name="lsp.root-markers", description="d", type="boolean", default=true, mutability="startup" }"#, + ) + .unwrap(); + + flag.set_complete(); + assert!( + !reg.borrow().is_frozen(), + "precondition: nothing has triggered the lazy freeze yet" + ); + + // The first post-init operation is a set_local on a Live key. + let buf = BufferId::next(); + lua.globals().set("__buf", BufferIdLua(buf)).unwrap(); + run( + &lua, + "pmacs.config.set_local(__buf, 'editing.live-one', false)", + ) + .unwrap(); + + // The StartupOnly write must still be refused. + let err = run(&lua, "pmacs.config.set('lsp.root-markers', false)").unwrap_err(); + assert!( + err.to_string().contains("startup-only"), + "a set_local first must not open a write window: {err}" + ); + assert!( + reg.borrow().is_frozen(), + "the set that was refused is itself what triggered the freeze" + ); + } + #[test] fn define_startup_only_always_rejects_set_local() { let (lua, _reg) = fresh(); From f86c966090d0f5847be1681202b6a86568bf0922 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Tue, 21 Jul 2026 16:50:48 -0400 Subject: [PATCH 4/4] fix(config): reject wrongly-typed spec fields; make trim-on-save buffer-aware Review round 1, findings 2-4 plus doc notes. Finding 1 landed in fd80bcb. Finding 3 --- spec fields meaningless for the declared type are now rejected. DEFINE_SPEC_FIELDS whitelists all nine keys for every type and the kind parser only reads its own arm's fields, so `{ type = "string", choices = {...} }` silently defined a string that accepts anything (the author meant enum) and `min` on a boolean was dropped. These are typo-shaped bugs the R50 whitelist structurally cannot see: the key is spelled correctly, it is on the wrong type. `check_fields_relevant_to_kind` closes it with a pointed error naming the misplaced field, and a companion test pins that each field is still accepted where it belongs, including `min`/`max` on number as well as integer. Finding 4 --- the after_buffer_removed purge had no end-to-end test. Every existing test called ConfigRegistry::remove_buffer directly, so deleting the three lines wired into mod.rs would have left the whole suite green. The new acceptance test kills a buffer through pmacs.buffer.remove (the real remove_buffer_and_fire route) and asserts the locals are gone; bite-verified by removing the hunk and watching it fail. Finding 2 (the half with a natural buffer) --- editing.trim-on-save is now resolved against the buffer being saved rather than the global chain. Reading globally meant set_local was accepted, stored, and reported by describe, then never consulted: a pin the user believes in that does nothing, which is the shape F1 exists to prevent. Two tests, one for the override and one for the global fallback the change could have broken; the override test fails against the old global read. Both new save tests initially passed VACUOUSLY and were rewritten: pmacs.editor.save() is the raw save, while buffer.before-save fires inside the buffer.save COMMAND (default.lua:224), and save() no-ops on an unmodified buffer --- so the original form asserted on a file that was never rewritten. They now insert content to dirty the buffer and go through pmacs.command.invoke("buffer.save"). The other half of finding 2 --- a per-buffer autosave.interval-ms is semantically meaningless yet still accepted --- is recorded as a named deferral proposing a define-time `scope = "global"` flag, alongside deferrals for bound-parse field naming and StartupOnly reset symmetry. Also recorded: interval_ms(1e30) now raises instead of storing a nonsense float, an improvement but a real divergence from "the wrapper's shape stays exactly as it was". Doc: the module header cited framing revision 2; the shipped doc is revision 3, whose corrections are what the code implements. Gates: fmt, clippy -D warnings, --lib (1691), --lib --features crdt (1865), lua54 backend, config_registry_acceptance (16), editops (72), autosave (29), PMACS_REQUIRE_GPU=1 pmacs-gpu (109), and the full workspace sweep (2806 tests, exit 0). git diff --check clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- builtin/runtime/editops.lua | 10 +- docs/config-registry-framing.md | 29 ++++++ src/config_registry.rs | 2 +- src/lua_bindings/config.rs | 149 ++++++++++++++++++++++++++++ tests/config_registry_acceptance.rs | 94 ++++++++++++++++++ 5 files changed, 282 insertions(+), 2 deletions(-) diff --git a/builtin/runtime/editops.lua b/builtin/runtime/editops.lua index f78b677..6a071f5 100644 --- a/builtin/runtime/editops.lua +++ b/builtin/runtime/editops.lua @@ -874,7 +874,15 @@ pmacs.hook.add("buffer.before-save", function() -- either way). Both reports are pcall'd — a broken reporting -- channel must not resurrect the veto. local ok, err = pcall(function() - if pmacs.config.get("editing.trim-on-save") then + -- Resolved against the buffer being saved, not the global chain + -- (review round 1, finding 2). `buffer.before-save` fires for the + -- ACTIVE buffer -- which is also the one `trim_active` rewrites -- + -- so passing it here is what makes a buffer-local override mean + -- something. Reading globally would accept `set_local`, store it, + -- report it from `describe`, and then never consult it: a pin the + -- user believes in that does nothing, which is the same failure + -- shape F1 exists to prevent. + if pmacs.config.get("editing.trim-on-save", pmacs.window.buffer()) then trim_active("delete-trailing-whitespace (on save)") end end) diff --git a/docs/config-registry-framing.md b/docs/config-registry-framing.md index a6173b7..209b194 100644 --- a/docs/config-registry-framing.md +++ b/docs/config-registry-framing.md @@ -550,6 +550,14 @@ already-conforming value. The registry stays strict; the legacy API stays lenient; acceptance 27-28 pin both directions on inputs no current test covers. +One honest divergence from "the wrapper's shape stays exactly as it +was", found in review round 1: `pmacs.autosave.interval_ms(1e30)` +previously stored the float, and now raises `NonIntegral` because the +floored value exceeds `i64` range. The old behavior stored a nonsense +interval; the new one refuses it. An improvement, but a behavior change +at the extreme rather than a pure no-op migration, so it is recorded +here rather than claimed away. + Richer feature-specific APIs are explicitly not deleted: `pmacs.gpu.set_font` remains the font preference API until a separately framed migration decides how a daemon-global preference and @@ -809,6 +817,27 @@ points. - **`M-x list-settings`** as a listview panel (Q#CR11). - **A `customize`-style editing UI**, and a `:set`-style minibuffer command. +- **A `scope = "global"` define flag** (review round 1, finding 2). + `set_local` currently succeeds for any `Live` setting, including ones + whose consumer only ever reads the global chain. `editing.trim-on-save` + was fixed by making its consumer buffer-aware — the save hook now + resolves against the buffer being saved — but a per-buffer + `autosave.interval-ms` is *semantically* meaningless (there is one + sweep timer, not one per buffer) and the API still accepts it, stores + it, and reports it from `describe`. That is a stored value nothing + reads, the same shape F1 exists to prevent. The fix is a define-time + scope declaration letting the registry refuse `set_local` outright, + exactly as `StartupOnlyLocal` already refuses another meaningless + combination. Deferred rather than taken in review round 1 because it + adds public API surface after review. +- **Field-naming in bound-parse errors** — a bad `min` reports the + config name and the expected type but not *which* of `min`/`max` + offended. Cheap, but wants its own error variant rather than a + synthesized pseudo-name. +- **`reset(name, buf)` symmetry for `StartupOnly`** — `set_local` is + refused with `StartupOnlyLocal` but the buffer-local `reset` is + allowed. Unreachable-harmless today (no such local can exist), so a + rejection would be symmetry rather than a fix. --- diff --git a/src/config_registry.rs b/src/config_registry.rs index f089c6a..04db52a 100644 --- a/src/config_registry.rs +++ b/src/config_registry.rs @@ -2,7 +2,7 @@ //! Configuration settings. //! -//! Per `docs/config-registry-framing.md` (revision 2), every editor +//! Per `docs/config-registry-framing.md` (revision 3), every editor //! setting is a named, typed [`ConfigDefinition`] registered in a //! [`ConfigRegistry`], mirroring [`crate::command::CommandRegistry`] //! and [`crate::hook::HookRegistry`] in shape and error vocabulary: diff --git a/src/lua_bindings/config.rs b/src/lua_bindings/config.rs index 960576e..4739ce7 100644 --- a/src/lua_bindings/config.rs +++ b/src/lua_bindings/config.rs @@ -185,6 +185,22 @@ enum ConfigBindingError { name: String, }, + /// `define` carried a spec field that means nothing for the + /// declared `type` --- `choices` on a string, `min` on a boolean. + /// Correctly spelled, wrongly placed: the R50 whitelist cannot see + /// it, so it is checked against the kind instead. + #[error( + "config field `{field}` is meaningless for type = \"{got_type}\"; \ + `min`/`max` require integer or number, `choices` requires enum, \ + `allow_empty` requires string" + )] + FieldIrrelevantForType { + /// The offending field name. + field: &'static str, + /// The declared type it was paired with. + got_type: String, + }, + /// `define`'s `mutability` field wasn't `"live"` or `"startup"`. #[error("unknown config mutability `{got}`; expected one of: live, startup")] UnknownMutability { @@ -347,10 +363,42 @@ fn read_choices(spec: &Table, name: &str) -> mlua::Result> { Ok(choices) } +/// Reject spec fields that are meaningless for the declared `type` +/// (review round 1, finding 3). +/// +/// `DEFINE_SPEC_FIELDS` whitelists every key for every type, and the +/// kind parser below only reads the ones its own arm cares about --- +/// so without this check `{ type = "string", choices = {"a","b"} }` +/// silently defines a string that accepts anything (the author meant +/// `enum`), and `{ type = "boolean", min = 1 }` silently drops the +/// bound. Both are typo-shaped bugs of exactly the class R50 exists to +/// catch; the whitelist alone only catches misspelled keys, not +/// correctly-spelled keys on the wrong type. +fn check_fields_relevant_to_kind(spec: &Table, type_str: &str) -> mlua::Result<()> { + let numeric = matches!(type_str, "integer" | "number"); + for (field, allowed_for) in [ + ("min", numeric), + ("max", numeric), + ("choices", type_str == "enum"), + ("allow_empty", type_str == "string"), + ] { + if !allowed_for && !matches!(spec.raw_get::(field)?, Value::Nil) { + return Err(mlua::Error::external( + ConfigBindingError::FieldIrrelevantForType { + field, + got_type: type_str.to_owned(), + }, + )); + } + } + Ok(()) +} + fn parse_kind_and_default(spec: &Table, name: &str) -> mlua::Result<(ConfigKind, ConfigValue)> { let type_str: Option = spec.raw_get("type")?; let type_str = type_str.ok_or_else(|| mlua::Error::external(ConfigBindingError::MissingType))?; + check_fields_relevant_to_kind(spec, &type_str)?; let kind = match type_str.as_str() { "boolean" => ConfigKind::Boolean, @@ -931,6 +979,107 @@ mod tests { ); } + // ---- review round 1, finding 3: correctly spelled, wrongly typed -------- + + #[test] + fn define_rejects_spec_fields_meaningless_for_the_declared_type() { + // The R50 whitelist accepts all nine keys for every type, so + // these are invisible to it: each field below is spelled + // correctly but means nothing for the type it is paired with. + // Before the kind cross-check, `type = "string"` with `choices` + // silently defined a string that accepts ANYTHING -- the author + // plainly meant `enum` -- and `min` on a boolean was dropped. + for (spec, offender) in [ + ( + r#"{ name="a.b", description="d", type="string", default="x", choices={"a"} }"#, + "choices", + ), + ( + r#"{ name="a.b", description="d", type="boolean", default=true, min=1 }"#, + "min", + ), + ( + r#"{ name="a.b", description="d", type="boolean", default=true, max=1 }"#, + "max", + ), + ( + r#"{ name="a.b", description="d", type="integer", default=1, allow_empty=true }"#, + "allow_empty", + ), + ( + r#"{ name="a.b", description="d", type="enum", choices={"x"}, default="x", min=1 }"#, + "min", + ), + ] { + let (lua, _reg) = fresh(); + let err = run(&lua, &format!("pmacs.config.define {spec}")).unwrap_err(); + assert!( + err.to_string().contains(offender), + "the error must name the misplaced field {offender}: {err}" + ); + let n: i64 = eval(&lua, "return #pmacs.config.list()").unwrap(); + assert_eq!(n, 0, "a rejected define registers nothing"); + } + } + + #[test] + fn define_still_accepts_each_field_on_its_own_type() { + // The guard must not over-reject: every field remains legal + // where it belongs, including on `number` as well as `integer`. + let (lua, _reg) = fresh(); + for spec in [ + r#"{ name="a.int", description="d", type="integer", default=5, min=1, max=9 }"#, + r#"{ name="a.num", description="d", type="number", default=0.5, min=0.0, max=1.0 }"#, + r#"{ name="a.str", description="d", type="string", default="", allow_empty=true }"#, + r#"{ name="a.enum", description="d", type="enum", default="x", choices={"x","y"} }"#, + r#"{ name="a.bool", description="d", type="boolean", default=true }"#, + ] { + run(&lua, &format!("pmacs.config.define {spec}")) + .unwrap_or_else(|e| panic!("{spec} must be accepted: {e}")); + } + let n: i64 = eval(&lua, "return #pmacs.config.list()").unwrap(); + assert_eq!(n, 5); + } + + // ---- review round 1, finding 1: the saturating i64 boundary, via Lua ---- + + #[test] + fn set_rejects_the_saturating_i64_boundary_from_lua() { + // 2^63 is a float on BOTH backends (LuaJIT has no integer + // subtype; Lua 5.4's `2^63` is a float too). Before the `>=` + // fix this stored i64::MAX -- a silent off-by-one against the + // module's own "never silently change its value" contract. + let (lua, _reg) = fresh(); + run( + &lua, + r#"pmacs.config.define{ name="a.n", description="d", type="integer", default=0 }"#, + ) + .unwrap(); + let err = run(&lua, "pmacs.config.set('a.n', 2^63)").unwrap_err(); + assert!( + err.to_string().contains("integer") || err.to_string().contains("integral"), + "2^63 must be refused, not saturated: {err}" + ); + let still: i64 = eval(&lua, "return pmacs.config.get('a.n')").unwrap(); + assert_eq!(still, 0, "the refused set must not have stored anything"); + } + + #[test] + fn define_rejects_the_saturating_i64_boundary_in_a_bound() { + // `min`/`max` go through read_bound_i64 -> lua_exact_i64 -> + // int_from_f64, so the same boundary must be refused in a spec. + let (lua, _reg) = fresh(); + let err = run( + &lua, + r#"pmacs.config.define{ name="a.n", description="d", type="integer", default=0, max=2^63 }"#, + ) + .unwrap_err(); + assert!( + err.to_string().contains("integer") || err.to_string().contains("integral"), + "a 2^63 bound must be refused: {err}" + ); + } + // ---- acceptance 9: SourceLocation from the DEFINING module -------------- #[test] diff --git a/tests/config_registry_acceptance.rs b/tests/config_registry_acceptance.rs index b5e7a74..41749fa 100644 --- a/tests/config_registry_acceptance.rs +++ b/tests/config_registry_acceptance.rs @@ -224,6 +224,50 @@ fn auto_pair_defaults_on_so_the_migration_changed_no_default() { assert_eq!(active_text(&s), "()", "pairing is on by default"); } +// --------------------------------------------------------------------------- +// Item 13 — the purge runs on the REAL buffer-death path +// --------------------------------------------------------------------------- + +#[test] +fn killing_a_buffer_through_the_real_path_purges_its_locals() { + // Review round 1, finding 4. Every other test of the purge calls + // `ConfigRegistry::remove_buffer` directly, so deleting the three + // lines wired into `after_buffer_removed` would leave them all + // green. This drives `pmacs.buffer.remove`, which is the production + // route (`remove_buffer_and_fire` -> `after_buffer_removed`), and + // fails if that wiring is absent. + // + // The assertion reads through the DEAD handle on purpose: BufferIds + // are never reused (buffer_registry.rs), so a stale id cannot alias + // a later buffer, and `is_set` against it reports exactly whether + // the registry still holds that buffer's map. + let dir = fresh_state_dir(); + let s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("DEAD = pmacs.buffer.find_or_open({a:?})")); + exec( + &s, + "pmacs.config.set_local(DEAD, 'editing.auto-pair', false)", + ); + assert!( + eval::(&s, "return pmacs.config.is_set('editing.auto-pair', DEAD)"), + "precondition: the buffer-local override is stored" + ); + + // Switch away first so killing the buffer cannot leave the window + // pointing at a dead buffer, then remove it through the real path. + exec(&s, "pmacs.buffer.remove(DEAD)"); + + assert!( + !eval::(&s, "return pmacs.config.is_set('editing.auto-pair', DEAD)"), + "the buffer's locals must be purged when it is removed" + ); + assert!( + eval::(&s, "return pmacs.config.get('editing.auto-pair', DEAD)"), + "and resolution falls back to the global default" + ); +} + // --------------------------------------------------------------------------- // Items 27 / 28 — the migration wrappers keep their legacy coercion (F4) // --------------------------------------------------------------------------- @@ -246,6 +290,56 @@ fn trim_on_save_wrapper_and_registry_are_interchangeable_both_ways() { ); } +#[test] +fn trim_on_save_honors_a_buffer_local_override() { + // Review round 1, finding 2. The save hook resolves against the + // buffer being saved, so `set_local` is a real per-buffer switch + // rather than a stored value nothing ever reads. + let dir = fresh_state_dir(); + let s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("BUF = pmacs.buffer.find_or_open({a:?})")); + // The content must be INSERTED, not merely present on disk: + // `save()` no-ops on an unmodified buffer, so a freshly-opened + // buffer would leave the file byte-identical and this test would + // pass without the save hook ever running. + exec(&s, r#"BUF:insert(0, "keep me \n")"#); + + // Globally on, but off for this buffer: trailing space survives. + exec(&s, "pmacs.config.set('editing.trim-on-save', true)"); + exec( + &s, + "pmacs.config.set_local(BUF, 'editing.trim-on-save', false)", + ); + exec(&s, "pmacs.command.invoke('buffer.save')"); + assert_eq!( + std::fs::read_to_string(&a).unwrap(), + "keep me \n", + "a buffer-local false must suppress trimming for this buffer" + ); +} + +#[test] +fn trim_on_save_still_falls_back_to_the_global_value() { + // The other half of finding 2's fix, and its regression guard: + // now that the hook passes a buffer, a broken fallback would make + // the global setting silently stop working. A separate editor and + // file because `save()` no-ops on an unmodified buffer, so the two + // cases cannot share one save cycle. + let dir = fresh_state_dir(); + let s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("BUF = pmacs.buffer.find_or_open({a:?})")); + exec(&s, r#"BUF:insert(0, "trim me \n")"#); + exec(&s, "pmacs.config.set('editing.trim-on-save', true)"); + exec(&s, "pmacs.command.invoke('buffer.save')"); + assert_eq!( + std::fs::read_to_string(&a).unwrap(), + "trim me\n", + "with no buffer-local override the global setting must still apply" + ); +} + #[test] fn trim_on_save_keeps_its_lenient_truthiness() { // F4: the registry is strict (a real boolean or nothing), but this