Merge #3624

3624: Tidier tidy checks r=matklad a=matklad



bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

xtask/tests/tidy-tests/cli.rs

@@ -1,7 +1,6 @@
-use walkdir::WalkDir;
 use xtask::{
     codegen::{self, Mode},
-    project_root, run_rustfmt,
+    run_rustfmt,
 };
 
 #[test]
@@ -31,25 +30,3 @@ fn check_code_formatting() {
         panic!("{}. Please format the code by running `cargo format`", error);
     }
 }
-
-#[test]
-fn no_todo() {
-    WalkDir::new(project_root().join("crates")).into_iter().for_each(|e| {
-        let e = e.unwrap();
-        if e.path().extension().map(|it| it != "rs").unwrap_or(true) {
-            return;
-        }
-        if e.path().ends_with("tests/cli.rs") {
-            return;
-        }
-        let text = std::fs::read_to_string(e.path()).unwrap();
-        if text.contains("TODO") || text.contains("TOOD") || text.contains("todo!") {
-            panic!(
-                "\nTODO markers should not be committed to the master branch,\n\
-                 use FIXME instead\n\
-                 {}\n",
-                e.path().display(),
-            )
-        }
-    })
-}

xtask/tests/tidy-tests/docs.rs

@@ -1,106 +0,0 @@
-use std::{collections::HashMap, fs, io::prelude::*, io::BufReader, path::Path};
-
-use anyhow::Context;
-use walkdir::{DirEntry, WalkDir};
-use xtask::project_root;
-
-fn is_exclude_dir(p: &Path) -> bool {
-    // Test hopefully don't really need comments, and for assists we already
-    // have special comments which are source of doc tests and user docs.
-    let exclude_dirs = ["tests", "test_data", "handlers"];
-    let mut cur_path = p;
-    while let Some(path) = cur_path.parent() {
-        if exclude_dirs.iter().any(|dir| path.ends_with(dir)) {
-            return true;
-        }
-        cur_path = path;
-    }
-
-    false
-}
-
-fn is_exclude_file(d: &DirEntry) -> bool {
-    let file_names = ["tests.rs"];
-
-    d.file_name().to_str().map(|f_n| file_names.iter().any(|name| *name == f_n)).unwrap_or(false)
-}
-
-fn is_hidden(entry: &DirEntry) -> bool {
-    entry.file_name().to_str().map(|s| s.starts_with('.')).unwrap_or(false)
-}
-
-#[test]
-fn no_docs_comments() {
-    let crates = project_root().join("crates");
-    let iter = WalkDir::new(crates);
-    let mut missing_docs = Vec::new();
-    let mut contains_fixme = Vec::new();
-    for f in iter.into_iter().filter_entry(|e| !is_hidden(e)) {
-        let f = f.unwrap();
-        if f.file_type().is_dir() {
-            continue;
-        }
-        if f.path().extension().map(|it| it != "rs").unwrap_or(false) {
-            continue;
-        }
-        if is_exclude_dir(f.path()) {
-            continue;
-        }
-        if is_exclude_file(&f) {
-            continue;
-        }
-        let mut reader = BufReader::new(fs::File::open(f.path()).unwrap());
-        let mut line = String::new();
-        reader
-            .read_line(&mut line)
-            .with_context(|| format!("Failed to read {}", f.path().display()))
-            .unwrap();
-
-        if line.starts_with("//!") {
-            if line.contains("FIXME") {
-                contains_fixme.push(f.path().to_path_buf())
-            }
-        } else {
-            missing_docs.push(f.path().display().to_string());
-        }
-    }
-    if !missing_docs.is_empty() {
-        panic!(
-            "\nMissing docs strings\n\n\
-             modules:\n{}\n\n",
-            missing_docs.join("\n")
-        )
-    }
-
-    let whitelist = [
-        "ra_db",
-        "ra_hir",
-        "ra_hir_expand",
-        "ra_ide",
-        "ra_mbe",
-        "ra_parser",
-        "ra_prof",
-        "ra_project_model",
-        "ra_syntax",
-        "ra_text_edit",
-        "ra_tt",
-        "ra_hir_ty",
-    ];
-
-    let mut has_fixmes = whitelist.iter().map(|it| (*it, false)).collect::<HashMap<&str, bool>>();
-    'outer: for path in contains_fixme {
-        for krate in whitelist.iter() {
-            if path.components().any(|it| it.as_os_str() == *krate) {
-                has_fixmes.insert(krate, true);
-                continue 'outer;
-            }
-        }
-        panic!("FIXME doc in a fully-documented crate: {}", path.display())
-    }
-
-    for (krate, has_fixme) in has_fixmes.iter() {
-        if !has_fixme {
-            panic!("crate {} is fully documented, remove it from the white list", krate)
-        }
-    }
-}

