Use new DirectiveLine features in directive parsing

Author: Zalathar

src/tools/compiletest/src/directives.rs

@@ -835,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 {
@@ -846,14 +844,13 @@ 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 }
 }
@@ -915,7 +912,7 @@ fn iter_directives(
 
                 error!(
                     "{testfile}:{line_number}: detected unknown compiletest test directive `{}`",
-                    directive_line.raw_directive,
+                    directive_line.display(),
                 );
 
                 return;
@@ -1011,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(
@@ -1048,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) {
@@ -1453,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);
             });
@@ -1478,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);
@@ -1501,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);
@@ -1528,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;

src/tools/compiletest/src/directives/line.rs

@@ -1,5 +1,3 @@
-#![expect(dead_code)] // (removed later in this PR)
-
 use std::fmt;
 
 const COMPILETEST_DIRECTIVE_PREFIX: &str = "//@";
@@ -66,7 +64,7 @@ pub(crate) struct DirectiveLine<'ln> {
     ///
     /// This is "raw" because the directive's name and colon-separated value
     /// (if present) have not yet been extracted or checked.
-    pub(crate) raw_directive: &'ln str,
+    raw_directive: &'ln str,
 
     /// Name of the directive.
     ///

src/tools/compiletest/src/directives/needs.rs

@@ -181,17 +181,10 @@ pub(super) fn handle_needs(
         },
     ];
 
-    let &DirectiveLine { raw_directive: ln, .. } = ln;
+    let &DirectiveLine { name, .. } = ln;
 
-    let (name, rest) = match ln.split_once([':', ' ']) {
-        Some((name, rest)) => (name, Some(rest)),
-        None => (ln, None),
-    };
-
-    // FIXME(jieyouxu): tighten up this parsing to reject using both `:` and ` ` as means to
-    // delineate value.
     if name == "needs-target-has-atomic" {
-        let Some(rest) = rest else {
+        let Some(rest) = ln.value_after_colon() else {
             return IgnoreDecision::Error {
                 message: "expected `needs-target-has-atomic` to have a comma-separated list of atomic widths".to_string(),
             };
@@ -233,7 +226,7 @@ pub(super) fn handle_needs(
 
     // FIXME(jieyouxu): share multi-value directive logic with `needs-target-has-atomic` above.
     if name == "needs-crate-type" {
-        let Some(rest) = rest else {
+        let Some(rest) = ln.value_after_colon() else {
             return IgnoreDecision::Error {
                 message:
                     "expected `needs-crate-type` to have a comma-separated list of crate types"
@@ -295,7 +288,7 @@ pub(super) fn handle_needs(
                 break;
             } else {
                 return IgnoreDecision::Ignore {
-                    reason: if let Some(comment) = rest {
+                    reason: if let Some(comment) = ln.remark_after_space() {
                         format!("{} ({})", need.ignore_reason, comment.trim())
                     } else {
                         need.ignore_reason.into()

src/tools/compiletest/src/directives/tests.rs

@@ -3,12 +3,11 @@ use std::io::Read;
 use camino::Utf8Path;
 use semver::Version;
 
-use super::{
+use crate::common::{Config, Debugger, TestMode};
+use crate::directives::{
     DirectivesCache, EarlyProps, Edition, EditionRange, extract_llvm_version,
-    extract_version_range, iter_directives, parse_normalize_rule,
+    extract_version_range, iter_directives, line_directive, parse_edition, parse_normalize_rule,
 };
-use crate::common::{Config, Debugger, TestMode};
-use crate::directives::parse_edition;
 use crate::executor::{CollectedTestDesc, ShouldPanic};
 
 fn make_test_description<R: Read>(
@@ -955,7 +954,9 @@ fn test_needs_target_std() {
 
 fn parse_edition_range(line: &str) -> Option<EditionRange> {
     let config = cfg().build();
-    let line = super::DirectiveLine { line_number: 0, revision: None, raw_directive: line };
+
+    let line_with_comment = format!("//@ {line}");
+    let line = line_directive(0, &line_with_comment).unwrap();
 
     super::parse_edition_range(&config, &line, "tmp.rs".into())
 }