rustdoc: Fix doctest heuristic for main fn wrapping #140420

fmease · 2025-04-29T00:26:54Z

Fixes #140412 which regressed in #140220 that I reviewed. As mentioned in #140220 (comment), at the time I didn't have the time to re-review its latest changes and should've therefore invalided my previous "r=me" and blocked the PR on another review given the fragile nature of the doctest impl. This didn't happen which is my fault.

TODO:

Create a proper regression test from the cargotest smoke test that used to fail in Fix detection of main function if there are expressions around it #140220.
tests/rustdoc-ui/doctest/macro-after-main.rs was incorrect and has been removed
and one for the latest failure, Regression in doctest compile_fail detection with global_asm! #140412.

Contains some other small changes. Diff best reviewed modulo whitespace.
r? @GuillaumeGomez

fmease · 2025-04-29T00:35:41Z

src/librustdoc/doctest/make.rs

-    // Recurse through functions body. It is necessary because the doctest source code is
-    // wrapped in a function to limit the number of AST errors. If we don't recurse into
-    // functions, we would thing all top-level items (so basically nothing).


Outdated since #138104 which has removed the recursion as suggested in #138104 (comment).

fmease · 2025-04-29T00:36:33Z

src/librustdoc/doctest/make.rs

-                // We only push if it's the top item because otherwise, we would duplicate
-                // its content since the top-level item was already added.


Outdated since #138104 which has removed the recursion as suggested in #138104 (comment).

fmease · 2025-04-29T00:38:32Z

src/librustdoc/doctest/make.rs

                        has_non_items = true;
                    }
+                    StmtKind::Let(_) | StmtKind::Semi(_) | StmtKind::Empty => has_non_items = true,


Replaced wildcard with exhaustive matching to prevent silently dropping non-items ever again (context: #140220 (comment)).

fmease · 2025-04-29T00:38:52Z

src/librustdoc/doctest/make.rs

                            }
                        }
                    }
