Add packed flag to storage_item attribute and improve related diagnostics (#2722)

* ir: Add `packed` arg to ink! `storage_item`

* codegen: Use `packed` arg for ink! `storage_item`

* tests: Update ui tests

* tests: Update integration tests

* storage: Improve diagnostics for missing `Packed` bound

* tests: Update ui tests

* Update changelog

Author: davidsemakula

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 - Implement `bn128` precompiles ‒ [2708](https://github.com/use-ink/ink/pull/2718)
+- Add `packed` flag to `storage_item` attribute and improve related diagnostics - [](https://github.com/use-ink/ink/pull/2722)
 
 ### Changed
 - Refactor contract ref generation and add automatic re-exporting ‒ [#2710](https://github.com/use-ink/ink/pull/2710)

crates/ink/codegen/src/generator/storage_item.rs

@@ -53,9 +53,16 @@ impl GenerateCode for StorageItem<'_> {
             Data::Union(union_item) => self.generate_union(union_item),
         };
 
-        let mut derive = quote! {};
-        if self.item.config().derive() {
-            derive = quote! {
+        let config = self.item.config();
+        let derive = if config.packed() {
+            quote! {
+                #[cfg_attr(feature = "std", derive(
+                    ::ink::storage::traits::StorageLayout,
+                ))]
+                #[ink::scale_derive(Encode, Decode, TypeInfo)]
+            }
+        } else if config.derive() {
+            quote! {
                 #[cfg_attr(feature = "std", derive(
                     ::ink::storage::traits::StorageLayout,
                 ))]
@@ -65,10 +72,16 @@ impl GenerateCode for StorageItem<'_> {
                     ::ink::storage::traits::StorageKey,
                     ::ink::storage::traits::Storable,
                 )]
-            };
-        }
+            }
+        } else {
+            quote! {}
+        };
 
-        let type_check = self.generate_type_check();
+        let type_check = if self.item.config().packed() {
+            quote! {}
+        } else {
+            self.generate_type_check()
+        };
 
         quote! {
             #type_check
@@ -88,22 +101,32 @@ impl StorageItem<'_> {
         let generics = item.generics();
         let salt = item.salt();
 
-        let fields = struct_item.fields.iter().enumerate().map(|(i, field)| {
-            convert_into_storage_field(struct_ident, None, &salt, i, field)
-        });
+        let fields = if self.item.config().packed() {
+            let fields = struct_item.fields.iter();
+            quote! {
+                #(#fields),*
+            }
+        } else {
+            let fields = struct_item.fields.iter().enumerate().map(|(i, field)| {
+                convert_into_storage_field(struct_ident, None, &salt, i, field)
+            });
+            quote! {
+                #(#fields),*
+            }
+        };
 
         match struct_item.fields {
             Fields::Unnamed(_) => {
                 quote! {
                     #vis struct #struct_ident #generics (
-                        #(#fields),*
+                        #fields
                     );
                 }
             }
             _ => {
                 quote! {
                     #vis struct #struct_ident #generics {
-                        #(#fields),*
+                        #fields
                     }
                 }
             }
@@ -126,24 +149,29 @@ impl StorageItem<'_> {
                 quote! {}
             };
 
