From b949e8e6c1f3d22737452fa8b040d8070de9fe46 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 15 Mar 2023 17:51:20 +0100 Subject: [PATCH 1/4] Move inherits_doc_hidden function directly into TyCtxt --- compiler/rustc_middle/src/ty/context.rs | 25 +++++++++++++++++++ .../passes/check_doc_test_visibility.rs | 3 +-- src/librustdoc/passes/strip_hidden.rs | 3 +-- src/librustdoc/visit_ast.rs | 24 ++---------------- 4 files changed, 29 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index 8dd8f95fcc714..306cccbaa9eff 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -931,6 +931,31 @@ impl<'tcx> TyCtxt<'tcx> { self.def_path(def_id).to_string_no_crate_verbose() ) } + + /// Returns `true` if the provided `def_id` has any ancestor with the `#[doc(hidden)]` + /// attribute. + /// + /// It doesn't check if `def_id` itself has this attribute though. You will need to use + /// [`TyCtxt::is_doc_hidden`] to get this information. + pub fn inherits_doc_hidden(self, mut def_id: LocalDefId) -> bool { + let hir = self.hir(); + while let Some(id) = self.opt_local_parent(def_id) { + def_id = id; + if self.is_doc_hidden(def_id.to_def_id()) { + return true; + } else if let Some(node) = hir.find_by_def_id(def_id) && + matches!( + node, + hir::Node::Item(hir::Item { kind: hir::ItemKind::Impl(_), .. }), + ) + { + // `impl` blocks stand a bit on their own: unless they have `#[doc(hidden)]` directly + // on them, they don't inherit it from the parent context. + return false; + } + } + false + } } impl<'tcx> TyCtxtAt<'tcx> { diff --git a/src/librustdoc/passes/check_doc_test_visibility.rs b/src/librustdoc/passes/check_doc_test_visibility.rs index 6b13e6c9581ae..0872b60048471 100644 --- a/src/librustdoc/passes/check_doc_test_visibility.rs +++ b/src/librustdoc/passes/check_doc_test_visibility.rs @@ -11,7 +11,6 @@ use crate::clean::*; use crate::core::DocContext; use crate::html::markdown::{find_testable_code, ErrorCodes, Ignore, LangString}; use crate::visit::DocVisitor; -use crate::visit_ast::inherits_doc_hidden; use rustc_hir as hir; use rustc_middle::lint::LintLevelSource; use rustc_session::lint; @@ -95,7 +94,7 @@ pub(crate) fn should_have_doc_example(cx: &DocContext<'_>, item: &clean::Item) - } if cx.tcx.is_doc_hidden(def_id.to_def_id()) - || inherits_doc_hidden(cx.tcx, def_id) + || cx.tcx.inherits_doc_hidden(def_id) || cx.tcx.def_span(def_id.to_def_id()).in_derive_expansion() { return false; diff --git a/src/librustdoc/passes/strip_hidden.rs b/src/librustdoc/passes/strip_hidden.rs index 890b3e8d67f7a..2f30a15c81a47 100644 --- a/src/librustdoc/passes/strip_hidden.rs +++ b/src/librustdoc/passes/strip_hidden.rs @@ -9,7 +9,6 @@ use crate::clean::{Item, ItemIdSet, NestedAttributesExt}; use crate::core::DocContext; use crate::fold::{strip_item, DocFolder}; use crate::passes::{ImplStripper, Pass}; -use crate::visit_ast::inherits_doc_hidden; pub(crate) const STRIP_HIDDEN: Pass = Pass { name: "strip-hidden", @@ -92,7 +91,7 @@ impl<'a, 'tcx> DocFolder for Stripper<'a, 'tcx> { .item_id .as_def_id() .and_then(|def_id| def_id.as_local()) - .map(|def_id| inherits_doc_hidden(self.tcx, def_id)) + .map(|def_id| self.tcx.inherits_doc_hidden(def_id)) .unwrap_or(false); } } diff --git a/src/librustdoc/visit_ast.rs b/src/librustdoc/visit_ast.rs index e09a68069e8c7..29b0a0593f11d 100644 --- a/src/librustdoc/visit_ast.rs +++ b/src/librustdoc/visit_ast.rs @@ -59,26 +59,6 @@ fn def_id_to_path(tcx: TyCtxt<'_>, did: DefId) -> Vec { std::iter::once(crate_name).chain(relative).collect() } -pub(crate) fn inherits_doc_hidden(tcx: TyCtxt<'_>, mut def_id: LocalDefId) -> bool { - let hir = tcx.hir(); - while let Some(id) = tcx.opt_local_parent(def_id) { - def_id = id; - if tcx.is_doc_hidden(def_id.to_def_id()) { - return true; - } else if let Some(node) = hir.find_by_def_id(def_id) && - matches!( - node, - hir::Node::Item(hir::Item { kind: hir::ItemKind::Impl(_), .. }), - ) - { - // `impl` blocks stand a bit on their own: unless they have `#[doc(hidden)]` directly - // on them, they don't inherit it from the parent context. - return false; - } - } - false -} - pub(crate) struct RustdocVisitor<'a, 'tcx> { cx: &'a mut core::DocContext<'tcx>, view_item_stack: LocalDefIdSet, @@ -258,7 +238,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { let is_private = !self.cx.cache.effective_visibilities.is_directly_public(self.cx.tcx, ori_res_did); - let is_hidden = inherits_doc_hidden(self.cx.tcx, res_did); + let is_hidden = self.cx.tcx.inherits_doc_hidden(res_did); // Only inline if requested or if the item would otherwise be stripped. if (!please_inline && !is_private && !is_hidden) || is_no_inline { @@ -279,7 +259,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> { .cache .effective_visibilities .is_directly_public(self.cx.tcx, item_def_id.to_def_id()) && - !inherits_doc_hidden(self.cx.tcx, item_def_id) + !self.cx.tcx.inherits_doc_hidden(item_def_id) { // The imported item is public and not `doc(hidden)` so no need to inline it. return false; From f9f4f586e3f77947d69d80ff9cfc62e3e8c5151f Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 15 Mar 2023 17:51:53 +0100 Subject: [PATCH 2/4] Fix invalid missing documentation lint reporting for re-exports --- compiler/rustc_lint/messages.ftl | 1 + compiler/rustc_lint/src/builtin.rs | 117 +++++++++++++++++++++++++++-- compiler/rustc_lint/src/lints.rs | 2 + 3 files changed, 114 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_lint/messages.ftl b/compiler/rustc_lint/messages.ftl index 68e62c9789aed..91fcf5bf6d603 100644 --- a/compiler/rustc_lint/messages.ftl +++ b/compiler/rustc_lint/messages.ftl @@ -370,6 +370,7 @@ lint_builtin_decl_unsafe_method = declaration of an `unsafe` method lint_builtin_impl_unsafe_method = implementation of an `unsafe` method lint_builtin_missing_doc = missing documentation for {$article} {$desc} + .note = because it is re-exported here lint_builtin_missing_copy_impl = type could implement `Copy`; consider adding `impl Copy` diff --git a/compiler/rustc_lint/src/builtin.rs b/compiler/rustc_lint/src/builtin.rs index 5b2100b5da9d1..9d85f23afea1f 100644 --- a/compiler/rustc_lint/src/builtin.rs +++ b/compiler/rustc_lint/src/builtin.rs @@ -66,6 +66,7 @@ use rustc_middle::ty::subst::GenericArgKind; use rustc_middle::ty::{self, Instance, Ty, TyCtxt, VariantDef}; use rustc_session::lint::{BuiltinLintDiagnostics, FutureIncompatibilityReason}; use rustc_span::edition::Edition; +use rustc_span::hygiene::MacroKind; use rustc_span::source_map::Spanned; use rustc_span::symbol::{kw, sym, Ident, Symbol}; use rustc_span::{BytePos, InnerSpan, Span}; @@ -450,6 +451,8 @@ declare_lint! { pub struct MissingDoc { /// Stack of whether `#[doc(hidden)]` is set at each level which has lint attributes. doc_hidden_stack: Vec, + /// Set of types being re-exported while not directly public. + reexported_items: FxHashSet, } impl_lint_pass!(MissingDoc => [MISSING_DOCS]); @@ -480,13 +483,18 @@ fn has_doc(attr: &ast::Attribute) -> bool { impl MissingDoc { pub fn new() -> MissingDoc { - MissingDoc { doc_hidden_stack: vec![false] } + MissingDoc { doc_hidden_stack: vec![false], reexported_items: FxHashSet::default() } } fn doc_hidden(&self) -> bool { *self.doc_hidden_stack.last().expect("empty doc_hidden_stack") } + fn has_docs(&self, cx: &LateContext<'_>, def_id: DefId) -> bool { + let attrs = cx.tcx.get_attrs_unchecked(def_id); + attrs.iter().any(has_doc) + } + fn check_missing_docs_attrs( &self, cx: &LateContext<'_>, @@ -509,18 +517,21 @@ impl MissingDoc { // It's an option so the crate root can also use this function (it doesn't // have a `NodeId`). if def_id != CRATE_DEF_ID { - if !cx.effective_visibilities.is_exported(def_id) { + // We will check re-exported items at a later stage. + if matches!(cx.tcx.def_kind(def_id), DefKind::Macro(MacroKind::Bang)) { + if !cx.effective_visibilities.is_exported(def_id) { + return; + } + } else if !cx.effective_visibilities.is_directly_public(def_id) { return; } } - let attrs = cx.tcx.hir().attrs(cx.tcx.hir().local_def_id_to_hir_id(def_id)); - let has_doc = attrs.iter().any(has_doc); - if !has_doc { + if !self.has_docs(cx, def_id.to_def_id()) { cx.emit_spanned_lint( MISSING_DOCS, cx.tcx.def_span(def_id), - BuiltinMissingDoc { article, desc }, + BuiltinMissingDoc { article, desc, reexport: None }, ); } } @@ -570,6 +581,100 @@ impl<'tcx> LateLintPass<'tcx> for MissingDoc { | hir::ItemKind::Const(..) | hir::ItemKind::Static(..) => {} + hir::ItemKind::Use(ref use_path, hir::UseKind::Single) => { + let use_def_id = it.owner_id.def_id; + if !cx.tcx.is_doc_hidden(use_def_id) && + // If the re-export is `#[doc(hidden)]` or has an ancestor with this attribute, + // we ignore it. + !cx.tcx.inherits_doc_hidden(use_def_id) && + let Some(last_res) = use_path.res.last() && + let Some(res_def_id) = last_res.opt_def_id() && + // If the original item has `#[doc(hidden)]`, we don't consider it because it + // won't appear into the documentation, unless it's a macro. + (!cx.tcx.is_doc_hidden(res_def_id) || matches!(cx.tcx.def_kind(res_def_id), DefKind::Macro(_))) && + // We use this to differentiate betweeen `pub use` and `use` since we're only + // interested about re-exports. + match cx.tcx.local_visibility(use_def_id) { + ty::Visibility::Public => true, + ty::Visibility::Restricted(level) => { + level != cx.tcx.parent_module_from_def_id(use_def_id) + } + } && + // If the re-exported item is not local, it'll be inlined in the documentation + // so it needs to be checked. If it's local and the re-export is the "top one", + // then we check it. + (!res_def_id.is_local() || cx.effective_visibilities.is_directly_public(use_def_id)) && + !self.has_docs(cx, use_def_id.to_def_id()) && + !self.has_docs(cx, res_def_id) && + self.reexported_items.insert(res_def_id) + { + let (article, desc) = cx.tcx.article_and_description(res_def_id); + + cx.emit_spanned_lint( + MISSING_DOCS, + cx.tcx.def_span(res_def_id), + BuiltinMissingDoc { article, desc, reexport: Some(it.span) }, + ); + } + return; + } + hir::ItemKind::Use(ref use_path, hir::UseKind::Glob) => { + let use_def_id = it.owner_id.def_id; + if cx.effective_visibilities.is_directly_public(use_def_id) && + !cx.tcx.is_doc_hidden(use_def_id) && + // If the re-export is `#[doc(hidden)]` or has an ancestor with this attribute, + // we ignore it. + !cx.tcx.inherits_doc_hidden(use_def_id) && + // We use this to differentiate betweeen `pub use` and `use` since we're only + // interested about re-exports. + match cx.tcx.local_visibility(use_def_id) { + ty::Visibility::Public => true, + ty::Visibility::Restricted(level) => { + level != cx.tcx.parent_module_from_def_id(use_def_id) + } + } && + let Some(last_res) = use_path.res.last() && + let Some(res_def_id) = last_res.opt_def_id() && + let Some(res_def_id) = res_def_id.as_local() && + // We only check check glob re-exports of modules. + cx.tcx.def_kind(res_def_id) == DefKind::Mod + { + for child in cx.tcx.hir().module_items(res_def_id) { + let child_def_id = child.owner_id.to_def_id(); + // With glob re-exports, only public items are re-exported. + if cx.effective_visibilities.is_exported(child.owner_id.def_id) && + // If the original item has `#[doc(hidden)]`, we don't consider it because it + // won't appear into the documentation. + !cx.tcx.is_doc_hidden(child_def_id) && + !self.has_docs(cx, child_def_id) && + self.reexported_items.insert(child_def_id) && + matches!( + cx.tcx.def_kind(child_def_id), + DefKind::TyAlias + | DefKind::Trait + | DefKind::Fn + | DefKind::Macro(..) + | DefKind::Mod + | DefKind::Enum + | DefKind::Struct + | DefKind::Union + | DefKind::Const + | DefKind::Static(..) + ) + { + let (article, desc) = cx.tcx.article_and_description(child_def_id); + + cx.emit_spanned_lint( + MISSING_DOCS, + cx.tcx.def_span(child_def_id), + BuiltinMissingDoc { article, desc, reexport: Some(it.span) }, + ); + } + } + } + return; + } + _ => return, }; diff --git a/compiler/rustc_lint/src/lints.rs b/compiler/rustc_lint/src/lints.rs index 20ab0af585651..89d0efce1e23a 100644 --- a/compiler/rustc_lint/src/lints.rs +++ b/compiler/rustc_lint/src/lints.rs @@ -120,6 +120,8 @@ pub enum BuiltinUnsafe { pub struct BuiltinMissingDoc<'a> { pub article: &'a str, pub desc: &'a str, + #[note] + pub reexport: Option, } #[derive(LintDiagnostic)] From aa7e4706a6075120052435f46d04dc5e58079726 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 15 Mar 2023 17:52:03 +0100 Subject: [PATCH 3/4] Add test for #108570. Update UI test with new output. --- .../issue-108570-missing-docs-on-hidden.rs | 17 +++++++++++++++++ tests/ui/lint/lint-missing-doc.stderr | 18 ++++++++++++++++++ tests/ui/privacy/auxiliary/ctor_aux.rs | 2 +- 3 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 tests/ui/lint/issue-108570-missing-docs-on-hidden.rs diff --git a/tests/ui/lint/issue-108570-missing-docs-on-hidden.rs b/tests/ui/lint/issue-108570-missing-docs-on-hidden.rs new file mode 100644 index 0000000000000..2f04110bc5264 --- /dev/null +++ b/tests/ui/lint/issue-108570-missing-docs-on-hidden.rs @@ -0,0 +1,17 @@ +// Regression test for . +// If a `#[doc(hidden)]` item is re-exported or if a private item is re-exported +// with a `#[doc(hidden)]` re-export, it shouldn't complain. + +// check-pass + +#![crate_type = "lib"] + +mod priv_mod { + pub struct Foo; + #[doc(hidden)] + pub struct Bar; +} + +#[doc(hidden)] +pub use priv_mod::Foo; +pub use priv_mod::Bar; diff --git a/tests/ui/lint/lint-missing-doc.stderr b/tests/ui/lint/lint-missing-doc.stderr index 733d062a08ba0..b43469f228c2b 100644 --- a/tests/ui/lint/lint-missing-doc.stderr +++ b/tests/ui/lint/lint-missing-doc.stderr @@ -117,18 +117,36 @@ error: missing documentation for a function | LL | pub fn undocumented1() {} | ^^^^^^^^^^^^^^^^^^^^^^ + | +note: because it is re-exported here + --> $DIR/lint-missing-doc.rs:183:5 + | +LL | pub use internal_impl::undocumented1 as bar; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: missing documentation for a function --> $DIR/lint-missing-doc.rs:170:5 | LL | pub fn undocumented2() {} | ^^^^^^^^^^^^^^^^^^^^^^ + | +note: because it is re-exported here + --> $DIR/lint-missing-doc.rs:184:41 + | +LL | pub use internal_impl::{documented, undocumented2}; + | ^^^^^^^^^^^^^ error: missing documentation for a function --> $DIR/lint-missing-doc.rs:176:9 | LL | pub fn also_undocumented1() {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: because it is re-exported here + --> $DIR/lint-missing-doc.rs:185:5 + | +LL | pub use internal_impl::globbed::*; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: missing documentation for a function --> $DIR/lint-missing-doc.rs:191:5 diff --git a/tests/ui/privacy/auxiliary/ctor_aux.rs b/tests/ui/privacy/auxiliary/ctor_aux.rs index 9c99cca9ae6ed..24fe383eecadd 100644 --- a/tests/ui/privacy/auxiliary/ctor_aux.rs +++ b/tests/ui/privacy/auxiliary/ctor_aux.rs @@ -3,7 +3,7 @@ //! Use the lint to additionally verify that items are reachable //! but not exported. #![allow(non_camel_case_types)] -#![deny(missing_docs)] +// #![deny(missing_docs)] mod hidden { pub struct s; From a2ac0ffc3091ed805442a0935482c51065a0320b Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Wed, 15 Mar 2023 21:06:23 +0100 Subject: [PATCH 4/4] Allow "missing_docs" for some alloc items in test mode --- library/alloc/src/slice.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/alloc/src/slice.rs b/library/alloc/src/slice.rs index 093dcbbe8bf77..aef56feddb132 100644 --- a/library/alloc/src/slice.rs +++ b/library/alloc/src/slice.rs @@ -77,11 +77,13 @@ pub use core::slice::{SplitInclusive, SplitInclusiveMut}; // HACK(japaric) needed for the implementation of `vec!` macro during testing // N.B., see the `hack` module in this file for more details. #[cfg(test)] +#[allow(missing_docs)] pub use hack::into_vec; // HACK(japaric) needed for the implementation of `Vec::clone` during testing // N.B., see the `hack` module in this file for more details. #[cfg(test)] +#[allow(missing_docs)] pub use hack::to_vec; // HACK(japaric): With cfg(test) `impl [T]` is not available, these three