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

Extract serialize_map! macros #338

Merged
merged 20 commits into from
Jul 19, 2024
Merged

Conversation

emlun
Copy link
Contributor

@emlun emlun commented Jul 6, 2024

Prompted by #337 (comment) - it's better to just not have to worry about correctly calculating the map length.

This introduces macros util::serialize_map! and util::serialize_map_optional!, which take care of computing the map length of and serializing heterogeneous maps. serialize_map! requires all entries to always be present (and thus generates a map of constant length), and serialize_map_optional! takes Option<T> entries and omits None values.

The serialize_map_optional! API is a little bit awkward because it needs the caller to specify a bunch of arbitrary identifiers (this is because the macro needs to bind the values to internal local variables in order to not evaluate the expressions multiple times, but macro_rules! can't generate a non-constant number of internal identifiers as far as I've been able to tell), but I think this is at least much better than the raw serde API as this makes it impossible for the declared map length and actual entries to go out of sync.

This includes regression tests covering all occurrences of code that was refactored to use the new macros. I have manually verified that the tests cover both the map length computations and the serialization of each individual entry of each struct.

The tests inflate the diff volume quite a lot; the diff of just the refactorization is +239, -366. 😉

@emlun
Copy link
Contributor Author

emlun commented Jul 6, 2024

Here's an example macro expansion, just for fun:

impl Serialize for MakeCredentials {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        debug!("Serialize MakeCredentials");
        serialize_map_optional!(
            serializer,
            v1: &0x01 => Some(&self.client_data_hash),
            v2: &0x02 => Some(&self.rp),
            v3: &0x03 => Some(&self.user),
            v4: &0x04 => Some(&self.pub_cred_params),
            v5: &0x05 => (!self.exclude_list.is_empty()).then_some(&self.exclude_list),
            v6: &0x06 => self.extensions.has_content().then_some(&self.extensions),
            v7: &0x07 => self.options.has_some().then_some(&self.options),
            v8: &0x08 => &self.pin_uv_auth_param,
            v9: &0x09 => self.pin_uv_auth_param.as_ref().map(|p| p.pin_protocol.id()),
            va: &0x0a => &self.enterprise_attestation,
        )
    }
}

expands to:

impl Serialize for MakeCredentials {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        debug!("Serialize MakeCredentials");
        {
            let serializer = serializer;
            let v1 = (Some(&self.client_data_hash));
            let v2 = (Some(&self.rp));
            let v3 = (Some(&self.user));
            let v4 = (Some(&self.pub_cred_params));
            let v5 = ((!self.exclude_list.is_empty()).then_some(&self.exclude_list));
            let v6 = (self.extensions.has_content().then_some(&self.extensions));
            let v7 = (self.options.has_some().then_some(&self.options));
            let v8 = (&self.pin_uv_auth_param);
            let v9 = (self.pin_uv_auth_param.as_ref().map(|p| p.pin_protocol.id()));
            let va = (&self.enterprise_attestation);
            let map_len = 0usize
                + if v1.is_some() { 1usize } else { 0usize }
                + if v2.is_some() { 1usize } else { 0usize }
                + if v3.is_some() { 1usize } else { 0usize }
                + if v4.is_some() { 1usize } else { 0usize }
                + if v5.is_some() { 1usize } else { 0usize }
                + if v6.is_some() { 1usize } else { 0usize }
                + if v7.is_some() { 1usize } else { 0usize }
                + if v8.is_some() { 1usize } else { 0usize }
                + if v9.is_some() { 1usize } else { 0usize }
                + if va.is_some() { 1usize } else { 0usize };
            let mut map = serializer.serialize_map(core::option::Option::Some(map_len))?;
            if let core::option::Option::Some(v) = v1 {
                map.serialize_entry((&0x01), &v)?;
            }
            if let core::option::Option::Some(v) = v2 {
                map.serialize_entry((&0x02), &v)?;
            }
            if let core::option::Option::Some(v) = v3 {
                map.serialize_entry((&0x03), &v)?;
            }
            if let core::option::Option::Some(v) = v4 {
                map.serialize_entry((&0x04), &v)?;
            }
            if let core::option::Option::Some(v) = v5 {
                map.serialize_entry((&0x05), &v)?;
            }
            if let core::option::Option::Some(v) = v6 {
                map.serialize_entry((&0x06), &v)?;
            }
            if let core::option::Option::Some(v) = v7 {
                map.serialize_entry((&0x07), &v)?;
            }
            if let core::option::Option::Some(v) = v8 {
                map.serialize_entry((&0x08), &v)?;
            }
            if let core::option::Option::Some(v) = v9 {
                map.serialize_entry((&0x09), &v)?;
            }
            if let core::option::Option::Some(v) = va {
                map.serialize_entry((&0x0a), &v)?;
            }
            map.end()
        }
    }
}

@emlun
Copy link
Contributor Author

emlun commented Jul 10, 2024

I made serialize_map_optional! less awkward for most (all current) uses of it, by eliminating the need for the identifier argument for the first 10 entries. This comes at the cost of a bit of hardcoding, but I think the improved call site API is worth it.

Copy link
Collaborator

@jschanck jschanck left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks for doing this!

@jschanck jschanck merged commit 86ca747 into mozilla:ctap2-2021 Jul 19, 2024
@emlun emlun deleted the serialize-map branch July 19, 2024 00:28
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