Skip to content

Commit

Permalink
Auto merge of #15611 - Veykril:stability-import, r=Veykril
Browse files Browse the repository at this point in the history 
Prefer stable paths over unstable ones in import path calculation

Fixes #15610
  • Loading branch information
@bors
bors committed Sep 14, 2023
2 parents 47c51b7 + e63e323 commit 12e28c3
Show file tree
Hide file tree
Showing 2 changed files with 128 additions and 33 deletions.
103 changes: 82 additions & 21 deletions crates/hir-def/src/find_path.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,20 @@ pub fn find_path_prefixed(
find_path_inner(db, item, from, Some(prefix_kind), prefer_no_std)
}

#[derive(Copy, Clone, Debug)]
enum Stability {
Unstable,
Stable,
}
use Stability::*;

fn zip_stability(a: Stability, b: Stability) -> Stability {
match (a, b) {
(Stable, Stable) => Stable,
_ => Unstable,
}
}

const MAX_PATH_LEN: usize = 15;

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
Expand Down Expand Up @@ -95,7 +109,8 @@ fn find_path_inner(
MAX_PATH_LEN,
prefixed,
prefer_no_std || db.crate_supports_no_std(crate_root.krate),
);
)
.map(|(item, _)| item);
}

// - if the item is already in scope, return the name under which it is
Expand Down Expand Up @@ -143,6 +158,7 @@ fn find_path_inner(
prefer_no_std || db.crate_supports_no_std(crate_root.krate),
scope_name,
)
.map(|(item, _)| item)
}

