rustdoc: Properly detect syntactically invalid doctests (to fix a regression) #148101

fmease · 2025-10-25T11:15:22Z

Previously, we would only consider doctests to be syntactically invalid (for our crude static analysis pass) if (we failed to lex it or if) the parser returned an Err(_)¹.

However, since the parser can actually recover from a slew of invalid forms, it may return Ok(node) even in the presence of syntax errors. E.g., stray outer attrs may just get jettisoned. Combine that with the fact that we (send all diagnostics into the ether and) only forward selected spans of the original source to the final runnable doctest, it led to issues like #147999.

To remedy that, check if dcx.has_errors().

Fixes #147999 (regressed in #138104).

FIXME: Okay, due to the existence of error code checks like compile_fail,E0726 I actually have to preserve some existing behavior here and continue wrapping certain classes of syntactically invalid doctests in functions, sigh.

Or if the one of the expr stmts was an ExprKind::Err(_) (a special case / hack to paper over things). ↩

fmease · 2025-10-25T11:53:51Z

src/librustdoc/doctest/make.rs

+    // The parser might've recovered from some syntax errors and thus returned `Ok(_)`.
+    // However, since we've suppressed diagnostic emission above, we must ensure that
+    // we mark the doctest as syntactically invalid. Otherwise, we can end up generating
+    // a runnable doctest that's free of any errors even though the source was malformed.


[rework comment]

fmease · 2025-10-25T11:59:30Z

src/librustdoc/doctest/make.rs

-    // Any errors in parsing should also appear when the doctest is compiled for real, so just
-    // send all the errors that the parser emits directly into a `Sink` instead of stderr.
-    let emitter = HumanEmitter::new(AutoStream::never(Box::new(io::sink())), translator);


It makes no sense to use a HumanEmitter here that sends things into the void. I'm pretty sure that leads to extra work (rendering the diagnostic) that gets thrown away immediately after. Use a SilentEmitter instead which exists precisely for such use cases.

fmease · 2025-10-25T12:02:48Z

tests/rustdoc-ui/doctest/doctest-output-include-fail.stdout

Here the parser recovers from a missing semicolon and returns Ok(_). As a result, we used to consider the doctest syntactically valid and thus applied all the usual preprocessing, most importantly adding #![allow(unused)].

Now that we consider it syntactically invalid (rightfully so!), it no longer enjoys this privilege. Hence the warning.

fmease · 2025-10-25T12:16:30Z

tests/rustdoc-ui/doctest/nocapture-fail.stderr

   |

-error: aborting due to 1 previous error
+error[E0601]: `main` function not found in crate `rust_out`


I consider this an unfortunate regression. As mentioned in my other review comment, this doctest no longer gets preprocessed (like adding a main if absent).

If you like to see this addressed, I can of course change it to not bail out on has_errors (but still set failed_to_parse to true) in order to forward has_main and so on correctly (the code might get a bit gnarly tho).

