-
Notifications
You must be signed in to change notification settings - Fork 207
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
digest: newtype support #1775
base: master
Are you sure you want to change the base?
digest: newtype support #1775
Conversation
// https://github.com/taiki-e/pin-project/issues/102#issuecomment-540472282 | ||
#[doc(hidden)] | ||
#[derive(Clone, Default)] | ||
pub struct Wrapper<'a, T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I still don't quite understand the relevance of this wrapper. Why can't the macro implement the struct as a direct thin wrapper around CoreCtWrapper
and other wrapper types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An isolated example is probably easier to showcase the purpose of the wrapper
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=2c559bf521cd586849fb5ffc8ba5850f
But this allows me to implement trait for the newtype only if the inner carries an implementation for it.
This is only meant as a hack until https://doc.rust-lang.org/beta/unstable-book/language-features/trivial-bounds.html lands.
The wrapper was a workaround found in this comment: taiki-e/pin-project#102 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to deal with lifetimes here?
This snippet works just fine: https://play.rust-lang.org/?gist=9f86f1ec8c12cbb176cb8e1cce852e56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this doesn't work anymore: https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=0c16e92508e7903da2d6099d956f57dd
This is what the lifetime gives me: a bound on the trait implementation of the wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand. Why do you expect for wrapper!(String => Email);
to work in you snippet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the inner type may or may not implement Foo, and I want the wrapper to implement Foo only if the inner implements it.
This is what the lifetime hack gives me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. It's certainly a neat trick, but it's a bit too much magic for my taste. The macro itself already obscures the implementation a bit, so I would prefer to have something more straightforward.
We can either have macros for common scenarious (e.g. impl_fixed_digest!
) or explicitly list traits which we want to implement by delegation to the wrapped type. I am not sure which solution would be better, so I will need to experiment and think about it a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright well I'll pause a bit then :)
64e171f
to
54d2702
Compare
use super::Wrapper; | ||
use crate::const_oid::{AssociatedOid, ObjectIdentifier}; | ||
|
||
impl<T> AssociatedOid for Wrapper<'_, T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be nice to get rid of all of the current complexity we have around this trait (which is really just a hack around the fact that the digest crates couldn't define their own types, i.e. that we didn't have newtypes before) and just simply impl AssociatedOid
for the newtype. Then the newtype
macro doesn't need to concern itself with OIDs at all, and indeed we could remove everything related to OIDs from the digest
crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha good point, I just used the same pattern mechanically.
54d2702
to
3f1b908
Compare
3f1b908
to
57efa65
Compare
This is an attempt at fixing #1069
example use:
RustCrypto/hashes#659