Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Uniform pallet warnings #13798

Merged
merged 8 commits into from
Apr 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions frame/benchmarking/pov/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,14 @@ pub mod pallet {
#[pallet::call]
impl<T: Config> Pallet<T> {
#[pallet::call_index(0)]
#[pallet::weight(0)]
#[pallet::weight({0})]
Copy link
Member Author

@ggwpez ggwpez Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Some test-only pallets cannot be put in dev mode, since they access the storage info.
But we could add custom lint attributes to suppress certain warnings, similar to how clippy works.

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(())
}
Expand Down
2 changes: 1 addition & 1 deletion frame/benchmarking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down
4 changes: 2 additions & 2 deletions frame/benchmarking/src/tests_instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
Expand Down
2 changes: 1 addition & 1 deletion frame/collective/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down
6 changes: 3 additions & 3 deletions frame/examples/offchain-worker/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand All @@ -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,
Expand All @@ -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>,
Expand Down
2 changes: 1 addition & 1 deletion frame/executive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,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::*;
Expand Down
14 changes: 7 additions & 7 deletions frame/membership/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand All @@ -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)?;
Expand All @@ -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>,
Expand Down Expand Up @@ -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)?;

Expand All @@ -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)?;
Expand Down Expand Up @@ -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)?;
Expand All @@ -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();
Expand Down
8 changes: 4 additions & 4 deletions frame/nicks/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;

Expand Down Expand Up @@ -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)?;

Expand All @@ -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)?;

Expand All @@ -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>,
Expand Down
12 changes: 6 additions & 6 deletions frame/scored-pool/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
//!
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)?;

Expand All @@ -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>,
Expand All @@ -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>,
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion frame/sudo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
1 change: 1 addition & 0 deletions frame/support/procedural/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down
54 changes: 36 additions & 18 deletions frame/support/procedural/src/pallet/expand/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<_>>();

Expand Down Expand Up @@ -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
)*
}

Expand Down
8 changes: 4 additions & 4 deletions frame/support/test/tests/pallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
}
Expand Down
Loading