Skip to content

Correctly handle should_panic doctest attribute #143453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Correctly handle should_panic doctest attribute #143453

wants to merge 3 commits into from

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jul 4, 2025 •
edited by rustbot
Loading

Fixes #143009.

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 think last one who reviewed doctest changes was @fmease so setting you as reviewer. :)

r? fmease

@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2025

fmease is not on the review rotation at the moment.
They may take a while to respond.

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Jul 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2025

This PR modifies run-make tests.

cc @jieyouxu

@GuillaumeGomez
Copy link
Member Author

Ah, someone else then (sorry for the ping).

r? notriddle

@rustbot rustbot assigned notriddle and unassigned fmease Jul 4, 2025
This was referenced Jul 4, 2025
Closed
Open
@fmease fmease assigned fmease and unassigned notriddle Jul 4, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@purplesyringa
Copy link
Contributor

I don't like this: this causes std::process::exit(0); to be considered as a successfully passed test, and fn main() { panic!(); } is treated as an error rather than a panic.

@GuillaumeGomez
Copy link
Member Author

I can't get this information from just within the same process, I think I'll create a file in case the panic happened and then check from the parent if the file exists (or any IPC).

@purplesyringa
Copy link
Contributor

purplesyringa commented Jul 5, 2025 •
edited
Loading

Can we maybe just compare the exit code with 101? It's exceedingly that unlikely that someone uses this exit code within a should_panic doctest. That feels more robust than temporary files despite the possible false positive.

@GuillaumeGomez
Copy link
Member Author

Does it work on windows?

@purplesyringa
Copy link
Contributor

It should, yeah. We use the same exit code regardless of the platform..

@GuillaumeGomez
Copy link
Member Author

I wasn't super happy about this approach but I guess it's rare enough so that we can use it.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez GuillaumeGomez force-pushed the should-panic branch 2 times, most recently from f134d32 to 5bc0760 Compare July 7, 2025 11:59
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Implemented it with the exit code check.

@GuillaumeGomez GuillaumeGomez mentioned this pull request Jul 13, 2025
Open
@GuillaumeGomez
Copy link
Member Author

Closing in favour of #143900.

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 13, 2025
@GuillaumeGomez GuillaumeGomez deleted the should-panic branch July 13, 2025 18:52
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 1, 2025
@GuillaumeGomez
Rollup merge of rust-lang#143900 - GuillaumeGomez:fix-no-run, r=lolbi…
08058ee 
…narycat,fmease

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

Fixes rust-lang#143009.
Fixes rust-lang#143858.

Since it includes fixes from rust-lang#143453, it's taking it over (commits 2, 3 and 4 are from rust-lang#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`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 1, 2025
@GuillaumeGomez
Rollup merge of rust-lang#143900 - GuillaumeGomez:fix-no-run, r=lolbi…
8e98121 
…narycat,fmease

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

Fixes rust-lang#143009.
Fixes rust-lang#143858.

Since it includes fixes from rust-lang#143453, it's taking it over (commits 2, 3 and 4 are from rust-lang#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``
samueltardieu added a commit to samueltardieu/rust that referenced this pull request Aug 2, 2025
@samueltardieu
Rollup merge of rust-lang#143900 - GuillaumeGomez:fix-no-run, r=lolbi…
4ceba56 
…narycat,fmease

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

Fixes rust-lang#143009.
Fixes rust-lang#143858.

Since it includes fixes from rust-lang#143453, it's taking it over (commits 2, 3 and 4 are from rust-lang#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`
bors added a commit that referenced this pull request Aug 3, 2025
@bors
Auto merge of #143900 - GuillaumeGomez:fix-no-run, r=lolbinarycat,fmease
058edea 
[rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition

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`
Zalathar added a commit to Zalathar/rust that referenced this pull request Aug 3, 2025
@Zalathar
Rollup merge of rust-lang#143900 - GuillaumeGomez:fix-no-run, r=lolbi…
24ae2f0 
…narycat,fmease

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

Fixes rust-lang#143009.
Fixes rust-lang#143858.

Since it includes fixes from rust-lang#143453, it's taking it over (commits 2, 3 and 4 are from rust-lang#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`
bors added a commit that referenced this pull request Aug 3, 2025
@bors
Auto merge of #143900 - GuillaumeGomez:fix-no-run, r=lolbinarycat,fmease
b6a6d52 
[rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition

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`
Zalathar added a commit to Zalathar/rust that referenced this pull request Aug 3, 2025
@Zalathar
Rollup merge of rust-lang#143900 - GuillaumeGomez:fix-no-run, r=lolbi…
4ad057f 
…narycat,fmease

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

Fixes rust-lang#143009.
Fixes rust-lang#143858.

Since it includes fixes from rust-lang#143453, it's taking it over (commits 2, 3 and 4 are from rust-lang#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`
bors added a commit that referenced this pull request Aug 3, 2025
@bors
Auto merge of #143900 - GuillaumeGomez:fix-no-run, r=lolbinarycat,fmease
0721cd7 
[rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition

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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@fmease fmease

Labels
A-run-make Area: port run-make Makefiles to rmake.rs T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

should_panic in doctests accepts crashes, aborts, std::process::exit
6 participants
@GuillaumeGomez @rustbot @rust-log-analyzer @purplesyringa @notriddle @fmease