coreutils icon indicating copy to clipboard operation
coreutils copied to clipboard

Apply some useful pedantic rules into project

Open qarmin opened this issue 2 years ago • 3 comments

Looks that a lot of pedantic rules are really useful to improve/unify style. Command of manually chosen clippy rules

cargo clippy -- -Wclippy::bool_to_int_with_if -Wclippy::checked_conversions -Wclippy::cloned_instead_of_copied -Wclippy::explicit_deref_methods -Wclippy::explicit_into_iter_loop -Wclippy::explicit_iter_loop -Wclippy::filter_map_next -Wclippy::flat_map_option -Wclippy::float_cmp -Wclippy::from_iter_instead_of_collect -Wclippy::if_not_else -Wclippy::implicit_clone -Wclippy::inconsistent_struct_constructor -Wclippy::inefficient_to_string -Wclippy::invalid_upcast_comparisons -Wclippy::iter_not_returning_iterator -Wclippy::large_digit_groups -Wclippy::large_stack_arrays -Wclippy::large_types_passed_by_value -Wclippy::linkedlist -Wclippy::macro_use_imports -Wclippy::manual_assert -Wclippy::manual_instant_elapsed -Wclippy::manual_let_else -Wclippy::manual_ok_or -Wclippy::manual_string_new -Wclippy::map_unwrap_or -Wclippy::match_bool -Wclippy::match_same_arms -Wclippy::match_wildcard_for_single_variants -Wclippy::maybe_infinite_iter -Wclippy::mismatching_type_param_order -Wclippy::mut_mut -Wclippy::naive_bytecount -Wclippy::needless_bitwise_bool -Wclippy::needless_continue -Wclippy::needless_for_each -Wclippy::needless_pass_by_value -Wclippy::no_effect_underscore_binding -Wclippy::range_minus_one -Wclippy::range_plus_one -Wclippy::redundant_closure_for_method_calls -Wclippy::redundant_else -Wclippy::ref_option_ref -Wclippy::return_self_not_must_use -Wclippy::same_functions_in_if_condition -Wclippy::semicolon_if_nothing_returned -Wclippy::single_match_else -Wclippy::stable_sort_primitive -Wclippy::string_add_assign -Wclippy::uninlined_format_args -Wclippy::unnecessary_box_returns -Wclippy::unnecessary_join -Wclippy::unnecessary_wraps -Wclippy::unnested_or_patterns -Wclippy::unreadable_literal -Wclippy::unsafe_derive_deserialize -Wclippy::unused_async -Wclippy::unused_self -Wclippy::used_underscore_binding -Wclippy::zero_sized_map_values

example of warnings

warning: unused `self` argument
   --> src/uu/dd/src/parseargs.rs:281:20
    |
281 |     fn parse_bytes(&self, arg: &str, val: &str) -> Result<usize, ParseError> {
    |                    ^^^^^
    |
    = help: consider refactoring to an associated function
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unused_self

warning: called `map(<f>).unwrap_or(<a>)` on an `Option` value. This can be done more directly by calling `map_or(<a>, <f>)` instead
  --> src/uu/shuf/src/shuf.rs:67:13
   |
67 | /             matches
68 | |                 .get_one::<String>(options::FILE)
69 | |                 .map(|s| s.as_str())
70 | |                 .unwrap_or("-")
   | |_______________________________^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or
   = note: requested on the command line with `-W clippy::map-unwrap-or`
help: use `map_or(<a>, <f>)` instead
   |
69 -                 .map(|s| s.as_str())
69 +                 .map_or("-", |s| s.as_str())
   |

warning: redundant else block
   --> src/uu/split/src/split.rs:669:24
    |
669 |                   } else {
    |  ________________________^
670 | |                     // Move the window to look at only the remaining bytes.
671 | |                     buf = &buf[i..];
672 | |
673 | |                     // Remember for the next iteration that we wrote these bytes.
674 | |                     carryover_bytes_written += num_bytes_written;
675 | |                 }
    | |_________________^
    |
    = help: remove the `else` block and move the contents out
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_else

warning: used binding `_x` which is prefixed with an underscore. A leading underscore signals that a binding will not be used
  --> src/uu/factor/src/rho.rs:37:38
   |
37 |                 gcd(n.modulus(), max(_x, _y) - min(_x, _y))
   |                                      ^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#used_underscore_binding

warning: an inclusive range would be more readable
    --> src/uu/cp/src/cp.rs:1547:42
     |
