Better enforce clippy lints

[niklasmohrin]

Hey there, it's me again :D

I just saw that the clippy lints are run, but the annotations cannot be added to forked repos, so PR diffs cannot be annotated. To enforce the lints, I propose the following changes:

1. Project-wide deny(clippy::all): This will have clippy error if any of the default warnings is emitted
2. Project-wide warn(clippy::pedantic) with whitelisting: This will nit-pick things, which is why these are just warnings. Still, I would rather allow specific lints (like I did already) where needed if it is reasonable.
3. Finally, make the clippy workflow a required pass, so that no new code that doesn't pass the default lints can be merged (I haven't figured out how to do this yet, is this some maintainer setting?)

I think 1) and 3) are needed, 2) is maybe a bit radical. Still, I think it would be a nice addition. Because the annotations are still there for code that is already on this repository, these lints can be fixed when someone is already working on the affected code. Let me know what you think!

[niklasmohrin]

Note to self: Move this back into main.rs, because the warning still shows up (probably because the lint is in a macro that gets expanded in main)

[bee-san]

Finally, make the clippy workflow a required pass, so that no new code that doesn't pass the default lints can be merged (I haven't figured out how to do this yet, is this some maintainer setting?)

In CI, you just add it normally to either GitHub actions or Travis CI. You have to specify it's unrequired for it to be so. By adding it without specifying, it defaults to required :)

Let me know when you want this merged 👍

Alsssoooo 💖 if you submit a PR that isn't ready, please mark it as a draft 😄

[niklasmohrin]

@bee-san In #287 I just edited the .yml file, none of the entries had any special "required" attribute set, which is why I was so confused, as they all show up as required here 🤔 I also did not untick anything or so. is this in some maintainer settings? I don't really know much about Github workflows

Good to know, elsewhere my PRs get ignored if I don't set them to ready-to-review. which is why I now understand "ready-for-review" more as a "looking for input" than "waiting for merge". Anyway, I am gonna mind that in the future on this repo.

What do you think about clippy::pedantic? Should the warnings stay in?

[bee-san]

is this in some maintainer settings? I don't really know much about Github workflows
No, every entry is required unless specified that it isn't. We have no CI that is allowed to fail. It's not a maintainer setting either, it's a tag in the YML file for "allowed to fail" -- but we don't have that so everything in CI is set to be required :)

Project-wide warn(clippy::pedantic) with whitelisting: This will nit-pick things, which is why these are just warnings. Still, I would rather allow specific lints (like I did already) where needed if it is reasonable.

Honestly I'm not sure, I don't know how badly it'll nit-pick things -- what do you think? You are obviously more versed in this than I am 😄

[niklasmohrin]

@bee-san This is what cargo clippy outputs (with the two allows from the diff set):

warning: more than 3 bools in a struct
   --> src/input.rs:71:1
    |
71  | / pub struct Opts {
72  | |     /// A list of comma separated CIDRs, IPs, or hosts to be scanned.
73  | |     #[structopt(short, long, use_delimiter = true)]
74  | |     pub addresses: Vec<String>,
...   |
136 | |     pub command: Vec<String>,
137 | | }
    | |_^
    |
note: the lint level is defined here
   --> src/main.rs:2:9
    |
2   | #![warn(clippy::pedantic)]
    |         ^^^^^^^^^^^^^^^^
    = note: `#[warn(clippy::struct_excessive_bools)]` implied by `#[warn(clippy::pedantic)]`
    = help: consider using a state machine or refactoring bools into two-variant enums
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#struct_excessive_bools

warning: item name ends with its containing module's name
   --> src/scripts/mod.rs:64:1
    |
64  | / pub fn init_scripts(scripts: ScriptsRequired) -> Result<Vec<ScriptFile>> {
65  | |     let mut scripts_to_run: Vec<ScriptFile> = Vec::new();
66  | |
67  | |     match scripts {
...   |
118 | |     }
119 | | }
    | |_^
    |
note: the lint level is defined here
   --> src/main.rs:2:9
    |
