[rustdoc] Gracefully handle error in case we cannot run the compiler in doctests #147912

GuillaumeGomez · 2025-10-20T16:04:41Z

Fixes bug reported in this comment.

r? @lolbinarycat

lolbinarycat · 2025-10-20T17:51:30Z

I think we should include the path of the file we are trying to execute in the error
(related) the message "failed to spawn rustc process" is a bit misleading, since (as far as I understand it) the entire point of this flag is you can pass an executable that isn't rustc, and such an error would likely cause an error to debug issues with rustc, instead of debugging their wrapper.

I think a message like "failed to spawn {compiler_path:?}: {error:?}" would be significantly more clear and helpful.

GuillaumeGomez · 2025-10-21T10:25:57Z

Agreed, improved error messages.

GuillaumeGomez · 2025-10-31T14:11:30Z

ping @lolbinarycat

lolbinarycat · 2025-10-31T16:18:42Z

I would prefer using Command::get_program instead of returning a tuple with duplicate data.

Alternatively, we could just throw the whole command into the error message through it's Debug impl, but that might be a bit noisy.

GuillaumeGomez · 2025-10-31T17:11:57Z

Thanks, much better this way.

lolbinarycat · 2025-10-31T17:27:05Z

src/librustdoc/doctest.rs

@@ -500,7 +501,7 @@ fn add_exe_suffix(input: String, target: &TargetTuple) -> String {
    input + &exe_suffix
 }

