Rework directive checks and add tests

Author: jieyouxu

src/tools/compiletest/src/header.rs

@@ -14,7 +14,7 @@ use crate::common::{Config, Debugger, FailMode, Mode, PassMode};
 use crate::header::cfg::parse_cfg_name_directive;
 use crate::header::cfg::MatchOutcome;
 use crate::header::needs::CachedNeedsConditions;
-use crate::util::find_best_match_for_name;
+use crate::util::edit_distance::find_best_match_for_name;
 use crate::{extract_cdb_version, extract_gdb_version};
 
 mod cfg;
@@ -673,8 +673,8 @@ pub fn line_directive<'line>(
 /// This is generated by collecting directives from ui tests and then extracting their directive
 /// names. This is **not** an exhaustive list of all possible directives. Instead, this is a
 /// best-effort approximation for diagnostics.
-// tidy-alphabetical-start
 const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
+    // tidy-alphabetical-start
     "assembly-output",
     "aux-build",
     "aux-crate",
@@ -871,8 +871,8 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
     "unit-test",
     "unset-exec-env",
     "unset-rustc-env",
+    // tidy-alphabetical-end
 ];
-// tidy-alphabetical-end
 
 /// The broken-down contents of a line containing a test header directive,
 /// which [`iter_header`] passes to its callback function.
@@ -902,6 +902,22 @@ struct HeaderLine<'ln> {
     directive: &'ln str,
 }
 
