diff --git a/CHANGELOG.md b/CHANGELOG.md index f7cc0ee58fac..8b9bb3796928 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -185,6 +185,9 @@ execution failed][6918]. - [Performance and readability of documentation panel was improved][6893]. The documentation is now split into separate pages, which are much smaller. +- [IDE no longer inserts redundant imports when selecting options from dropdown + widgets][7028]. The code was unified with the component browser, and it now + correctly handles reexports and already existing imports. - [Fixed cursor position when ctrl-clicking the node][7014]. Sometimes ctrl-clicking to edit the node placed the mouse cursor in the wrong position in the text. This is fixed now. @@ -203,6 +206,7 @@ [6827]: https://github.com/enso-org/enso/pull/6827 [6918]: https://github.com/enso-org/enso/pull/6918 [6893]: https://github.com/enso-org/enso/pull/6893 +[7028]: https://github.com/enso-org/enso/pull/7028 [7014]: https://github.com/enso-org/enso/pull/7014 #### EnsoGL (rendering engine) diff --git a/app/gui/src/controller/graph.rs b/app/gui/src/controller/graph.rs index e08e2e80c501..3d1ed628fd3e 100644 --- a/app/gui/src/controller/graph.rs +++ b/app/gui/src/controller/graph.rs @@ -17,7 +17,10 @@ use double_representation::definition; use double_representation::definition::DefinitionProvider; use double_representation::graph::GraphInfo; use double_representation::identifier::generate_name; +use double_representation::import; use double_representation::module; +use double_representation::name::project; +use double_representation::name::QualifiedName; use double_representation::node; use double_representation::node::MainLine; use double_representation::node::NodeAst; @@ -463,6 +466,32 @@ impl EndpointInfo { +// ====================== +// === RequiredImport === +// ====================== + +/// An import that is needed for the picked suggestion. +#[derive(Debug, Clone)] +pub enum RequiredImport { + /// A specific entry needs to be imported. + Entry(Rc), + /// An entry with a specific name needs to be imported. + Name(QualifiedName), +} + +/// Whether the import is temporary or permanent. See [`Handle::add_required_imports`] +/// documentation. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ImportType { + /// The import is used for suggestion preview and can be removed after switching to the other + /// suggestion (or closing the component browser). + Temporary, + /// The import is permanent. + Permanent, +} + + + // ================== // === Controller === // ================== @@ -475,6 +504,7 @@ pub struct Handle { pub id: Rc, pub module: model::Module, pub suggestion_db: Rc, + project_name: project::QualifiedName, parser: Parser, } @@ -485,9 +515,10 @@ impl Handle { suggestion_db: Rc, parser: Parser, id: Id, + project_name: project::QualifiedName, ) -> Handle { let id = Rc::new(id); - Handle { id, module, suggestion_db, parser } + Handle { id, module, suggestion_db, parser, project_name } } /// Create a new graph controller. Given ID should uniquely identify a definition in the @@ -497,8 +528,9 @@ impl Handle { suggestion_db: Rc, parser: Parser, id: Id, + project_name: project::QualifiedName, ) -> FallibleResult { - let ret = Self::new_unchecked(module, suggestion_db, parser, id); + let ret = Self::new_unchecked(module, suggestion_db, parser, id, project_name); // Get and discard definition info, we are just making sure it can be obtained. let _ = ret.definition()?; Ok(ret) @@ -517,7 +549,13 @@ impl Handle { let module_path = model::module::Path::from_method(root_id, &method)?; let module = project.module(module_path).await?; let definition = module.lookup_method(project.qualified_name(), &method)?; - Self::new(module, project.suggestion_db(), project.parser(), definition) + Self::new( + module, + project.suggestion_db(), + project.parser(), + definition, + project.qualified_name(), + ) } /// Get the double representation description of the graph. @@ -670,24 +708,86 @@ impl Handle { /// Add a necessary unqualified import (`from module import name`) to the module, such that /// the provided fully qualified name is imported and available in the module. - pub fn add_import_if_missing( + pub fn add_import_if_missing(&self, qualified_name: QualifiedName) -> FallibleResult { + self.add_required_imports( + iter::once(RequiredImport::Name(qualified_name)), + ImportType::Permanent, + ) + } + + /// Add imports to the module, but avoid their duplication. Temporary imports added by passing + /// [`ImportType::Temporary`] can be removed by calling [`Self::clear_temporary_imports`]. + /// Temporary imports are used for suggestion preview and are removed when the previewed + /// suggesiton is switched or cancelled. + #[profile(Debug)] + pub fn add_required_imports<'a>( &self, - qualified_name: double_representation::name::QualifiedName, + import_requirements: impl Iterator, + import_type: ImportType, ) -> FallibleResult { - if let Some(module_path) = qualified_name.parent() { - let import = model::suggestion_database::entry::Import::Unqualified { - module: module_path.to_owned(), - name: qualified_name.name().into(), - }; - - let mut module = double_representation::module::Info { ast: self.module.ast() }; - let already_imported = module.iter_imports().any(|info| import.covered_by(&info)); - if !already_imported { - module.add_import(&self.parser, import.into()); - self.module.update_ast(module.ast)?; + let module_path = self.module.path(); + let project_name = self.project_name.clone_ref(); + let module_qualified_name = module_path.qualified_module_name(project_name); + let imports = import_requirements + .filter_map(|requirement| { + let defined_in = module_qualified_name.as_ref(); + let entry = match requirement { + RequiredImport::Entry(entry) => entry, + RequiredImport::Name(name) => + self.suggestion_db.lookup_by_qualified_name(&name).ok()?.1, + }; + Some(entry.required_imports(&self.suggestion_db, defined_in)) + }) + .flatten(); + let mut module = double_representation::module::Info { ast: self.module.ast() }; + for entry_import in imports { + let already_imported = + module.iter_imports().any(|existing| entry_import.covered_by(&existing)); + let import: import::Info = entry_import.into(); + let import_id = import.id(); + let already_inserted = module.contains_import(import_id); + let need_to_insert = !already_imported; + let old_import_became_permanent = + import_type == ImportType::Permanent && already_inserted; + let need_to_update_md = need_to_insert || old_import_became_permanent; + if need_to_insert { + module.add_import(&self.parser, import); + } + if need_to_update_md { + self.module.with_import_metadata( + import_id, + Box::new(|import_metadata| { + import_metadata.is_temporary = import_type == ImportType::Temporary; + }), + )?; + } + } + self.module.update_ast(module.ast) + } + + /// Remove temporary imports added by [`Self::add_required_imports`]. + pub fn clear_temporary_imports(&self) { + let mut module = double_representation::module::Info { ast: self.module.ast() }; + let import_metadata = self.module.all_import_metadata(); + let metadata_to_remove = import_metadata + .into_iter() + .filter_map(|(id, import_metadata)| { + import_metadata.is_temporary.then(|| { + if let Err(e) = module.remove_import_by_id(id) { + warn!("Failed to remove import because of: {e:?}"); + } + id + }) + }) + .collect_vec(); + if let Err(e) = self.module.update_ast(module.ast) { + warn!("Failed to update module ast when removing imports because of: {e:?}"); + } + for id in metadata_to_remove { + if let Err(e) = self.module.remove_import_metadata(id) { + warn!("Failed to remove import metadata for import id {id} because of: {e:?}"); } } - Ok(()) } /// Reorders lines so the former node is placed after the latter. Does nothing, if the latter @@ -1142,7 +1242,8 @@ pub mod tests { let module = self.module_data().plain(&parser, urm); let id = self.graph_id.clone(); let db = self.suggestion_db(); - Handle::new(module, db, parser, id).unwrap() + let project_name = self.project_name.clone_ref(); + Handle::new(module, db, parser, id, project_name).unwrap() } pub fn method(&self) -> MethodPointer { diff --git a/app/gui/src/controller/graph/executed.rs b/app/gui/src/controller/graph/executed.rs index ba49d4be70ac..8bbd2767e007 100644 --- a/app/gui/src/controller/graph/executed.rs +++ b/app/gui/src/controller/graph/executed.rs @@ -532,6 +532,11 @@ pub mod tests { impl MockData { pub fn controller(&self) -> Handle { + let db = self.graph.suggestion_db(); + self.controller_with_db(db) + } + + pub fn controller_with_db(&self, suggestion_db: Rc) -> Handle { let parser = parser::Parser::new(); let repository = Rc::new(model::undo_redo::Repository::new()); let module = self.module.plain(&parser, repository); @@ -547,7 +552,6 @@ pub mod tests { // Root ID is needed to generate module path used to get the module. model::project::test::expect_root_id(&mut project, crate::test::mock::data::ROOT_ID); // Both graph controllers need suggestion DB to provide context to their span trees. - let suggestion_db = self.graph.suggestion_db(); model::project::test::expect_suggestion_db(&mut project, suggestion_db); let project = Rc::new(project); Handle::new(project.clone_ref(), method).boxed_local().expect_ok() diff --git a/app/gui/src/controller/module.rs b/app/gui/src/controller/module.rs index dad69ddbd0ac..156d7cf31aeb 100644 --- a/app/gui/src/controller/module.rs +++ b/app/gui/src/controller/module.rs @@ -42,6 +42,7 @@ pub struct Handle { pub model: model::Module, pub language_server: Rc, pub parser: Parser, + pub project_name: project::QualifiedName, } impl Handle { @@ -51,7 +52,8 @@ impl Handle { let model = project.module(path).await?; let language_server = project.json_rpc(); let parser = project.parser(); - Ok(Handle { model, language_server, parser }) + let project_name = project.qualified_name(); + Ok(Handle { model, language_server, parser, project_name }) } /// Save the module to file. @@ -101,7 +103,13 @@ impl Handle { id: double_representation::graph::Id, suggestion_db: Rc, ) -> FallibleResult { - controller::Graph::new(self.model.clone_ref(), suggestion_db, self.parser.clone_ref(), id) + controller::Graph::new( + self.model.clone_ref(), + suggestion_db, + self.parser.clone_ref(), + id, + self.project_name.clone_ref(), + ) } /// Returns a graph controller for graph in this module's subtree identified by `id` without @@ -116,6 +124,7 @@ impl Handle { suggestion_db, self.parser.clone_ref(), id, + self.project_name.clone_ref(), ) } @@ -174,7 +183,7 @@ impl Handle { let ast = parser.parse(code.to_string(), id_map).try_into()?; let metadata = default(); let model = Rc::new(model::module::Plain::new(path, ast, metadata, repository, default())); - Ok(Handle { model, language_server, parser }) + Ok(Handle { model, language_server, parser, project_name: default() }) } #[cfg(test)] diff --git a/app/gui/src/controller/searcher.rs b/app/gui/src/controller/searcher.rs index 210cac59ca52..c9cd2d7b32b9 100644 --- a/app/gui/src/controller/searcher.rs +++ b/app/gui/src/controller/searcher.rs @@ -3,8 +3,13 @@ use crate::model::traits::*; use crate::prelude::*; +use crate::controller::graph::executed::Handle; use crate::controller::graph::FailedToCreateNode; +use crate::controller::graph::ImportType; +use crate::controller::graph::RequiredImport; use crate::controller::searcher::component::group; +use crate::model::execution_context::QualifiedMethodPointer; +use crate::model::execution_context::Visualization; use crate::model::module::NodeEditStatus; use crate::model::module::NodeMetadata; use crate::model::suggestion_database; @@ -12,7 +17,6 @@ use crate::model::suggestion_database; use breadcrumbs::Breadcrumbs; use double_representation::graph::GraphInfo; use double_representation::graph::LocationHint; -use double_representation::import; use double_representation::name::project; use double_representation::name::QualifiedName; use double_representation::name::QualifiedNameRef; @@ -36,12 +40,10 @@ pub mod breadcrumbs; pub mod component; pub mod input; -use crate::controller::graph::executed::Handle; -use crate::model::execution_context::QualifiedMethodPointer; -use crate::model::execution_context::Visualization; pub use action::Action; + // ================= // === Constants === // ================= @@ -161,21 +163,6 @@ impl Default for Actions { -// ====================== -// === RequiredImport === -// ====================== - -/// An import that is needed for the picked suggestion. -#[derive(Debug, Clone)] -pub enum RequiredImport { - /// A specific entry needs to be imported. - Entry(Rc), - /// An entry with a specific name needs to be imported. - Name(QualifiedName), -} - - - // ================ // === ThisNode === // ================ @@ -781,7 +768,7 @@ impl Searcher { let picked_suggestion_requirement = suggestion_change.and_then(|change| change.import); let all_requirements = current_input_requirements.chain(picked_suggestion_requirement.iter().cloned()); - self.add_required_imports(all_requirements, false)?; + self.graph.graph().add_required_imports(all_requirements, ImportType::Temporary)?; } self.graph.graph().set_expression_ast(self.mode.node_id(), expression)?; @@ -851,7 +838,7 @@ impl Searcher { { let data = self.data.borrow(); let requirements = data.picked_suggestions.iter().filter_map(|ps| ps.import.clone()); - self.add_required_imports(requirements, true)?; + self.graph.graph().add_required_imports(requirements, ImportType::Permanent)?; } let node_id = self.mode.node_id(); @@ -935,79 +922,15 @@ impl Searcher { data.picked_suggestions.drain_filter(|frag| !frag.is_still_unmodified(input)); } - #[profile(Debug)] - fn add_required_imports<'a>( - &self, - import_requirements: impl Iterator, - permanent: bool, - ) -> FallibleResult { - let imports = import_requirements - .filter_map(|requirement| match requirement { - RequiredImport::Entry(entry) => Some( - entry.required_imports(&self.database, self.module_qualified_name().as_ref()), - ), - RequiredImport::Name(name) => { - let (_id, entry) = self.database.lookup_by_qualified_name(&name)?; - let defined_in = self.module_qualified_name(); - Some(entry.required_imports(&self.database, defined_in.as_ref())) - } - }) - .flatten(); - let mut module = self.module(); - for entry_import in imports { - let already_imported = - module.iter_imports().any(|existing| entry_import.covered_by(&existing)); - let import: import::Info = entry_import.into(); - let import_id = import.id(); - let already_inserted = module.contains_import(import_id); - let need_to_insert = !already_imported; - let old_import_became_permanent = permanent && already_inserted; - let need_to_update_md = need_to_insert || old_import_became_permanent; - if need_to_insert { - module.add_import(self.ide.parser(), import); - } - if need_to_update_md { - self.graph.graph().module.with_import_metadata( - import_id, - Box::new(|import_metadata| { - import_metadata.is_temporary = !permanent; - }), - )?; - } - } - self.graph.graph().module.update_ast(module.ast) - } - fn clear_temporary_imports(&self) { let transaction_name = "Clearing temporary imports after closing searcher."; let _skip = self .graph .undo_redo_repository() .open_ignored_transaction_or_ignore_current(transaction_name); - let mut module = self.module(); - let import_metadata = self.graph.graph().module.all_import_metadata(); - let metadata_to_remove = import_metadata - .into_iter() - .filter_map(|(id, import_metadata)| { - import_metadata.is_temporary.then(|| { - if let Err(e) = module.remove_import_by_id(id) { - warn!("Failed to remove import because of: {e:?}"); - } - id - }) - }) - .collect_vec(); - if let Err(e) = self.graph.graph().module.update_ast(module.ast) { - warn!("Failed to update module ast when removing imports because of: {e:?}"); - } - for id in metadata_to_remove { - if let Err(e) = self.graph.graph().module.remove_import_metadata(id) { - warn!("Failed to remove import metadata for import id {id} because of: {e:?}"); - } - } + self.graph.graph().clear_temporary_imports(); } - /// Reload Action List. /// /// The current list will be set as "Loading" and Language Server will be requested for a new @@ -1167,10 +1090,6 @@ impl Searcher { self.location_to_utf16(location) } - fn module(&self) -> double_representation::module::Info { - double_representation::module::Info { ast: self.graph.graph().module.ast() } - } - fn module_qualified_name(&self) -> QualifiedName { self.graph.module_qualified_name(&*self.project) } @@ -1194,7 +1113,7 @@ fn component_list_builder_with_favorites<'a>( } else { component::builder::List::new() }; - if let Some((id, _)) = suggestion_db.lookup_by_qualified_name(local_scope_module) { + if let Ok((id, _)) = suggestion_db.lookup_by_qualified_name(local_scope_module) { builder = builder.with_local_scope_module_id(id); } builder.set_grouping_and_order_of_favorites(suggestion_db, groups); @@ -1402,6 +1321,7 @@ pub mod test { use crate::test::mock::data::MAIN_FINISH; use crate::test::mock::data::MODULE_NAME; + use crate::controller::graph::RequiredImport; use engine_protocol::language_server::types::test::value_update_with_type; use engine_protocol::language_server::SuggestionId; use enso_suggestion_database::entry::Argument; @@ -1484,12 +1404,12 @@ pub mod test { let start_of_code = enso_text::Location::default(); let end_of_code = code.location_of_text_end_utf16_code_unit(); let code_range = start_of_code..=end_of_code; - let graph = data.graph.controller(); + let database = database_setup(code_range); + let graph = data.graph.controller_with_db(database.clone_ref()); let node = &graph.graph().nodes().unwrap()[0]; let searcher_target = graph.graph().nodes().unwrap().last().unwrap().id(); let this = ThisNode::new(node.info.id(), &graph.graph()); let this = data.selected_node.and_option(this); - let database = database_setup(code_range); let mut ide = controller::ide::MockAPI::new(); let mut project = model::project::MockAPI::new(); let project_qname = project_qualified_name(); diff --git a/app/gui/src/controller/searcher/breadcrumbs.rs b/app/gui/src/controller/searcher/breadcrumbs.rs index 1c41bd3137dd..c2078f6eaae3 100644 --- a/app/gui/src/controller/searcher/breadcrumbs.rs +++ b/app/gui/src/controller/searcher/breadcrumbs.rs @@ -164,7 +164,7 @@ impl<'a> Builder<'a> { &self, name: impl Into>, ) -> Option<(component::Id, Rc)> { - self.database.lookup_by_qualified_name(name) + self.database.lookup_by_qualified_name(name).ok() } /// Collect all parent modules of the given module. diff --git a/app/gui/src/controller/searcher/component/builder.rs b/app/gui/src/controller/searcher/component/builder.rs index fc4ee8e404f5..a41fb1a4e0e8 100644 --- a/app/gui/src/controller/searcher/component/builder.rs +++ b/app/gui/src/controller/searcher/component/builder.rs @@ -285,7 +285,7 @@ impl List { db: &model::SuggestionDatabase, module: QualifiedNameRef, ) -> Option<&mut ModuleGroups> { - let (module_id, db_entry) = db.lookup_by_qualified_name(&module)?; + let (module_id, db_entry) = db.lookup_by_qualified_name(&module).ok()?; // Note: My heart is bleeding at this point, but because of lifetime checker limitations // we must do it in this suboptimal way. @@ -511,7 +511,7 @@ mod tests { let qn_of_db_entry_1 = db.lookup(1).unwrap().qualified_name(); let qn_of_db_entry_3 = db.lookup(3).unwrap().qualified_name(); let qn_not_in_db = QualifiedName::from_text("test.Test.NameNotInSuggestionDb").unwrap(); - assert_eq!(db.lookup_by_qualified_name(&qn_not_in_db), None); + assert!(db.lookup_by_qualified_name(&qn_not_in_db).is_err()); let groups = [ execution_context::ComponentGroup { project: project::QualifiedName::standard_base_library(), diff --git a/app/gui/src/controller/searcher/component/group.rs b/app/gui/src/controller/searcher/component/group.rs index e3a464dbe22f..446fb0cfa82b 100644 --- a/app/gui/src/controller/searcher/component/group.rs +++ b/app/gui/src/controller/searcher/component/group.rs @@ -176,7 +176,8 @@ impl Group { let looked_up_components = components .iter() .filter_map(|qualified_name| { - let (id, suggestion) = suggestion_db.lookup_by_qualified_name(qualified_name)?; + let (id, suggestion) = + suggestion_db.lookup_by_qualified_name(qualified_name).ok()?; Some(Component::new_from_database_entry(id, suggestion)) }) .collect_vec(); diff --git a/app/gui/src/controller/text.rs b/app/gui/src/controller/text.rs index 42071448a613..94c189aaeb25 100644 --- a/app/gui/src/controller/text.rs +++ b/app/gui/src/controller/text.rs @@ -214,6 +214,13 @@ mod test { let project = setup_mock_project(move |project| { model::project::test::expect_module(project, module_clone); model::project::test::expect_parser(project, &parser); + model::project::test::expect_qualified_name( + project, + &double_representation::name::project::QualifiedName::new( + "mock_namespace", + "MockProject", + ), + ); }); let file_path = module.path().file_path(); let text_ctrl = controller::Text::new(&project, file_path.clone()).await.unwrap(); diff --git a/app/gui/src/test.rs b/app/gui/src/test.rs index fbc28ca757cd..f51cad2c5d95 100644 --- a/app/gui/src/test.rs +++ b/app/gui/src/test.rs @@ -223,7 +223,8 @@ pub mod mock { let method = self.method_pointer(); let definition = module.lookup_method(self.project_name.clone(), &method).expect("Lookup failed."); - controller::Graph::new(module, db, parser, definition) + let project_name = self.project_name.clone_ref(); + controller::Graph::new(module, db, parser, definition, project_name) .expect("Graph could not be created") } @@ -423,6 +424,7 @@ pub mod mock { language_server: self.project.json_rpc(), model: model.clone(), parser: self.data.parser.clone(), + project_name: self.project.qualified_name(), }; (model, controller) } diff --git a/app/gui/suggestion-database/src/documentation_ir.rs b/app/gui/suggestion-database/src/documentation_ir.rs index 9a92d3d8559a..cdeacd75b937 100644 --- a/app/gui/suggestion-database/src/documentation_ir.rs +++ b/app/gui/suggestion-database/src/documentation_ir.rs @@ -205,13 +205,13 @@ impl EntryDocumentation { let defined_in = &entry.defined_in; let parent_module = db.lookup_by_qualified_name(defined_in); match parent_module { - Some((id, parent)) => match parent.kind { + Ok((id, parent)) => match parent.kind { Kind::Module => Ok(ModuleDocumentation::new(id, &parent, db) .map_err(|_| NoParentModule(entry.qualified_name().to_string()))?), _ => Err(NoParentModule(entry.qualified_name().to_string())), }, - None => { - error!("Parent module for entry {} not found.", entry.qualified_name()); + Err(err) => { + error!("Parent module for entry {} not found: {}", entry.qualified_name(), err); Err(NoParentModule(entry.qualified_name().to_string())) } } @@ -237,7 +237,7 @@ impl EntryDocumentation { }; let self_type = db.lookup_by_qualified_name(self_type); match self_type { - Some((id, parent)) => match parent.kind { + Ok((id, parent)) => match parent.kind { Kind::Type => { let docs = Function::from_entry(entry); let type_docs = TypeDocumentation::new(id, &parent, db)?; @@ -254,8 +254,8 @@ impl EntryDocumentation { Ok(Placeholder::NoDocumentation.into()) } }, - None => { - error!("Parent entry for method {} not found.", entry.qualified_name()); + Err(err) => { + error!("Parent entry for method {} not found: {}", entry.qualified_name(), err); Ok(Self::Placeholder(Placeholder::NoDocumentation)) } } @@ -269,14 +269,14 @@ impl EntryDocumentation { let return_type = db.lookup_by_qualified_name(return_type); match return_type { - Some((id, parent)) => { + Ok((id, parent)) => { let docs = Function::from_entry(entry); let type_docs = TypeDocumentation::new(id, &parent, db)?; let module_docs = Self::parent_module(db, &parent)?; Ok(Documentation::Constructor { docs, type_docs, module_docs }.into()) } - None => { - error!("No return type found for constructor {}.", entry.qualified_name()); + Err(err) => { + error!("No return type found for constructor {}: {}", entry.qualified_name(), err); Ok(Placeholder::NoDocumentation.into()) } } diff --git a/app/gui/suggestion-database/src/entry.rs b/app/gui/suggestion-database/src/entry.rs index 050363c36dce..eeb1c4cb1869 100644 --- a/app/gui/suggestion-database/src/entry.rs +++ b/app/gui/suggestion-database/src/entry.rs @@ -518,12 +518,12 @@ impl Entry { fn self_type_entry(&self, db: &SuggestionDatabase) -> Option> { let self_type_ref = self.self_type.as_ref(); - let lookup = self_type_ref.and_then(|tp| db.lookup_by_qualified_name(tp)); + let lookup = self_type_ref.and_then(|tp| db.lookup_by_qualified_name(tp).ok()); lookup.map(|(_, entry)| entry) } fn defined_in_entry(&self, db: &SuggestionDatabase) -> Option> { - let lookup = db.lookup_by_qualified_name(&self.defined_in); + let lookup = db.lookup_by_qualified_name(&self.defined_in).ok(); lookup.map(|(_, entry)| entry) } } @@ -891,7 +891,7 @@ fn resolve_tag_value<'a>( let stripped_expr = raw_expression.trim_start_matches(['(', ' ']).trim_end_matches([')', ' ']); let qualified_name = QualifiedName::from_text(stripped_expr).ok(); if let Some(qualified_name) = qualified_name { - let entry = db.lookup_by_qualified_name(&qualified_name); + let entry = db.lookup_by_qualified_name(&qualified_name).ok(); // If a lookup of a fully qualified name have failed, the name is likely not actually fully // qualified. Try to resolve it as partially qualified, ignoring potential ambiguity. Only diff --git a/app/gui/suggestion-database/src/lib.rs b/app/gui/suggestion-database/src/lib.rs index da8d2e76bf72..ce1bdcb04c32 100644 --- a/app/gui/suggestion-database/src/lib.rs +++ b/app/gui/suggestion-database/src/lib.rs @@ -292,6 +292,10 @@ impl MethodPointerToIdMap { #[fail(display = "The suggestion with id {} has not been found in the database.", _0)] pub struct NoSuchEntry(pub SuggestionId); +#[allow(missing_docs)] +#[derive(Debug, Clone, Eq, Fail, PartialEq)] +#[fail(display = "The suggestion with name {} has not been found in the database.", _0)] +pub struct NoSuchEntryWithName(pub String); // ==================== @@ -476,10 +480,13 @@ impl SuggestionDatabase { /// Search the database for an entry at `fully_qualified_name`. The parameter is expected to be /// composed of segments separated by the [`ACCESS`] character. - pub fn lookup_by_qualified_name_str(&self, qualified_name_str: &str) -> Option> { - let qualified_name = QualifiedName::from_text(qualified_name_str).ok()?; + pub fn lookup_by_qualified_name_str( + &self, + qualified_name_str: &str, + ) -> FallibleResult> { + let qualified_name = QualifiedName::from_text(qualified_name_str)?; let (_, entry) = self.lookup_by_qualified_name(&qualified_name)?; - Some(entry) + Ok(entry) } /// Search the database for an entry at `name` consisting fully qualified name segments, e.g. @@ -487,11 +494,15 @@ impl SuggestionDatabase { pub fn lookup_by_qualified_name<'a>( &self, name: impl Into>, - ) -> Option<(SuggestionId, Rc)> { + ) -> FallibleResult<(SuggestionId, Rc)> { let name = name.into(); let segments = name.segments().map(CloneRef::clone_ref); - let id = self.qualified_name_to_id_map.borrow().get(segments); - id.and_then(|id| Some((id, self.lookup(id).ok()?))) + let id = self + .qualified_name_to_id_map + .borrow() + .get(segments) + .ok_or_else(|| NoSuchEntryWithName(name.to_string()))?; + Ok((id, self.lookup(id)?)) } /// Search the database for entries with given name and visible at given location in module. @@ -668,14 +679,14 @@ fn swap_value_and_traverse_back_pruning_empty_subtrees( pub mod test { use super::*; - use enso_executor::test_utils::TestWithLocalPoolExecutor; - use double_representation::name::NamePath; use engine_protocol::language_server::FieldUpdate; use engine_protocol::language_server::SuggestionEntry; use engine_protocol::language_server::SuggestionsDatabaseEntry; use engine_protocol::language_server::SuggestionsDatabaseModification; + use enso_executor::test_utils::TestWithLocalPoolExecutor; use futures::stream::StreamExt; + use std::assert_matches::assert_matches; use wasm_bindgen_test::wasm_bindgen_test_configure; wasm_bindgen_test_configure!(run_in_browser); @@ -818,7 +829,9 @@ pub mod test { /// is [`None`]. fn lookup_and_verify_empty_result(db: &SuggestionDatabase, fully_qualified_name: &str) { let lookup = db.lookup_by_qualified_name_str(fully_qualified_name); - assert_eq!(lookup, None); + let err = lookup.expect_err("Lookup must return error."); + let err = err.downcast::(); + assert_matches!(err, Ok(NoSuchEntryWithName(fqn)) if fqn == fully_qualified_name); } fn db_entry(id: SuggestionId, suggestion: SuggestionEntry) -> SuggestionsDatabaseEntry { @@ -1326,22 +1339,22 @@ pub mod test { // === Hierarchy Index tests === - fn lookup_id_by_name(db: &SuggestionDatabase, name: &str) -> Option { - let name = QualifiedName::from_text(name).ok()?; + fn lookup_id_by_name(db: &SuggestionDatabase, name: &str) -> FallibleResult { + let name = QualifiedName::from_text(name)?; let (id, _) = db.lookup_by_qualified_name(&name)?; - Some(id) + Ok(id) } /// Verify that the entry with the given `name` has `expected` children. Children are defined as /// methods or constructors of the Type or Types defined in the module. fn verify_hierarchy_index(db: &SuggestionDatabase, name: &str, expected: &[&str]) { let id = lookup_id_by_name(db, name); - let id = id.unwrap_or_else(|| panic!("No entry with name {name}")); + let id = id.unwrap_or_else(|_| panic!("No entry with name {name}")); let hierarchy_index = db.hierarchy_index.borrow(); let actual_ids = hierarchy_index.get(&id); let actual_ids = actual_ids.unwrap_or_else(|| panic!("No entry for id {id}")); let id_from_name = |name: &&str| { - lookup_id_by_name(db, name).unwrap_or_else(|| panic!("No entry with name {name}")) + lookup_id_by_name(db, name).unwrap_or_else(|_| panic!("No entry with name {name}")) }; let expected_ids: HashSet<_> = expected.iter().map(id_from_name).collect(); let name_from_id = |id: &entry::Id| { diff --git a/app/gui/suggestion-database/src/mock.rs b/app/gui/suggestion-database/src/mock.rs index d6d7da9bd479..836e086ed6cf 100644 --- a/app/gui/suggestion-database/src/mock.rs +++ b/app/gui/suggestion-database/src/mock.rs @@ -60,8 +60,8 @@ pub const DEFAULT_TYPE: &str = "Standard.Base.Any"; /// builder.leave(); /// let db = builder.result; /// -/// assert!(db.lookup_by_qualified_name_str("local.Project.Type.Constructor").is_some()); -/// assert!(db.lookup_by_qualified_name_str("local.Project.module_method").is_some()); +/// assert!(db.lookup_by_qualified_name_str("local.Project.Type.Constructor").is_ok()); +/// assert!(db.lookup_by_qualified_name_str("local.Project.module_method").is_ok()); /// ``` #[derive(Debug, Default)] pub struct Builder {