Rollup merge of #96008 - fmease:warn-on-useless-doc-hidden-on-assoc-i…

…mpl-items, r=lcnr Warn on unused `#[doc(hidden)]` attributes on trait impl items [Zulip conversation](https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/.E2.9C.94.20Validy.20checks.20for.20.60.23.5Bdoc.28hidden.29.5D.60). Whether an associated item in a trait impl is shown or hidden in the documentation entirely depends on the corresponding item in the trait declaration. Rustdoc completely ignores `#[doc(hidden)]` attributes on impl items. No error or warning is emitted: ```rust pub trait Tr { fn f(); } pub struct Ty; impl Tr for Ty { #[doc(hidden)] fn f() {} } // ^^^^^^^^^^^^^^ ignored by rustdoc and currently // no error or warning issued ``` This may lead users to the wrong belief that the attribute has an effect. In fact, several such cases are found in the standard library (I've removed all of them in this PR). There does not seem to exist any incentive to allow this in the future either: Impl'ing a trait for a type means the type *fully* conforms to its API. Users can add `#[doc(hidden)]` to the whole impl if they want to hide the implementation or add the attribute to the corresponding associated item in the trait declaration to hide the specific item. Hiding an implementation of an associated item does not make much sense: The associated item can still be found on the trait page. This PR emits the warn-by-default lint `unused_attribute` for this case with a future-incompat warning. `@rustbot` label T-compiler T-rustdoc A-lint
rust-lang · May 9, 2022 · 6c8001b · 6c8001b
2 parents 28d800c + 9d157ad
commit 6c8001b
Show file tree

Hide file tree

Showing 18 changed files with 225 additions and 26 deletions.
diff --git a/compiler/rustc_passes/src/check_attr.rs b/compiler/rustc_passes/src/check_attr.rs
@@ -4,7 +4,8 @@
 //! conflicts between multiple such attributes attached to the same
 //! item.
 
-use rustc_ast::{ast, AttrStyle, Attribute, Lit, LitKind, MetaItemKind, NestedMetaItem};
+use rustc_ast::tokenstream::DelimSpan;
+use rustc_ast::{ast, AttrStyle, Attribute, Lit, LitKind, MacArgs, MetaItemKind, NestedMetaItem};
 use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::{pluralize, struct_span_err, Applicability, MultiSpan};
 use rustc_feature::{AttributeDuplicates, AttributeType, BuiltinAttribute, BUILTIN_ATTRIBUTE_MAP};
@@ -810,6 +811,68 @@ impl CheckAttrVisitor<'_> {
         }
     }
 
