Revert resolution of all glob imports first, replace with tracking of glob imports and shadowing when more specific

4 files changed, 119 insertions, 49 deletions

diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs
index c81b966c3..0184b6af9 100644
--- a/crates/ra_hir_def/src/item_scope.rs
+++ b/crates/ra_hir_def/src/item_scope.rs
@@ -128,15 +128,19 @@ impl ItemScope {
         let existing = self.visible.entry(name).or_default();
 
         macro_rules! check_changed {
-            ($changed:ident, $existing:expr, $def:expr) => {
-                match ($existing, $def) {
+            ($changed:ident, ($existing:ident/$def:ident).$field:ident) => {
+                match ($existing.$field, $def.$field) {
                     (None, Some(_)) => {
-                        $existing = $def;
+                        $existing.from_glob = $def.from_glob;
+                        $existing.$field = $def.$field;
                         $changed = true;
                     }
-                    (Some(e), Some(d)) if e.0 != d.0 => {
+                    // Only update if the new def came from a specific import and the existing
+                    // import came from a glob import.
+                    (Some(_), Some(_)) if $existing.from_glob && !$def.from_glob => {
                         mark::hit!(import_shadowed);
-                        $existing = $def;
+                        $existing.from_glob = $def.from_glob;
+                        $existing.$field = $def.$field;
                         $changed = true;
                     }
                     _ => {}
@@ -144,9 +148,9 @@ impl ItemScope {
             };
         }
 
-        check_changed!(changed, existing.types, def.types);
-        check_changed!(changed, existing.values, def.values);
-        check_changed!(changed, existing.macros, def.macros);
+        check_changed!(changed, (existing / def).types);
+        check_changed!(changed, (existing / def).values);
+        check_changed!(changed, (existing / def).macros);
 
         changed
     }
diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs
index 665dd106c..93f58e2c7 100644
--- a/crates/ra_hir_def/src/nameres/collector.rs
+++ b/crates/ra_hir_def/src/nameres/collector.rs
@@ -305,6 +305,7 @@ impl DefCollector<'_> {
                 self.def_map.root,
                 &[(name, PerNs::macros(macro_, Visibility::Public))],
                 Visibility::Public,
+                false,
             );
         }
     }
@@ -330,6 +331,7 @@ impl DefCollector<'_> {
             self.def_map.root,
             &[(name, PerNs::macros(macro_, Visibility::Public))],
             Visibility::Public,
+            false,
         );
     }
 
@@ -380,35 +382,25 @@ impl DefCollector<'_> {
 
         while self.unresolved_imports.len() < n_previous_unresolved {
             n_previous_unresolved = self.unresolved_imports.len();
-            let mut imports = std::mem::replace(&mut self.unresolved_imports, Vec::new());
-            for mut directive in &mut imports {
+            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);
-            }
-
-            let (glob_imports, non_glob_imports): (Vec<_>, Vec<_>) =
-                imports.into_iter().partition(|directive| directive.import.is_glob);
-            let mut record = |imports: Vec<ImportDirective>| {
-                for directive in imports {
-                    match directive.status {
-                        PartialResolvedImport::Indeterminate(_) => {
-                            self.record_resolved_import(&directive);
-                            // FIXME: For avoid performance regression,
-                            // we consider an imported resolved if it is indeterminate (i.e not all namespace resolved)
-                            self.resolved_imports.push(directive)
-                        }
-                        PartialResolvedImport::Resolved(_) => {
-                            self.record_resolved_import(&directive);
-                            self.resolved_imports.push(directive)
-                        }
-                        PartialResolvedImport::Unresolved => {
-                            self.unresolved_imports.push(directive);
-                        }
+                match directive.status {
+                    PartialResolvedImport::Indeterminate(_) => {
+                        self.record_resolved_import(&directive);
+                        // FIXME: For avoid performance regression,
+                        // we consider an imported resolved if it is indeterminate (i.e not all namespace resolved)
+                        self.resolved_imports.push(directive)
+                    }
+                    PartialResolvedImport::Resolved(_) => {
+                        self.record_resolved_import(&directive);
+                        self.resolved_imports.push(directive)
+                    }
+                    PartialResolvedImport::Unresolved => {
+                        self.unresolved_imports.push(directive);
                     }
                 }
-            };
-
-            record(glob_imports);
-            record(non_glob_imports);
+            }
         }
     }
 
