Rollup merge of #126370 - Zalathar:normalize, r=oli-obk

compiletest: Stricter parsing of `//@ normalize-*` headers

I noticed some problems with the existing parser for these headers:

- It is extremely lax, and basically ignores everything other than the text between two pairs of double-quote characters.
  - Unlike other name-value headers, it doesn't even check for a colon after the header name, so the test suite contains a mixture of with-colon and without-colon normalization rules.
- If parsing fails, the header is silently ignored.

The latter is especially bad for platform-specific normalization rules, because the lack of normalization probably won't be noticed until the test mysteriously fails in one of the full CI jobs.

5 files changed, 74 insertions, 61 deletions

diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs
index da0196dad2f..cc972223f6d 100644
--- a/src/tools/compiletest/src/header.rs
+++ b/src/tools/compiletest/src/header.rs
@@ -14,6 +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::static_regex;
 use crate::{extract_cdb_version, extract_gdb_version};
 
 mod cfg;
@@ -1186,11 +1187,11 @@ impl Config {
         }
     }
 
-    fn parse_custom_normalization(&self, mut line: &str, prefix: &str) -> Option<(String, String)> {
+    fn parse_custom_normalization(&self, line: &str, prefix: &str) -> Option<(String, String)> {
         if parse_cfg_name_directive(self, line, prefix).outcome == MatchOutcome::Match {
-            let from = parse_normalization_string(&mut line)?;
-            let to = parse_normalization_string(&mut line)?;
-            Some((from, to))
+            let (regex, replacement) = parse_normalize_rule(line)
+                .unwrap_or_else(|| panic!("couldn't parse custom normalization rule: `{line}`"));
+            Some((regex, replacement))
         } else {
             None
         }
@@ -1311,24 +1312,29 @@ fn expand_variables(mut value: String, config: &Config) -> String {
     value
 }
 
-/// Finds the next quoted string `"..."` in `line`, and extract the content from it. Move the `line`
-/// variable after the end of the quoted string.
-///
-/// # Examples
-///
-/// ```
-/// let mut s = "normalize-stderr-32bit: \"something (32 bits)\" -> \"something ($WORD bits)\".";
-/// let first = parse_normalization_string(&mut s);
-/// assert_eq!(first, Some("something (32 bits)".to_owned()));
-/// assert_eq!(s, " -> \"something ($WORD bits)\".");
+/// Parses the regex and replacement values of a `//@ normalize-*` header,
+/// in the format:
+/// ```text
+/// normalize-*: "REGEX" -> "REPLACEMENT"
 /// ```
-fn parse_normalization_string(line: &mut &str) -> Option<String> {
-    // FIXME support escapes in strings.
-    let begin = line.find('"')? + 1;
-    let end = line[begin..].find('"')? + begin;
-    let result = line[begin..end].to_owned();
-    *line = &line[end + 1..];
-    Some(result)
+fn parse_normalize_rule(header: &str) -> Option<(String, String)> {
+    // FIXME(#126370): A colon after the header name should be mandatory, but
+    // currently is not, and there are many tests that lack the colon.
+    // FIXME: Support escaped double-quotes in strings.
+    let captures = static_regex!(
+        r#"(?x) # (verbose mode regex)
+        ^
+        [^:\s]+:?\s*            # (header name followed by optional colon)
+        "(?<regex>[^"]*)"       # "REGEX"
+        \s+->\s+                # ->
+        "(?<replacement>[^"]*)" # "REPLACEMENT"
+        $
+        "#
+    )
+    .captures(header)?;
+    let regex = captures["regex"].to_owned();
+    let replacement = captures["replacement"].to_owned();
+    Some((regex, replacement))
 }
 
 pub fn extract_llvm_version(version: &str) -> Option<u32> {
diff --git a/src/tools/compiletest/src/header/tests.rs b/src/tools/compiletest/src/header/tests.rs
index 8a37a4d6d31..61a85b84ad6 100644
--- a/src/tools/compiletest/src/header/tests.rs
+++ b/src/tools/compiletest/src/header/tests.rs
@@ -3,7 +3,7 @@ use std::path::Path;
 use std::str::FromStr;
 
 use crate::common::{Config, Debugger, Mode};
-use crate::header::{parse_normalization_string, EarlyProps, HeadersCache};
+use crate::header::{parse_normalize_rule, EarlyProps, HeadersCache};
 
 use super::iter_header;
 
@@ -32,35 +32,41 @@ fn make_test_description<R: Read>(
 }
 
 #[test]
-fn test_parse_normalization_string() {
-    let mut s = "normalize-stderr-32bit: \"something (32 bits)\" -> \"something ($WORD bits)\".";
-    let first = parse_normalization_string(&mut s);
-    assert_eq!(first, Some("something (32 bits)".to_owned()));
-    assert_eq!(s, " -> \"something ($WORD bits)\".");
-
-    // Nothing to normalize (No quotes)
-    let mut s = "normalize-stderr-32bit: something (32 bits) -> something ($WORD bits).";
-    let first = parse_normalization_string(&mut s);
-    assert_eq!(first, None);
-    assert_eq!(s, r#"normalize-stderr-32bit: something (32 bits) -> something ($WORD bits)."#);
-
-    // Nothing to normalize (Only a single quote)
-    let mut s = "normalize-stderr-32bit: \"something (32 bits) -> something ($WORD bits).";
-    let first = parse_normalization_string(&mut s);
-    assert_eq!(first, None);
-    assert_eq!(s, "normalize-stderr-32bit: \"something (32 bits) -> something ($WORD bits).");
-
-    // Nothing to normalize (Three quotes)
-    let mut s = "normalize-stderr-32bit: \"something (32 bits)\" -> \"something ($WORD bits).";
-    let first = parse_normalization_string(&mut s);
-    assert_eq!(first, Some("something (32 bits)".to_owned()));
-    assert_eq!(s, " -> \"something ($WORD bits).");
-
-    // Nothing to normalize (No quotes, 16-bit)
-    let mut s = "normalize-stderr-16bit: something (16 bits) -> something ($WORD bits).";
-    let first = parse_normalization_string(&mut s);
-    assert_eq!(first, None);
-    assert_eq!(s, r#"normalize-stderr-16bit: something (16 bits) -> something ($WORD bits)."#);
+fn test_parse_normalize_rule() {
+    let good_data = &[
+        (
+            r#"normalize-stderr-32bit: "something (32 bits)" -> "something ($WORD bits)""#,
+            "something (32 bits)",
+            "something ($WORD bits)",
+        ),
+        // FIXME(#126370): A colon after the header name should be mandatory,
+        // but currently is not, and there are many tests that lack the colon.
+        (
+            r#"normalize-stderr-32bit "something (32 bits)" -> "something ($WORD bits)""#,
+            "something (32 bits)",
+            "something ($WORD bits)",
+        ),
+    ];
+
+    for &(input, expected_regex, expected_replacement) in good_data {
+        let parsed = parse_normalize_rule(input);
+        let parsed =
+            parsed.as_ref().map(|(regex, replacement)| (regex.as_str(), replacement.as_str()));
+        assert_eq!(parsed, Some((expected_regex, expected_replacement)));
+    }
+
+    let bad_data = &[
+        r#"normalize-stderr-16bit: something (16 bits) -> something ($WORD bits)"#,
+        r#"normalize-stderr-32bit: something (32 bits) -> something ($WORD bits)"#,
+        r#"normalize-stderr-32bit: "something (32 bits) -> something ($WORD bits)"#,
+        r#"normalize-stderr-32bit: "something (32 bits)" -> "something ($WORD bits)"#,
+        r#"normalize-stderr-32bit: "something (32 bits)" -> "something ($WORD bits)"."#,
+    ];
+
+    for &input in bad_data {
+        let parsed = parse_normalize_rule(input);
+        assert_eq!(parsed, None);
+    }
 }
 
 #[derive(Default)]
diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs
index cc15961b46e..9e1d5ea61aa 100644
--- a/src/tools/compiletest/src/runtest.rs
+++ b/src/tools/compiletest/src/runtest.rs
@@ -15,7 +15,7 @@ use crate::errors::{self, Error, ErrorKind};
 use crate::header::TestProps;
 use crate::json;
 use crate::read2::{read2_abbreviated, Truncated};
-use crate::util::{add_dylib_path, copy_dir_all, dylib_env_var, logv, PathBufExt};
+use crate::util::{add_dylib_path, copy_dir_all, dylib_env_var, logv, static_regex, PathBufExt};
 use crate::ColorConfig;
 use colored::Colorize;
 use miropt_test_tools::{files_for_miropt_test, MiroptTest, MiroptTestFile};
@@ -48,14 +48,6 @@ use debugger::DebuggerCommands;
 #[cfg(test)]
 mod tests;
 
-macro_rules! static_regex {
-    ($re:literal) => {{
-        static RE: ::std::sync::OnceLock<::regex::Regex> = ::std::sync::OnceLock::new();
-        RE.get_or_init(|| ::regex::Regex::new($re).unwrap())
-    }};
-}
-use static_regex;
-
 const FAKE_SRC_BASE: &str = "fake-test-src-base";
 
 #[cfg(windows)]
diff --git a/src/tools/compiletest/src/runtest/coverage.rs b/src/tools/compiletest/src/runtest/coverage.rs
index 8bd7c7e808d..6ee147da5a9 100644
--- a/src/tools/compiletest/src/runtest/coverage.rs
+++ b/src/tools/compiletest/src/runtest/coverage.rs
@@ -7,7 +7,8 @@ use std::process::Command;
 use glob::glob;
 
 use crate::common::{UI_COVERAGE, UI_COVERAGE_MAP};
-use crate::runtest::{static_regex, Emit, ProcRes, TestCx, WillExecute};
+use crate::runtest::{Emit, ProcRes, TestCx, WillExecute};
+use crate::util::static_regex;
 
 impl<'test> TestCx<'test> {
     fn coverage_dump_path(&self) -> &Path {
diff --git a/src/tools/compiletest/src/util.rs b/src/tools/compiletest/src/util.rs
index ec20bda8c18..cdec49a51d7 100644
--- a/src/tools/compiletest/src/util.rs
+++ b/src/tools/compiletest/src/util.rs
@@ -90,3 +90,11 @@ pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> std::io::Re
     }
     Ok(())
 }
+
+macro_rules! static_regex {
+    ($re:literal) => {{
+        static RE: ::std::sync::OnceLock<::regex::Regex> = ::std::sync::OnceLock::new();
+        RE.get_or_init(|| ::regex::Regex::new($re).unwrap())
+    }};
+}
+pub(crate) use static_regex;