Take visibility into account for glob imports

7 files changed, 165 insertions, 43 deletions

diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs
index d77f37f67..db5f469c7 100644
--- a/crates/ra_hir_def/src/item_scope.rs
+++ b/crates/ra_hir_def/src/item_scope.rs
@@ -149,16 +149,8 @@ impl ItemScope {
         changed
     }
 
-    #[cfg(test)]
-    pub(crate) fn collect_resolutions(&self) -> Vec<(Name, PerNs)> {
-        self.visible.iter().map(|(name, res)| (name.clone(), res.clone())).collect()
-    }
-
-    pub(crate) fn collect_resolutions_with_vis(
-        &self,
-        vis: ResolvedVisibility,
-    ) -> Vec<(Name, PerNs)> {
-        self.visible.iter().map(|(name, res)| (name.clone(), res.with_visibility(vis))).collect()
+    pub(crate) fn resolutions<'a>(&'a self) -> impl Iterator<Item = (Name, PerNs)> + 'a {
+        self.visible.iter().map(|(name, res)| (name.clone(), res.clone()))
     }
 
     pub(crate) fn collect_legacy_macros(&self) -> FxHashMap<Name, MacroDefId> {
diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs
index 5b8478037..51df44d2f 100644
--- a/crates/ra_hir_def/src/nameres/collector.rs
+++ b/crates/ra_hir_def/src/nameres/collector.rs
@@ -109,7 +109,7 @@ struct MacroDirective {
 struct DefCollector<'a, DB> {
     db: &'a DB,
     def_map: CrateDefMap,
-    glob_imports: FxHashMap<LocalModuleId, Vec<(LocalModuleId, raw::Import)>>,
+    glob_imports: FxHashMap<LocalModuleId, Vec<(LocalModuleId, ResolvedVisibility)>>,
     unresolved_imports: Vec<ImportDirective>,
     resolved_imports: Vec<ImportDirective>,
     unexpanded_macros: Vec<MacroDirective>,
@@ -218,6 +218,7 @@ where
             self.update(
                 self.def_map.root,
                 &[(name, PerNs::macros(macro_, ResolvedVisibility::Public))],
+                ResolvedVisibility::Public,
             );
         }
     }
@@ -352,7 +353,6 @@ where
 
     fn record_resolved_import(&mut self, directive: &ImportDirective) {
         let module_id = directive.module_id;
-        let import_id = directive.import_id;
         let import = &directive.import;
         let def = directive.status.namespaces();
         let vis = self
@@ -373,28 +373,48 @@ where
                         let item_map = self.db.crate_def_map(m.krate);
                         let scope = &item_map[m.local_id].scope;
 
-                        // TODO: only use names we can see
-
                         // Module scoped macros is included
-                        let items = scope.collect_resolutions_with_vis(vis);
-
-                        self.update(module_id, &items);
+                        let items = scope
+                            .resolutions()
+                            // only keep visible names...
+                            .map(|(n, res)| {
+                                (
+                                    n,
+                                    res.filter_visibility(|v| {
+                                        v.visible_from_def_map(&self.def_map, module_id)
+                                    }),
+                                )
+                            })
+                            .filter(|(_, res)| !res.is_none())
+                            .collect::<Vec<_>>();
+
+                        self.update(module_id, &items, vis);
                     } else {
                         // glob import from same crate => we do an initial
                         // import, and then need to propagate any further
                         // additions
                         let scope = &self.def_map[m.local_id].scope;
 
-                        // TODO: only use names we can see
-
                         // Module scoped macros is included
-                        let items = scope.collect_resolutions_with_vis(vis);
-
-                        self.update(module_id, &items);
+                        let items = scope
+                            .resolutions()
+                            // only keep visible names...
+                            .map(|(n, res)| {
+                                (
+                                    n,
+                                    res.filter_visibility(|v| {
+                                        v.visible_from_def_map(&self.def_map, module_id)
+                                    }),
+                                )
+                            })
+                            .filter(|(_, res)| !res.is_none())
+                            .collect::<Vec<_>>();
+
+                        self.update(module_id, &items, vis);
                         // 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(|it| *it == (module_id, import_id)) {
-                            glob.push((module_id, import_id));
+                        if !glob.iter().any(|(mid, _)| *mid == module_id) {
+                            glob.push((module_id, vis));
                         }
                     }
                 }
@@ -412,7 +432,7 @@ where
                             (name, res)
                         })
                         .collect::<Vec<_>>();
-                    self.update(module_id, &resolutions);
+                    self.update(module_id, &resolutions, vis);
                 }
                 Some(d) => {
                     log::debug!("glob import {:?} from non-module/enum {:?}", import, d);
@@ -434,21 +454,29 @@ where
                         }
                     }
 
