MultiIndexMap should derive Debug, when inner type derives Debug

[luxbe]

When the struct, where the derive macro #[derive(MultiIndexMap) is added, also derives the Debug trait, the generated MultiIndexMap should also derive the Debug trait.

As far as I can tell, this is currently not possible, but could be when specialization is stabilized:
dtolnay/syn#77 (comment)

But maybe you have another idea how to implement this feature already!

[lun3x]

Hey! Yeah indeed I do want to implement this but currently it only seems possible with nightly and the specialization feature. It could be possible to add an optional feature that works on nightly, I'll have a look at how much work this would be

[luxbe]

Sounds great! Let me know, if there is anything I can help you with!

[lun3x]

I had a go at an initial implementation on https://github.com/lun3x/multi_index_map/tree/trivial_bounds
Seems like we just need the trivial_bounds unstable feature rather than the specialization feature. However I cannot seem to work out the correct incantation to get both #![feature(trivial_bounds)] and the Debug impl to be compiled.
You can test by putting #![feature(trivial_bounds)] in any test module, then running tests on nightly with cargo test --features="experimental".
I think this should work with a bit of fiddling with it

[luxbe]

One thing you were probably missing was adding the debug guards for the other fields of the original elements.
I imlemented an initial version in #33. Both cargo test and cargo +nightly test --features="experimental" work as expected.

Let me know, if I missed something!

[eirenik0]

It would be great to have the same to Serialize\Deserialize if possible.

[luxbe]

It would be great to have the same to Serialize\Deserialize if possible.

That's an interesting idea! When #33 is merged, I will look into this if that's alright with @lun3x.
Maybe a second feature e.g. serde would be a good option to implement this change

[lun3x]

Yeah absolutely agree, I've merged the trivial_bounds feature to master. Now we can add some more derived impls that rely on this, Clone would be an easy one to start with.

Serialize/Deserialize is also possible, although I suppose this would add a requirement on serde, so I guess then we need a new feature, we can just call it serde and have it both add a requirement on serde, and enable trivial_bounds.

[luxbe]

Serialize/Deserialize is also possible, although I suppose this would add a requirement on serde, so I guess then we need a new feature, we can just call it serde and have it both add a requirement on serde, and enable trivial_bounds.

This is what I was thinking. I will hopefully be able to work on this next week if that is alright with you!

[luxbe]

@lun3x I wanted to discuss following problem with you:

Currently we use the trivial_bounds feature to manually derive e.g. Debug, Clone, etc. However, this is getting quite complicated and verbose in the codebase - it would be nicer if we could append #[derive(Debug, Clone)] to the type itself. That way we don't have to manually implement the methods and are more resistant to breaking changes in case the implementation details of the trait impl changes.

Adding #[derive(...)] to types is apparently possible with proc_macro_attributes - would it make sense to switch over to a proc_macro_attribute? In combination with the impls crate, this could be a viable alternative to our current approach.

[lun3x]

Yeah I agree, it is quite difficult to reason about exactly how these traits get added right now. I think it might be possible to refactor things a bit to move this section of the generation into a separate module, but if we can just use Derive and Clone that would be much nicer.

The impls crate looks nice, although on first glance it seems like it would be difficult to use the generated map_name inside this macro?

So you mean instead of a Derive macro, potentially we could use a raw proc_macro_attribute? Yeah I'm not opposed to that, obviously it would be a breaking change, but if we keep the interface similar it shouldn't be a large code change for end users.

[luxbe]

So I experimented a bit. It is possible to use e.g. #[derive(Debug)] in the macros, the biggest problem is detecting if a struct has implemented a trait during the macro execution. As far as I can tell, this is not possible.

So the way I see it is, we could continue to implement all traits manually within the macro - that means writing out debug, clone, serialize, etc. which is more difficult to maintain, or we could introduce something like the following syntax:

#[derive(MultiIndexMap, Debug)]
#[multi_index_map(debug)]
pub(crate) struct Order {
    #[multi_index(hashed_unique)]
    pub(crate) order_id: u32,
}

Here the new attribute #[multi_index_map(debug)] would tell us to add #[derive(Debug)] to the generated struct, so we don't have to implement Debug manually. However, you would have to write the keyword debug multiple times here - I dislike that a lot.

Do you have any other ideas on how to proceed? While the current solution is not a problem for Clone and Debug, implementing serdes (de-)serialize methods gets ugly quite quickly...

[lun3x]

Hey so I've had a bit of a look around the rust ecosystem to try to find out what the accepted approach is here.

soa_derive uses something similar, also with support for cfg_attr:

#[derive(Debug, PartialEq, StructOfArray)]
#[soa_derive(Debug, PartialEq)]
pub struct Cheese {
  ...
}

#[derive(Debug, PartialEq, StructOfArray)]
#[soa_attr(Vec, cfg_attr(test, derive(PartialEq)))]
pub struct Cheese {
  ...
}

rkyv also uses roughly this pattern:

#[derive(Archive, Deserialize, Serialize, Debug, PartialEq)]
#[archive(
    // This will generate a PartialEq impl between our unarchived and archived
    // types:
    compare(PartialEq),
    // bytecheck can be used to validate your data if you want. To use the safe
    // API, you have to derive CheckBytes for the archived type:
    check_bytes,
)]
// Derives can be passed through to the generated type:
#[archive_attr(derive(Debug))]
struct Test {
  ...
}

I think this is fine, obviously a bit verbose, but that seems pretty standard. I would suggest we go with the following syntax, and potentially later add support for cfg_attr:

#[derive(MultiIndexMap, Debug)]
#[multi_index_derive(Debug)]
struct Order {
    #[multi_index(hashed_unique)]
    order_id: u32,
}

[luxbe]

Awesome! Looks good to me.
I would be interested to implement this, however I can get to it next week by the earliest. I was quite busy the past few days. Is that alright with you or did you already work on this problem? @lun3x

[lun3x]

Awesome no worries, I've not worked on this yet so that would be great, thanks a lot!

[RuofengX]

serde derive for generated struct, both Serialize and Deserialize, still not work

[luxbe]

Thank you so much for your work @lun3x!
Sorry I didn't come around to this earlier, but your implementation looks great ❤️