Repository: jonasbb/serde_with
Pull request #586 (2023-04-02 10:50:13)

Improve error messaged by dropping untagged enums
Untagged enums do not provide good error messages and likely never will,
given that there are multiple PRs which are just completely ignored
([serde#2376](https://github.com/serde-rs/serde/pull/2376) and
[serde#1544](https://github.com/serde-rs/serde/pull/1544)).

Instead using `content::de` the untagged enums can be replaced by custom
buffering. The error messages for `OneOrMany` and `PickFirst` now look
like this, including the original failure for each variant.

```text
OneOrMany could not deserialize any variant:
  One: invalid type: map, expected u32
  Many: invalid type: map, expected a sequence
```

```text
PickFirst could not deserialize any variant:
  First: invalid type: string "Abc", expected u32
  Second: invalid digit found in string
```

The implementations of `VecSkipError` and `DefaultOnError` are updated
too, but should not result in any visible changes.


Patch:
diff --git a/serde_with/CHANGELOG.md b/serde_with/CHANGELOG.md
index 23a7f44..7634637 100644
--- a/serde_with/CHANGELOG.md
+++ b/serde_with/CHANGELOG.md
@@ -7,6 +7,26 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
+### Changed
+
+* Improve the error message when deserializing `OneOrMany` or `PickFirst` fails.
+    It now includes the original error message for each of the individual variants.
+    This is possible by dropping untagged enums as the internal implementations, since they will likely never support this, as these old PRs show [serde#2376](https://github.com/serde-rs/serde/pull/2376) and [serde#1544](https://github.com/serde-rs/serde/pull/1544).
+
+    The new errors look like:
+
+    ```text
+    OneOrMany could not deserialize any variant:
+      One: invalid type: map, expected u32
+      Many: invalid type: map, expected a sequence
+    ```
+
+    ```text
+    PickFirst could not deserialize any variant:
+      First: invalid type: string "Abc", expected u32
+      Second: invalid digit found in string
+    ```
+
 ## [2.3.1] - 2023-03-10
 
 ### Fixed
diff --git a/serde_with/Cargo.toml b/serde_with/Cargo.toml
index 969dd04..895aaa2 100644
--- a/serde_with/Cargo.toml
+++ b/serde_with/Cargo.toml
@@ -65,6 +65,8 @@ chrono_0_4 = {package = "chrono", version = "0.4.20", optional = true, default-f
 doc-comment = {version = "0.3.3", optional = true}
 hex = {version = "0.4.3", optional = true, default-features = false}
 indexmap_1 = {package = "indexmap", version = "1.8", optional = true, default-features = false, features = ["serde-1"]}
+# The derive feature is needed for the flattened_maybe macro.
+# https://github.com/jonasbb/serde_with/blob/eb1965a74a3be073ecd13ec05f02a01bc1c44309/serde_with/src/flatten_maybe.rs#L67
 serde = {version = "1.0.122", default-features = false, features = ["derive"]}
 serde_json = {version = "1.0.45", optional = true, default-features = false}
 serde_with_macros = {path = "../serde_with_macros", version = "=2.3.1", optional = true}
diff --git a/serde_with/src/de/impls.rs b/serde_with/src/de/impls.rs
index 92fbf83..066ad57 100644
--- a/serde_with/src/de/impls.rs
+++ b/serde_with/src/de/impls.rs
@@ -842,22 +842,32 @@ where
     where
         D: Deserializer<'de>,
     {
-        #[derive(serde::Deserialize)]
-        #[serde(
-            untagged,
-            bound(deserialize = "DeserializeAsWrap<T, TAs>: Deserialize<'de>")
-        )]
-        enum GoodOrError<'a, T, TAs>
+        enum GoodOrError<T, TAs> {
+            Good(T),
+            // Only here to consume the TAs generic
+            Error(PhantomData<TAs>),
+        }
+
+        impl<'de, T, TAs> Deserialize<'de> for GoodOrError<T, TAs>
         where
-            TAs: DeserializeAs<'a, T>,
+            TAs: DeserializeAs<'de, T>,
         {
-            Good(DeserializeAsWrap<T, TAs>),
-            // This consumes one "item" when `T` errors while deserializing.
-            // This is necessary to make this work, when instead of having a direct value
-            // like integer or string, the deserializer sees a list or map.
-            Error(IgnoredAny),
-            #[serde(skip)]
-            _JustAMarkerForTheLifetime(PhantomData<&'a u32>),
+            fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+            where
+                D: Deserializer<'de>,
+            {
+                let is_hr = deserializer.is_human_readable();
+                let content: content::de::Content<'de> = Deserialize::deserialize(deserializer)?;
+
+                Ok(
+                    match <DeserializeAsWrap<T, TAs>>::deserialize(
+                        content::de::ContentDeserializer::<D::Error>::new(content, is_hr),
+                    ) {
+                        Ok(elem) => GoodOrError::Good(elem.into_inner()),
+                        Err(_) => GoodOrError::Error(PhantomData),
+                    },
+                )
+            }
         }
 
         struct SeqVisitor<T, U> {
@@ -865,9 +875,9 @@ where
             marker2: PhantomData<U>,
         }
 
-        impl<'de, T, U> Visitor<'de> for SeqVisitor<T, U>
+        impl<'de, T, TAs> Visitor<'de> for SeqVisitor<T, TAs>
         where
-            U: DeserializeAs<'de, T>,
+            TAs: DeserializeAs<'de, T>,
         {
             type Value = Vec<T>;
 
@@ -875,18 +885,17 @@ where
                 formatter.write_str("a sequence")
             }
 
-            fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
+            fn visit_seq<A>(self, seq: A) -> Result<Self::Value, A::Error>
             where
                 A: SeqAccess<'de>,
             {
-                let mut values = Vec::with_capacity(seq.size_hint().unwrap_or_default());
-
-                while let Some(value) = seq.next_element()? {
-                    if let GoodOrError::<T, U>::Good(value) = value {
-                        values.push(value.into_inner());
-                    }
-                }
-                Ok(values)
+                utils::SeqIter::new(seq)
+                    .filter_map(|res: Result<GoodOrError<T, TAs>, A::Error>| match res {
+                        Ok(GoodOrError::Good(value)) => Some(Ok(value)),
+                        Ok(GoodOrError::Error(_)) => None,
+                        Err(err) => Some(Err(err)),
+                    })
+                    .collect()
             }
         }
 
@@ -952,28 +961,21 @@ where
     where
         D: Deserializer<'de>,
     {
-        #[derive(serde::Deserialize)]
-        #[serde(
-            untagged,
-            bound(deserialize = "DeserializeAsWrap<T, TAs>: Deserialize<'de>")
-        )]
-        enum GoodOrError<'a, T, TAs>
-        where
-            TAs: DeserializeAs<'a, T>,
-        {
-            Good(DeserializeAsWrap<T, TAs>),
-            // This consumes one "item" when `T` errors while deserializing.
-            // This is necessary to make this work, when instead of having a direct value
-            // like integer or string, the deserializer sees a list or map.
-            Error(IgnoredAny),
-            #[serde(skip)]
-            _JustAMarkerForTheLifetime(PhantomData<&'a u32>),
-        }
+        let is_hr = deserializer.is_human_readable();
+        let content: content::de::Content<'de> = match Deserialize::deserialize(deserializer) {
+            Ok(content) => content,
+            Err(_) => return Ok(Default::default()),
+        };
 
-        Ok(match Deserialize::deserialize(deserializer) {
-            Ok(GoodOrError::<T, TAs>::Good(res)) => res.into_inner(),
-            _ => Default::default(),
-        })
+        Ok(
+            match <DeserializeAsWrap<T, TAs>>::deserialize(content::de::ContentDeserializer::<
+                D::Error,
+            >::new(content, is_hr))
+            {
+                Ok(elem) => elem.into_inner(),
+                Err(_) => Default::default(),
+            },
+        )
     }
 }
 
@@ -1522,38 +1524,34 @@ impl<'de, const N: usize> DeserializeAs<'de, Box<[u8; N]>> for Bytes {
 }
 
 #[cfg(feature = "alloc")]
-impl<'de, T, U, FORMAT> DeserializeAs<'de, Vec<T>> for OneOrMany<U, FORMAT>
+impl<'de, T, TAs, FORMAT> DeserializeAs<'de, Vec<T>> for OneOrMany<TAs, FORMAT>
 where
-    U: DeserializeAs<'de, T>,
+    TAs: DeserializeAs<'de, T>,
     FORMAT: Format,
 {
     fn deserialize_as<D>(deserializer: D) -> Result<Vec<T>, D::Error>
     where
         D: Deserializer<'de>,
     {
-        #[derive(serde::Deserialize)]
-        #[serde(
-            untagged,
-            bound(deserialize = r#"DeserializeAsWrap<T, U>: Deserialize<'de>,
-                DeserializeAsWrap<Vec<T>, Vec<U>>: Deserialize<'de>"#),
-            expecting = "a list or single element"
-        )]
-        enum Helper<'a, T, U>
-        where
-            U: DeserializeAs<'a, T>,
-        {
-            One(DeserializeAsWrap<T, U>),
-            Many(DeserializeAsWrap<Vec<T>, Vec<U>>),
-            #[serde(skip)]
-            _JustAMarkerForTheLifetime(PhantomData<&'a u32>),
-        }
-
-        let h: Helper<'de, T, U> = Deserialize::deserialize(deserializer)?;
-        match h {
-            Helper::One(one) => Ok(alloc::vec![one.into_inner()]),
-            Helper::Many(many) => Ok(many.into_inner()),
-            Helper::_JustAMarkerForTheLifetime(_) => unreachable!(),
-        }
+        let is_hr = deserializer.is_human_readable();
+        let content: content::de::Content<'de> = Deserialize::deserialize(deserializer)?;
+
+        let one_err: D::Error = match <DeserializeAsWrap<T, TAs>>::deserialize(
+            content::de::ContentRefDeserializer::new(&content, is_hr),
+        ) {
+            Ok(one) => return Ok(alloc::vec![one.into_inner()]),
+            Err(err) => err,
+        };
+        let many_err: D::Error = match <DeserializeAsWrap<Vec<T>, Vec<TAs>>>::deserialize(
+            content::de::ContentDeserializer::new(content, is_hr),
+        ) {
+            Ok(many) => return Ok(many.into_inner()),
+            Err(err) => err,
+        };
+        Err(DeError::custom(format_args!(
+            "OneOrMany could not deserialize any variant:\n  One: {}\n  Many: {}",
+            one_err, many_err
+        )))
     }
 }
 
@@ -1580,32 +1578,25 @@ where
     where
         D: Deserializer<'de>,
     {
-        #[derive(serde::Deserialize)]
-        #[serde(
-            untagged,
-            bound(deserialize = r#"
-                DeserializeAsWrap<T, TAs1>: Deserialize<'de>,
-                DeserializeAsWrap<T, TAs2>: Deserialize<'de>,
-            "#),
-            expecting = "PickFirst could not deserialize data"
-        )]
-        enum Helper<'a, T, TAs1, TAs2>
-        where
-            TAs1: DeserializeAs<'a, T>,
-            TAs2: DeserializeAs<'a, T>,
-        {
-            First(DeserializeAsWrap<T, TAs1>),
-            Second(DeserializeAsWrap<T, TAs2>),
-            #[serde(skip)]
-            _JustAMarkerForTheLifetime(PhantomData<&'a u32>),
-        }
-
-        let h: Helper<'de, T, TAs1, TAs2> = Deserialize::deserialize(deserializer)?;
-        match h {
-            Helper::First(first) => Ok(first.into_inner()),
-            Helper::Second(second) => Ok(second.into_inner()),
-            Helper::_JustAMarkerForTheLifetime(_) => unreachable!(),
-        }
+        let is_hr = deserializer.is_human_readable();
+        let content: content::de::Content<'de> = Deserialize::deserialize(deserializer)?;
+
+        let first_err: D::Error = match <DeserializeAsWrap<T, TAs1>>::deserialize(
+            content::de::ContentRefDeserializer::new(&content, is_hr),
+        ) {
+            Ok(first) => return Ok(first.into_inner()),
+            Err(err) => err,
+        };
+        let second_err: D::Error = match <DeserializeAsWrap<T, TAs2>>::deserialize(
+            content::de::ContentDeserializer::new(content, is_hr),
+        ) {
+            Ok(second) => return Ok(second.into_inner()),
+            Err(err) => err,
+        };
+        Err(DeError::custom(format_args!(
+            "PickFirst could not deserialize any variant:\n  First: {}\n  Second: {}",
+            first_err, second_err
+        )))
     }
 }
 
