Auto merge of #6829 - matthiaskrgr:lintcheck_unclippy, r=<try>

lintcheck: clippy fixes, test on ci

fixes clippy warnings in lintcheck and adds a small test (run on 3 small crates) to gha ci

changelog: none

Author: bors

.github/workflows/clippy_dev.yml

@@ -8,10 +8,10 @@ on:
   pull_request:
     # Only run on paths, that get checked by the clippy_dev tool
     paths:
-    - 'CHANGELOG.md'
-    - 'README.md'
-    - '**.stderr'
-    - '**.rs'
+      - "CHANGELOG.md"
+      - "README.md"
+      - "**.stderr"
+      - "**.rs"
 
 env:
   RUST_BACKTRACE: 1
@@ -21,35 +21,49 @@ jobs:
     runs-on: ubuntu-latest
 
     steps:
-    # Setup
-    - name: Checkout
-      uses: actions/[email protected]
-
-    - name: remove toolchain file
-      run: rm rust-toolchain
-
-    - name: rust-toolchain
-      uses: actions-rs/[email protected]
-      with:
-        toolchain: nightly
-        target: x86_64-unknown-linux-gnu
-        profile: minimal
-        components: rustfmt
-        default: true
-
-    # Run
-    - name: Build
-      run: cargo build --features deny-warnings
-      working-directory: clippy_dev
-
-    - name: Test limit_stderr_length
-      run: cargo dev limit_stderr_length
-
-    - name: Test update_lints
-      run: cargo dev update_lints --check
-
-    - name: Test fmt
-      run: cargo dev fmt --check
+      # Setup
+      - name: Checkout
+        uses: actions/[email protected]
+
+      # To test formatting, we need to temporarily disable the rust-toolchain file
+      - name: remove toolchain file
+        run: rm rust-toolchain
+
+      - name: rust-toolchain
+        uses: actions-rs/[email protected]
+        with:
+          toolchain: nightly
+          target: x86_64-unknown-linux-gnu
+          profile: minimal
+          components: rustfmt
+          default: true
+
+      - name: Test fmt
+        run: cargo dev fmt --check
+
+      # Restore the rust-toolchain-file
+      - name: Restore rust-toolchain-file
+        run: git checkout rust-toolchain
+
+      # Run
+      - name: Build
+        run: cargo build --features deny-warnings
+        working-directory: clippy_dev
+
+      - name: Test limit_stderr_length
+        run: cargo dev limit_stderr_length
+
+      - name: Test update_lints
+        run: cargo dev update_lints --check
+
+      # Lintcheck
+      - name: build lintcheck
+        run: cargo build --features lintcheck,deny-warnings
+        working-directory: clippy_dev
+
+      - name: test lintcheck
+        run: cargo test --features lintcheck,deny-warnings
+        working-directory: clippy_dev
 
   # These jobs doesn't actually test anything, but they're only used to tell
   # bors the build completed, as there is no practical way to detect when a

clippy_dev/ci_test_sources.toml

@@ -0,0 +1,4 @@
+[crates]
+cc = {name = "cc", versions = ['1.0.67']}
+home = {name = "home", git_url = "https://github.com/brson/home", git_hash = "32044e53dfbdcd32bafad3109d1fbab805fc0f40"}
+rustc_tools_util = {name = "rustc_tools_util", versions = ['0.2.0']}

clippy_dev/src/lintcheck.rs

@@ -5,14 +5,18 @@
 // positives.
 
 #![cfg(feature = "lintcheck")]
-#![allow(clippy::filter_map)]
+#![allow(clippy::filter_map, clippy::collapsible_else_if)]
 
 use crate::clippy_project_root;
 
 use std::collections::HashMap;
 use std::process::Command;
 use std::sync::atomic::{AtomicUsize, Ordering};
-use std::{env, fmt, fs::write, path::PathBuf};
+use std::{
+    env, fmt,
+    fs::write,
+    path::{Path, PathBuf},
+};
 
 use clap::ArgMatches;
 use rayon::prelude::*;
