From 6f84bf0341b5bd6970a8fe44599eeb3a123ea8a7 Mon Sep 17 00:00:00 2001 From: Juniper Langenstein Date: Sun, 24 Jul 2022 18:23:11 +0000 Subject: [PATCH 1/5] Added explicit error variants for serde errors --- src/error.rs | 253 +++++++++++++++++++++++++++++++++- tests/203_error_positions.rs | 82 +++++++---- tests/359_deserialize_seed.rs | 7 +- 3 files changed, 312 insertions(+), 30 deletions(-) diff --git a/src/error.rs b/src/error.rs index 29fbe878..d37225a0 100644 --- a/src/error.rs +++ b/src/error.rs @@ -58,6 +58,29 @@ pub enum Error { Utf8Error(Utf8Error), TrailingCharacters, + + ExpectedDifferentType { + expected: String, + found: UnexpectedSerdeTypeValue, + }, + InvalidValueForType { + expected: String, + found: UnexpectedSerdeTypeValue, + }, + ExpectedDifferentLength { + expected: String, + found: usize, + }, + NoSuchEnumVariant { + expected: &'static [&'static str], + found: String, + }, + NoSuchStructField { + expected: &'static [&'static str], + found: String, + }, + MissingStructField(&'static str), + DuplicateStructField(&'static str), } impl fmt::Display for SpannedError { @@ -97,10 +120,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 +132,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,6 +140,70 @@ impl fmt::Display for Error { } Error::UnexpectedByte(ref byte) => write!(f, "Unexpected byte {:?}", byte), Error::TrailingCharacters => f.write_str("Non-whitespace trailing characters"), + Error::ExpectedDifferentType { + ref expected, + ref found, + } => { + write!( + f, + "Expected a value of type {} but found {} instead", + expected, found + ) + } + 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, + } => { + write!( + f, + "Unexpected enum variant named `{}`, {}", + found, + OneOf { + alts: expected, + none: "variants" + } + ) + } + Error::NoSuchStructField { + expected, + ref found, + } => { + write!( + f, + "Unexpected field named `{}`, {}", + found, + OneOf { + alts: expected, + none: "fields" + } + ) + } + Error::MissingStructField(field) => { + write!(f, "Unexpected missing field `{}`", field) + } + Error::DuplicateStructField(field) => { + write!(f, "Unexpected duplicate field `{}`", field) + } } } } @@ -134,15 +221,67 @@ impl fmt::Display for Position { } impl ser::Error for Error { + #[cold] fn custom(msg: T) -> Self { Error::Message(msg.to_string()) } } impl de::Error for Error { + #[cold] fn custom(msg: T) -> Self { Error::Message(msg.to_string()) } + + #[cold] + fn invalid_type(unexp: de::Unexpected, exp: &dyn de::Expected) -> Self { + Error::ExpectedDifferentType { + expected: exp.to_string(), + found: unexp.into(), + } + } + + #[cold] + fn invalid_value(unexp: de::Unexpected, exp: &dyn de::Expected) -> Self { + Error::InvalidValueForType { + expected: exp.to_string(), + found: unexp.into(), + } + } + + #[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(), + } + } + + #[cold] + fn unknown_field(field: &str, expected: &'static [&'static str]) -> Self { + Error::NoSuchStructField { + expected, + found: field.to_string(), + } + } + + #[cold] + fn missing_field(field: &'static str) -> Self { + Error::MissingStructField(field) + } + + #[cold] + fn duplicate_field(field: &'static str) -> Self { + Error::DuplicateStructField(field) + } } impl StdError for SpannedError {} @@ -180,3 +319,111 @@ impl From for Error { e.code } } + +#[derive(Clone, Debug, PartialEq)] +pub enum UnexpectedSerdeTypeValue { + Bool(bool), + Unsigned(u64), + Signed(i64), + Float(f64), + Char(char), + Str(String), + Bytes(Vec), + Unit, + Option, + NewtypeStruct, + Seq, + Map, + Enum, + UnitVariant, + NewtypeVariant, + TupleVariant, + StructVariant, + Other(String), +} + +impl<'a> From> for UnexpectedSerdeTypeValue { + fn from(unexpected: de::Unexpected<'a>) -> Self { + use de::Unexpected::*; + + match unexpected { + Bool(b) => Self::Bool(b), + Unsigned(u) => Self::Unsigned(u), + Signed(s) => Self::Signed(s), + Float(f) => Self::Float(f), + Char(c) => Self::Char(c), + Str(s) => Self::Str(s.to_owned()), + Bytes(b) => Self::Bytes(b.to_owned()), + Unit => Self::Unit, + Option => Self::Option, + NewtypeStruct => Self::NewtypeStruct, + Seq => Self::Seq, + Map => Self::Map, + Enum => Self::Enum, + UnitVariant => Self::UnitVariant, + NewtypeVariant => Self::NewtypeVariant, + TupleVariant => Self::TupleVariant, + StructVariant => Self::StructVariant, + Other(o) => Self::Other(o.to_owned()), + } + } +} + +impl fmt::Display for UnexpectedSerdeTypeValue { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use self::UnexpectedSerdeTypeValue::*; + + match *self { + Bool(b) => write!(f, "the boolean `{}`", b), + Unsigned(i) => write!(f, "the integer `{}`", i), + Signed(i) => write!(f, "the integer `{}`", i), + Float(n) => write!(f, "the floating point number `{}`", n), + Char(c) => write!(f, "the UTF-8 character `{}`", c), + Str(ref s) => write!(f, "the string {:?}", s), + Bytes(ref 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(ref other) => f.write_str(other), + } + } +} + +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/tests/203_error_positions.rs b/tests/203_error_positions.rs index f1537aa9..05a1872c 100644 --- a/tests/203_error_positions.rs +++ b/tests/203_error_positions.rs @@ -1,54 +1,88 @@ use std::num::NonZeroU32; -use ron::error::{Error, Position, SpannedError}; -use serde::de::{value::Error as DeError, Deserialize, IntoDeserializer}; +use ron::error::{Error, Position, SpannedError, UnexpectedSerdeTypeValue}; +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>(_deserializer: D) -> Result { + Err(D::Error::invalid_type(Unexpected::Unit, &"impossible")) + } +} + #[test] fn test_error_positions() { assert_eq!( - ron::from_str::("NotAVariant"), + ron::from_str::(" ()"), Err(SpannedError { - code: Error::Message(String::from( - "unknown variant `NotAVariant`, expected `TupleVariant` or `StructVariant`" - )), - position: Position { line: 1, col: 12 }, + code: Error::ExpectedDifferentType { + expected: String::from("impossible"), + found: UnexpectedSerdeTypeValue::Unit, + }, + position: Position { line: 1, col: 3 }, }) ); assert_eq!( - ron::from_str::("TupleVariant(1, 0)"), + ron::from_str::("StructVariant(a: true, b: 0, c: -42)"), Err(SpannedError { - code: Error::Message( - NonZeroU32::deserialize(IntoDeserializer::::into_deserializer(0_u32)) - .unwrap_err() - .to_string() - ), - position: Position { line: 1, col: 18 }, + code: Error::InvalidValueForType { + expected: String::from("a nonzero u32"), + found: UnexpectedSerdeTypeValue::Unsigned(0), + }, + position: Position { line: 1, col: 28 }, }) ); assert_eq!( - ron::from_str::("StructVariant(a: true, b: 0, c: -42)"), + ron::from_str::("TupleVariant(42)"), Err(SpannedError { - code: Error::Message( - NonZeroU32::deserialize(IntoDeserializer::::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::("NotAVariant"), + Err(SpannedError { + code: Error::NoSuchEnumVariant { + expected: &["TupleVariant", "StructVariant"], + found: String::from("NotAVariant"), + }, + position: Position { line: 1, col: 12 }, + }) + ); + + assert_eq!( + ron::from_str::("StructVariant(a: true, b: 1, c: -42, d: \"gotcha\")"), + Err(SpannedError { + code: Error::NoSuchStructField { + expected: &["a", "b", "c"], + found: String::from("d"), + }, + position: Position { line: 1, col: 39 }, }) ); assert_eq!( ron::from_str::("StructVariant(a: true, c: -42)"), Err(SpannedError { - code: Error::Message(String::from("missing field `b`")), + code: Error::MissingStructField("b"), position: Position { line: 1, col: 30 }, }) ); @@ -56,7 +90,7 @@ fn test_error_positions() { assert_eq!( ron::from_str::("StructVariant(a: true, b: 1, a: false, c: -42)"), Err(SpannedError { - code: Error::Message(String::from("duplicate field `a`")), + code: Error::DuplicateStructField("a"), position: Position { line: 1, col: 31 }, }) ); diff --git a/tests/359_deserialize_seed.rs b/tests/359_deserialize_seed.rs index 74afd05a..0d4df07d 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::ExpectedDifferentType { + expected: String::from("an integer"), + found: ron::error::UnexpectedSerdeTypeValue::Str(String::from("a")), + }, position: ron::error::Position { line: 1, col: 4 }, }) ); From 9407e2eaaaa059e78bd2e8c37c2dfddb30a334f6 Mon Sep 17 00:00:00 2001 From: Juniper Langenstein Date: Sun, 24 Jul 2022 21:08:40 +0000 Subject: [PATCH 2/5] Annotate serde struct+enum errors with struct/enum/variant names --- src/de/mod.rs | 73 ++++++++++++++++-- src/error.rs | 62 ++++++++++++--- tests/203_error_positions.rs | 12 ++- tests/393_serde_errors.rs | 146 +++++++++++++++++++++++++++++++++++ 4 files changed, 273 insertions(+), 20 deletions(-) create mode 100644 tests/393_serde_errors.rs 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( self, - _name: &'static str, + name: &'static str, _variants: &'static [&'static str], visitor: V, ) -> Result @@ -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(self, visitor: V) -> Result 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(self, visitor: V) -> Result @@ -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 d37225a0..81bd462c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -74,13 +74,21 @@ pub enum Error { NoSuchEnumVariant { expected: &'static [&'static str], found: String, + outer: Option, }, NoSuchStructField { expected: &'static [&'static str], found: String, + outer: Option, + }, + MissingStructField { + field: &'static str, + outer: Option, + }, + DuplicateStructField { + field: &'static str, + outer: Option, }, - MissingStructField(&'static str), - DuplicateStructField(&'static str), } impl fmt::Display for SpannedError { @@ -173,11 +181,23 @@ impl fmt::Display for Error { 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, - "Unexpected enum variant named `{}`, {}", - found, + ", {}", OneOf { alts: expected, none: "variants" @@ -187,22 +207,38 @@ impl fmt::Display for Error { Error::NoSuchStructField { expected, ref found, + ref outer, } => { + write!(f, "Unexpected field named `{}`", found)?; + + if let Some(outer) = outer { + write!(f, "in `{}`", outer)?; + } + write!( f, - "Unexpected field named `{}`, {}", - found, + ", {}", OneOf { alts: expected, none: "fields" } ) } - Error::MissingStructField(field) => { - write!(f, "Unexpected missing field `{}`", field) + Error::MissingStructField { field, ref outer } => { + write!(f, "Unexpected missing field `{}`", field)?; + + match outer { + Some(outer) => write!(f, " in `{}`", outer), + None => Ok(()), + } } - Error::DuplicateStructField(field) => { - write!(f, "Unexpected duplicate field `{}`", field) + Error::DuplicateStructField { field, ref outer } => { + write!(f, "Unexpected duplicate field `{}`", field)?; + + match outer { + Some(outer) => write!(f, " in `{}`", outer), + None => Ok(()), + } } } } @@ -262,6 +298,7 @@ impl de::Error for Error { Error::NoSuchEnumVariant { expected, found: variant.to_string(), + outer: None, } } @@ -270,17 +307,18 @@ impl de::Error for Error { Error::NoSuchStructField { expected, found: field.to_string(), + outer: None, } } #[cold] fn missing_field(field: &'static str) -> Self { - Error::MissingStructField(field) + Error::MissingStructField { field, outer: None } } #[cold] fn duplicate_field(field: &'static str) -> Self { - Error::DuplicateStructField(field) + Error::DuplicateStructField { field, outer: None } } } diff --git a/tests/203_error_positions.rs b/tests/203_error_positions.rs index 05a1872c..e7ee7d25 100644 --- a/tests/203_error_positions.rs +++ b/tests/203_error_positions.rs @@ -63,6 +63,7 @@ fn test_error_positions() { code: Error::NoSuchEnumVariant { expected: &["TupleVariant", "StructVariant"], found: String::from("NotAVariant"), + outer: Some(String::from("Test")), }, position: Position { line: 1, col: 12 }, }) @@ -74,6 +75,7 @@ fn test_error_positions() { code: Error::NoSuchStructField { expected: &["a", "b", "c"], found: String::from("d"), + outer: Some(String::from("StructVariant")), }, position: Position { line: 1, col: 39 }, }) @@ -82,7 +84,10 @@ fn test_error_positions() { assert_eq!( ron::from_str::("StructVariant(a: true, c: -42)"), Err(SpannedError { - code: Error::MissingStructField("b"), + code: Error::MissingStructField { + field: "b", + outer: Some(String::from("StructVariant")), + }, position: Position { line: 1, col: 30 }, }) ); @@ -90,7 +95,10 @@ fn test_error_positions() { assert_eq!( ron::from_str::("StructVariant(a: true, b: 1, a: false, c: -42)"), Err(SpannedError { - code: Error::DuplicateStructField("a"), + code: Error::DuplicateStructField { + field: "a", + outer: Some(String::from("StructVariant")), + }, position: Position { line: 1, col: 31 }, }) ); diff --git a/tests/393_serde_errors.rs b/tests/393_serde_errors.rs new file mode 100644 index 00000000..576d4165 --- /dev/null +++ b/tests/393_serde_errors.rs @@ -0,0 +1,146 @@ +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(deny_unknown_fields)] +struct TestStruct { + a: bool, + b: char, + c: i32, +} + +#[test] +fn test_unknown_enum_variant() { + assert_eq!( + ron::from_str::("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::("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::("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::("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::("#![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::( + "#![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::( + "#![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(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(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(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 }, + }) + ); +} From 61d8164879f0abfd731ea1de7c54217d33be211f Mon Sep 17 00:00:00 2001 From: Juniper Langenstein Date: Sun, 24 Jul 2022 21:53:48 +0000 Subject: [PATCH 3/5] Add tests for non-externally tagged enums --- tests/393_serde_errors.rs | 69 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/tests/393_serde_errors.rs b/tests/393_serde_errors.rs index 576d4165..f57532bf 100644 --- a/tests/393_serde_errors.rs +++ b/tests/393_serde_errors.rs @@ -7,6 +7,24 @@ enum TestEnum { 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 { @@ -144,3 +162,54 @@ fn test_struct_fields() { }) ); } + +#[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::("(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::("(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::("(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 }, + }) + ); +} From 9890bc7cc5258de148b90648fd44ac0f3268387d Mon Sep 17 00:00:00 2001 From: Juniper Langenstein Date: Mon, 15 Aug 2022 05:18:07 +0000 Subject: [PATCH 4/5] Removed duplicate of serde::de::Unexpected from ron error API --- src/error.rs | 151 ++++++++++------------------------ src/parse.rs | 4 +- tests/203_error_positions.rs | 8 +- tests/359_deserialize_seed.rs | 4 +- tests/roundtrip.rs | 2 +- 5 files changed, 53 insertions(+), 116 deletions(-) diff --git a/src/error.rs b/src/error.rs index 81bd462c..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 = std::result::Result; pub type SpannedResult = std::result::Result; -#[derive(Clone, Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq, Eq)] #[non_exhaustive] pub enum Error { Io(String), @@ -59,13 +59,9 @@ pub enum Error { Utf8Error(Utf8Error), TrailingCharacters, - ExpectedDifferentType { - expected: String, - found: UnexpectedSerdeTypeValue, - }, InvalidValueForType { expected: String, - found: UnexpectedSerdeTypeValue, + found: String, }, ExpectedDifferentLength { expected: String, @@ -148,16 +144,6 @@ impl fmt::Display for Error { } Error::UnexpectedByte(ref byte) => write!(f, "Unexpected byte {:?}", byte), Error::TrailingCharacters => f.write_str("Non-whitespace trailing characters"), - Error::ExpectedDifferentType { - ref expected, - ref found, - } => { - write!( - f, - "Expected a value of type {} but found {} instead", - expected, found - ) - } Error::InvalidValueForType { ref expected, ref found, @@ -244,7 +230,7 @@ impl fmt::Display for Error { } } -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct Position { pub line: usize, pub col: usize, @@ -271,17 +257,52 @@ impl de::Error for Error { #[cold] fn invalid_type(unexp: de::Unexpected, exp: &dyn de::Expected) -> Self { - Error::ExpectedDifferentType { - expected: exp.to_string(), - found: unexp.into(), - } + // 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: unexp.into(), + found: UnexpectedSerdeTypeValue(unexp).to_string(), } } @@ -358,90 +379,6 @@ impl From for Error { } } -#[derive(Clone, Debug, PartialEq)] -pub enum UnexpectedSerdeTypeValue { - Bool(bool), - Unsigned(u64), - Signed(i64), - Float(f64), - Char(char), - Str(String), - Bytes(Vec), - Unit, - Option, - NewtypeStruct, - Seq, - Map, - Enum, - UnitVariant, - NewtypeVariant, - TupleVariant, - StructVariant, - Other(String), -} - -impl<'a> From> for UnexpectedSerdeTypeValue { - fn from(unexpected: de::Unexpected<'a>) -> Self { - use de::Unexpected::*; - - match unexpected { - Bool(b) => Self::Bool(b), - Unsigned(u) => Self::Unsigned(u), - Signed(s) => Self::Signed(s), - Float(f) => Self::Float(f), - Char(c) => Self::Char(c), - Str(s) => Self::Str(s.to_owned()), - Bytes(b) => Self::Bytes(b.to_owned()), - Unit => Self::Unit, - Option => Self::Option, - NewtypeStruct => Self::NewtypeStruct, - Seq => Self::Seq, - Map => Self::Map, - Enum => Self::Enum, - UnitVariant => Self::UnitVariant, - NewtypeVariant => Self::NewtypeVariant, - TupleVariant => Self::TupleVariant, - StructVariant => Self::StructVariant, - Other(o) => Self::Other(o.to_owned()), - } - } -} - -impl fmt::Display for UnexpectedSerdeTypeValue { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - use self::UnexpectedSerdeTypeValue::*; - - match *self { - Bool(b) => write!(f, "the boolean `{}`", b), - Unsigned(i) => write!(f, "the integer `{}`", i), - Signed(i) => write!(f, "the integer `{}`", i), - Float(n) => write!(f, "the floating point number `{}`", n), - Char(c) => write!(f, "the UTF-8 character `{}`", c), - Str(ref s) => write!(f, "the string {:?}", s), - Bytes(ref 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(ref other) => f.write_str(other), - } - } -} - struct OneOf { alts: &'static [&'static str], none: &'static str, 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 { - self.bytes.get(0).cloned() + self.bytes.first().cloned() } pub fn peek_or_eof(&self) -> Result { - self.bytes.get(0).cloned().ok_or(Error::Eof) + self.bytes.first().cloned().ok_or(Error::Eof) } pub fn signed_integer(&mut self) -> Result diff --git a/tests/203_error_positions.rs b/tests/203_error_positions.rs index e7ee7d25..0ef486a1 100644 --- a/tests/203_error_positions.rs +++ b/tests/203_error_positions.rs @@ -1,6 +1,6 @@ use std::num::NonZeroU32; -use ron::error::{Error, Position, SpannedError, UnexpectedSerdeTypeValue}; +use ron::error::{Error, Position, SpannedError}; use serde::{ de::{Deserialize, Error as DeError, Unexpected}, Deserializer, @@ -27,9 +27,9 @@ fn test_error_positions() { assert_eq!( ron::from_str::(" ()"), Err(SpannedError { - code: Error::ExpectedDifferentType { + code: Error::InvalidValueForType { expected: String::from("impossible"), - found: UnexpectedSerdeTypeValue::Unit, + found: String::from("a unit value"), }, position: Position { line: 1, col: 3 }, }) @@ -40,7 +40,7 @@ fn test_error_positions() { Err(SpannedError { code: Error::InvalidValueForType { expected: String::from("a nonzero u32"), - found: UnexpectedSerdeTypeValue::Unsigned(0), + found: String::from("the unsigned integer `0`"), }, position: Position { line: 1, col: 28 }, }) diff --git a/tests/359_deserialize_seed.rs b/tests/359_deserialize_seed.rs index 0d4df07d..4bca6c43 100644 --- a/tests/359_deserialize_seed.rs +++ b/tests/359_deserialize_seed.rs @@ -47,9 +47,9 @@ fn test_deserialize_seed() { assert_eq!( ron::Options::default().from_str_seed("'a'", Seed(42)), Err(ron::error::SpannedError { - code: ron::Error::ExpectedDifferentType { + code: ron::Error::InvalidValueForType { expected: String::from("an integer"), - found: ron::error::UnexpectedSerdeTypeValue::Str(String::from("a")), + found: String::from("the string \"a\""), }, position: ron::error::Position { line: 1, col: 4 }, }) 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, From 021eb4c6850e20cfb303efca60f700121388855a Mon Sep 17 00:00:00 2001 From: Juniper Langenstein Date: Mon, 15 Aug 2022 05:29:39 +0000 Subject: [PATCH 5/5] Added CHANGELOG entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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