Name collisions in generated Proxy code when propery getter/setter exists as a method #231

Open zeenix opened 2 years ago

In GitLab by @hansihe on Dec 24, 2021, 19:44

There are several different variants of this, all of which I found in interfaces by NetworkManager.

Case 1: Method collision with property

See the org.freedesktop.NetworkManager interface for an example.

This interface has both a signal and a method named State:

    /// state method
    fn state(&self) -> zbus::Result<u32>;

    /// State property
    #[dbus_proxy(property)]
    fn state(&self) -> zbus::Result<u32>;

This will generate the following error when compiled:

error[E0201]: duplicate definitions with name `state`:
  --> service/src/dbus/network_manager/gen/org_freedesktop_NetworkManager.rs:15:1
   |
15 | #[dbus_proxy(interface = "org.freedesktop.NetworkManager")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   | |
   | previous definition of `state` here
   | duplicate definition
   |
   = note: this error originates in the attribute macro `dbus_proxy` (in Nightly builds, run with -Z macro-backtrace for more info)

Case 2: Method/property/signal collision with [signal]_changed function

See the org.freedesktop.NetworkManager interface for an example.

The code generation will generate a function named receive_state_changed for both of these:

    /// StateChanged signal
    #[dbus_proxy(signal)]
    fn state_changed(&self, state: u32) -> zbus::Result<()>;

    /// State property
    #[dbus_proxy(property)]
    fn state(&self) -> zbus::Result<u32>;

This will generate the following error when compiled:

error[E0201]: duplicate definitions with name `receive_state_changed`:
  --> service/src/dbus/network_manager/gen/org_freedesktop_NetworkManager.rs:15:1
   |
15 | #[dbus_proxy(interface = "org.freedesktop.NetworkManager")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   | |
   | previous definition of `receive_state_changed` here
   | duplicate definition
   |
   = note: this error originates in the attribute macro `dbus_proxy` (in Nightly builds, run with -Z macro-backtrace for more info)

Thoughts

I imagine almost all of these can be solved by adding an argument to the dbus_proxy attribute like #[dbus_proxy(property, rename = "state_property"].

While the XML code generator would still fail, the code would be fixable very easily.

As is today, unless I have missed something, cases like these are unrepresentable with zbus_macros.

Thanks for reporting this.

I imagine almost all of these can be solved by adding an argument to the dbus_proxy attribute like #[dbus_proxy(property, rename = "state_property"]

We already do have name attribute for that actually, does that not work for properties?

While the XML code generator would still fail, the code would be fixable very easily.

Yeah but we could also make xmlgen smarter. :)

I imagine almost all of these can be solved by adding an argument to the dbus_proxy attribute like #[dbus_proxy(property, rename = "state_property"]

We already do have name attribute for that actually, does that not work for properties?

Note that it's called name and not rename because you're providing the name of the property/method on D-Bus, which is in PascalCase.

In GitLab by @hansihe on Dec 25, 2021, 14:37

Ah! I must have missed that attribute somehow. Thanks for pointing me in the right direction!

I guess this is now mainly an issue for making xmlgen smarter :)

I'll update the issue text in a bit.

Ah! I must have missed that attribute somehow. Thanks for pointing me in the right direction!

Np. :)

I guess this is now mainly an issue for making xmlgen smarter

Yeah, it's not super smart at the moment, unfortunately.

I'll update the issue text in a bit.

No hurry. Happy holidays. :)

@zeenix Do you have some pointers where to start for making xmlgen smarter?

I started digging into the zbus_xmlgen code but I'm not sure what would even be to proper solution to this. Further I also think that https://github.com/dbus2/zbus/issues/169 might also be related and could be solved with the same approach.

I see several ways of preventing this issue:

Make xmlgen detect collisions and just append _ until there is no more collision. This is is what dbus-codegen from dbus-rs seems to be doing.
Pre or post fix all generated methods with something like method_, property_ or signal. This would avoid collisions but it doesn't seem to be very nice.
Only namspace properties and signals. Ideally we would find a way of namespacing properties and signals in a way that they would never conflict with generated method names. This would also solve #169

Make xmlgen detect collisions and just append _ until there is no more collision.

Sure.

2. Pre or post fix all generated methods with something like method_, property_ or signal. This would avoid collisions but it doesn't seem to be very nice.

Yeah, that's not good.

3. Only namspace properties and signals.

Not sure I like this one. Ideally, the names should be natural derivatives of the underlying names. We should employ other methods (e.g suggestion #1).

Hitting this now with login1 unfortunately - there is a collision with set_wall_message

Wouldn't solution # 1 cause end users of the autogenerated code to not know which of the underscored functions they should use?

Wouldn't solution # 1 cause end users of the autogenerated code to not know which of the underscored functions they should use?

AFAICT this happens when there is a method that does the same as the property setter/getter so at least in most cases, this won't be an issue. The name attribute will inform you of the D-Bus method the method corresponds to, and property attribute on the property, informs you that it's a getter/setter and for which property exactly.

Hello, just a post to submit a "solution" for NetworkManager After generate trait with zbus xmlgen not possible to compile cause 'state' is already define.

zbus version 5.0.1

Rename

    /// state method
    #[zbus(name = "state")]
    fn state_method(&self) -> zbus::Result<u32>; // <--- rename method

    /// StateChanged signal
    #[zbus(signal, name="StateChanged")] // <--- add name with member value i find in dbus-monitor
    fn signal_state_changed(&self, state: u32) -> zbus::Result<()>;

My function to listen NetworkManager.StateChanged become receive_signal_state_changed()

just a post to submit a "solution" for NetworkManager

It's called a workaround. :)

dbus2 / zbus