diff --git a/crates/ide_assists/src/handlers/qualify_path.rs b/crates/ide_assists/src/handlers/qualify_path.rs index e7444f7dbbb..f91770a7637 100644 --- a/crates/ide_assists/src/handlers/qualify_path.rs +++ b/crates/ide_assists/src/handlers/qualify_path.rs @@ -536,6 +536,7 @@ fn main() { } #[test] + #[ignore = "FIXME: non-trait assoc items completion is unsupported yet, see FIXME in the import_assets.rs for more details"] fn associated_struct_const_unqualified() { check_assist( qualify_path, diff --git a/crates/ide_assists/src/handlers/replace_derive_with_manual_impl.rs b/crates/ide_assists/src/handlers/replace_derive_with_manual_impl.rs index 88fe2fe9047..2608b56da6f 100644 --- a/crates/ide_assists/src/handlers/replace_derive_with_manual_impl.rs +++ b/crates/ide_assists/src/handlers/replace_derive_with_manual_impl.rs @@ -1,5 +1,5 @@ use hir::ModuleDef; -use ide_db::helpers::mod_path_to_ast; +use ide_db::helpers::{import_assets::NameToImport, mod_path_to_ast}; use ide_db::items_locator; use itertools::Itertools; use syntax::{ @@ -65,20 +65,25 @@ pub(crate) fn replace_derive_with_manual_impl( let current_module = ctx.sema.scope(annotated_name.syntax()).module()?; let current_crate = current_module.krate(); - let found_traits = - items_locator::with_exact_name(&ctx.sema, current_crate, trait_token.text().to_string()) - .into_iter() - .filter_map(|item| match ModuleDef::from(item.as_module_def_id()?) { - ModuleDef::Trait(trait_) => Some(trait_), - _ => None, - }) - .flat_map(|trait_| { - current_module - .find_use_path(ctx.sema.db, hir::ModuleDef::Trait(trait_)) - .as_ref() - .map(mod_path_to_ast) - .zip(Some(trait_)) - }); + let found_traits = items_locator::items_with_name( + &ctx.sema, + current_crate, + NameToImport::Exact(trait_token.text().to_string()), + items_locator::AssocItemSearch::Exclude, + Some(items_locator::DEFAULT_QUERY_SEARCH_LIMIT), + ) + .into_iter() + .filter_map(|item| match ModuleDef::from(item.as_module_def_id()?) { + ModuleDef::Trait(trait_) => Some(trait_), + _ => None, + }) + .flat_map(|trait_| { + current_module + .find_use_path(ctx.sema.db, hir::ModuleDef::Trait(trait_)) + .as_ref() + .map(mod_path_to_ast) + .zip(Some(trait_)) + }); let mut no_traits_found = true; for (trait_path, trait_) in found_traits.inspect(|_| no_traits_found = false) { diff --git a/crates/ide_completion/src/completions/flyimport.rs b/crates/ide_completion/src/completions/flyimport.rs index 08df2df3f68..eb2cba6319c 100644 --- a/crates/ide_completion/src/completions/flyimport.rs +++ b/crates/ide_completion/src/completions/flyimport.rs @@ -943,6 +943,38 @@ mod foo { fn main() { bar::Ass$0 +}"#, + expect![[]], + ) + } + + #[test] + fn unqualified_assoc_items_are_omitted() { + check( + r#" +mod something { + pub trait BaseTrait { + fn test_function() -> i32; + } + + pub struct Item1; + pub struct Item2; + + impl BaseTrait for Item1 { + fn test_function() -> i32 { + 1 + } + } + + impl BaseTrait for Item2 { + fn test_function() -> i32 { + 2 + } + } +} + +fn main() { + test_f$0 }"#, expect![[]], ) diff --git a/crates/ide_completion/src/lib.rs b/crates/ide_completion/src/lib.rs index 995970fcab8..87cddb98eec 100644 --- a/crates/ide_completion/src/lib.rs +++ b/crates/ide_completion/src/lib.rs @@ -14,7 +14,10 @@ mod completions; use completions::flyimport::position_for_import; use ide_db::{ base_db::FilePosition, - helpers::{import_assets::LocatedImport, insert_use::ImportScope}, + helpers::{ + import_assets::{LocatedImport, NameToImport}, + insert_use::ImportScope, + }, items_locator, RootDatabase, }; use text_edit::TextEdit; @@ -151,15 +154,20 @@ pub fn resolve_completion_edits( let current_module = ctx.sema.scope(position_for_import).module()?; let current_crate = current_module.krate(); - let (import_path, item_to_import) = - items_locator::with_exact_name(&ctx.sema, current_crate, imported_name) - .into_iter() - .filter_map(|candidate| { - current_module - .find_use_path_prefixed(db, candidate, config.insert_use.prefix_kind) - .zip(Some(candidate)) - }) - .find(|(mod_path, _)| mod_path.to_string() == full_import_path)?; + let (import_path, item_to_import) = items_locator::items_with_name( + &ctx.sema, + current_crate, + NameToImport::Exact(imported_name), + items_locator::AssocItemSearch::Include, + Some(items_locator::DEFAULT_QUERY_SEARCH_LIMIT), + ) + .into_iter() + .filter_map(|candidate| { + current_module + .find_use_path_prefixed(db, candidate, config.insert_use.prefix_kind) + .zip(Some(candidate)) + }) + .find(|(mod_path, _)| mod_path.to_string() == full_import_path)?; let import = LocatedImport::new(import_path.clone(), item_to_import, item_to_import, Some(import_path)); diff --git a/crates/ide_db/src/helpers/import_assets.rs b/crates/ide_db/src/helpers/import_assets.rs index 7c8844e959b..0da7a1a9d5a 100644 --- a/crates/ide_db/src/helpers/import_assets.rs +++ b/crates/ide_db/src/helpers/import_assets.rs @@ -61,7 +61,7 @@ pub struct FirstSegmentUnresolved { } /// A name that will be used during item lookups. -#[derive(Debug)] +#[derive(Debug, Clone)] pub enum NameToImport { /// Requires items with names that exactly match the given string, case-sensitive. Exact(String), @@ -201,129 +201,103 @@ impl ImportAssets { sema: &Semantics<RootDatabase>, prefixed: Option<PrefixKind>, ) -> Vec<LocatedImport> { - let items_with_candidate_name = match self.name_to_import() { - NameToImport::Exact(exact_name) => items_locator::with_exact_name( - sema, - self.module_with_candidate.krate(), - exact_name.clone(), - ), - // FIXME: ideally, we should avoid using `fst` for seacrhing trait imports for assoc items: - // instead, we need to look up all trait impls for a certain struct and search through them only - // see https://github.com/rust-analyzer/rust-analyzer/pull/7293#issuecomment-761585032 - // and https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0/topic/Blanket.20trait.20impls.20lookup - // for the details - NameToImport::Fuzzy(fuzzy_name) => { - let (assoc_item_search, limit) = if self.import_candidate.is_trait_candidate() { - (AssocItemSearch::AssocItemsOnly, None) - } else { - (AssocItemSearch::Include, Some(DEFAULT_QUERY_SEARCH_LIMIT)) - }; - - items_locator::with_similar_name( - sema, - self.module_with_candidate.krate(), - fuzzy_name.clone(), - assoc_item_search, - limit, - ) - } - }; + let _p = profile::span("import_assets::search_for"); let scope_definitions = self.scope_definitions(sema); - self.applicable_defs(sema.db, prefixed, items_with_candidate_name) - .into_iter() - .filter(|import| import.import_path.len() > 1) - .filter(|import| !scope_definitions.contains(&ScopeDef::from(import.item_to_import))) - .sorted_by_key(|import| import.import_path.clone()) - .collect() + let current_crate = self.module_with_candidate.krate(); + let mod_path = |item| { + get_mod_path( + sema.db, + item_for_path_search(sema.db, item)?, + &self.module_with_candidate, + prefixed, + ) + }; + + match &self.import_candidate { + ImportCandidate::Path(path_candidate) => { + path_applicable_imports(sema, current_crate, path_candidate, mod_path) + } + ImportCandidate::TraitAssocItem(trait_candidate) => { + trait_applicable_items(sema, current_crate, trait_candidate, true, mod_path) + } + ImportCandidate::TraitMethod(trait_candidate) => { + trait_applicable_items(sema, current_crate, trait_candidate, false, mod_path) + } + } + .into_iter() + .filter(|import| import.import_path.len() > 1) + .filter(|import| !scope_definitions.contains(&ScopeDef::from(import.item_to_import))) + .sorted_by_key(|import| import.import_path.clone()) + .collect() } fn scope_definitions(&self, sema: &Semantics<RootDatabase>) -> FxHashSet<ScopeDef> { + let _p = profile::span("import_assets::scope_definitions"); let mut scope_definitions = FxHashSet::default(); sema.scope(&self.candidate_node).process_all_names(&mut |_, scope_def| { scope_definitions.insert(scope_def); }); scope_definitions } - - fn name_to_import(&self) -> &NameToImport { - match &self.import_candidate { - ImportCandidate::Path(candidate) => &candidate.name, - ImportCandidate::TraitAssocItem(candidate) - | ImportCandidate::TraitMethod(candidate) => &candidate.assoc_item_name, - } - } - - fn applicable_defs( - &self, - db: &RootDatabase, - prefixed: Option<PrefixKind>, - items_with_candidate_name: FxHashSet<ItemInNs>, - ) -> FxHashSet<LocatedImport> { - let _p = profile::span("import_assets::applicable_defs"); - let current_crate = self.module_with_candidate.krate(); - - let mod_path = |item| { - get_mod_path(db, item_for_path_search(db, item)?, &self.module_with_candidate, prefixed) - }; - - match &self.import_candidate { - ImportCandidate::Path(path_candidate) => { - path_applicable_imports(db, path_candidate, mod_path, items_with_candidate_name) - } - ImportCandidate::TraitAssocItem(trait_candidate) => trait_applicable_items( - db, - current_crate, - trait_candidate, - true, - mod_path, - items_with_candidate_name, - ), - ImportCandidate::TraitMethod(trait_candidate) => trait_applicable_items( - db, - current_crate, - trait_candidate, - false, - mod_path, - items_with_candidate_name, - ), - } - } } fn path_applicable_imports( - db: &RootDatabase, + sema: &Semantics<RootDatabase>, + current_crate: Crate, path_candidate: &PathImportCandidate, mod_path: impl Fn(ItemInNs) -> Option<ModPath> + Copy, - items_with_candidate_name: FxHashSet<ItemInNs>, ) -> FxHashSet<LocatedImport> { let _p = profile::span("import_assets::path_applicable_imports"); - let (unresolved_first_segment, unresolved_qualifier) = match &path_candidate.qualifier { + match &path_candidate.qualifier { None => { - return items_with_candidate_name - .into_iter() - .filter_map(|item| { - let mut mod_path = mod_path(item)?; - if let Some(assoc_item) = item_as_assoc(db, item) { - mod_path.push_segment(assoc_item.name(db)?); - } - Some(LocatedImport::new(mod_path.clone(), item, item, Some(mod_path))) - }) - .collect(); + items_locator::items_with_name( + sema, + current_crate, + path_candidate.name.clone(), + // FIXME: we could look up assoc items by the input and propose those in completion, + // but that requries more preparation first: + // * store non-trait assoc items in import_map to fully enable this lookup + // * ensure that does not degrade the performance (bencmark it) + // * write more logic to check for corresponding trait presence requirement (we're unable to flyimport multiple item right now) + // * improve the associated completion item matching and/or scoring to ensure no noisy completions appear + // + // see also an ignored test under FIXME comment in the qualify_path.rs module + AssocItemSearch::Exclude, + Some(DEFAULT_QUERY_SEARCH_LIMIT), + ) + .into_iter() + .filter_map(|item| { + let mod_path = mod_path(item)?; + Some(LocatedImport::new(mod_path.clone(), item, item, Some(mod_path))) + }) + .collect() } - Some(first_segment_unresolved) => ( - first_segment_unresolved.fist_segment.to_string(), - path_to_string_stripping_turbo_fish(&first_segment_unresolved.full_qualifier), - ), - }; - - items_with_candidate_name - .into_iter() - .filter_map(|item| { - import_for_item(db, mod_path, &unresolved_first_segment, &unresolved_qualifier, item) - }) - .collect() + Some(first_segment_unresolved) => { + let unresolved_qualifier = + path_to_string_stripping_turbo_fish(&first_segment_unresolved.full_qualifier); + let unresolved_first_segment = first_segment_unresolved.fist_segment.text(); + items_locator::items_with_name( + sema, + current_crate, + path_candidate.name.clone(), + AssocItemSearch::Include, + Some(DEFAULT_QUERY_SEARCH_LIMIT), + ) + .into_iter() + .filter_map(|item| { + import_for_item( + sema.db, + mod_path, + unresolved_first_segment, + &unresolved_qualifier, + item, + ) + }) + .collect() + } + } } fn import_for_item( @@ -438,25 +412,32 @@ fn module_with_segment_name( } fn trait_applicable_items( - db: &RootDatabase, + sema: &Semantics<RootDatabase>, current_crate: Crate, trait_candidate: &TraitImportCandidate, trait_assoc_item: bool, mod_path: impl Fn(ItemInNs) -> Option<ModPath>, - items_with_candidate_name: FxHashSet<ItemInNs>, ) -> FxHashSet<LocatedImport> { let _p = profile::span("import_assets::trait_applicable_items"); - let mut required_assoc_items = FxHashSet::default(); - let trait_candidates = items_with_candidate_name - .into_iter() - .filter_map(|input| item_as_assoc(db, input)) - .filter_map(|assoc| { - let assoc_item_trait = assoc.containing_trait(db)?; - required_assoc_items.insert(assoc); - Some(assoc_item_trait.into()) - }) - .collect(); + let db = sema.db; + + let mut required_assoc_items = FxHashSet::default(); + let trait_candidates = items_locator::items_with_name( + sema, + current_crate, + trait_candidate.assoc_item_name.clone(), + AssocItemSearch::AssocItemsOnly, + Some(DEFAULT_QUERY_SEARCH_LIMIT), + ) + .into_iter() + .filter_map(|input| item_as_assoc(db, input)) + .filter_map(|assoc| { + let assoc_item_trait = assoc.containing_trait(db)?; + required_assoc_items.insert(assoc); + Some(assoc_item_trait.into()) + }) + .collect(); let mut located_imports = FxHashSet::default(); @@ -565,10 +546,6 @@ impl ImportCandidate { ) -> Option<Self> { path_import_candidate(sema, qualifier, NameToImport::Fuzzy(fuzzy_name)) } - - fn is_trait_candidate(&self) -> bool { - matches!(self, ImportCandidate::TraitAssocItem(_) | ImportCandidate::TraitMethod(_)) - } } fn path_import_candidate( diff --git a/crates/ide_db/src/items_locator.rs b/crates/ide_db/src/items_locator.rs index 8a7f0293537..518cddd7407 100644 --- a/crates/ide_db/src/items_locator.rs +++ b/crates/ide_db/src/items_locator.rs @@ -1,6 +1,7 @@ -//! This module contains an import search functionality that is provided to the assists module. -//! Later, this should be moved away to a separate crate that is accessible from the assists module. - +//! This module has the functionality to search the project and its dependencies for a certain item, +//! by its name and a few criteria. +//! The main reason for this module to exist is the fact that project's items and dependencies' items +//! are located in different caches, with different APIs. use either::Either; use hir::{ import_map::{self, ImportKind}, @@ -10,75 +11,112 @@ use syntax::{ast, AstNode, SyntaxKind::NAME}; use crate::{ defs::{Definition, NameClass}, + helpers::import_assets::NameToImport, symbol_index::{self, FileSymbol}, RootDatabase, }; use rustc_hash::FxHashSet; -pub(crate) const DEFAULT_QUERY_SEARCH_LIMIT: usize = 40; +/// A value to use, when uncertain which limit to pick. +pub const DEFAULT_QUERY_SEARCH_LIMIT: usize = 40; -pub fn with_exact_name( - sema: &Semantics<'_, RootDatabase>, - krate: Crate, - exact_name: String, -) -> FxHashSet<ItemInNs> { - let _p = profile::span("find_exact_imports"); - find_items( - sema, - krate, - { - let mut local_query = symbol_index::Query::new(exact_name.clone()); - local_query.exact(); - local_query.limit(DEFAULT_QUERY_SEARCH_LIMIT); - local_query - }, - import_map::Query::new(exact_name) - .limit(DEFAULT_QUERY_SEARCH_LIMIT) - .name_only() - .search_mode(import_map::SearchMode::Equals) - .case_sensitive(), - ) -} - -#[derive(Debug)] +/// Three possible ways to search for the name in associated and/or other items. +#[derive(Debug, Clone, Copy)] pub enum AssocItemSearch { + /// Search for the name in both associated and other items. Include, + /// Search for the name in other items only. Exclude, + /// Search for the name in the associated items only. AssocItemsOnly, } -pub fn with_similar_name( +/// Searches for importable items with the given name in the crate and its dependencies. +pub fn items_with_name( sema: &Semantics<'_, RootDatabase>, krate: Crate, - fuzzy_search_string: String, + name: NameToImport, assoc_item_search: AssocItemSearch, limit: Option<usize>, ) -> FxHashSet<ItemInNs> { - let _p = profile::span("find_similar_imports"); + let _p = profile::span("items_with_name").detail(|| { + format!( + "Name: {} ({:?}), crate: {:?}, limit: {:?}", + name.text(), + assoc_item_search, + krate.display_name(sema.db).map(|name| name.to_string()), + limit, + ) + }); - let mut external_query = import_map::Query::new(fuzzy_search_string.clone()) - .search_mode(import_map::SearchMode::Fuzzy) - .name_only(); + let (mut local_query, mut external_query) = match name { + NameToImport::Exact(exact_name) => { + let mut local_query = symbol_index::Query::new(exact_name.clone()); + local_query.exact(); - match assoc_item_search { - AssocItemSearch::Include => {} - AssocItemSearch::Exclude => { - external_query = external_query.exclude_import_kind(ImportKind::AssociatedItem); + let external_query = import_map::Query::new(exact_name) + .name_only() + .search_mode(import_map::SearchMode::Equals) + .case_sensitive(); + + (local_query, external_query) } - AssocItemSearch::AssocItemsOnly => { - external_query = external_query.assoc_items_only(); - } - } + NameToImport::Fuzzy(fuzzy_search_string) => { + let mut external_query = import_map::Query::new(fuzzy_search_string.clone()) + .search_mode(import_map::SearchMode::Fuzzy) + .name_only(); + match assoc_item_search { + AssocItemSearch::Include => {} + AssocItemSearch::Exclude => { + external_query = external_query.exclude_import_kind(ImportKind::AssociatedItem); + } + AssocItemSearch::AssocItemsOnly => { + external_query = external_query.assoc_items_only(); + } + } - let mut local_query = symbol_index::Query::new(fuzzy_search_string); + (symbol_index::Query::new(fuzzy_search_string), external_query) + } + }; if let Some(limit) = limit { external_query = external_query.limit(limit); local_query.limit(limit); } - find_items(sema, krate, local_query, external_query) + find_items(sema, krate, assoc_item_search, local_query, external_query) +} + +fn find_items( + sema: &Semantics<'_, RootDatabase>, + krate: Crate, + assoc_item_search: AssocItemSearch, + local_query: symbol_index::Query, + external_query: import_map::Query, +) -> FxHashSet<ItemInNs> { + let _p = profile::span("find_items"); + let db = sema.db; + + let external_importables = + krate.query_external_importables(db, external_query).map(|external_importable| { + match external_importable { + Either::Left(module_def) => ItemInNs::from(module_def), + Either::Right(macro_def) => ItemInNs::from(macro_def), + } + }); + + // Query the local crate using the symbol index. + let local_results = symbol_index::crate_symbols(db, krate.into(), local_query) .into_iter() + .filter_map(|local_candidate| get_name_definition(sema, &local_candidate)) + .filter_map(|name_definition_to_import| match name_definition_to_import { + Definition::ModuleDef(module_def) => Some(ItemInNs::from(module_def)), + Definition::Macro(macro_def) => Some(ItemInNs::from(macro_def)), + _ => None, + }); + + external_importables + .chain(local_results) .filter(move |&item| match assoc_item_search { AssocItemSearch::Include => true, AssocItemSearch::Exclude => !is_assoc_item(item, sema.db), @@ -87,47 +125,6 @@ pub fn with_similar_name( .collect() } -fn is_assoc_item(item: ItemInNs, db: &RootDatabase) -> bool { - item.as_module_def_id() - .and_then(|module_def_id| ModuleDef::from(module_def_id).as_assoc_item(db)) - .is_some() -} - -fn find_items( - sema: &Semantics<'_, RootDatabase>, - krate: Crate, - local_query: symbol_index::Query, - external_query: import_map::Query, -) -> FxHashSet<ItemInNs> { - let _p = profile::span("find_similar_imports"); - let db = sema.db; - - // Query dependencies first. - let mut candidates = krate - .query_external_importables(db, external_query) - .map(|external_importable| match external_importable { - Either::Left(module_def) => ItemInNs::from(module_def), - Either::Right(macro_def) => ItemInNs::from(macro_def), - }) - .collect::<FxHashSet<_>>(); - - // Query the local crate using the symbol index. - let local_results = symbol_index::crate_symbols(db, krate.into(), local_query); - - candidates.extend( - local_results - .into_iter() - .filter_map(|local_candidate| get_name_definition(sema, &local_candidate)) - .filter_map(|name_definition_to_import| match name_definition_to_import { - Definition::ModuleDef(module_def) => Some(ItemInNs::from(module_def)), - Definition::Macro(macro_def) => Some(ItemInNs::from(macro_def)), - _ => None, - }), - ); - - candidates -} - fn get_name_definition( sema: &Semantics<'_, RootDatabase>, import_candidate: &FileSymbol, @@ -144,3 +141,9 @@ fn get_name_definition( let name = ast::Name::cast(candidate_name_node)?; NameClass::classify(sema, &name)?.defined(sema.db) } + +fn is_assoc_item(item: ItemInNs, db: &RootDatabase) -> bool { + item.as_module_def_id() + .and_then(|module_def_id| ModuleDef::from(module_def_id).as_assoc_item(db)) + .is_some() +}