Auto merge of #62037 - Mark-Simulacrum:tidy-fast, r=eddyb

Speed up tidy

master:
  Time (mean ± σ):      3.478 s ±  0.033 s    [User: 3.298 s, System: 0.178 s]
  Range (min … max):    3.425 s …  3.525 s    10 runs

This PR:
  Time (mean ± σ):      1.098 s ±  0.006 s    [User: 783.7 ms, System: 310.2 ms]
  Range (min … max):    1.092 s …  1.113 s    10 runs

Alleviates #59884. For the most part each commit stands on its own. Timings are on warm filesystem cache.

r? @eddyb

Cargo.lock

@@ -3801,9 +3801,11 @@ dependencies = [
 name = "tidy"
 version = "0.1.0"
 dependencies = [
+ "lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "regex 1.1.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "serde 1.0.92 (registry+https://github.com/rust-lang/crates.io-index)",
  "serde_json 1.0.33 (registry+https://github.com/rust-lang/crates.io-index)",
+ "walkdir 2.2.7 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]

src/bootstrap/test.rs

@@ -709,8 +709,8 @@ impl Step for Tidy {
         if !builder.config.vendor {
             cmd.arg("--no-vendor");
         }
-        if !builder.config.verbose_tests {
-            cmd.arg("--quiet");
+        if builder.is_verbose() {
+            cmd.arg("--verbose");
         }
 
         let _folder = builder.fold_output(|| "tidy");

src/tools/tidy/Cargo.toml

@@ -8,3 +8,5 @@ edition = "2018"
 regex = "1"
 serde = { version = "1.0.8", features = ["derive"] }
 serde_json = "1.0.2"
+lazy_static = "1"
+walkdir = "2"

src/tools/tidy/src/bins.rs

@@ -25,16 +25,17 @@ pub fn check(path: &Path, bad: &mut bool) {
         }
     }
 
-    super::walk(path,
+    super::walk_no_read(path,
                 &mut |path| super::filter_dirs(path) || path.ends_with("src/etc"),
-                &mut |file| {
+                &mut |entry| {
+        let file = entry.path();
         let filename = file.file_name().unwrap().to_string_lossy();
         let extensions = [".py", ".sh"];
         if extensions.iter().any(|e| filename.ends_with(e)) {
             return;
         }
 
-        let metadata = t!(fs::symlink_metadata(&file), &file);
+        let metadata = t!(entry.metadata(), file);
         if metadata.mode() & 0o111 != 0 {
             let rel_path = file.strip_prefix(path).unwrap();
             let git_friendly_path = rel_path.to_str().unwrap().replace("\\", "/");

src/tools/tidy/src/errors.rs

@@ -4,24 +4,19 @@
 //! statistics about the error codes.
 
 use std::collections::HashMap;
-use std::fs::File;
-use std::io::prelude::*;
 use std::path::Path;
 
 pub fn check(path: &Path, bad: &mut bool) {
-    let mut contents = String::new();
     let mut map: HashMap<_, Vec<_>> = HashMap::new();
     super::walk(path,
                 &mut |path| super::filter_dirs(path) || path.ends_with("src/test"),
-                &mut |file| {
+                &mut |entry, contents| {
+        let file = entry.path();
         let filename = file.file_name().unwrap().to_string_lossy();
         if filename != "error_codes.rs" {
             return
         }
 
-        contents.truncate(0);
-        t!(t!(File::open(file)).read_to_string(&mut contents));
-
         // In the `register_long_diagnostics!` macro, entries look like this:
         //
         // ```

src/tools/tidy/src/features.rs

@@ -11,11 +11,10 @@
 
 use std::collections::HashMap;
 use std::fmt;
-use std::fs::{self, File};
-use std::io::prelude::*;
+use std::fs;
 use std::path::Path;
 
-use regex::{Regex, escape};
+use regex::Regex;
 
 mod version;
 use version::Version;
@@ -51,20 +50,48 @@ pub struct Feature {
 
 pub type Features = HashMap<String, Feature>;
 
-pub fn check(path: &Path, bad: &mut bool, quiet: bool) {
+pub struct CollectedFeatures {
+    pub lib: Features,
+    pub lang: Features,
+}
+
+// Currently only used for unstable book generation
+pub fn collect_lib_features(base_src_path: &Path) -> Features {
+    let mut lib_features = Features::new();
+
+    // This library feature is defined in the `compiler_builtins` crate, which
+    // has been moved out-of-tree. Now it can no longer be auto-discovered by
+    // `tidy`, because we need to filter out its (submodule) directory. Manually
+    // add it to the set of known library features so we can still generate docs.
+    lib_features.insert("compiler_builtins_lib".to_owned(), Feature {
+        level: Status::Unstable,
+        since: None,
+        has_gate_test: false,
+        tracking_issue: None,
+    });
+
+    map_lib_features(base_src_path,
+                     &mut |res, _, _| {
+        if let Ok((name, feature)) = res {
+            lib_features.insert(name.to_owned(), feature);
+        }
+    });
+   lib_features
+}
+
+pub fn check(path: &Path, bad: &mut bool, verbose: bool) -> CollectedFeatures {
     let mut features = collect_lang_features(path, bad);
     assert!(!features.is_empty());
 
     let lib_features = get_and_check_lib_features(path, bad, &features);
     assert!(!lib_features.is_empty());
 
-    let mut contents = String::new();
-
     super::walk_many(&[&path.join("test/ui"),
                        &path.join("test/ui-fulldeps"),
                        &path.join("test/compile-fail")],
                      &mut |path| super::filter_dirs(path),
-                     &mut |file| {
+                     &mut |entry, contents| {
+        let file = entry.path();
         let filename = file.file_name().unwrap().to_string_lossy();
         if !filename.ends_with(".rs") || filename == "features.rs" ||
            filename == "diagnostic_list.rs" {
@@ -74,9 +101,6 @@ pub fn check(path: &Path, bad: &mut bool, quiet: bool) {
         let filen_underscore = filename.replace('-',"_").replace(".rs","");
         let filename_is_gate_test = test_filen_gate(&filen_underscore, &mut features);
 
-        contents.truncate(0);
-        t!(t!(File::open(&file), &file).read_to_string(&mut contents));
-
         for (i, line) in contents.lines().enumerate() {
             let mut err = |msg: &str| {
                 tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg);
@@ -130,21 +154,23 @@ pub fn check(path: &Path, bad: &mut bool, quiet: bool) {
     }
 
     if *bad {
-        return;
-    }
-    if quiet {
-        println!("* {} features", features.len());
-        return;
+        return CollectedFeatures { lib: lib_features, lang: features };
     }
 
-    let mut lines = Vec::new();
-    lines.extend(format_features(&features, "lang"));
-    lines.extend(format_features(&lib_features, "lib"));
+    if verbose {
+        let mut lines = Vec::new();
+        lines.extend(format_features(&features, "lang"));
+        lines.extend(format_features(&lib_features, "lib"));
 
-    lines.sort();
-    for line in lines {
-        println!("* {}", line);
+        lines.sort();
+        for line in lines {
+            println!("* {}", line);
+        }
+    } else {
+        println!("* {} features", features.len());
     }
+
+    CollectedFeatures { lib: lib_features, lang: features }
 }
 
 fn format_features<'a>(features: &'a Features, family: &'a str) -> impl Iterator<Item = String> + 'a {
@@ -159,8 +185,19 @@ fn format_features<'a>(features: &'a Features, family: &'a str) -> impl Iterator
 }
 
 fn find_attr_val<'a>(line: &'a str, attr: &str) -> Option<&'a str> {
-    let r = Regex::new(&format!(r#"{}\s*=\s*"([^"]*)""#, escape(attr)))
-        .expect("malformed regex for find_attr_val");
+    lazy_static::lazy_static! {
+        static ref ISSUE: Regex = Regex::new(r#"issue\s*=\s*"([^"]*)""#).unwrap();
+        static ref FEATURE: Regex = Regex::new(r#"feature\s*=\s*"([^"]*)""#).unwrap();
+        static ref SINCE: Regex = Regex::new(r#"since\s*=\s*"([^"]*)""#).unwrap();
+    }
+
+    let r = match attr {
+        "issue" => &*ISSUE,
+        "feature" => &*FEATURE,
+        "since" => &*SINCE,
+        _ => unimplemented!("{} not handled", attr),
+    };
+
     r.captures(line)
         .and_then(|c| c.get(1))
         .map(|m| m.as_str())
@@ -175,9 +212,11 @@ fn test_find_attr_val() {
 }
 
 fn test_filen_gate(filen_underscore: &str, features: &mut Features) -> bool {
-    if filen_underscore.starts_with("feature_gate") {
+    let prefix = "feature_gate_";
+    if filen_underscore.starts_with(prefix) {
         for (n, f) in features.iter_mut() {
-            if filen_underscore == format!("feature_gate_{}", n) {
+            // Equivalent to filen_underscore == format!("feature_gate_{}", n)
+            if &filen_underscore[prefix.len()..] == n {
                 f.has_gate_test = true;
                 return true;
             }
@@ -295,32 +334,6 @@ pub fn collect_lang_features(base_src_path: &Path, bad: &mut bool) -> Features {
         .collect()
 }
 
-pub fn collect_lib_features(base_src_path: &Path) -> Features {
-    let mut lib_features = Features::new();
-
-    // This library feature is defined in the `compiler_builtins` crate, which
-    // has been moved out-of-tree. Now it can no longer be auto-discovered by
-    // `tidy`, because we need to filter out its (submodule) directory. Manually
-    // add it to the set of known library features so we can still generate docs.
-    lib_features.insert("compiler_builtins_lib".to_owned(), Feature {
-        level: Status::Unstable,
-        since: None,
-        has_gate_test: false,
-        tracking_issue: None,
-    });
-
-    map_lib_features(base_src_path,
-                     &mut |res, _, _| {
-        if let Ok((name, feature)) = res {
-            if lib_features.contains_key(name) {
-                return;
-            }
-            lib_features.insert(name.to_owned(), feature);
-        }
-    });
-   lib_features
-}
-
 fn get_and_check_lib_features(base_src_path: &Path,
                               bad: &mut bool,
                               lang_features: &Features) -> Features {
@@ -355,20 +368,25 @@ fn get_and_check_lib_features(base_src_path: &Path,
 
 fn map_lib_features(base_src_path: &Path,
                     mf: &mut dyn FnMut(Result<(&str, Feature), &str>, &Path, usize)) {
-    let mut contents = String::new();
     super::walk(base_src_path,
                 &mut |path| super::filter_dirs(path) || path.ends_with("src/test"),
-                &mut |file| {
+                &mut |entry, contents| {
+        let file = entry.path();
         let filename = file.file_name().unwrap().to_string_lossy();
         if !filename.ends_with(".rs") || filename == "features.rs" ||
            filename == "diagnostic_list.rs" {
             return;
         }
 
-        contents.truncate(0);
-        t!(t!(File::open(&file), &file).read_to_string(&mut contents));
+        // This is an early exit -- all the attributes we're concerned with must contain this:
+        // * rustc_const_unstable(
+        // * unstable(
+        // * stable(
+        if !contents.contains("stable(") {
+            return;
+        }
 
-        let mut becoming_feature: Option<(String, Feature)> = None;
+        let mut becoming_feature: Option<(&str, Feature)> = None;
         for (i, line) in contents.lines().enumerate() {
             macro_rules! err {
                 ($msg:expr) => {{
@@ -447,7 +465,7 @@ fn map_lib_features(base_src_path: &Path,
             if line.contains(']') {
                 mf(Ok((feature_name, feature)), file, i + 1);
             } else {
-                becoming_feature = Some((feature_name.to_owned(), feature));
+                becoming_feature = Some((feature_name, feature));
             }
         }
     });

src/tools/tidy/src/lib.rs

@@ -3,7 +3,9 @@
 //! This library contains the tidy lints and exposes it
 //! to be used by tools.
 
-use std::fs;
+use walkdir::{DirEntry, WalkDir};
+use std::fs::File;
+use std::io::Read;
 
 use std::path::Path;
 
@@ -65,25 +67,35 @@ fn filter_dirs(path: &Path) -> bool {
     skip.iter().any(|p| path.ends_with(p))
 }
 
-fn walk_many(paths: &[&Path], skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&Path)) {
+
+fn walk_many(
+    paths: &[&Path], skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&DirEntry, &str)
+) {
     for path in paths {
         walk(path, skip, f);
     }
 }
 
-fn walk(path: &Path, skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&Path)) {
-    if let Ok(dir) = fs::read_dir(path) {
-        for entry in dir {
-            let entry = t!(entry);
-            let kind = t!(entry.file_type());
-            let path = entry.path();
-            if kind.is_dir() {
-                if !skip(&path) {
-                    walk(&path, skip, f);
-                }
-            } else {
-                f(&path);
+fn walk(path: &Path, skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&DirEntry, &str)) {
+    let mut contents = String::new();
+    walk_no_read(path, skip, &mut |entry| {
+        contents.clear();
+        if t!(File::open(entry.path()), entry.path()).read_to_string(&mut contents).is_err() {
+            contents.clear();
+        }
+        f(&entry, &contents);
+    });
+}
+
+fn walk_no_read(path: &Path, skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&DirEntry)) {
+    let walker = WalkDir::new(path).into_iter()
+        .filter_entry(|e| !skip(e.path()));
+    for entry in walker {
+        if let Ok(entry) = entry {
+            if entry.file_type().is_dir() {
+                continue;
             }
+            f(&entry);
         }
     }
 }

src/tools/tidy/src/libcoretest.rs

@@ -4,29 +4,22 @@
 //! item. All tests must be written externally in `libcore/tests`.
 
 use std::path::Path;
-use std::fs::read_to_string;
 
 pub fn check(path: &Path, bad: &mut bool) {
     let libcore_path = path.join("libcore");
     super::walk(
         &libcore_path,
         &mut |subpath| t!(subpath.strip_prefix(&libcore_path)).starts_with("tests"),
-        &mut |subpath| {
+        &mut |entry, contents| {
+            let subpath = entry.path();
             if let Some("rs") = subpath.extension().and_then(|e| e.to_str()) {
-                match read_to_string(subpath) {
-                    Ok(contents) => {
-                        if contents.contains("#[test]") {
-                            tidy_error!(
-                                bad,
-                                "{} contains #[test]; libcore tests must be placed inside \
-                                `src/libcore/tests/`",
-                                subpath.display()
-                            );
-                        }
-                    }
-                    Err(err) => {
-                        panic!("failed to read file {:?}: {}", subpath, err);
-                    }
+                if contents.contains("#[test]") {
+                    tidy_error!(
+                        bad,
+                        "{} contains #[test]; libcore tests must be placed inside \
+                        `src/libcore/tests/`",
+                        subpath.display()
+                    );
                 }
             }
         },