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

Add blanket trait impls for references #210

Merged
merged 6 commits into from
Jul 10, 2020
Merged

Add blanket trait impls for references #210

merged 6 commits into from
Jul 10, 2020

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Jul 7, 2020

Closes RustCrypto/MACs#47

IIUC strictly speaking these changes are not backwards compatible, since downstream crates may define their own trait implementations for references. I wonder if we can overlook it and release these changes under patch releases.

Unfortunately it's not possible to write impl<Alg: BlockCipherMut> BlockCipherMut for &mut Alg { .. }, since it potentially conflicts with impl<Alg: BlockCipher> BlockCipherMut for Alg { .. }. Same goes for StreamCipher, which conflicts with the SyncStreamCipher impl. I think it's impossible to solve this problem without negative trait bounds, which are not even on horizon. Specialization may help, but I failed to make it work.

@newpavlov newpavlov requested a review from tarcieri July 7, 2020 07:01
@burdges
Copy link

burdges commented Jul 7, 2020

I advocated for rand doing this way back, which I still support since it makes traits more ergonomic, but it brings some complexity too, and more breakage than expected. I'd be cautious here since the symmetric crypto traits are more complex than rand's traits. Asymmetric crypto traits become more complex still, making this unlikely for them, but they'd benefit far more than symmetric crypto.

Ideally, rust would fix E0666 so that impl Borrow<impl Trait> worked, and provide enough lazy normalization so that associated type bounds work correctly: rust-lang/rfcs#2289 (comment)

cc @hdevalence

@newpavlov
Copy link
Member Author

@burdges
An analogous blanket impl for RngCore does exist in the latest rand_core version and AFAIK works without issues. Can you remind issues to be aware of?

@burdges
Copy link

burdges commented Jul 7, 2020

@newpavlov
Copy link
Member Author

Ah, indeed. Then to be safe we should do a breaking release after all.

@newpavlov
Copy link
Member Author

@tarcieri
If you don't have objections, I'll go ahead with merging this PR and published the new versions of block-cipher and stream-cipher.

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.

Sounds good to me!

@newpavlov newpavlov merged commit 54ded88 into master Jul 10, 2020
@newpavlov newpavlov deleted the block_blank branch July 10, 2020 14:59
dns2utf8 pushed a commit to dns2utf8/traits that referenced this pull request Jan 24, 2023
Updates `argon2`, `pbkdf2`, and `scrypt` with the following upstream
changes from the unreleased `password-hash` crate:

- Add `version` param to `PasswordHasher` (RustCrypto#719)
- Refactor `PasswordHasher` (RustCrypto#720)
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.

Support non-cloneable BlockCiphers
3 participants