chore: Implement latest clippy warnings #3220

umgefahren · 2022-12-09T10:26:17Z

Description

As I do frequently, I corrected for the latest clippy warnings. This will make sure the CI won't complain in the future. We could automate this btw and maybe run the nightly version of clippy.

Change checklist

I have performed a self-review of my own code

(No changes to pubic APIs)

umgefahren · 2022-12-09T10:46:43Z

I discovered a really mysterious function. Could someone explain the async block in this swarm test. This async block doesn't make any sense. Since it's not executed we could avoid the async block. But I don't know what we want to test here.

jxs

Looks good to me overall!

jxs · 2022-12-09T22:32:51Z

protocols/gossipsub/src/behaviour.rs

@@ -1425,7 +1425,8 @@ where
                                // check the flood cutoff
                                let flood_cutoff = (backoff_time
                                    + self.config.graft_flood_threshold())
-                                    - self.config.prune_backoff();
+                                .checked_sub(self.config.prune_backoff())
+                                .unwrap();


this seems to be unchecked_duration_subtraction right?
If so, we are switching a possible unintentional panic for an unwrap. I'd give it more context with expect.
(the same applies for the other .checked_sub().unwrap() substitutions)

Yes I think you are correct. I must have overlooked it (I tried to vet the automatic fixes but you know...)

This could be a bug. I'll look into filing an issue. Simply because the panic from the arithmetic error is probably more descriptive then calling unwrap on an empty option.

yeah, makes sense. Thanks Hannes.

umgefahren · 2022-12-10T18:22:16Z

Let's wait until I get feedback on rust-lang/rust-clippy#10061.

thomaseizinger · 2022-12-10T21:29:32Z

We could automate this btw

Are you suggesting a workflow that automatically opens a PR with cargo clippy --fix changes? I think that could be a cool GitHub action in general. Knock yourself out 😁

maybe run the nightly version of clippy.

We run beta. I think that makes more sense because those are likely to be more stable so less false positives and less churn :)

rkuhn · 2022-12-10T22:16:47Z

Just my 2c: clippy has crossed from useful into useless land long ago … it is mostly churn with each new release now. Implementing suggestions would have made my code worse on several occasions and introduced subtle bugs. Automating that is not a good idea. (yes, I tried raising issues, but the clippy crew is somewhat religious about their kraken)

thomaseizinger · 2022-12-12T05:46:16Z

I discovered a really mysterious function. Could someone explain the async block in this swarm test. This async block doesn't make any sense. Since it's not executed we could avoid the async block. But I don't know what we want to test here.

The async block allows us to .await which means we can match on the event which asserts which type is returned in SwarmEvent::Behaviour.

mxinden

Looks good to me minus the discussion https://github.com/libp2p/rust-libp2p/pull/3220/files#r1044889735.

Thanks @umgefahren!

umgefahren · 2022-12-12T15:04:34Z

Looks good to me minus the discussion https://github.com/libp2p/rust-libp2p/pull/3220/files#r1044889735.

Thanks @umgefahren!

I suggest just waiting until they figured out what to do over there. If they will stick with their plans I understand #3220 (comment). Although, personally, I never experienced any issues with clippy and always considered it a good tool for encouraging idiomatic Rust. It's just, as every linter, heavily opinionated. I really like your last sentence though, should remember that... 😄

They will decide tomorrow 16:00 UTC. I'll decide what to do then.

umgefahren · 2022-12-12T15:06:05Z

I discovered a really mysterious function. Could someone explain the async block in this swarm test. This async block doesn't make any sense. Since it's not executed we could avoid the async block. But I don't know what we want to test here.

The async block allows us to .await which means we can match on the event which asserts which type is returned in SwarmEvent::Behaviour.

But why not just make the entire function async? It's just a compilation test anyway and it has no influence on that. I might be mistaken here.

rkuhn · 2022-12-12T16:28:28Z

It's just, as every linter, heavily opinionated.

For the record: that’s a very good thing as far as I’m concerned! The issue is when opinion clashes with correctness, i.e. when the linter regulates aspects that are not subject to stylistic choices. I’d be very careful around lints that change

borrowing, especially in async blocks (“needless_collect” is a frequent offender — this issue stems from a language bug that requires values borrowed across .await implement Sync even though access will be via an exclusive reference only (cf. signature of poll))
locking (which is just borrowing of a guard): correctness may place non-local constraints on how long a lock can at most be held, i.e. which actions are not permitted to be performed while holding the lock; clippy cannot know the rules, yet it feels entitled to make suggestions

As an example of useless churn consider explicit_auto_deref, which hides essential detail from people who read code without an IDE.

mergify · 2022-12-12T21:00:50Z

This pull request has merge conflicts. Could you please resolve them @umgefahren? 🙏

thomaseizinger · 2022-12-12T21:08:44Z

I discovered a really mysterious function. Could someone explain the async block in this swarm test. This async block doesn't make any sense. Since it's not executed we could avoid the async block. But I don't know what we want to test here.

The async block allows us to .await which means we can match on the event which asserts which type is returned in SwarmEvent::Behaviour.

But why not just make the entire function async?

I think that is a possibility yes.

mxinden · 2022-12-14T12:50:49Z

Our CI currently fails on our clippy check. See e.g. https://github.com/libp2p/rust-libp2p/actions/runs/3694864006/jobs/6256559595. That would be fixed with this pull request.

Contrary to what I suggested before, with the above in mind I suggest we move forward and delay #3220 (comment) to another pull request.

mxinden · 2022-12-14T12:57:13Z

Preparing a merge commit right now.

…mplementing-clippy-warning-1.67-nightly

umgefahren · 2022-12-14T13:07:04Z

