Skip to content

Commit 2e718f8

Browse files
ChayimFriedman2ChayimFriedman2
authored
Merge pull request rust-lang#19793 from Hmikihiro/unused_import_conlict_derive
fix: Removing all unused imports removes used imports for imports used for Derive macros
2 parents 7a84531 + 034d2a2 commit 2e718f8

File tree

4 files changed

+246
-32
lines changed

4 files changed

+246
-32
lines changed

‎src/tools/rust-analyzer/crates/hir/src/lib.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,8 @@ pub use crate::{
9797
diagnostics::*,
9898
has_source::HasSource,
9999
semantics::{
100-
PathResolution, Semantics, SemanticsImpl, SemanticsScope, TypeInfo, VisibleTraits,
100+
PathResolution, PathResolutionPerNs, Semantics, SemanticsImpl, SemanticsScope, TypeInfo,
101+
VisibleTraits,
101102
},
102103
};
103104

‎src/tools/rust-analyzer/crates/hir/src/semantics.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,26 @@ impl PathResolution {
103103
}
104104
}
105105

106+
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
107+
pub struct PathResolutionPerNs {
108+
pub type_ns: Option<PathResolution>,
109+
pub value_ns: Option<PathResolution>,
110+
pub macro_ns: Option<PathResolution>,
111+
}
112+
113+
impl PathResolutionPerNs {
114+
pub fn new(
115+
type_ns: Option<PathResolution>,
116+
value_ns: Option<PathResolution>,
117+
macro_ns: Option<PathResolution>,
118+
) -> Self {
119+
PathResolutionPerNs { type_ns, value_ns, macro_ns }
120+
}
121+
pub fn any(&self) -> Option<PathResolution> {
122+
self.type_ns.or(self.value_ns).or(self.macro_ns)
123+
}
124+
}
125+
106126
#[derive(Debug)]
107127
pub struct TypeInfo {
108128
/// The original type of the expression or pattern.
@@ -1606,6 +1626,10 @@ impl<'db> SemanticsImpl<'db> {
16061626
self.resolve_path_with_subst(path).map(|(it, _)| it)
16071627
}
16081628

1629+
pub fn resolve_path_per_ns(&self, path: &ast::Path) -> Option<PathResolutionPerNs> {
1630+
self.analyze(path.syntax())?.resolve_hir_path_per_ns(self.db, path)
1631+
}
1632+
16091633
pub fn resolve_path_with_subst(
16101634
&self,
16111635
path: &ast::Path,

‎src/tools/rust-analyzer/crates/hir/src/source_analyzer.rs

Lines changed: 54 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@ use std::iter::{self, once};
1010
use crate::{
1111
Adt, AssocItem, BindingMode, BuiltinAttr, BuiltinType, Callable, Const, DeriveHelper, Field,
1212
Function, GenericSubstitution, Local, Macro, ModuleDef, Static, Struct, ToolModule, Trait,
13-
TraitAlias, TupleField, Type, TypeAlias, Variant, db::HirDatabase, semantics::PathResolution,
13+
TraitAlias, TupleField, Type, TypeAlias, Variant,
14+
db::HirDatabase,
15+
semantics::{PathResolution, PathResolutionPerNs},
1416
};
1517
use either::Either;
1618
use hir_def::{
@@ -1159,7 +1161,9 @@ impl<'db> SourceAnalyzer<'db> {
11591161
prefer_value_ns,
11601162
name_hygiene(db, InFile::new(self.file_id, path.syntax())),
11611163
Some(&store),
1162-
)?;
1164+
false,
1165+
)
1166+
.any()?;
11631167
let subst = (|| {
11641168
let parent = parent()?;
11651169
let ty = if let Some(expr) = ast::Expr::cast(parent.clone()) {
@@ -1209,6 +1213,26 @@ impl<'db> SourceAnalyzer<'db> {
12091213
}
12101214
}
12111215

1216+
pub(crate) fn resolve_hir_path_per_ns(
1217+
&self,
1218+
db: &dyn HirDatabase,
1219+
path: &ast::Path,
1220+
) -> Option<PathResolutionPerNs> {
1221+
let mut collector = ExprCollector::new(db, self.resolver.module(), self.file_id);
1222+
let hir_path =
1223+
collector.lower_path(path.clone(), &mut ExprCollector::impl_trait_error_allocator)?;
1224+
let store = collector.store.finish();
1225+
Some(resolve_hir_path_(
1226+
db,
1227+
&self.resolver,
1228+
&hir_path,
1229+
false,
1230+
name_hygiene(db, InFile::new(self.file_id, path.syntax())),
1231+
Some(&store),
1232+
true,
1233+
))
1234+
}
1235+
12121236
pub(crate) fn record_literal_missing_fields(
12131237
&self,
12141238
db: &'db dyn HirDatabase,
@@ -1532,7 +1556,7 @@ pub(crate) fn resolve_hir_path(
15321556
hygiene: HygieneId,
15331557
store: Option<&ExpressionStore>,
15341558
) -> Option<PathResolution> {
1535-
resolve_hir_path_(db, resolver, path, false, hygiene, store)
1559+
resolve_hir_path_(db, resolver, path, false, hygiene, store, false).any()
15361560
}
15371561

15381562
#[inline]
@@ -1554,7 +1578,8 @@ fn resolve_hir_path_(
15541578
prefer_value_ns: bool,
15551579
hygiene: HygieneId,
15561580
store: Option<&ExpressionStore>,
1557-
) -> Option<PathResolution> {
1581+
resolve_per_ns: bool,
1582+
) -> PathResolutionPerNs {
15581583
let types = || {
15591584
let (ty, unresolved) = match path.type_anchor() {
15601585
Some(type_ref) => resolver.generic_def().and_then(|def| {
@@ -1635,9 +1660,31 @@ fn resolve_hir_path_(
16351660
.map(|(def, _)| PathResolution::Def(ModuleDef::Macro(def.into())))
16361661
};
16371662

1638-
if prefer_value_ns { values().or_else(types) } else { types().or_else(values) }
1639-
.or_else(items)
1640-
.or_else(macros)
1663+
if resolve_per_ns {
1664+
PathResolutionPerNs {
1665+
type_ns: types().or_else(items),
1666+
value_ns: values(),
1667+
macro_ns: macros(),
1668+
}
1669+
} else {
1670+
let res = if prefer_value_ns {
1671+
values()
1672+
.map(|value_ns| PathResolutionPerNs::new(None, Some(value_ns), None))
1673+
.unwrap_or_else(|| PathResolutionPerNs::new(types(), None, None))
1674+
} else {
1675+
types()
1676+
.map(|type_ns| PathResolutionPerNs::new(Some(type_ns), None, None))
1677+
.unwrap_or_else(|| PathResolutionPerNs::new(None, values(), None))
1678+
};
1679+
1680+
if res.any().is_some() {
1681+
res
1682+
} else if let Some(type_ns) = items() {
1683+
PathResolutionPerNs::new(Some(type_ns), None, None)
1684+
} else {
1685+
PathResolutionPerNs::new(None, None, macros())
1686+
}
1687+
}
16411688
}
16421689

16431690
fn resolve_hir_value_path(

‎src/tools/rust-analyzer/crates/ide-assists/src/handlers/remove_unused_imports.rs

Lines changed: 166 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
use std::collections::hash_map::Entry;
22

3-
use hir::{FileRange, InFile, InRealFile, Module, ModuleSource};
3+
use hir::{
4+
FileRange, InFile, InRealFile, Module, ModuleDef, ModuleSource, PathResolution,
5+
PathResolutionPerNs,
6+
};
47
use ide_db::text_edit::TextRange;
58
use ide_db::{
69
FxHashMap, RootDatabase,
@@ -77,22 +80,17 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
7780
};
7881

7982
// Get the actual definition associated with this use item.
80-
let res = match ctx.sema.resolve_path(&path) {
81-
Some(x) => x,
82-
None => {
83+
let res = match ctx.sema.resolve_path_per_ns(&path) {
84+
Some(x) if x.any().is_some() => x,
85+
Some(_) | None => {
8386
return None;
8487
}
8588
};
8689

87-
let def = match res {
88-
hir::PathResolution::Def(d) => Definition::from(d),
89-
_ => return None,
90-
};
91-
9290
if u.star_token().is_some() {
9391
// Check if any of the children of this module are used
94-
let def_mod = match def {
95-
Definition::Module(module) => module,
92+
let def_mod = match res.type_ns {
93+
Some(PathResolution::Def(ModuleDef::Module(module))) => module,
9694
_ => return None,
9795
};
9896

@@ -105,21 +103,13 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
105103
})
106104
.any(|d| used_once_in_scope(ctx, d, u.rename(), scope))
107105
{
108-
return Some(u);
109-
}
110-
} else if let Definition::Trait(ref t) = def {
111-
// If the trait or any item is used.
112-
if !std::iter::once((def, u.rename()))
113-
.chain(t.items(ctx.db()).into_iter().map(|item| (item.into(), None)))
114-
.any(|(d, rename)| used_once_in_scope(ctx, d, rename, scope))
115-
{
116-
return Some(u);
106+
Some(u)
107+
} else {
108+
None
117109
}
118-
} else if !used_once_in_scope(ctx, def, u.rename(), scope) {
119-
return Some(u);
110+
} else {
111+
is_path_per_ns_unused_in_scope(ctx, &u, scope, &res).then_some(u)
120112
}
121-
122-
None
123113
})
124114
.peekable();
125115

@@ -141,6 +131,52 @@ pub(crate) fn remove_unused_imports(acc: &mut Assists, ctx: &AssistContext<'_>)
141131
}
142132
}
143133

134+
fn is_path_per_ns_unused_in_scope(
135+
ctx: &AssistContext<'_>,
136+
u: &ast::UseTree,
137+
scope: &mut Vec<SearchScope>,
138+
path: &PathResolutionPerNs,
139+
) -> bool {
140+
if let Some(PathResolution::Def(ModuleDef::Trait(ref t))) = path.type_ns {
141+
if is_trait_unused_in_scope(ctx, u, scope, t) {
142+
let path = [path.value_ns, path.macro_ns];
143+
is_path_unused_in_scope(ctx, u, scope, &path)
144+
} else {
145+
false
146+
}
147+
} else {
148+
let path = [path.type_ns, path.value_ns, path.macro_ns];
149+
is_path_unused_in_scope(ctx, u, scope, &path)
150+
}
151+
}
152+
153+
fn is_path_unused_in_scope(
154+
ctx: &AssistContext<'_>,
155+
u: &ast::UseTree,
156+
scope: &mut Vec<SearchScope>,
157+
path: &[Option<PathResolution>],
158+
) -> bool {
159+
!path
160+
.iter()
161+
.filter_map(|path| *path)
162+
.filter_map(|res| match res {
163+
PathResolution::Def(d) => Some(Definition::from(d)),
164+
_ => None,
165+
})
166+
.any(|def| used_once_in_scope(ctx, def, u.rename(), scope))
167+
}
168+
169+
fn is_trait_unused_in_scope(
170+
ctx: &AssistContext<'_>,
171+
u: &ast::UseTree,
172+
scope: &mut Vec<SearchScope>,
173+
t: &hir::Trait,
174+
) -> bool {
175+
!std::iter::once((Definition::Trait(*t), u.rename()))
176+
.chain(t.items(ctx.db()).into_iter().map(|item| (item.into(), None)))
177+
.any(|(d, rename)| used_once_in_scope(ctx, d, rename, scope))
178+
}
179+
144180
fn used_once_in_scope(
145181
ctx: &AssistContext<'_>,
146182
def: Definition,
@@ -1009,6 +1045,112 @@ fn test(_: Bar) {
10091045
let a = ();
10101046
a.quxx();
10111047
}
1048+
"#,
1049+
);
1050+
}
1051+
1052+
#[test]
1053+
fn test_unused_macro() {
1054+
check_assist(
1055+
remove_unused_imports,
1056+
r#"
1057+
//- /foo.rs crate:foo
1058+
#[macro_export]
1059+
macro_rules! m { () => {} }
1060+
1061+
//- /main.rs crate:main deps:foo
1062+
use foo::m;$0
1063+
fn main() {}
1064+
"#,
1065+
r#"
1066+
fn main() {}
1067+
"#,
1068+
);
1069+
1070+
check_assist_not_applicable(
1071+
remove_unused_imports,
1072+
r#"
1073+
//- /foo.rs crate:foo
1074+
#[macro_export]
1075+
macro_rules! m { () => {} }
1076+
1077+
//- /main.rs crate:main deps:foo
1078+
use foo::m;$0
1079+
fn main() {
1080+
m!();
1081+
}
1082+
"#,
1083+
);
1084+
1085+
check_assist_not_applicable(
1086+
remove_unused_imports,
1087+
r#"
1088+
//- /foo.rs crate:foo
1089+
#[macro_export]
1090+
macro_rules! m { () => {} }
1091+
1092+
//- /bar.rs crate:bar deps:foo
1093+
pub use foo::m;
1094+
fn m() {}
1095+
1096+
1097+
//- /main.rs crate:main deps:bar
1098+
use bar::m;$0
1099+
fn main() {
1100+
m!();
1101+
}
1102+
"#,
1103+
);
1104+
}
1105+
1106+
#[test]
1107+
fn test_conflict_derive_macro() {
1108+
check_assist_not_applicable(
1109+
remove_unused_imports,
1110+
r#"
1111+
//- proc_macros: derive_identity
1112+
//- minicore: derive
1113+
//- /bar.rs crate:bar
1114+
pub use proc_macros::DeriveIdentity;
1115+
pub trait DeriveIdentity {}
1116+
1117+
//- /main.rs crate:main deps:bar
1118+
$0use bar::DeriveIdentity;$0
1119+
#[derive(DeriveIdentity)]
1120+
struct S;
1121+
"#,
1122+
);
1123+
1124+
check_assist_not_applicable(
1125+
remove_unused_imports,
1126+
r#"
1127+
//- proc_macros: derive_identity
1128+
//- minicore: derive
1129+
//- /bar.rs crate:bar
1130+
pub use proc_macros::DeriveIdentity;
1131+
pub fn DeriveIdentity() {}
1132+
1133+
//- /main.rs crate:main deps:bar
1134+
$0use bar::DeriveIdentity;$0
1135+
#[derive(DeriveIdentity)]
1136+
struct S;
1137+
"#,
1138+
);
1139+
1140+
check_assist_not_applicable(
1141+
remove_unused_imports,
1142+
r#"
1143+
//- proc_macros: derive_identity
1144+
//- minicore: derive
1145+
//- /bar.rs crate:bar
1146+
pub use proc_macros::DeriveIdentity;
1147+
pub fn DeriveIdentity() {}
1148+
1149+
//- /main.rs crate:main deps:bar
1150+
$0use bar::DeriveIdentity;$0
1151+
fn main() {
1152+
DeriveIdentity();
1153+
}
10121154
"#,
10131155
);
10141156
}

0 commit comments

Comments
 (0)