Prefer stable paths over unstable ones in import path calculation

2 files changed, 128 insertions, 33 deletions

diff --git a/crates/hir-def/src/find_path.rs b/crates/hir-def/src/find_path.rs
index b60e7909105..b9c5ff72792 100644
--- a/crates/hir-def/src/find_path.rs
+++ b/crates/hir-def/src/find_path.rs
@@ -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)]
@@ -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
@@ -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(
@@ -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;
     }
@@ -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));
         }
     }
 
@@ -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,
@@ -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) {
@@ -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 {
@@ -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,
@@ -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) {
@@ -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 {
@@ -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
@@ -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
@@ -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",
+        );
+    }
 }
diff --git a/crates/hir-def/src/import_map.rs b/crates/hir-def/src/import_map.rs
index 721da29ce81..eb32c76b066 100644
--- a/crates/hir-def/src/import_map.rs
+++ b/crates/hir-def/src/import_map.rs
@@ -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.
@@ -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()),
@@ -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));
                     }
                 }
 
@@ -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);
     }