Merge #3516

3516: Handle visibility in more cases in completion r=matklad a=flodiebold

This means we don't show private items when completing paths or method calls.

We might want to show private items if we can edit their definition and provide a "make public" assist, but I feel like we'd need better sorting of completion items for that, so they can be not shown or sorted to the bottom by default. Until then, they're usually more of a distraction to me.

Co-authored-by: Florian Diebold <flodiebold@gmail.com>

crates/ra_hir/src/code_model.rs

@@ -204,10 +204,26 @@ impl Module {
     }
 
     /// Returns a `ModuleScope`: a set of items, visible in this module.
-    pub fn scope(self, db: &impl HirDatabase) -> Vec<(Name, ScopeDef)> {
+    pub fn scope(
+        self,
+        db: &impl HirDatabase,
+        visible_from: Option<Module>,
+    ) -> Vec<(Name, ScopeDef)> {
         db.crate_def_map(self.id.krate)[self.id.local_id]
             .scope
             .entries()
+            .filter_map(|(name, def)| {
+                if let Some(m) = visible_from {
+                    let filtered = def.filter_visibility(|vis| vis.is_visible_from(db, m.id));
+                    if filtered.is_none() && !def.is_none() {
+                        None
+                    } else {
+                        Some((name, filtered))
+                    }
+                } else {
+                    Some((name, def))
+                }
+            })
             .map(|(name, def)| (name.clone(), def.into()))
             .collect()
     }
@@ -571,6 +587,14 @@ impl Function {
     }
 }
 
