rustdoc: Fix doctest heuristic for main fn wrapping #140420
Conversation
fmease
added
the
S-waiting-on-author
rustbot
added
S-waiting-on-review
fmease
removed
the
S-waiting-on-review
fmease
force-pushed
the
rustdoc-fix-doctest-heur
branch
from
657e3dd to
944ee37
Compare
| // 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). |
There was a problem hiding this comment.
Outdated since #138104 which has removed the recursion as suggested in #138104 (comment).
| // We only push if it's the top item because otherwise, we would duplicate | ||
| // its content since the top-level item was already added. |
There was a problem hiding this comment.
Outdated since #138104 which has removed the recursion as suggested in #138104 (comment).
| has_non_items = true; | ||
| } | ||
| StmtKind::Let(_) | StmtKind::Semi(_) | StmtKind::Empty => has_non_items = true, |
There was a problem hiding this comment.
Replaced wildcard with exhaustive matching to prevent silently dropping non-items ever again (context: #140220 (comment)).
| } | ||
| } | ||
| } | ||
| _ => { | ||
| StmtKind::Expr(ref expr) => { |
There was a problem hiding this comment.
Moved this one closer to the other non-items.
| iter.next(); | ||
| iter.peek() | ||
| if !info.has_main_fn { | ||
| // For backward compatibility, we look for the token sequence `fn main(…)` |
There was a problem hiding this comment.
Re-added the historical context that was dropped in #138104 (I rephrased it).
| StmtKind::MacCall(ref mac_call) => { | ||
| if info.has_main_fn { | ||
| continue; |
There was a problem hiding this comment.
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
force-pushed
the
rustdoc-fix-doctest-heur
branch
from
944ee37 to
714f6f2
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
| { | ||
| info.has_main_fn = true; | ||
| break; |
There was a problem hiding this comment.
Why removing this break?
|
Nah, not your fault. All tests passed and the "try" command was happy was well so I (wrongly) assumed that it was all good. |
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:
cargotestsmoke 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.rswas incorrect and has been removedContains some other small changes. Diff best reviewed modulo whitespace.
r? @GuillaumeGomez