Skip to content
GitLab
  • Menu
Projects Groups Snippets
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
    • Switch to GitLab Next
  • Sign in / Register
  • veloren veloren
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
    • Locked Files
  • Issues 425
    • Issues 425
    • List
    • Boards
    • Service Desk
    • Milestones
    • Iterations
    • Requirements
  • Merge requests 55
    • Merge requests 55
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
    • Test Cases
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Packages & Registries
    • Packages & Registries
    • Package Registry
    • Container Registry
    • Infrastructure Registry
  • Monitor
    • Monitor
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Code review
    • Insights
    • Issue
    • Repository
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • Veloren
  • velorenveloren
  • Issues
  • #587
Closed
Open
Created Jun 08, 2020 by Ben Wallis@xvar🦊Maintainer3 of 6 tasks completed3/6 tasks

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:

  1. 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.
  2. 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.
  3. 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 the cargo 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

Clippy Lint Instances MR
type_complexity 39
useless_conversion 35 !1198 (merged)
or_fun_call 30
new_without_default 27
option_map_unit_fn 26 !1071 (merged)
redundant_closure 20 !1065 (merged)
collapsible_if 20 !1064 (merged)
too_many_arguments 19
unused_unit 18
many_single_char_names 12 !1066 (merged)
float_cmp 11
clone_on_copy 10 !1092 (merged)
single_match 10
identity_op 9
useless_format 8
needless_range_loop 8
if_same_then_else 7
assign_op_pattern 7
blocks_in_if_conditions 7
into_iter_on_ref 7
large_enum_variant 7
let_and_return 7
approx_constant 5
manual_saturating_arithmetic 5
excessive_precision 5
needless_update 5
redundant_clone 4
len_without_is_empty 4
match_single_binding 4
expect_fun_call 3
op_ref 3
unit_arg 3
needless_return 3
nonminimal_bool 3
len_zero 2
neg_multiply 2
drop_copy 2
erasing_op 2
redundant_pattern_matching 2
cmp_owned 2
extra_unused_lifetimes 2
chars_next_cmp 2
unnecessary_operation 2
zero_ptr 2
needless_lifetimes 2
while_let_on_iterator 2
unused_io_amount 1
logic_bug 1
match_ref_pats 1
for_loops_over_fallibles 1
unnecessary_mut_passed 1
map_clone 1
eval_order_dependence 1
modulo_one 1
toplevel_ref_arg 1
map_entry 1
option_as_ref_deref 1
redundant_static_lifetimes 1
int_plus_one 1
single_char_pattern 1
filter_next 1
single_component_path_imports 1
useless_let_if_seq 1
match_bool 1
panic_params 1
print_literal 1
blacklisted_name 1
same_item_push 1
unnested_or_patterns ?
bind_instead_of_map ?
Edited Aug 15, 2020 by Joshua Yanovski
Assignee
Assign to
Time tracking