From 6274d461cf3005ff1e22afa77c1a96090f63b40f Mon Sep 17 00:00:00 2001 From: Hayashi Mikihiro <34ttrweoewiwe28@gmail.com> Date: Wed, 14 May 2025 09:59:20 +0900 Subject: [PATCH 1/4] fix: Removing all unused imports removes used imports for imports used for Derive macros Signed-off-by: Hayashi Mikihiro <34ttrweoewiwe28@gmail.com> --- src/tools/rust-analyzer/crates/hir/src/lib.rs | 3 +- .../rust-analyzer/crates/hir/src/semantics.rs | 24 ++ .../crates/hir/src/source_analyzer.rs | 61 ++++- .../src/handlers/remove_unused_imports.rs | 213 +++++++++++++++--- 4 files changed, 256 insertions(+), 45 deletions(-) diff --git a/src/tools/rust-analyzer/crates/hir/src/lib.rs b/src/tools/rust-analyzer/crates/hir/src/lib.rs index c5d37cafd989..3a91050d15fa 100644 --- a/src/tools/rust-analyzer/crates/hir/src/lib.rs +++ b/src/tools/rust-analyzer/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, }, }; diff --git a/src/tools/rust-analyzer/crates/hir/src/semantics.rs b/src/tools/rust-analyzer/crates/hir/src/semantics.rs index 327fe8a75839..7ee72614e220 100644 --- a/src/tools/rust-analyzer/crates/hir/src/semantics.rs +++ b/src/tools/rust-analyzer/crates/hir/src/semantics.rs @@ -103,6 +103,26 @@ impl PathResolution { } } +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub struct PathResolutionPerNs { + pub type_ns: Option, + pub value_ns: Option, + pub macro_ns: Option, +} + +impl PathResolutionPerNs { + pub fn new( + type_ns: Option, + value_ns: Option, + macro_ns: Option, + ) -> Self { + PathResolutionPerNs { type_ns, value_ns, macro_ns } + } + pub fn take_path(&self) -> Option { + 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 { + self.analyze(path.syntax())?.resolve_hir_path_per_ns(self.db, path) + } + pub fn resolve_path_with_subst( &self, path: &ast::Path, diff --git a/src/tools/rust-analyzer/crates/hir/src/source_analyzer.rs b/src/tools/rust-analyzer/crates/hir/src/source_analyzer.rs index 42da7b942c73..610770d0f31e 100644 --- a/src/tools/rust-analyzer/crates/hir/src/source_analyzer.rs +++ b/src/tools/rust-analyzer/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 { + 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 { - 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 { + 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) } - .or_else(items) - .or_else(macros) + if resolve_per_ns { + PathResolutionPerNs { + 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( diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs index 1baf814ca682..fb96882ed428 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs +++ b/src/tools/rust-analyzer/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,49 +80,52 @@ 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) { - Some(x) => x, - None => { + let res = match ctx.sema.resolve_path_per_ns(&path) { + Some(x) if x.take_path().is_some() => x, + Some(_) | None => { return None; } }; + match res { + PathResolutionPerNs { type_ns: Some(type_ns), .. } if u.star_token().is_some() => { + // Check if any of the children of this module are used + let def_mod = match type_ns { + PathResolution::Def(ModuleDef::Module(module)) => module, + _ => return None, + }; - let def = match res { - 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 { - Definition::Module(module) => module, - _ => return None, - }; - - if !def_mod - .scope(ctx.db(), Some(use_module)) - .iter() - .filter_map(|(_, x)| match x { - hir::ScopeDef::ModuleDef(d) => Some(Definition::from(*d)), - _ => None, - }) - .any(|d| used_once_in_scope(ctx, d, u.rename(), scope)) - { - return Some(u); + if !def_mod + .scope(ctx.db(), Some(use_module)) + .iter() + .filter_map(|(_, x)| match x { + hir::ScopeDef::ModuleDef(d) => Some(Definition::from(*d)), + _ => None, + }) + .any(|d| used_once_in_scope(ctx, d, u.rename(), scope)) + { + Some(u) + } else { + None + } } - } 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); + 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) } - } else if !used_once_in_scope(ctx, def, u.rename(), scope) { - return Some(u); } - - None }) .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, + path: &[Option], +) -> 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, + 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(); +} "#, ); } From 5adbda4ee5d3013cf9e9a9479268fc8e17d24c40 Mon Sep 17 00:00:00 2001 From: Hayashi Mikihiro <34ttrweoewiwe28@gmail.com> Date: Thu, 15 May 2025 13:18:30 +0900 Subject: [PATCH 2/4] rename fn name take_path to any Signed-off-by: Hayashi Mikihiro <34ttrweoewiwe28@gmail.com> --- src/tools/rust-analyzer/crates/hir/src/semantics.rs | 2 +- src/tools/rust-analyzer/crates/hir/src/source_analyzer.rs | 6 +++--- .../ide-assists/src/handlers/remove_unused_imports.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/tools/rust-analyzer/crates/hir/src/semantics.rs b/src/tools/rust-analyzer/crates/hir/src/semantics.rs index 7ee72614e220..caa6700de9f9 100644 --- a/src/tools/rust-analyzer/crates/hir/src/semantics.rs +++ b/src/tools/rust-analyzer/crates/hir/src/semantics.rs @@ -118,7 +118,7 @@ impl PathResolutionPerNs { ) -> Self { PathResolutionPerNs { type_ns, value_ns, macro_ns } } - pub fn take_path(&self) -> Option { + pub fn any(&self) -> Option { self.type_ns.or(self.value_ns).or(self.macro_ns) } } diff --git a/src/tools/rust-analyzer/crates/hir/src/source_analyzer.rs b/src/tools/rust-analyzer/crates/hir/src/source_analyzer.rs index 610770d0f31e..ea21546f9d76 100644 --- a/src/tools/rust-analyzer/crates/hir/src/source_analyzer.rs +++ b/src/tools/rust-analyzer/crates/hir/src/source_analyzer.rs @@ -1163,7 +1163,7 @@ impl<'db> SourceAnalyzer<'db> { Some(&store), false, ) - .take_path()?; + .any()?; let subst = (|| { let parent = parent()?; let ty = if let Some(expr) = ast::Expr::cast(parent.clone()) { @@ -1556,7 +1556,7 @@ pub(crate) fn resolve_hir_path( hygiene: HygieneId, store: Option<&ExpressionStore>, ) -> Option { - resolve_hir_path_(db, resolver, path, false, hygiene, store, false).take_path() + resolve_hir_path_(db, resolver, path, false, hygiene, store, false).any() } #[inline] @@ -1677,7 +1677,7 @@ fn resolve_hir_path_( .unwrap_or_else(|| PathResolutionPerNs::new(None, values(), None)) }; - if res.take_path().is_some() { + if res.any().is_some() { res } else if let Some(type_ns) = items() { PathResolutionPerNs::new(Some(type_ns), None, None) diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs index fb96882ed428..dff9a660cf59 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs +++ b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs @@ -81,7 +81,7 @@ 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_per_ns(&path) { - Some(x) if x.take_path().is_some() => x, + Some(x) if x.any().is_some() => x, Some(_) | None => { return None; } From 2c55a78848cebf3bede3339362bbfcbc6c18fa8c Mon Sep 17 00:00:00 2001 From: Hayashi Mikihiro <34ttrweoewiwe28@gmail.com> Date: Thu, 15 May 2025 14:01:05 +0900 Subject: [PATCH 3/4] check glob Signed-off-by: Hayashi Mikihiro <34ttrweoewiwe28@gmail.com> --- .../src/handlers/remove_unused_imports.rs | 41 ++++++++++--------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs index dff9a660cf59..994e7c446cc5 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs +++ b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs @@ -86,28 +86,29 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>) return None; } }; - match res { - PathResolutionPerNs { type_ns: Some(type_ns), .. } if u.star_token().is_some() => { - // Check if any of the children of this module are used - let def_mod = match type_ns { - PathResolution::Def(ModuleDef::Module(module)) => module, - _ => return None, - }; - if !def_mod - .scope(ctx.db(), Some(use_module)) - .iter() - .filter_map(|(_, x)| match x { - hir::ScopeDef::ModuleDef(d) => Some(Definition::from(*d)), - _ => None, - }) - .any(|d| used_once_in_scope(ctx, d, u.rename(), scope)) - { - Some(u) - } else { - None - } + if u.star_token().is_some() { + // Check if any of the children of this module are used + let def_mod = match res.type_ns { + Some(PathResolution::Def(ModuleDef::Module(module))) => module, + _ => return None, + }; + + if !def_mod + .scope(ctx.db(), Some(use_module)) + .iter() + .filter_map(|(_, x)| match x { + hir::ScopeDef::ModuleDef(d) => Some(Definition::from(*d)), + _ => None, + }) + .any(|d| used_once_in_scope(ctx, d, u.rename(), scope)) + { + return Some(u); + } else { + return None; } + } + match res { PathResolutionPerNs { type_ns: Some(PathResolution::Def(ModuleDef::Trait(ref t))), value_ns, From 034d2a2fe778d0effe9532775f08a5652159b207 Mon Sep 17 00:00:00 2001 From: Hayashi Mikihiro <34ttrweoewiwe28@gmail.com> Date: Thu, 15 May 2025 15:12:31 +0900 Subject: [PATCH 4/4] handle trait in function Signed-off-by: Hayashi Mikihiro <34ttrweoewiwe28@gmail.com> --- .../src/handlers/remove_unused_imports.rs | 44 ++++++++++--------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs index 994e7c446cc5..16debc4d7285 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs +++ b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs @@ -103,29 +103,12 @@ 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 { - return None; - } - } - match res { - 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) + None } + } else { + is_path_per_ns_unused_in_scope(ctx, &u, scope, &res).then_some(u) } }) .peekable(); @@ -148,6 +131,25 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>) } } +fn is_path_per_ns_unused_in_scope( + ctx: &AssistContext<'_>, + u: &ast::UseTree, + scope: &mut Vec, + path: &PathResolutionPerNs, +) -> bool { + if let Some(PathResolution::Def(ModuleDef::Trait(ref t))) = path.type_ns { + if is_trait_unused_in_scope(ctx, u, scope, t) { + let path = [path.value_ns, path.macro_ns]; + is_path_unused_in_scope(ctx, u, scope, &path) + } else { + false + } + } else { + let path = [path.type_ns, path.value_ns, path.macro_ns]; + is_path_unused_in_scope(ctx, u, scope, &path) + } +} + fn is_path_unused_in_scope( ctx: &AssistContext<'_>, u: &ast::UseTree,