fix: Removing all unused imports removes used imports for imports used for Derive macros

Signed-off-by: Hayashi Mikihiro <34ttrweoewiwe28@gmail.com>

crates/hir/src/lib.rs

@@ -97,7 +97,8 @@ pub use crate::{
     diagnostics::*,
     has_source::HasSource,
     semantics::{
-        PathResolution, Semantics, SemanticsImpl, SemanticsScope, TypeInfo, VisibleTraits,
+        PathResolution, PathResolutionPerNs, Semantics, SemanticsImpl, SemanticsScope, TypeInfo,
+        VisibleTraits,
     },
 };
 

crates/hir/src/semantics.rs

@@ -103,6 +103,26 @@ impl PathResolution {
     }
 }
 
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub struct PathResolutionPerNs {
+    pub type_ns: Option<PathResolution>,
+    pub value_ns: Option<PathResolution>,
+    pub macro_ns: Option<PathResolution>,
+}
+
+impl PathResolutionPerNs {
+    pub fn new(
+        type_ns: Option<PathResolution>,
+        value_ns: Option<PathResolution>,
+        macro_ns: Option<PathResolution>,
+    ) -> Self {
+        PathResolutionPerNs { type_ns, value_ns, macro_ns }
+    }
+    pub fn take_path(&self) -> Option<PathResolution> {
+        self.type_ns.or(self.value_ns).or(self.macro_ns)
+    }
+}
+
 #[derive(Debug)]
 pub struct TypeInfo {
     /// The original type of the expression or pattern.
@@ -1606,6 +1626,10 @@ impl<'db> SemanticsImpl<'db> {
         self.resolve_path_with_subst(path).map(|(it, _)| it)
     }
 
+    pub fn resolve_path_per_ns(&self, path: &ast::Path) -> Option<PathResolutionPerNs> {
+        self.analyze(path.syntax())?.resolve_hir_path_per_ns(self.db, path)
+    }
+
     pub fn resolve_path_with_subst(
         &self,
         path: &ast::Path,

crates/hir/src/source_analyzer.rs

@@ -10,7 +10,9 @@ use std::iter::{self, once};
 use crate::{
     Adt, AssocItem, BindingMode, BuiltinAttr, BuiltinType, Callable, Const, DeriveHelper, Field,
     Function, GenericSubstitution, Local, Macro, ModuleDef, Static, Struct, ToolModule, Trait,
-    TraitAlias, TupleField, Type, TypeAlias, Variant, db::HirDatabase, semantics::PathResolution,
+    TraitAlias, TupleField, Type, TypeAlias, Variant,
+    db::HirDatabase,
+    semantics::{PathResolution, PathResolutionPerNs},
 };
 use either::Either;
 use hir_def::{
@@ -1159,7 +1161,9 @@ impl<'db> SourceAnalyzer<'db> {
                 prefer_value_ns,
                 name_hygiene(db, InFile::new(self.file_id, path.syntax())),
                 Some(&store),
-            )?;
+                false,
+            )
+            .take_path()?;
             let subst = (|| {
                 let parent = parent()?;
                 let ty = if let Some(expr) = ast::Expr::cast(parent.clone()) {
@@ -1209,6 +1213,26 @@ impl<'db> SourceAnalyzer<'db> {
         }
     }
 
+    pub(crate) fn resolve_hir_path_per_ns(
+        &self,
+        db: &dyn HirDatabase,
+        path: &ast::Path,
+    ) -> Option<PathResolutionPerNs> {
+        let mut collector = ExprCollector::new(db, self.resolver.module(), self.file_id);
+        let hir_path =
+            collector.lower_path(path.clone(), &mut ExprCollector::impl_trait_error_allocator)?;
+        let store = collector.store.finish();
+        Some(resolve_hir_path_(
+            db,
+            &self.resolver,
+            &hir_path,
+            false,
+            name_hygiene(db, InFile::new(self.file_id, path.syntax())),
+            Some(&store),
+            true,
+        ))
+    }
+
     pub(crate) fn record_literal_missing_fields(
         &self,
         db: &'db dyn HirDatabase,
@@ -1532,7 +1556,7 @@ pub(crate) fn resolve_hir_path(
     hygiene: HygieneId,
     store: Option<&ExpressionStore>,
 ) -> Option<PathResolution> {
-    resolve_hir_path_(db, resolver, path, false, hygiene, store)
+    resolve_hir_path_(db, resolver, path, false, hygiene, store, false).take_path()
 }
 
 #[inline]
@@ -1554,7 +1578,8 @@ fn resolve_hir_path_(
     prefer_value_ns: bool,
     hygiene: HygieneId,
     store: Option<&ExpressionStore>,
-) -> Option<PathResolution> {
+    resolve_per_ns: bool,
+) -> PathResolutionPerNs {
     let types = || {
         let (ty, unresolved) = match path.type_anchor() {
             Some(type_ref) => resolver.generic_def().and_then(|def| {
@@ -1635,9 +1660,31 @@ fn resolve_hir_path_(
             .map(|(def, _)| PathResolution::Def(ModuleDef::Macro(def.into())))
     };
 
-    if prefer_value_ns { values().or_else(types) } else { types().or_else(values) }
+    if resolve_per_ns {
-        .or_else(items)
+        PathResolutionPerNs {
-        .or_else(macros)
+            type_ns: types().or_else(items),
+            value_ns: values(),
+            macro_ns: macros(),
+        }
+    } else {
+        let res = if prefer_value_ns {
+            values()
+                .map(|value_ns| PathResolutionPerNs::new(None, Some(value_ns), None))
+                .unwrap_or_else(|| PathResolutionPerNs::new(types(), None, None))
+        } else {
+            types()
+                .map(|type_ns| PathResolutionPerNs::new(Some(type_ns), None, None))
+                .unwrap_or_else(|| PathResolutionPerNs::new(None, values(), None))
+        };
+
+        if res.take_path().is_some() {
+            res
+        } else if let Some(type_ns) = items() {
+            PathResolutionPerNs::new(Some(type_ns), None, None)
+        } else {
+            PathResolutionPerNs::new(None, None, macros())
+        }
+    }
 }
 
 fn resolve_hir_value_path(

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

@@ -1,6 +1,9 @@
 use std::collections::hash_map::Entry;
 
-use hir::{FileRange, InFile, InRealFile, Module, ModuleSource};
+use hir::{
+    FileRange, InFile, InRealFile, Module, ModuleDef, ModuleSource, PathResolution,
+    PathResolutionPerNs,
+};
 use ide_db::text_edit::TextRange;
 use ide_db::{
     FxHashMap, RootDatabase,
@@ -77,22 +80,17 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
             };
 
             // Get the actual definition associated with this use item.
-            let res = match ctx.sema.resolve_path(&path) {
+            let res = match ctx.sema.resolve_path_per_ns(&path) {
-                Some(x) => x,
+                Some(x) if x.take_path().is_some() => x,
-                None => {
+                Some(_) | None => {
                     return None;
                 }
             };
-
+            match res {
-            let def = match res {
+                PathResolutionPerNs { type_ns: Some(type_ns), .. } if u.star_token().is_some() => {
-                hir::PathResolution::Def(d) => Definition::from(d),
-                _ => return None,
-            };
-
-            if u.star_token().is_some() {
                     // Check if any of the children of this module are used
-                let def_mod = match def {
+                    let def_mod = match type_ns {
-                    Definition::Module(module) => module,
+                        PathResolution::Def(ModuleDef::Module(module)) => module,
                         _ => return None,
                     };
 
@@ -105,21 +103,29 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
                         })
                         .any(|d| used_once_in_scope(ctx, d, u.rename(), scope))
                     {
-                    return Some(u);
+                        Some(u)
-                }
+                    } else {
-            } else if let Definition::Trait(ref t) = def {
-                // If the trait or any item is used.
-                if !std::iter::once((def, u.rename()))
-                    .chain(t.items(ctx.db()).into_iter().map(|item| (item.into(), None)))
-                    .any(|(d, rename)| used_once_in_scope(ctx, d, rename, scope))
-                {
-                    return Some(u);
-                }
-            } else if !used_once_in_scope(ctx, def, u.rename(), scope) {
-                return Some(u);
-            }
-
                         None
+                    }
+                }
+                PathResolutionPerNs {
+                    type_ns: Some(PathResolution::Def(ModuleDef::Trait(ref t))),
+                    value_ns,
+                    macro_ns,
+                } => {
+                    // If the trait or any item is used.
+                    if is_trait_unused_in_scope(ctx, &u, scope, t) {
+                        let path = [value_ns, macro_ns];
+                        is_path_unused_in_scope(ctx, &u, scope, &path).then_some(u)
+                    } else {
+                        None
+                    }
+                }
+                PathResolutionPerNs { type_ns, value_ns, macro_ns } => {
+                    let path = [type_ns, value_ns, macro_ns];
+                    is_path_unused_in_scope(ctx, &u, scope, &path).then_some(u)
+                }
+            }
         })
         .peekable();
 
@@ -141,6 +147,33 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
     }
 }
 
+fn is_path_unused_in_scope(
+    ctx: &AssistContext<'_>,
+    u: &ast::UseTree,
+    scope: &mut Vec<SearchScope>,
+    path: &[Option<PathResolution>],
+) -> bool {
+    !path
+        .iter()
+        .filter_map(|path| *path)
+        .filter_map(|res| match res {
+            PathResolution::Def(d) => Some(Definition::from(d)),
+            _ => None,
+        })
+        .any(|def| used_once_in_scope(ctx, def, u.rename(), scope))
+}
+
+fn is_trait_unused_in_scope(
+    ctx: &AssistContext<'_>,
+    u: &ast::UseTree,
+    scope: &mut Vec<SearchScope>,
+    t: &hir::Trait,
+) -> bool {
+    !std::iter::once((Definition::Trait(*t), u.rename()))
+        .chain(t.items(ctx.db()).into_iter().map(|item| (item.into(), None)))
+        .any(|(d, rename)| used_once_in_scope(ctx, d, rename, scope))
+}
+
 fn used_once_in_scope(
     ctx: &AssistContext<'_>,
     def: Definition,
@@ -1009,6 +1042,112 @@ fn test(_: Bar) {
     let a = ();
     a.quxx();
 }
+"#,
+        );
+    }
+
+    #[test]
+    fn test_unused_macro() {
+        check_assist(
+            remove_unused_imports,
+            r#"
+//- /foo.rs crate:foo
+#[macro_export]
+macro_rules! m { () => {} }
+
+//- /main.rs crate:main deps:foo
+use foo::m;$0
+fn main() {}
+"#,
+            r#"
+fn main() {}
+"#,
+        );
+
+        check_assist_not_applicable(
+            remove_unused_imports,
+            r#"
+//- /foo.rs crate:foo
+#[macro_export]
+macro_rules! m { () => {} }
+
+//- /main.rs crate:main deps:foo
+use foo::m;$0
+fn main() {
+    m!();
+}
+"#,
+        );
+
+        check_assist_not_applicable(
+            remove_unused_imports,
+            r#"
+//- /foo.rs crate:foo
+#[macro_export]
+macro_rules! m { () => {} }
+
+//- /bar.rs crate:bar deps:foo
+pub use foo::m;
+fn m() {}
+
+
+//- /main.rs crate:main deps:bar
+use bar::m;$0
+fn main() {
+    m!();
+}
+"#,
+        );
+    }
+
+    #[test]
+    fn test_conflict_derive_macro() {
+        check_assist_not_applicable(
+            remove_unused_imports,
+            r#"
+//- proc_macros: derive_identity
+//- minicore: derive
+//- /bar.rs crate:bar
+pub use proc_macros::DeriveIdentity;
+pub trait DeriveIdentity {}
+
+//- /main.rs crate:main deps:bar
+$0use bar::DeriveIdentity;$0
+#[derive(DeriveIdentity)]
+struct S;
+"#,
+        );
+
+        check_assist_not_applicable(
+            remove_unused_imports,
+            r#"
+//- proc_macros: derive_identity
+//- minicore: derive
+//- /bar.rs crate:bar
+pub use proc_macros::DeriveIdentity;
+pub fn DeriveIdentity() {}
+
+//- /main.rs crate:main deps:bar
+$0use bar::DeriveIdentity;$0
+#[derive(DeriveIdentity)]
+struct S;
+"#,
+        );
+
+        check_assist_not_applicable(
+            remove_unused_imports,
+            r#"
+//- proc_macros: derive_identity
+//- minicore: derive
+//- /bar.rs crate:bar
+pub use proc_macros::DeriveIdentity;
+pub fn DeriveIdentity() {}
+
+//- /main.rs crate:main deps:bar
+$0use bar::DeriveIdentity;$0
+fn main() {
+    DeriveIdentity();
+}
 "#,
         );
     }