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

GuillaumeGomez · 2025-07-13T18:52:32Z

Fixes #143009.
Fixes #143858.

Since it includes fixes from #143453, it's taking it over (commits 2, 3 and 4 are from #143453).

For --no-run, we forgot to check the "global" options in the 2024 edition, fixed in the first commit.

For should_panic fix, the exit code check has been fixed.

cc @TroyKomodo (thanks so much for providing such a complete test, made my life a lot easier!)
r? @notriddle

rustbot · 2025-07-13T18:52:40Z

This PR modifies run-make tests.

cc @jieyouxu

purplesyringa · 2025-07-14T02:57:10Z

For should_panic fix, instead of checking the exit code, we directly wrap the doctest code with catch_unwind and if a panic happened, then we return success for the test, otherwise we display the appropriate error message.

I... do not see anything like that in the patch? There's two checks for exit code 101.

GuillaumeGomez · 2025-07-14T10:17:00Z

Woups, copied/pasted comment from original PR which was outdated after discussion with you (on the previous PR). ^^'

EDIT: Updated comment.

purplesyringa · 2025-07-14T10:26:41Z

if langstr.should_panic {
    if out.status.code() == Some(101) {
        return Ok(());
    } else if out.status.success() {
        return Err(TestFailure::UnexpectedRunPass);
    }
}
if !out.status.success() {
    return Err(TestFailure::ExecutionFailure(out));
}

Do you think something like this would make sense, so that it's easier for the user to differentiate between aborts and panic-less success?

GuillaumeGomez · 2025-07-14T10:29:30Z

TestFailure::UnexpectedRunPass displays "Test didn't panic, but it's marked `should_panic`.", which means we'd need to add a new case. Do you think it's necessary to differentiate between success and non-panic failures?

purplesyringa · 2025-07-14T10:31:43Z

Wouldn't changing UnexpectedRunPass to spell "Test executable succeeded, but it's marked should_panic." (i.e. rolling back the change in this PR) work? After all, if the change was to let UnexpectedRunPass represent non-panicking failures, it won't be necessary if we just use ExecutionFailure for that.

GuillaumeGomez · 2025-07-14T10:32:54Z

That wouldn't cover exit(1) anymore if we did so.

purplesyringa · 2025-07-14T10:34:19Z

I've amended my comment a moment before you replied, apologies.

GuillaumeGomez · 2025-07-14T10:37:44Z

Replied too fast then. 😅

The message for ExecutionFailure is too general imo ("Test executable failed"). It doesn't help to understand should_panic didn't panic. I think once this PR is merged, you can send a new one to create a new variant in case it failed but didn't panic, like that we can have a message that is covering this case. What do you think?

purplesyringa · 2025-07-14T10:39:38Z

The message for ExecutionFailure is too general imo ("Test executable failed"). It doesn't help to understand should_panic didn't panic.

I think we're talking past each other. I suggest that:

If the program didn't panic and exited with code 0, we keep using UnexpectedRunPass ("test didn't panic"). This is the main use case for should_panic and has a perfectly legible description.
If the program didn't panic and exited with a non-zero code, we use ExecutionFailure ("executable failed"). I believe that it's highly unlikely this failure mode of a should_panic test can be attributed to anything but UB or an abort, exactly like with non-should_panic tests, so I don't think it's necessary to specify that it's happened in a should_panic test specifically.

Wouldn't that work?

GuillaumeGomez · 2025-07-14T10:43:18Z

I still think it's too general. For end users, should_panic means the execution is supposed to fail. However it might be confusing to get "executable failed" when it's exactly what's expected (well, except it was expecting a panic and it's different). Hence why I think the current message is better and why, if we go down this road, we should add a new case to handle this case instead of relying on the too generated ExecutionFailure.

purplesyringa · 2025-07-14T10:44:24Z

That makes sense, yeah. Let's delay it until after this PR lands then.

bors · 2025-07-30T22:29:39Z