They decided to move it to the restriction group (https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Meeting.202022-12-13/near/315627353). It might take some time to implement the change though. I don't have any information here. I will revert the changes in that file for now.

…:umgefahren/rust-libp2p into implementing-clippy-warning-1.67-nightly

mxinden · 2022-12-14T15:45:20Z

Thanks @umgefahren!

thomaseizinger · 2022-12-14T20:14:55Z

Our CI currently fails on our clippy check. See e.g. https://github.com/libp2p/rust-libp2p/actions/runs/3694864006/jobs/6256559595. That would be fixed with this pull request.

Contrary to what I suggested before, with the above in mind I suggest we move forward and delay #3220 (comment) to another pull request.

Only the clippy (beta) though which is not required for merging! :)

thomaseizinger · 2023-02-15T23:34:12Z

@Mergifyio backport rust-v050-testplan

As I do frequently, I corrected for the latest clippy warnings. This will make sure the CI won't complain in the future. We could automate this btw and maybe run the nightly version of clippy. (cherry picked from commit d79c93a) # Conflicts: # protocols/request-response/tests/ping.rs # transports/webrtc/tests/smoke.rs

mergify · 2023-02-15T23:34:21Z

backport rust-v050-testplan

✅ Backports have been created

#3471 chore: Implement latest clippy warnings (backport #3220) has been created for branch rust-v050-testplan

As I do frequently, I corrected for the latest clippy warnings. This will make sure the CI won't complain in the future. We could automate this btw and maybe run the nightly version of clippy.

umgefahren added 3 commits December 9, 2022 11:24

Implement latest clippy warnings

8afb538

Remove undetected clippy warnings

4e2ca79

Use cargo fmt

e9da795

Merge branch 'master' into implementing-clippy-warning-1.67-nightly

973a6bc

jxs reviewed Dec 9, 2022

View reviewed changes

Merge branch 'master' into implementing-clippy-warning-1.67-nightly

f33239d

umgefahren mentioned this pull request Dec 10, 2022

Fix in clippy::unchecked_duration_subtraction produces unclearer panic rust-lang/rust-clippy#10061

Open

mxinden reviewed Dec 12, 2022

View reviewed changes

Merge branch 'master' into implementing-clippy-warning-1.67-nightly

2b720ef

Merge branch 'master' of https://github.com/libp2p/rust-libp2p into i…

3573eda

…mplementing-clippy-warning-1.67-nightly

mxinden and others added 3 commits December 14, 2022 14:46

transports/webrtc/: Fix panic formatting

e799205

Reverted changes

c5175e5

Merge branch 'implementing-clippy-warning-1.67-nightly' of github.com…

0c3966d

…:umgefahren/rust-libp2p into implementing-clippy-warning-1.67-nightly

mxinden approved these changes Dec 14, 2022

View reviewed changes

mxinden added the send-it label Dec 14, 2022

mergify bot merged commit d79c93a into libp2p:master Dec 14, 2022

mergify bot mentioned this pull request Feb 15, 2023

chore: Implement latest clippy warnings (backport #3220) #3471

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Implement latest clippy warnings #3220

chore: Implement latest clippy warnings #3220

umgefahren commented Dec 9, 2022 •

edited

Loading

umgefahren commented Dec 9, 2022

jxs left a comment

jxs Dec 9, 2022

umgefahren Dec 9, 2022 •

edited

Loading

jxs Dec 10, 2022

umgefahren commented Dec 10, 2022

thomaseizinger commented Dec 10, 2022

rkuhn commented Dec 10, 2022

thomaseizinger commented Dec 12, 2022

mxinden left a comment

umgefahren commented Dec 12, 2022 •

edited

Loading

umgefahren commented Dec 12, 2022

rkuhn commented Dec 12, 2022

mergify bot commented Dec 12, 2022

thomaseizinger commented Dec 12, 2022 •

edited

Loading

mxinden commented Dec 14, 2022

mxinden commented Dec 14, 2022

umgefahren commented Dec 14, 2022

mxinden commented Dec 14, 2022

thomaseizinger commented Dec 14, 2022

thomaseizinger commented Feb 15, 2023

mergify bot commented Feb 15, 2023

chore: Implement latest clippy warnings #3220

chore: Implement latest clippy warnings #3220

Conversation

umgefahren commented Dec 9, 2022 • edited Loading

Description

Change checklist

umgefahren commented Dec 9, 2022

jxs left a comment

Choose a reason for hiding this comment

jxs Dec 9, 2022

Choose a reason for hiding this comment

umgefahren Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

jxs Dec 10, 2022

Choose a reason for hiding this comment

umgefahren commented Dec 10, 2022

thomaseizinger commented Dec 10, 2022

rkuhn commented Dec 10, 2022

thomaseizinger commented Dec 12, 2022

mxinden left a comment

Choose a reason for hiding this comment

umgefahren commented Dec 12, 2022 • edited Loading

umgefahren commented Dec 12, 2022

rkuhn commented Dec 12, 2022

mergify bot commented Dec 12, 2022

thomaseizinger commented Dec 12, 2022 • edited Loading

mxinden commented Dec 14, 2022

mxinden commented Dec 14, 2022

umgefahren commented Dec 14, 2022

mxinden commented Dec 14, 2022

thomaseizinger commented Dec 14, 2022

thomaseizinger commented Feb 15, 2023

mergify bot commented Feb 15, 2023

✅ Backports have been created

umgefahren commented Dec 9, 2022 •

edited

Loading

umgefahren Dec 9, 2022 •

edited

Loading

umgefahren commented Dec 12, 2022 •

edited

Loading

thomaseizinger commented Dec 12, 2022 •

edited

Loading