Auto merge of #124706 - Zalathar:revision-checker, r=jieyouxu

Tidy check for test revisions that are mentioned but not declared

If a `[revision]` name appears in a test header directive or error annotation, but isn't declared in the `//@ revisions:` header, that is almost always a mistake.

In cases where a revision needs to be temporarily disabled, adding it to an `//@ unused-revision-names:` header will suppress these checks for that name.

Adding the wildcard name `*` to the unused list will suppress these checks for the entire file.

(None of the tests actually use `*`; it's just there because it was easy to add and could be handy as an escape hatch when dealing with other problems.)

---

Most of the existing problems discovered by this check were fairly straightforward to fix (or ignore); the trickiest cases are in `borrowck` tests.

src/tools/compiletest/src/header.rs

@@ -932,6 +932,8 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
     "test-mir-pass",
     "unset-exec-env",
     "unset-rustc-env",
+    // Used by the tidy check `unknown_revision`.
+    "unused-revision-names",
     // tidy-alphabetical-end
 ];
 

src/tools/tidy/src/iter_header.rs

@@ -3,6 +3,7 @@ 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) line_number: usize,
     pub(crate) revision: Option<&'ln str>,
     pub(crate) directive: &'ln str,
 }
@@ -11,7 +12,7 @@ pub(crate) struct HeaderLine<'ln> {
 ///
 /// 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() {
+    for (line_number, ln) in (1..).zip(contents.lines()) {
         let ln = ln.trim();
 
         // We're left with potentially `[rev]name: value`.
@@ -24,9 +25,9 @@ pub(crate) fn iter_header<'ln>(contents: &'ln str, it: &mut dyn FnMut(HeaderLine
                 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() });
+            it(HeaderLine { line_number, revision: Some(revision), directive: remainder.trim() });
         } else {
-            it(HeaderLine { revision: None, directive: remainder.trim() });
+            it(HeaderLine { line_number, revision: None, directive: remainder.trim() });
         }
     }
 }

src/tools/tidy/src/lib.rs

@@ -87,6 +87,7 @@ pub mod tests_placement;
 pub mod tests_revision_unpaired_stdout_stderr;
 pub mod ui_tests;
 pub mod unit_tests;
+pub mod unknown_revision;
 pub mod unstable_book;
 pub mod walk;
 pub mod x_version;

src/tools/tidy/src/main.rs

@@ -110,6 +110,7 @@ fn main() {
         check!(rustdoc_gui_tests, &tests_path);
         check!(rustdoc_css_themes, &librustdoc_path);
         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);

src/tools/tidy/src/target_specific_tests.rs

