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

async-signature: remove AsyncDigestSigner blanket implementation #1584

Conversation

baloo
Copy link
Member

@baloo baloo commented Jun 2, 2024

This introduces a basic check to make sure our blanket implementation would not block manual implementation of the traits in this crate.

Before this commit AsyncDigestSigner was causing a conflict:

  --> async-signature/tests/mock_impl.rs:14:1
   |
14 | / impl<D> async_signature::AsyncDigestSigner<D, Signature> for MockSigner
15 | | where
16 | |     D: async_signature::Digest,
   | |_______________________________^
   |
   = note: conflicting implementation in crate `async_signature`:
           - impl<D, S, T> AsyncDigestSigner<D, S> for T
             where D: Digest, T: DigestSigner<D, S>;
   = note: downstream crates may implement trait `async_signature::signature::DigestSigner<_, Signature>` for type `MockSigner`

@baloo baloo force-pushed the baloo/async-signature/remove-asyncdigest-signer branch from f8311d7 to 57ca7d3 Compare June 2, 2024 04:19
This introduces a basic check to make sure our blanket implementation
would not block manual implementation of the traits in this crate.

Before this commit `AsyncDigestSigner` was causing a conflict:
```
  --> async-signature/tests/mock_impl.rs:14:1
   |
14 | / impl<D> async_signature::AsyncDigestSigner<D, Signature> for MockSigner
15 | | where
16 | |     D: async_signature::Digest,
   | |_______________________________^
   |
   = note: conflicting implementation in crate `async_signature`:
           - impl<D, S, T> AsyncDigestSigner<D, S> for T
             where D: Digest, T: DigestSigner<D, S>;
   = note: downstream crates may implement trait `async_signature::signature::DigestSigner<_, Signature>` for type `MockSigner`
```
@baloo baloo force-pushed the baloo/async-signature/remove-asyncdigest-signer branch from 57ca7d3 to 6d812ea Compare June 2, 2024 20:41
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.

Hmm, that's unfortunate. I wonder if we should reconsider the other blanket impls as well.

@tarcieri
Copy link
Member

tarcieri commented Jun 3, 2024

@baloo perhaps we should bump the version number as well? I think this is a technically a breaking change in that someone could previously use a DigestSigner as an AsyncDigestSigner

@tarcieri tarcieri merged commit e5da021 into RustCrypto:master Jun 3, 2024
10 checks passed
@baloo baloo deleted the baloo/async-signature/remove-asyncdigest-signer branch June 3, 2024 17:47
@baloo
Copy link
Member Author

baloo commented Jun 3, 2024

I think other blankets were fine (I believe I tested them here, but they might still cause issues, I don't know).

This is a breaking change. I'll submit a follow-up PR to bump it.
I think the only consumers for now are in formats.git and those are not tagged yet (so easy to bump)

@baloo
Copy link
Member Author

baloo commented Jun 3, 2024

Actually, the async-signature is currently sitting at 0.6.0-pre.1, I think I'm alright with such an API break in the next -pre or -rc release.

@tarcieri
Copy link
Member

tarcieri commented Jun 3, 2024

Aah right! Seems good then

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.

2 participants