undetected unused variables in frame benchmarking V1

[kianenigma]

Merging of #6018 made some unused variable imports to appear in the code. I first thought @re-gius had some mistake, but it is not the case. Looking at the old benchmarking code, it seems like some variables are indeed unused, and it goes undetected in the v1 benchmarking macro.

In the v2 macro, it gets detected though.

Example: call_hash and multi_account_id here:

polkadot-sdk/substrate/frame/multisig/src/benchmarking.rs

Lines 51 to 70 in 7c2d349

	as_multi_threshold_1 {
	// Transaction Length
	let z in 0 .. 10_000;
	let max_signatories = T::MaxSignatories::get().into();
	let (mut signatories, _) = setup_multi::<T>(max_signatories, z)?;
	let call: <T as Config>::RuntimeCall = frame_system::Call::<T>::remark {
	remark: vec![0; z as usize]
	}.into();
	let call_hash = call.using_encoded(blake2_256);
	let multi_account_id = Multisig::<T>::multi_account_id(&signatories, 1);
	let caller = signatories.pop().ok_or("signatories should have len 2 or more")?;
	// Whitelist caller account from further DB operations.
	let caller_key = frame_system::Account::<T>::hashed_key_for(&caller);
	frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
	}: _(RawOrigin::Signed(caller.clone()), signatories, Box::new(call))
	verify {
	// If the benchmark resolves, then the call was dispatched successfully.
	}
	as_multi_create {

[runcomet]

@kianenigma removal of this footgun is needed?

[kianenigma]

We would first need to understand what is the disparity between v1 and v2 syntax that is causing this. Then, yes, fixing it is needed.

[ggwpez]

In the V2 syntax we actually expand these code of blocks into proper functions. We end up not using those functions, but they are annotated with the same spans as the original code, getting us proper clippy warning support.

I dont remember how it works in the V1 code, but i think "unused warnings" are normally ignored in macro expanded code, so that could be it.
Depending on the time it takes to fix this, maybe its better to port more pallets to the V2 syntax. We eventually want to remove the old syntax...

[runcomet]

Locating the issue while porting more pallets to V2 seems like two birds one long weekend :) but could then see which pallets generate the warning?

[ggwpez]

The tracking issue for moving them is here: #6202

but could then see which pallets generate the warning?

Yes.