Skip to content

Commit

Permalink
Revert "Optimize find_path for sysroot library search some more"
Browse files Browse the repository at this point in the history 
This reverts commit 733cb1e.

Reverting this commit fixes the behavior of
[`rust-analyzer.imports.preferNoStd`][].

This fixes #17840.

[`rust-analyzer.imports.preferNoStd`]: https://rust-analyzer.github.io/manual.html#rust-analyzer.imports.preferNoStd
  • Loading branch information
@traviscross
traviscross committed Aug 9, 2024
1 parent 56f63df commit 5910e88
Showing 1 changed file with 98 additions and 112 deletions.
210 changes: 98 additions & 112 deletions crates/hir-def/src/find_path.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! An algorithm to find a path to refer to a certain item.
use std::{cell::Cell, cmp::Ordering, iter};
use std::{cell::Cell, cmp::Ordering, iter, ops::BitOr};

use base_db::{CrateId, CrateOrigin, LangCrateOrigin};
use hir_expand::{
Expand All @@ -13,7 +13,7 @@ use rustc_hash::FxHashSet;
use crate::{
db::DefDatabase,
item_scope::ItemInNs,
nameres::DefMap,
nameres::{DefMap, ModuleData},

Check failure on line 16 in crates/hir-def/src/find_path.rs

View workflow job for this annotation

GitHub Actions / Rust (ubuntu-latest) 

unused import: `ModuleData`
path::{ModPath, PathKind},
visibility::{Visibility, VisibilityExplicitness},
ImportPathConfig, ModuleDefId, ModuleId,
Expand Down Expand Up @@ -52,11 +52,11 @@ 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(),
)
}

Expand All @@ -67,6 +67,13 @@ enum Stability {
}
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;

Expand Down Expand Up @@ -100,22 +107,17 @@ 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,
is_std_item: bool,
) -> Option<ModPath> {
fn find_path_inner(ctx: &FindPathCtx<'_>, item: ItemInNs, max_len: usize) -> 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 {
return find_path_for_module(ctx, &mut FxHashSet::default(), module_id, true, max_len)
.map(|choice| choice.path);
}
if let ItemInNs::Types(ModuleDefId::ModuleId(module_id)) = item {
let mut visited_modules = FxHashSet::default();
return find_path_for_module(ctx, &mut visited_modules, module_id, true, max_len)
.map(|choice| choice.path);
}

let may_be_in_scope = match ctx.prefix {
Expand All @@ -138,12 +140,9 @@ fn find_path_inner(

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) = find_path_inner(
ctx,
ItemInNs::Types(variant.lookup(ctx.db).parent.into()),
max_len,
is_std_item,
) {
if let Some(mut path) =
find_path_inner(ctx, ItemInNs::Types(variant.lookup(ctx.db).parent.into()), max_len)
{
path.push_segment(ctx.db.enum_variant_data(variant).name.clone());
return Some(path);
}
Expand All @@ -152,18 +151,10 @@ fn find_path_inner(
// variant somewhere
}

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);
}
}
let mut visited_modules = FxHashSet::default();

let mut best_choice = None;
calculate_best_path(ctx, &mut FxHashSet::default(), item, max_len, &mut best_choice);
calculate_best_path(ctx, &mut visited_modules, item, max_len, &mut best_choice);
best_choice.map(|choice| choice.path)
}

Expand All @@ -187,6 +178,7 @@ fn find_path_for_module(
path_text_len: 5,
stability: Stable,
prefer_due_to_prelude: false,
sysroot_score: 0,
});
}
// - otherwise if the item is the crate root of a dependency crate, return the name from the extern prelude
Expand Down Expand Up @@ -249,6 +241,7 @@ fn find_path_for_module(
path_text_len: path_kind_len(kind),
stability: Stable,
prefer_due_to_prelude: false,
sysroot_score: 0,
});
}
}
Expand Down Expand Up @@ -367,97 +360,86 @@ fn calculate_best_path(
// dependency in this case.
calculate_best_path_local(ctx, visited_modules, item, max_len, best_choice)
} else {
let db = ctx.db;
// 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.

ctx.db.crate_graph()[ctx.from.krate].dependencies.iter().for_each(|dep| {
find_in_dep(ctx, visited_modules, item, max_len, best_choice, dep.crate_id)
});
}
}
let mut process_dep = |dep: CrateId, score| {
let import_map = db.import_map(dep);
let Some(import_info_for) = import_map.import_info_for(item) else {
return false;
};
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
continue;
}

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,
// 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 choice = find_path_for_module(
ctx,
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;
};
choice.sysroot_score = score;
cov_mark::hit!(partially_imported);
choice.stability = zip_stability(
choice.stability,
if info.is_unstable { Unstable } else { Stable },
);

Choice::try_select(best_choice, choice, ctx.cfg.prefer_prelude, info.name.clone());
processed_something = true;
}
}) {
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;
};
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
continue;
}

// 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 choice = find_path_for_module(
ctx,
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;
processed_something
};
cov_mark::hit!(partially_imported);
if info.is_unstable {
choice.stability = Unstable;

let crate_graph = db.crate_graph();
let dependencies = &crate_graph[ctx.from.krate].dependencies;
if ctx.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.
let processed = dependencies
.iter()
.filter(|it| it.is_sysroot())
.map(|dep| {
(
match crate_graph[dep.crate_id].origin {
CrateOrigin::Lang(LangCrateOrigin::Std) if ctx.cfg.prefer_no_std => 5,
CrateOrigin::Lang(LangCrateOrigin::Std) => 1,
CrateOrigin::Lang(LangCrateOrigin::Alloc) => 2,
CrateOrigin::Lang(LangCrateOrigin::ProcMacro) => 3,
CrateOrigin::Lang(LangCrateOrigin::Test) => 3,
CrateOrigin::Lang(LangCrateOrigin::Core) => 4,
CrateOrigin::Lang(LangCrateOrigin::Other) => 1,
_ => 0,
},
dep.crate_id,
)
})
.map(|(score, crate_id)| process_dep(crate_id, score))
.reduce(BitOr::bitor)
.unwrap_or(false);
if processed {
// Found a path in a sysroot crate
return;
}
}

Choice::try_select(best_choice, choice, ctx.cfg.prefer_prelude, info.name.clone());
dependencies
.iter()
.filter(|it| !ctx.is_std_item || !it.is_sysroot())
.for_each(|dep| _ = process_dep(dep.crate_id, 0));
}
}

Expand Down Expand Up @@ -499,6 +481,8 @@ struct Choice {
stability: Stability,
/// Whether this path contains a prelude segment and preference for it has been signaled
prefer_due_to_prelude: bool,
/// non-zero if this is a path in a sysroot crate, we want to choose the highest ranked std crate
sysroot_score: u8,
}

impl Choice {
Expand All @@ -507,6 +491,7 @@ impl Choice {
path_text_len: path_kind_len(kind) + name.as_str().len(),
stability,
prefer_due_to_prelude: prefer_prelude && name == sym::prelude,
sysroot_score: 0,
path: ModPath::from_segments(kind, iter::once(name)),
}
}
Expand All @@ -532,6 +517,7 @@ impl Choice {
.stability
.cmp(&current.stability)
.then_with(|| other.prefer_due_to_prelude.cmp(&current.prefer_due_to_prelude))
.then_with(|| current.sysroot_score.cmp(&other.sysroot_score))
.then_with(|| (current.path.len()).cmp(&(other.path.len() + 1)))
{
Ordering::Less => return,
Expand Down

0 comments on commit 5910e88

Please sign in to comment.