From 4b2ca118666ded0d55d8a6c1657320658d3395d9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tom=C3=A1s=20Senovilla=20Polo?=
<117524919+tsenovilla@users.noreply.github.com>
Date: Fri, 14 Feb 2025 12:51:25 +0100
Subject: [PATCH] refactor: Move `T:Config` into where clause in
`#[benchmarks]` macro if needed (#7418)
# Description
Currently, the `#[benchmarks]` macro always add `<T:Config>` to the
expanded code even if a where clause is used. Using a where clause which
also includes a trait bound for the generic `T` is triggering [this
clippy
warning](https://rust-lang.github.io/rust-clippy/master/index.html#multiple_bound_locations)
from Rust 1.78 onwards. We've found that
[here](https://github.com/freeverseio/laos/blob/main/pallets/precompiles-benchmark/src/precompiles/vesting/benchmarking.rs#L126-L132)
in LAOS, as we need to include `T: pallet_vesting::Config` in the where
clause, here's the outcome:
```rust
error: bound is defined in more than one place
--> pallets/precompiles-benchmark/src/precompiles/vesting/benchmarking.rs:130:1
|
130 | / #[benchmarks(
131 | | where
132 | | T: Config + pallet_vesting::Config,
| | ^
133 | | T::AccountIdToH160: ConvertBack<T::AccountId, H160>,
134 | | BalanceOf<T>: Into<U256>,
135 | | BlockNumberFor<T>: Into<U256>
136 | | )]
| |__^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#multiple_bound_locations
= note: `-D clippy::multiple-bound-locations` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::multiple_bound_locations)]`
= note: this error originates in the attribute macro `benchmarks` (in Nightly builds, run with -Z macro-backtrace for more info)
```
While this is a harmless warning, only thrown due to a trait bound for T
is being defined twice in an expanded code that nobody will see, and
while annotating the benchmarks module with
`#[allow(clippy::multiple_bound_locations)]` is enough to get rid of it,
it might cause unnecessary concerns.
Hence, I think it's worth slightly modifying the macro to avoid this.
## Review Notes
What I propose is to include `<T: Config>` (or its instance version) in
the expanded code only if no where clause was specified, and include
that trait bound in the where clause if one is present.
I considered always creating a where clause which includes `<T: Config>`
even if the macro doesn't specify a where clause and totally getting rid
of `<T: Config>`, but discarded the idea for simplicity.
I also considered checking if `T:Config` is present in the provided
where clause (as it's the case in the LAOS example I provided) before
adding it, but discarded the idea as well due to it implies a bit more
computation and having `T:Config` defined twice in the where clause is
harmless: the compiler will ignore the second occurrence and nobody will
see it's there.
If you think this change is worth it and one of the discarded ideas
would be a better approach, I'm happy to push that code instead.
---
prdoc/pr_7418.prdoc | 7 +++
.../frame/support/procedural/src/benchmark.rs | 52 +++++++++++--------
2 files changed, 36 insertions(+), 23 deletions(-)
create mode 100644 prdoc/pr_7418.prdoc
diff --git a/prdoc/pr_7418.prdoc b/prdoc/pr_7418.prdoc
new file mode 100644
index 00000000000..15e47e2525c
--- /dev/null
+++ b/prdoc/pr_7418.prdoc
@@ -0,0 +1,7 @@
+title: Refactor #[benchmarks] macro to don't define trait bounds twice
+doc:
+- audience: Runtime Dev
+ description: 'This PR contains a small refactor in the logic of #[benchmarks] so if a where clause is included the expanded code set the bound T:Config inside the where clause'
+crates:
+- name: frame-support-procedural
+ bump: patch
diff --git a/substrate/frame/support/procedural/src/benchmark.rs b/substrate/frame/support/procedural/src/benchmark.rs
index 6ba7ad05829..967eceb884a 100644
--- a/substrate/frame/support/procedural/src/benchmark.rs
+++ b/substrate/frame/support/procedural/src/benchmark.rs
@@ -30,7 +30,7 @@ use syn::{
token::{Comma, Gt, Lt, PathSep},
Attribute, Error, Expr, ExprBlock, ExprCall, ExprPath, FnArg, Item, ItemFn, ItemMod, Pat, Path,
PathArguments, PathSegment, Result, ReturnType, Signature, Stmt, Token, Type, TypePath,
- Visibility, WhereClause,
+ Visibility, WhereClause, WherePredicate,
};
mod keywords {
@@ -481,8 +481,25 @@ pub fn benchmarks(
let module: ItemMod = syn::parse(tokens)?;
let mod_span = module.span();
let where_clause = match syn::parse::<Nothing>(attrs.clone()) {
- Ok(_) => quote!(),
- Err(_) => syn::parse::<WhereClause>(attrs)?.predicates.to_token_stream(),
+ Ok(_) =>
+ if instance {
+ quote!(T: Config<I>, I: 'static)
+ } else {
+ quote!(T: Config)
+ },
+ Err(_) => {
+ let mut where_clause_predicates = syn::parse::<WhereClause>(attrs)?.predicates;
+
+ // Ensure the where clause contains the Config trait bound
+ if instance {
+ where_clause_predicates.push(syn::parse_str::<WherePredicate>("T: Config<I>")?);
+ where_clause_predicates.push(syn::parse_str::<WherePredicate>("I:'static")?);
+ } else {
+ where_clause_predicates.push(syn::parse_str::<WherePredicate>("T: Config")?);
+ }
+
+ where_clause_predicates.to_token_stream()
+ },
};
let mod_vis = module.vis;
let mod_name = module.ident;
@@ -568,10 +585,6 @@ pub fn benchmarks(
false => quote!(T),
true => quote!(T, I),
};
- let type_impl_generics = match instance {
- false => quote!(T: Config),
- true => quote!(T: Config<I>, I: 'static),
- };
let frame_system = generate_access_from_frame_or_crate("frame-system")?;
@@ -640,7 +653,7 @@ pub fn benchmarks(
*
}
- impl<#type_impl_generics> #krate::BenchmarkingSetup<#type_use_generics> for SelectedBenchmark where #where_clause {
+ impl<#type_use_generics> #krate::BenchmarkingSetup<#type_use_generics> for SelectedBenchmark where #where_clause {
fn components(&self) -> #krate::__private::Vec<(#krate::BenchmarkParameter, u32, u32)> {
match self {
#(
@@ -671,8 +684,8 @@ pub fn benchmarks(
}
}
#[cfg(any(feature = "runtime-benchmarks", test))]
- impl<#type_impl_generics> #krate::Benchmarking for Pallet<#type_use_generics>
- where T: #frame_system::Config, #where_clause
+ impl<#type_use_generics> #krate::Benchmarking for Pallet<#type_use_generics>
+ where T: #frame_system::Config,#where_clause
{
fn benchmarks(
extra: bool,
@@ -837,7 +850,7 @@ pub fn benchmarks(
}
#[cfg(test)]
- impl<#type_impl_generics> Pallet<#type_use_generics> where T: #frame_system::Config, #where_clause {
+ impl<#type_use_generics> Pallet<#type_use_generics> where T: #frame_system::Config, #where_clause {
/// Test a particular benchmark by name.
///
/// This isn't called `test_benchmark_by_name` just in case some end-user eventually
@@ -930,11 +943,6 @@ fn expand_benchmark(
true => quote!(T, I),
};
- let type_impl_generics = match is_instance {
- false => quote!(T: Config),
- true => quote!(T: Config<I>, I: 'static),
- };
-
// used in the benchmarking impls
let (pre_call, post_call, fn_call_body) = match &benchmark_def.call_def {
BenchmarkCallDef::ExtrinsicCall { origin, expr_call, attr_span: _ } => {
@@ -1030,13 +1038,11 @@ fn expand_benchmark(
// modify signature generics, ident, and inputs, e.g:
// before: `fn bench(u: Linear<1, 100>) -> Result<(), BenchmarkError>`
- // after: `fn _bench <T: Config<I>, I: 'static>(u: u32, verify: bool) -> Result<(),
+ // after: `fn _bench <T, I>(u: u32, verify: bool) where T: Config<I>, I: 'static -> Result<(),
// BenchmarkError>`
let mut sig = benchmark_def.fn_sig;
- sig.generics = parse_quote!(<#type_impl_generics>);
- if !where_clause.is_empty() {
- sig.generics.where_clause = parse_quote!(where #where_clause);
- }
+ sig.generics = parse_quote!(<#type_use_generics>);
+ sig.generics.where_clause = parse_quote!(where #where_clause);
sig.ident =
Ident::new(format!("_{}", name.to_token_stream().to_string()).as_str(), Span::call_site());
let mut fn_param_inputs: Vec<TokenStream2> =
@@ -1081,7 +1087,7 @@ fn expand_benchmark(
struct #name;
#[allow(unused_variables)]
- impl<#type_impl_generics> #krate::BenchmarkingSetup<#type_use_generics>
+ impl<#type_use_generics> #krate::BenchmarkingSetup<#type_use_generics>
for #name where #where_clause {
fn components(&self) -> #krate::__private::Vec<(#krate::BenchmarkParameter, u32, u32)> {
#krate::__private::vec! [
@@ -1123,7 +1129,7 @@ fn expand_benchmark(
}
#[cfg(test)]
- impl<#type_impl_generics> Pallet<#type_use_generics> where T: #frame_system::Config, #where_clause {
+ impl<#type_use_generics> Pallet<#type_use_generics> where T: #frame_system::Config, #where_clause {
#[allow(unused)]
fn #test_ident() -> Result<(), #krate::BenchmarkError> {
let selected_benchmark = SelectedBenchmark::#name;
--
GitLab