Skip to content

fix: let glob imports override other globs' visibility #14930

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: let glob imports override other globs' visibility #14930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 49 additions & 8 deletions crates/hir-def/src/item_scope.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,18 @@ pub struct PerNsGlobImports {
macros: FxHashSet<(LocalModuleId, Name)>,
}

impl PerNsGlobImports {
pub(crate) fn contains_type(&self, module_id: LocalModuleId, name: Name) -> bool {
self.types.contains(&(module_id, name))
}
pub(crate) fn contains_value(&self, module_id: LocalModuleId, name: Name) -> bool {
self.values.contains(&(module_id, name))
}
pub(crate) fn contains_macro(&self, module_id: LocalModuleId, name: Name) -> bool {
self.macros.contains(&(module_id, name))
}
}

#[derive(Debug, Default, PartialEq, Eq)]
pub struct ItemScope {
_c: Count<Self>,
Expand Down Expand Up @@ -300,16 +312,24 @@ impl ItemScope {
entry.insert(fld);
$changed = true;
}
Entry::Occupied(mut entry)
if matches!($def_import_type, ImportType::Named) =>
{
if $glob_imports.$field.remove(&$lookup) {
cov_mark::hit!(import_shadowed);
entry.insert(fld);
$changed = true;
Entry::Occupied(mut entry) => {
match $def_import_type {
ImportType::Glob => {
// Multiple globs may import the same item and they may
// override visibility from previously resolved globs. This is
// currently handled by `DefCollector`, because we need to
// compute the max visibility for items and we need `DefMap`
// for that.
}
ImportType::Named => {
if $glob_imports.$field.remove(&$lookup) {
cov_mark::hit!(import_shadowed);
entry.insert(fld);
$changed = true;
}
}
}
}
_ => {}
}
}
}};
Expand Down Expand Up @@ -416,6 +436,27 @@ impl ItemScope {
}
}

// These methods are a temporary measure only meant to be used by `DefCollector::push_res_and_update_glob_vis()`.
impl ItemScope {
pub(crate) fn update_visibility_types(&mut self, name: &Name, vis: Visibility) {
let res =
self.types.get_mut(name).expect("tried to update visibility of non-existent type");
res.1 = vis;
}

pub(crate) fn update_visibility_values(&mut self, name: &Name, vis: Visibility) {
let res =
self.values.get_mut(name).expect("tried to update visibility of non-existent value");
res.1 = vis;
}

pub(crate) fn update_visibility_macros(&mut self, name: &Name, vis: Visibility) {
let res =
self.macros.get_mut(name).expect("tried to update visibility of non-existent macro");
res.1 = vis;
}
}

