Repository: 3Hren/msgpack-rust
Pull request #286 (2021-06-25 17:43:43)

Use string variants
This is a continuation of https://github.com/3Hren/msgpack-rust/pull/285

vedantroy: Note: I'm sure there will be some criticisms of removing the ability to serialize enums to integers, but I think this use case is still supported if people use https://github.com/dtolnay/serde-repr, and more importantly, a ton of normal serde behavior breaks b/c enums are serialized to maps.

For example, for manual deserialization of structs, serde uses a `Field` enum (https://serde.rs/deserialize-struct.html). But, this will break with the current method (which expects enums to be serialized as maps w/ a single K/V).


kornelski: Does it have to be a string, specifically? I mean, was the problem that it was a map, or was the problem that it wasn't a string?

For example, could it serialize as a bare integer, or a 1-byte array that contains the numeric tag?

vedantroy: It can be a map with a single K/V pair, otherwise it must be a string.

See: https://github.com/serde-rs/serde/issues/2046

Patch:
diff --git a/rmp-serde/src/config.rs b/rmp-serde/src/config.rs
index 460063e..2b52c9a 100644
--- a/rmp-serde/src/config.rs
+++ b/rmp-serde/src/config.rs
@@ -86,14 +86,14 @@ impl sealed::SerializerConfig for DefaultConfig {
     #[inline]
     fn write_variant_ident<S>(
         ser: &mut S,
-        variant_index: u32,
-        _variant: &'static str,
+        _variant_index: u32,
+        variant: &'static str,
     ) -> Result<(), Error>
     where
         S: UnderlyingWrite,
         for<'a> &'a mut S: Serializer<Ok = (), Error = Error>,
     {
-        ser.serialize_u32(variant_index)
+        ser.serialize_str(variant)
     }
 
     #[inline(always)]
@@ -219,117 +219,6 @@ where
     }
 }
 