+impl HasVisibility for Function {
+    fn visibility(&self, db: &impl HirDatabase) -> Visibility {
+        let function_data = db.function_data(self.id);
+        let visibility = &function_data.visibility;
+        visibility.resolve(db, &self.id.resolver(db))
+    }
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct Const {
     pub(crate) id: ConstId,
@@ -590,6 +614,14 @@ impl Const {
     }
 }
 
+impl HasVisibility for Const {
+    fn visibility(&self, db: &impl HirDatabase) -> Visibility {
+        let function_data = db.const_data(self.id);
+        let visibility = &function_data.visibility;
+        visibility.resolve(db, &self.id.resolver(db))
+    }
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct Static {
     pub(crate) id: StaticId,
@@ -664,6 +696,14 @@ impl TypeAlias {
     }
 }
 
+impl HasVisibility for TypeAlias {
+    fn visibility(&self, db: &impl HirDatabase) -> Visibility {
+        let function_data = db.type_alias_data(self.id);
+        let visibility = &function_data.visibility;
+        visibility.resolve(db, &self.id.resolver(db))
+    }
+}
+
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub struct MacroDef {
     pub(crate) id: MacroDefId,
@@ -751,6 +791,16 @@ impl AssocItem {
     }
 }
 
+impl HasVisibility for AssocItem {
+    fn visibility(&self, db: &impl HirDatabase) -> Visibility {
+        match self {
+            AssocItem::Function(f) => f.visibility(db),
+            AssocItem::Const(c) => c.visibility(db),
+            AssocItem::TypeAlias(t) => t.visibility(db),
+        }
+    }
+}
+
 #[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)]
 pub enum GenericDef {
     Function(Function),

crates/ra_hir_def/src/data.rs

@@ -7,13 +7,16 @@ use hir_expand::{
     AstId, InFile,
 };
 use ra_prof::profile;
-use ra_syntax::ast::{self, AstNode, ImplItem, ModuleItemOwner, NameOwner, TypeAscriptionOwner};
+use ra_syntax::ast::{
+    self, AstNode, ImplItem, ModuleItemOwner, NameOwner, TypeAscriptionOwner, VisibilityOwner,
+};
 
 use crate::{
     db::DefDatabase,
     path::{path, GenericArgs, Path},
     src::HasSource,
     type_ref::{Mutability, TypeBound, TypeRef},
+    visibility::RawVisibility,
     AssocContainerId, AssocItemId, ConstId, ConstLoc, Expander, FunctionId, FunctionLoc, HasModule,
     ImplId, Intern, Lookup, ModuleId, StaticId, TraitId, TypeAliasId, TypeAliasLoc,
 };
@@ -26,6 +29,7 @@ pub struct FunctionData {
     /// True if the first param is `self`. This is relevant to decide whether this
     /// can be called as a method.
     pub has_self_param: bool,
+    pub visibility: RawVisibility,
 }
 
 impl FunctionData {
@@ -72,7 +76,9 @@ impl FunctionData {
             ret_type
         };
 
-        let sig = FunctionData { name, params, ret_type, has_self_param };
+        let visibility = RawVisibility::from_ast(db, src.map(|s| s.visibility()));
+
+        let sig = FunctionData { name, params, ret_type, has_self_param, visibility };
         Arc::new(sig)
     }
 }
@@ -91,6 +97,7 @@ fn desugar_future_path(orig: TypeRef) -> Path {
 pub struct TypeAliasData {
     pub name: Name,
     pub type_ref: Option<TypeRef>,
+    pub visibility: RawVisibility,
 }
 
 impl TypeAliasData {
@@ -98,10 +105,11 @@ impl TypeAliasData {
         db: &impl DefDatabase,
         typ: TypeAliasId,
     ) -> Arc<TypeAliasData> {
-        let node = typ.lookup(db).source(db).value;
-        let name = node.name().map_or_else(Name::missing, |n| n.as_name());
-        let type_ref = node.type_ref().map(TypeRef::from_ast);
-        Arc::new(TypeAliasData { name, type_ref })
+        let node = typ.lookup(db).source(db);
+        let name = node.value.name().map_or_else(Name::missing, |n| n.as_name());
+        let type_ref = node.value.type_ref().map(TypeRef::from_ast);
+        let visibility = RawVisibility::from_ast(db, node.map(|n| n.visibility()));
+        Arc::new(TypeAliasData { name, type_ref, visibility })
     }
 }
 
@@ -217,23 +225,28 @@ pub struct ConstData {
     /// const _: () = ();
     pub name: Option<Name>,
     pub type_ref: TypeRef,
+    pub visibility: RawVisibility,
 }
 
 impl ConstData {
     pub(crate) fn const_data_query(db: &impl DefDatabase, konst: ConstId) -> Arc<ConstData> {
-        let node = konst.lookup(db).source(db).value;
-        Arc::new(ConstData::new(&node))
+        let node = konst.lookup(db).source(db);
+        Arc::new(ConstData::new(db, node))
     }
 
     pub(crate) fn static_data_query(db: &impl DefDatabase, konst: StaticId) -> Arc<ConstData> {
-        let node = konst.lookup(db).source(db).value;
-        Arc::new(ConstData::new(&node))
+        let node = konst.lookup(db).source(db);
+        Arc::new(ConstData::new(db, node))
     }
 
-    fn new<N: NameOwner + TypeAscriptionOwner>(node: &N) -> ConstData {
-        let name = node.name().map(|n| n.as_name());
-        let type_ref = TypeRef::from_ast_opt(node.ascribed_type());
-        ConstData { name, type_ref }
+    fn new<N: NameOwner + TypeAscriptionOwner + VisibilityOwner>(
+        db: &impl DefDatabase,
+        node: InFile<N>,
+    ) -> ConstData {
+        let name = node.value.name().map(|n| n.as_name());
+        let type_ref = TypeRef::from_ast_opt(node.value.ascribed_type());
+        let visibility = RawVisibility::from_ast(db, node.map(|n| n.visibility()));
+        ConstData { name, type_ref, visibility }
     }
 }
 

crates/ra_ide/src/completion/complete_dot.rs

@@ -57,7 +57,10 @@ fn complete_methods(acc: &mut Completions, ctx: &CompletionContext, receiver: &T
         let mut seen_methods = FxHashSet::default();
         let traits_in_scope = ctx.scope().traits_in_scope();
         receiver.iterate_method_candidates(ctx.db, krate, &traits_in_scope, None, |_ty, func| {
-            if func.has_self_param(ctx.db) && seen_methods.insert(func.name(ctx.db)) {
+            if func.has_self_param(ctx.db)
+                && ctx.scope().module().map_or(true, |m| func.is_visible_from(ctx.db, m))
+                && seen_methods.insert(func.name(ctx.db))
+            {
                 acc.add_function(ctx, func);
             }
             None::<()>
@@ -307,6 +310,39 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_method_completion_private() {
+        assert_debug_snapshot!(
+            do_ref_completion(
+                r"
+            struct A {}
+            mod m {
+                impl super::A {
+                    fn private_method(&self) {}
+                    pub(super) fn the_method(&self) {}
+                }
+            }
+            fn foo(a: A) {
+               a.<|>
+            }
+            ",
+            ),
+            @r###"
+        [
+            CompletionItem {
+                label: "the_method()",
+                source_range: [256; 256),
+                delete: [256; 256),
+                insert: "the_method()$0",
+                kind: Method,
+                lookup: "the_method",
+                detail: "pub(super) fn the_method(&self)",
+            },
+        ]
+        "###
+        );
+    }
+
     #[test]
     fn test_trait_method_completion() {
         assert_debug_snapshot!(

crates/ra_ide/src/completion/complete_path.rs

@@ -1,6 +1,6 @@
 //! Completion of paths, including when writing a single name.
 
-use hir::{Adt, PathResolution, ScopeDef};
+use hir::{Adt, HasVisibility, PathResolution, ScopeDef};
 use ra_syntax::AstNode;
 use test_utils::tested_by;
 
@@ -15,9 +15,10 @@ pub(super) fn complete_path(acc: &mut Completions, ctx: &CompletionContext) {
         Some(PathResolution::Def(def)) => def,
         _ => return,
     };
+    let context_module = ctx.scope().module();
     match def {
         hir::ModuleDef::Module(module) => {
-            let module_scope = module.scope(ctx.db);
+            let module_scope = module.scope(ctx.db, context_module);
             for (name, def) in module_scope {
                 if ctx.use_item_syntax.is_some() {
                     if let ScopeDef::Unknown = def {
@@ -51,6 +52,9 @@ pub(super) fn complete_path(acc: &mut Completions, ctx: &CompletionContext) {
             if let Some(krate) = krate {
                 let traits_in_scope = ctx.scope().traits_in_scope();
                 ty.iterate_path_candidates(ctx.db, krate, &traits_in_scope, None, |_ty, item| {
+                    if context_module.map_or(false, |m| !item.is_visible_from(ctx.db, m)) {
+                        return None;
+                    }
                     match item {
                         hir::AssocItem::Function(func) => {
                             if !func.has_self_param(ctx.db) {
@@ -64,6 +68,9 @@ pub(super) fn complete_path(acc: &mut Completions, ctx: &CompletionContext) {
                 });
 
                 ty.iterate_impl_items(ctx.db, krate, |item| {
+                    if context_module.map_or(false, |m| !item.is_visible_from(ctx.db, m)) {
+                        return None;
+                    }
                     match item {
                         hir::AssocItem::Function(_) | hir::AssocItem::Const(_) => {}
                         hir::AssocItem::TypeAlias(ty) => acc.add_type_alias(ctx, ty),
@@ -74,6 +81,9 @@ pub(super) fn complete_path(acc: &mut Completions, ctx: &CompletionContext) {
         }
         hir::ModuleDef::Trait(t) => {
             for item in t.items(ctx.db) {
+                if context_module.map_or(false, |m| !item.is_visible_from(ctx.db, m)) {
+                    continue;
+                }
                 match item {
                     hir::AssocItem::Function(func) => {
                         if !func.has_self_param(ctx.db) {
@@ -169,6 +179,41 @@ mod tests {
         );
     }
 
+    #[test]
+    fn path_visibility() {
+        assert_debug_snapshot!(
+            do_reference_completion(
+                r"
+                use self::my::<|>;
+
+                mod my {
+                    struct Bar;
+                    pub struct Foo;
+                    pub use Bar as PublicBar;
+                }
+                "
+            ),
+            @r###"
+        [
+            CompletionItem {
+                label: "Foo",
+                source_range: [31; 31),
+                delete: [31; 31),
+                insert: "Foo",
+                kind: Struct,
+            },
+            CompletionItem {
+                label: "PublicBar",
+                source_range: [31; 31),
+                delete: [31; 31),
+                insert: "PublicBar",
+                kind: Struct,
+            },
+        ]
+        "###
+        );
+    }
+
     #[test]
     fn completes_use_item_starting_with_self() {
         assert_debug_snapshot!(
@@ -177,7 +222,7 @@ mod tests {
                 use self::m::<|>;
 
                 mod m {
-                    struct Bar;
+                    pub struct Bar;
                 }
                 "
             ),
@@ -501,6 +546,60 @@ mod tests {
         );
     }
 
+    #[test]
+    fn associated_item_visibility() {
+        assert_debug_snapshot!(
+            do_reference_completion(
+                "
+                //- /lib.rs
+                struct S;
+
+                mod m {
+                    impl super::S {
+                        pub(super) fn public_method() { }
+                        fn private_method() { }
+                        pub(super) type PublicType = u32;
+                        type PrivateType = u32;
+                        pub(super) const PUBLIC_CONST: u32 = 1;
+                        const PRIVATE_CONST: u32 = 1;
+                    }
+                }
+
+                fn foo() { let _ = S::<|> }
+                "
+            ),
+            @r###"
+        [
+            CompletionItem {
+                label: "PUBLIC_CONST",
+                source_range: [302; 302),
+                delete: [302; 302),
+                insert: "PUBLIC_CONST",
+                kind: Const,
+                detail: "pub(super) const PUBLIC_CONST: u32 = 1;",
+            },
+            CompletionItem {
+                label: "PublicType",
+                source_range: [302; 302),
+                delete: [302; 302),
+                insert: "PublicType",
+                kind: TypeAlias,
+                detail: "pub(super) type PublicType = u32;",
+            },
+            CompletionItem {
+                label: "public_method()",
+                source_range: [302; 302),
+                delete: [302; 302),
+                insert: "public_method()$0",
+                kind: Function,
+                lookup: "public_method",
+                detail: "pub(super) fn public_method()",
+            },
+        ]
+        "###
+        );
+    }
+
     #[test]
     fn completes_enum_associated_method() {
         assert_debug_snapshot!(