Auto merge of #52181 - QuietMisdreavus:panicked-tester, r=GuillaumeGomez

rustdoc: set panic output before starting compiler thread pool

When the compiler was updated to run on a thread pool, rustdoc's processing of compiler/doctest stderr/stdout was moved into each compiler thread. However, this caused output of the test to be lost if the test failed at *runtime* instead of compile time. This change sets up the `set_panic` call and output bomb before starting the compiler thread pool, so that the `Drop` call that writes back to the test's stdout happens after the test runs, not just after it compiles.

Fixes #51162

Author: bors

src/librustc_driver/lib.rs

@@ -1497,10 +1497,12 @@ fn parse_crate_attrs<'a>(sess: &'a Session, input: &Input) -> PResult<'a, Vec<as
     }
 }
 
-/// Runs `f` in a suitable thread for running `rustc`; returns a
-/// `Result` with either the return value of `f` or -- if a panic
-/// occurs -- the panic value.
-pub fn in_rustc_thread<F, R>(f: F) -> Result<R, Box<dyn Any + Send>>
+/// Runs `f` in a suitable thread for running `rustc`; returns a `Result` with either the return
+/// value of `f` or -- if a panic occurs -- the panic value.
+///
+/// This version applies the given name to the thread. This is used by rustdoc to ensure consistent
+/// doctest output across platforms and executions.
+pub fn in_named_rustc_thread<F, R>(name: String, f: F) -> Result<R, Box<dyn Any + Send>>
     where F: FnOnce() -> R + Send + 'static,
           R: Send + 'static,
 {
@@ -1564,7 +1566,7 @@ pub fn in_rustc_thread<F, R>(f: F) -> Result<R, Box<dyn Any + Send>>
 
     // The or condition is added from backward compatibility.
     if spawn_thread || env::var_os("RUST_MIN_STACK").is_some() {
-        let mut cfg = thread::Builder::new().name("rustc".to_string());
+        let mut cfg = thread::Builder::new().name(name);
 
         // FIXME: Hacks on hacks. If the env is trying to override the stack size
         // then *don't* set it explicitly.
@@ -1580,6 +1582,16 @@ pub fn in_rustc_thread<F, R>(f: F) -> Result<R, Box<dyn Any + Send>>
     }
 }
 
+/// Runs `f` in a suitable thread for running `rustc`; returns a
+/// `Result` with either the return value of `f` or -- if a panic
+/// occurs -- the panic value.
+pub fn in_rustc_thread<F, R>(f: F) -> Result<R, Box<dyn Any + Send>>
+    where F: FnOnce() -> R + Send + 'static,
+          R: Send + 'static,
+{
+    in_named_rustc_thread("rustc".to_string(), f)
+}
+
 /// Get a list of extra command-line flags provided by the user, as strings.
 ///
 /// This function is used during ICEs to show more information useful for

src/librustdoc/test.rs

@@ -232,40 +232,42 @@ fn run_test(test: &str, cratename: &str, filename: &FileName, line: usize,
         ..config::basic_options().clone()
     };
 
