From dd816a9c2e88b3e55c24991e356a9d13d683c545 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 9 Aug 2022 15:39:58 +0200 Subject: [PATCH 1/2] Prevent impl blocks containing only private items to be documented by default --- src/librustdoc/passes/strip_hidden.rs | 8 +++- src/librustdoc/passes/strip_private.rs | 10 ++++- src/librustdoc/passes/stripper.rs | 53 +++++++++++++++++++------- 3 files changed, 54 insertions(+), 17 deletions(-) diff --git a/src/librustdoc/passes/strip_hidden.rs b/src/librustdoc/passes/strip_hidden.rs index 533e2ce46ddd8..9914edf3036e4 100644 --- a/src/librustdoc/passes/strip_hidden.rs +++ b/src/librustdoc/passes/strip_hidden.rs @@ -17,6 +17,7 @@ pub(crate) const STRIP_HIDDEN: Pass = Pass { /// Strip items marked `#[doc(hidden)]` pub(crate) fn strip_hidden(krate: clean::Crate, cx: &mut DocContext<'_>) -> clean::Crate { let mut retained = ItemIdSet::default(); + let is_json_output = cx.output_format.is_json() && !cx.show_coverage; // strip all #[doc(hidden)] items let krate = { @@ -25,7 +26,12 @@ pub(crate) fn strip_hidden(krate: clean::Crate, cx: &mut DocContext<'_>) -> clea }; // strip all impls referencing stripped items - let mut stripper = ImplStripper { retained: &retained, cache: &cx.cache }; + let mut stripper = ImplStripper { + retained: &retained, + cache: &cx.cache, + is_json_output, + document_private: cx.render_options.document_private, + }; stripper.fold_crate(krate) } diff --git a/src/librustdoc/passes/strip_private.rs b/src/librustdoc/passes/strip_private.rs index 9ba841a31cf95..f3aa3c7ce2459 100644 --- a/src/librustdoc/passes/strip_private.rs +++ b/src/librustdoc/passes/strip_private.rs @@ -17,6 +17,7 @@ pub(crate) const STRIP_PRIVATE: Pass = Pass { pub(crate) fn strip_private(mut krate: clean::Crate, cx: &mut DocContext<'_>) -> clean::Crate { // This stripper collects all *retained* nodes. let mut retained = ItemIdSet::default(); + let is_json_output = cx.output_format.is_json() && !cx.show_coverage; // strip all private items { @@ -24,12 +25,17 @@ pub(crate) fn strip_private(mut krate: clean::Crate, cx: &mut DocContext<'_>) -> retained: &mut retained, access_levels: &cx.cache.access_levels, update_retained: true, - is_json_output: cx.output_format.is_json() && !cx.show_coverage, + is_json_output, }; krate = ImportStripper.fold_crate(stripper.fold_crate(krate)); } // strip all impls referencing private items - let mut stripper = ImplStripper { retained: &retained, cache: &cx.cache }; + let mut stripper = ImplStripper { + retained: &retained, + cache: &cx.cache, + is_json_output, + document_private: cx.render_options.document_private, + }; stripper.fold_crate(krate) } diff --git a/src/librustdoc/passes/stripper.rs b/src/librustdoc/passes/stripper.rs index 0d419042a10a4..3f069e8393f7e 100644 --- a/src/librustdoc/passes/stripper.rs +++ b/src/librustdoc/passes/stripper.rs @@ -14,17 +14,19 @@ pub(crate) struct Stripper<'a> { pub(crate) is_json_output: bool, } -impl<'a> Stripper<'a> { - // We need to handle this differently for the JSON output because some non exported items could - // be used in public API. And so, we need these items as well. `is_exported` only checks if they - // are in the public API, which is not enough. - #[inline] - fn is_item_reachable(&self, item_id: ItemId) -> bool { - if self.is_json_output { - self.access_levels.is_reachable(item_id.expect_def_id()) - } else { - self.access_levels.is_exported(item_id.expect_def_id()) - } +// We need to handle this differently for the JSON output because some non exported items could +// be used in public API. And so, we need these items as well. `is_exported` only checks if they +// are in the public API, which is not enough. +#[inline] +fn is_item_reachable( + is_json_output: bool, + access_levels: &AccessLevels, + item_id: ItemId, +) -> bool { + if is_json_output { + access_levels.is_reachable(item_id.expect_def_id()) + } else { + access_levels.is_exported(item_id.expect_def_id()) } } @@ -61,7 +63,9 @@ impl<'a> DocFolder for Stripper<'a> { | clean::MacroItem(..) | clean::ForeignTypeItem => { let item_id = i.item_id; - if item_id.is_local() && !self.is_item_reachable(item_id) { + if item_id.is_local() + && !is_item_reachable(self.is_json_output, self.access_levels, item_id) + { debug!("Stripper: stripping {:?} {:?}", i.type_(), i.name); return None; } @@ -133,6 +137,8 @@ impl<'a> DocFolder for Stripper<'a> { pub(crate) struct ImplStripper<'a> { pub(crate) retained: &'a ItemIdSet, pub(crate) cache: &'a Cache, + pub(crate) is_json_output: bool, + pub(crate) document_private: bool, } impl<'a> DocFolder for ImplStripper<'a> { @@ -140,8 +146,27 @@ impl<'a> DocFolder for ImplStripper<'a> { if let clean::ImplItem(ref imp) = *i.kind { // Impl blocks can be skipped if they are: empty; not a trait impl; and have no // documentation. - if imp.trait_.is_none() && imp.items.is_empty() && i.doc_value().is_none() { - return None; + // + // There is one special case: if the impl block contains only private items. + if imp.trait_.is_none() { + // If the only items present are private ones and we're not rendering private items, + // we don't document it. + if !imp.items.is_empty() + && !self.document_private + && imp.items.iter().all(|i| { + let item_id = i.item_id; + item_id.is_local() + && !is_item_reachable( + self.is_json_output, + &self.cache.access_levels, + item_id, + ) + }) + { + return None; + } else if imp.items.is_empty() && i.doc_value().is_none() { + return None; + } } if let Some(did) = imp.for_.def_id(self.cache) { if did.is_local() && !imp.for_.is_assoc_ty() && !self.retained.contains(&did.into()) From c634852cfb986c8530c6ad7133da51d70cd63f9d Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 9 Aug 2022 15:49:55 +0200 Subject: [PATCH 2/2] Add test for impl blocks containing only private items --- .../empty-impl-block-private-with-doc.rs | 44 +++++++++++++++++++ src/test/rustdoc/empty-impl-block-private.rs | 40 +++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 src/test/rustdoc/empty-impl-block-private-with-doc.rs create mode 100644 src/test/rustdoc/empty-impl-block-private.rs diff --git a/src/test/rustdoc/empty-impl-block-private-with-doc.rs b/src/test/rustdoc/empty-impl-block-private-with-doc.rs new file mode 100644 index 0000000000000..4397199616302 --- /dev/null +++ b/src/test/rustdoc/empty-impl-block-private-with-doc.rs @@ -0,0 +1,44 @@ +// compile-flags: --document-private-items + +#![feature(inherent_associated_types)] +#![allow(incomplete_features)] +#![crate_name = "foo"] + +// @has 'foo/struct.Foo.html' +pub struct Foo; + +// There are 3 impl blocks with public item and one that should not be displayed +// by default because it only contains private items (but not in this case because +// we used `--document-private-items`). +// @count - '//*[@class="impl has-srclink"]' 'impl Foo' 4 + +// Impl block only containing private items should not be displayed unless the +// `--document-private-items` flag is used. +/// Private +impl Foo { + const BAR: u32 = 0; + type FOO = i32; + fn hello() {} +} + +// But if any element of the impl block is public, it should be displayed. +/// Not private +impl Foo { + pub const BAR: u32 = 0; + type FOO = i32; + fn hello() {} +} + +/// Not private +impl Foo { + const BAR: u32 = 0; + pub type FOO = i32; + fn hello() {} +} + +/// Not private +impl Foo { + const BAR: u32 = 0; + type FOO = i32; + pub fn hello() {} +} diff --git a/src/test/rustdoc/empty-impl-block-private.rs b/src/test/rustdoc/empty-impl-block-private.rs new file mode 100644 index 0000000000000..5caf020658c5f --- /dev/null +++ b/src/test/rustdoc/empty-impl-block-private.rs @@ -0,0 +1,40 @@ +#![feature(inherent_associated_types)] +#![allow(incomplete_features)] +#![crate_name = "foo"] + +// @has 'foo/struct.Foo.html' +pub struct Foo; + +// There are 3 impl blocks with public item and one that should not be displayed +// because it only contains private items. +// @count - '//*[@class="impl has-srclink"]' 'impl Foo' 3 + +// Impl block only containing private items should not be displayed. +/// Private +impl Foo { + const BAR: u32 = 0; + type FOO = i32; + fn hello() {} +} + +// But if any element of the impl block is public, it should be displayed. +/// Not private +impl Foo { + pub const BAR: u32 = 0; + type FOO = i32; + fn hello() {} +} + +/// Not private +impl Foo { + const BAR: u32 = 0; + pub type FOO = i32; + fn hello() {} +} + +/// Not private +impl Foo { + const BAR: u32 = 0; + type FOO = i32; + pub fn hello() {} +}