From b81a3c81998b03cab4925801edf11c3113b26184 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Tue, 8 Oct 2024 04:30:19 +0000 Subject: [PATCH] Drop compiletest legacy directive checks 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. --- src/tools/compiletest/src/header.rs | 129 +++++++----------- .../test-auxillary/known_legacy_directive.rs | 1 - src/tools/compiletest/src/header/tests.rs | 11 -- 3 files changed, 48 insertions(+), 93 deletions(-) delete mode 100644 src/tools/compiletest/src/header/test-auxillary/known_legacy_directive.rs diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 83a10c5620864..a6830a6aa173f 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -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}; @@ -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>, } @@ -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( @@ -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. @@ -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 = OnceLock::new(); - let revision_magic_comment_re = - REVISION_MAGIC_COMMENT_RE.get_or_init(|| Regex::new("//(\\[.*\\])?~.*").unwrap()); - loop { line_number += 1; ln.clear(); @@ -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, + }); } } diff --git a/src/tools/compiletest/src/header/test-auxillary/known_legacy_directive.rs b/src/tools/compiletest/src/header/test-auxillary/known_legacy_directive.rs deleted file mode 100644 index 108ca432e1308..0000000000000 --- a/src/tools/compiletest/src/header/test-auxillary/known_legacy_directive.rs +++ /dev/null @@ -1 +0,0 @@ -// ignore-wasm diff --git a/src/tools/compiletest/src/header/tests.rs b/src/tools/compiletest/src/header/tests.rs index 76a8b129198dc..b70b4b27f400d 100644 --- a/src/tools/compiletest/src/header/tests.rs +++ b/src/tools/compiletest/src/header/tests.rs @@ -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;