Auto merge of #143900 - GuillaumeGomez:fix-no-run, r=<try>

[rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition

try-job: armhf-gnu

Author: rust-bors[bot]

library/std/src/error.rs

@@ -123,7 +123,7 @@ use crate::fmt::{self, Write};
 /// the `Debug` output means `Report` is an ideal starting place for formatting errors returned
 /// from `main`.
 ///
-/// ```should_panic
+/// ```
 /// #![feature(error_reporter)]
 /// use std::error::Report;
 /// # use std::error::Error;
@@ -154,10 +154,14 @@ use crate::fmt::{self, Write};
 /// #     Err(SuperError { source: SuperErrorSideKick })
 /// # }
 ///
-/// fn main() -> Result<(), Report<SuperError>> {
+/// fn run() -> Result<(), Report<SuperError>> {
 ///     get_super_error()?;
 ///     Ok(())
 /// }
+///
+/// fn main() {
+///     assert!(run().is_err());
+/// }
 /// ```
 ///
 /// This example produces the following output:
@@ -170,7 +174,7 @@ use crate::fmt::{self, Write};
 /// output format. If you want to make sure your `Report`s are pretty printed and include backtrace
 /// you will need to manually convert and enable those flags.
 ///
-/// ```should_panic
+/// ```
 /// #![feature(error_reporter)]
 /// use std::error::Report;
 /// # use std::error::Error;
@@ -201,12 +205,16 @@ use crate::fmt::{self, Write};
 /// #     Err(SuperError { source: SuperErrorSideKick })
 /// # }
 ///
-/// fn main() -> Result<(), Report<SuperError>> {
+/// fn run() -> Result<(), Report<SuperError>> {
 ///     get_super_error()
 ///         .map_err(Report::from)
 ///         .map_err(|r| r.pretty(true).show_backtrace(true))?;
 ///     Ok(())
 /// }
+///
+/// fn main() {
+///     assert!(run().is_err());
+/// }
 /// ```
 ///
 /// This example produces the following output:

src/librustdoc/doctest.rs

@@ -7,6 +7,8 @@ mod rust;
 use std::fs::File;
 use std::hash::{Hash, Hasher};
 use std::io::{self, Write};
+#[cfg(unix)]
+use std::os::unix::process::ExitStatusExt;
 use std::path::{Path, PathBuf};
 use std::process::{self, Command, Stdio};
 use std::sync::atomic::{AtomicUsize, Ordering};
@@ -351,7 +353,7 @@ pub(crate) fn run_tests(
         );
 
         for (doctest, scraped_test) in &doctests {
-            tests_runner.add_test(doctest, scraped_test, &target_str);
+            tests_runner.add_test(doctest, scraped_test, &target_str, rustdoc_options);
         }
         let (duration, ret) = tests_runner.run_merged_tests(
             rustdoc_test_options,
@@ -461,8 +463,8 @@ enum TestFailure {
     ///
     /// This typically means an assertion in the test failed or another form of panic occurred.
     ExecutionFailure(process::Output),
-    /// The test is marked `should_panic` but the test binary executed successfully.
-    UnexpectedRunPass,
+    /// The test is marked `should_panic` but the test binary didn't panic.
+    NoPanic(Option<String>),
 }
 
 enum DirState {
@@ -801,6 +803,25 @@ fn run_test(
     let duration = instant.elapsed();
     if doctest.no_run {
         return (duration, Ok(()));
+    } else if doctest.langstr.should_panic
+        // Equivalent of:
+        //
+        // ```
+        // (cfg!(target_family = "wasm") || cfg!(target_os = "zkvm"))
+        //     && !cfg!(target_os = "emscripten")
+        // ```
+        //
+        // FIXME: All this code is terrible and doesn't take into account `TargetTuple::TargetJson`.
+        // If `libtest` doesn't allow to handle this case, we'll need to use a rustc's API instead.
+        && let TargetTuple::TargetTuple(ref s) = rustdoc_options.target
+        && let mut iter = s.split('-')
+        && let Some(arch) = iter.next()
+        && iter.next().is_some()
+        && let os = iter.next()
+        && (arch.starts_with("wasm") || os == Some("zkvm")) && os != Some("emscripten")
+    {
+        // We cannot correctly handle `should_panic` in some wasm targets so we exit early.
+        return (duration, Ok(()));
     }
 
     // Run the code!
@@ -831,12 +852,68 @@ fn run_test(
     } else {
         cmd.output()
     };
+
+    // FIXME: Make `test::get_result_from_exit_code` public and use this code instead of this.
+    //
+    // On Zircon (the Fuchsia kernel), an abort from userspace calls the
+    // LLVM implementation of __builtin_trap(), e.g., ud2 on x86, which
+    // raises a kernel exception. If a userspace process does not
+    // otherwise arrange exception handling, the kernel kills the process
+    // with this return code.
+    #[cfg(target_os = "fuchsia")]
+    const ZX_TASK_RETCODE_EXCEPTION_KILL: i32 = -1028;
+    // On Windows we use __fastfail to abort, which is documented to use this
+    // exception code.
+    #[cfg(windows)]
+    const STATUS_FAIL_FAST_EXCEPTION: i32 = 0xC0000409u32 as i32;
+    #[cfg(unix)]
+    const SIGABRT: std::ffi::c_int = 6;
     match result {
         Err(e) => return (duration, Err(TestFailure::ExecutionError(e))),
         Ok(out) => {
-            if langstr.should_panic && out.status.success() {
-                return (duration, Err(TestFailure::UnexpectedRunPass));
-            } else if !langstr.should_panic && !out.status.success() {
+            if langstr.should_panic {
+                match out.status.code() {
+                    Some(test::ERROR_EXIT_CODE) => {}
+                    #[cfg(windows)]
+                    Some(STATUS_FAIL_FAST_EXCEPTION) => {}
+                    #[cfg(unix)]
+                    None => match out.status.signal() {
+                        Some(SIGABRT) => {}
+                        Some(signal) => {
+                            return (
+                                duration,
+                                Err(TestFailure::NoPanic(Some(format!(
+                                    "Test didn't panic, but it's marked `should_panic` (exit signal: {signal}).",
+                                )))),
+                            );
+                        }
+                        None => {
+                            return (
+                                duration,
+                                Err(TestFailure::NoPanic(Some(format!(
+                                    "Test didn't panic, but it's marked `should_panic` and exited with no error code and no signal.",
+                                )))),
+                            );
+                        }
+                    },
+                    #[cfg(not(unix))]
+                    None => return (duration, Err(TestFailure::NoPanic(None))),
+                    // Upon an abort, Fuchsia returns the status code
+                    // `ZX_TASK_RETCODE_EXCEPTION_KILL`.
+                    #[cfg(target_os = "fuchsia")]
+                    Some(ZX_TASK_RETCODE_EXCEPTION_KILL) => {}
+                    Some(exit_code) => {
+                        let err_msg = if !out.status.success() {
+                            Some(format!(
+                                "Test didn't panic, but it's marked `should_panic` (exit status: {exit_code}).",
+                            ))
+                        } else {
+                            None
+                        };
+                        return (duration, Err(TestFailure::NoPanic(err_msg)));
+                    }
+                }
+            } else if !out.status.success() {
                 return (duration, Err(TestFailure::ExecutionFailure(out)));
             }
         }
@@ -1143,8 +1220,12 @@ fn doctest_run_fn(
             TestFailure::UnexpectedCompilePass => {
                 eprint!("Test compiled successfully, but it's marked `compile_fail`.");
             }
-            TestFailure::UnexpectedRunPass => {
-                eprint!("Test executable succeeded, but it's marked `should_panic`.");
+            TestFailure::NoPanic(msg) => {
+                if let Some(msg) = msg {
+                    eprint!("{msg}");
+                } else {
+                    eprint!("Test didn't panic, but it's marked `should_panic`.");
+                }
             }
             TestFailure::MissingErrorCodes(codes) => {
                 eprint!("Some expected error codes were not found: {codes:?}");

src/librustdoc/doctest/runner.rs

@@ -39,6 +39,7 @@ impl DocTestRunner {
         doctest: &DocTestBuilder,
         scraped_test: &ScrapedDocTest,
         target_str: &str,
+        opts: &RustdocOptions,
     ) {
         let ignore = match scraped_test.langstr.ignore {
             Ignore::All => true,
@@ -62,6 +63,7 @@ impl DocTestRunner {
                 self.nb_tests,
                 &mut self.output,
                 &mut self.output_merged_tests,
+                opts,
             ),
         ));
         self.supports_color &= doctest.supports_color;
@@ -121,26 +123,81 @@ impl DocTestRunner {
 {output}
 
 mod __doctest_mod {{
-    use std::sync::OnceLock;
+    #[cfg(unix)]
+    use std::os::unix::process::ExitStatusExt;
     use std::path::PathBuf;
     use std::process::ExitCode;
+    use std::sync::OnceLock;
 
     pub static BINARY_PATH: OnceLock<PathBuf> = OnceLock::new();
     pub const RUN_OPTION: &str = \"RUSTDOC_DOCTEST_RUN_NB_TEST\";
+    pub const SHOULD_PANIC_DISABLED: bool = (
+        cfg!(target_family = \"wasm\") || cfg!(target_os = \"zkvm\")
+    ) && !cfg!(target_os = \"emscripten\");
 
     #[allow(unused)]
     pub fn doctest_path() -> Option<&'static PathBuf> {{
         self::BINARY_PATH.get()
     }}
 
     #[allow(unused)]
-    pub fn doctest_runner(bin: &std::path::Path, test_nb: usize) -> ExitCode {{
+    pub fn doctest_runner(bin: &std::path::Path, test_nb: usize, should_panic: bool) -> ExitCode {{
         let out = std::process::Command::new(bin)
             .env(self::RUN_OPTION, test_nb.to_string())
             .args(std::env::args().skip(1).collect::<Vec<_>>())
             .output()
             .expect(\"failed to run command\");
-        if !out.status.success() {{
+        if should_panic {{
+            // FIXME: Make `test::get_result_from_exit_code` public and use this code instead of this.
+            //
+            // On Zircon (the Fuchsia kernel), an abort from userspace calls the
+            // LLVM implementation of __builtin_trap(), e.g., ud2 on x86, which
+            // raises a kernel exception. If a userspace process does not
+            // otherwise arrange exception handling, the kernel kills the process
+            // with this return code.
+            #[cfg(target_os = \"fuchsia\")]
+            const ZX_TASK_RETCODE_EXCEPTION_KILL: i32 = -1028;
+            // On Windows we use __fastfail to abort, which is documented to use this
+            // exception code.
+            #[cfg(windows)]
+            const STATUS_FAIL_FAST_EXCEPTION: i32 = 0xC0000409u32 as i32;
+            #[cfg(unix)]
+            const SIGABRT: std::ffi::c_int = 6;
+
+            match out.status.code() {{
+                Some(test::ERROR_EXIT_CODE) => ExitCode::SUCCESS,
+                #[cfg(windows)]
+                Some(STATUS_FAIL_FAST_EXCEPTION) => ExitCode::SUCCESS,
+                #[cfg(unix)]
+                None => match out.status.signal() {{
+                    Some(SIGABRT) => ExitCode::SUCCESS,
+                    Some(signal) => {{
+                        eprintln!(\"Test didn't panic, but it's marked `should_panic` (exit signal: {{signal}}).\");
+                        ExitCode::FAILURE
+                    }}
+                    None => {{
+                        eprintln!(\"Test didn't panic, but it's marked `should_panic` and exited with no error code and no signal.\");
+                        ExitCode::FAILURE
+                    }}
+                }},
+                #[cfg(not(unix))]
+                None => {{
+                    eprintln!(\"Test didn't panic, but it's marked `should_panic`.\");
+                    ExitCode::FAILURE
+                }}
+                // Upon an abort, Fuchsia returns the status code ZX_TASK_RETCODE_EXCEPTION_KILL.
+                #[cfg(target_os = \"fuchsia\")]
+                Some(ZX_TASK_RETCODE_EXCEPTION_KILL) => ExitCode::SUCCESS,
+                Some(exit_code) => {{
+                    if !out.status.success() {{
+                        eprintln!(\"Test didn't panic, but it's marked `should_panic` (exit status: {{exit_code}}).\");
+                    }} else {{
+                        eprintln!(\"Test didn't panic, but it's marked `should_panic`.\");
+                    }}
+                    ExitCode::FAILURE
+                }}
+            }}
+        }} else if !out.status.success() {{
             if let Some(code) = out.status.code() {{
                 eprintln!(\"Test executable failed (exit status: {{code}}).\");
             }} else {{
@@ -223,6 +280,7 @@ fn generate_mergeable_doctest(
     id: usize,
     output: &mut String,
     output_merged_tests: &mut String,
+    opts: &RustdocOptions,
 ) -> String {
     let test_id = format!("__doctest_{id}");
 
@@ -256,31 +314,33 @@ fn main() {returns_result} {{
         )
         .unwrap();
     }
-    let not_running = ignore || scraped_test.langstr.no_run;
+    let should_panic = scraped_test.langstr.should_panic;
+    let not_running = ignore || scraped_test.no_run(opts);
     writeln!(
         output_merged_tests,
         "
 mod {test_id} {{
 pub const TEST: test::TestDescAndFn = test::TestDescAndFn::new_doctest(
-{test_name:?}, {ignore}, {file:?}, {line}, {no_run}, {should_panic},
+{test_name:?}, {ignore} || ({should_panic} && crate::__doctest_mod::SHOULD_PANIC_DISABLED), {file:?}, {line}, {no_run}, false,
 test::StaticTestFn(
     || {{{runner}}},
 ));
 }}",
         test_name = scraped_test.name,
         file = scraped_test.path(),
         line = scraped_test.line,
-        no_run = scraped_test.langstr.no_run,
-        should_panic = !scraped_test.langstr.no_run && scraped_test.langstr.should_panic,
+        no_run = scraped_test.no_run(opts),
         // Setting `no_run` to `true` in `TestDesc` still makes the test run, so we simply
         // don't give it the function to run.
         runner = if not_running {
             "test::assert_test_result(Ok::<(), String>(()))".to_string()
         } else {
             format!(
                 "
-if let Some(bin_path) = crate::__doctest_mod::doctest_path() {{
-    test::assert_test_result(crate::__doctest_mod::doctest_runner(bin_path, {id}))
+if {should_panic} && crate::__doctest_mod::SHOULD_PANIC_DISABLED {{
+    test::assert_test_result(Ok::<(), String>(()))
+}} else if let Some(bin_path) = crate::__doctest_mod::doctest_path() {{
+    test::assert_test_result(crate::__doctest_mod::doctest_runner(bin_path, {id}, {should_panic}))
 }} else {{
     test::assert_test_result(doctest_bundle::{test_id}::__main_fn())
 }}

tests/run-make/rustdoc-should-panic/rmake.rs

@@ -0,0 +1,43 @@
+// Ensure that `should_panic` doctests only succeed if the test actually panicked.
+// Regression test for <https://github.com/rust-lang/rust/issues/143009>.
+
+//@ ignore-cross-compile
+
+use run_make_support::rustdoc;
+
+fn check_output(edition: &str, panic_abort: bool) {
+    let mut rustdoc_cmd = rustdoc();
+    rustdoc_cmd.input("test.rs").arg("--test").edition(edition);
+    if panic_abort {
+        rustdoc_cmd.args(["-C", "panic=abort"]);
+    }
+    let output = rustdoc_cmd.run_fail().stdout_utf8();
+    let should_contain = &[
+        "test test.rs - bad_exit_code (line 1) ... FAILED",
+        "test test.rs - did_not_panic (line 6) ... FAILED",
+        "test test.rs - did_panic (line 11) ... ok",
+        "---- test.rs - bad_exit_code (line 1) stdout ----
+Test executable failed (exit status: 1).",
+        "---- test.rs - did_not_panic (line 6) stdout ----
+Test didn't panic, but it's marked `should_panic` (exit status: 1).",
+        "test result: FAILED. 1 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out;",
+    ];
+    for text in should_contain {
+        assert!(
+            output.contains(text),
+            "output (edition: {edition}) doesn't contain {:?}\nfull output: {output}",
+            text
+        );
+    }
+}
+
+fn main() {
+    check_output("2015", false);
+
+    // Same check with the merged doctest feature (enabled with the 2024 edition).
+    check_output("2024", false);
+
+    // Checking that `-C panic=abort` is working too.
+    check_output("2015", true);
+    check_output("2024", true);
+}