+pub(crate) struct CheckDirectiveResult<'ln> {
+    is_known_directive: bool,
+    directive_name: &'ln str,
+}
+
+// Returns `(is_known_directive, directive_name)`.
+pub(crate) fn check_directive(directive_ln: &str) -> CheckDirectiveResult<'_> {
+    let directive_name =
+        directive_ln.split_once([':', ' ']).map(|(pre, _)| pre).unwrap_or(directive_ln);
+
+    CheckDirectiveResult {
+        is_known_directive: KNOWN_DIRECTIVE_NAMES.contains(&directive_name),
+        directive_name: directive_ln,
+    }
+}
+
 fn iter_header(
     mode: Mode,
     _suite: &str,
@@ -941,6 +957,7 @@ fn iter_header(
     let mut ln = String::new();
     let mut line_number = 0;
 
+    // Match on error annotations like `//~ERROR`.
     static REVISION_MAGIC_COMMENT_RE: Lazy<Regex> =
         Lazy::new(|| Regex::new("//(\\[.*\\])?~.*").unwrap());
 
@@ -959,33 +976,19 @@ fn iter_header(
         if ln.starts_with("fn") || ln.starts_with("mod") {
             return;
 
-        // First try to accept `ui_test` style comments
-        } else if let Some((header_revision, original_directive_line)) = line_directive(comment, ln)
+        // First try to accept `ui_test` style comments (`//@`)
+        } else if let Some((header_revision, non_revisioned_directive_line)) =
+            line_directive(comment, ln)
         {
-            // Let Makefiles and non-rs files just get handled in the regular way.
-            if testfile.extension().map(|e| e != "rs").unwrap_or(true) {
-                it(HeaderLine {
-                    line_number,
-                    original_line,
-                    header_revision,
-                    directive: original_directive_line,
-                });
-                continue;
-            }
+            // 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 directive_ln = original_directive_line.trim();
+                let CheckDirectiveResult { is_known_directive, directive_name } = check_directive(directive_ln);
 
-            if let Some((directive_name, _)) = directive_ln.split_once([':', ' ']) {
-                if KNOWN_DIRECTIVE_NAMES.contains(&directive_name) {
-                    it(HeaderLine {
-                        line_number,
-                        original_line,
-                        header_revision,
-                        directive: original_directive_line,
-                    });
-                    continue;
-                } else {
+                if !is_known_directive {
                     *poisoned = true;
+
                     eprintln!(
                         "error: detected unknown compiletest test directive `{}` in {}:{}",
                         directive_ln,
@@ -998,32 +1001,19 @@ fn iter_header(
                     {
                         eprintln!("help: did you mean `{}` instead?", suggestion);
                     }
-                    return;
-                }
-            } else if KNOWN_DIRECTIVE_NAMES.contains(&directive_ln) {
-                it(HeaderLine {
-                    line_number,
-                    original_line,
-                    header_revision,
-                    directive: original_directive_line,
-                });
-                continue;
-            } else {
-                *poisoned = true;
-                eprintln!(
-                    "error: detected unknown compiletest test directive `{}` in {}:{}",
-                    directive_ln,
-                    testfile.display(),
-                    line_number,
-                );
 
-                if let Some(suggestion) =
-                    find_best_match_for_name(KNOWN_DIRECTIVE_NAMES, directive_ln, None)
-                {
-                    eprintln!("help: did you mean `{}` instead?", suggestion);
+                    return;
                 }
-                return;
             }
+
+            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;
@@ -1037,34 +1027,18 @@ fn iter_header(
 
             let rest = rest.trim_start();
 
-            for candidate in KNOWN_DIRECTIVE_NAMES.iter() {
-                if rest.starts_with(candidate) {
-                    let Some(prefix_removed) = rest.strip_prefix(candidate) else {
-                        // We have a comment that's *successfully* parsed as an legacy-style
-                        // directive. We emit an error here to warn the user.
-                        *poisoned = true;
-                        eprintln!(
-                            "error: detected legacy-style directives in compiletest test: {}:{}, please use `ui_test`-style directives `//@` instead:{:#?}",
-                            testfile.display(),
-                            line_number,
-                            line_directive("//", ln),
-                        );
-                        return;
-                    };
+            let CheckDirectiveResult { is_known_directive, directive_name } = check_directive(rest);
 
-                    if prefix_removed.starts_with([' ', ':']) {
-                        // We have a comment that's *successfully* parsed as an legacy-style
-                        // directive. We emit an error here to warn the user.
-                        *poisoned = true;
-                        eprintln!(
-                            "error: detected legacy-style directives in compiletest test: {}:{}, please use `ui_test`-style directives `//@` instead:{:#?}",
-                            testfile.display(),
-                            line_number,
-                            line_directive("//", ln),
-                        );
-                        return;
-                    }
-                }
+            if is_known_directive {
+                *poisoned = true;
+                eprintln!(
+                    "error: detected legacy-style directive {} in compiletest test: {}:{}, please use `ui_test`-style directives `//@` instead: {:#?}",
+                    directive_name,
+                    testfile.display(),
+                    line_number,
+                    line_directive("//", ln),
+                );
+                return;
             }
         }
     }

src/tools/compiletest/src/header/directive_checks/error_annotation.rs

@@ -0,0 +1,6 @@
+//@ check-pass
+
+//~ HELP
+fn main() { } //~ERROR
+//~^ ERROR
+//~| ERROR

src/tools/compiletest/src/header/directive_checks/known_directive.rs

@@ -0,0 +1,4 @@
+//! ignore-wasm
+//@ ignore-wasm
+//@ check-pass
+// regular comment

src/tools/compiletest/src/header/directive_checks/known_legacy_directive.rs

@@ -0,0 +1 @@
+// ignore-wasm

src/tools/compiletest/src/header/directive_checks/not_rs.Makefile

@@ -0,0 +1 @@
+# ignore-owo

src/tools/compiletest/src/header/directive_checks/unknown_directive.rs

@@ -0,0 +1 @@
+//@ needs-headpat

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

@@ -5,6 +5,8 @@ use std::str::FromStr;
 use crate::common::{Config, Debugger, Mode};
 use crate::header::{parse_normalization_string, EarlyProps, HeadersCache};
 
+use super::iter_header;
+
 fn make_test_description<R: Read>(
     config: &Config,
     name: test::TestName,
@@ -582,3 +584,43 @@ fn ignore_mode() {
         assert!(!check_ignore(&config, &format!("//@ ignore-mode-{other}")));
     }
 }
+
+fn run_path(poisoned: &mut bool, path: &Path, buf: &[u8]) {
+    let rdr = std::io::Cursor::new(&buf);
+    iter_header(Mode::Ui, "ui", poisoned, path, rdr, &mut |_| {});
+}
+
+#[test]
+fn test_unknown_directive_check() {
+    let mut poisoned = false;
+    run_path(&mut poisoned, Path::new("a.rs"), include_bytes!("./directive_checks/unknown_directive.rs"));
+    assert!(poisoned);
+}
+
+#[test]
+fn test_known_legacy_directive_check() {
+    let mut poisoned = false;
+    run_path(&mut poisoned, Path::new("a.rs"), include_bytes!("./directive_checks/known_legacy_directive.rs"));
+    assert!(poisoned);
+}
+
+#[test]
+fn test_known_directive_check_no_error() {
+    let mut poisoned = false;
+    run_path(&mut poisoned, Path::new("a.rs"), include_bytes!("./directive_checks/known_directive.rs"));
+    assert!(!poisoned);
+}
+
+#[test]
+fn test_error_annotation_no_error() {
+    let mut poisoned = false;
+    run_path(&mut poisoned, Path::new("a.rs"), include_bytes!("./directive_checks/error_annotation.rs"));
+    assert!(!poisoned);
+}
+
+#[test]
+fn test_non_rs_unknown_directive_not_checked() {
+    let mut poisoned = false;
+    run_path(&mut poisoned, Path::new("a.Makefile"), include_bytes!("./directive_checks/not_rs.Makefile"));
+    assert!(!poisoned);
+}