1547 |     let dest_ancestors = &dest_ancestors[1..1 + k];
     |                                          ^^^^^^^^ help: use: `1..=k`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#range_plus_one
     = note: requested on the command line with `-W clippy::range-plus-one`

warning: redundant closure
  --> src/uu/cp/src/copydir.rs:97:36
   |
97 |             root_path.parent().map(|p| p.to_path_buf())
   |                                    ^^^^^^^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::path::Path::to_path_buf`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls
   = note: requested on the command line with `-W clippy::redundant-closure-for-method-calls`
warning: this function's return value is unnecessary
   --> src/uu/head/src/head.rs:436:1
    |
436 | / fn uu_head(options: &HeadOptions) -> UResult<()> {
437 | |     let mut first = true;
438 | |     for file in &options.files {
439 | |         let res = match (file.as_str(), options.presume_input_pipe) {
...   |
513 | |     Ok(())
514 | | }
    | |_^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps
    = note: requested on the command line with `-W clippy::unnecessary-wraps`
help: remove the return type...
    |
436 | fn uu_head(options: &HeadOptions) -> UResult<()> {
    |                                      ~~~~~~~~~~~
help: ...and then remove returned values
    |
513 -     Ok(())
513 +     
    |

https://rust-lang.github.io/rust-clippy/master/index.html#match_same_arms: x457 https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls: x90 https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else: x49 https://rust-lang.github.io/rust-clippy/master/index.html#redundant_else: x35 https://rust-lang.github.io/rust-clippy/master/index.html#implicit_clone: x34 https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or: x26 https://rust-lang.github.io/rust-clippy/master/index.html#used_underscore_binding: x18 https://rust-lang.github.io/rust-clippy/master/index.html#unnested_or_patterns: x10 https://rust-lang.github.io/rust-clippy/master/index.html#inefficient_to_string: x10 https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps: x9 https://rust-lang.github.io/rust-clippy/master/index.html#flat_map_option: x6 https://rust-lang.github.io/rust-clippy/master/index.html#unused_self: x5 https://rust-lang.github.io/rust-clippy/master/index.html#no_effect_underscore_binding: x5 https://rust-lang.github.io/rust-clippy/master/index.html#cloned_instead_of_copied: x5 https://rust-lang.github.io/rust-clippy/master/index.html#range_plus_one: x4 https://rust-lang.github.io/rust-clippy/master/index.html#non_minimal_cfg: x3 https://rust-lang.github.io/rust-clippy/master/index.html#bool_to_int_with_if: x3 https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns: x2 https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor: x2 https://rust-lang.github.io/rust-clippy/master/index.html#default_constructed_unit_structs: x2 https://rust-lang.github.io/rust-clippy/master/index.html#unreadable_literal: x1 https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_join: x1 https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args: x1 https://rust-lang.github.io/rust-clippy/master/index.html#needless_bool_assign: x1 https://rust-lang.github.io/rust-clippy/master/index.html#match_bool: x1 https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back: x1 https://rust-lang.github.io/rust-clippy/master/index.html#manual_assert: x1 https://rust-lang.github.io/rust-clippy/master/index.html#explicit_into_iter_loop: x1

qarmin avatar Jun 05 '23 15:06 qarmin

yeah, we removed a bunch of the last few months. We should enable them in the CI but first, we need to fix all of them

sylvestre avatar Jun 05 '23 15:06 sylvestre

As mentioned in closed PR, if there is a definitive list of rules that you would like applied, I would be happy to apply them and submit per category. A couple of my guesses would be:

  1. method in place of bare closure
  2. map_or() > map().unwrap_or()
  3. joining match arms that share the same result (although I could definitely see an argument against it)

Rules I would guess you probably wouldn't want applied:

  1. Unused self
  2. if let > match for single arm
  3. Unnecessary return for Result<()>

I realize now that this was the way to go about this, instead of submitting a PR full of random ones :)

PThorpe92 avatar Jun 12 '23 11:06 PThorpe92

I'm fixing a bunch of the remaining clippy pedantics: https://github.com/dcampbell24/coreutils/tree/fix-clippy .

dcampbell24 avatar Sep 19 '24 23:09 dcampbell24

Are there more rules we would like to have enabled? I'd be willing to work on some of them.

memark avatar Nov 09 '25 17:11 memark

@memark there are some rules in Cargo.toml (at the bottom of the file, those without a comment at the line end) where we have to decide whether we want to re-enable them or not. They were enabled in the past, but I disabled them when I enabled the workspace lints for uucore.

cakebaker avatar Nov 09 '25 18:11 cakebaker