-fn wrapped_rustc_command(rustc_wrappers: &[PathBuf], rustc_binary: &Path) -> Command {
+fn wrapped_rustc_command<'a>(rustc_wrappers: &'a [PathBuf], rustc_binary: &'a Path) -> Command {


Why the explicit lifetime? leftover from when it returned a tuple?

Oups indeed, removing that.

lolbinarycat · 2025-10-31T17:30:18Z

src/librustdoc/doctest.rs

@@ -670,7 +671,14 @@ fn run_test(

    debug!("compiler invocation for doctest: {compiler:?}");

-    let mut child = compiler.spawn().expect("Failed to spawn rustc process");
+    let binary_path = compiler.get_program().to_os_string();


I don't think we need to clone this, it's not like spawn consumes the command or holds a mutable borrow to it, so we just need to make sure we're calling get_program after spawn.

I was sure spawn was taking self and not &mut self. Well, good to know. :)

lolbinarycat · 2025-10-31T17:31:21Z

src/librustdoc/doctest.rs

+            let binary_path = runner_compiler.get_program().to_os_string();
+            let mut child_runner = match runner_compiler.spawn() {
+                Ok(child) => child,
+                Err(error) => {
+                    eprintln!("Failed to spawn {binary_path:?}: {error:?}");
+                    return (Duration::default(), Err(TestFailure::CompileError));
+                }


Suggested change

let binary_path = runner_compiler.get_program().to_os_string();

let mut child_runner = match runner_compiler.spawn() {

Ok(child) => child,

Err(error) => {

eprintln!("Failed to spawn {binary_path:?}: {error:?}");

return (Duration::default(), Err(TestFailure::CompileError));

}

let mut child_runner = match runner_compiler.spawn() {

Ok(child) => child,

Err(error) => {

let binary_path = runner_compiler.get_program();

eprintln!("Failed to spawn {binary_path:?}: {error:?}");

return (Duration::default(), Err(TestFailure::CompileError));

}

as explained above

lolbinarycat · 2025-10-31T17:37:10Z

tests/run-make/rustdoc-test-builder/rmake.rs

+        .arg("--test-builder")
+        .arg(&absolute_path)
+        .run_fail();
+


Lets also assert that the exit code is 1 (ICE gives 101, like other panics), seems more robust than string comparisons.

panic! isn't always 101 as exit code (depends on the target). So we can't really check for that. ^^'

Discovered that while reading https://github.com/rust-lang/rust/blob/master/library/test/src/test_result.rs#L107.

I mean, you can just stick it behind a cfg(unix) or something, it would still me more reliable than only looking at the strings.

Is normal error exit always 1? Because that's what I was reccomending we actually put in the assertion.

In this case should be enough.

Hum, when we fail doctests, we exit with 101. ^^'

GuillaumeGomez · 2025-10-31T23:11:56Z

Updated!

rustbot · 2025-11-03T16:48:23Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

GuillaumeGomez · 2025-11-03T16:48:42Z

So I added the assert for 101 exit code and limited the test to be run only on linux target.

lolbinarycat · 2025-11-03T18:19:27Z

If 101 is used both for failing to build doctests and for ICE, then checking exit code unfortunately won't actually help us here and we might as well revert it and have it run on all platforms.

I think what I would actually like is for there to be a function in run-make-support that can reliably detect if an ICE happened, and we use that in several tests (including those that intentionally ICE) so if it stops working we'll get test failures. I think that would be better done as a follow-up

GuillaumeGomez · 2025-11-03T19:24:40Z

I really like this idea! Gonna need to involve someone else from run-make-support side. Gonna ping them once the change is done.

rustbot · 2025-11-03T19:56:24Z

The run-make-support library was changed

cc @jieyouxu

GuillaumeGomez · 2025-11-03T19:56:57Z

@jieyouxu: I added a new CompletedProcess::assert_ice method which checks if the ICE string is found in the stderr.

lolbinarycat · 2025-11-03T21:24:19Z

src/tools/run-make-support/src/command.rs

+    /// Checks that `stderr` contains the Internal Compiler Error message.
+    #[track_caller]
+    pub fn assert_ice(&self) -> &Self {


Suggested change

/// Checks that `stderr` contains the Internal Compiler Error message.

#[track_caller]

pub fn assert_ice(&self) -> &Self {

/// Checks that `stderr` does not contain the Internal Compiler Error message.

#[track_caller]

pub fn assert_not_ice(&self) -> &Self {

Previously it looked like a function that asserted that the compiler did encounter an ICE error.

Euh yeah, completely reversed everything...

GuillaumeGomez · 2025-11-04T10:33:58Z

Fixed doc and method's name.

jieyouxu · 2025-11-04T11:48:26Z

@jieyouxu: I added a new CompletedProcess::assert_ice method which checks if the ICE string is found in the stderr.

Seems good 👍

lolbinarycat

I just realized we don't actually have any test coverage for the non-error case of --test-builder.

Unsure if we want to add that here or in a followup, I'm not familiar enough with the option to know how much effort that would be.

View changes since this review

lolbinarycat · 2025-11-05T21:29:21Z

tests/run-make/rustdoc-test-builder/rmake.rs

+        .arg(&absolute_path)
+        .run_fail();
+
+    // We also double-check that we don't have the panic text in the output.


The wording of this comment doesn't really fit anymore

Arf indeed, missed it.

lolbinarycat · 2025-11-05T21:33:05Z

@rustbot author

GuillaumeGomez · 2025-11-05T21:43:52Z

I just realized we don't actually have any test coverage for the non-error case of --test-builder.

That's... well pretty bad. Gonna open an issue for that.

Anyway, updated.

lolbinarycat · 2025-11-05T22:02:29Z

Since this is nightly-only and the actual changes to rustdoc are so simple, I think it's fine to push this through without waiting for for --test-builder to have more robust testing.

@bors r+

bors · 2025-11-05T22:02:31Z

📌 Commit 2c3c82c has been approved by lolbinarycat

It is now in the queue for this repository.

…ror-handling, r=lolbinarycat [rustdoc] Gracefully handle error in case we cannot run the compiler in doctests Fixes bug reported in [this comment](rust-lang#102981 (comment)). r? `@lolbinarycat`

Rollup of 7 pull requests Successful merges: - #145656 (Stabilize s390x `vector` target feature and `is_s390x_feature_detected!` macro) - #147043 (Add default sanitizers to TargetOptions) - #147803 (Add -Zannotate-moves for profiler visibility of move/copy operations (codegen)) - #147912 ([rustdoc] Gracefully handle error in case we cannot run the compiler in doctests) - #148540 (Minor fixes to StdNonZeroNumberProvider for gdb) - #148541 (Add num_children method to some gdb pretty-printers) - #148549 (Fix broken qemu-cskyv2 link) Failed merges: - #147586 (std-detect: improve detect macro docs) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 7 pull requests Successful merges: - #143037 (Make named asm_labels lint not trigger on hexagon register spans) - #147043 (Add default sanitizers to TargetOptions) - #147586 (std-detect: improve detect macro docs) - #147912 ([rustdoc] Gracefully handle error in case we cannot run the compiler in doctests) - #148540 (Minor fixes to StdNonZeroNumberProvider for gdb) - #148541 (Add num_children method to some gdb pretty-printers) - #148549 (Fix broken qemu-cskyv2 link) Failed merges: - #147935 (Add LLVM realtime sanitizer) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #147912 - GuillaumeGomez:graceful-doctest-error-handling, r=lolbinarycat [rustdoc] Gracefully handle error in case we cannot run the compiler in doctests Fixes bug reported in [this comment](#102981 (comment)). r? ``@lolbinarycat``

rustbot assigned lolbinarycat Oct 20, 2025

GuillaumeGomez force-pushed the graceful-doctest-error-handling branch from 4e0541a to 0c495ae Compare October 21, 2025 10:23

GuillaumeGomez changed the title ~~[rustdoc] Gracefully handle in case we cannot run the compiler in doctests~~ [rustdoc] Gracefully handle error in case we cannot run the compiler in doctests Oct 22, 2025

GuillaumeGomez force-pushed the graceful-doctest-error-handling branch from 0c495ae to ce42e06 Compare October 31, 2025 17:11

lolbinarycat reviewed Oct 31, 2025

View reviewed changes

GuillaumeGomez force-pushed the graceful-doctest-error-handling branch from ce42e06 to 29fe476 Compare October 31, 2025 23:11

This comment has been minimized.

Sign in to view

Gracefully handle in case we cannot run the compiler in doctests

ec1b2fa

GuillaumeGomez force-pushed the graceful-doctest-error-handling branch from 29fe476 to db69dbc Compare November 3, 2025 16:48

Add regression test when unable to run compiler in doctest

5d0011c

GuillaumeGomez force-pushed the graceful-doctest-error-handling branch from db69dbc to bc50a23 Compare November 3, 2025 19:56

lolbinarycat reviewed Nov 3, 2025

View reviewed changes

GuillaumeGomez force-pushed the graceful-doctest-error-handling branch from bc50a23 to ea38070 Compare November 4, 2025 10:33

lolbinarycat reviewed Nov 5, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2025

Add new run_make_support::CompletedProcess::assert_ice method

2c3c82c

GuillaumeGomez force-pushed the graceful-doctest-error-handling branch from ea38070 to 2c3c82c Compare November 5, 2025 21:43

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2025

Zalathar mentioned this pull request Nov 6, 2025

Rollup of 7 pull requests #148553

Closed

Zalathar mentioned this pull request Nov 6, 2025

Rollup of 7 pull requests #148560

Merged

bors merged commit dd80361 into rust-lang:master Nov 6, 2025
11 checks passed

rustbot added this to the 1.93.0 milestone Nov 6, 2025

GuillaumeGomez deleted the graceful-doctest-error-handling branch November 6, 2025 16:54

github-actions bot mentioned this pull request Nov 7, 2025

Toolchain upgrade to nightly-2025-11-07 flux-rs/flux#1345

Draft

[rustdoc] Gracefully handle error in case we cannot run the compiler in doctests #147912

[rustdoc] Gracefully handle error in case we cannot run the compiler in doctests #147912

Uh oh!

Conversation

GuillaumeGomez commented Oct 20, 2025

Uh oh!

lolbinarycat commented Oct 20, 2025

Uh oh!

GuillaumeGomez commented Oct 21, 2025

Uh oh!

GuillaumeGomez commented Oct 31, 2025

Uh oh!

lolbinarycat commented Oct 31, 2025

Uh oh!

GuillaumeGomez commented Oct 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

GuillaumeGomez commented Oct 31, 2025

Uh oh!

rustbot commented Nov 3, 2025

Uh oh!

GuillaumeGomez commented Nov 3, 2025

Uh oh!

lolbinarycat commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuillaumeGomez commented Nov 3, 2025

Uh oh!

rustbot commented Nov 3, 2025

Uh oh!

GuillaumeGomez commented Nov 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Nov 4, 2025

Uh oh!

jieyouxu commented Nov 4, 2025

Uh oh!

lolbinarycat left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lolbinarycat commented Nov 5, 2025

Uh oh!

GuillaumeGomez commented Nov 5, 2025

Uh oh!

lolbinarycat commented Nov 5, 2025

Uh oh!

bors commented Nov 5, 2025

Uh oh!

Uh oh!

Reviewers

GuillaumeGomez Oct 31, 2025 •

edited

Loading

lolbinarycat commented Nov 3, 2025 •

edited

Loading

lolbinarycat left a comment •

edited by rustbot

Loading