.cargo: Run `clippy` on ALL the source files #2949

thomaseizinger · 2022-09-28T06:12:34Z

Description

I turns out you need a PhD in clippology to figure out how to have it lint every source file in a repository. Previously, we were not passing --workspace to clippy which means it didn't lint a lot of source files. I am not sure which subset it was linting and which one it skipped but with this PR, we should be linting every source file in this repository.

Review guide

This PR is best reviewed patch-by-patch. The massive diff comes from the gossipsub tests but most of that is just removing one layer of modules which is done in a separate commit.

The first commit is completely auto-generated by running clippy --fix. Probably not worth reviewing. If you run the command mentioned in the commit locally and follow it with cargo fmt, you should get the same result!
The second commit is manual changes. Those should probably be reviewed.
The third and forth commit just moves everything from the tests module one layer up, fixing the last clippy lint. I tried to split this up so the diff shows how minimal the changes are but Git chokes on it and likes to see unchanged newlines instead of just showing the whitespace diff. You can verify locally that it is just whitespace changes by running: git diff 9443c7a4 9443c7a4^ --ignore-all-space
The fifth commit runs cargo fmt. You can again verify this locally if you want. Splitting this out makes the previous diff command work.
The sixth commit enforces clippy across the entire workspace.

Links to any relevant issues

Open Questions

Change checklist

I have performed a self-review of my own code
~~I have made corresponding changes to the documentation~~
~~I have added tests that prove my fix is effective or that my feature works~~
~~A changelog entry has been made in the appropriate crates~~

Previously, we were running clippy without --workspace which left out examples and tests of all sub-crates.

thomaseizinger

Made some mistakes apparently when fixing the lints.

protocols/gossipsub/src/peer_score/tests.rs

protocols/gossipsub/src/behaviour/tests.rs

mxinden

First off, thanks for doing this!

Thanks for making it easy to review.

Thanks for making it very entertaining!

Goodbye to all those .clone() calls.

mxinden · 2022-09-30T10:28:57Z

protocols/gossipsub/tests/smoke.rs

