Skip to content

Eagerly implemented Debug trait #343

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rynibami
Copy link

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?

@cpu
Copy link
Member

cpu commented May 16, 2025

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.

@Rynibami
Copy link
Author

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

@Rynibami Rynibami force-pushed the eager-impl-debug branch from 26cede4 to 8c327f4 Compare May 16, 2025 16:12
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems okay to me.

I'd prefer to avoid implementing Clone on types that include a private key.

Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug impl of CertifiedKey

@Rynibami Rynibami force-pushed the eager-impl-debug branch from 3982c1a to 7d6bd16 Compare May 17, 2025 08:44
@Rynibami Rynibami requested a review from est31 May 17, 2025 08:47
Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Rynibami
Copy link
Author

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.

@djc
Copy link
Member

djc commented May 17, 2025

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

@Rynibami
Copy link
Author

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.

@est31
Copy link
Member

est31 commented May 17, 2025

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.

@Rynibami Rynibami force-pushed the eager-impl-debug branch from 7d6bd16 to b48caf8 Compare May 17, 2025 21:35
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

Successfully merging this pull request may close these issues.

4 participants