Stricter Linting with Rust 1.81+ and Clippy #2346

wilwade · 2025-04-21T13:35:01Z

Goal

The goal of this PR is to update and enable a bunch of Clippy lints.

Closes #1855

Discussion

Most of these things are normal clippy fixes
Highlighted the ones that are strange
Added make lint-fix that runs clippy fix

Checklist

Spec version incremented?

Additional Fixes Formatting More fixes More fixes #[allow(dead_code)] in weights Small fixes Clippy is happy

wilwade · 2025-04-21T13:35:40Z

.maintain/runtime-weight-template.hbs

+  #[allow(dead_code)]
  struct BlockWeights;


This isn't always dead code, but sometimes is and we just don't need generated files throwing lints

wilwade · 2025-04-21T13:36:07Z

common/helpers/src/avro.rs

@@ -190,13 +190,13 @@ pub fn get_schema_data_map<'a>(
 /// let schema_fingerprint = avro::fingerprint_raw_schema(raw_schema);
 /// assert!(schema_fingerprint.is_ok());
 /// ```
-pub fn validate_raw_avro_schema(json_schema: &Vec<u8>) -> Result<(), AvroError> {
-	match String::from_utf8(json_schema.clone()) {
+pub fn validate_raw_avro_schema(json_schema: &[u8]) -> Result<(), AvroError> {


This is the new syntax that is preferred and reduces a step for Rust referencing. I also like it better.

wilwade · 2025-04-21T13:37:58Z

common/primitives/src/messages.rs

-		assert_eq!(false, is_full);
+		assert!(!is_full);


Ok. I didn't like this until I learned about how rust is improving the assert! macro. See rust-lang/rust#44838 for more info. Although not all of it works now, it is the future.

wilwade · 2025-04-21T13:38:48Z

common/primitives/src/offchain.rs

-	vec![MSA_ACCOUNT_LOCK_NAME_PREFIX, msa_id.numtoa(10, &mut buff)].concat()
+	[MSA_ACCOUNT_LOCK_NAME_PREFIX, msa_id.numtoa(10, &mut buff)].concat()


There are a lot of these. Don't need to vec! everywhere anymore 🎉

wilwade · 2025-04-21T13:39:28Z

common/primitives/src/utils.rs

-	pub fn deserialize<'de, D: Deserializer<'de>>(
-		deserializer: D,
-	) -> Result<Option<Vec<u8>>, D::Error>
+	pub fn deserialize<'de, D>(deserializer: D) -> Result<Option<Vec<u8>>, D::Error>
 	where
 		D: Deserializer<'de>,


Don't double your wheres

wilwade · 2025-04-21T13:39:41Z

common/primitives/src/utils.rs

 	where
 		D: Deserializer<'de>,
 	{
-		impl_serde::serialize::deserialize(deserializer).map(|r| Some(r))
+		impl_serde::serialize::deserialize(deserializer).map(Some)


wilwade · 2025-04-21T13:40:07Z

node/cli/src/benchmarking.rs

-				dest: self.dest.clone().into(),
-				value: self.value.into(),
-			}
-			.into(),


You'll see a lot of unneeded into()s being removed.

wilwade · 2025-04-21T13:40:56Z

node/cli/src/command.rs

@@ -47,6 +47,7 @@ impl IdentifyChain for dyn sc_service::ChainSpec {

 impl PartialEq for ChainIdentity {
 	fn eq(&self, other: &Self) -> bool {
+		#[allow(clippy::match_like_matches_macro)]


I almost disabled this lint entirely, but the new matches! macro is nice. Just not in a few more complex places.

wilwade · 2025-04-21T13:41:33Z

pallets/capacity/src/benchmarking.rs

+	#[allow(clippy::useless_conversion)]
 	let name = Vec::from(name).try_into().expect("error");


This conversion is not useless as it is testing the error

wilwade · 2025-04-21T13:42:41Z

pallets/capacity/src/lib.rs

+				amount,
+				capacity_details.total_tokens_staked,
+				capacity_details.total_capacity_issued,
+			)


If else (if else) -> if elseif else

pallets/capacity/src/lib.rs

wilwade · 2025-04-21T13:44:13Z

pallets/capacity/src/lib.rs

@@ -1162,10 +1156,12 @@ impl<T: Config> Pallet<T> {
 	/// - Arrange the chunks such that we overwrite a complete chunk only when it is not needed
 	/// - The cycle is thus era modulo (history limit + chunk length)
 	/// - `[0,1,2],[3,4,5],[6,7,8],[]`
+	///


New line is "needed" after a list now to help make sure that you don't miss a -

wilwade · 2025-04-21T13:45:26Z

pallets/capacity/src/types.rs

-		while let Some((era, balance)) = bmap_iter.next() {
+		for (era, balance) in bmap_iter {


This is the suggested use instead of while.

wilwade · 2025-04-21T13:45:47Z

pallets/frequency-tx-payment/src/lib.rs

-			InitialPayment::Free => true,
-			_ => false,
-		}
+		matches!(*self, InitialPayment::Free)


Here's a great example of a good use of the new matches! macro

wilwade · 2025-04-21T13:46:50Z

pallets/handles/src/handles-utils/constants.rs

+// for all the languages supported.
+#[test]
+#[ignore = "use only to regenerate compacted ALLOWED_UNICODE_CHARACTER_RANGES"]
+fn test_build_allowed_char_ranges() {


I moved this here as the original location pallets/handles/src/handles-utils/src/tests/constants_tests.rs was not able to locate build_allowed_char_ranges correctly

wilwade · 2025-04-21T13:50:31Z

pallets/schemas/src/lib.rs

@@ -647,12 +647,12 @@ pub mod pallet {
 			model_type: &ModelType,
 			model: &BoundedVec<u8, T::SchemaModelMaxBytesBoundedVecLimit>,
 		) -> DispatchResult {
-			match model_type {
-				&ModelType::Parquet => {
+			match *model_type {


This * makes it so we don't have to & every match

wilwade · 2025-04-21T13:51:49Z

pallets/time-release/src/types.rs

-pub struct ReleaseSchedule<BlockNumber, Balance: MaxEncodedLen + HasCompact> {
+pub struct ReleaseSchedule<BlockNumber, Balance>
+where
+	Balance: MaxEncodedLen + HasCompact,


Anytime you have more than one, it is now suggested to do a where to reduce the complexity of the signature. I agree.

wilwade · 2025-04-21T13:52:07Z

pallets/treasury/src/tests.rs

@@ -17,8 +17,6 @@

 //! Treasury pallet tests.

-#![cfg(test)]


At the level up

runtime/common/src/constants.rs

codecov · 2025-04-21T15:19:18Z

Codecov Report

Attention: Patch coverage is 90.97222% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pallets/handles/src/handles-utils/constants.rs	0.00%	8 Missing ⚠️
pallets/msa/src/offchain_storage.rs	60.00%	2 Missing ⚠️
pallets/messages/src/lib.rs	75.00%	1 Missing ⚠️
pallets/msa/src/lib.rs	93.75%	1 Missing ⚠️
pallets/treasury/src/lib.rs	85.71%	1 Missing ⚠️

Files with missing lines	Coverage Δ
common/helpers/src/avro.rs	`92.59% <100.00%> (ø)`
common/primitives/src/messages.rs	`98.13% <100.00%> (ø)`
common/primitives/src/offchain.rs	`98.80% <100.00%> (ø)`
common/primitives/src/schema.rs	`49.29% <ø> (ø)`
common/primitives/src/signatures.rs	`58.93% <100.00%> (ø)`
common/primitives/src/utils.rs	`92.72% <100.00%> (-0.09%)`	⬇️
pallets/capacity/src/lib.rs	`89.22% <100.00%> (-0.07%)`	⬇️
pallets/capacity/src/types.rs	`92.85% <100.00%> (ø)`
pallets/frequency-tx-payment/src/lib.rs	`86.66% <100.00%> (-0.36%)`	⬇️
pallets/handles/src/handles-utils/build.rs	`100.00% <100.00%> (ø)`
... and 19 more

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mattheworris · 2025-04-21T19:05:33Z

pallets/capacity/src/tests/other_tests.rs

@@ -90,15 +90,15 @@ fn set_target_details_is_successful() {
 		let staker = 100;
 		let target: MessageSourceId = 1;

-		assert_eq!(StakingTargetLedger::<Test>::get(&staker, target), None);
+		assert_eq!(StakingTargetLedger::<Test>::get(staker, target), None);


nit: does the new assert! apply here?

Perhaps? But Clippy didn't think so.

Ah I see it. If would if I converted to == None, but I'd rather not do that until the assert macro is more setup

pallets/capacity/src/tests/withdrawal_tests.rs

aramikm

Looks good and a lot of nice cleanups, a few nits and questions for me.

aramikm · 2025-04-21T16:47:27Z

common/primitives/src/utils.rs

@@ -129,8 +127,8 @@ pub mod as_string_option {
 	}
 }

-const PREFIX: &'static str = "<Bytes>";
-const POSTFIX: &'static str = "</Bytes>";
+const PREFIX: &str = "<Bytes>";


Interesting! So static is going to get assigned by default

I was surprised as well.

aramikm · 2025-04-21T17:55:37Z

pallets/handles/src/benchmarking.rs

@@ -1,4 +1,3 @@
-#![cfg(feature = "runtime-benchmarks")]


nit: Any reason why this is removed?

It is put on the mod level, so shouldn't be here as well.

aramikm · 2025-04-21T18:19:14Z

pallets/handles/src/lib.rs

@@ -704,7 +701,7 @@ pub mod pallet {

 			// Validation: `BaseHandle` character length must be within range
 			ensure!(
-				len >= HANDLE_CHARS_MIN && len <= HANDLE_CHARS_MAX,
+				(HANDLE_CHARS_MIN..=HANDLE_CHARS_MAX).contains(&len),


nit, it's weird that this is preferred since it should be slower than the old expression. (but maybe the compiler knows that the difference between max and min to be close enough)

This was the auto fix, so I guess so? Perhaps because HANDLE_CHARS_MIN and HANDLE_CHARS_MAX are consts?

aramikm · 2025-04-21T18:43:03Z

pallets/messages/src/benchmarking.rs

@@ -1,4 +1,3 @@
-#![cfg(feature = "runtime-benchmarks")]


nit: why removed?

It is put on the mod level, so shouldn't be here as well.

runtime/common/src/constants.rs

mattheworris

Read through all changes.
Ran all local tests and benchmarks.
🚢 it!!

wilwade added 3 commits April 21, 2025 09:32

Remove the allows

954187e

Fixes

c59d8d3

Additional Fixes Formatting More fixes More fixes #[allow(dead_code)] in weights Small fixes Clippy is happy

Bump spec version

182c528

wilwade commented Apr 21, 2025

View reviewed changes

pallets/capacity/src/lib.rs Show resolved Hide resolved

Re-add comment

a95a51e

wilwade commented Apr 21, 2025

View reviewed changes

pallets/treasury/src/tests.rs

@@ -17,8 +17,6 @@

//! Treasury pallet tests.

#![cfg(test)]

Copy link

Collaborator Author

wilwade Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the level up

wilwade commented Apr 21, 2025

View reviewed changes

runtime/common/src/constants.rs Show resolved Hide resolved

wilwade added 2 commits April 21, 2025 09:59

Minor fixes from self-review

9b2d77e

Fix readme

f8a5baf

github-actions bot added the metadata-changed Metadata has changed since the latest full release label Apr 21, 2025

Test fixes

50a5357

github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Apr 21, 2025

Formatting

11ec2f8

github-actions bot added the metadata-changed Metadata has changed since the latest full release label Apr 21, 2025

wilwade marked this pull request as ready for review April 21, 2025 15:50

wilwade requested review from a team, shannonwells, mattheworris, enddynayn, aramikm, claireclark1 and JoeCap08055 and removed request for a team April 21, 2025 15:50

mattheworris reviewed Apr 21, 2025

View reviewed changes

pallets/capacity/src/tests/withdrawal_tests.rs Outdated Show resolved Hide resolved

Remove unneeded conversions in capacity tests

c0396d6

github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Apr 21, 2025

aramikm approved these changes Apr 21, 2025

View reviewed changes

Complete the fixes moving to u64 in tests instead of into

5b88bce

mattheworris approved these changes Apr 21, 2025

View reviewed changes

github-actions bot added the metadata-changed Metadata has changed since the latest full release label Apr 21, 2025

wilwade merged commit a7d80af into main Apr 21, 2025
30 checks passed

wilwade deleted the chore/fix-lints branch April 21, 2025 21:57

		vec![MSA_ACCOUNT_LOCK_NAME_PREFIX, msa_id.numtoa(10, &mut buff)].concat()
		[MSA_ACCOUNT_LOCK_NAME_PREFIX, msa_id.numtoa(10, &mut buff)].concat()

		#[allow(clippy::useless_conversion)]
		let name = Vec::from(name).try_into().expect("error");

		while let Some((era, balance)) = bmap_iter.next() {
		for (era, balance) in bmap_iter {

Stricter Linting with Rust 1.81+ and Clippy #2346

Stricter Linting with Rust 1.81+ and Clippy #2346

Uh oh!

Conversation

wilwade commented Apr 21, 2025

Goal

Discussion

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilwade Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aramikm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattheworris left a comment

Choose a reason for hiding this comment

wilwade Apr 21, 2025 •

edited

Loading

codecov bot commented Apr 21, 2025 •

edited

Loading