-                    self.update(module_id, &[(name, def.with_visibility(vis))]);
+                    self.update(module_id, &[(name, def)], vis);
                 }
                 None => tested_by!(bogus_paths),
             }
         }
     }
 
-    fn update(&mut self, module_id: LocalModuleId, resolutions: &[(Name, PerNs)]) {
-        self.update_recursive(module_id, resolutions, 0)
+    fn update(
+        &mut self,
+        module_id: LocalModuleId,
+        resolutions: &[(Name, PerNs)],
+        vis: ResolvedVisibility,
+    ) {
+        self.update_recursive(module_id, resolutions, vis, 0)
     }
 
     fn update_recursive(
         &mut self,
         module_id: LocalModuleId,
         resolutions: &[(Name, PerNs)],
+        // All resolutions are imported with this visibility; the visibilies in
+        // the `PerNs` values are ignored and overwritten
+        vis: ResolvedVisibility,
         depth: usize,
     ) {
         if depth > 100 {
@@ -458,7 +486,7 @@ where
         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);
+            changed |= scope.push_res(name.clone(), res.with_visibility(vis));
         }
 
         if !changed {
@@ -471,9 +499,13 @@ where
             .flat_map(|v| v.iter())
             .cloned()
             .collect::<Vec<_>>();
-        for (glob_importing_module, _glob_import) in glob_imports {
-            // We pass the glob import so that the tracked import in those modules is that glob import
-            self.update_recursive(glob_importing_module, resolutions, depth + 1);
+        for (glob_importing_module, glob_import_vis) in glob_imports {
+            // we know all resolutions have the same visibility (`vis`), so we
+            // just need to check that once
+            if !vis.visible_from_def_map(&self.def_map, glob_importing_module) {
+                continue;
+            }
+            self.update_recursive(glob_importing_module, resolutions, glob_import_vis, depth + 1);
         }
     }
 
@@ -715,7 +747,7 @@ where
         let def: ModuleDefId = module.into();
         let vis = ResolvedVisibility::Public; // TODO handle module visibility
         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))]);
+        self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))], vis);
         res
     }
 
@@ -780,7 +812,7 @@ where
             .def_map
             .resolve_visibility(self.def_collector.db, self.module_id, vis)
             .unwrap_or(ResolvedVisibility::Public);
