From 7a7698aa1bbe479d56ed26e9d559c421882539be Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Fri, 12 Nov 2021 13:40:20 -0800 Subject: [PATCH 1/3] rustdoc: Clean `Visibility` outside of `display_macro_source` This change should make the code a bit clearer and easier to change. --- src/librustdoc/clean/inline.rs | 9 ++------- src/librustdoc/clean/mod.rs | 9 ++++++--- src/librustdoc/clean/utils.rs | 4 +--- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/librustdoc/clean/inline.rs b/src/librustdoc/clean/inline.rs index 4a8a3160379..bdc96cf3570 100644 --- a/src/librustdoc/clean/inline.rs +++ b/src/librustdoc/clean/inline.rs @@ -592,14 +592,9 @@ fn build_macro( match CStore::from_tcx(cx.tcx).load_macro_untracked(def_id, cx.sess()) { LoadedMacro::MacroDef(item_def, _) => { if let ast::ItemKind::MacroDef(ref def) = item_def.kind { + let vis = cx.tcx.visibility(import_def_id.unwrap_or(def_id)).clean(cx); clean::MacroItem(clean::Macro { - source: utils::display_macro_source( - cx, - name, - def, - def_id, - cx.tcx.visibility(import_def_id.unwrap_or(def_id)), - ), + source: utils::display_macro_source(cx, name, def, def_id, vis), }) } else { unreachable!() diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 70401065689..c527e7f9725 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -1792,9 +1792,12 @@ impl Clean> for (&hir::Item<'_>, Option) { ItemKind::Fn(ref sig, ref generics, body_id) => { clean_fn_or_proc_macro(item, sig, generics, body_id, &mut name, cx) } - ItemKind::Macro(ref macro_def) => MacroItem(Macro { - source: display_macro_source(cx, name, macro_def, def_id, item.vis), - }), + ItemKind::Macro(ref macro_def) => { + let vis = item.vis.clean(cx); + MacroItem(Macro { + source: display_macro_source(cx, name, macro_def, def_id, vis), + }) + } ItemKind::Trait(is_auto, unsafety, ref generics, bounds, item_ids) => { let items = item_ids .iter() diff --git a/src/librustdoc/clean/utils.rs b/src/librustdoc/clean/utils.rs index 2fa7efcc650..62a15694232 100644 --- a/src/librustdoc/clean/utils.rs +++ b/src/librustdoc/clean/utils.rs @@ -529,7 +529,7 @@ pub(super) fn display_macro_source( name: Symbol, def: &ast::MacroDef, def_id: DefId, - vis: impl Clean, + vis: Visibility, ) -> String { let tts: Vec<_> = def.body.inner_tokens().into_trees().collect(); // Extract the spans of all matchers. They represent the "interface" of the macro. @@ -538,8 +538,6 @@ pub(super) fn display_macro_source( if def.macro_rules { format!("macro_rules! {} {{\n{}}}", name, render_macro_arms(matchers, ";")) } else { - let vis = vis.clean(cx); - if matchers.len() <= 1 { format!( "{}macro {}{} {{\n ...\n}}", From 94ae60a2a381a143b926450ae5a8cc945c42efc0 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Fri, 12 Nov 2021 13:44:35 -0800 Subject: [PATCH 2/3] rustdoc: Remove Clean impl for `hir::Visibility` This should be the last bit of the transition to computed visibility, rather than syntactic visibility. --- src/librustdoc/clean/mod.rs | 44 +++++++++++++++---------------------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index c527e7f9725..6eaea4f3b06 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -1560,14 +1560,23 @@ impl<'tcx> Clean for ty::Const<'tcx> { impl Clean for hir::FieldDef<'_> { fn clean(&self, cx: &mut DocContext<'_>) -> Item { - let what_rustc_thinks = Item::from_hir_id_and_parts( - self.hir_id, + let def_id = cx.tcx.hir().local_def_id(self.hir_id).to_def_id(); + let what_rustc_thinks = Item::from_def_id_and_parts( + def_id, Some(self.ident.name), StructFieldItem(self.ty.clean(cx)), cx, ); - // Don't show `pub` for fields on enum variants; they are always public - Item { visibility: self.vis.clean(cx), ..what_rustc_thinks } + let parent = cx.tcx.parent(def_id).unwrap(); + match cx.tcx.def_kind(parent) { + DefKind::Struct | DefKind::Union => what_rustc_thinks, + DefKind::Variant => { + // Variant fields inherit their enum's visibility. + Item { visibility: Visibility::Inherited, ..what_rustc_thinks } + } + // FIXME: what about DefKind::Ctor? + parent_kind => panic!("unexpected parent kind: {:?}", parent_kind), + } } } @@ -1584,24 +1593,6 @@ impl Clean for ty::FieldDef { } } -impl Clean for hir::Visibility<'_> { - fn clean(&self, cx: &mut DocContext<'_>) -> Visibility { - match self.node { - hir::VisibilityKind::Public => Visibility::Public, - hir::VisibilityKind::Inherited => Visibility::Inherited, - hir::VisibilityKind::Crate(_) => { - let krate = DefId::local(CRATE_DEF_INDEX); - Visibility::Restricted(krate) - } - hir::VisibilityKind::Restricted { ref path, .. } => { - let path = path.clean(cx); - let did = register_res(cx, path.res); - Visibility::Restricted(did) - } - } - } -} - impl Clean for ty::Visibility { fn clean(&self, _cx: &mut DocContext<'_>) -> Visibility { match *self { @@ -1793,9 +1784,9 @@ impl Clean> for (&hir::Item<'_>, Option) { clean_fn_or_proc_macro(item, sig, generics, body_id, &mut name, cx) } ItemKind::Macro(ref macro_def) => { - let vis = item.vis.clean(cx); + let ty_vis = cx.tcx.visibility(def_id).clean(cx); MacroItem(Macro { - source: display_macro_source(cx, name, macro_def, def_id, vis), + source: display_macro_source(cx, name, macro_def, def_id, ty_vis), }) } ItemKind::Trait(is_auto, unsafety, ref generics, bounds, item_ids) => { @@ -1884,7 +1875,8 @@ fn clean_extern_crate( // this is the ID of the crate itself let crate_def_id = DefId { krate: cnum, index: CRATE_DEF_INDEX }; let attrs = cx.tcx.hir().attrs(krate.hir_id()); - let please_inline = cx.tcx.visibility(krate.def_id).is_public() + let ty_vis = cx.tcx.visibility(krate.def_id); + let please_inline = ty_vis.is_public() && attrs.iter().any(|a| { a.has_name(sym::doc) && match a.meta_item_list() { @@ -1916,7 +1908,7 @@ fn clean_extern_crate( name: Some(name), attrs: box attrs.clean(cx), def_id: crate_def_id.into(), - visibility: krate.vis.clean(cx), + visibility: ty_vis.clean(cx), kind: box ExternCrateItem { src: orig_name }, cfg: attrs.cfg(cx.tcx, &cx.cache.hidden_cfg), }] From 64d69977a034ca64ca7ec3eb950645e5638f4577 Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Fri, 12 Nov 2021 20:19:10 -0800 Subject: [PATCH 3/3] rustdoc: Cleanup visibility cleaning some more * Remove outdated comment * Remove duplicated code * Extract helper function --- src/librustdoc/clean/mod.rs | 83 ++++++++++++++----------------------- 1 file changed, 31 insertions(+), 52 deletions(-) diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 6eaea4f3b06..9786c2a5e3d 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -1561,35 +1561,36 @@ impl<'tcx> Clean for ty::Const<'tcx> { impl Clean for hir::FieldDef<'_> { fn clean(&self, cx: &mut DocContext<'_>) -> Item { let def_id = cx.tcx.hir().local_def_id(self.hir_id).to_def_id(); - let what_rustc_thinks = Item::from_def_id_and_parts( - def_id, - Some(self.ident.name), - StructFieldItem(self.ty.clean(cx)), - cx, - ); - let parent = cx.tcx.parent(def_id).unwrap(); - match cx.tcx.def_kind(parent) { - DefKind::Struct | DefKind::Union => what_rustc_thinks, - DefKind::Variant => { - // Variant fields inherit their enum's visibility. - Item { visibility: Visibility::Inherited, ..what_rustc_thinks } - } - // FIXME: what about DefKind::Ctor? - parent_kind => panic!("unexpected parent kind: {:?}", parent_kind), - } + clean_field(def_id, self.ident.name, self.ty.clean(cx), cx) } } impl Clean for ty::FieldDef { fn clean(&self, cx: &mut DocContext<'_>) -> Item { - let what_rustc_thinks = Item::from_def_id_and_parts( - self.did, - Some(self.ident.name), - StructFieldItem(cx.tcx.type_of(self.did).clean(cx)), - cx, - ); - // Don't show `pub` for fields on enum variants; they are always public - Item { visibility: self.vis.clean(cx), ..what_rustc_thinks } + clean_field(self.did, self.ident.name, cx.tcx.type_of(self.did).clean(cx), cx) + } +} + +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 + } +} + +fn is_field_vis_inherited(tcx: TyCtxt<'_>, def_id: DefId) -> bool { + let parent = tcx + .parent(def_id) + .expect("is_field_vis_inherited can only be called on struct or variant fields"); + match tcx.def_kind(parent) { + DefKind::Struct | DefKind::Union => false, + DefKind::Variant => true, + // FIXME: what about DefKind::Ctor? + parent_kind => panic!("unexpected parent kind: {:?}", parent_kind), } } @@ -1600,8 +1601,7 @@ impl Clean for ty::Visibility { // 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. - // This is the main reason `impl Clean for hir::Visibility` still exists; various parts of clean - // override `tcx.visibility` explicitly to make sure this distinction is captured. + // 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), } @@ -1628,39 +1628,18 @@ impl Clean for ty::VariantDef { fn clean(&self, cx: &mut DocContext<'_>) -> Item { let kind = match self.ctor_kind { CtorKind::Const => Variant::CLike, - CtorKind::Fn => Variant::Tuple( - self.fields - .iter() - .map(|field| { - let name = Some(field.ident.name); - let kind = StructFieldItem(cx.tcx.type_of(field.did).clean(cx)); - let what_rustc_thinks = - Item::from_def_id_and_parts(field.did, name, kind, cx); - // don't show `pub` for fields, which are always public - Item { visibility: Visibility::Inherited, ..what_rustc_thinks } - }) - .collect(), - ), + CtorKind::Fn => { + Variant::Tuple(self.fields.iter().map(|field| field.clean(cx)).collect()) + } CtorKind::Fictive => Variant::Struct(VariantStruct { struct_type: CtorKind::Fictive, fields_stripped: false, - fields: self - .fields - .iter() - .map(|field| { - let name = Some(field.ident.name); - let kind = StructFieldItem(cx.tcx.type_of(field.did).clean(cx)); - let what_rustc_thinks = - Item::from_def_id_and_parts(field.did, name, kind, cx); - // don't show `pub` for fields, which are always public - Item { visibility: Visibility::Inherited, ..what_rustc_thinks } - }) - .collect(), + 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 fields, which are always public + // don't show `pub` for variants, which always inherit visibility Item { visibility: Inherited, ..what_rustc_thinks } } }