Auto merge of rust-lang#146592 - Kobzol:tidy-diag, r=jieyouxu

Implement a simple diagnostic system for tidy

In rust-lang#146316 and rust-lang#146580, contributors independently wanted to reduce the verbose output of tidy. But before, the output was quite ad-hoc, so it was not easy to control it.

In this PR, I implemented a simple diagnostic system for tidy, which allows us to:
1) Only print certain information in verbose mode (`-v`)
2) Associate each (error) output to a specific check, so that it is easier to find out what exactly has failed and which check you might want to examine (not fully done, there are some random `println`s left, but most output should be scoped to a specific check)
3) Print output with colors, based on the message level (message, warning, error)
4) Show the start/end execution of each check in verbose mode, for better progress indication

Failure output:
<img width="1134" height="157" alt="image" src="https://github.com/user-attachments/assets/578a9302-e1c2-47e5-9370-a3556c49d9fc" />

Success output:
<img width="388" height="113" alt="image" src="https://github.com/user-attachments/assets/cf27faf8-3d8b-49e3-88d0-fac27a9c36a8" />

Verbose output (shortened):
<img width="380" height="158" alt="image" src="https://github.com/user-attachments/assets/ce7102b8-c2f3-42a8-a2ec-ca30389be91e" />

CC `@nnethercote` `@RalfJung` `@GuillaumeGomez`

The first two commits and the last commit are interesting, the rest is just mechanical port of the code from `bad: &mut bool` to `DiagCtx` and `RunningCheck`.

The `extra_checks` check could be further split, but I'd leave that for another PR.

r? `@jieyouxu`

Author: bors

src/tools/features-status-dump/src/main.rs

@@ -5,6 +5,7 @@ use std::path::PathBuf;
 
 use anyhow::{Context, Result};
 use clap::Parser;
+use tidy::diagnostics::RunningCheck;
 use tidy::features::{Feature, collect_lang_features, collect_lib_features};
 
 #[derive(Debug, Parser)]
@@ -29,7 +30,7 @@ struct FeaturesStatus {
 fn main() -> Result<()> {
     let Cli { compiler_path, library_path, output_path } = Cli::parse();
 
-    let lang_features_status = collect_lang_features(&compiler_path, &mut false);
+    let lang_features_status = collect_lang_features(&compiler_path, &mut RunningCheck::new_noop());
     let lib_features_status = collect_lib_features(&library_path)
         .into_iter()
         .filter(|&(ref name, _)| !lang_features_status.contains_key(name))

src/tools/tidy/src/alphabetical.rs

@@ -24,6 +24,7 @@ use std::fmt::Display;
 use std::iter::Peekable;
 use std::path::Path;
 
+use crate::diagnostics::{CheckId, DiagCtx, RunningCheck};
 use crate::walk::{filter_dirs, walk};
 
 #[cfg(test)]
@@ -43,8 +44,7 @@ const END_MARKER: &str = "tidy-alphabetical-end";
 fn check_section<'a>(
     file: impl Display,
     lines: impl Iterator<Item = (usize, &'a str)>,
-    err: &mut dyn FnMut(&str) -> std::io::Result<()>,
-    bad: &mut bool,
+    check: &mut RunningCheck,
 ) {
     let mut prev_line = String::new();
     let mut first_indent = None;
@@ -56,12 +56,10 @@ fn check_section<'a>(
         }
 
         if line.contains(START_MARKER) {
-            tidy_error_ext!(
-                err,
-                bad,
+            check.error(format!(
                 "{file}:{} found `{START_MARKER}` expecting `{END_MARKER}`",
                 idx + 1
-            );
+            ));
             return;
         }
 
@@ -104,45 +102,44 @@ fn check_section<'a>(
         let prev_line_trimmed_lowercase = prev_line.trim_start_matches(' ');
 
         if version_sort(trimmed_line, prev_line_trimmed_lowercase).is_lt() {
-            tidy_error_ext!(err, bad, "{file}:{}: line not in alphabetical order", idx + 1);
+            check.error(format!("{file}:{}: line not in alphabetical order", idx + 1));
         }
 
         prev_line = line;
     }
 
-    tidy_error_ext!(err, bad, "{file}: reached end of file expecting `{END_MARKER}`")
+    check.error(format!("{file}: reached end of file expecting `{END_MARKER}`"));
 }
 
 fn check_lines<'a>(
     file: &impl Display,
     mut lines: impl Iterator<Item = (usize, &'a str)>,
-    err: &mut dyn FnMut(&str) -> std::io::Result<()>,
-    bad: &mut bool,
+    check: &mut RunningCheck,
 ) {
     while let Some((idx, line)) = lines.next() {
         if line.contains(END_MARKER) {
-            tidy_error_ext!(
-                err,
-                bad,
+            check.error(format!(
                 "{file}:{} found `{END_MARKER}` expecting `{START_MARKER}`",
                 idx + 1
-            )
+            ));
         }
 
         if line.contains(START_MARKER) {
-            check_section(file, &mut lines, err, bad);
+            check_section(file, &mut lines, check);
         }
     }
 }
 
