Skip to content

emit lints warnings on doc private if --document-private-items is used #76767

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

emit lints warnings on doc private if --document-private-items is used #76767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e023587
Emit missing doc lints when documenting private items
GuillaumeGomez Sep 15, 2020
22ed092
Add tests
GuillaumeGomez Sep 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions src/librustdoc/passes/calculate_doc_coverage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub const CALCULATE_DOC_COVERAGE: Pass = Pass {
};

fn calculate_doc_coverage(krate: clean::Crate, ctx: &DocContext<'_>) -> clean::Crate {
let mut calc = CoverageCalculator::new();
let mut calc = CoverageCalculator::new(ctx);
let krate = calc.fold_crate(krate);

calc.print_results(ctx.renderinfo.borrow().output_format);
Expand Down Expand Up @@ -94,8 +94,9 @@ impl ops::AddAssign for ItemCount {
}
}

struct CoverageCalculator {
struct CoverageCalculator<'a, 'tcx> {
items: BTreeMap<FileName, ItemCount>,
cx: &'a DocContext<'tcx>,
}

fn limit_filename_len(filename: String) -> String {
Expand All @@ -108,9 +109,9 @@ fn limit_filename_len(filename: String) -> String {
}
}

impl CoverageCalculator {
fn new() -> CoverageCalculator {
CoverageCalculator { items: Default::default() }
impl<'a, 'tcx> CoverageCalculator<'a, 'tcx> {
fn new(cx: &'a DocContext<'tcx>) -> CoverageCalculator<'a, 'tcx> {
CoverageCalculator { cx, items: Default::default() }
}

fn to_json(&self) -> String {
Expand Down Expand Up @@ -178,7 +179,7 @@ impl CoverageCalculator {
}
}

impl fold::DocFolder for CoverageCalculator {
impl<'a, 'tcx> fold::DocFolder for CoverageCalculator<'a, 'tcx> {
fn fold_item(&mut self, i: clean::Item) -> Option<clean::Item> {
match i.inner {
_ if !i.def_id.is_local() => {
Expand Down Expand Up @@ -244,7 +245,7 @@ impl fold::DocFolder for CoverageCalculator {
self.items.entry(i.source.filename.clone()).or_default().count_item(
has_docs,
has_doc_example,
should_have_doc_example(&i.inner),
should_have_doc_example(self.cx, &i),
);
}
}
Expand Down
25 changes: 20 additions & 5 deletions src/librustdoc/passes/doc_test_lints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ impl crate::doctest::Tester for Tests {
}
}

pub fn should_have_doc_example(item_kind: &clean::ItemEnum) -> bool {
!matches!(item_kind,
pub fn should_have_doc_example(cx: &DocContext<'_>, item: &clean::Item) -> bool {
!matches!(item.inner,
clean::StructFieldItem(_)
| clean::VariantItem(_)
| clean::AssocConstItem(_, _)
Expand All @@ -73,7 +73,8 @@ pub fn should_have_doc_example(item_kind: &clean::ItemEnum) -> bool {
| clean::ImportItem(_)
| clean::PrimitiveItem(_)
| clean::KeywordItem(_)
)
) && (cx.renderinfo.borrow().access_levels.is_public(item.def_id)
|| cx.render_options.document_private)
Comment on lines +76 to +77
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

pub fn look_for_tests<'tcx>(cx: &DocContext<'tcx>, dox: &str, item: &Item) {
Expand All @@ -89,10 +90,22 @@ pub fn look_for_tests<'tcx>(cx: &DocContext<'tcx>, dox: &str, item: &Item) {

find_testable_code(&dox, &mut tests, ErrorCodes::No, false, None);

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()
});
}
}
Comment on lines +93 to +103
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this change.

  • missing_docs is a rustc pass, not a rustdoc pass. 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Sep 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I do not like that the pass is duplicated with different diagnostics.

Clippy is an outside tool as much as rustdoc is :)

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Sep 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if tests.found_tests == 0
&& rustc_feature::UnstableFeatures::from_environment().is_nightly_build()
{
if should_have_doc_example(&item.inner) {
if should_have_doc_example(cx, &item) {
debug!("reporting error for {:?} (hir_id={:?})", item, hir_id);
let sp = span_of_attrs(&item.attrs).unwrap_or(item.source.span());
cx.tcx.struct_span_lint_hir(
Expand All @@ -102,7 +115,9 @@ pub fn look_for_tests<'tcx>(cx: &DocContext<'tcx>, dox: &str, item: &Item) {
|lint| lint.build("missing code example in this documentation").emit(),
);
}
} else if tests.found_tests > 0 && !cx.renderinfo.borrow().access_levels.is_public(item.def_id)
} else if tests.found_tests > 0
&& !cx.renderinfo.borrow().access_levels.is_public(item.def_id)
&& !cx.render_options.document_private
{
cx.tcx.struct_span_lint_hir(
lint::builtin::PRIVATE_DOC_TESTS,
Expand Down
7 changes: 4 additions & 3 deletions src/librustdoc/passes/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ struct Stripper<'a> {
retained: &'a mut DefIdSet,
access_levels: &'a AccessLevels<DefId>,
update_retained: bool,
document_private: bool,
}

impl<'a> DocFolder for Stripper<'a> {
Expand Down Expand Up @@ -181,21 +182,21 @@ impl<'a> DocFolder for Stripper<'a> {
| clean::TraitAliasItem(..)
| clean::ForeignTypeItem => {
if i.def_id.is_local() {
if !self.access_levels.is_exported(i.def_id) {
if !self.access_levels.is_exported(i.def_id) && !self.document_private {
debug!("Stripper: stripping {:?} {:?}", i.type_(), i.name);
return None;
}
}
}

clean::StructFieldItem(..) => {
if i.visibility != clean::Public {
if i.visibility != clean::Public && !self.document_private {
return StripItem(i).strip();
}
}

clean::ModuleItem(..) => {
if i.def_id.is_local() && i.visibility != clean::Public {
if i.def_id.is_local() && i.visibility != clean::Public && !self.document_private {
debug!("Stripper: stripping module {:?}", i.name);
let old = mem::replace(&mut self.update_retained, false);
let ret = StripItem(self.fold_item_recur(i).unwrap()).strip();
Expand Down
2 changes: 2 additions & 0 deletions src/librustdoc/passes/strip_private.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@ pub fn strip_private(mut krate: clean::Crate, cx: &DocContext<'_>) -> clean::Cra
// This stripper collects all *retained* nodes.
let mut retained = DefIdSet::default();
let access_levels = cx.renderinfo.borrow().access_levels.clone();
let document_private = cx.render_options.document_private;

// strip all private items
{
let mut stripper = Stripper {
retained: &mut retained,
access_levels: &access_levels,
update_retained: true,
document_private,
};
krate = ImportStripper.fold_crate(stripper.fold_crate(krate));
}
Expand Down
14 changes: 14 additions & 0 deletions src/test/rustdoc-ui/lint-missing-doc-code-example-private.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// compile-flags: --document-private-items

#![deny(missing_docs)]
#![deny(missing_doc_code_examples)]

//! hello
//!
//! ```
//! let x = 0;
//! ```

fn private_fn() {}
//~^ ERROR
//~^^ ERROR
26 changes: 26 additions & 0 deletions src/test/rustdoc-ui/lint-missing-doc-code-example-private.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
error: missing documentation
--> $DIR/lint-missing-doc-code-example-private.rs:12:1
|
LL | fn private_fn() {}
| ^^^^^^^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/lint-missing-doc-code-example-private.rs:3:9
|
LL | #![deny(missing_docs)]
| ^^^^^^^^^^^^

error: missing code example in this documentation
--> $DIR/lint-missing-doc-code-example-private.rs:12:1
|
LL | fn private_fn() {}
| ^^^^^^^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/lint-missing-doc-code-example-private.rs:4:9
|
LL | #![deny(missing_doc_code_examples)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

8 changes: 6 additions & 2 deletions src/test/rustdoc-ui/lint-missing-doc-code-example.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ fn test() {
}

#[allow(missing_docs)]
mod module1 { //~ ERROR
pub mod module1 { //~ ERROR
}

#[allow(missing_doc_code_examples)]
Expand Down Expand Up @@ -63,9 +63,13 @@ pub enum Enum {
/// Doc
//~^ ERROR
#[repr(C)]
union Union {
pub union Union {
/// Doc, but no code example and it's fine!
a: i32,
/// Doc, but no code example and it's fine!
b: f32,
}

mod private {
fn private_fn() {}
}
2 changes: 1 addition & 1 deletion src/test/rustdoc-ui/lint-missing-doc-code-example.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ LL | /// Doc
error: missing code example in this documentation
--> $DIR/lint-missing-doc-code-example.rs:19:1
|
LL | / mod module1 {
LL | / pub mod module1 {
LL | | }
| |_^

Expand Down