From 685fe4b8dcdb8336966d2240ceb94265bafaa0e6 Mon Sep 17 00:00:00 2001
From: Shoyu Vanilla <modulo641@gmail.com>
Date: Wed, 23 Oct 2024 22:10:53 +0900
Subject: [PATCH] fix: Prevent public reexport of private item

---
 crates/hir-def/src/find_path.rs           |  4 +--
 crates/hir-def/src/nameres/collector.rs   | 22 ++++++++----
 crates/hir-def/src/nameres/tests/globs.rs | 39 +++++++++++++++++++++
 crates/hir-def/src/visibility.rs          | 42 +++++++++++++++++++++++
 4 files changed, 98 insertions(+), 9 deletions(-)

diff --git a/crates/hir-def/src/find_path.rs b/crates/hir-def/src/find_path.rs
index f5e03e5281..a615abd1bb 100644
--- a/crates/hir-def/src/find_path.rs
+++ b/crates/hir-def/src/find_path.rs
@@ -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
@@ -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;
 }
 $0
diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs
index 22b9c2b4e3..b9aec93690 100644
--- a/crates/hir-def/src/nameres/collector.rs
+++ b/crates/hir-def/src/nameres/collector.rs
@@ -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() {
@@ -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,
             );
@@ -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 {
diff --git a/crates/hir-def/src/nameres/tests/globs.rs b/crates/hir-def/src/nameres/tests/globs.rs
index a2696055ca..543ab41cd5 100644
--- a/crates/hir-def/src/nameres/tests/globs.rs
+++ b/crates/hir-def/src/nameres/tests/globs.rs
@@ -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
+        "#]],
+    );
+}
diff --git a/crates/hir-def/src/visibility.rs b/crates/hir-def/src/visibility.rs
index 3aeb88047a..4edb683592 100644
--- a/crates/hir-def/src/visibility.rs
+++ b/crates/hir-def/src/visibility.rs
@@ -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 =
@@ -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