Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are name and renamed both needed? It seems like name is completely ignored if renamed is Some.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered if I should pass the "computed" name or if I should do it in the function. In the end I picked this approach because it's not possible to pass the "non-computed" name since you need to pass both name and renamed.

) -> 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 @@ -2288,29 +2320,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 @@ -260,7 +279,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 @@ -277,7 +296,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 @@ -426,7 +445,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 @@ -479,8 +498,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 @@ -500,19 +526,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;