☔ The latest upstream changes (presumably #144692) made this pull request unmergeable. Please resolve the merge conflicts.

…Amanieu Make `libtest::ERROR_EXIT_CODE` const public to not redefine it in rustdoc I think it's better to make this constant public so it can be used by crates using `libtest` as dependency. As a side-note, I will update rust-lang#143900 to make use of this constant once this is current PR is merged.

Zalathar · 2025-10-07T11:43:18Z

@bors try jobs=x86_64-gnu-aux

[rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition try-job: x86_64-gnu-aux

rust-bors · 2025-10-07T12:48:50Z

💔 Test for 75572d4 failed: CI. Failed jobs:

try - x86_64-gnu-aux (web logs, extended logs)

GuillaumeGomez · 2025-10-07T12:52:05Z

Ah, this time a cargo test failed. Checking what's wrong.

GuillaumeGomez · 2025-10-07T14:28:19Z

@bors try jobs=x86_64-gnu-aux,test-various

[rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition try-job: x86_64-gnu-aux try-job: test-various

rust-bors · 2025-10-07T16:29:05Z

☀️ Try build successful (CI)
Build commit: 1514cdd (1514cddf55f57c7a67702bced791e2db666e031f, parent: 4a54b26d30dac43778afb0e503524b763fce0eee)

GuillaumeGomez · 2025-10-07T16:36:34Z

This failure definitely illustrates why I should finish writing the lint in case the merged doctests failed compilation...

GuillaumeGomez · 2025-10-07T17:01:17Z

@bors try jobs=x86_64-gnu-aux,test-various

[rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition try-job: x86_64-gnu-aux try-job: test-various

rust-bors · 2025-10-07T19:02:59Z

☀️ Try build successful (CI)
Build commit: 7c0af15 (7c0af154b8a6f0d2bb19a467cc9916ce269f3613, parent: 4a54b26d30dac43778afb0e503524b763fce0eee)

fmease

Some suggestions and questions. As requested, feel free to r=me once they're all addressed in one way or another.

Although, I've got to admit that I feel slightly uneasy about some of these changes or rather the fact that this will ship "insta-stably". Anyhow, it's probably fine.

View changes since this review

fmease · 2025-10-08T00:40:48Z

library/std/src/error.rs

Could you at least consolidate b001ba6 and 030b664?

While your PR is technically only fixing bugs, at least the should_panic changes are actually breaking changes (rustdoc never performed these checks before) as can be seen by the modifications that you had to do to the standard library.

Because of that I've now added relnotes, so users will at least kind of get to know what's up when their doctests "break" which will inevitably happen (I still don't know what the stability policies / guarantees are for rustdoc ^^' they seem to be laxer compared to the language ones).

fmease · 2025-10-08T00:55:13Z

src/librustdoc/doctest.rs

+        && (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(()));


So this marks them as OK? Wouldn't it be more ideal to mark them as ignored (somehow)? Does that make any sense? I didn't have the time to refamiliarize myself with these parts of the doctest impl.

They are compiled, so it's not ignore but no_run instead. And just like libtest, no_run are not marked.

fmease · 2025-10-08T01:09:29Z

src/librustdoc/doctest.rs

+        // (cfg!(target_family = "wasm") || cfg!(target_os = "zkvm"))
+        //     && !cfg!(target_os = "emscripten")
+        // ```
+        && let TargetTuple::TargetTuple(ref s) = rustdoc_options.target


_{We both know that this check is super brittle and icky :D We can ignore that for now I guess ^^'}

Okay, so I'm not super familiar with target specifications but ideally this would also handle TargetJson, right? The user may pass --target=custom.json which may specify the panic_strategy (IIUC if that's set to Some(Abort | ImmediateAbort) then that implies that unwinding panics are not supported?) as well as target_family and so on.

If what I write makes sense, could you at least leave a FIXME that we should somehow support TargetJson here, too?

Ideally, we would use some preexisting compiler API for querying this (one that provides a richer representation of the target), maybe there is one we could use?

We both know that this check is super brittle and icky :D We can ignore that for now I guess ^^'

Yes. T_T

Okay, so I'm not super familiar with target specifications but ideally this would also handle TargetJson, right? The user may pass --target=custom.json which may specify the panic_strategy (IIUC if that's set to Some(Abort | ImmediateAbort) then that implies that unwinding panics are not supported?) as well as target_family and so on.

If what I write makes sense, could you at least leave a FIXME that we should somehow support TargetJson here, too?

Yeah you're right, adding a FIXME.

Ideally, we would use some preexisting compiler API for querying this (one that provides a richer representation of the target), maybe there is one we could use?

Hopefully yes. I'll take time to investigate how to make this all clean afterwards. Adding FIXME comments.

fmease · 2025-10-08T01:16:29Z

src/librustdoc/doctest.rs

+                    // `ZX_TASK_RETCODE_EXCEPTION_KILL`.
+                    #[cfg(target_os = "fuchsia")]
+                    Some(ZX_TASK_RETCODE_EXCEPTION_KILL) => {}
+                    _ => return (duration, Err(TestFailure::UnexpectedRunPass)),


Could you rename the variant UnexpectedRunPass to something more appropriate now that it's no longer only used for successful exits but also for "non-panicking" ones?

Yeah good idea.

fmease · 2025-10-08T01:17:45Z

src/librustdoc/doctest.rs

            }
            TestFailure::UnexpectedRunPass => {
-                eprint!("Test executable succeeded, but it's marked `should_panic`.");
+                eprint!("Test didn't panic, but it's marked `should_panic`.");


It might be worthwhile to print the actual exit code / signal here.

Good idea, adding a regression test as well.

fmease · 2025-10-08T01:27:56Z

src/librustdoc/doctest.rs

+                    Some(test::ERROR_EXIT_CODE) => {}
+                    #[cfg(windows)]
+                    Some(STATUS_FAIL_FAST_EXCEPTION) => {}
+                    #[cfg(unix)]
+                    None if out.status.signal() == Some(SIGABRT) => {}


I know that you've copied this from libtest since you've told me but I'm a bit unsure about accepting aborts unconditionally even if -Cpanic=unwind.

I wondering if it would be more correct to only accept these (SIGABRT, …) if -Cpanic=abort. Similarly, I'm wondering if under -Cpanic=abort an exit code of 101 would not be deemed acceptable for should_panic.

It's 3AM for me, so I can't really judge this right now. I'm just a bit uncomfortable with the idea of immediately shipping these changes given that the scope of this "bug fix" PR has grown in scope. It feels like there are open design questions, I could be wrong tho and I don't want to block this on an FCP.

E.g., in the following examples, all tests pass IINM:

//@ compile-flags: --test -Cpanic=unwind //! ```should_panic //! std::process::abort(); //! ```

//@ compile-flags: --test -Cpanic=abort //! ```should_panic //! std::process::exit(101); //! ```

Should they? Feels kinda weird to me tbh but maybe I'm the only one.

Should we document any of these details in the rustdoc book?

To be transparent, I don't know how libtest's --test -Cpanic=abort -Zpanic_abort_tests works under the hood & is supposed to work. It seems to be able to detect aborting panic!()s but also recognize that std::process::abort() is not a panic! No clue how it does that, I have no time to investigate rn.

I plan to use libtest directly after this PR is merged because this is a mess currently. ^^'

fmease · 2025-10-08T01:38:26Z

src/librustdoc/doctest.rs

    let duration = instant.elapsed();
    if doctest.no_run {
        return (duration, Ok(()));
+    } else if doctest.langstr.should_panic


Should we leave a FIXME somewhere that we (probably) don't handle -Cpanic=immediate-abort correctly (very recently added)?

Maybe we do already? As I've PM'ed you, on master I actually get illegal instruction for panics under this new strategy and I don't know if that's intentional or not.

I believe that's due to llvm lowering core::intrinsics::abort to ub2 which gets translated to an illegal instruction

#81895

src/librustdoc/doctest.rs

Added missing FIXME comments

GuillaumeGomez · 2025-10-08T14:34:41Z

Let's go again then!

@bors r=fmease

bors · 2025-10-08T14:34:44Z

📌 Commit 6060bcc has been approved by fmease

It is now in the queue for this repository.

rustbot assigned notriddle Jul 13, 2025