From a6eccf126b31f21b311cb0d4d397354dcd493f0b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 20:59:08 +0000 Subject: [PATCH] ogar-loco: close the shared-core arity gap (50 -> 90 covered) The core NAMED 95 operations and gave stack arities for 50. The other 45 resolved to a legal FnIndex and then could not say what they consumed -- so a body containing one could not be validated or segmented, and the failure surfaced only in a consumer, far from the cause. The doc already called these "real palette entries deliberately not yet covered" with the standing invitation that "coverage grows in the core, once, for everyone". This is that sweep: 40 newly covered, 5 deliberately refused. ## Where the numbers come from Not judgement. Each arity is corroborated by the frontends' own declarations, harvested from source: Blockly's block definitions (AdaWorldAPI/blockly `blocks/*.ts`) and Apache-2.0 scratch-blocks (`src/blocks/*.ts`, the `args0` input_value / input_statement counts). Where both frontends have the block they agree; where only one does, that one decides. `control_wait` declares one input, `control_stop` none, `logic_ternary` and `math_constrain` three, `math_atan2` and `operator_random` two, `math_random_float` none. Two conventions were READ from the existing table rather than invented: - `VAR_GET => 0` ("leaves: they push, they do not consume") establishes that an identifier rides as an immediate value byte, never a stack operand. So VAR_SET / VAR_CHANGE / APPEND evaluate only the incoming value -- exactly what both frontends declare, the variable being a field in each. - The two-quantity split governs PROC_DEF: it evaluates nothing (arity 0) and REFERENCES its body (body_refs 1), which is why it also lands in the body_refs table. Scratch's `procedures_definition` independently declares zero value inputs and one statement input. ## The list family, and why one table serves two frontends Blockly passes a list EXPRESSION (`lists_length` takes a LIST input); Scratch names a global list in a FIELD (`data_lengthoflist` declares zero inputs). On a stack machine these converge: Scratch's field lowers to an explicit push of the list reference -- the shape VAR_GET already has -- and the op pops it. So the list is counted as an ordinary operand, and no second opcode family is needed for the second frontend. ## What stays refused, and why that is not an oversight Five entries: RETURN, NUMBER_PROPERTY, PROMPT, LIST_WITH, PROC_CALL. Every one is genuinely VARIADIC -- a void return evaluates nothing and a value return evaluates one; `divisible by` takes a divisor the other number properties do not; `text_prompt` carries its message as a field where `text_prompt_ext` makes it an operand; a list literal and a procedure call hold as many items/arguments as their mutator declares. These want an arity that is a property of the CALL rather than the function, which is a design question and not a table row. Guessing would be worse than refusing: a wrong arity desynchronizes the stack and reattributes every later operand. ## The enumeration that was missing `every_named_function_has_an_arity_except_the_documented_variadics` walks the whole byte range instead of a hand-listed set, so a newly minted constant with no arity row fails HERE immediately rather than years later in a consumer -- which is exactly how this gap was found. Verified as a real falsifier by deleting one arity row: it fails naming the exact opcode. Three existing tests pinned WAIT as deliberately-uncovered and failed as designed; each is re-pinned onto the still-variadic set and gained a can-fire half, so "uncovered" keeps discriminating instead of labelling whatever happens to be missing. cargo test --workspace: 78 test targets, 0 failures. clippy clean. --- crates/ogar-loco/src/statements.rs | 9 +- crates/ogar-loco/src/vocabulary.rs | 211 +++++++++++++++++++++++++++-- 2 files changed, 202 insertions(+), 18 deletions(-) diff --git a/crates/ogar-loco/src/statements.rs b/crates/ogar-loco/src/statements.rs index 8d5322d1..718866cb 100644 --- a/crates/ogar-loco/src/statements.rs +++ b/crates/ogar-loco/src/statements.rs @@ -285,12 +285,15 @@ mod tests { ), Err(StatementError::DanglingOperands { depth: 2 }) ); - // Uncovered: WAIT has no shared-core tables — refuse, never guess. + // Uncovered: RETURN has no shared-core arity — refuse, never guess. + // (Re-pinned from WAIT when the coverage sweep landed: WAIT is now + // covered, RETURN stays refused because it is genuinely variadic — + // void vs value. See `shared_core::stack_arity`'s doc table.) assert_eq!( - statement_bounds(&v, &body(&[Call::with_value(FnIndex::WAIT, 3)])), + statement_bounds(&v, &body(&[Call::with_value(FnIndex::RETURN, 3)])), Err(StatementError::Uncovered { index: 0, - f: FnIndex::WAIT + f: FnIndex::RETURN }) ); } diff --git a/crates/ogar-loco/src/vocabulary.rs b/crates/ogar-loco/src/vocabulary.rs index c3d863f4..68dc24b1 100644 --- a/crates/ogar-loco/src/vocabulary.rs +++ b/crates/ogar-loco/src/vocabulary.rs @@ -13,9 +13,10 @@ //! - Bytes **at/above the floor** belong to the vocabulary. A [`Vocabulary`] //! implementation answers for exactly that range via its `domain_*` hooks; //! the composed methods route each byte to the right table. -//! - A shared-core byte the core does not cover (e.g. `WAIT` today) is -//! **refused everywhere** — a vocabulary does not get to guess for it. -//! Coverage grows in the core, once, for everyone. +//! - A shared-core byte the core does not cover (today: the five genuinely +//! variadic entries listed on [`shared_core::stack_arity`]) is **refused +//! everywhere** — a vocabulary does not get to guess for it. Coverage +//! grows in the core, once, for everyone. //! //! [`conformance::check`] is the mechanical enforcement: run it in every //! vocabulary crate's tests. It catches a composed-method override that @@ -51,8 +52,26 @@ pub mod shared_core { /// `None` means the core does not cover this function — refused rather /// than guessed, because a wrong arity does not produce a slightly-wrong /// result: it desynchronizes the stack and reattributes every later - /// operand. (`WAIT`, `STOP`, `RETURN`, `TERNARY` and others are real - /// palette entries deliberately not yet covered.) + /// operand. + /// + /// # What is still uncovered, and why it is not an oversight + /// + /// Exactly five entries answer `None`, and every one is **genuinely + /// variadic** — it has no single correct arity to record, so covering it + /// would mean choosing a number that is wrong for some legitimate call: + /// + /// | function | why it has no fixed arity | + /// |---|---| + /// | `RETURN` | a void return evaluates nothing; a value return evaluates one | + /// | `NUMBER_PROPERTY` | `even`/`odd`/`prime`/… take the number alone, `divisible by` takes a divisor too | + /// | `PROMPT` | `text_prompt` carries its message as a field, `text_prompt_ext` as an operand | + /// | `LIST_WITH` | a list literal holds as many items as its mutator says | + /// | `PROC_CALL` | a call passes as many arguments as its procedure declares | + /// + /// These want an arity that is a property of the CALL rather than of the + /// function — a per-call count carried in the call's own bytes. That is a + /// deliberate design question, not a table row, so they stay refused + /// until it is answered. Everything else the palette names is covered. /// /// For control flow this counts **only** the evaluated operands. A loop /// body is not among them; see [`body_refs`]. @@ -115,6 +134,84 @@ pub mod shared_core { | FnIndex::AND | FnIndex::OR | FnIndex::JOIN => 2, + + // ── control: the suspend/stop family. + // A duration or a condition, evaluated first; `STOP`'s target + // (this script / all / others) is a selector immediate, not an + // operand — both frontends agree (`control_wait` declares one + // input, `control_stop` declares none). + FnIndex::WAIT | FnIndex::WAIT_UNTIL => 1, + FnIndex::STOP => 0, + + // ── expressions with a fixed shape. + // Corroborated by BOTH frontends where both have the block, and + // by the surviving one where only one does: `logic_ternary` and + // `math_constrain` declare three inputs, `math_atan2` and + // `math_random_int`/`operator_random` two, `math_random_float` + // none, `text_isEmpty` one. + FnIndex::TERNARY | FnIndex::CONSTRAIN => 3, + FnIndex::ATAN2 | FnIndex::RANDOM_INT => 2, + FnIndex::RANDOM_FLOAT => 0, + + // ── text. + // The selector-style ops take the subject only; the dropdown + // (which case, which end to trim) is a value byte, never an + // operand — the same split `math_single[ROOT]` already uses. + FnIndex::IS_EMPTY + | FnIndex::CHANGE_CASE + | FnIndex::TRIM + | FnIndex::REVERSE + | FnIndex::PRINT => 1, + // `APPEND` follows the `VAR_SET` shape below: the variable is an + // immediate, so only the appended value is evaluated + // (`text_append` declares one input). + FnIndex::APPEND => 1, + FnIndex::CHAR_AT | FnIndex::INDEX_OF | FnIndex::CONTAINS | FnIndex::COUNT => 2, + FnIndex::SUBSTRING | FnIndex::REPLACE => 3, + + // ── variables. + // `VAR_GET` is a leaf (above): the variable id rides as an + // immediate value byte and nothing is popped. Its writing twins + // therefore evaluate only the incoming value — which is exactly + // what both frontends declare (`data_setvariableto` and + // `variables_set` one input each, the variable itself a field). + FnIndex::VAR_SET | FnIndex::VAR_CHANGE => 1, + + // ── lists. + // + // The list is an ORDINARY OPERAND here, not an immediate, and + // that is what lets one table serve two frontends that model + // lists differently. Blockly passes a list *expression* + // (`lists_length` takes a LIST input); Scratch names a global + // list in a *field* (`data_lengthoflist` declares zero inputs). + // On a stack machine the two converge: Scratch's field lowers to + // an explicit push of the list reference — the same shape + // `VAR_GET` already has — and the op then pops it. So the arity + // below counts the list, and a Scratch lowering supplies it + // rather than needing a second opcode family. + FnIndex::LIST_EMPTY => 0, + FnIndex::LIST_LENGTH + | FnIndex::LIST_IS_EMPTY + | FnIndex::LIST_DELETE_ALL + | FnIndex::LIST_SORT + | FnIndex::ON_LIST => 1, + FnIndex::LIST_REPEAT + | FnIndex::LIST_GET + | FnIndex::LIST_ADD + | FnIndex::LIST_DELETE + | FnIndex::LIST_INDEX_OF + | FnIndex::LIST_CONTAINS + | FnIndex::LIST_SPLIT => 2, + FnIndex::LIST_SET | FnIndex::LIST_INSERT | FnIndex::LIST_SUBLIST => 3, + + // ── procedures. + // `PROC_DEF` evaluates nothing — its body is a REFERENCE, counted + // by `body_refs`, which is the two-quantity split this module + // exists to keep apart. `PROC_ARG` is a leaf like `VAR_GET`. + FnIndex::PROC_DEF | FnIndex::PROC_ARG => 0, + + // Deliberately still uncovered — see the module note on the five + // variadic entries. Refused rather than guessed. _ => return None, }) } @@ -134,7 +231,13 @@ pub mod shared_core { | FnIndex::REPEAT_UNTIL | FnIndex::FOREVER | FnIndex::FOR_EACH - | FnIndex::FOR_RANGE => 1, + | FnIndex::FOR_RANGE + // A function definition references the body it defines. It + // evaluates nothing (`stack_arity` 0), which is the two-quantity + // split in its purest form — and both frontends declare exactly + // this shape: `procedures_definition` has one statement input and + // zero value inputs. + | FnIndex::PROC_DEF => 1, FnIndex::IF_ELSE => 2, _ => 0, } @@ -944,17 +1047,29 @@ mod tests { ); assert!(shared_core::branches(f), "{f:?} should reference a body"); } - // Silence twin: uncovered control flow stays uncovered rather than - // being quietly assigned a plausible shape. WAIT/STOP/RETURN are real - // palette entries the core does not yet model. + // Silence twin: what the core does NOT model stays refused rather + // than being quietly assigned a plausible shape. + // + // Re-pinned when the coverage sweep landed: WAIT / WAIT_UNTIL / STOP + // are now covered (both frontends declare their shape, so there was + // nothing left to guess). What remains refused is the genuinely + // VARIADIC set — an arity that belongs to the call, not the function + // — which is a design question and not a table row. See + // `shared_core::stack_arity`'s doc table. for f in [ - FnIndex::WAIT, - FnIndex::WAIT_UNTIL, - FnIndex::STOP, FnIndex::RETURN, + FnIndex::NUMBER_PROPERTY, + FnIndex::PROMPT, + FnIndex::LIST_WITH, + FnIndex::PROC_CALL, ] { assert_eq!(shared_core::stack_arity(f), None, "{f:?} must stay refused"); } + // …and the newly covered half, so this test cannot pass by refusing + // everything: a silence twin with no can-fire half proves nothing. + assert_eq!(shared_core::stack_arity(FnIndex::WAIT), Some(1)); + assert_eq!(shared_core::stack_arity(FnIndex::STOP), Some(0)); + assert_eq!(shared_core::stack_arity(FnIndex::LIST_SET), Some(3)); } #[test] @@ -1081,7 +1196,7 @@ mod tests { // uncovered stays None. assert_eq!(checked.pushes_result(FnIndex::ADD), Some(true)); assert_eq!(checked.pushes_result(FnIndex::REPEAT), Some(false)); - assert_eq!(checked.pushes_result(FnIndex::WAIT), None); + assert_eq!(checked.pushes_result(FnIndex::RETURN), None); // Domain half sampled from the hooks; the undeclared pushes column // defaults to None (refused by segmentation, never guessed). assert_eq!(checked.table().stack_arity(FnIndex(0x90)), Some(1)); @@ -1156,8 +1271,8 @@ mod tests { // W-2: names are not coverage — refused-but-real entries are named // so a legend can say "exists, refused" rather than omitting them. assert_eq!(shared_core::name(FnIndex::ADD), Some("ADD")); - assert_eq!(shared_core::name(FnIndex::WAIT), Some("WAIT")); - assert_eq!(shared_core::stack_arity(FnIndex::WAIT), None); + assert_eq!(shared_core::name(FnIndex::RETURN), Some("RETURN")); + assert_eq!(shared_core::stack_arity(FnIndex::RETURN), None); assert_eq!(shared_core::name(FnIndex::NOP), None, "NOP is not an op"); assert_eq!(shared_core::name(FnIndex(0x1F)), None, "reserved = unnamed"); @@ -1241,3 +1356,69 @@ mod tests { ); } } + +#[cfg(test)] +mod coverage { + use super::*; + + /// Every function the palette NAMES has an arity, except the five + /// documented variadic entries. + /// + /// The core used to name 96 operations and give arities for 50 — so a + /// consumer could resolve an opcode, get a legal `FnIndex`, and then find + /// the substrate unable to say what it consumed. That gap was invisible + /// from inside `ogar-loco` (nothing here enumerated it) and surfaced only + /// when a consumer mapped a second frontend onto the same core. + /// + /// This test is the enumeration that was missing. It walks the whole byte + /// range rather than a hand-listed set, so a NEWLY MINTED constant with no + /// arity row fails here immediately instead of years later in a consumer. + #[test] + fn every_named_function_has_an_arity_except_the_documented_variadics() { + const VARIADIC: [FnIndex; 5] = [ + FnIndex::RETURN, + FnIndex::NUMBER_PROPERTY, + FnIndex::PROMPT, + FnIndex::LIST_WITH, + FnIndex::PROC_CALL, + ]; + + let mut named = 0usize; + let mut missing = Vec::new(); + for b in 0..DOMAIN_FLOOR { + let f = FnIndex(b); + if shared_core::name(f).is_none() { + // Not a minted operation — nothing is owed for it. But it + // must not answer either, or the range would be lying. + assert_eq!( + shared_core::stack_arity(f), + None, + "{f:?} is unnamed yet reports an arity" + ); + continue; + } + named += 1; + if shared_core::stack_arity(f).is_none() && !VARIADIC.contains(&f) { + missing.push((b, shared_core::name(f))); + } + } + assert!( + missing.is_empty(), + "named operations with no arity and no documented reason: {missing:?}" + ); + + // Can-stay-silent: the variadics really are still refused, so the + // assertion above cannot be satisfied by covering everything. + for f in VARIADIC { + assert_eq!(shared_core::stack_arity(f), None, "{f:?} must stay refused"); + assert!( + shared_core::name(f).is_some(), + "{f:?} must still be NAMED — refused is not the same as absent" + ); + } + + // And the census itself, pinned: 95 named operations (96 constants + // minus NOP, which is not an operation), 5 refused, 90 covered. + assert_eq!(named, 95, "the named census moved — re-pin deliberately"); + } +}