Prefer visible paths which are not doc-hidden

compiler-errors · In-line · compiler-errors · commit 1b420a554c5f · 2022-07-03T23:41:50.000-07:00
Co-authored-by: Alik Aslanyan &lt;inline0@protonmail.com&gt;
diff --git a/compiler/rustc_data_structures/src/stable_hasher.rs b/compiler/rustc_data_structures/src/stable_hasher.rs
@@ -394,9 +394,10 @@ where
     }
 }
 
-impl<A, CTX> HashStable<CTX> for SmallVec<[A; 1]>
+impl<T, CTX, const N: usize> HashStable<CTX> for SmallVec<[T; N]>
 where
-    A: HashStable<CTX>,
+    [T; N]: smallvec::Array,
+    <[T; N] as smallvec::Array>::Item: HashStable<CTX>,
 {
     #[inline]
     fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) {
diff --git a/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs b/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs
@@ -1,10 +1,11 @@
 use crate::creader::{CStore, LoadedMacro};
 use crate::foreign_modules;
 use crate::native_libs;
+use crate::rmeta::ty::util::is_doc_hidden;
 
 use rustc_ast as ast;
 use rustc_attr::Deprecation;
-use rustc_hir::def::{CtorKind, DefKind, Res};
+use rustc_hir::def::{CtorKind, DefKind};
 use rustc_hir::def_id::{CrateNum, DefId, DefIdMap, LOCAL_CRATE};
 use rustc_hir::definitions::{DefKey, DefPath, DefPathHash};
 use rustc_middle::arena::ArenaAllocatable;
@@ -367,15 +368,15 @@ pub(in crate::rmeta) fn provide(providers: &mut Providers) {
         // external item that is visible from at least one local module) to a
         // sufficiently visible parent (considering modules that re-export the
         // external item to be parents).
-        visible_parent_map: |tcx, ()| {
-            use std::collections::hash_map::Entry;
-            use std::collections::vec_deque::VecDeque;
+        // Returns a map from a sufficiently visible external item (i.e., an
+        // external item that is visible from at least one local module) to a
+        // sufficiently visible parent (considering modules that re-export the
+        // external item to be parents).
+        visible_parents_map: |tcx, ()| {
+            use rustc_data_structures::fx::FxHashSet;
+            use std::collections::VecDeque;
 
-            let mut visible_parent_map: DefIdMap<DefId> = Default::default();
-            // This is a secondary visible_parent_map, storing the DefId of parents that re-export
-            // the child as `_`. Since we prefer parents that don't do this, merge this map at the
-            // end, only if we're missing any keys from the former.
-            let mut fallback_map: DefIdMap<DefId> = Default::default();
+            let mut visible_parents_map: DefIdMap<SmallVec<[_; 4]>> = DefIdMap::default();
 
             // Issue 46112: We want the map to prefer the shortest
             // paths when reporting the path to an item. Therefore we
@@ -387,62 +388,88 @@ pub(in crate::rmeta) fn provide(providers: &mut Providers) {
             // only get paths that are locally minimal with respect to
             // whatever crate we happened to encounter first in this
             // traversal, but not globally minimal across all crates.
-            let bfs_queue = &mut VecDeque::new();
-
-            for &cnum in tcx.crates(()) {
-                // Ignore crates without a corresponding local `extern crate` item.
-                if tcx.missing_extern_crate_item(cnum) {
-                    continue;
+            let mut bfs_queue = VecDeque::default();
+
+            bfs_queue.extend(
+                tcx.crates(())
+                    .into_iter()
+                    // Ignore crates without a corresponding local `extern crate` item.
+                    .filter(|cnum| !tcx.missing_extern_crate_item(**cnum))
+                    .map(|cnum| DefId { krate: *cnum, index: super::CRATE_DEF_INDEX }),
+            );
+
+            // Iterate over graph using BFS.
+            // Filter out any non-public items.
+            while let Some(parent) = bfs_queue.pop_front() {
+                if matches!(tcx.def_kind(parent), DefKind::Mod | DefKind::Enum | DefKind::Trait) {
+                    for (child_def_id, child_name) in tcx
+                        .module_children(parent)
+                        .iter()
+                        .filter(|child| child.vis.is_public())
+                        .filter_map(|child| Some((child.res.opt_def_id()?, child.ident.name)))
+                    {
+                        visible_parents_map
+                            .entry(child_def_id)
+                            .or_insert_with(|| {
+                                // If we encounter node the first time
+                                // add it to queue for next iterations
+                                bfs_queue.push_back(child_def_id);
+                                Default::default()
+                            })
+                            .push((parent, child_name));
+                    }
                 }
-
-                bfs_queue.push_back(cnum.as_def_id());
             }
 
-            let mut add_child = |bfs_queue: &mut VecDeque<_>, child: &ModChild, parent: DefId| {
-                if !child.vis.is_public() {
-                    return;
-                }
+            // Iterate over parents vector to remove duplicate elements
+            // while preserving order
+            let mut dedup_set = FxHashSet::default();
+            for (_, parents) in &mut visible_parents_map {
+                parents.retain(|parent| dedup_set.insert(*parent));
+
+                // Reuse hashset allocation.
+                dedup_set.clear();
+            }
 
-                if let Some(def_id) = child.res.opt_def_id() {
-                    if child.ident.name == kw::Underscore {
-                        fallback_map.insert(def_id, parent);
-                        return;
+            visible_parents_map
+        },
+        best_visible_parent: |tcx, child| {
+            // Use `min_by_key` because it returns
+            // first match in case keys are equal
+            tcx.visible_parents_map(())
+                .get(&child)?
+                .into_iter()
+                .min_by_key(|(parent, child_name)| {
+                    // If this is just regular export in another module, assign it a neutral score.
+                    let mut score = 0;
+
+                    // If child and parent are local, we prefer them
+                    if child.is_local() && parent.is_local() {
+                        score += 1;
                     }
 
-                    match visible_parent_map.entry(def_id) {
-                        Entry::Occupied(mut entry) => {
-                            // If `child` is defined in crate `cnum`, ensure
-                            // that it is mapped to a parent in `cnum`.
-                            if def_id.is_local() && entry.get().is_local() {
-                                entry.insert(parent);
-                            }
-                        }
-                        Entry::Vacant(entry) => {
-                            entry.insert(parent);
-                            if matches!(
-                                child.res,
-                                Res::Def(DefKind::Mod | DefKind::Enum | DefKind::Trait, _)
-                            ) {
-                                bfs_queue.push_back(def_id);
-                            }
-                        }
+                    // Even if child and parent are local, if parent is `#[doc(hidden)]`
+                    // We reduce their score to avoid showing items not popping in documentation.
+                    if is_doc_hidden(tcx, *parent) {
+                        score -= 2;
                     }
-                }
-            };
 
-            while let Some(def) = bfs_queue.pop_front() {
-                for child in tcx.module_children(def).iter() {
-                    add_child(bfs_queue, child, def);
-                }
-            }
+                    // If parent identifier is _ we prefer it only as last resort if other items are not available
+                    if let Some(name) = tcx.opt_item_name(*parent)
+                        && name == kw::Underscore
+                    {
+                        score -= 3;
+                    }
 
-            // Fill in any missing entries with the (less preferable) path ending in `::_`.
-            // We still use this path in a diagnostic that suggests importing `::*`.
-            for (child, parent) in fallback_map {
-                visible_parent_map.entry(child).or_insert(parent);
-            }
+                    // If the name of the child inside of the parent (i.e. its re-exported name),
+                    // then also don't prefer that...
+                    if *child_name == kw::Underscore {
+                        score -= 3;
+                    }
 
-            visible_parent_map
+                    -score
+                })
+                .map(|(parent, _)| *parent)
         },
 
         dependency_formats: |tcx, ()| Lrc::new(crate::dependency_format::calculate(tcx)),
diff --git a/compiler/rustc_middle/src/middle/stability.rs b/compiler/rustc_middle/src/middle/stability.rs
@@ -369,7 +369,7 @@ impl<'tcx> TyCtxt<'tcx> {
                 let depr_attr = &depr_entry.attr;
                 if !skip || depr_attr.is_since_rustc_version {
                     // Calculating message for lint involves calling `self.def_path_str`.
-                    // Which by default to calculate visible path will invoke expensive `visible_parent_map` query.
+                    // Which by default to calculate visible path will invoke expensive `visible_parents_map` query.
                     // So we skip message calculation altogether, if lint is allowed.
                     let is_in_effect = deprecation_in_effect(depr_attr);
                     let lint = deprecation_lint(is_in_effect);
diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs
@@ -1668,10 +1668,13 @@ rustc_queries! {
         desc { "calculating the missing lang items in a crate" }
         separate_provide_extern
     }
-    query visible_parent_map(_: ()) -> DefIdMap<DefId> {
+    query visible_parents_map(_: ()) -> DefIdMap<smallvec::SmallVec<[(DefId, Symbol); 4]>> {
         storage(ArenaCacheSelector<'tcx>)
         desc { "calculating the visible parent map" }
     }
+    query best_visible_parent(def_id: DefId) -> Option<DefId> {
+        desc { |tcx| "computing the best visible parent for `{}`", tcx.def_path_str(def_id) }
+    }
     query trimmed_def_paths(_: ()) -> FxHashMap<DefId, Symbol> {
         storage(ArenaCacheSelector<'tcx>)
         desc { "calculating trimmed def paths" }
diff --git a/compiler/rustc_middle/src/ty/print/pretty.rs b/compiler/rustc_middle/src/ty/print/pretty.rs
@@ -390,8 +390,6 @@ pub trait PrettyPrinter<'tcx>:
             return Ok((self, false));
         }
 
-        let visible_parent_map = self.tcx().visible_parent_map(());
-
         let mut cur_def_key = self.tcx().def_key(def_id);
         debug!("try_print_visible_def_path: cur_def_key={:?}", cur_def_key);
 
@@ -407,7 +405,7 @@ pub trait PrettyPrinter<'tcx>:
             cur_def_key = self.tcx().def_key(parent);
         }
 
-        let Some(visible_parent) = visible_parent_map.get(&def_id).cloned() else {
+        let Some(visible_parent) = self.tcx().best_visible_parent(def_id) else {
             return Ok((self, false));
         };
 
@@ -433,7 +431,7 @@ pub trait PrettyPrinter<'tcx>:
             // `std::os::unix` reexports the contents of `std::sys::unix::ext`. `std::sys` is
             // private so the "true" path to `CommandExt` isn't accessible.
             //
-            // In this case, the `visible_parent_map` will look something like this:
+            // In this case, the `visible_parents_map` will look something like this:
             //
             // (child) -> (parent)
             // `std::sys::unix::ext::process::CommandExt` -> `std::sys::unix::ext::process`
@@ -453,7 +451,7 @@ pub trait PrettyPrinter<'tcx>:
             // do this, we compare the parent of `std::sys::unix::ext` (`std::sys::unix`) with
             // the visible parent (`std::os`). If these do not match, then we iterate over
             // the children of the visible parent (as was done when computing
-            // `visible_parent_map`), looking for the specific child we currently have and then
+            // `visible_parents_map`), looking for the specific child we currently have and then
             // have access to the re-exported name.
             DefPathData::TypeNs(ref mut name) if Some(visible_parent) != actual_parent => {
                 // Item might be re-exported several times, but filter for the one
diff --git a/compiler/rustc_typeck/src/check/method/suggest.rs b/compiler/rustc_typeck/src/check/method/suggest.rs
@@ -1642,17 +1642,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     }
 
     fn suggest_use_candidates(&self, err: &mut Diagnostic, msg: String, candidates: Vec<DefId>) {
-        let parent_map = self.tcx.visible_parent_map(());
-
         // Separate out candidates that must be imported with a glob, because they are named `_`
         // and cannot be referred with their identifier.
         let (candidates, globs): (Vec<_>, Vec<_>) = candidates.into_iter().partition(|trait_did| {
-            if let Some(parent_did) = parent_map.get(trait_did) {
+            if let Some(parent_did) = self.tcx.best_visible_parent(trait_did) {
                 // If the item is re-exported as `_`, we should suggest a glob-import instead.
-                if *parent_did != self.tcx.parent(*trait_did)
+                if parent_did != self.tcx.parent(*trait_did)
                     && self
                         .tcx
-                        .module_children(*parent_did)
+                        .module_children(parent_did)
                         .iter()
                         .filter(|child| child.res.opt_def_id() == Some(*trait_did))
                         .all(|child| child.ident.name == kw::Underscore)
@@ -1673,10 +1671,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         });
 
         let glob_path_strings = globs.iter().map(|trait_did| {
-            let parent_did = parent_map.get(trait_did).unwrap();
+            let parent_did = self.tcx.best_visible_parent(trait_did).unwrap();
             format!(
                 "use {}::*; // trait {}\n",
-                with_crate_prefix!(self.tcx.def_path_str(*parent_did)),
+                with_crate_prefix!(self.tcx.def_path_str(parent_did)),
                 self.tcx.item_name(*trait_did),
             )
         });
diff --git a/src/test/ui/proc-macro/issue-37788.rs b/src/test/ui/proc-macro/issue-37788.rs
@@ -4,6 +4,6 @@
 extern crate test_macros;
 
 fn main() {
-    // Test that constructing the `visible_parent_map` (in `cstore_impl.rs`) does not ICE.
+    // Test that constructing the `visible_parents_map` (in `cstore_impl.rs`) does not ICE.
     std::cell::Cell::new(0) //~ ERROR mismatched types
 }
diff --git a/src/test/ui/proc-macro/issue-37788.stderr b/src/test/ui/proc-macro/issue-37788.stderr
@@ -3,7 +3,7 @@ error[E0308]: mismatched types
    |
 LL | fn main() {
    |           - expected `()` because of default return type
-LL |     // Test that constructing the `visible_parent_map` (in `cstore_impl.rs`) does not ICE.
+LL |     // Test that constructing the `visible_parents_map` (in `cstore_impl.rs`) does not ICE.
 LL |     std::cell::Cell::new(0)
    |     ^^^^^^^^^^^^^^^^^^^^^^^- help: consider using a semicolon here: `;`
    |     |
diff --git a/src/test/ui/suggestions/dont-suggest-doc-hidden-variant-for-enum/dont-suggest-doc-hidden-variant-for-enum.stderr b/src/test/ui/suggestions/dont-suggest-doc-hidden-variant-for-enum/dont-suggest-doc-hidden-variant-for-enum.stderr
@@ -2,14 +2,16 @@ error[E0308]: mismatched types
   --> $DIR/dont-suggest-doc-hidden-variant-for-enum.rs:6:26
    |
 LL |     let x: Option<i32> = 1i32;
-   |            -----------   ^^^^
-   |            |             |
-   |            |             expected enum `Option`, found `i32`
-   |            |             help: try using a variant of the expected enum: `Some(1i32)`
+   |            -----------   ^^^^ expected enum `Option`, found `i32`
+   |            |
    |            expected due to this
    |
    = note: expected enum `Option<i32>`
               found type `i32`
+help: try wrapping the expression in `Some`
+   |
+LL |     let x: Option<i32> = Some(1i32);
+   |                          +++++    +
 
 error: aborting due to previous error
 

Original file line number	Diff line number	Diff line change
`@@ -394,9 +394,10 @@ where`
`394`	`394`	`}`
`395`	`395`	`}`
`396`	`396`
`397`		`-impl<A, CTX> HashStable<CTX> for SmallVec<[A; 1]>`
	`397`	`+impl<T, CTX, const N: usize> HashStable<CTX> for SmallVec<[T; N]>`
`398`	`398`	`where`
`399`		`- A: HashStable<CTX>,`
	`399`	`+ [T; N]: smallvec::Array,`
	`400`	`+ <[T; N] as smallvec::Array>::Item: HashStable<CTX>,`
`400`	`401`	`{`
`401`	`402`	`#[inline]`
`402`	`403`	`fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) {`
Original file line number	Diff line number	Diff line change
`@@ -4,6 +4,6 @@`
`4`	`4`	`extern crate test_macros;`
`5`	`5`
`6`	`6`	`fn main() {`
`7`		- // Test that constructing the `visible_parent_map` (in `cstore_impl.rs`) does not ICE.
	`7`	+ // Test that constructing the `visible_parents_map` (in `cstore_impl.rs`) does not ICE.
`8`	`8`	`std::cell::Cell::new(0) //~ ERROR mismatched types`
`9`	`9`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,7 @@ error[E0308]: mismatched types`
`3`	`3`	`\|`
`4`	`4`	`LL \| fn main() {`
`5`	`5`	\| - expected `()` because of default return type
`6`		-LL \| // Test that constructing the `visible_parent_map` (in `cstore_impl.rs`) does not ICE.
	`6`	+LL \| // Test that constructing the `visible_parents_map` (in `cstore_impl.rs`) does not ICE.
`7`	`7`	`LL \| std::cell::Cell::new(0)`
`8`	`8`	\| ^^^^^^^^^^^^^^^^^^^^^^^- help: consider using a semicolon here: `;`
`9`	`9`	`\| \|`