-        self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))])
+        self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))], vis)
     }
 
     fn collect_derives(&mut self, attrs: &Attrs, def: &raw::DefData) {
diff --git a/crates/ra_hir_def/src/nameres/tests.rs b/crates/ra_hir_def/src/nameres/tests.rs
index ff474b53b..78bcdc850 100644
--- a/crates/ra_hir_def/src/nameres/tests.rs
+++ b/crates/ra_hir_def/src/nameres/tests.rs
@@ -12,8 +12,8 @@ use test_utils::covers;
 
 use crate::{db::DefDatabase, nameres::*, test_db::TestDB, LocalModuleId};
 
-fn def_map(fixtute: &str) -> String {
-    let dm = compute_crate_def_map(fixtute);
+fn def_map(fixture: &str) -> String {
+    let dm = compute_crate_def_map(fixture);
     render_crate_def_map(&dm)
 }
 
@@ -32,7 +32,7 @@ fn render_crate_def_map(map: &CrateDefMap) -> String {
         *buf += path;
         *buf += "\n";
 
-        let mut entries = map.modules[module].scope.collect_resolutions();
+        let mut entries: Vec<_> = map.modules[module].scope.resolutions().collect();
         entries.sort_by_key(|(name, _)| name.clone());
 
         for (name, def) in entries {
diff --git a/crates/ra_hir_def/src/nameres/tests/globs.rs b/crates/ra_hir_def/src/nameres/tests/globs.rs
index 5e24cb94d..5f137d3a6 100644
--- a/crates/ra_hir_def/src/nameres/tests/globs.rs
+++ b/crates/ra_hir_def/src/nameres/tests/globs.rs
@@ -74,6 +74,84 @@ fn glob_2() {
 }
 
 #[test]
+fn glob_privacy_1() {
+    let map = def_map(
+        "
+        //- /lib.rs
+        mod foo;
+        use foo::*;
+
+        //- /foo/mod.rs
+        pub mod bar;
+        pub use self::bar::*;
+        struct PrivateStructFoo;
+
+        //- /foo/bar.rs
+        pub struct Baz;
+        struct PrivateStructBar;
+        pub use super::*;
+        ",
+    );
+    assert_snapshot!(map, @r###"
+    crate
+    Baz: t v
+    bar: t
+    foo: t
+    
+    crate::foo
+    Baz: t v
+    PrivateStructFoo: t v
+    bar: t
+    
+    crate::foo::bar
+    Baz: t v
+    PrivateStructBar: t v
+    PrivateStructFoo: t v
+    bar: t
+    "###
+    );
+}
+
+#[test]
+fn glob_privacy_2() {
+    let map = def_map(
+        "
+        //- /lib.rs
+        mod foo;
+        use foo::*;
+        use foo::bar::*;
+
+        //- /foo/mod.rs
+        pub mod bar;
+        fn Foo() {};
+        pub struct Foo {};
+
+        //- /foo/bar.rs
+        pub(super) struct PrivateBaz;
+        struct PrivateBar;
+        pub(crate) struct PubCrateStruct;
+        ",
+    );
+    assert_snapshot!(map, @r###"
+    crate
+    Foo: t
+    PubCrateStruct: t v
+    bar: t
+    foo: t
+    
+    crate::foo
+    Foo: t v
+    bar: t
+    
+    crate::foo::bar
+    PrivateBar: t v
+    PrivateBaz: t v
+    PubCrateStruct: t v
+    "###
+    );
+}
+
+#[test]
 fn glob_across_crates() {
     covers!(glob_across_crates);
     let map = def_map(
diff --git a/crates/ra_hir_def/src/nameres/tests/incremental.rs b/crates/ra_hir_def/src/nameres/tests/incremental.rs
index ef2e9435c..faeb7aa4d 100644
--- a/crates/ra_hir_def/src/nameres/tests/incremental.rs
+++ b/crates/ra_hir_def/src/nameres/tests/incremental.rs
@@ -116,7 +116,7 @@ fn typing_inside_a_macro_should_not_invalidate_def_map() {
         let events = db.log_executed(|| {
             let crate_def_map = db.crate_def_map(krate);
             let (_, module_data) = crate_def_map.modules.iter().last().unwrap();
-            assert_eq!(module_data.scope.collect_resolutions().len(), 1);
+            assert_eq!(module_data.scope.resolutions().collect::<Vec<_>>().len(), 1);
         });
         assert!(format!("{:?}", events).contains("crate_def_map"), "{:#?}", events)
     }
@@ -126,7 +126,7 @@ fn typing_inside_a_macro_should_not_invalidate_def_map() {
         let events = db.log_executed(|| {
             let crate_def_map = db.crate_def_map(krate);
             let (_, module_data) = crate_def_map.modules.iter().last().unwrap();
-            assert_eq!(module_data.scope.collect_resolutions().len(), 1);
+            assert_eq!(module_data.scope.resolutions().collect::<Vec<_>>().len(), 1);
         });
         assert!(!format!("{:?}", events).contains("crate_def_map"), "{:#?}", events)
     }
diff --git a/crates/ra_hir_def/src/per_ns.rs b/crates/ra_hir_def/src/per_ns.rs
index 16e61a6a5..7637d8a37 100644
--- a/crates/ra_hir_def/src/per_ns.rs
+++ b/crates/ra_hir_def/src/per_ns.rs
@@ -61,6 +61,14 @@ impl PerNs {
         self.macros.map(|it| it.0)
     }
 
+    pub fn filter_visibility(self, mut f: impl FnMut(ResolvedVisibility) -> bool) -> PerNs {
+        PerNs {
+            types: self.types.filter(|(_, v)| f(*v)),
+            values: self.values.filter(|(_, v)| f(*v)),
+            macros: self.macros.filter(|(_, v)| f(*v)),
+        }
+    }
+
     pub fn with_visibility(self, vis: ResolvedVisibility) -> PerNs {
         PerNs {
             types: self.types.map(|(it, _)| (it, vis)),
diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs
index 8cac52630..990b2975c 100644
--- a/crates/ra_hir_def/src/visibility.rs
+++ b/crates/ra_hir_def/src/visibility.rs
@@ -134,13 +134,25 @@ impl ResolvedVisibility {
         if from_module.krate != to_module.krate {
             return false;
         }
-        // from_module needs to be a descendant of to_module
         let def_map = db.crate_def_map(from_module.krate);
+        self.visible_from_def_map(&def_map, from_module.local_id)
+    }
+
+    pub(crate) fn visible_from_def_map(
+        self,
+        def_map: &crate::nameres::CrateDefMap,
+        from_module: crate::LocalModuleId,
+    ) -> bool {
+        let to_module = match self {
+            ResolvedVisibility::Module(m) => m,
+            ResolvedVisibility::Public => return true,
+        };
+        // from_module needs to be a descendant of to_module
         let mut ancestors = std::iter::successors(Some(from_module), |m| {
-            let parent_id = def_map[m.local_id].parent?;
-            Some(ModuleId { local_id: parent_id, ..*m })
+            let parent_id = def_map[*m].parent?;
+            Some(parent_id)
         });
-        ancestors.any(|m| m == to_module)
+        ancestors.any(|m| m == to_module.local_id)
     }
 }