prost icon indicating copy to clipboard operation
prost copied to clipboard

Make a way so that prost-derive doesn't necessarily derive Debug for you

Open cbeck88 opened this issue 4 years ago • 12 comments

Sometimes you have structs that you want to give a custom implementation of Debug, but if you do that then they cannot derive Message which is unfortunate.

Would you take a patch that allows something like the following?

#[derive(Eq, PartialEq, Hash, Message)]
#[prost(NoDebug)]
pub struct MyStruct { ... };

impl core::fmt::Debug for MyStruct {
    ...
}

cbeck88 avatar May 23 '20 01:05 cbeck88

Alternatively, I think maybe we can have Message: Debug, or maybe not have that requirement at all? Either way this has my vote! It would be super helpful to not be forced to use the default derive(Debug) since that limits you to only structs whose members all implement Debug, and as @garbageslam mentioned it makes it impossible to use Prost with structs that implements a custom Debug.

eranrund avatar May 25 '20 06:05 eranrund

Yeah I think ideally we would not include the Debug derive for #[derive(Message)], and instead have something like #[derive(prost::Debug)] to add it, and have the code generator start outputting that. Another reason to do that is people get confused and think there isn't a Debug impl for prost generated types.

danburkert avatar Jul 18 '20 21:07 danburkert

im interested in this as well. removing Debug from Message: Debug + Send + Sync can the code a lot smaller for embedded use (where i dont want debug or fmt at all in certain places). Dont know if the default should be to generate Debug or not. generating the impl by default matches the current behaviour, at least. i can give it a shot, unless @garbageslam is already on it.

TheVova avatar Sep 08 '20 12:09 TheVova

i haven't done anything on this, feel free to give it a shot :smile:

cbeck88 avatar Sep 08 '20 16:09 cbeck88

cool. i'll fix this then. do we want it via codegen option in prost build so it can be set via prost_build::Config or as a feature in prost-derive?

TheVova avatar Sep 09 '20 11:09 TheVova

I would suggest that, it doesn't make sense for it to be an option in prost_build::Config, because prost_build is only related to the case that prost is making struct definitions from proto files. It merely uses the proc-macro offered by prost-derive.

Instead, this feature should be part of prost-derive config (affecting the code created by the proc-macro), and either do it like (1) make a proc macro attribute that affects derive(Message) and enables the derivation of Debug of as well, so like

#[derive(Message)]
#[prost(Debug)]
struct Foo {
    #[prost(fixed64, tag = 1)]
    a: i64,
    #[prost(fixed64, tag = 2)]
    b: i64,
}

To do this, you would look in the DeriveInput object inside the prost-derive crate, and try to find the prost(Debug) attribute among the attributes. See syn crate documentation here: https://docs.rs/syn/1.0.40/syn/struct.DeriveInput.html

OR do exactly what @danburkert suggested above, and just have a completely separate derive macro for the prost::Debug thing.

#[derive(Message)]
#[derive(prost::Debug)]
struct Foo {
    #[prost(fixed64, tag = 1)]
    a: i64,
    #[prost(fixed64, tag = 2)]
    b: i64,
}

Then you would have basically two proc-macros being offered by prost-derive. That's probably simpler unless for some reason rustc won't let you make a custom derive for Debug like this (I don't know, I never tried)

cbeck88 avatar Sep 09 '20 18:09 cbeck88

I mainly meant the ergonomics of it for end users. if i wanna setup my codegen in my build.rs as is usually the case, i wanna be able to specify whether or not to derive debugs, so i still think prost-build needs to add that option in config.

as for implementation, the problem with prosts' Debug is that its tied internally to prosts' Field type, as each field has a debug method that does some wrapping, etc. Currently the building of debug is bundled with the derive for Message, where it reuses that data it generates for message for debug. Separating it out will cause a bit of code duplication as i'd still need to generate Fields again. i think the easiest solution is actually what you mentioned first, as in, add an attribute and check it for the Derive input. that'd allow me to not change the code in prost-derive except for that check. i'll go with that, make a PR, and we can discuss it then :)

TheVova avatar Sep 10 '20 08:09 TheVova

Big +1 to this - just ran into this being a problem where I'd like to customize my Debug impls to provide better output to the tracing crate and without the ability to override them you end up needing to do some ugly workarounds like making your own versions of Debug and implementing it for a bunch of stuff.

Sushisource avatar Mar 04 '21 21:03 Sushisource

Also needing this except for Default, as I'd like to set the default values myself. It would be great.

Instead of creating new configuration options for each derive argument, Config.exclude_type_attribute would be even better and very close to the existing Config.type_attribute

cortopy avatar Sep 14 '21 16:09 cortopy

@cortopy one issue is, in proto3 they dropped support for custom default fields, so if prost allows to have custom defaults it's somewhat a hazard for proto3 users. But maybe it's okay and helpful to proto2 users?

cbeck88 avatar Sep 14 '21 17:09 cbeck88

@garbageslam totally right and that's why sometimes I need to implement Default trait myself. I didn't mean the implementation of default values but the trait itself. All types seem to get the Default trait derived and AFAIK there's no way to disable that

cortopy avatar Sep 14 '21 17:09 cortopy

+1 on this, and any status update on this? it looks like there was a PR open from 2+ years ago? sometimes if I want to pretty print something, default implementation of Debug makes it bit hard, especially for nested messages.

gen-xu avatar May 08 '23 00:05 gen-xu