Auto merge of #16265 - Patryk27:suggest-pub-crate-imports, r=Veykril

fix: Acknowledge `pub(crate)` imports in import suggestions

rust-analyzer has logic that discounts suggesting `use`s for private imports, but that logic is unnecessarily strict - for instance given this code:

```rust
mod foo {
    pub struct Foo;
}

pub(crate) use self::foo::*;

mod bar {
    fn main() {
        Foo$0;
    }
}
```

... RA will suggest to add `use crate::foo::Foo;`, which not only makes the code overly verbose (especially in larger code bases), but also is disjoint with what rustc itself suggests.

This commit adjusts the logic, so that `pub(crate)` imports are taken into account when generating the suggestions; considering rustc's behavior, I think this change doesn't warrant any extra configuration flag.

Note that this is my first commit to RA, so I guess the approach taken here might be suboptimal - certainly feels somewhat hacky, maybe there's some better way of finding out the optimal import path 😅

crates/hir-def/src/find_path.rs

@@ -551,7 +551,18 @@ fn find_local_import_locations(
         if let Some((name, vis)) = data.scope.name_of(item) {
             if vis.is_visible_from(db, from) {
                 let is_private = match vis {
-                    Visibility::Module(private_to) => private_to.local_id == module.local_id,
+                    Visibility::Module(private_mod, private_vis) => {
+                        if private_mod == def_map.module_id(DefMap::ROOT)
+                            && private_vis.is_explicit()
+                        {
+                            // Treat `pub(crate)` imports as non-private, so
+                            // that we suggest adding `use crate::Foo;` instead
+                            // of `use crate::foo::Foo;` etc.
+                            false
+                        } else {
+                            private_mod.local_id == module.local_id
+                        }
+                    }
                     Visibility::Public => false,
                 };
                 let is_original_def = match item.as_module_def_id() {
@@ -1021,6 +1032,24 @@ $0
         );
     }
 
+    #[test]
+    fn promote_pub_crate_imports() {
+        check_found_path(
+            r#"
+//- /main.rs
+mod foo;
+pub mod bar { pub struct S; }
+pub(crate) use bar::S;
+//- /foo.rs
+$0
+        "#,
+            "crate::S",
+            "crate::S",
+            "crate::S",
+            "crate::S",
+        );
+    }
+
     #[test]
     fn import_cycle() {
         check_found_path(

crates/hir-def/src/item_scope.rs

@@ -628,14 +628,14 @@ impl ItemScope {
             .chain(self.values.values_mut().map(|(def, vis, _)| (def, vis)))
             .map(|(_, v)| v)
             .chain(self.unnamed_trait_imports.values_mut().map(|(vis, _)| vis))
-            .for_each(|vis| *vis = Visibility::Module(this_module));
+            .for_each(|vis| *vis = Visibility::Module(this_module, Default::default()));
 
         for (mac, vis, import) in self.macros.values_mut() {
             if matches!(mac, MacroId::ProcMacroId(_) if import.is_none()) {
                 continue;
             }
 
-            *vis = Visibility::Module(this_module);
+            *vis = Visibility::Module(this_module, Default::default());
         }
     }
 

crates/hir-def/src/nameres.rs

@@ -332,7 +332,8 @@ impl DefMap {
         // 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 visibility =
+            Visibility::Module(ModuleId { krate, local_id, block: None }, Default::default());
         let module_data = ModuleData::new(
             ModuleOrigin::BlockExpr { block: block.ast_id, id: block_id },
             visibility,

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

@@ -21,7 +21,7 @@ use crate::{
     nameres::{sub_namespace_match, BlockInfo, BuiltinShadowMode, DefMap, MacroSubNs},
     path::{ModPath, PathKind},
     per_ns::PerNs,
-    visibility::{RawVisibility, Visibility},
+    visibility::{RawVisibility, Visibility, VisibilityExplicity},
     AdtId, CrateId, EnumVariantId, LocalModuleId, ModuleDefId,
 };
 
@@ -94,8 +94,13 @@ impl DefMap {
                     return None;
                 }
                 let types = result.take_types()?;
+                let mv = if path.is_pub_crate() {
+                    VisibilityExplicity::Explicit
+                } else {
+                    VisibilityExplicity::Implicit
+                };
                 match types {
-                    ModuleDefId::ModuleId(m) => Visibility::Module(m),
+                    ModuleDefId::ModuleId(m) => Visibility::Module(m, mv),
                     // error: visibility needs to refer to module
                     _ => {
                         return None;
@@ -108,11 +113,11 @@ impl DefMap {
         // In block expressions, `self` normally refers to the containing non-block module, and
         // `super` to its parent (etc.). However, visibilities must only refer to a module in the
         // DefMap they're written in, so we restrict them when that happens.
-        if let Visibility::Module(m) = vis {
+        if let Visibility::Module(m, mv) = vis {
             // ...unless we're resolving visibility for an associated item in an impl.
             if self.block_id() != m.block && !within_impl {
                 cov_mark::hit!(adjust_vis_in_block_def_map);
-                vis = Visibility::Module(self.module_id(Self::ROOT));
+                vis = Visibility::Module(self.module_id(Self::ROOT), mv);
                 tracing::debug!("visibility {:?} points outside DefMap, adjusting to {:?}", m, vis);
             }
         }

crates/hir-def/src/visibility.rs

@@ -94,15 +94,15 @@ impl RawVisibility {
 #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
 pub enum Visibility {
     /// Visibility is restricted to a certain module.
-    Module(ModuleId),
+    Module(ModuleId, VisibilityExplicity),
     /// Visibility is unrestricted.
     Public,
 }
 
 impl Visibility {
     pub fn is_visible_from(self, db: &dyn DefDatabase, from_module: ModuleId) -> bool {
         let to_module = match self {
-            Visibility::Module(m) => m,
+            Visibility::Module(m, _) => m,
             Visibility::Public => return true,
         };
         // if they're not in the same crate, it can't be visible
@@ -124,7 +124,7 @@ impl Visibility {
         mut from_module: LocalModuleId,
     ) -> bool {
         let mut to_module = match self {
-            Visibility::Module(m) => m,
+            Visibility::Module(m, _) => m,
             Visibility::Public => return true,
         };
 
@@ -181,9 +181,9 @@ impl Visibility {
     /// visible in unrelated modules).
     pub(crate) fn max(self, other: Visibility, def_map: &DefMap) -> Option<Visibility> {
         match (self, other) {
-            (Visibility::Module(_) | Visibility::Public, Visibility::Public)
-            | (Visibility::Public, Visibility::Module(_)) => Some(Visibility::Public),
-            (Visibility::Module(mod_a), Visibility::Module(mod_b)) => {
+            (Visibility::Module(_, _) | Visibility::Public, Visibility::Public)
+            | (Visibility::Public, Visibility::Module(_, _)) => Some(Visibility::Public),
+            (Visibility::Module(mod_a, vis_a), Visibility::Module(mod_b, vis_b)) => {
                 if mod_a.krate != mod_b.krate {
                     return None;
                 }
@@ -199,12 +199,12 @@ impl Visibility {
 
                 if a_ancestors.any(|m| m == mod_b.local_id) {
                     // B is above A
-                    return Some(Visibility::Module(mod_b));
+                    return Some(Visibility::Module(mod_b, vis_b));
                 }
 
                 if b_ancestors.any(|m| m == mod_a.local_id) {
                     // A is above B
-                    return Some(Visibility::Module(mod_a));
+                    return Some(Visibility::Module(mod_a, vis_a));
                 }
 
                 None
@@ -213,6 +213,20 @@ impl Visibility {
     }
 }
 
+/// Whether the item was imported through `pub(crate) use` or just `use`.
+#[derive(Debug, Default, Copy, Clone, PartialEq, Eq, Hash)]
+pub enum VisibilityExplicity {
+    Explicit,
+    #[default]
+    Implicit,
+}
+
+impl VisibilityExplicity {
+    pub fn is_explicit(&self) -> bool {
+        matches!(self, Self::Explicit)
+    }
+}
+
 /// Resolve visibility of all specific fields of a struct or union variant.
 pub(crate) fn field_visibilities_query(
     db: &dyn DefDatabase,

crates/hir-expand/src/mod_path.rs

@@ -96,6 +96,10 @@ impl ModPath {
         self.kind == PathKind::Super(0) && self.segments.is_empty()
     }
 
+    pub fn is_pub_crate(&self) -> bool {
+        self.kind == PathKind::Crate && self.segments.is_empty()
+    }
+
     #[allow(non_snake_case)]
     pub fn is_Self(&self) -> bool {
         self.kind == PathKind::Plain

crates/hir-ty/src/display.rs

@@ -1629,7 +1629,7 @@ pub fn write_visibility(
 ) -> Result<(), HirDisplayError> {
     match vis {
         Visibility::Public => write!(f, "pub "),
-        Visibility::Module(vis_id) => {
+        Visibility::Module(vis_id, _) => {
             let def_map = module_id.def_map(f.db.upcast());
             let root_module_id = def_map.module_id(DefMap::ROOT);
             if vis_id == module_id {

crates/ide-assists/src/handlers/auto_import.rs

@@ -1551,4 +1551,39 @@ use foo::Foo$0;
 ",
         );
     }
+
+    #[test]
+    fn considers_pub_crate() {
+        check_assist(
+            auto_import,
+            r#"
+mod foo {
+    pub struct Foo;
+}
+
+pub(crate) use self::foo::*;
+
+mod bar {
+    fn main() {
+        Foo$0;
+    }
+}
+"#,
+            r#"
+mod foo {
+    pub struct Foo;
+}
+
+pub(crate) use self::foo::*;
+
+mod bar {
+    use crate::Foo;
+
+    fn main() {
+        Foo;
+    }
+}
+"#,
+        );
+    }
 }

crates/ide-db/src/search.rs

@@ -356,7 +356,7 @@ impl Definition {
         if let Some(Visibility::Public) = vis {
             return SearchScope::reverse_dependencies(db, module.krate());
         }
-        if let Some(Visibility::Module(module)) = vis {
+        if let Some(Visibility::Module(module, _)) = vis {
             return SearchScope::module_and_children(db, module.into());
         }