Fix Value & related API and make them more consistent

zbus/src/lib.rs

@@ -1057,4 +1057,19 @@ mod tests {
             event.notify(1);
         }
     }
+
+    #[test]
+    #[ignore]
+    fn issue_466() {
+        #[crate::dbus_proxy(interface = "org.Some.Thing1", assume_defaults = true)]
+        trait MyGreeter {
+            fn foo(
+                &self,
+                arg: &(u32, zbus::zvariant::Value<'_>),
+            ) -> zbus::Result<(u32, zbus::zvariant::OwnedValue)>;
+
+            #[dbus_proxy(property)]
+            fn bar(&self) -> zbus::Result<(u32, zbus::zvariant::OwnedValue)>;
+        }
+    }
 }

zvariant/src/array.rs

@@ -60,10 +60,23 @@ impl<'a> Array<'a> {
     }
 
     /// Get all the elements.
-    pub fn get(&self) -> &[Value<'a>] {
+    pub fn inner(&self) -> &[Value<'a>] {
         &self.elements
     }
 
+    /// Get the value at the given index.
+    pub fn get<V>(&'a self, idx: usize) -> Result<Option<V>>
+    where
+        V: ?Sized + TryFrom<&'a Value<'a>>,
+        <V as TryFrom<&'a Value<'a>>>::Error: Into<crate::Error>,
+    {
+        self.elements
+            .get(idx)
+            .map(|v| v.downcast_ref::<V>())
+            .transpose()
+            .map_err(Into::into)
+    }
+
     /// Get the number of elements.
     pub fn len(&self) -> usize {
         self.elements.len()
@@ -138,7 +151,7 @@ pub(crate) fn array_display_fmt(
             let bytes = leading
                 .iter()
                 .map(|v| {
-                    *v.downcast_ref::<u8>()
+                    v.downcast_ref::<u8>()
                         .expect("item must have a signature of a byte")
                 })
                 .collect::<Vec<_>>();
@@ -231,7 +244,7 @@ impl<'a> std::ops::Deref for Array<'a> {
     type Target = [Value<'a>];
 
     fn deref(&self) -> &Self::Target {
-        self.get()
+        self.inner()
     }
 }
 

zvariant/src/dict.rs

@@ -1,7 +1,7 @@
 use std::{
-    collections::{BTreeSet, HashMap},
+    collections::{BTreeMap, HashMap},
     fmt::{Display, Write},
-    hash::BuildHasher,
+    hash::{BuildHasher, Hash},
 };
 
 use serde::ser::{Serialize, SerializeSeq, SerializeStruct, Serializer};
@@ -17,7 +17,7 @@ use crate::{value_display_fmt, Basic, DynamicType, Error, Signature, Type, Value
 /// [`HashMap`]: https://doc.rust-lang.org/std/collections/struct.HashMap.html
 #[derive(Debug, Hash, PartialEq, PartialOrd, Eq, Ord)]
 pub struct Dict<'k, 'v> {
-    entries: BTreeSet<DictEntry<'k, 'v>>,
+    map: BTreeMap<Value<'k>, Value<'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: BTreeSet::new(),
+            map: BTreeMap::new(),
             key_signature,
             value_signature,
             signature,
@@ -56,49 +56,37 @@ 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.insert(DictEntry { key, value });
+        self.map.insert(key, value);
 
         Ok(())
     }
 
     /// Add a new entry.
     pub fn add<K, V>(&mut self, key: K, value: V) -> Result<(), Error>
     where
-        K: Basic + Into<Value<'k>> + std::hash::Hash + std::cmp::Eq,
+        K: Basic + Into<Value<'k>> + Ord,
         V: Into<Value<'v>> + DynamicType,
     {
         check_child_value_signature!(self.key_signature, K::signature(), "key");
         check_child_value_signature!(self.value_signature, value.dynamic_signature(), "value");
 
-        self.entries.insert(DictEntry {
-            key: Value::new(key),
-            value: Value::new(value),
-        });
+        self.map.insert(Value::new(key), Value::new(value));
 
         Ok(())
     }
 
     /// Get the value for the given key.
-    pub fn get<'d, K, V>(&'d self, key: &K) -> Result<Option<&'v V>, Error>
+    pub fn get<'d, K, V>(&'d self, key: &'k K) -> Result<Option<V>, Error>
     where
         'd: 'k + 'v,
-        K: ?Sized + std::cmp::Eq + 'k,
-        V: ?Sized,
-        &'k K: TryFrom<&'k Value<'k>>,
-        &'v V: TryFrom<&'v Value<'v>>,
+        &'k K: TryInto<Value<'k>>,
+        <&'k K as TryInto<Value<'k>>>::Error: Into<crate::Error>,
+        V: TryFrom<&'v Value<'v>>,
+        <V as TryFrom<&'v Value<'v>>>::Error: Into<crate::Error>,
     {
-        for entry in &self.entries {
-            let entry_key = entry.key.downcast_ref::<K>().ok_or(Error::IncorrectType)?;
-            if *entry_key == *key {
-                return entry
-                    .value
-                    .downcast_ref()
-                    .ok_or(Error::IncorrectType)
-                    .map(Some);
-            }
-        }
+        let key: Value<'_> = key.try_into().map_err(Into::into)?;
 
-        Ok(None)
+        self.map.get(&key).map(|v| v.downcast_ref()).transpose()
     }
 
     /// Get the signature of this `Dict`.
@@ -121,24 +109,29 @@ impl<'k, 'v> Dict<'k, 'v> {
             key_signature: self.key_signature.to_owned(),
             value_signature: self.value_signature.to_owned(),
             signature: self.signature.to_owned(),
-            entries: self
-                .entries
+            map: self
+                .map
                 .iter()
-                .map(|v| v.try_to_owned().map(Into::into))
+                .map(|(k, v)| {
+                    Ok((
+                        k.try_to_owned().map(Into::into)?,
+                        v.try_to_owned().map(Into::into)?,
+                    ))
+                })
                 .collect::<crate::Result<_>>()?,
         })
     }
 
     /// Try to clone the `Dict`.
     pub fn try_clone(&self) -> Result<Self, Error> {
         let entries = self
-            .entries
+            .map
             .iter()
-            .map(|v| v.try_clone())
-            .collect::<Result<_, _>>()?;
+            .map(|(k, v)| Ok((k.try_clone()?, v.try_clone()?)))
+            .collect::<Result<_, crate::Error>>()?;
 
         Ok(Self {
-            entries,
+            map: entries,
             key_signature: self.key_signature.clone(),
             value_signature: self.value_signature.clone(),
             signature: self.signature.clone(),
@@ -151,7 +144,7 @@ impl<'k, 'v> Dict<'k, 'v> {
         let value_signature = signature.slice(3..signature.len() - 1);
 
         Self {
-            entries: BTreeSet::new(),
+            map: BTreeMap::new(),
             key_signature,
             value_signature,
             signature,
@@ -172,7 +165,7 @@ pub(crate) fn dict_display_fmt(
     f: &mut std::fmt::Formatter<'_>,
     type_annotate: bool,
 ) -> std::fmt::Result {
-    if dict.entries.is_empty() {
+    if dict.map.is_empty() {
         if type_annotate {
             write!(f, "@{} ", dict.full_signature())?;
         }
@@ -183,13 +176,13 @@ pub(crate) fn dict_display_fmt(
         // Annotate only the first entry as the rest will be of the same type.
         let mut type_annotate = type_annotate;
 
-        for (i, entry) in dict.entries.iter().enumerate() {
-            value_display_fmt(&entry.key, f, type_annotate)?;
+        for (i, (key, value)) in dict.map.iter().enumerate() {
+            value_display_fmt(key, f, type_annotate)?;
             f.write_str(": ")?;
-            value_display_fmt(&entry.value, f, type_annotate)?;
+            value_display_fmt(value, f, type_annotate)?;
             type_annotate = false;
 
-            if i + 1 < dict.entries.len() {
+            if i + 1 < dict.map.len() {
                 f.write_str(", ")?;
             }
         }
@@ -205,105 +198,94 @@ impl<'k, 'v> Serialize for Dict<'k, 'v> {
     where
         S: Serializer,
     {
-        let mut seq = serializer.serialize_seq(Some(self.entries.len()))?;
-        for entry in &self.entries {
-            seq.serialize_element(entry)?;
+        let mut seq = serializer.serialize_seq(Some(self.map.len()))?;
+        for (key, value) in self.map.iter() {
+            seq.serialize_element(&DictEntry { key, value })?;
         }
 
         seq.end()
     }
 }
 
-// Conversion of Dict to HashMap
-impl<'k, 'v, K, V, H> TryFrom<Dict<'k, 'v>> for HashMap<K, V, H>
-where
-    K: Basic + TryFrom<Value<'k>> + std::hash::Hash + std::cmp::Eq,
-    V: TryFrom<Value<'v>>,
-    H: BuildHasher + Default,
-    K::Error: Into<crate::Error>,
-    V::Error: Into<crate::Error>,
-{
-    type Error = Error;
-
-    fn try_from(v: Dict<'k, 'v>) -> Result<Self, Self::Error> {
-        let mut map = HashMap::default();
-        for e in v.entries.into_iter() {
-            let key = if let Value::Value(v) = e.key {
-                K::try_from(*v)
-            } else {
-                K::try_from(e.key)
-            }
-            .map_err(Into::into)?;
-
-            let value = if let Value::Value(v) = e.value {
-                V::try_from(*v)
-            } else {
-                V::try_from(e.value)
+// Conversion of Dict to Map types
+macro_rules! from_dict {
+    ($ty:ident <K $(: $kbound1:ident $(+ $kbound2:ident)*)*, V $(, $typaram:ident)*>) => {
+        impl<'k, 'v, K, V $(, $typaram)*> TryFrom<Dict<'k, 'v>> for $ty<K, V $(, $typaram)*>
+        where
+            K: Basic + TryFrom<Value<'k>> $(+ $kbound1 $(+ $kbound2)*)*,
+            V: TryFrom<Value<'v>>,
+            K::Error: Into<crate::Error>,
+            V::Error: Into<crate::Error>,
+            $($typaram: BuildHasher + Default,)*
+        {
+            type Error = Error;
+
+            fn try_from(v: Dict<'k, 'v>) -> Result<Self, Self::Error> {
+                v.map.into_iter().map(|(key, value)| {
+                    let key = if let Value::Value(v) = key {
+                        K::try_from(*v)
+                    } else {
+                        K::try_from(key)
+                    }
+                    .map_err(Into::into)?;
+
+                    let value = if let Value::Value(v) = value {
+                        V::try_from(*v)
+                    } else {
+                        V::try_from(value)
+                    }
+                    .map_err(Into::into)?;
+
+                    Ok((key, value))
+                }).collect::<Result<_, _>>()
             }
-            .map_err(Into::into)?;
-
-            map.insert(key, value);
         }
-        Ok(map)
-    }
+    };
 }
+from_dict!(HashMap<K: Eq + Hash, V, H>);
+from_dict!(BTreeMap<K: Ord, V>);
 
 // TODO: this could be useful
 // impl<'d, 'k, 'v, K, V, H> TryFrom<&'d Dict<'k, 'v>> for HashMap<&'k K, &'v V, H>
 
 // Conversion of Hashmap to Dict
-impl<'k, 'v, K, V, H> From<HashMap<K, V, H>> for Dict<'k, 'v>
-where
-    K: Type + Into<Value<'k>> + std::hash::Hash + std::cmp::Eq,
-    V: Type + Into<Value<'v>>,
-    H: BuildHasher + Default,
-{
-    fn from(value: HashMap<K, V, H>) -> Self {
-        let entries = value
-            .into_iter()
-            .map(|(key, value)| DictEntry {
-                key: Value::new(key),
-                value: Value::new(value),
-            })
-            .collect();
-        let key_signature = K::signature();
-        let value_signature = V::signature();
-        let signature = create_signature(&key_signature, &value_signature);
-
-        Self {
-            entries,
-            key_signature,
-            value_signature,
-            signature,
+macro_rules! to_dict {
+    ($ty:ident <K $(: $kbound1:ident $(+ $kbound2:ident)*)*, V $(, $typaram:ident)*>) => {
+        impl<'k, 'v, K, V $(, $typaram)*> From<$ty<K, V $(, $typaram)*>> for Dict<'k, 'v>
+        where
+            K: Type + Into<Value<'k>>,
+            V: Type + Into<Value<'v>>,
+            $($typaram: BuildHasher,)*
+        {
+            fn from(value: $ty<K, V $(, $typaram)*>) -> Self {
+                let entries = value
+                    .into_iter()
+                    .map(|(key, value)| (Value::new(key), Value::new(value)))
+                    .collect();
+                let key_signature = K::signature();
+                let value_signature = V::signature();
+                let signature = create_signature(&key_signature, &value_signature);
+
+                Self {
+                    map: entries,
+                    key_signature,
+                    value_signature,
+                    signature,
+                }
+            }
         }
-    }
-}
-
-// TODO: Conversion of Dict from/to BTreeMap
-
-#[derive(Debug, Hash, PartialOrd, Ord, PartialEq, Eq)]
-struct DictEntry<'k, 'v> {
-    key: Value<'k>,
-    value: Value<'v>,
+    };
 }
+to_dict!(HashMap<K: Eq + Hash, V, H>);
+to_dict!(BTreeMap<K: Ord, V>);
 
-impl<'k, 'v> DictEntry<'k, 'v> {
-    fn try_to_owned(&self) -> crate::Result<DictEntry<'static, 'static>> {
-        Ok(DictEntry {
-            key: self.key.try_to_owned().map(Into::into)?,
-            value: self.value.try_to_owned().map(Into::into)?,
-        })
-    }
-
-    fn try_clone(&self) -> Result<Self, Error> {
-        Ok(Self {
-            key: self.key.try_clone()?,
-            value: self.value.try_clone()?,
-        })
-    }
+#[derive(Debug)]
+struct DictEntry<'kref, 'k, 'vref, 'v> {
+    key: &'kref Value<'k>,
+    value: &'vref Value<'v>,
 }
 
-impl<'k, 'v> Serialize for DictEntry<'k, 'v> {
+impl Serialize for DictEntry<'_, '_, '_, '_> {
     fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
     where
         S: Serializer,