Auto merge of #103010 - petrochenkov:effvisdoc, r=GuillaumeGomez

rustdoc: Simplify modifications of effective visibility table

It is now obvious that rustdoc only calls `set_access_level` with foreign def ids and `AccessLevel::Public`.

The second commit makes one more step and separates effective visibilities coming from rustc from similar data collected by rustdoc for extern `DefId`s.
The original table is no longer modified and now only contains local def ids as populated by rustc.

cc #102026 `@Bryanskiy`

compiler/rustc_middle/src/middle/privacy.rs

@@ -6,8 +6,7 @@ use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_macros::HashStable;
 use rustc_query_system::ich::StableHashingContext;
-use rustc_span::def_id::{DefId, LocalDefId};
-use std::hash::Hash;
+use rustc_span::def_id::LocalDefId;
 
 /// Represents the levels of effective visibility an item can have.
 ///
@@ -75,33 +74,33 @@ impl EffectiveVisibility {
 }
 
 /// Holds a map of effective visibilities for reachable HIR nodes.
-#[derive(Debug, Clone)]
-pub struct EffectiveVisibilities<Id = LocalDefId> {
-    map: FxHashMap<Id, EffectiveVisibility>,
+#[derive(Default, Clone, Debug)]
+pub struct EffectiveVisibilities {
+    map: FxHashMap<LocalDefId, EffectiveVisibility>,
 }
 
-impl<Id: Hash + Eq + Copy> EffectiveVisibilities<Id> {
-    pub fn is_public_at_level(&self, id: Id, level: Level) -> bool {
+impl EffectiveVisibilities {
+    pub fn is_public_at_level(&self, id: LocalDefId, level: Level) -> bool {
         self.effective_vis(id)
             .map_or(false, |effective_vis| effective_vis.is_public_at_level(level))
     }
 
     /// See `Level::Reachable`.
-    pub fn is_reachable(&self, id: Id) -> bool {
+    pub fn is_reachable(&self, id: LocalDefId) -> bool {
         self.is_public_at_level(id, Level::Reachable)
     }
 
     /// See `Level::Reexported`.
-    pub fn is_exported(&self, id: Id) -> bool {
+    pub fn is_exported(&self, id: LocalDefId) -> bool {
         self.is_public_at_level(id, Level::Reexported)
     }
 
     /// See `Level::Direct`.
-    pub fn is_directly_public(&self, id: Id) -> bool {
+    pub fn is_directly_public(&self, id: LocalDefId) -> bool {
         self.is_public_at_level(id, Level::Direct)
     }
 
-    pub fn public_at_level(&self, id: Id) -> Option<Level> {
+    pub fn public_at_level(&self, id: LocalDefId) -> Option<Level> {
         self.effective_vis(id).and_then(|effective_vis| {
             for level in Level::all_levels() {
                 if effective_vis.is_public_at_level(level) {
@@ -112,24 +111,17 @@ impl<Id: Hash + Eq + Copy> EffectiveVisibilities<Id> {
         })
     }
 
-    pub fn effective_vis(&self, id: Id) -> Option<&EffectiveVisibility> {
+    pub fn effective_vis(&self, id: LocalDefId) -> Option<&EffectiveVisibility> {
         self.map.get(&id)
     }
 
-    pub fn iter(&self) -> impl Iterator<Item = (&Id, &EffectiveVisibility)> {
+    pub fn iter(&self) -> impl Iterator<Item = (&LocalDefId, &EffectiveVisibility)> {
         self.map.iter()
     }
 
-    pub fn map_id<OutId: Hash + Eq + Copy>(
-        &self,
-        f: impl Fn(Id) -> OutId,
-    ) -> EffectiveVisibilities<OutId> {
-        EffectiveVisibilities { map: self.map.iter().map(|(k, v)| (f(*k), *v)).collect() }
-    }
-
     pub fn set_public_at_level(
         &mut self,
-        id: Id,
+        id: LocalDefId,
         default_vis: impl FnOnce() -> Visibility,
         level: Level,
     ) {
@@ -144,23 +136,21 @@ impl<Id: Hash + Eq + Copy> EffectiveVisibilities<Id> {
         }
         self.map.insert(id, effective_vis);
     }
-}
 
-impl<Id: Hash + Eq + Copy + Into<DefId>> EffectiveVisibilities<Id> {
     // `parent_id` is not necessarily a parent in source code tree,
     // it is the node from which the maximum effective visibility is inherited.
     pub fn update(
         &mut self,
-        id: Id,
+        id: LocalDefId,
         nominal_vis: Visibility,
         default_vis: impl FnOnce() -> Visibility,
-        parent_id: Id,
+        parent_id: LocalDefId,
         level: Level,
         tree: impl DefIdTree,
     ) -> bool {
         let mut changed = false;
         let mut current_effective_vis = self.effective_vis(id).copied().unwrap_or_else(|| {
-            if id.into().is_crate_root() {
+            if id.is_top_level_module() {
                 EffectiveVisibility::from_vis(Visibility::Public)
             } else {
                 EffectiveVisibility::from_vis(default_vis())
@@ -204,12 +194,6 @@ impl<Id: Hash + Eq + Copy + Into<DefId>> EffectiveVisibilities<Id> {
     }
 }
 
-impl<Id> Default for EffectiveVisibilities<Id> {
-    fn default() -> Self {
-        EffectiveVisibilities { map: Default::default() }
-    }
-}
-
 impl<'a> HashStable<StableHashingContext<'a>> for EffectiveVisibilities {
     fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
         let EffectiveVisibilities { ref map } = *self;

src/librustdoc/clean/blanket_impl.rs

@@ -20,7 +20,7 @@ impl<'a, 'tcx> BlanketImplFinder<'a, 'tcx> {
         trace!("get_blanket_impls({:?})", ty);
         let mut impls = Vec::new();
         for trait_def_id in cx.tcx.all_traits() {
-            if !cx.cache.effective_visibilities.is_directly_public(trait_def_id)
+            if !cx.cache.effective_visibilities.is_directly_public(cx.tcx, trait_def_id)
                 || cx.generated_synthetics.get(&(ty.0, trait_def_id)).is_some()
             {
                 continue;

src/librustdoc/clean/inline.rs

@@ -374,7 +374,7 @@ pub(crate) fn build_impl(
     if !did.is_local() {
         if let Some(traitref) = associated_trait {
             let did = traitref.def_id;
-            if !cx.cache.effective_visibilities.is_directly_public(did) {
+            if !cx.cache.effective_visibilities.is_directly_public(tcx, did) {
                 return;
             }
 
@@ -403,7 +403,7 @@ pub(crate) fn build_impl(
     // reachable in rustdoc generated documentation
     if !did.is_local() {
         if let Some(did) = for_.def_id(&cx.cache) {
-            if !cx.cache.effective_visibilities.is_directly_public(did) {
+            if !cx.cache.effective_visibilities.is_directly_public(tcx, did) {
                 return;
             }
 

src/librustdoc/clean/mod.rs

@@ -1431,7 +1431,7 @@ fn maybe_expand_private_type_alias<'tcx>(
     let Res::Def(DefKind::TyAlias, def_id) = path.res else { return None };
     // Substitute private type aliases
     let def_id = def_id.as_local()?;
-    let alias = if !cx.cache.effective_visibilities.is_exported(def_id.to_def_id()) {
+    let alias = if !cx.cache.effective_visibilities.is_exported(cx.tcx, def_id.to_def_id()) {
         &cx.tcx.hir().expect_item(def_id).kind
     } else {
         return None;

src/librustdoc/clean/utils.rs

@@ -8,7 +8,6 @@ use crate::clean::{
 };
 use crate::core::DocContext;
 use crate::formats::item_type::ItemType;
-use crate::visit_lib::LibEmbargoVisitor;
 
 use rustc_ast as ast;
 use rustc_ast::tokenstream::TokenTree;
@@ -32,7 +31,7 @@ pub(crate) fn krate(cx: &mut DocContext<'_>) -> Crate {
 
     for &cnum in cx.tcx.crates(()) {
         // Analyze doc-reachability for extern items
-        LibEmbargoVisitor::new(cx).visit_lib(cnum);
+        crate::visit_lib::lib_embargo_visit_item(cx, cnum.as_def_id());
     }
 
     // Clean the crate, translating the entire librustc_ast AST to one that is

src/librustdoc/core.rs

@@ -348,7 +348,6 @@ pub(crate) fn run_global_ctxt(
 
     let auto_traits =
         tcx.all_traits().filter(|&trait_def_id| tcx.trait_is_auto(trait_def_id)).collect();
-    let effective_visibilities = tcx.effective_visibilities(()).map_id(Into::into);
 
     let mut ctxt = DocContext {
         tcx,
@@ -361,7 +360,7 @@ pub(crate) fn run_global_ctxt(
         impl_trait_bounds: Default::default(),
         generated_synthetics: Default::default(),
         auto_traits,
-        cache: Cache::new(effective_visibilities, render_options.document_private),
+        cache: Cache::new(render_options.document_private),
         inlined: FxHashSet::default(),
         output_format,
         render_options,

src/librustdoc/formats/cache.rs

@@ -2,7 +2,6 @@ use std::mem;
 
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_hir::def_id::{CrateNum, DefId};
-use rustc_middle::middle::privacy::EffectiveVisibilities;
 use rustc_middle::ty::{self, TyCtxt};
 use rustc_span::Symbol;
 
@@ -15,6 +14,7 @@ use crate::html::format::join_with_double_colon;
 use crate::html::markdown::short_markdown_summary;
 use crate::html::render::search_index::get_function_type_for_search;
 use crate::html::render::IndexItem;
+use crate::visit_lib::RustdocEffectiveVisibilities;
 
 /// This cache is used to store information about the [`clean::Crate`] being
 /// rendered in order to provide more useful documentation. This contains
@@ -78,7 +78,7 @@ pub(crate) struct Cache {
     // Note that external items for which `doc(hidden)` applies to are shown as
     // non-reachable while local items aren't. This is because we're reusing
     // the effective visibilities from the privacy check pass.
-    pub(crate) effective_visibilities: EffectiveVisibilities<DefId>,
+    pub(crate) effective_visibilities: RustdocEffectiveVisibilities,
 
     /// The version of the crate being documented, if given from the `--crate-version` flag.
     pub(crate) crate_version: Option<String>,
@@ -132,11 +132,8 @@ struct CacheBuilder<'a, 'tcx> {
 }
 
 impl Cache {
-    pub(crate) fn new(
-        effective_visibilities: EffectiveVisibilities<DefId>,
-        document_private: bool,
-    ) -> Self {
-        Cache { effective_visibilities, document_private, ..Cache::default() }
+    pub(crate) fn new(document_private: bool) -> Self {
+        Cache { document_private, ..Cache::default() }
     }
 
     /// Populates the `Cache` with more data. The returned `Crate` will be missing some data that was
@@ -387,7 +384,7 @@ impl<'a, 'tcx> DocFolder for CacheBuilder<'a, 'tcx> {
                         || self
                             .cache
                             .effective_visibilities
-                            .is_directly_public(item.item_id.expect_def_id())
+                            .is_directly_public(self.tcx, item.item_id.expect_def_id())
                     {
                         self.cache.paths.insert(
                             item.item_id.expect_def_id(),

src/librustdoc/html/format.rs

@@ -659,7 +659,7 @@ pub(crate) fn href_with_root_path(
     }
 
     if !did.is_local()
-        && !cache.effective_visibilities.is_directly_public(did)
+        && !cache.effective_visibilities.is_directly_public(tcx, did)
         && !cache.document_private
         && !cache.primitive_locations.values().any(|&id| id == did)
     {

src/librustdoc/passes/check_doc_test_visibility.rs

@@ -56,7 +56,7 @@ impl crate::doctest::Tester for Tests {
 }
 
 pub(crate) fn should_have_doc_example(cx: &DocContext<'_>, item: &clean::Item) -> bool {
-    if !cx.cache.effective_visibilities.is_directly_public(item.item_id.expect_def_id())
+    if !cx.cache.effective_visibilities.is_directly_public(cx.tcx, item.item_id.expect_def_id())
         || matches!(
             *item.kind,
             clean::StructFieldItem(_)
@@ -130,7 +130,7 @@ pub(crate) fn look_for_tests<'tcx>(cx: &DocContext<'tcx>, dox: &str, item: &Item
             );
         }
     } else if tests.found_tests > 0
-        && !cx.cache.effective_visibilities.is_exported(item.item_id.expect_def_id())
+        && !cx.cache.effective_visibilities.is_exported(cx.tcx, item.item_id.expect_def_id())
     {
         cx.tcx.struct_span_lint_hir(
             crate::lint::PRIVATE_DOC_TESTS,

src/librustdoc/passes/strip_hidden.rs

@@ -27,6 +27,7 @@ pub(crate) fn strip_hidden(krate: clean::Crate, cx: &mut DocContext<'_>) -> clea
 
     // strip all impls referencing stripped items
     let mut stripper = ImplStripper {
+        tcx: cx.tcx,
         retained: &retained,
         cache: &cx.cache,
         is_json_output,

src/librustdoc/passes/strip_private.rs

@@ -22,6 +22,7 @@ pub(crate) fn strip_private(mut krate: clean::Crate, cx: &mut DocContext<'_>) ->
     // strip all private items
     {
         let mut stripper = Stripper {
+            tcx: cx.tcx,
             retained: &mut retained,
             effective_visibilities: &cx.cache.effective_visibilities,
             update_retained: true,
@@ -32,6 +33,7 @@ pub(crate) fn strip_private(mut krate: clean::Crate, cx: &mut DocContext<'_>) ->
 
     // strip all impls referencing private items
     let mut stripper = ImplStripper {
+        tcx: cx.tcx,
         retained: &retained,
         cache: &cx.cache,
         is_json_output,

src/librustdoc/passes/stripper.rs

@@ -1,17 +1,19 @@
 //! A collection of utility functions for the `strip_*` passes.
 use rustc_hir::def_id::DefId;
-use rustc_middle::middle::privacy::EffectiveVisibilities;
+use rustc_middle::ty::TyCtxt;
 use rustc_span::symbol::sym;
 
 use std::mem;
 
 use crate::clean::{self, Item, ItemId, ItemIdSet, NestedAttributesExt};
 use crate::fold::{strip_item, DocFolder};
 use crate::formats::cache::Cache;
+use crate::visit_lib::RustdocEffectiveVisibilities;
 
-pub(crate) struct Stripper<'a> {
+pub(crate) struct Stripper<'a, 'tcx> {
+    pub(crate) tcx: TyCtxt<'tcx>,
     pub(crate) retained: &'a mut ItemIdSet,
-    pub(crate) effective_visibilities: &'a EffectiveVisibilities<DefId>,
+    pub(crate) effective_visibilities: &'a RustdocEffectiveVisibilities,
     pub(crate) update_retained: bool,
     pub(crate) is_json_output: bool,
 }
@@ -21,18 +23,19 @@ pub(crate) struct Stripper<'a> {
 // are in the public API, which is not enough.
 #[inline]
 fn is_item_reachable(
+    tcx: TyCtxt<'_>,
     is_json_output: bool,
-    effective_visibilities: &EffectiveVisibilities<DefId>,
+    effective_visibilities: &RustdocEffectiveVisibilities,
     item_id: ItemId,
 ) -> bool {
     if is_json_output {
-        effective_visibilities.is_reachable(item_id.expect_def_id())
+        effective_visibilities.is_reachable(tcx, item_id.expect_def_id())
     } else {
-        effective_visibilities.is_exported(item_id.expect_def_id())
+        effective_visibilities.is_exported(tcx, item_id.expect_def_id())
     }
 }
 
-impl<'a> DocFolder for Stripper<'a> {
+impl<'a> DocFolder for Stripper<'a, '_> {
     fn fold_item(&mut self, i: Item) -> Option<Item> {
         match *i.kind {
             clean::StrippedItem(..) => {
@@ -66,7 +69,12 @@ impl<'a> DocFolder for Stripper<'a> {
             | clean::ForeignTypeItem => {
                 let item_id = i.item_id;
                 if item_id.is_local()
-                    && !is_item_reachable(self.is_json_output, self.effective_visibilities, item_id)
+                    && !is_item_reachable(
+                        self.tcx,
+                        self.is_json_output,
+                        self.effective_visibilities,
+                        item_id,
+                    )
                 {
                     debug!("Stripper: stripping {:?} {:?}", i.type_(), i.name);
                     return None;
@@ -146,30 +154,31 @@ impl<'a> DocFolder for Stripper<'a> {
 }
 
 /// This stripper discards all impls which reference stripped items
-pub(crate) struct ImplStripper<'a> {
+pub(crate) struct ImplStripper<'a, 'tcx> {
+    pub(crate) tcx: TyCtxt<'tcx>,
     pub(crate) retained: &'a ItemIdSet,
     pub(crate) cache: &'a Cache,
     pub(crate) is_json_output: bool,
     pub(crate) document_private: bool,
 }
 
-impl<'a> ImplStripper<'a> {
+impl<'a> ImplStripper<'a, '_> {
     #[inline]
     fn should_keep_impl(&self, item: &Item, for_def_id: DefId) -> bool {
         if !for_def_id.is_local() || self.retained.contains(&for_def_id.into()) {
             true
         } else if self.is_json_output {
             // If the "for" item is exported and the impl block isn't `#[doc(hidden)]`, then we
             // need to keep it.
-            self.cache.effective_visibilities.is_exported(for_def_id)
+            self.cache.effective_visibilities.is_exported(self.tcx, for_def_id)
                 && !item.attrs.lists(sym::doc).has_word(sym::hidden)
         } else {
             false
         }
     }
 }
 
-impl<'a> DocFolder for ImplStripper<'a> {
+impl<'a> DocFolder for ImplStripper<'a, '_> {
     fn fold_item(&mut self, i: Item) -> Option<Item> {
         if let clean::ImplItem(ref imp) = *i.kind {
             // Impl blocks can be skipped if they are: empty; not a trait impl; and have no
@@ -185,6 +194,7 @@ impl<'a> DocFolder for ImplStripper<'a> {
                         let item_id = i.item_id;
                         item_id.is_local()
                             && !is_item_reachable(
+                                self.tcx,
                                 self.is_json_output,
                                 &self.cache.effective_visibilities,
                                 item_id,