[bug] zbus-xmlgen generates code that fails to compile with a dictionary-like property

[emilio]

(First of all, thanks for this crate, looks amazing)

I was poking at wpa_supplicant's dbus functionality: https://w1.fi/wpa_supplicant/devel/dbus.html

I generated a rust file like:

$ zbus-xmlgen system fi.w1.wpa_supplicant1 /fi/w1/wpa_supplicant1/Interfaces/1

Which generated a p2pdevice.rs file like this (among others):

//! # D-Bus interface proxy for: `fi.w1.wpa_supplicant1.Interface.P2PDevice`
//!
//! This code was generated by `zbus-xmlgen` `5.0.1` from D-Bus introspection data.
//! Source: `Interface '/fi/w1/wpa_supplicant1/Interfaces/1' from service 'fi.w1.wpa_supplicant1' on system bus`.
//!
//! You may prefer to adapt it, instead of using it verbatim.
//!
//! More information can be found in the [Writing a client proxy] section of the zbus
//! documentation.
//!
//! This type implements the [D-Bus standard interfaces], (`org.freedesktop.DBus.*`) for which the
//! following zbus API can be used:
//!
//! * [`zbus::fdo::IntrospectableProxy`]
//! * [`zbus::fdo::PropertiesProxy`]
//!
//! Consequently `zbus-xmlgen` did not generate code for the above interfaces.
//!
//! [Writing a client proxy]: https://dbus2.github.io/zbus/client.html
//! [D-Bus standard interfaces]: https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces,
use zbus::proxy;
#[proxy(
    interface = "fi.w1.wpa_supplicant1.Interface.P2PDevice",
    default_service = "fi.w1.wpa_supplicant1",
    default_path = "/fi/w1/wpa_supplicant1/Interfaces/1"
)]
pub trait P2PDevice {
    /// AddPersistentGroup method
    fn add_persistent_group(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<zbus::zvariant::OwnedObjectPath>;

    /// AddService method
    fn add_service(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// Cancel method
    fn cancel(&self) -> zbus::Result<()>;

    /// Connect method
    fn connect(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<String>;

    /// DeleteService method
    fn delete_service(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// Disconnect method
    fn disconnect(&self) -> zbus::Result<()>;

    /// ExtendedListen method
    fn extended_listen(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// Find method
    fn find(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// Flush method
    fn flush(&self) -> zbus::Result<()>;

    /// FlushService method
    fn flush_service(&self) -> zbus::Result<()>;

    /// GroupAdd method
    fn group_add(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// Invite method
    fn invite(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// Listen method
    fn listen(&self, timeout: i32) -> zbus::Result<()>;

    /// PresenceRequest method
    fn presence_request(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// ProvisionDiscoveryRequest method
    fn provision_discovery_request(
        &self,
        peer: &zbus::zvariant::ObjectPath<'_>,
        config_method: &str,
    ) -> zbus::Result<()>;

    /// RejectPeer method
    fn reject_peer(&self, peer: &zbus::zvariant::ObjectPath<'_>) -> zbus::Result<()>;

    /// RemoveAllPersistentGroups method
    fn remove_all_persistent_groups(&self) -> zbus::Result<()>;

    /// RemoveClient method
    fn remove_client(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// RemovePersistentGroup method
    fn remove_persistent_group(&self, path: &zbus::zvariant::ObjectPath<'_>) -> zbus::Result<()>;

    /// ServiceDiscoveryCancelRequest method
    fn service_discovery_cancel_request(&self, args: u64) -> zbus::Result<()>;

    /// ServiceDiscoveryExternal method
    fn service_discovery_external(&self, arg: i32) -> zbus::Result<()>;

    /// ServiceDiscoveryRequest method
    fn service_discovery_request(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<u64>;

    /// ServiceDiscoveryResponse method
    fn service_discovery_response(
        &self,
        args: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// ServiceUpdate method
    fn service_update(&self) -> zbus::Result<()>;

    /// StopFind method
    fn stop_find(&self) -> zbus::Result<()>;

    /// DeviceFound signal
    #[zbus(signal)]
    fn device_found(&self, path: zbus::zvariant::ObjectPath<'_>) -> zbus::Result<()>;

    /// DeviceFoundProperties signal
    #[zbus(signal)]
    fn device_found_properties(
        &self,
        path: zbus::zvariant::ObjectPath<'_>,
        properties: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// DeviceLost signal
    #[zbus(signal)]
    fn device_lost(&self, path: zbus::zvariant::ObjectPath<'_>) -> zbus::Result<()>;

    /// FindStopped signal
    #[zbus(signal)]
    fn find_stopped(&self) -> zbus::Result<()>;

    /// GONegotiationFailure signal
    #[zbus(signal, name = "GONegotiationFailure")]
    fn gonegotiation_failure(
        &self,
        properties: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// GONegotiationRequest signal
    #[zbus(signal, name = "GONegotiationRequest")]
    fn gonegotiation_request(
        &self,
        path: zbus::zvariant::ObjectPath<'_>,
        dev_passwd_id: u16,
        device_go_intent: u8,
    ) -> zbus::Result<()>;

    /// GONegotiationSuccess signal
    #[zbus(signal, name = "GONegotiationSuccess")]
    fn gonegotiation_success(
        &self,
        properties: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// GroupFinished signal
    #[zbus(signal)]
    fn group_finished(
        &self,
        properties: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// GroupFormationFailure signal
    #[zbus(signal)]
    fn group_formation_failure(&self, reason: &str) -> zbus::Result<()>;

    /// GroupStarted signal
    #[zbus(signal)]
    fn group_started(
        &self,
        properties: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// InvitationReceived signal
    #[zbus(signal)]
    fn invitation_received(
        &self,
        properties: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// InvitationResult signal
    #[zbus(signal)]
    fn invitation_result(
        &self,
        invite_result: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// PersistentGroupAdded signal
    #[zbus(signal)]
    fn persistent_group_added(
        &self,
        path: zbus::zvariant::ObjectPath<'_>,
        properties: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// PersistentGroupRemoved signal
    #[zbus(signal)]
    fn persistent_group_removed(&self, path: zbus::zvariant::ObjectPath<'_>) -> zbus::Result<()>;

    /// ProvisionDiscoveryFailure signal
    #[zbus(signal)]
    fn provision_discovery_failure(
        &self,
        peer_object: zbus::zvariant::ObjectPath<'_>,
        status: i32,
    ) -> zbus::Result<()>;

    /// ProvisionDiscoveryPBCRequest signal
    #[zbus(signal, name = "ProvisionDiscoveryPBCRequest")]
    fn provision_discovery_pbcrequest(
        &self,
        peer_object: zbus::zvariant::ObjectPath<'_>,
    ) -> zbus::Result<()>;

    /// ProvisionDiscoveryPBCResponse signal
    #[zbus(signal, name = "ProvisionDiscoveryPBCResponse")]
    fn provision_discovery_pbcresponse(
        &self,
        peer_object: zbus::zvariant::ObjectPath<'_>,
    ) -> zbus::Result<()>;

    /// ProvisionDiscoveryRequestDisplayPin signal
    #[zbus(signal)]
    fn provision_discovery_request_display_pin(
        &self,
        peer_object: zbus::zvariant::ObjectPath<'_>,
        pin: &str,
    ) -> zbus::Result<()>;

    /// ProvisionDiscoveryRequestEnterPin signal
    #[zbus(signal)]
    fn provision_discovery_request_enter_pin(
        &self,
        peer_object: zbus::zvariant::ObjectPath<'_>,
    ) -> zbus::Result<()>;

    /// ProvisionDiscoveryResponseDisplayPin signal
    #[zbus(signal)]
    fn provision_discovery_response_display_pin(
        &self,
        peer_object: zbus::zvariant::ObjectPath<'_>,
        pin: &str,
    ) -> zbus::Result<()>;

    /// ProvisionDiscoveryResponseEnterPin signal
    #[zbus(signal)]
    fn provision_discovery_response_enter_pin(
        &self,
        peer_object: zbus::zvariant::ObjectPath<'_>,
    ) -> zbus::Result<()>;

    /// ServiceDiscoveryRequest signal
    #[zbus(signal)]
    fn service_discovery_request(
        &self,
        sd_request: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// ServiceDiscoveryResponse signal
    #[zbus(signal)]
    fn service_discovery_response(
        &self,
        sd_response: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// WpsFailed signal
    #[zbus(signal)]
    fn wps_failed(
        &self,
        name: &str,
        args: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// Group property
    #[zbus(property)]
    fn group(&self) -> zbus::Result<zbus::zvariant::OwnedObjectPath>;

    /// P2PDeviceConfig property
    #[zbus(property, name = "P2PDeviceConfig")]
    fn p2pdevice_config(
        &self,
    ) -> zbus::Result<std::collections::HashMap<String, zbus::zvariant::OwnedValue>>;
    #[zbus(property, name = "P2PDeviceConfig")]
    fn set_p2pdevice_config(
        &self,
        value: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

    /// PeerGO property
    #[zbus(property, name = "PeerGO")]
    fn peer_go(&self) -> zbus::Result<zbus::zvariant::OwnedObjectPath>;

    /// Peers property
    #[zbus(property)]
    fn peers(&self) -> zbus::Result<Vec<zbus::zvariant::OwnedObjectPath>>;

    /// PersistentGroups property
    #[zbus(property)]
    fn persistent_groups(&self) -> zbus::Result<Vec<zbus::zvariant::OwnedObjectPath>>;

    /// Role property
    #[zbus(property)]
    fn role(&self) -> zbus::Result<String>;
}

Which looks good / believable. I need to remove the default path in order to use it more generally but that's also expected.

However when building it, I get:

error[E0277]: the trait bound `zbus::zvariant::Value<'_>: From<HashMap<&str, &zbus::zvariant::Value<'_>>>` is not satisfied
   --> examples/dbus/wpa_supplicant/p2pdevice.rs:315:9
    |
22  | / #[proxy(
23  | |     interface = "fi.w1.wpa_supplicant1.Interface.P2PDevice",
24  | |     default_service = "fi.w1.wpa_supplicant1",
25  | | )]
    | |__- required by a bound introduced by this call
...
315 |           value: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    |           ^^^^^ the trait `From<HashMap<&str, &zbus::zvariant::Value<'_>>>` is not implemented for `zbus::zvariant::Value<'_>`, which is required by `HashMap<&str, &zbus::zvariant::Value<'_>>: Into<zbus::zvariant::Value<'_>>`
    |
    = help: the following other types implement trait `From<T>`:
              `zbus::zvariant::Value<'_>` implements `From<OwnedBusName>`
              `zbus::zvariant::Value<'_>` implements `From<OwnedObjectPath>`
              `zbus::zvariant::Value<'_>` implements `From<OwnedValue>`
              `zbus::zvariant::Value<'_>` implements `From<std::string::String>`
              `zbus::zvariant::Value<'a>` implements `From<&'a &'a str>`
              `zbus::zvariant::Value<'a>` implements `From<&'a bool>`
              `zbus::zvariant::Value<'a>` implements `From<&'a f32>`
              `zbus::zvariant::Value<'a>` implements `From<&'a f64>`
            and 48 others
    = note: required for `HashMap<&str, &zbus::zvariant::Value<'_>>` to implement `Into<zbus::zvariant::Value<'_>>`
note: required by a bound in `Proxy::<'a>::set_property`
   --> /home/emilio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zbus-5.2.0/src/proxy/mod.rs:810:17
    |
808 |     pub async fn set_property<'t, T>(&self, property_name: &str, value: T) -> fdo::Result<()>
    |                  ------------ required by a bound in this associated function
809 |     where
810 |         T: 't + Into<Value<'t>>,
    |                 ^^^^^^^^^^^^^^^ required by this bound in `Proxy::<'a>::set_property`

error[E0277]: the trait bound `zbus::zvariant::Value<'_>: From<HashMap<&str, &zbus::zvariant::Value<'_>>>` is not satisfied
   --> examples/dbus/wpa_supplicant/p2pdevice.rs:22:1
    |
22  | / #[proxy(
23  | |     interface = "fi.w1.wpa_supplicant1.Interface.P2PDevice",
24  | |     default_service = "fi.w1.wpa_supplicant1",
25  | | )]
    | |__^ the trait `From<HashMap<&str, &zbus::zvariant::Value<'_>>>` is not implemented for `zbus::zvariant::Value<'_>`, which is required by `HashMap<&str, &zbus::zvariant::Value<'_>>: Into<zbus::zvariant::Value<'_>>`
    |
    = help: the following other types implement trait `From<T>`:
              `zbus::zvariant::Value<'_>` implements `From<OwnedBusName>`
              `zbus::zvariant::Value<'_>` implements `From<OwnedObjectPath>`
              `zbus::zvariant::Value<'_>` implements `From<OwnedValue>`
              `zbus::zvariant::Value<'_>` implements `From<std::string::String>`
              `zbus::zvariant::Value<'a>` implements `From<&'a &'a str>`
              `zbus::zvariant::Value<'a>` implements `From<&'a bool>`
              `zbus::zvariant::Value<'a>` implements `From<&'a f32>`
              `zbus::zvariant::Value<'a>` implements `From<&'a f64>`
            and 48 others
    = note: required for `HashMap<&str, &zbus::zvariant::Value<'_>>` to implement `Into<zbus::zvariant::Value<'_>>`
note: required by a bound in `Proxy::<'a>::set_property`
   --> /home/emilio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zbus-5.2.0/src/proxy/mod.rs:810:17
    |
808 |     pub async fn set_property<'t, T>(&self, property_name: &str, value: T) -> fdo::Result<()>
    |                  ------------ required by a bound in this associated function
809 |     where
810 |         T: 't + Into<Value<'t>>,
    |                 ^^^^^^^^^^^^^^^ required by this bound in `Proxy::<'a>::set_property`
    = note: this error originates in the attribute macro `proxy` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `zbus::zvariant::Value<'_>: From<HashMap<&str, &zbus::zvariant::Value<'_>>>` is not satisfied
   --> examples/dbus/wpa_supplicant/p2pdevice.rs:22:1
    |
22  | / #[proxy(
23  | |     interface = "fi.w1.wpa_supplicant1.Interface.P2PDevice",
24  | |     default_service = "fi.w1.wpa_supplicant1",
25  | | )]
    | |__^ the trait `From<HashMap<&str, &zbus::zvariant::Value<'_>>>` is not implemented for `zbus::zvariant::Value<'_>`, which is required by `HashMap<&str, &zbus::zvariant::Value<'_>>: Into<zbus::zvariant::Value<'_>>`
    |
    = help: the following other types implement trait `From<T>`:
              `zbus::zvariant::Value<'_>` implements `From<OwnedBusName>`
              `zbus::zvariant::Value<'_>` implements `From<OwnedObjectPath>`
              `zbus::zvariant::Value<'_>` implements `From<OwnedValue>`
              `zbus::zvariant::Value<'_>` implements `From<std::string::String>`
              `zbus::zvariant::Value<'a>` implements `From<&'a &'a str>`
              `zbus::zvariant::Value<'a>` implements `From<&'a bool>`
              `zbus::zvariant::Value<'a>` implements `From<&'a f32>`
              `zbus::zvariant::Value<'a>` implements `From<&'a f64>`
            and 48 others
    = note: required for `HashMap<&str, &zbus::zvariant::Value<'_>>` to implement `Into<zbus::zvariant::Value<'_>>`
note: required by a bound in `Proxy::<'a>::set_property`
   --> /home/emilio/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zbus-5.2.0/src/proxy/mod.rs:810:17
    |
808 |     pub async fn set_property<'t, T>(&self, property_name: &str, value: T) -> fdo::Result<()>
    |                  ------------ required by a bound in this associated function
809 |     where
810 |         T: 't + Into<Value<'t>>,
    |                 ^^^^^^^^^^^^^^^ required by this bound in `Proxy::<'a>::set_property`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `ngn` (example "dbus") due to 3 previous errors

I guess a general From<HashMap<K, V>> for Value<'_> might be needed? I can comment out / remove the problematic bit, which seems to be:

    #[zbus(property, name = "P2PDeviceConfig")]
    fn set_p2pdevice_config(
        &self,
        value: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
    ) -> zbus::Result<()>;

And then use the lower-level set_property I suppose. But that is slightly annoying bBelieve it or not, that was one of the setters I was interested in using.

If implementing From<HashMap<...>> for Value is the right fix, I can give a shot at fixing it btw :)

[emilio]

Ah, so it was a bit tricky... This fixes it locally:

diff --git a/zvariant/src/into_value.rs b/zvariant/src/into_value.rs
index 1e35b96f..cfffc054 100644
--- a/zvariant/src/into_value.rs
+++ b/zvariant/src/into_value.rs
@@ -84,6 +84,12 @@ impl From<String> for Value<'_> {
     }
 }

+impl<'a, 'v> From<&'a Value<'v>> for Value<'v> {
+    fn from(v: &'a Value<'v>) -> Value<'v> {
+        v.clone()
+    }
+}
+
 impl<'v, 's: 'v, T> From<T> for Value<'v>
 where
     T: Into<Structure<'s>>,
diff --git a/zvariant/src/value.rs b/zvariant/src/value.rs
index 76e90d20..30fe0e8f 100644
--- a/zvariant/src/value.rs
+++ b/zvariant/src/value.rs
@@ -966,14 +966,6 @@ impl Type for Value<'_> {
     const SIGNATURE: &'static Signature = &Signature::Variant;
 }

-impl<'a> TryFrom<&Value<'a>> for Value<'a> {
-    type Error = crate::Error;
-
-    fn try_from(value: &Value<'a>) -> crate::Result<Value<'a>> {
-        value.try_clone()
-    }
-}
-
 impl Clone for Value<'_> {
     /// Clone the value.
     ///

But is of course not ideal...

[emilio]

Other alternatives which would be better for my use case, I think would be to make the hashmap values owned in the generated code, for properties, so:

diff --git a/examples/dbus/wpa_supplicant/p2pdevice.rs b/examples/dbus/wpa_supplicant/p2pdevice.rs
index dfd406f..3512081 100644
--- a/examples/dbus/wpa_supplicant/p2pdevice.rs
+++ b/examples/dbus/wpa_supplicant/p2pdevice.rs
@@ -312,7 +312,7 @@ pub trait P2PDevice {
     #[zbus(property, name = "P2PDeviceConfig")]
     fn set_p2pdevice_config(
         &self,
-        value: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
+        value: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,
     ) -> zbus::Result<()>;

     /// PeerGO property

But a bit out of my dbus depth, so not sure whether that'd be more generally desirable. For now I think I'll roll with that tweak, since for my use case it should work just fine.

[zeenix]

(First of all, thanks for this crate, looks amazing)

Thank you. 🙏

Other alternatives which would be better for my use case, I think would be to make the hashmap values owned in the generated code, for properties, so:

diff --git a/examples/dbus/wpa_supplicant/p2pdevice.rs b/examples/dbus/wpa_supplicant/p2pdevice.rs
index dfd406f..3512081 100644
--- a/examples/dbus/wpa_supplicant/p2pdevice.rs
+++ b/examples/dbus/wpa_supplicant/p2pdevice.rs
@@ -312,7 +312,7 @@ pub trait P2PDevice {
     #[zbus(property, name = "P2PDeviceConfig")]
     fn set_p2pdevice_config(
         &self,
-        value: std::collections::HashMap<&str, &zbus::zvariant::Value<'_>>,
+        value: std::collections::HashMap<&str, zbus::zvariant::Value<'_>>,

Yes, I believe this is the desirable output. If you could provide a PR for this, that would be great.

---

P.S. Please keep in mind that xmlgen is currently meant to be just a helper tool to get your started easily and in most cases, you'd want to manually adjust the generated code. It needs some love before it should be used as a reusable tool (e.g as part of a build).

[emilio]

Yes, I believe this is the desirable output. If you could provide a PR for this, that would be great.

So fwiw I thought of that, and fixing this case is trivial, but there are other cases that are a bit trickier I believe. E.g. for slices we currently generate &[&Value<'_>] or so (fine), but they also have that issue. I guess we could also just turn them into &[Value<'_>], but that'd end up with useless clones in the conversion.