Rollup merge of #100323 - GuillaumeGomez:impl-blocks-only-private, r=notriddle

[rustdoc] Don't render impl blocks with doc comment if they only contain private items by default

Fixes #100001.

cc `@jhpratt`
r? `@notriddle`

Author: matthiaskrgr

Diff for: 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 for: src/librustdoc/passes/strip_private.rs

@@ -17,19 +17,25 @@ 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
     {
         let mut stripper = Stripper {
             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 for: 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<DefId>,
+    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,15 +137,36 @@ 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> {
     fn fold_item(&mut self, i: Item) -> Option<Item> {
         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())

Diff for: 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 for: 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() {}
+}