From 507c312d980a60f97cc3a167f812edeb3a56c06b Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Wed, 2 Jun 2021 20:46:05 -0700 Subject: [PATCH 1/8] Implement parsing for #[pallet::storage_name] on storage items --- .../procedural/src/pallet/parse/storage.rs | 69 +++++++++++++++---- .../pallet_ui/storage_invalid_attribute.rs | 24 +++++++ .../storage_invalid_attribute.stderr | 5 ++ .../pallet_ui/storage_multiple_getters.rs | 25 +++++++ .../pallet_ui/storage_multiple_getters.stderr | 5 ++ .../pallet_ui/storage_multiple_renames.rs | 25 +++++++ .../pallet_ui/storage_multiple_renames.stderr | 5 ++ 7 files changed, 146 insertions(+), 12 deletions(-) create mode 100644 frame/support/test/tests/pallet_ui/storage_invalid_attribute.rs create mode 100644 frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr create mode 100644 frame/support/test/tests/pallet_ui/storage_multiple_getters.rs create mode 100644 frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr create mode 100644 frame/support/test/tests/pallet_ui/storage_multiple_renames.rs create mode 100644 frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr diff --git a/frame/support/procedural/src/pallet/parse/storage.rs b/frame/support/procedural/src/pallet/parse/storage.rs index 6b842ab7fa401..7ccbb54a9cd6c 100644 --- a/frame/support/procedural/src/pallet/parse/storage.rs +++ b/frame/support/procedural/src/pallet/parse/storage.rs @@ -25,13 +25,26 @@ mod keyword { syn::custom_keyword!(Error); syn::custom_keyword!(pallet); syn::custom_keyword!(getter); + syn::custom_keyword!(storage_name); syn::custom_keyword!(OptionQuery); syn::custom_keyword!(ValueQuery); } -/// Parse for `#[pallet::getter(fn dummy)]` -pub struct PalletStorageAttr { - getter: syn::Ident, +/// Parse for one of the following: +/// * `#[pallet::getter(fn dummy)]` +/// * `#[pallet::storage_name = "CustomName"]` +pub enum PalletStorageAttr { + Getter(syn::Ident), + StorageName(syn::Expr), +} + +impl PalletStorageAttr { + fn span(&self) -> proc_macro2::Span { + match self { + Self::Getter(ident) => ident.span(), + Self::StorageName(expr) => expr.span(), + } + } } impl syn::parse::Parse for PalletStorageAttr { @@ -41,12 +54,23 @@ impl syn::parse::Parse for PalletStorageAttr { syn::bracketed!(content in input); content.parse::()?; content.parse::()?; - content.parse::()?; - let generate_content; - syn::parenthesized!(generate_content in content); - generate_content.parse::()?; - Ok(Self { getter: generate_content.parse::()? }) + let lookahead = content.lookahead1(); + if lookahead.peek(keyword::getter) { + content.parse::()?; + + let generate_content; + syn::parenthesized!(generate_content in content); + generate_content.parse::()?; + Ok(Self::Getter(generate_content.parse::()?)) + } else if lookahead.peek(keyword::storage_name) { + content.parse::()?; + content.parse::()?; + + Ok(Self::StorageName(content.parse::()?)) + } else { + Err(lookahead.error()) + } } } @@ -89,6 +113,8 @@ pub struct StorageDef { pub instances: Vec, /// Optional getter to generate. If some then query_kind is ensured to be some as well. pub getter: Option, + /// Optional expression that evaluates to a type that can be used as StoragePrefix instead of ident. + pub rename_as: Option, /// Whereas the querytype of the storage is OptionQuery or ValueQuery. /// Note that this is best effort as it can't be determined when QueryKind is generic, and /// result can be false if user do some unexpected type alias. @@ -552,12 +578,30 @@ impl StorageDef { return Err(syn::Error::new(item.span(), "Invalid pallet::storage, expect item type.")); }; - let mut attrs: Vec = helper::take_item_pallet_attrs(&mut item.attrs)?; - if attrs.len() > 1 { + let attrs: Vec = helper::take_item_pallet_attrs(&mut item.attrs)?; + let (mut getters, mut names) = attrs + .into_iter() + .partition::, _>(|attr| matches!(attr, PalletStorageAttr::Getter(_))); + if getters.len() > 1 { let msg = "Invalid pallet::storage, multiple argument pallet::getter found"; - return Err(syn::Error::new(attrs[1].getter.span(), msg)); + return Err(syn::Error::new(getters[1].span(), msg)); + } + if names.len() > 1 { + let msg = "Invalid pallet::storage, multiple argument pallet::storage_name found"; + return Err(syn::Error::new(names[1].span(), msg)); } - let getter = attrs.pop().map(|attr| attr.getter); + let getter = getters.pop().map(|attr| { + match attr { + PalletStorageAttr::Getter(ident) => ident, + _ => unreachable!(), + } + }); + let rename_as = names.pop().map(|attr| { + match attr { + PalletStorageAttr::StorageName(expr) => expr, + _ => unreachable!(), + } + }); let cfg_attrs = helper::get_item_cfg_attrs(&item.attrs); @@ -609,6 +653,7 @@ impl StorageDef { metadata, docs, getter, + rename_as, query_kind, where_clause, cfg_attrs, diff --git a/frame/support/test/tests/pallet_ui/storage_invalid_attribute.rs b/frame/support/test/tests/pallet_ui/storage_invalid_attribute.rs new file mode 100644 index 0000000000000..82ed6610ba1a4 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/storage_invalid_attribute.rs @@ -0,0 +1,24 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::Hooks; + use frame_system::pallet_prelude::BlockNumberFor; + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(core::marker::PhantomData); + + #[pallet::hooks] + impl Hooks> for Pallet {} + + #[pallet::call] + impl Pallet {} + + #[pallet::storage] + #[pallet::generate_store(pub trait Store)] + type Foo = StorageValue; +} + +fn main() { +} diff --git a/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr b/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr new file mode 100644 index 0000000000000..fa7facba46e97 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr @@ -0,0 +1,5 @@ +error: expected `getter` or `storage_name` + --> $DIR/storage_invalid_attribute.rs:19:15 + | +19 | #[pallet::generate_store(pub trait Store)] + | ^^^^^^^^^^^^^^ diff --git a/frame/support/test/tests/pallet_ui/storage_multiple_getters.rs b/frame/support/test/tests/pallet_ui/storage_multiple_getters.rs new file mode 100644 index 0000000000000..5c403371c5471 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/storage_multiple_getters.rs @@ -0,0 +1,25 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::Hooks; + use frame_system::pallet_prelude::BlockNumberFor; + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(core::marker::PhantomData); + + #[pallet::hooks] + impl Hooks> for Pallet {} + + #[pallet::call] + impl Pallet {} + + #[pallet::storage] + #[pallet::getter(fn get_foo)] + #[pallet::getter(fn foo_error)] + type Foo = StorageValue<_, u8>; +} + +fn main() { +} diff --git a/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr b/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr new file mode 100644 index 0000000000000..039f2b8282beb --- /dev/null +++ b/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr @@ -0,0 +1,5 @@ +error: Invalid pallet::storage, multiple argument pallet::getter found + --> $DIR/storage_multiple_getters.rs:20:25 + | +20 | #[pallet::getter(fn foo_error)] + | ^^^^^^^^^ diff --git a/frame/support/test/tests/pallet_ui/storage_multiple_renames.rs b/frame/support/test/tests/pallet_ui/storage_multiple_renames.rs new file mode 100644 index 0000000000000..0dd33a3b4c21f --- /dev/null +++ b/frame/support/test/tests/pallet_ui/storage_multiple_renames.rs @@ -0,0 +1,25 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::Hooks; + use frame_system::pallet_prelude::BlockNumberFor; + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(core::marker::PhantomData); + + #[pallet::hooks] + impl Hooks> for Pallet {} + + #[pallet::call] + impl Pallet {} + + #[pallet::storage] + #[pallet::storage_name = "Bar"] + #[pallet::storage_name = "Baz"] + type Foo = StorageValue<_, u8>; +} + +fn main() { +} diff --git a/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr b/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr new file mode 100644 index 0000000000000..8e7864973aa13 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr @@ -0,0 +1,5 @@ +error: Invalid pallet::storage, multiple argument pallet::storage_name found + --> $DIR/storage_multiple_renames.rs:20:30 + | +20 | #[pallet::storage_name = "Baz"] + | ^^^^^ From a1e75b22762ac05cf6691e25786d6516a9a78dd1 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Tue, 8 Jun 2021 17:27:28 -0700 Subject: [PATCH 2/8] Rename storage prefix when a #[pallet::storage_name] is supplied --- .../procedural/src/pallet/expand/storage.rs | 32 +++++++++++++++---- .../procedural/src/pallet/parse/storage.rs | 10 +++--- frame/support/test/tests/pallet.rs | 15 +++++++++ 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/frame/support/procedural/src/pallet/expand/storage.rs b/frame/support/procedural/src/pallet/expand/storage.rs index c956425379c53..5e974799ba21b 100644 --- a/frame/support/procedural/src/pallet/expand/storage.rs +++ b/frame/support/procedural/src/pallet/expand/storage.rs @@ -15,14 +15,30 @@ // See the License for the specific language governing permissions and // limitations under the License. -use crate::pallet::Def; +use crate::pallet::{Def, parse::storage::StorageDef}; use crate::pallet::parse::storage::{Metadata, QueryKind, StorageGenerics}; use frame_support_procedural_tools::clean_type_string; +fn literal_to_string(lit: &syn::Lit) -> String { + match lit { + syn::Lit::Str(s) => s.value(), + syn::Lit::ByteStr(s) => + String::from_utf8(s.value()).expect("All byte strings are valid UTF-8 strings; qed"), + syn::Lit::Byte(b) => char::from(b.value()).to_string(), + syn::Lit::Char(c) => c.value().to_string(), + syn::Lit::Int(i) => i.base10_digits().to_owned(), + syn::Lit::Float(f) => f.base10_digits().to_owned(), + syn::Lit::Bool(b) => b.value.to_string(), + syn::Lit::Verbatim(l) => l.to_string(), + } +} + /// Generate the prefix_ident related the the storage. /// prefix_ident is used for the prefix struct to be given to storage as first generic param. -fn prefix_ident(storage_ident: &syn::Ident) -> syn::Ident { - syn::Ident::new(&format!("_GeneratedPrefixForStorage{}", storage_ident), storage_ident.span()) +fn prefix_ident(storage: &StorageDef) -> syn::Ident { + let storage_ident = &storage.ident; + let ident = storage.rename_as.as_ref().map(literal_to_string).unwrap_or(storage_ident.to_string()); + syn::Ident::new(&format!("_GeneratedPrefixForStorage{}", ident), storage_ident.span()) } /// * if generics are unnamed: replace the first generic `_` by the generated prefix structure @@ -50,7 +66,7 @@ pub fn process_generics(def: &mut Def) { _ => unreachable!("Checked by def"), }; - let prefix_ident = prefix_ident(&storage_def.ident); + let prefix_ident = prefix_ident(&storage_def); let type_use_gen = if def.config.has_instance { quote::quote_spanned!(storage_def.attr_span => T, I) } else { @@ -344,9 +360,13 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream { let prefix_structs = def.storages.iter().map(|storage_def| { let type_impl_gen = &def.type_impl_generics(storage_def.attr_span); let type_use_gen = &def.type_use_generics(storage_def.attr_span); - let prefix_struct_ident = prefix_ident(&storage_def.ident); + let prefix_struct_ident = prefix_ident(&storage_def); let prefix_struct_vis = &storage_def.vis; - let prefix_struct_const = storage_def.ident.to_string(); + let prefix_struct_const = storage_def + .rename_as + .as_ref() + .map(literal_to_string) + .unwrap_or(storage_def.ident.to_string()); let config_where_clause = &def.config.where_clause; let cfg_attrs = &storage_def.cfg_attrs; diff --git a/frame/support/procedural/src/pallet/parse/storage.rs b/frame/support/procedural/src/pallet/parse/storage.rs index 7ccbb54a9cd6c..4a0b1fdab26cc 100644 --- a/frame/support/procedural/src/pallet/parse/storage.rs +++ b/frame/support/procedural/src/pallet/parse/storage.rs @@ -35,14 +35,14 @@ mod keyword { /// * `#[pallet::storage_name = "CustomName"]` pub enum PalletStorageAttr { Getter(syn::Ident), - StorageName(syn::Expr), + StorageName(syn::Lit), } impl PalletStorageAttr { fn span(&self) -> proc_macro2::Span { match self { Self::Getter(ident) => ident.span(), - Self::StorageName(expr) => expr.span(), + Self::StorageName(lit) => lit.span(), } } } @@ -67,7 +67,7 @@ impl syn::parse::Parse for PalletStorageAttr { content.parse::()?; content.parse::()?; - Ok(Self::StorageName(content.parse::()?)) + Ok(Self::StorageName(content.parse::()?)) } else { Err(lookahead.error()) } @@ -114,7 +114,7 @@ pub struct StorageDef { /// Optional getter to generate. If some then query_kind is ensured to be some as well. pub getter: Option, /// Optional expression that evaluates to a type that can be used as StoragePrefix instead of ident. - pub rename_as: Option, + pub rename_as: Option, /// Whereas the querytype of the storage is OptionQuery or ValueQuery. /// Note that this is best effort as it can't be determined when QueryKind is generic, and /// result can be false if user do some unexpected type alias. @@ -598,7 +598,7 @@ impl StorageDef { }); let rename_as = names.pop().map(|attr| { match attr { - PalletStorageAttr::StorageName(expr) => expr, + PalletStorageAttr::StorageName(lit) => lit, _ => unreachable!(), } }); diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index 7478da189df07..40da8da7dc135 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -198,6 +198,10 @@ pub mod pallet { #[pallet::storage] pub type Value = StorageValue; + #[pallet::storage] + #[pallet::storage_name = "Value2"] + pub type RenamedValue = StorageValue; + #[pallet::type_value] pub fn MyDefault() -> u16 where T::AccountId: From + From + SomeAssociation1 @@ -577,6 +581,10 @@ fn storage_expand() { let k = [twox_128(b"Example"), twox_128(b"Value")].concat(); assert_eq!(unhashed::get::(&k), Some(1u32)); + pallet::RenamedValue::::put(2); + let k = [twox_128(b"Example"), twox_128(b"Value2")].concat(); + assert_eq!(unhashed::get::(&k), Some(2)); + pallet::Map::::insert(1, 2); let mut k = [twox_128(b"Example"), twox_128(b"Map")].concat(); k.extend(1u8.using_encoded(blake2_128_concat)); @@ -697,6 +705,13 @@ fn metadata() { default: DecodeDifferent::Decoded(vec![0]), documentation: DecodeDifferent::Decoded(vec![]), }, + StorageEntryMetadata { + name: DecodeDifferent::Decoded("Value2".to_string()), + modifier: StorageEntryModifier::Optional, + ty: StorageEntryType::Plain(DecodeDifferent::Decoded("u64".to_string())), + default: DecodeDifferent::Decoded(vec![0]), + documentation: DecodeDifferent::Decoded(vec![]), + }, StorageEntryMetadata { name: DecodeDifferent::Decoded("Map".to_string()), modifier: StorageEntryModifier::Default, From c87e4d011e9d68b8fb3152aeef36db5f54d01e9f Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Tue, 8 Jun 2021 18:03:17 -0700 Subject: [PATCH 3/8] Fix test_storage_info --- frame/support/test/tests/pallet.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index 40da8da7dc135..ae15d3ba4bd56 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -1008,6 +1008,11 @@ fn test_storage_info() { max_values: Some(1), max_size: Some(4), }, + StorageInfo { + prefix: prefix(b"Example", b"Value2"), + max_values: Some(1), + max_size: Some(8), + }, StorageInfo { prefix: prefix(b"Example", b"Map"), max_values: None, From 9ea82517808524290eb6a582a8482f72246a0b8a Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Tue, 8 Jun 2021 19:35:47 -0700 Subject: [PATCH 4/8] Rename storage_name to storage_prefix --- frame/support/procedural/src/pallet/parse/storage.rs | 10 +++++----- frame/support/test/tests/pallet.rs | 2 +- .../test/tests/pallet_ui/storage_invalid_attribute.rs | 7 ++----- .../tests/pallet_ui/storage_invalid_attribute.stderr | 6 +++--- .../test/tests/pallet_ui/storage_multiple_getters.rs | 4 ++-- .../tests/pallet_ui/storage_multiple_getters.stderr | 2 +- .../test/tests/pallet_ui/storage_multiple_renames.rs | 4 ++-- .../tests/pallet_ui/storage_multiple_renames.stderr | 8 ++++---- 8 files changed, 20 insertions(+), 23 deletions(-) diff --git a/frame/support/procedural/src/pallet/parse/storage.rs b/frame/support/procedural/src/pallet/parse/storage.rs index 4a0b1fdab26cc..a999c6c35e2c0 100644 --- a/frame/support/procedural/src/pallet/parse/storage.rs +++ b/frame/support/procedural/src/pallet/parse/storage.rs @@ -25,14 +25,14 @@ mod keyword { syn::custom_keyword!(Error); syn::custom_keyword!(pallet); syn::custom_keyword!(getter); - syn::custom_keyword!(storage_name); + syn::custom_keyword!(storage_prefix); syn::custom_keyword!(OptionQuery); syn::custom_keyword!(ValueQuery); } /// Parse for one of the following: /// * `#[pallet::getter(fn dummy)]` -/// * `#[pallet::storage_name = "CustomName"]` +/// * `#[pallet::storage_prefix = "CustomName"]` pub enum PalletStorageAttr { Getter(syn::Ident), StorageName(syn::Lit), @@ -63,8 +63,8 @@ impl syn::parse::Parse for PalletStorageAttr { syn::parenthesized!(generate_content in content); generate_content.parse::()?; Ok(Self::Getter(generate_content.parse::()?)) - } else if lookahead.peek(keyword::storage_name) { - content.parse::()?; + } else if lookahead.peek(keyword::storage_prefix) { + content.parse::()?; content.parse::()?; Ok(Self::StorageName(content.parse::()?)) @@ -587,7 +587,7 @@ impl StorageDef { return Err(syn::Error::new(getters[1].span(), msg)); } if names.len() > 1 { - let msg = "Invalid pallet::storage, multiple argument pallet::storage_name found"; + let msg = "Invalid pallet::storage, multiple argument pallet::storage_prefix found"; return Err(syn::Error::new(names[1].span(), msg)); } let getter = getters.pop().map(|attr| { diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs index ae15d3ba4bd56..fbef663ef4c55 100644 --- a/frame/support/test/tests/pallet.rs +++ b/frame/support/test/tests/pallet.rs @@ -199,7 +199,7 @@ pub mod pallet { pub type Value = StorageValue; #[pallet::storage] - #[pallet::storage_name = "Value2"] + #[pallet::storage_prefix = "Value2"] pub type RenamedValue = StorageValue; #[pallet::type_value] diff --git a/frame/support/test/tests/pallet_ui/storage_invalid_attribute.rs b/frame/support/test/tests/pallet_ui/storage_invalid_attribute.rs index 82ed6610ba1a4..c6a88c083135d 100644 --- a/frame/support/test/tests/pallet_ui/storage_invalid_attribute.rs +++ b/frame/support/test/tests/pallet_ui/storage_invalid_attribute.rs @@ -7,16 +7,13 @@ mod pallet { pub trait Config: frame_system::Config {} #[pallet::pallet] - pub struct Pallet(core::marker::PhantomData); - - #[pallet::hooks] - impl Hooks> for Pallet {} + pub struct Pallet(_); #[pallet::call] impl Pallet {} #[pallet::storage] - #[pallet::generate_store(pub trait Store)] + #[pallet::generate_store(pub trait Store)] type Foo = StorageValue; } diff --git a/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr b/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr index fa7facba46e97..bf93d99cf56bd 100644 --- a/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr +++ b/frame/support/test/tests/pallet_ui/storage_invalid_attribute.stderr @@ -1,5 +1,5 @@ -error: expected `getter` or `storage_name` - --> $DIR/storage_invalid_attribute.rs:19:15 +error: expected `getter` or `storage_prefix` + --> $DIR/storage_invalid_attribute.rs:16:12 | -19 | #[pallet::generate_store(pub trait Store)] +16 | #[pallet::generate_store(pub trait Store)] | ^^^^^^^^^^^^^^ diff --git a/frame/support/test/tests/pallet_ui/storage_multiple_getters.rs b/frame/support/test/tests/pallet_ui/storage_multiple_getters.rs index 5c403371c5471..309b9b24136fa 100644 --- a/frame/support/test/tests/pallet_ui/storage_multiple_getters.rs +++ b/frame/support/test/tests/pallet_ui/storage_multiple_getters.rs @@ -16,8 +16,8 @@ mod pallet { impl Pallet {} #[pallet::storage] - #[pallet::getter(fn get_foo)] - #[pallet::getter(fn foo_error)] + #[pallet::getter(fn get_foo)] + #[pallet::getter(fn foo_error)] type Foo = StorageValue<_, u8>; } diff --git a/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr b/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr index 039f2b8282beb..46ea882a83f09 100644 --- a/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr +++ b/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr @@ -1,5 +1,5 @@ error: Invalid pallet::storage, multiple argument pallet::getter found - --> $DIR/storage_multiple_getters.rs:20:25 + --> $DIR/storage_multiple_getters.rs:20:22 | 20 | #[pallet::getter(fn foo_error)] | ^^^^^^^^^ diff --git a/frame/support/test/tests/pallet_ui/storage_multiple_renames.rs b/frame/support/test/tests/pallet_ui/storage_multiple_renames.rs index 0dd33a3b4c21f..f3caef80a7ee2 100644 --- a/frame/support/test/tests/pallet_ui/storage_multiple_renames.rs +++ b/frame/support/test/tests/pallet_ui/storage_multiple_renames.rs @@ -16,8 +16,8 @@ mod pallet { impl Pallet {} #[pallet::storage] - #[pallet::storage_name = "Bar"] - #[pallet::storage_name = "Baz"] + #[pallet::storage_prefix = "Bar"] + #[pallet::storage_prefix = "Baz"] type Foo = StorageValue<_, u8>; } diff --git a/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr b/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr index 8e7864973aa13..e8f058b4d094a 100644 --- a/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr +++ b/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr @@ -1,5 +1,5 @@ -error: Invalid pallet::storage, multiple argument pallet::storage_name found - --> $DIR/storage_multiple_renames.rs:20:30 +error: Invalid pallet::storage, multiple argument pallet::storage_prefix found + --> $DIR/storage_multiple_renames.rs:20:29 | -20 | #[pallet::storage_name = "Baz"] - | ^^^^^ +20 | #[pallet::storage_prefix = "Baz"] + | ^^^^^ From 9d6a9e8f2f84506675660426383db305c22783f5 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 11 Jun 2021 21:47:39 -0700 Subject: [PATCH 5/8] Check for duplicates when renaming storage prefixes --- .../procedural/src/pallet/expand/storage.rs | 55 +++++++++++-------- .../procedural/src/pallet/parse/storage.rs | 34 +++++++++++- .../pallet_ui/duplicate_storage_prefix.rs | 21 +++++++ .../pallet_ui/duplicate_storage_prefix.stderr | 17 ++++++ 4 files changed, 104 insertions(+), 23 deletions(-) create mode 100644 frame/support/test/tests/pallet_ui/duplicate_storage_prefix.rs create mode 100644 frame/support/test/tests/pallet_ui/duplicate_storage_prefix.stderr diff --git a/frame/support/procedural/src/pallet/expand/storage.rs b/frame/support/procedural/src/pallet/expand/storage.rs index 5e974799ba21b..d44489ad9f3a3 100644 --- a/frame/support/procedural/src/pallet/expand/storage.rs +++ b/frame/support/procedural/src/pallet/expand/storage.rs @@ -18,35 +18,46 @@ use crate::pallet::{Def, parse::storage::StorageDef}; use crate::pallet::parse::storage::{Metadata, QueryKind, StorageGenerics}; use frame_support_procedural_tools::clean_type_string; - -fn literal_to_string(lit: &syn::Lit) -> String { - match lit { - syn::Lit::Str(s) => s.value(), - syn::Lit::ByteStr(s) => - String::from_utf8(s.value()).expect("All byte strings are valid UTF-8 strings; qed"), - syn::Lit::Byte(b) => char::from(b.value()).to_string(), - syn::Lit::Char(c) => c.value().to_string(), - syn::Lit::Int(i) => i.base10_digits().to_owned(), - syn::Lit::Float(f) => f.base10_digits().to_owned(), - syn::Lit::Bool(b) => b.value.to_string(), - syn::Lit::Verbatim(l) => l.to_string(), - } -} +use std::collections::HashSet; /// Generate the prefix_ident related the the storage. /// prefix_ident is used for the prefix struct to be given to storage as first generic param. fn prefix_ident(storage: &StorageDef) -> syn::Ident { let storage_ident = &storage.ident; - let ident = storage.rename_as.as_ref().map(literal_to_string).unwrap_or(storage_ident.to_string()); + let ident = storage.prefix(); syn::Ident::new(&format!("_GeneratedPrefixForStorage{}", ident), storage_ident.span()) } +/// Check for duplicated storage prefixes. This step is necessary since users can specify an +/// alternative storage prefix using the #[pallet::storage_prefix] syntax, and we need to ensure +/// that the prefix specified by the user is not a duplicate of an existing one. +fn check_prefix_duplicates( + storage_def: &StorageDef, + set: &mut HashSet, +) -> syn::Result<()> { + let prefix = storage_def.prefix(); + + if !set.insert(prefix.clone()) { + let err = syn::Error::new( + storage_def.prefix_span(), + format!("Duplicate storage prefixes found for `{}`", prefix), + ); + return Err(err); + } + + Ok(()) +} + /// * if generics are unnamed: replace the first generic `_` by the generated prefix structure /// * if generics are named: reorder the generic, remove their name, and add the missing ones. /// * Add `#[allow(type_alias_bounds)]` -pub fn process_generics(def: &mut Def) { +pub fn process_generics(def: &mut Def) -> syn::Result<()> { let frame_support = &def.frame_support; + let mut prefix_set = HashSet::new(); + for storage_def in def.storages.iter_mut() { + check_prefix_duplicates(storage_def, &mut prefix_set)?; + let item = &mut def.item.content.as_mut().expect("Checked by def").1[storage_def.index]; let typ_item = match item { @@ -132,6 +143,8 @@ pub fn process_generics(def: &mut Def) { args.args[0] = syn::parse_quote!( #prefix_ident<#type_use_gen> ); } } + + Ok(()) } /// * generate StoragePrefix structs (e.g. for a storage `MyStorage` a struct with the name @@ -141,7 +154,9 @@ pub fn process_generics(def: &mut Def) { /// * Add `#[allow(type_alias_bounds)]` on storages type alias /// * generate metadatas pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream { - process_generics(def); + if let Err(e) = process_generics(def) { + return e.into_compile_error().into(); + } let frame_support = &def.frame_support; let frame_system = &def.frame_system; @@ -362,11 +377,7 @@ pub fn expand_storages(def: &mut Def) -> proc_macro2::TokenStream { let type_use_gen = &def.type_use_generics(storage_def.attr_span); let prefix_struct_ident = prefix_ident(&storage_def); let prefix_struct_vis = &storage_def.vis; - let prefix_struct_const = storage_def - .rename_as - .as_ref() - .map(literal_to_string) - .unwrap_or(storage_def.ident.to_string()); + let prefix_struct_const = storage_def.prefix(); let config_where_clause = &def.config.where_clause; let cfg_attrs = &storage_def.cfg_attrs; diff --git a/frame/support/procedural/src/pallet/parse/storage.rs b/frame/support/procedural/src/pallet/parse/storage.rs index a999c6c35e2c0..4d89ba970bbc1 100644 --- a/frame/support/procedural/src/pallet/parse/storage.rs +++ b/frame/support/procedural/src/pallet/parse/storage.rs @@ -131,7 +131,6 @@ pub struct StorageDef { pub named_generics: Option, } - /// The parsed generic from the #[derive(Clone)] pub enum StorageGenerics { @@ -203,6 +202,21 @@ enum StorageKind { NMap, } +/// Convert syn::Lit to its string representation +fn literal_to_string(lit: &syn::Lit) -> String { + match lit { + syn::Lit::Str(s) => s.value(), + syn::Lit::ByteStr(s) => + String::from_utf8(s.value()).expect("All byte strings are valid UTF-8 strings; qed"), + syn::Lit::Byte(b) => char::from(b.value()).to_string(), + syn::Lit::Char(c) => c.value().to_string(), + syn::Lit::Int(i) => i.base10_digits().to_owned(), + syn::Lit::Float(f) => f.base10_digits().to_owned(), + syn::Lit::Bool(b) => b.value.to_string(), + syn::Lit::Verbatim(l) => l.to_string(), + } +} + /// Check the generics in the `map` contains the generics in `gen` may contains generics in /// `optional_gen`, and doesn't contains any other. fn check_generics( @@ -567,6 +581,24 @@ fn extract_key(ty: &syn::Type) -> syn::Result { } impl StorageDef { + /// Return the storage prefix for this storage item + pub fn prefix(&self) -> String { + self + .rename_as + .as_ref() + .map(literal_to_string) + .unwrap_or(self.ident.to_string()) + } + + /// Return either the span of the ident or the span of the #[storage_prefix] attribute + pub fn prefix_span(&self) -> proc_macro2::Span { + self + .rename_as + .as_ref() + .map(syn::Lit::span) + .unwrap_or(self.ident.span()) + } + pub fn try_from( attr_span: proc_macro2::Span, index: usize, diff --git a/frame/support/test/tests/pallet_ui/duplicate_storage_prefix.rs b/frame/support/test/tests/pallet_ui/duplicate_storage_prefix.rs new file mode 100644 index 0000000000000..d103fa09d991b --- /dev/null +++ b/frame/support/test/tests/pallet_ui/duplicate_storage_prefix.rs @@ -0,0 +1,21 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::StorageValue; + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + #[pallet::generate_store(trait Store)] + pub struct Pallet(core::marker::PhantomData); + + #[pallet::storage] + type Foo = StorageValue<_, u8>; + + #[pallet::storage] + #[pallet::storage_prefix = "Foo"] + type NotFoo = StorageValue<_, u16>; +} + +fn main() { +} diff --git a/frame/support/test/tests/pallet_ui/duplicate_storage_prefix.stderr b/frame/support/test/tests/pallet_ui/duplicate_storage_prefix.stderr new file mode 100644 index 0000000000000..63a6e71e44045 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/duplicate_storage_prefix.stderr @@ -0,0 +1,17 @@ +error: Duplicate storage prefixes found for `Foo` + --> $DIR/duplicate_storage_prefix.rs:16:32 + | +16 | #[pallet::storage_prefix = "Foo"] + | ^^^^^ + +error[E0412]: cannot find type `_GeneratedPrefixForStorageFoo` in this scope + --> $DIR/duplicate_storage_prefix.rs:13:7 + | +13 | type Foo = StorageValue<_, u8>; + | ^^^ not found in this scope + +error[E0121]: the type placeholder `_` is not allowed within types on item signatures + --> $DIR/duplicate_storage_prefix.rs:17:35 + | +17 | type NotFoo = StorageValue<_, u16>; + | ^ not allowed in type signatures From 34fbf929048fcfd508792f06eda5d84baa0ae2dc Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 11 Jun 2021 22:42:32 -0700 Subject: [PATCH 6/8] Allow only string literals for storage_prefix renames --- .../procedural/src/pallet/parse/storage.rs | 25 ++++--------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/frame/support/procedural/src/pallet/parse/storage.rs b/frame/support/procedural/src/pallet/parse/storage.rs index 4d89ba970bbc1..2829dee719bc9 100644 --- a/frame/support/procedural/src/pallet/parse/storage.rs +++ b/frame/support/procedural/src/pallet/parse/storage.rs @@ -35,7 +35,7 @@ mod keyword { /// * `#[pallet::storage_prefix = "CustomName"]` pub enum PalletStorageAttr { Getter(syn::Ident), - StorageName(syn::Lit), + StorageName(syn::LitStr), } impl PalletStorageAttr { @@ -67,7 +67,7 @@ impl syn::parse::Parse for PalletStorageAttr { content.parse::()?; content.parse::()?; - Ok(Self::StorageName(content.parse::()?)) + Ok(Self::StorageName(content.parse::()?)) } else { Err(lookahead.error()) } @@ -114,7 +114,7 @@ pub struct StorageDef { /// Optional getter to generate. If some then query_kind is ensured to be some as well. pub getter: Option, /// Optional expression that evaluates to a type that can be used as StoragePrefix instead of ident. - pub rename_as: Option, + pub rename_as: Option, /// Whereas the querytype of the storage is OptionQuery or ValueQuery. /// Note that this is best effort as it can't be determined when QueryKind is generic, and /// result can be false if user do some unexpected type alias. @@ -202,21 +202,6 @@ enum StorageKind { NMap, } -/// Convert syn::Lit to its string representation -fn literal_to_string(lit: &syn::Lit) -> String { - match lit { - syn::Lit::Str(s) => s.value(), - syn::Lit::ByteStr(s) => - String::from_utf8(s.value()).expect("All byte strings are valid UTF-8 strings; qed"), - syn::Lit::Byte(b) => char::from(b.value()).to_string(), - syn::Lit::Char(c) => c.value().to_string(), - syn::Lit::Int(i) => i.base10_digits().to_owned(), - syn::Lit::Float(f) => f.base10_digits().to_owned(), - syn::Lit::Bool(b) => b.value.to_string(), - syn::Lit::Verbatim(l) => l.to_string(), - } -} - /// Check the generics in the `map` contains the generics in `gen` may contains generics in /// `optional_gen`, and doesn't contains any other. fn check_generics( @@ -586,7 +571,7 @@ impl StorageDef { self .rename_as .as_ref() - .map(literal_to_string) + .map(syn::LitStr::value) .unwrap_or(self.ident.to_string()) } @@ -595,7 +580,7 @@ impl StorageDef { self .rename_as .as_ref() - .map(syn::Lit::span) + .map(syn::LitStr::span) .unwrap_or(self.ident.span()) } From fc49baf24546f80e935510e101bfe1c7b5b3911a Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Sat, 12 Jun 2021 02:18:48 -0700 Subject: [PATCH 7/8] Use proper spans for attribute errors --- .../procedural/src/pallet/parse/storage.rs | 27 ++++++++++--------- .../pallet_ui/storage_multiple_getters.stderr | 4 +-- .../pallet_ui/storage_multiple_renames.stderr | 4 +-- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/frame/support/procedural/src/pallet/parse/storage.rs b/frame/support/procedural/src/pallet/parse/storage.rs index 2829dee719bc9..41f063d7197e0 100644 --- a/frame/support/procedural/src/pallet/parse/storage.rs +++ b/frame/support/procedural/src/pallet/parse/storage.rs @@ -34,15 +34,14 @@ mod keyword { /// * `#[pallet::getter(fn dummy)]` /// * `#[pallet::storage_prefix = "CustomName"]` pub enum PalletStorageAttr { - Getter(syn::Ident), - StorageName(syn::LitStr), + Getter(syn::Ident, proc_macro2::Span), + StorageName(syn::LitStr, proc_macro2::Span), } impl PalletStorageAttr { - fn span(&self) -> proc_macro2::Span { + fn attr_span(&self) -> proc_macro2::Span { match self { - Self::Getter(ident) => ident.span(), - Self::StorageName(lit) => lit.span(), + Self::Getter(_, span) | Self::StorageName(_, span) => *span, } } } @@ -50,6 +49,7 @@ impl PalletStorageAttr { impl syn::parse::Parse for PalletStorageAttr { fn parse(input: syn::parse::ParseStream) -> syn::Result { input.parse::()?; + let attr_span = input.span(); let content; syn::bracketed!(content in input); content.parse::()?; @@ -62,12 +62,12 @@ impl syn::parse::Parse for PalletStorageAttr { let generate_content; syn::parenthesized!(generate_content in content); generate_content.parse::()?; - Ok(Self::Getter(generate_content.parse::()?)) + Ok(Self::Getter(generate_content.parse::()?, attr_span)) } else if lookahead.peek(keyword::storage_prefix) { content.parse::()?; content.parse::()?; - Ok(Self::StorageName(content.parse::()?)) + Ok(Self::StorageName(content.parse::()?, attr_span)) } else { Err(lookahead.error()) } @@ -575,7 +575,8 @@ impl StorageDef { .unwrap_or(self.ident.to_string()) } - /// Return either the span of the ident or the span of the #[storage_prefix] attribute + /// Return either the span of the ident or the span of the literal in the + /// #[storage_prefix] attribute pub fn prefix_span(&self) -> proc_macro2::Span { self .rename_as @@ -598,24 +599,24 @@ impl StorageDef { let attrs: Vec = helper::take_item_pallet_attrs(&mut item.attrs)?; let (mut getters, mut names) = attrs .into_iter() - .partition::, _>(|attr| matches!(attr, PalletStorageAttr::Getter(_))); + .partition::, _>(|attr| matches!(attr, PalletStorageAttr::Getter(..))); if getters.len() > 1 { let msg = "Invalid pallet::storage, multiple argument pallet::getter found"; - return Err(syn::Error::new(getters[1].span(), msg)); + return Err(syn::Error::new(getters[1].attr_span(), msg)); } if names.len() > 1 { let msg = "Invalid pallet::storage, multiple argument pallet::storage_prefix found"; - return Err(syn::Error::new(names[1].span(), msg)); + return Err(syn::Error::new(names[1].attr_span(), msg)); } let getter = getters.pop().map(|attr| { match attr { - PalletStorageAttr::Getter(ident) => ident, + PalletStorageAttr::Getter(ident, _) => ident, _ => unreachable!(), } }); let rename_as = names.pop().map(|attr| { match attr { - PalletStorageAttr::StorageName(lit) => lit, + PalletStorageAttr::StorageName(lit, _) => lit, _ => unreachable!(), } }); diff --git a/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr b/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr index 46ea882a83f09..188eed3cb0d17 100644 --- a/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr +++ b/frame/support/test/tests/pallet_ui/storage_multiple_getters.stderr @@ -1,5 +1,5 @@ error: Invalid pallet::storage, multiple argument pallet::getter found - --> $DIR/storage_multiple_getters.rs:20:22 + --> $DIR/storage_multiple_getters.rs:20:3 | 20 | #[pallet::getter(fn foo_error)] - | ^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr b/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr index e8f058b4d094a..9288d131d95af 100644 --- a/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr +++ b/frame/support/test/tests/pallet_ui/storage_multiple_renames.stderr @@ -1,5 +1,5 @@ error: Invalid pallet::storage, multiple argument pallet::storage_prefix found - --> $DIR/storage_multiple_renames.rs:20:29 + --> $DIR/storage_multiple_renames.rs:20:3 | 20 | #[pallet::storage_prefix = "Baz"] - | ^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From b8895a2daff81f7ce88e78291a97bb089c9efd11 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Sat, 12 Jun 2021 02:54:48 -0700 Subject: [PATCH 8/8] Check for valid identifiers when parsing storage prefix renames --- .../procedural/src/pallet/expand/storage.rs | 3 +-- .../procedural/src/pallet/parse/storage.rs | 10 +++++++++- .../pallet_ui/storage_invalid_rename_value.rs | 18 ++++++++++++++++++ .../storage_invalid_rename_value.stderr | 5 +++++ 4 files changed, 33 insertions(+), 3 deletions(-) create mode 100644 frame/support/test/tests/pallet_ui/storage_invalid_rename_value.rs create mode 100644 frame/support/test/tests/pallet_ui/storage_invalid_rename_value.stderr diff --git a/frame/support/procedural/src/pallet/expand/storage.rs b/frame/support/procedural/src/pallet/expand/storage.rs index d44489ad9f3a3..0000051dd9b94 100644 --- a/frame/support/procedural/src/pallet/expand/storage.rs +++ b/frame/support/procedural/src/pallet/expand/storage.rs @@ -24,8 +24,7 @@ use std::collections::HashSet; /// prefix_ident is used for the prefix struct to be given to storage as first generic param. fn prefix_ident(storage: &StorageDef) -> syn::Ident { let storage_ident = &storage.ident; - let ident = storage.prefix(); - syn::Ident::new(&format!("_GeneratedPrefixForStorage{}", ident), storage_ident.span()) + syn::Ident::new(&format!("_GeneratedPrefixForStorage{}", storage_ident), storage_ident.span()) } /// Check for duplicated storage prefixes. This step is necessary since users can specify an diff --git a/frame/support/procedural/src/pallet/parse/storage.rs b/frame/support/procedural/src/pallet/parse/storage.rs index 41f063d7197e0..9ec890e66e57a 100644 --- a/frame/support/procedural/src/pallet/parse/storage.rs +++ b/frame/support/procedural/src/pallet/parse/storage.rs @@ -67,7 +67,15 @@ impl syn::parse::Parse for PalletStorageAttr { content.parse::()?; content.parse::()?; - Ok(Self::StorageName(content.parse::()?, attr_span)) + let renamed_prefix = content.parse::()?; + // Ensure the renamed prefix is a proper Rust identifier + syn::parse_str::(&renamed_prefix.value()) + .map_err(|_| { + let msg = format!("`{}` is not a valid identifier", renamed_prefix.value()); + syn::Error::new(renamed_prefix.span(), msg) + })?; + + Ok(Self::StorageName(renamed_prefix, attr_span)) } else { Err(lookahead.error()) } diff --git a/frame/support/test/tests/pallet_ui/storage_invalid_rename_value.rs b/frame/support/test/tests/pallet_ui/storage_invalid_rename_value.rs new file mode 100644 index 0000000000000..c3a08e05e2ac7 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/storage_invalid_rename_value.rs @@ -0,0 +1,18 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::Hooks; + use frame_system::pallet_prelude::BlockNumberFor; + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(core::marker::PhantomData); + + #[pallet::storage] + #[pallet::storage_prefix = "pub"] + type Foo = StorageValue<_, u8>; +} + +fn main() { +} diff --git a/frame/support/test/tests/pallet_ui/storage_invalid_rename_value.stderr b/frame/support/test/tests/pallet_ui/storage_invalid_rename_value.stderr new file mode 100644 index 0000000000000..513970f98a4f7 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/storage_invalid_rename_value.stderr @@ -0,0 +1,5 @@ +error: `pub` is not a valid identifier + --> $DIR/storage_invalid_rename_value.rs:13:29 + | +13 | #[pallet::storage_prefix = "pub"] + | ^^^^^