From d99800027c88c4928e6329c9e6a42aac2262e452 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 4 Jan 2023 02:16:24 +0000 Subject: [PATCH 1/5] Ignore things in .gitignore in tidy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Switch from `walkdir` to `ignore`. This required various changes to make `skip` thread-safe. - Ignore `build` anywhere in the source tree, not just at the top-level. We support this in bootstrap, we should support it in tidy too. As a nice side benefit, this also makes tidy a bit faster. Before: ``` ; hyperfine -i '"/home/gh-jyn514/rust2/build/aarch64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/home/gh-jyn514/rust2" "/home/gh-jyn514/rust2/build/aarch64-unknown-linux-gnu/stage0/bin/cargo" "/home/gh-jyn514/rust2/build" "32"' Benchmark 1: "/home/gh-jyn514/rust2/build/aarch64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/home/gh-jyn514/rust2" "/home/gh-jyn514/rust2/build/aarch64-unknown-linux-gnu/stage0/bin/cargo" "/home/gh-jyn514/rust2/build" "32" Time (mean ± σ): 1.080 s ± 0.008 s [User: 2.616 s, System: 3.243 s] Range (min … max): 1.069 s … 1.099 s 10 runs ``` After: ``` ; hyperfine '"/home/gh-jyn514/rust2/build/aarch64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/home/gh-jyn514/rust2" "/home/gh-jyn514/rust2/build/aarch64-unknown-linux-gnu/stage0/bin/cargo" "/home/gh-jyn514/rust2/build" "32"' Benchmark 1: "/home/gh-jyn514/rust2/build/aarch64-unknown-linux-gnu/stage0-tools-bin/rust-tidy" "/home/gh-jyn514/rust2" "/home/gh-jyn514/rust2/build/aarch64-unknown-linux-gnu/stage0/bin/cargo" "/home/gh-jyn514/rust2/build" "32" Time (mean ± σ): 705.0 ms ± 1.4 ms [User: 3179.1 ms, System: 1517.5 ms] Range (min … max): 702.3 ms … 706.9 ms 10 runs ``` --- .gitignore | 2 +- src/tools/tidy/src/alphabetical.rs | 2 +- src/tools/tidy/src/bins.rs | 2 +- src/tools/tidy/src/debug_artifacts.rs | 2 +- src/tools/tidy/src/edition.rs | 34 ++++----- src/tools/tidy/src/error_codes_check.rs | 2 +- src/tools/tidy/src/errors.rs | 80 ++++++++++----------- src/tools/tidy/src/features.rs | 6 +- src/tools/tidy/src/pal.rs | 2 +- src/tools/tidy/src/style.rs | 2 +- src/tools/tidy/src/target_specific_tests.rs | 2 +- src/tools/tidy/src/ui_tests.rs | 2 +- src/tools/tidy/src/unit_tests.rs | 18 ++--- src/tools/tidy/src/walk.rs | 38 +++++----- 14 files changed, 96 insertions(+), 98 deletions(-) diff --git a/.gitignore b/.gitignore index 37972532b3614..33167e03044f9 100644 --- a/.gitignore +++ b/.gitignore @@ -41,7 +41,7 @@ no_llvm_build /inst/ /llvm/ /mingw-build/ -/build/ +build/ /build-rust-analyzer/ /dist/ /unicode-downloads diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs index f913f6cde3889..9bfee1efc0b2e 100644 --- a/src/tools/tidy/src/alphabetical.rs +++ b/src/tools/tidy/src/alphabetical.rs @@ -95,7 +95,7 @@ fn check_section<'a>( } pub fn check(path: &Path, bad: &mut bool) { - walk(path, &mut filter_dirs, &mut |entry, contents| { + walk(path, filter_dirs, &mut |entry, contents| { let file = &entry.path().display(); let mut lines = contents.lines().enumerate().peekable(); diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs index b898f20a5d018..335d8eb914386 100644 --- a/src/tools/tidy/src/bins.rs +++ b/src/tools/tidy/src/bins.rs @@ -103,7 +103,7 @@ mod os_impl { walk_no_read( path, - &mut |path| { + |path| { filter_dirs(path) || path.ends_with("src/etc") // This is a list of directories that we almost certainly diff --git a/src/tools/tidy/src/debug_artifacts.rs b/src/tools/tidy/src/debug_artifacts.rs index 9880a32ad0c28..2b06015d93502 100644 --- a/src/tools/tidy/src/debug_artifacts.rs +++ b/src/tools/tidy/src/debug_artifacts.rs @@ -8,7 +8,7 @@ const GRAPHVIZ_POSTFLOW_MSG: &str = "`borrowck_graphviz_postflow` attribute in t pub fn check(path: &Path, bad: &mut bool) { let test_dir: PathBuf = path.join("test"); - walk(&test_dir, &mut filter_dirs, &mut |entry, contents| { + walk(&test_dir, filter_dirs, &mut |entry, contents| { let filename = entry.path(); let is_rust = filename.extension().map_or(false, |ext| ext == "rs"); if !is_rust { diff --git a/src/tools/tidy/src/edition.rs b/src/tools/tidy/src/edition.rs index 8a7c4460dc7ea..57fad91a4da58 100644 --- a/src/tools/tidy/src/edition.rs +++ b/src/tools/tidy/src/edition.rs @@ -9,24 +9,20 @@ fn is_edition_2021(mut line: &str) -> bool { } pub fn check(path: &Path, bad: &mut bool) { - walk( - path, - &mut |path| filter_dirs(path) || path.ends_with("src/test"), - &mut |entry, contents| { - let file = entry.path(); - let filename = file.file_name().unwrap(); - if filename != "Cargo.toml" { - return; - } + walk(path, |path| filter_dirs(path) || path.ends_with("src/test"), &mut |entry, contents| { + let file = entry.path(); + let filename = file.file_name().unwrap(); + if filename != "Cargo.toml" { + return; + } - let is_2021 = contents.lines().any(is_edition_2021); - if !is_2021 { - tidy_error!( - bad, - "{} doesn't have `edition = \"2021\"` on a separate line", - file.display() - ); - } - }, - ); + let is_2021 = contents.lines().any(is_edition_2021); + if !is_2021 { + tidy_error!( + bad, + "{} doesn't have `edition = \"2021\"` on a separate line", + file.display() + ); + } + }); } diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index 3f060e437aca7..b776c746ad877 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -213,7 +213,7 @@ pub fn check(paths: &[&Path], bad: &mut bool) { let regex = Regex::new(r#"[(,"\s](E\d{4})[,)"]"#).unwrap(); for path in paths { - walk(path, &mut filter_dirs, &mut |entry, contents| { + walk(path, filter_dirs, &mut |entry, contents| { let file_name = entry.file_name(); let entry_path = entry.path(); diff --git a/src/tools/tidy/src/errors.rs b/src/tools/tidy/src/errors.rs index fe5fd72b91a49..6881e9f6c6cbd 100644 --- a/src/tools/tidy/src/errors.rs +++ b/src/tools/tidy/src/errors.rs @@ -9,51 +9,47 @@ use std::path::Path; pub fn check(path: &Path, bad: &mut bool) { let mut map: HashMap<_, Vec<_>> = HashMap::new(); - walk( - path, - &mut |path| filter_dirs(path) || path.ends_with("src/test"), - &mut |entry, contents| { - let file = entry.path(); - let filename = file.file_name().unwrap().to_string_lossy(); - if filename != "error_codes.rs" { - return; - } - - // In the `register_long_diagnostics!` macro, entries look like this: - // - // ``` - // EXXXX: r##" - // - // "##, - // ``` - // - // and these long messages often have error codes themselves inside - // them, but we don't want to report duplicates in these cases. This - // variable keeps track of whether we're currently inside one of these - // long diagnostic messages. - let mut inside_long_diag = false; - for (num, line) in contents.lines().enumerate() { - if inside_long_diag { - inside_long_diag = !line.contains("\"##"); - continue; - } + walk(path, |path| filter_dirs(path) || path.ends_with("src/test"), &mut |entry, contents| { + let file = entry.path(); + let filename = file.file_name().unwrap().to_string_lossy(); + if filename != "error_codes.rs" { + return; + } - let mut search = line; - while let Some(i) = search.find('E') { - search = &search[i + 1..]; - let code = if search.len() > 4 { search[..4].parse::() } else { continue }; - let code = match code { - Ok(n) => n, - Err(..) => continue, - }; - map.entry(code).or_default().push((file.to_owned(), num + 1, line.to_owned())); - break; - } + // In the `register_long_diagnostics!` macro, entries look like this: + // + // ``` + // EXXXX: r##" + // + // "##, + // ``` + // + // and these long messages often have error codes themselves inside + // them, but we don't want to report duplicates in these cases. This + // variable keeps track of whether we're currently inside one of these + // long diagnostic messages. + let mut inside_long_diag = false; + for (num, line) in contents.lines().enumerate() { + if inside_long_diag { + inside_long_diag = !line.contains("\"##"); + continue; + } - inside_long_diag = line.contains("r##\""); + let mut search = line; + while let Some(i) = search.find('E') { + search = &search[i + 1..]; + let code = if search.len() > 4 { search[..4].parse::() } else { continue }; + let code = match code { + Ok(n) => n, + Err(..) => continue, + }; + map.entry(code).or_default().push((file.to_owned(), num + 1, line.to_owned())); + break; } - }, - ); + + inside_long_diag = line.contains("r##\""); + } + }); let mut max = 0; for (&code, entries) in map.iter() { diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index f10ecf5f201e3..6ff0f987dc6ed 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -100,7 +100,7 @@ pub fn check( &src_path.join("test/rustdoc-ui"), &src_path.join("test/rustdoc"), ], - &mut filter_dirs, + filter_dirs, &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); @@ -476,11 +476,11 @@ fn get_and_check_lib_features( fn map_lib_features( base_src_path: &Path, - mf: &mut dyn FnMut(Result<(&str, Feature), &str>, &Path, usize), + mf: &mut (dyn Send + Sync + FnMut(Result<(&str, Feature), &str>, &Path, usize)), ) { walk( base_src_path, - &mut |path| filter_dirs(path) || path.ends_with("src/test"), + |path| filter_dirs(path) || path.ends_with("src/test"), &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); diff --git a/src/tools/tidy/src/pal.rs b/src/tools/tidy/src/pal.rs index f4592fdcff9dc..33938ac9a0a5f 100644 --- a/src/tools/tidy/src/pal.rs +++ b/src/tools/tidy/src/pal.rs @@ -68,7 +68,7 @@ pub fn check(path: &Path, bad: &mut bool) { // Sanity check that the complex parsing here works. let mut saw_target_arch = false; let mut saw_cfg_bang = false; - walk(path, &mut filter_dirs, &mut |entry, contents| { + walk(path, filter_dirs, &mut |entry, contents| { let file = entry.path(); let filestr = file.to_string_lossy().replace("\\", "/"); if !filestr.ends_with(".rs") { diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index f91e38262f64f..0a65e02f2c0a1 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -226,7 +226,7 @@ pub fn check(path: &Path, bad: &mut bool) { .chain(PROBLEMATIC_CONSTS.iter().map(|v| format!("{:X}", v))) .collect(); let problematic_regex = RegexSet::new(problematic_consts_strings.as_slice()).unwrap(); - walk(path, &mut skip, &mut |entry, contents| { + walk(path, skip, &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); let extensions = [".rs", ".py", ".js", ".sh", ".c", ".cpp", ".h", ".md", ".css", ".ftl"]; diff --git a/src/tools/tidy/src/target_specific_tests.rs b/src/tools/tidy/src/target_specific_tests.rs index 8ba25705666a9..462e2e7fb9b24 100644 --- a/src/tools/tidy/src/target_specific_tests.rs +++ b/src/tools/tidy/src/target_specific_tests.rs @@ -38,7 +38,7 @@ pub fn check(path: &Path, bad: &mut bool) { let tests = path.join("test"); crate::walk::walk( &tests, - &mut |path| path.extension().map(|p| p == "rs") == Some(false), + |path| path.extension().map(|p| p == "rs") == Some(false), &mut |entry, content| { let file = entry.path().display(); let mut header_map = BTreeMap::new(); diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 070e72437be80..1e9b416d42054 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -54,7 +54,7 @@ fn check_entries(path: &Path, bad: &mut bool) { pub fn check(path: &Path, bad: &mut bool) { check_entries(&path, bad); for path in &[&path.join("test/ui"), &path.join("test/ui-fulldeps")] { - crate::walk::walk_no_read(path, &mut |_| false, &mut |entry| { + crate::walk::walk_no_read(path, |_| false, &mut |entry| { let file_path = entry.path(); if let Some(ext) = file_path.extension() { if ext == "stderr" || ext == "stdout" { diff --git a/src/tools/tidy/src/unit_tests.rs b/src/tools/tidy/src/unit_tests.rs index 2c23b6ebc75e2..7062cf2279f47 100644 --- a/src/tools/tidy/src/unit_tests.rs +++ b/src/tools/tidy/src/unit_tests.rs @@ -11,14 +11,16 @@ use crate::walk::{filter_dirs, walk}; use std::path::Path; pub fn check(root_path: &Path, bad: &mut bool) { - let core = &root_path.join("core"); - let core_tests = &core.join("tests"); - let core_benches = &core.join("benches"); - let is_core = |path: &Path| { - path.starts_with(core) && !(path.starts_with(core_tests) || path.starts_with(core_benches)) + let core = root_path.join("core"); + let core_copy = core.clone(); + let core_tests = core.join("tests"); + let core_benches = core.join("benches"); + let is_core = move |path: &Path| { + path.starts_with(&core) + && !(path.starts_with(&core_tests) || path.starts_with(&core_benches)) }; - let mut skip = |path: &Path| { + let skip = move |path: &Path| { let file_name = path.file_name().unwrap_or_default(); if path.is_dir() { filter_dirs(path) @@ -35,9 +37,9 @@ pub fn check(root_path: &Path, bad: &mut bool) { } }; - walk(root_path, &mut skip, &mut |entry, contents| { + walk(root_path, skip, &mut |entry, contents| { let path = entry.path(); - let is_core = path.starts_with(core); + let is_core = path.starts_with(&core_copy); for (i, line) in contents.lines().enumerate() { let line = line.trim(); let is_test = || line.contains("#[test]") && !line.contains("`#[test]"); diff --git a/src/tools/tidy/src/walk.rs b/src/tools/tidy/src/walk.rs index 4cfb70fa31c4e..f7f2c647eb49a 100644 --- a/src/tools/tidy/src/walk.rs +++ b/src/tools/tidy/src/walk.rs @@ -1,9 +1,8 @@ -use std::fs::File; -use std::io::Read; -use walkdir::{DirEntry, WalkDir}; +use ignore::DirEntry; -use std::path::Path; +use std::{fs, path::Path}; +/// The default directory filter. pub fn filter_dirs(path: &Path) -> bool { let skip = [ "tidy-test-file", @@ -36,34 +35,39 @@ pub fn filter_dirs(path: &Path) -> bool { pub fn walk_many( paths: &[&Path], - skip: &mut dyn FnMut(&Path) -> bool, + skip: impl Clone + Send + Sync + 'static + Fn(&Path) -> bool, f: &mut dyn FnMut(&DirEntry, &str), ) { for path in paths { - walk(path, skip, f); + walk(path, skip.clone(), f); } } -pub fn walk(path: &Path, skip: &mut dyn FnMut(&Path) -> bool, f: &mut dyn FnMut(&DirEntry, &str)) { - let mut contents = String::new(); +pub fn walk( + path: &Path, + skip: impl Send + Sync + 'static + Fn(&Path) -> bool, + f: &mut dyn FnMut(&DirEntry, &str), +) { walk_no_read(path, skip, &mut |entry| { - contents.clear(); - if t!(File::open(entry.path()), entry.path()).read_to_string(&mut contents).is_err() { - contents.clear(); - } - f(&entry, &contents); + let contents = t!(fs::read(entry.path()), entry.path()); + let contents_str = match String::from_utf8(contents) { + Ok(s) => s, + Err(_) => return, // skip this file + }; + f(&entry, &contents_str); }); } pub(crate) fn walk_no_read( path: &Path, - skip: &mut dyn FnMut(&Path) -> bool, + skip: impl Send + Sync + 'static + Fn(&Path) -> bool, f: &mut dyn FnMut(&DirEntry), ) { - let walker = WalkDir::new(path).into_iter().filter_entry(|e| !skip(e.path())); - for entry in walker { + let mut walker = ignore::WalkBuilder::new(path); + let walker = walker.filter_entry(move |e| !skip(e.path())); + for entry in walker.build() { if let Ok(entry) = entry { - if entry.file_type().is_dir() { + if entry.file_type().map_or(true, |kind| kind.is_dir() || kind.is_symlink()) { continue; } f(&entry); From 270a005c50d7b241d029a1a660ae57c7712e344a Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 4 Jan 2023 03:12:31 +0000 Subject: [PATCH 2/5] tmp --- src/tools/tidy/src/walk.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/tools/tidy/src/walk.rs b/src/tools/tidy/src/walk.rs index f7f2c647eb49a..16edc4aa57aca 100644 --- a/src/tools/tidy/src/walk.rs +++ b/src/tools/tidy/src/walk.rs @@ -1,4 +1,4 @@ -use ignore::DirEntry; +use ignore::{DirEntry, WalkState}; use std::{fs, path::Path}; @@ -36,7 +36,7 @@ pub fn filter_dirs(path: &Path) -> bool { pub fn walk_many( paths: &[&Path], skip: impl Clone + Send + Sync + 'static + Fn(&Path) -> bool, - f: &mut dyn FnMut(&DirEntry, &str), + f: &mut (dyn Send + Sync + Fn(&DirEntry, &str)), ) { for path in paths { walk(path, skip.clone(), f); @@ -46,7 +46,7 @@ pub fn walk_many( pub fn walk( path: &Path, skip: impl Send + Sync + 'static + Fn(&Path) -> bool, - f: &mut dyn FnMut(&DirEntry, &str), + f: &mut (dyn Send + Sync + Fn(&DirEntry, &str)), ) { walk_no_read(path, skip, &mut |entry| { let contents = t!(fs::read(entry.path()), entry.path()); @@ -61,16 +61,18 @@ pub fn walk( pub(crate) fn walk_no_read( path: &Path, skip: impl Send + Sync + 'static + Fn(&Path) -> bool, - f: &mut dyn FnMut(&DirEntry), + f: &mut (dyn Send + Sync + Fn(&DirEntry)), ) { let mut walker = ignore::WalkBuilder::new(path); let walker = walker.filter_entry(move |e| !skip(e.path())); - for entry in walker.build() { + // for entry in walker.build() { + walker.build_parallel().run(|| Box::new(|entry| { if let Ok(entry) = entry { if entry.file_type().map_or(true, |kind| kind.is_dir() || kind.is_symlink()) { - continue; + return WalkState::Continue; } f(&entry); } - } + WalkState::Continue + })); } From ac1b394ed11a224af13e62d845e50e5a5e1fc7f5 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 4 Jan 2023 03:22:44 +0000 Subject: [PATCH 3/5] wip: parallel-tidy --- src/tools/tidy/src/alphabetical.rs | 6 +++--- src/tools/tidy/src/bins.rs | 6 ++++-- src/tools/tidy/src/debug_artifacts.rs | 4 ++-- src/tools/tidy/src/deps.rs | 18 +++++++++--------- src/tools/tidy/src/edition.rs | 4 ++-- src/tools/tidy/src/error_codes_check.rs | 3 ++- src/tools/tidy/src/errors.rs | 5 +++-- src/tools/tidy/src/extdeps.rs | 3 ++- src/tools/tidy/src/features.rs | 11 ++++++----- src/tools/tidy/src/lib.rs | 6 +++--- src/tools/tidy/src/mir_opt_tests.rs | 7 ++++--- src/tools/tidy/src/pal.rs | 5 +++-- src/tools/tidy/src/primitive_docs.rs | 4 ++-- src/tools/tidy/src/style.rs | 4 ++-- src/tools/tidy/src/target_specific_tests.rs | 14 +++++++------- src/tools/tidy/src/ui_tests.rs | 5 +++-- src/tools/tidy/src/unit_tests.rs | 4 ++-- src/tools/tidy/src/unstable_book.rs | 3 ++- 18 files changed, 61 insertions(+), 51 deletions(-) diff --git a/src/tools/tidy/src/alphabetical.rs b/src/tools/tidy/src/alphabetical.rs index 9bfee1efc0b2e..1b97c6e5ec535 100644 --- a/src/tools/tidy/src/alphabetical.rs +++ b/src/tools/tidy/src/alphabetical.rs @@ -17,7 +17,7 @@ //! If a line ends with an opening bracket, the line is ignored and the next line will have //! its extra indentation ignored. -use std::{fmt::Display, path::Path}; +use std::{fmt::Display, path::Path, sync::atomic::AtomicBool}; use crate::walk::{filter_dirs, walk}; @@ -36,7 +36,7 @@ const END_COMMENT: &str = "// tidy-alphabetical-end"; fn check_section<'a>( file: impl Display, lines: impl Iterator, - bad: &mut bool, + bad: &AtomicBool, ) { let content_lines = lines.take_while(|(_, line)| !line.contains(END_COMMENT)); @@ -94,7 +94,7 @@ fn check_section<'a>( } } -pub fn check(path: &Path, bad: &mut bool) { +pub fn check(path: &Path, bad: &AtomicBool) { walk(path, filter_dirs, &mut |entry, contents| { let file = &entry.path().display(); diff --git a/src/tools/tidy/src/bins.rs b/src/tools/tidy/src/bins.rs index 335d8eb914386..1e06bbfe473f4 100644 --- a/src/tools/tidy/src/bins.rs +++ b/src/tools/tidy/src/bins.rs @@ -11,12 +11,13 @@ pub use os_impl::*; #[cfg(windows)] mod os_impl { use std::path::Path; + use std::sync::atomic::AtomicBool; 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, _bad: &AtomicBool) {} } #[cfg(unix)] @@ -26,6 +27,7 @@ mod os_impl { use std::os::unix::prelude::*; use std::path::Path; use std::process::{Command, Stdio}; + use std::sync::atomic::AtomicBool; enum FilesystemSupport { Supported, @@ -96,7 +98,7 @@ mod os_impl { } #[cfg(unix)] - pub fn check(path: &Path, bad: &mut bool) { + pub fn check(path: &Path, bad: &AtomicBool) { use std::ffi::OsStr; const ALLOWED: &[&str] = &["configure", "x"]; diff --git a/src/tools/tidy/src/debug_artifacts.rs b/src/tools/tidy/src/debug_artifacts.rs index 2b06015d93502..7b1a19a718b39 100644 --- a/src/tools/tidy/src/debug_artifacts.rs +++ b/src/tools/tidy/src/debug_artifacts.rs @@ -1,11 +1,11 @@ //! Tidy check to prevent creation of unnecessary debug artifacts while running tests. use crate::walk::{filter_dirs, walk}; -use std::path::{Path, PathBuf}; +use std::{path::{Path, PathBuf}, sync::atomic::AtomicBool}; const GRAPHVIZ_POSTFLOW_MSG: &str = "`borrowck_graphviz_postflow` attribute in test"; -pub fn check(path: &Path, bad: &mut bool) { +pub fn check(path: &Path, bad: &AtomicBool) { let test_dir: PathBuf = path.join("test"); walk(&test_dir, filter_dirs, &mut |entry, contents| { diff --git a/src/tools/tidy/src/deps.rs b/src/tools/tidy/src/deps.rs index 75454cbdc5fe6..c72be0c7108fb 100644 --- a/src/tools/tidy/src/deps.rs +++ b/src/tools/tidy/src/deps.rs @@ -3,6 +3,7 @@ use cargo_metadata::{Metadata, Package, PackageId, Resolve}; use std::collections::{BTreeSet, HashSet}; use std::path::Path; +use std::sync::atomic::AtomicBool; /// These are licenses that are allowed for all crates, including the runtime, /// rustc, tools, etc. @@ -346,7 +347,7 @@ const FORBIDDEN_TO_HAVE_DUPLICATES: &[&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, bad: &mut bool) { +pub fn check(root: &Path, cargo: &Path, bad: &AtomicBool) { let mut cmd = cargo_metadata::MetadataCommand::new(); cmd.cargo_path(cargo) .manifest_path(root.join("Cargo.toml")) @@ -397,7 +398,7 @@ fn check_license_exceptions( metadata: &Metadata, exceptions: &[(&str, &str)], runtime_ids: HashSet<&PackageId>, - bad: &mut bool, + bad: &AtomicBool, ) { // Validate the EXCEPTIONS list hasn't changed. for (name, license) in exceptions { @@ -422,10 +423,9 @@ fn check_license_exceptions( } Some(pkg_license) => { if pkg_license.as_str() != *license { - println!("dependency exception `{name}` license has changed"); - println!(" previously `{license}` now `{pkg_license}`"); - println!(" update EXCEPTIONS for the new license"); - *bad = true; + tidy_error!(bad, concat!("dependency exception `{}` license has changed\n", + " previously `{}` now `{}`\n", + " update EXCEPTIONS for the new license\n"), name, license, pkg_license); } } } @@ -472,7 +472,7 @@ fn check_permitted_dependencies( descr: &str, permitted_dependencies: &[&'static str], restricted_dependency_crates: &[&'static str], - bad: &mut bool, + bad: &AtomicBool, ) { // Check that the PERMITTED_DEPENDENCIES does not have unused entries. for name in permitted_dependencies { @@ -547,7 +547,7 @@ fn check_crate_dependencies<'a>( fn check_crate_duplicate( metadata: &Metadata, forbidden_to_have_duplicates: &[&str], - bad: &mut bool, + bad: &AtomicBool, ) { for &name in forbidden_to_have_duplicates { let matches: Vec<_> = metadata.packages.iter().filter(|pkg| pkg.name == name).collect(); @@ -634,7 +634,7 @@ fn normal_deps_of_r<'a>( } } -fn check_rustfix(metadata: &Metadata, bad: &mut bool) { +fn check_rustfix(metadata: &Metadata, bad: &AtomicBool) { let cargo = pkg_from_name(metadata, "cargo"); let compiletest = pkg_from_name(metadata, "compiletest"); let cargo_deps = deps_of(metadata, &cargo.id); diff --git a/src/tools/tidy/src/edition.rs b/src/tools/tidy/src/edition.rs index 57fad91a4da58..9876bffa7cf80 100644 --- a/src/tools/tidy/src/edition.rs +++ b/src/tools/tidy/src/edition.rs @@ -1,14 +1,14 @@ //! Tidy check to ensure that crate `edition` is '2018' or '2021'. use crate::walk::{filter_dirs, walk}; -use std::path::Path; +use std::{path::Path, sync::atomic::AtomicBool}; fn is_edition_2021(mut line: &str) -> bool { line = line.trim(); line == "edition = \"2021\"" } -pub fn check(path: &Path, bad: &mut bool) { +pub fn check(path: &Path, bad: &AtomicBool) { walk(path, |path| filter_dirs(path) || path.ends_with("src/test"), &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap(); diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index b776c746ad877..5fef8e559c555 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -6,6 +6,7 @@ use std::collections::{HashMap, HashSet}; use std::ffi::OsStr; use std::fs::read_to_string; use std::path::Path; +use std::sync::atomic::AtomicBool; use regex::Regex; @@ -198,7 +199,7 @@ fn extract_error_codes_from_source( } } -pub fn check(paths: &[&Path], bad: &mut bool) { +pub fn check(paths: &[&Path], bad: &AtomicBool) { let mut errors = Vec::new(); let mut found_explanations = 0; let mut found_tests = 0; diff --git a/src/tools/tidy/src/errors.rs b/src/tools/tidy/src/errors.rs index 6881e9f6c6cbd..e0762d3adf70f 100644 --- a/src/tools/tidy/src/errors.rs +++ b/src/tools/tidy/src/errors.rs @@ -6,8 +6,9 @@ use crate::walk::{filter_dirs, walk}; use std::collections::HashMap; use std::path::Path; +use std::sync::atomic::{AtomicBool, Ordering}; -pub fn check(path: &Path, bad: &mut bool) { +pub fn check(path: &Path, bad: &AtomicBool) { let mut map: HashMap<_, Vec<_>> = HashMap::new(); walk(path, |path| filter_dirs(path) || path.ends_with("src/test"), &mut |entry, contents| { let file = entry.path(); @@ -66,7 +67,7 @@ pub fn check(path: &Path, bad: &mut bool) { } } - if !*bad { + if !bad.load(Ordering::Relaxed) { println!("* {} error codes", map.len()); println!("* highest error code: E{:04}", max); } diff --git a/src/tools/tidy/src/extdeps.rs b/src/tools/tidy/src/extdeps.rs index aad57cacbb41e..0ab3229eb96db 100644 --- a/src/tools/tidy/src/extdeps.rs +++ b/src/tools/tidy/src/extdeps.rs @@ -2,13 +2,14 @@ use std::fs; use std::path::Path; +use std::sync::atomic::AtomicBool; /// List of allowed sources for packages. const ALLOWED_SOURCES: &[&str] = &["\"registry+https://github.com/rust-lang/crates.io-index\""]; /// 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, bad: &AtomicBool) { // `Cargo.lock` of rust. let path = root.join("Cargo.lock"); diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index 6ff0f987dc6ed..df511d0dbdefa 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -15,6 +15,7 @@ use std::fmt; use std::fs; use std::num::NonZeroU32; use std::path::Path; +use std::sync::atomic::{AtomicBool, Ordering}; use regex::Regex; @@ -84,7 +85,7 @@ pub fn check( src_path: &Path, compiler_path: &Path, lib_path: &Path, - bad: &mut bool, + bad: &AtomicBool, verbose: bool, ) -> CollectedFeatures { let mut features = collect_lang_features(compiler_path, bad); @@ -205,7 +206,7 @@ pub fn check( } } - if *bad { + if bad.load(Ordering::Relaxed) { return CollectedFeatures { lib: lib_features, lang: features }; } @@ -279,7 +280,7 @@ fn test_filen_gate(filen_underscore: &str, features: &mut Features) -> bool { false } -pub fn collect_lang_features(base_compiler_path: &Path, bad: &mut bool) -> Features { +pub fn collect_lang_features(base_compiler_path: &Path, bad: &AtomicBool) -> Features { let mut features = Features::new(); collect_lang_features_in(&mut features, base_compiler_path, "active.rs", bad); collect_lang_features_in(&mut features, base_compiler_path, "accepted.rs", bad); @@ -287,7 +288,7 @@ pub fn collect_lang_features(base_compiler_path: &Path, bad: &mut bool) -> Featu 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, bad: &AtomicBool) { let path = base.join("rustc_feature").join("src").join(file); let contents = t!(fs::read_to_string(&path)); @@ -442,7 +443,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, + bad: &AtomicBool, lang_features: &Features, ) -> Features { let mut lib_features = Features::new(); diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index ce7e7ac5cd4ca..9950832565225 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -3,7 +3,7 @@ //! This library contains the tidy lints and exposes it //! to be used by tools. -use std::fmt::Display; +use std::{fmt::Display, sync::atomic::{AtomicBool, Ordering}}; use termcolor::WriteColor; @@ -35,11 +35,11 @@ macro_rules! tidy_error { }); } -fn tidy_error(bad: &mut bool, args: impl Display) -> std::io::Result<()> { +fn tidy_error(bad: &AtomicBool, args: impl Display) -> std::io::Result<()> { use std::io::Write; use termcolor::{Color, ColorChoice, ColorSpec, StandardStream}; - *bad = true; + bad.store(true, Ordering::Relaxed); let mut stderr = StandardStream::stdout(ColorChoice::Auto); stderr.set_color(ColorSpec::new().set_fg(Some(Color::Red)))?; diff --git a/src/tools/tidy/src/mir_opt_tests.rs b/src/tools/tidy/src/mir_opt_tests.rs index 018573284ea79..b5a77fe66820f 100644 --- a/src/tools/tidy/src/mir_opt_tests.rs +++ b/src/tools/tidy/src/mir_opt_tests.rs @@ -2,8 +2,9 @@ use std::collections::HashSet; use std::path::{Path, PathBuf}; +use std::sync::atomic::AtomicBool; -fn check_unused_files(path: &Path, bless: bool, bad: &mut bool) { +fn check_unused_files(path: &Path, bless: bool, bad: &AtomicBool) { let mut rs_files = Vec::::new(); let mut output_files = HashSet::::new(); let files = walkdir::WalkDir::new(&path.join("test/mir-opt")).into_iter(); @@ -40,7 +41,7 @@ fn check_unused_files(path: &Path, bless: bool, bad: &mut bool) { } } -fn check_dash_files(path: &Path, bless: bool, bad: &mut bool) { +fn check_dash_files(path: &Path, bless: bool, bad: &AtomicBool) { for file in walkdir::WalkDir::new(&path.join("test/mir-opt")) .into_iter() .filter_map(Result::ok) @@ -68,7 +69,7 @@ fn check_dash_files(path: &Path, bless: bool, bad: &mut bool) { } } -pub fn check(path: &Path, bless: bool, bad: &mut bool) { +pub fn check(path: &Path, bless: bool, bad: &AtomicBool) { check_unused_files(path, bless, bad); check_dash_files(path, bless, bad); } diff --git a/src/tools/tidy/src/pal.rs b/src/tools/tidy/src/pal.rs index 33938ac9a0a5f..23b7762341bc5 100644 --- a/src/tools/tidy/src/pal.rs +++ b/src/tools/tidy/src/pal.rs @@ -33,6 +33,7 @@ use crate::walk::{filter_dirs, walk}; use std::iter::Iterator; use std::path::Path; +use std::sync::atomic::AtomicBool; // Paths that may contain platform-specific code. const EXCEPTION_PATHS: &[&str] = &[ @@ -64,7 +65,7 @@ const EXCEPTION_PATHS: &[&str] = &[ "library/std/src/personality/", ]; -pub fn check(path: &Path, bad: &mut bool) { +pub fn check(path: &Path, bad: &AtomicBool) { // Sanity check that the complex parsing here works. let mut saw_target_arch = false; let mut saw_cfg_bang = false; @@ -95,7 +96,7 @@ pub fn check(path: &Path, bad: &mut bool) { fn check_cfgs( contents: &str, file: &Path, - bad: &mut bool, + bad: &AtomicBool, saw_target_arch: &mut bool, saw_cfg_bang: &mut bool, ) { diff --git a/src/tools/tidy/src/primitive_docs.rs b/src/tools/tidy/src/primitive_docs.rs index f3200e0afd71a..15023685e7588 100644 --- a/src/tools/tidy/src/primitive_docs.rs +++ b/src/tools/tidy/src/primitive_docs.rs @@ -2,9 +2,9 @@ //! different files so that relative links work properly without having to have `CARGO_PKG_NAME` //! set, but conceptually they should always be the same. -use std::path::Path; +use std::{path::Path, sync::atomic::AtomicBool}; -pub fn check(library_path: &Path, bad: &mut bool) { +pub fn check(library_path: &Path, bad: &AtomicBool) { let std_name = "std/src/primitive_docs.rs"; let core_name = "core/src/primitive_docs.rs"; let std_contents = std::fs::read_to_string(library_path.join(std_name)) diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index 0a65e02f2c0a1..86f5eebe0edc3 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -18,7 +18,7 @@ use crate::walk::{filter_dirs, walk}; use regex::{Regex, RegexSet}; -use std::path::Path; +use std::{path::Path, sync::atomic::AtomicBool}; /// Error code markdown is restricted to 80 columns because they can be /// displayed on the console with --example. @@ -217,7 +217,7 @@ fn is_unexplained_ignore(extension: &str, line: &str) -> bool { true } -pub fn check(path: &Path, bad: &mut bool) { +pub fn check(path: &Path, bad: &AtomicBool) { fn skip(path: &Path) -> bool { filter_dirs(path) || skip_markdown_path(path) } diff --git a/src/tools/tidy/src/target_specific_tests.rs b/src/tools/tidy/src/target_specific_tests.rs index 462e2e7fb9b24..6ac24f6d7f799 100644 --- a/src/tools/tidy/src/target_specific_tests.rs +++ b/src/tools/tidy/src/target_specific_tests.rs @@ -3,6 +3,7 @@ use std::collections::BTreeMap; use std::path::Path; +use std::sync::atomic::AtomicBool; const COMMENT: &str = "//"; const LLVM_COMPONENTS_HEADER: &str = "needs-llvm-components:"; @@ -34,7 +35,7 @@ struct RevisionInfo<'a> { llvm_components: Option>, } -pub fn check(path: &Path, bad: &mut bool) { +pub fn check(path: &Path, bad: &AtomicBool) { let tests = path.join("test"); crate::walk::walk( &tests, @@ -61,8 +62,7 @@ pub fn check(path: &Path, bad: &mut bool) { let info = header_map.entry(cfg).or_insert(RevisionInfo::default()); info.target_arch.replace(arch); } else { - eprintln!("{file}: seems to have a malformed --target value"); - *bad = true; + tidy_error!(bad, "{file}: seems to have a malformed --target value"); } } } @@ -72,18 +72,18 @@ pub fn check(path: &Path, bad: &mut bool) { match (target_arch, llvm_components) { (None, None) => {} (Some(_), None) => { - eprintln!( + tidy_error!( + bad, "{}: revision {} should specify `{}` as it has `--target` set", file, rev, LLVM_COMPONENTS_HEADER ); - *bad = true; } (None, Some(_)) => { - eprintln!( + tidy_error!( + bad, "{}: revision {} should not specify `{}` as it doesn't need `--target`", file, rev, LLVM_COMPONENTS_HEADER ); - *bad = true; } (Some(_), Some(_)) => { // FIXME: check specified components against the target architectures we diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs index 1e9b416d42054..3a03faf67246c 100644 --- a/src/tools/tidy/src/ui_tests.rs +++ b/src/tools/tidy/src/ui_tests.rs @@ -6,13 +6,14 @@ use ignore::Walk; use ignore::WalkBuilder; use std::fs; use std::path::Path; +use std::sync::atomic::AtomicBool; const ENTRY_LIMIT: usize = 1000; // FIXME: The following limits should be reduced eventually. const ROOT_ENTRY_LIMIT: usize = 939; const ISSUES_ENTRY_LIMIT: usize = 2050; -fn check_entries(path: &Path, bad: &mut bool) { +fn check_entries(path: &Path, bad: &AtomicBool) { for dir in Walk::new(&path.join("test/ui")) { if let Ok(entry) = dir { if entry.file_type().map(|ft| ft.is_dir()).unwrap_or(false) { @@ -51,7 +52,7 @@ fn check_entries(path: &Path, bad: &mut bool) { } } -pub fn check(path: &Path, bad: &mut bool) { +pub fn check(path: &Path, bad: &AtomicBool) { check_entries(&path, bad); for path in &[&path.join("test/ui"), &path.join("test/ui-fulldeps")] { crate::walk::walk_no_read(path, |_| false, &mut |entry| { diff --git a/src/tools/tidy/src/unit_tests.rs b/src/tools/tidy/src/unit_tests.rs index 7062cf2279f47..d2bb031e0e15e 100644 --- a/src/tools/tidy/src/unit_tests.rs +++ b/src/tools/tidy/src/unit_tests.rs @@ -8,9 +8,9 @@ //! during normal build. use crate::walk::{filter_dirs, walk}; -use std::path::Path; +use std::{path::Path, sync::atomic::AtomicBool}; -pub fn check(root_path: &Path, bad: &mut bool) { +pub fn check(root_path: &Path, bad: &AtomicBool) { let core = root_path.join("core"); let core_copy = core.clone(); let core_tests = core.join("tests"); diff --git a/src/tools/tidy/src/unstable_book.rs b/src/tools/tidy/src/unstable_book.rs index 7dfb6224d240a..0e3cc49cd90be 100644 --- a/src/tools/tidy/src/unstable_book.rs +++ b/src/tools/tidy/src/unstable_book.rs @@ -2,6 +2,7 @@ use crate::features::{CollectedFeatures, Features, Status}; use std::collections::BTreeSet; use std::fs; use std::path::{Path, PathBuf}; +use std::sync::atomic::AtomicBool; pub const PATH_STR: &str = "doc/unstable-book"; @@ -71,7 +72,7 @@ fn collect_unstable_book_lib_features_section_file_names(base_src_path: &Path) - collect_unstable_book_section_file_names(&unstable_book_lib_features_path(base_src_path)) } -pub fn check(path: &Path, features: CollectedFeatures, bad: &mut bool) { +pub fn check(path: &Path, features: CollectedFeatures, bad: &AtomicBool) { let lang_features = features.lang; let lib_features = features .lib From f8399f73bcce15bec123412ce0657978655635da Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 4 Jan 2023 03:54:41 +0000 Subject: [PATCH 4/5] [wip] add a check step for tidy\n this allows running clippy on it --- src/bootstrap/builder.rs | 1 + src/bootstrap/check.rs | 89 ++++++++++++++++++++++++++++++++++------ 2 files changed, 77 insertions(+), 13 deletions(-) diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs index b54bf43262198..248cb0e701c5e 100644 --- a/src/bootstrap/builder.rs +++ b/src/bootstrap/builder.rs @@ -649,6 +649,7 @@ impl<'a> Builder<'a> { check::Rustc, check::Rustdoc, check::CodegenBackend, + check::Tidy, check::Clippy, check::Miri, check::CargoMiri, diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index b203ecd3844b0..6d490ed0380d0 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -361,6 +361,81 @@ impl Step for RustAnalyzer { } } +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] +pub struct Tidy; + +impl Step for Tidy { + type Output = (); + const ONLY_HOSTS: bool = true; + const DEFAULT: bool = true; + + fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { + run.path("src/tools/tidy") + } + + fn make_run(run: RunConfig<'_>) { + run.builder.ensure(Tidy); + } + + fn run(self, builder: &Builder<'_>) -> Self::Output { + let compiler = builder.compiler(builder.top_stage, builder.config.build); + let host = builder.build.build; + + let mut cargo = prepare_tool_cargo( + builder, + compiler, + Mode::ToolBootstrap, + host, + cargo_subcommand(builder.kind), + "src/tools/tidy", + SourceType::InTree, + &[], + ); + + // For ./x.py clippy, don't run with --all-targets because + // linting tests and benchmarks can produce very noisy results + if builder.kind != Kind::Clippy { + cargo.arg("--all-targets"); + } + + cargo.args(args(builder)); + + // Enable internal lints for clippy and rustdoc + // NOTE: this doesn't enable lints for any other tools unless they explicitly add `#![warn(rustc::internal)]` + // See https://github.com/rust-lang/rust/pull/80573#issuecomment-754010776 + cargo.rustflag("-Zunstable-options"); + + builder.info(&format!( + "Checking stage{} tidy artifacts ({} -> {})", + builder.top_stage, + &compiler.host.triple, + host.triple + )); + run_cargo( + builder, + cargo, + &tool_stamp(builder, compiler, host, Mode::ToolBootstrap, "tidy"), + vec![], + true, + false, + ); + } +} + +/// Cargo's output path in a given stage, compiled by a particular +/// compiler for the specified target. +fn tool_stamp( + builder: &Builder<'_>, + compiler: Compiler, + target: TargetSelection, + mode: Mode, + name: &str, +) -> PathBuf { + builder + .cargo_out(compiler, mode, target) + .join(format!(".{name}-check.stamp")) +} + macro_rules! tool_check_step { ($name:ident, $path:literal, $($alias:literal, )* $source_type:path $(, $default:literal )?) => { #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] @@ -422,23 +497,11 @@ macro_rules! tool_check_step { run_cargo( builder, cargo, - &stamp(builder, compiler, target), + &tool_stamp(builder, compiler, target, Mode::ToolRustc, &stringify!($name).to_lowercase()), vec![], true, false, ); - - /// Cargo's output path in a given stage, compiled by a particular - /// compiler for the specified target. - fn stamp( - builder: &Builder<'_>, - compiler: Compiler, - target: TargetSelection, - ) -> PathBuf { - builder - .cargo_out(compiler, Mode::ToolRustc, target) - .join(format!(".{}-check.stamp", stringify!($name).to_lowercase())) - } } } }; From 0e33cc52f77e08aa53a048a09422aa172d9345e1 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Wed, 4 Jan 2023 04:35:15 +0000 Subject: [PATCH 5/5] a miserable disappointment --- src/bootstrap/check.rs | 8 +--- src/tools/tidy/src/debug_artifacts.rs | 5 ++- src/tools/tidy/src/deps.rs | 14 ++++-- src/tools/tidy/src/error_codes_check.rs | 49 ++++++++++++++------- src/tools/tidy/src/errors.rs | 11 ++++- src/tools/tidy/src/features.rs | 26 +++++++---- src/tools/tidy/src/lib.rs | 5 ++- src/tools/tidy/src/main.rs | 13 +----- src/tools/tidy/src/pal.rs | 24 +++++----- src/tools/tidy/src/style.rs | 8 ++-- src/tools/tidy/src/target_specific_tests.rs | 8 +++- src/tools/tidy/src/walk.rs | 18 ++++---- 12 files changed, 114 insertions(+), 75 deletions(-) diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index 6d490ed0380d0..d39774916b383 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -407,9 +407,7 @@ impl Step for Tidy { builder.info(&format!( "Checking stage{} tidy artifacts ({} -> {})", - builder.top_stage, - &compiler.host.triple, - host.triple + builder.top_stage, &compiler.host.triple, host.triple )); run_cargo( builder, @@ -431,9 +429,7 @@ fn tool_stamp( mode: Mode, name: &str, ) -> PathBuf { - builder - .cargo_out(compiler, mode, target) - .join(format!(".{name}-check.stamp")) + builder.cargo_out(compiler, mode, target).join(format!(".{name}-check.stamp")) } macro_rules! tool_check_step { diff --git a/src/tools/tidy/src/debug_artifacts.rs b/src/tools/tidy/src/debug_artifacts.rs index 7b1a19a718b39..73d5ccc7488b0 100644 --- a/src/tools/tidy/src/debug_artifacts.rs +++ b/src/tools/tidy/src/debug_artifacts.rs @@ -1,7 +1,10 @@ //! Tidy check to prevent creation of unnecessary debug artifacts while running tests. use crate::walk::{filter_dirs, walk}; -use std::{path::{Path, PathBuf}, sync::atomic::AtomicBool}; +use std::{ + path::{Path, PathBuf}, + sync::atomic::AtomicBool, +}; const GRAPHVIZ_POSTFLOW_MSG: &str = "`borrowck_graphviz_postflow` attribute in test"; diff --git a/src/tools/tidy/src/deps.rs b/src/tools/tidy/src/deps.rs index c72be0c7108fb..649befb64f0f1 100644 --- a/src/tools/tidy/src/deps.rs +++ b/src/tools/tidy/src/deps.rs @@ -423,9 +423,17 @@ fn check_license_exceptions( } Some(pkg_license) => { if pkg_license.as_str() != *license { - tidy_error!(bad, concat!("dependency exception `{}` license has changed\n", - " previously `{}` now `{}`\n", - " update EXCEPTIONS for the new license\n"), name, license, pkg_license); + tidy_error!( + bad, + concat!( + "dependency exception `{}` license has changed\n", + " previously `{}` now `{}`\n", + " update EXCEPTIONS for the new license\n" + ), + name, + license, + pkg_license + ); } } } diff --git a/src/tools/tidy/src/error_codes_check.rs b/src/tools/tidy/src/error_codes_check.rs index 5fef8e559c555..dc5c61dfc5124 100644 --- a/src/tools/tidy/src/error_codes_check.rs +++ b/src/tools/tidy/src/error_codes_check.rs @@ -6,7 +6,8 @@ use std::collections::{HashMap, HashSet}; use std::ffi::OsStr; use std::fs::read_to_string; use std::path::Path; -use std::sync::atomic::AtomicBool; +use std::sync::atomic::{AtomicBool, AtomicI32, Ordering}; +use std::sync::RwLock; use regex::Regex; @@ -166,7 +167,9 @@ fn extract_error_codes( } } -fn extract_error_codes_from_tests(f: &str, error_codes: &mut HashMap) { +fn extract_error_codes_from_tests(f: &str, error_codes: &RwLock>) { + let mut error_codes = error_codes.write().unwrap(); + for line in f.lines() { let s = line.trim(); if s.starts_with("error[E") || s.starts_with("warning[E") { @@ -184,9 +187,10 @@ fn extract_error_codes_from_tests(f: &str, error_codes: &mut HashMap, + error_codes: &RwLock>, regex: &Regex, ) { + let mut error_codes = error_codes.write().unwrap(); for line in f.lines() { if line.trim_start().starts_with("//") { continue; @@ -200,11 +204,11 @@ fn extract_error_codes_from_source( } pub fn check(paths: &[&Path], bad: &AtomicBool) { - let mut errors = Vec::new(); - let mut found_explanations = 0; - let mut found_tests = 0; - let mut error_codes: HashMap = HashMap::new(); - let mut explanations: HashSet = HashSet::new(); + let errors = RwLock::new(Vec::new()); + let found_explanations = AtomicI32::new(0); + let found_tests = AtomicI32::new(0); + let error_codes = RwLock::new(HashMap::new()); + let explanations = RwLock::new(HashSet::new()); // We want error codes which match the following cases: // // * foo(a, E0111, a) @@ -219,15 +223,20 @@ pub fn check(paths: &[&Path], bad: &AtomicBool) { let entry_path = entry.path(); if file_name == "error_codes.rs" { - extract_error_codes(contents, &mut error_codes, entry.path(), &mut errors); - found_explanations += 1; + extract_error_codes( + contents, + &mut error_codes.write().unwrap(), + entry.path(), + &mut errors.write().unwrap(), + ); + found_explanations.fetch_add(1, Ordering::Relaxed); } else if entry_path.extension() == Some(OsStr::new("stderr")) { - extract_error_codes_from_tests(contents, &mut error_codes); - found_tests += 1; + extract_error_codes_from_tests(contents, &error_codes); + found_tests.fetch_add(1, Ordering::Relaxed); } else if entry_path.extension() == Some(OsStr::new("rs")) { let path = entry.path().to_string_lossy(); if PATHS_TO_IGNORE_FOR_EXTRACTION.iter().all(|c| !path.contains(c)) { - extract_error_codes_from_source(contents, &mut error_codes, ®ex); + extract_error_codes_from_source(contents, &error_codes, ®ex); } } else if entry_path .parent() @@ -236,14 +245,22 @@ pub fn check(paths: &[&Path], bad: &AtomicBool) { .unwrap_or(false) && entry_path.extension() == Some(OsStr::new("md")) { - explanations.insert(file_name.to_str().unwrap().replace(".md", "")); + explanations + .write() + .unwrap() + .insert(file_name.to_str().unwrap().replace(".md", "")); } }); } - if found_explanations == 0 { + + let explanations = explanations.into_inner().unwrap(); + let error_codes = error_codes.into_inner().unwrap(); + let mut errors = errors.into_inner().unwrap(); + + if found_explanations.load(Ordering::Relaxed) == 0 { tidy_error!(bad, "No error code explanation was tested!"); } - if found_tests == 0 { + if found_tests.load(Ordering::Relaxed) == 0 { tidy_error!(bad, "No error code was found in compilation errors!"); } if explanations.is_empty() { diff --git a/src/tools/tidy/src/errors.rs b/src/tools/tidy/src/errors.rs index e0762d3adf70f..4ffa019f9db0b 100644 --- a/src/tools/tidy/src/errors.rs +++ b/src/tools/tidy/src/errors.rs @@ -7,9 +7,10 @@ use crate::walk::{filter_dirs, walk}; use std::collections::HashMap; use std::path::Path; use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::RwLock; pub fn check(path: &Path, bad: &AtomicBool) { - let mut map: HashMap<_, Vec<_>> = HashMap::new(); + let map: RwLock>> = RwLock::new(HashMap::new()); walk(path, |path| filter_dirs(path) || path.ends_with("src/test"), &mut |entry, contents| { let file = entry.path(); let filename = file.file_name().unwrap().to_string_lossy(); @@ -44,7 +45,11 @@ pub fn check(path: &Path, bad: &AtomicBool) { Ok(n) => n, Err(..) => continue, }; - map.entry(code).or_default().push((file.to_owned(), num + 1, line.to_owned())); + map.write().unwrap().entry(code).or_default().push(( + file.to_owned(), + num + 1, + line.to_owned(), + )); break; } @@ -52,6 +57,8 @@ pub fn check(path: &Path, bad: &AtomicBool) { } }); + let map = map.into_inner().unwrap(); + let mut max = 0; for (&code, entries) in map.iter() { if code > max { diff --git a/src/tools/tidy/src/features.rs b/src/tools/tidy/src/features.rs index df511d0dbdefa..6df41dcfd6cf5 100644 --- a/src/tools/tidy/src/features.rs +++ b/src/tools/tidy/src/features.rs @@ -16,6 +16,7 @@ use std::fs; use std::num::NonZeroU32; use std::path::Path; use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::RwLock; use regex::Regex; @@ -71,14 +72,14 @@ pub struct CollectedFeatures { // Currently only used for unstable book generation pub fn collect_lib_features(base_src_path: &Path) -> Features { - let mut lib_features = Features::new(); + let lib_features = RwLock::new(Features::new()); map_lib_features(base_src_path, &mut |res, _, _| { if let Ok((name, feature)) = res { - lib_features.insert(name.to_owned(), feature); + lib_features.write().unwrap().insert(name.to_owned(), feature); } }); - lib_features + lib_features.into_inner().unwrap() } pub fn check( @@ -88,12 +89,13 @@ pub fn check( bad: &AtomicBool, verbose: bool, ) -> CollectedFeatures { - let mut features = collect_lang_features(compiler_path, bad); + let features = collect_lang_features(compiler_path, bad); assert!(!features.is_empty()); let lib_features = get_and_check_lib_features(lib_path, bad, &features); assert!(!lib_features.is_empty()); + let features = RwLock::new(features); walk_many( &[ &src_path.join("test/ui"), @@ -112,11 +114,13 @@ pub fn check( return; } + let mut features = features.write().unwrap(); + let filen_underscore = filename.replace('-', "_").replace(".rs", ""); let filename_is_gate_test = test_filen_gate(&filen_underscore, &mut features); for (i, line) in contents.lines().enumerate() { - let mut err = |msg: &str| { + let err = |msg: &str| { tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg); }; @@ -150,6 +154,8 @@ pub fn check( }, ); + let features = features.into_inner().unwrap(); + // Only check the number of lang features. // Obligatory testing for library features is dumb. let gate_untested = features @@ -446,10 +452,10 @@ fn get_and_check_lib_features( bad: &AtomicBool, lang_features: &Features, ) -> Features { - let mut lib_features = Features::new(); + let lib_features = RwLock::new(Features::new()); map_lib_features(base_src_path, &mut |res, file, line| match res { Ok((name, f)) => { - let mut check_features = |f: &Feature, list: &Features, display: &str| { + let check_features = |f: &Feature, list: &Features, display: &str| { if let Some(ref s) = list.get(name) { if f.tracking_issue != s.tracking_issue && f.level != Status::Stable { tidy_error!( @@ -465,6 +471,8 @@ fn get_and_check_lib_features( } }; check_features(&f, &lang_features, "corresponding lang feature"); + + let mut lib_features = lib_features.write().unwrap(); check_features(&f, &lib_features, "previous"); lib_features.insert(name.to_owned(), f); } @@ -472,12 +480,12 @@ fn get_and_check_lib_features( tidy_error!(bad, "{}:{}: {}", file.display(), line, msg); } }); - lib_features + lib_features.into_inner().unwrap() } fn map_lib_features( base_src_path: &Path, - mf: &mut (dyn Send + Sync + FnMut(Result<(&str, Feature), &str>, &Path, usize)), + mf: &(dyn Send + Sync + Fn(Result<(&str, Feature), &str>, &Path, usize)), ) { walk( base_src_path, diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 9950832565225..e03fc81e27859 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -3,7 +3,10 @@ //! This library contains the tidy lints and exposes it //! to be used by tools. -use std::{fmt::Display, sync::atomic::{AtomicBool, Ordering}}; +use std::{ + fmt::Display, + sync::atomic::{AtomicBool, Ordering}, +}; use termcolor::WriteColor; diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 6714c63ee62a1..3486ff668ec99 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -57,11 +57,7 @@ fn main() { drain_handles(&mut handles); let handle = s.spawn(|| { - let mut flag = false; - $p::check($($args),* , &mut flag); - if (flag) { - bad.store(true, Ordering::Relaxed); - } + $p::check($($args),* , &bad); }); handles.push_back(handle); } @@ -110,12 +106,7 @@ fn main() { let collected = { drain_handles(&mut handles); - let mut flag = false; - let r = features::check(&src_path, &compiler_path, &library_path, &mut flag, verbose); - if flag { - bad.store(true, Ordering::Relaxed); - } - r + features::check(&src_path, &compiler_path, &library_path, &bad, verbose) }; check!(unstable_book, &src_path, collected); }); diff --git a/src/tools/tidy/src/pal.rs b/src/tools/tidy/src/pal.rs index 23b7762341bc5..25aad0ef01574 100644 --- a/src/tools/tidy/src/pal.rs +++ b/src/tools/tidy/src/pal.rs @@ -33,7 +33,7 @@ use crate::walk::{filter_dirs, walk}; use std::iter::Iterator; use std::path::Path; -use std::sync::atomic::AtomicBool; +use std::sync::atomic::{AtomicBool, Ordering}; // Paths that may contain platform-specific code. const EXCEPTION_PATHS: &[&str] = &[ @@ -67,8 +67,8 @@ const EXCEPTION_PATHS: &[&str] = &[ pub fn check(path: &Path, bad: &AtomicBool) { // Sanity check that the complex parsing here works. - let mut saw_target_arch = false; - let mut saw_cfg_bang = false; + let saw_target_arch = AtomicBool::new(false); + let saw_cfg_bang = AtomicBool::new(false); walk(path, filter_dirs, &mut |entry, contents| { let file = entry.path(); let filestr = file.to_string_lossy().replace("\\", "/"); @@ -86,19 +86,19 @@ pub fn check(path: &Path, bad: &AtomicBool) { return; } - check_cfgs(contents, &file, bad, &mut saw_target_arch, &mut saw_cfg_bang); + check_cfgs(contents, &file, bad, &saw_target_arch, &saw_cfg_bang); }); - assert!(saw_target_arch); - assert!(saw_cfg_bang); + assert!(saw_target_arch.load(Ordering::Relaxed)); + assert!(saw_cfg_bang.load(Ordering::Relaxed)); } fn check_cfgs( contents: &str, file: &Path, bad: &AtomicBool, - saw_target_arch: &mut bool, - saw_cfg_bang: &mut bool, + saw_target_arch: &AtomicBool, + saw_cfg_bang: &AtomicBool, ) { // Pull out all `cfg(...)` and `cfg!(...)` strings. let cfgs = parse_cfgs(contents); @@ -118,11 +118,11 @@ fn check_cfgs( for (idx, cfg) in cfgs { // Sanity check that the parsing here works. - if !*saw_target_arch && cfg.contains("target_arch") { - *saw_target_arch = true + if cfg.contains("target_arch") { + saw_target_arch.store(true, Ordering::Relaxed); } - if !*saw_cfg_bang && cfg.contains("cfg!") { - *saw_cfg_bang = true + if cfg.contains("cfg!") { + saw_cfg_bang.store(true, Ordering::Relaxed); } let contains_platform_specific_cfg = cfg.contains("target_os") diff --git a/src/tools/tidy/src/style.rs b/src/tools/tidy/src/style.rs index 86f5eebe0edc3..35181d42b56e9 100644 --- a/src/tools/tidy/src/style.rs +++ b/src/tools/tidy/src/style.rs @@ -303,7 +303,7 @@ pub fn check(path: &Path, bad: &AtomicBool) { lines += 1; } - let mut err = |msg: &str| { + let err = |msg: &str| { tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg); }; if !under_rustfmt @@ -376,12 +376,12 @@ pub fn check(path: &Path, bad: &AtomicBool) { } } if leading_new_lines { - let mut err = |_| { + let err = |_| { tidy_error!(bad, "{}: leading newline", file.display()); }; suppressible_tidy_err!(err, skip_leading_newlines, "mising leading newline"); } - let mut err = |msg: &str| { + let err = |msg: &str| { tidy_error!(bad, "{}: {}", file.display(), msg); }; match trailing_new_lines { @@ -394,7 +394,7 @@ pub fn check(path: &Path, bad: &AtomicBool) { ), }; if lines > LINES { - let mut err = |_| { + let err = |_| { tidy_error!( bad, "{}: too many lines ({}) (add `// \ diff --git a/src/tools/tidy/src/target_specific_tests.rs b/src/tools/tidy/src/target_specific_tests.rs index 6ac24f6d7f799..209249b089fa6 100644 --- a/src/tools/tidy/src/target_specific_tests.rs +++ b/src/tools/tidy/src/target_specific_tests.rs @@ -75,14 +75,18 @@ pub fn check(path: &Path, bad: &AtomicBool) { tidy_error!( bad, "{}: revision {} should specify `{}` as it has `--target` set", - file, rev, LLVM_COMPONENTS_HEADER + file, + rev, + LLVM_COMPONENTS_HEADER ); } (None, Some(_)) => { tidy_error!( bad, "{}: revision {} should not specify `{}` as it doesn't need `--target`", - file, rev, LLVM_COMPONENTS_HEADER + file, + rev, + LLVM_COMPONENTS_HEADER ); } (Some(_), Some(_)) => { diff --git a/src/tools/tidy/src/walk.rs b/src/tools/tidy/src/walk.rs index 16edc4aa57aca..0bc4944e28eaa 100644 --- a/src/tools/tidy/src/walk.rs +++ b/src/tools/tidy/src/walk.rs @@ -66,13 +66,15 @@ pub(crate) fn walk_no_read( let mut walker = ignore::WalkBuilder::new(path); let walker = walker.filter_entry(move |e| !skip(e.path())); // for entry in walker.build() { - walker.build_parallel().run(|| Box::new(|entry| { - if let Ok(entry) = entry { - if entry.file_type().map_or(true, |kind| kind.is_dir() || kind.is_symlink()) { - return WalkState::Continue; + walker.build_parallel().run(|| { + Box::new(|entry| { + if let Ok(entry) = entry { + if entry.file_type().map_or(true, |kind| kind.is_dir() || kind.is_symlink()) { + return WalkState::Continue; + } + f(&entry); } - f(&entry); - } - WalkState::Continue - })); + WalkState::Continue + }) + }); }