Skip to content

Commit

Permalink
Rollup merge of #111095 - GuillaumeGomez:fix-assoc-item-trait-inside-…
Browse files Browse the repository at this point in the history
…hidden, r=notriddle

Correctly handle associated items of a trait inside a `#[doc(hidden)]` item

Fixes #111064.

cc `@compiler-errors`
r? `@notriddle`
  • Loading branch information
matthiaskrgr authored May 10, 2023
2 parents f192227 + 8de4308 commit d117b41
Show file tree
Hide file tree
Showing 7 changed files with 162 additions and 46 deletions.
65 changes: 41 additions & 24 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,39 @@ pub(crate) fn clean_doc_module<'tcx>(doc: &DocModule<'tcx>, cx: &mut DocContext<
});

let kind = ModuleItem(Module { items, span });
Item::from_def_id_and_parts(doc.def_id.to_def_id(), Some(doc.name), kind, cx)
generate_item_with_correct_attrs(cx, kind, doc.def_id, doc.name, doc.import_id, doc.renamed)
}

fn generate_item_with_correct_attrs(
cx: &mut DocContext<'_>,
kind: ItemKind,
local_def_id: LocalDefId,
name: Symbol,
import_id: Option<LocalDefId>,
renamed: Option<Symbol>,
) -> Item {
let def_id = local_def_id.to_def_id();
let target_attrs = inline::load_attrs(cx, def_id);
let attrs = if let Some(import_id) = import_id {
let is_inline = inline::load_attrs(cx, import_id.to_def_id())
.lists(sym::doc)
.get_word_attr(sym::inline)
.is_some();
let mut attrs = get_all_import_attributes(cx, import_id, local_def_id, is_inline);
add_without_unwanted_attributes(&mut attrs, target_attrs, is_inline, None);
attrs
} else {
// We only keep the item's attributes.
target_attrs.iter().map(|attr| (Cow::Borrowed(attr), None)).collect()
};

let cfg = attrs.cfg(cx.tcx, &cx.cache.hidden_cfg);
let attrs = Attributes::from_ast_iter(attrs.iter().map(|(attr, did)| (&**attr, *did)), false);

let name = renamed.or(Some(name));
let mut item = Item::from_def_id_and_attrs_and_parts(def_id, name, kind, Box::new(attrs), cfg);
item.inline_stmt_id = import_id.map(|local| local.to_def_id());
item
}

fn clean_generic_bound<'tcx>(
Expand Down Expand Up @@ -2345,29 +2377,14 @@ fn clean_maybe_renamed_item<'tcx>(
_ => unreachable!("not yet converted"),
};

let target_attrs = inline::load_attrs(cx, def_id);
let attrs = if let Some(import_id) = import_id {
let is_inline = inline::load_attrs(cx, import_id.to_def_id())
.lists(sym::doc)
.get_word_attr(sym::inline)
.is_some();
let mut attrs =
get_all_import_attributes(cx, import_id, item.owner_id.def_id, is_inline);
add_without_unwanted_attributes(&mut attrs, target_attrs, is_inline, None);
attrs
} else {
// We only keep the item's attributes.
target_attrs.iter().map(|attr| (Cow::Borrowed(attr), None)).collect()
};

let cfg = attrs.cfg(cx.tcx, &cx.cache.hidden_cfg);
let attrs =
Attributes::from_ast_iter(attrs.iter().map(|(attr, did)| (&**attr, *did)), false);

let mut item =
Item::from_def_id_and_attrs_and_parts(def_id, Some(name), kind, Box::new(attrs), cfg);
item.inline_stmt_id = import_id.map(|local| local.to_def_id());
vec![item]
vec![generate_item_with_correct_attrs(
cx,
kind,
item.owner_id.def_id,
name,
import_id,
renamed,
)]
})
}

