user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Add diagnostics context and migrate the style check to it

Browse source
This commit is contained in:
Jakub Beránek 2025-09-15 13:16:25 +02:00
parent 1d23da6b73
commit b0010dd5ae
No known key found for this signature in database
GPG key ID: 909CD0D26483516B
4 changed files with 187 additions and 89 deletions
Download patch file Download diff file Expand all files Collapse all files

102
src/tools/tidy/src/diagnostics.rs Normal file
View file

@ -0,0 +1,102 @@
use std::collections::HashSet;
use std::fmt::Display;
use std::sync::{Arc, Mutex};
use crate::tidy_error;
/// 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(verbose: bool) -> Self {
Self(Arc::new(Mutex::new(DiagCtxInner {
running_checks: Default::default(),
finished_checks: Default::default(),
verbose,
})))
}
pub fn start_check<T: Display>(&self, name: T) -> RunningCheck {
let name = name.to_string();
let mut ctx = self.0.lock().unwrap();
ctx.start_check(&name);
RunningCheck { name, bad: false, ctx: self.0.clone() }
}
pub fn into_conclusion(self) -> bool {
let ctx = self.0.lock().unwrap();
assert!(ctx.running_checks.is_empty(), "Some checks are still running");
ctx.finished_checks.iter().any(|c| c.bad)
}
}
struct DiagCtxInner {
running_checks: HashSet<String>,
finished_checks: HashSet<FinishedCheck>,
verbose: bool,
}
impl DiagCtxInner {
fn start_check(&mut self, name: &str) {
if self.has_check(name) {
panic!("Starting a check named {name} for the second time");
}
self.running_checks.insert(name.to_string());
}
fn finish_check(&mut self, check: FinishedCheck) {
assert!(
self.running_checks.remove(&check.name),
"Finishing check {} that was not started",
check.name
);
self.finished_checks.insert(check);
}
fn has_check(&self, name: &str) -> bool {
self.running_checks
.iter()
.chain(self.finished_checks.iter().map(|c| &c.name))
.any(|c| c == name)
}
}
#[derive(PartialEq, Eq, Hash, Debug)]
struct FinishedCheck {
name: String,
bad: bool,
}
/// Represents a single tidy check, identified by its `name`, running.
pub struct RunningCheck {
name: String,
bad: bool,
ctx: Arc<Mutex<DiagCtxInner>>,
}
impl RunningCheck {
/// Immediately output an error and mark the check as failed.
pub fn error<T: Display>(&mut self, t: T) {
self.mark_as_bad();
tidy_error(&t.to_string()).expect("failed to output error");
}
fn mark_as_bad(&mut self) {
self.bad = true;
}
}
impl Drop for RunningCheck {
fn drop(&mut self) {
self.ctx
.lock()
.unwrap()
.finish_check(FinishedCheck { name: self.name.clone(), bad: self.bad })
}
}

8
src/tools/tidy/src/lib.rs
View file

@ -50,13 +50,6 @@ macro_rules! tidy_error {
});
}
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;
@ -250,6 +243,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;

129
src/tools/tidy/src/main.rs
View file

