From fc78e52c25029f74261745e3cb400cc484f16673 Mon Sep 17 00:00:00 2001 From: grom Date: Thu, 5 Nov 2026 10:37:20 -0700 Subject: [PATCH] A `NAME = nil` line in an addon's saved file deletes the default it just set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit grom, 2026-11-05: *"I'd also like to get advanced trade skill window2 working … I can't even get number 2 to load right now."* His log has three runtime errors from it, all the same shape: AdvancedTradeSkillWindow2.lua:844: attempt to index global `ATSW_Profession' AdvancedTradeSkillWindow2.lua:5931: attempt to index global `ATSW_Bags' AdvancedTradeSkillWindow2.lua:5990: attempt to index global `ATSW_Bank' All three are assigned `{}` at FILE SCOPE — lines 294, 5929 and 5986 — and the chunk ran: the traceback carries `ATSW_OnEvent` (2504) calling `ATSW_ShowTrainingPoints` (4269), both past the first of them. So the tables existed and were taken away, and `benilla-config/saved/AdvancedTradeSkillWindow2.lua` says by what: 19 of its 24 declared names stood at `NAME = nil`. An addon's saved file is executed AFTER its own Lua (`0x51f4b5`, "after the files set their defaults"), which is what makes a nil line destructive rather than merely useless — in a fresh VM a global is already nil, so the line carries nothing and can only delete. And once written it is self-sustaining: the nil it forces is what the next shutdown reads back and writes again, so one bad write is permanent. The reference does not write them, which is why no real SavedVariables file has ever had a `= nil` line in it. Both sides, because the write fix alone still costs a session: * `saved_variables_bytes_for` skips a nil global. The flat `RegisterForSave` file still writes them — it is read BEFORE FrameXML, where a nil line and an absent one mean the same thing — so the two paths split and `serialize_saved` takes the flag. * `addon::drop_nil_assignments` strips `NAME = nil` lines a previous build already wrote, matched narrowly against that addon's own `## SavedVariables` set, one assignment per line, logged once per file. A player's hand-written `X = nil` of a name the addon never declared is left alone. It is not one addon: his install carried poisoned files for five — ATSW2 (20 lines), BigWigs (4), ImprovedErrorFrame (2), MasterTradeSkills (1), SuperCleveRoidMacros (1). Also the harness gap next door, which is why a capture could not have found this. A capture answered `GetRealmName()` as "" and `UnitName("player")` as "Benilla" even with `$WOW_CAPTURE_SAVED=C_Thun-Gromgosh` naming the character whose saved variables it had just loaded — so `ATSW_Profession[realm][player] = nil` (line 296) raised `table index is nil` and took 6,000 lines of chunk with it. That is a broken instrument that reads exactly like the defect being measured, and it hit every addon keyed on `[realm][player]`. `lifecycle.rs` now seats both from the capture identity, before the addon walk. Verified: `a_never_assigned_saved_variable_writes_no_line_and_a_nil_line_is_dropped_on_load` covers both halves. In a real session with his addons, all 24 of ATSW2's variables read as tables at the first handler and `ATSW_ShowTrainingPoints()` — the function that raised fifteen times — returns. Addon corpus unchanged at 86/87 clean loads, 86/87 UI probe. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/ui_script/addons.rs | 70 +++++++++++++++++++ crates/benilla-app/src/ui_script/lifecycle.rs | 26 +++++++ crates/benilla-ui/src/script/addon.rs | 55 +++++++++++++++ crates/benilla-ui/src/script/saved.rs | 28 +++++++- 4 files changed, 176 insertions(+), 3 deletions(-) diff --git a/crates/benilla-app/src/ui_script/addons.rs b/crates/benilla-app/src/ui_script/addons.rs index 493c95f6..664be124 100644 --- a/crates/benilla-app/src/ui_script/addons.rs +++ b/crates/benilla-app/src/ui_script/addons.rs @@ -2722,6 +2722,76 @@ mod tests { let _ = std::fs::remove_dir_all(home.parent().unwrap()); } + /// A saved variable the addon never assigned writes **no line**, and a `NAME = nil` line a + /// previous build already wrote is dropped on the way in. + /// + /// Both halves of one bug. An addon's saved file runs *after* its own Lua, so a `NAME = nil` + /// line deletes the default the addon set at file scope — and because the global is then nil + /// at logout, the next write puts the line back, which makes one bad write permanent. grom, + /// 2026-11-05: *"I can't even get number 2 to load right now."* Advanced Trade Skill Window 2 + /// declares 24 saved variables; 19 of them stood at `nil` in his file, including the three its + /// event handlers index unguarded (`ATSW_Profession`, `ATSW_Bags`, `ATSW_Bank`), each assigned + /// `{}` at file scope and each nil by the time the first handler ran. + #[test] + fn a_never_assigned_saved_variable_writes_no_line_and_a_nil_line_is_dropped_on_load() { + let _l = crate::local_state::test_env::ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _c = crate::local_state::test_env::EnvGuard::unset("WOW_CAPTURE"); + let (home, _h) = hermetic_root("nilsaved"); + // `Untouched` is declared and never assigned; `Defaulted` is ATSW2's shape — a table set + // at file scope and read by the addon's own code. + write_addon( + &home, + "Nilly", + "## Interface: 11200\n## SavedVariables: NillyDefaulted, NillyUntouched\nnilly.lua\n", + &[("nilly.lua", "NillyDefaulted = { seeded = true }\n")], + ); + let id = ("Realm".to_string(), "Char".to_string()); + + let mut script = UiScript::new().unwrap(); + script.set_screen_size(1024.0, 768.0); + let failures = load_third_party(&mut script, Some(&id), &[], true); + assert!(failures.is_empty(), "{failures:?}"); + crate::ui_script::shutdown_ui_state(&mut script, Some(&id), true); + + let path = crate::local_state::addon_saved_account_dir() + .expect("an account saved dir") + .join("Nilly.lua"); + let written = std::fs::read_to_string(&path).expect("the file was written"); + assert!( + written.contains("NillyDefaulted = {"), + "the table the addon set is saved:\n{written}" + ); + assert!( + !written.contains("NillyUntouched"), + "a nil global writes nothing at all — a `= nil` line would delete the addon's own \ + default on the next load:\n{written}" + ); + + // Now the migration: a file an older build poisoned still has to load harmlessly. + std::fs::write( + &path, + "NillyDefaulted = nil\nNillyUntouched = nil\nNillyOther = nil\n", + ) + .unwrap(); + let mut next = UiScript::new().unwrap(); + next.set_screen_size(1024.0, 768.0); + let failures = load_third_party(&mut next, Some(&id), &[], true); + assert!(failures.is_empty(), "{failures:?}"); + assert_eq!( + next.eval::("return NillyDefaulted ~= nil and NillyDefaulted.seeded") + .ok(), + Some(true), + "the addon's file-scope default survives a poisoned file" + ); + assert!( + next.eval::("return NillyOther == nil").unwrap(), + "a name the addon does not declare is left exactly as the file has it" + ); + let _ = std::fs::remove_dir_all(home.parent().unwrap()); + } + /// The reference writer's grammar (`0x7043f0`/`0x704480`): `NAME = value`, bracketed keys, TAB /// indent, a trailing comma on every entry, a bracketed key for a list entry too (`[1] = 1,`), /// and the file split by scope. Asserted as bytes, not by reloading. diff --git a/crates/benilla-app/src/ui_script/lifecycle.rs b/crates/benilla-app/src/ui_script/lifecycle.rs index b08f6cce..76649f59 100644 --- a/crates/benilla-app/src/ui_script/lifecycle.rs +++ b/crates/benilla-app/src/ui_script/lifecycle.rs @@ -794,9 +794,19 @@ pub(crate) fn load_ingame_ui_on_world_entry(world: &mut World) { } // The realm before the load, since addons read `GetRealmName()` at file scope; the roster // carries the realm-list entry this session connected to. + // + // A capture never logs in and so has no roster, and `$WOW_CAPTURE_SAVED=-` is + // already the way a capture is pointed at a player's own saved variables — so it names the + // realm here too. Without it the verb answered `""` while the saved file it had just loaded + // was keyed on `"C'Thun"`, and an addon that indexes `[realm][player]` at file scope then + // behaves under capture in a way it never does in play: `AdvancedTradeSkillWindow2.lua:296` + // is `ATSW_Profession[realm][player] = nil`, which with an empty realm and an unknown player + // raises `table index is nil` and takes the rest of a 6,000-line chunk with it. That is a + // broken instrument, not a broken addon — it reads exactly like the defect being measured. let realm = world .get_resource::() .and_then(|r| r.realm.as_ref().map(|r| r.name.clone())) + .or_else(|| crate::local_state::capture_saved_identity().map(|(realm, _)| realm)) .unwrap_or_default(); script.set_realm_name(&realm); // The player too, before the addon walk. The record first: the reference's @@ -806,6 +816,22 @@ pub(crate) fn load_ingame_ui_on_world_entry(world: &mut World) { if let Some(record) = world .get_resource::() .and_then(record_from_roster) + .or_else(|| { + // The other half of the realm above, and for the same reason: `$WOW_CAPTURE_SAVED` + // names the **character** whose saved variables this capture has just loaded, so + // `UnitName("player")` has to answer that name or an addon's `[realm][player]` key + // looks up rows it was not handed. Race, class and sex are presence-only here, as + // `manifest.rs`'s own offline record documents; a fixture that seats a real one + // later overwrites this, and the addon walk has already run by then. + crate::local_state::capture_saved_identity().map(|(_, name)| { + benilla_ui::script::PlayerRecord { + name, + race: Some(("Human".into(), "Human".into())), + class: Some(("Warrior".into(), "WARRIOR".into())), + sex: 2, + } + }) + }) { script.set_player_record(record); } diff --git a/crates/benilla-ui/src/script/addon.rs b/crates/benilla-ui/src/script/addon.rs index 09fe45ee..608558cb 100644 --- a/crates/benilla-ui/src/script/addon.rs +++ b/crates/benilla-ui/src/script/addon.rs @@ -514,6 +514,41 @@ fn load_bindings(lua: &Lua, name: &str, read: &Reader) { /// Run the addon's saved-variables files, account then per-character (`0x51f4b5`, `0x51f53b`), /// as chunks in the shared globals with no `setfenv` (`0x704bc0`, `0x704ae0`), between its files /// and `ADDON_LOADED` so saved values win; a missing file is skipped (`0x51f4a9`, `0x51f530`). +/// Strip `NAME = nil` lines naming one of this addon's declared saved variables. +/// +/// An addon's saved file runs **after** its own Lua (`0x51f4b5`), so such a line cannot carry +/// information — a global is already nil in a fresh VM — and can only delete the default the +/// addon set moments earlier. Ours used to write them (see +/// [`crate::script::UiScript::saved_variables_bytes_for`], which no longer does), and once one +/// was written the state was self-sustaining: the nil it forced was what the next shutdown read +/// back and wrote again. This is the migration for a file an older build already poisoned. +/// +/// Deliberately narrow: the exact shape our own writer emitted, one assignment per line, the name +/// matched against the `## SavedVariables` set rather than any identifier, and nothing else about +/// the chunk touched. A player's hand-written `X = nil` of a name the addon never declared is +/// left alone, and so is anything inside a table literal, which is indented. +fn drop_nil_assignments(bytes: Vec, declared: &[String]) -> (Vec, usize) { + if declared.is_empty() || !bytes.windows(6).any(|w| w == b" = nil") { + return (bytes, 0); + } + let mut out = Vec::with_capacity(bytes.len()); + let mut dropped = 0usize; + for line in bytes.split_inclusive(|b| *b == b'\n') { + let trimmed = line.strip_suffix(b"\n").unwrap_or(line); + let trimmed = trimmed.strip_suffix(b"\r").unwrap_or(trimmed); + let name = std::str::from_utf8(trimmed) + .ok() + .and_then(|s| s.strip_suffix(" = nil")) + .filter(|n| declared.iter().any(|d| d == n)); + if name.is_some() { + dropped += 1; + continue; + } + out.extend_from_slice(line); + } + (out, dropped) +} + fn load_saved_variables(lua: &Lua, i: usize) { let (name, account, character, has_account, has_character) = { let model = lua.app_data_ref::().expect("model"); @@ -526,6 +561,15 @@ fn load_saved_variables(lua: &Lua, i: usize) { !a.saved_variables_per_character.is_empty(), ) }; + let declared_names: Vec = { + let model = lua.app_data_ref::().expect("model"); + let a = &model.addons[i]; + a.saved_variables + .iter() + .chain(a.saved_variables_per_character.iter()) + .cloned() + .collect() + }; for (dir, declared) in [(account, has_account), (character, has_character)] { if !declared { continue; // declares none: no file is read, even if one exists @@ -545,6 +589,17 @@ fn load_saved_variables(lua: &Lua, i: usize) { continue; } }; + let (bytes, dropped) = drop_nil_assignments(bytes, &declared_names); + if dropped > 0 { + // A migration, not a policy: see `drop_nil_assignments`. + lua.app_data_mut::() + .expect("model") + .record_warning(format!( + "{}: dropped {dropped} `NAME = nil` line(s) written by an older build — \ + they would have deleted the defaults this addon just set", + path.display() + )); + } if let Err(e) = run_chunk( lua, &bytes, diff --git a/crates/benilla-ui/src/script/saved.rs b/crates/benilla-ui/src/script/saved.rs index f052b1ee..974336a0 100644 --- a/crates/benilla-ui/src/script/saved.rs +++ b/crates/benilla-ui/src/script/saved.rs @@ -46,14 +46,33 @@ impl super::UiScript { /// Serialize every registered global as `NAME = value` lines, in bytes, since a Lua string is /// bytes and the reference writes them raw. A value with no literal (a function, userdata, /// `inf`, `NaN`) is skipped with a warning where the reference drops it silently; `nil` is - /// written, as the reference records an untouched toggle. + /// written, as the reference records an untouched toggle. This is the **flat** file, read + /// before FrameXML runs, so a `NAME = nil` line and an absent one mean the same thing there. pub fn saved_variables_bytes(&self) -> Vec { - self.saved_variables_bytes_for(&self.saved_variable_names()) + self.serialize_saved(&self.saved_variable_names(), true) } /// [`UiScript::saved_variables_bytes`] over an explicit name list, an addon's own - /// `## SavedVariables` set. + /// `## SavedVariables` set — **and a nil global writes no line at all**, which is the one + /// difference from the flat file. + /// + /// An addon's file is executed *after* its own Lua has run (`0x51f4b5`, "after the files set + /// their defaults"), so here the two are not the same thing: a `NAME = nil` line does not + /// record "untouched", it **deletes the default the addon just set**. A session that wrote + /// one poisons every session after it, because the nil it forces is then what the next + /// shutdown reads back and writes again. + /// + /// That is not a hypothetical. grom, 2026-11-05: *"I can't even get number 2 to load right + /// now"* — `benilla-config/saved/AdvancedTradeSkillWindow2.lua` held `nil` for 19 of its 24 + /// names, including `ATSW_Profession` (set at `AdvancedTradeSkillWindow2.lua:294`), + /// `ATSW_Bags` (`:5929`) and `ATSW_Bank` (`:5986`) — all three assigned at file scope and all + /// three `nil` by the time the first event handler read them, which is the whole of that + /// addon's reported breakage. pub fn saved_variables_bytes_for(&self, names: &[String]) -> Vec { + self.serialize_saved(names, false) + } + + fn serialize_saved(&self, names: &[String], write_nil: bool) -> Vec { let mut out = Vec::new(); let mut unwritable = Vec::new(); // A widget writes as its global name, and only the model can turn a `T[0]` id into one. @@ -74,6 +93,9 @@ impl super::UiScript { continue; } }; + if !write_nil && matches!(value, Value::Nil) { + continue; + } let mut seen = HashSet::new(); match serialize(&value, 1, &mut seen, &name_of) { Some(text) => {