Skip to content
Snippets Groups Projects
  • Tomás Senovilla Polo's avatar
    refactor: Move `T:Config` into where clause in `#[benchmarks]` macro if needed (#7418) · 4b2ca118
    Tomás Senovilla Polo authored
    # 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.
    Unverified
    4b2ca118
Code owners
Assign users and groups as approvers for specific file changes. Learn more.