zb: interface property setters should return fdo::Error

[wisp3rwind]

The book explains how to write fallible methods and property getters in interfaces at https://dbus2.github.io/zbus/service.html#method-errors.

However, it seems that this is not actually the full picture: Fallible property setters only compile when returning zbus::Error, whereas zbus::fdo::Error produces very opaque error messages like the following:

#[zbus::interface(name = "org.mpris.MediaPlayer2.Player")]
impl MprisPlayer {
    // ...

    #[zbus(property(emits_changed_signal = "true"))]
    async fn volume(&self) -> Volume {
        self.volume
    }

    #[zbus(property)]
    async fn set_volume(&self, _value: Volume) -> zbus::fdo::Result<()> {
        Err(zbus::fdo::Error::NotSupported("...".to_owned()))
    }
}

error[E0271]: expected `{async block@src/mpris_event_handler.rs:309:1: 309:59}` to be a future that resolves to `Result<(), Error>`, but it resolves to `Result<(), Error>`
   --> src/mpris_event_handler.rs:309:1
    |
309 | #[zbus::interface(name = "org.mpris.MediaPlayer2.Player")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `zbus::Error`, found `zbus::fdo::Error`
    |
    = note: expected enum `std::result::Result<_, zbus::Error>`
               found enum `std::result::Result<_, zbus::fdo::Error>`
    = note: required for the cast from `Pin<Box<{async block@src/mpris_event_handler.rs:309:1: 309:59}>>` to `Pin<Box<dyn futures_util::Future<Output = std::result::Result<(), zbus::Error>> + std::marker::Send>>`
    = note: this error originates in the attribute macro `zbus::interface` (in Nightly builds, run with -Z macro-backtrace for more info)

which does not point to the code span that causes the error.

It would be very convenient (and, in my opinion, more consistent) if method setters could also return zbus::fdo::Error. In any case, it would be great if the documentation could be updated to mention the expected return type, and if the interface examples could be extended to contain a fallible property setter. Maybe it would also be possible to enhance the proc macro such that it yields errors that point to the offending code?
Thanks!

This is zbus = 4.4.0.

[zeenix]

It would be very convenient (and, in my opinion, more consistent) if method setters could also return zbus::fdo::Error.

This is very strange. We've a test case for this and it does use fdo::Error. 🤔

This is zbus = 4.4.0.

Hmm.. that test case hasn't changed for a long while since before 4.4.0 so I'm a bit puzzled why you're seeing this.

[zeenix]

Ah, you're looking into setter, not getter methods. Now it makes sense. 😄

[wisp3rwind]

Yep, getters work for me with fdo::Error.

[zeenix]

It would be very convenient (and, in my opinion, more consistent) if method setters could also return zbus::fdo::Error

At first thought, that sounds right but we've to think about whether or not fdo::Error covers the typical cause of failure. For the getter, the typical case of failure is that the property value is currently not available/set. For setter, I can only imagine it happening for some internal failure that shouldn't even happen in the first place (why expose an interface with a property setter before you're ready for letting client give you the value for it?).

Having said that, I do think it makes sense for consistency. Fortunately, we're current in an API break so this could be fixed w/o caring about backwards-compatibility. I doubt many folks use fallible setters anyway so impact of porting to 5.0 would be minimal and even affected folks can easily port this part.

Maybe it would also be possible to enhance the proc macro such that it yields errors that point to the offending code?

Sure. PRs welcome. :)

[wisp3rwind]

Seems to work for non-async, though? At least there's the test case

zbus/zbus/tests/e2e.rs

Line 248 in e12129b

fn set_count(&mut self, val: u32) -> zbus::fdo::Result<()> {

I don't really know much about dbus, so I can't really comment on whether anyone actually uses fallible setters. One usecase might be to return an error when setting out-of-range values on numeric properties?

A workaround is

        Err(zbus::Error::FDO(Box::new(zbus::fdo::Error::NotSupported("...".to_owned()))))

but that is quite verbose. (I'm wondering what happens with non-fdo errors in that case, though: They can't be returned on the bus, can they? They don't seem to implement DBusError.)

Maybe it would also be possible to enhance the proc macro such that it yields errors that point to the offending code?

Sure. PRs welcome. :)

Fair enough :) I doubt I'll be able to do that, however: I'm not at all familiar with developing proc macros.

[zeenix]

Seems to work for non-async, though? At least there's the test case

Oh, that's very strange. Maybe some inconsistency in the macro then. Should be an easier fix then.

I don't really know much about dbus, so I can't really comment on whether anyone actually uses fallible setters. One usecase might be to return an error when setting out-of-range values on numeric properties?

I guess, yeah but then fdo::Error definitely doesn't cover this specific case. For that we need to support returning any error that implements DBusError trait (like we do for methods).

Maybe it would also be possible to enhance the proc macro such that it yields errors that point to the offending code?

Sure. PRs welcome. :)

Fair enough :) I doubt I'll be able to do that, however: I'm not at all familiar with developing proc macros.

Most thing I've learnt are by doing them. The beauty of proc macros is that they're just normal Rust code that runs at compile-time. Of course, it's up to you but I'd suggest giving it a try at least, if you've the motivation and time.

[zeenix]

I guess, yeah but then fdo::Error definitely doesn't cover this specific case. For that we need to support returning any error that implements DBusError trait (like we do for methods).