@@ -20,7 +20,7 @@ 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 |HeaderLine { revision, directive }| {
+        iter_header(content, &mut |HeaderLine { revision, directive, .. }| {
             if let Some(value) = directive.strip_prefix(LLVM_COMPONENTS_HEADER) {
                 let info = header_map.entry(revision).or_insert(RevisionInfo::default());
                 let comp_vec = info.llvm_components.get_or_insert(Vec::new());

src/tools/tidy/src/tests_revision_unpaired_stdout_stderr.rs

@@ -61,7 +61,7 @@ pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) {
             let contents = std::fs::read_to_string(test).unwrap();
 
             // Collect directives.
-            iter_header(&contents, &mut |HeaderLine { revision, directive }| {
+            iter_header(&contents, &mut |HeaderLine { revision, directive, .. }| {
                 // We're trying to *find* `//@ revision: xxx` directives themselves, not revisioned
                 // directives.
                 if revision.is_some() {

src/tools/tidy/src/unknown_revision.rs

@@ -0,0 +1,114 @@
+//! Checks that test revision names appearing in header directives and error
+//! annotations have actually been declared in `revisions`.
+
+// FIXME(jieyouxu) Ideally these checks would be integrated into compiletest's
+// own directive and revision handling, but for now they've been split out as a
+// separate `tidy` check to avoid making compiletest even messier.
+
+use std::collections::{BTreeSet, HashMap, HashSet};
+use std::path::Path;
+use std::sync::OnceLock;
+
+use ignore::DirEntry;
+use regex::Regex;
+
+use crate::iter_header::{iter_header, HeaderLine};
+use crate::walk::{filter_dirs, filter_not_rust, walk};
+
+pub fn check(tests_path: impl AsRef<Path>, bad: &mut bool) {
+    walk(
+        tests_path.as_ref(),
+        |path, is_dir| {
+            filter_dirs(path) || filter_not_rust(path) || {
+                // Auxiliary source files for incremental tests can refer to revisions
+                // declared by the main file, which this check doesn't handle.
+                is_dir && path.file_name().is_some_and(|name| name == "auxiliary")
+            }
+        },
+        &mut |entry, contents| visit_test_file(entry, contents, bad),
+    );
+}
+
+fn visit_test_file(entry: &DirEntry, contents: &str, bad: &mut bool) {
+    let mut revisions = HashSet::new();
+    let mut unused_revision_names = HashSet::new();
+
+    // Maps each mentioned revision to the first line it was mentioned on.
+    let mut mentioned_revisions = HashMap::<&str, usize>::new();
+    let mut add_mentioned_revision = |line_number: usize, revision| {
+        let first_line = mentioned_revisions.entry(revision).or_insert(line_number);
+        *first_line = (*first_line).min(line_number);
+    };
+
+    // Scan all `//@` headers to find declared revisions and mentioned revisions.
+    iter_header(contents, &mut |HeaderLine { line_number, revision, directive }| {
+        if let Some(revs) = directive.strip_prefix("revisions:") {
+            revisions.extend(revs.split_whitespace());
+        } else if let Some(revs) = directive.strip_prefix("unused-revision-names:") {
+            unused_revision_names.extend(revs.split_whitespace());
+        }
+
+        if let Some(revision) = revision {
+            add_mentioned_revision(line_number, revision);
+        }
+    });
+
+    // If a wildcard appears in `unused-revision-names`, skip all revision name
+    // checking for this file.
+    if unused_revision_names.contains(&"*") {
+        return;
+    }
+
+    // Scan all `//[rev]~` error annotations to find mentioned revisions.
+    for_each_error_annotation_revision(contents, &mut |ErrorAnnRev { line_number, revision }| {
+        add_mentioned_revision(line_number, revision);
+    });
+
+    let path = entry.path().display();
+
+    // 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,
+            "revision name [{rev}] appears in both `revisions` and `unused-revision-names` in {path}"
+        );
+    }
+
+    // Compute the set of revisions that were mentioned but not declared,
+    // sorted by the first line number they appear on.
+    let mut bad_revisions = mentioned_revisions
+        .into_iter()
+        .filter(|(rev, _)| !revisions.contains(rev) && !unused_revision_names.contains(rev))
+        .map(|(rev, line_number)| (line_number, rev))
+        .collect::<Vec<_>>();
+    bad_revisions.sort();
+
+    for (line_number, rev) in bad_revisions {
+        tidy_error!(bad, "unknown revision [{rev}] at {path}:{line_number}");
+    }
+}
+
+struct ErrorAnnRev<'a> {
+    line_number: usize,
+    revision: &'a str,
+}
+
+fn for_each_error_annotation_revision<'a>(
+    contents: &'a str,
+    callback: &mut dyn FnMut(ErrorAnnRev<'a>),
+) {
+    let error_regex = {
+        // Simplified from the regex used by `parse_expected` in `src/tools/compiletest/src/errors.rs`,
+        // because we only care about extracting revision names.
+        static RE: OnceLock<Regex> = OnceLock::new();
+        RE.get_or_init(|| Regex::new(r"//\[(?<revs>[^]]*)\]~").unwrap())
+    };
+
+    for (line_number, line) in (1..).zip(contents.lines()) {
+        let Some(captures) = error_regex.captures(line) else { continue };
+
+        for revision in captures.name("revs").unwrap().as_str().split(',') {
+            callback(ErrorAnnRev { line_number, revision });
+        }
+    }
+}

tests/ui/associated-types/bound-lifetime-constrained.ok.stderr

@@ -0,0 +1,8 @@
+error: fatal error triggered by #[rustc_error]
+  --> $DIR/bound-lifetime-constrained.rs:48:1
+   |
+LL | fn main() { }
+   | ^^^^^^^^^
+
+error: aborting due to 1 previous error
+

tests/ui/associated-types/bound-lifetime-constrained.rs

@@ -1,4 +1,4 @@
-//@ revisions: func object clause
+//@ revisions: func object clause ok
 
 #![allow(dead_code)]
 #![feature(rustc_attrs)]

tests/ui/borrowck/two-phase-activation-sharing-interference.nll_target.stderr

@@ -1,5 +1,5 @@
 error[E0502]: cannot borrow `x` as immutable because it is also borrowed as mutable
-  --> $DIR/two-phase-activation-sharing-interference.rs:28:15
+  --> $DIR/two-phase-activation-sharing-interference.rs:29:15
    |
 LL |     let y = &mut x;
    |             ------ mutable borrow occurs here
@@ -10,7 +10,7 @@ LL |     *y += 1;
    |     ------- mutable borrow later used here
 
 error[E0502]: cannot borrow `x` as immutable because it is also borrowed as mutable
-  --> $DIR/two-phase-activation-sharing-interference.rs:36:13
+  --> $DIR/two-phase-activation-sharing-interference.rs:37:13
    |
 LL |     let y = &mut x;
    |             ------ mutable borrow occurs here
@@ -32,7 +32,7 @@ LL |     *y += 1;
    |     ------- mutable borrow later used here
 
 error[E0502]: cannot borrow `x` as immutable because it is also borrowed as mutable
-  --> $DIR/two-phase-activation-sharing-interference.rs:58:14
+  --> $DIR/two-phase-activation-sharing-interference.rs:56:14
    |
 LL |     let y = &mut x;
    |             ------ mutable borrow occurs here

tests/ui/borrowck/two-phase-activation-sharing-interference.rs

@@ -1,6 +1,7 @@
 //@ revisions: nll_target
 
 // The following revisions are disabled due to missing support from two-phase beyond autorefs
+//@ unused-revision-names: nll_beyond
 //@[nll_beyond] compile-flags: -Z two-phase-beyond-autoref
 
 // This is an important corner case pointed out by Niko: one is
@@ -36,8 +37,7 @@ fn not_ok() {
     let z = &x;
     //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable
     *y += 1;
-    //[lxl_beyond]~^   ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
-    //[nll_beyond]~^^  ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
+    //[nll_beyond]~^   ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
     read(z);
 }
 
@@ -48,8 +48,6 @@ fn should_be_ok_with_nll() {
     //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable
     read(z);
     *y += 1;
-    //[lxl_beyond]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
-    // (okay with (generalized) nll today)
 }
 
 fn should_also_eventually_be_ok_with_nll() {
@@ -58,8 +56,6 @@ fn should_also_eventually_be_ok_with_nll() {
     let _z = &x;
     //[nll_target]~^ ERROR cannot borrow `x` as immutable because it is also borrowed as mutable
     *y += 1;
-    //[lxl_beyond]~^ ERROR cannot borrow `x` as mutable because it is also borrowed as immutable
-    // (okay with (generalized) nll today)
 }
 
 fn main() { }

tests/ui/borrowck/two-phase-allow-access-during-reservation.nll_target.stderr

@@ -1,5 +1,5 @@
 error[E0503]: cannot use `i` because it was mutably borrowed
-  --> $DIR/two-phase-allow-access-during-reservation.rs:26:19
+  --> $DIR/two-phase-allow-access-during-reservation.rs:27:19
    |
 LL |     /*1*/ let p = &mut i; // (reservation of `i` starts here)
    |                   ------ `i` is borrowed here
@@ -11,7 +11,7 @@ LL |     /*3*/ *p += 1;        // (mutable borrow of `i` starts here, since `p`
    |           ------- borrow later used here
 
 error[E0503]: cannot use `i` because it was mutably borrowed
-  --> $DIR/two-phase-allow-access-during-reservation.rs:31:19
+  --> $DIR/two-phase-allow-access-during-reservation.rs:32:19
    |
 LL |     /*1*/ let p = &mut i; // (reservation of `i` starts here)
    |                   ------ `i` is borrowed here

tests/ui/borrowck/two-phase-allow-access-during-reservation.rs

@@ -1,6 +1,7 @@
 //@ revisions: nll_target
 
 // The following revisions are disabled due to missing support for two_phase_beyond_autoref
+//@ unused-revision-names: nll_beyond
 //@[nll_beyond] compile-flags: -Z two_phase_beyond_autoref
 
 // This is the second counter-example from Niko's blog post

tests/ui/borrowck/two-phase-nonrecv-autoref.base.stderr

@@ -1,5 +1,5 @@
 error[E0499]: cannot borrow `*f` as mutable more than once at a time
-  --> $DIR/two-phase-nonrecv-autoref.rs:50:11
+  --> $DIR/two-phase-nonrecv-autoref.rs:51:11
    |
 LL |         f(f(10));
    |         - ^ second mutable borrow occurs here
@@ -8,7 +8,7 @@ LL |         f(f(10));
    |         first borrow later used by call
 
 error[E0382]: use of moved value: `f`
-  --> $DIR/two-phase-nonrecv-autoref.rs:57:11
+  --> $DIR/two-phase-nonrecv-autoref.rs:58:11
    |
 LL |     fn twice_ten_so<F: FnOnce(i32) -> i32>(f: Box<F>) {
    |                                            - move occurs because `f` has type `Box<F>`, which does not implement the `Copy` trait
@@ -18,7 +18,7 @@ LL |         f(f(10));
    |         value moved here
 
 error[E0499]: cannot borrow `*f` as mutable more than once at a time
-  --> $DIR/two-phase-nonrecv-autoref.rs:62:11
+  --> $DIR/two-phase-nonrecv-autoref.rs:63:11
    |
 LL |         f(f(10));
    |         - ^ second mutable borrow occurs here
@@ -27,7 +27,7 @@ LL |         f(f(10));
    |         first borrow later used by call
 
 error[E0382]: use of moved value: `f`
-  --> $DIR/two-phase-nonrecv-autoref.rs:69:11
+  --> $DIR/two-phase-nonrecv-autoref.rs:70:11
    |
 LL |     fn twice_ten_oo(f: Box<dyn FnOnce(i32) -> i32>) {
    |                     - move occurs because `f` has type `Box<dyn FnOnce(i32) -> i32>`, which does not implement the `Copy` trait
@@ -37,7 +37,7 @@ LL |         f(f(10));
    |         value moved here
 
 error[E0502]: cannot borrow `a` as immutable because it is also borrowed as mutable
-  --> $DIR/two-phase-nonrecv-autoref.rs:107:27
+  --> $DIR/two-phase-nonrecv-autoref.rs:108:27
    |
 LL |     double_access(&mut a, &a);
    |     ------------- ------  ^^ immutable borrow occurs here
@@ -46,7 +46,7 @@ LL |     double_access(&mut a, &a);
    |     mutable borrow later used by call
 
 error[E0502]: cannot borrow `i` as immutable because it is also borrowed as mutable
-  --> $DIR/two-phase-nonrecv-autoref.rs:132:7
+  --> $DIR/two-phase-nonrecv-autoref.rs:133:7
    |
 LL |     i[i[3]] = 4;
    |     --^----
@@ -56,18 +56,18 @@ LL |     i[i[3]] = 4;
    |     mutable borrow occurs here
    |
 help: try adding a local storing this...
-  --> $DIR/two-phase-nonrecv-autoref.rs:132:8
+  --> $DIR/two-phase-nonrecv-autoref.rs:133:8
    |
 LL |     i[i[3]] = 4;
    |        ^^^
 help: ...and then using that local here
-  --> $DIR/two-phase-nonrecv-autoref.rs:132:6
+  --> $DIR/two-phase-nonrecv-autoref.rs:133:6
    |
 LL |     i[i[3]] = 4;
    |      ^^^^^^
 
 error[E0502]: cannot borrow `i` as immutable because it is also borrowed as mutable
-  --> $DIR/two-phase-nonrecv-autoref.rs:138:7
+  --> $DIR/two-phase-nonrecv-autoref.rs:139:7
    |
 LL |     i[i[3]] = i[4];
    |     --^----
@@ -77,12 +77,12 @@ LL |     i[i[3]] = i[4];
    |     mutable borrow occurs here
    |
 help: try adding a local storing this...
-  --> $DIR/two-phase-nonrecv-autoref.rs:138:8
+  --> $DIR/two-phase-nonrecv-autoref.rs:139:8
    |
 LL |     i[i[3]] = i[4];
    |        ^^^
 help: ...and then using that local here
-  --> $DIR/two-phase-nonrecv-autoref.rs:138:6
+  --> $DIR/two-phase-nonrecv-autoref.rs:139:6
    |
 LL |     i[i[3]] = i[4];
    |      ^^^^^^

tests/ui/borrowck/two-phase-nonrecv-autoref.rs

@@ -1,5 +1,6 @@
 //@ revisions: base
 
+//@ unused-revision-names: g2p
 //@[g2p]compile-flags: -Z two-phase-beyond-autoref
 // the above revision is disabled until two-phase-beyond-autoref support is better
 

tests/ui/borrowck/two-phase-reservation-sharing-interference.nll_target.stderr

@@ -1,5 +1,5 @@
 error[E0502]: cannot borrow `vec` as mutable because it is also borrowed as immutable
-  --> $DIR/two-phase-reservation-sharing-interference.rs:32:17
+  --> $DIR/two-phase-reservation-sharing-interference.rs:33:17
    |
 LL |         let shared = &vec;
    |                      ---- immutable borrow occurs here

tests/ui/borrowck/two-phase-reservation-sharing-interference.rs

@@ -1,6 +1,7 @@
 //@ revisions: nll_target
 
 // The nll_beyond revision is disabled due to missing support from two-phase beyond autorefs
+//@ unused-revision-names: nll_beyond
 //@[nll_beyond]compile-flags: -Z two-phase-beyond-autoref
 //@[nll_beyond]should-fail