Auto merge of #17655 - Veykril:std-find-path, r=Veykril

More `find_path` improvements

crates/hir-def/src/find_path.rs

@@ -1,13 +1,13 @@
 //! An algorithm to find a path to refer to a certain item.
 
-use std::{cell::Cell, cmp::Ordering, iter, ops::BitOr};
+use std::{cell::Cell, cmp::Ordering, iter};
 
-use base_db::CrateId;
+use base_db::{CrateId, CrateOrigin, LangCrateOrigin};
 use hir_expand::{
     name::{AsName, Name},
     Lookup,
 };
-use intern::{sym, Symbol};
+use intern::sym;
 use rustc_hash::FxHashSet;
 
 use crate::{
@@ -52,28 +52,21 @@ pub fn find_path(
             ignore_local_imports,
             from,
             from_def_map: &from.def_map(db),
-            is_std_item: db.crate_graph()[item_module.krate()].origin.is_lang(),
             fuel: Cell::new(FIND_PATH_FUEL),
         },
         item,
         MAX_PATH_LEN,
+        db.crate_graph()[item_module.krate()].origin.is_lang(),
     )
 }
 
-#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
 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;
 const FIND_PATH_FUEL: usize = 10000;
 
@@ -107,17 +100,22 @@ struct FindPathCtx<'db> {
     ignore_local_imports: bool,
     from: ModuleId,
     from_def_map: &'db DefMap,
-    is_std_item: bool,
     fuel: Cell<usize>,
 }
 
 /// Attempts to find a path to refer to the given `item` visible from the `from` ModuleId
