Skip to content

compiletest: Make DirectiveLine responsible for name/value splitting #147288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 5, 2025
+188 −156
Merged

compiletest: Make DirectiveLine responsible for name/value splitting #147288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
757d98c
Move `DirectiveLine` into its own submodule
Zalathar Oct 3, 2025
6783e94
Allow easy extraction of name/value from a `DirectiveLine`
Zalathar Oct 2, 2025
33c99a0
Use new `DirectiveLine` features in directive parsing
Zalathar Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 58 additions & 122 deletions src/tools/compiletest/src/directives.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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>,
Expand All @@ -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 {
Expand All @@ -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,
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -977,7 +912,7 @@ fn iter_directives(

error!(
"{testfile}:{line_number}: detected unknown compiletest test directive `{}`",
directive_line.raw_directive,
directive_line.display(),
);

return;
Expand Down Expand Up @@ -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
Comment on lines +1031 to +1034
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

fn parse_name_value_directive(
Expand All @@ -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)
Comment on lines +1049 to +1062
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remark: yeah, I would honestly vote for hard reject eventually.

}

fn set_name_directive(&self, line: &DirectiveLine<'_>, directive: &str, value: &mut bool) {
Expand Down Expand Up @@ -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);
});
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
});
Expand Down
4 changes: 1 addition & 3 deletions src/tools/compiletest/src/directives/auxiliary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
26 changes: 11 additions & 15 deletions src/tools/compiletest/src/directives/cfg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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());
Comment on lines +57 to +60
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remark: agreed, we should reject the colon case eventually


// Some of the matchers might be "" depending on what the target information is. To avoid
// problems we outright reject empty directives.
Expand Down Expand Up @@ -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;
Expand Down
Loading
Loading