Skip to content
Snippets Groups Projects
Unverified Commit 4b2ca118 authored by Tomás Senovilla Polo's avatar Tomás Senovilla Polo Committed by GitHub
Browse files

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.
parent 9117f70f
No related merge requests found
Pipeline #515415 waiting for manual action with stages
in 10 minutes and 1 second
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment