zbus_macros: generated Proxy::new() method is sometimes misleading

[zeenix]

In GitLab by @lucab on Oct 28, 2022, 10:00

This ticket is based on a prior conversation at https://github.com/lucab/zbus_systemd/issues/6.

The #[dbus_proxy] macro logic has the following templated code:

impl<'c> #proxy_name<'c> {
  /// Creates a new proxy with the default service & path.
  pub #usage fn new(conn: &#connection) -> #zbus::Result<#proxy_name<'c>> {
    Self::builder(conn).build()#wait
  }
}

This generates a FooProxy::new() method which assumes that both "default service" and "default path" for a proxy have meaningful values. But that is not always the case, for example when default_service or default_path attributes are not specified.

We hit this when working with systemd DBus API, here is a reduced reproducer:

use zbus::dbus_proxy;

#[dbus_proxy(
    interface = "org.freedesktop.systemd1.Service",
    gen_blocking = false,
    default_service = "org.freedesktop.systemd1"
)]
trait Service {
    #[dbus_proxy(property, name = "MainPID")]
    fn main_pid(&self) -> zbus::Result<u32>;
}

type ExResult<T> = Result<T, Box<dyn std::error::Error + 'static>>;

fn main() {
    let rt = tokio::runtime::Runtime::new().unwrap();
    let ret = rt.block_on(run());
    if let Err(e) = ret {
        eprintln!("Error: {}", e);
        std::process::exit(1);
    }
}

async fn run() -> ExResult<()> {
    let conn = zbus::Connection::system().await?;

    {
        let path = "/org/freedesktop/systemd1/unit/systemd_2djournald_2eservice";
        let service = ServiceProxy::builder(&conn).path(path)?.build().await?;
        let main_pid = service.main_pid().await?;
        println!("Service main PID: {}", main_pid);
    }

    {
        let service = ServiceProxy::new(&conn).await?;
        let main_pid = service.main_pid().await?;
        println!("Service main PID: {}", main_pid);
    }

    Ok(())
}

Running this example will result in the following output:

Service main PID: 497
Error: org.freedesktop.DBus.Error.UnknownObject: Unknown object '/org/freedesktop/Service'.

The first line shows that the current way to properly use the generated proxy is through the builder() flow, which is a bit verbose and not ergonomic.

The second line shows that the generated new() method is misleading: it will point to a synthetic /org/freedesktop/Service path, which does not really exist. In fact, this Service interface only exists on individual units, not a as a singleton. For this reason, there is no default_path attribute specified on the macro.

[zeenix]

In GitLab by @lucab on Oct 28, 2022, 10:10

Broadly speaking, I think there are currently four cases:

1. default_path set, default_service set
2. default_path NOT set, default_service set
3. default_path set, default_service NOT set
4. default_path not set, default_service not set

Current generated code covers case 1) only.

For the other three remaining cases, we can either decide to:

• plainly skip generating the new() method, force all consumers to go through the builder object.
• generate dedicated methods instead of new(), different for each case. Something like new_with_path(), new_with_service(), and new_custom().

My slightly preference goes to the latter (with names open to bikeshedding).

[zeenix]

which is a bit verbose and not ergonomic.

I disagree with this one but I do agree that builder shouldn't be needed here and things should work if you don't use it.

• plainly skip generating the new() method, force all consumers to go through the builder object.

The problem is that while we can force them to use builder, we can't force them to pass a path at compile time.

• generate dedicated methods instead of new(), different for each case. Something like new_with_path(), new_with_service(), and new_custom().

We were actually doing that at first and it became a bit of a spaghetti quick so we went for builder pattern.

My slightly preference goes to the latter (with names open to bikeshedding).

The thing is that both options are breakage. zbus itself even relies on the fact the assumed default path/service when they're not specified. I think what we need is an attribute(s) that instruct dbus_proxy to not assume the path/service. Then new takes arguments and all's good.

[zeenix]

In GitLab by @lucab on Oct 31, 2022, 12:48

The thing is that both options are breakage.

We could avoid dropping the new() and just mark it as #[deprecated] for now, if you prefer. But overall if there exist any real-world code today using this buggy new() flow, it is already broken by itself. You can decide how you want to surface this to users (deprecation notice, semver break, panic at runtime, or whatever else), but trying to retain compatibility on something which is known not to work perhaps is not that worthy.

zbus itself even relies on the fact the assumed default path/service when they're not specified.

I'm not that familiar with all zbus internals, I guess you are talking about the ProxyDefault trait here? I guess that one would have to be dropped too when the default values are not defined. Or some fields turned into Option ones. Or maybe split into separate ProxyDefaultInterface / ProxyDefaultDestination / ProxyDefaultPath.

I think what we need is an attribute(s) that instruct dbus_proxy to not assume the path/service. Then new takes arguments and all's good.

I'm fine with adapting new() with different signatures depending on the context.

I'd rather not introduce more attributes, as the macro is already quite complex. The same signal can be already derived from the presence/lack of default_path and default_service.