impl PerNs {
pub(crate) fn from_def(def: ModuleDefId, v: Visibility, has_constructor: bool) -> PerNs {
match def {
Expand Down
87 changes: 82 additions & 5 deletions crates/hir-def/src/nameres/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -994,7 +994,7 @@ impl DefCollector<'_> {

fn update_recursive(
&mut self,
// The module for which `resolutions` have been resolve
// The module for which `resolutions` have been resolved.
module_id: LocalModuleId,
resolutions: &[(Option<Name>, PerNs)],
// All resolutions are imported with this visibility; the visibilities in
Expand All @@ -1012,10 +1012,9 @@ impl DefCollector<'_> {
for (name, res) in resolutions {
match name {
Some(name) => {
let scope = &mut self.def_map.modules[module_id].scope;
changed |= scope.push_res_with_import(
&mut self.from_glob_import,
(module_id, name.clone()),
changed |= self.push_res_and_update_glob_vis(
module_id,
name,
res.with_visibility(vis),
import_type,
);
Expand Down Expand Up @@ -1081,6 +1080,84 @@ impl DefCollector<'_> {
}
}

fn push_res_and_update_glob_vis(
&mut self,
module_id: LocalModuleId,
name: &Name,
mut defs: PerNs,
def_import_type: ImportType,
) -> bool {
let mut changed = false;

if let ImportType::Glob = def_import_type {
let prev_defs = self.def_map[module_id].scope.get(name);

// Multiple globs may import the same item and they may override visibility from
// previously resolved globs. Handle overrides here and leave the rest to
// `ItemScope::push_res_with_import()`.
if let Some((def, def_vis)) = defs.types {
if let Some((prev_def, prev_vis)) = prev_defs.types {
if def == prev_def
&& self.from_glob_import.contains_type(module_id, name.clone())
&& def_vis != prev_vis
&& def_vis.max(prev_vis, &self.def_map) == Some(def_vis)
{
changed = true;
// This import is being handled here, don't pass it down to
// `ItemScope::push_res_with_import()`.
defs.types = None;
self.def_map.modules[module_id]
.scope
.update_visibility_types(name, def_vis);
}
}
}

if let Some((def, def_vis)) = defs.values {
if let Some((prev_def, prev_vis)) = prev_defs.values {
if def == prev_def
&& self.from_glob_import.contains_value(module_id, name.clone())
&& def_vis != prev_vis
&& def_vis.max(prev_vis, &self.def_map) == Some(def_vis)
{
changed = true;
// See comment above.
defs.values = None;
self.def_map.modules[module_id]
.scope
.update_visibility_values(name, def_vis);
}
}
}

if let Some((def, def_vis)) = defs.macros {
if let Some((prev_def, prev_vis)) = prev_defs.macros {
if def == prev_def
&& self.from_glob_import.contains_macro(module_id, name.clone())
&& def_vis != prev_vis
&& def_vis.max(prev_vis, &self.def_map) == Some(def_vis)
{
changed = true;
// See comment above.
defs.macros = None;
self.def_map.modules[module_id]
.scope
.update_visibility_macros(name, def_vis);
}
}
}
}

changed |= self.def_map.modules[module_id].scope.push_res_with_import(
&mut self.from_glob_import,
(module_id, name.clone()),
defs,
def_import_type,
);

changed
}

fn resolve_macros(&mut self) -> ReachedFixedPoint {
let mut macros = mem::take(&mut self.unresolved_macros);
let mut resolved = Vec::new();
Expand Down
45 changes: 45 additions & 0 deletions crates/hir-def/src/nameres/tests/globs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,3 +367,48 @@ use event::Event;
"#]],
);
}

#[test]
fn glob_may_override_visibility() {
check(
r#"
mod reexport {
use crate::defs::*;
mod inner {
pub use crate::defs::{Trait, function, makro};
}
pub use inner::*;
}
mod defs {
pub trait Trait {}
pub fn function() {}
pub macro makro($t:item) { $t }
}
use reexport::*;
"#,
expect![[r#"
crate
Trait: t
defs: t
function: v
makro: m
reexport: t

crate::defs
Trait: t
function: v
makro: m

crate::reexport
Trait: t
function: v
inner: t
makro: m

crate::reexport::inner
Trait: t
function: v
makro: m
"#]],
);
}
4 changes: 2 additions & 2 deletions crates/hir-def/src/per_ns.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ impl PerNs {
self.types
.map(|it| ItemInNs::Types(it.0))
.into_iter()
.chain(self.values.map(|it| ItemInNs::Values(it.0)).into_iter())
.chain(self.macros.map(|it| ItemInNs::Macros(it.0)).into_iter())
.chain(self.values.map(|it| ItemInNs::Values(it.0)))
.chain(self.macros.map(|it| ItemInNs::Macros(it.0)))
}
}
12 changes: 4 additions & 8 deletions crates/hir-def/src/visibility.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,14 +188,10 @@ impl Visibility {
return None;
}

let mut a_ancestors = iter::successors(Some(mod_a.local_id), |&m| {
let parent_id = def_map[m].parent?;
Some(parent_id)
});
let mut b_ancestors = iter::successors(Some(mod_b.local_id), |&m| {
let parent_id = def_map[m].parent?;
Some(parent_id)
});
let mut a_ancestors =
iter::successors(Some(mod_a.local_id), |&m| def_map[m].parent);
let mut b_ancestors =
iter::successors(Some(mod_b.local_id), |&m| def_map[m].parent);

if a_ancestors.any(|m| m == mod_b.local_id) {
// B is above A
Expand Down