@@ -486,7 +478,7 @@ impl DefCollector<'_> {
                             .filter(|(_, res)| !res.is_none())
                             .collect::<Vec<_>>();
 
-                        self.update(module_id, &items, vis);
+                        self.update(module_id, &items, vis, true);
                     } else {
                         // glob import from same crate => we do an initial
                         // import, and then need to propagate any further
@@ -508,7 +500,7 @@ impl DefCollector<'_> {
                             .filter(|(_, res)| !res.is_none())
                             .collect::<Vec<_>>();
 
-                        self.update(module_id, &items, vis);
+                        self.update(module_id, &items, vis, true);
                         // 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) {
@@ -538,7 +530,7 @@ impl DefCollector<'_> {
                             (name, res)
                         })
                         .collect::<Vec<_>>();
-                    self.update(module_id, &resolutions, vis);
+                    self.update(module_id, &resolutions, vis, true);
                 }
                 Some(d) => {
                     log::debug!("glob import {:?} from non-module/enum {:?}", import, d);
@@ -564,15 +556,21 @@ impl DefCollector<'_> {
                         }
                     }
 
-                    self.update(module_id, &[(name, def)], vis);
+                    self.update(module_id, &[(name, def)], vis, false);
                 }
                 None => mark::hit!(bogus_paths),
             }
         }
     }
 
-    fn update(&mut self, module_id: LocalModuleId, resolutions: &[(Name, PerNs)], vis: Visibility) {
-        self.update_recursive(module_id, resolutions, vis, 0)
+    fn update(
+        &mut self,
+        module_id: LocalModuleId,
+        resolutions: &[(Name, PerNs)],
+        vis: Visibility,
+        is_from_glob: bool,
+    ) {
+        self.update_recursive(module_id, resolutions, vis, is_from_glob, 0)
     }
 
     fn update_recursive(
@@ -582,6 +580,9 @@ impl DefCollector<'_> {
         // All resolutions are imported with this visibility; the visibilies in
         // the `PerNs` values are ignored and overwritten
         vis: Visibility,
+        // All resolutions are imported with this glob status; the glob status
+        // in the `PerNs` values are ignored and overwritten
+        is_from_glob: bool,
         depth: usize,
     ) {
         if depth > 100 {
@@ -591,7 +592,8 @@ 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(name.clone(), res.with_visibility(vis));
+            changed |=
+                scope.push_res(name.clone(), res.with_visibility(vis).from_glob(is_from_glob));
         }
 
         if !changed {
@@ -610,7 +612,13 @@ impl DefCollector<'_> {
             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,
+                true,
+                depth + 1,
+            );
         }
     }
 
@@ -932,6 +940,7 @@ impl ModCollector<'_, '_> {
                         self.module_id,
                         &[(name.clone(), PerNs::from_def(id, vis, has_constructor))],
                         vis,
+                        false,
                     )
                 }
             }
@@ -1034,7 +1043,12 @@ impl ModCollector<'_, '_> {
         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,
+            false,
+        );
         res
     }
 
diff --git a/crates/ra_hir_def/src/nameres/tests/globs.rs b/crates/ra_hir_def/src/nameres/tests/globs.rs
index d5a02137c..7f3d7509c 100644
--- a/crates/ra_hir_def/src/nameres/tests/globs.rs
+++ b/crates/ra_hir_def/src/nameres/tests/globs.rs
@@ -322,3 +322,47 @@ fn glob_shadowed_def_reversed() {
     "###
     );
 }