@@ -1620,36 +1611,31 @@ where
     where
         D: Deserializer<'de>,
     {
-        #[derive(serde::Deserialize)]
-        #[serde(
-            untagged,
-            bound(deserialize = r#"
-                DeserializeAsWrap<T, TAs1>: Deserialize<'de>,
-                DeserializeAsWrap<T, TAs2>: Deserialize<'de>,
-                DeserializeAsWrap<T, TAs3>: Deserialize<'de>,
-            "#),
-            expecting = "PickFirst could not deserialize data"
-        )]
-        enum Helper<'a, T, TAs1, TAs2, TAs3>
-        where
-            TAs1: DeserializeAs<'a, T>,
-            TAs2: DeserializeAs<'a, T>,
-            TAs3: DeserializeAs<'a, T>,
-        {
-            First(DeserializeAsWrap<T, TAs1>),
-            Second(DeserializeAsWrap<T, TAs2>),
-            Third(DeserializeAsWrap<T, TAs3>),
-            #[serde(skip)]
-            _JustAMarkerForTheLifetime(PhantomData<&'a u32>),
-        }
-
-        let h: Helper<'de, T, TAs1, TAs2, TAs3> = Deserialize::deserialize(deserializer)?;
-        match h {
-            Helper::First(first) => Ok(first.into_inner()),
-            Helper::Second(second) => Ok(second.into_inner()),
-            Helper::Third(third) => Ok(third.into_inner()),
-            Helper::_JustAMarkerForTheLifetime(_) => unreachable!(),
-        }
+        let is_hr = deserializer.is_human_readable();
+        let content: content::de::Content<'de> = Deserialize::deserialize(deserializer)?;
+
+        let first_err: D::Error = match <DeserializeAsWrap<T, TAs1>>::deserialize(
+            content::de::ContentRefDeserializer::new(&content, is_hr),
+        ) {
+            Ok(first) => return Ok(first.into_inner()),
+            Err(err) => err,
+        };
+        let second_err: D::Error = match <DeserializeAsWrap<T, TAs2>>::deserialize(
+            content::de::ContentRefDeserializer::new(&content, is_hr),
+        ) {
+            Ok(second) => return Ok(second.into_inner()),
+            Err(err) => err,
+        };
+        let third_err: D::Error = match <DeserializeAsWrap<T, TAs3>>::deserialize(
+            content::de::ContentDeserializer::new(content, is_hr),
+        ) {
+            Ok(third) => return Ok(third.into_inner()),
+            Err(err) => err,
+        };
+        Err(DeError::custom(format_args!(
+            "PickFirst could not deserialize any variant:\n  First: {}\n  Second: {}\n  Third: {}",
+            first_err, second_err, third_err,
+        )))
     }
 }
 
