digest: add optional impl: section to buffer_ct_variable! - #2477
Open
MavenRain wants to merge 2 commits into
Open
digest: add optional impl: section to buffer_ct_variable!#2477MavenRain wants to merge 2 commits into
impl: section to buffer_ct_variable!#2477MavenRain wants to merge 2 commits into
Conversation
`buffer_fixed!` and `buffer_xof!` both accept an `impl:` trait list and both already have a `CustomizedInit` arm, but `buffer_ct_variable!` has no such section. Downstream crates whose wrapper is generated by it therefore have to hand-write the impl, which is what blake2 does today behind two `// TODO: impl in the macro` comments (RustCrypto/hashes#747). Add an optional trailing `impl: ...;` section served by two new public arms which forward to an `impl_inner:` muncher, mirroring how `buffer_fixed!` already works. `CustomizedInit` is the only trait in the vocabulary for now; it requires the "core" type to implement `VariableOutputCoreCustomized`. The section is optional and the new arms are additive, so both pre-existing call forms keep matching byte for byte. Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
Signed-off-by: Onyeka Obi <softwareengineerasaservant@isurvivable.cv>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an optional trailing
impl: ...;section tobuffer_ct_variable!, withCustomizedInitas its first (currently only) supported trait.This is the
digesthalf of RustCrypto/hashes#747. The blake2 half is a small follow-up, described at the bottom.Problem
buffer_fixed!andbuffer_xof!both take animpl:trait list and both already carry aCustomizedInitarm.buffer_ct_variable!takes no such section, so a crate whose wrapper it generates has to hand-write the impl. blake2 does exactly that, twice, each time under a// TODO: impl in the macrocomment:Fix
Two new public arms accept the extra section and forward to an
impl_inner:muncher, in the same shapebuffer_fixed!uses (a per-trait arm that peels one ident and tail-recurses, plus a terminator). The generated impl carries the same$out_size: ArraySize + IsLessOrEqual<$max_size, Output = True>bound as every other impl the macro emits.Both pre-existing call forms are unaffected: the section is optional and the arms are purely additive, so existing callers match byte for byte.
groestlandkupyna, the only otherbuffer_ct_variable!callers I could find, compile unchanged against this branch.One deliberate difference from blake2's hand-written impls: the generated
new_customizedis#[inline], matchingfixed.rsandxof.rs.Testing
New
digest/tests/dummy_ct_variable.rs, modelled on the existingdummy_fixed.rs. It defines dummy variable-output cores (one customizable, one not), covers all four section combinations (exclude: SerializableState;present or absent, crossed withimpl:present or absent), asserts absolute output values, and checks that the new section composes with the delegatingmax_size:-only arm through aSerializableStateround trip.Gates, all green:
cargo build/test -p digest --all-features,cargo fmt --check,cargo clippy --all-features -- -D warnings,cargo doc --all-features --no-deps, andcargo +1.85.0 check -p digest --all-features --testsfor MSRV.Confirmed by mutation rather than by assertion count:
new_customizeddrop its argument fails both new testsimpl: CustomizedInit;from a call site makesnew_customizeddisappear with E0599, so the opt-in is real and nothing is emitted implicitlyEnd to end against real blake2, with a local
[patch.crates-io]pointingdigestat this branch: deleting both hand-written impls and passingimpl: CustomizedInit;keeps the existingblake2b_personaKAT byte-identical, withpersona.rsunchanged between the two runs. Keeping the hand-written impl and the section gives E0119 naming this macro, which is the positive proof that the section really emits an impl for the concreteBlake2b<OutSize>rather than silently expanding to nothing.Questions for the maintainer
CustomizedInitorTryCustomizedInit? Given digest: addTryCustomizedInittrait #2395 and ascon-xof128: replace implementation ofCustomizedInitwithTryCustomizedInithashes#859 you may prefer the fallible trait here. I emitted the infallible one because the blanketimpl<T: CustomizedInit> TryCustomizedInit for Tmakes emitting both a coherence error, and because there is no fallible counterpart toVariableOutputCoreCustomizedforCtOutWrapperto delegate to. Happy to switch, or to add a secondTryCustomizedInitident to the vocabulary, if you'd rather add that core trait first.ZeroizeOnDropfor the output ofbuffer_ct_variable#2353. That PR addsZeroizeOnDropto this same macro and appends in the same tail region. If it lands first,ZeroizeOnDropwould be a natural second entry in thisimpl:vocabulary instead of a standalone impl. I'm happy to rebase in either order, or to rework this on top of it.Follow-up
RustCrypto/hashes#747 then becomes: pass
impl: CustomizedInit;to the twodigest::buffer_ct_variable!invocations, delete the two hand-written impls (26 lines), and drop the now-unusedCustomizedInitimport. That change is ready and verified locally; I'll open it once this lands and is released, since it needsdigest = "0.11.4".Drafted with AI assistance; the design, the verification above, and this description were reviewed and re-run by me.