emit lints warnings on doc private if --document-private-items is used #76767
Conversation
rust-highfive
added
the
S-waiting-on-review
jyn514
added
A-lints
| ) && (cx.renderinfo.borrow().access_levels.is_public(item.def_id) | ||
| || cx.render_options.document_private) |
There was a problem hiding this comment.
Hmm, I see this used a lot in rustdoc code. Maybe it's worth adding a cx.appears_in_docs(def_id) function? Doesn't need to block this PR though.
| if cx.render_options.document_private | ||
| && dox.is_empty() | ||
| && !matches!(item.inner, clean::ImportItem(..)) | ||
| { | ||
| let sp = span_of_attrs(&item.attrs).unwrap_or(item.source.span()); | ||
| if !sp.is_dummy() { | ||
| cx.tcx.struct_span_lint_hir(rustc_lint::builtin::MISSING_DOCS, hir_id, sp, |lint| { | ||
| lint.build("missing documentation").emit() | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure about this change.
missing_docsis a rustc pass, not arustdocpass. I don't like that both rustc and rustdoc will run the same pass but with different diagnostics.- clippy already lints for missing docs in private items (
missing_docs_in_private_items). Do we really need to duplicate that here?
There was a problem hiding this comment.
It's an "outside" tool. Also, --document-private-items is a rustdoc option, meaning you can't check it in the rustc pass. This is more an extension than a pass in itself considering that it's on top.
There was a problem hiding this comment.
Right, but my concern is that this doesn't belong in rustdoc. At least I'd want an FCP to be sure the rest of the team is agrees, if you feel strongly about it go ahead and start one.
There was a problem hiding this comment.
An FCP seems a bit big for this. Let's ping others, it might just be simpler. If we can't reach a consensus, it'll always be time to make an FCP. :)
What do you think about this @Manishearth, @ollie27 ?
There was a problem hiding this comment.
Yeah I do not like that the pass is duplicated with different diagnostics.
Clippy is an outside tool as much as rustdoc is :)
There was a problem hiding this comment.
@Manishearth Wo, I didn't that about clippy, that's great! Also, I think you're misunderstanding something: it is not in place, it is an extension. The original lint check is still run (before this one in fact). So it's definitely not a duplication.
@ollie27: It doesn't change behavior, it's just that if you're documenting private items, it doesn't make sense to complain about the fact that they're private. And there is no way to know from rustc that --document-private-items has been passed to rustdoc (well, unless we add it to rustc directly obviously, but I'd rather not).
There was a problem hiding this comment.
The original lint check is still run (before this one in fact). So it's definitely not a duplication
Right, but the diagnostics are different. The code for this should be in one place.
Such a lint already exists in clippy so the only possible next step would be to move that lint to rustc.
Totally happy to do this too. I'm not a huge fan of the lint changing behavior either.
There was a problem hiding this comment.
I have no issue putting this code in rustc directly, however it means that rustc will need to be aware of the --document-private-items rustdoc setting.
There was a problem hiding this comment.
No, i'm saying that we can make rustc aware of that setting, by making it an internal flag that can be swapped by consumers of the rustc API (like rustdoc)
There was a problem hiding this comment.
I'm not sure we should make rustc aware of it. I'd expect this to be a separate pass: --document-private should only affect whether the docs are rendered, not whether they're processed at all. Strip makes me uncomfortable for that reason, I'd prefer rustdoc linted consistently.
crlf0710
added
the
S-waiting-on-team
|
☔ The latest upstream changes (presumably #76196) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
camelid
added
S-waiting-on-review
|
I'm going to close this; I think the best way forward if you want to lint private items is to uplift the clippy lint, so that the warnings are strongly delimited and rustdoc doesn't behave differently from rustc. |
r? @jyn514