Skip to content

rustdoc: calculate visibility on-demand #91408

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

rustdoc: calculate visibility on-demand #91408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/librustdoc/clean/auto_trait.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
Some(Item {
name: None,
attrs: Default::default(),
visibility: Inherited,
def_id: ItemId::Auto { trait_: trait_def_id, for_: item_def_id },
kind: box ImplItem(Impl {
unsafety: hir::Unsafety::Normal,
Expand All @@ -124,6 +123,7 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
kind: ImplKind::Auto,
}),
cfg: None,
inline_stmt_id: None,
})
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/clean/blanket_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> {
impls.push(Item {
name: None,
attrs: Default::default(),
visibility: Inherited,
def_id: ItemId::Blanket { impl_id: impl_def_id, for_: item_def_id },
kind: box ImplItem(Impl {
unsafety: hir::Unsafety::Normal,
Expand All @@ -127,6 +126,7 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> {
kind: ImplKind::Blanket(box trait_ref.self_ty().clean(self.cx)),
}),
cfg: None,
inline_stmt_id: None,
});
}
}
Expand Down
26 changes: 6 additions & 20 deletions src/librustdoc/clean/inline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::clean::{
use crate::core::DocContext;
use crate::formats::item_type::ItemType;

use super::{Clean, Visibility};
use super::Clean;

type Attrs<'hir> = rustc_middle::ty::Attributes<'hir>;

Expand Down Expand Up @@ -125,11 +125,8 @@ crate fn try_inline(
let (attrs, cfg) = merge_attrs(cx, Some(parent_module), load_attrs(cx, did), attrs_clone);
cx.inlined.insert(did.into());
let mut item =
clean::Item::from_def_id_and_attrs_and_parts(did, Some(name), kind, box attrs, cx, cfg);
if let Some(import_def_id) = import_def_id {
// The visibility needs to reflect the one from the reexport and not from the "source" DefId.
item.visibility = cx.tcx.visibility(import_def_id).clean(cx);
}
clean::Item::from_def_id_and_attrs_and_parts(did, Some(name), kind, box attrs, cfg);
item.inline_stmt_id = import_def_id;
ret.push(item);
Some(ret)
}
Expand Down Expand Up @@ -194,18 +191,8 @@ crate fn record_extern_fqn(cx: &mut DocContext<'_>, did: DefId, kind: ItemType)
}

crate fn build_external_trait(cx: &mut DocContext<'_>, did: DefId) -> clean::Trait {
let trait_items = cx
.tcx
.associated_items(did)
.in_definition_order()
.map(|item| {
// When building an external trait, the cleaned trait will have all items public,
// which causes methods to have a `pub` prefix, which is invalid since items in traits
// can not have a visibility prefix. Thus we override the visibility here manually.
// See https://github.com/rust-lang/rust/issues/81274
clean::Item { visibility: Visibility::Inherited, ..item.clean(cx) }
})
.collect();
let trait_items =
cx.tcx.associated_items(did).in_definition_order().map(|item| item.clean(cx)).collect();

let predicates = cx.tcx.predicates_of(did);
let generics = (cx.tcx.generics_of(did), predicates).clean(cx);
Expand Down Expand Up @@ -497,7 +484,6 @@ crate fn build_impl(
kind: ImplKind::Normal,
}),
box merged_attrs,
cx,
cfg,
));
}
Expand Down Expand Up @@ -527,7 +513,6 @@ fn build_module(
name: None,
attrs: box clean::Attributes::default(),
def_id: ItemId::Primitive(prim_ty, did.krate),
visibility: clean::Public,
kind: box clean::ImportItem(clean::Import::new_simple(
item.ident.name,
clean::ImportSource {
Expand All @@ -546,6 +531,7 @@ fn build_module(
true,
)),
cfg: None,
inline_stmt_id: None,
});
} else if let Some(i) = try_inline(cx, did, None, res, item.ident.name, None, visited) {
items.extend(i)
Expand Down
65 changes: 20 additions & 45 deletions src/librustdoc/clean/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,10 +909,7 @@ impl Clean<Item> for hir::TraitItem<'_> {
AssocTypeItem(bounds, default)
}
};
let what_rustc_thinks =
Item::from_def_id_and_parts(local_did, Some(self.ident.name), inner, cx);
// Trait items always inherit the trait's visibility -- we don't want to show `pub`.
Item { visibility: Inherited, ..what_rustc_thinks }
Item::from_def_id_and_parts(local_did, Some(self.ident.name), inner, cx)
})
}
}
Expand Down Expand Up @@ -948,20 +945,7 @@ impl Clean<Item> for hir::ImplItem<'_> {
}
};

