Speed up file walking in tidy

jyn514 · jyn514 · commit 9b606a3203da · 2023-03-18T00:38:17.000-05:00
- Skip files in `skip` wherever possible to avoid reading their contents
- Don't look for `tidy-alphabetic-start` in tests. It's never currently used and slows the check down a lot.
- Add new `filter_not_rust` helper function
diff --git a/src/tools/tidy/src/debug_artifacts.rs b/src/tools/tidy/src/debug_artifacts.rs
@@ -1,21 +1,15 @@
 //! Tidy check to prevent creation of unnecessary debug artifacts while running tests.
 
-use crate::walk::{filter_dirs, walk};
+use crate::walk::{filter_dirs, filter_not_rust, walk};
 use std::path::Path;
 
 const GRAPHVIZ_POSTFLOW_MSG: &str = "`borrowck_graphviz_postflow` attribute in test";
 
 pub fn check(test_dir: &Path, bad: &mut bool) {
-    walk(test_dir, filter_dirs, &mut |entry, contents| {
-        let filename = entry.path();
-        let is_rust = filename.extension().map_or(false, |ext| ext == "rs");
-        if !is_rust {
-            return;
-        }
-
+    walk(test_dir, |path| filter_dirs(path) || filter_not_rust(path), &mut |entry, contents| {
         for (i, line) in contents.lines().enumerate() {
             if line.contains("borrowck_graphviz_postflow") {
-                tidy_error!(bad, "{}:{}: {}", filename.display(), i + 1, GRAPHVIZ_POSTFLOW_MSG);
+                tidy_error!(bad, "{}:{}: {}", entry.path().display(), i + 1, GRAPHVIZ_POSTFLOW_MSG);
             }
         }
     });
diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs
@@ -9,8 +9,9 @@
 //! * All unstable lang features have tests to ensure they are actually unstable.
 //! * Language features in a group are sorted by feature name.
 
-use crate::walk::{filter_dirs, walk, walk_many};
+use crate::walk::{filter_dirs, filter_not_rust, walk, walk_many};
 use std::collections::hash_map::{Entry, HashMap};
+use std::ffi::OsStr;
 use std::fmt;
 use std::fs;
 use std::num::NonZeroU32;
@@ -101,17 +102,15 @@ pub fn check(
             &tests_path.join("rustdoc-ui"),
             &tests_path.join("rustdoc"),
         ],
-        filter_dirs,
+        |path| {
+            filter_dirs(path)
+                || filter_not_rust(path)
+                || path.file_name() == Some(OsStr::new("features.rs"))
+                || path.file_name() == Some(OsStr::new("diagnostic_list.rs"))
+        },
         &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;
-            }
-
             let filen_underscore = filename.replace('-', "_").replace(".rs", "");
             let filename_is_gate_test = test_filen_gate(&filen_underscore, &mut features);
 
diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs
@@ -120,7 +120,6 @@ fn main() {
         check!(edition, &library_path);
 
         check!(alphabetical, &src_path);
-        check!(alphabetical, &tests_path);
         check!(alphabetical, &compiler_path);
         check!(alphabetical, &library_path);
 
diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs
@@ -19,7 +19,7 @@
 
 use crate::walk::{filter_dirs, walk};
 use regex::{Regex, RegexSet};
-use std::path::Path;
+use std::{ffi::OsStr, path::Path};
 
 /// Error code markdown is restricted to 80 columns because they can be
 /// displayed on the console with --example.
@@ -228,21 +228,28 @@ fn is_unexplained_ignore(extension: &str, line: &str) -> bool {
 
 pub fn check(path: &Path, bad: &mut bool) {
     fn skip(path: &Path) -> bool {
-        filter_dirs(path) || skip_markdown_path(path)
+        if filter_dirs(path) || skip_markdown_path(path) {
+            return true;
+        }
+
+        let extensions = ["rs", "py", "js", "sh", "c", "cpp", "h", "md", "css", "ftl", "goml"];
+        if extensions.iter().all(|e| path.extension() != Some(OsStr::new(e))) {
+            return true;
+        }
+
+        // We only check CSS files in rustdoc.
+        path.extension().map_or(false, |e| e == "css") && !is_in(path, "src", "librustdoc")
     }
+
     let problematic_consts_strings: Vec<String> = (PROBLEMATIC_CONSTS.iter().map(u32::to_string))
         .chain(PROBLEMATIC_CONSTS.iter().map(|v| format!("{:x}", v)))
         .chain(PROBLEMATIC_CONSTS.iter().map(|v| format!("{:X}", v)))
         .collect();
     let problematic_regex = RegexSet::new(problematic_consts_strings.as_slice()).unwrap();
+
     walk(path, skip, &mut |entry, contents| {
         let file = entry.path();
         let filename = file.file_name().unwrap().to_string_lossy();
-        let extensions =
-            [".rs", ".py", ".js", ".sh", ".c", ".cpp", ".h", ".md", ".css", ".ftl", ".goml"];
-        if extensions.iter().all(|e| !filename.ends_with(e)) || filename.starts_with(".#") {
-            return;
-        }
 
         let is_style_file = filename.ends_with(".css");
         let under_rustfmt = filename.ends_with(".rs") &&
@@ -253,11 +260,6 @@ pub fn check(path: &Path, bad: &mut bool) {
                     a.ends_with("src/doc/book")
             });
 
-        if is_style_file && !is_in(file, "src", "librustdoc") {
-            // We only check CSS files in rustdoc.
-            return;
-        }
-
         if contents.is_empty() {
             tidy_error!(bad, "{}: empty file", file.display());
         }
diff --git a/src/tools/tidy/src/target_specific_tests.rs b/src/tools/tidy/src/target_specific_tests.rs
@@ -4,6 +4,8 @@
 use std::collections::BTreeMap;
 use std::path::Path;
 
+use crate::walk::filter_not_rust;
+
 const COMMENT: &str = "//";
 const LLVM_COMPONENTS_HEADER: &str = "needs-llvm-components:";
 const COMPILE_FLAGS_HEADER: &str = "compile-flags:";
@@ -35,61 +37,57 @@ struct RevisionInfo<'a> {
 }
 
 pub fn check(path: &Path, bad: &mut bool) {
-    crate::walk::walk(
-        path,
-        |path| path.extension().map(|p| p == "rs") == Some(false),
-        &mut |entry, content| {
-            let file = entry.path().display();
-            let mut header_map = BTreeMap::new();
-            iter_header(content, &mut |cfg, directive| {
-                if let Some(value) = directive.strip_prefix(LLVM_COMPONENTS_HEADER) {
-                    let info = header_map.entry(cfg).or_insert(RevisionInfo::default());
-                    let comp_vec = info.llvm_components.get_or_insert(Vec::new());
-                    for component in value.split(' ') {
-                        let component = component.trim();
-                        if !component.is_empty() {
-                            comp_vec.push(component);
-                        }
-                    }
-                } else if directive.starts_with(COMPILE_FLAGS_HEADER) {
-                    let compile_flags = &directive[COMPILE_FLAGS_HEADER.len()..];
-                    if let Some((_, v)) = compile_flags.split_once("--target") {
-                        if let Some((arch, _)) =
-                            v.trim_start_matches(|c| c == ' ' || c == '=').split_once("-")
-                        {
-                            let info = header_map.entry(cfg).or_insert(RevisionInfo::default());
-                            info.target_arch.replace(arch);
-                        } else {
-                            eprintln!("{file}: seems to have a malformed --target value");
-                            *bad = true;
-                        }
+    crate::walk::walk(path, filter_not_rust, &mut |entry, content| {
+        let file = entry.path().display();
+        let mut header_map = BTreeMap::new();
+        iter_header(content, &mut |cfg, directive| {
+            if let Some(value) = directive.strip_prefix(LLVM_COMPONENTS_HEADER) {
+                let info = header_map.entry(cfg).or_insert(RevisionInfo::default());
+                let comp_vec = info.llvm_components.get_or_insert(Vec::new());
+                for component in value.split(' ') {
+                    let component = component.trim();
+                    if !component.is_empty() {
+                        comp_vec.push(component);
                     }
                 }
-            });
-            for (rev, RevisionInfo { target_arch, llvm_components }) in &header_map {
-                let rev = rev.unwrap_or("[unspecified]");
-                match (target_arch, llvm_components) {
-                    (None, None) => {}
-                    (Some(_), None) => {
-                        eprintln!(
-                            "{}: revision {} should specify `{}` as it has `--target` set",
-                            file, rev, LLVM_COMPONENTS_HEADER
-                        );
+            } else if directive.starts_with(COMPILE_FLAGS_HEADER) {
+                let compile_flags = &directive[COMPILE_FLAGS_HEADER.len()..];
+                if let Some((_, v)) = compile_flags.split_once("--target") {
+                    if let Some((arch, _)) =
+                        v.trim_start_matches(|c| c == ' ' || c == '=').split_once("-")
+                    {
+                        let info = header_map.entry(cfg).or_insert(RevisionInfo::default());
+                        info.target_arch.replace(arch);
+                    } else {
+                        eprintln!("{file}: seems to have a malformed --target value");
                         *bad = true;
                     }
-                    (None, Some(_)) => {
-                        eprintln!(
-                            "{}: revision {} should not specify `{}` as it doesn't need `--target`",
-                            file, rev, LLVM_COMPONENTS_HEADER
-                        );
-                        *bad = true;
-                    }
-                    (Some(_), Some(_)) => {
-                        // FIXME: check specified components against the target architectures we
-                        // gathered.
-                    }
                 }
             }
-        },
-    );
+        });
+        for (rev, RevisionInfo { target_arch, llvm_components }) in &header_map {
+            let rev = rev.unwrap_or("[unspecified]");
+            match (target_arch, llvm_components) {
+                (None, None) => {}
+                (Some(_), None) => {
+                    eprintln!(
+                        "{}: revision {} should specify `{}` as it has `--target` set",
+                        file, rev, LLVM_COMPONENTS_HEADER
+                    );
+                    *bad = true;
+                }
+                (None, Some(_)) => {
+                    eprintln!(
+                        "{}: revision {} should not specify `{}` as it doesn't need `--target`",
+                        file, rev, LLVM_COMPONENTS_HEADER
+                    );
+                    *bad = true;
+                }
+                (Some(_), Some(_)) => {
+                    // FIXME: check specified components against the target architectures we
+                    // gathered.
+                }
+            }
+        }
+    });
 }
diff --git a/src/tools/tidy/src/walk.rs b/src/tools/tidy/src/walk.rs
@@ -1,6 +1,6 @@
 use ignore::DirEntry;
 
-use std::{fs::File, io::Read, path::Path};
+use std::{ffi::OsStr, fs::File, io::Read, path::Path};
 
 /// The default directory filter.
 pub fn filter_dirs(path: &Path) -> bool {
@@ -33,6 +33,11 @@ pub fn filter_dirs(path: &Path) -> bool {
     skip.iter().any(|p| path.ends_with(p))
 }
 
+/// Filter for only files that end in `.rs`.
+pub fn filter_not_rust(path: &Path) -> bool {
+    !path.is_dir() && path.extension() != Some(OsStr::new("rs"))
+}
+
 pub fn walk_many(
     paths: &[&Path],
     skip: impl Clone + Send + Sync + 'static + Fn(&Path) -> bool,
