Merge #5033

5033: Order of glob imports should not affect import shadowing r=Nashenas88 a=Nashenas88

Fixes #5032

Co-authored-by: Paul Daniel Faria <Nashenas88@users.noreply.github.com>

crates/ra_hir_def/src/item_scope.rs

@@ -4,14 +4,27 @@
 use hir_expand::name::Name;
 use once_cell::sync::Lazy;
 use ra_db::CrateId;
-use rustc_hash::FxHashMap;
+use rustc_hash::{FxHashMap, FxHashSet};
 use test_utils::mark;
 
 use crate::{
     db::DefDatabase, per_ns::PerNs, visibility::Visibility, AdtId, BuiltinType, HasModule, ImplId,
-    Lookup, MacroDefId, ModuleDefId, TraitId,
+    LocalModuleId, Lookup, MacroDefId, ModuleDefId, TraitId,
 };
 
+#[derive(Copy, Clone)]
+pub(crate) enum ImportType {
+    Glob,
+    Named,
+}
+
+#[derive(Debug, Default)]
+pub struct PerNsGlobImports {
+    types: FxHashSet<(LocalModuleId, Name)>,
+    values: FxHashSet<(LocalModuleId, Name)>,
+    macros: FxHashSet<(LocalModuleId, Name)>,
+}
+
 #[derive(Debug, Default, PartialEq, Eq)]
 pub struct ItemScope {
     visible: FxHashMap<Name, PerNs>,
@@ -127,16 +140,62 @@ pub(crate) fn push_res(&mut self, name: Name, def: PerNs) -> bool {
         let mut changed = false;
         let existing = self.visible.entry(name).or_default();
 
+        if existing.types.is_none() && def.types.is_some() {
+            existing.types = def.types;
+            changed = true;
+        }
+
+        if existing.values.is_none() && def.values.is_some() {
+            existing.values = def.values;
+            changed = true;
+        }
+
+        if existing.macros.is_none() && def.macros.is_some() {
+            existing.macros = def.macros;
+            changed = true;
+        }
+
+        changed
+    }
+
+    pub(crate) fn push_res_with_import(
+        &mut self,
+        glob_imports: &mut PerNsGlobImports,
+        lookup: (LocalModuleId, Name),
+        def: PerNs,
+        def_import_type: ImportType,
+    ) -> bool {
+        let mut changed = false;
+        let existing = self.visible.entry(lookup.1.clone()).or_default();
+
         macro_rules! check_changed {
-            ($changed:ident, $existing:expr, $def:expr) => {
+            (
-                match ($existing, $def) {
+                $changed:ident,
+                ( $existing:ident / $def:ident ) . $field:ident,
+                $glob_imports:ident [ $lookup:ident ],
+                $def_import_type:ident
+            ) => {
+                match ($existing.$field, $def.$field) {
                     (None, Some(_)) => {
-                        $existing = $def;
+                        match $def_import_type {
+                            ImportType::Glob => {
+                                $glob_imports.$field.insert($lookup.clone());
+                            }
+                            ImportType::Named => {
+                                $glob_imports.$field.remove(&$lookup);
+                            }
+                        }
+
+                        $existing.$field = $def.$field;
                         $changed = true;
                     }
-                    (Some(e), Some(d)) if e.0 != d.0 => {
+                    (Some(_), Some(_))
+                        if $glob_imports.$field.contains(&$lookup)
+                            && matches!($def_import_type, ImportType::Named) =>
+                    {
                         mark::hit!(import_shadowed);
-                        $existing = $def;
+                        $glob_imports.$field.remove(&$lookup);
+                        $existing.$field = $def.$field;
                         $changed = true;
                     }
                     _ => {}
@@ -144,9 +203,9 @@ macro_rules! check_changed {
             };
         }
 
-        check_changed!(changed, existing.types, def.types);
+        check_changed!(changed, (existing / def).types, glob_imports[lookup], def_import_type);
-        check_changed!(changed, existing.values, def.values);
+        check_changed!(changed, (existing / def).values, glob_imports[lookup], def_import_type);
-        check_changed!(changed, existing.macros, def.macros);
+        check_changed!(changed, (existing / def).macros, glob_imports[lookup], def_import_type);
 
         changed
     }

crates/ra_hir_def/src/nameres/collector.rs

@@ -20,6 +20,7 @@
 use crate::{
     attr::Attrs,
     db::DefDatabase,
+    item_scope::{ImportType, PerNsGlobImports},
     item_tree::{
         self, FileItemTreeId, ItemTree, ItemTreeId, MacroCall, Mod, ModItem, ModKind, StructDefKind,
     },
@@ -80,6 +81,7 @@ pub(super) fn collect_defs(db: &dyn DefDatabase, mut def_map: CrateDefMap) -> Cr
         mod_dirs: FxHashMap::default(),
         cfg_options,
         proc_macros,
+        from_glob_import: Default::default(),
     };
     collector.collect();
     collector.finish()
@@ -186,6 +188,7 @@ struct DefCollector<'a> {
     mod_dirs: FxHashMap<LocalModuleId, ModDir>,
     cfg_options: &'a CfgOptions,
     proc_macros: Vec<(Name, ProcMacroExpander)>,
+    from_glob_import: PerNsGlobImports,
 }
 
 impl DefCollector<'_> {
@@ -305,6 +308,7 @@ fn define_macro(
                 self.def_map.root,
                 &[(name, PerNs::macros(macro_, Visibility::Public))],
                 Visibility::Public,
+                ImportType::Named,
             );
         }
     }
@@ -330,6 +334,7 @@ fn define_proc_macro(&mut self, name: Name, macro_: MacroDefId) {
             self.def_map.root,
             &[(name, PerNs::macros(macro_, Visibility::Public))],
             Visibility::Public,
+            ImportType::Named,
         );
     }
 
@@ -383,7 +388,6 @@ fn resolve_imports(&mut self) {
             let imports = std::mem::replace(&mut self.unresolved_imports, Vec::new());
             for mut directive in imports {
                 directive.status = self.resolve_import(directive.module_id, &directive.import);
-
                 match directive.status {
                     PartialResolvedImport::Indeterminate(_) => {
                         self.record_resolved_import(&directive);
@@ -477,7 +481,7 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) {
                             .filter(|(_, res)| !res.is_none())
                             .collect::<Vec<_>>();
 
-                        self.update(module_id, &items, vis);
+                        self.update(module_id, &items, vis, ImportType::Glob);
                     } else {
                         // glob import from same crate => we do an initial
                         // import, and then need to propagate any further
@@ -499,7 +503,7 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) {
                             .filter(|(_, res)| !res.is_none())
                             .collect::<Vec<_>>();
 
-                        self.update(module_id, &items, vis);
+                        self.update(module_id, &items, vis, ImportType::Glob);
                         // record the glob import in case we add further items
                         let glob = self.glob_imports.entry(m.local_id).or_default();
                         if !glob.iter().any(|(mid, _)| *mid == module_id) {
@@ -529,7 +533,7 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) {
                             (name, res)
                         })
                         .collect::<Vec<_>>();
-                    self.update(module_id, &resolutions, vis);
+                    self.update(module_id, &resolutions, vis, ImportType::Glob);
                 }
                 Some(d) => {
                     log::debug!("glob import {:?} from non-module/enum {:?}", import, d);
@@ -555,15 +559,21 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) {
                         }
                     }
 
-                    self.update(module_id, &[(name, def)], vis);
+                    self.update(module_id, &[(name, def)], vis, ImportType::Named);
                 }
                 None => mark::hit!(bogus_paths),
             }
         }
     }
 