let what_rustc_thinks =
Item::from_def_id_and_parts(local_did, Some(self.ident.name), inner, cx);
let parent_item = cx.tcx.hir().expect_item(cx.tcx.hir().get_parent_did(self.hir_id()));
if let hir::ItemKind::Impl(impl_) = &parent_item.kind {
if impl_.of_trait.is_some() {
// Trait impl items always inherit the impl's visibility --
// we don't want to show `pub`.
Item { visibility: Inherited, ..what_rustc_thinks }
} else {
what_rustc_thinks
}
} else {
panic!("found impl item with non-impl parent {:?}", parent_item);
}
Item::from_def_id_and_parts(local_did, Some(self.ident.name), inner, cx)
})
}
}
Expand Down Expand Up @@ -1570,14 +1554,7 @@ impl Clean<Item> for ty::FieldDef {
}

fn clean_field(def_id: DefId, name: Symbol, ty: Type, cx: &mut DocContext<'_>) -> Item {
let what_rustc_thinks =
Item::from_def_id_and_parts(def_id, Some(name), StructFieldItem(ty), cx);
if is_field_vis_inherited(cx.tcx, def_id) {
// Variant fields inherit their enum's visibility.
Item { visibility: Visibility::Inherited, ..what_rustc_thinks }
} else {
what_rustc_thinks
}
Item::from_def_id_and_parts(def_id, Some(name), StructFieldItem(ty), cx)
}

fn is_field_vis_inherited(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
Expand All @@ -1592,17 +1569,21 @@ fn is_field_vis_inherited(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
}
}

fn clean_vis(vis: ty::Visibility) -> Visibility {
match vis {
ty::Visibility::Public => Visibility::Public,
// NOTE: this is not quite right: `ty` uses `Invisible` to mean 'private',
// while rustdoc really does mean inherited. That means that for enum variants, such as
// `pub enum E { V }`, `V` will be marked as `Public` by `ty`, but as `Inherited` by rustdoc.
// `Item::visibility()` overrides `tcx.visibility` explicitly to make sure this distinction is captured.
ty::Visibility::Invisible => Visibility::Inherited,
ty::Visibility::Restricted(module) => Visibility::Restricted(module),
}
}

impl Clean<Visibility> for ty::Visibility {
fn clean(&self, _cx: &mut DocContext<'_>) -> Visibility {
match *self {
ty::Visibility::Public => Visibility::Public,
// NOTE: this is not quite right: `ty` uses `Invisible` to mean 'private',
// while rustdoc really does mean inherited. That means that for enum variants, such as
// `pub enum E { V }`, `V` will be marked as `Public` by `ty`, but as `Inherited` by rustdoc.
// Various parts of clean override `tcx.visibility` explicitly to make sure this distinction is captured.
ty::Visibility::Invisible => Visibility::Inherited,
ty::Visibility::Restricted(module) => Visibility::Restricted(module),
}
clean_vis(*self)
}
}

Expand Down Expand Up @@ -1635,10 +1616,7 @@ impl Clean<Item> for ty::VariantDef {
fields: self.fields.iter().map(|field| field.clean(cx)).collect(),
}),
};
let what_rustc_thinks =
Item::from_def_id_and_parts(self.def_id, Some(self.ident.name), VariantItem(kind), cx);
// don't show `pub` for variants, which always inherit visibility
Item { visibility: Inherited, ..what_rustc_thinks }
Item::from_def_id_and_parts(self.def_id, Some(self.ident.name), VariantItem(kind), cx)
}
}

Expand Down Expand Up @@ -1798,10 +1776,7 @@ impl Clean<Vec<Item>> for (&hir::Item<'_>, Option<Symbol>) {
impl Clean<Item> for hir::Variant<'_> {
fn clean(&self, cx: &mut DocContext<'_>) -> Item {
let kind = VariantItem(self.data.clean(cx));
let what_rustc_thinks =
Item::from_hir_id_and_parts(self.id, Some(self.ident.name), kind, cx);
// don't show `pub` for variants, which are always public
Item { visibility: Inherited, ..what_rustc_thinks }
Item::from_hir_id_and_parts(self.id, Some(self.ident.name), kind, cx)
}
}

Expand Down Expand Up @@ -1887,9 +1862,9 @@ fn clean_extern_crate(
name: Some(name),
attrs: box attrs.clean(cx),
def_id: crate_def_id.into(),
visibility: ty_vis.clean(cx),
kind: box ExternCrateItem { src: orig_name },
kind: box ExternCrateItem { src: orig_name, crate_stmt_id: krate.def_id },
cfg: attrs.cfg(cx.tcx, &cx.cache.hidden_cfg),
inline_stmt_id: None,
}]
}