-/// Config wrapper, that overrides enum serialization by serializing enum variant names as strings.
-///
-/// Default `Serializer` implementation writes enum names as integers, i.e. only indices are encoded,
-/// because it is the most compact representation.
-#[derive(Copy, Clone, Debug)]
-pub struct VariantStringConfig<C>(C);
-
-impl<C> VariantStringConfig<C> {
-    /// Creates a `VariantStringConfig` inheriting unchanged configuration options from the given configuration.
-    #[inline]
-    pub fn new(inner: C) -> Self {
-        VariantStringConfig(inner)
-    }
-}
-
-impl<C> sealed::SerializerConfig for VariantStringConfig<C>
-where
-    C: sealed::SerializerConfig,
-{
-    #[inline]
-    fn write_struct_len<S>(ser: &mut S, len: usize) -> Result<(), Error>
-    where
-        S: UnderlyingWrite,
-        for<'a> &'a mut S: Serializer<Ok = (), Error = Error>,
-    {
-        C::write_struct_len(ser, len)
-    }
-
-    #[inline]
-    fn write_struct_field<S, T>(ser: &mut S, key: &'static str, value: &T) -> Result<(), Error>
-    where
-        S: UnderlyingWrite,
-        for<'a> &'a mut S: Serializer<Ok = (), Error = Error>,
-        T: ?Sized + Serialize,
-    {
-        C::write_struct_field(ser, key, value)
-    }
-
-    #[inline]
-    fn write_variant_ident<S>(
-        ser: &mut S,
-        _variant_index: u32,
-        variant: &'static str,
-    ) -> Result<(), Error>
-    where
-        S: UnderlyingWrite,
-        for<'a> &'a mut S: Serializer<Ok = (), Error = Error>,
-    {
-        ser.serialize_str(variant)
-    }
-
-    #[inline(always)]
-    fn is_human_readable() -> bool {
-        C::is_human_readable()
-    }
-}
-
-/// Config wrapper that overrides enum variant serialization by packing enum names as their integer indices.
-#[derive(Copy, Clone, Debug)]
-pub struct VariantIntegerConfig<C>(C);
-
-impl<C> VariantIntegerConfig<C> {
-    /// Creates a `VariantIntegerConfig` inheriting unchanged configuration options from the given configuration.
-    #[inline]
-    pub fn new(inner: C) -> Self {
-        VariantIntegerConfig(inner)
-    }
-}
-
-impl<C> sealed::SerializerConfig for VariantIntegerConfig<C>
-where
-    C: sealed::SerializerConfig,
-{
-    #[inline]
-    fn write_struct_len<S>(ser: &mut S, len: usize) -> Result<(), Error>
-    where
-        S: UnderlyingWrite,
-        for<'a> &'a mut S: Serializer<Ok = (), Error = Error>,
-    {
-        C::write_struct_len(ser, len)
-    }
-
-    #[inline]
-    fn write_struct_field<S, T>(ser: &mut S, key: &'static str, value: &T) -> Result<(), Error>
-    where
-        S: UnderlyingWrite,
-        for<'a> &'a mut S: Serializer<Ok = (), Error = Error>,
-        T: ?Sized + Serialize,
-    {
-        C::write_struct_field(ser, key, value)
-    }
-
-    #[inline]
-    fn write_variant_ident<S>(
-        ser: &mut S,
-        variant_index: u32,
-        _variant: &'static str,
-    ) -> Result<(), Error>
-    where
-        S: UnderlyingWrite,
-        for<'a> &'a mut S: Serializer<Ok = (), Error = Error>,
-    {
-        ser.serialize_u32(variant_index)
-    }
-
-    #[inline(always)]
-    fn is_human_readable() -> bool {
-        C::is_human_readable()
-    }
-}
-
 /// Config wrapper that overrides `Serializer::is_human_readable` and
 /// `Deserializer::is_human_readable` to return `true`.
 #[derive(Copy, Clone, Debug)]
diff --git a/rmp-serde/src/decode.rs b/rmp-serde/src/decode.rs
index 3b021bb..2c10b4b 100644
--- a/rmp-serde/src/decode.rs
+++ b/rmp-serde/src/decode.rs
@@ -8,7 +8,7 @@ use std::str::{self, Utf8Error};
 use byteorder::{self, ReadBytesExt};
 
 use serde;
-use serde::de::{self, Deserialize, DeserializeOwned, DeserializeSeed, Visitor};
+use serde::de::{self, Deserialize, DeserializeOwned, DeserializeSeed, Unexpected, Visitor};
 
 use rmp;
 use rmp::decode::{self, DecodeStringError, MarkerReadError, NumValueReadError, ValueReadError};
@@ -178,6 +178,16 @@ impl<R: Read, C> Deserializer<R, C> {
             .take()
             .map_or_else(|| rmp::decode::read_marker(&mut self.rd), Ok)
     }
+
+    #[inline]
+    fn peek_or_read_marker(&mut self) -> Result<Marker, MarkerReadError> {
+        if let Some(m) = self.marker {
+            Ok(m)
+        } else {
+            let m = rmp::decode::read_marker(&mut self.rd)?;
+            Ok(self.marker.insert(m).to_owned())
+        }
+    }
 }
 
 impl<R: Read> Deserializer<ReadReader<R>, DefaultConfig> {
@@ -585,10 +595,20 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
     fn deserialize_enum<V>(self, _name: &str, _variants: &[&str], visitor: V) -> Result<V::Value, Error>
         where V: Visitor<'de>
     {
-        let marker = self.take_or_read_marker()?;
-        match rmp::decode::marker_to_len(&mut self.rd, marker)? {
-            1 => visitor.visit_enum(VariantAccess::new(self)),
-            n => Err(Error::LengthMismatch(n as u32)),
+        let marker = self.peek_or_read_marker()?;
+        match rmp::decode::marker_to_len(&mut self.rd, marker) {
+            Ok(len) => match len {
+                // Enums are either encoded as maps with a single K/V pair
+                // where the K = the variant & V = associated data
+                // or as just the variant
+                1 => {
+                    self.marker = None;
+                    visitor.visit_enum(VariantAccess::new(self))
+                }
+                n => Err(Error::LengthMismatch(n as u32)),
+            },
+            // TODO: Check this is a string
+            Err(_) => visitor.visit_enum(UnitVariantAccess::new(self)),
         }
     }
 
@@ -771,6 +791,76 @@ impl<'de, 'a, R: ReadSlice<'de> + 'a, C: SerializerConfig> de::MapAccess<'de> fo
     }
 }
 
+struct UnitVariantAccess<'a, R: 'a, C> {
+    de: &'a mut Deserializer<R, C>,
+}
+
+impl<'a, R: 'a, C> UnitVariantAccess<'a, R, C> {
+    pub fn new(de: &'a mut Deserializer<R, C>) -> Self {
+        UnitVariantAccess { de }
+    }
+}
+
+impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> de::EnumAccess<'de>
+    for UnitVariantAccess<'a, R, C>
+{
+    type Error = Error;
+    type Variant = Self;
+
+    #[inline]
+    fn variant_seed<V>(self, seed: V) -> Result<(V::Value, Self), Error>
+    where
+        V: de::DeserializeSeed<'de>,
+    {
+        let variant = seed.deserialize(&mut *self.de)?;
+        Ok((variant, self))
+    }
+}
+
+impl<'de, 'a, R: ReadSlice<'de> + 'a, C: SerializerConfig> de::VariantAccess<'de>
+    for UnitVariantAccess<'a, R, C>
+{
+    type Error = Error;
+
+    fn unit_variant(self) -> Result<(), Error> {
+        Ok(())
+    }
+
+    fn newtype_variant_seed<T>(self, _seed: T) -> Result<T::Value, Error>
+    where
+        T: de::DeserializeSeed<'de>,
+    {
+        Err(de::Error::invalid_type(
+            Unexpected::UnitVariant,
+            &"newtype variant",
+        ))
+    }
+
+    fn tuple_variant<V>(self, _len: usize, _visitor: V) -> Result<V::Value, Error>
+    where
+        V: de::Visitor<'de>,
+    {
+        Err(de::Error::invalid_type(
+            Unexpected::UnitVariant,
+            &"tuple variant",
+        ))
+    }
+
+    fn struct_variant<V>(
+        self,
+        _fields: &'static [&'static str],
+        _visitor: V,
+    ) -> Result<V::Value, Error>
+    where
+        V: de::Visitor<'de>,
+    {
+        Err(de::Error::invalid_type(
+            Unexpected::UnitVariant,
+            &"struct variant",
+        ))
+    }
+}
+
 struct VariantAccess<'a, R, C> {
     de: &'a mut Deserializer<R, C>,
 }