+
+#[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
+    "###
+    );
+}
diff --git a/crates/ra_hir_def/src/per_ns.rs b/crates/ra_hir_def/src/per_ns.rs
index 74665c588..e5cbca71d 100644
--- a/crates/ra_hir_def/src/per_ns.rs
+++ b/crates/ra_hir_def/src/per_ns.rs
@@ -9,6 +9,7 @@ use crate::{item_scope::ItemInNs, visibility::Visibility, ModuleDefId};
 
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub struct PerNs {
+    pub from_glob: bool,
     pub types: Option<(ModuleDefId, Visibility)>,
     pub values: Option<(ModuleDefId, Visibility)>,
     pub macros: Option<(MacroDefId, Visibility)>,
@@ -16,29 +17,29 @@ pub struct PerNs {
 
 impl Default for PerNs {
     fn default() -> Self {
-        PerNs { types: None, values: None, macros: None }
+        PerNs { from_glob: false, types: None, values: None, macros: None }
     }
 }
 
 impl PerNs {
     pub fn none() -> PerNs {
-        PerNs { types: None, values: None, macros: None }
+        PerNs { from_glob: false, types: None, values: None, macros: None }
     }
 
     pub fn values(t: ModuleDefId, v: Visibility) -> PerNs {
-        PerNs { types: None, values: Some((t, v)), macros: None }
+        PerNs { from_glob: false, types: None, values: Some((t, v)), macros: None }
     }
 
     pub fn types(t: ModuleDefId, v: Visibility) -> PerNs {
-        PerNs { types: Some((t, v)), values: None, macros: None }
+        PerNs { from_glob: false, types: Some((t, v)), values: None, macros: None }
     }
 
     pub fn both(types: ModuleDefId, values: ModuleDefId, v: Visibility) -> PerNs {
-        PerNs { types: Some((types, v)), values: Some((values, v)), macros: None }
+        PerNs { from_glob: false, types: Some((types, v)), values: Some((values, v)), macros: None }
     }
 
     pub fn macros(macro_: MacroDefId, v: Visibility) -> PerNs {
-        PerNs { types: None, values: None, macros: Some((macro_, v)) }
+        PerNs { from_glob: false, types: None, values: None, macros: Some((macro_, v)) }
     }
 
     pub fn is_none(&self) -> bool {
@@ -63,6 +64,7 @@ impl PerNs {
 
     pub fn filter_visibility(self, mut f: impl FnMut(Visibility) -> bool) -> PerNs {
         PerNs {
+            from_glob: self.from_glob,
             types: self.types.filter(|(_, v)| f(*v)),
             values: self.values.filter(|(_, v)| f(*v)),
             macros: self.macros.filter(|(_, v)| f(*v)),
@@ -71,6 +73,7 @@ impl PerNs {
 
     pub fn with_visibility(self, vis: Visibility) -> PerNs {
         PerNs {
+            from_glob: self.from_glob,
             types: self.types.map(|(it, _)| (it, vis)),
             values: self.values.map(|(it, _)| (it, vis)),
             macros: self.macros.map(|(it, _)| (it, vis)),
@@ -79,6 +82,7 @@ impl PerNs {
 
     pub fn or(self, other: PerNs) -> PerNs {
         PerNs {
+            from_glob: self.from_glob,
             types: self.types.or(other.types),
             values: self.values.or(other.values),
             macros: self.macros.or(other.macros),
@@ -92,4 +96,8 @@ impl PerNs {
             .chain(self.values.map(|it| ItemInNs::Values(it.0)).into_iter())
             .chain(self.macros.map(|it| ItemInNs::Macros(it.0)).into_iter())
     }
+
+    pub fn from_glob(self, from_glob: bool) -> PerNs {
+        PerNs { from_glob, ..self }
+    }
 }