@@ -1665,40 +1651,37 @@ where
     where
         D: Deserializer<'de>,
     {
-        #[derive(serde::Deserialize)]
-        #[serde(
-            untagged,
-            bound(deserialize = r#"
-                DeserializeAsWrap<T, TAs1>: Deserialize<'de>,
-                DeserializeAsWrap<T, TAs2>: Deserialize<'de>,
-                DeserializeAsWrap<T, TAs3>: Deserialize<'de>,
-                DeserializeAsWrap<T, TAs4>: Deserialize<'de>,
-            "#),
-            expecting = "PickFirst could not deserialize data"
-        )]
-        enum Helper<'a, T, TAs1, TAs2, TAs3, TAs4>
-        where
-            TAs1: DeserializeAs<'a, T>,
-            TAs2: DeserializeAs<'a, T>,
-            TAs3: DeserializeAs<'a, T>,
-            TAs4: DeserializeAs<'a, T>,
-        {
-            First(DeserializeAsWrap<T, TAs1>),
-            Second(DeserializeAsWrap<T, TAs2>),
-            Third(DeserializeAsWrap<T, TAs3>),
-            Forth(DeserializeAsWrap<T, TAs4>),
-            #[serde(skip)]
-            _JustAMarkerForTheLifetime(PhantomData<&'a u32>),
-        }
-
-        let h: Helper<'de, T, TAs1, TAs2, TAs3, TAs4> = Deserialize::deserialize(deserializer)?;
-        match h {
-            Helper::First(first) => Ok(first.into_inner()),
-            Helper::Second(second) => Ok(second.into_inner()),
-            Helper::Third(third) => Ok(third.into_inner()),
-            Helper::Forth(forth) => Ok(forth.into_inner()),
-            Helper::_JustAMarkerForTheLifetime(_) => unreachable!(),
-        }
+        let is_hr = deserializer.is_human_readable();
+        let content: content::de::Content<'de> = Deserialize::deserialize(deserializer)?;
+
+        let first_err: D::Error = match <DeserializeAsWrap<T, TAs1>>::deserialize(
+            content::de::ContentRefDeserializer::new(&content, is_hr),
+        ) {
+            Ok(first) => return Ok(first.into_inner()),
+            Err(err) => err,
+        };
+        let second_err: D::Error = match <DeserializeAsWrap<T, TAs2>>::deserialize(
+            content::de::ContentRefDeserializer::new(&content, is_hr),
+        ) {
+            Ok(second) => return Ok(second.into_inner()),
+            Err(err) => err,
+        };
+        let third_err: D::Error = match <DeserializeAsWrap<T, TAs3>>::deserialize(
+            content::de::ContentRefDeserializer::new(&content, is_hr),
+        ) {
+            Ok(third) => return Ok(third.into_inner()),
+            Err(err) => err,
+        };
+        let fourth_err: D::Error = match <DeserializeAsWrap<T, TAs4>>::deserialize(
+            content::de::ContentDeserializer::new(content, is_hr),
+        ) {
+            Ok(fourth) => return Ok(fourth.into_inner()),
+            Err(err) => err,
+        };
+        Err(DeError::custom(format_args!(
+            "PickFirst could not deserialize any variant:\n  First: {}\n  Second: {}\n  Third: {}\n  Fourth: {}",
+            first_err, second_err, third_err, fourth_err,
+        )))
     }
 }
 