diff --git a/rmp-serde/src/encode.rs b/rmp-serde/src/encode.rs
index 765b2b6..51849e5 100644
--- a/rmp-serde/src/encode.rs
+++ b/rmp-serde/src/encode.rs
@@ -16,7 +16,7 @@ use rmp::{encode, Marker};
 
 use crate::config::{
     BinaryConfig, DefaultConfig, HumanReadableConfig, SerializerConfig, StructMapConfig,
-    StructTupleConfig, VariantIntegerConfig, VariantStringConfig,
+    StructTupleConfig
 };
 use crate::MSGPACK_EXT_STRUCT_NAME;
 
@@ -239,35 +239,6 @@ impl<W: Write, C> Serializer<W, C> {
         }
     }
 
-    /// Consumes this serializer returning the new one, which will serialize enum variants as strings.
-    ///
-    /// This is used, when the default struct serialization as integers does not fit your
-    /// requirements.
-    #[inline]
-    pub fn with_string_variants(self) -> Serializer<W, VariantStringConfig<C>> {
-        let Serializer { wr, depth, config } = self;
-        Serializer {
-            wr,
-            depth,
-            config: VariantStringConfig::new(config),
-        }
-    }
-
-    /// Consumes this serializer returning the new one, which will serialize enum variants as a their
-    /// integer indices.
-    ///
-    /// This is the default MessagePack serialization mechanism, emitting the most compact
-    /// representation.
-    #[inline]
-    pub fn with_integer_variants(self) -> Serializer<W, VariantIntegerConfig<C>> {
-        let Serializer { wr, depth, config } = self;
-        Serializer {
-            wr,
-            depth,
-            config: VariantIntegerConfig::new(config),
-        }
-    }
-
     /// Consumes this serializer returning the new one, which will serialize some types in
     /// human-readable representations (`Serializer::is_human_readable` will return `true`). Note
     /// that the overall representation is still binary, but some types such as IP addresses will