-                    _ => {
+                    StmtKind::Expr(ref expr) => {


Moved this one closer to the other non-items.

fmease · 2025-04-29T00:39:55Z

src/librustdoc/doctest/make.rs

-                                    iter.next();
-                                    iter.peek()
+                        if !info.has_main_fn {
+                            // For backward compatibility, we look for the token sequence `fn main(…)`


Re-added the historical context that was dropped in #138104 (I rephrased it).

fmease · 2025-04-29T00:42:08Z

src/librustdoc/doctest/make.rs

                    StmtKind::MacCall(ref mac_call) => {
-                        if info.has_main_fn {
-                            continue;


This was the culprit that led to #140412: The continue skipped the entire span-to-string logic happening below the match which lead to the (compilefail) global_asm!(…); getting dropped right on the floor and thus not even appearing in the generated crate.

fmease · 2025-04-29T01:06:05Z

tests/rustdoc-ui/doctest/macro-after-main.rs

This doctest should definitely not pass! It never used to pass until the regression PR which added it. It's not a proper minimization of the diesel cargotest smoke test.

We're in an item context since there's a main fn and all three statements are considered items by us as expected. Therefore, the eprintln!(); which is only valid in expression contexts will and should lead to an error down the line.

The problem I identified was that the MacCall pattern was skipped if we already found a main function, making all other items "non function items", leading to the regression. My change was to instead always enter in this arm and inside it check if there is a main function only if we didn't find one yet.

The code does seem valid to me considering that you can generate items with macros inside a function.

Right, that was a real regression, I'm not refuting that. As I wrote in #140420 (comment), the problem was the continue you added as a fix which skipped the entire span-to-string logic which leads to us now dropping certain things on the floor.

With my PR applied, the ParseSourceInfo.everything_else contains:

include!("./auxiliary/macro-after-main.rs"); fn main() {} eprintln!();

Which is entirely reasonable (we consider all macro calls as item macro calls which is the only sensible thing to do with all the constraints we have).

However, on master (which continues) it contains:

include!("./auxiliary/macro-after-main.rs"); fn main() {}

Notice how we dropped the eprintln!();! That's why this doctest passes and tests the wrong things and also why ehuss's doctest in #140412 is no longer compile_fail: On master, it simply drops the malformed global_asm!(…);! That's what I'm fixing here.

Yes your new fix is good. However, this doctest should compile as it should be wrapped in a function since there are expressions (making the main function potentially generated by the macro not run).

However, this doctest should compile as it should be wrapped in a function since there are expressions

I still disagree. That's impossible without performing macro expansion, we have to assume that all macro calls expand to items as previously discussed (if there is a main fn ofc). There are no expressions as far as I'm concerned, include!(…);, fn main() {} and eprintln!(…); could all be items. We can't wrap it in a function because include!(…) isn't allowed in functions contexts, similarly global_asm!(…). We don't know that println!(…); expands to an expression, not without macro expansion.

What you're asking is not possible, we still need to keep the doctest from #140412 passing. Also, your suggestion doesn't reflect the behavior of all previous non-broken versions of rustdoc! You'd need an FCP for that. This here is meant to be a backport-on-a-backport PR to fix a beta regression.

Hum, I see. Yeah that makes sense. We should likely expand macros at some point. Saving that for a later time. :')

GuillaumeGomez · 2025-04-29T08:47:59Z

src/librustdoc/doctest/make.rs

                                }
-                            {
-                                info.has_main_fn = true;
-                                break;


Why removing this break?

I was tired and thought it would break to the outer for. Reverted.

GuillaumeGomez · 2025-04-29T08:52:33Z

Nah, not your fault. All tests passed and the "try" command was happy was well so I (wrongly) assumed that it was all good.

fmease added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 29, 2025

rustbot assigned GuillaumeGomez Apr 29, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 29, 2025

fmease removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 29, 2025

fmease force-pushed the rustdoc-fix-doctest-heur branch from 657e3dd to 944ee37 Compare April 29, 2025 00:53

fmease commented Apr 29, 2025

View reviewed changes

rustdoc: Fix doctest heuristic for main fn wrapping

714f6f2

fmease force-pushed the rustdoc-fix-doctest-heur branch from 944ee37 to 714f6f2 Compare April 29, 2025 00:57

fmease commented Apr 29, 2025

View reviewed changes

fmease marked this pull request as draft April 29, 2025 08:04

GuillaumeGomez reviewed Apr 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Fix doctest heuristic for main fn wrapping #140420

rustdoc: Fix doctest heuristic for main fn wrapping #140420

fmease commented Apr 29, 2025 •

edited

Loading

fmease Apr 29, 2025 •

edited

Loading

fmease Apr 29, 2025 •

edited

Loading

fmease Apr 29, 2025

fmease Apr 29, 2025

fmease Apr 29, 2025

fmease Apr 29, 2025 •

edited

Loading

fmease Apr 29, 2025 •

edited

Loading

GuillaumeGomez Apr 29, 2025

fmease Apr 30, 2025 •

edited

Loading

GuillaumeGomez Apr 30, 2025

fmease Apr 30, 2025 •

edited

Loading

GuillaumeGomez Apr 30, 2025

GuillaumeGomez Apr 29, 2025

fmease Apr 30, 2025

GuillaumeGomez commented Apr 29, 2025

		// We only push if it's the top item because otherwise, we would duplicate
		// its content since the top-level item was already added.

rustdoc: Fix doctest heuristic for main fn wrapping #140420

Are you sure you want to change the base?

rustdoc: Fix doctest heuristic for main fn wrapping #140420

Conversation

fmease commented Apr 29, 2025 • edited Loading

fmease Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

fmease Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

fmease Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Apr 29, 2025

fmease commented Apr 29, 2025 •

edited

Loading

fmease Apr 29, 2025 •

edited

Loading

fmease Apr 29, 2025 •

edited

Loading

fmease Apr 29, 2025 •

edited

Loading

fmease Apr 29, 2025 •

edited

Loading

fmease Apr 30, 2025 •

edited

Loading

fmease Apr 30, 2025 •

edited

Loading