Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
cbeck88 opened this issue May 23, 2020 · 14 comments
Closed

Comments

@cbeck88
Copy link
Contributor

cbeck88 commented May 23, 2020

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 cbeck88 changed the title Make a way so that prost-derive doesn't derive Debug Make a way so that prost-derive doesn't necessarily derive Debug for you May 23, 2020
@eranrund
Copy link

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.

@danburkert
Copy link
Collaborator

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.

@TheVova
Copy link

TheVova commented Sep 8, 2020

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.

@cbeck88
Copy link
Contributor Author

cbeck88 commented Sep 8, 2020

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

@TheVova
Copy link

TheVova commented Sep 9, 2020

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?

@cbeck88
Copy link
Contributor Author

cbeck88 commented Sep 9, 2020

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)

@TheVova
Copy link

TheVova commented Sep 10, 2020

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 :)

@Sushisource
Copy link

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.

@cortopy
Copy link

cortopy commented Sep 14, 2021

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

@cbeck88
Copy link
Contributor Author

cbeck88 commented Sep 14, 2021

@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?

@cortopy
Copy link

cortopy commented Sep 14, 2021

@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

@gen-xu
Copy link

gen-xu commented May 8, 2023

+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.

@barafael
Copy link

barafael commented Nov 5, 2024

+1 from me. Sometimes data is secret. In that case one wants to either override debug/display or completely avoid it.

@caspermeijn
Copy link
Collaborator

If this comment helps you, please consider updating the crate documentation to make these features more findable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants