Auto merge of #147288 - Zalathar:directive, r=<try>

compiletest: Make `DirectiveLine` responsible for name/value splitting

try-job: x86_64-msvc-1
try-job: i686-msvc-1
try-job: x86_64-mingw-1
try-job: test-various
try-job: armhf-gnu
try-job: aarch64-apple

Author: rust-bors[bot]

src/tools/compiletest/src/directives.rs

@@ -15,6 +15,7 @@ use crate::directives::auxiliary::{AuxProps, parse_and_update_aux};
 use crate::directives::directive_names::{
     KNOWN_DIRECTIVE_NAMES, KNOWN_HTMLDOCCK_DIRECTIVE_NAMES, KNOWN_JSONDOCCK_DIRECTIVE_NAMES,
 };
+use crate::directives::line::{DirectiveLine, line_directive};
 use crate::directives::needs::CachedNeedsConditions;
 use crate::edition::{Edition, parse_edition};
 use crate::errors::ErrorKind;
@@ -25,6 +26,7 @@ use crate::{fatal, help};
 pub(crate) mod auxiliary;
 mod cfg;
 mod directive_names;
+mod line;
 mod needs;
 #[cfg(test)]
 mod tests;
@@ -824,70 +826,6 @@ impl TestProps {
     }
 }
 
-/// If the given line begins with the appropriate comment prefix for a directive,
-/// returns a struct containing various parts of the directive.
-fn line_directive<'line>(
-    line_number: usize,
-    original_line: &'line str,
-) -> Option<DirectiveLine<'line>> {
-    // Ignore lines that don't start with the comment prefix.
-    let after_comment =
-        original_line.trim_start().strip_prefix(COMPILETEST_DIRECTIVE_PREFIX)?.trim_start();
-
-    let revision;
-    let raw_directive;
-
-    if let Some(after_open_bracket) = after_comment.strip_prefix('[') {
-        // A comment like `//@[foo]` only applies to revision `foo`.
-        let Some((line_revision, after_close_bracket)) = after_open_bracket.split_once(']') else {
-            panic!(
-                "malformed condition directive: expected `{COMPILETEST_DIRECTIVE_PREFIX}[foo]`, found `{original_line}`"
-            )
-        };
-
-        revision = Some(line_revision);
-        raw_directive = after_close_bracket.trim_start();
-    } else {
-        revision = None;
-        raw_directive = after_comment;
-    };
-
-    Some(DirectiveLine { line_number, revision, raw_directive })
-}
-
-/// The (partly) broken-down contents of a line containing a test directive,
-/// which [`iter_directives`] passes to its callback function.
-///
-/// For example:
-///
-/// ```text
-/// //@ compile-flags: -O
-///     ^^^^^^^^^^^^^^^^^ raw_directive
-///
-/// //@ [foo] compile-flags: -O
-///      ^^^                    revision
-///           ^^^^^^^^^^^^^^^^^ raw_directive
-/// ```
-struct DirectiveLine<'ln> {
-    line_number: usize,
-    /// Some test directives start with a revision name in square brackets
-    /// (e.g. `[foo]`), and only apply to that revision of the test.
-    /// If present, this field contains the revision name (e.g. `foo`).
-    revision: Option<&'ln str>,
-    /// The main part of the directive, after removing the comment prefix
-    /// and the optional revision specifier.
-    ///
-    /// This is "raw" because the directive's name and colon-separated value
-    /// (if present) have not yet been extracted or checked.
-    raw_directive: &'ln str,
-}
-
-impl<'ln> DirectiveLine<'ln> {
-    fn applies_to_test_revision(&self, test_revision: Option<&str>) -> bool {
-        self.revision.is_none() || self.revision == test_revision
-    }
-}
-
 pub(crate) struct CheckDirectiveResult<'ln> {
     is_known_directive: bool,
     trailing_directive: Option<&'ln str>,
@@ -897,9 +835,7 @@ fn check_directive<'a>(
     directive_ln: &DirectiveLine<'a>,
     mode: TestMode,
 ) -> CheckDirectiveResult<'a> {
-    let &DirectiveLine { raw_directive: directive_ln, .. } = directive_ln;
-
-    let (directive_name, post) = directive_ln.split_once([':', ' ']).unwrap_or((directive_ln, ""));
+    let &DirectiveLine { name: directive_name, .. } = directive_ln;
 
     let is_known_directive = KNOWN_DIRECTIVE_NAMES.contains(&directive_name)
         || match mode {
@@ -908,20 +844,17 @@ fn check_directive<'a>(
             _ => false,
         };
 