However in general (not necessarily in this specific case), wrapping invalid doctests in a function is always a trade-off (it may lead to better errors in some cases but to worse ones in others; and it may leak internal names in diagnostics like _doctest_main_tests_rustdoc_ui_doctest_comment_in_attr_134221_2_rs_11_0; see #142446 (comment) for context).

Okay, due to the existence of error code checks like compile_fail,ENNNN I have to preserve some of the original behavior actually (namely continuing to preprocess certain syntactically invalid doctests (at the very least ones that have syntax errors that the parser recovered from)).

fmease · 2025-10-25T12:16:46Z

tests/rustdoc-ui/doctest/recovered-syntax-error.rs

@@ -0,0 +1,9 @@
+// issue: <https://github.com/rust-lang/rust/issues/147999>


[add description]

fmease · 2025-10-25T12:18:40Z

src/librustdoc/doctest/make.rs

+        if self.failed_to_parse {
+            // If we failed to parse the doctest, there's no need to go generate a complete doctest,
+            // the diagnostics will be better this way.
+            debug!("failed to parse doctest:\n{test_code}");


Applies #138104 (comment).

fmease · 2025-10-25T12:19:59Z

src/librustdoc/doctest/make.rs

    let dcx = DiagCtxt::new(Box::new(emitter)).disable_warnings();
    let psess = ParseSess::with_dcx(dcx, Arc::new(SourceMap::new(FilePathMapping::empty())));
+    // Reset errors at the end so that we don't panic on delayed bugs.
+    let _defer = rustc_data_structures::defer(|| psess.dcx().reset_err_count());


Instead of having to ensure that we call reset_err_count() at every possible function exit, just use defer to make it happen automatically which is a lot more robust and legible.

fmease · 2025-10-25T12:34:59Z

src/librustdoc/doctest/make.rs

-    info.supports_color =
-        HumanEmitter::new(stderr_destination(ColorConfig::Auto), translator.clone())
-            .supports_color();


I've now fully removed this logic. Before I rebased past #147207, I actually simplified it to info.supports_color = stderr_destination(ColorConfig::Auto).supports_color();.

However, since said PR (#147207: migrating coloring crates), HumanEmitter::supports_color() unconditionally(!) returns false (in fact, Emitter::supports_color is no longer used by anyone else and should be removed), so there's no reason to keep it. Rephrased, since that PR all compiler diagnostics for doctests are uncolored.

You could argue that I should keep it and patch supports_color in rustc to "work again". However, I'd rather rework our doctest coloring wholesale in a separate PR. At least before that migration PR, our setup was quite busted:

First of all, it didn't query+set supports_color for syntactically invalid doctests, so syntax errors were always shown without color (contrary to e.g., name resolution errors).

Second of all, calling supports_color() here was quite frankly wrong: Piping the output of rustdoc … --test into a file (or | cat or whatever) did not suppress colors. I'm not actually sure if we can ever address that nicely (without stripping ANSI codes after the fact) since we pass that diagnostic to libtest, right? I might very well be wrong here, maybe it's a non-issue.

fmease · 2025-10-25T12:36:21Z

src/librustdoc/doctest/make.rs

-                        if matches!(expr.kind, ast::ExprKind::Err(_)) {
-                            reset_error_count(&psess);
-                            return Err(());
-                        }


We no longer need to do that. Due to the ErrorGuaranteed in ExprKind::Err we know that an error diagnostic had to be emitted prior meaning dcx.has_errors() has to be some (meaning we already bailed out with an Err(_) above)!

That is, even in the presence of parse error recovery.

fmease · 2025-10-25T12:54:20Z

compiler/rustc_attr_parsing/src/attributes/mod.rs

Prime example ^^

fmease · 2025-10-25T12:55:42Z

compiler/rustc_errors/src/emitter.rs

-    /// Checks if we can use colors in the current output stream.
-    fn supports_color(&self) -> bool {
-        false
-    }


See #148101 (comment).

rust-log-analyzer · 2025-10-25T13:37:16Z

The job aarch64-gnu-llvm-20-2 failed! Check out the build log: (web) (plain enhanced) (plain)

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

---- /checkout/obj/build/aarch64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0726 (line 15729) stdout ----
error[E0670]: `async fn` is not permitted in Rust 2015
##[error] --> /checkout/obj/build/aarch64-unknown-linux-gnu/test/error-index.md:15735:1
  |
6 | async fn create(content: Content) { // error: implicit elided
  | ^^^^^ to use `async fn`, switch to Rust 2018 or later
  |
  = help: pass `--edition 2024` to `rustc`
  = note: for more on editions, read https://doc.rust-lang.org/edition-guide

error: expected item, found keyword `let`
##[error]  --> /checkout/obj/build/aarch64-unknown-linux-gnu/test/error-index.md:15740:1
   |
11 | let content = Content { title: "Rust", body: "is great!" };
   | ^^^
   | |
   | `let` cannot be used for global variables
   | help: consider using `static` or `const` instead of `let`
   |
   = note: for a full list of items that can appear in modules, see <https://doc.rust-lang.org/reference/items.html>

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0670`.
Some expected error codes were not found: ["E0726"]

failures:
    /checkout/obj/build/aarch64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0726 (line 15729)

test result: FAILED. 1040 passed; 1 failed; 73 ignored; 0 measured; 0 filtered out; finished in 23.73s

Bootstrap failed while executing `--stage 2 test --skip tests --skip coverage-map --skip coverage-run --skip library --skip tidyselftest`
Command `/checkout/obj/build/bootstrap/debug/rustdoc -Wrustdoc::invalid_codeblock_attributes -Dwarnings -Znormalize-docs -Z unstable-options --test /checkout/obj/build/aarch64-unknown-linux-gnu/test/error-index.md --test-args ` failed with exit code 101
Created at: src/bootstrap/src/core/builder/mod.rs:1693:23
Executed at: src/bootstrap/src/core/build_steps/test.rs:2719:13

Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:38:14
  local time: Sat Oct 25 13:37:05 UTC 2025
  network time: Sat, 25 Oct 2025 13:37:05 GMT
##[error]Process completed with exit code 1.
##[group]Run echo "disk usage:"

fmease · 2025-10-25T14:32:23Z

src/librustdoc/doctest/make.rs

+    // Any parse errors should also appear when the doctest is compiled for real,
+    // so just suppress them here.


Also for e.g., compile_fail we don't necessary want to output any errors "unless e.g., --nocapture or --test-args --show-output" …; so it really isn't our responsibility.

Therefore elaborate that source comment.

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 25, 2025

fmease changed the title ~~rustdoc: Slightly refactor code pertaining to doctest parsing~~ rustdoc: Properly detect syntactically invalid doctests Oct 25, 2025

This comment has been minimized.

Sign in to view

fmease commented Oct 25, 2025

View reviewed changes

fmease added 2 commits October 25, 2025 14:53

rustdoc: Properly detect syntactically invalid doctests

39d00f4

That is, even in the presence of parse error recovery.

rustdoc: Slightly refactor code pertaining to doctest parsing

066a329

fmease force-pushed the recogn-syn-inva-doctests branch from 81c015e to 066a329 Compare October 25, 2025 12:53

rustbot added the A-attributes Area: Attributes (`#[…]`, `#![…]`) label Oct 25, 2025

fmease assigned GuillaumeGomez Oct 25, 2025

fmease commented Oct 25, 2025

View reviewed changes

compiler/rustc_attr_parsing/src/attributes/mod.rs

Copy link

Member Author

fmease Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prime example ^^

fmease commented Oct 25, 2025

View reviewed changes

fmease changed the title ~~rustdoc: Properly detect syntactically invalid doctests~~ rustdoc: Properly detect syntactically invalid doctests (to fix a regression) Oct 25, 2025

fmease commented Oct 25, 2025

View reviewed changes

		@@ -0,0 +1,9 @@
		// issue: <https://github.com/rust-lang/rust/issues/147999>

		// Any parse errors should also appear when the doctest is compiled for real,
		// so just suppress them here.

rustdoc: Properly detect syntactically invalid doctests (to fix a regression) #148101

Are you sure you want to change the base?

rustdoc: Properly detect syntactically invalid doctests (to fix a regression) #148101

Conversation

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

Uh oh!

Footnotes

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

fmease Oct 25, 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!

fmease Oct 25, 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!

fmease Oct 25, 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!

fmease Oct 25, 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!

fmease Oct 25, 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!

rust-log-analyzer commented Oct 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fmease commented Oct 25, 2025 •

edited

Loading

fmease Oct 25, 2025 •

edited

Loading

fmease Oct 25, 2025 •

edited

Loading

fmease Oct 25, 2025 •

edited

Loading

fmease Oct 25, 2025 •

edited

Loading

fmease Oct 25, 2025 •

edited

Loading