Remove the default bound from Packable::Visitor (#76)

* Remove the default bound from Packable::Visitor and replace const VERIFY with an Option * better impl * destructures * fix some tests * Fix 2 tests * fix last test * version + changelog * Apply suggestions from code review * fix no_std * review * fix last test error * Fix test * Update packable/packable-derive/CHANGELOG.md --------- Co-authored-by: Thibault Martinez <thibault@iota.org> Co-authored-by: Thibault Martinez <thibault.martinez.30@gmail.com>
iotaledger · Feb 9, 2024 · 42f76ea · 42f76ea
1 parent 7317d01
commit 42f76ea
Show file tree

Hide file tree

Showing 48 changed files with 254 additions and 199 deletions.
diff --git a/packable/packable-derive-test/Cargo.toml b/packable/packable-derive-test/Cargo.toml
@@ -16,7 +16,7 @@ name = "tests"
 path = "tests/lib.rs"
 
 [dev-dependencies]
-packable = { version = "=0.10.1", path = "../packable", default-features = false }
+packable = { version = "=0.11.0", path = "../packable", default-features = false }
 
 rustversion = { version = "1.0.14", default-features = false }
 trybuild = { version = "1.0.88", default-features = false, features = ["diff"] }

diff --git a/packable/packable-derive-test/tests/fail/invalid_verify_with_field_type.rs b/packable/packable-derive-test/tests/fail/invalid_verify_with_field_type.rs
@@ -21,8 +21,8 @@ impl From<Infallible> for PickyError {
     }
 }
 