+    /// Checks `#[doc(hidden)]` attributes. Returns `true` if valid.
+    fn check_doc_hidden(
+        &self,
+        attr: &Attribute,
+        meta_index: usize,
+        meta: &NestedMetaItem,
+        hir_id: HirId,
+        target: Target,
+    ) -> bool {
+        if let Target::AssocConst
+        | Target::AssocTy
+        | Target::Method(MethodKind::Trait { body: true }) = target
+        {
+            let parent_hir_id = self.tcx.hir().get_parent_item(hir_id);
+            let containing_item = self.tcx.hir().expect_item(parent_hir_id);
+
+            if Target::from_item(containing_item) == Target::Impl {
+                let meta_items = attr.meta_item_list().unwrap();
+
+                let (span, replacement_span) = if meta_items.len() == 1 {
+                    (attr.span, attr.span)
+                } else {
+                    let meta_span = meta.span();
+                    (
+                        meta_span,
+                        meta_span.until(match meta_items.get(meta_index + 1) {
+                            Some(next_item) => next_item.span(),
+                            None => match attr.get_normal_item().args {
+                                MacArgs::Delimited(DelimSpan { close, .. }, ..) => close,
+                                _ => unreachable!(),
+                            },
+                        }),
+                    )
+                };
+
+                // FIXME: #[doc(hidden)] was previously erroneously allowed on trait impl items,
+                // so for backward compatibility only emit a warning and do not mark it as invalid.
+                self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, span, |lint| {
+                    lint.build("`#[doc(hidden)]` is ignored on trait impl items")
+                        .warn(
+                            "this was previously accepted by the compiler but is \
+                             being phased out; it will become a hard error in \
+                             a future release!",
+                        )
+                        .note(
+                            "whether the impl item is `doc(hidden)` or not \
+                             entirely depends on the corresponding trait item",
+                        )
+                        .span_suggestion(
+                            replacement_span,
+                            "remove this attribute",
+                            String::new(),
+                            Applicability::MachineApplicable,
+                        )
+                        .emit();
+                });
+            }
+        }
+
+        true
+    }
+
     /// Checks that an attribute is *not* used at the crate level. Returns `true` if valid.
     fn check_attr_not_crate_level(
         &self,
@@ -928,7 +991,7 @@ impl CheckAttrVisitor<'_> {
         let mut is_valid = true;
 
         if let Some(mi) = attr.meta() && let Some(list) = mi.meta_item_list() {
-            for meta in list {
+            for (meta_index, meta) in list.into_iter().enumerate() {
                 if let Some(i_meta) = meta.meta_item() {
                     match i_meta.name_or_empty() {
                         sym::alias
@@ -969,6 +1032,15 @@ impl CheckAttrVisitor<'_> {
                             is_valid = false;
                         }
 
+                        sym::hidden if !self.check_doc_hidden(attr,
+                            meta_index,
+                            meta,
+                            hir_id,
+                            target,
+                            ) => {
+                            is_valid = false;
+                        }
+
                         // no_default_passes: deprecated
                         // passes: deprecated
                         // plugins: removed, but rustdoc warns about it itself

diff --git a/library/alloc/src/collections/vec_deque/iter.rs b/library/alloc/src/collections/vec_deque/iter.rs
@@ -122,7 +122,6 @@ impl<'a, T> Iterator for Iter<'a, T> {
     }
 
     #[inline]
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         // Safety: The TrustedRandomAccess contract requires that callers only pass an index
         // that is in bounds.

diff --git a/library/alloc/src/collections/vec_deque/iter_mut.rs b/library/alloc/src/collections/vec_deque/iter_mut.rs
@@ -100,7 +100,6 @@ impl<'a, T> Iterator for IterMut<'a, T> {
     }
 
     #[inline]
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         // Safety: The TrustedRandomAccess contract requires that callers only pass an index
         // that is in bounds.

diff --git a/library/alloc/src/vec/into_iter.rs b/library/alloc/src/vec/into_iter.rs
@@ -202,7 +202,6 @@ impl<T, A: Allocator> Iterator for IntoIter<T, A> {
         self.len()
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, i: usize) -> Self::Item
     where
         Self: TrustedRandomAccessNoCoerce,

diff --git a/library/core/src/convert/num.rs b/library/core/src/convert/num.rs
@@ -25,7 +25,6 @@ macro_rules! impl_float_to_int {
         $(
             #[unstable(feature = "convert_float_to_int", issue = "67057")]
             impl FloatToInt<$Int> for $Float {
-                #[doc(hidden)]
                 #[inline]
                 unsafe fn to_int_unchecked(self) -> $Int {
                     // SAFETY: the safety contract must be upheld by the caller.

diff --git a/library/core/src/iter/adapters/cloned.rs b/library/core/src/iter/adapters/cloned.rs
@@ -60,7 +60,6 @@ where
         self.it.map(T::clone).fold(init, f)
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> T
     where
         Self: TrustedRandomAccessNoCoerce,

diff --git a/library/core/src/iter/adapters/copied.rs b/library/core/src/iter/adapters/copied.rs
@@ -81,7 +81,6 @@ where
         self.it.advance_by(n)
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> T
     where
         Self: TrustedRandomAccessNoCoerce,

diff --git a/library/core/src/iter/adapters/enumerate.rs b/library/core/src/iter/adapters/enumerate.rs
@@ -128,7 +128,6 @@ where
     }
 
     #[rustc_inherit_overflow_checks]
-    #[doc(hidden)]
     #[inline]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> <Self as Iterator>::Item
     where

diff --git a/library/core/src/iter/adapters/fuse.rs b/library/core/src/iter/adapters/fuse.rs
@@ -129,7 +129,6 @@ where
     }
 
     #[inline]
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
     where
         Self: TrustedRandomAccessNoCoerce,

diff --git a/library/core/src/iter/adapters/map.rs b/library/core/src/iter/adapters/map.rs
@@ -124,7 +124,6 @@ where
         self.iter.fold(init, map_fold(self.f, g))
     }
 
-    #[doc(hidden)]
     #[inline]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> B
     where

diff --git a/library/core/src/iter/adapters/zip.rs b/library/core/src/iter/adapters/zip.rs
@@ -95,7 +95,6 @@ where
     }
 
     #[inline]
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
     where
         Self: TrustedRandomAccessNoCoerce,

diff --git a/library/core/src/iter/range.rs b/library/core/src/iter/range.rs
@@ -752,7 +752,6 @@ impl<A: Step> Iterator for ops::Range<A> {
     }
 
     #[inline]
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item
     where
         Self: TrustedRandomAccessNoCoerce,

diff --git a/library/core/src/slice/iter.rs b/library/core/src/slice/iter.rs
@@ -1322,7 +1322,6 @@ impl<'a, T> Iterator for Windows<'a, T> {
         }
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         // SAFETY: since the caller guarantees that `i` is in bounds,
         // which means that `i` cannot overflow an `isize`, and the
@@ -1478,7 +1477,6 @@ impl<'a, T> Iterator for Chunks<'a, T> {
         }
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         let start = idx * self.chunk_size;
         // SAFETY: the caller guarantees that `i` is in bounds,
@@ -1657,7 +1655,6 @@ impl<'a, T> Iterator for ChunksMut<'a, T> {
         }
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         let start = idx * self.chunk_size;
         // SAFETY: see comments for `Chunks::__iterator_get_unchecked`.
@@ -1830,7 +1827,6 @@ impl<'a, T> Iterator for ChunksExact<'a, T> {
         self.next_back()
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         let start = idx * self.chunk_size;
         // SAFETY: mostly identical to `Chunks::__iterator_get_unchecked`.
@@ -1984,7 +1980,6 @@ impl<'a, T> Iterator for ChunksExactMut<'a, T> {
         self.next_back()
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         let start = idx * self.chunk_size;
         // SAFETY: see comments for `ChunksMut::__iterator_get_unchecked`.
@@ -2248,7 +2243,6 @@ impl<'a, T, const N: usize> Iterator for ArrayChunks<'a, T, N> {
         self.iter.last()
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, i: usize) -> &'a [T; N] {
         // SAFETY: The safety guarantees of `__iterator_get_unchecked` are
         // transferred to the caller.
@@ -2367,7 +2361,6 @@ impl<'a, T, const N: usize> Iterator for ArrayChunksMut<'a, T, N> {
         self.iter.last()
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, i: usize) -> &'a mut [T; N] {
         // SAFETY: The safety guarantees of `__iterator_get_unchecked` are transferred to
         // the caller.
@@ -2520,7 +2513,6 @@ impl<'a, T> Iterator for RChunks<'a, T> {
         }
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         let end = self.v.len() - idx * self.chunk_size;
         let start = match end.checked_sub(self.chunk_size) {
@@ -2689,7 +2681,6 @@ impl<'a, T> Iterator for RChunksMut<'a, T> {
         }
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         let end = self.v.len() - idx * self.chunk_size;
         let start = match end.checked_sub(self.chunk_size) {
@@ -2856,7 +2847,6 @@ impl<'a, T> Iterator for RChunksExact<'a, T> {
         self.next_back()
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         let end = self.v.len() - idx * self.chunk_size;
         let start = end - self.chunk_size;
@@ -3016,7 +3006,6 @@ impl<'a, T> Iterator for RChunksExactMut<'a, T> {
         self.next_back()
     }
 
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
         let end = self.v.len() - idx * self.chunk_size;
         let start = end - self.chunk_size;

diff --git a/library/core/src/slice/iter/macros.rs b/library/core/src/slice/iter/macros.rs
@@ -325,7 +325,6 @@ macro_rules! iterator {
                 None
             }
 
-            #[doc(hidden)]
             #[inline]
             unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> Self::Item {
                 // SAFETY: the caller must guarantee that `i` is in bounds of

diff --git a/library/core/src/str/iter.rs b/library/core/src/str/iter.rs
@@ -298,7 +298,6 @@ impl Iterator for Bytes<'_> {
     }
 
     #[inline]
-    #[doc(hidden)]
     unsafe fn __iterator_get_unchecked(&mut self, idx: usize) -> u8 {
         // SAFETY: the caller must uphold the safety contract
         // for `Iterator::__iterator_get_unchecked`.

diff --git a/src/test/ui/lint/unused/unused-attr-doc-hidden.fixed b/src/test/ui/lint/unused/unused-attr-doc-hidden.fixed
@@ -0,0 +1,42 @@
+#![deny(unused_attributes)]
+#![crate_type = "lib"]
+// run-rustfix
+
+pub trait Trait {
+    type It;
+    const IT: ();
+    fn it0();
+    fn it1();
+    fn it2();
+}
+
+pub struct Implementor;
+
+impl Trait for Implementor {
+
+    type It = ();
+    //~^^ ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+
+
+    const IT: () = ();
+    //~^^ ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+
+    #[doc(alias = "aka")]
+    fn it0() {}
+    //~^^ ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+
+    #[doc(alias = "this", )]
+    fn it1() {}
+    //~^^ ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+
+    #[doc()]
+    fn it2() {}
+    //~^^ ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+    //~|  ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+}
diff --git a/src/test/ui/lint/unused/unused-attr-doc-hidden.rs b/src/test/ui/lint/unused/unused-attr-doc-hidden.rs
@@ -0,0 +1,42 @@
+#![deny(unused_attributes)]
+#![crate_type = "lib"]
+// run-rustfix
+
+pub trait Trait {
+    type It;
+    const IT: ();
+    fn it0();
+    fn it1();
+    fn it2();
+}
+
+pub struct Implementor;
+
+impl Trait for Implementor {
+    #[doc(hidden)]
+    type It = ();
+    //~^^ ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+
+    #[doc(hidden)]
+    const IT: () = ();
+    //~^^ ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+
+    #[doc(hidden, alias = "aka")]
+    fn it0() {}
+    //~^^ ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+
+    #[doc(alias = "this", hidden,)]
+    fn it1() {}
+    //~^^ ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+
+    #[doc(hidden, hidden)]
+    fn it2() {}
+    //~^^ ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+    //~|  ERROR `#[doc(hidden)]` is ignored
+    //~|  WARNING this was previously accepted
+}