-
Notifications
You must be signed in to change notification settings - Fork 231
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
Blanket implementation for DomainType #571
Conversation
proto-derive/src/lib.rs
Outdated
@@ -55,41 +55,9 @@ fn expand_domaintype(input: &syn::DeriveInput) -> TokenStream { | |||
} | |||
}; | |||
|
|||
// This is what all the hubbub is about. |
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.
Not entirely. I was also wondering if we need the whole proc_macro at all.
You can replace all these two lines that look like this
#[derive(DomainType)]
#[rawtype(RawPartSetHeader)]
with:
impl DomainType<MyRawType> for ActualType {}
.
As a consequence you can delete the whole macro code:
tendermint-rs/proto-derive/src/lib.rs
Lines 10 to 56 in e892067
#[proc_macro_derive(DomainType, attributes(rawtype))] | |
pub fn domaintype(input: TokenStream) -> TokenStream { | |
let input = syn::parse_macro_input!(input as syn::DeriveInput); | |
expand_domaintype(&input) | |
} | |
fn expand_domaintype(input: &syn::DeriveInput) -> TokenStream { | |
let ident = &input.ident; | |
// Todo: Make this function more robust and easier to read. | |
let rawtype_attributes = &input | |
.attrs | |
.iter() | |
.filter(|&attr| attr.path.is_ident("rawtype")) | |
.collect::<Vec<&syn::Attribute>>(); | |
if rawtype_attributes.len() != 1 { | |
return syn::Error::new( | |
rawtype_attributes.first().span(), | |
"exactly one #[rawtype(RawType)] expected", | |
) | |
.to_compile_error() | |
.into(); | |
} | |
let rawtype_tokens = rawtype_attributes[0] | |
.tokens | |
.clone() | |
.into_iter() | |
.collect::<Vec<quote::__private::TokenTree>>(); | |
if rawtype_tokens.len() != 1 { | |
return syn::Error::new(rawtype_attributes[0].span(), "#[rawtype(RawType)] expected") | |
.to_compile_error() | |
.into(); | |
} | |
let rawtype = match &rawtype_tokens[0] { | |
proc_macro2::TokenTree::Group(group) => group.stream(), | |
_ => { | |
return syn::Error::new( | |
rawtype_tokens[0].span(), | |
"#[rawtype(RawType)] group expected", | |
) | |
.to_compile_error() | |
.into() | |
} | |
}; |
And in fact the whole proto-derive crate can be deleted, without losing any functionality or any further consequences. I would argue whole code becomes easier to navigate as the macro creates some unnecessary indirection for a single line that explains what is happing.
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, true. Annotations seem cleaner to me, because I don't expect to look at the derive function. Serde does the same thing and it worked out pretty well there.
Also, we need to do some kind of JSON serialization for these domain types too and I didn't have time to get into the details of how we're going to do it. Maybe the derive macro helps, maybe not. Well, currently not really, it's just syntactic sugar.
I'll open this up to the team, I'm happy to do it either way, I just really wanted to do annotations. :)
- it is not really necessary
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.
Great work 👍
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 was too excited. It needs some cleanup in the documentation but everything else looks 👌
proto/src/domaintype.rs
Outdated
//! Requirements: | ||
//! * The DomainType trait requires the struct to implement the Clone trait. | ||
//! * The DomainType trait requires the struct to have a #[rawtype(MYRAWTYPE)] attribute set where | ||
//! the MYRAWTYPE is a structure that has the prost::Message trait implemented. (protobuf type) | ||
//! * The DomainType trait requires that the TryFrom<RawType> implemented on the structure has an | ||
//! error type that implements Into<BoxError>. (The current implementations with anomaly are | ||
//! fine.) |
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.
Actually some of the comments need updating now.
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'll do that.
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.
Minor updates in the documentation: removed references to the DomainType macro. I hope this is it. :)
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.
LGTM
Closes #535 again.
This change implements default functions for the DomainType trait, instead of implementing the functionality in the DomainType macro. The macro becomes syntactic sugar for
impl DomainType<RawType> for MyDomainStruct {}
.It also fixes a "feature" that wasn't correctly implemented: all functions in the trait that relate to Prost::Message (encode, decode, encode_len, etc) now work exactly as intended and they don't consume the DomainType struct.
The trait now requires additional traits to be implemented on the struct (notably
Clone
) but it's a small price to pay.It also enhances the DomainType trait with direct encoding/decoding using
Vec<u8>
. This functionality was available in the AminoMessage trait before.Thanks to Ismail for pointing out that this can be done.