-fn verify_value<const VERIFY: bool>(&value: &u64) -> Result<(), PickyError> {
-    if !VERIFY || value == 42 {
+fn verify_value(&value: &u64) -> Result<(), PickyError> {
+    if value == 42 {
         Ok(())
     } else {
         Err(PickyError(value as u8))

diff --git a/packable/packable-derive-test/tests/fail/invalid_verify_with_field_type.stderr b/packable/packable-derive-test/tests/fail/invalid_verify_with_field_type.stderr
@@ -2,16 +2,16 @@ error[E0308]: mismatched types
   --> tests/fail/invalid_verify_with_field_type.rs:32:10
    |
 32 | #[derive(Packable)]
-   |          ^^^^^^^^
-   |          |
-   |          expected `&u64`, found `&u8`
-   |          arguments to this function are incorrect
+   |          ^^^^^^^^ expected `&u64`, found `&u8`
+33 | #[packable(unpack_error = PickyError)]
+34 | pub struct Picky(#[packable(verify_with = verify_value)] u8);
+   |                                           ------------ arguments to this function are incorrect
    |
    = note: expected reference `&u64`
               found reference `&u8`
 note: function defined here
   --> tests/fail/invalid_verify_with_field_type.rs:24:4
    |
-24 | fn verify_value<const VERIFY: bool>(&value: &u64) -> Result<(), PickyError> {
-   |    ^^^^^^^^^^^^                     ------------
+24 | fn verify_value(&value: &u64) -> Result<(), PickyError> {
+   |    ^^^^^^^^^^^^ ------------
    = note: this error originates in the derive macro `Packable` (in Nightly builds, run with -Z macro-backtrace for more info)
diff --git a/packable/packable-derive-test/tests/fail/invalid_verify_with_struct_type.rs b/packable/packable-derive-test/tests/fail/invalid_verify_with_struct_type.rs
@@ -21,8 +21,8 @@ impl From<Infallible> for PickyError {
     }
 }
 
-fn verify<const VERIFY: bool>(&value: &u64) -> Result<(), PickyError> {
-    if !VERIFY || value == 42 {
+fn verify(&value: &u64) -> Result<(), PickyError> {
+    if value == 42 {
         Ok(())
     } else {
         Err(PickyError(value as u8))

diff --git a/packable/packable-derive-test/tests/fail/invalid_verify_with_struct_type.stderr b/packable/packable-derive-test/tests/fail/invalid_verify_with_struct_type.stderr
@@ -2,16 +2,16 @@ error[E0308]: mismatched types
   --> tests/fail/invalid_verify_with_struct_type.rs:32:10
    |
 32 | #[derive(Packable)]
-   |          ^^^^^^^^
-   |          |
-   |          expected `&u64`, found `&Picky`
-   |          arguments to this function are incorrect
+   |          ^^^^^^^^ expected `&u64`, found `&Picky`
+33 | #[packable(unpack_error = PickyError)]
+34 | #[packable(verify_with = verify)]
+   |                          ------ arguments to this function are incorrect
    |
    = note: expected reference `&u64`
               found reference `&Picky`
 note: function defined here
   --> tests/fail/invalid_verify_with_struct_type.rs:24:4
    |
-24 | fn verify<const VERIFY: bool>(&value: &u64) -> Result<(), PickyError> {
-   |    ^^^^^^                     ------------
+24 | fn verify(&value: &u64) -> Result<(), PickyError> {
+   |    ^^^^^^ ------------
    = note: this error originates in the derive macro `Packable` (in Nightly builds, run with -Z macro-backtrace for more info)
diff --git a/packable/packable-derive-test/tests/lib.rs b/packable/packable-derive-test/tests/lib.rs
@@ -60,4 +60,4 @@ macro_rules! make_test {
 #[rustversion::stable]
 make_test!();
 #[rustversion::not(stable)]
-make_test!();
+make_test!(incorrect_tag_enum);
diff --git a/packable/packable-derive-test/tests/pass/error_coercion.rs b/packable/packable-derive-test/tests/pass/error_coercion.rs
@@ -31,11 +31,11 @@ impl Packable for Picky {
         self.0.pack(packer)
     }
 
-    fn unpack<U: Unpacker, const VERIFY: bool>(
+    fn unpack<U: Unpacker>(
         unpacker: &mut U,
-        visitor: &Self::UnpackVisitor,
+        visitor: Option<&Self::UnpackVisitor>,
     ) -> Result<Self, UnpackError<Self::UnpackError, U::Error>> {
-        let value = u8::unpack::<_, VERIFY>(unpacker, visitor).coerce()?;
+        let value = u8::unpack(unpacker, visitor).coerce()?;
 
         if value == 42 {
             Ok(Self(value))

diff --git a/packable/packable-derive-test/tests/pass/verify_with_field.rs b/packable/packable-derive-test/tests/pass/verify_with_field.rs
@@ -21,12 +21,8 @@ impl From<Infallible> for PickyError {
     }
 }
 
-fn verify_value<const VERIFY: bool>(&value: &u8) -> Result<(), PickyError> {
-    if !VERIFY || value == 42 {
-        Ok(())
-    } else {
-        Err(PickyError(value))
-    }
+fn verify_value(&value: &u8) -> Result<(), PickyError> {
+    if value == 42 { Ok(()) } else { Err(PickyError(value)) }
 }
 
 #[derive(Packable)]

diff --git a/packable/packable-derive-test/tests/pass/verify_with_field_visitor.rs b/packable/packable-derive-test/tests/pass/verify_with_field_visitor.rs
@@ -21,12 +21,8 @@ impl From<Infallible> for PickyError {
     }
 }
 
-fn verify_value<const VERIFY: bool>(&value: &u8, _: &()) -> Result<(), PickyError> {
-    if !VERIFY || value == 42 {
-        Ok(())
-    } else {
-        Err(PickyError(value))
-    }
+fn verify_value(&value: &u8, _: &()) -> Result<(), PickyError> {
+    if value == 42 { Ok(()) } else { Err(PickyError(value)) }
 }
 
 #[derive(Packable)]

diff --git a/packable/packable-derive-test/tests/pass/verify_with_struct.rs b/packable/packable-derive-test/tests/pass/verify_with_struct.rs
@@ -21,8 +21,8 @@ impl From<Infallible> for PickyError {
     }
 }
 
-fn verify<const VERIFY: bool>(value: &Picky) -> Result<(), PickyError> {
-    if !VERIFY || value.0 == 42 {
+fn verify(value: &Picky) -> Result<(), PickyError> {
+    if value.0 == 42 {
         Ok(())
     } else {
         Err(PickyError(value.0))

diff --git a/packable/packable-derive-test/tests/pass/verify_with_struct_visitor.rs b/packable/packable-derive-test/tests/pass/verify_with_struct_visitor.rs
@@ -21,8 +21,8 @@ impl From<Infallible> for PickyError {
     }
 }
 
-fn verify<const VERIFY: bool>(value: &Picky, _: &()) -> Result<(), PickyError> {
-    if !VERIFY || value.0 == 42 {
+fn verify(value: &Picky, _: &()) -> Result<(), PickyError> {
+    if value.0 == 42 {
         Ok(())
     } else {
         Err(PickyError(value.0))

diff --git a/packable/packable-derive/CHANGELOG.md b/packable/packable-derive/CHANGELOG.md
@@ -19,6 +19,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Security -->
 
+## 0.10.0 - 2024-02-09
+
+### Changed
+
+- Updated to new `Packable` trait;
+- `verify_with` function is no longer called if the visitor is `None`;
+
 ## 0.9.0 - 2023-11-17
 
 ### Changed

diff --git a/packable/packable-derive/Cargo.toml b/packable/packable-derive/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "packable-derive"
-version = "0.9.0"
+version = "0.10.0"
 authors = ["IOTA Stiftung"]
 edition = "2021"
 description = "Derive macro for the `packable` crate."

diff --git a/packable/packable-derive/src/fragments.rs b/packable/packable-derive/src/fragments.rs
@@ -32,21 +32,39 @@ impl Fragments {
             fields_type,
         } = info;
 
-        let fields_verification = fields_verify_with.into_iter().zip(fields_ident.iter()).map(|(verify_with, field_ident)| match verify_with {
-            Some(verify_with) => if unpack_visitor_info.explicit {
-                quote!(#verify_with::<VERIFY>(&#field_ident, visitor).map_err(#crate_name::error::UnpackError::from_packable)?;)
-            } else {
-                quote!(#verify_with::<VERIFY>(&#field_ident).map_err(#crate_name::error::UnpackError::from_packable)?;)
-            }
-            None => quote!(),
-        });
+        let fields_verification = fields_verify_with.into_iter().zip(fields_ident.iter()).map(
+            |(verify_with, field_ident)| match verify_with {
+                Some(verify_with) => if unpack_visitor_info.explicit {
+                    quote! {
+                        if let Some(visitor) = visitor {
+                            #verify_with(&#field_ident, visitor).map_err(#crate_name::error::UnpackError::from_packable)?;
+                        }
+                    }
+                } else {
+                    quote! {
+                        if visitor.is_some() {
+                            #verify_with(&#field_ident).map_err(#crate_name::error::UnpackError::from_packable)?;
+                        }
+                    }
+                },
+                None => quote!(),
+            },
+        );
 
         let verify_with = match verify_with {
             Some(verify_with) => {
                 if unpack_visitor_info.explicit {
-                    quote!(#verify_with::<VERIFY>(&unpacked, visitor).map_err(#crate_name::error::UnpackError::from_packable)?;)
+                    quote! {
+                        if let Some(visitor) = visitor {
+                            #verify_with(&unpacked, visitor).map_err(#crate_name::error::UnpackError::from_packable)?;
+                        }
+                    }
                 } else {
-                    quote!(#verify_with::<VERIFY>(&unpacked).map_err(#crate_name::error::UnpackError::from_packable)?;)
+                    quote! {
+                        if visitor.is_some() {
+                            #verify_with(&unpacked).map_err(#crate_name::error::UnpackError::from_packable)?;
+                        }
+                    }
                 }
             }
             None => quote!(),
@@ -60,7 +78,7 @@ impl Fragments {
             },
             unpack: quote! {
                 #(
-                    let #fields_ident = <#fields_type as #crate_name::Packable>::unpack::<_, VERIFY>(unpacker, Borrow::<<#fields_type as #crate_name::Packable>::UnpackVisitor>::borrow(visitor)).map_packable_err(#fields_unpack_error_with).coerce()?;
+                    let #fields_ident = <#fields_type as #crate_name::Packable>::unpack(unpacker, visitor.map(Borrow::<<#fields_type as #crate_name::Packable>::UnpackVisitor>::borrow)).map_packable_err(#fields_unpack_error_with).coerce()?;
                     #fields_verification
                 )*
 

diff --git a/packable/packable-derive/src/trait_impl.rs b/packable/packable-derive/src/trait_impl.rs
@@ -113,7 +113,7 @@ impl TraitImpl {
                         #(#tag_decls)*
                         #(#tag_asserts)*
 
-                        match <#tag_type as #crate_name::Packable>::unpack::<_, VERIFY>(unpacker, Borrow::<<#tag_type as #crate_name::Packable>::UnpackVisitor>::borrow(visitor)).coerce()? {
+                        match <#tag_type as #crate_name::Packable>::unpack(unpacker, visitor.map(Borrow::<<#tag_type as #crate_name::Packable>::UnpackVisitor>::borrow)).coerce()? {
                             #(#unpack_arms)*
                             tag => Err(#crate_name::error::UnpackError::from_packable(#tag_with_error(tag)))
                         }
@@ -153,7 +153,7 @@ impl ToTokens for TraitImpl {
                     #pack
                 }
 
-                fn unpack<U: #crate_name::unpacker::Unpacker, const VERIFY: bool>(unpacker: &mut U, visitor: &Self::UnpackVisitor) -> Result<Self, #crate_name::error::UnpackError<Self::UnpackError, U::Error>> {
+                fn unpack<U: #crate_name::unpacker::Unpacker>(unpacker: &mut U, visitor: Option<&Self::UnpackVisitor>) -> Result<Self, #crate_name::error::UnpackError<Self::UnpackError, U::Error>> {
                     use #crate_name::error::UnpackErrorExt;
                     use core::borrow::Borrow;
                     #unpack

diff --git a/packable/packable/CHANGELOG.md b/packable/packable/CHANGELOG.md
@@ -19,6 +19,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Security -->
 
+## 0.11.0 - 2024-02-09
+
+### Added
+
+- `Packable::unpack_inner`, `Packable::unpack_verified`, and `Packable::unpack_unverified`;
+
+### Changed
+
+- Removed `Default` bound from `Packable::UnpackVisitor`;
+- Replaced `const VERIFY` in `Packable::unpack` with `Option<&Self::UnpackVisitor>`, which can be checked instead;
+- Renamed `PackableExt::unpack_verified` and `unpack_unverified` to `unpack_bytes_verified` and `unpack_bytes_unverified`;
+
 ## 0.10.1 - 2024-01-08
 
 ### Added

diff --git a/packable/packable/Cargo.toml b/packable/packable/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "packable"
-version = "0.10.1"
+version = "0.11.0"
 authors = ["IOTA Stiftung"]
 edition = "2021"
 description = "A crate for packing and unpacking binary representations."
@@ -20,7 +20,7 @@ usize = []
 autocfg = { version = "1.1.0", default-features = false }
 
 [dependencies]
-packable-derive = { version = "=0.9.0", path = "../packable-derive", default-features = false }
+packable-derive = { version = "=0.10.0", path = "../packable-derive", default-features = false }
 
 hashbrown = { version = "0.14.3", default-features = false, features = [
     "ahash",
@@ -29,4 +29,5 @@ hashbrown = { version = "0.14.3", default-features = false, features = [
 primitive-types = { version = "0.12.2", default-features = false, optional = true }
 serde = { version = "1.0.195", default-features = false, features = [
     "derive",
+    "alloc"
 ], optional = true }
diff --git a/packable/packable/src/packable/array.rs b/packable/packable/src/packable/array.rs
@@ -25,9 +25,9 @@ impl<T: Packable, const N: usize> Packable for [T; N] {
     }
 
     #[inline]
-    fn unpack<U: Unpacker, const VERIFY: bool>(
+    fn unpack<U: Unpacker>(
         unpacker: &mut U,
-        visitor: &Self::UnpackVisitor,
+        visitor: Option<&Self::UnpackVisitor>,
     ) -> Result<Self, UnpackError<Self::UnpackError, U::Error>> {
         if TypeId::of::<T>() == TypeId::of::<u8>() {
             let mut bytes = [0u8; N];
@@ -40,7 +40,7 @@ impl<T: Packable, const N: usize> Packable for [T; N] {
             let mut array = unsafe { MaybeUninit::<[MaybeUninit<T>; N]>::uninit().assume_init() };
 
             for item in array.iter_mut() {
-                let unpacked = T::unpack::<_, VERIFY>(unpacker, visitor)?;
+                let unpacked = T::unpack(unpacker, visitor)?;
 
                 // Safety: each `item` is only visited once so we are never overwriting nor dropping values that are
                 // already initialized.

diff --git a/packable/packable/src/packable/bool.rs b/packable/packable/src/packable/bool.rs
@@ -22,10 +22,10 @@ impl Packable for bool {
 
     /// Booleans are unpacked if the byte used to represent them is non-zero.
     #[inline]
-    fn unpack<U: Unpacker, const VERIFY: bool>(
+    fn unpack<U: Unpacker>(
         unpacker: &mut U,
-        visitor: &Self::UnpackVisitor,
+        visitor: Option<&Self::UnpackVisitor>,
     ) -> Result<Self, UnpackError<Self::UnpackError, U::Error>> {
-        Ok(u8::unpack::<_, VERIFY>(unpacker, visitor).coerce()? != 0)
+        Ok(u8::unpack(unpacker, visitor).coerce()? != 0)
     }
 }