rcgen icon indicating copy to clipboard operation
rcgen copied to clipboard

Eagerly implemented `Debug` trait

Open Rynibami opened this issue 7 months ago • 6 comments

Issue #340 discussed implementing the most common traits for this crate. I implemented the Debug trait for all possible stucts and enums in this PR.

I also came across the CertifiedKey and Issuer types that didn't derive the Clone trait, but are able to. Should this be addressed as well, or should we keep it that way?

Rynibami avatar May 16 '25 16:05 Rynibami

I also came across the CertifiedKey and Issuer types that didn't derive the Clone trait, but are able to. Should this be addressed as well, or should we keep it that way?

For the same reason I flagged these as not being appropriate for Debug they aren't appropriate for Clone and should be left as-is.

cpu avatar May 16 '25 16:05 cpu

I'll update them and push the next commit online :)

Rynibami avatar May 16 '25 16:05 Rynibami

I think instead of removing the Debug impl, it would be better to manually implement it. that way, we don't add it back by accident. can be a followup though.

I can remove the implementations and leave it at that... but that just doesn't sit right to leave it at that. Maybe it is a great idea to create a proc macro crate that implements derive macros. Then we could derive a similar macro that actually implements the basic traits in a certain manner. For example:

#[derive(DebugLimited)]
struct Issuer<'a, S> {
    distinguished_name: &'a DistinguishedName,
    key_identifier_method: &'a KeyIdMethod,
    key_usages: &'a [KeyUsagePurpose],
    #[debug_remove] // or #[debug_redact] to show the field but not the value
    key_pair: &'a S,
}

This would (in this case) implement the Debug trait normally as derive trait, but would change the actual implementation where *_remove would remove the whole field and *_redact replaces the field value with *** REDACTED *** (or something similar). This pattern can the also be used to extend other limited derive implementations.

Rynibami avatar May 17 '25 16:05 Rynibami

The costs of adding yet another derive macro are substantial. You could just do a manual Debug implementation instead, it's fine.

djc avatar May 17 '25 16:05 djc

The costs of adding yet another derive macro are substantial. You could just do a manual Debug implementation instead, it's fine.

In that case I would like to know what fields / types you would like me to redact / remove.

Rynibami avatar May 17 '25 16:05 Rynibami

Only remove key_pair please. It's not hard to manually implement Debug, see the second example in the libstd rustdocs. Maybe do a destructuring exhaustive pattern match of the struct, i.e. let Issuer { field_a, field_b, field_c: _ } = ..., so that if a field gets added they must also update that function (and add it to the debug impl). plus a comment explaining the choice.

est31 avatar May 17 '25 19:05 est31