zvariant::Dict please allow iterating the key/value pairs

[zeenix]

In GitLab by @estokes on Oct 12, 2022, 22:46

Hi, at the moment there is no way to just iterate the key/value pairs of a zvariant::Dict. If you want to do it you need to convert it to a hashmap, and to do that you need to read the signature and pick a concrete type that implements Basic, it's actually quite a lot of code to do this, and what I (and probably others) really want is just to iterate the values. I see that it's stored as a Vec of DictEntries, which are just the key and value, so it would be trivial to allow this iteration.

Thanks, zbus is awesome!

[zeenix]

Hi there,

at the moment there is no way to just iterate the key/value pairs of a zvariant::Dict. If you want to do it you need to convert it to a hashmap, and to do that you need to read the signature and pick a concrete type that implements Basic, it's actually quite a lot of code to do this,

So Dict isn't really meant to be used directly. Typically you want to:

1. use a HashMap itself. Type is implemented for that.
2. if you need to (de)serialize it as a Value:
   1. use your own struct by making use of SerializeDict and DeserializeDict derives.
   2. Convert from/to HashMap to/from Value or OwnedValue types.

Having said that, I don't know your exact use case and I have no objections on providing the API you're requesting.

I'm having trouble finding a lot of time these days for zbus work so it'll take a while, unless you could contribute this.

[zeenix]

In GitLab by @estokes on Oct 14, 2022, 24:44

I'm bridging dbus values to another quite similar dynamically typed network protocol (netidx), so I really need to just iterate and translate the values directly (e.g. I have a recursive fn dbus_value_to_netidx_value(v: &zvariant::Value) -> Value { ... }, all of the cases are quite straightforward except zvariant::Dict).

I would be happy to work on this.

[zeenix]

I'm bridging dbus values to another quite similar dynamically typed network protocol (netidx), so I really need to just iterate and translate the values directly (e.g. I have a recursive fn dbus_value_to_netidx_value(v: &zvariant::Value) -> Value { ... }, all of the cases are quite straightforward except zvariant::Dict).

I see. I just wanted to make sure you're not making your life harder than it needs to be. :smile:

I would be happy to work on this.

:thumbsup: I would prefer not to expose DictEntry in pub api btw and have a generic iterator so people can iterate over specific types.

[adamcavendish]

Hi @zeenix, I would still suggest that we should expose a generic iterator for zvariant::Dict. It is because TryFrom signature is listed as following:

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>

and K requires Hash. For f64, it is a valid D-Bus key but it cannot be converted from Dict to HashMap because f64 doesn't implement std::hash::Hash.

Feel free to try:

let key_signature = zvariant::Signature::from_str_unchecked("d");
let val_signature = zvariant::Signature::from_str_unchecked("s");
let dict = zvariant::Dict::new(key_signature, val_signature);
let hashmap = HashMap::<f64, String>::try_from(v).unwrap();

Note that I snapshot this from zbus v3. However, even on the latest zvariant version (v4), the Dict to either HashMap or BTreeMap is requiring Hash or Ord correspondingly. Neither of them are implemented by f64.

To conclude, currently a Dict taking f64 as key won't have any safe way to take out data after the data is added to Dict unless users manually implement a f64 wrapper with Type + TryFrom<Value> + Hash + Eq implemented.

[adamcavendish]

For anyone who meets the same issue or possibly locked into a legacy zbus version can use ordered_float package and make a f64 wrapper to workaround this issue:

use std::{fmt, hash};

use serde::ser::Error;
use zvariant::Basic;

/// F64Wrapper is a wrapper for ordered_float::OrderedFloat package.
/// We would need this wrapper is because of a zvariant issue that its Dict doesn't implement iterations.
/// Nor can it be converted into HashMap<f64, BuscliVariant> because f64 doesn't implement Hash trait.
/// Remove F64Wrapper after zvariant::Dict supports iteration.
#[derive(PartialEq, Eq, hash::Hash, PartialOrd, Ord)]
struct F64Wrapper(ordered_float::OrderedFloat<f64>);

impl zvariant::Basic for F64Wrapper {
    const SIGNATURE_CHAR: char = f64::SIGNATURE_CHAR;
    const SIGNATURE_STR: &'static str = f64::SIGNATURE_STR;

    fn alignment(format: zvariant::EncodingFormat) -> usize {
        f64::alignment(format)
    }
}

impl zvariant::Type for F64Wrapper {
    fn signature() -> zvariant::Signature<'static> {
        zvariant::Signature::from_static_str_unchecked(Self::SIGNATURE_STR)
    }
}

impl<'a> TryFrom<zvariant::Value<'a>> for F64Wrapper {
    type Error = zvariant::Error;

    fn try_from(value: zvariant::Value<'a>) -> Result<Self, Self::Error> {
        match value {
            zvariant::Value::U8(v) => Ok(Self(ordered_float::OrderedFloat(v as f64))),
            zvariant::Value::I16(v) => Ok(Self(ordered_float::OrderedFloat(v as f64))),
            zvariant::Value::U16(v) => Ok(Self(ordered_float::OrderedFloat(v as f64))),
            zvariant::Value::I32(v) => Ok(Self(ordered_float::OrderedFloat(v as f64))),
            zvariant::Value::U32(v) => Ok(Self(ordered_float::OrderedFloat(v as f64))),
            zvariant::Value::I64(v) => Ok(Self(ordered_float::OrderedFloat(v as f64))),
            zvariant::Value::U64(v) => Ok(Self(ordered_float::OrderedFloat(v as f64))),
            zvariant::Value::F64(v) => Ok(Self(ordered_float::OrderedFloat(v as f64))),
            _ => Err(zvariant::Error::custom(
                "invalid TryFrom<zvariant::Value> type for F64Wrapper",
            )),
        }
    }
}

impl<'a> Into<zvariant::Value<'a>> for F64Wrapper {
    fn into(self) -> zvariant::Value<'a> {
        zvariant::Value::F64(self.0 .0)
    }
}

impl fmt::Display for F64Wrapper {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.write_str(self.0 .0.to_string().as_str())
    }
}

[zeenix]

Hi @zeenix, I would still suggest that we should expose a generic iterator for zvariant::Dict. It is because TryFrom signature is listed as following:

Isn't this what I wrote at the end of my last comment? :thinking:

For f64, it is a valid D-Bus key but it cannot be converted from Dict to HashMap because f64 doesn't implement std::hash::Hash.

True but can't we just take the code you provided into zvariant? So it's a supported solution instead of a workaround? We can just call it F64.

impl zvariant::Type for F64Wrapper {
    fn signature() -> zvariant::Signature<'static> {
        zvariant::Signature::from_static_str_unchecked(Self::SIGNATURE_STR)
    }
}

Can't it just use zvariant::Type derive macro?

[adamcavendish]

Hi @zeenix, I would still suggest that we should expose a generic iterator for zvariant::Dict. It is because TryFrom signature is listed as following:

Isn't this what I wrote at the end of my last comment? 🤔

For f64, it is a valid D-Bus key but it cannot be converted from Dict to HashMap because f64 doesn't implement std::hash::Hash.

True but can't we just take the code you provided into zvariant? So it's a supported solution instead of a workaround? We can just call it F64.

impl zvariant::Type for F64Wrapper {
    fn signature() -> zvariant::Signature<'static> {
        zvariant::Signature::from_static_str_unchecked(Self::SIGNATURE_STR)
    }
}

Can't it just use zvariant::Type derive macro?

Yeah, definitely. I am writing it as a comment is because it provides as a reference implementation for users that are locked down to a specific zbus version. Feel free to add it as a part of zvariant in the future version.

Note that it cannot derive either zvariant::Type nor zvariant::Basic is because ordered_float::OrderedFloat doesn't implement Type nor Basic.

[zeenix]

Feel free to add it as a part of zvariant in the future version.

I was hoping you'd provide a PR, since you've written most of the code already anyway.

Note that it cannot derive either zvariant::Type nor zvariant::Basic is because ordered_float::OrderedFloat doesn't implement Type nor Basic.

Ah gotcha.

[adamcavendish]

I can provide a PR if you are ok to depend on ordered_float crate: https://crates.io/crates/ordered-float

[zeenix]

I can provide a PR if you are ok to depend on ordered_float crate: https://crates.io/crates/ordered-float

Hmm.. we can do that as an optional dep but there is another way too: F64 struct can keep the integer and fractional part separately.

[adamcavendish]

I can provide a PR if you are ok to depend on ordered_float crate: https://crates.io/crates/ordered-float

Hmm.. we can do that as an optional dep but there is another way too: F64 struct can keep the integer and fractional part separately.

The stackoverflow reference is a shabby implementation of ordered-float crate. ordered-float is splitting integer and fractional parts separately but also takes in count for +/-0 and NaN float values, etc.

Other than the float implementation, would you think it's appropriate to expose an API to iterate via a tuple of key and value, aka. (Value, Value) type? In such way, we'll keep the internal data structure private but still providing an acceptable interface to iterate through Dict with a similar way like what HashMap or BTreeMap does.

[zeenix]

The stackoverflow reference is a shabby implementation of ordered-float crate. ordered-float is splitting integer and fractional parts separately but also takes in count for +/-0 and NaN float values, etc.

Right, I would just like to avoid adding deps (even if they're small and don't drag other deps). We already take a lot of hate for number of deps.

Other than the float implementation, would you think it's appropriate to expose an API to iterate via a tuple of key and value, aka. (Value, Value) type? In such way, we'll keep the internal data structure private but still providing an acceptable interface to iterate through Dict with a similar way like what HashMap or BTreeMap does.

Ah, that's what you meant by "generic". Yeah, sure that might actually be a better approach.

[adamcavendish]

The stackoverflow reference is a shabby implementation of ordered-float crate. ordered-float is splitting integer and fractional parts separately but also takes in count for +/-0 and NaN float values, etc.

Right, I would just like to avoid adding deps (even if they're small and don't drag other deps). We already take a lot of hate for number of deps.

Other than the float implementation, would you think it's appropriate to expose an API to iterate via a tuple of key and value, aka. (Value, Value) type? In such way, we'll keep the internal data structure private but still providing an acceptable interface to iterate through Dict with a similar way like what HashMap or BTreeMap does.

Ah, that's what you meant by "generic". Yeah, sure that might actually be a better approach.

Sorry that I got laid off last week and many things have happened. I've implemented the iteration with (Value, Value) as Item in PR #734 . However, because f64 doesn't implement Ord we still cannot create a valid Dict with a{dv} signature.

I've put the PR as a draft currently and if you have any specific plan to make the f64 support possible, I can follow up.

[zeenix]

Sorry that I got laid off last week and many things have happened.

Oh, I'm sorry to hear. I know from personal experience that really sucks. I hope you find an even better job very soon.

I've implemented the iteration with (Value, Value) as Item in PR #734 .

Awesome, I'm looking at it now. Sorry for the delay.

However, because f64 doesn't implement Ord we still cannot create a valid Dict with a{dv} signature.

Right, we can (and probably should) treat that as a separate issue anyway.

I've put the PR as a draft currently and if you have any specific plan to make the f64 support possible, I can follow up.

I will think about it but your PR shouldn't need to depend on that IMO.