-    let trailing = post.trim().split_once(' ').map(|(pre, _)| pre).unwrap_or(post);
-    let trailing_directive = {
-        // 1. is the directive name followed by a space? (to exclude `:`)
-        directive_ln.get(directive_name.len()..).is_some_and(|s| s.starts_with(' '))
-            // 2. is what is after that directive also a directive (ex: "only-x86 only-arm")
-            && KNOWN_DIRECTIVE_NAMES.contains(&trailing)
-    }
-    .then_some(trailing);
+    // If it looks like the user tried to put two directives on the same line
+    // (e.g. `//@ only-linux only-x86_64`), signal an error, because the
+    // second "directive" would actually be ignored with no effect.
+    let trailing_directive = directive_ln
+        .remark_after_space()
+        .map(|remark| remark.trim_start().split(' ').next().unwrap())
+        .filter(|token| KNOWN_DIRECTIVE_NAMES.contains(token));
 
     CheckDirectiveResult { is_known_directive, trailing_directive }
 }
 
-const COMPILETEST_DIRECTIVE_PREFIX: &str = "//@";
-
 fn iter_directives(
     mode: TestMode,
     poisoned: &mut bool,
@@ -939,15 +872,17 @@ fn iter_directives(
     // FIXME(jieyouxu): I feel like there's a better way to do this, leaving for later.
     if mode == TestMode::CoverageRun {
         let extra_directives: &[&str] = &[
-            "needs-profiler-runtime",
+            "//@ needs-profiler-runtime",
             // 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",
+            "//@ ignore-cross-compile",
         ];
         // Process the extra implied directives, with a dummy line number of 0.
-        for raw_directive in extra_directives {
-            it(DirectiveLine { line_number: 0, revision: None, raw_directive });
+        for directive_str in extra_directives {
+            let directive_line = line_directive(0, directive_str)
+                .unwrap_or_else(|| panic!("bad extra-directive line: {directive_str:?}"));
+            it(directive_line);
         }
     }
 
@@ -977,7 +912,7 @@ fn iter_directives(
 
                 error!(
                     "{testfile}:{line_number}: detected unknown compiletest test directive `{}`",
-                    directive_line.raw_directive,
+                    directive_line.display(),
                 );
 
                 return;
@@ -1073,35 +1008,30 @@ impl Config {
     }
 
     fn parse_custom_normalization(&self, line: &DirectiveLine<'_>) -> Option<NormalizeRule> {
-        let &DirectiveLine { raw_directive, .. } = line;
-
-        // FIXME(Zalathar): Integrate name/value splitting into `DirectiveLine`
-        // instead of doing it here.
-        let (directive_name, raw_value) = raw_directive.split_once(':')?;
+        let &DirectiveLine { name, .. } = line;
 
-        let kind = match directive_name {
+        let kind = match name {
             "normalize-stdout" => NormalizeKind::Stdout,
             "normalize-stderr" => NormalizeKind::Stderr,
             "normalize-stderr-32bit" => NormalizeKind::Stderr32bit,
             "normalize-stderr-64bit" => NormalizeKind::Stderr64bit,
             _ => return None,
         };
 
-        let Some((regex, replacement)) = parse_normalize_rule(raw_value) else {
-            error!("couldn't parse custom normalization rule: `{raw_directive}`");
-            help!("expected syntax is: `{directive_name}: \"REGEX\" -> \"REPLACEMENT\"`");
+        let Some((regex, replacement)) = line.value_after_colon().and_then(parse_normalize_rule)
+        else {
+            error!("couldn't parse custom normalization rule: `{}`", line.display());
+            help!("expected syntax is: `{name}: \"REGEX\" -> \"REPLACEMENT\"`");
             panic!("invalid normalization rule detected");
         };
         Some(NormalizeRule { kind, regex, replacement })
     }
 
     fn parse_name_directive(&self, line: &DirectiveLine<'_>, directive: &str) -> bool {
-        let &DirectiveLine { raw_directive: line, .. } = line;
-
-        // Ensure the directive is a whole word. Do not match "ignore-x86" when
-        // the line says "ignore-x86_64".
-        line.starts_with(directive)
-            && matches!(line.as_bytes().get(directive.len()), None | Some(&b' ') | Some(&b':'))
+        // FIXME(Zalathar): Ideally, this should raise an error if a name-only
+        // directive is followed by a colon, since that's the wrong syntax.
+        // But we would need to fix tests that rely on the current behaviour.
+        line.name == directive
     }
 
     fn parse_name_value_directive(
@@ -1110,22 +1040,26 @@ impl Config {
         directive: &str,
         testfile: &Utf8Path,
     ) -> Option<String> {
-        let &DirectiveLine { line_number, raw_directive: line, .. } = line;
-
-        let colon = directive.len();
-        if line.starts_with(directive) && line.as_bytes().get(colon) == Some(&b':') {
-            let value = line[(colon + 1)..].to_owned();
-            debug!("{}: {}", directive, value);
-            let value = expand_variables(value, self);
-            if value.is_empty() {
-                error!("{testfile}:{line_number}: empty value for directive `{directive}`");
-                help!("expected syntax is: `{directive}: value`");
-                panic!("empty directive value detected");
-            }
-            Some(value)
-        } else {
-            None
+        let &DirectiveLine { line_number, .. } = line;
+
+        if line.name != directive {
+            return None;
+        };
+
+        // FIXME(Zalathar): This silently discards directives with a matching
+        // name but no colon. Unfortunately, some directives (e.g. "pp-exact")
+        // currently rely on _not_ panicking here.
+        let value = line.value_after_colon()?;
+        debug!("{}: {}", directive, value);
+        let value = expand_variables(value.to_owned(), self);
+
+        if value.is_empty() {
+            error!("{testfile}:{line_number}: empty value for directive `{directive}`");
+            help!("expected syntax is: `{directive}: value`");
+            panic!("empty directive value detected");
         }
+
+        Some(value)
     }
 
     fn set_name_directive(&self, line: &DirectiveLine<'_>, directive: &str, value: &mut bool) {
@@ -1515,14 +1449,14 @@ pub(crate) fn make_test_description<R: Read>(
 }
 
 fn ignore_cdb(config: &Config, line: &DirectiveLine<'_>) -> IgnoreDecision {
-    let &DirectiveLine { raw_directive: line, .. } = line;
-
     if config.debugger != Some(Debugger::Cdb) {
         return IgnoreDecision::Continue;
     }
 
     if let Some(actual_version) = config.cdb_version {
-        if let Some(rest) = line.strip_prefix("min-cdb-version:").map(str::trim) {
+        if line.name == "min-cdb-version"
+            && let Some(rest) = line.value_after_colon().map(str::trim)
+        {
             let min_version = extract_cdb_version(rest).unwrap_or_else(|| {
                 panic!("couldn't parse version range: {:?}", rest);
             });
@@ -1540,14 +1474,14 @@ fn ignore_cdb(config: &Config, line: &DirectiveLine<'_>) -> IgnoreDecision {
 }
 
 fn ignore_gdb(config: &Config, line: &DirectiveLine<'_>) -> IgnoreDecision {
-    let &DirectiveLine { raw_directive: line, .. } = line;
-
     if config.debugger != Some(Debugger::Gdb) {
         return IgnoreDecision::Continue;
     }
 
     if let Some(actual_version) = config.gdb_version {
-        if let Some(rest) = line.strip_prefix("min-gdb-version:").map(str::trim) {
+        if line.name == "min-gdb-version"
+            && let Some(rest) = line.value_after_colon().map(str::trim)
+        {
             let (start_ver, end_ver) = extract_version_range(rest, extract_gdb_version)
                 .unwrap_or_else(|| {
                     panic!("couldn't parse version range: {:?}", rest);
@@ -1563,7 +1497,9 @@ fn ignore_gdb(config: &Config, line: &DirectiveLine<'_>) -> IgnoreDecision {
                     reason: format!("ignored when the GDB version is lower than {rest}"),
                 };
             }
-        } else if let Some(rest) = line.strip_prefix("ignore-gdb-version:").map(str::trim) {
+        } else if line.name == "ignore-gdb-version"
+            && let Some(rest) = line.value_after_colon().map(str::trim)
+        {
             let (min_version, max_version) = extract_version_range(rest, extract_gdb_version)
                 .unwrap_or_else(|| {
                     panic!("couldn't parse version range: {:?}", rest);
@@ -1590,14 +1526,14 @@ fn ignore_gdb(config: &Config, line: &DirectiveLine<'_>) -> IgnoreDecision {
 }
 
 fn ignore_lldb(config: &Config, line: &DirectiveLine<'_>) -> IgnoreDecision {
-    let &DirectiveLine { raw_directive: line, .. } = line;
-
     if config.debugger != Some(Debugger::Lldb) {
         return IgnoreDecision::Continue;
     }
 
     if let Some(actual_version) = config.lldb_version {
-        if let Some(rest) = line.strip_prefix("min-lldb-version:").map(str::trim) {
+        if line.name == "min-lldb-version"
+            && let Some(rest) = line.value_after_colon().map(str::trim)
+        {
             let min_version = rest.parse().unwrap_or_else(|e| {
                 panic!("Unexpected format of LLDB version string: {}\n{:?}", rest, e);
             });

src/tools/compiletest/src/directives/auxiliary.rs

@@ -50,9 +50,7 @@ pub(super) fn parse_and_update_aux(
     testfile: &Utf8Path,
     aux: &mut AuxProps,
 ) {
-    let &DirectiveLine { raw_directive: ln, .. } = directive_line;
-
-    if !(ln.starts_with("aux-") || ln.starts_with("proc-macro")) {
+    if !(directive_line.name.starts_with("aux-") || directive_line.name == "proc-macro") {
         return;
     }
 

src/tools/compiletest/src/directives/cfg.rs

@@ -6,8 +6,8 @@ use crate::directives::{DirectiveLine, IgnoreDecision};
 const EXTRA_ARCHS: &[&str] = &["spirv"];
 
 pub(super) fn handle_ignore(config: &Config, line: &DirectiveLine<'_>) -> IgnoreDecision {
-    let parsed = parse_cfg_name_directive(config, line, "ignore");
-    let &DirectiveLine { raw_directive: line, .. } = line;
+    let parsed = parse_cfg_name_directive(config, line, "ignore-");
+    let line = line.display();
 
     match parsed.outcome {
         MatchOutcome::NoMatch => IgnoreDecision::Continue,
@@ -24,8 +24,8 @@ pub(super) fn handle_ignore(config: &Config, line: &DirectiveLine<'_>) -> Ignore
 }
 
 pub(super) fn handle_only(config: &Config, line: &DirectiveLine<'_>) -> IgnoreDecision {
-    let parsed = parse_cfg_name_directive(config, line, "only");
-    let &DirectiveLine { raw_directive: line, .. } = line;
+    let parsed = parse_cfg_name_directive(config, line, "only-");
+    let line = line.display();
 
     match parsed.outcome {
         MatchOutcome::Match => IgnoreDecision::Continue,
@@ -50,18 +50,14 @@ fn parse_cfg_name_directive<'a>(
     line: &'a DirectiveLine<'a>,
     prefix: &str,
 ) -> ParsedNameDirective<'a> {
-    let &DirectiveLine { raw_directive: line, .. } = line;
-
-    if !line.as_bytes().starts_with(prefix.as_bytes()) {
-        return ParsedNameDirective::not_a_directive();
-    }
-    if line.as_bytes().get(prefix.len()) != Some(&b'-') {
+    let Some(name) = line.name.strip_prefix(prefix) else {
         return ParsedNameDirective::not_a_directive();
-    }
-    let line = &line[prefix.len() + 1..];
+    };
 
-    let (name, comment) =
-        line.split_once(&[':', ' ']).map(|(l, c)| (l, Some(c))).unwrap_or((line, None));
+    // FIXME(Zalathar): This currently allows either a space or a colon, and
+    // treats any "value" after a colon as though it were a remark.
+    // We should instead forbid the colon syntax for these directives.
+    let comment = line.remark_after_space().or_else(|| line.value_after_colon());
 
     // Some of the matchers might be "" depending on what the target information is. To avoid
     // problems we outright reject empty directives.
@@ -269,7 +265,7 @@ fn parse_cfg_name_directive<'a>(
         message: "when performing tests on dist toolchain"
     }
 
-    if prefix == "ignore" && outcome == MatchOutcome::Invalid {
+    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-") {
             outcome = MatchOutcome::External;