-    fn update(&mut self, module_id: LocalModuleId, resolutions: &[(Name, PerNs)], vis: Visibility) {
+    fn update(
-        self.update_recursive(module_id, resolutions, vis, 0)
+        &mut self,
+        module_id: LocalModuleId,
+        resolutions: &[(Name, PerNs)],
+        vis: Visibility,
+        import_type: ImportType,
+    ) {
+        self.update_recursive(module_id, resolutions, vis, import_type, 0)
     }
 
     fn update_recursive(
@@ -573,6 +583,7 @@ fn update_recursive(
         // All resolutions are imported with this visibility; the visibilies in
         // the `PerNs` values are ignored and overwritten
         vis: Visibility,
+        import_type: ImportType,
         depth: usize,
     ) {
         if depth > 100 {
@@ -582,7 +593,12 @@ fn update_recursive(
         let scope = &mut self.def_map.modules[module_id].scope;
         let mut changed = false;
         for (name, res) in resolutions {
-            changed |= scope.push_res(name.clone(), res.with_visibility(vis));
+            changed |= scope.push_res_with_import(
+                &mut self.from_glob_import,
+                (module_id, name.clone()),
+                res.with_visibility(vis),
+                import_type,
+            );
         }
 
         if !changed {
@@ -601,7 +617,13 @@ fn update_recursive(
             if !vis.is_visible_from_def_map(&self.def_map, glob_importing_module) {
                 continue;
             }
-            self.update_recursive(glob_importing_module, resolutions, glob_import_vis, depth + 1);
+            self.update_recursive(
+                glob_importing_module,
+                resolutions,
+                glob_import_vis,
+                ImportType::Glob,
+                depth + 1,
+            );
         }
     }
 
@@ -923,6 +945,7 @@ fn collect(&mut self, items: &[ModItem]) {
                         self.module_id,
                         &[(name.clone(), PerNs::from_def(id, vis, has_constructor))],
                         vis,
+                        ImportType::Named,
                     )
                 }
             }
@@ -1025,7 +1048,12 @@ fn push_child_module(
         let module = ModuleId { krate: self.def_collector.def_map.krate, local_id: res };
         let def: ModuleDefId = module.into();
         self.def_collector.def_map.modules[self.module_id].scope.define_def(def);
-        self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis, false))], vis);
+        self.def_collector.update(
+            self.module_id,
+            &[(name, PerNs::from_def(def, vis, false))],
+            vis,
+            ImportType::Named,
+        );
         res
     }
 