-            let fields: Vec<_> = variant
-                .fields
-                .iter()
-                .enumerate()
-                .map(|(i, field)| {
+            let fields = if self.item.config().packed() {
+                let fields = variant.fields.iter();
+                quote! {
+                    #(#fields),*
+                }
+            } else {
+                let fields = variant.fields.iter().enumerate().map(|(i, field)| {
                     convert_into_storage_field(
                         enum_ident,
                         Some(variant_ident),
                         &salt,
                         i,
                         field,
                     )
-                })
-                .collect();
+                });
+                quote! {
+                    #(#fields),*
+                }
+            };
 
             let fields = match variant.fields {
-                Fields::Named(_) => quote! { { #(#fields),* } },
-                Fields::Unnamed(_) => quote! { ( #(#fields),* ) },
+                Fields::Named(_) => quote! { { #fields } },
+                Fields::Unnamed(_) => quote! { ( #fields ) },
                 Fields::Unit => quote! {},
             };
 
@@ -167,18 +195,28 @@ impl StorageItem<'_> {
         let generics = item.generics();
         let salt = item.salt();
 
-        let fields = union_item
-            .fields
-            .named
-            .iter()
-            .enumerate()
-            .map(|(i, field)| {
-                convert_into_storage_field(union_ident, None, &salt, i, field)
-            });
+        let fields = if self.item.config().packed() {
+            let fields = union_item.fields.named.iter();
+            quote! {
+                #(#fields),*
+            }
+        } else {
+            let fields = union_item
+                .fields
+                .named
+                .iter()
+                .enumerate()
+                .map(|(i, field)| {
+                    convert_into_storage_field(union_ident, None, &salt, i, field)
+                });
+            quote! {
+                #(#fields),*
+            }
+        };
 
         quote! {
             #vis union #union_ident #generics {
-                #(#fields),*
+                #fields
             }
         }
     }

crates/ink/ir/src/ir/storage_item/config.rs

@@ -12,28 +12,58 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use syn::spanned::Spanned;
+
 use crate::{
     ast,
     utils::duplicate_config_err,
 };
 
-/// The ink! configuration.
-#[derive(Debug, Default, PartialEq, Eq)]
+/// The ink! storage item configuration.
+#[derive(Debug, PartialEq, Eq)]
 pub struct StorageItemConfig {
+    /// If set to `true`, the derived storage item will use a "packed" layout.
+    /// If set to `false`, the derived storage item will use a "non-packed" layout,
+    /// this is the default value.
+    packed: bool,
     /// If set to `true`, all storage related traits are implemented automatically,
     /// this is the default value.
     /// If set to `false`, implementing all storage traits is disabled. In some cases
     /// this can be helpful to override the default implementation of the trait.
     derive: bool,
 }
 
+impl Default for StorageItemConfig {
+    fn default() -> Self {
+        Self {
+            packed: false,
+            derive: true,
+        }
+    }
+}
+
 impl TryFrom<ast::AttributeArgs> for StorageItemConfig {
     type Error = syn::Error;
 
     fn try_from(args: ast::AttributeArgs) -> Result<Self, Self::Error> {
+        let mut packed: Option<syn::Path> = None;
         let mut derive: Option<syn::LitBool> = None;
-        for arg in args.into_iter() {
-            if arg.name().is_ident("derive") {
+        let args_span = args.span();
+        for arg in args {
+            if arg.name().is_ident("packed") {
+                if let Some(path) = packed {
+                    return Err(duplicate_config_err(path, arg, "packed", "storage item"));
+                }
+                if let ast::Meta::Path(path) = arg {
+                    packed = Some(path)
+                } else {
+                    return Err(format_err_spanned!(
+                        arg,
+                        "encountered an unexpected value for `packed` ink! storage item configuration argument. \
+                        Did you mean `#[ink::storage_item(packed)]` ?",
+                    ));
+                }
+            } else if arg.name().is_ident("derive") {
                 if let Some(lit_bool) = derive {
                     return Err(duplicate_config_err(
                         lit_bool,
@@ -58,15 +88,86 @@ impl TryFrom<ast::AttributeArgs> for StorageItemConfig {
                 ));
             }
         }
-        Ok(StorageItemConfig {
-            derive: derive.map(|lit_bool| lit_bool.value).unwrap_or(true),
-        })
+
+        // Sanitize user-provided configuration.
+        let (packed, derive) = match (packed, derive.map(|lit_bool| lit_bool.value)) {
+            // `packed` (i.e. `packed=true`) and `derive=false` conflict.
+            // Note: There's really no reasonable use case for this combination.
+            (Some(_), Some(false)) => {
+                return Err(format_err!(
+                    args_span,
+                    "cannot use `derive = false` with `packed` flag",
+                ))
+            }
+            // Otherwise, accept the user provided configuration,
+            // while defaulting to "non-packed" layout (resolved as `packed=false`) and
+            // `derive=true`.
+            (packed, derive) => (packed.is_some(), derive.unwrap_or(true)),
+        };
+
+        Ok(StorageItemConfig { packed, derive })
     }
 }
 
 impl StorageItemConfig {
-    /// Returns the derive configuration argument.
+    /// Returns the `packed` configuration argument.
+    pub fn packed(&self) -> bool {
+        self.packed
+    }
+
+    /// Returns the `derive` configuration argument.
     pub fn derive(&self) -> bool {
         self.derive
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use quote::quote;
+
+    #[test]
+    fn valid_args_works() {
+        for (config, packed, derive) in [
+            // Defaults are "non-packed" layout (resolved as `packed=false`) with
+            // `derive=true`.
+            (quote!(), false, true),
+            // `packed` (resolved as `packed=true`) works only with `derive=true`.
+            (quote! { packed }, true, true),
+            (quote! { packed, derive = true }, true, true),
+            // "non-packed" layout (resolved as `packed=false`) works with any `derive`
+            // arg.
+            (quote! { derive = true }, false, true),
+            (quote! { derive = false }, false, false),
+        ] {
+            let parsed_config = syn::parse2::<crate::ast::AttributeArgs>(config).unwrap();
+            let result = StorageItemConfig::try_from(parsed_config);
+            assert!(result.is_ok());
+            let storage_item_config = result.unwrap();
+            assert_eq!(storage_item_config.packed(), packed);
+            assert_eq!(storage_item_config.derive(), derive);
+        }
+    }
+
+    #[test]
+    #[should_panic = "cannot use `derive = false` with `packed` flag"]
+    fn conflicting_args_fails() {
+        let config = quote! {
+            // `packed` and `derive = false` conflict.
+            packed, derive = false
+        };
+        let parsed_config = syn::parse2::<crate::ast::AttributeArgs>(config).unwrap();
+        StorageItemConfig::try_from(parsed_config).unwrap();
+    }
+
+    #[test]
+    #[should_panic = "encountered an unexpected value for `packed` ink! storage item configuration argument. Did you mean `#[ink::storage_item(packed)]` ?"]
+    fn invalid_packed_value_fails() {
+        let config = quote! {
+            // `packed` arg doesn't accept a value.
+            packed = true
+        };
+        let parsed_config = syn::parse2::<crate::ast::AttributeArgs>(config).unwrap();
+        StorageItemConfig::try_from(parsed_config).unwrap();
+    }
+}