Skip to content

Revert "signature: replace signature_derive with blanket impls (#1827)" #1840

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

Merged
merged 2 commits into from
May 1, 2025

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented May 1, 2025

This reverts commit bf47748.

Per #1831 this change breaks inference when there is a single explicit impl of the Signer trait.

It also wasn't possible to add corresponding blanket impls to the Async* traits, e.g. AsyncSigner for AsyncDigestSigner, because of the existing blanket impl of AsyncSigner for Signer which we definitely want to preserve.

As a general rule of thumb, blanket impls only make sense if they work 100% of the time, which doesn't seem to be happening here.

Closes #1831

tarcieri added 2 commits May 1, 2025 11:17
…)"

This reverts commit bf47748.

Per #1831 this change breaks inference when there is a single explicit
impl of the `Signer` trait.

It also wasn't possible to add corresponding blanket impls to the
`Async*` traits, e.g. `AsyncSigner` for `AsyncDigestSigner`, because of
the existing blanket impl of `AsyncSigner` for `Signer` which we
definitely want to preserve.

As a general rule of thumb, blanket impls only make sense if they work
100% of the time, which doesn't seem to be happening here.

Closes #1831
@tarcieri tarcieri merged commit ac54439 into master May 1, 2025
81 checks passed
@tarcieri tarcieri deleted the signature/revert-blanket-impls branch May 1, 2025 17:24
@baloo
Copy link
Member

baloo commented May 5, 2025

Wait, which crates actually consumes the signature_derive again?

@tarcieri
Copy link
Member Author

tarcieri commented May 6, 2025

@baloo I think most of the crates avoid it to avoid pulling in the proc macro stack (in which case maybe it's not a great solution)

@baloo
Copy link
Member

baloo commented May 6, 2025

well where I'm trying to go is: maybe we can just pull the signature_derive altogether? I can't find a single consumer of it.

@tarcieri
Copy link
Member Author

tarcieri commented May 6, 2025

Sure

@baloo
Copy link
Member

baloo commented May 6, 2025

This is actually used in yubihsm.rs!

@tarcieri
Copy link
Member Author

tarcieri commented May 6, 2025

Yeah, that's probably the one thing that uses it

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.

signature v3: type inference regression
2 participants