Added comments

Cleaned up the search scope logic to use a map from modules to search
scopes.

crates/ide-assists/src/handlers/remove_unused_imports.rs

@@ -1,14 +1,32 @@
 use std::collections::{hash_map::Entry, HashMap};
 
+use hir::Module;
 use ide_db::{
     defs::Definition,
     search::{FileReference, ReferenceCategory, SearchScope},
 };
-use syntax::{ast, AstNode, SyntaxNode};
+use syntax::{ast, AstNode};
 
 use crate::{AssistContext, AssistId, AssistKind, Assists};
 
+// Assist: remove_unused_imports
+//
+// Removes any use statements in the current selection that are unused.
+//
+// ```
+// struct X();
+// mod foo {
+//     use super::X$0;
+// }
+// ```
+// ->
+// ```
+// struct X();
+// mod foo {
+// }
+// ```
 pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>) -> Option<()> {
+    // First, grab the uses that intersect with the current selection.
     let selected_el = match ctx.covering_element() {
         syntax::NodeOrToken::Node(n) => n,
         syntax::NodeOrToken::Token(t) => t.parent()?,
@@ -22,29 +40,29 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
         .filter_map(ast::Use::cast);
     let uses = uses_up.chain(uses_down).collect::<Vec<_>>();
 
-    let scope_for_node = |node| match ctx.sema.scope(&node).map(|s| s.module()) {
-        Some(m) => Some(SearchScope::module_and_children(ctx.db(), m)),
-        None => None,
-    };
-
-    let mut search_scopes = HashMap::<SyntaxNode, SearchScope>::new();
+    // Maps use nodes to the scope that we should search through to find
+    let mut search_scopes = HashMap::<Module, SearchScope>::new();
 
+    // iterator over all unused use trees
     let mut unused = uses
         .into_iter()
         .flat_map(|u| u.syntax().descendants().filter_map(ast::UseTree::cast))
         .filter(|u| u.use_tree_list().is_none())
         .filter_map(|u| {
-            let scope = match search_scopes.entry(u.syntax().clone()) {
+            // Find any uses trees that are unused
+
+            let module = ctx.sema.scope(&u.syntax()).map(|s| s.module())?;
+            let scope = match search_scopes.entry(module) {
                 Entry::Occupied(o) => o.into_mut(),
-                Entry::Vacant(v) => match scope_for_node(u.syntax().clone()) {
-                    Some(x) => v.insert(x),
-                    None => return None,
-                },
+                Entry::Vacant(v) => v.insert(SearchScope::module_and_children(ctx.db(), module)),
             };
 
+            // Gets the path associated with this use tree. If there isn't one, then ignore this use tree.
             let path = if let Some(path) = u.path() {
                 path
             } else if u.star_token().is_some() {
+                // This case maps to the situation where the * token is braced.
+                // In this case, the parent use tree's path is the one we should use to resolve the glob.
                 match u.syntax().ancestors().skip(1).find_map(ast::UseTree::cast) {
                     Some(parent_u) if parent_u.path().is_some() => parent_u.path().unwrap(),
                     _ => return None,
@@ -53,6 +71,7 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
                 return None;
             };
 
+            // Get the actual definition associated with this use item.
             let res = match ctx.sema.resolve_path(&path) {
                 Some(x) => x,
                 None => {
@@ -66,6 +85,7 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
             };
 
             if u.star_token().is_some() {
+                // Check if any of the children of this module are used
                 let module = match def {
                     Definition::Module(module) => module,
                     _ => return None,
@@ -102,6 +122,7 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
         })
         .peekable();
 
+    // Peek so we terminate early if an unused use is found. Only do the rest of the work if the user selects the assist.
     if unused.peek().is_some() {
         acc.add(
             AssistId("remove_unused_imports", AssistKind::QuickFix),
@@ -122,6 +143,7 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
 fn used_once_in_scope(ctx: &AssistContext<'_>, def: Definition, scope: &SearchScope) -> bool {
     let mut found = false;
     let mut search_non_import = |_, r: FileReference| {
+        // The import itself is a use; we must skip that.
         if r.category != Some(ReferenceCategory::Import) {
             found = true;
             true

crates/ide-assists/src/tests/generated.rs

@@ -2093,6 +2093,24 @@ fn main() {
     )
 }
 
+#[test]
+fn doctest_remove_unused_imports() {
+    check_doc_test(
+        "remove_unused_imports",
+        r#####"
+struct X();
+mod foo {
+    use super::X$0;
+}
+"#####,
+        r#####"
+struct X();
+mod foo {
+}
+"#####,
+    )
+}
+
 #[test]
 fn doctest_remove_unused_param() {
     check_doc_test(