Auto merge of rust-lang#6744 - matthiaskrgr:lintcheck, r=flip1995

more lintcheck updates

* do some refactoring and renaming here and there
* add comments to functions
* fix bug where git repos would not get checked out to the proper commits (cmd was not actually run in repo directory 😅 )
* print warnings if we can't clone or check out a git repo
* filter out noise from cargo-metadata errors and lint messages that contained absolute file paths (these would change with every pinned-nightly bump, polluting the logs)

changelog: more lintcheck refactoring and fixes for git crates

clippy_dev/src/lintcheck.rs

@@ -17,14 +17,14 @@ use clap::ArgMatches;
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
 
-// use this to store the crates when interacting with the crates.toml file
+/// List of sources to check, loaded from a .toml file
 #[derive(Debug, Serialize, Deserialize)]
-struct CrateList {
+struct SourceList {
     crates: HashMap<String, TomlCrate>,
 }
 
-// crate data we stored in the toml, can have multiple versions per crate
-// A single TomlCrate is laster mapped to several CrateSources in that case
+/// A crate source stored inside the .toml
+/// will be translated into on one of the `CrateSource` variants
 #[derive(Debug, Serialize, Deserialize)]
 struct TomlCrate {
     name: String,
@@ -34,17 +34,16 @@ struct TomlCrate {
     path: Option<String>,
 }
 
-// represents an archive we download from crates.io, or a git repo, or a local repo
+/// Represents an archive we download from crates.io, or a git repo, or a local repo/folder
+/// Once processed (downloaded/extracted/cloned/copied...), this will be translated into a `Crate`
 #[derive(Debug, Serialize, Deserialize, Eq, Hash, PartialEq)]
 enum CrateSource {
     CratesIo { name: String, version: String },
     Git { name: String, url: String, commit: String },
     Path { name: String, path: PathBuf },
 }
 
-// represents the extracted sourcecode of a crate
-// we actually don't need to special-case git repos here because it does not matter for clippy, yay!
-// (clippy only needs a simple path)
+/// Represents the actual source code of a crate that we ran "cargo clippy" on
 #[derive(Debug)]
 struct Crate {
     version: String,
@@ -53,6 +52,7 @@ struct Crate {
     path: PathBuf,
 }
 
+/// A single warning that clippy issued while checking a `Crate`
 #[derive(Debug)]
 struct ClippyWarning {
     crate_name: String,
@@ -62,7 +62,7 @@ struct ClippyWarning {
     column: String,
     linttype: String,
     message: String,
-    ice: bool,
+    is_ice: bool,
 }
 
 impl std::fmt::Display for ClippyWarning {
@@ -76,6 +76,9 @@ impl std::fmt::Display for ClippyWarning {
 }
 
 impl CrateSource {
+    /// Makes the sources available on the disk for clippy to check.
+    /// Clones a git repo and checks out the specified commit or downloads a crate from crates.io or
+    /// copies a local folder
     fn download_and_extract(&self) -> Crate {
         match self {
             CrateSource::CratesIo { name, version } => {
@@ -122,19 +125,28 @@ impl CrateSource {
                 // clone the repo if we have not done so
                 if !repo_path.is_dir() {
                     println!("Cloning {} and checking out {}", url, commit);
-                    Command::new("git")
+                    if !Command::new("git")
                         .arg("clone")
                         .arg(url)
                         .arg(&repo_path)
-                        .output()
-                        .expect("Failed to clone git repo!");
+                        .status()
+                        .expect("Failed to clone git repo!")
+                        .success()
+                    {
+                        eprintln!("Failed to clone {} into {}", url, repo_path.display())
+                    }
                 }
                 // check out the commit/branch/whatever
-                Command::new("git")
+                if !Command::new("git")
                     .arg("checkout")
                     .arg(commit)
-                    .output()
-                    .expect("Failed to check out commit");
+                    .current_dir(&repo_path)
+                    .status()
+                    .expect("Failed to check out commit")
+                    .success()
+                {
+                    eprintln!("Failed to checkout {} of repo at {}", commit, repo_path.display())
+                }
 
                 Crate {
                     version: commit.clone(),
@@ -178,6 +190,8 @@ impl CrateSource {
 }
 
 impl Crate {
+    /// Run `cargo clippy` on the `Crate` and collect and return all the lint warnings that clippy
+    /// issued
     fn run_clippy_lints(&self, cargo_clippy_path: &PathBuf) -> Vec<ClippyWarning> {
         println!("Linting {} {}...", &self.name, &self.version);
         let cargo_clippy_path = std::fs::canonicalize(cargo_clippy_path).unwrap();
@@ -211,21 +225,45 @@ impl Crate {
         let warnings: Vec<ClippyWarning> = output_lines
             .into_iter()
             // get all clippy warnings and ICEs
-            .filter(|line| line.contains("clippy::") || line.contains("internal compiler error: "))
+            .filter(|line| filter_clippy_warnings(&line))
             .map(|json_msg| parse_json_message(json_msg, &self))
             .collect();
         warnings
     }
 }
 
+/// takes a single json-formatted clippy warnings and returns true (we are interested in that line)
+/// or false (we aren't)
+fn filter_clippy_warnings(line: &str) -> bool {
+    // we want to collect ICEs because clippy might have crashed.
+    // these are summarized later
+    if line.contains("internal compiler error: ") {
+        return true;
+    }
+    // in general, we want all clippy warnings
+    // however due to some kind of bug, sometimes there are absolute paths
+    // to libcore files inside the message
+    // or we end up with cargo-metadata output (https://github.com/rust-lang/rust-clippy/issues/6508)
+
+    // filter out these message to avoid unnecessary noise in the logs
+    if line.contains("clippy::")
+        && !(line.contains("could not read cargo metadata")
+            || (line.contains(".rustup") && line.contains("toolchains")))
+    {
+        return true;
+    }
+    false
+}
+
+/// Builds clippy inside the repo to make sure we have a clippy executable we can use.
 fn build_clippy() {
     Command::new("cargo")
         .arg("build")
         .output()
         .expect("Failed to build clippy!");
 }
 
-// get a list of CrateSources we want to check from a "lintcheck_crates.toml" file.
+/// Read a `toml` file and return a list of `CrateSources` that we want to check with clippy
 fn read_crates(toml_path: Option<&str>) -> (String, Vec<CrateSource>) {
     let toml_path = PathBuf::from(
         env::var("LINTCHECK_TOML").unwrap_or(toml_path.unwrap_or("clippy_dev/lintcheck_crates.toml").to_string()),
@@ -234,7 +272,7 @@ fn read_crates(toml_path: Option<&str>) -> (String, Vec<CrateSource>) {
     let toml_filename = toml_path.file_stem().unwrap().to_str().unwrap().to_string();
     let toml_content: String =
         std::fs::read_to_string(&toml_path).unwrap_or_else(|_| panic!("Failed to read {}", toml_path.display()));
-    let crate_list: CrateList =
+    let crate_list: SourceList =
         toml::from_str(&toml_content).unwrap_or_else(|e| panic!("Failed to parse {}: \n{}", toml_path.display(), e));
     // parse the hashmap of the toml file into a list of crates
     let tomlcrates: Vec<TomlCrate> = crate_list
@@ -288,7 +326,7 @@ fn read_crates(toml_path: Option<&str>) -> (String, Vec<CrateSource>) {
     (toml_filename, crate_sources)
 }
 
-// extract interesting data from a json lint message
+/// Parse the json output of clippy and return a `ClippyWarning`
 fn parse_json_message(json_message: &str, krate: &Crate) -> ClippyWarning {
     let jmsg: Value = serde_json::from_str(&json_message).unwrap_or_else(|e| panic!("Failed to parse json:\n{:?}", e));
 
@@ -309,11 +347,31 @@ fn parse_json_message(json_message: &str, krate: &Crate) -> ClippyWarning {
             .into(),
         linttype: jmsg["message"]["code"]["code"].to_string().trim_matches('"').into(),
         message: jmsg["message"]["message"].to_string().trim_matches('"').into(),
-        ice: json_message.contains("internal compiler error: "),
+        is_ice: json_message.contains("internal compiler error: "),
     }
 }
 
-// the main fn
+/// Generate a short list of occuring lints-types and their count
+fn gather_stats(clippy_warnings: &[ClippyWarning]) -> String {
+    // count lint type occurrences
+    let mut counter: HashMap<&String, usize> = HashMap::new();
+    clippy_warnings
+        .iter()
+        .for_each(|wrn| *counter.entry(&wrn.linttype).or_insert(0) += 1);
+
+    // collect into a tupled list for sorting
+    let mut stats: Vec<(&&String, &usize)> = counter.iter().map(|(lint, count)| (lint, count)).collect();
+    // sort by "000{count} {clippy::lintname}"
+    // to not have a lint with 200 and 2 warnings take the same spot
+    stats.sort_by_key(|(lint, count)| format!("{:0>4}, {}", count, lint));
+
+    stats
+        .iter()
+        .map(|(lint, count)| format!("{} {}\n", lint, count))
+        .collect::<String>()
+}
+
+/// lintchecks `main()` function
 pub fn run(clap_config: &ArgMatches) {
     let cargo_clippy_path: PathBuf = PathBuf::from("target/debug/cargo-clippy");
 
@@ -374,32 +432,16 @@ pub fn run(clap_config: &ArgMatches) {
             .collect()
     };
 
-    // generate some stats:
+    // generate some stats
+    let stats_formatted = gather_stats(&clippy_warnings);
 
     // grab crashes/ICEs, save the crate name and the ice message
     let ices: Vec<(&String, &String)> = clippy_warnings
         .iter()
-        .filter(|warning| warning.ice)
+        .filter(|warning| warning.is_ice)
         .map(|w| (&w.crate_name, &w.message))
         .collect();
 
-    // count lint type occurrences
-    let mut counter: HashMap<&String, usize> = HashMap::new();
-    clippy_warnings
-        .iter()
-        .for_each(|wrn| *counter.entry(&wrn.linttype).or_insert(0) += 1);
-
-    // collect into a tupled list for sorting
-    let mut stats: Vec<(&&String, &usize)> = counter.iter().map(|(lint, count)| (lint, count)).collect();
-    // sort by "000{count} {clippy::lintname}"
-    // to not have a lint with 200 and 2 warnings take the same spot
-    stats.sort_by_key(|(lint, count)| format!("{:0>4}, {}", count, lint));
-
-    let stats_formatted: String = stats
-        .iter()
-        .map(|(lint, count)| format!("{} {}\n", lint, count))
-        .collect::<String>();
-
     let mut all_msgs: Vec<String> = clippy_warnings.iter().map(|warning| warning.to_string()).collect();
     all_msgs.sort();
     all_msgs.push("\n\n\n\nStats\n\n".into());