Repository: ron-rs/ron
Pull request #394 (2022-07-24 18:27:35)

Improve error information for serde errors
Followup to #393

Adds explicit variants in `ron::Error` for `serde` errors from [`serde::de::Error`](https://docs.rs/serde/latest/serde/de/trait.Error.html) (`serde::ser::Error` only has the `Error::custom` method).

* [x] I've included my change in `CHANGELOG.md`


MomoLangenstein: ?r @torkleyy
MomoLangenstein: This is the first proof-of-concept. Notably, I've also ported `serde`'s [`Unexpected`](https://docs.rs/serde/latest/serde/de/enum.Unexpected.html) enum, which I'm not quite sure about. It does give us slightly nicer error messages and would allow users to match on the unexpected value (though I'm not quite sure if anyone would need that). However, it also adds some very `serde`-specific terms to our codebase, which don't map quite so well to `ron`. We could decide to just remove that part and stringify the unexpected values as `serde` does, or we could limit the enum to entries that make sense for us (we'd still adopt `serde`'s decision to only store `[u|i]64`).
codecov-commenter: # [Codecov](https://codecov.io/gh/ron-rs/ron/pull/394?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#394](https://codecov.io/gh/ron-rs/ron/pull/394?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (72d71cb) into [master](https://codecov.io/gh/ron-rs/ron/commit/b9c4b75f06ec45bcb8cb48372341289193844970?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (b9c4b75) will **decrease** coverage by `2.34%`.
> The diff coverage is `41.22%`.

```diff
@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
- Coverage   89.65%   87.30%   -2.35%     
==========================================
  Files          48       48              
  Lines        5228     5444     +216     
==========================================
+ Hits         4687     4753      +66     
- Misses        541      691     +150     
```


| [Impacted Files](https://codecov.io/gh/ron-rs/ron/pull/394?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [src/error.rs](https://codecov.io/gh/ron-rs/ron/pull/394/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2Vycm9yLnJz) | `19.53% <27.16%> (+11.63%)` | :arrow_up: |
| [tests/203\_error\_positions.rs](https://codecov.io/gh/ron-rs/ron/pull/394/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dGVzdHMvMjAzX2Vycm9yX3Bvc2l0aW9ucy5ycw==) | `84.70% <84.31%> (-15.30%)` | :arrow_down: |
| [tests/359\_deserialize\_seed.rs](https://codecov.io/gh/ron-rs/ron/pull/394/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-dGVzdHMvMzU5X2Rlc2VyaWFsaXplX3NlZWQucnM=) | `100.00% <100.00%> (ø)` | |
| [src/parse.rs](https://codecov.io/gh/ron-rs/ron/pull/394/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL3BhcnNlLnJz) | `93.59% <0.00%> (-0.17%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/ron-rs/ron/pull/394?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/ron-rs/ron/pull/394?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Last update [b9c4b75...72d71cb](https://codecov.io/gh/ron-rs/ron/pull/394?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).

MomoLangenstein: What I haven't touched yet is the area of adding more contextual information to those new errors. Something we might be able to do is to decorate the `deserialize_enum` and `deserialize_struct`-like methods to check for the new errors and insert struct/enum name information.

We could probably also try to find the name of the current field, though in my opinion, that would be overkill and is already handled by crates like [`serde_path_to_error`](https://docs.rs/serde_path_to_error/latest/serde_path_to_error/).
MomoLangenstein: I've tried to do the struct/enum/variant name extraction for even better error messages. Just extracting struct/enum names is easy, and we can store them as `&'static str`. For variant names, however, which are important for struct variants which to the `ron`-writing user look just like structs, I had to add a small hack. The `Deserializer` now remembers what the last deserialized identifier was. Since enums in `ron` use identifiers for the variants, I can check for that last one and assume it was the variant id. This does provide better errors (for externally tagged enums, internally tagged ones cannot get any context since they are based on `deserialize_any`, adjacently tagged and untagged enums get ok-ish errors), but means that the struct/enum/variant name is stored as a `String`.
MomoLangenstein: @torkleyy @d86leader What are your thoughts?
torkleyy: I will have to give your comments a closer look later this week, I'm busy right now, sorry. But feel free to proceed without me if you.
I think workarounds are fine if they don't overly complicate the codebase and provide useful information, but I'd be cautious of exposing those details via an API because there's a higher chance we'll change them in the future. In general I think we shouldn't expose the full error enum as we do know.
MomoLangenstein: @torkleyy I've opted to simplify the error enum a bit by merging `serde`'s `invalid_type` and `invalid_value` into the same `InvalidValueForType` type in `ron`. Furthermore, this error now contains the stringified version of `found` field instead of replicating `serde`'s `de::Unexpected` type (however, I still implement some custom formatting for the conversion to get slightly nicer error messages). This now seems like a good compromise to me where we get some extra info from `serde`'s useful errors like the missing field or unknown variant ones without having to also include its expected vs unexpected system.
MomoLangenstein: CI is blocked on https://github.com/serde-rs/serde/issues/2255
torkleyy: Since we're doing a API-breaking release, I think we should use the opportunity to bump MSRV and get rid of the CI problem at the same time :)
MomoLangenstein: > Since we're doing a API-breaking release, I think we should use the opportunity to bump MSRV and get rid of the CI problem at the same time :)

@torkleyy I've opened #396 to bump the MSRV to 1.56. Once that PR is merged I'll rebase this one off it.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d95e834e..644ba6be 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fix issue [#301](https://github.com/ron-rs/ron/issues/301) with better error messages ([#354](https://github.com/ron-rs/ron/pull/354))
 - Fix issue [#337](https://github.com/ron-rs/ron/issues/337) by removing `decimal_floats` PrettyConfig option and unconditional decimals in floats ([#363](https://github.com/ron-rs/ron/pull/363))
 - Fix issue [#203](https://github.com/ron-rs/ron/issues/203) with full de error positioning ([#356](https://github.com/ron-rs/ron/pull/356))
+- Expand the `ron::Error` enum to distinguish `serde` errors like `NoSuchEnumVariant` and `MissingStructField` with error positioning ([#394](https://github.com/ron-rs/ron/pull/394))
 - Bump MSRV to 1.56.0 ([#396](https://github.com/ron-rs/ron/pull/396))
 
 ## [0.7.1] - 2022-06-15
diff --git a/src/de/mod.rs b/src/de/mod.rs
index 6f4afbdd..0920a67b 100644
--- a/src/de/mod.rs
+++ b/src/de/mod.rs
@@ -25,6 +25,7 @@ mod value;
 pub struct Deserializer<'de> {
     bytes: Bytes<'de>,
     newtype_variant: bool,
+    last_identifier: Option<&'de str>,
 }
 
 impl<'de> Deserializer<'de> {
@@ -46,6 +47,7 @@ impl<'de> Deserializer<'de> {
         let mut deserializer = Deserializer {
             bytes: Bytes::new(input)?,
             newtype_variant: false,
+            last_identifier: None,
         };
 
         deserializer.bytes.exts |= options.default_extensions;
@@ -528,7 +530,19 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
             let old_newtype_variant = self.newtype_variant;
             self.newtype_variant = false;
 
-            let value = visitor.visit_map(CommaSeparated::new(b')', self))?;
+            let value = visitor
+                .visit_map(CommaSeparated::new(b')', self))
+                .map_err(|err| {
+                    struct_error_name(
+                        err,
+                        if !old_newtype_variant && !name.is_empty() {
+                            Some(name)
+                        } else {
+                            None
+                        },
+                    )
+                })?;
+
             self.bytes.comma()?;
 
             if old_newtype_variant || self.bytes.consume(")") {
@@ -545,7 +559,7 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
 
     fn deserialize_enum<V>(
         self,
-        _name: &'static str,
+        name: &'static str,
         _variants: &'static [&'static str],
         visitor: V,
     ) -> Result<V::Value>
@@ -554,14 +568,30 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
     {
         self.newtype_variant = false;
 
-        visitor.visit_enum(Enum::new(self))
+        match visitor.visit_enum(Enum::new(self)) {
+            Ok(value) => Ok(value),
+            Err(Error::NoSuchEnumVariant {
+                expected,
+                found,
+                outer: None,
+            }) if !name.is_empty() => Err(Error::NoSuchEnumVariant {
+                expected,
+                found,
+                outer: Some(String::from(name)),
+            }),
+            Err(e) => Err(e),
+        }
     }
 
     fn deserialize_identifier<V>(self, visitor: V) -> Result<V::Value>
     where
         V: Visitor<'de>,
     {
-        visitor.visit_str(str::from_utf8(self.bytes.identifier()?).map_err(Error::from)?)
+        let identifier = str::from_utf8(self.bytes.identifier()?).map_err(Error::from)?;
+
+        self.last_identifier = Some(identifier);
+
+        visitor.visit_str(identifier)
     }
 
     fn deserialize_ignored_any<V>(self, visitor: V) -> Result<V::Value>
@@ -699,6 +729,8 @@ impl<'de, 'a> de::VariantAccess<'de> for Enum<'a, 'de> {
     where
         T: DeserializeSeed<'de>,
     {
+        let newtype_variant = self.de.last_identifier;
+
         self.de.bytes.skip_ws()?;
 
         if self.de.bytes.consume("(") {
@@ -710,7 +742,9 @@ impl<'de, 'a> de::VariantAccess<'de> for Enum<'a, 'de> {
                 .exts
                 .contains(Extensions::UNWRAP_VARIANT_NEWTYPES);
 
-            let val = seed.deserialize(&mut *self.de)?;
+            let val = seed
+                .deserialize(&mut *self.de)
+                .map_err(|err| struct_error_name(err, newtype_variant))?;
 
             self.de.newtype_variant = false;
 
@@ -739,8 +773,35 @@ impl<'de, 'a> de::VariantAccess<'de> for Enum<'a, 'de> {
     where
         V: Visitor<'de>,
     {
+        let struct_variant = self.de.last_identifier;
+
         self.de.bytes.skip_ws()?;
 
-        self.de.deserialize_struct("", fields, visitor)
+        self.de
+            .deserialize_struct("", fields, visitor)
+            .map_err(|err| struct_error_name(err, struct_variant))
+    }
+}
+
+fn struct_error_name(error: Error, name: Option<&str>) -> Error {
+    match error {
+        Error::NoSuchStructField {
+            expected,
+            found,
+            outer: None,
+        } => Error::NoSuchStructField {
+            expected,
+            found,
+            outer: name.map(ToOwned::to_owned),
+        },
+        Error::MissingStructField { field, outer: None } => Error::MissingStructField {
+            field,
+            outer: name.map(ToOwned::to_owned),
+        },
+        Error::DuplicateStructField { field, outer: None } => Error::DuplicateStructField {
+            field,
+            outer: name.map(ToOwned::to_owned),
+        },
+        e => e,
     }
 }
diff --git a/src/error.rs b/src/error.rs
index 29fbe878..7f7cc9b9 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -3,7 +3,7 @@ use std::{error::Error as StdError, fmt, io, str::Utf8Error, string::FromUtf8Err
 
 /// This type represents all possible errors that can occur when
 /// serializing or deserializing RON data.
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq, Eq)]
 pub struct SpannedError {
     pub code: Error,
     pub position: Position,
@@ -12,7 +12,7 @@ pub struct SpannedError {
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 pub type SpannedResult<T> = std::result::Result<T, SpannedError>;
 
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq, Eq)]
 #[non_exhaustive]
 pub enum Error {
     Io(String),
@@ -58,6 +58,33 @@ pub enum Error {
 
     Utf8Error(Utf8Error),
     TrailingCharacters,
+
+    InvalidValueForType {
+        expected: String,
+        found: String,
+    },
+    ExpectedDifferentLength {
+        expected: String,
+        found: usize,
+    },
+    NoSuchEnumVariant {
+        expected: &'static [&'static str],
+        found: String,
+        outer: Option<String>,
+    },
+    NoSuchStructField {
+        expected: &'static [&'static str],
+        found: String,
+        outer: Option<String>,
+    },
+    MissingStructField {
+        field: &'static str,
+        outer: Option<String>,
+    },
+    DuplicateStructField {
+        field: &'static str,
+        outer: Option<String>,
+    },
 }
 
 impl fmt::Display for SpannedError {
@@ -97,10 +124,10 @@ impl fmt::Display for Error {
             Error::ExpectedDifferentStructName {
                 expected,
                 ref found,
-            } => write!(f, "Expected struct '{}' but found '{}'", expected, found),
+            } => write!(f, "Expected struct `{}` but found `{}`", expected, found),
             Error::ExpectedStructLike => f.write_str("Expected opening `(`"),
             Error::ExpectedNamedStructLike(name) => {
-                write!(f, "Expected opening `(` for struct '{}'", name)
+                write!(f, "Expected opening `(` for struct `{}`", name)
             }
             Error::ExpectedStructLikeEnd => f.write_str("Expected closing `)`"),
             Error::ExpectedUnit => f.write_str("Expected unit"),
@@ -109,7 +136,7 @@ impl fmt::Display for Error {
             Error::ExpectedIdentifier => f.write_str("Expected identifier"),
             Error::InvalidEscape(s) => f.write_str(s),
             Error::IntegerOutOfBounds => f.write_str("Integer is out of bounds"),
-            Error::NoSuchExtension(ref name) => write!(f, "No RON extension named '{}'", name),
+            Error::NoSuchExtension(ref name) => write!(f, "No RON extension named `{}`", name),
             Error::Utf8Error(ref e) => fmt::Display::fmt(e, f),
             Error::UnclosedBlockComment => f.write_str("Unclosed block comment"),
             Error::UnderscoreAtBeginning => {
@@ -117,11 +144,93 @@ impl fmt::Display for Error {
             }
             Error::UnexpectedByte(ref byte) => write!(f, "Unexpected byte {:?}", byte),
             Error::TrailingCharacters => f.write_str("Non-whitespace trailing characters"),
+            Error::InvalidValueForType {
+                ref expected,
+                ref found,
+            } => {
+                write!(f, "Expected {} but found {} instead", expected, found)
+            }
+            Error::ExpectedDifferentLength {
+                ref expected,
+                found,
+            } => {
+                write!(f, "Expected {} but found ", expected)?;
+
+                match found {
+                    0 => f.write_str("zero elements")?,
+                    1 => f.write_str("one element")?,
+                    n => write!(f, "{} elements", n)?,
+                }
+
+                f.write_str(" instead")
+            }
+            Error::NoSuchEnumVariant {
+                expected,
+                ref found,
+                ref outer,
+            } => {
+                f.write_str("Unexpected ")?;
+
+                if outer.is_none() {
+                    f.write_str("enum ")?;
+                }
+
+                write!(f, "variant named `{}`", found)?;
+
+                if let Some(outer) = outer {
+                    write!(f, "in enum `{}`", outer)?;
+                }
+
+                write!(
+                    f,
+                    ", {}",
+                    OneOf {
+                        alts: expected,
+                        none: "variants"
+                    }
+                )
+            }
+            Error::NoSuchStructField {
+                expected,
+                ref found,
+                ref outer,
+            } => {
+                write!(f, "Unexpected field named `{}`", found)?;
+
+                if let Some(outer) = outer {
+                    write!(f, "in `{}`", outer)?;
+                }
+
+                write!(
+                    f,
+                    ", {}",
+                    OneOf {
+                        alts: expected,
+                        none: "fields"
+                    }
+                )
+            }
+            Error::MissingStructField { field, ref outer } => {
+                write!(f, "Unexpected missing field `{}`", field)?;
+
+                match outer {
+                    Some(outer) => write!(f, " in `{}`", outer),
+                    None => Ok(()),
+                }
+            }
+            Error::DuplicateStructField { field, ref outer } => {
+                write!(f, "Unexpected duplicate field `{}`", field)?;
+
+                match outer {
+                    Some(outer) => write!(f, " in `{}`", outer),
+                    None => Ok(()),
+                }
+            }
         }
     }
 }
 
-#[derive(Clone, Copy, Debug, PartialEq)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub struct Position {
     pub line: usize,
     pub col: usize,
@@ -134,15 +243,104 @@ impl fmt::Display for Position {
 }
 
 impl ser::Error for Error {
+    #[cold]
     fn custom<T: fmt::Display>(msg: T) -> Self {
         Error::Message(msg.to_string())
     }
 }
 
 impl de::Error for Error {
+    #[cold]
     fn custom<T: fmt::Display>(msg: T) -> Self {
         Error::Message(msg.to_string())
     }
+
+    #[cold]
+    fn invalid_type(unexp: de::Unexpected, exp: &dyn de::Expected) -> Self {
+        // Invalid type and invalid value are merged given their similarity in ron
+        Self::invalid_value(unexp, exp)
+    }
+
+    #[cold]
+    fn invalid_value(unexp: de::Unexpected, exp: &dyn de::Expected) -> Self {
+        struct UnexpectedSerdeTypeValue<'a>(de::Unexpected<'a>);
+
+        impl<'a> fmt::Display for UnexpectedSerdeTypeValue<'a> {
+            fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+                use de::Unexpected::*;
+
+                match self.0 {
+                    Bool(b) => write!(f, "the boolean `{}`", b),
+                    Unsigned(i) => write!(f, "the unsigned integer `{}`", i),
+                    Signed(i) => write!(f, "the signed integer `{}`", i),
+                    Float(n) => write!(f, "the floating point number `{}`", n),
+                    Char(c) => write!(f, "the UTF-8 character `{}`", c),
+                    Str(s) => write!(f, "the string {:?}", s),
+                    Bytes(b) => {
+                        f.write_str("the bytes b\"")?;
+
+                        for b in b {
+                            write!(f, "\\x{:02x}", b)?;
+                        }
+
+                        f.write_str("\"")
+                    }
+                    Unit => write!(f, "a unit value"),
+                    Option => write!(f, "an optional value"),
+                    NewtypeStruct => write!(f, "a newtype struct"),
+                    Seq => write!(f, "a sequence"),
+                    Map => write!(f, "a map"),
+                    Enum => write!(f, "an enum"),
+                    UnitVariant => write!(f, "a unit variant"),
+                    NewtypeVariant => write!(f, "a newtype variant"),
+                    TupleVariant => write!(f, "a tuple variant"),
+                    StructVariant => write!(f, "a struct variant"),
+                    Other(other) => f.write_str(other),
+                }
+            }
+        }
+
+        Error::InvalidValueForType {
+            expected: exp.to_string(),
+            found: UnexpectedSerdeTypeValue(unexp).to_string(),
+        }
+    }
+
+    #[cold]
+    fn invalid_length(len: usize, exp: &dyn de::Expected) -> Self {
+        Error::ExpectedDifferentLength {
+            expected: exp.to_string(),
+            found: len,
+        }
+    }
+
+    #[cold]
+    fn unknown_variant(variant: &str, expected: &'static [&'static str]) -> Self {
+        Error::NoSuchEnumVariant {
+            expected,
+            found: variant.to_string(),
+            outer: None,
+        }
+    }
+
+    #[cold]
+    fn unknown_field(field: &str, expected: &'static [&'static str]) -> Self {
+        Error::NoSuchStructField {
+            expected,
+            found: field.to_string(),
+            outer: None,
+        }
+    }
+
+    #[cold]
+    fn missing_field(field: &'static str) -> Self {
+        Error::MissingStructField { field, outer: None }
+    }
+
+    #[cold]
+    fn duplicate_field(field: &'static str) -> Self {
+        Error::DuplicateStructField { field, outer: None }
+    }
 }
 
 impl StdError for SpannedError {}
@@ -180,3 +378,27 @@ impl From<SpannedError> for Error {
         e.code
     }
 }
+
+struct OneOf {
+    alts: &'static [&'static str],
+    none: &'static str,
+}
+
+impl fmt::Display for OneOf {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self.alts {
+            [] => write!(f, "there are no {}", self.none),
+            [a1] => write!(f, "expected `{}` instead", a1),
+            [a1, a2] => write!(f, "expected either `{}` or `{}` instead", a1, a2),
+            [a1, ref alts @ ..] => {
+                write!(f, "expected one of `{}`", a1)?;
+
+                for alt in alts {
+                    write!(f, ", `{}`", alt)?;
+                }
+
+                f.write_str(" instead")
+            }
+        }
+    }
+}
diff --git a/src/parse.rs b/src/parse.rs
index f3265baa..b0cc632f 100644
--- a/src/parse.rs
+++ b/src/parse.rs
@@ -656,11 +656,11 @@ impl<'a> Bytes<'a> {
     }
 
     pub fn peek(&self) -> Option<u8> {
-        self.bytes.get(0).cloned()
+        self.bytes.first().cloned()
     }
 
     pub fn peek_or_eof(&self) -> Result<u8> {
-        self.bytes.get(0).cloned().ok_or(Error::Eof)
+        self.bytes.first().cloned().ok_or(Error::Eof)
     }
 
     pub fn signed_integer<T>(&mut self) -> Result<T>


Test patch:
diff --git a/tests/203_error_positions.rs b/tests/203_error_positions.rs
index f1537aa9..0ef486a1 100644
--- a/tests/203_error_positions.rs
+++ b/tests/203_error_positions.rs
@@ -1,54 +1,93 @@
 use std::num::NonZeroU32;
 
 use ron::error::{Error, Position, SpannedError};
-use serde::de::{value::Error as DeError, Deserialize, IntoDeserializer};
+use serde::{
+    de::{Deserialize, Error as DeError, Unexpected},
+    Deserializer,
+};
 
 #[derive(Debug, serde::Deserialize, PartialEq)]
+#[serde(deny_unknown_fields)]
 enum Test {
-    TupleVariant(i32, NonZeroU32),
+    TupleVariant(i32, String),
     StructVariant { a: bool, b: NonZeroU32, c: i32 },
 }
 
+#[derive(Debug, PartialEq)]
+struct TypeError;
+
+impl<'de> Deserialize<'de> for TypeError {
+    fn deserialize<D: Deserializer<'de>>(_deserializer: D) -> Result<Self, D::Error> {
+        Err(D::Error::invalid_type(Unexpected::Unit, &"impossible"))
+    }
+}
+
 #[test]
 fn test_error_positions() {
     assert_eq!(
-        ron::from_str::<Test>("NotAVariant"),
+        ron::from_str::<TypeError>("  ()"),
         Err(SpannedError {
-            code: Error::Message(String::from(
-                "unknown variant `NotAVariant`, expected `TupleVariant` or `StructVariant`"
-            )),
-            position: Position { line: 1, col: 12 },
+            code: Error::InvalidValueForType {
+                expected: String::from("impossible"),
+                found: String::from("a unit value"),
+            },
+            position: Position { line: 1, col: 3 },
         })
     );
 
     assert_eq!(
-        ron::from_str::<Test>("TupleVariant(1, 0)"),
+        ron::from_str::<Test>("StructVariant(a: true, b: 0, c: -42)"),
         Err(SpannedError {
-            code: Error::Message(
-                NonZeroU32::deserialize(IntoDeserializer::<DeError>::into_deserializer(0_u32))
-                    .unwrap_err()
-                    .to_string()
-            ),
-            position: Position { line: 1, col: 18 },
+            code: Error::InvalidValueForType {
+                expected: String::from("a nonzero u32"),
+                found: String::from("the unsigned integer `0`"),
+            },
+            position: Position { line: 1, col: 28 },
         })
     );
 
     assert_eq!(
-        ron::from_str::<Test>("StructVariant(a: true, b: 0, c: -42)"),
+        ron::from_str::<Test>("TupleVariant(42)"),
         Err(SpannedError {
-            code: Error::Message(
-                NonZeroU32::deserialize(IntoDeserializer::<DeError>::into_deserializer(0_u32))
-                    .unwrap_err()
-                    .to_string()
-            ),
-            position: Position { line: 1, col: 28 },
+            code: Error::ExpectedDifferentLength {
+                expected: String::from("tuple variant Test::TupleVariant with 2 elements"),
+                found: 1,
+            },
+            position: Position { line: 1, col: 16 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<Test>("NotAVariant"),
+        Err(SpannedError {
+            code: Error::NoSuchEnumVariant {
+                expected: &["TupleVariant", "StructVariant"],
+                found: String::from("NotAVariant"),
+                outer: Some(String::from("Test")),
+            },
+            position: Position { line: 1, col: 12 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<Test>("StructVariant(a: true, b: 1, c: -42, d: \"gotcha\")"),
+        Err(SpannedError {
+            code: Error::NoSuchStructField {
+                expected: &["a", "b", "c"],
+                found: String::from("d"),
+                outer: Some(String::from("StructVariant")),
+            },
+            position: Position { line: 1, col: 39 },
         })
     );
 
     assert_eq!(
         ron::from_str::<Test>("StructVariant(a: true, c: -42)"),
         Err(SpannedError {
-            code: Error::Message(String::from("missing field `b`")),
+            code: Error::MissingStructField {
+                field: "b",
+                outer: Some(String::from("StructVariant")),
+            },
             position: Position { line: 1, col: 30 },
         })
     );
@@ -56,7 +95,10 @@ fn test_error_positions() {
     assert_eq!(
         ron::from_str::<Test>("StructVariant(a: true, b: 1, a: false, c: -42)"),
         Err(SpannedError {
-            code: Error::Message(String::from("duplicate field `a`")),
+            code: Error::DuplicateStructField {
+                field: "a",
+                outer: Some(String::from("StructVariant")),
+            },
             position: Position { line: 1, col: 31 },
         })
     );
diff --git a/tests/359_deserialize_seed.rs b/tests/359_deserialize_seed.rs
index 74afd05a..4bca6c43 100644
--- a/tests/359_deserialize_seed.rs
+++ b/tests/359_deserialize_seed.rs
@@ -47,9 +47,10 @@ fn test_deserialize_seed() {
     assert_eq!(
         ron::Options::default().from_str_seed("'a'", Seed(42)),
         Err(ron::error::SpannedError {
-            code: ron::Error::Message(String::from(
-                "invalid type: string \"a\", expected an integer"
-            )),
+            code: ron::Error::InvalidValueForType {
+                expected: String::from("an integer"),
+                found: String::from("the string \"a\""),
+            },
             position: ron::error::Position { line: 1, col: 4 },
         })
     );
diff --git a/tests/393_serde_errors.rs b/tests/393_serde_errors.rs
new file mode 100644
index 00000000..f57532bf
--- /dev/null
+++ b/tests/393_serde_errors.rs
@@ -0,0 +1,215 @@
+use ron::error::{Error, Position, SpannedError};
+
+#[derive(Debug, serde::Deserialize, PartialEq)]
+#[serde(deny_unknown_fields)]
+enum TestEnum {
+    StructVariant { a: bool, b: char, c: i32 },
+    NewtypeVariant(TestStruct),
+}
+
+#[derive(Debug, serde::Deserialize, PartialEq)]
+#[serde(tag = "type")]
+enum TestEnumInternal {
+    StructVariant { a: bool },
+}
+
+#[derive(Debug, serde::Deserialize, PartialEq)]
+#[serde(tag = "type", content = "content")]
+enum TestEnumAdjacent {
+    StructVariant { a: bool },
+}
+
+#[derive(Debug, serde::Deserialize, PartialEq)]
+#[serde(untagged)]
+enum TestEnumUntagged {
+    StructVariant { a: bool },
+}
+
+#[derive(Debug, serde::Deserialize, PartialEq)]
+#[serde(deny_unknown_fields)]
+struct TestStruct {
+    a: bool,
+    b: char,
+    c: i32,
+}
+
+#[test]
+fn test_unknown_enum_variant() {
+    assert_eq!(
+        ron::from_str::<TestEnum>("NotAVariant"),
+        Err(SpannedError {
+            code: Error::NoSuchEnumVariant {
+                expected: &["StructVariant", "NewtypeVariant"],
+                found: String::from("NotAVariant"),
+                outer: Some(String::from("TestEnum")),
+            },
+            position: Position { line: 1, col: 12 },
+        })
+    );
+}
+
+#[test]
+fn test_struct_enum_fields() {
+    assert_eq!(
+        ron::from_str::<TestEnum>("StructVariant(a: true, b: 'b', c: -42, d: \"gotcha\")"),
+        Err(SpannedError {
+            code: Error::NoSuchStructField {
+                expected: &["a", "b", "c"],
+                found: String::from("d"),
+                outer: Some(String::from("StructVariant")),
+            },
+            position: Position { line: 1, col: 41 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<TestEnum>("StructVariant(a: true, c: -42)"),
+        Err(SpannedError {
+            code: Error::MissingStructField {
+                field: "b",
+                outer: Some(String::from("StructVariant")),
+            },
+            position: Position { line: 1, col: 30 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<TestEnum>("StructVariant(a: true, b: 'b', a: false, c: -42)"),
+        Err(SpannedError {
+            code: Error::DuplicateStructField {
+                field: "a",
+                outer: Some(String::from("StructVariant")),
+            },
+            position: Position { line: 1, col: 33 },
+        })
+    );
+}
+
+#[test]
+fn test_newtype_enum_fields() {
+    assert_eq!(
+        ron::from_str::<TestEnum>("#![enable(unwrap_variant_newtypes)] NewtypeVariant(a: true, b: 'b', c: -42, d: \"gotcha\")"),
+        Err(SpannedError {
+            code: Error::NoSuchStructField {
+                expected: &["a", "b", "c"],
+                found: String::from("d"),
+                outer: Some(String::from("NewtypeVariant")),
+            },
+            position: Position { line: 1, col: 78 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<TestEnum>(
+            "#![enable(unwrap_variant_newtypes)] NewtypeVariant(a: true, c: -42)"
+        ),
+        Err(SpannedError {
+            code: Error::MissingStructField {
+                field: "b",
+                outer: Some(String::from("NewtypeVariant")),
+            },
+            position: Position { line: 1, col: 67 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<TestEnum>(
+            "#![enable(unwrap_variant_newtypes)] NewtypeVariant(a: true, b: 'b', a: false, c: -42)"
+        ),
+        Err(SpannedError {
+            code: Error::DuplicateStructField {
+                field: "a",
+                outer: Some(String::from("NewtypeVariant")),
+            },
+            position: Position { line: 1, col: 70 },
+        })
+    );
+}
+
+#[test]
+fn test_struct_fields() {
+    assert_eq!(
+        ron::from_str::<TestStruct>("TestStruct(a: true, b: 'b', c: -42, d: \"gotcha\")"),
+        Err(SpannedError {
+            code: Error::NoSuchStructField {
+                expected: &["a", "b", "c"],
+                found: String::from("d"),
+                outer: Some(String::from("TestStruct")),
+            },
+            position: Position { line: 1, col: 38 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<TestStruct>("TestStruct(a: true, c: -42)"),
+        Err(SpannedError {
+            code: Error::MissingStructField {
+                field: "b",
+                outer: Some(String::from("TestStruct")),
+            },
+            position: Position { line: 1, col: 27 },
+        })
+    );
+
+    assert_eq!(
+        ron::from_str::<TestStruct>("TestStruct(a: true, b: 'b', a: false, c: -42)"),
+        Err(SpannedError {
+            code: Error::DuplicateStructField {
+                field: "a",
+                outer: Some(String::from("TestStruct")),
+            },
+            position: Position { line: 1, col: 30 },
+        })
+    );
+}
+
+#[test]
+fn test_internally_tagged_enum() {
+    // Note: Not extracting the variant type is not great,
+    //        but at least not wrong either
+    //       Since the error occurs in serde-generated user code,
+    //        after successfully deserialising, we cannot annotate
+
+    assert_eq!(
+        ron::from_str::<TestEnumInternal>("(type: \"StructVariant\")"),
+        Err(SpannedError {
+            code: Error::MissingStructField {
+                field: "a",
+                outer: None,
+            },
+            position: Position { line: 1, col: 24 },
+        })
+    );
+}
+
+#[test]
+fn test_adjacently_tagged_enum() {
+    // Note: TestEnumAdjacent makes sense here since we are now treating
+    //        the enum as a struct
+
+    assert_eq!(
+        ron::from_str::<TestEnumAdjacent>("(type: \"StructVariant\", content: (d: 4))"),
+        Err(SpannedError {
+            code: Error::MissingStructField {
+                field: "a",
+                outer: Some(String::from("TestEnumAdjacent")),
+            },
+            position: Position { line: 1, col: 39 },
+        })
+    );
+}
+
+#[test]
+fn test_untagged_enum() {
+    // Note: Errors inside untagged enums are not bubbled up
+
+    assert_eq!(
+        ron::from_str::<TestEnumUntagged>("(a: true, a: false)"),
+        Err(SpannedError {
+            code: Error::Message(String::from(
+                "data did not match any variant of untagged enum TestEnumUntagged"
+            )),
+            position: Position { line: 1, col: 20 },
+        })
+    );
+}
diff --git a/tests/roundtrip.rs b/tests/roundtrip.rs
index 78517d9e..7ca5bd8a 100644
--- a/tests/roundtrip.rs
+++ b/tests/roundtrip.rs
@@ -82,7 +82,7 @@ fn roundtrip_pretty() {
 
 #[test]
 fn roundtrip_sep_tuple_members() {
-    #[derive(Debug, Deserialize, PartialEq, Serialize)]
+    #[derive(Debug, Deserialize, PartialEq, Eq, Serialize)]
     pub enum FileOrMem {
         File(String),
         Memory,