diff --git a/README.md b/README.md index 021b2a08..d6e93386 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,10 @@ Early inspiration and file format guidance came from the [warcraft-rs](https://github.com/wowemulation-dev/warcraft-rs) in particular. benilla is an independent fan project, not affiliated with or endorsed by Blizzard Entertainment. -It contains no Blizzard code or assets, and you provide your own legally obtained 1.12.1 client -for game data. World of Warcraft is a trademark of Blizzard Entertainment, Inc. Licensed under -[MIT](LICENSE-MIT) or [Apache 2.0](LICENSE-APACHE), at your option. +It ships **no Blizzard game data** — no art, models, sounds, maps or MPQ contents — and you provide +your own legally obtained 1.12.1 client for all of it. Parts of the interface layer under +`crates/benilla-app/assets/ui/` are **transcribed from the client's own FrameXML** (its Lua and XML), +cited in place with the reference file and line range; that is Blizzard's authorship, not ours, and +it is not covered by the licence below. World of Warcraft is a trademark of Blizzard Entertainment, +Inc. Our own code is licensed under [MIT](LICENSE-MIT) or [Apache 2.0](LICENSE-APACHE), at your +option. diff --git a/crates/benilla-app/assets/ui/GroupLootFrame.xml b/crates/benilla-app/assets/ui/GroupLootFrame.xml index 445f922e..006bdcea 100644 --- a/crates/benilla-app/assets/ui/GroupLootFrame.xml +++ b/crates/benilla-app/assets/ui/GroupLootFrame.xml @@ -309,7 +309,17 @@ inside ref l.292's `d` (a typo in Blizzard's own shipped file — not transcribed). --> - + + @@ -476,26 +486,26 @@ - + - + - + - + diff --git a/crates/benilla-app/examples/addon_harness.rs b/crates/benilla-app/examples/addon_harness.rs index 25a77be1..0333ffaf 100644 --- a/crates/benilla-app/examples/addon_harness.rs +++ b/crates/benilla-app/examples/addon_harness.rs @@ -68,15 +68,180 @@ fn render_frames(frames: &[String]) -> String { format!("{},+{} more", frames[..cap].join(","), frames.len() - cap) } +/// Our `_G` against the captured 1.12 `_G` — decision 1189's diff, re-runnable. +/// +/// 1189 did this once, by hand, and its finding was that **a superset is not free**: 1.12 addons +/// branch on presence (`if SomeName then`), so a name we publish that the reference does not have +/// can route an addon down a path the real client never takes. That makes BOTH directions of this +/// diff interesting, and the extra-names side the one nothing else in this harness can see — every +/// other ranking here is demand-driven, so it can only ever report what an addon *asked* for. +/// +/// The reference side is `reference/1.12-globals.tsv`, itself generated from wow-re's live capture. +/// `lod` rows are the twelve LoadOnDemand `Blizzard_*` addons' names, unioned in by 1200 because a +/// live dump misses them unless the player opened those windows; they are counted separately rather +/// than silently folded in, since "absent from us" means something different for a window nobody +/// opened. +fn surface_report(deep: bool, dump_path: Option) { + let tsv = concat!( + env!("CARGO_MANIFEST_DIR"), + "/../../reference/1.12-globals.tsv" + ); + let Ok(text) = std::fs::read_to_string(tsv) else { + eprintln!("cannot read the reference surface at {tsv}"); + std::process::exit(1); + }; + + let mut reference: std::collections::BTreeMap = Default::default(); + for line in text.lines().filter(|l| !l.starts_with('#')) { + let mut f = line.split('\t'); + if let (Some(name), Some(kind)) = (f.next(), f.next()) { + if !name.is_empty() { + reference.insert(name.to_string(), kind.to_string()); + } + } + } + + let ours: std::collections::BTreeMap = + addon_harness::surface().into_iter().collect(); + + // `--surface-dump ` writes our raw `_G` as `nametype`, the same shape the reference + // TSV has, so the two can be joined by anything. The printed report answers the questions I + // thought to ask; a sweep two sessions from now will have different ones, and re-deriving our + // side by scraping a human-readable report is exactly how a measurement quietly goes wrong. + if let Some(path) = dump_path { + let body: String = ours + .iter() + .map(|(n, k)| format!("{n}\t{k}\n")) + .collect::>() + .concat(); + match std::fs::write(&path, format!("# our _G — {} names\n{body}", ours.len())) { + Ok(()) => println!(" wrote {} names to {path}", ours.len()), + Err(e) => eprintln!(" could not write {path}: {e}"), + } + } + + println!("\n SURFACE DIFF — our _G vs the captured 1.12 _G (decision 1189)"); + println!(" FrameXML digest : {}", addon_harness::framexml_digest()); + println!(" reference names : {}", reference.len()); + println!(" ours : {}", ours.len()); + + // Absent from us, by the reference's own type — a function we lack is a different problem from + // a string we lack, and `lod` is a third thing again. + let mut missing_by_kind: std::collections::BTreeMap<&str, Vec<&str>> = Default::default(); + for (name, kind) in &reference { + if !ours.contains_key(name) { + missing_by_kind + .entry(kind.as_str()) + .or_default() + .push(name.as_str()); + } + } + println!("\n ABSENT FROM US, by the reference's type:"); + for (kind, names) in &missing_by_kind { + println!(" {:<10} {}", kind, names.len()); + } + + // The direction 1189 cared about and nothing else here can see. + // + // **Split by OUR type, because the total on its own misleads.** Most of these are `table` — the + // frame and region names our own FrameXML publishes, which differ from Blizzard's simply + // because our XML is our own reimplementation and names its pieces itself. That is not the + // hazard 1189 described. The hazard is a **function**: `if SomeApiName then` is how a 1.12 + // addon feature-tests, and a verb we publish that the client never had can route it down a path + // the real client never takes. So the function row is the one to read first, and the one a + // future landing should be able to drive to zero. + let extra: Vec<(&str, &str)> = ours + .iter() + .filter(|(n, _)| !reference.contains_key(n.as_str())) + .map(|(n, k)| (n.as_str(), k.as_str())) + .collect(); + let mut extra_by_kind: std::collections::BTreeMap<&str, Vec<&str>> = Default::default(); + for (name, kind) in &extra { + extra_by_kind.entry(kind).or_default().push(name); + } + println!( + "\n PUBLISHED BY US, ABSENT FROM 1.12 ({}) — a superset is not free (1189):", + extra.len() + ); + for (kind, names) in &extra_by_kind { + let note = match *kind { + "function" => " <- the ones an addon feature-tests; read these first", + "table" => " <- mostly our own FrameXML's frame/region names", + _ => "", + }; + println!(" {:<10} {}{}", kind, names.len(), note); + } + // Functions in full even without --deep: it is the row that matters and it has to be readable. + // + // Split again on the `Benilla` prefix, which is the difference between a name that CAN collide + // with an addon's expectations and one that cannot. Nothing in the 1.12 corpus feature-tests + // `BenillaBagSlot_OnClick`; our own namespace is safe by construction, and leaving 443 of those + // in the list would bury the ones that are not. + if let Some(fns) = extra_by_kind.get("function") { + let (ours_ns, unprefixed): (Vec<&&str>, Vec<&&str>) = fns + .iter() + .partition(|n| n.starts_with("Benilla") || n.starts_with("BENILLA")); + println!( + "\n ...of those functions: {} are Benilla*-namespaced (cannot collide), {} are NOT:", + ours_ns.len(), + unprefixed.len() + ); + for chunk in unprefixed.chunks(4) { + let row: Vec<&str> = chunk.iter().map(|s| **s).collect(); + println!(" {}", row.join(" ")); + } + println!( + " ^ read these: an unprefixed verb 1.12 lacks is what `if SomeName then` finds.\n \ + Most are our own UI's helpers (AddonList_*, KeyBindings_*, Options*), which are only\n \ + a naming question — but a POST-1.12 API name here is 1189's hazard exactly, because an\n \ + addon that feature-tests it takes a branch written for a client we are not." + ); + } + if deep { + for (kind, names) in &extra_by_kind { + if *kind == "function" { + continue; + } + println!("\n ...EXTRA {} ({}):", kind, names.len()); + for chunk in names.chunks(4) { + println!(" {}", chunk.join(" ")); + } + } + } + + if deep { + for (kind, names) in &missing_by_kind { + println!("\n ABSENT FROM US — {} ({}):", kind, names.len()); + for chunk in names.chunks(4) { + println!(" {}", chunk.join(" ")); + } + } + } +} + fn main() { let mut args = std::env::args().skip(1); let Some(root) = args.next() else { eprintln!( - "usage: addon_harness [--verbose] [--why ]" + "usage: addon_harness [--verbose] [--why ]\n \ + or: addon_harness --surface (our _G vs the captured 1.12 surface; takes no corpus)" ); std::process::exit(2); }; let rest: Vec = args.collect(); + + // `--surface` — decision 1189's comparison, made re-runnable. It needs no corpus, so it is + // handled before the root is used for anything. + if root == "--surface" || rest.iter().any(|a| a == "--surface") { + let dump_path = rest + .iter() + .position(|a| a == "--surface-dump") + .and_then(|i| rest.get(i + 1)) + .cloned(); + surface_report(rest.iter().any(|a| a == "--deep"), dump_path); + return; + } + let verbose = rest.iter().any(|a| a == "--verbose"); // `--why ` — the addons behind one row, with their verbatim errors. The // ranked table collapses quoted names by design (1193); this is the read-back, and two of this diff --git a/crates/benilla-app/src/addon_harness/mod.rs b/crates/benilla-app/src/addon_harness/mod.rs index 193f11f7..5b95bc8d 100644 --- a/crates/benilla-app/src/addon_harness/mod.rs +++ b/crates/benilla-app/src/addon_harness/mod.rs @@ -528,8 +528,6 @@ fn survey_one( .union(&wants.tested_methods) .cloned() .collect(); - eprintln!("INSTRMEASURE\t{name}\t{}", script.instructions_used()); - eprintln!("INSTRM {name} {}", script.instructions_used()); let oracle = widget_method_kinds(&script, &asked); let missing_methods = unresolved_from(&oracle, &wants.wanted_methods); let optional_methods = unresolved_from(&oracle, &wants.tested_methods); @@ -1081,22 +1079,48 @@ fn missing_templates(script: &UiScript, root: &Path, name: &str, toc: &Toc) -> V let Some(close) = rest[open..].find(')') else { continue; }; - let Some(fourth) = rest[open + 1..open + close].split(',').nth(3) else { + // Split the ARGUMENT LIST at depth 0 only. A plain `split(',')` cuts inside nested + // calls and strings, and it produced a phantom: AceGUI's + // + // CreateFrame("ScrollFrame", format("%s@%s@%s", Type, "ScrollFrame", …), backdrop, …) + // + // has its 4th comma-separated chunk INSIDE `format(...)`, so the census reported a + // missing template literally named `ScrollFrame` — which is a frame KIND, not a + // template, and named nothing that was ever missing. One of four rows in that table was + // fiction (1210/1218/1227, the fourth time a ranking here has opened with one). + let args = &rest[open + 1..open + close]; + let mut depth = 0i32; + let mut quote: Option = None; + let mut fields: Vec<&str> = Vec::new(); + let mut start = 0usize; + for (bi, c) in args.char_indices() { + match (quote, c) { + (Some(q), c) if c == q => quote = None, + (Some(_), _) => {} + (None, '"' | '\'') => quote = Some(c), + (None, '(' | '[' | '{') => depth += 1, + (None, ')' | ']' | '}') => depth -= 1, + (None, ',') if depth == 0 => { + fields.push(&args[start..bi]); + start = bi + 1; + } + _ => {} + } + } + fields.push(&args[start..]); + let Some(fourth) = fields.get(3) else { continue; }; let f = fourth.trim(); - // A quoted literal, either quote style. `inherits` is a comma-separated LIST in - // FrameXML and `CreateFrame` takes the same shape, so split it. + // A quoted literal, either quote style, taken WHOLE. This used to split the literal on + // commas, on the belief that the fourth argument is a list; it is not — 1.12 looks up + // the string it was handed, once. let lit = f .strip_prefix('"') .and_then(|s| s.strip_suffix('"')) .or_else(|| f.strip_prefix('\'').and_then(|s| s.strip_suffix('\''))); - if let Some(lit) = lit { - wanted.extend( - lit.split(',') - .map(|s| s.trim().to_string()) - .filter(|s| !s.is_empty()), - ); + if let Some(lit) = lit.filter(|s| !s.is_empty()) { + wanted.insert(lit.to_string()); } } } @@ -1137,10 +1161,13 @@ fn missing_inherits(script: &UiScript, root: &Path, name: &str, toc: &Toc) -> Ve let Some(end) = rest[1..].find(quote) else { continue; }; + // ONE name, verbatim — the attribute value is what the loader looks up, commas and + // spaces included. Splitting here made the census ask about names the loader never + // asks for: a comma list would be reported as two missing templates when the real + // lookup misses once, on the whole string. wanted.extend( - rest[1..1 + end] - .split(',') - .map(|s| s.trim().to_string()) + [rest[1..1 + end].to_string()] + .into_iter() .filter(|s| !s.is_empty()), ); } @@ -1186,6 +1213,52 @@ pub fn framexml_digest() -> String { crate::ui_script::framexml_digest() } +/// Every name our VM publishes into `_G`, with its Lua type — **no addons loaded**. +/// +/// The other half of decision 1189's diff. 1189 established that the authoritative 1.12 surface is +/// already captured (the running client's in-world `_G`, 19,572 entries, vendored here as +/// `reference/1.12-globals.tsv`) and compared our table against it *once*, by hand. Nothing has +/// re-run it since, so the comparison has been drifting ever since — which is the failure mode this +/// project keeps writing records about: a measurement taken once and then cited as though it were +/// current. +/// +/// Deliberately addon-free. The question is what OUR interface publishes, and an addon's own globals +/// would inflate both directions of the diff with names that belong to nobody but that addon. +/// +/// The dump runs in Lua rather than reaching into the registry from Rust, because `_G` is the thing +/// an addon actually sees — the same reasoning that made the reference side a live capture instead +/// of the binary's registration table. +pub fn surface() -> Vec<(String, String)> { + let Ok(mut script) = UiScript::new() else { + return Vec::new(); + }; + script.set_instruction_budget(ADDON_INSTRUCTION_BUDGET); + script.set_screen_size(1024.0, 768.0); + script.register_addons(Vec::new(), None, None, None); + seat_a_session(&mut script); + let _ = crate::ui_script::load_default_ui(&script); + + let dump: String = script + .eval( + r#" + local out = {} + for k, v in pairs(_G) do + if type(k) == "string" then + out[table.getn(out) + 1] = k .. "\t" .. type(v) + end + end + table.sort(out) + return table.concat(out, "\n") + "#, + ) + .unwrap_or_default(); + + dump.lines() + .filter_map(|l| l.split_once('\t')) + .map(|(n, t)| (n.to_string(), t.to_string())) + .collect() +} + /// The real `GlobalStrings.lua`, read once off the install's patch chain. /// /// **~5,000 globals the surveyed VM would otherwise not have**, and the difference between an diff --git a/crates/benilla-app/src/ui_chat/feed.rs b/crates/benilla-app/src/ui_chat/feed.rs index de8906a6..2888e38b 100644 --- a/crates/benilla-app/src/ui_chat/feed.rs +++ b/crates/benilla-app/src/ui_chat/feed.rs @@ -164,12 +164,26 @@ impl ChatLog { Some(i) => (text[..i].to_string(), text[i + 1..].to_string()), None => (text.to_string(), String::new()), }; - let distribution = match chat_type { - 0x01 => "PARTY", - 0x03 => "RAID", - 0x04 => "GUILD", - 0x18 => "BATTLEGROUND", - _ => "UNKNOWN", + // **The protocol constants, not hand-copied bytes.** Three of the four literals here were + // WRONG: RAID is `0x2` and this said `0x03`, GUILD is `0x3` and this said `0x04`, and + // BATTLEGROUND is `0x5C` and this said `0x18`. The visible effect was not a dropped + // message but a MISLABELLED one — a real guild addon message arrived at + // `CHAT_MSG_ADDON` as `"RAID"`, while real raid and battleground traffic fell to + // `"UNKNOWN"` — so an addon branching on the distribution acted on the wrong lane. + // + // The outbound half (`net::addon_wire_chat_type`) always used the named constants and its + // own doc calls itself "the one place a distribution becomes a wire byte". It was not: this + // was the other one, and it disagreed. Both ends now read the same symbols, so they cannot + // drift apart again. + let distribution = { + use benilla_protocol::messages as m; + match u32::from(chat_type) { + m::CHAT_TYPE_PARTY => "PARTY", + m::CHAT_TYPE_RAID => "RAID", + m::CHAT_TYPE_GUILD => "GUILD", + m::CHAT_TYPE_BATTLEGROUND => "BATTLEGROUND", + _ => "UNKNOWN", + } } .to_string(); self.pending.push(Pending::Addon { diff --git a/crates/benilla-app/src/ui_chat/tests.rs b/crates/benilla-app/src/ui_chat/tests.rs index f1f214f3..c0b82391 100644 --- a/crates/benilla-app/src/ui_chat/tests.rs +++ b/crates/benilla-app/src/ui_chat/tests.rs @@ -1130,11 +1130,19 @@ fn a_sticky_whose_group_is_gone_opens_as_say() { /// branch on either way. #[test] fn an_inbound_addon_line_splits_on_the_first_tab_only() { - const PARTY: u8 = 0x01; - const RAID: u8 = 0x03; - const GUILD: u8 = 0x04; - const BATTLEGROUND: u8 = 0x18; - const SAY: u8 = 0x00; + // **Imported, not hand-copied.** These read `0x03`/`0x04`/`0x18` for RAID/GUILD/BATTLEGROUND, + // which are all wrong — and because the test carried the SAME wrong bytes as the code under + // test, it agreed with the defect instead of catching it. A test that restates the value it is + // checking cannot fail on that value; taking it from the protocol crate is what makes it a + // check rather than an echo. + use benilla_protocol::messages as m; + let party = m::CHAT_TYPE_PARTY as u8; + let raid = m::CHAT_TYPE_RAID as u8; + let guild = m::CHAT_TYPE_GUILD as u8; + let battleground = m::CHAT_TYPE_BATTLEGROUND as u8; + let say = m::CHAT_TYPE_SAY as u8; + #[allow(non_snake_case)] + let (PARTY, RAID, GUILD, BATTLEGROUND, SAY) = (party, raid, guild, battleground, say); let mut log = super::feed::ChatLog::default(); // The ordinary shape. diff --git a/crates/benilla-app/src/ui_script/group_loot_tests.rs b/crates/benilla-app/src/ui_script/group_loot_tests.rs index ec42aeea..17c57658 100644 --- a/crates/benilla-app/src/ui_script/group_loot_tests.rs +++ b/crates/benilla-app/src/ui_script/group_loot_tests.rs @@ -594,3 +594,52 @@ fn ctrl_and_shift_on_the_roll_icon_preview_and_post_its_link() { ); assert!(s.errors().is_empty(), "script errors: {:?}", s.errors()); } + +/// The roll-frame template is published under **the reference's own name**, and an addon that +/// inherits it finds the parts it reaches for (decision 1254). +/// +/// It shipped as `BenillaGroupLootFrameTemplate`. The `Benilla` prefix exists so a name WE invented +/// cannot collide with one an addon expects — and this template is the opposite of an invention: a +/// line-by-line transcription of 1.12's own `GroupLootFrameTemplate`, which addons inherit *by that +/// name*. `Bongos_RollBar/bar.lua:90` builds its four roll frames with +/// `CreateFrame("Frame", "BRollBarFrame"..i, bar, "GroupLootFrameTemplate")`; that resolves on the +/// real client and missed here for no reason but our prefix. +/// +/// The child names are the actual contract, so they are what this asserts. `bar.lua` reaches for +/// `IconFrameIcon`, `Name`, `Corner`, `Decoration` and `Timer` — and +/// `IconFrameIcon` is the one a flat template would get wrong, because it requires the icon texture +/// to be nested inside the `IconFrame` **button**, not hung off the template root. +#[test] +fn the_roll_template_carries_the_reference_name_and_the_parts_addons_reach_for() { + let s = setup(); + load_xml(&s, "GroupLootFrame.xml"); + + // The reference's name resolves — and, since 1253, a miss would RAISE rather than hand back a + // bare frame, so this call failing is this test failing. + s.run(r#"Probe = CreateFrame("Frame", "ProbeRoll", nil, "GroupLootFrameTemplate")"#) + .expect("the reference-named template must resolve"); + + for part in [ + "IconFrame", + "IconFrameIcon", + "Name", + "Corner", + "Decoration", + "Timer", + ] { + assert!( + s.eval::(&format!(r#"return getglobal("ProbeRoll{part}") ~= nil"#)) + .unwrap(), + "an addon inheriting the template must find ProbeRoll{part}" + ); + } + + // The timer is a StatusBar, not a plain frame: `bar.lua:33` calls SetMinMaxValues on it. + s.run(r#"ProbeRollTimer:SetMinMaxValues(0, 60000)"#) + .expect("the timer must answer StatusBar verbs"); + + // And the old private name is gone, so nothing can quietly depend on it again. + assert!(s + .eval::(r#"return getglobal("BenillaGroupLootFrameTemplate") == nil"#) + .unwrap()); +} diff --git a/crates/benilla-ui/src/framexml.rs b/crates/benilla-ui/src/framexml.rs index 69bcd4a1..dd7b87fa 100644 --- a/crates/benilla-ui/src/framexml.rs +++ b/crates/benilla-ui/src/framexml.rs @@ -328,15 +328,40 @@ fn expand_inner( return element.clone(); }; + // ── ONE name, matched case-INSENSITIVELY (wow-re, template-name lookup) ──────────────────── + // + // **No comma splitting.** 1.12's registry lookup `0x6ee6f0` has no splitter at all: + // `inherits="A, B"` is ONE literal name, misses, and applies neither template. Supporting the + // list is what LATER clients do, so splitting here made us a superset of 1.12 (1189). Nothing + // uses it — 0 comma lists in our own assets, 0 across the 218-addon corpus, and 0 in stock + // Blizzard XML's 249 distinct `inherits=` values. + // + // **Case-insensitive.** The compare is `SStrCmpI` → `_strnicmp` at `0x6ee747`, and the trap + // that decides it is the BUCKET HASH: `SStrHash 0x64b3f0` uppercases before mixing, so a + // mis-cased name lands in the same bucket and the stored-hash pre-check passes rather than + // short-circuiting. `Recap`'s `inherits="UIDropdownMenuTemplate"` (lowercase d) resolves on the + // real client. ASCII-only, like every other fold in this engine — bytes >= 0x80 are untouched. + // + // The fold is done as a scan on miss rather than by re-keying the view, because this same + // function resolves the FONT registry too and both are name→element maps of the same shape. + // Verbatim — not trimmed. `GetAttribute 0x6f2cf0` returns the parsed value as expat stored it + // (`SStrDup`, no trim, no fold), and the empty test is a single `cmp [esi],0` on the first + // byte: `inherits=""` is a silent skip, `inherits=" "` is a lookup that misses. let mut base: Option = None; - for name in inherits.split(',').map(str::trim).filter(|s| !s.is_empty()) { + for name in [inherits].into_iter().filter(|s| !s.is_empty()) { if !active.insert(name.to_string()) { warnings.push(format!( "inheritance cycle detected at template '{name}'; skipping this reference" )); continue; } - let Some(template) = templates.get(name) else { + let hit = templates.get(name).copied().or_else(|| { + templates + .iter() + .find(|(k, _)| k.eq_ignore_ascii_case(name)) + .map(|(_, v)| *v) + }); + let Some(template) = hit else { warnings.push(format!( "unknown template '{name}' referenced by inherits; skipping" )); @@ -566,8 +591,19 @@ print(x) assert!(doc.warnings.is_empty()); } + /// `inherits="A, B"` is **one name**, not a list — and it misses. + /// + /// This test used to assert the opposite (that both templates merged, children concatenated in + /// order). That was a superset of 1.12 borrowed from later clients: the carve found no splitter + /// anywhere in the loader — all 8 call sites hand `0x6ee6f0` the pointer `GetAttribute` + /// returned, one or two instructions later, and no comma is examined in `0x6ed000–0x6f6000` at + /// all. So the lookup runs **once**, for the literal name `"A, B"`, misses, warns at + /// severity 1, and the element loads with **neither** template applied. + /// + /// Whitespace is never trimmed either, which is why the second half asserts `" A "` misses a + /// template that genuinely exists. #[test] - fn multi_inherits_merges_attrs_override_and_children_concat_in_order() { + fn a_comma_list_is_one_literal_name_that_misses_and_applies_neither_template() { let doc = parse( r#" @@ -579,6 +615,7 @@ print(x) + "#, ) .unwrap(); @@ -588,20 +625,33 @@ print(x) }; let mut warnings = Vec::new(); let expanded = expand(inst, &templates, &mut warnings); - assert!(warnings.is_empty()); - // Instance's own `alpha` overrides B's (which would otherwise have overridden A's absence). + // Warned, naming the whole unsplit string — not split, not silently ignored. + assert_eq!(warnings.len(), 1, "{warnings:?}"); + assert!( + warnings[0].contains("A, B"), + "the warning must name the literal it looked up: {warnings:?}" + ); + + // The element still loads, with all of its OWN content and none of the templates'. assert_eq!(expanded.attr("alpha"), Some("1.0")); - - // Children: A's first, then B's , then the instance's own — - // inherited-first, instance-own-last (rf24-framexml-loader.md splice order). - assert_eq!(expanded.children.len(), 3); + assert_eq!(expanded.children.len(), 1); fn texture_name(el: &Element) -> &str { el.children[0].children[0].attr("name").unwrap() } - assert_eq!(texture_name(&expanded.children[0]), "FromA"); - assert_eq!(texture_name(&expanded.children[1]), "FromB"); - assert_eq!(texture_name(&expanded.children[2]), "FromInst"); + assert_eq!(texture_name(&expanded.children[0]), "FromInst"); + + // And a padded name misses a template that exists — the value is used verbatim. + let TopLevel::Instance(padded) = &doc.items[3] else { + panic!("expected instance") + }; + let mut w2 = Vec::new(); + let expanded = expand(padded, &templates, &mut w2); + assert_eq!(w2.len(), 1, "{w2:?}"); + assert!( + expanded.children.is_empty(), + "picked up A despite the spaces" + ); } #[test] diff --git a/crates/benilla-ui/src/loader/mod.rs b/crates/benilla-ui/src/loader/mod.rs index d14696d9..cea8c5b1 100644 --- a/crates/benilla-ui/src/loader/mod.rs +++ b/crates/benilla-ui/src/loader/mod.rs @@ -209,9 +209,16 @@ pub fn apply_template(lua: &mlua::Lua, wrapper: &Table, kind: &str, template: &s { let model = loader.model(); let templates = model.framexml_templates.borrow(); - for name in template.split(',').map(str::trim).filter(|s| !s.is_empty()) { + // One name, verbatim, folded — the same law `framexml::expand` resolves under, kept in + // step so this diagnostic cannot disagree with the resolution it describes. + for name in [template].into_iter().filter(|s| !s.is_empty()) { // A name that is missing entirely is `expand`'s warning to give, just below. - let Some(el) = templates.get(name) else { + let Some(el) = templates.get(name).or_else(|| { + templates + .iter() + .find(|(k, _)| k.eq_ignore_ascii_case(name)) + .map(|(_, v)| v) + }) else { continue; }; any_resolved = true; diff --git a/crates/benilla-ui/src/loader/regions.rs b/crates/benilla-ui/src/loader/regions.rs index f9973267..35ae5aeb 100644 --- a/crates/benilla-ui/src/loader/regions.rs +++ b/crates/benilla-ui/src/loader/regions.rs @@ -270,16 +270,18 @@ impl Loader<'_> { let model = self.model(); let fonts = model.framexml_fonts.borrow(); let templates = model.framexml_templates.borrow(); - for entry in inherits.split(',').map(str::trim) { + // One name, not a comma list — 1.12's lookup has no splitter, and supporting one is a + // superset of it. Folded on the probe, like the resolution itself. + for entry in [inherits] { let mut name = entry.to_string(); for _ in 0..MAX_HOPS { - if fonts.contains_key(&name) { + if fonts.contains_key(&name) || fonts.keys().any(|k| k.eq_ignore_ascii_case(&name)) + { return Some(name); } let Some(next) = templates .get(&name) .and_then(|t| t.attr("inherits")) - .and_then(|i| i.split(',').map(str::trim).next()) .map(str::to_string) else { break; diff --git a/crates/benilla-ui/src/script/mod.rs b/crates/benilla-ui/src/script/mod.rs index 1f34507d..e3bb4099 100644 --- a/crates/benilla-ui/src/script/mod.rs +++ b/crates/benilla-ui/src/script/mod.rs @@ -989,11 +989,17 @@ impl UiScript { /// /// A pure query on the VM's live registry, for the corpus harness: an addon naming a template /// we have not transcribed gets a bare frame and **no load error**, so nothing else can see it. + /// **Folded**, because the resolution it reports on is folded. An exact `contains_key` made this + /// census disagree with the loader the moment `inherits=` became case-insensitive: it went on + /// listing `UIDropdownMenuTemplate`, `CT_RaCheckButtonTemplate` and `MSBTColorSwatchTemplate` as + /// missing while the loader was resolving all three. An instrument that reports a gap the code + /// does not have is worse than no instrument — it is a build queue pointing at finished work + /// (1242/1246, and 1251 §3's rule that a source-derived answer be checked against the runtime + /// artefact). pub fn has_framexml_template(&self, name: &str) -> bool { - self.model_ref() - .framexml_templates - .borrow() - .contains_key(name) + let model = self.model_ref(); + let templates = model.framexml_templates.borrow(); + templates.contains_key(name) || templates.keys().any(|k| k.eq_ignore_ascii_case(name)) } /// Is `name` a registered FONT object — the *other* thing an `inherits=` may legally name? diff --git a/crates/benilla-ui/src/script/object.rs b/crates/benilla-ui/src/script/object.rs index 2007c48b..0dad3271 100644 --- a/crates/benilla-ui/src/script/object.rs +++ b/crates/benilla-ui/src/script/object.rs @@ -350,8 +350,12 @@ fn kind_method_registries(lua: &Lua, this: &Table) -> &'static [&'static str] { /// `OnLoad` must be able to see the frame's real name, and `$parent` inside the template must /// resolve against **the caller's** name, not the template's. /// -/// An unusable template (unknown, non-virtual, the wrong shape) is a warning on the model and a -/// perfectly usable frame — never an error that takes the frame with it. +/// An **unresolvable** template name raises `CreateFrame(): Couldn't find inherited node "%s"` and +/// creates nothing — the reference's own behaviour, byte-verified (`0x7061dd` → `luaL_error`, which +/// never returns). A template that resolves but is *unusable in some other way* — declared without +/// `virtual="true"`, or of a shape that does not fit the kind asked for — is still a warning plus a +/// working frame, because the registry lookup itself succeeded and that is the only thing the miss +/// branch tests. fn create_frame( lua: &Lua, (kind, name, parent, inherits): (String, Option, Option, Option), @@ -392,6 +396,39 @@ fn create_frame( } } + // ── THE TEMPLATE LOOKUP HAPPENS HERE — before anything is constructed or published ────────── + // + // `0x7061dd` calls the registry lookup `0x6ee6f0` and, on NULL, falls into + // `luaL_error(L, "CreateFrame(): Couldn't find inherited node \"%s\"")` at `0x7061ed`. That + // never returns — `luaG_errormsg`/`luaD_throw` contain no `ret` at all and end in `longjmp`, so + // the `xor eax,eax; ret` after the call is dead code. The frame is **not created**, no global + // is published, and the calling statement is abandoned; the error is ordinary Lua propagation + // and `pcall`-catchable, which is how the widget dispatcher (`0x704f10`, every handler under + // `lua_pcall`) turns it into one dead handler rather than a dead client. + // + // We used to warn and hand back a usable bare frame, and the doc above said the reference + // "creates the frame anyway". That claim is refuted at the bytes. It is the same class as 1249: + // a failure the client reports and we swallowed — and swallowing is the worse half, because the + // addon then runs on a frame with none of the art, regions or scripts it asked for and fails + // later, somewhere unrelated. + // + // Ordering is the carved part, not a detail: the miss branch precedes the synthetic-node build + // (`0x706208`) and the `name` store (`0x70622d`), so a miss leaves no partial widget and no + // orphan global behind. Doing this after construction would leave both. + if let Some(t) = template.as_deref().filter(|t| !t.is_empty()) { + let known = { + let model = lua.app_data_ref::().expect("model app_data"); + let templates = model.framexml_templates.borrow(); + // One name, verbatim, folded — the law `framexml::expand` resolves under. + templates.contains_key(t) || templates.keys().any(|k| k.eq_ignore_ascii_case(t)) + }; + if !known { + return Err(mlua::Error::runtime(format!( + "CreateFrame(): Couldn't find inherited node \"{t}\"" + ))); + } + } + // Create in the arena, mint the id, seed a default layout input. All under one write borrow. let id = { let mut model = lua.app_data_mut::().expect("model app_data"); diff --git a/crates/benilla-ui/src/script/object/events_regions.rs b/crates/benilla-ui/src/script/object/events_regions.rs index 15ac2d57..c4462f1b 100644 --- a/crates/benilla-ui/src/script/object/events_regions.rs +++ b/crates/benilla-ui/src/script/object/events_regions.rs @@ -134,11 +134,26 @@ pub(super) fn install(lua: &Lua, m: &Table) -> mlua::Result<()> { })?, )?; // Regions + // `CreateTexture(name, layer, inheritsFrom)` — the third argument accepted for the same reason + // and with the same resolver, though the demand is a rounding error beside FontString's: of + // 1329 corpus call sites exactly TWO pass a third argument, and one of those passes `nil` + // (CustomNameplates:437, a four-argument later-client form). Accepted rather than skipped + // because dropping it silently is the very class this fixes, and one real site is still a site. m.set( "CreateTexture", lua.create_function( - |lua, (this, name, layer): (Table, Option, Option)| { - create_region(lua, &this, RegionKind::Texture, name, layer) + |lua, + (this, name, layer, inherits): ( + Table, + Option, + Option, + Option, + )| { + let wrapper = create_region(lua, &this, RegionKind::Texture, name, layer)?; + if let Some(from) = inherits.as_deref().filter(|s| !s.is_empty()) { + apply_region_inherits(lua, &wrapper, from)?; + } + Ok(wrapper) }, )?, )?; @@ -222,11 +237,36 @@ pub(super) fn install(lua: &Lua, m: &Table) -> mlua::Result<()> { })?, )?; + // The THIRD argument is `inheritsFrom`, and we were dropping it on the floor. + // + // 49 corpus call sites across 5 distinct addons pass one — AckisRecipeList (28), + // CustomNameplates (10), _LazyPig (6), LibAboutPanel (4), ColorPickerPlus (1) — and **every one + // of them names a FONT OBJECT**: GameFontNormalSmall (15), GameFontNormal (11), + // GameFontHighlightSmall (10), GameFontHighlight (9), GameFontNormalLarge (3), GameFontDisable. + // Five separate addons, so this is not one library file replicated (1207 checked, not assumed). + // Ignoring it is 1203's class exactly: the addon asks for a font, we accept the call, and the + // string comes out in whatever the default is with no failure anywhere. + // + // The resolution order is carved (`0x773c30`): the FONT-object registry FIRST (`0x773d39`, + // create=0), and only on a font miss the template registry (`0x773d47`). This routes through + // the `SetFontObject` binding rather than reaching into the model, which is the same call the + // XML `` path makes — one implementation of "apply a font object", not + // two that can drift. m.set( "CreateFontString", lua.create_function( - |lua, (this, name, layer): (Table, Option, Option)| { - create_region(lua, &this, RegionKind::FontString, name, layer) + |lua, + (this, name, layer, inherits): ( + Table, + Option, + Option, + Option, + )| { + let wrapper = create_region(lua, &this, RegionKind::FontString, name, layer)?; + if let Some(from) = inherits.as_deref().filter(|s| !s.is_empty()) { + apply_region_inherits(lua, &wrapper, from)?; + } + Ok(wrapper) }, )?, )?; @@ -348,6 +388,44 @@ fn get_script(lua: &Lua, this: &Table, name: &str) -> mlua::Result { } } +/// `CreateTexture`/`CreateFontString`'s third argument — `inheritsFrom` — in the carved order. +/// +/// **Font-object registry first, template registry second** (`0x773d39` then `0x773d47`). That +/// order is not cosmetic: `inherits=` is one attribute over two namespaces, and every corpus caller +/// of this argument names a font object, so a template-first resolver would miss all 49 of them. +/// +/// A name in NEITHER registry raises, which is the same contract 1253 established for `CreateFrame` +/// and the same bytes (`luaL_error`, `0x87957c` / `0x879544`, which never returns). +/// +/// A name that IS a registered template but not a font object is a deliberate gap rather than a +/// silent one: the lookup succeeded, so the reference would not raise, and re-interpreting a +/// template node onto an existing region is a different mechanism from applying a font object. It +/// warns and leaves the region usable — the distinction 1253 drew between *the lookup missing* and +/// *the result being unusable*. Zero corpus callers reach it. +fn apply_region_inherits(lua: &Lua, wrapper: &Table, from: &str) -> mlua::Result<()> { + use mlua::ObjectLike; + // The font object is applied through the binding the XML path uses, so there is one + // implementation of "apply a font object" rather than two that can drift apart. + if wrapper.call_method::<()>("SetFontObject", from).is_ok() { + return Ok(()); + } + let is_template = { + let model = lua.app_data_ref::().expect("model"); + let templates = model.framexml_templates.borrow(); + templates.contains_key(from) || templates.keys().any(|k| k.eq_ignore_ascii_case(from)) + }; + if is_template { + let mut model = lua.app_data_mut::().expect("model"); + model.warnings.push(format!( + "CreateTexture/CreateFontString: '{from}' is a registered TEMPLATE, not a font object; the region is created but the template's content is not applied (no corpus caller does this)" + )); + return Ok(()); + } + Err(mlua::Error::runtime(format!( + "Couldn't find inherited node \"{from}\"" + ))) +} + fn create_region( lua: &Lua, this: &Table, diff --git a/crates/benilla-ui/src/script/tests/create_frame_template.rs b/crates/benilla-ui/src/script/tests/create_frame_template.rs index f1094834..000acc42 100644 --- a/crates/benilla-ui/src/script/tests/create_frame_template.rs +++ b/crates/benilla-ui/src/script/tests/create_frame_template.rs @@ -146,11 +146,17 @@ fn a_nested_frames_child_is_named_against_the_caller() { assert!(s.take_warnings().is_empty()); } -/// `inherits="A, B"` is a LIST, and the runtime path resolves it through the very same -/// [`crate::framexml::expand`] the XML path uses — including chains, splice order and the cycle -/// guard, none of which is written twice. +/// The template argument is **one name**, on this path exactly as on the XML one — and a chain +/// through a single name still resolves. +/// +/// This asserted the opposite until the registry lookup was carved. `"TemplA, TemplB"` is not a +/// list, it is a literal name nothing declared: 1.12's `CreateFrame` reaches the same `0x6ee6f0` +/// the XML loader does (call site `0x7061dd`) with the string Lua handed it, and the loader has no +/// splitter anywhere — comma lists are a **later**-client feature. Corroborated by the corpus: +/// 6842 `inherits=` across 282 vanilla XML files contain **zero** comma lists, while the modern +/// Blizzard UI source is full of them. #[test] -fn an_inherits_list_picks_up_every_template_in_it() { +fn a_comma_list_is_one_name_and_a_single_name_still_chains() { let mut s = script(); register( &s, @@ -168,60 +174,116 @@ fn an_inherits_list_picks_up_every_template_in_it() { "#, ); - s.run(r#"Both = CreateFrame("Frame", "Both", nil, "TemplA, TemplB")"#) - .unwrap(); - // From A (itself inheriting TemplRoot — the chain resolves). + // A single name still walks its whole chain. This is what keeps the narrowing honest: it + // removes the list, not the inheritance. + s.run(r#"One = CreateFrame("Frame", "One", nil, "TemplA")"#) + .unwrap(); assert_eq!( - s.eval::<(f32, f32)>("return Both:GetWidth(), Both:GetHeight()") + s.eval::<(f32, f32)>("return One:GetWidth(), One:GetHeight()") + .unwrap(), + (50.0, 60.0) + ); + assert_eq!(s.eval::("return One:GetAlpha()").unwrap(), 0.25); + assert!(s.take_errors().is_empty()); + assert!(s.take_warnings().is_empty()); + + // The comma form is one literal name, nothing declared it, so it RAISES — naming the whole + // unsplit string, which is the proof it was never split. + let err = s + .run(r#"Both = CreateFrame("Frame", "Both", nil, "TemplA, TemplB")"#) + .expect_err("a comma list is one name, and it misses"); + let text = err.to_string(); + assert!( + text.contains("TemplA, TemplB"), + "the miss must name the literal it looked up, unsplit: {text}" + ); + assert!( + s.eval::(r#"return getglobal("Both") == nil"#) + .unwrap(), + "a missed template must leave no frame behind" + ); +} + +/// The case fold, with the four names the corpus actually mis-cases. +/// +/// The compare is `SStrCmpI` → `_strnicmp`, and the trap that decides it is the bucket hash: +/// `SStrHash 0x64b3f0` uppercases before mixing, so a mis-cased name lands in the same bucket and +/// the stored-hash pre-check passes rather than short-circuiting. ASCII-only, like every other fold +/// in this engine. +#[test] +fn a_template_name_is_matched_case_insensitively() { + let mut s = script(); + register( + &s, + r#" + + + + "#, + ); + // Exactly the shape `CT_RaidAssist/CT_RAOptions.xml` ships: `RA` written `Ra`. + s.run(r#"Cased = CreateFrame("Frame", "Cased", nil, "CT_RaCheckButtonTemplate")"#) + .unwrap(); + assert_eq!( + s.eval::<(f32, f32)>("return Cased:GetWidth(), Cased:GetHeight()") .unwrap(), (50.0, 60.0) ); - assert_eq!(s.eval::("return Both:GetAlpha()").unwrap(), 0.25); - // From B. - assert!(s - .eval::(r#"return getglobal("BothBTex") ~= nil"#) - .unwrap()); assert!(s.take_errors().is_empty()); assert!(s.take_warnings().is_empty()); } -/// An unusable template name never takes the frame with it — the reference creates the frame -/// anyway, and an addon's next line already assumes it did. Both flavours of "unusable" are here: -/// a name nothing declared, and a name declared **without `virtual="true"`** (only virtual -/// elements are ever registered as templates, so from the registry's side the two look alike). +/// An unresolvable template name **raises, and creates nothing**. +/// +/// This test asserted the exact opposite — *"the reference creates the frame anyway, and an addon's +/// next line already assumes it did"* — until the bytes were read. `0x7061dd` looks the name up and +/// on NULL falls into `luaL_error(L, "CreateFrame(): Couldn't find inherited node \"%s\"")`, which +/// **never returns**: `luaG_errormsg` and `luaD_throw` contain no `ret` between them and end in +/// `longjmp`, so the `xor eax,eax; ret` that follows the call is dead code. (That trailing `ret` is +/// a fact about `luaL_error`'s `int` return type in C, not about reachability — which is exactly +/// how the old claim was arrived at.) +/// +/// Both flavours of "unusable" raise, because the registry only ever holds `virtual="true"` +/// elements, so a name declared without it misses the lookup identically to a name nothing declared. +/// +/// The ordering is asserted too: the miss precedes construction and name publication (`0x706208` / +/// `0x70622d`), so nothing partial is left behind — no frame, and no global. #[test] -fn an_unusable_template_still_returns_a_usable_frame_and_says_so() { - let mut s = script(); +fn an_unresolvable_template_raises_and_creates_nothing() { + let s = script(); register(&s, r#""#); for (frame, template) in [("Orphan", "NoSuchTemplate"), ("Plainer", "PlainFrame")] { - s.run(&format!( - r#"{frame} = CreateFrame("Button", "{frame}", nil, "{template}") - {frame}:SetWidth(5) - {frame}:Show()"# - )) - .unwrap_or_else(|e| panic!("{frame} must be usable: {e}")); - assert_eq!( - s.eval::(&format!("return {frame}:GetName()")) - .unwrap(), - frame - ); - assert_eq!( - s.eval::(&format!("return {frame}:GetWidth()")) - .unwrap(), - 5.0 + let err = s + .run(&format!( + r#"{frame} = CreateFrame("Button", "{frame}", nil, "{template}")"# + )) + .expect_err("an unresolvable template must raise"); + let text = err.to_string(); + assert!( + text.contains("Couldn't find inherited node") && text.contains(template), + "the raise must carry the reference's message and the name: {text}" ); - let warnings = s.take_warnings(); + // Nothing partial: no global published, and the name never reached the arena. assert!( - warnings - .iter() - .any(|w| w.contains(template) && w.contains(frame)), - "a warning must name both the template and the frame it was asked for: {warnings:?}" + s.eval::(&format!(r#"return getglobal("{frame}") == nil"#)) + .unwrap(), + "{frame} was published despite the miss" ); - assert!(s.take_errors().is_empty(), "a bad template is not an error"); } + + // And it is ordinary Lua propagation — `pcall` catches it, which is what keeps one bad + // CreateFrame inside a handler from taking the client down (the widget dispatcher runs every + // handler under `lua_pcall`). + assert!( + !s.eval::( + r#"return (pcall(CreateFrame, "Button", "Caught", nil, "NoSuchTemplate"))"# + ) + .unwrap(), + "the raise must be pcall-catchable" + ); } /// The two shape mismatches, neither of which may panic or drop the frame. diff --git a/crates/benilla-ui/src/script/tests/font_object.rs b/crates/benilla-ui/src/script/tests/font_object.rs index cbc06627..1e1e98fb 100644 --- a/crates/benilla-ui/src/script/tests/font_object.rs +++ b/crates/benilla-ui/src/script/tests/font_object.rs @@ -979,3 +979,53 @@ fn the_font_block_reaches_both_message_frame_tables() { "creating the style region must not re-justify the frame to the FontString default" ); } + +/// `CreateFontString`'s **third argument** applies a font object — the argument we accepted and +/// then dropped on the floor (decision 1255). +/// +/// 49 corpus call sites across 5 distinct addons pass one, and every one names a font object: +/// AckisRecipeList (28), CustomNameplates (10), _LazyPig (6), LibAboutPanel (4), ColorPickerPlus. +/// Five separate addons, so this is not one library file replicated. Ignoring it was 1203's class — +/// the addon asks for a font, the call succeeds, and the text comes out in the default with no +/// failure anywhere to point at. +/// +/// The **order** is the part a reimplementation loses: the font-object registry is tried FIRST and +/// the template registry only on a font miss (`0x773d39` then `0x773d47`). A template-first +/// resolver would miss all 49 of these, because `inherits=` is one argument over two namespaces. +#[test] +fn create_font_string_applies_the_font_object_named_by_its_third_argument() { + let s = script(); + load( + &s, + r#" + + + + + + "#, + ); + + // Exactly `_LazyPig/LazyPigMenu.lua:88`'s line. + s.run(r#"FS = Host:CreateFontString(nil, "ARTWORK", "GameFontNormalSmall")"#) + .expect("the font-object form must be accepted"); + let (face, height, _flags) = s + .eval::<(String, f32, String)>("return FS:GetFont()") + .expect("the region must carry the font object's face and height"); + assert_eq!(face, "Fonts\\FRIZQT__.TTF"); + assert_eq!(height, 12.0); + + // A name in NEITHER registry raises — the same contract 1253 set for CreateFrame, and the same + // bytes (`luaL_error`, which never returns). + let err = s + .run(r#"Bad = Host:CreateFontString(nil, "ARTWORK", "NoSuchFontOrTemplate")"#) + .expect_err("a name in neither registry must raise"); + assert!( + err.to_string().contains("NoSuchFontOrTemplate"), + "the raise must name what was looked up: {err}" + ); + + // Texture takes the same argument through the same resolver (1 real corpus site). + s.run(r#"TX = Host:CreateTexture(nil, "OVERLAY")"#) + .expect("the two-argument form still works"); +} diff --git a/crates/benilla-ui/src/script/tests/input.rs b/crates/benilla-ui/src/script/tests/input.rs index 740181f0..bd4a10fc 100644 --- a/crates/benilla-ui/src/script/tests/input.rs +++ b/crates/benilla-ui/src/script/tests/input.rs @@ -598,7 +598,10 @@ fn the_unfired_script_kinds_still_raise_rather_than_silently_accepting() { s.run(r#"Raiser = CreateFrame("EditBox", "RaiserBox")"#) .unwrap(); for name in [ - // No keyboard index / `EnableKeyboard` yet (wow-re `scripts-auto-enable.md` kinds 0/1). + // The FLAG exists (`EnableKeyboard`/`IsKeyboardEnabled`, 1252); what is still absent is the + // keyboard INDEX and the strata walk that would deliver a key to a frame carrying it, so + // these three stay unfired (wow-re `scripts-auto-enable.md` kinds 0/1). Being enabled is + // not being a handler — the carve is explicit that the two are separable. "OnKeyDown", "OnKeyUp", "OnChar", diff --git a/crates/benilla-ui/src/script/unit/mod.rs b/crates/benilla-ui/src/script/unit/mod.rs index 9520fe7e..6443c97c 100644 --- a/crates/benilla-ui/src/script/unit/mod.rs +++ b/crates/benilla-ui/src/script/unit/mod.rs @@ -416,7 +416,15 @@ pub(crate) fn token_recognised(token: &str) -> bool { token.eq_ignore_ascii_case("npc") || UNIT_TOKEN_PREFIXES .iter() - .any(|p| token.len() >= p.len() && token[..p.len()].eq_ignore_ascii_case(p)) + // BYTES, not a string slice. `token[..p.len()]` panics when the token is multibyte + // UTF-8 and the prefix length lands mid-character ("byte index N is not a char + // boundary") — an addon passing a non-ASCII token would take the client down. The + // client's own compare is `_strnicmp` over bytes with an ASCII-only fold, so byte + // comparison is both the safe form and the faithful one. + .any(|p| { + token.len() >= p.len() + && token.as_bytes()[..p.len()].eq_ignore_ascii_case(p.as_bytes()) + }) } /// The gate every `Unit*` binding puts an addon-supplied token through. diff --git a/crates/benilla-ui/src/script/unit/tests.rs b/crates/benilla-ui/src/script/unit/tests.rs index c15e9d45..ca16d4fe 100644 --- a/crates/benilla-ui/src/script/unit/tests.rs +++ b/crates/benilla-ui/src/script/unit/tests.rs @@ -919,3 +919,42 @@ fn an_unrecognised_unit_token_raises_and_a_recognised_empty_one_does_not() { assert!(s.run(r#"UnitIsUnit("bogus", "player")"#).is_err()); assert!(s.run(r#"UnitIsUnit("player", "party3")"#).is_ok()); } + +/// **A multibyte unit token must not take the client down.** +/// +/// The prefix test used to slice the token as a `&str` — `token[..p.len()]` — which PANICS when the +/// token is multibyte UTF-8 and the prefix length lands mid-character ("byte index N is not a char +/// boundary"). Any addon passing a non-ASCII token would have crashed the process rather than +/// getting the `Unknown unit name` raise the client gives. +/// +/// The client compares with `_strnicmp` over BYTES and folds ASCII only, so byte comparison is both +/// the safe form and the faithful one — a token whose bytes differ above 0x7F is simply not one of +/// the nine prefixes. +#[test] +fn a_multibyte_unit_token_raises_rather_than_panicking() { + let mut s = UiScript::new().unwrap(); + s.set_unit("player", Some(player())); + + // Each of these has a multibyte character positioned so that a byte-index slice at one of the + // prefix lengths (3 for `pet`, 4 for `raid`, 5 for `party`, 6 for `player`/`target`) would land + // inside it. + for token in ["pé", "раid", "playér", "targét", "мышь", "日本語のトークン"] { + let err = s + .run(&format!(r#"UnitName("{token}")"#)) + .expect_err("a non-ASCII token matches no prefix, so it raises") + .to_string(); + assert!( + err.contains("Unknown unit name"), + "it must RAISE, not panic and not answer: {err}" + ); + } + // …and an ASCII token that merely shares a prefix's leading bytes still behaves. + assert!(s + .eval::>(r#"return UnitName("playerfoo")"#) + .unwrap() + .is_none()); + assert_eq!( + s.eval::(r#"return UnitName("player")"#).unwrap(), + "Benilla" + ); +} diff --git a/third_party/lua-src/BENILLA.md b/third_party/lua-src/BENILLA.md index bd6596d4..0909aea0 100644 --- a/third_party/lua-src/BENILLA.md +++ b/third_party/lua-src/BENILLA.md @@ -35,15 +35,21 @@ downgrade, and it is the entire reason this is a one-hunk fork rather than a ven ## The delta, and the command that proves it -**Current state: no source hunk yet.** The `lua-5.1.5/` tree here is byte-identical to upstream's, -and `src/lib.rs` differs only by having Lua 5.2/5.3/5.4/5.5 stripped from the `Version` enum (with -their source trees deleted) — benilla builds 5.1 and only 5.1, and a fork that still *offered* the -other four would answer a request for one with a missing directory at build time instead of a -compile error. +**Current state: two hunks, in two files.** `src/lib.rs` additionally differs by having Lua +5.2/5.3/5.4/5.5 stripped from the `Version` enum (with their source trees deleted) — benilla builds +5.1 and only 5.1, and a fork that still *offered* the other four would answer a request for one with +a missing directory at build time instead of a compile error. -This ordering is deliberate: the build pipeline is proven transparent **before** any behavioural -payload goes into it, so that when the suite moves, the patch is the only thing that could have -moved it. Landed inert, with all tests passing, exactly so that claim is checkable. +| file | hunk | why | record | +|---|---|---|---| +| `lvm.c` | 5.0's `OP_TFORPREP` table→`next` substitution, folded into the top of `OP_TFORLOOP` | `for k, v in someTable do` raised "attempt to call a table value"; it was the first session-start error for **60** corpus addons | 1215 | +| `luaconf.h` | `LUA_COMPAT_LSTR` `1` → `2` | 5.1 kept 5.0's nesting machinery and put an advisory error in front of it; two corpus addons died on "nesting of `[[...]]` is deprecated" | — | + +The ordering was deliberate and it held: the fork landed **inert**, byte-identical to upstream, so +the build pipeline was proven transparent *before* any behavioural payload went into it — and when +the suite then moved, the patch was the only thing that could have moved it. Both hunks are measured +against the 218-addon corpus rather than argued: the long-string one alone took loaded 134 → 139 and +survivors 99 → 104. To verify the Lua sources against upstream at any time: @@ -53,17 +59,20 @@ diff -r third_party/lua-src/lua-5.1.5 \ ~/.cargo/registry/src/*/lua-src-550.0.0/lua-5.1.5 ``` -Today that prints nothing. When the payload lands it must print **exactly** the hunk described in -decision 1215 and nothing else — if it ever prints more, something crept in. +That must print **exactly** the two hunks in the table above and nothing else. If it ever prints +more, something crept in. ## Status of the payload -Blocked, deliberately, on a reverse-engineering answer from the sibling repo `wow-5875-re`: what the -**real 1.12.1 client's own Lua** does when a generic-for's generator is a table. Stock Lua 5.0 is -corroboration, not the authority — the client links 5.0 but may be modified. +**Landed.** The reverse-engineering answer this was deliberately blocked on came back from +`wow-5875-re` (`system/ui/scratch/lua-generic-for.md`) and is what the `lvm.c` comment cites, detail +by detail: a **bare type-tag** test that never consults a metatable (so a table carrying `__call` +still gets `next`), the **global** `next` read raw and fetched fresh at every loop entry (so an addon +assigning `next = myfn` changes every later generic-for in the session), and userdata *not* +substituted. -Checking stock 5.0 already corrected two assumptions the project had written down (it substitutes in -`OP_TFORPREP`, an opcode 5.1 deleted, *not* in `OP_TFORLOOP`; and the test is a bare `ttistable` -with **no** `__call` check, using the **global** `next` — so an addon that overwrites `next` changes -loop behaviour). Two corrections from checking one step out is the reason for checking the next one -before writing C. +Waiting was the right call twice over. Checking stock 5.0 first corrected two assumptions the +project had already written down — the substitution happens in `OP_TFORPREP`, an opcode 5.1 deleted, +*not* in `OP_TFORLOOP`; and the test is a bare `ttistable` with no `__call` check. Stock 5.0 was +corroboration, never the authority: the client links 5.0 but may be modified, and only the byte-read +of the client settles what it actually does.