Uniform pallet warnings (#13798)

* Use proc-macro-warning crate Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Fixup Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Fix pallet_ui tests Also renamed some of the odd-named ones. Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Update dep Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Ignore hardcoded weight warning To be fixed in https://github.com/paritytech/substrate/issues/13813 Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Fix test pallet Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Fix more tests Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> --------- Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Co-authored-by: parity-processbot <>
paritytech · Apr 4, 2023 · 2961138 · 2961138
1 parent 842e651
commit 2961138
Show file tree

Hide file tree

Showing 30 changed files with 299 additions and 75 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/frame/benchmarking/pov/src/lib.rs b/frame/benchmarking/pov/src/lib.rs
@@ -120,14 +120,14 @@ pub mod pallet {
 	#[pallet::call]
 	impl<T: Config> Pallet<T> {
 		#[pallet::call_index(0)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn emit_event(_origin: OriginFor<T>) -> DispatchResult {
 			Self::deposit_event(Event::TestEvent);
 			Ok(())
 		}
 
 		#[pallet::call_index(1)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn noop(_origin: OriginFor<T>) -> DispatchResult {
 			Ok(())
 		}

diff --git a/frame/benchmarking/src/tests.rs b/frame/benchmarking/src/tests.rs
@@ -29,7 +29,7 @@ use sp_runtime::{
 use sp_std::prelude::*;
 use std::cell::RefCell;
 
-#[frame_support::pallet]
+#[frame_support::pallet(dev_mode)]
 mod pallet_test {
 	use frame_support::pallet_prelude::*;
 	use frame_system::pallet_prelude::*;

diff --git a/frame/benchmarking/src/tests_instance.rs b/frame/benchmarking/src/tests_instance.rs
@@ -61,15 +61,15 @@ mod pallet_test {
 		<T as OtherConfig>::OtherEvent: Into<<T as Config<I>>::RuntimeEvent>,
 	{
 		#[pallet::call_index(0)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn set_value(origin: OriginFor<T>, n: u32) -> DispatchResult {
 			let _sender = ensure_signed(origin)?;
 			Value::<T, I>::put(n);
 			Ok(())
 		}
 
 		#[pallet::call_index(1)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn dummy(origin: OriginFor<T>, _n: u32) -> DispatchResult {
 			let _sender = ensure_none(origin)?;
 			Ok(())

diff --git a/frame/collective/src/tests.rs b/frame/collective/src/tests.rs
@@ -51,7 +51,7 @@ frame_support::construct_runtime!(
 
 mod mock_democracy {
 	pub use pallet::*;
-	#[frame_support::pallet]
+	#[frame_support::pallet(dev_mode)]
 	pub mod pallet {
 		use frame_support::pallet_prelude::*;
 		use frame_system::pallet_prelude::*;

diff --git a/frame/examples/offchain-worker/src/lib.rs b/frame/examples/offchain-worker/src/lib.rs
@@ -229,7 +229,7 @@ pub mod pallet {
 		/// This example is not focused on correctness of the oracle itself, but rather its
 		/// purpose is to showcase offchain worker capabilities.
 		#[pallet::call_index(0)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn submit_price(origin: OriginFor<T>, price: u32) -> DispatchResultWithPostInfo {
 			// Retrieve sender of the transaction.
 			let who = ensure_signed(origin)?;
@@ -255,7 +255,7 @@ pub mod pallet {
 		/// This example is not focused on correctness of the oracle itself, but rather its
 		/// purpose is to showcase offchain worker capabilities.
 		#[pallet::call_index(1)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn submit_price_unsigned(
 			origin: OriginFor<T>,
 			_block_number: T::BlockNumber,
@@ -272,7 +272,7 @@ pub mod pallet {
 		}
 
 		#[pallet::call_index(2)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn submit_price_unsigned_with_signed_payload(
 			origin: OriginFor<T>,
 			price_payload: PricePayload<T::Public, T::BlockNumber>,

diff --git a/frame/executive/src/lib.rs b/frame/executive/src/lib.rs
@@ -694,7 +694,7 @@ mod tests {
 
 	const TEST_KEY: &[u8] = b":test:key:";
 
-	#[frame_support::pallet]
+	#[frame_support::pallet(dev_mode)]
 	mod custom {
 		use frame_support::pallet_prelude::*;
 		use frame_system::pallet_prelude::*;

diff --git a/frame/membership/src/lib.rs b/frame/membership/src/lib.rs
@@ -168,7 +168,7 @@ pub mod pallet {
 		///
 		/// May only be called from `T::AddOrigin`.
 		#[pallet::call_index(0)]
-		#[pallet::weight(50_000_000)]
+		#[pallet::weight({50_000_000})]
 		pub fn add_member(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
 			T::AddOrigin::ensure_origin(origin)?;
 			let who = T::Lookup::lookup(who)?;
@@ -191,7 +191,7 @@ pub mod pallet {
 		///
 		/// May only be called from `T::RemoveOrigin`.
 		#[pallet::call_index(1)]
-		#[pallet::weight(50_000_000)]
+		#[pallet::weight({50_000_000})]
 		pub fn remove_member(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
 			T::RemoveOrigin::ensure_origin(origin)?;
 			let who = T::Lookup::lookup(who)?;
@@ -215,7 +215,7 @@ pub mod pallet {
 		///
 		/// Prime membership is *not* passed from `remove` to `add`, if extant.
 		#[pallet::call_index(2)]
-		#[pallet::weight(50_000_000)]
+		#[pallet::weight({50_000_000})]
 		pub fn swap_member(
 			origin: OriginFor<T>,
 			remove: AccountIdLookupOf<T>,
@@ -249,7 +249,7 @@ pub mod pallet {
 		///
 		/// May only be called from `T::ResetOrigin`.
 		#[pallet::call_index(3)]
-		#[pallet::weight(50_000_000)]
+		#[pallet::weight({50_000_000})]
 		pub fn reset_members(origin: OriginFor<T>, members: Vec<T::AccountId>) -> DispatchResult {
 			T::ResetOrigin::ensure_origin(origin)?;
 
@@ -272,7 +272,7 @@ pub mod pallet {
 		///
 		/// Prime membership is passed from the origin account to `new`, if extant.
 		#[pallet::call_index(4)]
-		#[pallet::weight(50_000_000)]
+		#[pallet::weight({50_000_000})]
 		pub fn change_key(origin: OriginFor<T>, new: AccountIdLookupOf<T>) -> DispatchResult {
 			let remove = ensure_signed(origin)?;
 			let new = T::Lookup::lookup(new)?;
@@ -307,7 +307,7 @@ pub mod pallet {
 		///
 		/// May only be called from `T::PrimeOrigin`.
 		#[pallet::call_index(5)]
-		#[pallet::weight(50_000_000)]
+		#[pallet::weight({50_000_000})]
 		pub fn set_prime(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
 			T::PrimeOrigin::ensure_origin(origin)?;
 			let who = T::Lookup::lookup(who)?;
@@ -321,7 +321,7 @@ pub mod pallet {
 		///
 		/// May only be called from `T::PrimeOrigin`.
 		#[pallet::call_index(6)]
-		#[pallet::weight(50_000_000)]
+		#[pallet::weight({50_000_000})]
 		pub fn clear_prime(origin: OriginFor<T>) -> DispatchResult {
 			T::PrimeOrigin::ensure_origin(origin)?;
 			Prime::<T, I>::kill();

diff --git a/frame/nicks/src/lib.rs b/frame/nicks/src/lib.rs
@@ -131,7 +131,7 @@ pub mod pallet {
 		/// ## Complexity
 		/// - O(1).
 		#[pallet::call_index(0)]
-		#[pallet::weight(50_000_000)]
+		#[pallet::weight({50_000_000})]
 		pub fn set_name(origin: OriginFor<T>, name: Vec<u8>) -> DispatchResult {
 			let sender = ensure_signed(origin)?;
 
@@ -160,7 +160,7 @@ pub mod pallet {
 		/// ## Complexity
 		/// - O(1).
 		#[pallet::call_index(1)]
-		#[pallet::weight(70_000_000)]
+		#[pallet::weight({70_000_000})]
 		pub fn clear_name(origin: OriginFor<T>) -> DispatchResult {
 			let sender = ensure_signed(origin)?;
 
@@ -183,7 +183,7 @@ pub mod pallet {
 		/// ## Complexity
 		/// - O(1).
 		#[pallet::call_index(2)]
-		#[pallet::weight(70_000_000)]
+		#[pallet::weight({70_000_000})]
 		pub fn kill_name(origin: OriginFor<T>, target: AccountIdLookupOf<T>) -> DispatchResult {
 			T::ForceOrigin::ensure_origin(origin)?;
 
@@ -207,7 +207,7 @@ pub mod pallet {
 		/// ## Complexity
 		/// - O(1).
 		#[pallet::call_index(3)]
-		#[pallet::weight(70_000_000)]
+		#[pallet::weight({70_000_000})]
 		pub fn force_name(
 			origin: OriginFor<T>,
 			target: AccountIdLookupOf<T>,

diff --git a/frame/scored-pool/src/lib.rs b/frame/scored-pool/src/lib.rs
@@ -70,7 +70,7 @@
 //!
 //! 	#[pallet::call]
 //! 	impl<T: Config> Pallet<T> {
-//! 		#[pallet::weight(0)]
+//! 		#[pallet::weight({0})]
 //! 		pub fn candidate(origin: OriginFor<T>) -> DispatchResult {
 //! 			let who = ensure_signed(origin)?;
 //!
@@ -311,7 +311,7 @@ pub mod pallet {
 		/// The `index` parameter of this function must be set to
 		/// the index of the transactor in the `Pool`.
 		#[pallet::call_index(0)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn submit_candidacy(origin: OriginFor<T>) -> DispatchResult {
 			let who = ensure_signed(origin)?;
 			ensure!(!<CandidateExists<T, I>>::contains_key(&who), Error::<T, I>::AlreadyInPool);
@@ -341,7 +341,7 @@ pub mod pallet {
 		/// The `index` parameter of this function must be set to
 		/// the index of the transactor in the `Pool`.
 		#[pallet::call_index(1)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn withdraw_candidacy(origin: OriginFor<T>, index: u32) -> DispatchResult {
 			let who = ensure_signed(origin)?;
 
@@ -360,7 +360,7 @@ pub mod pallet {
 		/// The `index` parameter of this function must be set to
 		/// the index of `dest` in the `Pool`.
 		#[pallet::call_index(2)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn kick(
 			origin: OriginFor<T>,
 			dest: AccountIdLookupOf<T>,
@@ -385,7 +385,7 @@ pub mod pallet {
 		/// The `index` parameter of this function must be set to
 		/// the index of the `dest` in the `Pool`.
 		#[pallet::call_index(3)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn score(
 			origin: OriginFor<T>,
 			dest: AccountIdLookupOf<T>,
@@ -425,7 +425,7 @@ pub mod pallet {
 		///
 		/// May only be called from root.
 		#[pallet::call_index(4)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})]
 		pub fn change_member_count(origin: OriginFor<T>, count: u32) -> DispatchResult {
 			ensure_root(origin)?;
 			Self::update_member_count(count).map_err(Into::into)

diff --git a/frame/sudo/src/lib.rs b/frame/sudo/src/lib.rs
@@ -195,7 +195,7 @@ pub mod pallet {
 		/// ## Complexity
 		/// - O(1).
 		#[pallet::call_index(2)]
-		#[pallet::weight(0)]
+		#[pallet::weight({0})] // FIXME
 		pub fn set_key(
 			origin: OriginFor<T>,
 			new: AccountIdLookupOf<T>,

diff --git a/frame/support/procedural/Cargo.toml b/frame/support/procedural/Cargo.toml
@@ -23,6 +23,7 @@ proc-macro2 = "1.0.37"
 quote = "1.0.10"
 syn = { version = "1.0.98", features = ["full"] }
 frame-support-procedural-tools = { version = "4.0.0-dev", path = "./tools" }
+proc-macro-warning = { version = "0.2.0", default-features = false }
 
 [features]
 default = ["std"]

diff --git a/frame/support/procedural/src/pallet/expand/call.rs b/frame/support/procedural/src/pallet/expand/call.rs
@@ -54,30 +54,47 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream {
 		.map(|fn_name| format!("Create a call with the variant `{}`.", fn_name))
 		.collect::<Vec<_>>();
 
-	let mut warning_structs = Vec::new();
-	let mut warning_names = Vec::new();
+	let mut call_index_warnings = Vec::new();
 	// Emit a warning for each call that is missing `call_index` when not in dev-mode.
 	for method in &methods {
 		if method.explicit_call_index || def.dev_mode {
 			continue
 		}
 
-		let name = syn::Ident::new(&format!("{}", method.name), method.name.span());
-		let warning: syn::ItemStruct = syn::parse_quote!(
-			#[deprecated(note = r"
-			Implicit call indices are deprecated in favour of explicit ones.
-			Please ensure that all calls have the `pallet::call_index` attribute or that the
-			`dev-mode` of the pallet is enabled. For more info see:
-			<https://github.com/paritytech/substrate/pull/12891> and
-			<https://github.com/paritytech/substrate/pull/11381>.")]
-			#[allow(non_camel_case_types)]
-			struct #name;
-		);
-		warning_names.push(name);
-		warning_structs.push(warning);
+		let warning = proc_macro_warning::Warning::new_deprecated("ImplicitCallIndex")
+			.index(call_index_warnings.len())
+			.old("use implicit call indices")
+			.new("ensure that all calls have a `pallet::call_index` attribute or put the pallet into `dev` mode")
+			.help_links(&[
+				"https://github.com/paritytech/substrate/pull/12891",
+				"https://github.com/paritytech/substrate/pull/11381"
+			])
+			.span(method.name.span())
+			.build();
+		call_index_warnings.push(warning);
 	}
 
 	let fn_weight = methods.iter().map(|method| &method.weight);
+	let mut weight_warnings = Vec::new();
+	for weight in fn_weight.clone() {
+		if def.dev_mode {
+			continue
+		}
+
+		match weight {
+			syn::Expr::Lit(lit) => {
+				let warning = proc_macro_warning::Warning::new_deprecated("ConstantWeight")
+					.index(weight_warnings.len())
+					.old("use hard-coded constant as call weight")
+					.new("benchmark all calls or put the pallet into `dev` mode")
+					.help_link("https://github.com/paritytech/substrate/pull/13798")
+					.span(lit.span())
+					.build();
+				weight_warnings.push(warning);
+			},
+			_ => {},
+		}
+	}
 
 	let fn_doc = methods.iter().map(|method| &method.docs).collect::<Vec<_>>();
 
@@ -203,9 +220,10 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream {
 	quote::quote_spanned!(span =>
 		mod warnings {
 			#(
-				#warning_structs
-				// This triggers each deprecated warning once.
-				const _: Option<#warning_names> = None;
+				#call_index_warnings
+			)*
+			#(
+				#weight_warnings
 			)*
 		}
 

diff --git a/frame/support/test/tests/pallet.rs b/frame/support/test/tests/pallet.rs
@@ -218,7 +218,7 @@ pub mod pallet {
 
 		/// Doc comment put in metadata
 		#[pallet::call_index(1)]
-		#[pallet::weight(1)]
+		#[pallet::weight({1})]
 		pub fn foo_storage_layer(
 			_origin: OriginFor<T>,
 			#[pallet::compact] foo: u32,
@@ -232,20 +232,20 @@ pub mod pallet {
 		}
 
 		#[pallet::call_index(4)]
-		#[pallet::weight(1)]
+		#[pallet::weight({1})]
 		pub fn foo_index_out_of_order(_origin: OriginFor<T>) -> DispatchResult {
 			Ok(())
 		}
 
 		// Test for DispatchResult return type
 		#[pallet::call_index(2)]
-		#[pallet::weight(1)]
+		#[pallet::weight({1})]
 		pub fn foo_no_post_info(_origin: OriginFor<T>) -> DispatchResult {
 			Ok(())
 		}
 
 		#[pallet::call_index(3)]
-		#[pallet::weight(1)]
+		#[pallet::weight({1})]
 		pub fn check_for_dispatch_context(_origin: OriginFor<T>) -> DispatchResult {
 			with_context::<(), _>(|_| ()).ok_or_else(|| DispatchError::Unavailable)
 		}