From f11713be75dd8c50d9735d9bed7608a149e0f415 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Thu, 22 Feb 2024 18:42:53 +0000 Subject: [PATCH] Error on stray .stderr/.stdout files for (un-)revisioned tests --- src/tools/tidy/src/iter_header.rs | 32 ++++ src/tools/tidy/src/lib.rs | 2 + src/tools/tidy/src/main.rs | 1 + src/tools/tidy/src/target_specific_tests.rs | 28 +--- .../tests_revision_unpaired_stdout_stderr.rs | 146 ++++++++++++++++++ src/tools/tidy/src/walk.rs | 17 ++ 6 files changed, 202 insertions(+), 24 deletions(-) create mode 100644 src/tools/tidy/src/iter_header.rs create mode 100644 src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs diff --git a/src/tools/tidy/src/iter_header.rs b/src/tools/tidy/src/iter_header.rs new file mode 100644 index 0000000000000..ae635904607e7 --- /dev/null +++ b/src/tools/tidy/src/iter_header.rs @@ -0,0 +1,32 @@ +const COMMENT: &str = "//@"; + +/// A header line, like `//@name: value` consists of the prefix `//@` and the directive +/// `name: value`. It is also possibly revisioned, e.g. `//@[revision] name: value`. +pub(crate) struct HeaderLine<'ln> { + pub(crate) revision: Option<&'ln str>, + pub(crate) directive: &'ln str, +} + +/// Iterate through compiletest headers in a test contents. +/// +/// Adjusted from compiletest/src/header.rs. +pub(crate) fn iter_header<'ln>(contents: &'ln str, it: &mut dyn FnMut(HeaderLine<'ln>)) { + for ln in contents.lines() { + let ln = ln.trim(); + + // We're left with potentially `[rev]name: value`. + let Some(remainder) = ln.strip_prefix(COMMENT) else { + continue; + }; + + if let Some(remainder) = remainder.trim_start().strip_prefix('[') { + let Some((revision, remainder)) = remainder.split_once(']') else { + panic!("malformed revision directive: expected `//@[rev]`, found `{ln}`"); + }; + // We trimmed off the `[rev]` portion, left with `name: value`. + it(HeaderLine { revision: Some(revision), directive: remainder.trim() }); + } else { + it(HeaderLine { revision: None, directive: remainder.trim() }); + } + } +} diff --git a/src/tools/tidy/src/lib.rs b/src/tools/tidy/src/lib.rs index 9514998703346..6f3ade0ab58c7 100644 --- a/src/tools/tidy/src/lib.rs +++ b/src/tools/tidy/src/lib.rs @@ -65,6 +65,7 @@ pub mod ext_tool_checks; pub mod extdeps; pub mod features; pub mod fluent_alphabetical; +pub(crate) mod iter_header; pub mod mir_opt_tests; pub mod pal; pub mod rustdoc_css_themes; @@ -73,6 +74,7 @@ pub mod style; pub mod target_policy; pub mod target_specific_tests; pub mod tests_placement; +pub mod tests_revision_unpaired_stdout_stderr; pub mod ui_tests; pub mod unit_tests; pub mod unstable_book; diff --git a/src/tools/tidy/src/main.rs b/src/tools/tidy/src/main.rs index 870322c44fb85..4b98c91319da0 100644 --- a/src/tools/tidy/src/main.rs +++ b/src/tools/tidy/src/main.rs @@ -100,6 +100,7 @@ fn main() { // Checks over tests. check!(tests_placement, &root_path); + check!(tests_revision_unpaired_stdout_stderr, &tests_path); check!(debug_artifacts, &tests_path); check!(ui_tests, &tests_path, bless); check!(mir_opt_tests, &tests_path, bless); diff --git a/src/tools/tidy/src/target_specific_tests.rs b/src/tools/tidy/src/target_specific_tests.rs index c6136a18bd8c6..cb242bff05d1d 100644 --- a/src/tools/tidy/src/target_specific_tests.rs +++ b/src/tools/tidy/src/target_specific_tests.rs @@ -4,32 +4,12 @@ use std::collections::BTreeMap; use std::path::Path; +use crate::iter_header::{iter_header, HeaderLine}; use crate::walk::filter_not_rust; -const COMMENT: &str = "//@"; const LLVM_COMPONENTS_HEADER: &str = "needs-llvm-components:"; const COMPILE_FLAGS_HEADER: &str = "compile-flags:"; -/// Iterate through compiletest headers in a test contents. -/// -/// Adjusted from compiletest/src/header.rs. -fn iter_header<'a>(contents: &'a str, it: &mut dyn FnMut(Option<&'a str>, &'a str)) { - for ln in contents.lines() { - let ln = ln.trim(); - if ln.starts_with(COMMENT) && ln[COMMENT.len()..].trim_start().starts_with('[') { - if let Some(close_brace) = ln.find(']') { - let open_brace = ln.find('[').unwrap(); - let lncfg = &ln[open_brace + 1..close_brace]; - it(Some(lncfg), ln[(close_brace + 1)..].trim_start()); - } else { - panic!("malformed condition directive: expected `//[foo]`, found `{ln}`") - } - } else if ln.starts_with(COMMENT) { - it(None, ln[COMMENT.len()..].trim_start()); - } - } -} - #[derive(Default, Debug)] struct RevisionInfo<'a> { target_arch: Option<&'a str>, @@ -40,9 +20,9 @@ pub fn check(path: &Path, bad: &mut bool) { crate::walk::walk(path, |path, _is_dir| filter_not_rust(path), &mut |entry, content| { let file = entry.path().display(); let mut header_map = BTreeMap::new(); - iter_header(content, &mut |cfg, directive| { + iter_header(content, &mut |HeaderLine { revision, directive }| { if let Some(value) = directive.strip_prefix(LLVM_COMPONENTS_HEADER) { - let info = header_map.entry(cfg).or_insert(RevisionInfo::default()); + let info = header_map.entry(revision).or_insert(RevisionInfo::default()); let comp_vec = info.llvm_components.get_or_insert(Vec::new()); for component in value.split(' ') { let component = component.trim(); @@ -56,7 +36,7 @@ pub fn check(path: &Path, bad: &mut bool) { if let Some((arch, _)) = v.trim_start_matches(|c| c == ' ' || c == '=').split_once("-") { - let info = header_map.entry(cfg).or_insert(RevisionInfo::default()); + let info = header_map.entry(revision).or_insert(RevisionInfo::default()); info.target_arch.replace(arch); } else { eprintln!("{file}: seems to have a malformed --target value"); diff --git a/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs b/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs new file mode 100644 index 0000000000000..394f95e9144d2 --- /dev/null +++ b/src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs @@ -0,0 +1,146 @@ +//! Checks that there are no unpaired `.stderr` or `.stdout` for a test with and without revisions. + +use std::collections::{BTreeMap, BTreeSet}; +use std::ffi::OsStr; +use std::path::Path; + +use crate::iter_header::*; +use crate::walk::*; + +// Should be kept in sync with `CompareMode` in `src/tools/compiletest/src/common.rs`, +// as well as `run`. +const IGNORES: &[&str] = &[ + "polonius", + "chalk", + "split-dwarf", + "split-dwarf-single", + "next-solver-coherence", + "next-solver", + "run", +]; +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, bad: &mut bool) { + // 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`, + // `.stdout` under the current folder (shallow). + let mut files_under_inspection = BTreeSet::new(); + for sibling in std::fs::read_dir(entry.path()).unwrap() { + let Ok(sibling) = sibling else { + continue; + }; + + if sibling.path().is_dir() { + continue; + } + + let sibling_path = sibling.path(); + + let Some(ext) = sibling_path.extension().map(OsStr::to_str).flatten() else { + continue; + }; + + if ext == "rs" || EXTENSIONS.contains(&ext) { + files_under_inspection.insert(sibling_path); + } + } + + let mut test_info = BTreeMap::new(); + + for test in + files_under_inspection.iter().filter(|f| f.extension().is_some_and(|ext| ext == "rs")) + { + if test.ends_with(SPECIAL_TEST) { + continue; + } + + let mut expected_revisions = BTreeSet::new(); + + let contents = std::fs::read_to_string(test).unwrap(); + + // Collect directives. + iter_header(&contents, &mut |HeaderLine { revision, directive }| { + // We're trying to *find* `//@ revision: xxx` directives themselves, not revisioned + // directives. + if revision.is_some() { + return; + } + + let directive = directive.trim(); + + if directive.starts_with("revisions") { + let Some((name, value)) = directive.split_once([':', ' ']) else { + return; + }; + + if name == "revisions" { + let revs = value.split(' '); + for rev in revs { + expected_revisions.insert(rev.to_owned()); + } + } + } + }); + + let Some((test_name, _)) = test.to_str().map(|s| s.split_once('.')).flatten() else { + continue; + }; + + test_info.insert(test_name.to_string(), (test, expected_revisions)); + } + + // Our test file `foo.rs` has specified no revisions. There should not be any + // `foo.rev{.stderr,.stdout}` files. rustc-dev-guide says test output files can have names + // of the form: `test-name.revision.compare_mode.extension`, but our only concern is + // `test-name.revision` and `extension`. + for sibling in files_under_inspection.iter().filter(|f| { + f.extension().map(OsStr::to_str).flatten().is_some_and(|ext| EXTENSIONS.contains(&ext)) + }) { + let filename_components = sibling.to_str().unwrap().split('.').collect::>(); + let file_prefix = filename_components[0]; + + let Some((test_path, expected_revisions)) = test_info.get(file_prefix) else { + continue; + }; + + match filename_components[..] { + // Cannot have a revision component, skip. + [] | [_] => return, + [_, _] if !expected_revisions.is_empty() => { + // Found unrevisioned output files for a revisioned test. + tidy_error!( + bad, + "found unrevisioned output file `{}` for a revisioned test `{}`", + sibling.display(), + test_path.display(), + ); + } + [_, _] => return, + [_, found_revision, .., extension] => { + if !IGNORES.contains(&found_revision) + && !expected_revisions.contains(found_revision) + // This is from `//@ stderr-per-bitwidth` + && !(extension == "stderr" && ["32bit", "64bit"].contains(&found_revision)) + { + // Found some unexpected revision-esque component that is not a known + // compare-mode or expected revision. + tidy_error!( + bad, + "found output file `{}` for unexpected revision `{}` of test `{}`", + sibling.display(), + found_revision, + test_path.display() + ); + } + } + } + } + }); +} + +fn filter(path: &Path) -> bool { + filter_dirs(path) // ignore certain dirs + || (path.file_name().is_some_and(|name| name == "auxiliary")) // ignore auxiliary folder +} diff --git a/src/tools/tidy/src/walk.rs b/src/tools/tidy/src/walk.rs index 185e1f3209b10..851c21f1c0f05 100644 --- a/src/tools/tidy/src/walk.rs +++ b/src/tools/tidy/src/walk.rs @@ -86,3 +86,20 @@ pub(crate) fn walk_no_read( } } } + +// Walk through directories and skip symlinks. +pub(crate) fn walk_dir( + path: &Path, + skip: impl Send + Sync + 'static + Fn(&Path) -> bool, + f: &mut dyn FnMut(&DirEntry), +) { + 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.path().is_dir() { + f(&entry); + } + } + } +}