-pub fn check(path: &Path, bad: &mut bool) {
+pub fn check(path: &Path, diag_ctx: DiagCtx) {
+    let mut check = diag_ctx.start_check(CheckId::new("alphabetical").path(path));
+
     let skip =
         |path: &_, _is_dir| filter_dirs(path) || path.ends_with("tidy/src/alphabetical/tests.rs");
 
     walk(path, skip, &mut |entry, contents| {
         let file = &entry.path().display();
         let lines = contents.lines().enumerate();
-        check_lines(file, lines, &mut crate::tidy_error, bad)
+        check_lines(file, lines, &mut check)
     });
 }
 

src/tools/tidy/src/alphabetical/tests.rs

@@ -1,19 +1,22 @@
-use std::io::Write;
-use std::str::from_utf8;
+use std::path::Path;
 
-use super::*;
+use crate::alphabetical::check_lines;
+use crate::diagnostics::DiagCtx;
 
 #[track_caller]
 fn test(lines: &str, name: &str, expected_msg: &str, expected_bad: bool) {
-    let mut actual_msg = Vec::new();
-    let mut actual_bad = false;
-    let mut err = |args: &_| {
-        write!(&mut actual_msg, "{args}")?;
-        Ok(())
-    };
-    check_lines(&name, lines.lines().enumerate(), &mut err, &mut actual_bad);
-    assert_eq!(expected_msg, from_utf8(&actual_msg).unwrap());
-    assert_eq!(expected_bad, actual_bad);
+    let diag_ctx = DiagCtx::new(Path::new("/"), false);
+    let mut check = diag_ctx.start_check("alphabetical-test");
+    check_lines(&name, lines.lines().enumerate(), &mut check);
+
+    assert_eq!(expected_bad, check.is_bad());
+    let errors = check.get_errors();
+    if expected_bad {
+        assert_eq!(errors.len(), 1);
+        assert_eq!(expected_msg, errors[0]);
+    } else {
+        assert!(errors.is_empty());
+    }
 }
 
 #[track_caller]

src/tools/tidy/src/bins.rs

@@ -12,11 +12,13 @@ pub use os_impl::*;
 mod os_impl {
     use std::path::Path;
 
+    use crate::diagnostics::DiagCtx;
+
     pub fn check_filesystem_support(_sources: &[&Path], _output: &Path) -> bool {
         return false;
     }
 
-    pub fn check(_path: &Path, _bad: &mut bool) {}
+    pub fn check(_path: &Path, _diag_ctx: DiagCtx) {}
 }
 
 #[cfg(unix)]
@@ -36,6 +38,8 @@ mod os_impl {
 
     use FilesystemSupport::*;
 
+    use crate::diagnostics::DiagCtx;
+
     fn is_executable(path: &Path) -> std::io::Result<bool> {
         Ok(path.metadata()?.mode() & 0o111 != 0)
     }
@@ -106,14 +110,16 @@ mod os_impl {
     }
 
     #[cfg(unix)]
-    pub fn check(path: &Path, bad: &mut bool) {
+    pub fn check(path: &Path, diag_ctx: DiagCtx) {
+        let mut check = diag_ctx.start_check("bins");
+
         use std::ffi::OsStr;
 
         const ALLOWED: &[&str] = &["configure", "x"];
 
         for p in RI_EXCLUSION_LIST {
             if !path.join(Path::new(p)).exists() {
-                tidy_error!(bad, "rust-installer test bins missed: {p}");
+                check.error(format!("rust-installer test bins missed: {p}"));
             }
         }
 
@@ -153,7 +159,7 @@ mod os_impl {
                         });
                     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());
+                        check.error(format!("binary checked into source: {}", file.display()));
                     }
                 }
             },

src/tools/tidy/src/debug_artifacts.rs

@@ -2,24 +2,25 @@
 
 use std::path::Path;
 
+use crate::diagnostics::{CheckId, DiagCtx};
 use crate::walk::{filter_dirs, filter_not_rust, walk};
 
 const GRAPHVIZ_POSTFLOW_MSG: &str = "`borrowck_graphviz_postflow` attribute in test";
 
-pub fn check(test_dir: &Path, bad: &mut bool) {
+pub fn check(test_dir: &Path, diag_ctx: DiagCtx) {
+    let mut check = diag_ctx.start_check(CheckId::new("debug_artifacts").path(test_dir));
+
     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,
-                        "{}:{}: {}",
+                    check.error(format!(
+                        "{}:{}: {GRAPHVIZ_POSTFLOW_MSG}",
                         entry.path().display(),
-                        i + 1,
-                        GRAPHVIZ_POSTFLOW_MSG
-                    );
+                        i + 1
+                    ));
                 }
             }
         },