Auto merge of #12574 - dtolnay-contrib:untagged, r=epage

Improve deserialization errors of untagged enums ### What does this PR try to resolve? ```toml # .cargo/config.toml [http] ssl-version.min = false ``` **Before:** ```console $ cargo check error: data did not match any variant of untagged enum SslVersionConfig ``` **After:** ```console $ cargo check error: error in /path/to/.cargo/config.toml: could not load config key `http.ssl-version` Caused by: error in /path/to/.cargo/config.toml: `http.ssl-version.min` expected a string, but found a boolean ``` ### How should we test and review this PR? The first commit adds tests showing the pre-existing error messages — mostly just _"data did not match any variant of untagged enum T"_ with no location information. The second commit replaces all `#[derive(Deserialize)] #[serde(untagged)]` with Deserialize impls based on https://docs.rs/serde-untagged/0.1, showing the effect on the error messages. Tested with `cargo test`, and by handwriting some bad .cargo/config.toml files and looking at the error produced by `rust-lang/cargo/target/release/cargo check`.
rust-lang · Aug 28, 2023 · 97e73c7 · 97e73c7
2 parents bfa04bb + 3871aec
commit 97e73c7
Show file tree

Hide file tree

Showing 6 changed files with 266 additions and 15 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -77,6 +77,7 @@ same-file = "1.0.6"
 security-framework = "2.9.2"
 semver = { version = "1.0.18", features = ["serde"] }
 serde = "1.0.188"
+serde-untagged = "0.1.0"
 serde-value = "0.7.0"
 serde_ignored = "0.1.9"
 serde_json = "1.0.104"
@@ -163,6 +164,7 @@ rand.workspace = true
 rustfix.workspace = true
 semver.workspace = true
 serde = { workspace = true, features = ["derive"] }
+serde-untagged.workspace = true
 serde-value.workspace = true
 serde_ignored.workspace = true
 serde_json = { workspace = true, features = ["raw_value"] }

diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs
@@ -84,6 +84,7 @@ use curl::easy::Easy;
 use lazycell::LazyCell;
 use serde::de::IntoDeserializer as _;
 use serde::Deserialize;
+use serde_untagged::UntaggedEnumVisitor;
 use time::OffsetDateTime;
 use toml_edit::Item;
 use url::Url;
