diff --git a/config/src/explain.rs b/config/src/explain.rs index cd22a629c..7d2507679 100644 --- a/config/src/explain.rs +++ b/config/src/explain.rs @@ -66,6 +66,11 @@ pub fn explain(resolved: &Resolved, key: &str) -> Option { // The spec's own spelling, not the prose an error message uses: a reader searching the docs // for "a non-negative integer" finds nothing, and `uint` is what the author wrote. let _ = writeln!(out, " {:<8}{}", "type", meta.ty.name()); + // What it will take, when it says. A user reading an explanation because their value was refused + // needs the list here rather than in a warning they have already scrolled past. + if !meta.choices.is_empty() { + let _ = writeln!(out, " {:<8}{}", "one of", one_line(&meta.allowed())); + } if let Some(help) = meta.help { // Through the same helper as everything else: an adopter's help is a doc comment, and a // doc comment with a second paragraph in it split one fact into several records. diff --git a/config/src/layer.rs b/config/src/layer.rs index f8c7d19e5..7d0ed14a3 100644 --- a/config/src/layer.rs +++ b/config/src/layer.rs @@ -166,10 +166,16 @@ impl LayerCtx { // binding carries the same information as one built from a key. let (id, renamed_from) = self.folded(id); match self.parse(id, raw) { - Ok(value) => Ok(Entry { - renamed_from, - ..Entry::new(id, value, origin) - }), + Ok(value) => { + let key = renamed_from.unwrap_or(self.registry.get(id).key); + if let Some(refused) = self.refused(id, &value, key, &origin) { + return Err(refused); + } + Ok(Entry { + renamed_from, + ..Entry::new(id, value, origin) + }) + } Err(err) => { // The name that was written, not the one it folded to: a message about a key // the user cannot find in their own file is no help. @@ -187,6 +193,26 @@ impl LayerCtx { } impl LayerCtx { + /// The warning for a value the setting's `choice` nodes do not allow, if it is one. + /// + /// Beside the type check and for the same reason: a declared type and a declared set of values + /// are both the spec saying what may be here, and a value that is neither costs its own key and + /// nothing else. Until this, choices reached the docs, the JSON schema and completions, and + /// nothing that *resolved* a value — so a CLI documenting three allowed values took a fourth + /// without a word, and only failed later, somewhere that could not say why. + fn refused(&self, id: PropId, value: &Value, key: &str, origin: &Origin) -> Option { + let meta = self.registry.get(id); + let refused = meta.refuses(value)?; + Some(Warning::at( + format!( + "{key} expected one of {} but has `{}`", + meta.allowed(), + crate::value::shown(refused) + ), + origin.clone(), + )) + } + /// An entry for a dotted key, which is what a layer reading a file has in hand. /// /// The path worth taking: it looks the key up, follows a rename while remembering the name @@ -222,10 +248,16 @@ impl LayerCtx { }; let meta = self.registry.get(found.id); match meta.ty.coerce(value) { - Ok(value) => Ok(Entry { - renamed_from: found.renamed_from, - ..Entry::new(found.id, value, origin) - }), + Ok(value) => { + let key = found.renamed_from.unwrap_or(meta.key); + if let Some(refused) = self.refused(found.id, &value, key, &origin) { + return Err(refused); + } + Ok(Entry { + renamed_from: found.renamed_from, + ..Entry::new(found.id, value, origin) + }) + } Err(err) => Err(Warning::at( format!( "{} expected {} but has `{}`", @@ -253,6 +285,7 @@ mod tests { use super::*; use crate::registry::PropMeta; use crate::ty::{Parser, Ty}; + use crate::value::Const; static PROPS: &[PropMeta] = &[ PropMeta::new("jobs", Ty::Uint), @@ -260,9 +293,93 @@ mod tests { parse: Some(Parser::ListByComma), ..PropMeta::new("exclude", Ty::List(&Ty::String)) }, + // A setting the spec limits to three values, which is hk's `stash` and mise's nine + // enum-valued settings. + PropMeta { + choices: &[ + Const::Str("git"), + Const::Str("patch-file"), + Const::Str("none"), + ], + ..PropMeta::new("stash", Ty::String) + }, + // Choices on a list: each *item* is one of them, the way the JSON schema reads it. + PropMeta { + parse: Some(Parser::ListByComma), + choices: &[Const::Str("lint"), Const::Str("test")], + ..PropMeta::new("skip", Ty::List(&Ty::String)) + }, ]; const REGISTRY: Registry = Registry::new(PROPS); + #[test] + fn a_value_the_spec_does_not_allow_is_refused_with_the_list_of_what_is() { + // Choices reached the docs, the JSON schema and completions, and nothing that *resolved* a + // value: a CLI documenting three allowed values took a fourth in silence, and failed later + // somewhere that could not say why. + let ctx = LayerCtx::new(REGISTRY); + let origin = Origin::new(SourceKind::ENV, "HK_STASH"); + let warning = ctx + .entry_for_key("stash", "svn", origin.clone()) + .expect_err("not one of the three"); + assert_eq!( + warning.message, + "stash expected one of git, patch-file, none but has `svn`" + ); + // And a value that is one of them is just a value. + assert_eq!( + ctx.entry_for_key("stash", "git", origin.clone()) + .map(|entry| entry.value), + Ok(Value::from("git")) + ); + + // A list is checked item by item, and the *item* is what the message quotes — naming the + // whole list would leave the user to work out which of five items was the problem. + let warning = ctx + .entry_for_key("skip", "lint,fmt", origin.clone()) + .expect_err("`fmt` is not one of them"); + assert_eq!( + warning.message, + "skip expected one of lint, test but has `fmt`" + ); + assert!(ctx.entry_for_key("skip", "lint,test", origin).is_ok()); + } + + #[test] + fn a_setting_with_no_choices_takes_what_its_type_takes() { + // Most settings say nothing about their values, and the check has to cost them nothing and + // refuse them nothing. + let ctx = LayerCtx::new(REGISTRY); + let origin = Origin::new(SourceKind::ENV, "HK_JOBS"); + assert_eq!( + ctx.entry_for_key("jobs", "8", origin).map(|e| e.value), + Ok(Value::Int(8)) + ); + } + + #[test] + fn a_structured_value_is_held_to_the_same_choices() { + // The other way a value arrives — a table or a list out of a file, which never passes + // through a parser. Checking one path and not the other is how a rule ends up applying to + // the environment and not to the file beside it. + let ctx = LayerCtx::new(REGISTRY); + let origin = Origin::new(SourceKind::FILE, "hk.toml"); + let warning = ctx + .entry_from_value( + "skip", + Value::List(vec![Value::from("test"), Value::from("deploy")]), + origin.clone(), + ) + .expect_err("`deploy` is not one of them"); + assert_eq!( + warning.message, + "skip expected one of lint, test but has `deploy`" + ); + assert!(ctx + .entry_from_value("skip", Value::List(vec![Value::from("test")]), origin) + .is_ok()); + } + #[test] fn a_raw_string_is_read_the_way_the_spec_says() { let ctx = LayerCtx::new(REGISTRY); diff --git a/config/src/registry.rs b/config/src/registry.rs index 129516c71..1a94fc551 100644 --- a/config/src/registry.rs +++ b/config/src/registry.rs @@ -10,7 +10,7 @@ use crate::source::SourceKind; use crate::ty::{Parser, Ty}; -use crate::value::Const; +use crate::value::{Const, Value}; /// A setting's index in its registry. /// @@ -76,6 +76,12 @@ pub struct PropMeta { /// asks the registry for its own kind and iterates what it finds, which is the whole /// mechanism behind hk's git and pkl layers and aube's `.npmrc`. pub bindings: &'static [(&'static str, &'static str)], + /// The only values this setting accepts, when it says. + /// + /// Empty means anything the type allows. Declared in the spec as `choice` nodes, where they + /// already reach the docs, the JSON schema and completions — and, until this, nothing that + /// *resolved* a value, so a CLI documenting three allowed values accepted a fourth in silence. + pub choices: &'static [Const], /// Kept out of documentation and completions. Still settable. pub hide: bool, /// Why not to use this any more. @@ -98,6 +104,7 @@ impl PropMeta { parse: None, envs: &[], bindings: &[], + choices: &[], hide: false, deprecated: None, renamed_to: None, @@ -121,6 +128,37 @@ pub struct Lookup { pub renamed_from: Option<&'static str>, } +impl PropMeta { + /// The first value here that this setting does not allow, if there is one. + /// + /// A collection is checked item by item, because choices on a `list` mean each item is + /// one of them — the same rule `usage g json-schema` follows, which puts the enum on every value + /// position rather than on the container. Returning the offender rather than a bool is what lets + /// the warning quote the item that is wrong instead of the whole list it was in. + pub fn refuses<'v>(&self, value: &'v Value) -> Option<&'v Value> { + if self.choices.is_empty() { + return None; + } + match value { + Value::List(items) => items.iter().find_map(|item| self.refuses(item)), + Value::Map(entries) => entries.values().find_map(|item| self.refuses(item)), + scalar => match self.choices.iter().any(|choice| choice.matches(scalar)) { + true => None, + false => Some(scalar), + }, + } + } + + /// What it allows, written the way the spec declared them, for a message. + pub fn allowed(&self) -> String { + self.choices + .iter() + .map(|choice| choice.to_value().display()) + .collect::>() + .join(", ") + } +} + impl Registry { pub const fn new(props: &'static [PropMeta]) -> Self { Self { props } diff --git a/config/src/value.rs b/config/src/value.rs index 4e01dac1a..85fe7f9de 100644 --- a/config/src/value.rs +++ b/config/src/value.rs @@ -157,6 +157,23 @@ pub enum Const { } impl Const { + /// Whether `value` is this constant. + /// + /// Without building the `Value` it stands for: this runs once per declared choice for every + /// value a layer supplies, and a setting with choices is usually a string, where the comparison + /// would otherwise allocate a copy of the choice to throw away. + pub fn matches(self, value: &Value) -> bool { + match (self, value) { + (Self::Bool(a), Value::Bool(b)) => a == *b, + (Self::Int(a), Value::Int(b)) => a == *b, + (Self::Float(a), Value::Float(b)) => a == *b, + (Self::Str(a), Value::String(b)) => a == b, + // A list or a table is not something a `choice` node can hold, and comparing one to a + // scalar is not a near miss to be generous about. + _ => false, + } + } + /// The owned value this stands for. pub fn to_value(self) -> Value { match self {