From 98fb7d94aa3d5ba7055a07494ab561da7868a9cc Mon Sep 17 00:00:00 2001 From: Mingun Date: Sat, 17 Aug 2024 01:32:20 +0500 Subject: [PATCH 1/9] Move all untagged enum tests (except flatten) into a dedicated module Moved and renamed: From test_annotations - test_expecting_message_untagged_tagged_enum => expecting_message - flatten::enum_::untagged::straitforward => contains_flatten From test_macros - test_untagged_newtype_struct => newtype_struct - test_untagged_enum => complex - test_untagged_enum_with_flattened_integer_key => contains_flatten_with_integer_key - test_enum_in_untagged_enum => newtype_enum - test_untagged_bytes => string_and_bytes - test_untagged_newtype_variant_containing_unit_struct_not_map => newtype_unit_and_empty_map --- test_suite/tests/test_annotations.rs | 47 --- test_suite/tests/test_enum_untagged.rs | 409 +++++++++++++++++++++++++ test_suite/tests/test_macros.rs | 353 +-------------------- 3 files changed, 410 insertions(+), 399 deletions(-) create mode 100644 test_suite/tests/test_enum_untagged.rs diff --git a/test_suite/tests/test_annotations.rs b/test_suite/tests/test_annotations.rs index 9d03280d2..b26ec8722 100644 --- a/test_suite/tests/test_annotations.rs +++ b/test_suite/tests/test_annotations.rs @@ -1895,18 +1895,6 @@ fn test_expecting_message_externally_tagged_enum() { ); } -#[test] -fn test_expecting_message_untagged_tagged_enum() { - #[derive(Deserialize)] - #[serde(untagged)] - #[serde(expecting = "something strange...")] - enum Enum { - Untagged, - } - - assert_de_tokens_error::(&[Token::Str("Untagged")], "something strange..."); -} - #[test] fn test_expecting_message_identifier_enum() { #[derive(Deserialize)] @@ -2958,41 +2946,6 @@ mod flatten { mod untagged { use super::*; - #[test] - fn straightforward() { - #[derive(Serialize, Deserialize, PartialEq, Debug)] - #[serde(untagged)] - enum Data { - A { - a: i32, - #[serde(flatten)] - flat: Flat, - }, - } - - #[derive(Serialize, Deserialize, PartialEq, Debug)] - struct Flat { - b: i32, - } - - let data = Data::A { - a: 0, - flat: Flat { b: 0 }, - }; - - assert_tokens( - &data, - &[ - Token::Map { len: None }, - Token::Str("a"), - Token::I32(0), - Token::Str("b"), - Token::I32(0), - Token::MapEnd, - ], - ); - } - #[derive(Debug, PartialEq, Serialize, Deserialize)] struct Flatten { #[serde(flatten)] diff --git a/test_suite/tests/test_enum_untagged.rs b/test_suite/tests/test_enum_untagged.rs new file mode 100644 index 000000000..1c7ce542c --- /dev/null +++ b/test_suite/tests/test_enum_untagged.rs @@ -0,0 +1,409 @@ +#![deny(trivial_numeric_casts)] +#![allow( + clippy::derive_partial_eq_without_eq, + clippy::enum_variant_names, + clippy::redundant_field_names, + clippy::too_many_lines +)] + +mod bytes; + +use serde_derive::{Deserialize, Serialize}; +use serde_test::{assert_de_tokens, assert_de_tokens_error, assert_tokens, Token}; +use std::collections::BTreeMap; + +#[test] +fn complex() { + #[derive(Debug, PartialEq, Serialize, Deserialize)] + #[serde(untagged)] + enum Untagged { + A { a: u8 }, + B { b: u8 }, + C, + D(u8), + E(String), + F(u8, u8), + } + + assert_tokens( + &Untagged::A { a: 1 }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("a"), + Token::U8(1), + Token::StructEnd, + ], + ); + + assert_tokens( + &Untagged::B { b: 2 }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("b"), + Token::U8(2), + Token::StructEnd, + ], + ); + + // Serializes to unit, deserializes from either depending on format's + // preference. + assert_tokens(&Untagged::C, &[Token::Unit]); + assert_de_tokens(&Untagged::C, &[Token::None]); + + assert_tokens(&Untagged::D(4), &[Token::U8(4)]); + assert_tokens(&Untagged::E("e".to_owned()), &[Token::Str("e")]); + + assert_tokens( + &Untagged::F(1, 2), + &[ + Token::Tuple { len: 2 }, + Token::U8(1), + Token::U8(2), + Token::TupleEnd, + ], + ); + + assert_de_tokens_error::( + &[Token::Tuple { len: 1 }, Token::U8(1), Token::TupleEnd], + "data did not match any variant of untagged enum Untagged", + ); + + assert_de_tokens_error::( + &[ + Token::Tuple { len: 3 }, + Token::U8(1), + Token::U8(2), + Token::U8(3), + Token::TupleEnd, + ], + "data did not match any variant of untagged enum Untagged", + ); +} + +#[test] +fn newtype_unit_and_empty_map() { + #[derive(Debug, PartialEq, Serialize, Deserialize)] + struct Unit; + + #[derive(Debug, PartialEq, Serialize, Deserialize)] + #[serde(untagged)] + enum Message { + Unit(Unit), + Map(BTreeMap), + } + + assert_tokens( + &Message::Map(BTreeMap::new()), + &[Token::Map { len: Some(0) }, Token::MapEnd], + ); +} + +#[test] +fn newtype_struct() { + #[derive(Debug, PartialEq, Serialize, Deserialize)] + struct GenericNewTypeStruct(T); + + #[derive(Debug, PartialEq, Serialize, Deserialize)] + #[serde(untagged)] + enum E { + Newtype(GenericNewTypeStruct), + Null, + } + + assert_tokens( + &E::Newtype(GenericNewTypeStruct(5u32)), + &[ + Token::NewtypeStruct { + name: "GenericNewTypeStruct", + }, + Token::U32(5), + ], + ); +} + +#[test] +fn newtype_enum() { + #[derive(Debug, PartialEq, Serialize, Deserialize)] + #[serde(untagged)] + enum Outer { + Inner(Inner), + } + + #[derive(Debug, PartialEq, Serialize, Deserialize)] + enum Inner { + Unit, + Newtype(u8), + Tuple(u8, u8), + Struct { f: u8 }, + } + + assert_tokens( + &Outer::Inner(Inner::Unit), + &[Token::UnitVariant { + name: "Inner", + variant: "Unit", + }], + ); + + assert_tokens( + &Outer::Inner(Inner::Newtype(1)), + &[ + Token::NewtypeVariant { + name: "Inner", + variant: "Newtype", + }, + Token::U8(1), + ], + ); + + assert_tokens( + &Outer::Inner(Inner::Tuple(1, 1)), + &[ + Token::TupleVariant { + name: "Inner", + variant: "Tuple", + len: 2, + }, + Token::U8(1), + Token::U8(1), + Token::TupleVariantEnd, + ], + ); + + assert_tokens( + &Outer::Inner(Inner::Struct { f: 1 }), + &[ + Token::StructVariant { + name: "Inner", + variant: "Struct", + len: 1, + }, + Token::Str("f"), + Token::U8(1), + Token::StructVariantEnd, + ], + ); +} + +#[test] +fn string_and_bytes() { + #[derive(Debug, PartialEq, Deserialize)] + #[serde(untagged)] + enum Untagged { + String { + string: String, + }, + Bytes { + #[serde(with = "bytes")] + bytes: Vec, + }, + } + + assert_de_tokens( + &Untagged::String { + string: "\0".to_owned(), + }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("string"), + Token::Str("\0"), + Token::StructEnd, + ], + ); + + assert_de_tokens( + &Untagged::String { + string: "\0".to_owned(), + }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("string"), + Token::String("\0"), + Token::StructEnd, + ], + ); + + assert_de_tokens( + &Untagged::String { + string: "\0".to_owned(), + }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("string"), + Token::Bytes(b"\0"), + Token::StructEnd, + ], + ); + + assert_de_tokens( + &Untagged::String { + string: "\0".to_owned(), + }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("string"), + Token::ByteBuf(b"\0"), + Token::StructEnd, + ], + ); + + assert_de_tokens( + &Untagged::Bytes { bytes: vec![0] }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("bytes"), + Token::Str("\0"), + Token::StructEnd, + ], + ); + + assert_de_tokens( + &Untagged::Bytes { bytes: vec![0] }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("bytes"), + Token::String("\0"), + Token::StructEnd, + ], + ); + + assert_de_tokens( + &Untagged::Bytes { bytes: vec![0] }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("bytes"), + Token::Bytes(b"\0"), + Token::StructEnd, + ], + ); + + assert_de_tokens( + &Untagged::Bytes { bytes: vec![0] }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("bytes"), + Token::ByteBuf(b"\0"), + Token::StructEnd, + ], + ); + + assert_de_tokens( + &Untagged::Bytes { bytes: vec![0] }, + &[ + Token::Struct { + name: "Untagged", + len: 1, + }, + Token::Str("bytes"), + Token::Seq { len: Some(1) }, + Token::U8(0), + Token::SeqEnd, + Token::StructEnd, + ], + ); +} + +#[test] +fn contains_flatten() { + #[derive(Serialize, Deserialize, PartialEq, Debug)] + #[serde(untagged)] + enum Data { + A { + a: i32, + #[serde(flatten)] + flat: Flat, + }, + } + + #[derive(Serialize, Deserialize, PartialEq, Debug)] + struct Flat { + b: i32, + } + + let data = Data::A { + a: 0, + flat: Flat { b: 0 }, + }; + + assert_tokens( + &data, + &[ + Token::Map { len: None }, + Token::Str("a"), + Token::I32(0), + Token::Str("b"), + Token::I32(0), + Token::MapEnd, + ], + ); +} + +#[test] +fn contains_flatten_with_integer_key() { + #[derive(Debug, PartialEq, Serialize, Deserialize)] + #[serde(untagged)] + pub enum Untagged { + Variant { + #[serde(flatten)] + map: BTreeMap, + }, + } + + assert_tokens( + &Untagged::Variant { + map: { + let mut map = BTreeMap::new(); + map.insert(100, "BTreeMap".to_owned()); + map + }, + }, + &[ + Token::Map { len: None }, + Token::U64(100), + Token::Str("BTreeMap"), + Token::MapEnd, + ], + ); +} + +#[test] +fn expecting_message() { + #[derive(Deserialize)] + #[serde(untagged)] + #[serde(expecting = "something strange...")] + enum Enum { + Untagged, + } + + assert_de_tokens_error::(&[Token::Str("Untagged")], "something strange..."); +} diff --git a/test_suite/tests/test_macros.rs b/test_suite/tests/test_macros.rs index aaab95816..6b2fc4c5d 100644 --- a/test_suite/tests/test_macros.rs +++ b/test_suite/tests/test_macros.rs @@ -6,13 +6,8 @@ clippy::too_many_lines )] -mod bytes; - use serde_derive::{Deserialize, Serialize}; -use serde_test::{ - assert_de_tokens, assert_de_tokens_error, assert_ser_tokens, assert_tokens, Token, -}; -use std::collections::BTreeMap; +use serde_test::{assert_de_tokens, assert_ser_tokens, assert_tokens, Token}; use std::marker::PhantomData; // That tests that the derived Serialize implementation doesn't trigger @@ -433,26 +428,6 @@ fn test_generic_newtype_struct() { ); } -#[test] -fn test_untagged_newtype_struct() { - #[derive(Debug, PartialEq, Serialize, Deserialize)] - #[serde(untagged)] - enum E { - Newtype(GenericNewTypeStruct), - Null, - } - - assert_tokens( - &E::Newtype(GenericNewTypeStruct(5u32)), - &[ - Token::NewtypeStruct { - name: "GenericNewTypeStruct", - }, - Token::U32(5), - ], - ); -} - #[test] fn test_generic_tuple_struct() { assert_tokens( @@ -577,80 +552,6 @@ fn test_enum_state_field() { ); } -#[test] -fn test_untagged_enum() { - #[derive(Debug, PartialEq, Serialize, Deserialize)] - #[serde(untagged)] - enum Untagged { - A { a: u8 }, - B { b: u8 }, - C, - D(u8), - E(String), - F(u8, u8), - } - - assert_tokens( - &Untagged::A { a: 1 }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("a"), - Token::U8(1), - Token::StructEnd, - ], - ); - - assert_tokens( - &Untagged::B { b: 2 }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("b"), - Token::U8(2), - Token::StructEnd, - ], - ); - - // Serializes to unit, deserializes from either depending on format's - // preference. - assert_tokens(&Untagged::C, &[Token::Unit]); - assert_de_tokens(&Untagged::C, &[Token::None]); - - assert_tokens(&Untagged::D(4), &[Token::U8(4)]); - assert_tokens(&Untagged::E("e".to_owned()), &[Token::Str("e")]); - - assert_tokens( - &Untagged::F(1, 2), - &[ - Token::Tuple { len: 2 }, - Token::U8(1), - Token::U8(2), - Token::TupleEnd, - ], - ); - - assert_de_tokens_error::( - &[Token::Tuple { len: 1 }, Token::U8(1), Token::TupleEnd], - "data did not match any variant of untagged enum Untagged", - ); - - assert_de_tokens_error::( - &[ - Token::Tuple { len: 3 }, - Token::U8(1), - Token::U8(2), - Token::U8(3), - Token::TupleEnd, - ], - "data did not match any variant of untagged enum Untagged", - ); -} - #[test] fn test_internally_tagged_struct() { #[derive(Debug, PartialEq, Serialize, Deserialize)] @@ -750,240 +651,6 @@ fn test_internally_tagged_struct_with_flattened_field() { ); } -#[test] -fn test_untagged_enum_with_flattened_integer_key() { - #[derive(Debug, PartialEq, Serialize, Deserialize)] - #[serde(untagged)] - pub enum Untagged { - Variant { - #[serde(flatten)] - map: BTreeMap, - }, - } - - assert_tokens( - &Untagged::Variant { - map: { - let mut map = BTreeMap::new(); - map.insert(100, "BTreeMap".to_owned()); - map - }, - }, - &[ - Token::Map { len: None }, - Token::U64(100), - Token::Str("BTreeMap"), - Token::MapEnd, - ], - ); -} - -#[test] -fn test_enum_in_untagged_enum() { - #[derive(Debug, PartialEq, Serialize, Deserialize)] - #[serde(untagged)] - enum Outer { - Inner(Inner), - } - - #[derive(Debug, PartialEq, Serialize, Deserialize)] - enum Inner { - Unit, - Newtype(u8), - Tuple(u8, u8), - Struct { f: u8 }, - } - - assert_tokens( - &Outer::Inner(Inner::Unit), - &[Token::UnitVariant { - name: "Inner", - variant: "Unit", - }], - ); - - assert_tokens( - &Outer::Inner(Inner::Newtype(1)), - &[ - Token::NewtypeVariant { - name: "Inner", - variant: "Newtype", - }, - Token::U8(1), - ], - ); - - assert_tokens( - &Outer::Inner(Inner::Tuple(1, 1)), - &[ - Token::TupleVariant { - name: "Inner", - variant: "Tuple", - len: 2, - }, - Token::U8(1), - Token::U8(1), - Token::TupleVariantEnd, - ], - ); - - assert_tokens( - &Outer::Inner(Inner::Struct { f: 1 }), - &[ - Token::StructVariant { - name: "Inner", - variant: "Struct", - len: 1, - }, - Token::Str("f"), - Token::U8(1), - Token::StructVariantEnd, - ], - ); -} - -#[test] -fn test_untagged_bytes() { - #[derive(Debug, PartialEq, Deserialize)] - #[serde(untagged)] - enum Untagged { - String { - string: String, - }, - Bytes { - #[serde(with = "bytes")] - bytes: Vec, - }, - } - - assert_de_tokens( - &Untagged::String { - string: "\0".to_owned(), - }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("string"), - Token::Str("\0"), - Token::StructEnd, - ], - ); - - assert_de_tokens( - &Untagged::String { - string: "\0".to_owned(), - }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("string"), - Token::String("\0"), - Token::StructEnd, - ], - ); - - assert_de_tokens( - &Untagged::String { - string: "\0".to_owned(), - }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("string"), - Token::Bytes(b"\0"), - Token::StructEnd, - ], - ); - - assert_de_tokens( - &Untagged::String { - string: "\0".to_owned(), - }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("string"), - Token::ByteBuf(b"\0"), - Token::StructEnd, - ], - ); - - assert_de_tokens( - &Untagged::Bytes { bytes: vec![0] }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("bytes"), - Token::Str("\0"), - Token::StructEnd, - ], - ); - - assert_de_tokens( - &Untagged::Bytes { bytes: vec![0] }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("bytes"), - Token::String("\0"), - Token::StructEnd, - ], - ); - - assert_de_tokens( - &Untagged::Bytes { bytes: vec![0] }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("bytes"), - Token::Bytes(b"\0"), - Token::StructEnd, - ], - ); - - assert_de_tokens( - &Untagged::Bytes { bytes: vec![0] }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("bytes"), - Token::ByteBuf(b"\0"), - Token::StructEnd, - ], - ); - - assert_de_tokens( - &Untagged::Bytes { bytes: vec![0] }, - &[ - Token::Struct { - name: "Untagged", - len: 1, - }, - Token::Str("bytes"), - Token::Seq { len: Some(1) }, - Token::U8(0), - Token::SeqEnd, - Token::StructEnd, - ], - ); -} - #[test] fn test_rename_all() { #[derive(Serialize, Deserialize, Debug, PartialEq)] @@ -1167,24 +834,6 @@ fn test_rename_all_fields() { ); } -#[test] -fn test_untagged_newtype_variant_containing_unit_struct_not_map() { - #[derive(Debug, PartialEq, Serialize, Deserialize)] - struct Unit; - - #[derive(Debug, PartialEq, Serialize, Deserialize)] - #[serde(untagged)] - enum Message { - Unit(Unit), - Map(BTreeMap), - } - - assert_tokens( - &Message::Map(BTreeMap::new()), - &[Token::Map { len: Some(0) }, Token::MapEnd], - ); -} - #[test] fn test_packed_struct_can_derive_serialize() { #[derive(Copy, Clone, Serialize)] From 8514f4119aa1833bbd7a73231fedcbfeb5970e78 Mon Sep 17 00:00:00 2001 From: Mingun Date: Sat, 17 Aug 2024 12:48:27 +0500 Subject: [PATCH 2/9] Remove unnecessary generics --- test_suite/tests/test_enum_untagged.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test_suite/tests/test_enum_untagged.rs b/test_suite/tests/test_enum_untagged.rs index 1c7ce542c..d06002ce8 100644 --- a/test_suite/tests/test_enum_untagged.rs +++ b/test_suite/tests/test_enum_untagged.rs @@ -107,20 +107,20 @@ fn newtype_unit_and_empty_map() { #[test] fn newtype_struct() { #[derive(Debug, PartialEq, Serialize, Deserialize)] - struct GenericNewTypeStruct(T); + struct NewtypeStruct(u32); #[derive(Debug, PartialEq, Serialize, Deserialize)] #[serde(untagged)] enum E { - Newtype(GenericNewTypeStruct), + Newtype(NewtypeStruct), Null, } assert_tokens( - &E::Newtype(GenericNewTypeStruct(5u32)), + &E::Newtype(NewtypeStruct(5)), &[ Token::NewtypeStruct { - name: "GenericNewTypeStruct", + name: "NewtypeStruct", }, Token::U32(5), ], From 2dddc7796d436b2bfa0d1016d252250a3f7c4d18 Mon Sep 17 00:00:00 2001 From: Mingun Date: Sat, 17 Aug 2024 11:58:28 +0500 Subject: [PATCH 3/9] Cover ContentRefDeserializer::deserialize_option --- serde/src/private/de.rs | 7 +++ test_suite/tests/test_enum_untagged.rs | 74 ++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/serde/src/private/de.rs b/serde/src/private/de.rs index 6c81b720b..5b5d9b6a1 100644 --- a/serde/src/private/de.rs +++ b/serde/src/private/de.rs @@ -1898,10 +1898,17 @@ mod content { where V: Visitor<'de>, { + // Covered by tests/test_enum_untagged.rs + // with_optional_field::* match *self.content { Content::None => visitor.visit_none(), Content::Some(ref v) => visitor.visit_some(ContentRefDeserializer::new(v)), Content::Unit => visitor.visit_unit(), + // This case is necessary for formats which does not store marker of optionality of value, + // for example, JSON. When `deserialize_any` is requested from such formats, they will + // report value without using `Visitor::visit_some`, because they do not known in which + // contexts this value will be used. + // RON is example of format which preserve markers. _ => visitor.visit_some(self), } } diff --git a/test_suite/tests/test_enum_untagged.rs b/test_suite/tests/test_enum_untagged.rs index d06002ce8..60c62bac6 100644 --- a/test_suite/tests/test_enum_untagged.rs +++ b/test_suite/tests/test_enum_untagged.rs @@ -191,6 +191,80 @@ fn newtype_enum() { ); } +// Reaches crate::private::de::content::ContentRefDeserializer::deserialize_option +mod with_optional_field { + use super::*; + + #[derive(Debug, PartialEq, Serialize, Deserialize)] + #[serde(untagged)] + enum Enum { + Struct { optional: Option }, + Null, + } + + #[test] + fn some() { + assert_tokens( + &Enum::Struct { optional: Some(42) }, + &[ + Token::Struct { + name: "Enum", + len: 1, + }, + Token::Str("optional"), + Token::Some, + Token::U32(42), + Token::StructEnd, + ], + ); + } + + #[test] + fn some_without_marker() { + assert_de_tokens( + &Enum::Struct { optional: Some(42) }, + &[ + Token::Struct { + name: "Enum", + len: 1, + }, + Token::Str("optional"), + Token::U32(42), + Token::StructEnd, + ], + ); + } + + #[test] + fn none() { + assert_tokens( + &Enum::Struct { optional: None }, + &[ + Token::Struct { + name: "Enum", + len: 1, + }, + Token::Str("optional"), + Token::None, + Token::StructEnd, + ], + ); + } + + #[test] + fn unit() { + assert_de_tokens( + &Enum::Struct { optional: None }, + &[ + Token::Map { len: None }, + Token::Str("optional"), + Token::Unit, + Token::MapEnd, + ], + ); + } +} + #[test] fn string_and_bytes() { #[derive(Debug, PartialEq, Deserialize)] From 171c6da57af712cfcf01c6c124b14cabfca364ba Mon Sep 17 00:00:00 2001 From: Mingun Date: Sat, 17 Aug 2024 11:31:50 +0500 Subject: [PATCH 4/9] Complete coverage of ContentRefDeserializer::deserialize_newtype_struct --- serde/src/private/de.rs | 7 +++++++ test_suite/tests/test_enum_untagged.rs | 9 ++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/serde/src/private/de.rs b/serde/src/private/de.rs index 5b5d9b6a1..846647e0c 100644 --- a/serde/src/private/de.rs +++ b/serde/src/private/de.rs @@ -1938,10 +1938,17 @@ mod content { where V: Visitor<'de>, { + // Covered by tests/test_enum_untagged.rs + // newtype_struct match *self.content { Content::Newtype(ref v) => { visitor.visit_newtype_struct(ContentRefDeserializer::new(v)) } + // This case is necessary for formats which does not store marker of a newtype, + // for example, JSON. When `deserialize_any` is requested from such formats, they will + // report value without using `Visitor::visit_newtype_struct`, because they do not + // known in which contexts this value will be used. + // RON is example of format which preserve markers. _ => visitor.visit_newtype_struct(self), } } diff --git a/test_suite/tests/test_enum_untagged.rs b/test_suite/tests/test_enum_untagged.rs index 60c62bac6..9079eeb60 100644 --- a/test_suite/tests/test_enum_untagged.rs +++ b/test_suite/tests/test_enum_untagged.rs @@ -104,6 +104,7 @@ fn newtype_unit_and_empty_map() { ); } +// Reaches crate::private::de::content::ContentRefDeserializer::deserialize_newtype_struct #[test] fn newtype_struct() { #[derive(Debug, PartialEq, Serialize, Deserialize)] @@ -116,8 +117,11 @@ fn newtype_struct() { Null, } + let value = E::Newtype(NewtypeStruct(5)); + + // Content::Newtype case assert_tokens( - &E::Newtype(NewtypeStruct(5)), + &value, &[ Token::NewtypeStruct { name: "NewtypeStruct", @@ -125,6 +129,9 @@ fn newtype_struct() { Token::U32(5), ], ); + + // _ case + assert_de_tokens(&value, &[Token::U32(5)]); } #[test] From 0093f74cfee5ee3239514a7aad5fb44843eddcdd Mon Sep 17 00:00:00 2001 From: Mingun Date: Sat, 17 Aug 2024 14:53:41 +0500 Subject: [PATCH 5/9] Split test newtype_enum into four tests for each variant (review this commit with "ignore whitespace changes" option on) --- serde/src/private/de.rs | 16 ++++ test_suite/tests/test_enum_untagged.rs | 110 ++++++++++++++----------- 2 files changed, 80 insertions(+), 46 deletions(-) diff --git a/serde/src/private/de.rs b/serde/src/private/de.rs index 846647e0c..864f93b02 100644 --- a/serde/src/private/de.rs +++ b/serde/src/private/de.rs @@ -2153,6 +2153,10 @@ mod content { fn unit_variant(self) -> Result<(), E> { match self.value { Some(value) => de::Deserialize::deserialize(ContentRefDeserializer::new(value)), + // Covered by tests/test_annotations.rs + // test_partially_untagged_adjacently_tagged_enum + // Covered by tests/test_enum_untagged.rs + // newtype_enum::unit None => Ok(()), } } @@ -2162,6 +2166,11 @@ mod content { T: de::DeserializeSeed<'de>, { match self.value { + // Covered by tests/test_annotations.rs + // test_partially_untagged_enum_desugared + // test_partially_untagged_enum_generic + // Covered by tests/test_enum_untagged.rs + // newtype_enum::newtype Some(value) => seed.deserialize(ContentRefDeserializer::new(value)), None => Err(de::Error::invalid_type( de::Unexpected::UnitVariant, @@ -2175,6 +2184,11 @@ mod content { V: de::Visitor<'de>, { match self.value { + // Covered by tests/test_annotations.rs + // test_partially_untagged_enum + // test_partially_untagged_enum_desugared + // Covered by tests/test_enum_untagged.rs + // newtype_enum::tuple2 Some(Content::Seq(v)) => { de::Deserializer::deserialize_any(SeqRefDeserializer::new(v), visitor) } @@ -2198,6 +2212,8 @@ mod content { V: de::Visitor<'de>, { match self.value { + // Covered by tests/test_enum_untagged.rs + // newtype_enum::struct_from_map Some(Content::Map(v)) => { de::Deserializer::deserialize_any(MapRefDeserializer::new(v), visitor) } diff --git a/test_suite/tests/test_enum_untagged.rs b/test_suite/tests/test_enum_untagged.rs index 9079eeb60..8c212a970 100644 --- a/test_suite/tests/test_enum_untagged.rs +++ b/test_suite/tests/test_enum_untagged.rs @@ -134,8 +134,9 @@ fn newtype_struct() { assert_de_tokens(&value, &[Token::U32(5)]); } -#[test] -fn newtype_enum() { +mod newtype_enum { + use super::*; + #[derive(Debug, PartialEq, Serialize, Deserialize)] #[serde(untagged)] enum Outer { @@ -146,56 +147,73 @@ fn newtype_enum() { enum Inner { Unit, Newtype(u8), - Tuple(u8, u8), + Tuple2(u8, u8), Struct { f: u8 }, } - assert_tokens( - &Outer::Inner(Inner::Unit), - &[Token::UnitVariant { - name: "Inner", - variant: "Unit", - }], - ); - - assert_tokens( - &Outer::Inner(Inner::Newtype(1)), - &[ - Token::NewtypeVariant { + // Reaches crate::private::de::content::VariantRefDeserializer::unit_variant + #[test] + fn unit() { + assert_tokens( + &Outer::Inner(Inner::Unit), + &[Token::UnitVariant { name: "Inner", - variant: "Newtype", - }, - Token::U8(1), - ], - ); + variant: "Unit", + }], + ); + } - assert_tokens( - &Outer::Inner(Inner::Tuple(1, 1)), - &[ - Token::TupleVariant { - name: "Inner", - variant: "Tuple", - len: 2, - }, - Token::U8(1), - Token::U8(1), - Token::TupleVariantEnd, - ], - ); + // Reaches crate::private::de::content::VariantRefDeserializer::newtype_variant_seed + #[test] + fn newtype() { + assert_tokens( + &Outer::Inner(Inner::Newtype(1)), + &[ + Token::NewtypeVariant { + name: "Inner", + variant: "Newtype", + }, + Token::U8(1), + ], + ); + } - assert_tokens( - &Outer::Inner(Inner::Struct { f: 1 }), - &[ - Token::StructVariant { - name: "Inner", - variant: "Struct", - len: 1, - }, - Token::Str("f"), - Token::U8(1), - Token::StructVariantEnd, - ], - ); + // Reaches crate::private::de::content::VariantRefDeserializer::tuple_variant + #[test] + fn tuple2() { + assert_tokens( + &Outer::Inner(Inner::Tuple2(1, 1)), + &[ + Token::TupleVariant { + name: "Inner", + variant: "Tuple2", + len: 2, + }, + Token::U8(1), + Token::U8(1), + Token::TupleVariantEnd, + ], + ); + } + + // Reaches crate::private::de::content::VariantRefDeserializer::struct_variant + // Content::Map case + #[test] + fn struct_from_map() { + assert_tokens( + &Outer::Inner(Inner::Struct { f: 1 }), + &[ + Token::StructVariant { + name: "Inner", + variant: "Struct", + len: 1, + }, + Token::Str("f"), + Token::U8(1), + Token::StructVariantEnd, + ], + ); + } } // Reaches crate::private::de::content::ContentRefDeserializer::deserialize_option From 6588b0ad3777f7ad930d68ab4b9ec5b9c25398e0 Mon Sep 17 00:00:00 2001 From: Mingun Date: Sat, 17 Aug 2024 15:26:59 +0500 Subject: [PATCH 6/9] Cover Content::Seq case in VariantRefDeserializer::struct_variant --- serde/src/private/de.rs | 2 ++ test_suite/tests/test_enum_untagged.rs | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/serde/src/private/de.rs b/serde/src/private/de.rs index 864f93b02..0c6f4d3a0 100644 --- a/serde/src/private/de.rs +++ b/serde/src/private/de.rs @@ -2217,6 +2217,8 @@ mod content { Some(Content::Map(v)) => { de::Deserializer::deserialize_any(MapRefDeserializer::new(v), visitor) } + // Covered by tests/test_enum_untagged.rs + // newtype_enum::struct_from_seq Some(Content::Seq(v)) => { de::Deserializer::deserialize_any(SeqRefDeserializer::new(v), visitor) } diff --git a/test_suite/tests/test_enum_untagged.rs b/test_suite/tests/test_enum_untagged.rs index 8c212a970..01ecae9c1 100644 --- a/test_suite/tests/test_enum_untagged.rs +++ b/test_suite/tests/test_enum_untagged.rs @@ -214,6 +214,25 @@ mod newtype_enum { ], ); } + + // Reaches crate::private::de::content::VariantRefDeserializer::struct_variant + // Content::Seq case + #[test] + fn struct_from_seq() { + assert_de_tokens( + &Outer::Inner(Inner::Struct { f: 1 }), + &[ + Token::Map { len: Some(1) }, + // tag + Token::Str("Struct"), + // content + Token::Seq { len: Some(1) }, + Token::U8(1), + Token::SeqEnd, + Token::MapEnd, + ], + ); + } } // Reaches crate::private::de::content::ContentRefDeserializer::deserialize_option From 4c5fec1363d363f995375426f72db11c28f357c1 Mon Sep 17 00:00:00 2001 From: Mingun Date: Sat, 17 Aug 2024 16:05:34 +0500 Subject: [PATCH 7/9] Test special cases that reaches SeqRefDeserializer::deserialize_any len==0 condition failures (2): newtype_enum::empty_struct_from_seq newtype_enum::tuple0 --- serde/src/private/de.rs | 2 + test_suite/tests/test_enum_untagged.rs | 56 ++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/serde/src/private/de.rs b/serde/src/private/de.rs index 0c6f4d3a0..cb34f1c31 100644 --- a/serde/src/private/de.rs +++ b/serde/src/private/de.rs @@ -2188,6 +2188,7 @@ mod content { // test_partially_untagged_enum // test_partially_untagged_enum_desugared // Covered by tests/test_enum_untagged.rs + // newtype_enum::tuple0 // newtype_enum::tuple2 Some(Content::Seq(v)) => { de::Deserializer::deserialize_any(SeqRefDeserializer::new(v), visitor) @@ -2219,6 +2220,7 @@ mod content { } // Covered by tests/test_enum_untagged.rs // newtype_enum::struct_from_seq + // newtype_enum::empty_struct_from_seq Some(Content::Seq(v)) => { de::Deserializer::deserialize_any(SeqRefDeserializer::new(v), visitor) } diff --git a/test_suite/tests/test_enum_untagged.rs b/test_suite/tests/test_enum_untagged.rs index 01ecae9c1..48f50c9c2 100644 --- a/test_suite/tests/test_enum_untagged.rs +++ b/test_suite/tests/test_enum_untagged.rs @@ -147,8 +147,10 @@ mod newtype_enum { enum Inner { Unit, Newtype(u8), + Tuple0(), Tuple2(u8, u8), Struct { f: u8 }, + EmptyStruct {}, } // Reaches crate::private::de::content::VariantRefDeserializer::unit_variant @@ -178,6 +180,22 @@ mod newtype_enum { ); } + // Reaches crate::private::de::content::VariantRefDeserializer::tuple_variant + #[test] + fn tuple0() { + assert_tokens( + &Outer::Inner(Inner::Tuple0()), + &[ + Token::TupleVariant { + name: "Inner", + variant: "Tuple0", + len: 0, + }, + Token::TupleVariantEnd, + ], + ); + } + // Reaches crate::private::de::content::VariantRefDeserializer::tuple_variant #[test] fn tuple2() { @@ -233,6 +251,44 @@ mod newtype_enum { ], ); } + + // Reaches crate::private::de::content::VariantRefDeserializer::struct_variant + // Content::Map case + // Special case - empty map + #[test] + fn empty_struct_from_map() { + assert_de_tokens( + &Outer::Inner(Inner::EmptyStruct {}), + &[ + Token::Map { len: Some(1) }, + // tag + Token::Str("EmptyStruct"), + // content + Token::Map { len: Some(0) }, + Token::MapEnd, + Token::MapEnd, + ], + ); + } + + // Reaches crate::private::de::content::VariantRefDeserializer::struct_variant + // Content::Seq case + // Special case - empty seq + #[test] + fn empty_struct_from_seq() { + assert_de_tokens( + &Outer::Inner(Inner::EmptyStruct {}), + &[ + Token::Map { len: Some(1) }, + // tag + Token::Str("EmptyStruct"), + // content + Token::Seq { len: Some(0) }, + Token::SeqEnd, + Token::MapEnd, + ], + ); + } } // Reaches crate::private::de::content::ContentRefDeserializer::deserialize_option From da7fc795ee654252effa232a62a5a1e6d4f551ee Mon Sep 17 00:00:00 2001 From: Mingun Date: Sat, 17 Aug 2024 16:32:36 +0500 Subject: [PATCH 8/9] Fix deserialization of empty struct variant in untagged enums SeqRefDeserializer::deserialize_any has a special condition for empty sequence, which emits visit_unit. That condition assumes that type would be able to deserialized from unit, but: 1) struct variants was never able to deserialize from it (they expect only visit_map or visit_seq) 2) tuple variants even with zero fields expect visit_seq only. The suggestion to accept visit_unit instead was rejected in #2520 Fixes (2): newtype_enum::tuple0 newtype_enum::empty_struct_from_seq --- serde/src/private/de.rs | 83 +---------------------------------------- 1 file changed, 2 insertions(+), 81 deletions(-) diff --git a/serde/src/private/de.rs b/serde/src/private/de.rs index cb34f1c31..be2b119d2 100644 --- a/serde/src/private/de.rs +++ b/serde/src/private/de.rs @@ -2190,9 +2190,7 @@ mod content { // Covered by tests/test_enum_untagged.rs // newtype_enum::tuple0 // newtype_enum::tuple2 - Some(Content::Seq(v)) => { - de::Deserializer::deserialize_any(SeqRefDeserializer::new(v), visitor) - } + Some(Content::Seq(v)) => visit_content_seq_ref(v, visitor), Some(other) => Err(de::Error::invalid_type( other.unexpected(), &"tuple variant", @@ -2221,9 +2219,7 @@ mod content { // Covered by tests/test_enum_untagged.rs // newtype_enum::struct_from_seq // newtype_enum::empty_struct_from_seq - Some(Content::Seq(v)) => { - de::Deserializer::deserialize_any(SeqRefDeserializer::new(v), visitor) - } + Some(Content::Seq(v)) => visit_content_seq_ref(v, visitor), Some(other) => Err(de::Error::invalid_type( other.unexpected(), &"struct variant", @@ -2236,81 +2232,6 @@ mod content { } } - struct SeqRefDeserializer<'a, 'de: 'a, E> - where - E: de::Error, - { - iter: <&'a [Content<'de>] as IntoIterator>::IntoIter, - err: PhantomData, - } - - impl<'a, 'de, E> SeqRefDeserializer<'a, 'de, E> - where - E: de::Error, - { - fn new(slice: &'a [Content<'de>]) -> Self { - SeqRefDeserializer { - iter: slice.iter(), - err: PhantomData, - } - } - } - - impl<'de, 'a, E> de::Deserializer<'de> for SeqRefDeserializer<'a, 'de, E> - where - E: de::Error, - { - type Error = E; - - #[inline] - fn deserialize_any(mut self, visitor: V) -> Result - where - V: de::Visitor<'de>, - { - let len = self.iter.len(); - if len == 0 { - visitor.visit_unit() - } else { - let ret = tri!(visitor.visit_seq(&mut self)); - let remaining = self.iter.len(); - if remaining == 0 { - Ok(ret) - } else { - Err(de::Error::invalid_length(len, &"fewer elements in array")) - } - } - } - - forward_to_deserialize_any! { - bool i8 i16 i32 i64 i128 u8 u16 u32 u64 u128 f32 f64 char str string - bytes byte_buf option unit unit_struct newtype_struct seq tuple - tuple_struct map struct enum identifier ignored_any - } - } - - impl<'de, 'a, E> de::SeqAccess<'de> for SeqRefDeserializer<'a, 'de, E> - where - E: de::Error, - { - type Error = E; - - fn next_element_seed(&mut self, seed: T) -> Result, Self::Error> - where - T: de::DeserializeSeed<'de>, - { - match self.iter.next() { - Some(value) => seed - .deserialize(ContentRefDeserializer::new(value)) - .map(Some), - None => Ok(None), - } - } - - fn size_hint(&self) -> Option { - size_hint::from_bounds(&self.iter) - } - } - struct MapRefDeserializer<'a, 'de: 'a, E> where E: de::Error, From 7bde100237875d4f435de5ad90074b0479c37486 Mon Sep 17 00:00:00 2001 From: Mingun Date: Sat, 17 Aug 2024 17:09:53 +0500 Subject: [PATCH 9/9] Replace MapRefDeserializer with value::MapDeserializer Although they are slightly different, this difference is irrelevant: - MapDeserializer has a specialization for deserialize_seq and deserialize_tuple, but only MapRefDeserializer::deserialize_any is used by the code which is almost the same - MapDeserializer checks that map was consumed after visit_map, but MapRefDeserializer does not. Actually, each derived implementation consumes map and each manual implementation also should consume it Also, MapDeserializer already used when value deserialized from ContentRefDeserializer directly and MapRefDeserializer was only used to deserialize Struct variants of enums. There are no reasons why the behavior should be different in those two cases --- serde/src/private/de.rs | 81 +---------------------------------------- 1 file changed, 1 insertion(+), 80 deletions(-) diff --git a/serde/src/private/de.rs b/serde/src/private/de.rs index be2b119d2..2682136cb 100644 --- a/serde/src/private/de.rs +++ b/serde/src/private/de.rs @@ -2213,9 +2213,7 @@ mod content { match self.value { // Covered by tests/test_enum_untagged.rs // newtype_enum::struct_from_map - Some(Content::Map(v)) => { - de::Deserializer::deserialize_any(MapRefDeserializer::new(v), visitor) - } + Some(Content::Map(v)) => visit_content_map_ref(v, visitor), // Covered by tests/test_enum_untagged.rs // newtype_enum::struct_from_seq // newtype_enum::empty_struct_from_seq @@ -2232,83 +2230,6 @@ mod content { } } - struct MapRefDeserializer<'a, 'de: 'a, E> - where - E: de::Error, - { - iter: <&'a [(Content<'de>, Content<'de>)] as IntoIterator>::IntoIter, - value: Option<&'a Content<'de>>, - err: PhantomData, - } - - impl<'a, 'de, E> MapRefDeserializer<'a, 'de, E> - where - E: de::Error, - { - fn new(map: &'a [(Content<'de>, Content<'de>)]) -> Self { - MapRefDeserializer { - iter: map.iter(), - value: None, - err: PhantomData, - } - } - } - - impl<'de, 'a, E> de::MapAccess<'de> for MapRefDeserializer<'a, 'de, E> - where - E: de::Error, - { - type Error = E; - - fn next_key_seed(&mut self, seed: T) -> Result, Self::Error> - where - T: de::DeserializeSeed<'de>, - { - match self.iter.next() { - Some((key, value)) => { - self.value = Some(value); - seed.deserialize(ContentRefDeserializer::new(key)).map(Some) - } - None => Ok(None), - } - } - - fn next_value_seed(&mut self, seed: T) -> Result - where - T: de::DeserializeSeed<'de>, - { - match self.value.take() { - Some(value) => seed.deserialize(ContentRefDeserializer::new(value)), - None => Err(de::Error::custom("value is missing")), - } - } - - fn size_hint(&self) -> Option { - size_hint::from_bounds(&self.iter) - } - } - - impl<'de, 'a, E> de::Deserializer<'de> for MapRefDeserializer<'a, 'de, E> - where - E: de::Error, - { - type Error = E; - - #[inline] - fn deserialize_any(self, visitor: V) -> Result - where - V: de::Visitor<'de>, - { - visitor.visit_map(self) - } - - forward_to_deserialize_any! { - bool i8 i16 i32 i64 i128 u8 u16 u32 u64 u128 f32 f64 char str string - bytes byte_buf option unit unit_struct newtype_struct seq tuple - tuple_struct map struct enum identifier ignored_any - } - } - impl<'de, E> de::IntoDeserializer<'de, E> for ContentDeserializer<'de, E> where E: de::Error,