Emit a warning if the doctest main function will not be run
#140527
Merged
Conversation
This file contains hidden or 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
force-pushed
the
doctest-main-fn
branch
from
cd960d9 to
c548a27
Compare
notriddle
reviewed
Apr 30, 2025
src/librustdoc/html/markdown.rs
Outdated
| @@ -303,7 +303,9 @@ impl<'a, I: Iterator<Item = Event<'a>>> Iterator for CodeBlocks<'_, 'a, I> { | |||
| attrs: vec![], | |||
| args_file: PathBuf::new(), | |||
| }; | |||
| let doctest = doctest::DocTestBuilder::new(&test, krate, edition, false, None, None); | |||
| let doctest = doctest::DocTestBuilder::new( | |||
| &test, krate, edition, false, None, None, None, DUMMY_SP, | |||
There was a problem hiding this comment.
This needs a builder pattern, badly.
There was a problem hiding this comment.
Yep. Do you want it in this PR or in a follow-up?
There was a problem hiding this comment.
Either way is fine, as long as it actually gets done.
notriddle
reviewed
May 1, 2025
src/librustdoc/doctest/rust.rs
Outdated
| let line = self.get_base_line() + rel_line.offset(); | ||
| let base_line = self.get_base_line(); | ||
| let line = base_line + rel_line.offset(); | ||
| let count = AtomicUsize::new(base_line); |
There was a problem hiding this comment.
Why is an atomic needed here? This code isn't multithreaded.
There was a problem hiding this comment.
Hum right, a Cell might be enough. Force of habit...
GuillaumeGomez
force-pushed
the
doctest-main-fn
branch
from
c548a27 to
eecc07d
Compare
|
Added the builder type. For setters, I wondered if I should allow generics for |
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
doctest-main-fn
branch
from
eecc07d to
d4bfa2f
Compare
|
Forgot to remove some imports... |
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
doctest-main-fn
branch
from
d4bfa2f to
f4d41a5
Compare
|
And rebased on |
|
@notriddle: Is there anything else to be done here? |
|
Not AFAIK. @bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 2cd3783 (parent) -> 1d67944 (this PR) Test differencesShow 33262 test diffsStage 1
Stage 2
(and 16558 additional test diffs) Additionally, 16604 doctest diffs were found. These are ignored, as they are noisy. Job group index Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 1d679446b01e65f9bc9ae609d0ae1e4a9c0ccaa3 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Finished benchmarking commit (1d67944): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 2.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -0.9%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 775.776s -> 777.175s (0.18%) |
|
This code is wrong: /// ```
/// struct Question;
///
/// fn main() {
/// assert!(false);
/// }
/// ```
pub struct Answer;Running this panics. What should be checked is if there is one non- |
|
Right it panics … but that's correct. I'm not sure what issue you're trying to point out here. Master doesn't emit a warning for this code (good), |
|
Nah all good. Just my brain at late times... |
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.
Fixes #140310.
I think we could try to go much further like adding a "link" (ie UI annotations) on the
mainfunction in the doctest. However that will require some more computation, not sure if it's worth it or not. Can still be done in a follow-up if we want it.For now, this PR does two things:
DiagCtxtto the doctest parser to emit the warning.Spanto where the doctest is starting (I hope the way I did it isn't too bad either...).cc @fmease
r? @notriddle