Split glob import map to per-ns, switch ExprCollector to use a simpler push_res

4 files changed, 110 insertions, 33 deletions

diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs
index d749c828d..3ced648e5 100644
--- a/crates/ra_hir_def/src/body/lower.rs
+++ b/crates/ra_hir_def/src/body/lower.rs
@@ -26,7 +26,7 @@ use crate::{
         dummy_expr_id, ArithOp, Array, BinaryOp, BindingAnnotation, CmpOp, Expr, ExprId, Literal,
         LogicOp, MatchArm, Ordering, Pat, PatId, RecordFieldPat, RecordLitField, Statement,
     },
-    item_scope::{BuiltinShadowMode, ImportType},
+    item_scope::BuiltinShadowMode,
     item_tree::{FileItemTreeId, ItemTree, ItemTreeNode},
     path::{GenericArgs, Path},
     type_ref::{Mutability, Rawness, TypeRef},
@@ -81,7 +81,6 @@ pub(super) fn lower(
             map
         },
         expander,
-        import_types: FxHashMap::default(),
     }
     .collect(params, body)
 }
@@ -94,7 +93,6 @@ struct ExprCollector<'a> {
     source_map: BodySourceMap,
 
     item_trees: FxHashMap<HirFileId, Arc<ItemTree>>,
-    import_types: FxHashMap<Name, ImportType>,
 }
 
 impl ExprCollector<'_> {
@@ -713,10 +711,8 @@ impl ExprCollector<'_> {
                     _ => true,
                 };
                 self.body.item_scope.push_res(
-                    &mut self.import_types,
                     name.as_name(),
                     crate::per_ns::PerNs::from_def(def, vis, has_constructor),
-                    ImportType::Named,
                 );
             }
         }
diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs
index 511c08a8d..d0923df6d 100644
--- a/crates/ra_hir_def/src/item_scope.rs
+++ b/crates/ra_hir_def/src/item_scope.rs
@@ -4,12 +4,12 @@
 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)]
@@ -19,13 +19,6 @@ pub(crate) enum ImportType {
 }
 
 impl ImportType {
-    fn is_glob(&self) -> bool {
-        match self {
-            ImportType::Glob => true,
-            ImportType::Named => false,
-        }
-    }
-
     fn is_named(&self) -> bool {
         match self {
             ImportType::Glob => false,
@@ -34,6 +27,13 @@ impl ImportType {
     }
 }
 
+#[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>,
@@ -145,30 +145,65 @@ impl ItemScope {
         self.legacy_macros.insert(name, mac);
     }
 
-    pub(crate) fn push_res(
+    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,
-        existing_import_map: &mut FxHashMap<Name, ImportType>,
-        name: Name,
+        glob_imports: &mut PerNsGlobImports,
+        lookup: (LocalModuleId, Name),
         def: PerNs,
         def_import_type: ImportType,
     ) -> bool {
         let mut changed = false;
-        let existing = self.visible.entry(name.clone()).or_default();
-        let existing_import_type = existing_import_map.entry(name).or_insert(def_import_type);
+        let existing = self.visible.entry(lookup.1.clone()).or_default();
 
         macro_rules! check_changed {
-            ($changed:ident, ($existing:ident/$def:ident).$field:ident, $existing_import_type:ident, $def_import_type:ident) => {
+            (
+                $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_import_type = $def_import_type;
+                        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(_), Some(_))
-                        if $existing_import_type.is_glob() && $def_import_type.is_named() =>
+                        if $glob_imports.$field.contains(&$lookup)
+                            && $def_import_type.is_named() =>
                     {
                         mark::hit!(import_shadowed);
-                        *$existing_import_type = $def_import_type;
+                        $glob_imports.$field.remove(&$lookup);
                         $existing.$field = $def.$field;
                         $changed = true;
                     }
@@ -177,9 +212,9 @@ impl ItemScope {
             };
         }
 
-        check_changed!(changed, (existing / def).types, existing_import_type, def_import_type);
-        check_changed!(changed, (existing / def).values, existing_import_type, def_import_type);
-        check_changed!(changed, (existing / def).macros, existing_import_type, def_import_type);
+        check_changed!(changed, (existing / def).types, glob_imports[lookup], def_import_type);
+        check_changed!(changed, (existing / def).values, glob_imports[lookup], def_import_type);
+        check_changed!(changed, (existing / def).macros, glob_imports[lookup], def_import_type);
 
         changed
     }
diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs
index f7b99e0be..e74f67b3d 100644
--- a/crates/ra_hir_def/src/nameres/collector.rs
+++ b/crates/ra_hir_def/src/nameres/collector.rs
@@ -20,7 +20,7 @@ use test_utils::mark;
 use crate::{
     attr::Attrs,
     db::DefDatabase,
-    item_scope::ImportType,
+    item_scope::{ImportType, PerNsGlobImports},
     item_tree::{
         self, FileItemTreeId, ItemTree, ItemTreeId, MacroCall, Mod, ModItem, ModKind, StructDefKind,
     },
@@ -81,7 +81,7 @@ pub(super) fn collect_defs(db: &dyn DefDatabase, mut def_map: CrateDefMap) -> Cr
         mod_dirs: FxHashMap::default(),
         cfg_options,
         proc_macros,
-        import_types: FxHashMap::default(),
+        from_glob_import: Default::default(),
     };
     collector.collect();
     collector.finish()
@@ -188,7 +188,7 @@ struct DefCollector<'a> {
     mod_dirs: FxHashMap<LocalModuleId, ModDir>,
     cfg_options: &'a CfgOptions,
     proc_macros: Vec<(Name, ProcMacroExpander)>,
-    import_types: FxHashMap<Name, ImportType>,
+    from_glob_import: PerNsGlobImports,
 }
 
 impl DefCollector<'_> {
@@ -595,9 +595,9 @@ impl DefCollector<'_> {
         let scope = &mut self.def_map.modules[module_id].scope;
         let mut changed = false;
         for (name, res) in resolutions {
-            changed |= scope.push_res(
-                &mut self.import_types,
-                name.clone(),
+            changed |= scope.push_res_with_import(
+                &mut self.from_glob_import,
+                (module_id, name.clone()),
                 res.with_visibility(vis),
                 import_type,
             );
@@ -1184,7 +1184,7 @@ mod tests {
             mod_dirs: FxHashMap::default(),
             cfg_options: &CfgOptions::default(),
             proc_macros: Default::default(),
-            import_types: FxHashMap::default(),
+            from_glob_import: Default::default(),
         };
         collector.collect();
         collector.def_map
diff --git a/crates/ra_hir_ty/src/tests/simple.rs b/crates/ra_hir_ty/src/tests/simple.rs
index d7ef9add6..7d8197f8b 100644
--- a/crates/ra_hir_ty/src/tests/simple.rs
+++ b/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!(