Don't skip all directories when tidy-checking

WaffleLapkin · WaffleLapkin · commit 436afdf1feab · 2023-03-27T18:52:47.000Z
diff --git a/src/tools/replace-version-placeholder/src/main.rs b/src/tools/replace-version-placeholder/src/main.rs
@@ -10,7 +10,7 @@ fn main() {
     let version_str = version_str.trim();
     walk::walk(
         &root_path,
-        |path| {
+        |path, _is_dir| {
             walk::filter_dirs(path)
                 // We exempt these as they require the placeholder
                 // for their operation
diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs
@@ -95,7 +95,7 @@ fn check_section<'a>(
 }
 
 pub fn check(path: &Path, bad: &mut bool) {
-    walk(path, filter_dirs, &mut |entry, contents| {
+    walk(path, |path, _is_dir| filter_dirs(path), &mut |entry, contents| {
         let file = &entry.path().display();
 
         let mut lines = contents.lines().enumerate().peekable();
diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs
@@ -103,36 +103,40 @@ mod os_impl {
 
         // FIXME: we don't need to look at all binaries, only files that have been modified in this branch
         // (e.g. using `git ls-files`).
-        walk_no_read(&[path], |path| filter_dirs(path) || path.ends_with("src/etc"), &mut |entry| {
-            let file = entry.path();
-            let extension = file.extension();
-            let scripts = ["py", "sh", "ps1"];
-            if scripts.into_iter().any(|e| extension == Some(OsStr::new(e))) {
-                return;
-            }
-
-            if t!(is_executable(&file), file) {
-                let rel_path = file.strip_prefix(path).unwrap();
-                let git_friendly_path = rel_path.to_str().unwrap().replace("\\", "/");
-
-                if ALLOWED.contains(&git_friendly_path.as_str()) {
+        walk_no_read(
+            &[path],
+            |path, _is_dir| filter_dirs(path) || path.ends_with("src/etc"),
+            &mut |entry| {
+                let file = entry.path();
+                let extension = file.extension();
+                let scripts = ["py", "sh", "ps1"];
+                if scripts.into_iter().any(|e| extension == Some(OsStr::new(e))) {
                     return;
                 }
 
-                let output = Command::new("git")
-                    .arg("ls-files")
-                    .arg(&git_friendly_path)
-                    .current_dir(path)
-                    .stderr(Stdio::null())
-                    .output()
-                    .unwrap_or_else(|e| {
-                        panic!("could not run git ls-files: {e}");
-                    });
-                let path_bytes = rel_path.as_os_str().as_bytes();
-                if output.status.success() && output.stdout.starts_with(path_bytes) {
-                    tidy_error!(bad, "binary checked into source: {}", file.display());
+                if t!(is_executable(&file), file) {
+                    let rel_path = file.strip_prefix(path).unwrap();
+                    let git_friendly_path = rel_path.to_str().unwrap().replace("\\", "/");
+
+                    if ALLOWED.contains(&git_friendly_path.as_str()) {
+                        return;
+                    }
+
+                    let output = Command::new("git")
+                        .arg("ls-files")
+                        .arg(&git_friendly_path)
+                        .current_dir(path)
+                        .stderr(Stdio::null())
+                        .output()
+                        .unwrap_or_else(|e| {
+                            panic!("could not run git ls-files: {e}");
+                        });
+                    let path_bytes = rel_path.as_os_str().as_bytes();
+                    if output.status.success() && output.stdout.starts_with(path_bytes) {
+                        tidy_error!(bad, "binary checked into source: {}", file.display());
+                    }
                 }
-            }
-        })
+            },
+        )
     }
 }
diff --git a/src/tools/tidy/src/debug_artifacts.rs b/src/tools/tidy/src/debug_artifacts.rs
@@ -6,11 +6,21 @@ 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, |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, "{}:{}: {}", entry.path().display(), i + 1, GRAPHVIZ_POSTFLOW_MSG);
+    walk(
+        test_dir,
+        |path, _is_dir| 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,
+                        "{}:{}: {}",
+                        entry.path().display(),
+                        i + 1,
+                        GRAPHVIZ_POSTFLOW_MSG
+                    );
+                }
             }
-        }
-    });
+        },
+    );
 }
