Apply latest clippy

[timotree3]

PR summary

Wow! I didn't realize just how good clippy is. This saved me tons of manual work.

I realize this is a lot of code change to review. Two suggestions:

• Only review changes to tests, if the tests are still passing after these changes, then the code probably didn't get broken.
• Run the commands yourself and compare the changes, I would expect them to be exactly the same, if they are, then you can approve the changes.
  • I have included the exact commands used for reproducibility.

testing/benchmarking notes

I tested that the applied code still works on the nightly used by nix.

followups

None.

changelog

Please check one of the following, relating to the CHANGELOG-UNRELEASED.md

• [ ] this is a code change that effects some consumer (e.g. zome developers) of holochain core so it is added to the CHANGELOG-UNRELEASED.md (linked above), with the format - summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)
• [X] this is not a code change, or doesn't effect anyone outside holochain core development

documentation

• [X] this code has been documented according to our docs checklist

[thedavidmeister]

@timotree3 can i get more context around what this represents?

we have clippy running in CI atm, so where do these additional changes come from?

[timotree3]

@timotree3 can i get more context around what this represents?

we have clippy running in CI atm, so where do these additional changes come from?

Sure! This changes were produced by the version of clippy that shipped in nightly-2019-10-23. All I know for sure is that the version running on our CI doesn't lint these things and latest clippy does. It could be that there were some bugs in CI or that these are new lints. Three months old is a long time in rust development.

[timotree3]

If you start to look at the change list, I think all of them are pretty irrefutably positive. Here are some of the changes I noticed that it made:

• Remove unnecessary clones
  • I've seen so many of these in the code base and I've been confused why clippy wasn't catching them. This is the main reason I sounded so excited in the PR summary
• Remove unnecessary conversions:
  • to_owned()
  • into()
  • etc.
• Replace and_then(|v| Some(...)) with map(|v| ...)

and so many more things that I thought I was going to have to do manual work to address

[thedavidmeister]

@timotree3 i guess what i'm asking is whether this is really new, or that you're just using different clippy config than what is on CI (and so getting a different result)

what's the exact command that you ran?

[timotree3]

what's the exact command that you ran?

@thedavidmeister, the commands for each of the commits are their commit message.

Edit: the first commit message is wrong actually. Here are the actual commands I ran:

• cargo +nightly-2019-10-23 fix -Z unstable-options --clippy
• cargo +nightly-2019-10-23 fmt

[thedavidmeister]

@timotree3 @lucksus thanks for that, have a look at https://github.com/holochain/holonix/blob/develop/rust/clippy/default.nix

we can see that the flags to clippy are different upstream

 cargo clippy -- \
 -A clippy::nursery -A clippy::style -A clippy::cargo \
 -A clippy::pedantic -A clippy::restriction \
 -D clippy::complexity -D clippy::perf -D clippy::correctness

notably -A means "allow", so there's a lot of things being skipped by design

i don't see any reason to block this PR when it passes tests but i also think it is only a temporary solution as people will be hacking away on the same LOC, with tools configured differently and no CI tests to provide feedback along the way, and so it will all naturally erode again over time

for a more permanent solution there are two things to do here:

• bring holonix up to the same or similar nightly used in this PR
• review the upstream clippy flags to include more high value clips as appropriate

both of these changes need to be propagated into releases across the repos charted here https://docs.holochain.love/docs/holochain/

[timotree3]

Thanks for pointing that out. I can confirm that at least some of the changes wouldn't have been skipped (at least unnecessary clone which is in clippy::perf). Would you like me to redo this PR with the flags you posted @thedavidmeister?

[thedavidmeister]

@timotree3 like i said there's no harm in merging fixes, but they will start to rot immediately

i'd suggest getting it all in asap (already seeing some merge conflicts) then looking at how we might use clippy more effectively as a followup

[timotree3]

Cool. I can rebase whenever, seems like this is pretty much impossible to merge for now though given that the team doesn't have enough time for my first (almost rotless) PR that touched a lot of code #1701, they're not gonna have time for this very rotful one that also touches a lot of code.

I'll rebase in a few hours but moving forward I think it's wiser for me to save my effort and just rebase after the team is under less pressure... :-)

[thedavidmeister]

@timotree3 if you want to PR tweaking the flags upstream in holonix the changes will flow downstream as teams/repos update, you don't need to do anything more to make that happen

[timotree3]

@thedavidmeister, I have rebased and then re-run the commands on the base, the commit messages now accurately reflect commands run.

@timotree3 if you want to PR tweaking the flags upstream in holonix the changes will flow downstream as teams/repos update, you don't need to do anything more to make that happen

I don't understand what you're suggesting I tweak.

[thedavidmeister]

@timotree3 this https://github.com/holochain/holonix/blob/develop/rust/clippy/default.nix

[timotree3]

@thedavidmeister gotcha, and thanks for the approve! Only one to go... The only difference between the one in that file and the default, is that the file disables style lints. Most of the lints being fixed in this PR aren't style anyway so I don't think that's important to change.

[thedavidmeister]

@timotree3 so then https://github.com/holochain/holonix/blob/develop/rust/config.nix#L10

[thedavidmeister]

@timotree3 soooo we actually bumped rust nightly!

i think that means most or all of these clips are done?

[timotree3]

I'll run the steps again from latest develop and find out!

[timotree3]

@thedavidmeister, nope! There are still things that cargo fix changes... I've updated the commits to be based on latest develop and use (almost) latest nightly.

[thedavidmeister]

@timotree3 can you paste the exact command you're running here so i can try myself? i wonder what is causing differences :thinking:

[timotree3]

Yeah. It's in the commit messages but here

$ cargo +nightly fix --clippy -Z unstable-options
$ cargo fmt +nightly

At

$ cargo +nightly -V
cargo 1.41.0-nightly (626f0f40e 2019-12-03)

[thedavidmeister]

@timotree3 ok i see, the upstream looks like this:

 cargo clippy -- \
 -A clippy::nursery -A clippy::style -A clippy::cargo \
 -A clippy::pedantic -A clippy::restriction \
 -D clippy::complexity -D clippy::perf -D clippy::correctness

do you think we could benefit from changing the flags there?

[timotree3]

The difference between the settings you linked me to and the default clippy settings (which are listed on the README) is that style lints are enabled by default.

I looked at the list of lints and filtered by "style" and all of the lints I found seemed like good things to me.

Provided that we like these style lints, the question for me is if those style fixes are worth the time it takes people to make them. I think the answer is yes, because these are just as valuable as the changes that cargo fmt makes, and we currently require cargo fmt to pass for PRs.

[timotree3]

An alternative to running clippy and failing CI if there are warnings, would be to run cargo fix --clippy and fail if it would make any changes. That way we never waste developer time on things that can't be automatically fixed.

Should I amend this PR to also change the CI setup to

1. Enable style lints and
2. Only fail if the clippy warnings can be fixed automatically?

[thedavidmeister]

@timotree3 i think you should put a PR up on the holonix repo with the updated flags, don't worry about "wasting developer's time" because it's also a waste of time to go back and forward on lint rules every time a new rust dev wants to contribute and asks all the same questions (just my 2c)