[zeenix]

The thing is that both options are breakage.

We could avoid dropping the new() and just mark it as #[deprecated] for now, if you prefer.

Yeah, sure. I'm good with that.

But overall if there exist any real-world code today using this buggy new() flow, it is already broken by itself.

I actually thought about it. It is only broken if the default assumptions (or freedesktop service and/or path) are not valid. Unfortunately there is no way of knowing for how much of the existing users that's valid (if at all).

but trying to retain compatibility on something which is known not to work perhaps is not that worthy.

If it wasn't for cases where it is not broken, I would have just removed new for sure. We could do a zbus 4.0 but I'm not too excited about doing that just for this.

zbus itself even relies on the fact the assumed default path/service when they're not specified.

I'm not that familiar with all zbus internals, I guess you are talking about the ProxyDefault trait here? I guess that one would have to be dropped too when the default values are not defined. Or some fields turned into Option ones. Or maybe split into separate ProxyDefaultInterface / ProxyDefaultDestination / ProxyDefaultPath.

Oh I actually didn't even remember about this silly trait. :disappointed: This really complicates things. Ideally I would very much want to drop this trait but that's a big API break.

I think what we need is an attribute(s) that instruct dbus_proxy to not assume the path/service. Then new takes arguments and all's good.

I'm fine with adapting new() with different signatures depending on the context.

I'd rather not introduce more attributes, as the macro is already quite complex. The same signal can be already derived from the presence/lack of default_path and default_service.

Yeah but the breakage again. :) Hence the solution I proposed. We only need 1 attribute though (e.g assume_defaults the defaults to true). We should also emit a compiler warning when either assume_defaults or defaults are not specified.

---

Tl'dr I mostly agree with you here but unfortunately we do need to keep backwards compatibility here. I guess zbus 4.0 will have to happen at some point.

[zeenix]

In GitLab by @lucab on Nov 3, 2022, 15:00

Thanks for the feedback. So it sounds like you want to introduce a new assume_defaults attribute, and then adapt the signature of new(...) based on the combination of attributes, correct?

The semantics may look somehow like these:

• assume_defaults = true and no defaults values -> generated defaults
• assume_defaults = true and at least a default value -> (build error ?)
• assume_defaults = false and no defaults values -> (build error ?)
• assume_defaults = false and at least a default value -> custom new(...) with parameters
• missing assume_defaults and no defaults values -> generated defaults and compiler warning
• missing assume_defaults and at least a default value -> custom new(...) with parameters (and compiler warning?)

Or did I misunderstand something?

[zeenix]

Thanks for the feedback. So it sounds like you want to introduce a new assume_defaults attribute, and then adapt the signature of new(...) based on the combination of attributes, correct?

Genau. :)

assume_defaults = true and at least a default value -> (build error ?)

Since assume_defaults = true is the default, I don't think that's an error. Maybe I misunderstood?

assume_defaults = false and no defaults values -> (build error ?)

No, when this assume_defaults = false, the new() method takes the missing values as args.

assume_defaults = false and at least a default value -> custom new(...) with parameters

Sort of correct, except user can provide both or neither of the default values and new is adapted based on what was provided.

missing assume_defaults and no defaults values -> generated defaults and compiler warning

:thumbsup:

missing assume_defaults and at least a default value -> custom new(...) with parameters (and compiler warning?)

No, I think this needs to be handled the same as the previous one. This is because missing assume_defaults defaults to assume_defaults = true.

Makes sense?

[zeenix]

@lucab In case you provide an MR for this, once your MR for this is created, kindly create an issue for tracking the assume_defaults = false by default switch with API break label applied to it. I'm trying to collect all API breaking changes needed so we can handle them all at once when the time is right.

[zeenix]

btw, we probably should also deprecate ProxyDefault trait.

[zeenix]

In GitLab by @lucab on Nov 8, 2022, 17:28

A patch for this is at https://gitlab.freedesktop.org/dbus/zbus/-/merge_requests/587.

The future semver-breaking ticket for changing the default value is at https://gitlab.freedesktop.org/dbus/zbus/-/issues/288.

Regarding the ProxyDefault trait, I think it may stay as is. I was able to change the macro implementation so that the implementation block is only emitted if both default destination and default path are known.

[zeenix]

A patch for this is at https://gitlab.freedesktop.org/dbus/zbus/-/merge_requests/587.

The future semver-breaking ticket for changing the default value is at https://gitlab.freedesktop.org/dbus/zbus/-/issues/288.

Thanks for both. :thumbsup:

Regarding the ProxyDefault trait, I think it may stay as is. I was able to change the macro implementation so that the implementation block is only emitted if both default destination and default path are known.

Right. I had actually forgotten that you can use the ProxyBuilder w/o ProxyDefault trait. So the #288 would become less of a break then.

[zeenix]

mentioned in commit 1abab5525ed3f92b6e2d07601e837231a9387395