Skip to content
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

Drop compiletest legacy directive check #131392

Merged
merged 1 commit into from
Oct 8, 2024
Merged

Drop compiletest legacy directive check #131392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
129 changes: 48 additions & 81 deletions src/tools/compiletest/src/header.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@ use std::io::BufReader;
use std::io::prelude::*;
use std::path::{Path, PathBuf};
use std::process::Command;
use std::sync::OnceLock;

use regex::Regex;
use tracing::*;

use crate::common::{Config, Debugger, FailMode, Mode, PassMode};
Expand Down Expand Up @@ -797,7 +795,6 @@ struct HeaderLine<'ln> {

pub(crate) struct CheckDirectiveResult<'ln> {
is_known_directive: bool,
directive_name: &'ln str,
trailing_directive: Option<&'ln str>,
}

Expand Down Expand Up @@ -832,11 +829,7 @@ pub(crate) fn check_directive<'a>(
}
.then_some(trailing);

CheckDirectiveResult {
is_known_directive: is_known(&directive_name),
directive_name: directive_ln,
trailing_directive,
}
CheckDirectiveResult { is_known_directive: is_known(&directive_name), trailing_directive }
}

fn iter_header(
Expand All @@ -851,16 +844,17 @@ fn iter_header(
return;
}

// Coverage tests in coverage-run mode always have these extra directives,
// without needing to specify them manually in every test file.
// (Some of the comments below have been copied over from the old
// `tests/run-make/coverage-reports/Makefile`, which no longer exists.)
// Coverage tests in coverage-run mode always have these extra directives, without needing to
// specify them manually in every test file. (Some of the comments below have been copied over
// from the old `tests/run-make/coverage-reports/Makefile`, which no longer exists.)
//
// FIXME(jieyouxu): I feel like there's a better way to do this, leaving for later.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is currently the least-awful way to achieve the desired effect, because there's no good way to hook into the ignore system. But with some appropriate refactoring there should definitely be a better approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, just left a note for myself.

if mode == Mode::CoverageRun {
let extra_directives: &[&str] = &[
"needs-profiler-support",
// 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.
// 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",
];
// Process the extra implied directives, with a dummy line number of 0.
Expand All @@ -869,17 +863,13 @@ fn iter_header(
}
}

// NOTE(jieyouxu): once we get rid of `Makefile`s we can unconditionally check for `//@`.
let comment = if testfile.extension().is_some_and(|e| e == "rs") { "//@" } else { "#" };

let mut rdr = BufReader::with_capacity(1024, rdr);
let mut ln = String::new();
let mut line_number = 0;

// Match on error annotations like `//~ERROR`.
static REVISION_MAGIC_COMMENT_RE: OnceLock<Regex> = OnceLock::new();
let revision_magic_comment_re =
REVISION_MAGIC_COMMENT_RE.get_or_init(|| Regex::new("//(\\[.*\\])?~.*").unwrap());

loop {
line_number += 1;
ln.clear();
Expand All @@ -892,85 +882,62 @@ fn iter_header(
// with a warm page cache. Maybe with a cold one.
let original_line = &ln;
let ln = ln.trim();

// Assume that any directives will be found before the first module or function. This
// doesn't seem to be an optimization with a warm page cache. Maybe with a cold one.
// FIXME(jieyouxu): this will cause `//@` directives in the rest of the test file to
// not be checked.
if ln.starts_with("fn") || ln.starts_with("mod") {
return;
}

// First try to accept `ui_test` style comments (`//@`)
} else if let Some((header_revision, non_revisioned_directive_line)) =
line_directive(comment, ln)
{
// 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 CheckDirectiveResult { is_known_directive, trailing_directive, .. } =
check_directive(directive_ln, mode, ln);

if !is_known_directive {
*poisoned = true;

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

return;
}
let Some((header_revision, non_revisioned_directive_line)) = line_directive(comment, ln)
else {
continue;
};

if let Some(trailing_directive) = &trailing_directive {
*poisoned = true;
// 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();

eprintln!(
"error: detected trailing compiletest test directive `{}` in {}:{}\n \
help: put the trailing directive in it's own line: `//@ {}`",
trailing_directive,
testfile.display(),
line_number,
trailing_directive,
);
let CheckDirectiveResult { is_known_directive, trailing_directive } =
check_directive(directive_ln, mode, ln);

return;
}
}
if !is_known_directive {
*poisoned = true;

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;
};
eprintln!(
"error: detected unknown compiletest test directive `{}` in {}:{}",
directive_ln,
testfile.display(),
line_number,
);

if rest.trim_start().starts_with(':') {
// This is likely a markdown link:
// `[link_name]: https://example.org`
continue;
return;
}

let rest = rest.trim_start();

let CheckDirectiveResult { is_known_directive, directive_name, .. } =
check_directive(rest, mode, ln);

if is_known_directive {
if let Some(trailing_directive) = &trailing_directive {
*poisoned = true;

eprintln!(
"error: detected legacy-style directive {} in compiletest test: {}:{}, please use `ui_test`-style directives `//@` instead: {:#?}",
directive_name,
"error: detected trailing compiletest test directive `{}` in {}:{}\n \
help: put the trailing directive in it's own line: `//@ {}`",
trailing_directive,
testfile.display(),
line_number,
line_directive("//", ln),
trailing_directive,
);

return;
}
}

it(HeaderLine {
line_number,
original_line,
header_revision,
directive: non_revisioned_directive_line,
});
}
}

Expand Down
1 change: 0 additions & 1 deletion src/tools/compiletest/src/header/test-auxillary/known_legacy_directive.rs
View file
Open in desktop

This file was deleted.

11 changes: 0 additions & 11 deletions src/tools/compiletest/src/header/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,17 +618,6 @@ fn test_unknown_directive_check() {
assert!(poisoned);
}

#[test]
fn test_known_legacy_directive_check() {
let mut poisoned = false;
run_path(
&mut poisoned,
Path::new("a.rs"),
include_bytes!("./test-auxillary/known_legacy_directive.rs"),
);
assert!(poisoned);
}

#[test]
fn test_known_directive_check_no_error() {
let mut poisoned = false;
Expand Down
Loading