Skip to content

digest: add buffering macros #1799

New issue

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

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

Already on GitHub? # to your account

Merged
merged 35 commits into from
May 26, 2025
Merged

digest: add buffering macros #1799

merged 35 commits into from
May 26, 2025

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Mar 19, 2025

The new macros create buffered wrapper types and replace the old wrapper types (except CtVariableCoreWrapper).

Closes #1069

@baloo
Copy link
Member

baloo commented Mar 19, 2025

I'm a bit worried we have to list all the traits in the invocation of the macro and that consumers will drift inconsistent over time.

We can't centrally add/implement a trait (like we could with the CoreWrapper or my set of macros (#1775)).

@newpavlov
Copy link
Member Author

newpavlov commented Mar 19, 2025

What do you mean by "drift"? The list of traits should not change across semver-compatible releases of digest.

To make the list of traits less annoying, I plan to add several templates for the most common cases. In other words, instead of:

newtype!(
    /// SHA-256 hash
    Sha256(..);
    delegate:
        Debug AlgorithmName
        Clone Default Reset
        BlockSizeUser OutputSizeUser HashMarker
        Update FixedOutput FixedOutputReset
);

We will write:

newtype!(
    /// SHA-256 hash
    Sha256(..);
    delegate_template: FixedOutputHash
);

@baloo
Copy link
Member

baloo commented Mar 19, 2025

We've added traits in the previous cycle: #1098 (released in digest 0.10.4)

We may continue to do so in the next one. Having to list the traits means we have to each consumers and bump the list everywhere and release them.
It's less of a problem with a template though.

newpavlov added a commit that referenced this pull request Apr 7, 2025
Together with #1799 users would
need to implement `AssociatedOid` manually for created newtypes.

Also replaces `feature = "const-oid"` with `feature = "oid"`.
@tarcieri
Copy link
Member

tarcieri commented May 1, 2025

@newpavlov can we get this wrapped up?

@newpavlov
Copy link
Member Author

@tarcieri
I plan to do it in the near future. Unfortunately, I failed to make the macro generic enough, so I probably will just introduce separate macros for fixed, variable, and XOF hashes. Less conventional hashes (e.g. Skein) would have to either rely on type aliases, or introduce their own newtype macros.

@newpavlov newpavlov changed the title digest: add newtype! macro digest: add newtype macros May 4, 2025
@tarcieri
Copy link
Member

It would be good to either do that or get this merged ASAP

@newpavlov
Copy link
Member Author

I will try to finalize the remaining parts during this weekend. If I will not be able to do it for some reason, we can revert #1810.

@newpavlov newpavlov changed the title digest: add newtype macros digest: add buffering macros May 19, 2025
@newpavlov newpavlov marked this pull request as ready for review May 19, 2025 12:03
@newpavlov newpavlov requested a review from tarcieri May 19, 2025 12:04
macro_rules! buffer_fixed {
(
$(#[$attr:meta])*
$v:vis struct $name:ident$(<$gp:ident: $bound:ident>)?($core_ty:ty);
Copy link
Member Author

Choose a reason for hiding this comment

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

We could simplify the macro by removing the $(<$gp:ident: $bound:ident>)? part. Currently it's used only by gost94 and skein.

My initial intent was for this macro to support cases covered by the buffer_ct_variable macro, but, unfortunately, the required pattern becomes annoyingly complex, so I decided against it.

Copy link
Member

@baloo baloo left a comment

Choose a reason for hiding this comment

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

I've been upgrading downstream consumers of this crate (using this branch). I haven't found any usability issues.

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

I'm a bit confused by these macros calling themselves recursively. It makes the logic somewhat hard to follow. I guess that's to avoid having separate non-reusable macros for writing the various trait impls which would need to be part of the public API?

In general I'm eager to see this landed so I guess I won't hold it up, but I did want to note I found the macro expansions a bit hard to follow due to the large number of macro match arms.

@newpavlov
Copy link
Member Author

newpavlov commented May 22, 2025

@tarcieri
It's an unfortunate workaround for zipping behavior of declarative macros. We can't naively write the following code:

$(
    $crate::buffer_fixed!(
        impl_inner:
        $name$(<$gp: $bound>)?($core_ty);
        $trait_name
    );
)

Because it expects for $gp/$bound and $trait_name to repeat the same number of times (i.e. we get this error). As I wrote above, we could simplify it by removing $gp, but we need it not only for the two hash crates, but also for most of the MAC crates as well.

Maybe someone more familiar with declarative macros could write a better macro, but right now it's the best I could do.

@baloo
Copy link
Member

baloo commented May 24, 2025

anything else blocking here?

@baloo
Copy link
Member

baloo commented May 25, 2025

About MACs, I believe you mentioned you wanted to introduce a similar macro. Could this be done in a followup? Just so we can get back to a usable master.

🤷

tarcieri pushed a commit that referenced this pull request May 25, 2025
)

This reverts commit c48ea2a (#1810)

This bring back `master` in a state that can be consumed with a
`[patch.crates-io]` (see
#1799 (comment))
@newpavlov newpavlov merged commit d7807bf into master May 26, 2025
10 checks passed
@newpavlov newpavlov deleted the digest/newtype branch May 26, 2025 02:10
tarcieri pushed a commit to RustCrypto/password-hashes that referenced this pull request May 27, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

digest: type name readability regression
3 participants