@@ -196,11 +200,9 @@ impl CrateSource {
                 if !crate_root.exists() {
                     println!("Copying {} to {}", path.display(), copy_dest.display());
 
-                    dir::copy(path, &copy_dest, &dir::CopyOptions::new()).expect(&format!(
-                        "Failed to copy from {}, to  {}",
-                        path.display(),
-                        crate_root.display()
-                    ));
+                    dir::copy(path, &copy_dest, &dir::CopyOptions::new()).unwrap_or_else(|_| {
+                        panic!("Failed to copy from {}, to  {}", path.display(), crate_root.display())
+                    });
                 } else {
                     println!(
                         "Not copying {} to {}, destination already exists",
@@ -225,7 +227,7 @@ impl Crate {
     /// issued
     fn run_clippy_lints(
         &self,
-        cargo_clippy_path: &PathBuf,
+        cargo_clippy_path: &Path,
         target_dir_index: &AtomicUsize,
         thread_limit: usize,
         total_crates_to_lint: usize,
@@ -308,13 +310,13 @@ impl LintcheckConfig {
         // first, check if we got anything passed via the LINTCHECK_TOML env var,
         // if not, ask clap if we got any value for --crates-toml  <foo>
         // if not, use the default "clippy_dev/lintcheck_crates.toml"
-        let sources_toml = env::var("LINTCHECK_TOML").unwrap_or(
+        let sources_toml = env::var("LINTCHECK_TOML").unwrap_or_else(|_| {
             clap_config
                 .value_of("crates-toml")
                 .clone()
                 .unwrap_or("clippy_dev/lintcheck_crates.toml")
-                .to_string(),
-        );
+                .to_string()
+        });
 
         let sources_toml_path = PathBuf::from(sources_toml);
 
@@ -328,9 +330,8 @@ impl LintcheckConfig {
         // by default use a single thread
         let max_jobs = match clap_config.value_of("threads") {
             Some(threads) => {
-                let threads: usize = threads
-                    .parse()
-                    .expect(&format!("Failed to parse '{}' to a digit", threads));
+                let err_msg = format!("Failed to parse '{}' to a digit", threads);
+                let threads: usize = threads.parse().expect(&err_msg);
                 if threads == 0 {
                     // automatic choice
                     // Rayon seems to return thread count so half that for core count
@@ -387,7 +388,7 @@ fn build_clippy() {
 }
 
 /// Read a `toml` file and return a list of `CrateSources` that we want to check with clippy
-fn read_crates(toml_path: &PathBuf) -> Vec<CrateSource> {
+fn read_crates(toml_path: &Path) -> Vec<CrateSource> {
     let toml_content: String =
         std::fs::read_to_string(&toml_path).unwrap_or_else(|_| panic!("Failed to read {}", toml_path.display()));
     let crate_list: SourceList =
@@ -499,7 +500,10 @@ fn gather_stats(clippy_warnings: &[ClippyWarning]) -> (String, HashMap<&String,
 
 /// check if the latest modification of the logfile is older than the modification date of the
 /// clippy binary, if this is true, we should clean the lintchec shared target directory and recheck
-fn lintcheck_needs_rerun(lintcheck_logs_path: &PathBuf) -> bool {
+fn lintcheck_needs_rerun(lintcheck_logs_path: &Path) -> bool {
+    if !lintcheck_logs_path.exists() {
+        return true;
+    }
     let clippy_modified: std::time::SystemTime = {
         let mut times = [CLIPPY_DRIVER_PATH, CARGO_CLIPPY_PATH].iter().map(|p| {
             std::fs::metadata(p)
@@ -533,15 +537,13 @@ pub fn run(clap_config: &ArgMatches) {
     // refresh the logs
     if lintcheck_needs_rerun(&config.lintcheck_results_path) {
         let shared_target_dir = "target/lintcheck/shared_target_dir";
-        match std::fs::metadata(&shared_target_dir) {
-            Ok(metadata) => {
-                if metadata.is_dir() {
-                    println!("Clippy is newer than lint check logs, clearing lintcheck shared target dir...");
-                    std::fs::remove_dir_all(&shared_target_dir)
-                        .expect("failed to remove target/lintcheck/shared_target_dir");
-                }
-            },
-            Err(_) => { /*  dir probably does not exist, don't remove anything  */ },
+        // if we get an Err here, the shared target dir probably does simply not exist
+        if let Ok(metadata) = std::fs::metadata(&shared_target_dir) {
+            if metadata.is_dir() {
+                println!("Clippy is newer than lint check logs, clearing lintcheck shared target dir...");
+                std::fs::remove_dir_all(&shared_target_dir)
+                    .expect("failed to remove target/lintcheck/shared_target_dir");
+            }
         }
     }
 
@@ -660,11 +662,10 @@ pub fn run(clap_config: &ArgMatches) {
 }
 
 /// read the previous stats from the lintcheck-log file
-fn read_stats_from_file(file_path: &PathBuf) -> HashMap<String, usize> {
+fn read_stats_from_file(file_path: &Path) -> HashMap<String, usize> {
     let file_content: String = match std::fs::read_to_string(file_path).ok() {
         Some(content) => content,
         None => {
-            eprintln!("RETURND");
             return HashMap::new();
         },
     };
@@ -678,9 +679,9 @@ fn read_stats_from_file(file_path: &PathBuf) -> HashMap<String, usize> {
     let stats_lines = &lines[start + 1..=end - 1];
 
     stats_lines
-        .into_iter()
+        .iter()
         .map(|line| {
-            let mut spl = line.split(" ").into_iter();
+            let mut spl = line.split(' ');
             (
                 spl.next().unwrap().to_string(),
                 spl.next().unwrap().parse::<usize>().unwrap(),
@@ -733,3 +734,32 @@ fn print_stats(old_stats: HashMap<String, usize>, new_stats: HashMap<&String, us
             println!("{} {} => 0", old_key, old_value);
         });
 }
+
+#[test]
+fn lintcheck_test() {
+    let args = [
+        "run",
+        "--target-dir",
+        "clippy_dev/target",
+        "--package",
+        "clippy_dev",
+        "--bin",
+        "clippy_dev",
+        "--manifest-path",
+        "clippy_dev/Cargo.toml",
+        "--features",
+        "lintcheck",
+        "--",
+        "lintcheck",
+        //"--",
+        "--crates-toml",
+        //"--",
+        "clippy_dev/ci_test_sources.toml",
+    ];
+    let status = std::process::Command::new("cargo")
+        .args(&args)
+        .current_dir("../" /* repo root */)
+        .status();
+
+    assert!(status.unwrap().success());
+}