From 8a1b1a671ca1a20b63e4b21b7dd34b82c9751a36 Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Fri, 14 Aug 2026 02:00:50 +0000 Subject: [PATCH] feat(config): say what sort of thing each warning is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A warning's wording is for a person and is nobody's contract. What a *program* acts on is the kind: mise queues its deprecations until logging is up while a bad value goes to stderr at once, a `--strict` mode wants to exit on everything but a deprecation, and the conformance corpus needs to pin what happened without pinning how it was said — the same line the argv corpus holds, where a vector names an error class and never a message. Seven kinds, one per thing this crate can report: a key nothing declares, a value the type cannot read, a value the choices do not allow, a place that may not set the setting, a setting deprecated, a value read as the setting that replaced it, and a value passed over because another name won. Plus `Other`, which is what a CLI's own layer says — a git or pkl layer complaining that it could not read its source is not something this crate should make it invent a kind for. Chained rather than an argument, so the constructors read as they did and the seven sites each gain one call. --- config/src/env.rs | 31 +++++++++++---- config/src/layer.rs | 91 +++++++++++++++++++++++++++++++++++++------ config/src/lib.rs | 2 +- config/src/resolve.rs | 86 +++++++++++++++++++++++++++++++++------- 4 files changed, 176 insertions(+), 34 deletions(-) diff --git a/config/src/env.rs b/config/src/env.rs index ecdb5a67a..a76663576 100644 --- a/config/src/env.rs +++ b/config/src/env.rs @@ -13,7 +13,7 @@ use std::collections::BTreeMap; use std::ffi::OsStr; -use crate::layer::{Layer, LayerCtx, LayerError, LayerOutput, Warning}; +use crate::layer::{Layer, LayerCtx, LayerError, LayerOutput, Warning, WarningKind}; use crate::registry::PropId; use crate::source::{Origin, SourceKind}; @@ -133,13 +133,16 @@ impl Layer for EnvLayer { for (id, set_as, raw) in candidates { let origin = Origin::new(SourceKind::ENV, set_as); if let Some(first) = read_by { - out.warn(Warning::at( - format!( - "{set_as} was not read: {first} also sets {}", - registry.get(target).key - ), - origin, - )); + out.warn( + Warning::at( + format!( + "{set_as} was not read: {first} also sets {}", + registry.get(target).key + ), + origin, + ) + .of(WarningKind::NotRead), + ); continue; } match ctx.entry(id, raw, origin) { @@ -388,6 +391,18 @@ mod tests { .any(|w| w.starts_with("HK_THREADS was not read: HK_CONCURRENCY also sets jobs")), "{warnings:?}" ); + // Three kinds at once, which is what living through a rename actually produces: the name + // that was passed over, and — for the one that *was* read — that it is deprecated and what + // it was read as. A variable passed over is its own sort of thing: nothing is wrong with the + // value, and a `--strict` mode that stops for a bad one should not stop for this. + assert_eq!( + resolved.warnings.iter().map(|w| w.kind).collect::>(), + vec![ + WarningKind::NotRead, + WarningKind::Deprecated, + WarningKind::Renamed + ] + ); // And the current name still beats both of them. let layer = env(&[ diff --git a/config/src/layer.rs b/config/src/layer.rs index 5dc88a3f8..7e0cf3905 100644 --- a/config/src/layer.rs +++ b/config/src/layer.rs @@ -42,27 +42,77 @@ impl Entry { /// Returned rather than printed. mise queues these until its logging is up, and a library /// that writes to stderr on its own cannot be used by anything that has an opinion about /// output. +/// +/// Built through [`Warning::new`] or [`Warning::at`] rather than as a literal: this has gained a +/// field once already, and a warning is something a layer *reports* rather than a shape anything +/// downstream should be pattern-matched against exhaustively. Reading the fields, and matching with +/// `..`, are unaffected. #[derive(Debug, Clone, PartialEq)] +#[non_exhaustive] pub struct Warning { pub message: String, /// Where the value that caused it came from, when there was one. pub origin: Option, + /// What sort of thing happened, for a caller that wants to treat them differently. + pub kind: WarningKind, +} + +/// The kinds of thing a resolution has to say. +/// +/// The message is for a person and its wording is nobody's contract; this is what a *program* can +/// act on. mise wants its deprecations queued and printed once its logging is up while a bad value +/// goes to stderr immediately; a `--strict` mode wants to exit on anything but a deprecation; the +/// conformance corpus wants to pin what happened without pinning how it was worded, since that is a +/// quality-of-implementation concern and differs between implementations by design. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum WarningKind { + /// A key no setting declares. A config file written for a newer binary read by an older one. + UnknownSetting, + /// A value the declared type cannot read. + WrongType, + /// A value the declared `choice` nodes do not allow. + NotAllowed, + /// A place that may not set this setting: a `scope="global"` setting from a checkout. + OutOfScope, + /// A setting whose spec says not to use it any more. + Deprecated, + /// A value that arrived under an old name and was read as the setting that replaced it. + Renamed, + /// A value that was passed over because another name for the same setting won. + NotRead, + /// Something a layer of the CLI's own says, which this crate has no name for. + #[default] + Other, } impl Warning { + /// A warning of no particular kind, which is what a custom layer's own complaints are. pub fn new(message: impl Into) -> Self { Self { message: message.into(), origin: None, + kind: WarningKind::Other, } } + /// The same, about a value that came from somewhere nameable. pub fn at(message: impl Into, origin: Origin) -> Self { Self { message: message.into(), origin: Some(origin), + kind: WarningKind::Other, } } + + /// This warning, classified. + /// + /// Chained rather than an argument so the two constructors keep reading as they did, and so a + /// layer that has nothing useful to say about the kind is not made to invent one. + pub fn of(mut self, kind: WarningKind) -> Self { + self.kind = kind; + self + } } /// What a layer found. @@ -186,7 +236,8 @@ impl LayerCtx { Err(Warning::at( format!("{key} expected {} but has `{}`", err.expected, err.found), origin, - )) + ) + .of(WarningKind::WrongType)) } } } @@ -203,14 +254,17 @@ impl LayerCtx { 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(), - )) + Some( + Warning::at( + format!( + "{key} expected one of {} but has `{}`", + meta.allowed(), + crate::value::shown(refused) + ), + origin.clone(), + ) + .of(WarningKind::NotAllowed), + ) } /// An entry for a dotted key, which is what a layer reading a file has in hand. @@ -221,7 +275,8 @@ impl LayerCtx { /// do, and the reason a deprecated key in somebody's config file gets reported at all. pub fn entry_for_key(&self, key: &str, raw: &str, origin: Origin) -> Result { let Some(found) = self.prop(key) else { - return Err(Warning::at(format!("unknown setting `{key}`"), origin)); + return Err(Warning::at(format!("unknown setting `{key}`"), origin) + .of(WarningKind::UnknownSetting)); }; let mut entry = self.entry(found.id, raw, origin)?; // `lookup` already folded, so `entry` had nothing left to fold and nothing to report; @@ -244,7 +299,8 @@ impl LayerCtx { origin: Origin, ) -> Result { let Some(found) = self.prop(key) else { - return Err(Warning::at(format!("unknown setting `{key}`"), origin)); + return Err(Warning::at(format!("unknown setting `{key}`"), origin) + .of(WarningKind::UnknownSetting)); }; let meta = self.registry.get(found.id); match meta.ty.coerce(value) { @@ -266,7 +322,8 @@ impl LayerCtx { err.found ), origin, - )), + ) + .of(WarningKind::WrongType)), } } } @@ -282,6 +339,7 @@ pub trait Layer { #[cfg(test)] mod tests { + use super::WarningKind; use super::*; use crate::registry::PropMeta; use crate::ty::{Parser, Ty}; @@ -348,6 +406,15 @@ mod tests { warning.message, "stash expected one of git, patch-file, none but has `svn`" ); + // Which is a different sort of thing from a value of the wrong *type*, and a caller that + // treats them differently needs to be able to tell. + assert_eq!(warning.kind, WarningKind::NotAllowed); + assert_eq!( + ctx.entry_for_key("jobs", "lots", origin.clone()) + .expect_err("not a number") + .kind, + WarningKind::WrongType + ); // And a value that is one of them is just a value. assert_eq!( ctx.entry_for_key("stash", "git", origin.clone()) diff --git a/config/src/lib.rs b/config/src/lib.rs index 129ab57ae..820e82c6d 100644 --- a/config/src/lib.rs +++ b/config/src/lib.rs @@ -71,7 +71,7 @@ pub use env::EnvLayer; pub use explain::explain; #[cfg(any(feature = "toml", feature = "json"))] pub use files::{FileLayer, Format}; -pub use layer::{Entry, Layer, LayerCtx, LayerError, LayerOutput, Warning}; +pub use layer::{Entry, Layer, LayerCtx, LayerError, LayerOutput, Warning, WarningKind}; pub use read::{Fold, FromValue, ReadError, ReadErrorKind, ReadErrors}; pub use registry::{Lookup, Merge, PropId, PropMeta, Registry, Scope}; pub use resolve::{resolve, Layers, Resolved}; diff --git a/config/src/resolve.rs b/config/src/resolve.rs index 4cbccf78b..f5077f806 100644 --- a/config/src/resolve.rs +++ b/config/src/resolve.rs @@ -11,7 +11,7 @@ use std::collections::BTreeMap; -use crate::layer::{Layer, LayerCtx, LayerError, Warning}; +use crate::layer::{Layer, LayerCtx, LayerError, Warning, WarningKind}; use crate::registry::{Merge, PropId, Registry, Scope}; use crate::source::{Origin, SourceKind, Trust}; use crate::value::Value; @@ -189,24 +189,30 @@ pub fn resolve(registry: Registry, layers: Layers<'_>) -> Result = resolved.warnings.iter().map(|w| w.kind).collect(); + assert_eq!( + kinds, + vec![ + WarningKind::OutOfScope, + WarningKind::Deprecated, + WarningKind::Renamed + ], + "{:?}", + resolved.warnings + ); + + // A layer of the CLI's own says whatever it likes, and is not made to invent a kind for it. + assert_eq!( + Warning::new("the git config could not be read").kind, + WarningKind::Other + ); + } + /// A layer holding whatever a test hands it. struct Fixed { kind: SourceKind,