Merge pull request #500 from mguentner/dict_ordered

zv: store entries of Dict as BTreeSet to keep it ordered

zvariant/src/array.rs

@@ -17,7 +17,7 @@ use crate::{
 ///
 /// [`Value`]: enum.Value.html#variant.Array
 /// [`Vec`]: https://doc.rust-lang.org/std/vec/struct.Vec.html
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, Hash, PartialEq, PartialOrd, Eq, Ord)]
 pub struct Array<'a> {
     element_signature: Signature<'a>,
     elements: Vec<Value<'a>>,

zvariant/src/dict.rs

@@ -1,5 +1,5 @@
 use std::{
-    collections::HashMap,
+    collections::{BTreeSet, HashMap},
     fmt::{Display, Write},
     hash::BuildHasher,
 };
@@ -15,9 +15,9 @@ use crate::{value_display_fmt, Basic, DynamicType, Error, Signature, Type, Value
 ///
 /// [`Value`]: enum.Value.html#variant.Dict
 /// [`HashMap`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, Hash, PartialEq, PartialOrd, Eq, Ord)]
 pub struct Dict<'k, 'v> {
-    entries: Vec<DictEntry<'k, 'v>>,
+    entries: BTreeSet<DictEntry<'k, 'v>>,
     key_signature: Signature<'k>,
     value_signature: Signature<'v>,
     // should use a separate lifetime or everything should use the same but API break.
@@ -32,7 +32,7 @@ impl<'k, 'v> Dict<'k, 'v> {
         let signature = create_signature(&key_signature, &value_signature);
 
         Self {
-            entries: vec![],
+            entries: BTreeSet::new(),
             key_signature,
             value_signature,
             signature,
@@ -56,7 +56,7 @@ impl<'k, 'v> Dict<'k, 'v> {
         check_child_value_signature!(self.key_signature, key.value_signature(), "key");
         check_child_value_signature!(self.value_signature, value.value_signature(), "value");
 
-        self.entries.push(DictEntry { key, value });
+        self.entries.insert(DictEntry { key, value });
 
         Ok(())
     }
@@ -70,7 +70,7 @@ impl<'k, 'v> Dict<'k, 'v> {
         check_child_value_signature!(self.key_signature, K::signature(), "key");
         check_child_value_signature!(self.value_signature, value.dynamic_signature(), "value");
 
-        self.entries.push(DictEntry {
+        self.entries.insert(DictEntry {
             key: Value::new(key),
             value: Value::new(value),
         });
@@ -131,7 +131,7 @@ impl<'k, 'v> Dict<'k, 'v> {
         let value_signature = signature.slice(3..signature.len() - 1);
 
         Self {
-            entries: vec![],
+            entries: BTreeSet::new(),
             key_signature,
             value_signature,
             signature,
@@ -261,7 +261,7 @@ where
 
 // TODO: Conversion of Dict from/to BTreeMap
 
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, Hash, PartialOrd, Ord, PartialEq, Eq)]
 struct DictEntry<'k, 'v> {
     key: Value<'k>,
     value: Value<'v>,

zvariant/src/fd.rs

@@ -16,7 +16,7 @@ use crate::{Basic, EncodingFormat, Signature, Type};
 /// [`Deserialize`]: https://docs.serde.rs/serde/de/trait.Deserialize.html
 /// [deserializer]: fn.from_slice_fds.html
 /// [serializer]: fn.to_bytes_fds.html
-#[derive(Debug, PartialEq, Eq, Hash, Clone, Copy)]
+#[derive(Debug, PartialEq, PartialOrd, Ord, Eq, Hash, Clone, Copy)]
 pub struct Fd(io::RawFd);
 
 macro_rules! fd_impl {

zvariant/src/lib.rs

@@ -1177,7 +1177,7 @@ mod tests {
         let v: Value<'_> = dict.into();
         assert_eq!(v.value_signature(), "a{sv}");
         let encoded = to_bytes(ctxt, &v).unwrap();
-        assert_eq!(dbg!(encoded.len()), 68);
+        assert_eq!(dbg!(encoded.len()), 66);
         let v: Value<'_> = encoded.deserialize().unwrap().0;
         if let Value::Dict(dict) = v {
             assert_eq!(
@@ -1256,6 +1256,21 @@ mod tests {
         );
     }
 
+    #[test]
+    fn dict_compare() {
+        // the order in which a dict has been constructed must not play a role
+        // https://github.com/dbus2/zbus/issues/484
+        let mut dict1 = Dict::new(<&str>::signature(), Value::signature());
+        dict1.add("first", Value::new("value")).unwrap();
+        dict1.add("second", Value::new("value")).unwrap();
+
+        let mut dict2 = Dict::new(<&str>::signature(), Value::signature());
+        dict2.add("second", Value::new("value")).unwrap();
+        dict2.add("first", Value::new("value")).unwrap();
+
+        assert_eq!(dict1, dict2);
+    }
+
     #[test]
     fn value_value() {
         let ctxt = Context::<BE>::new_dbus(0);

zvariant/src/maybe.rs

@@ -9,7 +9,7 @@ use crate::{value_display_fmt, Error, Signature, Type, Value};
 /// API is provided to convert from, and to `Option<T>`.
 ///
 /// [`Value`]: enum.Value.html
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
 pub struct Maybe<'a> {
     value: Box<Option<Value<'a>>>,
     value_signature: Signature<'a>,

zvariant/src/object_path.rs

@@ -32,7 +32,7 @@ use crate::{Basic, EncodingFormat, Error, Result, Signature, Str, Type};
 /// ObjectPath::try_from("/end/with/slash/").unwrap_err();
 /// ObjectPath::try_from("/ha.d").unwrap_err();
 /// ```
-#[derive(PartialEq, Eq, Hash, Clone)]
+#[derive(PartialEq, Eq, Hash, Clone, PartialOrd, Ord)]
 pub struct ObjectPath<'a>(Str<'a>);
 
 assert_impl_all!(ObjectPath<'_>: Send, Sync, Unpin);

zvariant/src/signature.rs

@@ -1,5 +1,7 @@
 use core::{
+    cmp::Ordering,
     fmt::{self, Debug, Display, Formatter},
+    hash::{Hash, Hasher},
     str,
 };
 use serde::{
@@ -20,7 +22,7 @@ use crate::{signature_parser::SignatureParser, Basic, EncodingFormat, Error, Res
 // breakage.
 //
 // [`bytes::Bytes`]: https://docs.rs/bytes/0.5.6/bytes/struct.Bytes.html
-#[derive(PartialEq, Eq, Hash, Clone)]
+#[derive(Debug, Clone)]
 enum Bytes<'b> {
     Borrowed(&'b [u8]),
     Static(&'static [u8]),
@@ -46,7 +48,7 @@ impl<'b> Bytes<'b> {
     }
 }
 
-impl<'b> std::ops::Deref for Bytes<'b> {
+impl std::ops::Deref for Bytes<'_> {
     type Target = [u8];
 
     fn deref(&self) -> &[u8] {
@@ -58,6 +60,32 @@ impl<'b> std::ops::Deref for Bytes<'b> {
     }
 }
 
+impl Eq for Bytes<'_> {}
+
+impl PartialEq for Bytes<'_> {
+    fn eq(&self, other: &Self) -> bool {
+        **self == **other
+    }
+}
+
+impl Ord for Bytes<'_> {
+    fn cmp(&self, other: &Self) -> Ordering {
+        (**self).cmp(&**other)
+    }
+}
+
+impl PartialOrd for Bytes<'_> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl Hash for Bytes<'_> {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        (**self).hash(state)
+    }
+}
+
 /// String that [identifies] the type of an encoded value.
 ///
 /// # Examples
@@ -96,7 +124,7 @@ impl<'b> std::ops::Deref for Bytes<'b> {
 ///
 /// [identifies]: https://dbus.freedesktop.org/doc/dbus-specification.html#type-system
 /// [`slice`]: #method.slice
-#[derive(Hash, Clone)]
+#[derive(Hash, Clone, PartialOrd, Ord)]
 pub struct Signature<'a> {
     bytes: Bytes<'a>,
     pos: usize,
@@ -551,7 +579,31 @@ impl std::fmt::Display for OwnedSignature {
 
 #[cfg(test)]
 mod tests {
-    use super::Signature;
+    use super::{Bytes, Signature};
+    use std::sync::Arc;
+
+    #[test]
+    fn bytes_equality() {
+        let borrowed1 = Bytes::Borrowed(b"foo");
+        let borrowed2 = Bytes::Borrowed(b"foo");
+        let static1 = Bytes::Static(b"foo");
+        let static2 = Bytes::Static(b"foo");
+        let owned1 = Bytes::Owned(Arc::new(*b"foo"));
+        let owned2 = Bytes::Owned(Arc::new(*b"foo"));
+
+        assert_eq!(borrowed1, borrowed2);
+        assert_eq!(static1, static2);
+        assert_eq!(owned1, owned2);
+
+        assert_eq!(borrowed1, static1);
+        assert_eq!(static1, borrowed1);
+
+        assert_eq!(static1, owned1);
+        assert_eq!(owned1, static1);
+
+        assert_eq!(borrowed1, owned1);
+        assert_eq!(owned1, borrowed1);
+    }
 
     #[test]
     fn signature_slicing() {

zvariant/src/structure.rs

@@ -155,7 +155,7 @@ impl<'de> Visitor<'de> for StructureVisitor<'de> {
 /// API is provided to convert from, and to tuples.
 ///
 /// [`Value`]: enum.Value.html
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
 pub struct Structure<'a> {
     fields: Vec<Value<'a>>,
     signature: Signature<'a>,

zvariant/src/value.rs

@@ -1,7 +1,10 @@
-use core::str;
-use std::{
+use core::{
+    cmp::Ordering,
     fmt::{Display, Write},
+    hash::{Hash, Hasher},
     marker::PhantomData,
+    mem::discriminant,
+    str,
 };
 
 use serde::{
@@ -72,7 +75,7 @@ use crate::Fd;
 /// ```
 ///
 /// [D-Bus specification]: https://dbus.freedesktop.org/doc/dbus-specification.html#container-types
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, PartialEq, PartialOrd)]
 pub enum Value<'a> {
     // Simple types
     U8(u8),
@@ -100,6 +103,50 @@ pub enum Value<'a> {
     Fd(Fd),
 }
 
+impl Hash for Value<'_> {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        discriminant(self).hash(state);
+        match self {
+            Self::U8(inner) => inner.hash(state),
+            Self::Bool(inner) => inner.hash(state),
+            Self::I16(inner) => inner.hash(state),
+            Self::U16(inner) => inner.hash(state),
+            Self::I32(inner) => inner.hash(state),
+            Self::U32(inner) => inner.hash(state),
+            Self::I64(inner) => inner.hash(state),
+            Self::U64(inner) => inner.hash(state),
+            Self::F64(inner) => inner.to_le_bytes().hash(state),
+            Self::Str(inner) => inner.hash(state),
+            Self::Signature(inner) => inner.hash(state),
+            Self::ObjectPath(inner) => inner.hash(state),
+            Self::Value(inner) => inner.hash(state),
+            Self::Array(inner) => inner.hash(state),
+            Self::Dict(inner) => inner.hash(state),
+            Self::Structure(inner) => inner.hash(state),
+            #[cfg(feature = "gvariant")]
+            Self::Maybe(inner) => inner.hash(state),
+            #[cfg(unix)]
+            Self::Fd(inner) => inner.hash(state),
+        }
+    }
+}
+
+impl Eq for Value<'_> {}
+
+impl Ord for Value<'_> {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.partial_cmp(other)
+            .unwrap_or_else(|| match (self, other) {
+                (Self::F64(lhs), Self::F64(rhs)) => lhs.total_cmp(rhs),
+                // `partial_cmp` returns `Some(_)` if either the discriminants are different
+                // or if both the left hand side and right hand side is `Self::F64(_)`,
+                // because `f64` is the only type in this enum, that does not implement `Ord`.
+                // This `match`-arm is therefore unreachable.
+                _ => unreachable!(),
+            })
+    }
+}
+
 assert_impl_all!(Value<'_>: Send, Sync, Unpin);
 
 macro_rules! serialize_value {