From 48742e009c9c4c009821f806818eba0d717f3170 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Thu, 23 May 2024 15:27:52 +0800 Subject: [PATCH 1/2] test: Add 'missing field' test --- tests/testsuite/config.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 8cd86ae0af3..fc93d779b9e 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1863,3 +1863,34 @@ fn trim_paths_parsing() { let trim_paths: TomlTrimPaths = gctx.get("profile.dev.trim-paths").unwrap(); assert_eq!(trim_paths, expected, "failed to parse {val}"); } + +#[cargo_test] +fn missing_fields() { + #[derive(Deserialize, Default, Debug)] + struct Foo { + bar: Bar, + } + + #[derive(Deserialize, Default, Debug)] + struct Bar { + bax: bool, + baz: bool, + } + + let gctx = GlobalContextBuilder::new() + .env("CARGO_FOO_BAR_BAZ", "true") + .build(); + assert_error(gctx.get::("foo").unwrap_err(), "missing field `bax`"); + let gctx: GlobalContext = GlobalContextBuilder::new() + .env("CARGO_FOO_BAR_BAZ", "true") + .env("CARGO_FOO_BAR_BAX", "true") + .build(); + let foo = gctx.get::("foo").unwrap(); + assert_eq!(foo.bar.bax, true); + assert_eq!(foo.bar.baz, true); + + let gctx: GlobalContext = GlobalContextBuilder::new() + .config_arg("foo.bar.baz=true") + .build(); + assert_error(gctx.get::("foo").unwrap_err(), "missing field `bax`"); +} From e05d930e16085184658954e9e94e8f276d022b91 Mon Sep 17 00:00:00 2001 From: Lin Yihai Date: Thu, 23 May 2024 15:29:38 +0800 Subject: [PATCH 2/2] fix: Add more context when 'missing feild' --- src/cargo/util/context/de.rs | 32 ++++++++++++++++++++++--------- src/cargo/util/context/mod.rs | 36 +++++++++++++++++++++++++++++++++-- tests/testsuite/config.rs | 18 ++++++++++++++++-- tests/testsuite/progress.rs | 5 ++++- 4 files changed, 77 insertions(+), 14 deletions(-) diff --git a/src/cargo/util/context/de.rs b/src/cargo/util/context/de.rs index 18a84823bed..e84322422cb 100644 --- a/src/cargo/util/context/de.rs +++ b/src/cargo/util/context/de.rs @@ -35,7 +35,7 @@ macro_rules! deserialize_method { .ok_or_else(|| ConfigError::missing(&self.key))?; let Value { val, definition } = v; let res: Result = visitor.$visit(val); - res.map_err(|e| e.with_key_context(&self.key, definition)) + res.map_err(|e| e.with_key_context(&self.key, Some(definition))) } }; } @@ -60,7 +60,7 @@ impl<'de, 'gctx> de::Deserializer<'de> for Deserializer<'gctx> { CV::Boolean(b, def) => (visitor.visit_bool(b), def), }; let (res, def) = res; - return res.map_err(|e| e.with_key_context(&self.key, def)); + return res.map_err(|e| e.with_key_context(&self.key, Some(def))); } Err(ConfigError::missing(&self.key)) } @@ -178,7 +178,7 @@ impl<'de, 'gctx> de::Deserializer<'de> for Deserializer<'gctx> { let Value { val, definition } = value; visitor .visit_enum(val.into_deserializer()) - .map_err(|e: ConfigError| e.with_key_context(&self.key, definition)) + .map_err(|e: ConfigError| e.with_key_context(&self.key, Some(definition))) } // These aren't really supported, yet. @@ -345,11 +345,25 @@ impl<'de, 'gctx> de::MapAccess<'de> for ConfigMapAccess<'gctx> { field.replace('-', "_").starts_with(&env_prefix) }); - let result = seed.deserialize(Deserializer { - gctx: self.de.gctx, - key: self.de.key.clone(), - env_prefix_ok, - }); + let result = seed + .deserialize(Deserializer { + gctx: self.de.gctx, + key: self.de.key.clone(), + env_prefix_ok, + }) + .map_err(|e| { + if !e.is_missing_field() { + return e; + } + e.with_key_context( + &self.de.key, + self.de + .gctx + .get_cv_with_env(&self.de.key) + .ok() + .and_then(|cv| cv.map(|cv| cv.get_definition().clone())), + ) + }); self.de.key.pop(); result } @@ -486,7 +500,7 @@ impl<'de, 'gctx> de::MapAccess<'de> for ValueDeserializer<'gctx> { if let Some(de) = &self.de { return seed .deserialize(de.clone()) - .map_err(|e| e.with_key_context(&de.key, self.definition.clone())); + .map_err(|e| e.with_key_context(&de.key, Some(self.definition.clone()))); } else { return seed .deserialize(self.str_value.as_ref().unwrap().clone().into_deserializer()); diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 914d57f2138..2ce4f2ace56 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -2030,6 +2030,10 @@ impl ConfigError { } } + fn is_missing_field(&self) -> bool { + self.error.downcast_ref::().is_some() + } + fn missing(key: &ConfigKey) -> ConfigError { ConfigError { error: anyhow!("missing config key `{}`", key), @@ -2037,11 +2041,11 @@ impl ConfigError { } } - fn with_key_context(self, key: &ConfigKey, definition: Definition) -> ConfigError { + fn with_key_context(self, key: &ConfigKey, definition: Option) -> ConfigError { ConfigError { error: anyhow::Error::from(self) .context(format!("could not load config key `{}`", key)), - definition: Some(definition), + definition: definition, } } } @@ -2062,6 +2066,17 @@ impl fmt::Display for ConfigError { } } +#[derive(Debug)] +struct MissingField(String); + +impl fmt::Display for MissingField { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "missing field `{}`", self.0) + } +} + +impl std::error::Error for MissingField {} + impl serde::de::Error for ConfigError { fn custom(msg: T) -> Self { ConfigError { @@ -2069,6 +2084,13 @@ impl serde::de::Error for ConfigError { definition: None, } } + + fn missing_field(field: &'static str) -> Self { + ConfigError { + error: anyhow::Error::new(MissingField(field.to_string())), + definition: None, + } + } } impl From for ConfigError { @@ -2111,6 +2133,16 @@ impl fmt::Debug for ConfigValue { } impl ConfigValue { + fn get_definition(&self) -> &Definition { + match self { + CV::Boolean(_, def) + | CV::Integer(_, def) + | CV::String(_, def) + | CV::List(_, def) + | CV::Table(_, def) => def, + } + } + fn from_toml(def: Definition, toml: toml::Value) -> CargoResult { match toml { toml::Value::String(val) => Ok(CV::String(val, def)), diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index fc93d779b9e..6f0b262dbc5 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -1880,7 +1880,14 @@ fn missing_fields() { let gctx = GlobalContextBuilder::new() .env("CARGO_FOO_BAR_BAZ", "true") .build(); - assert_error(gctx.get::("foo").unwrap_err(), "missing field `bax`"); + assert_error( + gctx.get::("foo").unwrap_err(), + "\ +could not load config key `foo.bar` + +Caused by: + missing field `bax`", + ); let gctx: GlobalContext = GlobalContextBuilder::new() .env("CARGO_FOO_BAR_BAZ", "true") .env("CARGO_FOO_BAR_BAX", "true") @@ -1892,5 +1899,12 @@ fn missing_fields() { let gctx: GlobalContext = GlobalContextBuilder::new() .config_arg("foo.bar.baz=true") .build(); - assert_error(gctx.get::("foo").unwrap_err(), "missing field `bax`"); + assert_error( + gctx.get::("foo").unwrap_err(), + "\ +error in --config cli option: could not load config key `foo.bar` + +Caused by: + missing field `bax`", + ); } diff --git a/tests/testsuite/progress.rs b/tests/testsuite/progress.rs index 0248a040d57..ffc00d50d43 100644 --- a/tests/testsuite/progress.rs +++ b/tests/testsuite/progress.rs @@ -70,7 +70,10 @@ fn bad_progress_config_missing_when() { .with_status(101) .with_stderr( "\ -error: missing field `when` +error: error in [..]: could not load config key `term.progress` + +Caused by: + missing field `when` ", ) .run();