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

Blanket implementation for DomainType #571

Merged
merged 4 commits into from
Sep 17, 2020
Merged

Blanket implementation for DomainType #571

merged 4 commits into from
Sep 17, 2020

Conversation

greg-szabo
Copy link
Member

@greg-szabo greg-szabo commented Sep 17, 2020

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.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md

@greg-szabo greg-szabo marked this pull request as ready for review September 17, 2020 15:51
@@ -55,41 +55,9 @@ fn expand_domaintype(input: &syn::DeriveInput) -> TokenStream {
}
};

// This is what all the hubbub is about.
Copy link
Member

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:

#[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.

Copy link
Member Author

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. :)

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 👍

Copy link
Member

@liamsi liamsi left a 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 👌

Comment on lines 36 to 42
//! 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.)
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do that.

Copy link
Member Author

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. :)

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@greg-szabo greg-szabo merged commit 43bba96 into master Sep 17, 2020
@greg-szabo greg-szabo deleted the greg/aminoff2 branch September 17, 2020 19:44
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.

Implement domain types for protobuf-encoded structs
2 participants