Auto merge of #17715 - Throne3d:fix/glob-may-override-vis-2, r=Veykril

fix: let glob imports override other globs' visibility

Follow up to #14930

Fixes #11858
Fixes #14902
Fixes #17704

I haven't reworked the code here at all - I don't feel confident in the codebase to do so - just rebased it onto the current main branch and fixed conflicts.

I'm not _entirely_ sure I understand the structure of the `check` function in `crates/hir-def/src/nameres` tests. I think the change to the test expectation from #14930 is correct, marking the `crate::reexport::inner` imports with `i`, and I understand it to mean there's a specific token in the import that we can match it to (in this case, `Trait`, `function` and `makro` of `pub use crate::defs::{Trait, function, makro};` respectively), but I had some trouble understanding the meaning of the different parts of `PerNs` to be sure.
Does this make sense?

I tested building and using RA locally with `cargo xtask install` and after this change the documentation for `arrow_array::ArrowPrimitiveType` seems to be picked up correctly!

crates/hir-def/src/item_scope.rs

@@ -61,6 +61,18 @@ pub struct ImportId {
     pub idx: Idx<ast::UseTree>,
 }
 
+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 {
     /// Defs visible in this scope. This includes `declarations`, but also
@@ -510,38 +522,48 @@ impl ItemScope {
                     entry.insert(fld);
                     changed = true;
                 }
-                Entry::Occupied(mut entry) if !matches!(import, Some(ImportType::Glob(..))) => {
+                Entry::Occupied(mut entry) => {
-                    if glob_imports.types.remove(&lookup) {
+                    match import {
-                        let import = match import {
+                        Some(ImportType::Glob(..)) => {
-                            Some(ImportType::ExternCrate(extern_crate)) => {
+                            // Multiple globs may import the same item and they may
-                                Some(ImportOrExternCrate::ExternCrate(extern_crate))
+                            // override visibility from previously resolved globs. This is
-                            }
+                            // currently handled by `DefCollector`, because we need to
-                            Some(ImportType::Import(import)) => {
+                            // compute the max visibility for items and we need `DefMap`
-                                Some(ImportOrExternCrate::Import(import))
+                            // for that.
-                            }
+                        }
-                            None | Some(ImportType::Glob(_)) => None,
+                        _ => {
-                        };
+                            if glob_imports.types.remove(&lookup) {
-                        let prev = std::mem::replace(&mut fld.2, import);
+                                let import = match import {
-                        if let Some(import) = import {
+                                    Some(ImportType::ExternCrate(extern_crate)) => {
-                            self.use_imports_types.insert(
+                                        Some(ImportOrExternCrate::ExternCrate(extern_crate))
-                                import,
+                                    }
-                                match prev {
+                                    Some(ImportType::Import(import)) => {
-                                    Some(ImportOrExternCrate::Import(import)) => {
+                                        Some(ImportOrExternCrate::Import(import))
-                                        ImportOrDef::Import(import)
+                                    }
-                                    }
+                                    None | Some(ImportType::Glob(_)) => None,
-                                    Some(ImportOrExternCrate::ExternCrate(import)) => {
+                                };
-                                        ImportOrDef::ExternCrate(import)
+                                let prev = std::mem::replace(&mut fld.2, import);
-                                    }
+                                if let Some(import) = import {
-                                    None => ImportOrDef::Def(fld.0),
+                                    self.use_imports_types.insert(
-                                },
+                                        import,
-                            );
+                                        match prev {
+                                            Some(ImportOrExternCrate::Import(import)) => {
+                                                ImportOrDef::Import(import)
+                                            }
+                                            Some(ImportOrExternCrate::ExternCrate(import)) => {
+                                                ImportOrDef::ExternCrate(import)
+                                            }
+                                            None => ImportOrDef::Def(fld.0),
+                                        },
+                                    );
+                                }
+                                cov_mark::hit!(import_shadowed);
+                                entry.insert(fld);
+                                changed = true;
+                            }
                         }
-                        cov_mark::hit!(import_shadowed);
-                        entry.insert(fld);
-                        changed = true;
                     }
                 }
-                _ => {}
             }
         }
 
@@ -756,6 +778,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,

crates/hir-def/src/nameres/collector.rs

@@ -1025,7 +1025,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
@@ -1043,10 +1043,9 @@ impl DefCollector<'_> {
         for (name, res) in resolutions {
             match name {
                 Some(name) => {
-                    let scope = &mut self.def_map.modules[module_id].scope;
+                    changed |= self.push_res_and_update_glob_vis(
-                    changed |= scope.push_res_with_import(
+                        module_id,
-                        &mut self.from_glob_import,
+                        name,
-                        (module_id, name.clone()),
                         res.with_visibility(vis),
                         import,
                     );
@@ -1112,6 +1111,84 @@ impl DefCollector<'_> {
         }
     }
 
+    fn push_res_and_update_glob_vis(
+        &mut self,
+        module_id: LocalModuleId,
+        name: &Name,
+        mut defs: PerNs,
+        def_import_type: Option<ImportType>,
+    ) -> bool {
+        let mut changed = false;
+
+        if let Some(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();

crates/hir-def/src/nameres/tests/globs.rs

@@ -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: ti
+            function: vi
+            makro: mi
+        "#]],
+    );
+}