@@ -2453,13 +2454,24 @@ impl CargoFutureIncompatConfig {
 /// ssl-version.min = "tlsv1.2"
 /// ssl-version.max = "tlsv1.3"
 /// ```
-#[derive(Clone, Debug, Deserialize, PartialEq)]
-#[serde(untagged)]
+#[derive(Clone, Debug, PartialEq)]
 pub enum SslVersionConfig {
     Single(String),
     Range(SslVersionConfigRange),
 }
 
+impl<'de> Deserialize<'de> for SslVersionConfig {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        UntaggedEnumVisitor::new()
+            .string(|single| Ok(SslVersionConfig::Single(single.to_owned())))
+            .map(|map| map.deserialize().map(SslVersionConfig::Range))
+            .deserialize(deserializer)
+    }
+}
+
 #[derive(Clone, Debug, Deserialize, PartialEq)]
 pub struct SslVersionConfigRange {
     pub min: Option<String>,
@@ -2493,13 +2505,24 @@ pub struct CargoSshConfig {
 /// [build]
 /// jobs = "default" # Currently only support "default".
 /// ```
-#[derive(Debug, Deserialize, Clone)]
-#[serde(untagged)]
+#[derive(Debug, Clone)]
 pub enum JobsConfig {
     Integer(i32),
     String(String),
 }
 
+impl<'de> Deserialize<'de> for JobsConfig {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        UntaggedEnumVisitor::new()
+            .i32(|int| Ok(JobsConfig::Integer(int)))
+            .string(|string| Ok(JobsConfig::String(string.to_owned())))
+            .deserialize(deserializer)
+    }
+}
+
 #[derive(Debug, Deserialize)]
 #[serde(rename_all = "kebab-case")]
 pub struct CargoBuildConfig {
@@ -2534,13 +2557,24 @@ pub struct BuildTargetConfig {
     inner: Value<BuildTargetConfigInner>,
 }
 
-#[derive(Debug, Deserialize)]
-#[serde(untagged)]
+#[derive(Debug)]
 enum BuildTargetConfigInner {
     One(String),
     Many(Vec<String>),
 }
 
+impl<'de> Deserialize<'de> for BuildTargetConfigInner {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        UntaggedEnumVisitor::new()
+            .string(|one| Ok(BuildTargetConfigInner::One(one.to_owned())))
+            .seq(|many| many.deserialize().map(BuildTargetConfigInner::Many))
+            .deserialize(deserializer)
+    }
+}
+
 impl BuildTargetConfig {
     /// Gets values of `build.target` as a list of strings.
     pub fn values(&self, config: &Config) -> CargoResult<Vec<String>> {
@@ -2652,19 +2686,44 @@ where
     deserializer.deserialize_option(ProgressVisitor)
 }
 
-#[derive(Debug, Deserialize)]
-#[serde(untagged)]
+#[derive(Debug)]
 enum EnvConfigValueInner {
     Simple(String),
     WithOptions {
         value: String,
-        #[serde(default)]
         force: bool,
-        #[serde(default)]
         relative: bool,
     },
 }
 
+impl<'de> Deserialize<'de> for EnvConfigValueInner {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        #[derive(Deserialize)]
+        struct WithOptions {
+            value: String,
+            #[serde(default)]
+            force: bool,
+            #[serde(default)]
+            relative: bool,
+        }
+
+        UntaggedEnumVisitor::new()
+            .string(|simple| Ok(EnvConfigValueInner::Simple(simple.to_owned())))
+            .map(|map| {
+                let with_options: WithOptions = map.deserialize()?;
+                Ok(EnvConfigValueInner::WithOptions {
+                    value: with_options.value,
+                    force: with_options.force,
+                    relative: with_options.relative,
+                })
+            })
+            .deserialize(deserializer)
+    }
+}
+
 #[derive(Debug, Deserialize)]
 #[serde(transparent)]
 pub struct EnvConfigValue {

diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs
@@ -12,9 +12,10 @@ use cargo_util::paths;
 use itertools::Itertools;
 use lazycell::LazyCell;
 use semver::{self, VersionReq};
-use serde::de::{self, Unexpected};
+use serde::de::{self, IntoDeserializer as _, Unexpected};
 use serde::ser;
 use serde::{Deserialize, Serialize};
+use serde_untagged::UntaggedEnumVisitor;
 use tracing::{debug, trace};
 use url::Url;
 
@@ -961,13 +962,25 @@ impl StringOrVec {
     }
 }
 
-#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
-#[serde(untagged, expecting = "expected a boolean or a string")]
+#[derive(Clone, Debug, Serialize, Eq, PartialEq)]
+#[serde(untagged)]
 pub enum StringOrBool {
     String(String),
     Bool(bool),
 }
 
+impl<'de> Deserialize<'de> for StringOrBool {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: de::Deserializer<'de>,
+    {
+        UntaggedEnumVisitor::new()
+            .string(|s| Ok(StringOrBool::String(s.to_owned())))
+            .bool(|b| Ok(StringOrBool::Bool(b)))
+            .deserialize(deserializer)
+    }
+}
+
 #[derive(PartialEq, Clone, Debug, Serialize)]
 #[serde(untagged)]
 pub enum VecStringOrBool {
@@ -3513,13 +3526,27 @@ pub type TomlLints = BTreeMap<String, TomlToolLints>;
 
 pub type TomlToolLints = BTreeMap<String, TomlLint>;
 
-#[derive(Serialize, Deserialize, Debug, Clone)]
+#[derive(Serialize, Debug, Clone)]
 #[serde(untagged)]
 pub enum TomlLint {
     Level(TomlLintLevel),
     Config(TomlLintConfig),
 }
 
+impl<'de> Deserialize<'de> for TomlLint {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: de::Deserializer<'de>,
+    {
+        UntaggedEnumVisitor::new()
+            .string(|string| {
+                TomlLintLevel::deserialize(string.into_deserializer()).map(TomlLint::Level)
+            })
+            .map(|map| map.deserialize().map(TomlLint::Config))
+            .deserialize(deserializer)
+    }
+}
+
 impl TomlLint {
     fn level(&self) -> TomlLintLevel {
         match self {

diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs
@@ -1392,7 +1392,7 @@ Caused by:
     |
   6 |                 build = 3
     |                         ^
-  expected a boolean or a string
+  invalid type: integer `3`, expected a boolean or string
 ",
         )
         .run();
@@ -1420,6 +1420,117 @@ fn warn_semver_metadata() {
         .run();
 }
 
+#[cargo_test]
+fn bad_http_ssl_version() {
+    // Invalid type in SslVersionConfig.
+    let p = project()
+        .file(
+            ".cargo/config.toml",
+            r#"
+            [http]
+            ssl-version = ["tlsv1.2", "tlsv1.3"]
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_status(101)
+        .with_stderr(
+            "\
+[ERROR] error in [..]/config.toml: could not load config key `http.ssl-version`
+
+Caused by:
+  invalid type: sequence, expected a string or map
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn bad_http_ssl_version_range() {
+    // Invalid type in SslVersionConfigRange.
+    let p = project()
+        .file(
+            ".cargo/config.toml",
+            r#"
+            [http]
+            ssl-version.min = false
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_status(101)
+        .with_stderr(
+            "\
+[ERROR] error in [..]/config.toml: could not load config key `http.ssl-version`
+
+Caused by:
+  error in [..]/config.toml: `http.ssl-version.min` expected a string, but found a boolean
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn bad_build_jobs() {
+    // Invalid type in JobsConfig.
+    let p = project()
+        .file(
+            ".cargo/config.toml",
+            r#"
+            [build]
+            jobs = { default = true }
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_status(101)
+        .with_stderr(
+            "\
+[ERROR] error in [..]/config.toml: could not load config key `build.jobs`
+
+Caused by:
+  invalid type: map, expected an integer or string
+",
+        )
+        .run();
+}
+
+#[cargo_test]
+fn bad_build_target() {
+    // Invalid type in BuildTargetConfig.
+    let p = project()
+        .file(
+            ".cargo/config.toml",
+            r#"
+            [build]
+            target.'cfg(unix)' = "x86_64"
+            "#,
+        )
+        .file("src/lib.rs", "")
+        .build();
+
+    p.cargo("check")
+        .with_status(101)
+        .with_stderr(
+            "\
+[ERROR] error in [..]/config.toml: could not load config key `build.target`
+
+Caused by:
+  error in [..]/config.toml: could not load config key `build.target`
+
+Caused by:
+  invalid type: map, expected a string or array
+",
+        )
+        .run();
+}
+
 #[cargo_test]
 fn bad_target_cfg() {
     // Invalid type in a StringList.