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..6a071f5 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,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 trim_enabled 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/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 new file mode 100644 index 0000000..209b194 --- /dev/null +++ b/docs/config-registry-framing.md @@ -0,0 +1,1022 @@ +# 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. + +**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`) + +### `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. + +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 +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. + +**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 +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 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 +`*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 — No runtime chunk; each module defines its own keys + +**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 +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. +- **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. + +--- + +## 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` 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 + `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. `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. 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: + +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), 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: + +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. diff --git a/src/config_registry.rs b/src/config_registry.rs new file mode 100644 index 0000000..04db52a --- /dev/null +++ b/src/config_registry.rs @@ -0,0 +1,2171 @@ +// config_registry.rs --- Typed, two-scope, introspectable configuration registry. + +//! Configuration settings. +//! +//! 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: +//! 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, + }); + } + // 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, + }); + } + 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 { .. })); + } + } + + #[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/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..4739ce7 --- /dev/null +++ b/src/lua_bindings/config.rs @@ -0,0 +1,1663 @@ +// 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` 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 { + /// 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) +} + +/// 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, + "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" + ); + } + + // ---- 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] + 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 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(); + 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..41749fa --- /dev/null +++ b/tests/config_registry_acceptance.rs @@ -0,0 +1,509 @@ +//! 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"); +} + +// --------------------------------------------------------------------------- +// 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) +// --------------------------------------------------------------------------- + +#[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_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 + // 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:?}" + ); +}