fn find_path_for_module(
Expand All @@ -155,7 +171,7 @@ fn find_path_for_module(
max_len: usize,
prefixed: Option<PrefixKind>,
prefer_no_std: bool,
) -> Option<ModPath> {
) -> Option<(ModPath, Stability)> {
if max_len == 0 {
return None;
}
Expand All @@ -165,19 +181,19 @@ fn find_path_for_module(
let scope_name = find_in_scope(db, def_map, from, ItemInNs::Types(module_id.into()));
if prefixed.is_none() {
if let Some(scope_name) = scope_name {
return Some(ModPath::from_segments(PathKind::Plain, Some(scope_name)));
return Some((ModPath::from_segments(PathKind::Plain, Some(scope_name)), Stable));
}
}

// - if the item is the crate root, return `crate`
if module_id == crate_root {
return Some(ModPath::from_segments(PathKind::Crate, None));
return Some((ModPath::from_segments(PathKind::Crate, None), Stable));
}

// - if relative paths are fine, check if we are searching for a parent
if prefixed.filter(PrefixKind::is_absolute).is_none() {
if let modpath @ Some(_) = find_self_super(def_map, module_id, from) {
return modpath;
return modpath.zip(Some(Stable));
}
}

Expand All @@ -201,14 +217,14 @@ fn find_path_for_module(
} else {
PathKind::Plain
};
return Some(ModPath::from_segments(kind, Some(name)));
return Some((ModPath::from_segments(kind, Some(name)), Stable));
}
}

if let value @ Some(_) =
find_in_prelude(db, &root_def_map, &def_map, ItemInNs::Types(module_id.into()), from)
{
return value;
return value.zip(Some(Stable));
}
calculate_best_path(
db,
Expand Down Expand Up @@ -301,11 +317,19 @@ fn calculate_best_path(
mut prefixed: Option<PrefixKind>,
prefer_no_std: bool,
scope_name: Option<Name>,
) -> Option<ModPath> {
) -> Option<(ModPath, Stability)> {
if max_len <= 1 {
return None;
}
let mut best_path = None;
let update_best_path =
|best_path: &mut Option<_>, new_path: (ModPath, Stability)| match best_path {
Some((old_path, old_stability)) => {
*old_path = new_path.0;
*old_stability = zip_stability(*old_stability, new_path.1);
}
None => *best_path = Some(new_path),
};
// Recursive case:
// - otherwise, look for modules containing (reexporting) it and import it from one of those
if item.krate(db) == Some(from.krate) {
Expand All @@ -328,14 +352,14 @@ fn calculate_best_path(
prefixed,
prefer_no_std,
) {
path.push_segment(name);
path.0.push_segment(name);

let new_path = match best_path {
let new_path = match best_path.take() {
Some(best_path) => select_best_path(best_path, path, prefer_no_std),
None => path,
};
best_path_len = new_path.len();
best_path = Some(new_path);
best_path_len = new_path.0.len();
update_best_path(&mut best_path, new_path);
}
}
} else {
Expand All @@ -354,7 +378,7 @@ fn calculate_best_path(

// Determine best path for containing module and append last segment from `info`.
// FIXME: we should guide this to look up the path locally, or from the same crate again?
let mut path = find_path_for_module(
let (mut path, path_stability) = find_path_for_module(
db,
def_map,
visited_modules,
Expand All @@ -367,16 +391,19 @@ fn calculate_best_path(
)?;
cov_mark::hit!(partially_imported);
path.push_segment(info.name.clone());
Some(path)
Some((
path,
zip_stability(path_stability, if info.is_unstable { Unstable } else { Stable }),
))
})
});

for path in extern_paths {
let new_path = match best_path {
let new_path = match best_path.take() {
Some(best_path) => select_best_path(best_path, path, prefer_no_std),
None => path,
};
best_path = Some(new_path);
update_best_path(&mut best_path, new_path);
}
}
if let Some(module) = item.module(db) {
Expand All @@ -387,15 +414,24 @@ fn calculate_best_path(
}
match prefixed.map(PrefixKind::prefix) {
Some(prefix) => best_path.or_else(|| {
scope_name.map(|scope_name| ModPath::from_segments(prefix, Some(scope_name)))
scope_name.map(|scope_name| (ModPath::from_segments(prefix, Some(scope_name)), Stable))
}),
None => best_path,
}
}

fn select_best_path(old_path: ModPath, new_path: ModPath, prefer_no_std: bool) -> ModPath {
fn select_best_path(
old_path: (ModPath, Stability),
new_path: (ModPath, Stability),
prefer_no_std: bool,
) -> (ModPath, Stability) {
match (old_path.1, new_path.1) {
(Stable, Unstable) => return old_path,
(Unstable, Stable) => return new_path,
_ => {}
}
const STD_CRATES: [Name; 3] = [known::std, known::core, known::alloc];
match (old_path.segments().first(), new_path.segments().first()) {
match (old_path.0.segments().first(), new_path.0.segments().first()) {
(Some(old), Some(new)) if STD_CRATES.contains(old) && STD_CRATES.contains(new) => {
let rank = match prefer_no_std {
false => |name: &Name| match name {
Expand All @@ -416,7 +452,7 @@ fn select_best_path(old_path: ModPath, new_path: ModPath, prefer_no_std: bool) -
match nrank.cmp(&orank) {
Ordering::Less => old_path,
Ordering::Equal => {
if new_path.len() < old_path.len() {
if new_path.0.len() < old_path.0.len() {
new_path
} else {
old_path
Expand All @@ -426,7 +462,7 @@ fn select_best_path(old_path: ModPath, new_path: ModPath, prefer_no_std: bool) -
}
}
_ => {
if new_path.len() < old_path.len() {
if new_path.0.len() < old_path.0.len() {
new_path
} else {
old_path
Expand Down Expand Up @@ -1360,4 +1396,29 @@ pub mod ops {
"std::ops::Deref",
);
}

#[test]
fn respect_unstable_modules() {
check_found_path(
r#"
//- /main.rs crate:main deps:std,core
#![no_std]
extern crate std;
$0
//- /longer.rs crate:std deps:core
pub mod error {
pub use core::error::Error;
}
//- /core.rs crate:core
pub mod error {
#![unstable(feature = "error_in_core", issue = "103765")]
pub trait Error {}
}
"#,
"std::error::Error",
"std::error::Error",
"std::error::Error",
"std::error::Error",
);
}
}
58 changes: 46 additions & 12 deletions crates/hir-def/src/import_map.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ pub struct ImportInfo {
pub is_trait_assoc_item: bool,
/// Whether this item is annotated with `#[doc(hidden)]`.
pub is_doc_hidden: bool,
/// Whether this item is annotated with `#[unstable(..)]`.
pub is_unstable: bool,
}

/// A map from publicly exported items to its name.
Expand Down Expand Up @@ -117,7 +119,6 @@ fn collect_import_map(db: &dyn DefDatabase, krate: CrateId) -> FxIndexMap<ItemIn

for (name, per_ns) in visible_items {
for (item, import) in per_ns.iter_items() {
// FIXME: Not yet used, but will be once we handle doc(hidden) import sources
let attr_id = if let Some(import) = import {
match import {
ImportOrExternCrate::ExternCrate(id) => Some(id.into()),
Expand All @@ -129,28 +130,59 @@ fn collect_import_map(db: &dyn DefDatabase, krate: CrateId) -> FxIndexMap<ItemIn
ItemInNs::Macros(id) => Some(id.into()),
}
};
let is_doc_hidden =
attr_id.map_or(false, |attr_id| db.attrs(attr_id).has_doc_hidden());
let status @ (is_doc_hidden, is_unstable) =
attr_id.map_or((false, false), |attr_id| {
let attrs = db.attrs(attr_id);
(attrs.has_doc_hidden(), attrs.is_unstable())
});

let import_info = ImportInfo {
name: name.clone(),
container: module,
is_trait_assoc_item: false,
is_doc_hidden,
is_unstable,
};

match depth_map.entry(item) {
Entry::Vacant(entry) => _ = entry.insert((depth, is_doc_hidden)),
Entry::Vacant(entry) => _ = entry.insert((depth, status)),
Entry::Occupied(mut entry) => {
let &(occ_depth, occ_is_doc_hidden) = entry.get();
// Prefer the one that is not doc(hidden),
// Otherwise, if both have the same doc(hidden)-ness and the new path is shorter, prefer that one.
let overwrite_entry = occ_is_doc_hidden && !is_doc_hidden
|| occ_is_doc_hidden == is_doc_hidden && depth < occ_depth;
if !overwrite_entry {
let &(occ_depth, (occ_is_doc_hidden, occ_is_unstable)) = entry.get();
(depth, occ_depth);
let overwrite = match (
is_doc_hidden,
occ_is_doc_hidden,
is_unstable,
occ_is_unstable,
) {
// no change of hiddeness or unstableness
(true, true, true, true)
| (true, true, false, false)
| (false, false, true, true)
| (false, false, false, false) => depth < occ_depth,

// either less hidden or less unstable, accept
(true, true, false, true)
| (false, true, true, true)
| (false, true, false, true)
| (false, true, false, false)
| (false, false, false, true) => true,
// more hidden or unstable, discard
(true, true, true, false)
| (true, false, true, true)
| (true, false, true, false)
| (true, false, false, false)
| (false, false, true, false) => false,

// exchanges doc(hidden) for unstable (and vice-versa),
(true, false, false, true) | (false, true, true, false) => {
depth < occ_depth
}
};
if !overwrite {
continue;
}
entry.insert((depth, is_doc_hidden));
entry.insert((depth, status));
}
}

Expand Down Expand Up @@ -204,11 +236,13 @@ fn collect_trait_assoc_items(
ItemInNs::Values(module_def_id)
};

let attrs = &db.attrs(item.into());
let assoc_item_info = ImportInfo {
container: trait_import_info.container,
name: assoc_item_name.clone(),
is_trait_assoc_item: true,
is_doc_hidden: db.attrs(item.into()).has_doc_hidden(),
is_doc_hidden: attrs.has_doc_hidden(),
is_unstable: attrs.is_unstable(),
};
map.insert(assoc_item, assoc_item_info);
}
Expand Down

0 comments on commit 12e28c3

Please sign in to comment.