@@ -636,10 +607,7 @@ where
     fn serialize_unit_variant(self, _name: &str, idx: u32, variant: &'static str) ->
         Result<Self::Ok, Self::Error>
     {
-        // encode as a map from variant idx to nil, like: {idx => nil}
-        encode::write_map_len(&mut self.wr, 1)?;
-        C::write_variant_ident(self, idx, variant)?;
-        self.serialize_unit()
+        C::write_variant_ident(self, idx, variant)
     }
 
     fn serialize_newtype_struct<T: ?Sized + serde::Serialize>(self, name: &'static str, value: &T) -> Result<(), Self::Error> {


Test patch:
diff --git a/rmp-serde/tests/encode_derive.rs b/rmp-serde/tests/encode_derive.rs
index d6f4fcd..910e346 100644
--- a/rmp-serde/tests/encode_derive.rs
+++ b/rmp-serde/tests/encode_derive.rs
@@ -30,8 +30,8 @@ fn pass_unit_variant() {
     Enum::V1.serialize(&mut Serializer::new(&mut buf)).unwrap();
     Enum::V2.serialize(&mut Serializer::new(&mut buf)).unwrap();
 
-    // Expect: {0 => nil} {1 => nil}.
-    assert_eq!(vec![0x81, 0x00, 0xC0, 0x81, 0x01, 0xC0], buf);
+    // Expect: "V1", "V2"
+    assert_eq!(vec![0xa2, 0x56, 0x31, 0xa2, 0x56, 0x32], buf);
 }
 
 #[test]
@@ -56,8 +56,8 @@ fn pass_newtype_variant() {
     let mut buf = Vec::new();
     Enum::V2(42).serialize(&mut Serializer::new(&mut buf)).unwrap();
 
-    // Expect: {0 => 42}.
-    assert_eq!(vec![0x81, 0x00, 0x2a], buf);
+    // Expect: {"V2" => 42}
+    assert_eq!(buf, vec![0x81, 0xa2, 0x56, 0x32, 42]);
 }
 
 #[test]
@@ -78,7 +78,8 @@ fn pass_untagged_newtype_variant() {
     let buf2 = rmps::to_vec(&Enum1::B(Enum2::C)).unwrap();
 
     assert_eq!(buf1, [123]);
-    assert_eq!(buf2, [0x81, 0x0, 0xC0]);
+    // Expect: "C"
+    assert_eq!(buf2, [0xa1, 0x43]);
 }
 
 #[test]
@@ -107,7 +108,11 @@ fn pass_tuple_variant() {
     Enum::V2(42, 100500).serialize(&mut Serializer::new(&mut buf)).unwrap();
 
     // Expect: {0 => nil} {1 => [42, 100500]}
-    assert_eq!(vec![0x81, 0x00, 0xC0, 0x81, 0x01, 0x92, 0x2a, 0xce, 0x00, 0x01, 0x88, 0x94], buf);
+    // Expect: "V1", {"V2" => [42, 100500] }
+    assert_eq!(
+        vec![0xa2, 0x56, 0x31, 0x81, 0xa2, 0x56, 0x32, 0x92, 0x2a, 0xce, 0x0, 0x1, 0x88, 0x94],
+        buf
+    );
 }
 
 #[test]
@@ -141,8 +146,11 @@ fn serialize_struct_variant() {
     Enum::V1 { f1: 42 }.serialize(&mut Serializer::new(&mut buf)).unwrap();
     Enum::V2 { f1: 43 }.serialize(&mut Serializer::new(&mut buf)).unwrap();
 
-    // Expect: {0 => [42]} {1 => [43]}.
-    assert_eq!(vec![0x81, 0x00, 0x91, 0x2a, 0x81, 0x01, 0x91, 0x2b], buf);
+    // Expect: { "V1" => [42] } { "V2" => [43] }
+    assert_eq!(
+        vec![0x81, 0xa2, 0x56, 0x31, 0x91, 0x2a, 0x81, 0xa2, 0x56, 0x32, 0x91, 0x2b],
+        buf
+    );
 }
 
 #[test]
@@ -157,8 +165,11 @@ fn serialize_struct_variant_as_map() {
     let mut se = Serializer::new(Vec::new()).with_struct_map();
     Enum::V1 { f1: 42 }.serialize(&mut se).unwrap();
 
-    // Expect: {0 => {"f1": 42}}.
-    assert_eq!(vec![0x81, 0x00, 0x81, 0xa2, 0x66, 0x31, 0x2a], se.into_inner());
+     // Expect: {"V1" => {"f1": 42}}.
+    assert_eq!(
+        vec![0x81, 0xa2, 0x56, 0x31, 0x81, 0xa2, 0x66, 0x31, 0x2a],
+        se.into_inner()
+    );
 }
 
 #[test]
diff --git a/rmp-serde/tests/round.rs b/rmp-serde/tests/round.rs
index ea615ea..107833f 100644
--- a/rmp-serde/tests/round.rs
+++ b/rmp-serde/tests/round.rs
@@ -460,20 +460,18 @@ fn round_variant_string() {
         }
     }
 
-    do_test!(|b| Serializer::new(b).with_string_variants());
-    do_test!(|b| Serializer::new(b).with_struct_map().with_string_variants());
-    do_test!(|b| Serializer::new(b).with_struct_tuple().with_string_variants());
-    do_test!(|b| Serializer::new(b).with_string_variants().with_struct_map());
-    do_test!(|b| Serializer::new(b).with_string_variants().with_struct_tuple());
+    do_test!(|b| Serializer::new(b));
+    do_test!(|b| Serializer::new(b).with_struct_map());
+    do_test!(|b| Serializer::new(b).with_struct_tuple());
+    do_test!(|b| Serializer::new(b).with_struct_map());
+    do_test!(|b| Serializer::new(b).with_struct_tuple());
     do_test!(|b| {
         Serializer::new(b)
-            .with_string_variants()
             .with_struct_tuple()
             .with_struct_map()
             .with_struct_tuple()
             .with_struct_map()
     });
-    do_test!(|b| Serializer::new(b).with_integer_variants().with_string_variants());
 }
 
 use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