-fn find_path_inner(ctx: &FindPathCtx<'_>, item: ItemInNs, max_len: usize) -> Option<ModPath> {
+fn find_path_inner(
+    ctx: &FindPathCtx<'_>,
+    item: ItemInNs,
+    max_len: usize,
+    is_std_item: bool,
+) -> Option<ModPath> {
     // - if the item is a module, jump straight to module search
+    if !is_std_item {
         if let ItemInNs::Types(ModuleDefId::ModuleId(module_id)) = item {
-        let mut visited_modules = FxHashSet::default();
+            return find_path_for_module(ctx, &mut FxHashSet::default(), module_id, true, max_len)
-        return find_path_for_module(ctx, &mut visited_modules, module_id, max_len)
+                .map(|choice| choice.path);
-            .map(|(item, _)| item);
+        }
     }
 
     let may_be_in_scope = match ctx.prefix {
@@ -135,14 +133,17 @@ fn find_path_inner(ctx: &FindPathCtx<'_>, item: ItemInNs, max_len: usize) -> Opt
 
     // - if the item is in the prelude, return the name from there
     if let Some(value) = find_in_prelude(ctx.db, ctx.from_def_map, item, ctx.from) {
-        return Some(value);
+        return Some(value.path);
     }
 
     if let Some(ModuleDefId::EnumVariantId(variant)) = item.as_module_def_id() {
         // - if the item is an enum variant, refer to it via the enum
-        if let Some(mut path) =
+        if let Some(mut path) = find_path_inner(
-            find_path_inner(ctx, ItemInNs::Types(variant.lookup(ctx.db).parent.into()), max_len)
+            ctx,
-        {
+            ItemInNs::Types(variant.lookup(ctx.db).parent.into()),
+            max_len,
+            is_std_item,
+        ) {
             path.push_segment(ctx.db.enum_variant_data(variant).name.clone());
             return Some(path);
         }
@@ -151,22 +152,42 @@ fn find_path_inner(ctx: &FindPathCtx<'_>, item: ItemInNs, max_len: usize) -> Opt
         // variant somewhere
     }
 
-    let mut visited_modules = FxHashSet::default();
+    if is_std_item {
+        // The item we are searching for comes from the sysroot libraries, so skip prefer looking in
+        // the sysroot libraries directly.
+        // We do need to fallback as the item in question could be re-exported by another crate
+        // while not being a transitive dependency of the current crate.
+        if let Some(choice) = find_in_sysroot(ctx, &mut FxHashSet::default(), item, max_len) {
+            return Some(choice.path);
+        }
+    }
 
-    calculate_best_path(ctx, &mut visited_modules, item, max_len).map(|(item, _)| item)
+    let mut best_choice = None;
+    calculate_best_path(ctx, &mut FxHashSet::default(), item, max_len, &mut best_choice);
+    best_choice.map(|choice| choice.path)
 }
 
 #[tracing::instrument(skip_all)]
 fn find_path_for_module(
     ctx: &FindPathCtx<'_>,
-    visited_modules: &mut FxHashSet<ModuleId>,
+    visited_modules: &mut FxHashSet<(ItemInNs, ModuleId)>,
     module_id: ModuleId,
+    maybe_extern: bool,
     max_len: usize,
-) -> Option<(ModPath, Stability)> {
+) -> Option<Choice> {
+    if max_len == 0 {
+        // recursive base case, we can't find a path of length 0
+        return None;
+    }
     if let Some(crate_root) = module_id.as_crate_root() {
-        if crate_root == ctx.from.derive_crate_root() {
+        if !maybe_extern || crate_root == ctx.from.derive_crate_root() {
             // - if the item is the crate root, return `crate`
-            return Some((ModPath::from_segments(PathKind::Crate, None), Stable));
+            return Some(Choice {
+                path: ModPath::from_segments(PathKind::Crate, None),
+                path_text_len: 5,
+                stability: Stable,
+                prefer_due_to_prelude: false,
+            });
         }
         // - otherwise if the item is the crate root of a dependency crate, return the name from the extern prelude
 
@@ -193,7 +214,7 @@ fn find_path_for_module(
             } else {
                 PathKind::Plain
             };
-            return Some((ModPath::from_segments(kind, iter::once(name.clone())), Stable));
+            return Some(Choice::new(ctx.cfg.prefer_prelude, kind, name.clone(), Stable));
         }
     }
 
@@ -211,27 +232,39 @@ fn find_path_for_module(
         );
         if let Some(scope_name) = scope_name {
             // - if the item is already in scope, return the name under which it is
-            return Some((
+            return Some(Choice::new(
-                ModPath::from_segments(ctx.prefix.path_kind(), iter::once(scope_name)),
+                ctx.cfg.prefer_prelude,
+                ctx.prefix.path_kind(),
+                scope_name,
                 Stable,
             ));
         }
     }
 
     // - if the module can be referenced as self, super or crate, do that
-    if let Some(mod_path) = is_kw_kind_relative_to_from(ctx.from_def_map, module_id, ctx.from) {
+    if let Some(kind) = is_kw_kind_relative_to_from(ctx.from_def_map, module_id, ctx.from) {
-        if ctx.prefix != PrefixKind::ByCrate || mod_path.kind == PathKind::Crate {
+        if ctx.prefix != PrefixKind::ByCrate || kind == PathKind::Crate {
-            return Some((mod_path, Stable));
+            return Some(Choice {
+                path: ModPath::from_segments(kind, None),
+                path_text_len: path_kind_len(kind),
+                stability: Stable,
+                prefer_due_to_prelude: false,
+            });
         }
     }
 
     // - if the module is in the prelude, return it by that path
-    if let Some(mod_path) =
+    let item = ItemInNs::Types(module_id.into());
-        find_in_prelude(ctx.db, ctx.from_def_map, ItemInNs::Types(module_id.into()), ctx.from)
+    if let Some(choice) = find_in_prelude(ctx.db, ctx.from_def_map, item, ctx.from) {
-    {
+        return Some(choice);
-        return Some((mod_path, Stable));
     }
-    calculate_best_path(ctx, visited_modules, ItemInNs::Types(module_id.into()), max_len)
+    let mut best_choice = None;
+    if maybe_extern {
+        calculate_best_path(ctx, visited_modules, item, max_len, &mut best_choice);
+    } else {
+        calculate_best_path_local(ctx, visited_modules, item, max_len, &mut best_choice);
+    }
+    best_choice
 }
 
 fn find_in_scope(
@@ -256,7 +289,7 @@ fn find_in_prelude(
     local_def_map: &DefMap,
     item: ItemInNs,
     from: ModuleId,
-) -> Option<ModPath> {
+) -> Option<Choice> {
     let (prelude_module, _) = local_def_map.prelude()?;
     let prelude_def_map = prelude_module.def_map(db);
     let prelude_scope = &prelude_def_map[prelude_module.local_id].scope;
@@ -278,7 +311,7 @@ fn find_in_prelude(
         });
 
     if found_and_same_def.unwrap_or(true) {
-        Some(ModPath::from_segments(PathKind::Plain, iter::once(name.clone())))
+        Some(Choice::new(false, PathKind::Plain, name.clone(), Stable))
     } else {
         None
     }
@@ -288,7 +321,7 @@ fn is_kw_kind_relative_to_from(
     def_map: &DefMap,
     item: ModuleId,
     from: ModuleId,
-) -> Option<ModPath> {
+) -> Option<PathKind> {
     if item.krate != from.krate || item.is_within_block() || from.is_within_block() {
         return None;
     }
@@ -296,14 +329,11 @@ fn is_kw_kind_relative_to_from(
     let from = from.local_id;
     if item == from {
         // - if the item is the module we're in, use `self`
-        Some(ModPath::from_segments(PathKind::SELF, None))
+        Some(PathKind::SELF)
     } else if let Some(parent_id) = def_map[from].parent {
         if item == parent_id {
             // - if the item is the parent module, use `super` (this is not used recursively, since `super::super` is ugly)
-            Some(ModPath::from_segments(
+            Some(if parent_id == DefMap::ROOT { PathKind::Crate } else { PathKind::Super(1) })
-                if parent_id == DefMap::ROOT { PathKind::Crate } else { PathKind::Super(1) },
-                None,
-            ))
         } else {
             None
         }
@@ -315,15 +345,11 @@ fn is_kw_kind_relative_to_from(
 #[tracing::instrument(skip_all)]
 fn calculate_best_path(
     ctx: &FindPathCtx<'_>,
-    visited_modules: &mut FxHashSet<ModuleId>,
+    visited_modules: &mut FxHashSet<(ItemInNs, ModuleId)>,
     item: ItemInNs,
     max_len: usize,
-) -> Option<(ModPath, Stability)> {
+    best_choice: &mut Option<Choice>,
-    if max_len <= 1 {
+) {
-        // recursive base case, we can't find a path prefix of length 0, one segment is occupied by
-        // the item's name itself.
-        return None;
-    }
     let fuel = ctx.fuel.get();
     if fuel == 0 {
         // we ran out of fuel, so we stop searching here
@@ -332,57 +358,82 @@ fn calculate_best_path(
             item.krate(ctx.db),
             ctx.from.krate()
         );
-        return None;
+        return;
     }
     ctx.fuel.set(fuel - 1);
 
-    let mut best_path = None;
+    if item.krate(ctx.db) == Some(ctx.from.krate) {
-    let mut best_path_len = max_len;
-    let mut process = |mut path: (ModPath, Stability), name, best_path_len: &mut _| {
-        path.0.push_segment(name);
-        let new_path = match best_path.take() {
-            Some(best_path) => select_best_path(best_path, path, ctx.cfg),
-            None => path,
-        };
-        if new_path.1 == Stable {
-            *best_path_len = new_path.0.len();
-        }
-        match &mut 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),
-        }
-    };
-    let db = ctx.db;
-    if item.krate(db) == Some(ctx.from.krate) {
         // Item was defined in the same crate that wants to import it. It cannot be found in any
         // dependency in this case.
-        // FIXME: cache the `find_local_import_locations` output?
+        calculate_best_path_local(ctx, visited_modules, item, max_len, best_choice)
-        find_local_import_locations(db, item, ctx.from, ctx.from_def_map, |name, module_id| {
-            if !visited_modules.insert(module_id) {
-                return;
-            }
-            // we are looking for paths of length up to best_path_len, any longer will make it be
-            // less optimal. The -1 is due to us pushing name onto it afterwards.
-            if let Some(path) =
-                find_path_for_module(ctx, visited_modules, module_id, best_path_len - 1)
-            {
-                process(path, name.clone(), &mut best_path_len);
-            }
-        })
     } else {
         // Item was defined in some upstream crate. This means that it must be exported from one,
         // too (unless we can't name it at all). It could *also* be (re)exported by the same crate
         // that wants to import it here, but we always prefer to use the external path here.
 
-        let mut process_dep = |dep: CrateId| {
+        ctx.db.crate_graph()[ctx.from.krate].dependencies.iter().for_each(|dep| {
-            let import_map = db.import_map(dep);
+            find_in_dep(ctx, visited_modules, item, max_len, best_choice, dep.crate_id)
-            let Some(import_info_for) = import_map.import_info_for(item) else {
+        });
-                return false;
+    }
+}
+
+fn find_in_sysroot(
+    ctx: &FindPathCtx<'_>,
+    visited_modules: &mut FxHashSet<(ItemInNs, ModuleId)>,
+    item: ItemInNs,
+    max_len: usize,
+) -> Option<Choice> {
+    let crate_graph = ctx.db.crate_graph();
+    let dependencies = &crate_graph[ctx.from.krate].dependencies;
+    let mut best_choice = None;
+    let mut search = |lang, best_choice: &mut _| {
+        if let Some(dep) = dependencies.iter().filter(|it| it.is_sysroot()).find(|dep| {
+            match crate_graph[dep.crate_id].origin {
+                CrateOrigin::Lang(l) => l == lang,
+                _ => false,
+            }
+        }) {
+            find_in_dep(ctx, visited_modules, item, max_len, best_choice, dep.crate_id);
+        }
+    };
+    if ctx.cfg.prefer_no_std {
+        search(LangCrateOrigin::Core, &mut best_choice);
+        if matches!(best_choice, Some(Choice { stability: Stable, .. })) {
+            return best_choice;
+        }
+        search(LangCrateOrigin::Std, &mut best_choice);
+        if matches!(best_choice, Some(Choice { stability: Stable, .. })) {
+            return best_choice;
+        }
+    } else {
+        search(LangCrateOrigin::Std, &mut best_choice);
+        if matches!(best_choice, Some(Choice { stability: Stable, .. })) {
+            return best_choice;
+        }
+        search(LangCrateOrigin::Core, &mut best_choice);
+        if matches!(best_choice, Some(Choice { stability: Stable, .. })) {
+            return best_choice;
+        }
+    }
+    let mut best_choice = None;
+    dependencies.iter().filter(|it| it.is_sysroot()).for_each(|dep| {
+        find_in_dep(ctx, visited_modules, item, max_len, &mut best_choice, dep.crate_id);
+    });
+    best_choice
+}
+
+fn find_in_dep(
+    ctx: &FindPathCtx<'_>,
+    visited_modules: &mut FxHashSet<(ItemInNs, ModuleId)>,
+    item: ItemInNs,
+    max_len: usize,
+    best_choice: &mut Option<Choice>,
+    dep: CrateId,
+) {
+    let import_map = ctx.db.import_map(dep);
+    let Some(import_info_for) = import_map.import_info_for(item) else {
+        return;
     };
-            let mut processed_something = false;
     for info in import_info_for {
         if info.is_doc_hidden {
             // the item or import is `#[doc(hidden)]`, so skip it as it is in an external crate
@@ -391,115 +442,124 @@ 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 path =
+        let choice = find_path_for_module(
-                    find_path_for_module(ctx, visited_modules, info.container, best_path_len - 1);
+            ctx,
-                let Some((path, path_stability)) = path else {
+            visited_modules,
+            info.container,
+            true,
+            best_choice.as_ref().map_or(max_len, |it| it.path.len()) - 1,
+        );
+        let Some(mut choice) = choice else {
             continue;
         };
         cov_mark::hit!(partially_imported);
-                let path = (
+        if info.is_unstable {
-                    path,
+            choice.stability = Unstable;
-                    zip_stability(path_stability, if info.is_unstable { Unstable } else { Stable }),
+        }
+
+        Choice::try_select(best_choice, choice, ctx.cfg.prefer_prelude, info.name.clone());
+    }
+}
+
+fn calculate_best_path_local(
+    ctx: &FindPathCtx<'_>,
+    visited_modules: &mut FxHashSet<(ItemInNs, ModuleId)>,
+    item: ItemInNs,
+    max_len: usize,
+    best_choice: &mut Option<Choice>,
+) {
+    // FIXME: cache the `find_local_import_locations` output?
+    find_local_import_locations(
+        ctx.db,
+        item,
+        ctx.from,
+        ctx.from_def_map,
+        visited_modules,
+        |visited_modules, name, module_id| {
+            // we are looking for paths of length up to best_path_len, any longer will make it be
+            // less optimal. The -1 is due to us pushing name onto it afterwards.
+            if let Some(choice) = find_path_for_module(
+                ctx,
+                visited_modules,
+                module_id,
+                false,
+                best_choice.as_ref().map_or(max_len, |it| it.path.len()) - 1,
+            ) {
+                Choice::try_select(best_choice, choice, ctx.cfg.prefer_prelude, name.clone());
+            }
+        },
     );
-
-                process(path, info.name.clone(), &mut best_path_len);
-                processed_something = true;
 }
-            processed_something
+
+struct Choice {
+    path: ModPath,
+    /// The length in characters of the path
+    path_text_len: usize,
+    /// The stability of the path
+    stability: Stability,
+    /// Whether this path contains a prelude segment and preference for it has been signaled
+    prefer_due_to_prelude: bool,
+}
+
+impl Choice {
+    fn new(prefer_prelude: bool, kind: PathKind, name: Name, stability: Stability) -> Self {
+        Self {
+            path_text_len: path_kind_len(kind) + name.as_str().len(),
+            stability,
+            prefer_due_to_prelude: prefer_prelude && name == sym::prelude,
+            path: ModPath::from_segments(kind, iter::once(name)),
+        }
+    }
+
+    fn push(mut self, prefer_prelude: bool, name: Name) -> Self {
+        self.path_text_len += name.as_str().len();
+        self.prefer_due_to_prelude |= prefer_prelude && name == sym::prelude;
+        self.path.push_segment(name);
+        self
+    }
+
+    fn try_select(
+        current: &mut Option<Choice>,
+        mut other: Choice,
+        prefer_prelude: bool,
+        name: Name,
+    ) {
+        let Some(current) = current else {
+            *current = Some(other.push(prefer_prelude, name));
+            return;
         };
-
+        match other
-        let dependencies = &db.crate_graph()[ctx.from.krate].dependencies;
+            .stability
-        if ctx.is_std_item {
+            .cmp(&current.stability)
-            // The item we are searching for comes from the sysroot libraries, so skip prefer looking in
+            .then_with(|| other.prefer_due_to_prelude.cmp(&current.prefer_due_to_prelude))
-            // the sysroot libraries directly.
+            .then_with(|| (current.path.len()).cmp(&(other.path.len() + 1)))
-            // We do need to fallback as the item in question could be re-exported by another crate
-            // while not being a transitive dependency of the current crate.
-            let processed = dependencies
-                .iter()
-                .filter(|it| it.is_sysroot())
-                .map(|dep| process_dep(dep.crate_id))
-                .reduce(BitOr::bitor)
-                .unwrap_or(false);
-            if processed {
-                // Found a path in a sysroot crate, so return it.
-                return best_path;
-            }
-        }
-
-        dependencies
-            .iter()
-            .filter(|it| !ctx.is_std_item || !it.is_sysroot())
-            .for_each(|dep| _ = process_dep(dep.crate_id));
-    }
-    best_path
-}
-
-/// Select the best (most relevant) path between two paths.
-/// This accounts for stability, path length whether, std should be chosen over alloc/core paths as
-/// well as ignoring prelude like paths or not.
-fn select_best_path(
-    old_path @ (_, old_stability): (ModPath, Stability),
-    new_path @ (_, new_stability): (ModPath, Stability),
-    cfg: ImportPathConfig,
-) -> (ModPath, Stability) {
-    match (old_stability, new_stability) {
-        (Stable, Unstable) => return old_path,
-        (Unstable, Stable) => return new_path,
-        _ => {}
-    }
-    let std_crates: [Symbol; 3] = [sym::std.clone(), sym::core.clone(), sym::alloc.clone()];
-
-    let choose = |new: (ModPath, _), old: (ModPath, _)| {
-        let (new_path, _) = &new;
-        let (old_path, _) = &old;
-        let new_has_prelude = new_path.segments().iter().any(|seg| *seg == sym::prelude.clone());
-        let old_has_prelude = old_path.segments().iter().any(|seg| *seg == sym::prelude.clone());
-        match (new_has_prelude, old_has_prelude, cfg.prefer_prelude) {
-            (true, false, true) | (false, true, false) => new,
-            (true, false, false) | (false, true, true) => old,
-            // no prelude difference in the paths, so pick the shorter one
-            (true, true, _) | (false, false, _) => {
-                let new_path_is_shorter = new_path
-                    .len()
-                    .cmp(&old_path.len())
-                    .then_with(|| new_path.textual_len().cmp(&old_path.textual_len()))
-                    .is_lt();
-                if new_path_is_shorter {
-                    new
-                } else {
-                    old
-                }
-            }
-        }
-    };
-
-    match (old_path.0.segments().first(), new_path.0.segments().first()) {
-        (Some(old), Some(new))
-            if std_crates.contains(old.symbol()) && std_crates.contains(new.symbol()) =>
         {
-            let rank = match cfg.prefer_no_std {
+            Ordering::Less => return,
-                false => |name: &Name| match name {
+            Ordering::Equal => {
-                    name if *name == sym::core.clone() => 0,
+                other.path_text_len += name.as_str().len();
-                    name if *name == sym::alloc.clone() => 1,
+                if let Ordering::Less | Ordering::Equal =
-                    name if *name == sym::std.clone() => 2,
+                    current.path_text_len.cmp(&other.path_text_len)
-                    _ => unreachable!(),
+                {
-                },
+                    return;
-                true => |name: &Name| match name {
-                    name if *name == sym::core.clone() => 2,
-                    name if *name == sym::alloc.clone() => 1,
-                    name if *name == sym::std.clone() => 0,
-                    _ => unreachable!(),
-                },
-            };
-            let nrank = rank(new);
-            let orank = rank(old);
-            match nrank.cmp(&orank) {
-                Ordering::Less => old_path,
-                Ordering::Equal => choose(new_path, old_path),
-                Ordering::Greater => new_path,
                 }
             }
-        _ => choose(new_path, old_path),
+            Ordering::Greater => {
+                other.path_text_len += name.as_str().len();
+            }
+        }
+        other.path.push_segment(name);
+        *current = other;
+    }
+}
+
+fn path_kind_len(kind: PathKind) -> usize {
+    match kind {
+        PathKind::Plain => 0,
+        PathKind::Super(0) => 4,
+        PathKind::Super(s) => s as usize * 5,
+        PathKind::Crate => 5,
+        PathKind::Abs => 2,
+        PathKind::DollarCrate(_) => 0,
     }
 }
 
@@ -509,7 +569,8 @@ fn find_local_import_locations(
     item: ItemInNs,
     from: ModuleId,
     def_map: &DefMap,
-    mut cb: impl FnMut(&Name, ModuleId),
+    visited_modules: &mut FxHashSet<(ItemInNs, ModuleId)>,
+    mut cb: impl FnMut(&mut FxHashSet<(ItemInNs, ModuleId)>, &Name, ModuleId),
 ) {
     let _p = tracing::info_span!("find_local_import_locations").entered();
 
@@ -522,32 +583,29 @@ fn find_local_import_locations(
     let mut worklist = def_map[from.local_id]
         .children
         .values()
-        .map(|child| def_map.module_id(*child))
+        .map(|&child| def_map.module_id(child))
-        // FIXME: do we need to traverse out of block expressions here?
         .chain(iter::successors(from.containing_module(db), |m| m.containing_module(db)))
+        .zip(iter::repeat(false))
         .collect::<Vec<_>>();
-    let mut seen: FxHashSet<_> = FxHashSet::default();
 
     let def_map = def_map.crate_root().def_map(db);
+    let mut block_def_map;
+    let mut cursor = 0;
 
-    while let Some(module) = worklist.pop() {
+    while let Some(&mut (module, ref mut processed)) = worklist.get_mut(cursor) {
-        if !seen.insert(module) {
+        cursor += 1;
-            continue; // already processed this module
+        if !visited_modules.insert((item, module)) {
+            // already processed this module
+            continue;
         }
-        let ext_def_map;
+        *processed = true;
-        let data = if module.krate == from.krate {
+        let data = if module.block.is_some() {
-            if module.block.is_some() {
             // Re-query the block's DefMap
-                ext_def_map = module.def_map(db);
+            block_def_map = module.def_map(db);
-                &ext_def_map[module.local_id]
+            &block_def_map[module.local_id]
         } else {
             // Reuse the root DefMap
             &def_map[module.local_id]
-            }
-        } else {
-            // The crate might reexport a module defined in another crate.
-            ext_def_map = module.def_map(db);
-            &ext_def_map[module.local_id]
         };
 
         if let Some((name, vis, declared)) = data.scope.name_of(item) {
@@ -570,18 +628,30 @@ fn find_local_import_locations(
                 // the item and we're a submodule of it, so can we.
                 // Also this keeps the cached data smaller.
                 if declared || is_pub_or_explicit {
-                    cb(name, module);
+                    cb(visited_modules, name, module);
                 }
             }
         }
 
         // Descend into all modules visible from `from`.
         for (module, vis) in data.scope.modules_in_scope() {
+            if module.krate != from.krate {
+                // We don't need to look at modules from other crates as our item has to be in the
+                // current crate
+                continue;
+            }
+            if visited_modules.contains(&(item, module)) {
+                continue;
+            }
+
             if vis.is_visible_from(db, from) {
-                worklist.push(module);
+                worklist.push((module, false));
             }
         }
     }
+    worklist.into_iter().filter(|&(_, processed)| processed).for_each(|(module, _)| {
+        visited_modules.remove(&(item, module));
+    });
 }
 
 #[cfg(test)]
@@ -1549,8 +1619,6 @@ fn main() {
 
     #[test]
     fn from_inside_module() {
-        // This worked correctly, but the test suite logic was broken.
-        cov_mark::check!(submodule_in_testdb);
         check_found_path(
             r#"
 mod baz {
@@ -1575,6 +1643,35 @@ mod bar {
         )
     }
 
+    #[test]
+    fn from_inside_module2() {
+        check_found_path(
+            r#"
+mod qux {
+    pub mod baz {
+        pub struct Foo {}
+    }
+
+    mod bar {
+        fn bar() {
+            $0;
+        }
+    }
+}
+
+            "#,
+            "crate::qux::baz::Foo",
+            expect![[r#"
+                Plain  (imports ✔): super::baz::Foo
+                Plain  (imports ✖): super::baz::Foo
+                ByCrate(imports ✔): crate::qux::baz::Foo
+                ByCrate(imports ✖): crate::qux::baz::Foo
+                BySelf (imports ✔): super::baz::Foo
+                BySelf (imports ✖): super::baz::Foo
+            "#]],
+        )
+    }
+
     #[test]
     fn from_inside_module_with_inner_items() {
         check_found_path(

crates/hir-def/src/test_db.rs

@@ -148,7 +148,6 @@ impl TestDB {
             };
 
             if size != Some(new_size) {
-                cov_mark::hit!(submodule_in_testdb);
                 size = Some(new_size);
                 res = module;
             }