Skip to content

Commit 656cbc6

Browse files
bors[bot]Nashenas88
bors[bot]
and
Nashenas88
authored
Merge #5033
5033: Order of glob imports should not affect import shadowing r=Nashenas88 a=Nashenas88 Fixes #5032 Co-authored-by: Paul Daniel Faria <[email protected]>
2 parents 9a4d02f + 1f5d30f commit 656cbc6

File tree

4 files changed

+244
-20
lines changed

4 files changed

+244
-20
lines changed

‎crates/ra_hir_def/src/item_scope.rs

Lines changed: 69 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,27 @@
44
use hir_expand::name::Name;
55
use once_cell::sync::Lazy;
66
use ra_db::CrateId;
7-
use rustc_hash::FxHashMap;
7+
use rustc_hash::{FxHashMap, FxHashSet};
88
use test_utils::mark;
99

1010
use crate::{
1111
db::DefDatabase, per_ns::PerNs, visibility::Visibility, AdtId, BuiltinType, HasModule, ImplId,
12-
Lookup, MacroDefId, ModuleDefId, TraitId,
12+
LocalModuleId, Lookup, MacroDefId, ModuleDefId, TraitId,
1313
};
1414

15+
#[derive(Copy, Clone)]
16+
pub(crate) enum ImportType {
17+
Glob,
18+
Named,
19+
}
20+
21+
#[derive(Debug, Default)]
22+
pub struct PerNsGlobImports {
23+
types: FxHashSet<(LocalModuleId, Name)>,
24+
values: FxHashSet<(LocalModuleId, Name)>,
25+
macros: FxHashSet<(LocalModuleId, Name)>,
26+
}
27+
1528
#[derive(Debug, Default, PartialEq, Eq)]
1629
pub struct ItemScope {
1730
visible: FxHashMap<Name, PerNs>,
@@ -127,26 +140,72 @@ impl ItemScope {
127140
let mut changed = false;
128141
let existing = self.visible.entry(name).or_default();
129142

143+
if existing.types.is_none() && def.types.is_some() {
144+
existing.types = def.types;
145+
changed = true;
146+
}
147+
148+
if existing.values.is_none() && def.values.is_some() {
149+
existing.values = def.values;
150+
changed = true;
151+
}
152+
153+
if existing.macros.is_none() && def.macros.is_some() {
154+
existing.macros = def.macros;
155+
changed = true;
156+
}
157+
158+
changed
159+
}
160+
161+
pub(crate) fn push_res_with_import(
162+
&mut self,
163+
glob_imports: &mut PerNsGlobImports,
164+
lookup: (LocalModuleId, Name),
165+
def: PerNs,
166+
def_import_type: ImportType,
167+
) -> bool {
168+
let mut changed = false;
169+
let existing = self.visible.entry(lookup.1.clone()).or_default();
170+
130171
macro_rules! check_changed {
131-
($changed:ident, $existing:expr, $def:expr) => {
132-
match ($existing, $def) {
172+
(
173+
$changed:ident,
174+
( $existing:ident / $def:ident ) . $field:ident,
175+
$glob_imports:ident [ $lookup:ident ],
176+
$def_import_type:ident
177+
) => {
178+
match ($existing.$field, $def.$field) {
133179
(None, Some(_)) => {
134-
$existing = $def;
180+
match $def_import_type {
181+
ImportType::Glob => {
182+
$glob_imports.$field.insert($lookup.clone());
183+
}
184+
ImportType::Named => {
185+
$glob_imports.$field.remove(&$lookup);
186+
}
187+
}
188+
189+
$existing.$field = $def.$field;
135190
$changed = true;
136191
}
137-
(Some(e), Some(d)) if e.0 != d.0 => {
192+
(Some(_), Some(_))
193+
if $glob_imports.$field.contains(&$lookup)
194+
&& matches!($def_import_type, ImportType::Named) =>
195+
{
138196
mark::hit!(import_shadowed);
139-
$existing = $def;
197+
$glob_imports.$field.remove(&$lookup);
198+
$existing.$field = $def.$field;
140199
$changed = true;
141200
}
142201
_ => {}
143202
}
144203
};
145204
}
146205

147-
check_changed!(changed, existing.types, def.types);
148-
check_changed!(changed, existing.values, def.values);
149-
check_changed!(changed, existing.macros, def.macros);
206+
check_changed!(changed, (existing / def).types, glob_imports[lookup], def_import_type);
207+
check_changed!(changed, (existing / def).values, glob_imports[lookup], def_import_type);
208+
check_changed!(changed, (existing / def).macros, glob_imports[lookup], def_import_type);
150209

151210
changed
152211
}

‎crates/ra_hir_def/src/nameres/collector.rs

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ use test_utils::mark;
2020
use crate::{
2121
attr::Attrs,
2222
db::DefDatabase,
23+
item_scope::{ImportType, PerNsGlobImports},
2324
item_tree::{
2425
self, FileItemTreeId, ItemTree, ItemTreeId, MacroCall, Mod, ModItem, ModKind, StructDefKind,
2526
},
@@ -80,6 +81,7 @@ pub(super) fn collect_defs(db: &dyn DefDatabase, mut def_map: CrateDefMap) -> Cr
8081
mod_dirs: FxHashMap::default(),
8182
cfg_options,
8283
proc_macros,
84+
from_glob_import: Default::default(),
8385
};
8486
collector.collect();
8587
collector.finish()
@@ -186,6 +188,7 @@ struct DefCollector<'a> {
186188
mod_dirs: FxHashMap<LocalModuleId, ModDir>,
187189
cfg_options: &'a CfgOptions,
188190
proc_macros: Vec<(Name, ProcMacroExpander)>,
191+
from_glob_import: PerNsGlobImports,
189192
}
190193

191194
impl DefCollector<'_> {
@@ -305,6 +308,7 @@ impl DefCollector<'_> {
305308
self.def_map.root,
306309
&[(name, PerNs::macros(macro_, Visibility::Public))],
307310
Visibility::Public,
311+
ImportType::Named,
308312
);
309313
}
310314
}
@@ -330,6 +334,7 @@ impl DefCollector<'_> {
330334
self.def_map.root,
331335
&[(name, PerNs::macros(macro_, Visibility::Public))],
332336
Visibility::Public,
337+
ImportType::Named,
333338
);
334339
}
335340

