diff --git a/builtin/runtime/compile.lua b/builtin/runtime/compile.lua index e6e85d2..1c2c674 100644 --- a/builtin/runtime/compile.lua +++ b/builtin/runtime/compile.lua @@ -803,12 +803,17 @@ local function start_run(slot, cmdline, opts) -- supersedes anything, rewrites the buffer, or spawns a process, so -- an unknown value leaves no half-started run behind. In Stages 1-2 -- omission means "current"; Stage 3 flips the default. - local display = opts.display - if display ~= nil and display ~= "current" and display ~= "panel" then - error(string.format( - "compile.run: unknown display %q (expected \"current\" or \"panel\")", - tostring(display))) - end + -- Q#S3-1: one shared rule for vocabulary, error text and default. + -- + -- `display_omitted` is captured SEPARATELY and deliberately. The + -- resolver collapses omission into its default, but the recompile gate + -- below distinguishes them: it fires on OMISSION only, never on an + -- explicit `display = "current"`, which is the documented opt-out and + -- must reach the raw switch even when the previous run was + -- panel-placed. Resolving first and testing `== "current"` afterwards + -- would silently merge the two and break that opt-out. + local display_omitted = opts.display == nil + local display = pmacs.window._resolve_display("compile.run", opts.display, "current") -- q-target discipline (Q#CM11): capture only when coming from a -- non-generated buffer, so `g` reruns don't re-capture and -- compile → g → q restores the original buffer. @@ -896,7 +901,7 @@ local function start_run(slot, cmdline, opts) -- so it must reach the raw switch even when the previous run was -- panel-placed. The duplicate presentation that produces is the -- escape hatch's documented cost (R3-rp2). - if display == "panel" or (display == nil and already_in_panel(slot.buf)) then + if display == "panel" or (display_omitted and already_in_panel(slot.buf)) then pmacs.window.display(slot.buf, { side = "bottom", select = false }) else pmacs.window.switch_buffer(slot.buf) diff --git a/builtin/runtime/dired.lua b/builtin/runtime/dired.lua index 11d78ce..f747f60 100644 --- a/builtin/runtime/dired.lua +++ b/builtin/runtime/dired.lua @@ -642,11 +642,13 @@ local function open_directory(path, opts, departed) error(string.format("pmacs.dired.open: unknown opts key %q", tostring(key))) end end - local wanted = opts.display - if wanted ~= nil and wanted ~= "current" and wanted ~= "panel" then - error(string.format('pmacs.dired.open: unknown display %q (expected "current" or "panel")', - tostring(wanted))) - end + -- Q#S3-1/§1.1a: the shared rule, with dired's default passed + -- EXPLICITLY as "current" and kept there through Stage 3. The + -- `pmacs.path.directory_handler` slot calls this with no `display` at + -- all, so flipping dired's default would open `pmacs .` in a bottom + -- panel. Dired produces a document the user works in, not output they + -- consult; the panel default is right for the latter only. + local wanted = pmacs.window._resolve_display("pmacs.dired.open", opts.display, "current") local canonical = canonicalize(path) -- Read first: a failure must leave no buffer, no window change, and diff --git a/builtin/runtime/listview.lua b/builtin/runtime/listview.lua index 3c83119..9c904a8 100644 --- a/builtin/runtime/listview.lua +++ b/builtin/runtime/listview.lua @@ -226,12 +226,10 @@ function pmacs.listview.open(spec) -- window. In Stages 1-2 omitting `display` keeps today's raw switch; -- Stage 3 flips the default. An unknown value errors before anything -- is displayed. - local display = spec.display - if display ~= nil and display ~= "current" and display ~= "panel" then - error(string.format( - "listview.open: unknown display %q (expected \"current\" or \"panel\")", - tostring(display))) - end + -- Q#S3-1: the vocabulary, the error and the default policy are one + -- rule (`window._resolve_display`), not a copy per adopter. The + -- default is passed in because the adopters do not share one. + local display = pmacs.window._resolve_display("listview.open", spec.display, "current") if display == "panel" then pmacs.window.display(p.buffer, { side = "bottom", select = true }) else diff --git a/docs/bottom-panel-stage3-framing.md b/docs/bottom-panel-stage3-framing.md index 52fdd6c..e7ba982 100644 --- a/docs/bottom-panel-stage3-framing.md +++ b/docs/bottom-panel-stage3-framing.md @@ -196,21 +196,17 @@ fallback is invisible from the adopter's side. ### 1.6 What is NOT established -- **Nothing has been implemented or measured.** Unlike the distribution - lane, there is no artifact to inspect; the evidence is the existing - suites' behaviour before and after. -- ~~The blast radius on existing acceptance suites is unmeasured.~~ - **MEASURED — see §1.6b: 37 failures across 5 suites.** Stage 1 - shipped the mechanism opt-in precisely so existing suites kept their - meaning. Flipping the default changes where output lands for every - suite that exercises listview, compile or terminal **without** passing - `display`. Criterion 58 says the default-placement suites are - *updated*; how many others move is a measurement this framing has not - taken and the implementation must take first. -- **Interaction with dired.** `dired.lua:18` documents - `opts.display = "current" | "panel"` with **default `"current"`**. - Dired is not in Q#BP12's table. Whether the flip reaches it, or dired - keeps an explicit `"current"`, is Q#S3-2. +- **The blast radius is MEASURED — §1.6b: 37 failures across 5 suites**, + classified per test in §1.6c. Stage 1 shipped the mechanism opt-in + precisely so existing suites kept their meaning; flipping the default + changes where output lands for every suite that exercises listview, + compile or terminal **without** passing `display`. +- **Steps 1 and 2 of §7 are done** (`0224c68`, `a2f4411`). The flip + itself, the test revisions, and the capability-fallback criterion are + not. +- ~~Interaction with dired.~~ **DECIDED — §1.1a and Q#S3-2: dired keeps + `"current"`**, passed explicitly to the shared resolver so the + exemption is visible at its call site. --- @@ -333,8 +329,26 @@ got both wrong. message is ever reached. The Lua callers instead `tostring()` whatever they got and report it inside their own error. These are different observable behaviours for the same bad input, and unifying the error - text without deciding this would silently change one of them. The - stage must state which it standardizes on and pin it. + text without deciding this would silently change one of them. + + **RESOLVED and PINNED at step 2.** The custom error wins, because it + names the legal vocabulary where mlua's type error does not. + Non-strings render by **type alone** (`unknown display (integer)`), + never quoted, so the message cannot imply a string was passed. Pinned + in `bottom_panel_stage1_acceptance::acc19` at the **terminal** entry + point — the one adopter whose behaviour actually changed — asserting + the shared error *and* that nothing is created, exactly as the + unknown-string case already does. + + **The type SPELLING is deliberately not pinned.** Lua 5.4 reports + `integer`; LuaJIT has no integer subtype. Asserting either literal + would pass on one CI flavor and fail on the other, so the test pins + the shape (`unknown display (`, the vocabulary, and the *absence* of a + quoted value). Verified 46/46 under both flavors. + + Consequently step 2 is **default-preserving with one intentional + normalization**, not "behaviour-preserving": every adopter kept its + default, but invalid-input behaviour moved on purpose. - **Q#S3-2 — DECIDED: dired does not flip.** It keeps `"current"` as its default, expressed as the `default` argument to the shared resolver so the exemption is visible at the call site rather than diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 2d4f359..0dfbd2d 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -9320,12 +9320,18 @@ fn install_terminal( // Bottom-panel arc (Q#BP11b): parse placement BEFORE the // session, process, buffer, or wrapper exists, so an // unknown `display` value creates nothing to roll back. + // Stage 3 step 2: the shared resolver, still carrying the + // PRE-FLIP default. The unification must be provably + // behaviour-preserving before the default moves, so the + // flip to `AdopterDefault::Panel` is its own commit. + let display_value = spec_table.get::("display")?; let placement = window_panel::parse_adopter_placement( &core, frontend_id, "pmacs.terminal.open", - spec_table.get::>("display")?.as_deref(), + Some(&display_value), spec_table.get::>("window")?, + window_panel::AdopterDefault::Current, )?; let buffer_id = { let mut manager = manager.borrow_mut(); diff --git a/src/lua_bindings/window_panel.rs b/src/lua_bindings/window_panel.rs index 1c700a5..667cb68 100644 --- a/src/lua_bindings/window_panel.rs +++ b/src/lua_bindings/window_panel.rs @@ -251,9 +251,97 @@ pub(crate) enum AdopterPlacement { Window(WindowId), } +/// The one rule for the adopter `display` vocabulary: which values are +/// legal, what the error says, and what omission means. +/// +/// **Bottom-panel Stage 3 (Q#S3-1).** Before this, four adopters +/// validated the same three-value vocabulary in three places — Rust for +/// the terminal, and hand-written Lua copies in `listview.lua`, +/// `compile.lua` and `dired.lua`, each with its own copy of the error +/// string. Four copies of one rule is how the next adopter gets it +/// subtly wrong, and the next adopter is DAP. +/// +/// `default` is a **parameter, not a constant**, because the adopters do +/// not share one. listview / compile / terminal resolve omission to the +/// panel; **dired resolves it to `"current"`** and must keep doing so — +/// `pmacs.path.directory_handler` calls it with no `display` at all, so +/// a flipped default would open `pmacs .` in a bottom panel (§1.1a). +/// Passing the default in is what makes dired's exemption visible at its +/// call site instead of hidden in a divergent copy. +/// +/// **Non-string values are reported as unknown, not as type errors**, +/// and this is a deliberate normalization (Q#S3-1). Terminal previously +/// read `get::>` and so raised mlua's type error *before* +/// reaching any custom message, while the Lua copies stringified and +/// reported their own. Nothing pinned either behaviour. The custom error +/// wins because it names the legal vocabulary and mlua's does not. +/// +/// # Errors +/// Any non-nil value that is not `"current"` or `"panel"`. +pub(crate) fn resolve_adopter_display( + operation: &str, + raw: Option<&mlua::Value>, + default: AdopterDefault, +) -> mlua::Result { + let raw = match raw { + None | Some(mlua::Value::Nil) => return Ok(default.placement()), + Some(value) => value, + }; + match raw.as_str().as_deref() { + Some("current") => Ok(AdopterPlacement::Current), + Some("panel") => Ok(AdopterPlacement::Panel), + _ => Err(mlua::Error::runtime(format!( + "{operation}: unknown display {} (expected \"current\" or \"panel\")", + display_for_error(raw) + ))), + } +} + +/// What omission means for one adopter. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub(crate) enum AdopterDefault { + /// listview, compile, terminal — Stage 3's flip. + Panel, + /// dired, and anything else whose output is a document the user + /// works in rather than output they consult. + Current, +} + +impl AdopterDefault { + fn placement(self) -> AdopterPlacement { + match self { + Self::Panel => AdopterPlacement::Panel, + Self::Current => AdopterPlacement::Current, + } + } +} + +/// Render a rejected `display` value for the error message. +/// +/// Strings are quoted so `display = "sideways"` reads as `"sideways"`; +/// anything else is shown **by type alone** — `unknown display +/// (number)` — because quoting a non-string as `"42"` would imply the +/// caller passed a string when they passed a number. +/// +/// The value itself is deliberately not interpolated: it is already +/// wrong, `mlua::Value`'s `Display` is not guaranteed useful for tables +/// or userdata, and the type is what tells the caller what to fix. +fn display_for_error(value: &mlua::Value) -> String { + value.as_str().map_or_else( + || format!("({})", value.type_name()), + |s| format!("{:?}", &*s), + ) +} + /// Parse an adopter's placement **before** it creates a buffer, session, /// process, or wrapper — so an unknown value leaves nothing to roll back. /// +/// Terminal-specific wrapper around [`resolve_adopter_display`]: only +/// the terminal accepts a `window` id, and only it must reject `window` +/// combined with `display = "panel"`. That asymmetry stays here rather +/// than in the shared resolver, because a helper that pretended the four +/// parsers were identical would be its own defect. +/// /// # Errors /// An unknown `display` value, a `window` combined with /// `display = "panel"`, or a window id that is not live in the acting @@ -262,18 +350,11 @@ pub(crate) fn parse_adopter_placement( core: &SharedCore, fid: FrontendId, operation: &str, - display: Option<&str>, + display: Option<&mlua::Value>, window: Option, + default: AdopterDefault, ) -> mlua::Result { - let display = match display { - None | Some("current") => AdopterPlacement::Current, - Some("panel") => AdopterPlacement::Panel, - Some(other) => { - return Err(mlua::Error::runtime(format!( - "{operation}: unknown display {other:?} (expected \"current\" or \"panel\")" - ))); - } - }; + let display = resolve_adopter_display(operation, display, default)?; match (window, &display) { (Some(_), AdopterPlacement::Panel) => Err(mlua::Error::runtime(format!( "{operation}: `window` and `display = \"panel\"` are mutually exclusive" @@ -481,6 +562,43 @@ pub(crate) fn install(lua: &Lua, core: &SharedCore, win: &Table) -> mlua::Result )?; } + { + // Q#S3-1 — the shared adopter-display rule, reachable from Lua. + // + // Underscore-prefixed because it is an internal seam between the + // builtin runtime modules and this one, not user-facing API: + // `listview.lua`, `compile.lua` and `dired.lua` call it instead + // of each keeping a hand-written copy of the same three-value + // check and error string. + // + // Returns the resolved `"current"` / `"panel"` rather than an + // opaque handle, so the Lua callers keep their existing + // `if display == "panel"` dispatch and this change stays a + // validation unification rather than a control-flow rewrite. + win.set( + "_resolve_display", + lua.create_function( + |_, + (operation, raw, default): (String, mlua::Value, String)| + -> mlua::Result<&'static str> { + let default = match default.as_str() { + "panel" => AdopterDefault::Panel, + "current" => AdopterDefault::Current, + other => { + return Err(mlua::Error::runtime(format!( + "window._resolve_display: bad default {other:?}" + ))); + } + }; + match resolve_adopter_display(&operation, Some(&raw), default)? { + AdopterPlacement::Panel => Ok("panel"), + AdopterPlacement::Current | AdopterPlacement::Window(_) => Ok("current"), + } + }, + )?, + )?; + } + { let cc = core.clone(); win.set( diff --git a/tests/bottom_panel_stage1_acceptance.rs b/tests/bottom_panel_stage1_acceptance.rs index 400754a..fb05e5e 100644 --- a/tests/bottom_panel_stage1_acceptance.rs +++ b/tests/bottom_panel_stage1_acceptance.rs @@ -1291,6 +1291,53 @@ fn acc19_adopters_place_side_affinely_through_real_entry_points() { "unknown display fails before session/process/buffer creation" ); assert_eq!(s.core.borrow().registry.borrow().ids().len(), before); + + // Bottom-panel Stage 3, Q#S3-1 — the NON-STRING normalization. + // + // Pinned because step 2 CHANGED this deliberately and nothing else + // covers it. Before the shared resolver, terminal read + // `get::>("display")?`, so a number raised mlua's + // TYPE error before any custom message existed; the Lua adopters + // stringified instead and reported their own. Unifying the error + // text without pinning this would have let the two drift back apart + // unnoticed, and the surrounding assertions could not have caught it + // — they all pass unknown STRINGS, which take the same path in both + // designs. + // + // The custom error wins because it names the legal vocabulary. The + // type is reported WITHOUT the value, so the message cannot imply a + // string was passed. + let before = s.core.borrow().registry.borrow().ids().len(); + let err = try_exec( + &s, + "pmacs.terminal.open { command = \"/bin/sh\", display = 42 }", + ) + .expect_err("a non-string display is rejected"); + // The TYPE SPELLING is deliberately not pinned: Lua 5.4 reports + // `integer` where LuaJIT has no integer subtype, so asserting either + // literal would pass on one CI flavor and fail on the other. What is + // pinned is the shape — our operation name, a parenthesised type + // rather than a quoted value, and the vocabulary. + assert!( + err.contains("pmacs.terminal.open: unknown display ("), + "a non-string display takes the shared unknown-display error naming the \ + operation and a type, not mlua's type error; got: {err}" + ); + assert!( + err.contains("expected \"current\" or \"panel\""), + "the error still names the legal values; got: {err}" + ); + assert!( + !err.contains("\"42\""), + "the rejected value is reported by TYPE, not quoted as though it were a \ + string; got: {err}" + ); + assert_eq!( + s.core.borrow().registry.borrow().ids().len(), + before, + "…and still creates nothing, exactly as an unknown string does" + ); + exec( &s, "TERM_BUF = pmacs.terminal.open { command = \"/bin/sh\", display = \"panel\" }",