2   | #![warn(clippy::pedantic)]
    |         ^^^^^^^^^^^^^^^^
    = note: `#[warn(clippy::module_name_repetitions)]` implied by `#[warn(clippy::pedantic)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#module_name_repetitions

warning: item name ends with its containing module's name
   --> src/scripts/mod.rs:121:1
    |
121 | / pub fn parse_scripts(scripts: Vec<PathBuf>) -> Vec<ScriptFile> {
122 | |     let mut parsed_scripts: Vec<ScriptFile> = Vec::with_capacity(scripts.len());
123 | |     for script in scripts {
124 | |         debug!("Parsing script {}", &script.display());
...   |
129 | |     parsed_scripts
130 | | }
    | |_^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#module_name_repetitions

warning: item name ends with its containing module's name
   --> src/scripts/mod.rs:270:1
    |
270 | / pub fn find_scripts(mut path: PathBuf) -> Result<Vec<PathBuf>> {
271 | |     path.push(".rustscan_scripts");
272 | |     if path.is_dir() {
273 | |         debug!("Scripts folder found {}", &path.display());
...   |
282 | |     }
283 | | }
    | |_^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#module_name_repetitions

warning: Unnecessary boolean `not` operation
   --> src/main.rs:230:9
    |
230 | /         if !parsed_ips.is_empty() {
231 | |             ips.extend(parsed_ips);
232 | |         } else {
233 | |             unresolved_addresses.push(address);
234 | |         }
    | |_________^
    |
note: the lint level is defined here
   --> src/main.rs:2:9
    |
2   | #![warn(clippy::pedantic)]
    |         ^^^^^^^^^^^^^^^^
    = note: `#[warn(clippy::if_not_else)]` implied by `#[warn(clippy::pedantic)]`
    = help: remove the `!` and swap the blocks of the `if`/`else`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#if_not_else

warning: redundant closure found
  --> src/input.rs:44:14
   |
44 |         .map(|x| x.parse::<u16>())
   |              ^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `str::parse`
   |
note: the lint level is defined here
  --> src/main.rs:2:9
   |
2  | #![warn(clippy::pedantic)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(clippy::redundant_closure_for_method_calls)]` implied by `#[warn(clippy::pedantic)]`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls

warning: usage of wildcard import
 --> src/scanner/mod.rs:9:5
  |
9 | use colored::*;
  |     ^^^^^^^^^^ help: try: `colored::Colorize`
  |
note: the lint level is defined here
 --> src/main.rs:2:9
  |
2 | #![warn(clippy::pedantic)]
  |         ^^^^^^^^^^^^^^^^
  = note: `#[warn(clippy::wildcard_imports)]` implied by `#[warn(clippy::pedantic)]`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_imports

warning: redundant closure found
  --> src/scanner/mod.rs:52:33
   |
52 |             ips: ips.iter().map(|ip| ip.to_owned()).collect(),
   |                                 ^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `std::borrow::ToOwned::to_owned`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls

warning: casting `u32` to `u16` may truncate the value
  --> src/port_strategy/range_iterator.rs:63:14
   |
63 |         Some((self.actual_start + current_pick) as u16)
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> src/main.rs:2:9
   |
2  | #![warn(clippy::pedantic)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(clippy::cast_possible_truncation)]` implied by `#[warn(clippy::pedantic)]`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cast_possible_truncation

warning: this argument is passed by value, but not consumed in the function body
  --> src/port_strategy/mod.rs:19:24
   |
