Follow-up for canonical se/de after an additional round of review (#588)

* Follow-up for canonical se/de after an additional round of review. - Removed all enum attributes. The manual implementation of the traits can do the same. - Removed `clone` for each `Input` required to call `into_full`. It is possible because of a new `Empty<Type>` type that implements canonical se/de with minimal implementation. - Removed `SIZE_NO_DYNAMIC` because it is not used in the codebase. - Removed `SerializedSizeFixed` because we don't have any specific logic that uses it. - Moved all methods from `SerializedSize` to `Serialize`. It looks unnatural that `#[derive(Serialize)]` implements 3 traits. Instead, it implements on trait now. - Removed `SIZE_STATIC` because it is impossible to calculate in the case of enums(or if the `struct` has an enum field). Supporting something like this requires another way of doing that, plus it doesn't provide a lot of wins for us right now because `#[inline(always]` handle that well enough. - Removed panics during size calculation. Instead we use `saturating_add`. If the size is incorrect we will fail in another place. - Removed usage of `num_enum` and replaced with canonical se/de. - Removed calculation of the discriminant on macro level. Replaced with native rust calculation that covers all cases. * Update CHANGELOG.md * Adddressed comments in the PR
FuelLabs · Sep 25, 2023 · 5e2f432 · 5e2f432
1 parent 31ef778
commit 5e2f432
Show file tree

Hide file tree

Showing 48 changed files with 564 additions and 924 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,15 +6,23 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 
+### Added
+
+- [#586](https://github.com/FuelLabs/fuel-vm/pull/586): Added `default_asset` method to the `ContractIdExt` trait implementation, to mirror the `default` method on AssetId in the Sway std lib.
+
+### Changed
+
 #### Breaking
 
 - [#578](https://github.com/FuelLabs/fuel-vm/pull/578): Support `no_std` environments for `fuel-crypto`, falling back to a pure-Rust crypto implementation.
 - [#582](https://github.com/FuelLabs/fuel-vm/pull/582): Make `fuel-vm` and `fuel-tx` crates compatible with `no_std` + `alloc`. This includes reworking all error handling that used `std::io::Error`, replacing some `std::collection::{HashMap, HashSet}` with `hashbrown::{HashMap, HashSet}` and many changes to feature-gating of APIs.
 - [#587](https://github.com/FuelLabs/fuel-vm/pull/587): Replace `thiserror` dependency with `derive_more`, so that `core::fmt::Display` is implemented without the `std` feature. Removes `std::io::Error` trait impls from the affected types.
+- [#588](https://github.com/FuelLabs/fuel-vm/pull/588): Re-worked the size calculation of the canonical serialization/deserialization.
 
-### Added
+### Removed
 
-- [#586](https://github.com/FuelLabs/fuel-vm/pull/586): Added `default_asset` method to the `ContractIdExt` trait implementation, to mirror the `default` method on AssetId in the Sway std lib.
+#### Breaking
+- [#588](https://github.com/FuelLabs/fuel-vm/pull/588): Removed `SerializedSize` and `SerializedFixedSize` traits. Removed support for `SIZE_NO_DYNAMIC` and `SIZE_STATIC`. Removed enum attributes from derive macro for `Serialize` and `Deserialize` traits..
 
 ## [Version 0.37.0]
 

diff --git a/fuel-derive/src/attribute.rs b/fuel-derive/src/attribute.rs
@@ -57,7 +57,6 @@ fn parse_attrs(s: &synstructure::Structure) -> HashMap<String, TokenStream> {
 /// Pop-level `canonical` attributes for a struct
 pub struct StructAttrs {
     /// The struct is prefixed with the given word.
-    /// Useful with`#[canonical(inner_discriminant)]`.
     /// The type must implement `Into<u64>`, which is used to serialize it.
     pub prefix: Option<TokenStream>,
 }
@@ -76,73 +75,6 @@ impl StructAttrs {
     }
 }
 
-/// Pop-level `canonical` attributes for an enum
-#[allow(non_snake_case)]
-pub struct EnumAttrs {
-    /// This is a wrapper enum where every variant can be recognized from it's first
-    /// word. This means that the enum itself doesn't have to serialize the
-    /// discriminant, but the field itself does so. This can be done using
-    /// `#[canonical(prefix = ...)]` attribute. `TryFromPrimitive` traits are used to
-    /// convert the raw bytes into the given type.
-    pub inner_discriminant: Option<TokenStream>,
-    /// Replaces calculation of the serialized static size with a custom function.
-    pub serialized_size_static_with: Option<TokenStream>,
-    /// Replaces calculation of the serialized dynamic size with a custom function.
-    pub serialized_size_dynamic_with: Option<TokenStream>,
-    /// Replaces serialization with a custom function.
-    pub serialize_with: Option<TokenStream>,
-    /// Replaces deserialization with a custom function.
-    pub deserialize_with: Option<TokenStream>,
-    /// Determines whether the enum has a dynamic size when `serialize_with` is used.
-    pub SIZE_NO_DYNAMIC: Option<TokenStream>,
-}
-
-impl EnumAttrs {
-    #[allow(non_snake_case)]
-    pub fn parse(s: &synstructure::Structure) -> Self {
-        let mut attrs = parse_attrs(s);
-
-        let inner_discriminant = attrs.remove("inner_discriminant");
-        let serialized_size_static_with = attrs.remove("serialized_size_static_with");
-        let serialized_size_dynamic_with = attrs.remove("serialized_size_dynamic_with");
-        let serialize_with = attrs.remove("serialize_with");
-        let deserialize_with = attrs.remove("deserialize_with");
-        let SIZE_NO_DYNAMIC = attrs.remove("SIZE_NO_DYNAMIC");
-
-        if !attrs.is_empty() {
-            panic!("unknown canonical attributes: {:?}", attrs.keys())
-        }
-
-        Self {
-            inner_discriminant,
-            serialized_size_static_with,
-            serialized_size_dynamic_with,
-            serialize_with,
-            deserialize_with,
-            SIZE_NO_DYNAMIC,
-        }
-    }
-}
-
-/// Parse `#[repr(int)]` attribute for an enum.
-pub fn parse_enum_repr(attrs: &[Attribute]) -> Option<String> {
-    for attr in attrs {
-        if attr.style != AttrStyle::Outer {
-            continue
-        }
-        if let Meta::List(ml) = &attr.meta {
-            if ml.path.segments.len() == 1 && ml.path.segments[0].ident == "repr" {
-                if let Some(TokenTree::Ident(ident)) =
-                    ml.tokens.clone().into_iter().next()
-                {
-                    return Some(ident.to_string())
-                }
-            }
-        }
-    }
-    None
-}
-
 /// Parse `#[canonical(skip)]` attribute for a binding field.
 pub fn should_skip_field_binding(binding: &BindingInfo<'_>) -> bool {
     should_skip_field(&binding.ast().attrs)

diff --git a/fuel-derive/src/deserialize.rs b/fuel-derive/src/deserialize.rs
@@ -1,14 +1,13 @@
 use proc_macro2::TokenStream as TokenStream2;
-use quote::quote;
-
-use crate::{
-    attribute::{
-        should_skip_field,
-        should_skip_field_binding,
-        EnumAttrs,
-        StructAttrs,
-    },
-    evaluate::evaluate_simple_expr,
+use quote::{
+    format_ident,
+    quote,
+};
+
+use crate::attribute::{
+    should_skip_field,
+    should_skip_field_binding,
+    StructAttrs,
 };
 
 fn deserialize_struct(s: &mut synstructure::Structure) -> TokenStream2 {
@@ -42,8 +41,8 @@ fn deserialize_struct(s: &mut synstructure::Structure) -> TokenStream2 {
 
     let remove_prefix = if let Some(expected_prefix) = StructAttrs::parse(s).prefix {
         quote! {{
-            let prefix = <u64 as ::fuel_types::canonical::Deserialize>::decode_static(buffer)?;
-            if prefix.try_into() != Ok(#expected_prefix) {
+            let prefix = <_ as ::fuel_types::canonical::Deserialize>::decode_static(buffer);
+            if prefix != Ok(#expected_prefix) {
                 return ::core::result::Result::Err(::fuel_types::canonical::Error::InvalidPrefix)
             }
         }}
@@ -69,16 +68,32 @@ fn deserialize_struct(s: &mut synstructure::Structure) -> TokenStream2 {
 }
 
 fn deserialize_enum(s: &synstructure::Structure) -> TokenStream2 {
-    let attrs = EnumAttrs::parse(s);
     let _name = &s.ast().ident;
 
     assert!(!s.variants().is_empty(), "got invalid empty enum");
 
-    let mut next_discriminant = 0u64;
+    let mut next_discriminant = quote! { { 0u64 } };
+    let enum_ident = &s.ast().ident;
+    let calculated_discriminants =
+        s.variants().iter().enumerate().map(|(index, variant)| {
+            if variant.ast().discriminant.is_some() {
+                let variant_ident = variant.ast().ident;
+                next_discriminant = quote! { { #enum_ident::#variant_ident as u64 } };
+            }
+
+            let const_ident = format_ident!("V{}", index);
+            let result = quote! { const #const_ident: ::core::primitive::u64 =  #next_discriminant; };
+
+            next_discriminant = quote! { ( (#next_discriminant) + 1u64 ) };
+
+            result
+        });
+
     let decode_static: TokenStream2 = s
         .variants()
         .iter()
-        .map(|variant| {
+        .enumerate()
+        .map(|(index, variant)| {
             let decode_main = variant.construct(|field, _| {
                 if should_skip_field(&field.attrs) {
                     quote! {
@@ -91,37 +106,24 @@ fn deserialize_enum(s: &synstructure::Structure) -> TokenStream2 {
                     }}
                 }
             });
-
-
-            let discr = if let Some(discr_type) = attrs.inner_discriminant.as_ref() {
-                let vname = variant.ast().ident;
-                quote! { #discr_type::#vname }
-            } else {
-                if let Some((_, d)) = variant.ast().discriminant {
-                    next_discriminant = evaluate_simple_expr(d).expect("Unable to evaluate discriminant expression");
-                };
-                let v = next_discriminant;
-                next_discriminant = next_discriminant.checked_add(1).expect("Discriminant overflow");
-                quote! { #v }
-            };
+            let const_ident = format_ident!("V{}", index);
 
             quote! {
-                #discr => {
+                #const_ident => {
                     ::core::result::Result::Ok(#decode_main)
                 }
             }
-        }).collect();
+        })
+        .collect();
 
     let decode_dynamic = s.variants().iter().map(|variant| {
         let decode_dynamic = variant.each(|binding| {
-            if should_skip_field_binding(binding) {
-                quote! {
-                    *#binding = ::core::default::Default::default();
-                }
-            } else {
+            if !should_skip_field_binding(binding) {
                 quote! {{
                     ::fuel_types::canonical::Deserialize::decode_dynamic(#binding, buffer)?;
                 }}
+            } else {
+                quote! {}
             }
         });
 
@@ -130,53 +132,18 @@ fn deserialize_enum(s: &synstructure::Structure) -> TokenStream2 {
         }
     });
 
-    // Handle #[canonical(inner_discriminant = Type)]
-    let decode_discriminant = if attrs.inner_discriminant.is_some() {
-        quote! {
-            let buf = buffer.clone();
-            let raw_discr = <::core::primitive::u64 as ::fuel_types::canonical::Deserialize>::decode(buffer)?;
-            *buffer = buf; // Restore buffer position
-        }
-    } else {
+    let discriminant = {
         quote! {
-            let raw_discr = <::core::primitive::u64 as ::fuel_types::canonical::Deserialize>::decode(buffer)?;
+            <::core::primitive::u64 as ::fuel_types::canonical::Deserialize>::decode(buffer)?
         }
     };
 
-    // Handle #[canonical(inner_discriminant = Type)]
-    let mapped_discr = if let Some(discr_type) = attrs.inner_discriminant {
-        quote! { {
-            use ::num_enum::{TryFromPrimitive, IntoPrimitive};
-            let Ok(discr) = #discr_type::try_from_primitive(raw_discr) else {
-                return ::core::result::Result::Err(::fuel_types::canonical::Error::UnknownDiscriminant)
-            };
-            discr
-        } }
-    } else {
-        quote! { raw_discr }
-    };
-
-    // Handle #[canonical(deserialize_with = function)]
-    if let Some(helper) = attrs.deserialize_with {
-        return s.gen_impl(quote! {
-            gen impl ::fuel_types::canonical::Deserialize for @Self {
-                fn decode_static<I: ::fuel_types::canonical::Input + ?Sized>(buffer: &mut I) -> ::core::result::Result<Self, ::fuel_types::canonical::Error> {
-                    let raw_discr = <::core::primitive::u64 as ::fuel_types::canonical::Deserialize>::decode(buffer)?;
-                    #helper(#mapped_discr, buffer)
-                }
-
-                fn decode_dynamic<I: ::fuel_types::canonical::Input + ?Sized>(&mut self, buffer: &mut I) -> ::core::result::Result<(), ::fuel_types::canonical::Error> {
-                    ::core::result::Result::Ok(())
-                }
-            }
-        })
-    }
-
     s.gen_impl(quote! {
         gen impl ::fuel_types::canonical::Deserialize for @Self {
             fn decode_static<I: ::fuel_types::canonical::Input + ?Sized>(buffer: &mut I) -> ::core::result::Result<Self, ::fuel_types::canonical::Error> {
-                #decode_discriminant
-                match #mapped_discr {
+                #( #calculated_discriminants )*
+
+                match #discriminant {
                     #decode_static
                     _ => ::core::result::Result::Err(::fuel_types::canonical::Error::UnknownDiscriminant),
                 }

diff --git a/fuel-derive/src/evaluate.rs b/fuel-derive/src/evaluate.rs
diff --git a/fuel-derive/src/lib.rs b/fuel-derive/src/lib.rs
@@ -3,12 +3,8 @@
 #![deny(unused_must_use, missing_docs)]
 
 extern crate proc_macro;
-
-mod utils;
-
 mod attribute;
 mod deserialize;
-mod evaluate;
 mod serialize;
 
 use self::{