@@ -383,7 +388,6 @@ impl DefCollector<'_> {
383388
let imports = std::mem::replace(&mut self.unresolved_imports, Vec::new());
384389
for mut directive in imports {
385390
directive.status = self.resolve_import(directive.module_id, &directive.import);
386-
387391
match directive.status {
388392
PartialResolvedImport::Indeterminate(_) => {
389393
self.record_resolved_import(&directive);
@@ -477,7 +481,7 @@ impl DefCollector<'_> {
477481
.filter(|(_, res)| !res.is_none())
478482
.collect::<Vec<_>>();
479483

480-
self.update(module_id, &items, vis);
484+
self.update(module_id, &items, vis, ImportType::Glob);
481485
} else {
482486
// glob import from same crate => we do an initial
483487
// import, and then need to propagate any further
@@ -499,7 +503,7 @@ impl DefCollector<'_> {
499503
.filter(|(_, res)| !res.is_none())
500504
.collect::<Vec<_>>();
501505

502-
self.update(module_id, &items, vis);
506+
self.update(module_id, &items, vis, ImportType::Glob);
503507
// record the glob import in case we add further items
504508
let glob = self.glob_imports.entry(m.local_id).or_default();
505509
if !glob.iter().any(|(mid, _)| *mid == module_id) {
@@ -529,7 +533,7 @@ impl DefCollector<'_> {
529533
(name, res)
530534
})
531535
.collect::<Vec<_>>();
532-
self.update(module_id, &resolutions, vis);
536+
self.update(module_id, &resolutions, vis, ImportType::Glob);
533537
}
534538
Some(d) => {
535539
log::debug!("glob import {:?} from non-module/enum {:?}", import, d);
@@ -555,15 +559,21 @@ impl DefCollector<'_> {
555559
}
556560
}
557561

558-
self.update(module_id, &[(name, def)], vis);
562+
self.update(module_id, &[(name, def)], vis, ImportType::Named);
559563
}
560564
None => mark::hit!(bogus_paths),
561565
}
562566
}
563567
}
564568

