compiletest: Simplify passing arguments to spawned test threads

src/tools/compiletest/src/directives.rs

@@ -18,7 +18,7 @@ use crate::directives::line::{DirectiveLine, line_directive};
 use crate::directives::needs::CachedNeedsConditions;
 use crate::edition::{Edition, parse_edition};
 use crate::errors::ErrorKind;
-use crate::executor::{CollectedTestDesc, ShouldPanic};
+use crate::executor::{CollectedTestDesc, ShouldFail};
 use crate::util::static_regex;
 use crate::{fatal, help};
 
@@ -1366,18 +1366,18 @@ pub(crate) fn make_test_description(
     // The `should-fail` annotation doesn't apply to pretty tests,
     // since we run the pretty printer across all tests by default.
     // If desired, we could add a `should-fail-pretty` annotation.
-    let should_panic = match config.mode {
-        TestMode::Pretty => ShouldPanic::No,
-        _ if should_fail => ShouldPanic::Yes,
-        _ => ShouldPanic::No,
+    let should_fail = if should_fail && config.mode != TestMode::Pretty {
+        ShouldFail::Yes
+    } else {
+        ShouldFail::No
     };
 
     CollectedTestDesc {
         name,
         filterable_path: filterable_path.to_owned(),
         ignore,
         ignore_message,
-        should_panic,
+        should_fail,
     }
 }
 

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

@@ -7,7 +7,7 @@ use crate::directives::{
     extract_llvm_version, extract_version_range, iter_directives, line_directive, parse_edition,
     parse_normalize_rule,
 };
-use crate::executor::{CollectedTestDesc, ShouldPanic};
+use crate::executor::{CollectedTestDesc, ShouldFail};
 
 fn make_test_description(
     config: &Config,
@@ -247,9 +247,9 @@ fn should_fail() {
     let p = Utf8Path::new("a.rs");
 
     let d = make_test_description(&config, tn.clone(), p, p, "", None);
-    assert_eq!(d.should_panic, ShouldPanic::No);
+    assert_eq!(d.should_fail, ShouldFail::No);
     let d = make_test_description(&config, tn, p, p, "//@ should-fail", None);
-    assert_eq!(d.should_panic, ShouldPanic::Yes);
+    assert_eq!(d.should_fail, ShouldFail::Yes);
 }
 
 #[test]

src/tools/compiletest/src/executor.rs

@@ -98,32 +98,43 @@ pub(crate) fn run_tests(config: &Config, tests: Vec<CollectedTest>) -> bool {
 fn spawn_test_thread(
     id: TestId,
     test: &CollectedTest,
-    completion_tx: mpsc::Sender<TestCompletion>,
+    completion_sender: mpsc::Sender<TestCompletion>,
 ) -> Option<thread::JoinHandle<()>> {
     if test.desc.ignore && !test.config.run_ignored {
-        completion_tx
+        completion_sender
             .send(TestCompletion { id, outcome: TestOutcome::Ignored, stdout: None })
             .unwrap();
         return None;
     }
 
-    let runnable_test = RunnableTest::new(test);
-    let should_panic = test.desc.should_panic;
-    let run_test = move || run_test_inner(id, should_panic, runnable_test, completion_tx);
-
+    let args = TestThreadArgs {
+        id,
+        config: Arc::clone(&test.config),
+        testpaths: test.testpaths.clone(),
+        revision: test.revision.clone(),
+        should_fail: test.desc.should_fail,
+        completion_sender,
+    };
     let thread_builder = thread::Builder::new().name(test.desc.name.clone());
-    let join_handle = thread_builder.spawn(run_test).unwrap();
+    let join_handle = thread_builder.spawn(move || test_thread_main(args)).unwrap();
     Some(join_handle)
 }
 
-/// Runs a single test, within the dedicated thread spawned by the caller.
-fn run_test_inner(
+/// All of the owned data needed by `test_thread_main`.
+struct TestThreadArgs {
     id: TestId,
-    should_panic: ShouldPanic,
-    runnable_test: RunnableTest,
+
+    config: Arc<Config>,
+    testpaths: TestPaths,
+    revision: Option<String>,
+    should_fail: ShouldFail,
+
     completion_sender: mpsc::Sender<TestCompletion>,
-) {
-    let capture = CaptureKind::for_config(&runnable_test.config);
+}
+
+/// Runs a single test, within the dedicated thread spawned by the caller.
+fn test_thread_main(args: TestThreadArgs) {
+    let capture = CaptureKind::for_config(&args.config);
 
     // Install a panic-capture buffer for use by the custom panic hook.
     if capture.should_set_panic_hook() {
@@ -133,24 +144,42 @@ fn run_test_inner(
     let stdout = capture.stdout();
     let stderr = capture.stderr();
 
-    let panic_payload = panic::catch_unwind(move || runnable_test.run(stdout, stderr)).err();
+    // Run the test, catching any panics so that we can gracefully report
+    // failure (or success).
+    //
+    // FIXME(Zalathar): Ideally we would report test failures with `Result`,
+    // and use panics only for bugs within compiletest itself, but that would
+    // require a major overhaul of error handling in the test runners.
+    let panic_payload = panic::catch_unwind(|| {
+        __rust_begin_short_backtrace(|| {
+            crate::runtest::run(
+                &args.config,
+                stdout,
+                stderr,
+                &args.testpaths,
+                args.revision.as_deref(),
+            );
+        });
+    })
+    .err();
 
     if let Some(panic_buf) = panic_hook::take_capture_buf() {
         let panic_buf = panic_buf.lock().unwrap_or_else(|e| e.into_inner());
         // Forward any captured panic message to (captured) stderr.
         write!(stderr, "{panic_buf}");
     }
 
-    let outcome = match (should_panic, panic_payload) {
-        (ShouldPanic::No, None) | (ShouldPanic::Yes, Some(_)) => TestOutcome::Succeeded,
-        (ShouldPanic::No, Some(_)) => TestOutcome::Failed { message: None },
-        (ShouldPanic::Yes, None) => {
-            TestOutcome::Failed { message: Some("test did not panic as expected") }
+    // Interpret the presence/absence of a panic as test failure/success.
+    let outcome = match (args.should_fail, panic_payload) {
+        (ShouldFail::No, None) | (ShouldFail::Yes, Some(_)) => TestOutcome::Succeeded,
+        (ShouldFail::No, Some(_)) => TestOutcome::Failed { message: None },
+        (ShouldFail::Yes, None) => {
+            TestOutcome::Failed { message: Some("`//@ should-fail` test did not fail as expected") }
         }
     };
 
     let stdout = capture.into_inner();
-    completion_sender.send(TestCompletion { id, outcome, stdout }).unwrap();
+    args.completion_sender.send(TestCompletion { id: args.id, outcome, stdout }).unwrap();
 }
 
 enum CaptureKind {
@@ -207,33 +236,6 @@ impl CaptureKind {
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
 struct TestId(usize);
 
-struct RunnableTest {
-    config: Arc<Config>,
-    testpaths: TestPaths,
-    revision: Option<String>,
-}
-
-impl RunnableTest {
-    fn new(test: &CollectedTest) -> Self {
-        let config = Arc::clone(&test.config);
-        let testpaths = test.testpaths.clone();
-        let revision = test.revision.clone();
-        Self { config, testpaths, revision }
-    }
-
-    fn run(&self, stdout: &dyn ConsoleOut, stderr: &dyn ConsoleOut) {
-        __rust_begin_short_backtrace(|| {
-            crate::runtest::run(
-                Arc::clone(&self.config),
-                stdout,
-                stderr,
-                &self.testpaths,
-                self.revision.as_deref(),
-            );
-        });
-    }
-}
-
 /// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`.
 #[inline(never)]
 fn __rust_begin_short_backtrace<T, F: FnOnce() -> T>(f: F) -> T {
@@ -336,7 +338,7 @@ pub(crate) struct CollectedTestDesc {
     pub(crate) filterable_path: Utf8PathBuf,
     pub(crate) ignore: bool,
     pub(crate) ignore_message: Option<Cow<'static, str>>,
-    pub(crate) should_panic: ShouldPanic,
+    pub(crate) should_fail: ShouldFail,
 }
 
 /// Whether console output should be colored or not.
@@ -348,9 +350,10 @@ pub enum ColorConfig {
     NeverColor,
 }
 
-/// Whether test is expected to panic or not.
+/// Tests with `//@ should-fail` are tests of compiletest itself, and should
+/// be reported as successful if and only if they would have _failed_.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
-pub(crate) enum ShouldPanic {
+pub(crate) enum ShouldFail {
     No,
     Yes,
 }

src/tools/compiletest/src/runtest.rs

@@ -6,7 +6,6 @@ use std::hash::{DefaultHasher, Hash, Hasher};
 use std::io::prelude::*;
 use std::io::{self, BufReader};
 use std::process::{Child, Command, ExitStatus, Output, Stdio};
-use std::sync::Arc;
 use std::{env, fmt, iter, str};
 
 use build_helper::fs::remove_and_create_dir_all;
@@ -110,7 +109,7 @@ fn dylib_name(name: &str) -> String {
 }
 
 pub fn run(
-    config: Arc<Config>,
+    config: &Config,
     stdout: &dyn ConsoleOut,
     stderr: &dyn ConsoleOut,
     testpaths: &TestPaths,