Add new failed_merged_doctest_compilation rustdoc lint
#148866
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-author
Collaborator
|
|
Member
Author
|
@bors2 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
rust-bors bot
added a commit
that referenced
this pull request
Nov 12, 2025
…compilation, r=<try> Add new `failed_merged_doctest_compilation` rustdoc lint
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
Member
Author
|
Another thing I thought: should we display errors? |
Contributor
This comment has been minimized.
This comment has been minimized.
Collaborator
|
Finished benchmarking commit (fa55e90): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 1.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 474.976s -> 474.831s (-0.03%) |
rustbot
removed
the
S-waiting-on-perf
24 tasks
Collaborator
|
☔ The latest upstream changes (presumably #149891) made this pull request unmergeable. Please resolve the merge conflicts. |
fmease
reviewed
Jan 13, 2026
Member
There was a problem hiding this comment.
What's the status of this PR? I've only given it a cursory glance. Perf came back clean, so does that mean the current approach can be taken or are there still other blockers?
Would you like me to review this code? I didn't review it so far because of the draft status & the failed CI (the failure seems to stem from lib alloc containing two unmergeable doctests).
| mergeable_tests: FxIndexMap<MergeableTestKey, Vec<(DocTestBuilder, ScrapedDocTest)>>, | ||
| // We pass this argument so we can drop it manually before using `exit`. | ||
| mut temp_dir: Option<TempDir>, | ||
| tcx: Option<TyCtxt<'_>>, |
Member
There was a problem hiding this comment.
(preliminary review) this is always Some(_); I'm probably missing sth.
Member
Author
There was a problem hiding this comment.
When you run rustdoc on a markdown file, there is no TyCtxt ever. ;)
| continue; | ||
| } | ||
| if let Some(tcx) = tcx { | ||
| tcx.node_span_lint( |
Member
There was a problem hiding this comment.
[…] I'd really love to see if it's possible to emit lints from
DiagCtxtHandlesomehow,
That's not possible, it doesn't contain the necessary information. DiagCtxtHandle just stores low-level things in order to properly emit diagnostics. For lints on the other hand we need to be able to check if they were suppressed via lint control attributes or flags (allow, expect) which form a hierarchy (apart from that, it needs access to the query system (so TyCtxt) & some other data that's stored in the Session).
Member
There was a problem hiding this comment.
I haven't taken a deeper look at the code yet, so I'm not fully up to speed re. all the constraints that are imposed here but I can say that if lack of access to TyCtxt is a problem (as you've alluded to in the PR description), consider "buffering" the lint instead of calling node_span_lint eagerly. That's what rustc also does / has to do for early lints, it just puts them into a vector and emits them at a later point where TyCtxt is available.
Hopefully this is relevant ^^' I've yet to take a proper look. Of course if you want to avoid keeping TyCtxt or Session or XYZ alive for a longer time, buffering won't help.
Member
Author
There was a problem hiding this comment.
Problem here is that the TyCtxt dies before we run the doctests normally, so I moved the doctest run earlier. Storing the warnings won't change anything sadly.
GuillaumeGomez
force-pushed
the
rustdoc-failed_merged_doctest_compilation
branch
from
3d4bd83 to
6eb8bd7
Compare
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
rustdoc-failed_merged_doctest_compilation
branch
from
6eb8bd7 to
5794f86
Compare
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Finally took the time to write this lint. We need to have a compiler context in order to be able to emit the lint, but I'm not sure if
DiagCtxtHandledoesn't provide it somehow... Anyway, I'm expecting performance regression because we keep the compiler and its internals around for much longer, stacking doctests memory on top of the allocated memory to the current crate (which is why I'd really love to see if it's possible to emit lints fromDiagCtxtHandlesomehow, gonna investigate).Also: both lint name and lint message are very much up to debate.
r? @fmease