Unnamed repository; edit this file 'description' to name the repository.
Cleanup auto-import ordering
Addresses issues raised by @Veykril in #12333
| -rw-r--r-- | crates/ide-assists/src/handlers/auto_import.rs | 102 |
1 files changed, 51 insertions, 51 deletions
diff --git a/crates/ide-assists/src/handlers/auto_import.rs b/crates/ide-assists/src/handlers/auto_import.rs index 11b2c59cd7..1ec24d8fcc 100644 --- a/crates/ide-assists/src/handlers/auto_import.rs +++ b/crates/ide-assists/src/handlers/auto_import.rs @@ -111,8 +111,17 @@ pub(crate) fn auto_import(acc: &mut Assists, ctx: &AssistContext) -> Option<()> // we aren't interested in different namespaces proposed_imports.dedup_by(|a, b| a.import_path == b.import_path); + let current_node = match ctx.covering_element() { + NodeOrToken::Node(node) => Some(node), + NodeOrToken::Token(token) => token.parent(), + }; + + let current_module = + current_node.as_ref().and_then(|node| ctx.sema.scope(node)).map(|scope| scope.module()); + // prioritize more relevant imports - proposed_imports.sort_by_key(|import| Reverse(relevance_score(ctx, import))); + proposed_imports + .sort_by_key(|import| Reverse(relevance_score(ctx, import, current_module.as_ref()))); for import in proposed_imports { acc.add_group( @@ -167,7 +176,11 @@ fn group_label(import_candidate: &ImportCandidate) -> GroupLabel { /// Determine how relevant a given import is in the current context. Higher scores are more /// relevant. -fn relevance_score(ctx: &AssistContext, import: &LocatedImport) -> i32 { +fn relevance_score( + ctx: &AssistContext, + import: &LocatedImport, + current_module: Option<&Module>, +) -> i32 { let mut score = 0; let db = ctx.db(); @@ -177,25 +190,11 @@ fn relevance_score(ctx: &AssistContext, import: &LocatedImport) -> i32 { hir::ItemInNs::Macros(makro) => Some(makro.module(db)), }; - let current_node = match ctx.covering_element() { - NodeOrToken::Node(node) => Some(node), - NodeOrToken::Token(token) => token.parent(), - }; - - if let Some(module) = item_module.as_ref() { - if module.krate().is_builtin(db) { - // prefer items builtin to the language - score += 5; - } - } - - let current_scope = current_node.as_ref().and_then(|node| ctx.sema.scope(node)); - - match item_module.zip(current_scope) { - // get the distance between the modules (prefer items that are more local) - Some((item_module, current_scope)) => { - let current_module = current_scope.module(); - score -= module_distance_hueristic(¤t_module, &item_module, db) as i32; + match item_module.zip(current_module) { + // get the distance between the imported path and the current module + // (prefer items that are more local) + Some((item_module, current_module)) => { + score -= module_distance_hueristic(db, ¤t_module, &item_module) as i32; } // could not find relevant modules, so just use the length of the path as an estimate @@ -206,30 +205,31 @@ fn relevance_score(ctx: &AssistContext, import: &LocatedImport) -> i32 { } /// A heuristic that gives a higher score to modules that are more separated. -fn module_distance_hueristic(current: &Module, item: &Module, db: &dyn HirDatabase) -> usize { +fn module_distance_hueristic(db: &dyn HirDatabase, current: &Module, item: &Module) -> usize { + // get the path starting from the item to the respective crate roots let mut current_path = current.path_to_root(db); let mut item_path = item.path_to_root(db); + // we want paths going from the root to the item current_path.reverse(); item_path.reverse(); - let mut i = 0; - while i < current_path.len() && i < item_path.len() { - if current_path[i] == item_path[i] { - i += 1 - } else { - break; - } - } + // length of the common prefix of the two paths + let prefix_length = current_path.iter().zip(&item_path).take_while(|(a, b)| a == b).count(); - let distinct_distance = current_path.len() + item_path.len(); - let common_prefix = 2 * i; + // how many modules differ between the two paths (all modules, removing any duplicates) + let distinct_length = current_path.len() + item_path.len() - 2 * prefix_length; - // prefer builtin crates and items within the same crate - let crate_boundary_cost = - if item.krate().is_builtin(db) || current.krate() == item.krate() { 0 } else { 4 }; + // cost of importing from another crate + let crate_boundary_cost = if current.krate() == item.krate() { + 0 + } else if item.krate().is_builtin(db) { + 2 + } else { + 4 + }; - distinct_distance - common_prefix + crate_boundary_cost + distinct_length + crate_boundary_cost } #[cfg(test)] @@ -266,14 +266,14 @@ mod tests { #[test] fn prefer_shorter_paths() { let before = r" - //- /main.rs crate:main deps:foo,bar - HashMap$0::new(); +//- /main.rs crate:main deps:foo,bar +HashMap$0::new(); - //- /lib.rs crate:foo - pub mod collections { pub struct HashMap; } +//- /lib.rs crate:foo +pub mod collections { pub struct HashMap; } - //- /lib.rs crate:bar - pub mod collections { pub mod hash_map { pub struct HashMap; } } +//- /lib.rs crate:bar +pub mod collections { pub mod hash_map { pub struct HashMap; } } "; check_auto_import_order( @@ -285,17 +285,17 @@ mod tests { #[test] fn prefer_same_crate() { let before = r" - //- /main.rs crate:main deps:foo - HashMap$0::new(); +//- /main.rs crate:main deps:foo +HashMap$0::new(); - mod collections { - pub mod hash_map { - pub struct HashMap; - } - } +mod collections { + pub mod hash_map { + pub struct HashMap; + } +} - //- /lib.rs crate:foo - pub struct HashMap; +//- /lib.rs crate:foo +pub struct HashMap; "; check_auto_import_order( |