-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
Add --check for rustdoc #8859
Closed
Closed
Add --check for rustdoc #8859
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will probably need an explicit opt-in.
Also, I'm a little concerned about the performance impact of adding this to the default set. Have you checked to see how slow it is on medium-to-large libraries?
Is it possible that rustdoc could generate duplicate warnings from rustc? I believe
missing_docs
would be duplicated, are there other warnings that could cause that?Related to the two above issues, if this is to be included in the default set, then the logic would go in
filter_default_targets
. Running the build logic twice would be a pretty big performance hit, and would cause cached warnings to be replayed twice, among other issues. Unfortunately I think it could make the "duplicate errors" problem even more apparent, which makes me more concerned about having it in the default set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think about lint duplicates, but it's a good point. We can always allow the ones impacted to prevent this duplication. As for the compilation duplication, normally, rustdoc doesn't recompile anything. Or did I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think this should be an opt-in with
cargo check --doc
orcargo check --all-targets
.The only warnings rustdoc emits are here: https://github.com/rust-lang/rust/blob/a1a13b2bc4fa6370b9501135d97c5fe0bc401894/src/librustdoc/core.rs#L336-L350
I think of those
renamed_and_removed_lints
is the only one run inrustc
other thanmissing_docs
. I find the code around lints really hard to follow though, so I could be wrong.-A missing_docs -A renamed_and_removed_lints
to rustdoc seems like a good idea.Slow. rust-lang/rust#78761
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since rust-lang/rust#80527 I think cargo can solve the duplicate lint problem generally by passing
-A warnings -W rustdoc::all
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's subtly different because it also warns about lints that are allowed by default. So
-A missing_docs -A renamed_and_removed_lints -A unknown_lints
is probably better. https://github.com/rust-lang/rust/blob/8fd946c63a6c3aae9788bd459d278cb2efa77099/src/librustdoc/core.rs#L261-L267