Drop compiletest legacy directive checks

jieyouxu · jieyouxu · commit b81a3c81998b · 2024-10-08T07:54:10.000Z
Sufficient time has passed (&gt; 6 months) since we migrated from `//` to
`//@`, so let's drop the legacy directive check as it causes friction
due to false positives.
diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs
@@ -5,9 +5,7 @@ use std::io::BufReader;
 use std::io::prelude::*;
 use std::path::{Path, PathBuf};
 use std::process::Command;
-use std::sync::OnceLock;
 
-use regex::Regex;
 use tracing::*;
 
 use crate::common::{Config, Debugger, FailMode, Mode, PassMode};
@@ -797,7 +795,6 @@ struct HeaderLine<'ln> {
 
 pub(crate) struct CheckDirectiveResult<'ln> {
     is_known_directive: bool,
-    directive_name: &'ln str,
     trailing_directive: Option<&'ln str>,
 }
 
@@ -832,11 +829,7 @@ pub(crate) fn check_directive<'a>(
     }
     .then_some(trailing);
 
-    CheckDirectiveResult {
-        is_known_directive: is_known(&directive_name),
-        directive_name: directive_ln,
-        trailing_directive,
-    }
+    CheckDirectiveResult { is_known_directive: is_known(&directive_name), trailing_directive }
 }
 
 fn iter_header(
@@ -851,16 +844,17 @@ fn iter_header(
         return;
     }
 
-    // Coverage tests in coverage-run mode always have these extra directives,
-    // without needing to specify them manually in every test file.
-    // (Some of the comments below have been copied over from the old
-    // `tests/run-make/coverage-reports/Makefile`, which no longer exists.)
+    // Coverage tests in coverage-run mode always have these extra directives, without needing to
+    // specify them manually in every test file. (Some of the comments below have been copied over
+    // from the old `tests/run-make/coverage-reports/Makefile`, which no longer exists.)
+    //
+    // FIXME(jieyouxu): I feel like there's a better way to do this, leaving for later.
     if mode == Mode::CoverageRun {
         let extra_directives: &[&str] = &[
             "needs-profiler-support",
-            // FIXME(pietroalbini): this test currently does not work on cross-compiled
-            // targets because remote-test is not capable of sending back the *.profraw
-            // files generated by the LLVM instrumentation.
+            // FIXME(pietroalbini): this test currently does not work on cross-compiled targets
+            // because remote-test is not capable of sending back the *.profraw files generated by
+            // the LLVM instrumentation.
             "ignore-cross-compile",
         ];
         // Process the extra implied directives, with a dummy line number of 0.
@@ -869,17 +863,13 @@ fn iter_header(
         }
     }
 
+    // NOTE(jieyouxu): once we get rid of `Makefile`s we can unconditionally check for `//@`.
     let comment = if testfile.extension().is_some_and(|e| e == "rs") { "//@" } else { "#" };
 
     let mut rdr = BufReader::with_capacity(1024, rdr);
     let mut ln = String::new();
     let mut line_number = 0;
 
-    // Match on error annotations like `//~ERROR`.
-    static REVISION_MAGIC_COMMENT_RE: OnceLock<Regex> = OnceLock::new();
-    let revision_magic_comment_re =
-        REVISION_MAGIC_COMMENT_RE.get_or_init(|| Regex::new("//(\\[.*\\])?~.*").unwrap());
-
     loop {
         line_number += 1;
         ln.clear();
@@ -892,85 +882,62 @@ fn iter_header(
         // with a warm page cache. Maybe with a cold one.
         let original_line = &ln;
         let ln = ln.trim();
+
+        // Assume that any directives will be found before the first module or function. This
+        // doesn't seem to be an optimization with a warm page cache. Maybe with a cold one.
+        // FIXME(jieyouxu): this will cause `//@` directives in the rest of the test file to
+        // not be checked.
         if ln.starts_with("fn") || ln.starts_with("mod") {
             return;
+        }
 
-        // First try to accept `ui_test` style comments (`//@`)
-        } else if let Some((header_revision, non_revisioned_directive_line)) =
-            line_directive(comment, ln)
-        {
-            // Perform unknown directive check on Rust files.
-            if testfile.extension().map(|e| e == "rs").unwrap_or(false) {
-                let directive_ln = non_revisioned_directive_line.trim();
-
-                let CheckDirectiveResult { is_known_directive, trailing_directive, .. } =
-                    check_directive(directive_ln, mode, ln);
-
-                if !is_known_directive {
-                    *poisoned = true;
-
-                    eprintln!(
-                        "error: detected unknown compiletest test directive `{}` in {}:{}",
-                        directive_ln,
-                        testfile.display(),
-                        line_number,
-                    );
-
-                    return;
-                }
+        let Some((header_revision, non_revisioned_directive_line)) = line_directive(comment, ln)
+        else {
+            continue;
+        };
 
-                if let Some(trailing_directive) = &trailing_directive {
-                    *poisoned = true;
+        // Perform unknown directive check on Rust files.
+        if testfile.extension().map(|e| e == "rs").unwrap_or(false) {
+            let directive_ln = non_revisioned_directive_line.trim();
 
-                    eprintln!(
-                        "error: detected trailing compiletest test directive `{}` in {}:{}\n \
-                          help: put the trailing directive in it's own line: `//@ {}`",
-                        trailing_directive,
-                        testfile.display(),
-                        line_number,
-                        trailing_directive,
-                    );
+            let CheckDirectiveResult { is_known_directive, trailing_directive } =
+                check_directive(directive_ln, mode, ln);
 
-                    return;
-                }
-            }
+            if !is_known_directive {
+                *poisoned = true;
 
-            it(HeaderLine {
-                line_number,
-                original_line,
-                header_revision,
-                directive: non_revisioned_directive_line,
-            });
-        // Then we try to check for legacy-style candidates, which are not the magic ~ERROR family
-        // error annotations.
-        } else if !revision_magic_comment_re.is_match(ln) {
-            let Some((_, rest)) = line_directive("//", ln) else {
-                continue;
-            };
+                eprintln!(
+                    "error: detected unknown compiletest test directive `{}` in {}:{}",
+                    directive_ln,
+                    testfile.display(),
+                    line_number,
+                );
 
-            if rest.trim_start().starts_with(':') {
-                // This is likely a markdown link:
-                // `[link_name]: https://example.org`
-                continue;
+                return;
             }
 
-            let rest = rest.trim_start();
-
-            let CheckDirectiveResult { is_known_directive, directive_name, .. } =
-                check_directive(rest, mode, ln);
-
-            if is_known_directive {
+            if let Some(trailing_directive) = &trailing_directive {
                 *poisoned = true;
+
                 eprintln!(
-                    "error: detected legacy-style directive {} in compiletest test: {}:{}, please use `ui_test`-style directives `//@` instead: {:#?}",
-                    directive_name,
+                    "error: detected trailing compiletest test directive `{}` in {}:{}\n \
+                      help: put the trailing directive in it's own line: `//@ {}`",
+                    trailing_directive,
                     testfile.display(),
                     line_number,
-                    line_directive("//", ln),
+                    trailing_directive,
                 );
+
                 return;
             }
         }
+
+        it(HeaderLine {
+            line_number,
+            original_line,
+            header_revision,
+            directive: non_revisioned_directive_line,
+        });
     }
 }
 
diff --git a/src/tools/compiletest/src/header/test-auxillary/known_legacy_directive.rs b/src/tools/compiletest/src/header/test-auxillary/known_legacy_directive.rs
diff --git a/src/tools/compiletest/src/header/tests.rs b/src/tools/compiletest/src/header/tests.rs
@@ -618,17 +618,6 @@ fn test_unknown_directive_check() {
     assert!(poisoned);
 }
 
-#[test]
-fn test_known_legacy_directive_check() {
-    let mut poisoned = false;
-    run_path(
-        &mut poisoned,
-        Path::new("a.rs"),
-        include_bytes!("./test-auxillary/known_legacy_directive.rs"),
-    );
-    assert!(poisoned);
-}
-
 #[test]
 fn test_known_directive_check_no_error() {
     let mut poisoned = false;
