Derive macro for generating method tuples from structs

[eaglesemanation]

I've been working on a small personal project, which is my first experience with both DBus and dbus-rs. To be more exact, I wanted to interact with Mutter DisplayConfig, and almost immediately I came to a conclusion that managing needed arguments in form of tuples would be a nightmare.
Instead, I spent 3 days writing a proof of concept derive macro, which takes a struct and generates a tuple type from it, plus implements TryFrom in both directions. Here is my derive macro and here is an example of how I'm using it.
So, my question: Is this something that is worth trying to upstream? I'd love to get any kind of feedback on this.

[diwic]

Hey, interesting stuff! :-)

As for trying to upstream it - let's have a discussion about it and see how things go?

I had a bird's eye look at the code, and so my main question is about the traits (DbusArg and DbusDictKey), any reason you can't use existing traits such as AppendAll, ArgAll and ReadAll? If you derive those instead, you shouldn't have to convert structs back and forth to a tuple just to be able to send them over dbus.

As for this question:

// I have no clue how dbus-rs has f64 and std::fs::File as a DictKey, they don't implement
// core::hash::Hash

The reason is that since dbus protocol specifies that f64 and Files can be dict keys I do my best to support that. But it would be rather weird and I haven't seen anyone using it in practice.

[eaglesemanation]

Thanks for taking a look.

1. I need a custom trait for mapping types that are not 1:1, for example in my use case I'm defining a enum Transform which maps to u8 as Dbus argument. I definitely want end user to be able to implement that trait for whatever crazy data type they want. This in turn makes containers implementation not trivial, so for example, I have to iterate through elements in Vec<Transform> and apply DbusArg conversion to u8 for every element. Originally I tried implementing DbusArg<T> where T: dbus::arg::Arg, but because there are no specializations in Rust - I couldn't do custom logic for Vec. I might be overthinking this, let me know if you see any better approach.
2. I originally tried to add both f64 and File, but DbusArg<HashMap<...>> implementation would not compile screaming at me, because they don't implement core::hash::Hash. I wasn't exactly sure how this codebase compiles without the same issues, so that comment was mostly a reminder for myself to dig into this.

[eaglesemanation]

Actually, nevermind, I thought through what you're actually suggesting, and I'll try implementing AppendAll and such directly. Got stuck in a mindset of specific solution, will try yours instead, will give an update probably this week

[diwic]

Right, so there is already code for generating some or maybe all of this in the dbus-codegen crate, just that it starts with an xml file instead of Rust types. I've not written any proc-macro crates so I'm not that familiar with syn, quote and friends, but any chance we could share code between the two code generation methods?

[eaglesemanation]

I'm almost done with a rewrite that uses your traits instead of custom ones. I don't know how much code can be reused from your codegen crate, because it seems to be mostly relying on String templating, while mine is first parsed from TokenStream into AST and then back. Making some sort of adapters between those 2 interfaces might be more code than proc-macro itself, but I'll let you judge that when my rewrite is done.

[eaglesemanation]

I'm done with the rewrite, and except 1 small bug annotated with //FIXME, my poor man's version of randr works as it did before. derive crate is almost entirely rewritten, I tried to replicate how this library uses macro_rules! for implementing traits on tuples, a lot of it is just copy-paste with quote! syntax adjustments. Hope it makes it more readable for you.

[diwic]

Hi! I've been looking at your crate and it looks better now :-)

I think I understand most of it, it might need a bit more of tests/documentation/readme so users understand how to use it. Also the "rename" option of DbusPropMap seems to be untested and undocumented.

I have a question about DbusArgs - it seems to assume the args is in a struct so it should probably be named DbusStruct instead? A version that does not descend into a struct might be even more useful - you could e g use it with Message::read_all or Message::append_all. Not saying you have to write one, but that is what I would expect a DbusArgs to do.

[eaglesemanation]

Doc test

Right now I'm working on documentation, wanted to add an example and got a really weird issue - linking libsystemd.so.0 fails while compiling doc test. Here is exact error:

running 1 test
test dbus-derive/src/lib.rs - derive_dbus_propmap (line 84) ... FAILED

failures:

---- dbus-derive/src/lib.rs - derive_dbus_propmap (line 84) stdout ----
Test executable failed (exit status: 127).

stderr:
/tmp/rustdoctestBfG30s/rust_out: error while loading shared libraries: libsystemd.so.0: cannot open shared object file: No such file or directory



failures:
    dbus-derive/src/lib.rs - derive_dbus_propmap (line 84)

Considering that my unit tests work, I think I don't have any issues with having library on my system. For now I'll just skip testing examples.

Flatten

Another observation - I wanted to add a feature to flatten a struct that implements ArgAll into another struct. So for example:

#[derive(DbusArgs)]
struct Flattened {
	x: i32,
	y: i32,
}

#[derive(DbusStruct)]
struct Wrapper {
	#[dbus_struct(flatten)]
	flat: Flattened
}

assert_eq!(Wrapper::signature(), "(ii)")

The issue is that I cannot really define type strs in ArgAll, because I cannot figure out the amount of fields inside Flattened struct at the moment of macro expansion. Assuming that we could break compatibility - is there reason why type strs needs to exist? Couldn't this work:

pub trait ArgAll {
-   /// A tuple of &static str. Used for introspection.
-   #[allow(non_camel_case_types)] // Note: This should be changed for 0.9 - but for now, don't break backwards compatibility
-   type strs;
    /// Enumerates all arguments with their signatures (introspection helper method).
-   fn strs_sig<F: FnMut(&'static str, Signature<'static>)>(a: Self::strs, f: F);
+   fn strs_sig<F: FnMut(&'static str, Signature<'static>)>(a: &[&'static str>], f: F);
}

[eaglesemanation]

I've added documentation for each derive macro, if it looks good to you - I'll start working on a PR for this repo

[diwic]

Cool!

I'm not sure about the question about removing type strs. Maybe start upstreaming what you've got so far, then if you like you could try removing it (and work on the flatten feature) and see if you run into trouble somewhere?

We've had 0.9 for a long time so I'm open to releasing something backwards incompatible, especially as this seems like an easy change to adapt to for users.

[diwic]

As for the doc test question, I do want cargo test to work before merging. Not sure if the problem is with your environment or with the example, we'll just have to see I guess.

[eaglesemanation]

Seems like linkage issue with rustdoc was just recently resolved: rust-lang/cargo#13490
That issue might be unique to my setup because I'm on Fedora with Nix for dev dependencies, so dynamic linkage is a bit of a mess. Running tests with --features vendored worked perfectly fine.
Made a draft PR here: #459.
Want to make DbusEnum more general purpose, not just enum specific, so maybe DbusType, or DbusInto. That might be useful for some wrapper types, or maybe tightly packed values. After that will move it out of draft.