diff --git a/src/tools/tidy/src/edition.rs b/src/tools/tidy/src/edition.rs
@@ -9,7 +9,7 @@ fn is_edition_2021(mut line: &str) -> bool {
 }
 
 pub fn check(path: &Path, bad: &mut bool) {
-    walk(path, |path| filter_dirs(path), &mut |entry, contents| {
+    walk(path, |path, _is_dir| filter_dirs(path), &mut |entry, contents| {
         let file = entry.path();
         let filename = file.file_name().unwrap();
         if filename != "Cargo.toml" {
diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs
@@ -129,7 +129,7 @@ fn check_error_codes_docs(
 
     let mut no_longer_emitted_codes = Vec::new();
 
-    walk(&docs_path, |_| false, &mut |entry, contents| {
+    walk(&docs_path, |_, _| false, &mut |entry, contents| {
         let path = entry.path();
 
         // Error if the file isn't markdown.
@@ -321,7 +321,7 @@ fn check_error_codes_used(
 
     let mut found_codes = Vec::new();
 
-    walk_many(search_paths, filter_dirs, &mut |entry, contents| {
+    walk_many(search_paths, |path, _is_dir| filter_dirs(path), &mut |entry, contents| {
         let path = entry.path();
 
         // Return early if we aren't looking at a source file.
diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs
@@ -102,7 +102,7 @@ pub fn check(
             &tests_path.join("rustdoc-ui"),
             &tests_path.join("rustdoc"),
         ],
-        |path| {
+        |path, _is_dir| {
             filter_dirs(path)
                 || filter_not_rust(path)
                 || path.file_name() == Some(OsStr::new("features.rs"))
@@ -478,7 +478,7 @@ fn map_lib_features(
 ) {
     walk(
         base_src_path,
-        |path| filter_dirs(path) || path.ends_with("tests"),
+        |path, _is_dir| filter_dirs(path) || path.ends_with("tests"),
         &mut |entry, contents| {
             let file = entry.path();
             let filename = file.file_name().unwrap().to_string_lossy();
diff --git a/src/tools/tidy/src/mir_opt_tests.rs b/src/tools/tidy/src/mir_opt_tests.rs
@@ -11,7 +11,7 @@ fn check_unused_files(path: &Path, bless: bool, bad: &mut bool) {
 
     walk_no_read(
         &[&path.join("mir-opt")],
-        |path| path.file_name() == Some("README.md".as_ref()),
+        |path, _is_dir| path.file_name() == Some("README.md".as_ref()),
         &mut |file| {
             let filepath = file.path();
             if filepath.extension() == Some("rs".as_ref()) {
diff --git a/src/tools/tidy/src/pal.rs b/src/tools/tidy/src/pal.rs
@@ -67,7 +67,7 @@ pub fn check(path: &Path, bad: &mut bool) {
     // Sanity check that the complex parsing here works.
     let mut saw_target_arch = false;
     let mut saw_cfg_bang = false;
-    walk(path, filter_dirs, &mut |entry, contents| {
+    walk(path, |path, _is_dir| filter_dirs(path), &mut |entry, contents| {
         let file = entry.path();
         let filestr = file.to_string_lossy().replace("\\", "/");
         if !filestr.ends_with(".rs") {
diff --git a/src/tools/tidy/src/rustdoc_gui_tests.rs b/src/tools/tidy/src/rustdoc_gui_tests.rs
@@ -5,10 +5,7 @@ use std::path::Path;
 pub fn check(path: &Path, bad: &mut bool) {
     crate::walk::walk(
         &path.join("rustdoc-gui"),
-        |p| {
-            // If there is no extension, it's very likely a folder and we want to go into it.
-            p.extension().map(|e| e != "goml").unwrap_or(false)
-        },
+        |p, is_dir| !is_dir && p.extension().map_or(true, |e| e != "goml"),
         &mut |entry, content| {
             for line in content.lines() {
                 if !line.starts_with("// ") {
diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs
@@ -227,7 +227,7 @@ fn is_unexplained_ignore(extension: &str, line: &str) -> bool {
 }
 
 pub fn check(path: &Path, bad: &mut bool) {
-    fn skip(path: &Path) -> bool {
+    fn skip(path: &Path, is_dir: bool) -> bool {
         if path.file_name().map_or(false, |name| name.to_string_lossy().starts_with(".#")) {
             // vim or emacs temporary file
             return true;
@@ -237,8 +237,15 @@ pub fn check(path: &Path, bad: &mut bool) {
             return true;
         }
 
+        // Don't check extensions for directories
+        if is_dir {
+            return false;
+        }
+
         let extensions = ["rs", "py", "js", "sh", "c", "cpp", "h", "md", "css", "ftl", "goml"];
-        if extensions.iter().all(|e| path.extension() != Some(OsStr::new(e))) {
+
+        // NB: don't skip paths without extensions (or else we'll skip all directories and will only check top level files)
+        if path.extension().map_or(true, |ext| !extensions.iter().any(|e| ext == OsStr::new(e))) {
             return true;
         }
 
diff --git a/src/tools/tidy/src/target_specific_tests.rs b/src/tools/tidy/src/target_specific_tests.rs
@@ -37,7 +37,7 @@ struct RevisionInfo<'a> {
 }
 
 pub fn check(path: &Path, bad: &mut bool) {
-    crate::walk::walk(path, filter_not_rust, &mut |entry, content| {
+    crate::walk::walk(path, |path, _is_dir| filter_not_rust(path), &mut |entry, content| {
         let file = entry.path().display();
         let mut header_map = BTreeMap::new();
         iter_header(content, &mut |cfg, directive| {
diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs
@@ -51,7 +51,7 @@ pub fn check(path: &Path, bad: &mut bool) {
     check_entries(&path, bad);
     let (ui, ui_fulldeps) = (path.join("ui"), path.join("ui-fulldeps"));
     let paths = [ui.as_path(), ui_fulldeps.as_path()];
-    crate::walk::walk_no_read(&paths, |_| false, &mut |entry| {
+    crate::walk::walk_no_read(&paths, |_, _| false, &mut |entry| {
         let file_path = entry.path();
         if let Some(ext) = file_path.extension() {
             if ext == "stderr" || ext == "stdout" {
diff --git a/src/tools/tidy/src/unit_tests.rs b/src/tools/tidy/src/unit_tests.rs
@@ -20,9 +20,9 @@ pub fn check(root_path: &Path, bad: &mut bool) {
             && !(path.starts_with(&core_tests) || path.starts_with(&core_benches))
     };
 
-    let skip = move |path: &Path| {
+    let skip = move |path: &Path, is_dir| {
         let file_name = path.file_name().unwrap_or_default();
-        if path.is_dir() {
+        if is_dir {
             filter_dirs(path)
                 || path.ends_with("src/doc")
                 || (file_name == "tests" || file_name == "benches") && !is_core(path)
diff --git a/src/tools/tidy/src/walk.rs b/src/tools/tidy/src/walk.rs
@@ -41,15 +41,15 @@ pub fn filter_not_rust(path: &Path) -> bool {
 
 pub fn walk(
     path: &Path,
-    skip: impl Clone + Send + Sync + 'static + Fn(&Path) -> bool,
+    skip: impl Clone + Send + Sync + 'static + Fn(&Path, bool) -> bool,
     f: &mut dyn FnMut(&DirEntry, &str),
 ) {
     walk_many(&[path], skip, f);
 }
 
 pub fn walk_many(
     paths: &[&Path],
-    skip: impl Clone + Send + Sync + 'static + Fn(&Path) -> bool,
+    skip: impl Clone + Send + Sync + 'static + Fn(&Path, bool) -> bool,
     f: &mut dyn FnMut(&DirEntry, &str),
 ) {
     let mut contents = Vec::new();
@@ -67,14 +67,16 @@ pub fn walk_many(
 
 pub(crate) fn walk_no_read(
     paths: &[&Path],
-    skip: impl Send + Sync + 'static + Fn(&Path) -> bool,
+    skip: impl Send + Sync + 'static + Fn(&Path, bool) -> bool,
     f: &mut dyn FnMut(&DirEntry),
 ) {
     let mut walker = ignore::WalkBuilder::new(paths[0]);
     for path in &paths[1..] {
         walker.add(path);
     }
-    let walker = walker.filter_entry(move |e| !skip(e.path()));
+    let walker = walker.filter_entry(move |e| {
+        !skip(e.path(), e.file_type().map(|ft| ft.is_dir()).unwrap_or(false))
+    });
     for entry in walker.build() {
         if let Ok(entry) = entry {
             if entry.file_type().map_or(true, |kind| kind.is_dir() || kind.is_symlink()) {

Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ fn check_section<'a>(`
`95`	`95`	`}`
`96`	`96`
`97`	`97`	`pub fn check(path: &Path, bad: &mut bool) {`
`98`		`- walk(path, filter_dirs, &mut \|entry, contents\| {`
	`98`	`+ walk(path, \|path, _is_dir\| filter_dirs(path), &mut \|entry, contents\| {`
`99`	`99`	`let file = &entry.path().display();`
`100`	`100`
`101`	`101`	`let mut lines = contents.lines().enumerate().peekable();`
Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,7 @@ fn is_edition_2021(mut line: &str) -> bool {`
`9`	`9`	`}`
`10`	`10`
`11`	`11`	`pub fn check(path: &Path, bad: &mut bool) {`
`12`		`- walk(path, \|path\| filter_dirs(path), &mut \|entry, contents\| {`
	`12`	`+ walk(path, \|path, _is_dir\| filter_dirs(path), &mut \|entry, contents\| {`
`13`	`13`	`let file = entry.path();`
`14`	`14`	`let filename = file.file_name().unwrap();`
`15`	`15`	`if filename != "Cargo.toml" {`