Auto merge of rust-lang#120881 - jieyouxu:migrate-ui-test-directives,…

… r=oli-obk

Migrate ui tests from legacy compiletest-style directives `//` to `ui_test`-style directives `//@`

## Preface

There's an on-going effort to rewrite parts of or the entirety of compiletest
(<rust-lang/compiler-team#536>). A step towards this involve migrating
ui tests to use the [`ui_test`](https://github.com/oli-obk/ui_test) framework, which involves
changing compiletest directives in `// <directive-name>` style to `ui_test` `//@ <directive-name>`
style (rust-lang/compiler-team#512).

This PR aims to implement the directive-style change from `//` to `//`@`` for ui tests only and
make compiletest only accept `//`@`` directives in ui test mode (only).

## Key Changes

1. All ui test `//` directives are replaced by `//`@`` directives.
2. Only accept `//`@`` directives for ui tests (specifically ui tests).
3. Errors if a comment could be interpreted as a legacy-style `//` directive.

## Diff Generation

The diff is generated by:

- Collecting directives from ui tests via hacking on compiletest.
- Using a migration tool to replace `//` directives in ui tests with `//`@`.`

### Reproduction Steps

0. Delete the temporary directory `$RUSTC_REPO_PATH/build/x86_64-apple-darwin/test/ui/__directive_lines` and the temporary file `$RUSTC_REPO_PATH/build/x86_64-apple-darwin/test/ui/__directive_lines.txt` (if you ran the collection script before).
1. Use the <https://github.com/jieyouxu/rust/tree/collect-test-directives> collect-test-directives
   script, which outputs temporary files recording headers occuring in each ui test.
   - You need to checkout this branch: `git checkout collect-test-directives`.
   - You might need to rebase on lastest master and ensure there are no conflicts.
   - You likely need to run `./x test tests/ui --stage 1 --force-rerun` to generate the temporary
     files consistently.
2. Checkout the `migrate-ui-test-directives` branch.
4. Run the migration tool <https://github.com/jieyouxu/compiletest-ui_test-header-migration>.
    - You will need to first generate a `migration_config.toml` via `cargo run -- generate-config` under `$CWD`.
    - Then, update `manual_directives = ["// should-fail"]` in `migration_config.toml`. This is required because the collection script doesn't deal with some special meta ui tests and there are no other `// should-fail` occurrences.
5. Check that the migration at least does not cause UI test failures if you change compiletest to
   accept `//`@`` directives for ui tests only.
   - `RUSTC_TEST_FAIL_FAST=1 ./x test tests/ui --stage 1 --bless`
6. Confirm that there is no difference after running the migration tool when you are on the
   `migrate-ui-test-directives` branch.

## Next Steps

- [x] ~~Need to implement some kind of warning or tidy script to help contributors catch old-style
  `// <directive-name>` directives, while only accepting `ui_test`-style `//@ <directive-name>`
  directives.~~ An error is emitted if a comment that could be interpreted as legacy-style test directive is encountered.
- [ ] Need to properly document this change in e.g. rustc-dev-guide (rust-lang/rustc-dev-guide#1885).
    - [x] Add a `README.md` to `tests/ui` describing the directive style change.

src/tools/compiletest/src/header/cfg.rs

@@ -221,8 +221,10 @@ pub(super) fn parse_cfg_name_directive<'a>(
     }
 
     if prefix == "ignore" && outcome == MatchOutcome::Invalid {
-        // Don't error out for ignore-tidy-* diretives, as those are not handled by compiletest.
-        if name.starts_with("tidy-") {
+        // `ignore-tidy` and `ignore-tidy-*` directives are *not* compiletest directives,
+        // they are handled by the tidy tool. But legacy compiletest treats them as
+        // external matches (in non-ui modes).
+        if name.starts_with("tidy") && config.mode != Mode::Ui {
             outcome = MatchOutcome::External;
         }
 

src/tools/compiletest/src/header/tests.rs

@@ -210,15 +210,15 @@ fn should_fail() {
 
     let d = make_test_description(&config, tn.clone(), p, std::io::Cursor::new(""), None);
     assert_eq!(d.should_panic, test::ShouldPanic::No);
-    let d = make_test_description(&config, tn, p, std::io::Cursor::new("// should-fail"), None);
+    let d = make_test_description(&config, tn, p, std::io::Cursor::new("//@ should-fail"), None);
     assert_eq!(d.should_panic, test::ShouldPanic::Yes);
 }
 
 #[test]
 fn revisions() {
     let config: Config = cfg().build();
 
-    assert_eq!(parse_rs(&config, "// revisions: a b c").revisions, vec!["a", "b", "c"],);
+    assert_eq!(parse_rs(&config, "//@ revisions: a b c").revisions, vec!["a", "b", "c"],);
     assert_eq!(
         parse_makefile(&config, "# revisions: hello there").revisions,
         vec!["hello", "there"],
@@ -233,8 +233,8 @@ fn aux_build() {
         parse_rs(
             &config,
             r"
-        // aux-build: a.rs
-        // aux-build: b.rs
+        //@ aux-build: a.rs
+        //@ aux-build: b.rs
         "
         )
         .aux,
@@ -245,128 +245,128 @@ fn aux_build() {
 #[test]
 fn llvm_version() {
     let config: Config = cfg().llvm_version("8.1.2").build();
-    assert!(check_ignore(&config, "// min-llvm-version: 9.0"));
+    assert!(check_ignore(&config, "//@ min-llvm-version: 9.0"));
 
     let config: Config = cfg().llvm_version("9.0.1").build();
-    assert!(check_ignore(&config, "// min-llvm-version: 9.2"));
+    assert!(check_ignore(&config, "//@ min-llvm-version: 9.2"));
 
     let config: Config = cfg().llvm_version("9.3.1").build();
-    assert!(!check_ignore(&config, "// min-llvm-version: 9.2"));
+    assert!(!check_ignore(&config, "//@ min-llvm-version: 9.2"));
 
     let config: Config = cfg().llvm_version("10.0.0").build();
-    assert!(!check_ignore(&config, "// min-llvm-version: 9.0"));
+    assert!(!check_ignore(&config, "//@ min-llvm-version: 9.0"));
 }
 
 #[test]
 fn system_llvm_version() {
     let config: Config = cfg().system_llvm(true).llvm_version("17.0.0").build();
-    assert!(check_ignore(&config, "// min-system-llvm-version: 18.0"));
+    assert!(check_ignore(&config, "//@ min-system-llvm-version: 18.0"));
 
     let config: Config = cfg().system_llvm(true).llvm_version("18.0.0").build();
-    assert!(!check_ignore(&config, "// min-system-llvm-version: 18.0"));
+    assert!(!check_ignore(&config, "//@ min-system-llvm-version: 18.0"));
 
     let config: Config = cfg().llvm_version("17.0.0").build();
-    assert!(!check_ignore(&config, "// min-system-llvm-version: 18.0"));
+    assert!(!check_ignore(&config, "//@ min-system-llvm-version: 18.0"));
 }
 
 #[test]
 fn ignore_target() {
     let config: Config = cfg().target("x86_64-unknown-linux-gnu").build();
 
-    assert!(check_ignore(&config, "// ignore-x86_64-unknown-linux-gnu"));
-    assert!(check_ignore(&config, "// ignore-x86_64"));
-    assert!(check_ignore(&config, "// ignore-linux"));
-    assert!(check_ignore(&config, "// ignore-gnu"));
-    assert!(check_ignore(&config, "// ignore-64bit"));
+    assert!(check_ignore(&config, "//@ ignore-x86_64-unknown-linux-gnu"));
+    assert!(check_ignore(&config, "//@ ignore-x86_64"));
+    assert!(check_ignore(&config, "//@ ignore-linux"));
+    assert!(check_ignore(&config, "//@ ignore-gnu"));
+    assert!(check_ignore(&config, "//@ ignore-64bit"));
 
-    assert!(!check_ignore(&config, "// ignore-x86"));
-    assert!(!check_ignore(&config, "// ignore-windows"));
-    assert!(!check_ignore(&config, "// ignore-msvc"));
-    assert!(!check_ignore(&config, "// ignore-32bit"));
+    assert!(!check_ignore(&config, "//@ ignore-x86"));
+    assert!(!check_ignore(&config, "//@ ignore-windows"));
+    assert!(!check_ignore(&config, "//@ ignore-msvc"));
+    assert!(!check_ignore(&config, "//@ ignore-32bit"));
 }
 
 #[test]
 fn only_target() {
     let config: Config = cfg().target("x86_64-pc-windows-gnu").build();
 
-    assert!(check_ignore(&config, "// only-x86"));
-    assert!(check_ignore(&config, "// only-linux"));
-    assert!(check_ignore(&config, "// only-msvc"));
-    assert!(check_ignore(&config, "// only-32bit"));
+    assert!(check_ignore(&config, "//@ only-x86"));
+    assert!(check_ignore(&config, "//@ only-linux"));
+    assert!(check_ignore(&config, "//@ only-msvc"));
+    assert!(check_ignore(&config, "//@ only-32bit"));
 
-    assert!(!check_ignore(&config, "// only-x86_64-pc-windows-gnu"));
-    assert!(!check_ignore(&config, "// only-x86_64"));
-    assert!(!check_ignore(&config, "// only-windows"));
-    assert!(!check_ignore(&config, "// only-gnu"));
-    assert!(!check_ignore(&config, "// only-64bit"));
+    assert!(!check_ignore(&config, "//@ only-x86_64-pc-windows-gnu"));
+    assert!(!check_ignore(&config, "//@ only-x86_64"));
+    assert!(!check_ignore(&config, "//@ only-windows"));
+    assert!(!check_ignore(&config, "//@ only-gnu"));
+    assert!(!check_ignore(&config, "//@ only-64bit"));
 }
 
 #[test]
 fn stage() {
     let config: Config = cfg().stage_id("stage1-x86_64-unknown-linux-gnu").build();
 
-    assert!(check_ignore(&config, "// ignore-stage1"));
-    assert!(!check_ignore(&config, "// ignore-stage2"));
+    assert!(check_ignore(&config, "//@ ignore-stage1"));
+    assert!(!check_ignore(&config, "//@ ignore-stage2"));
 }
 
 #[test]
 fn cross_compile() {
     let config: Config = cfg().host("x86_64-apple-darwin").target("wasm32-unknown-unknown").build();
-    assert!(check_ignore(&config, "// ignore-cross-compile"));
+    assert!(check_ignore(&config, "//@ ignore-cross-compile"));
 
     let config: Config = cfg().host("x86_64-apple-darwin").target("x86_64-apple-darwin").build();
-    assert!(!check_ignore(&config, "// ignore-cross-compile"));
+    assert!(!check_ignore(&config, "//@ ignore-cross-compile"));
 }
 
 #[test]
 fn debugger() {
     let mut config = cfg().build();
     config.debugger = None;
-    assert!(!check_ignore(&config, "// ignore-cdb"));
+    assert!(!check_ignore(&config, "//@ ignore-cdb"));
 
     config.debugger = Some(Debugger::Cdb);
-    assert!(check_ignore(&config, "// ignore-cdb"));
+    assert!(check_ignore(&config, "//@ ignore-cdb"));
 
     config.debugger = Some(Debugger::Gdb);
-    assert!(check_ignore(&config, "// ignore-gdb"));
+    assert!(check_ignore(&config, "//@ ignore-gdb"));
 
     config.debugger = Some(Debugger::Lldb);
-    assert!(check_ignore(&config, "// ignore-lldb"));
+    assert!(check_ignore(&config, "//@ ignore-lldb"));
 }
 
 #[test]
 fn git_hash() {
     let config: Config = cfg().git_hash(false).build();
-    assert!(check_ignore(&config, "// needs-git-hash"));
+    assert!(check_ignore(&config, "//@ needs-git-hash"));
 
     let config: Config = cfg().git_hash(true).build();
-    assert!(!check_ignore(&config, "// needs-git-hash"));
+    assert!(!check_ignore(&config, "//@ needs-git-hash"));
 }
 
 #[test]
 fn sanitizers() {
     // Target that supports all sanitizers:
     let config: Config = cfg().target("x86_64-unknown-linux-gnu").build();
-    assert!(!check_ignore(&config, "// needs-sanitizer-address"));
-    assert!(!check_ignore(&config, "// needs-sanitizer-leak"));
-    assert!(!check_ignore(&config, "// needs-sanitizer-memory"));
-    assert!(!check_ignore(&config, "// needs-sanitizer-thread"));
+    assert!(!check_ignore(&config, "//@ needs-sanitizer-address"));
+    assert!(!check_ignore(&config, "//@ needs-sanitizer-leak"));
+    assert!(!check_ignore(&config, "//@ needs-sanitizer-memory"));
+    assert!(!check_ignore(&config, "//@ needs-sanitizer-thread"));
 
     // Target that doesn't support sanitizers:
     let config: Config = cfg().target("wasm32-unknown-emscripten").build();
-    assert!(check_ignore(&config, "// needs-sanitizer-address"));
-    assert!(check_ignore(&config, "// needs-sanitizer-leak"));
-    assert!(check_ignore(&config, "// needs-sanitizer-memory"));
-    assert!(check_ignore(&config, "// needs-sanitizer-thread"));
+    assert!(check_ignore(&config, "//@ needs-sanitizer-address"));
+    assert!(check_ignore(&config, "//@ needs-sanitizer-leak"));
+    assert!(check_ignore(&config, "//@ needs-sanitizer-memory"));
+    assert!(check_ignore(&config, "//@ needs-sanitizer-thread"));
 }
 
 #[test]
 fn profiler_support() {
     let config: Config = cfg().profiler_support(false).build();
-    assert!(check_ignore(&config, "// needs-profiler-support"));
+    assert!(check_ignore(&config, "//@ needs-profiler-support"));
 
     let config: Config = cfg().profiler_support(true).build();
-    assert!(!check_ignore(&config, "// needs-profiler-support"));
+    assert!(!check_ignore(&config, "//@ needs-profiler-support"));
 }
 
 #[test]
@@ -382,21 +382,21 @@ fn asm_support() {
     for (target, has_asm) in asms {
         let config = cfg().target(target).build();
         assert_eq!(config.has_asm_support(), has_asm);
-        assert_eq!(check_ignore(&config, "// needs-asm-support"), !has_asm)
+        assert_eq!(check_ignore(&config, "//@ needs-asm-support"), !has_asm)
     }
 }
 
 #[test]
 fn channel() {
     let config: Config = cfg().channel("beta").build();
 
-    assert!(check_ignore(&config, "// ignore-beta"));
-    assert!(check_ignore(&config, "// only-nightly"));
-    assert!(check_ignore(&config, "// only-stable"));
+    assert!(check_ignore(&config, "//@ ignore-beta"));
+    assert!(check_ignore(&config, "//@ only-nightly"));
+    assert!(check_ignore(&config, "//@ only-stable"));
 
-    assert!(!check_ignore(&config, "// only-beta"));
-    assert!(!check_ignore(&config, "// ignore-nightly"));
-    assert!(!check_ignore(&config, "// ignore-stable"));
+    assert!(!check_ignore(&config, "//@ only-beta"));
+    assert!(!check_ignore(&config, "//@ ignore-nightly"));
+    assert!(!check_ignore(&config, "//@ ignore-stable"));
 }
 
 #[test]
@@ -418,7 +418,7 @@ fn test_extract_version_range() {
 #[should_panic(expected = "Duplicate revision: `rpass1` in line ` rpass1 rpass1`")]
 fn test_duplicate_revisions() {
     let config: Config = cfg().build();
-    parse_rs(&config, "// revisions: rpass1 rpass1");
+    parse_rs(&config, "//@ revisions: rpass1 rpass1");
 }
 
 #[test]
@@ -432,7 +432,7 @@ fn ignore_arch() {
     for (target, arch) in archs {
         let config: Config = cfg().target(target).build();
         assert!(config.matches_arch(arch), "{target} {arch}");
-        assert!(check_ignore(&config, &format!("// ignore-{arch}")));
+        assert!(check_ignore(&config, &format!("//@ ignore-{arch}")));
     }
 }
 
@@ -447,7 +447,7 @@ fn matches_os() {
     for (target, os) in oss {
         let config = cfg().target(target).build();
         assert!(config.matches_os(os), "{target} {os}");
-        assert!(check_ignore(&config, &format!("// ignore-{os}")));
+        assert!(check_ignore(&config, &format!("//@ ignore-{os}")));
     }
 }
 
@@ -461,7 +461,7 @@ fn matches_env() {
     for (target, env) in envs {
         let config: Config = cfg().target(target).build();
         assert!(config.matches_env(env), "{target} {env}");
-        assert!(check_ignore(&config, &format!("// ignore-{env}")));
+        assert!(check_ignore(&config, &format!("//@ ignore-{env}")));
     }
 }
 
@@ -475,7 +475,7 @@ fn matches_abi() {
     for (target, abi) in abis {
         let config: Config = cfg().target(target).build();
         assert!(config.matches_abi(abi), "{target} {abi}");
-        assert!(check_ignore(&config, &format!("// ignore-{abi}")));
+        assert!(check_ignore(&config, &format!("//@ ignore-{abi}")));
     }
 }
 
@@ -491,7 +491,7 @@ fn is_big_endian() {
     for (target, is_big) in endians {
         let config = cfg().target(target).build();
         assert_eq!(config.is_big_endian(), is_big, "{target} {is_big}");
-        assert_eq!(check_ignore(&config, "// ignore-endian-big"), is_big);
+        assert_eq!(check_ignore(&config, "//@ ignore-endian-big"), is_big);
     }
 }
 
@@ -506,9 +506,9 @@ fn pointer_width() {
     for (target, width) in widths {
         let config: Config = cfg().target(target).build();
         assert_eq!(config.get_pointer_width(), width, "{target} {width}");
-        assert_eq!(check_ignore(&config, "// ignore-16bit"), width == 16);
-        assert_eq!(check_ignore(&config, "// ignore-32bit"), width == 32);
-        assert_eq!(check_ignore(&config, "// ignore-64bit"), width == 64);
+        assert_eq!(check_ignore(&config, "//@ ignore-16bit"), width == 16);
+        assert_eq!(check_ignore(&config, "//@ ignore-32bit"), width == 32);
+        assert_eq!(check_ignore(&config, "//@ ignore-64bit"), width == 64);
     }
 }
 
@@ -534,7 +534,7 @@ fn wasm_special() {
     for (target, pattern, ignore) in ignores {
         let config: Config = cfg().target(target).build();
         assert_eq!(
-            check_ignore(&config, &format!("// ignore-{pattern}")),
+            check_ignore(&config, &format!("//@ ignore-{pattern}")),
             ignore,
             "{target} {pattern}"
         );
@@ -555,8 +555,8 @@ fn families() {
         assert!(config.matches_family(family));
         let other = if family == "windows" { "unix" } else { "windows" };
         assert!(!config.matches_family(other));
-        assert!(check_ignore(&config, &format!("// ignore-{family}")));
-        assert!(!check_ignore(&config, &format!("// ignore-{other}")));
+        assert!(check_ignore(&config, &format!("//@ ignore-{family}")));
+        assert!(!check_ignore(&config, &format!("//@ ignore-{other}")));
     }
 }
 
@@ -566,10 +566,17 @@ fn ignore_mode() {
         // Indicate profiler support so that "coverage-run" tests aren't skipped.
         let config: Config = cfg().mode(mode).profiler_support(true).build();
         let other = if mode == "coverage-run" { "coverage-map" } else { "coverage-run" };
+
         assert_ne!(mode, other);
         assert_eq!(config.mode, Mode::from_str(mode).unwrap());
         assert_ne!(config.mode, Mode::from_str(other).unwrap());
-        assert!(check_ignore(&config, &format!("// ignore-mode-{mode}")));
-        assert!(!check_ignore(&config, &format!("// ignore-mode-{other}")));
+
+        if mode == "ui" {
+            assert!(check_ignore(&config, &format!("//@ ignore-mode-{mode}")));
+            assert!(!check_ignore(&config, &format!("//@ ignore-mode-{other}")));
+        } else {
+            assert!(check_ignore(&config, &format!("// ignore-mode-{mode}")));
+            assert!(!check_ignore(&config, &format!("// ignore-mode-{other}")));
+        }
     }
 }

src/tools/tidy/src/style.rs

@@ -55,11 +55,14 @@ const ANNOTATIONS_TO_IGNORE: &[&str] = &[
     "// CHECK",
     "// EMIT_MIR",
     "// compile-flags",
+    "//@ compile-flags",
     "// error-pattern",
+    "//@ error-pattern",
     "// gdb",
     "// lldb",
     "// cdb",
     "// normalize-stderr-test",
+    "//@ normalize-stderr-test",
 ];
 
 // Intentionally written in decimal rather than hex
@@ -128,7 +131,15 @@ fn should_ignore(line: &str) -> bool {
     // This mirrors the regex in src/tools/compiletest/src/runtest.rs, please
     // update both if either are changed.
     let re = Regex::new("\\s*//(\\[.*\\])?~.*").unwrap();
-    re.is_match(line) || ANNOTATIONS_TO_IGNORE.iter().any(|a| line.contains(a))
+    // For `ui_test`-style UI test directives, also ignore
+    // - `//@[rev] compile-flags`
+    // - `//@[rev] normalize-stderr-test`
+    let ui_test_long_directives =
+        Regex::new("\\s*//@(\\[.*\\]) (compile-flags|normalize-stderr-test|error-pattern).*")
+            .unwrap();
+    re.is_match(line)
+        || ANNOTATIONS_TO_IGNORE.iter().any(|a| line.contains(a))
+        || ui_test_long_directives.is_match(line)
 }
 
 /// Returns `true` if `line` is allowed to be longer than the normal limit.

src/tools/tidy/src/ui_tests.rs

@@ -14,8 +14,9 @@ use std::path::{Path, PathBuf};
 // #73494.
 const ENTRY_LIMIT: usize = 900;
 // FIXME: The following limits should be reduced eventually.
+
 const ISSUES_ENTRY_LIMIT: usize = 1781;
-const ROOT_ENTRY_LIMIT: usize = 870;
+const ROOT_ENTRY_LIMIT: usize = 871;
 
 const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
     "rs",     // test source files