Skip to content

Commit

Permalink
Rollup merge of #131392 - jieyouxu:remove-legacy-directive-check, r=U…
Browse files Browse the repository at this point in the history 
…rgau

Drop compiletest legacy directive check

Sufficient time has passed (> 6 months) since we migrated from `//` to `//`@`,` so let's drop the
legacy directive check as it causes friction due to false positives.

As a side-effect, dropping the legacy directive check simplifies the directive scanning logic.

The legacy directive check was originally added to help people be aware of the migration.

Blocker for #131382 cc `@ehuss.`

Can be reviewed by any compiler/bootstrap reviewer.
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Oct 8, 2024
2 parents 7cb47b5 + b81a3c8 commit 46f821a
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 93 deletions.
129 changes: 48 additions & 81 deletions src/tools/compiletest/src/header.rs
View file
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.
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

This file was deleted.

11 changes: 0 additions & 11 deletions src/tools/compiletest/src/header/tests.rs
View file
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

0 comments on commit 46f821a

Please sign in to comment.