@@ -1154,6 +1182,7 @@ fn do_collect_defs(db: &dyn DefDatabase, def_map: CrateDefMap) -> CrateDefMap {
             mod_dirs: FxHashMap::default(),
             cfg_options: &CfgOptions::default(),
             proc_macros: Default::default(),
+            from_glob_import: Default::default(),
         };
         collector.collect();
         collector.def_map

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

@@ -276,3 +276,93 @@ pub mod baz {
     "###
     );
 }
+
+#[test]
+fn glob_shadowed_def_reversed() {
+    let map = def_map(
+        r###"
+        //- /lib.rs
+        mod foo;
+        mod bar;
+
+        use bar::baz;
+        use foo::*;
+
+        use baz::Bar;
+
+        //- /foo.rs
+        pub mod baz {
+            pub struct Foo;
+        }
+
+        //- /bar.rs
+        pub mod baz {
+            pub struct Bar;
+        }
+        "###,
+    );
+    assert_snapshot!(map, @r###"
+        ⋮crate
+        ⋮Bar: t v
+        ⋮bar: t
+        ⋮baz: t
+        ⋮foo: t
+        ⋮
+        ⋮crate::bar
+        ⋮baz: t
+        ⋮
+        ⋮crate::bar::baz
+        ⋮Bar: t v
+        ⋮
+        ⋮crate::foo
+        ⋮baz: t
+        ⋮
+        ⋮crate::foo::baz
+        ⋮Foo: t v
+    "###
+    );
+}
+
+#[test]
+fn glob_shadowed_def_dependencies() {
+    let map = def_map(
+        r###"
+        //- /lib.rs
+        mod a { pub mod foo { pub struct X; } }
+        mod b { pub use super::a::foo; }
+        mod c { pub mod foo { pub struct Y; } }
+        mod d {
+            use super::c::foo;
+            use super::b::*;
+            use foo::Y;
+        }
+        "###,
+    );
+    assert_snapshot!(map, @r###"
+        ⋮crate
+        ⋮a: t
+        ⋮b: t
+        ⋮c: t
+        ⋮d: t
+        ⋮
+        ⋮crate::d
+        ⋮Y: t v
+        ⋮foo: t
+        ⋮
+        ⋮crate::c
+        ⋮foo: t
+        ⋮
+        ⋮crate::c::foo
+        ⋮Y: t v
+        ⋮
+        ⋮crate::b
+        ⋮foo: t
+        ⋮
+        ⋮crate::a
+        ⋮foo: t
+        ⋮
+        ⋮crate::a::foo
+        ⋮X: t v
+    "###
+    );
+}

crates/ra_hir_ty/src/tests/simple.rs

@@ -1739,6 +1739,52 @@ fn main() {
     assert_eq!(t, "u32");
 }
 
+// This test is actually testing the shadowing behavior within ra_hir_def. It
+// lives here because the testing infrastructure in ra_hir_def isn't currently
+// capable of asserting the necessary conditions.
+#[test]
+fn should_be_shadowing_imports() {
+    let t = type_at(
+        r#"
+mod a {
+    pub fn foo() -> i8 {0}
+    pub struct foo { a: i8 }
+}
+mod b { pub fn foo () -> u8 {0} }
+mod c { pub struct foo { a: u8 } }
+mod d {
+    pub use super::a::*;
+    pub use super::c::foo;
+    pub use super::b::foo;
+}
+
+fn main() {
+    d::foo()<|>;
+}"#,
+    );
+    assert_eq!(t, "u8");
+
+    let t = type_at(
+        r#"
+mod a {
+    pub fn foo() -> i8 {0}
+    pub struct foo { a: i8 }
+}
+mod b { pub fn foo () -> u8 {0} }
+mod c { pub struct foo { a: u8 } }
+mod d {
+    pub use super::a::*;
+    pub use super::c::foo;
+    pub use super::b::foo;
+}
+
+fn main() {
+    d::foo{a:0<|>};
+}"#,
+    );
+    assert_eq!(t, "u8");
+}
+
 #[test]
 fn closure_return() {
     assert_snapshot!(