Allow for custom clippy lints for FRAME pallets

[ntn-x2]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Motivation

We are experimenting with more stringent Clippy lints. We have tried different approaches, and all of them hit us back when linting our pallets.
My assumption is that the #[frame_support::pallet] macro discards anything it does not recognize, including any #[allow(clippy::all)].

Screenshot:

#[allow(clippy::all)]
#[pallet::error]
pub enum Error<T> {
  ...
}
error: using a potentially dangerous silent `as` conversion
   --> pallets/ctype/src/lib.rs:131:12
    |
131 |     #[pallet::error]
    |               ^^^^^
    |
    = help: consider using a safe wrapper for this conversion
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#as_conversions
    = note: requested on the command line with `-D clippy::as-conversions`

Request

We would like to have granular control over what lints are disabled where, and right now this is not possible. The only way to lint our code without errors is to disable those lints crate-wise, which is not what we want.

Solution

Not sure what the best solution is, but one possible solution would be to let developers specify Clippy lint configurations before each #[pallet:*] macro, especially since they internally have not followed the strictest linting rules, hence getting in the way of projects that have enabled such stricter rules.

Are you willing to help with this request?

Maybe (please elaborate above)

[gui1117]

My assumption is that the #[frame_support::pallet] macro discards anything it does not recognize, including any #[allow(clippy::all)].

Usually no it doesn't discard anything put its own pallet attributes, and it shouldn't IMO.

My hypothesis is that the error comes from the code generated by the pallet, which is not covered by the added attribute in the example, probably this code here https://github.com/paritytech/substrate/blob/242858f392ad3062c9d3b2fe6c34989c1839c66c/frame/support/procedural/src/pallet/expand/error.rs#L156

Solution

Not sure what the best solution is, but one possible solution would be to let developers specify Clippy lint configurations before each #[pallet:*] macro, especially since they internally have not followed the strictest linting rules, hence getting in the way of projects that have enabled such stricter rules.

Maybe for all the generated code and only the generated code we can add a clippy attribute which makes sure only the subset of lint that substrate support his code are checked on it, without requiring the user to specify anything.

[ntn-x2]

Maybe for all the generated code and only the generated code we can add a clippy attribute which makes sure only the subset of lint that substrate support his code are checked on it, without requiring the user to specify anything.

This would also be a viable solution. I tried to do that with a #[allow(clippy::all))] attribute, but apparently it was not considered.

[ntn-x2]

Plus, I feel this should be applied pretty much to any generated code. We had other issues also inside the benchmarking macros and the construct_runtime macro. Did not explore all of them since we got way too many errors, but the problem applies basically everywhere we rely on a Substrate macro.

[ntn-x2]

Hi! Has there been any progress or further discussion on this issue? It would enable downstream projects, such as ourselves, to add our own clippy config without incurring into Polkadot-generated warnings.

[ggwpez]

SO IIUC we want to keep plastering more allow pragmas into the auto-generated code like this?

polkadot-sdk/substrate/frame/support/procedural/src/construct_runtime/expand/origin.rs

Lines 203 to 213 in 22393d5

	#[allow(non_camel_case_types)]
	pub enum OriginCaller {
	#[codec(index = #system_index)]
	system(#system_path::Origin<#runtime>),
	#caller_variants
	#[allow(dead_code)]
	Void(#scrate::__private::Void)
	}
	// For backwards compatibility and ease of accessing these functions.
	#[allow(dead_code)]

Sounds good to me, maybe @wentelteefje can to take a stab?
@ntn-x2 what exact clippy config are you using? Any good way to spot all the issues at once?

[ntn-x2]

@ggwpez this is the PR we were working on: KILTprotocol/kilt-node#529

Specifically, this is the initial set of flags we were considering to add in .cargo/config.toml, which we would probably refactor:

[target.'cfg(feature = "cargo-clippy")']
rustflags = [
  "-Aclippy::all",
  "-Dclippy::correctness",
  "-Aclippy::if-same-then-else",
  "-Aclippy::clone-double-ref",
  "-Dclippy::complexity",
  "-Aclippy::zero-prefixed-literal",     # 00_1000_000
  "-Aclippy::type_complexity",           # raison d'etre
  "-Aclippy::nonminimal-bool",           # maybe
  "-Aclippy::borrowed-box",              # Reasonable to fix this one
  "-Aclippy::too-many-arguments",        # (Turning this on would lead to)
  "-Aclippy::unnecessary_cast",          # Types may change
  "-Aclippy::identity-op",               # One case where we do 0 +
  "-Aclippy::useless_conversion",        # Types may change
  "-Aclippy::unit_arg",                  # styalistic.
  "-Aclippy::option-map-unit-fn",        # styalistic
  "-Aclippy::bind_instead_of_map",       # styalistic
  "-Aclippy::erasing_op",                # E.g. 0 * DOLLARS
  "-Aclippy::eq_op",                     # In tests we test equality.
  "-Aclippy::while_immutable_condition", # false positives
  "-Aclippy::needless_option_as_deref",  # false positives
  "-Aclippy::derivable_impls",           # false positives
  "-Aclippy::stable_sort_primitive",     # prefer stable sort
  "-Wclippy::as_conversions",             
  "-Wclippy::integer_division",          
  "-Wclippy::integer_arithmetic",        
  "-Wclippy::indexing_slicing",          
  "-Wclippy::index_refutable_slice",     
  "-Wclippy::cast_possible_wrap",        
  "-Wclippy::float_arithmetic",
  "-Wclippy::missing_errors_doc",
  "-Wclippy::missing_panics_doc",
]