diff --git a/config/src/registry.rs b/config/src/registry.rs index 685a8ced2..b6f306cfa 100644 --- a/config/src/registry.rs +++ b/config/src/registry.rs @@ -262,6 +262,90 @@ impl Registry { (0..self.props.len()).map(|i| PropId(i as u16)) } + /// Every way the flags a spec *declares* and the flags a CLI *binds* disagree. + /// + /// Empty means they agree. This is the check hk needed and did not have: it declares eighteen + /// `sources.cli` bindings and reads five, because the declaration lives in a spec and the + /// reading lives in a hand-written struct, and nothing has ever compared the two. A spec that + /// documents `--jobs` and a CLI that never puts it anywhere is a promise to a user that no test + /// could catch. + /// + /// `bound` is what the CLI actually does: pairs of a flag and the setting it sets. A CLI whose + /// flags come from `usage::Cli` can generate that list; one that binds by hand writes it out, + /// which is still one list rather than two behaviours. + /// + /// Both directions are reported, because they are different mistakes. A declared flag nothing + /// binds is documentation for something that does not happen. A bound flag the setting does not + /// declare happens without being documented — the user cannot discover it, and `explain` cannot + /// name it. + pub fn drift(&self, bound: &[(&str, &str)]) -> Vec { + let mut problems = Vec::new(); + + for (flag, key) in bound { + let Some(found) = self.lookup(key) else { + problems.push(format!( + "`{flag}` is bound to `{key}`, which is not a setting" + )); + continue; + }; + if !self.declares(found.id, flag) { + problems.push(format!( + "`{flag}` is bound to `{key}`, which does not declare it: add `cli \"{flag}\"` to the spec" + )); + } + } + + for id in self.ids() { + let meta = self.get(id); + // Asked once, and refused when it is `None`: two keys that resolve to nothing are not + // two keys that mean the same setting, and comparing the answers directly made a flag + // on a dangling `renamed_to` look bound by any flag bound to any other broken key. + let means = self.means(meta.key); + for flag in meta.cli { + let Some(means) = means else { + problems.push(format!( + "`{}` says `{flag}` sets it, and it is not a setting anything can reach: \ + its `renamed_to` names nothing, or the chain it starts loops", + meta.key + )); + continue; + }; + // Compared against the *setting* the binding names rather than the flag alone: a CLI + // may bind `--jobs` to something, and binding it to the wrong setting is not the same + // as binding it. + let bound_here = bound + .iter() + .any(|(bound_flag, key)| bound_flag == flag && self.means(key) == Some(means)); + if !bound_here { + problems.push(format!( + "`{}` says `{flag}` sets it, and nothing does", + meta.key + )); + } + } + } + problems + } + + /// Whether any declaration of the setting `id` lists `flag`. + /// + /// Any, because a rename leaves two declarations of one setting and either may be the one + /// carrying the flag: a CLI that has not dropped `--concurrency` yet is bound to a key that + /// *means* `jobs`, and the flag is declared where the old name is. Asking only the replacement + /// called a live binding drift; asking only the declaration named would miss a flag added to the + /// new name and still bound through the old one. + fn declares(&self, id: PropId, flag: &str) -> bool { + self.ids().any(|candidate| { + self.means(self.get(candidate).key) == Some(id) + && self.get(candidate).cli.contains(&flag) + }) + } + + /// Which setting a key means, following renames — `None` for a key that is not one. + fn means(&self, key: &str) -> Option { + self.lookup(key).map(|found| found.id) + } + /// Every setting bound to `kind`, with its key in that source. /// /// The generic mechanism a custom layer is written against: a git layer asks for `"git"` @@ -288,6 +372,7 @@ mod tests { PropMeta { key: "jobs", envs: &["HK_JOBS", "HK_JOB"], + cli: &["--jobs", "-j"], bindings: &[("git", "hk.jobs"), ("pkl", "jobs")], ..PropMeta::new("jobs", Ty::Uint) }, @@ -302,12 +387,153 @@ mod tests { ..PropMeta::new("threads", Ty::Uint) }, PropMeta { + // Declares a flag, which is what hk's dead `sources.cli` lines look like from here. + cli: &["--check"], bindings: &[("git", "hk.check")], ..PropMeta::new("check", Ty::Bool) }, ]; const REGISTRY: Registry = Registry::new(PROPS); + #[test] + fn a_cli_that_binds_what_the_spec_declares_has_no_drift() { + let bound = [("--jobs", "jobs"), ("-j", "jobs"), ("--check", "check")]; + assert_eq!(REGISTRY.drift(&bound), Vec::::new()); + } + + #[test] + fn a_declared_flag_nothing_binds_is_reported() { + // hk's thirteen dead `sources.cli` lines, and the reason they lasted: the declaration is in a + // spec, the reading is in a hand-written struct, and nothing compared them. A user reads + // `--check` in the documentation and it does nothing at all. + let bound = [("--jobs", "jobs"), ("-j", "jobs")]; + assert_eq!( + REGISTRY.drift(&bound), + vec!["`check` says `--check` sets it, and nothing does"] + ); + + // Every spelling counts. `-j` is as much a promise as `--jobs` is. + let bound = [("--jobs", "jobs"), ("--check", "check")]; + assert_eq!( + REGISTRY.drift(&bound), + vec!["`jobs` says `-j` sets it, and nothing does"] + ); + } + + #[test] + fn a_flag_bound_to_the_wrong_setting_is_not_a_flag_that_is_bound() { + // The failure a flag-only comparison would miss: `--check` is bound, so a check that only + // asked "is this flag bound anywhere" would pass — while `check` is still set by nothing and + // `jobs` is now set by a flag it never declared. + let bound = [("--jobs", "jobs"), ("-j", "jobs"), ("--check", "jobs")]; + assert_eq!( + REGISTRY.drift(&bound), + vec![ + "`--check` is bound to `jobs`, which does not declare it: add `cli \"--check\"` to the spec", + "`check` says `--check` sets it, and nothing does" + ] + ); + } + + #[test] + fn a_flag_bound_to_a_setting_that_does_not_exist_is_reported() { + let bound = [ + ("--jobs", "jobs"), + ("-j", "jobs"), + ("--check", "check"), + ("--nonesuch", "nonesuch"), + ]; + assert_eq!( + REGISTRY.drift(&bound), + vec!["`--nonesuch` is bound to `nonesuch`, which is not a setting"] + ); + } + + #[test] + fn a_flag_bound_through_an_old_name_is_bound() { + // A CLI written before a rename binds the name it knew. The value lands on the setting that + // replaced it, so the binding is real — reporting it as drift would make living through a + // rename impossible. + let bound = [("--jobs", "jobs"), ("-j", "jobs"), ("--check", "check")]; + assert_eq!(REGISTRY.drift(&bound), Vec::::new()); + let through_old_name = [ + ("--jobs", "concurrency"), + ("-j", "jobs"), + ("--check", "check"), + ]; + assert_eq!( + REGISTRY.drift(&through_old_name), + Vec::::new(), + "`concurrency` is `jobs`, and `jobs` declares `--jobs`" + ); + } + + // An old name that kept the flag it was documented with — what a rename looks like for a CLI + // that has not dropped the old spelling yet. Its own registry, because a declared flag is a + // promise: adding it to the shared one would make every other test's bindings incomplete. + static RENAMED_PROPS: &[PropMeta] = &[ + PropMeta { + cli: &["--jobs"], + ..PropMeta::new("jobs", Ty::Uint) + }, + PropMeta { + cli: &["--concurrency"], + renamed_to: Some("jobs"), + ..PropMeta::new("concurrency", Ty::Uint) + }, + ]; + const RENAMED: Registry = Registry::new(RENAMED_PROPS); + + #[test] + fn an_old_name_that_kept_its_flag_is_not_drift() { + // The two questions a rename separates. `--concurrency` is declared on the old name and + // binds the old key, so both sides are talking about `jobs` — but one asked `lookup` and the + // other did not, and the disagreement was reported twice: the flag "is not declared" by the + // replacement, and the declaration's flag "nothing does". A CLI would have deleted a live + // binding to satisfy it. + let bound = [("--jobs", "jobs"), ("--concurrency", "concurrency")]; + assert_eq!(RENAMED.drift(&bound), Vec::::new()); + + // And bound through the name that replaced it, which is the same setting and the same flag. + let by_new_name = [("--jobs", "jobs"), ("--concurrency", "jobs")]; + assert_eq!(RENAMED.drift(&by_new_name), Vec::::new()); + } + + #[test] + fn a_flag_on_a_rename_that_leads_nowhere_is_not_satisfied_by_another_broken_one() { + // `means` is `None` for a key whose rename names nothing or loops. Compared to each other, + // two of those were equal — so a declared flag counted as bound because some *other* dead + // key happened to be bound to the same spelling, and the dead declaration went unreported. + // The generator refuses a registry like this, but `drift` is also what a hand-written one + // is held to. + static BROKEN_PROPS: &[PropMeta] = &[ + PropMeta { + cli: &["--gone"], + renamed_to: Some("nowhere"), + ..PropMeta::new("gone", Ty::Uint) + }, + PropMeta { + cli: &["--gone"], + renamed_to: Some("also-nowhere"), + ..PropMeta::new("other", Ty::Uint) + }, + ]; + const BROKEN: Registry = Registry::new(BROKEN_PROPS); + + let bound = [("--gone", "other")]; + let problems = BROKEN.drift(&bound); + assert_eq!( + problems, + vec![ + "`--gone` is bound to `other`, which is not a setting", + "`gone` says `--gone` sets it, and it is not a setting anything can reach: its \ + `renamed_to` names nothing, or the chain it starts loops", + "`other` says `--gone` sets it, and it is not a setting anything can reach: its \ + `renamed_to` names nothing, or the chain it starts loops", + ] + ); + } + #[test] fn a_key_resolves_to_its_own_index() { let found = REGISTRY.lookup("jobs").expect("declared");