rustdoc: Refactor doctest collection and running code #125798
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
GuillaumeGomez
approved these changes
May 31, 2024
|
Changes are pretty straight-forward, looks good to me. Please ping me once ready for a final review. |
This comment has been minimized.
This comment has been minimized.
camelid
commented
May 31, 2024
Comment on lines
+417
to
+422
| // FIXME: why does this code check if it *shouldn't* persist doctests | ||
| // -- shouldn't it be the negation? |
There was a problem hiding this comment.
This code was already there, but it's really weird. Changing it to is_some breaks a doctest in core (for MaybeUninit::uninit_array.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
camelid
changed the title
|
There's definitely more work to be done, but I think this is a good stopping point for this PR. @GuillaumeGomez ready for review! |
camelid
added
C-cleanup
|
Also I should mention that a lot of the changes listed are just moving code into files. I made sure to put those in their own commits to aid in code review. In general this PR is best reviewed commit-by-commit. |
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #126016) made this pull request unmergeable. Please resolve the merge conflicts. |
GuillaumeGomez
approved these changes
Jun 7, 2024
The new name more accurately captures what it is.
I moved some local arguments and options to either the local options struct or, if it made sense, the global options struct.
This code turns the raw code given by the user into something actually runnable, e.g. by adding a `main` function if it doesn't already exist. I also made a couple other items private that didn't need to be crate-public.
It doesn't really make sense to skip part of the source when we're parsing it, so parse the whole doctest. This simplifies things too.
GuillaumeGomez
force-pushed
the
refactor-doctest
branch
from
01b3f38
to
3670ad5
Compare
This comment has been minimized.
This comment has been minimized.
|
Thanks for the cleanup, it was very much needed! I'll give a higher priority to this PR as it is very merge conflict-prone. @bors r+ p=1 |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (4dc24ae): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: missing data |
3 tasks
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 7, 2024
…2, r=<try> Greatly speed up doctests by compiling compatible doctests in one file Fixes rust-lang#75341. Take 2 at rust-lang#123974. It should now be much simpler to review since it's based on rust-lang#125798. I split the changes as much as possible to make it as easy as possible to review (even though it's a huge lot of code changes...). The following tests are not included into the combined doctests: * `compile_fail` * If there are crate attributes (`deny`/`allow`/`warn` are ok) * have invalid AST * `test_harness` * no capture * `--show-output` (because the output is nicer without the extra code surrounding it) Everything else is merged into one file. If this one file fails to compile, we go back to the current strategy: compile each doctest separately. Because of the `edition` codeblock attribute, I couldn't make them all into one file directly so I grouped them by edition, but it should be pretty anecdotic. In case the users want a specific doctest to be opted-out from this doctest merge, I added the `standalone` codeblock attribute: ```rust /// ```rust,standalone /// // This doctest will be run in its own process! /// ``` ``` Now the interesting part, I ran it on a few crates and here are the results (with `cargo test --doc` to only include doctests): | crate | nb doctests | before this PR | with this PR | | - | - | - | - | | sysinfo | 227 | 4.6s | 1.11s | | geos | 157 | 3.95s | 0.45s | | core | 4604 | 54.08s | 13.5s (merged: 0.9s, standalone: 12.6s) | | std | 1147 | 12s | 3.56s (merged: 2.1s, standalone: 1.46s) | r? `@camelid` try-job: x86_64-msvc try-job: aarch64-apple
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Aug 12, 2024
…-v2, r=t-rustdoc Greatly speed up doctests by compiling compatible doctests in one file Fixes rust-lang#75341. Take 2 at rust-lang#123974. It should now be much simpler to review since it's based on rust-lang#125798. I split the changes as much as possible to make it as easy as possible to review (even though it's a huge lot of code changes...). The following tests are not included into the combined doctests: * `compile_fail` * If there are crate attributes (`deny`/`allow`/`warn` are ok) * have invalid AST * `test_harness` * no capture * `--show-output` (because the output is nicer without the extra code surrounding it) Everything else is merged into one file. If this one file fails to compile, we go back to the current strategy: compile each doctest separately. Because of the `edition` codeblock attribute, I couldn't make them all into one file directly so I grouped them by edition, but it should be pretty anecdotic. In case the users want a specific doctest to be opted-out from this doctest merge, I added the `standalone` codeblock attribute: ```rust /// ```rust,standalone /// // This doctest will be run in its own process! /// ``` ``` Now the interesting part, I ran it on a few crates and here are the results (with `cargo test --doc` to only include doctests): | crate | nb doctests | before this PR | with this PR | | - | - | - | - | | sysinfo | 227 | 4.6s | 1.11s | | geos | 157 | 3.95s | 0.45s | | core | 4604 | 54.08s | 13.5s (merged: 0.9s, standalone: 12.6s) | | std | 1147 | 12s | 3.56s (merged: 2.1s, standalone: 1.46s) | r? `@camelid` try-job: x86_64-msvc try-job: aarch64-apple
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 12, 2024
…2, r=t-rustdoc Greatly speed up doctests by compiling compatible doctests in one file Fixes rust-lang#75341. Take 2 at rust-lang#123974. It should now be much simpler to review since it's based on rust-lang#125798. I split the changes as much as possible to make it as easy as possible to review (even though it's a huge lot of code changes...). The following tests are not included into the combined doctests: * `compile_fail` * If there are crate attributes (`deny`/`allow`/`warn` are ok) * have invalid AST * `test_harness` * no capture * `--show-output` (because the output is nicer without the extra code surrounding it) Everything else is merged into one file. If this one file fails to compile, we go back to the current strategy: compile each doctest separately. Because of the `edition` codeblock attribute, I couldn't make them all into one file directly so I grouped them by edition, but it should be pretty anecdotic. In case the users want a specific doctest to be opted-out from this doctest merge, I added the `standalone` codeblock attribute: ```rust /// ```rust,standalone /// // This doctest will be run in its own process! /// ``` ``` Now the interesting part, I ran it on a few crates and here are the results (with `cargo test --doc` to only include doctests): | crate | nb doctests | before this PR | with this PR | | - | - | - | - | | sysinfo | 227 | 4.6s | 1.11s | | geos | 157 | 3.95s | 0.45s | | core | 4604 | 54.08s | 13.5s (merged: 0.9s, standalone: 12.6s) | | std | 1147 | 12s | 3.56s (merged: 2.1s, standalone: 1.46s) | r? `@camelid` try-job: x86_64-msvc try-job: aarch64-apple
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 12, 2024
…2, r=t-rustdoc Greatly speed up doctests by compiling compatible doctests in one file Fixes rust-lang#75341. Take 2 at rust-lang#123974. It should now be much simpler to review since it's based on rust-lang#125798. I split the changes as much as possible to make it as easy as possible to review (even though it's a huge lot of code changes...). The following tests are not included into the combined doctests: * `compile_fail` * If there are crate attributes (`deny`/`allow`/`warn` are ok) * have invalid AST * `test_harness` * no capture * `--show-output` (because the output is nicer without the extra code surrounding it) Everything else is merged into one file. If this one file fails to compile, we go back to the current strategy: compile each doctest separately. Because of the `edition` codeblock attribute, I couldn't make them all into one file directly so I grouped them by edition, but it should be pretty anecdotic. In case the users want a specific doctest to be opted-out from this doctest merge, I added the `standalone` codeblock attribute: ```rust /// ```rust,standalone /// // This doctest will be run in its own process! /// ``` ``` Now the interesting part, I ran it on a few crates and here are the results (with `cargo test --doc` to only include doctests): | crate | nb doctests | before this PR | with this PR | | - | - | - | - | | sysinfo | 227 | 4.6s | 1.11s | | geos | 157 | 3.95s | 0.45s | | core | 4604 | 54.08s | 13.5s (merged: 0.9s, standalone: 12.6s) | | std | 1147 | 12s | 3.56s (merged: 2.1s, standalone: 1.46s) | r? `@camelid` try-job: x86_64-msvc try-job: aarch64-apple
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 13, 2024
…2, r=t-rustdoc Greatly speed up doctests by compiling compatible doctests in one file Fixes rust-lang#75341. Take 2 at rust-lang#123974. It should now be much simpler to review since it's based on rust-lang#125798. I split the changes as much as possible to make it as easy as possible to review (even though it's a huge lot of code changes...). The following tests are not included into the combined doctests: * `compile_fail` * If there are crate attributes (`deny`/`allow`/`warn` are ok) * have invalid AST * `test_harness` * no capture * `--show-output` (because the output is nicer without the extra code surrounding it) Everything else is merged into one file. If this one file fails to compile, we go back to the current strategy: compile each doctest separately. Because of the `edition` codeblock attribute, I couldn't make them all into one file directly so I grouped them by edition, but it should be pretty anecdotic. In case the users want a specific doctest to be opted-out from this doctest merge, I added the `standalone` codeblock attribute: ```rust /// ```rust,standalone /// // This doctest will be run in its own process! /// ``` ``` Now the interesting part, I ran it on a few crates and here are the results (with `cargo test --doc` to only include doctests): | crate | nb doctests | before this PR | with this PR | | - | - | - | - | | sysinfo | 227 | 4.6s | 1.11s | | geos | 157 | 3.95s | 0.45s | | core | 4604 | 54.08s | 13.5s (merged: 0.9s, standalone: 12.6s) | | std | 1147 | 12s | 3.56s (merged: 2.1s, standalone: 1.46s) | r? `@camelid` try-job: x86_64-msvc try-job: aarch64-apple
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 13, 2024
…2, r=t-rustdoc Greatly speed up doctests by compiling compatible doctests in one file Fixes rust-lang#75341. Take 2 at rust-lang#123974. It should now be much simpler to review since it's based on rust-lang#125798. I split the changes as much as possible to make it as easy as possible to review (even though it's a huge lot of code changes...). The following tests are not included into the combined doctests: * `compile_fail` * If there are crate attributes (`deny`/`allow`/`warn` are ok) * have invalid AST * `test_harness` * no capture * `--show-output` (because the output is nicer without the extra code surrounding it) Everything else is merged into one file. If this one file fails to compile, we go back to the current strategy: compile each doctest separately. Because of the `edition` codeblock attribute, I couldn't make them all into one file directly so I grouped them by edition, but it should be pretty anecdotic. In case the users want a specific doctest to be opted-out from this doctest merge, I added the `standalone` codeblock attribute: ```rust /// ```rust,standalone /// // This doctest will be run in its own process! /// ``` ``` Now the interesting part, I ran it on a few crates and here are the results (with `cargo test --doc` to only include doctests): | crate | nb doctests | before this PR | with this PR | | - | - | - | - | | sysinfo | 227 | 4.6s | 1.11s | | geos | 157 | 3.95s | 0.45s | | core | 4604 | 54.08s | 13.5s (merged: 0.9s, standalone: 12.6s) | | std | 1147 | 12s | 3.56s (merged: 2.1s, standalone: 1.46s) | r? `@camelid` try-job: x86_64-msvc try-job: aarch64-apple
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 13, 2024
…2, r=t-rustdoc Greatly speed up doctests by compiling compatible doctests in one file Fixes rust-lang#75341. Take 2 at rust-lang#123974. It should now be much simpler to review since it's based on rust-lang#125798. I split the changes as much as possible to make it as easy as possible to review (even though it's a huge lot of code changes...). The following tests are not included into the combined doctests: * `compile_fail` * If there are crate attributes (`deny`/`allow`/`warn` are ok) * have invalid AST * `test_harness` * no capture * `--show-output` (because the output is nicer without the extra code surrounding it) Everything else is merged into one file. If this one file fails to compile, we go back to the current strategy: compile each doctest separately. Because of the `edition` codeblock attribute, I couldn't make them all into one file directly so I grouped them by edition, but it should be pretty anecdotic. In case the users want a specific doctest to be opted-out from this doctest merge, I added the `standalone` codeblock attribute: ```rust /// ```rust,standalone /// // This doctest will be run in its own process! /// ``` ``` Now the interesting part, I ran it on a few crates and here are the results (with `cargo test --doc` to only include doctests): | crate | nb doctests | before this PR | with this PR | | - | - | - | - | | sysinfo | 227 | 4.6s | 1.11s | | geos | 157 | 3.95s | 0.45s | | core | 4604 | 54.08s | 13.5s (merged: 0.9s, standalone: 12.6s) | | std | 1147 | 12s | 3.56s (merged: 2.1s, standalone: 1.46s) | r? `@camelid` try-job: x86_64-msvc try-job: aarch64-apple
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 13, 2024
…2, r=t-rustdoc Greatly speed up doctests by compiling compatible doctests in one file Fixes rust-lang#75341. Take 2 at rust-lang#123974. It should now be much simpler to review since it's based on rust-lang#125798. I split the changes as much as possible to make it as easy as possible to review (even though it's a huge lot of code changes...). The following tests are not included into the combined doctests: * `compile_fail` * If there are crate attributes (`deny`/`allow`/`warn` are ok) * have invalid AST * `test_harness` * no capture * `--show-output` (because the output is nicer without the extra code surrounding it) Everything else is merged into one file. If this one file fails to compile, we go back to the current strategy: compile each doctest separately. Because of the `edition` codeblock attribute, I couldn't make them all into one file directly so I grouped them by edition, but it should be pretty anecdotic. In case the users want a specific doctest to be opted-out from this doctest merge, I added the `standalone` codeblock attribute: ```rust /// ```rust,standalone /// // This doctest will be run in its own process! /// ``` ``` Now the interesting part, I ran it on a few crates and here are the results (with `cargo test --doc` to only include doctests): | crate | nb doctests | before this PR | with this PR | | - | - | - | - | | sysinfo | 227 | 4.6s | 1.11s | | geos | 157 | 3.95s | 0.45s | | core | 4604 | 54.08s | 13.5s (merged: 0.9s, standalone: 12.6s) | | std | 1147 | 12s | 3.56s (merged: 2.1s, standalone: 1.46s) | r? `@camelid` try-job: x86_64-msvc try-job: aarch64-apple
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This code previously had a quite confusing structure, mixing the collection,
processing, and running of doctests with multiple layers of indirection. There
are also many cases where tons of parameters are passed to functions with little
typing information (e.g., booleans or strings are often used).
As a result, the source of bugs is obfuscated (e.g. #81070) and large changes
(e.g. #123974) become unnecessarily complicated. This PR is a first step to try
to simplify the code and make it easier to follow and less bug-prone.
r? @GuillaumeGomez