565-
fn update(&mut self, module_id: LocalModuleId, resolutions: &[(Name, PerNs)], vis: Visibility) {
566-
self.update_recursive(module_id, resolutions, vis, 0)
569+
fn update(
570+
&mut self,
571+
module_id: LocalModuleId,
572+
resolutions: &[(Name, PerNs)],
573+
vis: Visibility,
574+
import_type: ImportType,
575+
) {
576+
self.update_recursive(module_id, resolutions, vis, import_type, 0)
567577
}
568578

569579
fn update_recursive(
@@ -573,6 +583,7 @@ impl DefCollector<'_> {
573583
// All resolutions are imported with this visibility; the visibilies in
574584
// the `PerNs` values are ignored and overwritten
575585
vis: Visibility,
586+
import_type: ImportType,
576587
depth: usize,
577588
) {
578589
if depth > 100 {
@@ -582,7 +593,12 @@ impl DefCollector<'_> {
582593
let scope = &mut self.def_map.modules[module_id].scope;
583594
let mut changed = false;
584595
for (name, res) in resolutions {
585-
changed |= scope.push_res(name.clone(), res.with_visibility(vis));
596+
changed |= scope.push_res_with_import(
597+
&mut self.from_glob_import,
598+
(module_id, name.clone()),
599+
res.with_visibility(vis),
600+
import_type,
601+
);
586602
}
587603

588604
if !changed {
@@ -601,7 +617,13 @@ impl DefCollector<'_> {
601617
if !vis.is_visible_from_def_map(&self.def_map, glob_importing_module) {
602618
continue;
603619
}
604-
self.update_recursive(glob_importing_module, resolutions, glob_import_vis, depth + 1);
620+
self.update_recursive(
621+
glob_importing_module,
622+
resolutions,
623+
glob_import_vis,
624+
ImportType::Glob,
625+
depth + 1,
626+
);
605627
}
606628
}
607629

@@ -923,6 +945,7 @@ impl ModCollector<'_, '_> {
923945
self.module_id,
924946
&[(name.clone(), PerNs::from_def(id, vis, has_constructor))],
925947
vis,
948+
ImportType::Named,
926949
)
927950
}
928951
}
@@ -1025,7 +1048,12 @@ impl ModCollector<'_, '_> {
10251048
let module = ModuleId { krate: self.def_collector.def_map.krate, local_id: res };
10261049
let def: ModuleDefId = module.into();
10271050
self.def_collector.def_map.modules[self.module_id].scope.define_def(def);
1028-
self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis, false))], vis);
1051+
self.def_collector.update(
1052+
self.module_id,
1053+
&[(name, PerNs::from_def(def, vis, false))],
1054+
vis,
1055+
ImportType::Named,
1056+
);
10291057
res
10301058
}
10311059

@@ -1154,6 +1182,7 @@ mod tests {
11541182
mod_dirs: FxHashMap::default(),
11551183
cfg_options: &CfgOptions::default(),
11561184
proc_macros: Default::default(),
1185+
from_glob_import: Default::default(),
11571186
};
11581187
collector.collect();
11591188
collector.def_map

‎crates/ra_hir_def/src/nameres/tests/globs.rs

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -276,3 +276,93 @@ fn glob_shadowed_def() {
276276
"###
277277
);
278278
}
279+
280+
#[test]
281+
fn glob_shadowed_def_reversed() {
282+
let map = def_map(
283+
r###"
284+
//- /lib.rs
285+
mod foo;
286+
mod bar;
287+
288+
use bar::baz;
289+
use foo::*;
290+
291+
use baz::Bar;
292+
293+
//- /foo.rs
294+
pub mod baz {
295+
pub struct Foo;
296+
}
297+
298+
//- /bar.rs
299+
pub mod baz {
300+
pub struct Bar;
301+
}
302+
"###,
303+
);
304+
assert_snapshot!(map, @r###"
305+
⋮crate
306+
⋮Bar: t v
307+
⋮bar: t
308+
⋮baz: t
309+
⋮foo: t
310+
⋮
311+
⋮crate::bar
312+
⋮baz: t
313+
⋮
314+
⋮crate::bar::baz
315+
⋮Bar: t v
316+
⋮
317+
⋮crate::foo
318+
⋮baz: t
319+
⋮
320+
⋮crate::foo::baz
321+
⋮Foo: t v
322+
"###
323+
);
324+
}
325+
326+
#[test]
327+
fn glob_shadowed_def_dependencies() {
328+
let map = def_map(
329+
r###"
330+
//- /lib.rs
331+
mod a { pub mod foo { pub struct X; } }
332+
mod b { pub use super::a::foo; }
333+
mod c { pub mod foo { pub struct Y; } }
334+
mod d {
335+
use super::c::foo;
336+
use super::b::*;
337+
use foo::Y;
338+
}
339+
"###,
340+
);
341+
assert_snapshot!(map, @r###"
342+
⋮crate
343+
⋮a: t
344+
⋮b: t
345+
⋮c: t
346+
⋮d: t
347+
⋮
348+
⋮crate::d
349+
⋮Y: t v
350+
⋮foo: t
351+
⋮
352+
⋮crate::c
353+
⋮foo: t
354+
⋮
355+
⋮crate::c::foo
356+
⋮Y: t v
357+
⋮
358+
⋮crate::b
359+
⋮foo: t
360+
⋮
361+
⋮crate::a
362+
⋮foo: t
363+
⋮
364+
⋮crate::a::foo
365+
⋮X: t v
366+
"###
367+
);
368+
}

0 commit comments

Comments
 (0)