Skip to content

fix: Acknowledge pub(crate) imports in import suggestions #16265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion crates/hir-def/src/find_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions crates/hir-def/src/item_scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}

Expand Down
3 changes: 2 additions & 1 deletion crates/hir-def/src/nameres.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
13 changes: 9 additions & 4 deletions crates/hir-def/src/nameres/path_resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
}
Expand Down
30 changes: 22 additions & 8 deletions crates/hir-def/src/visibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
};

Expand Down Expand Up @@ -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;
}
Expand All @@ -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
Expand All @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions crates/hir-expand/src/mod_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion crates/hir-ty/src/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
35 changes: 35 additions & 0 deletions crates/ide-assists/src/handlers/auto_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
"#,
);
}
}
2 changes: 1 addition & 1 deletion crates/ide-db/src/search.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down