19 |     pub fn pick(range: Option<PortRange>, ports: Option<Vec<u16>>, order: ScanOrder) -> Self {
   |                        ^^^^^^^^^^^^^^^^^ help: consider taking a reference instead: `&Option<PortRange>`
   |
note: the lint level is defined here
  --> src/main.rs:2:9
   |
2  | #![warn(clippy::pedantic)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[warn(clippy::needless_pass_by_value)]` implied by `#[warn(clippy::pedantic)]`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value

warning: this argument is passed by value, but not consumed in the function body
  --> src/port_strategy/mod.rs:19:75
   |
19 |     pub fn pick(range: Option<PortRange>, ports: Option<Vec<u16>>, order: ScanOrder) -> Self {
   |                                                                           ^^^^^^^^^ help: consider taking a reference instead: `&ScanOrder`
   |
help: consider marking this type as `Copy`
  --> src/input.rs:9:1
   |
9  | / arg_enum! {
10 | |     /// Represents the strategy in which the port scanning will run.
11 | |     ///   - Serial will run from start to end, for example 1 to 1_000.
12 | |     ///   - Random will randomize the order in which ports will be scanned.
...  |
17 | |     }
18 | | }
   | |_^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value
   = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

warning: redundant closure found
   --> src/scripts/mod.rs:200:18
    |
200 |             .map(|port| port.to_string())
    |                  ^^^^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `std::string::ToString::to_string`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls

warning: redundant closure found
   --> src/scripts/mod.rs:236:22
    |
236 |                 .map(|arg| arg.to_string())
    |                      ^^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `std::string::ToString::to_string`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls

warning: wildcard match will miss any future added variants
   --> src/scripts/mod.rs:256:17
    |
256 |                 _ => -1,
    |                 ^ help: try this: `subprocess::ExitStatus::Undetermined`
    |
note: the lint level is defined here
   --> src/main.rs:2:9
    |
2   | #![warn(clippy::pedantic)]
    |         ^^^^^^^^^^^^^^^^
    = note: `#[warn(clippy::match_wildcard_for_single_variants)]` implied by `#[warn(clippy::pedantic)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#match_wildcard_for_single_variants

warning: casting `u32` to `i32` may wrap around the value
   --> src/scripts/mod.rs:253:42
    |
253 |                 ExitStatus::Exited(c) => c as i32,
    |                                          ^^^^^^^^
    |
note: the lint level is defined here
   --> src/main.rs:2:9
    |
2   | #![warn(clippy::pedantic)]
    |         ^^^^^^^^^^^^^^^^
    = note: `#[warn(clippy::cast_possible_wrap)]` implied by `#[warn(clippy::pedantic)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cast_possible_wrap

warning: casting `u8` to `i32` may become silently lossy if you later change the type
   --> src/scripts/mod.rs:254:44
    |
254 |                 ExitStatus::Signaled(c) => c as i32,
    |                                            ^^^^^^^^ help: try: `i32::from(c)`
    |
note: the lint level is defined here
   --> src/main.rs:2:9
    |
2   | #![warn(clippy::pedantic)]
    |         ^^^^^^^^^^^^^^^^
    = note: `#[warn(clippy::cast_lossless)]` implied by `#[warn(clippy::pedantic)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cast_lossless

warning: usage of wildcard import
  --> src/main.rs:35:26
   |
35 | use trust_dns_resolver::{config::*, Resolver};
   |                          ^^^^^^^^^ help: try: `config::{ResolverConfig, ResolverOpts}`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wildcard_imports

warning: this function has a large number of lines
   --> src/main.rs:51:1
    |
51  | / fn main() {
52  | |     env_logger::init();
53  | |     let mut benchmarks = Benchmark::init();
54  | |     let mut rustscan_bench = NamedTimer::start("RustScan");
...   |
190 | |     info!("{}", benchmarks.summary());
191 | | }
    | |_^
    |
note: the lint level is defined here
   --> src/main.rs:2:9
    |
2   | #![warn(clippy::pedantic)]
    |         ^^^^^^^^^^^^^^^^
    = note: `#[warn(clippy::too_many_lines)]` implied by `#[warn(clippy::pedantic)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_lines

warning: it is more concise to loop over references to containers instead of using explicit iteration methods
   --> src/main.rs:127:24
    |
127 |     for (ip, ports) in ports_per_ip.iter_mut() {
    |                        ^^^^^^^^^^^^^^^^^^^^^^^ help: to write this more concisely, try: `&mut ports_per_ip`
    |
note: the lint level is defined here
   --> src/main.rs:2:9
    |
2   | #![warn(clippy::pedantic)]
    |         ^^^^^^^^^^^^^^^^
    = note: `#[warn(clippy::explicit_iter_loop)]` implied by `#[warn(clippy::pedantic)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#explicit_iter_loop

warning: redundant closure found
   --> src/main.rs:128:59
    |
128 |         let vec_str_ports: Vec<String> = ports.iter().map(|port| port.to_string()).collect();
    |                                                           ^^^^^^^^^^^^^^^^^^^^^^^ help: remove closure as shown: `std::string::ToString::to_string`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls

warning: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
   --> src/main.rs:315:9
    |
315 | /         match setrlimit(Resource::NOFILE, limit, limit) {
316 | |             Ok(_) => {
317 | |                 detail!(
318 | |                     format!("Automatically increasing ulimit value to {}.", limit),
...   |
329 | |             }
330 | |         }
    | |_________^
    |
note: the lint level is defined here
   --> src/main.rs:2:9
    |
2   | #![warn(clippy::pedantic)]
    |         ^^^^^^^^^^^^^^^^
    = note: `#[warn(clippy::single_match_else)]` implied by `#[warn(clippy::pedantic)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else
help: try this
    |
315 |         if let Ok(_) = setrlimit(Resource::NOFILE, limit, limit) {
316 |             detail!(
317 |                 format!("Automatically increasing ulimit value to {}.", limit),
318 |                 opts.greppable,
319 |                 opts.accessible
320 |             );
  ...

warning: casting `u64` to `u16` may truncate the value
   --> src/main.rs:371:5
    |
371 |     batch_size as u16
    |     ^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cast_possible_truncation

warning: 22 warnings emitted

    Finished dev [unoptimized + debuginfo] target(s) in 0.27s

I think some of these are really high quality messages, but not all of them are helpful. On this output for example, I personally disagree with clippy::if_not_else. I think over time, more allows will accumulate, but I would still keep the rest of them on (I mean look at how great some of these are 😄).

---

Are you saying that clippy is required right now? Like I said, I just adapted the .yml config to what was already there, but this is what I am seeing under PRs:

Am I not getting something here? Workflows are confusing me hard right now .. 🤣

[bee-san]

Ah no, you didn't upload the Clippy yml. We have Clippy that doesn't run (cause I commented it out) but it appears in the output. Your PR does not include a new github action for Clippy, so this exact PR's Actions do not include your Action :)

I think originally months ago we had Clippy, but I turned it off because I just wanted to force push out RustScan 1.0 haha!

I can tell you haven't included the GitHub action because of the committed files tab:
https://github.com/RustScan/RustScan/pull/294/files

If you can make a change and add it, that'll be great.

[niklasmohrin]

I don't fully understand what you mean, the edits to basic.yml are already merged in #287. I just cannot figure out why some jobs under Continuous integration show up as required and some do not although none of them have anything set where they are defined and I can't find any further references to the workflow elsewhere.

[bee-san]

I don't fully understand what you mean, the edits to basic.yml are already merged in #287. I just cannot figure out why some jobs under Continuous integration show up as required and some do not although none of them have anything set where they are defined and I can't find any further references to the workflow elsewhere.

You are right, I have literally no idea why it's not required :(

[niklasmohrin]

I think I have finally found something. All CI checks and things like "This branch is out-of-date with the base branch" are called "status checks" (making it rather hard to find the articles when only looking for articles on workflows..). The can be adjusted in the repository settings. Setting them to required can also be done there:

https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/enabling-required-status-checks

On the basic article (https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/about-required-status-checks), it also says that applications with certain access rights can also adjust these settings (including their own), which would explain why some are already required.

It would be nice if you required them from there :) (only if you agree of course)

[bee-san]

Wow! I must look like a right idiot! I enabled them, I think you might have to push again to see it? But Clippy is now 100% required!

[niklasmohrin]

Nah, I am just as confused by the weird naming 😓 It already shows up here, I will fix some existing warnings before marking ready.

[niklasmohrin]

Alright, I fixed every pedantic complaint (including one from nightly 🎉) and I think it is all well now. I also rebased onto the current master.

Somehow there is another clippy status check showing up for me:

The "CI / Clippy" one is from my last PR but the just "clippy" one is new (and it does not seem to report back). Is this intended?

[bee-san]

I fixed by making it non required. :) Also! If you didn't know, when you are ready for a PR to be submitted unmark it as a draft. Generally:

• Want comments on your PR but don't want it to be merged? Submit it as a draft and request a code review
• Want your PR to be merged? Undraft it / submit it normally

😄

Anywhooo~~~~
💖 congrats on this PR getting accepted! :D Thank you so much for fixing all the clippy linting, that must have taken you forever 😅

[niklasmohrin]

I was gonna mark it ready once the status thing was talked through, but this is fine too. Fixing was not too bad, as you saw I sometimes just allowed stuff (like main having too many lines).

Contributing here is fun with you guys always responding relatively quickly :)