Merge #2667

2667: Visibility r=matklad a=flodiebold

This adds the infrastructure for handling visibility (for fields and methods, not in name resolution) in the HIR and code model, and as a first application hides struct fields from completions if they're not visible from the current module. (We might want to relax this again later, but I think it's ok for now?)

Co-authored-by: Florian Diebold <[email protected]>

20 files changed, 608 insertions, 113 deletions

diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs
index 76d8f85f1..488f74cfb 100644
--- a/crates/ra_hir/src/code_model.rs
+++ b/crates/ra_hir/src/code_model.rs
@@ -118,7 +118,7 @@ impl_froms!(
     BuiltinType
 );
 
-pub use hir_def::attr::Attrs;
+pub use hir_def::{attr::Attrs, visibility::Visibility};
 
 impl Module {
     pub(crate) fn new(krate: Crate, crate_module_id: LocalModuleId) -> Module {
@@ -255,6 +255,15 @@ impl StructField {
     }
 }
 
+impl HasVisibility for StructField {
+    fn visibility(&self, db: &impl HirDatabase) -> Visibility {
+        let variant_data = self.parent.variant_data(db);
+        let visibility = &variant_data.fields()[self.id].visibility;
+        let parent_id: hir_def::VariantId = self.parent.into();
+        visibility.resolve(db, &parent_id.resolver(db))
+    }
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct Struct {
     pub(crate) id: StructId,
@@ -1041,3 +1050,11 @@ impl<T: Into<AttrDef> + Copy> Docs for T {
         db.documentation(def.into())
     }
 }
+
+pub trait HasVisibility {
+    fn visibility(&self, db: &impl HirDatabase) -> Visibility;
+    fn is_visible_from(&self, db: &impl HirDatabase, module: Module) -> bool {
+        let vis = self.visibility(db);
+        vis.is_visible_from(db, module.id)
+    }
+}
diff --git a/crates/ra_hir/src/lib.rs b/crates/ra_hir/src/lib.rs
index 0008a8858..3d13978d4 100644
--- a/crates/ra_hir/src/lib.rs
+++ b/crates/ra_hir/src/lib.rs
@@ -40,8 +40,8 @@ mod from_source;
 pub use crate::{
     code_model::{
         Adt, AssocItem, AttrDef, Const, Crate, CrateDependency, DefWithBody, Docs, Enum,
-        EnumVariant, FieldSource, Function, GenericDef, HasAttrs, ImplBlock, Local, MacroDef,
-        Module, ModuleDef, ScopeDef, Static, Struct, StructField, Trait, Type, TypeAlias,
+        EnumVariant, FieldSource, Function, GenericDef, HasAttrs, HasVisibility, ImplBlock, Local,
+        MacroDef, Module, ModuleDef, ScopeDef, Static, Struct, StructField, Trait, Type, TypeAlias,
         TypeParam, Union, VariantDef,
     },
     from_source::FromSource,
diff --git a/crates/ra_hir_def/src/adt.rs b/crates/ra_hir_def/src/adt.rs
index d9ea693e3..aac5f3e15 100644
--- a/crates/ra_hir_def/src/adt.rs
+++ b/crates/ra_hir_def/src/adt.rs
@@ -9,11 +9,12 @@ use hir_expand::{
 };
 use ra_arena::{map::ArenaMap, Arena};
 use ra_prof::profile;
-use ra_syntax::ast::{self, NameOwner, TypeAscriptionOwner};
+use ra_syntax::ast::{self, NameOwner, TypeAscriptionOwner, VisibilityOwner};
 
 use crate::{
-    db::DefDatabase, src::HasChildSource, src::HasSource, trace::Trace, type_ref::TypeRef, EnumId,
-    LocalEnumVariantId, LocalStructFieldId, Lookup, StructId, UnionId, VariantId,
+    db::DefDatabase, src::HasChildSource, src::HasSource, trace::Trace, type_ref::TypeRef,
+    visibility::RawVisibility, EnumId, LocalEnumVariantId, LocalStructFieldId, Lookup, StructId,
+    UnionId, VariantId,
 };
 
 /// Note that we use `StructData` for unions as well!
@@ -47,13 +48,14 @@ pub enum VariantData {
 pub struct StructFieldData {
     pub name: Name,
     pub type_ref: TypeRef,
+    pub visibility: RawVisibility,
 }
 
 impl StructData {
     pub(crate) fn struct_data_query(db: &impl DefDatabase, id: StructId) -> Arc<StructData> {
         let src = id.lookup(db).source(db);
         let name = src.value.name().map_or_else(Name::missing, |n| n.as_name());
-        let variant_data = VariantData::new(src.value.kind());
+        let variant_data = VariantData::new(db, src.map(|s| s.kind()));
         let variant_data = Arc::new(variant_data);
         Arc::new(StructData { name, variant_data })
     }
@@ -61,10 +63,12 @@ impl StructData {
         let src = id.lookup(db).source(db);
         let name = src.value.name().map_or_else(Name::missing, |n| n.as_name());
         let variant_data = VariantData::new(
-            src.value
-                .record_field_def_list()
-                .map(ast::StructKind::Record)
-                .unwrap_or(ast::StructKind::Unit),
+            db,
+            src.map(|s| {
+                s.record_field_def_list()
+                    .map(ast::StructKind::Record)
+                    .unwrap_or(ast::StructKind::Unit)
+            }),
         );
         let variant_data = Arc::new(variant_data);
         Arc::new(StructData { name, variant_data })
@@ -77,7 +81,7 @@ impl EnumData {
         let src = e.lookup(db).source(db);
         let name = src.value.name().map_or_else(Name::missing, |n| n.as_name());
         let mut trace = Trace::new_for_arena();
-        lower_enum(&mut trace, &src.value);
+        lower_enum(db, &mut trace, &src);
         Arc::new(EnumData { name, variants: trace.into_arena() })
     }
 
@@ -93,30 +97,31 @@ impl HasChildSource for EnumId {
     fn child_source(&self, db: &impl DefDatabase) -> InFile<ArenaMap<Self::ChildId, Self::Value>> {
         let src = self.lookup(db).source(db);
         let mut trace = Trace::new_for_map();
-        lower_enum(&mut trace, &src.value);
+        lower_enum(db, &mut trace, &src);
         src.with_value(trace.into_map())
     }
 }
 
 fn lower_enum(
+    db: &impl DefDatabase,
     trace: &mut Trace<LocalEnumVariantId, EnumVariantData, ast::EnumVariant>,
-    ast: &ast::EnumDef,
+    ast: &InFile<ast::EnumDef>,
 ) {
-    for var in ast.variant_list().into_iter().flat_map(|it| it.variants()) {
+    for var in ast.value.variant_list().into_iter().flat_map(|it| it.variants()) {
         trace.alloc(
             || var.clone(),
             || EnumVariantData {
                 name: var.name().map_or_else(Name::missing, |it| it.as_name()),
-                variant_data: Arc::new(VariantData::new(var.kind())),
+                variant_data: Arc::new(VariantData::new(db, ast.with_value(var.kind()))),
             },
         );
     }
 }
 
 impl VariantData {
-    fn new(flavor: ast::StructKind) -> Self {
+    fn new(db: &impl DefDatabase, flavor: InFile<ast::StructKind>) -> Self {
         let mut trace = Trace::new_for_arena();
-        match lower_struct(&mut trace, &flavor) {
+        match lower_struct(db, &mut trace, &flavor) {
             StructKind::Tuple => VariantData::Tuple(trace.into_arena()),
             StructKind::Record => VariantData::Record(trace.into_arena()),
             StructKind::Unit => VariantData::Unit,
@@ -163,7 +168,7 @@ impl HasChildSource for VariantId {
             }),
         };
         let mut trace = Trace::new_for_map();
-        lower_struct(&mut trace, &src.value);
+        lower_struct(db, &mut trace, &src);
         src.with_value(trace.into_map())
     }
 }
@@ -175,14 +180,15 @@ enum StructKind {
 }
 
 fn lower_struct(
+    db: &impl DefDatabase,
     trace: &mut Trace<
         LocalStructFieldId,
         StructFieldData,
         Either<ast::TupleFieldDef, ast::RecordFieldDef>,
     >,
-    ast: &ast::StructKind,
+    ast: &InFile<ast::StructKind>,
 ) -> StructKind {
-    match ast {
+    match &ast.value {
         ast::StructKind::Tuple(fl) => {
             for (i, fd) in fl.fields().enumerate() {
                 trace.alloc(
@@ -190,6 +196,7 @@ fn lower_struct(
                     || StructFieldData {
                         name: Name::new_tuple_field(i),
                         type_ref: TypeRef::from_ast_opt(fd.type_ref()),
+                        visibility: RawVisibility::from_ast(db, ast.with_value(fd.visibility())),
                     },
                 );
             }
@@ -202,6 +209,7 @@ fn lower_struct(
                     || StructFieldData {
                         name: fd.name().map(|n| n.as_name()).unwrap_or_else(Name::missing),
                         type_ref: TypeRef::from_ast_opt(fd.ascribed_type()),
+                        visibility: RawVisibility::from_ast(db, ast.with_value(fd.visibility())),
                     },
                 );
             }
diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs
index 5323af097..fc3a028e0 100644
--- a/crates/ra_hir_def/src/body/lower.rs
+++ b/crates/ra_hir_def/src/body/lower.rs
@@ -543,7 +543,10 @@ where
             };
             self.body.item_scope.define_def(def);
             if let Some(name) = name {
-                self.body.item_scope.push_res(name.as_name(), def.into());
+                let vis = crate::visibility::Visibility::Public; // FIXME determine correctly
+                self.body
+                    .item_scope
+                    .push_res(name.as_name(), crate::per_ns::PerNs::from_def(def, vis));
             }
         }
     }
diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs
index b0288ee8d..fe7bb9779 100644
--- a/crates/ra_hir_def/src/item_scope.rs
+++ b/crates/ra_hir_def/src/item_scope.rs
@@ -5,7 +5,10 @@ use hir_expand::name::Name;
 use once_cell::sync::Lazy;
 use rustc_hash::FxHashMap;
 
-use crate::{per_ns::PerNs, AdtId, BuiltinType, ImplId, MacroDefId, ModuleDefId, TraitId};
+use crate::{
+    per_ns::PerNs, visibility::Visibility, AdtId, BuiltinType, ImplId, MacroDefId, ModuleDefId,
+    TraitId,
+};
 
 #[derive(Debug, Default, PartialEq, Eq)]
 pub struct ItemScope {
@@ -30,7 +33,7 @@ pub struct ItemScope {
 static BUILTIN_SCOPE: Lazy<FxHashMap<Name, PerNs>> = Lazy::new(|| {
     BuiltinType::ALL
         .iter()
-        .map(|(name, ty)| (name.clone(), PerNs::types(ty.clone().into())))
+        .map(|(name, ty)| (name.clone(), PerNs::types(ty.clone().into(), Visibility::Public)))
         .collect()
 });
 
@@ -144,8 +147,8 @@ impl ItemScope {
         changed
     }
 
-    pub(crate) fn collect_resolutions(&self) -> Vec<(Name, PerNs)> {
-        self.visible.iter().map(|(name, res)| (name.clone(), res.clone())).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> {
@@ -153,20 +156,20 @@ impl ItemScope {
     }
 }
 
-impl From<ModuleDefId> for PerNs {
-    fn from(def: ModuleDefId) -> PerNs {
+impl PerNs {
+    pub(crate) fn from_def(def: ModuleDefId, v: Visibility) -> PerNs {
         match def {
-            ModuleDefId::ModuleId(_) => PerNs::types(def),
-            ModuleDefId::FunctionId(_) => PerNs::values(def),
+            ModuleDefId::ModuleId(_) => PerNs::types(def, v),
+            ModuleDefId::FunctionId(_) => PerNs::values(def, v),
             ModuleDefId::AdtId(adt) => match adt {
-                AdtId::StructId(_) | AdtId::UnionId(_) => PerNs::both(def, def),
-                AdtId::EnumId(_) => PerNs::types(def),
+                AdtId::StructId(_) | AdtId::UnionId(_) => PerNs::both(def, def, v),
+                AdtId::EnumId(_) => PerNs::types(def, v),
             },
-            ModuleDefId::EnumVariantId(_) => PerNs::both(def, def),
-            ModuleDefId::ConstId(_) | ModuleDefId::StaticId(_) => PerNs::values(def),
-            ModuleDefId::TraitId(_) => PerNs::types(def),
-            ModuleDefId::TypeAliasId(_) => PerNs::types(def),
-            ModuleDefId::BuiltinType(_) => PerNs::types(def),
+            ModuleDefId::EnumVariantId(_) => PerNs::both(def, def, v),
+            ModuleDefId::ConstId(_) | ModuleDefId::StaticId(_) => PerNs::values(def, v),
+            ModuleDefId::TraitId(_) => PerNs::types(def, v),
+            ModuleDefId::TypeAliasId(_) => PerNs::types(def, v),
+            ModuleDefId::BuiltinType(_) => PerNs::types(def, v),
         }
     }
 }
diff --git a/crates/ra_hir_def/src/lib.rs b/crates/ra_hir_def/src/lib.rs
index f6c7f38d1..61f044ecf 100644
--- a/crates/ra_hir_def/src/lib.rs
+++ b/crates/ra_hir_def/src/lib.rs
@@ -36,6 +36,8 @@ pub mod nameres;
 pub mod src;
 pub mod child_by_source;
 
+pub mod visibility;
+
 #[cfg(test)]
 mod test_db;
 #[cfg(test)]
diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs
index b9f40d3dd..8a22b0585 100644
--- a/crates/ra_hir_def/src/nameres/collector.rs
+++ b/crates/ra_hir_def/src/nameres/collector.rs
@@ -24,6 +24,7 @@ use crate::{
     },
     path::{ModPath, PathKind},
     per_ns::PerNs,
+    visibility::Visibility,
     AdtId, AstId, ConstLoc, ContainerId, EnumLoc, EnumVariantId, FunctionLoc, ImplLoc, Intern,
     LocalModuleId, ModuleDefId, ModuleId, StaticLoc, StructLoc, TraitLoc, TypeAliasLoc, UnionLoc,
 };
@@ -108,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, Visibility)>>,
     unresolved_imports: Vec<ImportDirective>,
     resolved_imports: Vec<ImportDirective>,
     unexpanded_macros: Vec<MacroDirective>,
@@ -214,7 +215,11 @@ where
         // In Rust, `#[macro_export]` macros are unconditionally visible at the
         // crate root, even if the parent modules is **not** visible.
         if export {
-            self.update(self.def_map.root, &[(name, PerNs::macros(macro_))]);
+            self.update(
+                self.def_map.root,
+                &[(name, PerNs::macros(macro_, Visibility::Public))],
+                Visibility::Public,
+            );
         }
     }
 
@@ -348,9 +353,12 @@ 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
+            .def_map
+            .resolve_visibility(self.db, module_id, &directive.import.visibility)
+            .unwrap_or(Visibility::Public);
 
         if import.is_glob {
             log::debug!("glob import: {:?}", import);
@@ -366,9 +374,16 @@ where
                         let scope = &item_map[m.local_id].scope;
 
                         // Module scoped macros is included
-                        let items = scope.collect_resolutions();
-
-                        self.update(module_id, &items);
+                        let items = scope
+                            .resolutions()
+                            // only keep visible names...
+                            .map(|(n, res)| {
+                                (n, res.filter_visibility(|v| v.is_visible_from_other_crate()))
+                            })
+                            .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
@@ -376,13 +391,25 @@ where
                         let scope = &self.def_map[m.local_id].scope;
 
                         // Module scoped macros is included
-                        let items = scope.collect_resolutions();
-
-                        self.update(module_id, &items);
+                        let items = scope
+                            .resolutions()
+                            // only keep visible names...
+                            .map(|(n, res)| {
+                                (
+                                    n,
+                                    res.filter_visibility(|v| {
+                                        v.is_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));
                         }
                     }
                 }
@@ -396,11 +423,11 @@ where
                         .map(|(local_id, variant_data)| {
                             let name = variant_data.name.clone();
                             let variant = EnumVariantId { parent: e, local_id };
-                            let res = PerNs::both(variant.into(), variant.into());
+                            let res = PerNs::both(variant.into(), variant.into(), vis);
                             (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);
@@ -422,21 +449,24 @@ where
                         }
                     }
 
-                    self.update(module_id, &[(name, def)]);
+                    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: Visibility) {
+        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: Visibility,
         depth: usize,
     ) {
         if depth > 100 {
@@ -446,7 +476,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 {
@@ -459,9 +489,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.is_visible_from_def_map(&self.def_map, glob_importing_module) {
+                continue;
+            }
+            self.update_recursive(glob_importing_module, resolutions, glob_import_vis, depth + 1);
         }
     }
 
@@ -633,9 +667,13 @@ where
         let is_macro_use = attrs.by_key("macro_use").exists();
         match module {
             // inline module, just recurse
-            raw::ModuleData::Definition { name, items, ast_id } => {
-                let module_id =
-                    self.push_child_module(name.clone(), AstId::new(self.file_id, *ast_id), None);
+            raw::ModuleData::Definition { name, visibility, items, ast_id } => {
+                let module_id = self.push_child_module(
+                    name.clone(),
+                    AstId::new(self.file_id, *ast_id),
+                    None,
+                    &visibility,
+                );
 
                 ModCollector {
                     def_collector: &mut *self.def_collector,
@@ -650,7 +688,7 @@ where
                 }
             }
             // out of line module, resolve, parse and recurse
-            raw::ModuleData::Declaration { name, ast_id } => {
+            raw::ModuleData::Declaration { name, visibility, ast_id } => {
                 let ast_id = AstId::new(self.file_id, *ast_id);
                 match self.mod_dir.resolve_declaration(
                     self.def_collector.db,
@@ -659,7 +697,12 @@ where
                     path_attr,
                 ) {
                     Ok((file_id, mod_dir)) => {
-                        let module_id = self.push_child_module(name.clone(), ast_id, Some(file_id));
+                        let module_id = self.push_child_module(
+                            name.clone(),
+                            ast_id,
+                            Some(file_id),
+                            &visibility,
+                        );
                         let raw_items = self.def_collector.db.raw_items(file_id.into());
                         ModCollector {
                             def_collector: &mut *self.def_collector,
@@ -690,7 +733,13 @@ where
         name: Name,
         declaration: AstId<ast::Module>,
         definition: Option<FileId>,
+        visibility: &crate::visibility::RawVisibility,
     ) -> LocalModuleId {
+        let vis = self
+            .def_collector
+            .def_map
+            .resolve_visibility(self.def_collector.db, self.module_id, visibility)
+            .unwrap_or(Visibility::Public);
         let modules = &mut self.def_collector.def_map.modules;
         let res = modules.alloc(ModuleData::default());
         modules[res].parent = Some(self.module_id);
@@ -702,7 +751,7 @@ where
         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, def.into())]);
+        self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))], vis);
         res
     }
 
@@ -716,6 +765,7 @@ where
 
         let name = def.name.clone();
         let container = ContainerId::ModuleId(module);
+        let vis = &def.visibility;
         let def: ModuleDefId = match def.kind {
             raw::DefKind::Function(ast_id) => FunctionLoc {
                 container: container.into(),
@@ -761,7 +811,12 @@ where
             .into(),
         };
         self.def_collector.def_map.modules[self.module_id].scope.define_def(def);
-        self.def_collector.update(self.module_id, &[(name, def.into())])
+        let vis = self
+            .def_collector
+            .def_map
+            .resolve_visibility(self.def_collector.db, self.module_id, vis)
+            .unwrap_or(Visibility::Public);
+        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/path_resolution.rs b/crates/ra_hir_def/src/nameres/path_resolution.rs
index 695014c7b..fd6422d60 100644
--- a/crates/ra_hir_def/src/nameres/path_resolution.rs
+++ b/crates/ra_hir_def/src/nameres/path_resolution.rs
@@ -21,6 +21,7 @@ use crate::{
     nameres::{BuiltinShadowMode, CrateDefMap},
     path::{ModPath, PathKind},
     per_ns::PerNs,
+    visibility::{RawVisibility, Visibility},
     AdtId, CrateId, EnumVariantId, LocalModuleId, ModuleDefId, ModuleId,
 };
 
@@ -61,7 +62,35 @@ impl ResolvePathResult {
 
 impl CrateDefMap {
     pub(super) fn resolve_name_in_extern_prelude(&self, name: &Name) -> PerNs {
-        self.extern_prelude.get(name).map_or(PerNs::none(), |&it| PerNs::types(it))
+        self.extern_prelude
+            .get(name)
+            .map_or(PerNs::none(), |&it| PerNs::types(it, Visibility::Public))
+    }
+
+    pub(crate) fn resolve_visibility(
+        &self,
+        db: &impl DefDatabase,
+        original_module: LocalModuleId,
+        visibility: &RawVisibility,
+    ) -> Option<Visibility> {
+        match visibility {
+            RawVisibility::Module(path) => {
+                let (result, remaining) =
+                    self.resolve_path(db, original_module, &path, BuiltinShadowMode::Module);
+                if remaining.is_some() {
+                    return None;
+                }
+                let types = result.take_types()?;
+                match types {
+                    ModuleDefId::ModuleId(m) => Some(Visibility::Module(m)),
+                    _ => {
+                        // error: visibility needs to refer to module
+                        None
+                    }
+                }
+            }
+            RawVisibility::Public => Some(Visibility::Public),
+        }
     }
 
     // Returns Yes if we are sure that additions to `ItemMap` wouldn't change
@@ -88,17 +117,21 @@ impl CrateDefMap {
             PathKind::DollarCrate(krate) => {
                 if krate == self.krate {
                     tested_by!(macro_dollar_crate_self);
-                    PerNs::types(ModuleId { krate: self.krate, local_id: self.root }.into())
+                    PerNs::types(
+                        ModuleId { krate: self.krate, local_id: self.root }.into(),
+                        Visibility::Public,
+                    )
                 } else {
                     let def_map = db.crate_def_map(krate);
                     let module = ModuleId { krate, local_id: def_map.root };
                     tested_by!(macro_dollar_crate_other);
-                    PerNs::types(module.into())
+                    PerNs::types(module.into(), Visibility::Public)
                 }
             }
-            PathKind::Crate => {
-                PerNs::types(ModuleId { krate: self.krate, local_id: self.root }.into())
-            }
+            PathKind::Crate => PerNs::types(
+                ModuleId { krate: self.krate, local_id: self.root }.into(),
+                Visibility::Public,
+            ),
             // plain import or absolute path in 2015: crate-relative with
             // fallback to extern prelude (with the simplification in
             // rust-lang/rust#57745)
@@ -126,7 +159,10 @@ impl CrateDefMap {
                 let m = successors(Some(original_module), |m| self.modules[*m].parent)
                     .nth(lvl as usize);
                 if let Some(local_id) = m {
-                    PerNs::types(ModuleId { krate: self.krate, local_id }.into())
+                    PerNs::types(
+                        ModuleId { krate: self.krate, local_id }.into(),
+                        Visibility::Public,
+                    )
                 } else {
                     log::debug!("super path in root module");
                     return ResolvePathResult::empty(ReachedFixedPoint::Yes);
@@ -140,7 +176,7 @@ impl CrateDefMap {
                 };
                 if let Some(def) = self.extern_prelude.get(&segment) {
                     log::debug!("absolute path {:?} resolved to crate {:?}", path, def);
-                    PerNs::types(*def)
+                    PerNs::types(*def, Visibility::Public)
                 } else {
                     return ResolvePathResult::empty(ReachedFixedPoint::No); // extern crate declarations can add to the extern prelude
                 }
@@ -148,7 +184,7 @@ impl CrateDefMap {
         };
 
         for (i, segment) in segments {
-            let curr = match curr_per_ns.take_types() {
+            let (curr, vis) = match curr_per_ns.take_types_vis() {
                 Some(r) => r,
                 None => {
                     // we still have path segments left, but the path so far
@@ -189,11 +225,11 @@ impl CrateDefMap {
                     match enum_data.variant(&segment) {
                         Some(local_id) => {
                             let variant = EnumVariantId { parent: e, local_id };
-                            PerNs::both(variant.into(), variant.into())
+                            PerNs::both(variant.into(), variant.into(), Visibility::Public)
                         }
                         None => {
                             return ResolvePathResult::with(
-                                PerNs::types(e.into()),
+                                PerNs::types(e.into(), vis),
                                 ReachedFixedPoint::Yes,
                                 Some(i),
                                 Some(self.krate),
@@ -211,7 +247,7 @@ impl CrateDefMap {
                     );
 
                     return ResolvePathResult::with(
-                        PerNs::types(s),
+                        PerNs::types(s, vis),
                         ReachedFixedPoint::Yes,
                         Some(i),
                         Some(self.krate),
@@ -235,11 +271,15 @@ impl CrateDefMap {
         //  - current module / scope
         //  - extern prelude
         //  - std prelude
-        let from_legacy_macro =
-            self[module].scope.get_legacy_macro(name).map_or_else(PerNs::none, PerNs::macros);
+        let from_legacy_macro = self[module]
+            .scope
+            .get_legacy_macro(name)
+            .map_or_else(PerNs::none, |m| PerNs::macros(m, Visibility::Public));
         let from_scope = self[module].scope.get(name, shadow);
-        let from_extern_prelude =
-            self.extern_prelude.get(name).map_or(PerNs::none(), |&it| PerNs::types(it));
+        let from_extern_prelude = self
+            .extern_prelude
+            .get(name)
+            .map_or(PerNs::none(), |&it| PerNs::types(it, Visibility::Public));
         let from_prelude = self.resolve_in_prelude(db, name, shadow);
 
         from_legacy_macro.or(from_scope).or(from_extern_prelude).or(from_prelude)
diff --git a/crates/ra_hir_def/src/nameres/raw.rs b/crates/ra_hir_def/src/nameres/raw.rs
index 73dc08745..fac1169ef 100644
--- a/crates/ra_hir_def/src/nameres/raw.rs
+++ b/crates/ra_hir_def/src/nameres/raw.rs
@@ -16,12 +16,15 @@ use hir_expand::{
 use ra_arena::{impl_arena_id, Arena, RawId};
 use ra_prof::profile;
 use ra_syntax::{
-    ast::{self, AttrsOwner, NameOwner},
+    ast::{self, AttrsOwner, NameOwner, VisibilityOwner},
     AstNode,
 };
 use test_utils::tested_by;
 
-use crate::{attr::Attrs, db::DefDatabase, path::ModPath, FileAstId, HirFileId, InFile};
+use crate::{
+    attr::Attrs, db::DefDatabase, path::ModPath, visibility::RawVisibility, FileAstId, HirFileId,
+    InFile,
+};
 
 /// `RawItems` is a set of top-level items in a file (except for impls).
 ///
@@ -122,8 +125,17 @@ impl_arena_id!(Module);
 
 #[derive(Debug, PartialEq, Eq)]
 pub(super) enum ModuleData {
-    Declaration { name: Name, ast_id: FileAstId<ast::Module> },
-    Definition { name: Name, ast_id: FileAstId<ast::Module>, items: Vec<RawItem> },
+    Declaration {
+        name: Name,
+        visibility: RawVisibility,
+        ast_id: FileAstId<ast::Module>,
+    },
+    Definition {
+        name: Name,
+        visibility: RawVisibility,
+        ast_id: FileAstId<ast::Module>,
+        items: Vec<RawItem>,
+    },
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
@@ -138,6 +150,7 @@ pub struct ImportData {
     pub(super) is_prelude: bool,
     pub(super) is_extern_crate: bool,
     pub(super) is_macro_use: bool,
+    pub(super) visibility: RawVisibility,
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
@@ -148,6 +161,7 @@ impl_arena_id!(Def);
 pub(super) struct DefData {
     pub(super) name: Name,
     pub(super) kind: DefKind,
+    pub(super) visibility: RawVisibility,
 }
 
 #[derive(Debug, PartialEq, Eq, Clone, Copy)]
@@ -218,6 +232,7 @@ impl RawItemsCollector {
 
     fn add_item(&mut self, current_module: Option<Module>, item: ast::ModuleItem) {
         let attrs = self.parse_attrs(&item);
+        let visibility = RawVisibility::from_ast_with_hygiene(item.visibility(), &self.hygiene);
         let (kind, name) = match item {
             ast::ModuleItem::Module(module) => {
                 self.add_module(current_module, module);
@@ -266,7 +281,7 @@ impl RawItemsCollector {
         };
         if let Some(name) = name {
             let name = name.as_name();
-            let def = self.raw_items.defs.alloc(DefData { name, kind });
+            let def = self.raw_items.defs.alloc(DefData { name, kind, visibility });
             self.push_item(current_module, attrs, RawItemKind::Def(def));
         }
     }
@@ -277,10 +292,12 @@ impl RawItemsCollector {
             None => return,
         };
         let attrs = self.parse_attrs(&module);
+        let visibility = RawVisibility::from_ast_with_hygiene(module.visibility(), &self.hygiene);
 
         let ast_id = self.source_ast_id_map.ast_id(&module);
         if module.has_semi() {
-            let item = self.raw_items.modules.alloc(ModuleData::Declaration { name, ast_id });
+            let item =
+                self.raw_items.modules.alloc(ModuleData::Declaration { name, visibility, ast_id });
             self.push_item(current_module, attrs, RawItemKind::Module(item));
             return;
         }
@@ -288,6 +305,7 @@ impl RawItemsCollector {
         if let Some(item_list) = module.item_list() {
             let item = self.raw_items.modules.alloc(ModuleData::Definition {
                 name,
+                visibility,
                 ast_id,
                 items: Vec::new(),
             });
@@ -302,6 +320,7 @@ impl RawItemsCollector {
         // FIXME: cfg_attr
         let is_prelude = use_item.has_atom_attr("prelude_import");
         let attrs = self.parse_attrs(&use_item);
+        let visibility = RawVisibility::from_ast_with_hygiene(use_item.visibility(), &self.hygiene);
 
         let mut buf = Vec::new();
         ModPath::expand_use_item(
@@ -315,6 +334,7 @@ impl RawItemsCollector {
                     is_prelude,
                     is_extern_crate: false,
                     is_macro_use: false,
+                    visibility: visibility.clone(),
                 };
                 buf.push(import_data);
             },
@@ -331,6 +351,8 @@ impl RawItemsCollector {
     ) {
         if let Some(name_ref) = extern_crate.name_ref() {
             let path = ModPath::from_name_ref(&name_ref);
+            let visibility =
+                RawVisibility::from_ast_with_hygiene(extern_crate.visibility(), &self.hygiene);
             let alias = extern_crate.alias().and_then(|a| a.name()).map(|it| it.as_name());
             let attrs = self.parse_attrs(&extern_crate);
             // FIXME: cfg_attr
@@ -342,6 +364,7 @@ impl RawItemsCollector {
                 is_prelude: false,
                 is_extern_crate: true,
                 is_macro_use,
+                visibility,
             };
             self.push_import(current_module, attrs, import_data);
         }
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..71fa0abe8 100644
--- a/crates/ra_hir_def/src/nameres/tests/globs.rs
+++ b/crates/ra_hir_def/src/nameres/tests/globs.rs
@@ -74,6 +74,83 @@ 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
+        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
+    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(
@@ -93,6 +170,26 @@ fn glob_across_crates() {
 }
 
 #[test]
+fn glob_privacy_across_crates() {
+    covers!(glob_across_crates);
+    let map = def_map(
+        "
+        //- /main.rs crate:main deps:test_crate
+        use test_crate::*;
+
+        //- /lib.rs crate:test_crate
+        pub struct Baz;
+        struct Foo;
+        ",
+    );
+    assert_snapshot!(map, @r###"
+   ⋮crate
+   ⋮Baz: t v
+    "###
+    );
+}
+
+#[test]
 fn glob_enum() {
     covers!(glob_enum);
     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 3a5105028..6e435c8c1 100644
--- a/crates/ra_hir_def/src/per_ns.rs
+++ b/crates/ra_hir_def/src/per_ns.rs
@@ -5,13 +5,13 @@
 
 use hir_expand::MacroDefId;
 
-use crate::ModuleDefId;
+use crate::{visibility::Visibility, ModuleDefId};
 
-#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub struct PerNs {
-    pub types: Option<ModuleDefId>,
-    pub values: Option<ModuleDefId>,
-    pub macros: Option<MacroDefId>,
+    pub types: Option<(ModuleDefId, Visibility)>,
+    pub values: Option<(ModuleDefId, Visibility)>,
+    pub macros: Option<(MacroDefId, Visibility)>,
 }
 
 impl Default for PerNs {
@@ -25,20 +25,20 @@ impl PerNs {
         PerNs { types: None, values: None, macros: None }
     }
 
-    pub fn values(t: ModuleDefId) -> PerNs {
-        PerNs { types: None, values: Some(t), macros: None }
+    pub fn values(t: ModuleDefId, v: Visibility) -> PerNs {
+        PerNs { types: None, values: Some((t, v)), macros: None }
     }
 
-    pub fn types(t: ModuleDefId) -> PerNs {
-        PerNs { types: Some(t), values: None, macros: None }
+    pub fn types(t: ModuleDefId, v: Visibility) -> PerNs {
+        PerNs { types: Some((t, v)), values: None, macros: None }
     }
 
-    pub fn both(types: ModuleDefId, values: ModuleDefId) -> PerNs {
-        PerNs { types: Some(types), values: Some(values), macros: None }
+    pub fn both(types: ModuleDefId, values: ModuleDefId, v: Visibility) -> PerNs {
+        PerNs { types: Some((types, v)), values: Some((values, v)), macros: None }
     }
 
-    pub fn macros(macro_: MacroDefId) -> PerNs {
-        PerNs { types: None, values: None, macros: Some(macro_) }
+    pub fn macros(macro_: MacroDefId, v: Visibility) -> PerNs {
+        PerNs { types: None, values: None, macros: Some((macro_, v)) }
     }
 
     pub fn is_none(&self) -> bool {
@@ -46,15 +46,35 @@ impl PerNs {
     }
 
     pub fn take_types(self) -> Option<ModuleDefId> {
+        self.types.map(|it| it.0)
+    }
+
+    pub fn take_types_vis(self) -> Option<(ModuleDefId, Visibility)> {
         self.types
     }
 
     pub fn take_values(self) -> Option<ModuleDefId> {
-        self.values
+        self.values.map(|it| it.0)
     }
 
     pub fn take_macros(self) -> Option<MacroDefId> {
-        self.macros
+        self.macros.map(|it| it.0)
+    }
+
+    pub fn filter_visibility(self, mut f: impl FnMut(Visibility) -> 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: Visibility) -> PerNs {
+        PerNs {
+            types: self.types.map(|(it, _)| (it, vis)),
+            values: self.values.map(|(it, _)| (it, vis)),
+            macros: self.macros.map(|(it, _)| (it, vis)),
+        }
     }
 
     pub fn or(self, other: PerNs) -> PerNs {
diff --git a/crates/ra_hir_def/src/resolver.rs b/crates/ra_hir_def/src/resolver.rs
index cf3c33d78..43dc751d9 100644
--- a/crates/ra_hir_def/src/resolver.rs
+++ b/crates/ra_hir_def/src/resolver.rs
@@ -19,6 +19,7 @@ use crate::{
     nameres::CrateDefMap,
     path::{ModPath, PathKind},
     per_ns::PerNs,
+    visibility::{RawVisibility, Visibility},
     AdtId, AssocContainerId, ConstId, ContainerId, DefWithBodyId, EnumId, EnumVariantId,
     FunctionId, GenericDefId, HasModule, ImplId, LocalModuleId, Lookup, ModuleDefId, ModuleId,
     StaticId, StructId, TraitId, TypeAliasId, TypeParamId, VariantId,
@@ -231,6 +232,23 @@ impl Resolver {
         Some(res)
     }
 
+    pub fn resolve_visibility(
+        &self,
+        db: &impl DefDatabase,
+        visibility: &RawVisibility,
+    ) -> Option<Visibility> {
+        match visibility {
+            RawVisibility::Module(_) => {
+                let (item_map, module) = match self.module() {
+                    Some(it) => it,
+                    None => return None,
+                };
+                item_map.resolve_visibility(db, module, visibility)
+            }
+            RawVisibility::Public => Some(Visibility::Public),
+        }
+    }
+
     pub fn resolve_path_in_value_ns(
         &self,
         db: &impl DefDatabase,
@@ -448,10 +466,10 @@ impl Scope {
                     f(name.clone(), ScopeDef::PerNs(def));
                 });
                 m.crate_def_map[m.module_id].scope.legacy_macros().for_each(|(name, macro_)| {
-                    f(name.clone(), ScopeDef::PerNs(PerNs::macros(macro_)));
+                    f(name.clone(), ScopeDef::PerNs(PerNs::macros(macro_, Visibility::Public)));
                 });
                 m.crate_def_map.extern_prelude.iter().for_each(|(name, &def)| {
-                    f(name.clone(), ScopeDef::PerNs(PerNs::types(def.into())));
+                    f(name.clone(), ScopeDef::PerNs(PerNs::types(def.into(), Visibility::Public)));
                 });
                 if let Some(prelude) = m.crate_def_map.prelude {
                     let prelude_def_map = db.crate_def_map(prelude.krate);
diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs
new file mode 100644
index 000000000..d8296da4b
--- /dev/null
+++ b/crates/ra_hir_def/src/visibility.rs
@@ -0,0 +1,120 @@
+//! Defines hir-level representation of visibility (e.g. `pub` and `pub(crate)`).
+
+use hir_expand::{hygiene::Hygiene, InFile};
+use ra_syntax::ast;
+
+use crate::{
+    db::DefDatabase,
+    path::{ModPath, PathKind},
+    ModuleId,
+};
+
+/// Visibility of an item, not yet resolved.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum RawVisibility {
+    /// `pub(in module)`, `pub(crate)` or `pub(super)`. Also private, which is
+    /// equivalent to `pub(self)`.
+    Module(ModPath),
+    /// `pub`.
+    Public,
+}
+
+impl RawVisibility {
+    const fn private() -> RawVisibility {
+        let path = ModPath { kind: PathKind::Super(0), segments: Vec::new() };
+        RawVisibility::Module(path)
+    }
+
+    pub(crate) fn from_ast(
+        db: &impl DefDatabase,
+        node: InFile<Option<ast::Visibility>>,
+    ) -> RawVisibility {
+        Self::from_ast_with_hygiene(node.value, &Hygiene::new(db, node.file_id))
+    }
+
+    pub(crate) fn from_ast_with_hygiene(
+        node: Option<ast::Visibility>,
+        hygiene: &Hygiene,
+    ) -> RawVisibility {
+        let node = match node {
+            None => return RawVisibility::private(),
+            Some(node) => node,
+        };
+        match node.kind() {
+            ast::VisibilityKind::In(path) => {
+                let path = ModPath::from_src(path, hygiene);
+                let path = match path {
+                    None => return RawVisibility::private(),
+                    Some(path) => path,
+                };
+                RawVisibility::Module(path)
+            }
+            ast::VisibilityKind::PubCrate => {
+                let path = ModPath { kind: PathKind::Crate, segments: Vec::new() };
+                RawVisibility::Module(path)
+            }
+            ast::VisibilityKind::PubSuper => {
+                let path = ModPath { kind: PathKind::Super(1), segments: Vec::new() };
+                RawVisibility::Module(path)
+            }
+            ast::VisibilityKind::Pub => RawVisibility::Public,
+        }
+    }
+
+    pub fn resolve(
+        &self,
+        db: &impl DefDatabase,
+        resolver: &crate::resolver::Resolver,
+    ) -> Visibility {
+        // we fall back to public visibility (i.e. fail open) if the path can't be resolved
+        resolver.resolve_visibility(db, self).unwrap_or(Visibility::Public)
+    }
+}
+
+/// Visibility of an item, with the path resolved.
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub enum Visibility {
+    /// Visibility is restricted to a certain module.
+    Module(ModuleId),
+    /// Visibility is unrestricted.
+    Public,
+}
+
+impl Visibility {
+    pub fn is_visible_from(self, db: &impl DefDatabase, from_module: ModuleId) -> bool {
+        let to_module = match self {
+            Visibility::Module(m) => m,
+            Visibility::Public => return true,
+        };
+        // if they're not in the same crate, it can't be visible
+        if from_module.krate != to_module.krate {
+            return false;
+        }
+        let def_map = db.crate_def_map(from_module.krate);
+        self.is_visible_from_def_map(&def_map, from_module.local_id)
+    }
+
+    pub(crate) fn is_visible_from_other_crate(self) -> bool {
+        match self {
+            Visibility::Module(_) => false,
+            Visibility::Public => true,
+        }
+    }
+
+    pub(crate) fn is_visible_from_def_map(
+        self,
+        def_map: &crate::nameres::CrateDefMap,
+        from_module: crate::LocalModuleId,
+    ) -> bool {
+        let to_module = match self {
+            Visibility::Module(m) => m,
+            Visibility::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].parent?;
+            Some(parent_id)
+        });
+        ancestors.any(|m| m == to_module.local_id)
+    }
+}
diff --git a/crates/ra_ide/src/completion/complete_dot.rs b/crates/ra_ide/src/completion/complete_dot.rs
index 294964887..210a685e4 100644
--- a/crates/ra_ide/src/completion/complete_dot.rs
+++ b/crates/ra_ide/src/completion/complete_dot.rs
@@ -1,6 +1,6 @@
 //! FIXME: write short doc here
 
-use hir::Type;
+use hir::{HasVisibility, Type};
 
 use crate::completion::completion_item::CompletionKind;
 use crate::{
@@ -38,9 +38,15 @@ pub(super) fn complete_dot(acc: &mut Completions, ctx: &CompletionContext) {
 fn complete_fields(acc: &mut Completions, ctx: &CompletionContext, receiver: &Type) {
     for receiver in receiver.autoderef(ctx.db) {
         for (field, ty) in receiver.fields(ctx.db) {
+            if ctx.module.map_or(false, |m| !field.is_visible_from(ctx.db, m)) {
+                // Skip private field. FIXME: If the definition location of the
+                // field is editable, we should show the completion
+                continue;
+            }
             acc.add_field(ctx, field, &ty);
         }
         for (i, ty) in receiver.tuple_fields(ctx.db).into_iter().enumerate() {
+            // FIXME: Handle visibility
             acc.add_tuple_field(ctx, i, &ty);
         }
     }
@@ -187,6 +193,55 @@ mod tests {
     }
 
     #[test]
+    fn test_struct_field_visibility_private() {
+        assert_debug_snapshot!(
+            do_ref_completion(
+                r"
+            mod inner {
+                struct A {
+                    private_field: u32,
+                    pub pub_field: u32,
+                    pub(crate) crate_field: u32,
+                    pub(super) super_field: u32,
+                }
+            }
+            fn foo(a: inner::A) {
+               a.<|>
+            }
+            ",
+            ),
+            @r###"
+        [
+            CompletionItem {
+                label: "crate_field",
+                source_range: [313; 313),
+                delete: [313; 313),
+                insert: "crate_field",
+                kind: Field,
+                detail: "u32",
+            },
+            CompletionItem {
+                label: "pub_field",
+                source_range: [313; 313),
+                delete: [313; 313),
+                insert: "pub_field",
+                kind: Field,
+                detail: "u32",
+            },
+            CompletionItem {
+                label: "super_field",
+                source_range: [313; 313),
+                delete: [313; 313),
+                insert: "super_field",
+                kind: Field,
+                detail: "u32",
+            },
+        ]
+        "###
+        );
+    }
+
+    #[test]
     fn test_method_completion() {
         assert_debug_snapshot!(
         do_ref_completion(
diff --git a/crates/ra_syntax/src/ast.rs b/crates/ra_syntax/src/ast.rs
index 277532a8c..89cb9a9f3 100644
--- a/crates/ra_syntax/src/ast.rs
+++ b/crates/ra_syntax/src/ast.rs
@@ -17,7 +17,9 @@ use crate::{
 
 pub use self::{
     expr_extensions::{ArrayExprKind, BinOp, ElseBranch, LiteralKind, PrefixOp, RangeOp},
-    extensions::{FieldKind, PathSegmentKind, SelfParamKind, StructKind, TypeBoundKind},
+    extensions::{
+        FieldKind, PathSegmentKind, SelfParamKind, StructKind, TypeBoundKind, VisibilityKind,
+    },
     generated::*,
     tokens::*,
     traits::*,
diff --git a/crates/ra_syntax/src/ast/extensions.rs b/crates/ra_syntax/src/ast/extensions.rs
index baaef3023..d9666cdca 100644
--- a/crates/ra_syntax/src/ast/extensions.rs
+++ b/crates/ra_syntax/src/ast/extensions.rs
@@ -413,3 +413,32 @@ impl ast::TraitDef {
         self.syntax().children_with_tokens().any(|t| t.kind() == T![auto])
     }
 }
+
+pub enum VisibilityKind {
+    In(ast::Path),
+    PubCrate,
+    PubSuper,
+    Pub,
+}
+
+impl ast::Visibility {
+    pub fn kind(&self) -> VisibilityKind {
+        if let Some(path) = children(self).next() {
+            VisibilityKind::In(path)
+        } else if self.is_pub_crate() {
+            VisibilityKind::PubCrate
+        } else if self.is_pub_super() {
+            VisibilityKind::PubSuper
+        } else {
+            VisibilityKind::Pub
+        }
+    }
+
+    fn is_pub_crate(&self) -> bool {
+        self.syntax().children_with_tokens().any(|it| it.kind() == T![crate])
+    }
+
+    fn is_pub_super(&self) -> bool {
+        self.syntax().children_with_tokens().any(|it| it.kind() == T![super])
+    }
+}
diff --git a/crates/ra_syntax/src/ast/generated.rs b/crates/ra_syntax/src/ast/generated.rs
index 9f9d6e63c..e64c83d33 100644
--- a/crates/ra_syntax/src/ast/generated.rs
+++ b/crates/ra_syntax/src/ast/generated.rs
@@ -1064,6 +1064,7 @@ impl AstNode for ExternCrateItem {
     }
 }
 impl ast::AttrsOwner for ExternCrateItem {}
+impl ast::VisibilityOwner for ExternCrateItem {}
 impl ExternCrateItem {
     pub fn name_ref(&self) -> Option<NameRef> {
         AstChildren::new(&self.syntax).next()
@@ -2006,6 +2007,7 @@ impl AstNode for ModuleItem {
     }
 }
 impl ast::AttrsOwner for ModuleItem {}
+impl ast::VisibilityOwner for ModuleItem {}
 impl ModuleItem {}
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub struct Name {
@@ -3893,6 +3895,7 @@ impl AstNode for UseItem {
     }
 }
 impl ast::AttrsOwner for UseItem {}
+impl ast::VisibilityOwner for UseItem {}
 impl UseItem {
     pub fn use_tree(&self) -> Option<UseTree> {
         AstChildren::new(&self.syntax).next()
diff --git a/crates/ra_syntax/src/grammar.ron b/crates/ra_syntax/src/grammar.ron
index 08aafb610..e43a724f0 100644
--- a/crates/ra_syntax/src/grammar.ron
+++ b/crates/ra_syntax/src/grammar.ron
@@ -412,7 +412,7 @@ Grammar(
         "ModuleItem": (
             enum: ["StructDef", "UnionDef", "EnumDef", "FnDef", "TraitDef", "TypeAliasDef", "ImplBlock",
                    "UseItem", "ExternCrateItem", "ConstDef", "StaticDef", "Module" ],
-            traits: ["AttrsOwner"],
+            traits: ["AttrsOwner", "VisibilityOwner"],
         ),
         "ImplItem": (
             enum: ["FnDef", "TypeAliasDef", "ConstDef"],
@@ -683,7 +683,7 @@ Grammar(
             ]
         ),
         "UseItem": (
-            traits: ["AttrsOwner"],
+            traits: ["AttrsOwner", "VisibilityOwner"],
             options: [ "UseTree" ],
         ),
         "UseTree": (
@@ -696,7 +696,7 @@ Grammar(
             collections: [("use_trees", "UseTree")]
         ),
         "ExternCrateItem": (
-            traits: ["AttrsOwner"],
+            traits: ["AttrsOwner", "VisibilityOwner"],
             options: ["NameRef", "Alias"],
         ),
         "ArgList": (