Unnamed repository; edit this file 'description' to name the repository.
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!
| -rw-r--r-- | crates/hir-def/src/item_scope.rs | 97 | ||||
| -rw-r--r-- | crates/hir-def/src/nameres/collector.rs | 87 | ||||
| -rw-r--r-- | crates/hir-def/src/nameres/tests/globs.rs | 45 |
3 files changed, 197 insertions, 32 deletions
diff --git a/crates/hir-def/src/item_scope.rs b/crates/hir-def/src/item_scope.rs index 86c3e0f041..df6b1f55c1 100644 --- a/crates/hir-def/src/item_scope.rs +++ b/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(..))) => { - if glob_imports.types.remove(&lookup) { - let import = match import { - Some(ImportType::ExternCrate(extern_crate)) => { - Some(ImportOrExternCrate::ExternCrate(extern_crate)) - } - Some(ImportType::Import(import)) => { - Some(ImportOrExternCrate::Import(import)) - } - None | Some(ImportType::Glob(_)) => None, - }; - let prev = std::mem::replace(&mut fld.2, import); - if let Some(import) = import { - self.use_imports_types.insert( - import, - match prev { - Some(ImportOrExternCrate::Import(import)) => { - ImportOrDef::Import(import) + Entry::Occupied(mut entry) => { + match import { + Some(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. + } + _ => { + if glob_imports.types.remove(&lookup) { + let import = match import { + Some(ImportType::ExternCrate(extern_crate)) => { + Some(ImportOrExternCrate::ExternCrate(extern_crate)) } - Some(ImportOrExternCrate::ExternCrate(import)) => { - ImportOrDef::ExternCrate(import) + Some(ImportType::Import(import)) => { + Some(ImportOrExternCrate::Import(import)) } - None => ImportOrDef::Def(fld.0), - }, - ); + None | Some(ImportType::Glob(_)) => None, + }; + let prev = std::mem::replace(&mut fld.2, import); + if let Some(import) = import { + 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, diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs index c51eea22dc..6f435d1f58 100644 --- a/crates/hir-def/src/nameres/collector.rs +++ b/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 |= 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, ); @@ -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(); diff --git a/crates/hir-def/src/nameres/tests/globs.rs b/crates/hir-def/src/nameres/tests/globs.rs index 1ca74b5da6..a2696055ca 100644 --- a/crates/hir-def/src/nameres/tests/globs.rs +++ b/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 + "#]], + ); +} |