@@ -546,14 +544,8 @@ fn assert_roundtrips<T: PartialEq + std::fmt::Debug + Serialize + for<'a> Deseri
     assert_roundtrips_config(&val, ".with_struct_map()", |s| s.with_struct_map(), |d| d);
     assert_roundtrips_config(
         &val,
-        ".with_string_variants()",
-        |s| s.with_string_variants(),
-        |d| d,
-    );
-    assert_roundtrips_config(
-        &val,
-        ".with_struct_map().with_string_variants()",
-        |s| s.with_struct_map().with_string_variants(),
+        ".with_struct_map()",
+        |s| s.with_struct_map(),
         |d| d,
     );
     assert_roundtrips_config(
@@ -570,17 +562,16 @@ fn assert_roundtrips<T: PartialEq + std::fmt::Debug + Serialize + for<'a> Deseri
     );
     assert_roundtrips_config(
         &val,
-        ".with_human_readable().with_string_variants()",
-        |s| s.with_human_readable().with_string_variants(),
+        ".with_human_readable()",
+        |s| s.with_human_readable(),
         |d| d.with_human_readable(),
     );
     assert_roundtrips_config(
         &val,
-        ".with_human_readable().with_struct_map().with_string_variants()",
+        ".with_human_readable().with_struct_map()",
         |s| {
             s.with_human_readable()
                 .with_struct_map()
-                .with_string_variants()
         },
         |d| d.with_human_readable(),
     );