-    let (libdir, outdir) = driver::spawn_thread_pool(sessopts, |sessopts| {
-        // Shuffle around a few input and output handles here. We're going to pass
-        // an explicit handle into rustc to collect output messages, but we also
-        // want to catch the error message that rustc prints when it fails.
-        //
-        // We take our thread-local stderr (likely set by the test runner) and replace
-        // it with a sink that is also passed to rustc itself. When this function
-        // returns the output of the sink is copied onto the output of our own thread.
-        //
-        // The basic idea is to not use a default Handler for rustc, and then also
-        // not print things by default to the actual stderr.
-        struct Sink(Arc<Mutex<Vec<u8>>>);
-        impl Write for Sink {
-            fn write(&mut self, data: &[u8]) -> io::Result<usize> {
-                Write::write(&mut *self.0.lock().unwrap(), data)
-            }
-            fn flush(&mut self) -> io::Result<()> { Ok(()) }
+    // Shuffle around a few input and output handles here. We're going to pass
+    // an explicit handle into rustc to collect output messages, but we also
+    // want to catch the error message that rustc prints when it fails.
+    //
+    // We take our thread-local stderr (likely set by the test runner) and replace
+    // it with a sink that is also passed to rustc itself. When this function
+    // returns the output of the sink is copied onto the output of our own thread.
+    //
+    // The basic idea is to not use a default Handler for rustc, and then also
+    // not print things by default to the actual stderr.
+    struct Sink(Arc<Mutex<Vec<u8>>>);
+    impl Write for Sink {
+        fn write(&mut self, data: &[u8]) -> io::Result<usize> {
+            Write::write(&mut *self.0.lock().unwrap(), data)
         }
-        struct Bomb(Arc<Mutex<Vec<u8>>>, Box<Write+Send>);
-        impl Drop for Bomb {
-            fn drop(&mut self) {
-                let _ = self.1.write_all(&self.0.lock().unwrap());
-            }
+        fn flush(&mut self) -> io::Result<()> { Ok(()) }
+    }
+    struct Bomb(Arc<Mutex<Vec<u8>>>, Box<Write+Send>);
+    impl Drop for Bomb {
+        fn drop(&mut self) {
+            let _ = self.1.write_all(&self.0.lock().unwrap());
         }
-        let data = Arc::new(Mutex::new(Vec::new()));
+    }
+    let data = Arc::new(Mutex::new(Vec::new()));
+
+    let old = io::set_panic(Some(box Sink(data.clone())));
+    let _bomb = Bomb(data.clone(), old.unwrap_or(box io::stdout()));
+
+    let (libdir, outdir, compile_result) = driver::spawn_thread_pool(sessopts, |sessopts| {
         let codemap = Lrc::new(CodeMap::new_doctest(
             sessopts.file_path_mapping(), filename.clone(), line as isize - line_offset as isize
         ));
         let emitter = errors::emitter::EmitterWriter::new(box Sink(data.clone()),
                                                         Some(codemap.clone()),
                                                         false,
                                                         false);
-        let old = io::set_panic(Some(box Sink(data.clone())));
-        let _bomb = Bomb(data.clone(), old.unwrap_or(box io::stdout()));
 
         // Compile the code
         let diagnostic_handler = errors::Handler::with_emitter(true, false, box emitter);
@@ -312,28 +314,28 @@ fn run_test(test: &str, cratename: &str, filename: &FileName, line: usize,
             Err(_) | Ok(Err(CompileIncomplete::Errored(_))) => Err(())
         };
 
-        match (compile_result, compile_fail) {
-            (Ok(()), true) => {
-                panic!("test compiled while it wasn't supposed to")
-            }
-            (Ok(()), false) => {}
-            (Err(()), true) => {
-                if error_codes.len() > 0 {
-                    let out = String::from_utf8(data.lock().unwrap().to_vec()).unwrap();
-                    error_codes.retain(|err| !out.contains(err));
-                }
-            }
-            (Err(()), false) => {
-                panic!("couldn't compile the test")
+        (libdir, outdir, compile_result)
+    });
+
+    match (compile_result, compile_fail) {
+        (Ok(()), true) => {
+            panic!("test compiled while it wasn't supposed to")
+        }
+        (Ok(()), false) => {}
+        (Err(()), true) => {
+            if error_codes.len() > 0 {
+                let out = String::from_utf8(data.lock().unwrap().to_vec()).unwrap();
+                error_codes.retain(|err| !out.contains(err));
             }
         }
-
-        if error_codes.len() > 0 {
-            panic!("Some expected error codes were not found: {:?}", error_codes);
+        (Err(()), false) => {
+            panic!("couldn't compile the test")
         }
+    }
 
-        (libdir, outdir)
-    });
+    if error_codes.len() > 0 {
+        panic!("Some expected error codes were not found: {:?}", error_codes);
+    }
 
     if no_run { return }
 
@@ -548,7 +550,7 @@ impl Collector {
         debug!("Creating test {}: {}", name, test);
         self.tests.push(testing::TestDescAndFn {
             desc: testing::TestDesc {
-                name: testing::DynTestName(name),
+                name: testing::DynTestName(name.clone()),
                 ignore: should_ignore,
                 // compiler failures are test failures
                 should_panic: testing::ShouldPanic::No,
@@ -558,7 +560,7 @@ impl Collector {
                 let panic = io::set_panic(None);
                 let print = io::set_print(None);
                 match {
-                    rustc_driver::in_rustc_thread(move || with_globals(move || {
+                    rustc_driver::in_named_rustc_thread(name, move || with_globals(move || {
                         io::set_panic(panic);
                         io::set_print(print);
                         hygiene::set_default_edition(edition);

src/test/rustdoc-ui/failed-doctest-output.rs

@@ -0,0 +1,29 @@
+// Copyright 2013-2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Issue #51162: A failed doctest was not printing its stdout/stderr
+// FIXME: if/when the output of the test harness can be tested on its own, this test should be
+// adapted to use that, and that normalize line can go away
+
+// compile-flags:--test
+// normalize-stdout-test: "src/test/rustdoc-ui" -> "$$DIR"
+// failure-status: 101
+
+// doctest fails at runtime
+/// ```
+/// panic!("oh no");
+/// ```
+pub struct SomeStruct;
+
+// doctest fails at compile time
+/// ```
+/// no
+/// ```
+pub struct OtherStruct;

src/test/rustdoc-ui/failed-doctest-output.stdout

@@ -0,0 +1,32 @@
+
+running 2 tests
+test $DIR/failed-doctest-output.rs - OtherStruct (line 26) ... FAILED
+test $DIR/failed-doctest-output.rs - SomeStruct (line 20) ... FAILED
+
+failures:
+
+---- $DIR/failed-doctest-output.rs - OtherStruct (line 26) stdout ----
+error[E0425]: cannot find value `no` in this scope
+ --> $DIR/failed-doctest-output.rs:27:1
+  |
+3 | no
+  | ^^ not found in this scope
+
+thread '$DIR/failed-doctest-output.rs - OtherStruct (line 26)' panicked at 'couldn't compile the test', librustdoc/test.rs:332:13
+note: Run with `RUST_BACKTRACE=1` for a backtrace.
+
+---- $DIR/failed-doctest-output.rs - SomeStruct (line 20) stdout ----
+thread '$DIR/failed-doctest-output.rs - SomeStruct (line 20)' panicked at 'test executable failed:
+
+thread 'main' panicked at 'oh no', $DIR/failed-doctest-output.rs:3:1
+note: Run with `RUST_BACKTRACE=1` for a backtrace.
+
+', librustdoc/test.rs:367:17
+
+
+failures:
+    $DIR/failed-doctest-output.rs - OtherStruct (line 26)
+    $DIR/failed-doctest-output.rs - SomeStruct (line 20)
+
+test result: FAILED. 0 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out
+

src/tools/compiletest/src/header.rs

@@ -392,18 +392,20 @@ impl TestProps {
 
             if let Some(code) = config.parse_failure_status(ln) {
                 self.failure_status = code;
-            } else {
-                self.failure_status = match config.mode {
-                    Mode::RunFail => 101,
-                    _ => 1,
-                };
             }
 
             if !self.run_rustfix {
                 self.run_rustfix = config.parse_run_rustfix(ln);
             }
         });
 
+        if self.failure_status == -1 {
+            self.failure_status = match config.mode {
+                Mode::RunFail => 101,
+                _ => 1,
+            };
+        }
+
         for key in &["RUST_TEST_NOCAPTURE", "RUST_TEST_THREADS"] {
             if let Ok(val) = env::var(key) {
                 if self.exec_env.iter().find(|&&(ref x, _)| x == key).is_none() {