From eef64a6b10f3cee4f29e5c274ee5267cef7b8ad9 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 16 Aug 2026 09:34:17 +0800 Subject: [PATCH] fix(config): name the keys nothing reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `config.json` holding `font_siz` behaves exactly like one holding a setting that does not work: the value is dropped and nothing is said. Confirmed before changing anything — a config with `font_siz`, `totally_made_up` and an out-of-range `ui_font_size` produced one warning, about the range. Unknown keys are ignored on purpose and that stays: a retired key must not make a whole config unreadable, which `a_config_still_carrying_the_retired_theme_key_loads` pins. Ignoring a key and never mentioning it are different things, though, and only the second one leaves someone staring at a setting that looks right. The known set is the serialised default rather than a hand-written list, so it cannot drift from the struct. That trick only holds while `Config` skips no field on the way out, so the test asks each field individually whether it would be called a typo — a skipped one would be reported to its owner as a misspelling, and this fails the day that becomes possible. Behind `log_enabled!`, because it parses the file a second time and this path reloads per pane spawn. Verified: `font_siz` and `totally_made_up` each named, beside the clamp warning from the last commit. --- crates/tty7-core/src/core/config.rs | 80 +++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/crates/tty7-core/src/core/config.rs b/crates/tty7-core/src/core/config.rs index e00fc09f..4cb3d3a2 100644 --- a/crates/tty7-core/src/core/config.rs +++ b/crates/tty7-core/src/core/config.rs @@ -699,6 +699,7 @@ impl Config { }; match serde_json::from_str::(strip_bom(&text)) { Ok(mut cfg) => { + note_unknown_keys(strip_bom(&text)); cfg.sanitize(); (cfg, LoadOutcome::Parsed) } @@ -1034,6 +1035,47 @@ pub(crate) fn shell_command() -> Option<(String, Vec)> { Config::load().shell.map(|s| (s.program, s.args)) } +/// Name the keys in the file that nothing reads. +/// +/// The struct deliberately takes no `deny_unknown_fields`: a retired key must +/// not make a whole config unreadable, which is what +/// `a_config_still_carrying_the_retired_theme_key_loads` pins. But ignoring a +/// key and never mentioning it are different things — a mistyped setting name +/// behaves exactly like a setting that does not work, and there is nothing to +/// tell the two apart from the outside. +/// +/// The known set is the serialized default, so it cannot drift from the struct +/// the way a hand-listed set would; `Config` skips no field on the way out. +/// A retired key gets named too, which is true: nothing reads it. +/// +/// Behind `log_enabled!` because this parses the file a second time, and this +/// runs on a path that reloads per pane spawn. +fn note_unknown_keys(text: &str) { + if !log::log_enabled!(log::Level::Warn) { + return; + } + for key in unknown_keys(text) { + log::warn!("config {key}: not a setting tty7 reads — check the spelling"); + } +} + +/// The comparison behind [`note_unknown_keys`], separated so the property that +/// matters can be tested: no real field may ever be named here. +fn unknown_keys(text: &str) -> Vec { + let Ok(serde_json::Value::Object(found)) = serde_json::from_str::(text) + else { + return Vec::new(); + }; + let Ok(serde_json::Value::Object(known)) = serde_json::to_value(Config::default()) else { + return Vec::new(); + }; + found + .keys() + .filter(|k| !known.contains_key(*k)) + .cloned() + .collect() +} + /// Say when a configured value was not the one used. /// /// Only when it actually moved: a file already inside the range is the normal @@ -1418,6 +1460,44 @@ mod tests { assert!(!back.dim_inactive_panes); } + /// Every field the struct has must be recognised, and only those. + /// + /// The known set comes from serialising the default, so this is really + /// asking whether that trick holds: a field the struct skips on the way + /// out would be reported to its owner as a typo. `Config` skips none + /// today, and this fails the day one does. + #[test] + fn no_real_setting_is_ever_called_a_typo() { + let serde_json::Value::Object(all) = + serde_json::to_value(Config::default()).expect("the default serialises") + else { + panic!("a config is an object"); + }; + assert!(all.len() > 20, "sanity: {} fields", all.len()); + + let whole = serde_json::to_string(&Config::default()).expect("serialise"); + assert!( + unknown_keys(&whole).is_empty(), + "these real settings would be reported as typos: {:?}", + unknown_keys(&whole) + ); + + // One at a time, so a single skipped field cannot hide in the crowd. + for name in all.keys() { + let one = format!("{{\"{name}\": null}}"); + assert!( + unknown_keys(&one).is_empty(), + "{name} is a real setting but reads as a typo" + ); + } + + assert_eq!( + unknown_keys(r#"{"font_siz": 1, "font_size": 12}"#), + vec!["font_siz".to_string()], + "and a real typo beside a real field is still caught" + ); + } + /// `theme` was the theme id before `theme_preset` replaced it, and it went /// on being written into every config.json for a month after nothing read /// it. Dropping the field must not make those files unreadable — the