diff --git a/builtin/runtime/editops.lua b/builtin/runtime/editops.lua index f78b677..6a071f5 100644 --- a/builtin/runtime/editops.lua +++ b/builtin/runtime/editops.lua @@ -874,7 +874,15 @@ pmacs.hook.add("buffer.before-save", function() -- either way). Both reports are pcall'd — a broken reporting -- channel must not resurrect the veto. local ok, err = pcall(function() - if pmacs.config.get("editing.trim-on-save") then + -- Resolved against the buffer being saved, not the global chain + -- (review round 1, finding 2). `buffer.before-save` fires for the + -- ACTIVE buffer -- which is also the one `trim_active` rewrites -- + -- so passing it here is what makes a buffer-local override mean + -- something. Reading globally would accept `set_local`, store it, + -- report it from `describe`, and then never consult it: a pin the + -- user believes in that does nothing, which is the same failure + -- shape F1 exists to prevent. + if pmacs.config.get("editing.trim-on-save", pmacs.window.buffer()) then trim_active("delete-trailing-whitespace (on save)") end end) diff --git a/docs/config-registry-framing.md b/docs/config-registry-framing.md index a6173b7..209b194 100644 --- a/docs/config-registry-framing.md +++ b/docs/config-registry-framing.md @@ -550,6 +550,14 @@ already-conforming value. The registry stays strict; the legacy API stays lenient; acceptance 27-28 pin both directions on inputs no current test covers. +One honest divergence from "the wrapper's shape stays exactly as it +was", found in review round 1: `pmacs.autosave.interval_ms(1e30)` +previously stored the float, and now raises `NonIntegral` because the +floored value exceeds `i64` range. The old behavior stored a nonsense +interval; the new one refuses it. An improvement, but a behavior change +at the extreme rather than a pure no-op migration, so it is recorded +here rather than claimed away. + Richer feature-specific APIs are explicitly not deleted: `pmacs.gpu.set_font` remains the font preference API until a separately framed migration decides how a daemon-global preference and @@ -809,6 +817,27 @@ points. - **`M-x list-settings`** as a listview panel (Q#CR11). - **A `customize`-style editing UI**, and a `:set`-style minibuffer command. +- **A `scope = "global"` define flag** (review round 1, finding 2). + `set_local` currently succeeds for any `Live` setting, including ones + whose consumer only ever reads the global chain. `editing.trim-on-save` + was fixed by making its consumer buffer-aware — the save hook now + resolves against the buffer being saved — but a per-buffer + `autosave.interval-ms` is *semantically* meaningless (there is one + sweep timer, not one per buffer) and the API still accepts it, stores + it, and reports it from `describe`. That is a stored value nothing + reads, the same shape F1 exists to prevent. The fix is a define-time + scope declaration letting the registry refuse `set_local` outright, + exactly as `StartupOnlyLocal` already refuses another meaningless + combination. Deferred rather than taken in review round 1 because it + adds public API surface after review. +- **Field-naming in bound-parse errors** — a bad `min` reports the + config name and the expected type but not *which* of `min`/`max` + offended. Cheap, but wants its own error variant rather than a + synthesized pseudo-name. +- **`reset(name, buf)` symmetry for `StartupOnly`** — `set_local` is + refused with `StartupOnlyLocal` but the buffer-local `reset` is + allowed. Unreachable-harmless today (no such local can exist), so a + rejection would be symmetry rather than a fix. --- diff --git a/src/config_registry.rs b/src/config_registry.rs index f089c6a..04db52a 100644 --- a/src/config_registry.rs +++ b/src/config_registry.rs @@ -2,7 +2,7 @@ //! Configuration settings. //! -//! Per `docs/config-registry-framing.md` (revision 2), every editor +//! Per `docs/config-registry-framing.md` (revision 3), every editor //! setting is a named, typed [`ConfigDefinition`] registered in a //! [`ConfigRegistry`], mirroring [`crate::command::CommandRegistry`] //! and [`crate::hook::HookRegistry`] in shape and error vocabulary: diff --git a/src/lua_bindings/config.rs b/src/lua_bindings/config.rs index 960576e..4739ce7 100644 --- a/src/lua_bindings/config.rs +++ b/src/lua_bindings/config.rs @@ -185,6 +185,22 @@ enum ConfigBindingError { name: String, }, + /// `define` carried a spec field that means nothing for the + /// declared `type` --- `choices` on a string, `min` on a boolean. + /// Correctly spelled, wrongly placed: the R50 whitelist cannot see + /// it, so it is checked against the kind instead. + #[error( + "config field `{field}` is meaningless for type = \"{got_type}\"; \ + `min`/`max` require integer or number, `choices` requires enum, \ + `allow_empty` requires string" + )] + FieldIrrelevantForType { + /// The offending field name. + field: &'static str, + /// The declared type it was paired with. + got_type: String, + }, + /// `define`'s `mutability` field wasn't `"live"` or `"startup"`. #[error("unknown config mutability `{got}`; expected one of: live, startup")] UnknownMutability { @@ -347,10 +363,42 @@ fn read_choices(spec: &Table, name: &str) -> mlua::Result> { Ok(choices) } +/// Reject spec fields that are meaningless for the declared `type` +/// (review round 1, finding 3). +/// +/// `DEFINE_SPEC_FIELDS` whitelists every key for every type, and the +/// kind parser below only reads the ones its own arm cares about --- +/// so without this check `{ type = "string", choices = {"a","b"} }` +/// silently defines a string that accepts anything (the author meant +/// `enum`), and `{ type = "boolean", min = 1 }` silently drops the +/// bound. Both are typo-shaped bugs of exactly the class R50 exists to +/// catch; the whitelist alone only catches misspelled keys, not +/// correctly-spelled keys on the wrong type. +fn check_fields_relevant_to_kind(spec: &Table, type_str: &str) -> mlua::Result<()> { + let numeric = matches!(type_str, "integer" | "number"); + for (field, allowed_for) in [ + ("min", numeric), + ("max", numeric), + ("choices", type_str == "enum"), + ("allow_empty", type_str == "string"), + ] { + if !allowed_for && !matches!(spec.raw_get::(field)?, Value::Nil) { + return Err(mlua::Error::external( + ConfigBindingError::FieldIrrelevantForType { + field, + got_type: type_str.to_owned(), + }, + )); + } + } + Ok(()) +} + fn parse_kind_and_default(spec: &Table, name: &str) -> mlua::Result<(ConfigKind, ConfigValue)> { let type_str: Option = spec.raw_get("type")?; let type_str = type_str.ok_or_else(|| mlua::Error::external(ConfigBindingError::MissingType))?; + check_fields_relevant_to_kind(spec, &type_str)?; let kind = match type_str.as_str() { "boolean" => ConfigKind::Boolean, @@ -931,6 +979,107 @@ mod tests { ); } + // ---- review round 1, finding 3: correctly spelled, wrongly typed -------- + + #[test] + fn define_rejects_spec_fields_meaningless_for_the_declared_type() { + // The R50 whitelist accepts all nine keys for every type, so + // these are invisible to it: each field below is spelled + // correctly but means nothing for the type it is paired with. + // Before the kind cross-check, `type = "string"` with `choices` + // silently defined a string that accepts ANYTHING -- the author + // plainly meant `enum` -- and `min` on a boolean was dropped. + for (spec, offender) in [ + ( + r#"{ name="a.b", description="d", type="string", default="x", choices={"a"} }"#, + "choices", + ), + ( + r#"{ name="a.b", description="d", type="boolean", default=true, min=1 }"#, + "min", + ), + ( + r#"{ name="a.b", description="d", type="boolean", default=true, max=1 }"#, + "max", + ), + ( + r#"{ name="a.b", description="d", type="integer", default=1, allow_empty=true }"#, + "allow_empty", + ), + ( + r#"{ name="a.b", description="d", type="enum", choices={"x"}, default="x", min=1 }"#, + "min", + ), + ] { + let (lua, _reg) = fresh(); + let err = run(&lua, &format!("pmacs.config.define {spec}")).unwrap_err(); + assert!( + err.to_string().contains(offender), + "the error must name the misplaced field {offender}: {err}" + ); + let n: i64 = eval(&lua, "return #pmacs.config.list()").unwrap(); + assert_eq!(n, 0, "a rejected define registers nothing"); + } + } + + #[test] + fn define_still_accepts_each_field_on_its_own_type() { + // The guard must not over-reject: every field remains legal + // where it belongs, including on `number` as well as `integer`. + let (lua, _reg) = fresh(); + for spec in [ + r#"{ name="a.int", description="d", type="integer", default=5, min=1, max=9 }"#, + r#"{ name="a.num", description="d", type="number", default=0.5, min=0.0, max=1.0 }"#, + r#"{ name="a.str", description="d", type="string", default="", allow_empty=true }"#, + r#"{ name="a.enum", description="d", type="enum", default="x", choices={"x","y"} }"#, + r#"{ name="a.bool", description="d", type="boolean", default=true }"#, + ] { + run(&lua, &format!("pmacs.config.define {spec}")) + .unwrap_or_else(|e| panic!("{spec} must be accepted: {e}")); + } + let n: i64 = eval(&lua, "return #pmacs.config.list()").unwrap(); + assert_eq!(n, 5); + } + + // ---- review round 1, finding 1: the saturating i64 boundary, via Lua ---- + + #[test] + fn set_rejects_the_saturating_i64_boundary_from_lua() { + // 2^63 is a float on BOTH backends (LuaJIT has no integer + // subtype; Lua 5.4's `2^63` is a float too). Before the `>=` + // fix this stored i64::MAX -- a silent off-by-one against the + // module's own "never silently change its value" contract. + let (lua, _reg) = fresh(); + run( + &lua, + r#"pmacs.config.define{ name="a.n", description="d", type="integer", default=0 }"#, + ) + .unwrap(); + let err = run(&lua, "pmacs.config.set('a.n', 2^63)").unwrap_err(); + assert!( + err.to_string().contains("integer") || err.to_string().contains("integral"), + "2^63 must be refused, not saturated: {err}" + ); + let still: i64 = eval(&lua, "return pmacs.config.get('a.n')").unwrap(); + assert_eq!(still, 0, "the refused set must not have stored anything"); + } + + #[test] + fn define_rejects_the_saturating_i64_boundary_in_a_bound() { + // `min`/`max` go through read_bound_i64 -> lua_exact_i64 -> + // int_from_f64, so the same boundary must be refused in a spec. + let (lua, _reg) = fresh(); + let err = run( + &lua, + r#"pmacs.config.define{ name="a.n", description="d", type="integer", default=0, max=2^63 }"#, + ) + .unwrap_err(); + assert!( + err.to_string().contains("integer") || err.to_string().contains("integral"), + "a 2^63 bound must be refused: {err}" + ); + } + // ---- acceptance 9: SourceLocation from the DEFINING module -------------- #[test] diff --git a/tests/config_registry_acceptance.rs b/tests/config_registry_acceptance.rs index b5e7a74..41749fa 100644 --- a/tests/config_registry_acceptance.rs +++ b/tests/config_registry_acceptance.rs @@ -224,6 +224,50 @@ fn auto_pair_defaults_on_so_the_migration_changed_no_default() { assert_eq!(active_text(&s), "()", "pairing is on by default"); } +// --------------------------------------------------------------------------- +// Item 13 — the purge runs on the REAL buffer-death path +// --------------------------------------------------------------------------- + +#[test] +fn killing_a_buffer_through_the_real_path_purges_its_locals() { + // Review round 1, finding 4. Every other test of the purge calls + // `ConfigRegistry::remove_buffer` directly, so deleting the three + // lines wired into `after_buffer_removed` would leave them all + // green. This drives `pmacs.buffer.remove`, which is the production + // route (`remove_buffer_and_fire` -> `after_buffer_removed`), and + // fails if that wiring is absent. + // + // The assertion reads through the DEAD handle on purpose: BufferIds + // are never reused (buffer_registry.rs), so a stale id cannot alias + // a later buffer, and `is_set` against it reports exactly whether + // the registry still holds that buffer's map. + let dir = fresh_state_dir(); + let s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("DEAD = pmacs.buffer.find_or_open({a:?})")); + exec( + &s, + "pmacs.config.set_local(DEAD, 'editing.auto-pair', false)", + ); + assert!( + eval::(&s, "return pmacs.config.is_set('editing.auto-pair', DEAD)"), + "precondition: the buffer-local override is stored" + ); + + // Switch away first so killing the buffer cannot leave the window + // pointing at a dead buffer, then remove it through the real path. + exec(&s, "pmacs.buffer.remove(DEAD)"); + + assert!( + !eval::(&s, "return pmacs.config.is_set('editing.auto-pair', DEAD)"), + "the buffer's locals must be purged when it is removed" + ); + assert!( + eval::(&s, "return pmacs.config.get('editing.auto-pair', DEAD)"), + "and resolution falls back to the global default" + ); +} + // --------------------------------------------------------------------------- // Items 27 / 28 — the migration wrappers keep their legacy coercion (F4) // --------------------------------------------------------------------------- @@ -246,6 +290,56 @@ fn trim_on_save_wrapper_and_registry_are_interchangeable_both_ways() { ); } +#[test] +fn trim_on_save_honors_a_buffer_local_override() { + // Review round 1, finding 2. The save hook resolves against the + // buffer being saved, so `set_local` is a real per-buffer switch + // rather than a stored value nothing ever reads. + let dir = fresh_state_dir(); + let s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("BUF = pmacs.buffer.find_or_open({a:?})")); + // The content must be INSERTED, not merely present on disk: + // `save()` no-ops on an unmodified buffer, so a freshly-opened + // buffer would leave the file byte-identical and this test would + // pass without the save hook ever running. + exec(&s, r#"BUF:insert(0, "keep me \n")"#); + + // Globally on, but off for this buffer: trailing space survives. + exec(&s, "pmacs.config.set('editing.trim-on-save', true)"); + exec( + &s, + "pmacs.config.set_local(BUF, 'editing.trim-on-save', false)", + ); + exec(&s, "pmacs.command.invoke('buffer.save')"); + assert_eq!( + std::fs::read_to_string(&a).unwrap(), + "keep me \n", + "a buffer-local false must suppress trimming for this buffer" + ); +} + +#[test] +fn trim_on_save_still_falls_back_to_the_global_value() { + // The other half of finding 2's fix, and its regression guard: + // now that the hook passes a buffer, a broken fallback would make + // the global setting silently stop working. A separate editor and + // file because `save()` no-ops on an unmodified buffer, so the two + // cases cannot share one save cycle. + let dir = fresh_state_dir(); + let s = editor(&dir); + let a = write_file(&dir, "a.rs", ""); + exec(&s, &format!("BUF = pmacs.buffer.find_or_open({a:?})")); + exec(&s, r#"BUF:insert(0, "trim me \n")"#); + exec(&s, "pmacs.config.set('editing.trim-on-save', true)"); + exec(&s, "pmacs.command.invoke('buffer.save')"); + assert_eq!( + std::fs::read_to_string(&a).unwrap(), + "trim me\n", + "with no buffer-local override the global setting must still apply" + ); +} + #[test] fn trim_on_save_keeps_its_lenient_truthiness() { // F4: the registry is strict (a real boolean or nothing), but this