@ -9,9 +9,11 @@ use std::num::NonZeroUsize;
use std::path::PathBuf;
use std::str::FromStr;
use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::{Arc, Mutex};
use std::thread::{self, ScopedJoinHandle, scope};
use std::{env, process};
use tidy::diagnostics::DiagCtx;
use tidy::*;
fn main() {
@ -54,6 +56,8 @@ fn main() {
let ci_info = CiInfo::new(&mut bad);
let bad = std::sync::Arc::new(AtomicBool::new(bad));
let mut diag_ctx = DiagCtx::new(verbose);
let drain_handles = |handles: &mut VecDeque<ScopedJoinHandle<'_, ()>>| {
// poll all threads for completion before awaiting the oldest one
for i in (0..handles.len()).rev() {
@ -87,76 +91,73 @@ 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);
}
}
check!(target_specific_tests, &tests_path);
// check!(target_specific_tests, &tests_path);
// Checks that are done on the cargo workspace.
check!(deps, &root_path, &cargo, bless);
check!(extdeps, &root_path);
// check!(deps, &root_path, &cargo, bless);
// check!(extdeps, &root_path);
// Checks over tests.
check!(tests_placement, &root_path);
check!(tests_revision_unpaired_stdout_stderr, &tests_path);
check!(debug_artifacts, &tests_path);
check!(ui_tests, &root_path, bless);
check!(mir_opt_tests, &tests_path, bless);
check!(rustdoc_gui_tests, &tests_path);
check!(rustdoc_css_themes, &librustdoc_path);
check!(rustdoc_templates, &librustdoc_path);
check!(rustdoc_json, &src_path, &ci_info);
check!(known_bug, &crashes_path);
check!(unknown_revision, &tests_path);
// check!(tests_placement, &root_path);
// check!(tests_revision_unpaired_stdout_stderr, &tests_path);
// check!(debug_artifacts, &tests_path);
// check!(ui_tests, &root_path, bless);
// check!(mir_opt_tests, &tests_path, bless);
// check!(rustdoc_gui_tests, &tests_path);
// check!(rustdoc_css_themes, &librustdoc_path);
// check!(rustdoc_templates, &librustdoc_path);
// check!(rustdoc_json, &src_path, &ci_info);
// check!(known_bug, &crashes_path);
// 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!(fluent_alphabetical, &compiler_path, bless);
check!(fluent_period, &compiler_path);
check!(fluent_lowercase, &compiler_path);
check!(target_policy, &root_path);
check!(gcc_submodule, &root_path, &compiler_path);
// check!(error_codes, &root_path, &[&compiler_path, &librustdoc_path], verbose, &ci_info);
// check!(fluent_alphabetical, &compiler_path, bless);
// check!(fluent_period, &compiler_path);
// check!(fluent_lowercase, &compiler_path);
// check!(target_policy, &root_path);
// check!(gcc_submodule, &root_path, &compiler_path);
// Checks that only make sense for the std libs.
check!(pal, &library_path);
// check!(pal, &library_path);
// Checks that need to be done for both the compiler and std libraries.
check!(unit_tests, &src_path, false);
check!(unit_tests, &compiler_path, false);
check!(unit_tests, &library_path, true);
if bins::check_filesystem_support(&[&root_path], &output_directory) {
check!(bins, &root_path);
}
// check!(unit_tests, &src_path, false);
// check!(unit_tests, &compiler_path, false);
// check!(unit_tests, &library_path, true);
//
// if bins::check_filesystem_support(&[&root_path], &output_directory) {
// check!(bins, &root_path);
// }
check!(style, &src_path);
check!(style, &tests_path);
check!(style, &compiler_path);
check!(style, &library_path);
check!(edition, &src_path);
check!(edition, &compiler_path);
check!(edition, &library_path);
check!(alphabetical, &root_manifest);
check!(alphabetical, &src_path);
check!(alphabetical, &tests_path);
check!(alphabetical, &compiler_path);
check!(alphabetical, &library_path);
check!(x_version, &root_path, &cargo);
check!(triagebot, &root_path);
check!(filenames, &root_path);
// check!(edition, &src_path);
// check!(edition, &compiler_path);
// check!(edition, &library_path);
//
// check!(alphabetical, &root_manifest);
// check!(alphabetical, &src_path);
// check!(alphabetical, &tests_path);
// check!(alphabetical, &compiler_path);
// check!(alphabetical, &library_path);
//
// check!(x_version, &root_path, &cargo);
//
// check!(triagebot, &root_path);
//
// check!(filenames, &root_path);
let collected = {
drain_handles(&mut handles);
@ -175,24 +176,24 @@ fn main() {
}
r
};
check!(unstable_book, &src_path, collected);
check!(
extra_checks,
&root_path,
&output_directory,
&ci_info,
&librustdoc_path,
&tools_path,
&npm,
&cargo,
bless,
extra_checks,
pos_args
);
// check!(unstable_book, &src_path, collected);
//
// check!(
// extra_checks,
// &root_path,
// &output_directory,
// &ci_info,
// &librustdoc_path,
// &tools_path,
// &npm,
// &cargo,
// bless,
// extra_checks,
// pos_args
// );
});
if bad.load(Ordering::Relaxed) {
if diag_ctx.into_conclusion() {
eprintln!("some tidy checks failed");
process::exit(1);
}

37
src/tools/tidy/src/style.rs
View file

@ -24,6 +24,7 @@ use std::sync::LazyLock;
use regex::RegexSetBuilder;
use rustc_hash::FxHashMap;
use crate::diagnostics::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(format!("style {}", path.display()));
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;
})
});
}