Test patch:
diff --git a/serde_with/tests/serde_as/lib.rs b/serde_with/tests/serde_as/lib.rs
index 6209312..122c02f 100644
--- a/serde_with/tests/serde_as/lib.rs
+++ b/serde_with/tests/serde_as/lib.rs
@@ -844,8 +844,20 @@ fn test_one_or_many_prefer_one() {
     );
     check_deserialization(S1Vec(vec![1]), r#"1"#);
     check_deserialization(S1Vec(vec![1]), r#"[1]"#);
-    check_error_deserialization::<S1Vec>(r#"{}"#, expect![[r#"a list or single element"#]]);
-    check_error_deserialization::<S1Vec>(r#""xx""#, expect![[r#"a list or single element"#]]);
+    check_error_deserialization::<S1Vec>(
+        r#"{}"#,
+        expect![[r#"
+        OneOrMany could not deserialize any variant:
+          One: invalid type: map, expected u32
+          Many: invalid type: map, expected a sequence"#]],
+    );
+    check_error_deserialization::<S1Vec>(
+        r#""xx""#,
+        expect![[r#"
+        OneOrMany could not deserialize any variant:
+          One: invalid type: string "xx", expected u32
+          Many: invalid type: string "xx", expected a sequence"#]],
+    );
 
     #[serde_as]
     #[derive(Debug, Serialize, Deserialize, PartialEq)]
@@ -865,8 +877,20 @@ fn test_one_or_many_prefer_one() {
     );
     check_deserialization(S2Vec(vec![1]), r#""1""#);
     check_deserialization(S2Vec(vec![1]), r#"["1"]"#);
-    check_error_deserialization::<S2Vec>(r#"{}"#, expect![[r#"a list or single element"#]]);
-    check_error_deserialization::<S2Vec>(r#""xx""#, expect![[r#"a list or single element"#]]);
+    check_error_deserialization::<S2Vec>(
+        r#"{}"#,
+        expect![[r#"
+        OneOrMany could not deserialize any variant:
+          One: invalid type: map, expected a string
+          Many: invalid type: map, expected a sequence"#]],
+    );
+    check_error_deserialization::<S2Vec>(
+        r#""xx""#,
+        expect![[r#"
+        OneOrMany could not deserialize any variant:
+          One: invalid digit found in string
+          Many: invalid type: string "xx", expected a sequence"#]],
+    );
 }
 
 #[test]
@@ -897,8 +921,20 @@ fn test_one_or_many_prefer_many() {
     );
     check_deserialization(S1Vec(vec![1]), r#"1"#);
     check_deserialization(S1Vec(vec![1]), r#"[1]"#);
-    check_error_deserialization::<S1Vec>(r#"{}"#, expect![[r#"a list or single element"#]]);
-    check_error_deserialization::<S1Vec>(r#""xx""#, expect![[r#"a list or single element"#]]);
+    check_error_deserialization::<S1Vec>(
+        r#"{}"#,
+        expect![[r#"
+        OneOrMany could not deserialize any variant:
+          One: invalid type: map, expected u32
+          Many: invalid type: map, expected a sequence"#]],
+    );
+    check_error_deserialization::<S1Vec>(
+        r#""xx""#,
+        expect![[r#"
+        OneOrMany could not deserialize any variant:
+          One: invalid type: string "xx", expected u32
+          Many: invalid type: string "xx", expected a sequence"#]],
+    );
 
     #[serde_as]
     #[derive(Debug, Serialize, Deserialize, PartialEq)]
@@ -924,8 +960,20 @@ fn test_one_or_many_prefer_many() {
     );
     check_deserialization(S2Vec(vec![1]), r#""1""#);
     check_deserialization(S2Vec(vec![1]), r#"["1"]"#);
-    check_error_deserialization::<S2Vec>(r#"{}"#, expect![[r#"a list or single element"#]]);
-    check_error_deserialization::<S2Vec>(r#""xx""#, expect![[r#"a list or single element"#]]);
+    check_error_deserialization::<S2Vec>(
+        r#"{}"#,
+        expect![[r#"
+        OneOrMany could not deserialize any variant:
+          One: invalid type: map, expected a string
+          Many: invalid type: map, expected a sequence"#]],
+    );
+    check_error_deserialization::<S2Vec>(
+        r#""xx""#,
+        expect![[r#"
+        OneOrMany could not deserialize any variant:
+          One: invalid digit found in string
+          Many: invalid type: string "xx", expected a sequence"#]],
+    );
 }
 
 /// Test that Cow borrows from the input
diff --git a/serde_with/tests/serde_as/pickfirst.rs b/serde_with/tests/serde_as/pickfirst.rs
index 6443526..8c221df 100644
--- a/serde_with/tests/serde_as/pickfirst.rs
+++ b/serde_with/tests/serde_as/pickfirst.rs
@@ -14,7 +14,10 @@ fn test_pick_first_two() {
     check_deserialization(S(123), r#""123""#);
     check_error_deserialization::<S>(
         r#""Abc""#,
-        expect![[r#"PickFirst could not deserialize data"#]],
+        expect![[r#"
+            PickFirst could not deserialize any variant:
+              First: invalid type: string "Abc", expected u32
+              Second: invalid digit found in string"#]],
     );
 
     #[serde_as]
@@ -91,7 +94,11 @@ fn test_pick_first_three() {
     check_deserialization(S(vec![1, 2, 3]), r#""1,2,3""#);
     check_error_deserialization::<S>(
         r#""Abc""#,
-        expect![[r#"PickFirst could not deserialize data"#]],
+        expect![[r#"
+            PickFirst could not deserialize any variant:
+              First: invalid type: string "Abc", expected a sequence
+              Second: invalid type: string "Abc", expected a sequence
+              Third: invalid digit found in string"#]],
     );
 
     #[serde_as]
@@ -132,6 +139,11 @@ fn test_pick_first_four() {
     is_equal(S(123), expect![[r#"123"#]]);
     check_error_deserialization::<S>(
         r#""Abc""#,
-        expect![[r#"PickFirst could not deserialize data"#]],
+        expect![[r#"
+            PickFirst could not deserialize any variant:
+              First: invalid type: string "Abc", expected u32
+              Second: invalid type: string "Abc", expected u32
+              Third: invalid type: string "Abc", expected u32
+              Fourth: invalid type: string "Abc", expected u32"#]],
     );
 }