refactor: Move T:Config into where clause in #[benchmarks] macro if needed

[tsenovilla]

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 from Rust 1.78 onwards. We've found that here in LAOS, as we need to include T: pallet_vesting::Config in the where clause, here's the outcome:

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.

[gui1117]

Maybe we can always use the where clause to bound the types then? doesn't it look a bit better?

[gui1117]

Suggested change

	Ok(_) => quote!(),
	Ok(_) => match instance {
	false => quote!(T: Config),
	true => quote!(T: Config<I>, I: 'static),
	},

[gui1117]

replace the type_impl_generics with type_use_generics in the expansion.

Suggested change

	let type_impl_generics = if where_clause.is_empty() {
	match instance {
	false => quote!(T: Config),
	true => quote!(T: Config<I>, I: 'static),
	}
	} else {
	type_use_generics.clone()
	};

[gui1117]

Suggested change

	let type_impl_generics = if where_clause.is_empty() {
	match is_instance {
	false => quote!(T: Config),
	true => quote!(T: Config<I>, I: 'static),
	}
	} else {
	type_use_generics.clone()
	};

[tsenovilla]

Maybe we can always use the where clause to bound the types then? doesn't it look a bit better?

Yep I agree it looks nicer. I applied the changes + removed T: frame_system::Config from where clauses as T: Config would be in always in the where clause and all pallets are tightly coupled to frame_system