Skip to content

fix: Prevent public re-export of private item #18390

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

Merged
Veykril merged 1 commit into from
Oct 24, 2024
Merged

fix: Prevent public re-export of private item #18390

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
4 changes: 2 additions & 2 deletions crates/hir-def/src/find_path.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,7 +1025,7 @@ pub mod ast {
check_found_path(
r#"
mod bar {
mod foo { pub(super) struct S; }
mod foo { pub(crate) struct S; }
pub(crate) use foo::*;
}
$0
Expand All @@ -1047,7 +1047,7 @@ $0
check_found_path(
r#"
mod bar {
mod foo { pub(super) struct S; }
mod foo { pub(crate) struct S; }
pub(crate) use foo::S as U;
Copy link
Member Author

@ShoyuVanilla ShoyuVanilla Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case was wrong. It emits E0364 on rustc

mod bar {
    mod foo { pub(super) struct S; }
    pub(crate) use foo::S as U;
}

fn main() {}
error[E0364]: `S` is private, and cannot be re-exported
 --> src/main.rs:3:20
  |
3 |     pub(crate) use foo::S as U;
  |                    ^^^^^^^^^^^
  |
note: consider marking `S` as `pub` in the imported module
 --> src/main.rs:3:20
  |
3 |     pub(crate) use foo::S as U;
  |                    ^^^^^^^^^^^

}
$0
Expand Down
22 changes: 15 additions & 7 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 @@ -985,12 +985,8 @@ impl DefCollector<'_> {
for (name, res) in resolutions {
match name {
Some(name) => {
changed |= self.push_res_and_update_glob_vis(
module_id,
name,
res.with_visibility(vis),
import,
);
changed |=
self.push_res_and_update_glob_vis(module_id, name, *res, vis, import);
}
None => {
let tr = match res.take_types() {
Expand Down Expand Up @@ -1043,10 +1039,11 @@ impl DefCollector<'_> {
.collect::<Vec<_>>();

for (glob_importing_module, glob_import_vis, use_) in glob_imports {
let vis = glob_import_vis.min(vis, &self.def_map).unwrap_or(glob_import_vis);
self.update_recursive(
glob_importing_module,
resolutions,
glob_import_vis,
vis,
Some(ImportType::Glob(use_)),
depth + 1,
);
Expand All @@ -1058,8 +1055,19 @@ impl DefCollector<'_> {
module_id: LocalModuleId,
name: &Name,
mut defs: PerNs,
vis: Visibility,
def_import_type: Option<ImportType>,
) -> bool {
if let Some((_, v, _)) = defs.types.as_mut() {
*v = v.min(vis, &self.def_map).unwrap_or(vis);
}
if let Some((_, v, _)) = defs.values.as_mut() {
*v = v.min(vis, &self.def_map).unwrap_or(vis);
}
if let Some((_, v, _)) = defs.macros.as_mut() {
*v = v.min(vis, &self.def_map).unwrap_or(vis);
}

let mut changed = false;

if let Some(ImportType::Glob(_)) = def_import_type {
Expand Down
39 changes: 39 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 @@ -412,3 +412,42 @@ use reexport::*;
"#]],
);
}

#[test]
fn regression_18308() {
check(
r#"
use outer::*;

mod outer {
mod inner_superglob {
pub use super::*;
}

// The importing order matters!
pub use inner_superglob::*;
use super::glob_target::*;
}

mod glob_target {
pub struct ShouldBePrivate;
}
"#,
expect![[r#"
crate
glob_target: t
outer: t

crate::glob_target
ShouldBePrivate: t v

crate::outer
ShouldBePrivate: t v
inner_superglob: t

crate::outer::inner_superglob
ShouldBePrivate: t v
inner_superglob: t
"#]],
);
}
42 changes: 42 additions & 0 deletions crates/hir-def/src/visibility.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,11 @@ impl Visibility {
return None;
}

let def_block = def_map.block_id();
if (mod_a.containing_block(), mod_b.containing_block()) != (def_block, def_block) {
return None;
}

let mut a_ancestors =
iter::successors(Some(mod_a.local_id), |&m| def_map[m].parent);
let mut b_ancestors =
Expand All @@ -210,6 +215,43 @@ impl Visibility {
}
}
}

/// Returns the least permissive visibility of `self` and `other`.
///
/// If there is no subset relation between `self` and `other`, returns `None` (ie. they're only
/// visible in unrelated modules).
pub(crate) fn min(self, other: Visibility, def_map: &DefMap) -> Option<Visibility> {
match (self, other) {
(vis, Visibility::Public) | (Visibility::Public, vis) => Some(vis),
(Visibility::Module(mod_a, expl_a), Visibility::Module(mod_b, expl_b)) => {
if mod_a.krate != mod_b.krate {
return None;
}

let def_block = def_map.block_id();
if (mod_a.containing_block(), mod_b.containing_block()) != (def_block, def_block) {
return None;
}

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
return Some(Visibility::Module(mod_a, expl_b));
}

if b_ancestors.any(|m| m == mod_a.local_id) {
// A is above B
return Some(Visibility::Module(mod_b, expl_a));
}

None
}
}
}
}

/// Whether the item was imported through an explicit `pub(crate) use` or just a `use` without
Expand Down