Expand Down
8 changes: 7 additions & 1 deletion src/librustdoc/formats/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,13 @@ impl Cache {
impl<'a, 'tcx> DocFolder for CacheBuilder<'a, 'tcx> {
fn fold_item(&mut self, item: clean::Item) -> Option<clean::Item> {
if item.item_id.is_local() {
debug!("folding {} \"{:?}\", id {:?}", item.type_(), item.name, item.item_id);
let is_stripped = matches!(*item.kind, clean::ItemKind::StrippedItem(..));
debug!(
"folding {} (stripped: {is_stripped:?}) \"{:?}\", id {:?}",
item.type_(),
item.name,
item.item_id
);
}

// If this is a stripped module,
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/passes/check_doc_test_visibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,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)
|| inherits_doc_hidden(cx.tcx, def_id, None)
|| cx.tcx.def_span(def_id.to_def_id()).in_derive_expansion()
{
return false;
Expand Down
30 changes: 25 additions & 5 deletions src/librustdoc/passes/strip_hidden.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Strip all doc(hidden) items from the output.
use rustc_hir::def_id::LocalDefId;
use rustc_middle::ty::TyCtxt;
use rustc_span::symbol::sym;
use std::mem;
Expand Down Expand Up @@ -29,6 +30,7 @@ pub(crate) fn strip_hidden(krate: clean::Crate, cx: &mut DocContext<'_>) -> clea
update_retained: true,
tcx: cx.tcx,
is_in_hidden_item: false,
last_reexport: None,
};
stripper.fold_crate(krate)
};
Expand All @@ -49,13 +51,24 @@ struct Stripper<'a, 'tcx> {
update_retained: bool,
tcx: TyCtxt<'tcx>,
is_in_hidden_item: bool,
last_reexport: Option<LocalDefId>,
}

impl<'a, 'tcx> Stripper<'a, 'tcx> {
fn set_last_reexport_then_fold_item(&mut self, i: Item) -> Item {
let prev_from_reexport = self.last_reexport;
if i.inline_stmt_id.is_some() {
self.last_reexport = i.item_id.as_def_id().and_then(|def_id| def_id.as_local());
}
let ret = self.fold_item_recur(i);
self.last_reexport = prev_from_reexport;
ret
}

fn set_is_in_hidden_item_and_fold(&mut self, is_in_hidden_item: bool, i: Item) -> Item {
let prev = self.is_in_hidden_item;
self.is_in_hidden_item |= is_in_hidden_item;
let ret = self.fold_item_recur(i);
let ret = self.set_last_reexport_then_fold_item(i);
self.is_in_hidden_item = prev;
ret
}
Expand All @@ -64,7 +77,7 @@ impl<'a, 'tcx> Stripper<'a, 'tcx> {
/// of `is_in_hidden_item` to `true` because the impl children inherit its visibility.
fn recurse_in_impl_or_exported_macro(&mut self, i: Item) -> Item {
let prev = mem::replace(&mut self.is_in_hidden_item, false);
let ret = self.fold_item_recur(i);
let ret = self.set_last_reexport_then_fold_item(i);
self.is_in_hidden_item = prev;
ret
}
Expand All @@ -86,13 +99,20 @@ impl<'a, 'tcx> DocFolder for Stripper<'a, 'tcx> {
if !is_impl_or_exported_macro {
is_hidden = self.is_in_hidden_item || has_doc_hidden;
if !is_hidden && i.inline_stmt_id.is_none() {
// We don't need to check if it's coming from a reexport since the reexport itself was
// already checked.
// `i.inline_stmt_id` is `Some` if the item is directly reexported. If it is, we
// don't need to check it, because the reexport itself was already checked.
//
// If this item is the child of a reexported module, `self.last_reexport` will be
// `Some` even though `i.inline_stmt_id` is `None`. Hiddenness inheritance needs to
// account for the possibility that an item's true parent module is hidden, but it's
// inlined into a visible module true. This code shouldn't be reachable if the
// module's reexport is itself hidden, for the same reason it doesn't need to be
// checked if `i.inline_stmt_id` is Some: hidden reexports are never inlined.
is_hidden = i
.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| inherits_doc_hidden(self.tcx, def_id, self.last_reexport))
.unwrap_or(false);
}
}
Expand Down
51 changes: 36 additions & 15 deletions src/librustdoc/visit_ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ pub(crate) struct Module<'hir> {
pub(crate) where_inner: Span,
pub(crate) mods: Vec<Module<'hir>>,
pub(crate) def_id: LocalDefId,
pub(crate) renamed: Option<Symbol>,
pub(crate) import_id: Option<LocalDefId>,
/// The key is the item `ItemId` and the value is: (item, renamed, import_id).
/// We use `FxIndexMap` to keep the insert order.
pub(crate) items: FxIndexMap<
Expand All @@ -37,11 +39,19 @@ pub(crate) struct Module<'hir> {
}

impl Module<'_> {
pub(crate) fn new(name: Symbol, def_id: LocalDefId, where_inner: Span) -> Self {
pub(crate) fn new(
name: Symbol,
def_id: LocalDefId,
where_inner: Span,
renamed: Option<Symbol>,
import_id: Option<LocalDefId>,
) -> Self {
Module {
name,
def_id,
where_inner,
renamed,
import_id,
mods: Vec::new(),
items: FxIndexMap::default(),
foreigns: Vec::new(),
Expand All @@ -60,9 +70,16 @@ fn def_id_to_path(tcx: TyCtxt<'_>, did: DefId) -> Vec<Symbol> {
std::iter::once(crate_name).chain(relative).collect()
}

pub(crate) fn inherits_doc_hidden(tcx: TyCtxt<'_>, mut def_id: LocalDefId) -> bool {
pub(crate) fn inherits_doc_hidden(
tcx: TyCtxt<'_>,
mut def_id: LocalDefId,
stop_at: Option<LocalDefId>,
) -> bool {
let hir = tcx.hir();
while let Some(id) = tcx.opt_local_parent(def_id) {
if let Some(stop_at) = stop_at && id == stop_at {
return false;
}
def_id = id;
if tcx.is_doc_hidden(def_id.to_def_id()) {
return true;
Expand Down Expand Up @@ -100,6 +117,8 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
cx.tcx.crate_name(LOCAL_CRATE),
CRATE_DEF_ID,
cx.tcx.hir().root_module().spans.inner_span,
None,
None,
);

RustdocVisitor {
Expand Down Expand Up @@ -261,7 +280,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 = inherits_doc_hidden(self.cx.tcx, res_did, None);

// Only inline if requested or if the item would otherwise be stripped.
if (!please_inline && !is_private && !is_hidden) || is_no_inline {
Expand All @@ -278,7 +297,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)
!inherits_doc_hidden(self.cx.tcx, item_def_id, None)
{
// The imported item is public and not `doc(hidden)` so no need to inline it.
return false;
Expand Down Expand Up @@ -427,7 +446,7 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
}
}
hir::ItemKind::Mod(ref m) => {
self.enter_mod(item.owner_id.def_id, m, name);
self.enter_mod(item.owner_id.def_id, m, name, renamed, import_id);
}
hir::ItemKind::Fn(..)
| hir::ItemKind::ExternCrate(..)
Expand Down Expand Up @@ -480,8 +499,15 @@ impl<'a, 'tcx> RustdocVisitor<'a, 'tcx> {
/// This method will create a new module and push it onto the "modules stack" then call
/// `visit_mod_contents`. Once done, it'll remove it from the "modules stack" and instead
/// add into the list of modules of the current module.
fn enter_mod(&mut self, id: LocalDefId, m: &'tcx hir::Mod<'tcx>, name: Symbol) {
self.modules.push(Module::new(name, id, m.spans.inner_span));
fn enter_mod(
&mut self,
id: LocalDefId,
m: &'tcx hir::Mod<'tcx>,
name: Symbol,
renamed: Option<Symbol>,
import_id: Option<LocalDefId>,
) {
self.modules.push(Module::new(name, id, m.spans.inner_span, renamed, import_id));

self.visit_mod_contents(id, m);

Expand All @@ -501,19 +527,14 @@ impl<'a, 'tcx> Visitor<'tcx> for RustdocVisitor<'a, 'tcx> {

fn visit_item(&mut self, i: &'tcx hir::Item<'tcx>) {
self.visit_item_inner(i, None, None);
let new_value = if self.is_importable_from_parent {
matches!(
let new_value = self.is_importable_from_parent
&& matches!(
i.kind,
hir::ItemKind::Mod(..)
| hir::ItemKind::ForeignMod { .. }
| hir::ItemKind::Impl(..)
| hir::ItemKind::Trait(..)
)
} else {
// Whatever the context, if it's an impl block, the items inside it can be used so they
// should be visible.
matches!(i.kind, hir::ItemKind::Impl(..))
};
);
let prev = mem::replace(&mut self.is_importable_from_parent, new_value);
walk_item(self, i);
self.is_importable_from_parent = prev;
Expand Down
31 changes: 31 additions & 0 deletions tests/rustdoc/issue-111064-reexport-trait-from-hidden-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#![feature(no_core)]
#![no_core]
#![crate_name = "foo"]

// @!has 'foo/hidden/index.html'
// FIXME: add missing `@` for the two next tests once issue is fixed!
// To be done in <https://github.com/rust-lang/rust/issues/111249>.
// !has 'foo/hidden/inner/index.html'
// !has 'foo/hidden/inner/trait.Foo.html'
#[doc(hidden)]
pub mod hidden {
pub mod inner {
pub trait Foo {
/// Hello, world!
fn test();
}
}
}

// @has 'foo/visible/index.html'
// @has 'foo/visible/trait.Foo.html'
#[doc(inline)]
pub use hidden::inner as visible;

// @has 'foo/struct.Bar.html'
// @count - '//*[@id="impl-Foo-for-Bar"]' 1
pub struct Bar;

impl visible::Foo for Bar {
fn test() {}
}
21 changes: 21 additions & 0 deletions tests/rustdoc/issue-111064-reexport-trait-from-hidden.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Regression test for <https://github.com/rust-lang/rust/issues/111064>.
// Methods from a re-exported trait inside a `#[doc(hidden)]` item should
// be visible.

#![crate_name = "foo"]

// @has 'foo/index.html'
// @has - '//*[@id="main-content"]//*[@class="item-name"]/a[@href="trait.Foo.html"]' 'Foo'

// @has 'foo/trait.Foo.html'
// @has - '//*[@id="main-content"]//*[@class="code-header"]' 'fn test()'

#[doc(hidden)]
mod hidden {
pub trait Foo {
/// Hello, world!
fn test();
}
}

pub use hidden::Foo;

0 comments on commit d117b41

Please sign in to comment.