-        if num_nodes < 2 || num_nodes > 50 {
+        if !(2..=50).contains(&num_nodes) {


Not that it is important, though I am curious, do folks find this easier to read?

Pretty sure this one was done by clippy, I don't remember writing that.

If I were to write this test, I would probably split it into two ifs:

if num_nodes < 2 { return TestResult::discard(); } if num_nodes > 50 { return TestResult::discard(); }

Or make a newtype that implements Arbitrary that only generates numbers between 3 and 49. We can call it NonZeroOneTwoU6Minus15!

👍 on the above suggestions in general, though I think in this particular case leaving it as is is just fine.

jxs

LGTM, great effort Thomas!

jxs · 2022-09-30T16:14:32Z

protocols/autonat/tests/test_client.rs

@@ -497,7 +489,8 @@ async fn test_outbound_failure() {
        }

        let inactive = servers.split_off(1);
-        // Drop the handles of the inactive servers to kill them.
+
+        #[allow(clippy::needless_collect)] // Drop the handles of the inactive servers to kill them.


out of curiosity to understand in this case why clippy was complaining I commented the allow and ran the cargo +nightly clippy --all-features --all-targets -- -A clippy::type_complexity -A clippy::pedantic -D warnings command. But clippy didn't complain, is it not required?

You need the --workspace flag! My best guess is that this is because our root Cargo.toml is a crate and a workspace declaration.

I wonder whether we could avoid this if we put the meta crate in a directory too.

Thoughts @mxinden?

yeah, was running outdated rust version 😅 sorry. Btw with the latest nightly there are new lints triggered for large_enum_variant.

yeah, was running outdated rust version 😅 sorry. Btw with the latest nightly there are new lints triggered for large_enum_variant.

Yeah I know :)

I opened #2951 for this recently!

Currently, our top-level `Cargo.toml` manifest represents a crate AND a workspace. This causes surprising behaviour (e.g. #2949) where we need to explicitly pass `--workpace` to every command to run it on the entire workspace and not just the meta crate. My moving the meta crate into its own directory, the root manifest file is a virtual manifest only and thus, every `cargo` command will automatically default to running on the entire workspace. On top of this, I personally find it easier to understand if workspace and crate manifests are not mixed. Pull-Request: #3536.

thomaseizinger added 2 commits September 28, 2022 16:18

Run cargo clippy --workspace --all-features --all-targets --fix

133ddfe

Previously, we were running clippy without --workspace which left out examples and tests of all sub-crates.

Fix most clippy warnings

6e0946a

thomaseizinger force-pushed the clippy-workspace branch from 05a8bb5 to 04a01e7 Compare September 28, 2022 06:19

thomaseizinger added 2 commits September 28, 2022 16:35

Remove one level of indentation

9443c7a

Remove module

a663835

thomaseizinger force-pushed the clippy-workspace branch from 04a01e7 to 8ceba76 Compare September 28, 2022 06:37

thomaseizinger requested review from elenaf9 and mxinden September 28, 2022 06:42

thomaseizinger commented Sep 28, 2022

View reviewed changes

protocols/gossipsub/src/peer_score/tests.rs Outdated Show resolved Hide resolved

protocols/gossipsub/src/peer_score/tests.rs Outdated Show resolved Hide resolved

protocols/gossipsub/src/behaviour/tests.rs Outdated Show resolved Hide resolved

thomaseizinger added 2 commits September 28, 2022 16:46

Run cargo fmt

b2bed82

Enforce clippy across the entire workspace

cb4da08

thomaseizinger force-pushed the clippy-workspace branch from 8ceba76 to cb4da08 Compare September 28, 2022 06:46

thomaseizinger mentioned this pull request Sep 28, 2022

protocols/ping: Properly deprecate types with Ping prefix #2937

Merged

4 tasks

thomaseizinger added 2 commits September 30, 2022 16:03

Merge branch 'master' into clippy-workspace

3124a7f

Fix bad diff

2d2937a

mxinden approved these changes Sep 30, 2022

View reviewed changes

jxs approved these changes Sep 30, 2022

View reviewed changes

Merge branch 'master' into clippy-workspace

68f3b84

thomaseizinger merged commit 1b79324 into master Oct 4, 2022

thomaseizinger deleted the clippy-workspace branch October 4, 2022 07:24

thomaseizinger mentioned this pull request Oct 12, 2022

*: Move libp2p meta crate into its own directory #3012

Closed

5 tasks

thomaseizinger mentioned this pull request Mar 2, 2023

chore: move libp2p crate to a dedicated directory #3536

Merged

4 tasks

umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024

.cargo: Run clippy on ALL the source files (libp2p#2949)

4c0748f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.cargo: Run `clippy` on ALL the source files #2949

.cargo: Run `clippy` on ALL the source files #2949

thomaseizinger commented Sep 28, 2022 •

edited

Loading

thomaseizinger left a comment

mxinden left a comment

mxinden Sep 30, 2022

thomaseizinger Sep 30, 2022

thomaseizinger Sep 30, 2022 •

edited

Loading

mxinden Sep 30, 2022

jxs left a comment

jxs Sep 30, 2022

thomaseizinger Sep 30, 2022

jxs Oct 1, 2022

thomaseizinger Oct 2, 2022

		if num_nodes < 2 \|\| num_nodes > 50 {
		if !(2..=50).contains(&num_nodes) {

.cargo: Run clippy on ALL the source files #2949

.cargo: Run clippy on ALL the source files #2949

Conversation

thomaseizinger commented Sep 28, 2022 • edited Loading

Description

Review guide

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger left a comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

mxinden Sep 30, 2022

Choose a reason for hiding this comment

thomaseizinger Sep 30, 2022

Choose a reason for hiding this comment

thomaseizinger Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

mxinden Sep 30, 2022

Choose a reason for hiding this comment

jxs left a comment

Choose a reason for hiding this comment

jxs Sep 30, 2022

Choose a reason for hiding this comment

thomaseizinger Sep 30, 2022

Choose a reason for hiding this comment

jxs Oct 1, 2022

Choose a reason for hiding this comment

thomaseizinger Oct 2, 2022

Choose a reason for hiding this comment

.cargo: Run `clippy` on ALL the source files #2949

.cargo: Run `clippy` on ALL the source files #2949

thomaseizinger commented Sep 28, 2022 •

edited

Loading

thomaseizinger Sep 30, 2022 •

edited

Loading