From 76aaf17794c37cb134fc968d04e1982958af12fd Mon Sep 17 00:00:00 2001 From: Patryk Wychowaniec Date: Fri, 5 Jan 2024 11:00:29 +0100 Subject: [PATCH] Suggest `pub(crate)` imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 31 +++++++++++++++- crates/hir-def/src/item_scope.rs | 4 +-- crates/hir-def/src/nameres.rs | 3 +- crates/hir-def/src/nameres/path_resolution.rs | 13 ++++--- crates/hir-def/src/visibility.rs | 30 +++++++++++----- crates/hir-expand/src/mod_path.rs | 4 +++ crates/hir-ty/src/display.rs | 2 +- .../ide-assists/src/handlers/auto_import.rs | 35 +++++++++++++++++++ crates/ide-db/src/search.rs | 2 +- 9 files changed, 106 insertions(+), 18 deletions(-) diff --git a/crates/hir-def/src/find_path.rs b/crates/hir-def/src/find_path.rs index 4737b48703d..e8086be86f4 100644 --- a/crates/hir-def/src/find_path.rs +++ b/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( diff --git a/crates/hir-def/src/item_scope.rs b/crates/hir-def/src/item_scope.rs index 4902f24e2e3..0a6ba88065d 100644 --- a/crates/hir-def/src/item_scope.rs +++ b/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()); } } diff --git a/crates/hir-def/src/nameres.rs b/crates/hir-def/src/nameres.rs index 52a981fd19e..a97f57f5531 100644 --- a/crates/hir-def/src/nameres.rs +++ b/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, diff --git a/crates/hir-def/src/nameres/path_resolution.rs b/crates/hir-def/src/nameres/path_resolution.rs index be3438e427d..700264839b9 100644 --- a/crates/hir-def/src/nameres/path_resolution.rs +++ b/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); } } diff --git a/crates/hir-def/src/visibility.rs b/crates/hir-def/src/visibility.rs index 49688c5ee9c..163484e241b 100644 --- a/crates/hir-def/src/visibility.rs +++ b/crates/hir-def/src/visibility.rs @@ -94,7 +94,7 @@ 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, } @@ -102,7 +102,7 @@ pub enum Visibility { 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 { 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, diff --git a/crates/hir-expand/src/mod_path.rs b/crates/hir-expand/src/mod_path.rs index 30b8c189f52..4618dcec70b 100644 --- a/crates/hir-expand/src/mod_path.rs +++ b/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 diff --git a/crates/hir-ty/src/display.rs b/crates/hir-ty/src/display.rs index d81926f7c97..b01f742c3d1 100644 --- a/crates/hir-ty/src/display.rs +++ b/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 { diff --git a/crates/ide-assists/src/handlers/auto_import.rs b/crates/ide-assists/src/handlers/auto_import.rs index 1f785b5d0a8..43c2f820274 100644 --- a/crates/ide-assists/src/handlers/auto_import.rs +++ b/crates/ide-assists/src/handlers/auto_import.rs @@ -1548,4 +1548,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; + } +} +"#, + ); + } } diff --git a/crates/ide-db/src/search.rs b/crates/ide-db/src/search.rs index dbef3602682..6ea604740c6 100644 --- a/crates/ide-db/src/search.rs +++ b/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()); }