Fix redundant imports when selecting options from the dropdown widget

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)

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<enso_suggestion_database::Entry>),
+    /// 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<Id>,
     pub module:        model::Module,
     pub suggestion_db: Rc<model::SuggestionDatabase>,
+    project_name:      project::QualifiedName,
     parser:            Parser,
 }
 
@@ -485,9 +515,10 @@ impl Handle {
         suggestion_db: Rc<model::SuggestionDatabase>,
         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<model::SuggestionDatabase>,
         parser: Parser,
         id: Id,
+        project_name: project::QualifiedName,
     ) -> FallibleResult<Handle> {
-        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<Item = RequiredImport>,
+        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 {

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<model::SuggestionDatabase>) -> 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()

app/gui/src/controller/module.rs

@@ -42,6 +42,7 @@ pub struct Handle {
     pub model:           model::Module,
     pub language_server: Rc<language_server::Connection>,
     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<model::SuggestionDatabase>,
     ) -> FallibleResult<controller::Graph> {
-        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)]