xtask/tests/tidy-tests/main.rs

@@ -1,2 +1,145 @@
 mod cli;
-mod docs;
+
+use std::{
+    collections::HashMap,
+    path::{Path, PathBuf},
+};
+
+use walkdir::{DirEntry, WalkDir};
+use xtask::{not_bash::fs2, project_root};
+
+#[test]
+fn rust_files_are_tidy() {
+    let mut tidy_docs = TidyDocs::default();
+    for path in rust_files() {
+        let text = fs2::read_to_string(&path).unwrap();
+        check_todo(&path, &text);
+        tidy_docs.visit(&path, &text);
+    }
+    tidy_docs.finish();
+}
+
+fn check_todo(path: &Path, text: &str) {
+    if path.ends_with("tests/cli.rs") {
+        return;
+    }
+    if text.contains("TODO") || text.contains("TOOD") || text.contains("todo!") {
+        panic!(
+            "\nTODO markers should not be committed to the master branch,\n\
+             use FIXME instead\n\
+             {}\n",
+            path.display(),
+        )
+    }
+}
+
+#[derive(Default)]
+struct TidyDocs {
+    missing_docs: Vec<String>,
+    contains_fixme: Vec<PathBuf>,
+}
+
+impl TidyDocs {
+    fn visit(&mut self, path: &Path, text: &str) {
+        if is_exclude_dir(path) || is_exclude_file(path) {
+            return;
+        }
+
+        let first_line = match text.lines().next() {
+            Some(it) => it,
+            None => return,
+        };
+
+        if first_line.starts_with("//!") {
+            if first_line.contains("FIXME") {
+                self.contains_fixme.push(path.to_path_buf())
+            }
+        } else {
+            self.missing_docs.push(path.display().to_string());
+        }
+
+        fn is_exclude_dir(p: &Path) -> bool {
+            // Test hopefully don't really need comments, and for assists we already
+            // have special comments which are source of doc tests and user docs.
+            let exclude_dirs = ["tests", "test_data", "handlers"];
+            let mut cur_path = p;
+            while let Some(path) = cur_path.parent() {
+                if exclude_dirs.iter().any(|dir| path.ends_with(dir)) {
+                    return true;
+                }
+                cur_path = path;
+            }
+
+            false
+        }
+
+        fn is_exclude_file(d: &Path) -> bool {
+            let file_names = ["tests.rs"];
+
+            d.file_name()
+                .unwrap_or_default()
+                .to_str()
+                .map(|f_n| file_names.iter().any(|name| *name == f_n))
+                .unwrap_or(false)
+        }
+    }
+
+    fn finish(self) {
+        if !self.missing_docs.is_empty() {
+            panic!(
+                "\nMissing docs strings\n\n\
+                 modules:\n{}\n\n",
+                self.missing_docs.join("\n")
+            )
+        }
+
+        let whitelist = [
+            "ra_db",
+            "ra_hir",
+            "ra_hir_expand",
+            "ra_ide",
+            "ra_mbe",
+            "ra_parser",
+            "ra_prof",
+            "ra_project_model",
+            "ra_syntax",
+            "ra_text_edit",
+            "ra_tt",
+            "ra_hir_ty",
+        ];
+
+        let mut has_fixmes =
+            whitelist.iter().map(|it| (*it, false)).collect::<HashMap<&str, bool>>();
+        'outer: for path in self.contains_fixme {
+            for krate in whitelist.iter() {
+                if path.components().any(|it| it.as_os_str() == *krate) {
+                    has_fixmes.insert(krate, true);
+                    continue 'outer;
+                }
+            }
+            panic!("FIXME doc in a fully-documented crate: {}", path.display())
+        }
+
+        for (krate, has_fixme) in has_fixmes.iter() {
+            if !has_fixme {
+                panic!("crate {} is fully documented, remove it from the white list", krate)
+            }
+        }
+    }
+}
+
+fn rust_files() -> impl Iterator<Item = PathBuf> {
+    let crates = project_root().join("crates");
+    let iter = WalkDir::new(crates);
+    return iter
+        .into_iter()
+        .filter_entry(|e| !is_hidden(e))
+        .map(|e| e.unwrap())
+        .filter(|e| !e.file_type().is_dir())
+        .map(|e| e.into_path())
+        .filter(|path| path.extension().map(|it| it == "rs").unwrap_or(false));
+
+    fn is_hidden(entry: &DirEntry) -> bool {
+        entry.file_name().to_str().map(|s| s.starts_with('.')).unwrap_or(false)
+    }
+}