Clippy warnings should fail CI
At the time of writing, running cargo clippy
on master
results in 85 warnings and 5 errors - many of which indicate sub-optimal code or even potential bugs. Allowing clippy warnings past CI causes three major problems:
- It makes running clippy basically useless for developers who want to see if they've introduced any clippy warnings in their own code since it's impossible to filter out all of the existing warnings.
- Following on from 1), Fixing broken windows comes into play - the more clippy warnings that exist in the codebase, the more likely it is that new ones will be introduced in further commits.
- Some of the warnings represent genuine potential bugs
I've done a quick cleanup in a commit here which shows the kind of minor changes needed to fix 50% of the existing clippy warnings (this branch has 43 warnings 5 errors): 6762d354
Most of the remaining warnings probably need a bit more of an in-depth investigation since the fixes for some potentially have side effects such as replacing unwrap_or
with unwrap_or_else
.
In my opinion the best route to zero clippy warnings would be the following steps:
-
MR the above commit to deal with the simple fixes -
Create a tracking issue for the remaining clippy warnings/errors -
Add #![allow(clippy::<lint_name>)] // Pending review in #587
to suppress the remaining clippy warnings -
Now that there are zero clippy warnings/errors, modify check-compile.gitlab-ci.yml
to make clippy warnings fail the build by removing-- --warn clippy::all
from thecargo clippy
line -
Fix the remaining suppressed warnings over time by either refactoring or making a decision to permanently leave the suppression where appropriate -
Add a page to the Book explaining Clippy policy
Apologies if this proves to be a contentious suggestion from someone who's barely been a contributor for 24 hours - I believe it would have a positive impact on code quality with little downside though :)
Explanations of clippy lints can be found here for reference: https://rust-lang.github.io/rust-clippy/master/index.html
Remaining suppressed warnings with TODO commment