Skip to content

Prefer stable paths over unstable ones in import path calculation #15611

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Prefer stable paths over unstable ones in import path calculation #15611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
103 changes: 82 additions & 21 deletions crates/hir-def/src/find_path.rs
View file
Open in desktop
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
Open in desktop
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