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

[GuillaumeGomez]

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]

This PR modifies run-make tests.

cc @jieyouxu

[purplesyringa]

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]

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

EDIT: Updated comment.

[purplesyringa]

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]

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]

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]

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

[purplesyringa]

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

[GuillaumeGomez]

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]

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]

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]

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

[bors]

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

[GuillaumeGomez]

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

[GuillaumeGomez]

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

[rust-bors]

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

[GuillaumeGomez]

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

[GuillaumeGomez]

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

[rust-bors]

☀️ 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

[GuillaumeGomez]

Let's go again then!

@bors r=fmease

[bors]

📌 Commit 6060bcc has been approved by fmease

It is now in the queue for this repository.

[Zalathar]

Possibly failed in rollup in armhf-gnu: #147505 (comment)

@bors r-

[Zalathar]

@bors try jobs=armhf-gnu

[rust-log-analyzer]

The job armhf-gnu failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
---- library/alloc/src/slice.rs - slice::[T]::repeat (line 498) stdout end ----
---- library/alloc/src/vec/mod.rs - vec::Vec (line 253) stdout ----

thread 'library/alloc/src/vec/mod.rs - vec::Vec (line 253)' (11926) panicked at /tmp/rustdoctesthWSKOt/doctest_bundle_2024.rs:8785:17:
index out of bounds: the len is 4 but the index is 6
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_bounds_check
   3: <usize as core::slice::index::SliceIndex<[T]>>::index
   4: <alloc::vec::Vec<T,A> as core::ops::index::Index<I>>::index
   5: doctest_bundle_2024::__doctest_578::main
   6: doctest_bundle_2024::__doctest_578::__main_fn
   7: doctest_runner_2024::__doctest_578::TEST::{{closure}}
   8: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
---- library/alloc/src/vec/mod.rs - vec::Vec (line 253) stdout end ----

failures:

[rust-bors]

💔 Test for 4d1b974 failed: CI. Failed jobs:

• try - armhf-gnu (web logs, extended logs)

[purplesyringa]

This has been buggy since 796c4ef because new_doctests always receives false for should_panic. doctest_path returns None on this CI, so test::assert_test_result(doctest_bundle::{test_id}::__main_fn())) is invoked, and it panics; and new_doctest assumes the test shouldn't panic, so it says the test failed.

[GuillaumeGomez]

Superseded by #147674.

[GuillaumeGomez]

@purplesyringa: Thanks for the comment! So the problem is actually that should_panic tests are ignored by #[test] on some targets and rustdoc doesn't do this check.