Expand Down
57 changes: 43 additions & 14 deletions src/librustdoc/clean/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::thin_vec::ThinVec;
use rustc_hir as hir;
use rustc_hir::def::{CtorKind, DefKind, Res};
use rustc_hir::def_id::{CrateNum, DefId, DefIndex, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_hir::def_id::{CrateNum, DefId, DefIndex, LocalDefId, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_hir::lang_items::LangItem;
use rustc_hir::{BodyId, Mutability};
use rustc_index::vec::IndexVec;
Expand All @@ -32,10 +32,10 @@ use rustc_target::abi::VariantIdx;
use rustc_target::spec::abi::Abi;

use crate::clean::cfg::Cfg;
use crate::clean::external_path;
use crate::clean::inline::{self, print_inlined_const};
use crate::clean::utils::{is_literal_expr, print_const_expr, print_evaluated_const};
use crate::clean::Clean;
use crate::clean::{external_path, is_field_vis_inherited};
use crate::core::DocContext;
use crate::formats::cache::Cache;
use crate::formats::item_type::ItemType;
Expand Down Expand Up @@ -348,12 +348,12 @@ crate struct Item {
/// Optional because not every item has a name, e.g. impls.
crate name: Option<Symbol>,
crate attrs: Box<Attributes>,
crate visibility: Visibility,
/// Information about this item that is specific to what kind of item it is.
/// E.g., struct vs enum vs function.
crate kind: Box<ItemKind>,
crate def_id: ItemId,

/// If this item was inlined, the DefId of the `use` statement.
crate inline_stmt_id: Option<DefId>,
Comment on lines +355 to +356
Copy link
Member Author

@jyn514 jyn514 Dec 1, 2021

Choose a reason for hiding this comment

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

boooooo

crate cfg: Option<Arc<Cfg>>,
}

Expand Down Expand Up @@ -388,6 +388,42 @@ impl Item {
self.def_id.as_def_id().map(|did| tcx.get_attrs(did).inner_docs()).unwrap_or(false)
}

crate fn visibility(&self, tcx: TyCtxt<'_>) -> Visibility {
let mut def_id = match self.def_id {
// Anything but DefId *shouldn't* matter, but return a reasonable value anyway.
ItemId::Auto { .. } | ItemId::Blanket { .. } => return Visibility::Inherited,
ItemId::Primitive(..) => return Visibility::Public,
ItemId::DefId(did) => did,
};
match *self.kind {
// Variant fields inherit their enum's visibility.
StructFieldItem(..) if is_field_vis_inherited(tcx, def_id) => {
return Visibility::Inherited;
}
// Variants always inherit visibility
VariantItem(..) => return Visibility::Inherited,
// Return the visibility of `extern crate` statement, not the crate itself (which will always be public).
ExternCrateItem { crate_stmt_id, .. } => def_id = crate_stmt_id.to_def_id(),
// Trait items inherit the trait's visibility
AssocConstItem(..) | AssocTypeItem(..) | TyMethodItem(..) | MethodItem(..) => {
let is_trait_item = match tcx.associated_item(def_id).container {
ty::TraitContainer(_) => true,
ty::ImplContainer(impl_id) => tcx.impl_trait_ref(impl_id).is_some(),
};
if is_trait_item {
return Visibility::Inherited;
}
}
_ => {}
}
// If this item was inlined, show the visibility of the `use` statement, not the definition.
Copy link
Member

@camelid camelid Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't seem quite correct. Take this code for example:

// crate a

pub struct S {
  pub x: i32,
  y: i32,
}

// crate b

pub(crate) use a::S;

If --document-private-items is passed, then I think the documentation for b::S should look like this (although I may not be remembering the inlining rules quite right):

pub(crate) struct S {
  pub(crate) x: i32,
  y: i32,
}

But with your change, I think it will look like this:

pub(crate) struct S {
  pub(crate) x: i32,
  pub(crate) y: i32,
}

Can you test that code (or a variant of it to get the inlining to happen) before and after this change to check?

Copy link
Member

@camelid camelid Dec 1, 2021

Choose a reason for hiding this comment

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

Or perhaps this won't happen because inlining doesn't occur for struct fields? But I feel like this sort of bug could probably occur with other items.

Copy link
Member Author

@jyn514 jyn514 Dec 1, 2021

Choose a reason for hiding this comment

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

This is exactly the same behavior as before, whatever that happened to be. I just moved the logic from try_inline to visibility().

Copy link
Member Author

@jyn514 jyn514 Dec 1, 2021

Choose a reason for hiding this comment

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

also, I don't think this can happen for struct fields because you can't import them directly, only the struct itself. Enum variants can be imported, but they're special-cased earlier anyway.

Copy link
Member Author

@jyn514 jyn514 Dec 1, 2021

Choose a reason for hiding this comment

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

I couldn't get y to appear with pub(crate) use (maybe due to #91094 ?) but with pub use the docs look like this on nightly:
image
so uhh yeah this is a pre-existing bug, good catch 😅

Copy link
Member

@camelid camelid Dec 1, 2021

Choose a reason for hiding this comment

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

This is exactly the same behavior as before, whatever that happened to be. I just moved the logic from try_inline to visibility().

Ah, I see.

let def_id = match self.inline_stmt_id {
Some(inlined) => inlined,
None => def_id,
};
Comment on lines +420 to +423
Copy link
Member

@camelid camelid Dec 1, 2021

Choose a reason for hiding this comment

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

Shouldn't this check be done before the big match *self.kind { above? It seems like this does extra work for inlined items, and the code's a bit harder to understand.

Copy link
Member Author

@jyn514 jyn514 Dec 1, 2021

Choose a reason for hiding this comment

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

No, because then all the uses of def_id in the match would be wrong (e.g. associated_item and overriding the ExternCrateItem def_id).

Copy link
Member Author

@jyn514 jyn514 Dec 1, 2021

Choose a reason for hiding this comment

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

also I'm not sure what you mean by extra work - this isn't doing an early return, it's just reassigning def_id.

Copy link
Member

@camelid camelid Dec 1, 2021

Choose a reason for hiding this comment

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

What I'm suggesting is changing this to be an early return. What's the point of calculating def_id earlier if this code overrides it anyway?

super::clean_vis(tcx.visibility(def_id))
}

crate fn span(&self, tcx: TyCtxt<'_>) -> Span {
let kind = match &*self.kind {
ItemKind::StrippedItem(k) => k,
Expand Down Expand Up @@ -443,7 +479,6 @@ impl Item {
name,
kind,
box ast_attrs.clean(cx),
cx,
ast_attrs.cfg(cx.tcx, &cx.cache.hidden_cfg),
)
}
Expand All @@ -453,19 +488,11 @@ impl Item {
name: Option<Symbol>,
kind: ItemKind,
attrs: Box<Attributes>,
cx: &mut DocContext<'_>,
cfg: Option<Arc<Cfg>>,
) -> Item {
trace!("name={:?}, def_id={:?}", name, def_id);

Item {
def_id: def_id.into(),
kind: box kind,
name,
attrs,
visibility: cx.tcx.visibility(def_id).clean(cx),
cfg,
}
Item { def_id: def_id.into(), kind: box kind, name, attrs, cfg, inline_stmt_id: None }
}

/// Finds all `doc` attributes as NameValues and returns their corresponding values, joined
Expand Down Expand Up @@ -640,6 +667,8 @@ crate enum ItemKind {
ExternCrateItem {
/// The crate's name, *not* the name it's imported as.
src: Option<Symbol>,
/// The id of the `extern crate` statement, not the crate itself.
crate_stmt_id: LocalDefId,
Comment on lines +670 to +671
Copy link
Member

@camelid camelid Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why can't this use Item.inline_stmt_id? Or I guess is this not quite the same as inline_stmt_id? Still, it feels like they should be merged somehow since they're similar.

},
ImportItem(Import),
StructItem(Struct),
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/fold.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ crate trait DocFolder: Sized {
}
Variant::CLike => VariantItem(Variant::CLike),
},
ExternCrateItem { src: _ }
ExternCrateItem { .. }
| ImportItem(_)
| FunctionItem(_)
| TypedefItem(_, _)
Expand Down
4 changes: 2 additions & 2 deletions src/librustdoc/html/render/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ fn assoc_const(
w,
"{}{}const <a href=\"{}\" class=\"constant\">{}</a>: {}",
extra,
it.visibility.print_with_space(it.def_id, cx),
it.visibility(cx.tcx()).print_with_space(it.def_id, cx),
naive_assoc_href(it, link, cx),
it.name.as_ref().unwrap(),
ty.print(cx)
Expand Down Expand Up @@ -892,7 +892,7 @@ fn render_assoc_item(
}
}
};
let vis = meth.visibility.print_with_space(meth.def_id, cx).to_string();
let vis = meth.visibility(cx.tcx()).print_with_space(meth.def_id, cx).to_string();
let constness =
print_constness_with_space(&header.constness, meth.const_stability(cx.tcx()));
let asyncness = header.asyncness.print_with_space();
Expand Down
Loading