Skip to content

Prefer suggestion paths that are not #[doc(hidden)] #98876

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

Closed
Closed
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
5 changes: 3 additions & 2 deletions compiler/rustc_data_structures/src/stable_hasher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
137 changes: 82 additions & 55 deletions compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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)),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/middle/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
8 changes: 3 additions & 5 deletions compiler/rustc_middle/src/ty/print/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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));
};

Expand All @@ -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`
Expand All @@ -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
Expand Down
12 changes: 5 additions & 7 deletions compiler/rustc_typeck/src/check/method/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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),
)
});
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/proc-macro/issue-37788.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
2 changes: 1 addition & 1 deletion src/test/ui/proc-macro/issue-37788.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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: `;`
| |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#![crate_type = "lib"]

extern crate core;

#[doc(hidden)]
pub mod __private {
pub use core::option::Option::{self, None, Some};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// aux-build:other_crate.rs

extern crate other_crate;

fn main() {
let x: Option<i32> = 1i32; //~ ERROR 6:26: 6:30: mismatched types [E0308]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
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`
| |
| 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

For more information about this error, try `rustc --explain E0308`.