Oh, looks like I already even had the code to make this happen for getters (#313). If we can do it for getters, we can do it for setters too. However, the same question still remains. 🤔

[wisp3rwind]

I've been trying to follow method dispatch through the ObjectServer, and if I'm reading things right, currently any error will be coerced to fdo::Error anyway before being returned to the bus:

zbus/zbus/src/object_server/mod.rs

Lines 695 to 698 in b7762fa

	return f.await.map_err(|e| match e {
	Error::FDO(e) => *e,
	e => fdo::Error::Failed(format!("{e}")),
	});

i.e. a custom DBusError would in fact be formatted to a string and never reach Connection::reply_dbus_error.

Maybe, if the goal is to minimize allocations as much as possible, there could be an

enum DBusError {
    FDO(fdo::Error),
    Custom(Box<dyn DBusError>),
}

which would be used to pass around method results within zbus? Then, even in the error case, fdo::Error wouldn't require allocation an only custom error types would?

EDIT: In fact, this might be basically what you're saying above?

[zeenix]

i.e. a custom DBusError would in fact be formatted to a string and never reach Connection::reply_dbus_error.

Good catch. yeah I guess, that's what its doing currently. I think we can change it.

Maybe, if the goal is to minimize allocations as much as possible, there could be an

Yeah, but after my comment here, I realised perhaps allocation isn't so bad in this case.

[zeenix]

Maybe, if the goal is to minimize allocations as much as possible, there could be an

enum DBusError {
    FDO(fdo::Error),
    Custom(Box<dyn DBusError>),
}

Good idea though. :)

[wisp3rwind]

Oh, what's actually going is that there's a difference between set and set_mut, which expect zbus::Error and zbus::fdo::error, respectively:

zbus/zbus_macros/src/iface.rs

Lines 845 to 867 in e12129b

	fn set<'call>(
	&'call self,
	property_name: &'call str,
	value: &'call #zbus::zvariant::Value<'_>,
	signal_context: &'call #zbus::object_server::SignalContext<'_>,
	) -> #zbus::object_server::DispatchResult<'call> {
	match property_name {
	#set_dispatch
	_ => #zbus::object_server::DispatchResult::NotFound,
	}
	}
	async fn set_mut(
	&mut self,
	property_name: &str,
	value: &#zbus::zvariant::Value<'_>,
	signal_context: &#zbus::object_server::SignalContext<'_>,
	) -> ::std::option::Option<#zbus::fdo::Result<()>> {
	match property_name {
	#set_mut_dispatch
	_ => ::std::option::Option::None,
	}
	}

So the fn vs. async fn above was a red herring, since there's also a difference in mutability compared to my example code.

[zeenix]

Oh, what's actually going is that there's a difference between set and set_mut, which expect zbus::Error and zbus::fdo::error, respectively

Ah right. I think the issue is that DispatchResult is tailored for methods but is being reused here as well. Perhaps the best way forward is to make it generic over the Error:

pub enum DispatchResult<'a, T: DBusError> {
    /// This interface does not support the given method.
    NotFound,

    /// Retry with [Interface::call_mut].
    ///
    /// This is equivalent to NotFound if returned by call_mut.
    RequiresMut,

    /// The method was found and will be completed by running this Future.
    Async(Pin<Box<dyn Future<Output = std::result::Result<(), T>> + Send + 'a>>),
}

Interestingly, the new_async constructor is already generic so this might have been intended.

[zeenix]

Ah right. I think the issue is that DispatchResult is tailored for methods but is being reused here as well. Perhaps the best way forward is to make it generic over the Error:

Actually, no. There are two types of errors here. One is the error for the service-side (which should be zbus::Error) and the other is for sending over to the client. Reading the new_async constructor again, I realized, it's already doing the right thing: sending over the DBusError to the other side.

I'll need to investigate further what's going on but a likely place to look into is the macro code, specifically here.

P.S. DispatchResult::new_async is not even being used by the macros (or anything in zbus). 🤔

[wisp3rwind]

Perhaps the best way forward is to make it generic over the Error:

Indeed, boxing all errors might be the easiest approach:

Maybe, if the goal is to minimize allocations as much as possible, there could be an

enum DBusError {
    FDO(fdo::Error),
    Custom(Box<dyn DBusError>),
}

Good idea though. :)

I've been thinking about this a bit more, and I think that would require specialization to work: The following From impls which could be used to convert to the enum

enum DBusError {
    FDO(fdo::Error),
    Custom(Box<dyn DBusError>),
}

// Accept `fn prop() -> zbus::Result<u32>`
impl From<zbus::Error> for DBusError {
    fn from(val: zbus::Error) -> Self {
        Self::FDO(fdo::Error::Zbus(val)),
    }
}

// Accept `fn prop() -> zbus::fdo::Result<u32>`
impl From<fdo::Error> for DBusError {
    fn from(val: fdo::Error) -> Self {
        Self::FDO(val),
    }
}

// Accept `fn prop() -> std::result::Result<u32, CustomError>`
impl<E: DBusError> From<E> for DBusError {
    fn from(val: E) -> Self {
        Self::Custom(),
    }
}

would probably not compile, since fdo::Error implements trait DBusError.

I'm not sure that it would be sensible for any of the methods to return the service-side zbus::Error: For a running Connection/ObjectServer, there's no clear place to even report those to (aside from panicking).