fix: Don't show qualified path completions for private items

Author: Veykril

crates/hir-def/src/nameres.rs

@@ -219,7 +219,7 @@ impl DefMap {
 
         let edition = crate_graph[krate].edition;
         let origin = ModuleOrigin::CrateRoot { definition: crate_graph[krate].root_file_id };
-        let def_map = DefMap::empty(krate, edition, origin);
+        let def_map = DefMap::empty(krate, edition, ModuleData::new(origin, Visibility::Public));
         let def_map = collector::collect_defs(
             db,
             def_map,
@@ -241,30 +241,26 @@ impl DefMap {
             return None;
         }
 
-        let block_info = BlockInfo { block: block_id, parent: block.module };
-
         let parent_map = block.module.def_map(db);
-        let mut def_map = DefMap::empty(
-            block.module.krate,
-            parent_map.edition,
-            ModuleOrigin::BlockExpr { block: block.ast_id },
-        );
-        def_map.block = Some(block_info);
+        let krate = block.module.krate;
+        let local_id = LocalModuleId::from_raw(la_arena::RawIdx::from(0));
+        // NB: we use `None` as block here, which would be wrong for implicit
+        // modules declared by blocks with items. At the moment, we don't use
+        // this visibility for anything outside IDE, so that's probably OK.
+        let visibility = Visibility::Module(ModuleId { krate, local_id, block: None });
+        let module_data =
+            ModuleData::new(ModuleOrigin::BlockExpr { block: block.ast_id }, visibility);
+
+        let mut def_map = DefMap::empty(krate, parent_map.edition, module_data);
+        def_map.block = Some(BlockInfo { block: block_id, parent: block.module });
 
         let def_map = collector::collect_defs(db, def_map, tree_id);
         Some(Arc::new(def_map))
     }
 
-    fn empty(krate: CrateId, edition: Edition, root_module_origin: ModuleOrigin) -> DefMap {
+    fn empty(krate: CrateId, edition: Edition, module_data: ModuleData) -> DefMap {
         let mut modules: Arena<ModuleData> = Arena::default();
-
-        let local_id = LocalModuleId::from_raw(la_arena::RawIdx::from(0));
-        // NB: we use `None` as block here, which would be wrong for implicit
-        // modules declared by blocks with items. At the moment, we don't use
-        // this visibility for anything outside IDE, so that's probably OK.
-        let visibility = Visibility::Module(ModuleId { krate, local_id, block: None });
-        let root = modules.alloc(ModuleData::new(root_module_origin, visibility));
-        assert_eq!(local_id, root);
+        let root = modules.alloc(module_data);
 
         DefMap {
             _c: Count::new(),

crates/hir-def/src/nameres/collector.rs

@@ -2141,7 +2141,8 @@ mod tests {
 
         let edition = db.crate_graph()[krate].edition;
         let module_origin = ModuleOrigin::CrateRoot { definition: file_id };
-        let def_map = DefMap::empty(krate, edition, module_origin);
+        let def_map =
+            DefMap::empty(krate, edition, ModuleData::new(module_origin, Visibility::Public));
         do_collect_defs(&db, def_map)
     }
 

crates/hir/src/display.rs

@@ -505,7 +505,7 @@ impl HirDisplay for Module {
         // FIXME: Module doesn't have visibility saved in data.
         match self.name(f.db) {
             Some(name) => write!(f, "mod {}", name),
-            None if self.is_crate_root(f.db) => match self.krate().display_name(f.db) {
+            None if self.is_crate_root(f.db) => match self.krate(f.db).display_name(f.db) {
                 Some(name) => write!(f, "extern crate {}", name),
                 None => f.write_str("extern crate {unknown}"),
             },

crates/hir/src/lib.rs

@@ -3573,3 +3573,27 @@ impl HasCrate for Macro {
         self.module(db).krate()
     }
 }
+
+impl HasCrate for Trait {
+    fn krate(&self, db: &dyn HirDatabase) -> Crate {
+        self.module(db).krate()
+    }
+}
+
+impl HasCrate for Static {
+    fn krate(&self, db: &dyn HirDatabase) -> Crate {
+        self.module(db).krate()
+    }
+}
+
+impl HasCrate for Adt {
+    fn krate(&self, db: &dyn HirDatabase) -> Crate {
+        self.module(db).krate()
+    }
+}
+
+impl HasCrate for Module {
+    fn krate(&self, _: &dyn HirDatabase) -> Crate {
+        Module::krate(*self)
+    }
+}

crates/ide-completion/src/completions.rs

@@ -174,13 +174,19 @@ impl Completions {
         local_name: hir::Name,
         resolution: hir::ScopeDef,
     ) {
-        if ctx.is_scope_def_hidden(resolution) {
-            cov_mark::hit!(qualified_path_doc_hidden);
-            return;
-        }
+        let is_private_editable = match ctx.def_is_visible(&resolution) {
+            Visible::Yes => false,
+            Visible::Editable => true,
+            Visible::No => return,
+        };
         self.add(
-            render_path_resolution(RenderContext::new(ctx), path_ctx, local_name, resolution)
-                .build(),
+            render_path_resolution(
+                RenderContext::new(ctx).private_editable(is_private_editable),
+                path_ctx,
+                local_name,
+                resolution,
+            )
+            .build(),
         );
     }
 
@@ -191,13 +197,19 @@ impl Completions {
         local_name: hir::Name,
         resolution: hir::ScopeDef,
     ) {
-        if ctx.is_scope_def_hidden(resolution) {
-            cov_mark::hit!(qualified_path_doc_hidden);
-            return;
-        }
+        let is_private_editable = match ctx.def_is_visible(&resolution) {
+            Visible::Yes => false,
+            Visible::Editable => true,
+            Visible::No => return,
+        };
         self.add(
-            render_pattern_resolution(RenderContext::new(ctx), pattern_ctx, local_name, resolution)
-                .build(),
+            render_pattern_resolution(
+                RenderContext::new(ctx).private_editable(is_private_editable),
+                pattern_ctx,
+                local_name,
+                resolution,
+            )
+            .build(),
         );
     }
 

crates/ide-completion/src/context.rs

@@ -33,6 +33,7 @@ pub(crate) enum PatternRefutability {
     Irrefutable,
 }
 
+#[derive(Debug)]
 pub(crate) enum Visible {
     Yes,
     Editable,
@@ -355,10 +356,11 @@ pub(crate) struct CompletionContext<'a> {
 
     pub(super) locals: FxHashMap<Name, Local>,
 
+    /// The module depth of the current module of the cursor position.
     /// - crate-root
     ///  - mod foo
     ///   - mod bar
-    /// Here depth will be 2: {[bar<->foo], [foo<->crate-root]}
+    /// Here depth will be 2
     pub(super) depth_from_crate_root: usize,
 }
 
@@ -383,6 +385,30 @@ impl<'a> CompletionContext<'a> {
         FamousDefs(&self.sema, self.krate)
     }
 
+    /// Checks if an item is visible and not `doc(hidden)` at the completion site.
+    pub(crate) fn def_is_visible(&self, item: &ScopeDef) -> Visible {
+        match item {
+            ScopeDef::ModuleDef(def) => match def {
+                hir::ModuleDef::Module(it) => self.is_visible(it),
+                hir::ModuleDef::Function(it) => self.is_visible(it),
+                hir::ModuleDef::Adt(it) => self.is_visible(it),
+                hir::ModuleDef::Variant(it) => self.is_visible(it),
+                hir::ModuleDef::Const(it) => self.is_visible(it),
+                hir::ModuleDef::Static(it) => self.is_visible(it),
+                hir::ModuleDef::Trait(it) => self.is_visible(it),
+                hir::ModuleDef::TypeAlias(it) => self.is_visible(it),
+                hir::ModuleDef::Macro(it) => self.is_visible(it),
+                hir::ModuleDef::BuiltinType(_) => Visible::Yes,
+            },
+            ScopeDef::GenericParam(_)
+            | ScopeDef::ImplSelfType(_)
+            | ScopeDef::AdtSelfType(_)
+            | ScopeDef::Local(_)
+            | ScopeDef::Label(_)
+            | ScopeDef::Unknown => Visible::Yes,
+        }
+    }
+
     /// Checks if an item is visible and not `doc(hidden)` at the completion site.
     pub(crate) fn is_visible<I>(&self, item: &I) -> Visible
     where
@@ -393,14 +419,6 @@ impl<'a> CompletionContext<'a> {
         self.is_visible_impl(&vis, &attrs, item.krate(self.db))
     }
 
-    pub(crate) fn is_scope_def_hidden(&self, scope_def: ScopeDef) -> bool {
-        if let (Some(attrs), Some(krate)) = (scope_def.attrs(self.db), scope_def.krate(self.db)) {
-            return self.is_doc_hidden(&attrs, krate);
-        }
-
-        false
-    }
-
     /// Check if an item is `#[doc(hidden)]`.
     pub(crate) fn is_item_hidden(&self, item: &hir::ItemInNs) -> bool {
         let attrs = item.attrs(self.db);
@@ -468,6 +486,14 @@ impl<'a> CompletionContext<'a> {
         self.scope.process_all_names(&mut |name, def| f(name, def));
     }
 
+    fn is_scope_def_hidden(&self, scope_def: ScopeDef) -> bool {
+        if let (Some(attrs), Some(krate)) = (scope_def.attrs(self.db), scope_def.krate(self.db)) {
+            return self.is_doc_hidden(&attrs, krate);
+        }
+
+        false
+    }
+
     fn is_visible_impl(
         &self,
         vis: &hir::Visibility,

crates/ide-completion/src/tests/special.rs

@@ -419,9 +419,9 @@ mod m {
     pub use super::p::WrongType as RightType;
 }
 mod p {
-    fn wrong_fn() {}
-    const WRONG_CONST: u32 = 1;
-    struct WrongType {};
+    pub fn wrong_fn() {}
+    pub const WRONG_CONST: u32 = 1;
+    pub struct WrongType {};
 }
 "#,
         expect![[r#"
@@ -442,9 +442,9 @@ mod m {
     pub use super::p::WrongType as RightType;
 }
 mod p {
-    fn wrong_fn() {}
-    const WRONG_CONST: u32 = 1;
-    struct WrongType {};
+    pub fn wrong_fn() {}
+    pub const WRONG_CONST: u32 = 1;
+    pub struct WrongType {};
 }
 "#,
         r#"
@@ -456,9 +456,9 @@ mod m {
     pub use super::p::WrongType as RightType;
 }
 mod p {
-    fn wrong_fn() {}
-    const WRONG_CONST: u32 = 1;
-    struct WrongType {};
+    pub fn wrong_fn() {}
+    pub const WRONG_CONST: u32 = 1;
+    pub struct WrongType {};
 }
 "#,
     );
@@ -627,7 +627,6 @@ fn main() {
 
 #[test]
 fn respects_doc_hidden2() {
-    cov_mark::check!(qualified_path_doc_hidden);
     check(
         r#"
 //- /lib.rs crate:lib deps:dep

crates/ide/src/syntax_highlighting/highlight.rs

@@ -328,8 +328,10 @@ fn highlight_def(sema: &Semantics<RootDatabase>, krate: hir::Crate, def: Definit
         Definition::Field(_) => Highlight::new(HlTag::Symbol(SymbolKind::Field)),
         Definition::Module(module) => {
             let mut h = Highlight::new(HlTag::Symbol(SymbolKind::Module));
-            if module.parent(db).is_none() {
-                h |= HlMod::CrateRoot
+            if module.is_crate_root(db) {
+                h |= HlMod::CrateRoot;
+                // early return as we do not want to tag crate roots with public
+                return h;
             }
             h
         }