diff options
| author | bors <bors@rust-lang.org> | 2025-09-21 13:21:30 +0000 |
|---|---|---|
| committer | bors <bors@rust-lang.org> | 2025-09-21 13:21:30 +0000 |
| commit | 6710835ae739ca326441ff6c63d24fb123858300 (patch) | |
| tree | bdc8e9e29753eb1f7f555839350b53acb4e1d2c7 | |
| parent | 1d23da6b7304d9e2a2c3dcb1b0aaa709cb9bc4ad (diff) | |
| parent | 4c208f5c64c3a86e52f5ebff2792ed1e3d8ca5b8 (diff) | |
| download | rust-6710835ae739ca326441ff6c63d24fb123858300.tar.gz rust-6710835ae739ca326441ff6c63d24fb123858300.zip | |
Auto merge of #146592 - Kobzol:tidy-diag, r=jieyouxu
Implement a simple diagnostic system for tidy In https://github.com/rust-lang/rust/pull/146316 and https://github.com/rust-lang/rust/pull/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`
39 files changed, 781 insertions, 580 deletions
diff --git a/src/tools/features-status-dump/src/main.rs b/src/tools/features-status-dump/src/main.rs index 1ce98d1506d..a4f88362ab8 100644 --- a/src/tools/features-status-dump/src/main.rs +++ b/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)) diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs index 9ddce725106..f93d3b56113 100644 --- a/src/tools/tidy/src/alphabetical.rs +++ b/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) }); } diff --git a/src/tools/tidy/src/alphabetical/tests.rs b/src/tools/tidy/src/alphabetical/tests.rs index 4d05bc33ced..b181ab8f744 100644 --- a/src/tools/tidy/src/alphabetical/tests.rs +++ b/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] diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs index a18f549844b..10b61869971 100644 --- a/src/tools/tidy/src/bins.rs +++ b/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())); } } }, diff --git a/src/tools/tidy/src/debug_artifacts.rs b/src/tools/tidy/src/debug_artifacts.rs index 645534cc827..19effaede81 100644 --- a/src/tools/tidy/src/debug_artifacts.rs +++ b/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 + )); } } }, diff --git a/src/tools/tidy/src/deps.rs b/src/tools/tidy/src/deps.rs index 568ec0c1198..e275d3042cb 100644 --- a/src/tools/tidy/src/deps.rs +++ b/src/tools/tidy/src/deps.rs @@ -9,6 +9,8 @@ use build_helper::ci::CiEnv; use cargo_metadata::semver::Version; use cargo_metadata::{Metadata, Package, PackageId}; +use crate::diagnostics::{DiagCtx, RunningCheck}; + #[path = "../../../bootstrap/src/utils/proc_macro_deps.rs"] mod proc_macro_deps; @@ -661,10 +663,12 @@ const PERMITTED_CRANELIFT_DEPENDENCIES: &[&str] = &[ /// /// `root` is path to the directory with the root `Cargo.toml` (for the workspace). `cargo` is path /// to the cargo executable. -pub fn check(root: &Path, cargo: &Path, bless: bool, bad: &mut bool) { +pub fn check(root: &Path, cargo: &Path, bless: bool, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check("deps"); + let mut checked_runtime_licenses = false; - check_proc_macro_dep_list(root, cargo, bless, bad); + check_proc_macro_dep_list(root, cargo, bless, &mut check); for &WorkspaceInfo { path, exceptions, crates_and_deps, submodules } in WORKSPACES { if has_missing_submodule(root, submodules) { @@ -672,7 +676,7 @@ pub fn check(root: &Path, cargo: &Path, bless: bool, bad: &mut bool) { } if !root.join(path).join("Cargo.lock").exists() { - tidy_error!(bad, "the `{path}` workspace doesn't have a Cargo.lock"); + check.error(format!("the `{path}` workspace doesn't have a Cargo.lock")); continue; } @@ -683,16 +687,23 @@ pub fn check(root: &Path, cargo: &Path, bless: bool, bad: &mut bool) { .other_options(vec!["--locked".to_owned()]); let metadata = t!(cmd.exec()); - check_license_exceptions(&metadata, path, exceptions, bad); + check_license_exceptions(&metadata, path, exceptions, &mut check); if let Some((crates, permitted_deps, location)) = crates_and_deps { let descr = crates.get(0).unwrap_or(&path); - check_permitted_dependencies(&metadata, descr, permitted_deps, crates, location, bad); + check_permitted_dependencies( + &metadata, + descr, + permitted_deps, + crates, + location, + &mut check, + ); } if path == "library" { - check_runtime_license_exceptions(&metadata, bad); - check_runtime_no_duplicate_dependencies(&metadata, bad); - check_runtime_no_proc_macros(&metadata, bad); + check_runtime_license_exceptions(&metadata, &mut check); + check_runtime_no_duplicate_dependencies(&metadata, &mut check); + check_runtime_no_proc_macros(&metadata, &mut check); checked_runtime_licenses = true; } } @@ -703,7 +714,7 @@ pub fn check(root: &Path, cargo: &Path, bless: bool, bad: &mut bool) { } /// Ensure the list of proc-macro crate transitive dependencies is up to date -fn check_proc_macro_dep_list(root: &Path, cargo: &Path, bless: bool, bad: &mut bool) { +fn check_proc_macro_dep_list(root: &Path, cargo: &Path, bless: bool, check: &mut RunningCheck) { let mut cmd = cargo_metadata::MetadataCommand::new(); cmd.cargo_path(cargo) .manifest_path(root.join("Cargo.toml")) @@ -750,22 +761,22 @@ pub static CRATES: &[&str] = &[ ) .unwrap(); } else { - let old_bad = *bad; + let mut error_found = false; for missing in proc_macro_deps.difference(&expected) { - tidy_error!( - bad, + error_found = true; + check.error(format!( "proc-macro crate dependency `{missing}` is not registered in `src/bootstrap/src/utils/proc_macro_deps.rs`", - ); + )); } for extra in expected.difference(&proc_macro_deps) { - tidy_error!( - bad, + error_found = true; + check.error(format!( "`{extra}` is registered in `src/bootstrap/src/utils/proc_macro_deps.rs`, but is not a proc-macro crate dependency", - ); + )); } - if *bad != old_bad { - eprintln!("Run `./x.py test tidy --bless` to regenerate the list"); + if error_found { + check.message("Run `./x.py test tidy --bless` to regenerate the list"); } } } @@ -787,7 +798,7 @@ pub fn has_missing_submodule(root: &Path, submodules: &[&str]) -> bool { /// /// Unlike for tools we don't allow exceptions to the `LICENSES` list for the runtime with the sole /// exception of `fortanix-sgx-abi` which is only used on x86_64-fortanix-unknown-sgx. -fn check_runtime_license_exceptions(metadata: &Metadata, bad: &mut bool) { +fn check_runtime_license_exceptions(metadata: &Metadata, check: &mut RunningCheck) { for pkg in &metadata.packages { if pkg.source.is_none() { // No need to check local packages. @@ -796,7 +807,8 @@ fn check_runtime_license_exceptions(metadata: &Metadata, bad: &mut bool) { let license = match &pkg.license { Some(license) => license, None => { - tidy_error!(bad, "dependency `{}` does not define a license expression", pkg.id); + check + .error(format!("dependency `{}` does not define a license expression", pkg.id)); continue; } }; @@ -809,7 +821,7 @@ fn check_runtime_license_exceptions(metadata: &Metadata, bad: &mut bool) { continue; } - tidy_error!(bad, "invalid license `{}` in `{}`", license, pkg.id); + check.error(format!("invalid license `{}` in `{}`", license, pkg.id)); } } } @@ -821,37 +833,32 @@ fn check_license_exceptions( metadata: &Metadata, workspace: &str, exceptions: &[(&str, &str)], - bad: &mut bool, + check: &mut RunningCheck, ) { // Validate the EXCEPTIONS list hasn't changed. for (name, license) in exceptions { // Check that the package actually exists. if !metadata.packages.iter().any(|p| *p.name == *name) { - tidy_error!( - bad, - "could not find exception package `{}` in workspace `{workspace}`\n\ + check.error(format!( + "could not find exception package `{name}` in workspace `{workspace}`\n\ Remove from EXCEPTIONS list if it is no longer used.", - name - ); + )); } // Check that the license hasn't changed. for pkg in metadata.packages.iter().filter(|p| *p.name == *name) { match &pkg.license { None => { - tidy_error!( - bad, + check.error(format!( "dependency exception `{}` in workspace `{workspace}` does not declare a license expression", pkg.id - ); + )); } Some(pkg_license) => { if pkg_license.as_str() != *license { - println!( - "dependency exception `{name}` license in workspace `{workspace}` has changed" - ); - println!(" previously `{license}` now `{pkg_license}`"); - println!(" update EXCEPTIONS for the new license"); - *bad = true; + check.error(format!(r#"dependency exception `{name}` license in workspace `{workspace}` has changed + previously `{license}` now `{pkg_license}` + update EXCEPTIONS for the new license +"#)); } } } @@ -872,26 +879,23 @@ fn check_license_exceptions( let license = match &pkg.license { Some(license) => license, None => { - tidy_error!( - bad, + check.error(format!( "dependency `{}` in workspace `{workspace}` does not define a license expression", pkg.id - ); + )); continue; } }; if !LICENSES.contains(&license.as_str()) { - tidy_error!( - bad, + check.error(format!( "invalid license `{}` for package `{}` in workspace `{workspace}`", - license, - pkg.id - ); + license, pkg.id + )); } } } -fn check_runtime_no_duplicate_dependencies(metadata: &Metadata, bad: &mut bool) { +fn check_runtime_no_duplicate_dependencies(metadata: &Metadata, check: &mut RunningCheck) { let mut seen_pkgs = HashSet::new(); for pkg in &metadata.packages { if pkg.source.is_none() { @@ -902,25 +906,23 @@ fn check_runtime_no_duplicate_dependencies(metadata: &Metadata, bad: &mut bool) // depends on two version of (one for the `wasm32-wasip1` target and // another for the `wasm32-wasip2` target). if pkg.name.to_string() != "wasi" && !seen_pkgs.insert(&*pkg.name) { - tidy_error!( - bad, + check.error(format!( "duplicate package `{}` is not allowed for the standard library", pkg.name - ); + )); } } } -fn check_runtime_no_proc_macros(metadata: &Metadata, bad: &mut bool) { +fn check_runtime_no_proc_macros(metadata: &Metadata, check: &mut RunningCheck) { for pkg in &metadata.packages { if pkg.targets.iter().any(|target| target.is_proc_macro()) { - tidy_error!( - bad, + check.error(format!( "proc macro `{}` is not allowed as standard library dependency.\n\ Using proc macros in the standard library would break cross-compilation \ as proc-macros don't get shipped for the host tuple.", pkg.name - ); + )); } } } @@ -935,7 +937,7 @@ fn check_permitted_dependencies( permitted_dependencies: &[&'static str], restricted_dependency_crates: &[&'static str], permitted_location: ListLocation, - bad: &mut bool, + check: &mut RunningCheck, ) { let mut has_permitted_dep_error = false; let mut deps = HashSet::new(); @@ -957,11 +959,10 @@ fn check_permitted_dependencies( } } if !deps.iter().any(|dep_id| compare(pkg_from_id(metadata, dep_id), permitted)) { - tidy_error!( - bad, + check.error(format!( "could not find allowed package `{permitted}`\n\ Remove from PERMITTED_DEPENDENCIES list if it is no longer used.", - ); + )); has_permitted_dep_error = true; } } @@ -988,7 +989,7 @@ fn check_permitted_dependencies( false }; if !is_eq { - tidy_error!(bad, "Dependency for {descr} not explicitly permitted: {}", dep.id); + check.error(format!("Dependency for {descr} not explicitly permitted: {}", dep.id)); has_permitted_dep_error = true; } } diff --git a/src/tools/tidy/src/diagnostics.rs b/src/tools/tidy/src/diagnostics.rs new file mode 100644 index 00000000000..6e95f97d010 --- /dev/null +++ b/src/tools/tidy/src/diagnostics.rs @@ -0,0 +1,243 @@ +use std::collections::HashSet; +use std::fmt::{Display, Formatter}; +use std::path::{Path, PathBuf}; +use std::sync::{Arc, Mutex}; + +use termcolor::{Color, WriteColor}; + +/// Collects diagnostics from all tidy steps, and contains shared information +/// that determines how should message and logs be presented. +/// +/// Since checks are executed in parallel, the context is internally synchronized, to avoid +/// all checks to lock it explicitly. +#[derive(Clone)] +pub struct DiagCtx(Arc<Mutex<DiagCtxInner>>); + +impl DiagCtx { + pub fn new(root_path: &Path, verbose: bool) -> Self { + Self(Arc::new(Mutex::new(DiagCtxInner { + running_checks: Default::default(), + finished_checks: Default::default(), + root_path: root_path.to_path_buf(), + verbose, + }))) + } + + pub fn start_check<Id: Into<CheckId>>(&self, id: Id) -> RunningCheck { + let mut id = id.into(); + + let mut ctx = self.0.lock().unwrap(); + + // Shorten path for shorter diagnostics + id.path = match id.path { + Some(path) => Some(path.strip_prefix(&ctx.root_path).unwrap_or(&path).to_path_buf()), + None => None, + }; + + ctx.start_check(id.clone()); + RunningCheck { + id, + bad: false, + ctx: self.0.clone(), + #[cfg(test)] + errors: vec![], + } + } + + pub fn into_failed_checks(self) -> Vec<FinishedCheck> { + let ctx = Arc::into_inner(self.0).unwrap().into_inner().unwrap(); + assert!(ctx.running_checks.is_empty(), "Some checks are still running"); + ctx.finished_checks.into_iter().filter(|c| c.bad).collect() + } +} + +struct DiagCtxInner { + running_checks: HashSet<CheckId>, + finished_checks: HashSet<FinishedCheck>, + verbose: bool, + root_path: PathBuf, +} + +impl DiagCtxInner { + fn start_check(&mut self, id: CheckId) { + if self.has_check_id(&id) { + panic!("Starting a check named `{id:?}` for the second time"); + } + + self.running_checks.insert(id); + } + + fn finish_check(&mut self, check: FinishedCheck) { + assert!( + self.running_checks.remove(&check.id), + "Finishing check `{:?}` that was not started", + check.id + ); + + if check.bad { + output_message("FAIL", Some(&check.id), Some(COLOR_ERROR)); + } else if self.verbose { + output_message("OK", Some(&check.id), Some(COLOR_SUCCESS)); + } + + self.finished_checks.insert(check); + } + + fn has_check_id(&self, id: &CheckId) -> bool { + self.running_checks + .iter() + .chain(self.finished_checks.iter().map(|c| &c.id)) + .any(|c| c == id) + } +} + +/// Identifies a single step +#[derive(PartialEq, Eq, Hash, Clone, Debug)] +pub struct CheckId { + pub name: String, + pub path: Option<PathBuf>, +} + +impl CheckId { + pub fn new(name: &'static str) -> Self { + Self { name: name.to_string(), path: None } + } + + pub fn path(self, path: &Path) -> Self { + Self { path: Some(path.to_path_buf()), ..self } + } +} + +impl From<&'static str> for CheckId { + fn from(name: &'static str) -> Self { + Self::new(name) + } +} + +impl Display for CheckId { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.name)?; + if let Some(path) = &self.path { + write!(f, " ({})", path.display())?; + } + Ok(()) + } +} + +#[derive(PartialEq, Eq, Hash, Debug)] +pub struct FinishedCheck { + id: CheckId, + bad: bool, +} + +impl FinishedCheck { + pub fn id(&self) -> &CheckId { + &self.id + } +} + +/// Represents a single tidy check, identified by its `name`, running. +pub struct RunningCheck { + id: CheckId, + bad: bool, + ctx: Arc<Mutex<DiagCtxInner>>, + #[cfg(test)] + errors: Vec<String>, +} + +impl RunningCheck { + /// Creates a new instance of a running check without going through the diag + /// context. + /// Useful if you want to run some functions from tidy without configuring + /// diagnostics. + pub fn new_noop() -> Self { + let ctx = DiagCtx::new(Path::new(""), false); + ctx.start_check("noop") + } + + /// Immediately output an error and mark the check as failed. + pub fn error<T: Display>(&mut self, msg: T) { + self.mark_as_bad(); + let msg = msg.to_string(); + output_message(&msg, Some(&self.id), Some(COLOR_ERROR)); + #[cfg(test)] + self.errors.push(msg); + } + + /// Immediately output a warning. + pub fn warning<T: Display>(&mut self, msg: T) { + output_message(&msg.to_string(), Some(&self.id), Some(COLOR_WARNING)); + } + + /// Output an informational message + pub fn message<T: Display>(&mut self, msg: T) { + output_message(&msg.to_string(), Some(&self.id), None); + } + + /// Output a message only if verbose output is enabled. + pub fn verbose_msg<T: Display>(&mut self, msg: T) { + if self.is_verbose_enabled() { + self.message(msg); + } + } + + /// Has an error already occured for this check? + pub fn is_bad(&self) -> bool { + self.bad + } + + /// Is verbose output enabled? + pub fn is_verbose_enabled(&self) -> bool { + self.ctx.lock().unwrap().verbose + } + + #[cfg(test)] + pub fn get_errors(&self) -> Vec<String> { + self.errors.clone() + } + + fn mark_as_bad(&mut self) { + self.bad = true; + } +} + +impl Drop for RunningCheck { + fn drop(&mut self) { + self.ctx.lock().unwrap().finish_check(FinishedCheck { id: self.id.clone(), bad: self.bad }) + } +} + +pub const COLOR_SUCCESS: Color = Color::Green; +pub const COLOR_ERROR: Color = Color::Red; +pub const COLOR_WARNING: Color = Color::Yellow; + +/// Output a message to stderr. +/// The message can be optionally scoped to a certain check, and it can also have a certain color. +pub fn output_message(msg: &str, id: Option<&CheckId>, color: Option<Color>) { + use std::io::Write; + + use termcolor::{ColorChoice, ColorSpec, StandardStream}; + + let mut stderr = StandardStream::stderr(ColorChoice::Auto); + if let Some(color) = &color { + stderr.set_color(ColorSpec::new().set_fg(Some(*color))).unwrap(); + } + + match id { + Some(id) => { + write!(&mut stderr, "tidy [{}", id.name).unwrap(); + if let Some(path) = &id.path { + write!(&mut stderr, " ({})", path.display()).unwrap(); + } + write!(&mut stderr, "]").unwrap(); + } + None => { + write!(&mut stderr, "tidy").unwrap(); + } + } + if color.is_some() { + stderr.set_color(&ColorSpec::new()).unwrap(); + } + + writeln!(&mut stderr, ": {msg}").unwrap(); +} diff --git a/src/tools/tidy/src/edition.rs b/src/tools/tidy/src/edition.rs index 08f6a3909f8..448e0b0e0a8 100644 --- a/src/tools/tidy/src/edition.rs +++ b/src/tools/tidy/src/edition.rs @@ -2,9 +2,11 @@ use std::path::Path; +use crate::diagnostics::{CheckId, DiagCtx}; use crate::walk::{filter_dirs, walk}; -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("edition").path(path)); walk(path, |path, _is_dir| filter_dirs(path), &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap(); @@ -23,11 +25,10 @@ pub fn check(path: &Path, bad: &mut bool) { // Check that all packages use the 2021 edition. Virtual workspaces don't allow setting an // edition, so these shouldn't be checked. if is_package && !is_current_edition { - tidy_error!( - bad, + check.error(format!( "{} doesn't have `edition = \"2021\"` or `edition = \"2024\"` on a separate line", file.display() - ); + )); } }); } diff --git a/src/tools/tidy/src/error_codes.rs b/src/tools/tidy/src/error_codes.rs index 65aa89fe801..83fbefa43d9 100644 --- a/src/tools/tidy/src/error_codes.rs +++ b/src/tools/tidy/src/error_codes.rs @@ -22,6 +22,7 @@ use std::path::Path; use regex::Regex; +use crate::diagnostics::{DiagCtx, RunningCheck}; use crate::walk::{filter_dirs, walk, walk_many}; const ERROR_CODES_PATH: &str = "compiler/rustc_error_codes/src/lib.rs"; @@ -35,71 +36,50 @@ const IGNORE_DOCTEST_CHECK: &[&str] = &["E0464", "E0570", "E0601", "E0602", "E07 const IGNORE_UI_TEST_CHECK: &[&str] = &["E0461", "E0465", "E0514", "E0554", "E0640", "E0717", "E0729"]; -macro_rules! verbose_print { - ($verbose:expr, $($fmt:tt)*) => { - if $verbose { - println!("{}", format_args!($($fmt)*)); - } - }; -} - -pub fn check( - root_path: &Path, - search_paths: &[&Path], - verbose: bool, - ci_info: &crate::CiInfo, - bad: &mut bool, -) { - let mut errors = Vec::new(); +pub fn check(root_path: &Path, search_paths: &[&Path], ci_info: &crate::CiInfo, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check("error_codes"); // Check that no error code explanation was removed. - check_removed_error_code_explanation(ci_info, bad); + check_removed_error_code_explanation(ci_info, &mut check); // Stage 1: create list - let error_codes = extract_error_codes(root_path, &mut errors); - if verbose { - println!("Found {} error codes", error_codes.len()); - println!("Highest error code: `{}`", error_codes.iter().max().unwrap()); - } + let error_codes = extract_error_codes(root_path, &mut check); + check.verbose_msg(format!("Found {} error codes", error_codes.len())); + check.verbose_msg(format!("Highest error code: `{}`", error_codes.iter().max().unwrap())); // Stage 2: check list has docs - let no_longer_emitted = check_error_codes_docs(root_path, &error_codes, &mut errors, verbose); + let no_longer_emitted = check_error_codes_docs(root_path, &error_codes, &mut check); // Stage 3: check list has UI tests - check_error_codes_tests(root_path, &error_codes, &mut errors, verbose, &no_longer_emitted); + check_error_codes_tests(root_path, &error_codes, &mut check, &no_longer_emitted); // Stage 4: check list is emitted by compiler - check_error_codes_used(search_paths, &error_codes, &mut errors, &no_longer_emitted, verbose); - - // Print any errors. - for error in errors { - tidy_error!(bad, "{}", error); - } + check_error_codes_used(search_paths, &error_codes, &mut check, &no_longer_emitted); } -fn check_removed_error_code_explanation(ci_info: &crate::CiInfo, bad: &mut bool) { +fn check_removed_error_code_explanation(ci_info: &crate::CiInfo, check: &mut RunningCheck) { let Some(base_commit) = &ci_info.base_commit else { - eprintln!("Skipping error code explanation removal check"); + check.verbose_msg("Skipping error code explanation removal check"); return; }; let Some(diff) = crate::git_diff(base_commit, "--name-status") else { - *bad = true; - eprintln!("removed error code explanation tidy check: Failed to run git diff"); + check.error(format!("removed error code explanation: Failed to run git diff")); return; }; if diff.lines().any(|line| { line.starts_with('D') && line.contains("compiler/rustc_error_codes/src/error_codes/") }) { - *bad = true; - eprintln!("tidy check error: Error code explanations should never be removed!"); - eprintln!("Take a look at E0001 to see how to handle it."); + check.error(format!( + r#"Error code explanations should never be removed! +Take a look at E0001 to see how to handle it."# + )); return; } - println!("No error code explanation was removed!"); + check.verbose_msg("No error code explanation was removed!"); } /// Stage 1: Parses a list of error codes from `error_codes.rs`. -fn extract_error_codes(root_path: &Path, errors: &mut Vec<String>) -> Vec<String> { +fn extract_error_codes(root_path: &Path, check: &mut RunningCheck) -> Vec<String> { let path = root_path.join(Path::new(ERROR_CODES_PATH)); let file = fs::read_to_string(&path).unwrap_or_else(|e| panic!("failed to read `{path:?}`: {e}")); @@ -117,7 +97,7 @@ fn extract_error_codes(root_path: &Path, errors: &mut Vec<String>) -> Vec<String // Extract the error code from the line. Emit a fatal error if it is not in the correct // format. let Some(split_line) = split_line else { - errors.push(format!( + check.error(format!( "{path}:{line_index}: Expected a line with the format `Eabcd: abcd, \ but got \"{line}\" without a `:` delimiter", )); @@ -128,8 +108,9 @@ fn extract_error_codes(root_path: &Path, errors: &mut Vec<String>) -> Vec<String // If this is a duplicate of another error code, emit a fatal error. if error_codes.contains(&err_code) { - errors - .push(format!("{path}:{line_index}: Found duplicate error code: `{err_code}`")); + check.error(format!( + "{path}:{line_index}: Found duplicate error code: `{err_code}`" + )); continue; } @@ -140,14 +121,14 @@ fn extract_error_codes(root_path: &Path, errors: &mut Vec<String>) -> Vec<String // Ensure that the line references the correct markdown file. let rest = split_line.1.split_once(','); let Some(rest) = rest else { - errors.push(format!( + check.error(format!( "{path}:{line_index}: Expected a line with the format `Eabcd: abcd, \ but got \"{line}\" without a `,` delimiter", )); continue; }; if error_num_as_str != rest.0.trim() { - errors.push(format!( + check.error(format!( "{path}:{line_index}: `{}:` should be followed by `{},` but instead found `{}` in \ `compiler/rustc_error_codes/src/lib.rs`", err_code, @@ -157,7 +138,7 @@ fn extract_error_codes(root_path: &Path, errors: &mut Vec<String>) -> Vec<String continue; } if !rest.1.trim().is_empty() && !rest.1.trim().starts_with("//") { - errors.push(format!("{path}:{line_index}: should only have one error per line")); + check.error(format!("{path}:{line_index}: should only have one error per line")); continue; } @@ -172,8 +153,7 @@ fn extract_error_codes(root_path: &Path, errors: &mut Vec<String>) -> Vec<String fn check_error_codes_docs( root_path: &Path, error_codes: &[String], - errors: &mut Vec<String>, - verbose: bool, + check: &mut RunningCheck, ) -> Vec<String> { let docs_path = root_path.join(Path::new(ERROR_DOCS_PATH)); @@ -184,7 +164,7 @@ fn check_error_codes_docs( // Error if the file isn't markdown. if path.extension() != Some(OsStr::new("md")) { - errors.push(format!( + check.error(format!( "Found unexpected non-markdown file in error code docs directory: {}", path.display() )); @@ -196,7 +176,7 @@ fn check_error_codes_docs( let err_code = filename.unwrap().0; // `unwrap` is ok because we know the filename is in the correct format. if error_codes.iter().all(|e| e != err_code) { - errors.push(format!( + check.error(format!( "Found valid file `{}` in error code docs directory without corresponding \ entry in `rustc_error_codes/src/lib.rs`", path.display() @@ -208,11 +188,10 @@ fn check_error_codes_docs( check_explanation_has_doctest(contents, err_code); if emit_ignore_warning { - verbose_print!( - verbose, + check.verbose_msg(format!( "warning: Error code `{err_code}` uses the ignore header. This should not be used, add the error code to the \ `IGNORE_DOCTEST_CHECK` constant instead." - ); + )); } if no_longer_emitted { @@ -220,11 +199,10 @@ fn check_error_codes_docs( } if !found_code_example { - verbose_print!( - verbose, + check.verbose_msg(format!( "warning: Error code `{err_code}` doesn't have a code example, all error codes are expected to have one \ (even if untested)." - ); + )); return; } @@ -232,12 +210,12 @@ fn check_error_codes_docs( // Check that the explanation has a doctest, and if it shouldn't, that it doesn't if !found_proper_doctest && !test_ignored { - errors.push(format!( + check.error(format!( "`{}` doesn't use its own error code in compile_fail example", path.display(), )); } else if found_proper_doctest && test_ignored { - errors.push(format!( + check.error(format!( "`{}` has a compile_fail doctest with its own error code, it shouldn't \ be listed in `IGNORE_DOCTEST_CHECK`", path.display(), @@ -289,8 +267,7 @@ fn check_explanation_has_doctest(explanation: &str, err_code: &str) -> (bool, bo fn check_error_codes_tests( root_path: &Path, error_codes: &[String], - errors: &mut Vec<String>, - verbose: bool, + check: &mut RunningCheck, no_longer_emitted: &[String], ) { let tests_path = root_path.join(Path::new(ERROR_TESTS_PATH)); @@ -299,15 +276,14 @@ fn check_error_codes_tests( let test_path = tests_path.join(format!("{code}.stderr")); if !test_path.exists() && !IGNORE_UI_TEST_CHECK.contains(&code.as_str()) { - verbose_print!( - verbose, + check.verbose_msg(format!( "warning: Error code `{code}` needs to have at least one UI test in the `tests/error-codes/` directory`!" - ); + )); continue; } if IGNORE_UI_TEST_CHECK.contains(&code.as_str()) { if test_path.exists() { - errors.push(format!( + check.error(format!( "Error code `{code}` has a UI test in `tests/ui/error-codes/{code}.rs`, it shouldn't be listed in `EXEMPTED_FROM_TEST`!" )); } @@ -317,11 +293,10 @@ fn check_error_codes_tests( let file = match fs::read_to_string(&test_path) { Ok(file) => file, Err(err) => { - verbose_print!( - verbose, + check.verbose_msg(format!( "warning: Failed to read UI test file (`{}`) for `{code}` but the file exists. The test is assumed to work:\n{err}", test_path.display() - ); + )); continue; } }; @@ -343,10 +318,9 @@ fn check_error_codes_tests( } if !found_code { - verbose_print!( - verbose, + check.verbose_msg(format!( "warning: Error code `{code}` has a UI test file, but doesn't contain its own error code!" - ); + )); } } } @@ -355,9 +329,8 @@ fn check_error_codes_tests( fn check_error_codes_used( search_paths: &[&Path], error_codes: &[String], - errors: &mut Vec<String>, + check: &mut RunningCheck, no_longer_emitted: &[String], - verbose: bool, ) { // Search for error codes in the form `E0123`. let regex = Regex::new(r#"\bE\d{4}\b"#).unwrap(); @@ -384,7 +357,7 @@ fn check_error_codes_used( if !error_codes.contains(&error_code) { // This error code isn't properly defined, we must error. - errors.push(format!("Error code `{error_code}` is used in the compiler but not defined and documented in `compiler/rustc_error_codes/src/lib.rs`.")); + check.error(format!("Error code `{error_code}` is used in the compiler but not defined and documented in `compiler/rustc_error_codes/src/lib.rs`.")); continue; } @@ -397,7 +370,7 @@ fn check_error_codes_used( for code in error_codes { if !found_codes.contains(code) && !no_longer_emitted.contains(code) { - errors.push(format!( + check.error(format!( "Error code `{code}` exists, but is not emitted by the compiler!\n\ Please mark the code as no longer emitted by adding the following note to the top of the `EXXXX.md` file:\n\ `#### Note: this error code is no longer emitted by the compiler`\n\ @@ -406,10 +379,9 @@ fn check_error_codes_used( } if found_codes.contains(code) && no_longer_emitted.contains(code) { - verbose_print!( - verbose, + check.verbose_msg(format!( "warning: Error code `{code}` is used when it's marked as \"no longer emitted\"" - ); + )); } } } diff --git a/src/tools/tidy/src/extdeps.rs b/src/tools/tidy/src/extdeps.rs index 2b212cfa67a..f75de13b45c 100644 --- a/src/tools/tidy/src/extdeps.rs +++ b/src/tools/tidy/src/extdeps.rs @@ -4,6 +4,7 @@ use std::fs; use std::path::Path; use crate::deps::WorkspaceInfo; +use crate::diagnostics::DiagCtx; /// List of allowed sources for packages. const ALLOWED_SOURCES: &[&str] = &[ @@ -14,7 +15,9 @@ const ALLOWED_SOURCES: &[&str] = &[ /// Checks for external package sources. `root` is the path to the directory that contains the /// workspace `Cargo.toml`. -pub fn check(root: &Path, bad: &mut bool) { +pub fn check(root: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check("extdeps"); + for &WorkspaceInfo { path, submodules, .. } in crate::deps::WORKSPACES { if crate::deps::has_missing_submodule(root, submodules) { continue; @@ -25,7 +28,7 @@ pub fn check(root: &Path, bad: &mut bool) { let lockfile = root.join(path).join("Cargo.lock"); if !lockfile.exists() { - tidy_error!(bad, "the `{path}` workspace doesn't have a Cargo.lock"); + check.error(format!("the `{path}` workspace doesn't have a Cargo.lock")); continue; } @@ -44,7 +47,7 @@ pub fn check(root: &Path, bad: &mut bool) { // Ensure source is allowed. if !ALLOWED_SOURCES.contains(&source) { - tidy_error!(bad, "invalid source: {}", source); + check.error(format!("invalid source: {}", source)); } } } diff --git a/src/tools/tidy/src/extra_checks/mod.rs b/src/tools/tidy/src/extra_checks/mod.rs index a6b50b5ca47..9f2f2da2e86 100644 --- a/src/tools/tidy/src/extra_checks/mod.rs +++ b/src/tools/tidy/src/extra_checks/mod.rs @@ -24,6 +24,7 @@ use std::str::FromStr; use std::{fmt, fs, io}; use crate::CiInfo; +use crate::diagnostics::DiagCtx; mod rustdoc_js; @@ -54,8 +55,10 @@ pub fn check( bless: bool, extra_checks: Option<&str>, pos_args: &[String], - bad: &mut bool, + diag_ctx: DiagCtx, ) { + let mut check = diag_ctx.start_check("extra_checks"); + if let Err(e) = check_impl( root_path, outdir, @@ -68,7 +71,7 @@ pub fn check( extra_checks, pos_args, ) { - tidy_error!(bad, "{e}"); + check.error(e); } } diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index 6618ba24be6..0a0ba217c63 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -16,6 +16,7 @@ use std::num::NonZeroU32; use std::path::{Path, PathBuf}; use std::{fmt, fs}; +use crate::diagnostics::{DiagCtx, RunningCheck}; use crate::walk::{filter_dirs, filter_not_rust, walk, walk_many}; #[cfg(test)] @@ -91,13 +92,14 @@ pub fn check( tests_path: &Path, compiler_path: &Path, lib_path: &Path, - bad: &mut bool, - verbose: bool, + diag_ctx: DiagCtx, ) -> CollectedFeatures { - let mut features = collect_lang_features(compiler_path, bad); + let mut check = diag_ctx.start_check("features"); + + let mut features = collect_lang_features(compiler_path, &mut check); assert!(!features.is_empty()); - let lib_features = get_and_check_lib_features(lib_path, bad, &features); + let lib_features = get_and_check_lib_features(lib_path, &mut check, &features); assert!(!lib_features.is_empty()); walk_many( @@ -121,7 +123,7 @@ pub fn check( for (i, line) in contents.lines().enumerate() { let mut err = |msg: &str| { - tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg); + check.error(format!("{}:{}: {}", file.display(), i + 1, msg)); }; let gate_test_str = "gate-test-"; @@ -175,7 +177,7 @@ pub fn check( } if !gate_untested.is_empty() { - tidy_error!(bad, "Found {} features without a gate test.", gate_untested.len()); + check.error(format!("Found {} features without a gate test.", gate_untested.len())); } let (version, channel) = get_version_and_channel(src_path); @@ -189,39 +191,32 @@ pub fn check( let file = feature.file.display(); let line = feature.line; if since > version && since != Version::CurrentPlaceholder { - tidy_error!( - bad, + check.error(format!( "{file}:{line}: The stabilization version {since} of {kind} feature `{feature_name}` is newer than the current {version}" - ); + )); } if channel == "nightly" && since == version { - tidy_error!( - bad, + check.error(format!( "{file}:{line}: The stabilization version {since} of {kind} feature `{feature_name}` is written out but should be {}", version::VERSION_PLACEHOLDER - ); + )); } if channel != "nightly" && since == Version::CurrentPlaceholder { - tidy_error!( - bad, + check.error(format!( "{file}:{line}: The placeholder use of {kind} feature `{feature_name}` is not allowed on the {channel} channel", - ); + )); } } - if *bad { - return CollectedFeatures { lib: lib_features, lang: features }; - } - - if verbose { + if !check.is_bad() && check.is_verbose_enabled() { 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}"); - } + + check.verbose_msg( + lines.into_iter().map(|l| format!("* {l}")).collect::<Vec<String>>().join("\n"), + ); } CollectedFeatures { lib: lib_features, lang: features } @@ -275,15 +270,20 @@ fn test_filen_gate<'f>(filen_underscore: &'f str, features: &mut Features) -> Op None } -pub fn collect_lang_features(base_compiler_path: &Path, bad: &mut bool) -> Features { +pub fn collect_lang_features(base_compiler_path: &Path, check: &mut RunningCheck) -> Features { let mut features = Features::new(); - collect_lang_features_in(&mut features, base_compiler_path, "accepted.rs", bad); - collect_lang_features_in(&mut features, base_compiler_path, "removed.rs", bad); - collect_lang_features_in(&mut features, base_compiler_path, "unstable.rs", bad); + collect_lang_features_in(&mut features, base_compiler_path, "accepted.rs", check); + collect_lang_features_in(&mut features, base_compiler_path, "removed.rs", check); + collect_lang_features_in(&mut features, base_compiler_path, "unstable.rs", check); features } -fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, bad: &mut bool) { +fn collect_lang_features_in( + features: &mut Features, + base: &Path, + file: &str, + check: &mut RunningCheck, +) { let path = base.join("rustc_feature").join("src").join(file); let contents = t!(fs::read_to_string(&path)); @@ -315,13 +315,11 @@ fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, ba if line.starts_with(FEATURE_GROUP_START_PREFIX) { if in_feature_group { - tidy_error!( - bad, - "{}:{}: \ + check.error(format!( + "{}:{line_number}: \ new feature group is started without ending the previous one", - path.display(), - line_number, - ); + path.display() + )); } in_feature_group = true; @@ -353,14 +351,10 @@ fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, ba let since = match since_str.parse() { Ok(since) => Some(since), Err(err) => { - tidy_error!( - bad, - "{}:{}: failed to parse since: {} ({:?})", - path.display(), - line_number, - since_str, - err, - ); + check.error(format!( + "{}:{line_number}: failed to parse since: {since_str} ({err:?})", + path.display() + )); None } }; @@ -371,13 +365,10 @@ fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, ba let correct_index = match prev_names.binary_search(&name) { Ok(_) => { // This only occurs when the feature name has already been declared. - tidy_error!( - bad, - "{}:{}: duplicate feature {}", - path.display(), - line_number, - name, - ); + check.error(format!( + "{}:{line_number}: duplicate feature {name}", + path.display() + )); // skip any additional checks for this line continue; } @@ -398,14 +389,10 @@ fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, ba ) }; - tidy_error!( - bad, - "{}:{}: feature {} is not sorted by feature name (should be {})", + check.error(format!( + "{}:{line_number}: feature {name} is not sorted by feature name (should be {correct_placement})", path.display(), - line_number, - name, - correct_placement, - ); + )); } prev_names.push(name); } @@ -413,13 +400,10 @@ fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, ba let issue_str = parts.next().unwrap().trim(); let tracking_issue = if issue_str.starts_with("None") { if level == Status::Unstable && !next_feature_omits_tracking_issue { - tidy_error!( - bad, - "{}:{}: no tracking issue for feature {}", + check.error(format!( + "{}:{line_number}: no tracking issue for feature {name}", path.display(), - line_number, - name, - ); + )); } None } else { @@ -428,13 +412,11 @@ fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, ba }; match features.entry(name.to_owned()) { Entry::Occupied(e) => { - tidy_error!( - bad, - "{}:{} feature {name} already specified with status '{}'", + check.error(format!( + "{}:{line_number} feature {name} already specified with status '{}'", path.display(), - line_number, e.get().level, - ); + )); } Entry::Vacant(e) => { e.insert(Feature { @@ -458,7 +440,7 @@ fn collect_lang_features_in(features: &mut Features, base: &Path, file: &str, ba fn get_and_check_lib_features( base_src_path: &Path, - bad: &mut bool, + check: &mut RunningCheck, lang_features: &Features, ) -> Features { let mut lib_features = Features::new(); @@ -469,16 +451,12 @@ fn get_and_check_lib_features( && f.tracking_issue != s.tracking_issue && f.level != Status::Accepted { - tidy_error!( - bad, - "{}:{}: feature gate {} has inconsistent `issue`: \"{}\" mismatches the {} `issue` of \"{}\"", + check.error(format!( + "{}:{line}: feature gate {name} has inconsistent `issue`: \"{}\" mismatches the {display} `issue` of \"{}\"", file.display(), - line, - name, f.tracking_issue_display(), - display, s.tracking_issue_display(), - ); + )); } }; check_features(&f, lang_features, "corresponding lang feature"); @@ -486,7 +464,7 @@ fn get_and_check_lib_features( lib_features.insert(name.to_owned(), f); } Err(msg) => { - tidy_error!(bad, "{}:{}: {}", file.display(), line, msg); + check.error(format!("{}:{line}: {msg}", file.display())); } }); lib_features diff --git a/src/tools/tidy/src/filenames.rs b/src/tools/tidy/src/filenames.rs index 53115f4eaa4..835cbefbf69 100644 --- a/src/tools/tidy/src/filenames.rs +++ b/src/tools/tidy/src/filenames.rs @@ -10,7 +10,10 @@ use std::path::Path; use std::process::Command; -pub fn check(root_path: &Path, bad: &mut bool) { +use crate::diagnostics::DiagCtx; + +pub fn check(root_path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check("filenames"); let stat_output = Command::new("git") .arg("-C") .arg(root_path) @@ -20,20 +23,17 @@ pub fn check(root_path: &Path, bad: &mut bool) { .stdout; for filename in stat_output.split(|&b| b == 0) { match str::from_utf8(filename) { - Err(_) => tidy_error!( - bad, + Err(_) => check.error(format!( r#"non-UTF8 file names are not supported: "{}""#, String::from_utf8_lossy(filename), - ), - Ok(name) if name.chars().any(|c| c.is_control()) => tidy_error!( - bad, + )), + Ok(name) if name.chars().any(|c| c.is_control()) => check.error(format!( r#"control characters are not supported in file names: "{}""#, String::from_utf8_lossy(filename), - ), - Ok(name) if name.contains(':') => tidy_error!( - bad, + )), + Ok(name) if name.contains(':') => check.error(format!( r#"":" is not supported in file names because of Windows compatibility: "{name}""#, - ), + )), _ => (), } } diff --git a/src/tools/tidy/src/fluent_alphabetical.rs b/src/tools/tidy/src/fluent_alphabetical.rs index 48d14a37514..02b914c21ae 100644 --- a/src/tools/tidy/src/fluent_alphabetical.rs +++ b/src/tools/tidy/src/fluent_alphabetical.rs @@ -7,6 +7,7 @@ use std::path::Path; use regex::Regex; +use crate::diagnostics::{CheckId, DiagCtx, RunningCheck}; use crate::walk::{filter_dirs, walk}; fn message() -> &'static Regex { @@ -20,19 +21,17 @@ fn is_fluent(path: &Path) -> bool { fn check_alphabetic( filename: &str, fluent: &str, - bad: &mut bool, + check: &mut RunningCheck, all_defined_msgs: &mut HashMap<String, String>, ) { let mut matches = message().captures_iter(fluent).peekable(); while let Some(m) = matches.next() { let name = m.get(1).unwrap(); if let Some(defined_filename) = all_defined_msgs.get(name.as_str()) { - tidy_error!( - bad, - "{filename}: message `{}` is already defined in {}", + check.error(format!( + "{filename}: message `{}` is already defined in {defined_filename}", name.as_str(), - defined_filename, - ); + )); } all_defined_msgs.insert(name.as_str().to_owned(), filename.to_owned()); @@ -40,13 +39,12 @@ fn check_alphabetic( if let Some(next) = matches.peek() { let next = next.get(1).unwrap(); if name.as_str() > next.as_str() { - tidy_error!( - bad, + check.error(format!( "{filename}: message `{}` appears before `{}`, but is alphabetically later than it run `./x.py test tidy --bless` to sort the file correctly", name.as_str(), next.as_str() - ); + )); } } else { break; @@ -57,7 +55,7 @@ run `./x.py test tidy --bless` to sort the file correctly", fn sort_messages( filename: &str, fluent: &str, - bad: &mut bool, + check: &mut RunningCheck, all_defined_msgs: &mut HashMap<String, String>, ) -> String { let mut chunks = vec![]; @@ -65,12 +63,10 @@ fn sort_messages( for line in fluent.lines() { if let Some(name) = message().find(line) { if let Some(defined_filename) = all_defined_msgs.get(name.as_str()) { - tidy_error!( - bad, - "{filename}: message `{}` is already defined in {}", + check.error(format!( + "{filename}: message `{}` is already defined in {defined_filename}", name.as_str(), - defined_filename, - ); + )); } all_defined_msgs.insert(name.as_str().to_owned(), filename.to_owned()); @@ -88,7 +84,9 @@ fn sort_messages( out } -pub fn check(path: &Path, bless: bool, bad: &mut bool) { +pub fn check(path: &Path, bless: bool, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("fluent_alphabetical").path(path)); + let mut all_defined_msgs = HashMap::new(); walk( path, @@ -98,7 +96,7 @@ pub fn check(path: &Path, bless: bool, bad: &mut bool) { let sorted = sort_messages( ent.path().to_str().unwrap(), contents, - bad, + &mut check, &mut all_defined_msgs, ); if sorted != contents { @@ -110,12 +108,12 @@ pub fn check(path: &Path, bless: bool, bad: &mut bool) { check_alphabetic( ent.path().to_str().unwrap(), contents, - bad, + &mut check, &mut all_defined_msgs, ); } }, ); - crate::fluent_used::check(path, all_defined_msgs, bad); + crate::fluent_used::check(path, all_defined_msgs, diag_ctx); } diff --git a/src/tools/tidy/src/fluent_lowercase.rs b/src/tools/tidy/src/fluent_lowercase.rs index 13f0319909e..1d80fda8f3b 100644 --- a/src/tools/tidy/src/fluent_lowercase.rs +++ b/src/tools/tidy/src/fluent_lowercase.rs @@ -4,6 +4,7 @@ use std::path::Path; use fluent_syntax::ast::{Entry, Message, PatternElement}; +use crate::diagnostics::{CheckId, DiagCtx, RunningCheck}; use crate::walk::{filter_dirs, walk}; #[rustfmt::skip] @@ -34,7 +35,7 @@ fn is_allowed_capitalized_word(msg: &str) -> bool { }) } -fn check_lowercase(filename: &str, contents: &str, bad: &mut bool) { +fn check_lowercase(filename: &str, contents: &str, check: &mut RunningCheck) { let (Ok(parse) | Err((parse, _))) = fluent_syntax::parser::parse(contents); for entry in &parse.body { @@ -45,20 +46,20 @@ fn check_lowercase(filename: &str, contents: &str, bad: &mut bool) { && value.chars().next().is_some_and(char::is_uppercase) && !is_allowed_capitalized_word(value) { - tidy_error!( - bad, + check.error(format!( "{filename}: message `{value}` starts with an uppercase letter. Fix it or add it to `ALLOWED_CAPITALIZED_WORDS`" - ); + )); } } } -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("fluent_lowercase").path(path)); walk( path, |path, is_dir| filter_dirs(path) || (!is_dir && filter_fluent(path)), &mut |ent, contents| { - check_lowercase(ent.path().to_str().unwrap(), contents, bad); + check_lowercase(ent.path().to_str().unwrap(), contents, &mut check); }, ); } diff --git a/src/tools/tidy/src/fluent_period.rs b/src/tools/tidy/src/fluent_period.rs index 836b5699289..c7c760b8d54 100644 --- a/src/tools/tidy/src/fluent_period.rs +++ b/src/tools/tidy/src/fluent_period.rs @@ -4,6 +4,7 @@ use std::path::Path; use fluent_syntax::ast::{Entry, PatternElement}; +use crate::diagnostics::{CheckId, DiagCtx, RunningCheck}; use crate::walk::{filter_dirs, walk}; fn filter_fluent(path: &Path) -> bool { @@ -20,7 +21,7 @@ const ALLOWLIST: &[&str] = &[ "incremental_corrupt_file", ]; -fn check_period(filename: &str, contents: &str, bad: &mut bool) { +fn check_period(filename: &str, contents: &str, check: &mut RunningCheck) { if filename.contains("codegen") { // FIXME: Too many codegen messages have periods right now... return; @@ -40,7 +41,7 @@ fn check_period(filename: &str, contents: &str, bad: &mut bool) { if value.ends_with(".") && !value.ends_with("...") { let ll = find_line(contents, value); let name = m.id.name; - tidy_error!(bad, "{filename}:{ll}: message `{name}` ends in a period"); + check.error(format!("{filename}:{ll}: message `{name}` ends in a period")); } } @@ -56,7 +57,7 @@ fn check_period(filename: &str, contents: &str, bad: &mut bool) { { let ll = find_line(contents, value); let name = attr.id.name; - tidy_error!(bad, "{filename}:{ll}: attr `{name}` ends in a period"); + check.error(format!("{filename}:{ll}: attr `{name}` ends in a period")); } } } @@ -74,12 +75,14 @@ fn find_line(haystack: &str, needle: &str) -> usize { 1 } -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("fluent_period").path(path)); + walk( path, |path, is_dir| filter_dirs(path) || (!is_dir && filter_fluent(path)), &mut |ent, contents| { - check_period(ent.path().to_str().unwrap(), contents, bad); + check_period(ent.path().to_str().unwrap(), contents, &mut check); }, ); } diff --git a/src/tools/tidy/src/fluent_used.rs b/src/tools/tidy/src/fluent_used.rs index 909bf482ddf..2047089631b 100644 --- a/src/tools/tidy/src/fluent_used.rs +++ b/src/tools/tidy/src/fluent_used.rs @@ -3,6 +3,7 @@ use std::collections::HashMap; use std::path::Path; +use crate::diagnostics::{CheckId, DiagCtx}; use crate::walk::{filter_dirs, walk}; fn filter_used_messages( @@ -27,13 +28,15 @@ fn filter_used_messages( } } -pub fn check(path: &Path, mut all_defined_msgs: HashMap<String, String>, bad: &mut bool) { +pub fn check(path: &Path, mut all_defined_msgs: HashMap<String, String>, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("fluent_used").path(path)); + let mut msgs_appear_only_once = HashMap::new(); walk(path, |path, _| filter_dirs(path), &mut |_, contents| { filter_used_messages(contents, &mut all_defined_msgs, &mut msgs_appear_only_once); }); for (name, filename) in msgs_appear_only_once { - tidy_error!(bad, "{filename}: message `{}` is not used", name,); + check.error(format!("{filename}: message `{name}` is not used")); } } diff --git a/src/tools/tidy/src/gcc_submodule.rs b/src/tools/tidy/src/gcc_submodule.rs index 217eaf1758c..3a6e3247de6 100644 --- a/src/tools/tidy/src/gcc_submodule.rs +++ b/src/tools/tidy/src/gcc_submodule.rs @@ -4,7 +4,11 @@ use std::path::Path; use std::process::Command; -pub fn check(root_path: &Path, compiler_path: &Path, bad: &mut bool) { +use crate::diagnostics::DiagCtx; + +pub fn check(root_path: &Path, compiler_path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check("gcc_submodule"); + let cg_gcc_version_path = compiler_path.join("rustc_codegen_gcc/libgccjit.version"); let cg_gcc_version = std::fs::read_to_string(&cg_gcc_version_path) .unwrap_or_else(|_| { @@ -26,7 +30,7 @@ pub fn check(root_path: &Path, compiler_path: &Path, bad: &mut bool) { // Git is not available or we are in a tarball if !git_output.status.success() { - eprintln!("Cannot figure out the SHA of the GCC submodule"); + check.message("Cannot figure out the SHA of the GCC submodule"); return; } @@ -43,12 +47,11 @@ pub fn check(root_path: &Path, compiler_path: &Path, bad: &mut bool) { // The SHA can start with + if the submodule is modified or - if it is not checked out. let gcc_submodule_sha = git_output.trim_start_matches(['+', '-']); if gcc_submodule_sha != cg_gcc_version { - *bad = true; - eprintln!( + check.error(format!( r#"Commit SHA of the src/gcc submodule (`{gcc_submodule_sha}`) does not match the required GCC version of the GCC codegen backend (`{cg_gcc_version}`). Make sure to set the src/gcc submodule to commit {cg_gcc_version}. The GCC codegen backend commit is configured at {}."#, cg_gcc_version_path.display(), - ); + )); } } diff --git a/src/tools/tidy/src/known_bug.rs b/src/tools/tidy/src/known_bug.rs index e1921715ab9..d3b75e0cf5b 100644 --- a/src/tools/tidy/src/known_bug.rs +++ b/src/tools/tidy/src/known_bug.rs @@ -2,9 +2,11 @@ use std::path::Path; +use crate::diagnostics::{CheckId, DiagCtx}; use crate::walk::*; -pub fn check(filepath: &Path, bad: &mut bool) { +pub fn check(filepath: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("known_bug").path(filepath)); walk(filepath, |path, _is_dir| filter_not_rust(path), &mut |entry, contents| { let file: &Path = entry.path(); @@ -19,11 +21,10 @@ pub fn check(filepath: &Path, bad: &mut bool) { [.., "tests", "crashes", "auxiliary", _aux_file_rs] ) && !contents.lines().any(|line| line.starts_with("//@ known-bug: ")) { - tidy_error!( - bad, + check.error(format!( "{} crash/ice test does not have a \"//@ known-bug: \" directive", file.display() - ); + )); } }); } diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index f7920e3205a..0bfee93796b 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -11,7 +11,8 @@ use std::{env, io}; use build_helper::ci::CiEnv; use build_helper::git::{GitConfig, get_closest_upstream_commit}; use build_helper::stage0_parser::{Stage0Config, parse_stage0_file}; -use termcolor::WriteColor; + +use crate::diagnostics::{DiagCtx, RunningCheck}; macro_rules! static_regex { ($re:literal) => {{ @@ -43,35 +44,6 @@ macro_rules! t { }; } -macro_rules! tidy_error { - ($bad:expr, $($fmt:tt)*) => ({ - $crate::tidy_error(&format_args!($($fmt)*).to_string()).expect("failed to output error"); - *$bad = true; - }); -} - -macro_rules! tidy_error_ext { - ($tidy_error:path, $bad:expr, $($fmt:tt)*) => ({ - $tidy_error(&format_args!($($fmt)*).to_string()).expect("failed to output error"); - *$bad = true; - }); -} - -fn tidy_error(args: &str) -> std::io::Result<()> { - use std::io::Write; - - use termcolor::{Color, ColorChoice, ColorSpec, StandardStream}; - - let mut stderr = StandardStream::stdout(ColorChoice::Auto); - stderr.set_color(ColorSpec::new().set_fg(Some(Color::Red)))?; - - write!(&mut stderr, "tidy error")?; - stderr.set_color(&ColorSpec::new())?; - - writeln!(&mut stderr, ": {args}")?; - Ok(()) -} - pub struct CiInfo { pub git_merge_commit_email: String, pub nightly_branch: String, @@ -80,7 +52,9 @@ pub struct CiInfo { } impl CiInfo { - pub fn new(bad: &mut bool) -> Self { + pub fn new(diag_ctx: DiagCtx) -> Self { + let mut check = diag_ctx.start_check("CI history"); + let stage0 = parse_stage0_file(); let Stage0Config { nightly_branch, git_merge_commit_email, .. } = stage0.config; @@ -93,11 +67,14 @@ impl CiInfo { let base_commit = match get_closest_upstream_commit(None, &info.git_config(), info.ci_env) { Ok(Some(commit)) => Some(commit), Ok(None) => { - info.error_if_in_ci("no base commit found", bad); + info.error_if_in_ci("no base commit found", &mut check); None } Err(error) => { - info.error_if_in_ci(&format!("failed to retrieve base commit: {error}"), bad); + info.error_if_in_ci( + &format!("failed to retrieve base commit: {error}"), + &mut check, + ); None } }; @@ -112,12 +89,11 @@ impl CiInfo { } } - pub fn error_if_in_ci(&self, msg: &str, bad: &mut bool) { + pub fn error_if_in_ci(&self, msg: &str, check: &mut RunningCheck) { if self.ci_env.is_running_in_ci() { - *bad = true; - eprintln!("tidy check error: {msg}"); + check.error(msg); } else { - eprintln!("tidy check warning: {msg}. Some checks will be skipped."); + check.warning(format!("{msg}. Some checks will be skipped.")); } } } @@ -250,6 +226,7 @@ pub mod alphabetical; pub mod bins; pub mod debug_artifacts; pub mod deps; +pub mod diagnostics; pub mod edition; pub mod error_codes; pub mod extdeps; diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index f9e82341b7a..93bc1611199 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -8,10 +8,10 @@ use std::collections::VecDeque; use std::num::NonZeroUsize; use std::path::PathBuf; use std::str::FromStr; -use std::sync::atomic::{AtomicBool, Ordering}; use std::thread::{self, ScopedJoinHandle, scope}; use std::{env, process}; +use tidy::diagnostics::{COLOR_ERROR, COLOR_SUCCESS, DiagCtx, output_message}; use tidy::*; fn main() { @@ -50,9 +50,8 @@ fn main() { let extra_checks = cfg_args.iter().find(|s| s.starts_with("--extra-checks=")).map(String::as_str); - let mut bad = false; - let ci_info = CiInfo::new(&mut bad); - let bad = std::sync::Arc::new(AtomicBool::new(bad)); + let diag_ctx = DiagCtx::new(&root_path, verbose); + let ci_info = CiInfo::new(diag_ctx.clone()); let drain_handles = |handles: &mut VecDeque<ScopedJoinHandle<'_, ()>>| { // poll all threads for completion before awaiting the oldest one @@ -87,12 +86,9 @@ fn main() { (@ $p:ident, name=$name:expr $(, $args:expr)* ) => { drain_handles(&mut handles); + let diag_ctx = diag_ctx.clone(); let handle = thread::Builder::new().name($name).spawn_scoped(s, || { - let mut flag = false; - $p::check($($args, )* &mut flag); - if (flag) { - bad.store(true, Ordering::Relaxed); - } + $p::check($($args, )* diag_ctx); }).unwrap(); handles.push_back(handle); } @@ -118,7 +114,7 @@ fn main() { check!(unknown_revision, &tests_path); // Checks that only make sense for the compiler. - check!(error_codes, &root_path, &[&compiler_path, &librustdoc_path], verbose, &ci_info); + check!(error_codes, &root_path, &[&compiler_path, &librustdoc_path], &ci_info); check!(fluent_alphabetical, &compiler_path, bless); check!(fluent_period, &compiler_path); check!(fluent_lowercase, &compiler_path); @@ -155,25 +151,12 @@ fn main() { check!(x_version, &root_path, &cargo); check!(triagebot, &root_path); - check!(filenames, &root_path); let collected = { drain_handles(&mut handles); - let mut flag = false; - let r = features::check( - &src_path, - &tests_path, - &compiler_path, - &library_path, - &mut flag, - verbose, - ); - if flag { - bad.store(true, Ordering::Relaxed); - } - r + features::check(&src_path, &tests_path, &compiler_path, &library_path, diag_ctx.clone()) }; check!(unstable_book, &src_path, collected); @@ -192,8 +175,22 @@ fn main() { ); }); - if bad.load(Ordering::Relaxed) { - eprintln!("some tidy checks failed"); + let failed_checks = diag_ctx.into_failed_checks(); + if !failed_checks.is_empty() { + let mut failed: Vec<String> = + failed_checks.into_iter().map(|c| c.id().to_string()).collect(); + failed.sort(); + output_message( + &format!( + "The following check{} failed: {}", + if failed.len() > 1 { "s" } else { "" }, + failed.join(", ") + ), + None, + Some(COLOR_ERROR), + ); process::exit(1); + } else { + output_message("All tidy checks succeeded", None, Some(COLOR_SUCCESS)); } } diff --git a/src/tools/tidy/src/mir_opt_tests.rs b/src/tools/tidy/src/mir_opt_tests.rs index 6119eb58383..0f9fab51d09 100644 --- a/src/tools/tidy/src/mir_opt_tests.rs +++ b/src/tools/tidy/src/mir_opt_tests.rs @@ -5,9 +5,10 @@ use std::path::{Path, PathBuf}; use miropt_test_tools::PanicStrategy; +use crate::diagnostics::{CheckId, DiagCtx, RunningCheck}; use crate::walk::walk_no_read; -fn check_unused_files(path: &Path, bless: bool, bad: &mut bool) { +fn check_unused_files(path: &Path, bless: bool, check: &mut RunningCheck) { let mut rs_files = Vec::<PathBuf>::new(); let mut output_files = HashSet::<PathBuf>::new(); @@ -37,18 +38,17 @@ fn check_unused_files(path: &Path, bless: bool, bad: &mut bool) { for extra in output_files { if !bless { - tidy_error!( - bad, + check.error(format!( "the following output file is not associated with any mir-opt test, you can remove it: {}", extra.display() - ); + )); } else { let _ = std::fs::remove_file(extra); } } } -fn check_dash_files(path: &Path, bless: bool, bad: &mut bool) { +fn check_dash_files(path: &Path, bless: bool, check: &mut RunningCheck) { for file in walkdir::WalkDir::new(path.join("mir-opt")) .into_iter() .filter_map(Result::ok) @@ -60,11 +60,10 @@ fn check_dash_files(path: &Path, bless: bool, bad: &mut bool) { && name.contains('-') { if !bless { - tidy_error!( - bad, + check.error(format!( "mir-opt test files should not have dashes in them: {}", path.display() - ); + )); } else { let new_name = name.replace('-', "_"); let mut new_path = path.to_owned(); @@ -75,7 +74,9 @@ fn check_dash_files(path: &Path, bless: bool, bad: &mut bool) { } } -pub fn check(path: &Path, bless: bool, bad: &mut bool) { - check_unused_files(path, bless, bad); - check_dash_files(path, bless, bad); +pub fn check(path: &Path, bless: bool, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("mir_opt_tests").path(path)); + + check_unused_files(path, bless, &mut check); + check_dash_files(path, bless, &mut check); } diff --git a/src/tools/tidy/src/pal.rs b/src/tools/tidy/src/pal.rs index 991ad55809c..cefad7d9596 100644 --- a/src/tools/tidy/src/pal.rs +++ b/src/tools/tidy/src/pal.rs @@ -32,6 +32,7 @@ use std::path::Path; +use crate::diagnostics::{CheckId, DiagCtx, RunningCheck}; use crate::walk::{filter_dirs, walk}; // Paths that may contain platform-specific code. @@ -67,7 +68,9 @@ const EXCEPTION_PATHS: &[&str] = &[ "library/std/src/io/error.rs", // Repr unpacked needed for UEFI ]; -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("pal").path(path)); + // Sanity check that the complex parsing here works. let mut saw_target_arch = false; let mut saw_cfg_bang = false; @@ -88,7 +91,7 @@ pub fn check(path: &Path, bad: &mut bool) { return; } - check_cfgs(contents, file, bad, &mut saw_target_arch, &mut saw_cfg_bang); + check_cfgs(contents, file, &mut check, &mut saw_target_arch, &mut saw_cfg_bang); }); assert!(saw_target_arch); @@ -98,7 +101,7 @@ pub fn check(path: &Path, bad: &mut bool) { fn check_cfgs( contents: &str, file: &Path, - bad: &mut bool, + check: &mut RunningCheck, saw_target_arch: &mut bool, saw_cfg_bang: &mut bool, ) { @@ -115,7 +118,7 @@ fn check_cfgs( Ok(_) => unreachable!(), Err(i) => i + 1, }; - tidy_error!(bad, "{}:{}: platform-specific cfg: {}", file.display(), line, cfg); + check.error(format!("{}:{line}: platform-specific cfg: {cfg}", file.display())); }; for (idx, cfg) in cfgs { diff --git a/src/tools/tidy/src/rustdoc_css_themes.rs b/src/tools/tidy/src/rustdoc_css_themes.rs index af36f9ba58e..8d4af7a3bd5 100644 --- a/src/tools/tidy/src/rustdoc_css_themes.rs +++ b/src/tools/tidy/src/rustdoc_css_themes.rs @@ -3,7 +3,11 @@ use std::path::Path; -pub fn check(librustdoc_path: &Path, bad: &mut bool) { +use crate::diagnostics::{CheckId, DiagCtx, RunningCheck}; + +pub fn check(librustdoc_path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("rustdoc_css_themes").path(librustdoc_path)); + let rustdoc_css = "html/static/css/rustdoc.css"; let noscript_css = "html/static/css/noscript.css"; let rustdoc_css_contents = std::fs::read_to_string(librustdoc_path.join(rustdoc_css)) @@ -14,13 +18,13 @@ pub fn check(librustdoc_path: &Path, bad: &mut bool) { "light", rustdoc_css_contents.lines().enumerate().map(|(i, l)| (i + 1, l.trim())), noscript_css_contents.lines().enumerate().map(|(i, l)| (i + 1, l.trim())), - bad, + &mut check, ); compare_themes_from_files( "dark", rustdoc_css_contents.lines().enumerate(), noscript_css_contents.lines().enumerate(), - bad, + &mut check, ); } @@ -28,7 +32,7 @@ fn compare_themes_from_files<'a>( name: &str, mut rustdoc_css_lines: impl Iterator<Item = (usize, &'a str)>, mut noscript_css_lines: impl Iterator<Item = (usize, &'a str)>, - bad: &mut bool, + check: &mut RunningCheck, ) { let begin_theme_pat = format!("/* Begin theme: {name}"); let mut found_theme = None; @@ -38,10 +42,9 @@ fn compare_themes_from_files<'a>( continue; } if let Some(found_theme) = found_theme { - tidy_error!( - bad, + check.error(format!( "rustdoc.css contains two {name} themes on lines {rustdoc_css_line_number} and {found_theme}", - ); + )); return; } found_theme = Some(rustdoc_css_line_number); @@ -50,14 +53,13 @@ fn compare_themes_from_files<'a>( continue; } if let Some(found_theme_noscript) = found_theme_noscript { - tidy_error!( - bad, + check.error(format!( "noscript.css contains two {name} themes on lines {noscript_css_line_number} and {found_theme_noscript}", - ); + )); return; } found_theme_noscript = Some(noscript_css_line_number); - compare_themes(name, &mut rustdoc_css_lines, &mut noscript_css_lines, bad); + compare_themes(name, &mut rustdoc_css_lines, &mut noscript_css_lines, check); } } } @@ -66,7 +68,7 @@ fn compare_themes<'a>( name: &str, rustdoc_css_lines: impl Iterator<Item = (usize, &'a str)>, noscript_css_lines: impl Iterator<Item = (usize, &'a str)>, - bad: &mut bool, + check: &mut RunningCheck, ) { let end_theme_pat = format!("/* End theme: {name}"); for ( @@ -90,12 +92,11 @@ fn compare_themes<'a>( break; } if rustdoc_css_line != noscript_css_line { - tidy_error!( - bad, - "noscript.css:{noscript_css_line_number} and rustdoc.css:{rustdoc_css_line_number} contain copies of {name} theme that are not the same", - ); - eprintln!("- {noscript_css_line}"); - eprintln!("+ {rustdoc_css_line}"); + check.error(format!( + r#"noscript.css:{noscript_css_line_number} and rustdoc.css:{rustdoc_css_line_number} contain copies of {name} theme that are not the same +- {noscript_css_line} ++ {rustdoc_css_line}"#, + )); return; } } diff --git a/src/tools/tidy/src/rustdoc_gui_tests.rs b/src/tools/tidy/src/rustdoc_gui_tests.rs index 3b995f219d2..8ec300c42ce 100644 --- a/src/tools/tidy/src/rustdoc_gui_tests.rs +++ b/src/tools/tidy/src/rustdoc_gui_tests.rs @@ -2,18 +2,21 @@ use std::path::Path; -pub fn check(path: &Path, bad: &mut bool) { +use crate::diagnostics::{CheckId, DiagCtx}; + +pub fn check(path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("rustdoc_gui_tests").path(path)); + crate::walk::walk( &path.join("rustdoc-gui"), |p, is_dir| !is_dir && p.extension().is_none_or(|e| e != "goml"), &mut |entry, content| { for line in content.lines() { if !line.starts_with("// ") { - tidy_error!( - bad, + check.error(format!( "{}: rustdoc-gui tests must start with a small description", entry.path().display(), - ); + )); return; } else if line.starts_with("// ") { let parts = line[2..].trim(); diff --git a/src/tools/tidy/src/rustdoc_json.rs b/src/tools/tidy/src/rustdoc_json.rs index 722e1ebd0ca..7a53c08737f 100644 --- a/src/tools/tidy/src/rustdoc_json.rs +++ b/src/tools/tidy/src/rustdoc_json.rs @@ -4,19 +4,22 @@ use std::path::Path; use std::str::FromStr; +use crate::diagnostics::{CheckId, DiagCtx}; + const RUSTDOC_JSON_TYPES: &str = "src/rustdoc-json-types"; -pub fn check(src_path: &Path, ci_info: &crate::CiInfo, bad: &mut bool) { - println!("Checking tidy rustdoc_json..."); +pub fn check(src_path: &Path, ci_info: &crate::CiInfo, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("rustdoc_json").path(src_path)); + let Some(base_commit) = &ci_info.base_commit else { - eprintln!("No base commit, skipping rustdoc_json check"); + check.verbose_msg("No base commit, skipping rustdoc_json check"); return; }; // First we check that `src/rustdoc-json-types` was modified. if !crate::files_modified(ci_info, |p| p == RUSTDOC_JSON_TYPES) { // `rustdoc-json-types` was not modified so nothing more to check here. - println!("`rustdoc-json-types` was not modified."); + check.verbose_msg("`rustdoc-json-types` was not modified."); return; } // Then we check that if `FORMAT_VERSION` was updated, the `Latest feature:` was also updated. @@ -45,34 +48,29 @@ pub fn check(src_path: &Path, ci_info: &crate::CiInfo, bad: &mut bool) { } } if format_version_updated != latest_feature_comment_updated { - *bad = true; - if latest_feature_comment_updated { - eprintln!( - "error in `rustdoc_json` tidy check: `Latest feature` comment was updated \ - whereas `FORMAT_VERSION` wasn't in `{RUSTDOC_JSON_TYPES}/lib.rs`" - ); + let msg = if latest_feature_comment_updated { + format!( + "`Latest feature` comment was updated whereas `FORMAT_VERSION` wasn't in `{RUSTDOC_JSON_TYPES}/lib.rs`" + ) } else { - eprintln!( - "error in `rustdoc_json` tidy check: `Latest feature` comment was not \ - updated whereas `FORMAT_VERSION` was in `{RUSTDOC_JSON_TYPES}/lib.rs`" - ); - } + format!( + "`Latest feature` comment was not updated whereas `FORMAT_VERSION` was in `{RUSTDOC_JSON_TYPES}/lib.rs`" + ) + }; + check.error(msg); } match (new_version, old_version) { (Some(new_version), Some(old_version)) if new_version != old_version + 1 => { - *bad = true; - eprintln!( - "error in `rustdoc_json` tidy check: invalid `FORMAT_VERSION` increase in \ - `{RUSTDOC_JSON_TYPES}/lib.rs`, should be `{}`, found `{new_version}`", + check.error(format!( + "invalid `FORMAT_VERSION` increase in `{RUSTDOC_JSON_TYPES}/lib.rs`, should be `{}`, found `{new_version}`", old_version + 1, - ); + )); } _ => {} } } None => { - *bad = true; - eprintln!("error: failed to run `git diff` in rustdoc_json check"); + check.error("failed to run `git diff` in rustdoc_json check"); } } } diff --git a/src/tools/tidy/src/rustdoc_templates.rs b/src/tools/tidy/src/rustdoc_templates.rs index 597290a6a9a..4e5b9988d53 100644 --- a/src/tools/tidy/src/rustdoc_templates.rs +++ b/src/tools/tidy/src/rustdoc_templates.rs @@ -6,12 +6,15 @@ use std::path::Path; use ignore::DirEntry; +use crate::diagnostics::{CheckId, DiagCtx}; use crate::walk::walk; // Array containing `("beginning of tag", "end of tag")`. const TAGS: &[(&str, &str)] = &[("{#", "#}"), ("{%", "%}"), ("{{", "}}")]; -pub fn check(librustdoc_path: &Path, bad: &mut bool) { +pub fn check(librustdoc_path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("rustdoc_templates").path(librustdoc_path)); + walk( &librustdoc_path.join("html/templates"), |path, is_dir| is_dir || path.extension().is_none_or(|ext| ext != OsStr::new("html")), @@ -46,12 +49,11 @@ pub fn check(librustdoc_path: &Path, bad: &mut bool) { }) { // It seems like ending this line with a jinja tag is not needed after all. - tidy_error!( - bad, + check.error(format!( "`{}` at line {}: unneeded `{{# #}}` tag at the end of the line", path.path().display(), pos + 1, - ); + )); } continue; } @@ -67,12 +69,11 @@ pub fn check(librustdoc_path: &Path, bad: &mut bool) { }) { None => { // No it's not, let's error. - tidy_error!( - bad, + check.error(format!( "`{}` at line {}: missing `{{# #}}` at the end of the line", path.path().display(), pos + 1, - ); + )); } Some(end_tag) => { // We skip the tag. diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index fca097c091b..d17278edc84 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -24,6 +24,7 @@ use std::sync::LazyLock; use regex::RegexSetBuilder; use rustc_hash::FxHashMap; +use crate::diagnostics::{CheckId, DiagCtx}; use crate::walk::{filter_dirs, walk}; #[cfg(test)] @@ -338,7 +339,9 @@ fn is_unexplained_ignore(extension: &str, line: &str) -> bool { true } -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("style").path(path)); + fn skip(path: &Path, is_dir: bool) -> bool { if path.file_name().is_some_and(|name| name.to_string_lossy().starts_with(".#")) { // vim or emacs temporary file @@ -391,7 +394,7 @@ pub fn check(path: &Path, bad: &mut bool) { }); if contents.is_empty() { - tidy_error!(bad, "{}: empty file", file.display()); + check.error(format!("{}: empty file", file.display())); } let extension = file.extension().unwrap().to_string_lossy(); @@ -467,7 +470,7 @@ pub fn check(path: &Path, bad: &mut bool) { } let mut err = |msg: &str| { - tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg); + check.error(format!("{}:{}: {msg}", file.display(), i + 1)); }; if trimmed.contains("dbg!") @@ -611,7 +614,7 @@ pub fn check(path: &Path, bad: &mut bool) { && backtick_count % 2 == 1 { let mut err = |msg: &str| { - tidy_error!(bad, "{}:{start_line}: {msg}", file.display()); + check.error(format!("{}:{start_line}: {msg}", file.display())); }; let block_len = (i + 1) - start_line; if block_len == 1 { @@ -632,12 +635,12 @@ pub fn check(path: &Path, bad: &mut bool) { } if leading_new_lines { let mut err = |_| { - tidy_error!(bad, "{}: leading newline", file.display()); + check.error(format!("{}: leading newline", file.display())); }; suppressible_tidy_err!(err, skip_leading_newlines, "missing leading newline"); } let mut err = |msg: &str| { - tidy_error!(bad, "{}: {}", file.display(), msg); + check.error(format!("{}: {}", file.display(), msg)); }; match trailing_new_lines { 0 => suppressible_tidy_err!(err, skip_trailing_newlines, "missing trailing newline"), @@ -650,38 +653,36 @@ pub fn check(path: &Path, bad: &mut bool) { }; if lines > LINES { let mut err = |_| { - tidy_error!( - bad, - "{}: too many lines ({}) (add `// \ + check.error(format!( + "{}: too many lines ({lines}) (add `// \ ignore-tidy-filelength` to the file to suppress this error)", file.display(), - lines - ); + )); }; suppressible_tidy_err!(err, skip_file_length, ""); } if let Directive::Ignore(false) = skip_cr { - tidy_error!(bad, "{}: ignoring CR characters unnecessarily", file.display()); + check.error(format!("{}: ignoring CR characters unnecessarily", file.display())); } if let Directive::Ignore(false) = skip_tab { - tidy_error!(bad, "{}: ignoring tab characters unnecessarily", file.display()); + check.error(format!("{}: ignoring tab characters unnecessarily", file.display())); } if let Directive::Ignore(false) = skip_end_whitespace { - tidy_error!(bad, "{}: ignoring trailing whitespace unnecessarily", file.display()); + check.error(format!("{}: ignoring trailing whitespace unnecessarily", file.display())); } if let Directive::Ignore(false) = skip_trailing_newlines { - tidy_error!(bad, "{}: ignoring trailing newlines unnecessarily", file.display()); + check.error(format!("{}: ignoring trailing newlines unnecessarily", file.display())); } if let Directive::Ignore(false) = skip_leading_newlines { - tidy_error!(bad, "{}: ignoring leading newlines unnecessarily", file.display()); + check.error(format!("{}: ignoring leading newlines unnecessarily", file.display())); } if let Directive::Ignore(false) = skip_copyright { - tidy_error!(bad, "{}: ignoring copyright unnecessarily", file.display()); + check.error(format!("{}: ignoring copyright unnecessarily", file.display())); } // We deliberately do not warn about these being unnecessary, // that would just lead to annoying churn. let _unused = skip_line_length; let _unused = skip_file_length; - }) + }); } diff --git a/src/tools/tidy/src/target_policy.rs b/src/tools/tidy/src/target_policy.rs index 550932dbfdc..cfcfcaf2435 100644 --- a/src/tools/tidy/src/target_policy.rs +++ b/src/tools/tidy/src/target_policy.rs @@ -5,6 +5,7 @@ use std::collections::HashSet; use std::path::Path; +use crate::diagnostics::DiagCtx; use crate::walk::{filter_not_rust, walk}; const TARGET_DEFINITIONS_PATH: &str = "compiler/rustc_target/src/spec/targets/"; @@ -23,7 +24,9 @@ const EXCEPTIONS: &[&str] = &[ "xtensa_esp32s3_espidf", ]; -pub fn check(root_path: &Path, bad: &mut bool) { +pub fn check(root_path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check("target_policy"); + let mut targets_to_find = HashSet::new(); let definitions_path = root_path.join(TARGET_DEFINITIONS_PATH); @@ -55,7 +58,7 @@ pub fn check(root_path: &Path, bad: &mut bool) { for target in targets_to_find { if !EXCEPTIONS.contains(&target.as_str()) { - tidy_error!(bad, "{ASSEMBLY_LLVM_TEST_PATH}: missing assembly test for {target}") + check.error(format!("{ASSEMBLY_LLVM_TEST_PATH}: missing assembly test for {target}")); } } } diff --git a/src/tools/tidy/src/target_specific_tests.rs b/src/tools/tidy/src/target_specific_tests.rs index b2d5f259eb2..c1db3874ad5 100644 --- a/src/tools/tidy/src/target_specific_tests.rs +++ b/src/tools/tidy/src/target_specific_tests.rs @@ -4,6 +4,7 @@ use std::collections::BTreeMap; use std::path::Path; +use crate::diagnostics::{CheckId, DiagCtx}; use crate::iter_header::{HeaderLine, iter_header}; use crate::walk::filter_not_rust; @@ -16,7 +17,9 @@ struct RevisionInfo<'a> { llvm_components: Option<Vec<&'a str>>, } -pub fn check(tests_path: &Path, bad: &mut bool) { +pub fn check(tests_path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("target-specific-tests").path(tests_path)); + crate::walk::walk(tests_path, |path, _is_dir| filter_not_rust(path), &mut |entry, content| { if content.contains("// ignore-tidy-target-specific-tests") { return; @@ -44,8 +47,7 @@ pub fn check(tests_path: &Path, bad: &mut bool) { } else if let Some((arch, _)) = v.split_once("-") { info.target_arch.replace(Some(arch)); } else { - eprintln!("{file}: seems to have a malformed --target value"); - *bad = true; + check.error(format!("{file}: seems to have a malformed --target value")); } } }); @@ -62,25 +64,22 @@ pub fn check(tests_path: &Path, bad: &mut bool) { (Some(target_arch), None) => { let llvm_component = target_arch.map_or_else(|| "<arch>".to_string(), arch_to_llvm_component); - eprintln!( + check.error(format!( "{file}: revision {rev} should specify `{LLVM_COMPONENTS_HEADER} {llvm_component}` as it has `--target` set" - ); - *bad = true; + )); } (None, Some(_)) => { - eprintln!( + check.error(format!( "{file}: revision {rev} should not specify `{LLVM_COMPONENTS_HEADER}` as it doesn't need `--target`" - ); - *bad = true; + )); } (Some(target_arch), Some(llvm_components)) => { if let Some(target_arch) = target_arch { let llvm_component = arch_to_llvm_component(target_arch); if !llvm_components.contains(&llvm_component.as_str()) { - eprintln!( + check.error(format!( "{file}: revision {rev} should specify `{LLVM_COMPONENTS_HEADER} {llvm_component}` as it has `--target` set" - ); - *bad = true; + )); } } } diff --git a/src/tools/tidy/src/tests_placement.rs b/src/tools/tidy/src/tests_placement.rs index 9d0057df8bc..8ba8cf552bd 100644 --- a/src/tools/tidy/src/tests_placement.rs +++ b/src/tools/tidy/src/tests_placement.rs @@ -1,15 +1,18 @@ use std::path::Path; +use crate::diagnostics::DiagCtx; + const FORBIDDEN_PATH: &str = "src/test"; const ALLOWED_PATH: &str = "tests"; -pub fn check(root_path: impl AsRef<Path>, bad: &mut bool) { - if root_path.as_ref().join(FORBIDDEN_PATH).exists() { - tidy_error!( - bad, +pub fn check(root_path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check("tests_placement"); + + if root_path.join(FORBIDDEN_PATH).exists() { + check.error(format!( "Tests have been moved, please move them from {} to {}", - root_path.as_ref().join(FORBIDDEN_PATH).display(), - root_path.as_ref().join(ALLOWED_PATH).display() - ) + root_path.join(FORBIDDEN_PATH).display(), + root_path.join(ALLOWED_PATH).display() + )); } } diff --git a/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs b/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs index 02412b6f190..1738088a3a0 100644 --- a/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs +++ b/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs @@ -4,6 +4,7 @@ use std::collections::{BTreeMap, BTreeSet}; use std::ffi::OsStr; use std::path::Path; +use crate::diagnostics::{CheckId, DiagCtx}; use crate::iter_header::*; use crate::walk::*; @@ -21,7 +22,10 @@ const IGNORES: &[&str] = &[ const EXTENSIONS: &[&str] = &["stdout", "stderr"]; const SPECIAL_TEST: &str = "tests/ui/command/need-crate-arg-ignore-tidy.x.rs"; -pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { +pub fn check(tests_path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx + .start_check(CheckId::new("tests_revision_unpaired_stdout_stderr").path(tests_path)); + // Recurse over subdirectories under `tests/` walk_dir(tests_path.as_ref(), filter, &mut |entry| { // We are inspecting a folder. Collect the paths to interesting files `.rs`, `.stderr`, @@ -122,12 +126,11 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { [] | [_] => return, [_, _] if !expected_revisions.is_empty() => { // Found unrevisioned output files for a revisioned test. - tidy_error!( - bad, + check.error(format!( "found unrevisioned output file `{}` for a revisioned test `{}`", sibling.display(), test_path.display(), - ); + )); } [_, _] => return, [_, found_revision, .., extension] => { @@ -138,13 +141,12 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { { // Found some unexpected revision-esque component that is not a known // compare-mode or expected revision. - tidy_error!( - bad, + check.error(format!( "found output file `{}` for unexpected revision `{}` of test `{}`", sibling.display(), found_revision, test_path.display() - ); + )); } } } diff --git a/src/tools/tidy/src/triagebot.rs b/src/tools/tidy/src/triagebot.rs index 6f25ed616fa..41d61dcd141 100644 --- a/src/tools/tidy/src/triagebot.rs +++ b/src/tools/tidy/src/triagebot.rs @@ -4,7 +4,10 @@ use std::path::Path; use toml::Value; -pub fn check(path: &Path, bad: &mut bool) { +use crate::diagnostics::DiagCtx; + +pub fn check(path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check("triagebot"); let triagebot_path = path.join("triagebot.toml"); // This check is mostly to catch broken path filters *within* `triagebot.toml`, and not enforce @@ -30,17 +33,14 @@ pub fn check(path: &Path, bad: &mut bool) { let full_path = path.join(clean_path); if !full_path.exists() { - tidy_error!( - bad, - "triagebot.toml [mentions.*] contains path '{}' which doesn't exist", - clean_path - ); + check.error(format!( + "triagebot.toml [mentions.*] contains path '{clean_path}' which doesn't exist" + )); } } } else { - tidy_error!( - bad, - "triagebot.toml missing [mentions.*] section, this wrong for rust-lang/rust repo." + check.error( + "triagebot.toml missing [mentions.*] section, this wrong for rust-lang/rust repo.", ); } @@ -55,16 +55,13 @@ pub fn check(path: &Path, bad: &mut bool) { let full_path = path.join(clean_path); if !full_path.exists() { - tidy_error!( - bad, - "triagebot.toml [assign.owners] contains path '{}' which doesn't exist", - clean_path - ); + check.error(format!( + "triagebot.toml [assign.owners] contains path '{clean_path}' which doesn't exist" + )); } } } else { - tidy_error!( - bad, + check.error( "triagebot.toml missing [assign.owners] section, this wrong for rust-lang/rust repo." ); } @@ -86,12 +83,9 @@ pub fn check(path: &Path, bad: &mut bool) { // Handle both file and directory paths if !full_path.exists() { - tidy_error!( - bad, - "triagebot.toml [autolabel.{}] contains trigger_files path '{}' which doesn't exist", - label, - file_str - ); + check.error(format!( + "triagebot.toml [autolabel.{label}] contains trigger_files path '{file_str}' which doesn't exist", + )); } } } diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 5bf966b658c..12eca47c171 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -7,13 +7,16 @@ use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; +use crate::diagnostics::{CheckId, DiagCtx, RunningCheck}; + const ISSUES_TXT_HEADER: &str = r#"============================================================ ⚠️⚠️⚠️NOTHING SHOULD EVER BE ADDED TO THIS LIST⚠️⚠️⚠️ ============================================================ "#; -pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { +pub fn check(root_path: &Path, bless: bool, diag_ctx: DiagCtx) { let path = &root_path.join("tests"); + let mut check = diag_ctx.start_check(CheckId::new("ui_tests").path(path)); // the list of files in ui tests that are allowed to start with `issue-XXXX` // BTreeSet because we would like a stable ordering so --bless works @@ -33,16 +36,15 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { .collect(); if !is_sorted && !bless { - tidy_error!( - bad, + check.error( "`src/tools/tidy/src/issues.txt` is not in order, mostly because you modified it manually, please only update it with command `x test tidy --bless`" ); } - deny_new_top_level_ui_tests(bad, &path.join("ui")); + deny_new_top_level_ui_tests(&mut check, &path.join("ui")); - let remaining_issue_names = recursively_check_ui_tests(bad, path, &allowed_issue_names); + let remaining_issue_names = recursively_check_ui_tests(&mut check, path, &allowed_issue_names); // if there are any file names remaining, they were moved on the fs. // our data must remain up to date, so it must be removed from issues.txt @@ -64,16 +66,15 @@ pub fn check(root_path: &Path, bless: bool, bad: &mut bool) { for file_name in remaining_issue_names { let mut p = PathBuf::from(path); p.push(file_name); - tidy_error!( - bad, + check.error(format!( "file `{}` no longer exists and should be removed from the exclusions in `src/tools/tidy/src/issues.txt`", p.display() - ); + )); } } } -fn deny_new_top_level_ui_tests(bad: &mut bool, tests_path: &Path) { +fn deny_new_top_level_ui_tests(check: &mut RunningCheck, tests_path: &Path) { // See <https://github.com/rust-lang/compiler-team/issues/902> where we propose banning adding // new ui tests *directly* under `tests/ui/`. For more context, see: // @@ -93,16 +94,15 @@ fn deny_new_top_level_ui_tests(bad: &mut bool, tests_path: &Path) { }) .filter(|e| !e.file_type().is_dir()); for entry in top_level_ui_tests { - tidy_error!( - bad, + check.error(format!( "ui tests should be added under meaningful subdirectories: `{}`", entry.path().display() - ) + )); } } fn recursively_check_ui_tests<'issues>( - bad: &mut bool, + check: &mut RunningCheck, path: &Path, allowed_issue_names: &'issues BTreeSet<&'issues str>, ) -> BTreeSet<&'issues str> { @@ -113,19 +113,19 @@ fn recursively_check_ui_tests<'issues>( crate::walk::walk_no_read(&paths, |_, _| false, &mut |entry| { let file_path = entry.path(); if let Some(ext) = file_path.extension().and_then(OsStr::to_str) { - check_unexpected_extension(bad, file_path, ext); + check_unexpected_extension(check, file_path, ext); // NB: We do not use file_stem() as some file names have multiple `.`s and we // must strip all of them. let testname = file_path.file_name().unwrap().to_str().unwrap().split_once('.').unwrap().0; if ext == "stderr" || ext == "stdout" || ext == "fixed" { - check_stray_output_snapshot(bad, file_path, testname); - check_empty_output_snapshot(bad, file_path); + check_stray_output_snapshot(check, file_path, testname); + check_empty_output_snapshot(check, file_path); } deny_new_nondescriptive_test_names( - bad, + check, path, &mut remaining_issue_names, file_path, @@ -137,7 +137,7 @@ fn recursively_check_ui_tests<'issues>( remaining_issue_names } -fn check_unexpected_extension(bad: &mut bool, file_path: &Path, ext: &str) { +fn check_unexpected_extension(check: &mut RunningCheck, file_path: &Path, ext: &str) { const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[ "rs", // test source files "stderr", // expected stderr file, corresponds to a rs file @@ -178,11 +178,11 @@ fn check_unexpected_extension(bad: &mut bool, file_path: &Path, ext: &str) { if !(EXPECTED_TEST_FILE_EXTENSIONS.contains(&ext) || EXTENSION_EXCEPTION_PATHS.iter().any(|path| file_path.ends_with(path))) { - tidy_error!(bad, "file {} has unexpected extension {}", file_path.display(), ext); + check.error(format!("file {} has unexpected extension {}", file_path.display(), ext)); } } -fn check_stray_output_snapshot(bad: &mut bool, file_path: &Path, testname: &str) { +fn check_stray_output_snapshot(check: &mut RunningCheck, file_path: &Path, testname: &str) { // Test output filenames have one of the formats: // ``` // $testname.stderr @@ -197,20 +197,20 @@ fn check_stray_output_snapshot(bad: &mut bool, file_path: &Path, testname: &str) if !file_path.with_file_name(testname).with_extension("rs").exists() && !testname.contains("ignore-tidy") { - tidy_error!(bad, "Stray file with UI testing output: {:?}", file_path); + check.error(format!("Stray file with UI testing output: {:?}", file_path)); } } -fn check_empty_output_snapshot(bad: &mut bool, file_path: &Path) { +fn check_empty_output_snapshot(check: &mut RunningCheck, file_path: &Path) { if let Ok(metadata) = fs::metadata(file_path) && metadata.len() == 0 { - tidy_error!(bad, "Empty file with UI testing output: {:?}", file_path); + check.error(format!("Empty file with UI testing output: {:?}", file_path)); } } fn deny_new_nondescriptive_test_names( - bad: &mut bool, + check: &mut RunningCheck, path: &Path, remaining_issue_names: &mut BTreeSet<&str>, file_path: &Path, @@ -231,11 +231,10 @@ fn deny_new_nondescriptive_test_names( if !remaining_issue_names.remove(stripped_path.as_str()) && !stripped_path.starts_with("ui/issues/") { - tidy_error!( - bad, + check.error(format!( "file `tests/{stripped_path}` must begin with a descriptive name, consider `{{reason}}-issue-{issue_n}.rs`", issue_n = &test_name[1], - ); + )); } } } diff --git a/src/tools/tidy/src/unit_tests.rs b/src/tools/tidy/src/unit_tests.rs index 7396310ed37..cab445ac63a 100644 --- a/src/tools/tidy/src/unit_tests.rs +++ b/src/tools/tidy/src/unit_tests.rs @@ -11,9 +11,12 @@ use std::path::Path; +use crate::diagnostics::{CheckId, DiagCtx}; use crate::walk::{filter_dirs, walk}; -pub fn check(root_path: &Path, stdlib: bool, bad: &mut bool) { +pub fn check(root_path: &Path, stdlib: bool, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("unit_tests").path(root_path)); + let skip = move |path: &Path, is_dir| { let file_name = path.file_name().unwrap_or_default(); @@ -92,14 +95,11 @@ pub fn check(root_path: &Path, stdlib: bool, bad: &mut bool) { .to_owned() }; let name = if is_test() { "test" } else { "bench" }; - tidy_error!( - bad, - "`{}:{}` contains `#[{}]`; {}", + check.error(format!( + "`{}:{}` contains `#[{name}]`; {explanation}", path.display(), i + 1, - name, - explanation, - ); + )); return; } } diff --git a/src/tools/tidy/src/unknown_revision.rs b/src/tools/tidy/src/unknown_revision.rs index 0ba05c80a79..776d45e25de 100644 --- a/src/tools/tidy/src/unknown_revision.rs +++ b/src/tools/tidy/src/unknown_revision.rs @@ -12,12 +12,14 @@ use std::sync::OnceLock; use ignore::DirEntry; use regex::Regex; +use crate::diagnostics::{CheckId, DiagCtx, RunningCheck}; use crate::iter_header::{HeaderLine, iter_header}; use crate::walk::{filter_dirs, filter_not_rust, walk}; -pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { +pub fn check(tests_path: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("unknown_revision").path(tests_path)); walk( - tests_path.as_ref(), + tests_path, |path, is_dir| { filter_dirs(path) || filter_not_rust(path) || { // Auxiliary source files for incremental tests can refer to revisions @@ -25,11 +27,11 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) { is_dir && path.file_name().is_some_and(|name| name == "auxiliary") } }, - &mut |entry, contents| visit_test_file(entry, contents, bad), + &mut |entry, contents| visit_test_file(entry, contents, &mut check), ); } -fn visit_test_file(entry: &DirEntry, contents: &str, bad: &mut bool) { +fn visit_test_file(entry: &DirEntry, contents: &str, check: &mut RunningCheck) { let mut revisions = HashSet::new(); let mut unused_revision_names = HashSet::new(); @@ -68,10 +70,9 @@ fn visit_test_file(entry: &DirEntry, contents: &str, bad: &mut bool) { // Fail if any revision names appear in both places, since that's probably a mistake. for rev in revisions.intersection(&unused_revision_names).copied().collect::<BTreeSet<_>>() { - tidy_error!( - bad, + check.error(format!( "revision name [{rev}] appears in both `revisions` and `unused-revision-names` in {path}" - ); + )); } // Compute the set of revisions that were mentioned but not declared, @@ -84,7 +85,7 @@ fn visit_test_file(entry: &DirEntry, contents: &str, bad: &mut bool) { bad_revisions.sort(); for (line_number, rev) in bad_revisions { - tidy_error!(bad, "unknown revision [{rev}] at {path}:{line_number}"); + check.error(format!("unknown revision [{rev}] at {path}:{line_number}")); } } diff --git a/src/tools/tidy/src/unstable_book.rs b/src/tools/tidy/src/unstable_book.rs index 0ed954d48de..bab294abee0 100644 --- a/src/tools/tidy/src/unstable_book.rs +++ b/src/tools/tidy/src/unstable_book.rs @@ -2,6 +2,7 @@ use std::collections::BTreeSet; use std::fs; use std::path::{Path, PathBuf}; +use crate::diagnostics::{DiagCtx, RunningCheck}; use crate::features::{CollectedFeatures, Features, Status}; pub const PATH_STR: &str = "doc/unstable-book"; @@ -75,19 +76,18 @@ fn collect_unstable_book_lib_features_section_file_names(base_src_path: &Path) - } /// Would switching underscores for dashes work? -fn maybe_suggest_dashes(names: &BTreeSet<String>, feature_name: &str, bad: &mut bool) { +fn maybe_suggest_dashes(names: &BTreeSet<String>, feature_name: &str, check: &mut RunningCheck) { let with_dashes = feature_name.replace('_', "-"); if names.contains(&with_dashes) { - tidy_error!( - bad, - "the file `{}.md` contains underscores; use dashes instead: `{}.md`", - feature_name, - with_dashes, - ); + check.error(format!( + "the file `{feature_name}.md` contains underscores; use dashes instead: `{with_dashes}.md`", + )); } } -pub fn check(path: &Path, features: CollectedFeatures, bad: &mut bool) { +pub fn check(path: &Path, features: CollectedFeatures, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check("unstable_book"); + let lang_features = features.lang; let lib_features = features .lib @@ -108,26 +108,22 @@ pub fn check(path: &Path, features: CollectedFeatures, bad: &mut bool) { // Check for Unstable Book sections that don't have a corresponding unstable feature for feature_name in &unstable_book_lib_features_section_file_names - &unstable_lib_feature_names { - tidy_error!( - bad, - "The Unstable Book has a 'library feature' section '{}' which doesn't \ - correspond to an unstable library feature", - feature_name - ); - maybe_suggest_dashes(&unstable_lib_feature_names, &feature_name, bad); + check.error(format!( + "The Unstable Book has a 'library feature' section '{feature_name}' which doesn't \ + correspond to an unstable library feature" + )); + maybe_suggest_dashes(&unstable_lib_feature_names, &feature_name, &mut check); } // Check for Unstable Book sections that don't have a corresponding unstable feature. for feature_name in &unstable_book_lang_features_section_file_names - &unstable_lang_feature_names { - tidy_error!( - bad, - "The Unstable Book has a 'language feature' section '{}' which doesn't \ - correspond to an unstable language feature", - feature_name - ); - maybe_suggest_dashes(&unstable_lang_feature_names, &feature_name, bad); + check.error(format!( + "The Unstable Book has a 'language feature' section '{feature_name}' which doesn't \ + correspond to an unstable language feature" + )); + maybe_suggest_dashes(&unstable_lang_feature_names, &feature_name, &mut check); } // List unstable features that don't have Unstable Book sections. diff --git a/src/tools/tidy/src/x_version.rs b/src/tools/tidy/src/x_version.rs index 9f7f43c4000..b3e322d9403 100644 --- a/src/tools/tidy/src/x_version.rs +++ b/src/tools/tidy/src/x_version.rs @@ -3,12 +3,18 @@ use std::process::{Command, Stdio}; use semver::Version; -pub fn check(root: &Path, cargo: &Path, bad: &mut bool) { +use crate::diagnostics::{CheckId, DiagCtx}; + +pub fn check(root: &Path, cargo: &Path, diag_ctx: DiagCtx) { + let mut check = diag_ctx.start_check(CheckId::new("x_version").path(root)); let cargo_list = Command::new(cargo).args(["install", "--list"]).stdout(Stdio::piped()).spawn(); let child = match cargo_list { Ok(child) => child, - Err(e) => return tidy_error!(bad, "failed to run `cargo`: {}", e), + Err(e) => { + check.error(format!("failed to run `cargo`: {e}")); + return; + } }; let cargo_list = child.wait_with_output().unwrap(); @@ -47,13 +53,10 @@ pub fn check(root: &Path, cargo: &Path, bad: &mut bool) { ) } } else { - tidy_error!( - bad, - "Unable to parse the latest version of `x` at `src/tools/x/Cargo.toml`" - ) + check.error("Unable to parse the latest version of `x` at `src/tools/x/Cargo.toml`") } } else { - tidy_error!(bad, "failed to check version of `x`: {}", cargo_list.status) + check.error(format!("failed to check version of `x`: {}", cargo_list.status)) } } diff --git a/src/tools/unstable-book-gen/src/main.rs b/src/tools/unstable-book-gen/src/main.rs index a7c6173d88c..16550f83003 100644 --- a/src/tools/unstable-book-gen/src/main.rs +++ b/src/tools/unstable-book-gen/src/main.rs @@ -5,6 +5,7 @@ use std::env; use std::fs::{self, write}; use std::path::Path; +use tidy::diagnostics::RunningCheck; use tidy::features::{Features, collect_env_vars, collect_lang_features, collect_lib_features}; use tidy::t; use tidy::unstable_book::{ @@ -122,7 +123,7 @@ fn main() { let src_path = Path::new(&src_path_str); let dest_path = Path::new(&dest_path_str); - let lang_features = collect_lang_features(compiler_path, &mut false); + let lang_features = collect_lang_features(compiler_path, &mut RunningCheck::new_noop()); let lib_features = collect_lib_features(library_path) .into_iter() .filter(|&(ref name, _)| !lang_features.contains_key(name)) |