From 21359c3ab5fc497d11b2c0f0435c7635336a726e Mon Sep 17 00:00:00 2001 From: Florian Diebold Date: Wed, 25 Dec 2019 18:05:16 +0100 Subject: Take visibility into account for glob imports --- crates/ra_hir_def/src/item_scope.rs | 12 +--- crates/ra_hir_def/src/nameres/collector.rs | 80 +++++++++++++++------- crates/ra_hir_def/src/nameres/tests.rs | 6 +- crates/ra_hir_def/src/nameres/tests/globs.rs | 78 +++++++++++++++++++++ crates/ra_hir_def/src/nameres/tests/incremental.rs | 4 +- crates/ra_hir_def/src/per_ns.rs | 8 +++ crates/ra_hir_def/src/visibility.rs | 20 ++++-- 7 files changed, 165 insertions(+), 43 deletions(-) diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index d77f37f67..db5f469c7 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -149,16 +149,8 @@ impl ItemScope { changed } - #[cfg(test)] - pub(crate) fn collect_resolutions(&self) -> Vec<(Name, PerNs)> { - self.visible.iter().map(|(name, res)| (name.clone(), res.clone())).collect() - } - - pub(crate) fn collect_resolutions_with_vis( - &self, - vis: ResolvedVisibility, - ) -> Vec<(Name, PerNs)> { - self.visible.iter().map(|(name, res)| (name.clone(), res.with_visibility(vis))).collect() + pub(crate) fn resolutions<'a>(&'a self) -> impl Iterator + 'a { + self.visible.iter().map(|(name, res)| (name.clone(), res.clone())) } pub(crate) fn collect_legacy_macros(&self) -> FxHashMap { diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index 5b8478037..51df44d2f 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -109,7 +109,7 @@ struct MacroDirective { struct DefCollector<'a, DB> { db: &'a DB, def_map: CrateDefMap, - glob_imports: FxHashMap>, + glob_imports: FxHashMap>, unresolved_imports: Vec, resolved_imports: Vec, unexpanded_macros: Vec, @@ -218,6 +218,7 @@ where self.update( self.def_map.root, &[(name, PerNs::macros(macro_, ResolvedVisibility::Public))], + ResolvedVisibility::Public, ); } } @@ -352,7 +353,6 @@ where fn record_resolved_import(&mut self, directive: &ImportDirective) { let module_id = directive.module_id; - let import_id = directive.import_id; let import = &directive.import; let def = directive.status.namespaces(); let vis = self @@ -373,28 +373,48 @@ where let item_map = self.db.crate_def_map(m.krate); let scope = &item_map[m.local_id].scope; - // TODO: only use names we can see - // Module scoped macros is included - let items = scope.collect_resolutions_with_vis(vis); - - self.update(module_id, &items); + let items = scope + .resolutions() + // only keep visible names... + .map(|(n, res)| { + ( + n, + res.filter_visibility(|v| { + v.visible_from_def_map(&self.def_map, module_id) + }), + ) + }) + .filter(|(_, res)| !res.is_none()) + .collect::>(); + + self.update(module_id, &items, vis); } else { // glob import from same crate => we do an initial // import, and then need to propagate any further // additions let scope = &self.def_map[m.local_id].scope; - // TODO: only use names we can see - // Module scoped macros is included - let items = scope.collect_resolutions_with_vis(vis); - - self.update(module_id, &items); + let items = scope + .resolutions() + // only keep visible names... + .map(|(n, res)| { + ( + n, + res.filter_visibility(|v| { + v.visible_from_def_map(&self.def_map, module_id) + }), + ) + }) + .filter(|(_, res)| !res.is_none()) + .collect::>(); + + self.update(module_id, &items, vis); // record the glob import in case we add further items let glob = self.glob_imports.entry(m.local_id).or_default(); - if !glob.iter().any(|it| *it == (module_id, import_id)) { - glob.push((module_id, import_id)); + if !glob.iter().any(|(mid, _)| *mid == module_id) { + glob.push((module_id, vis)); } } } @@ -412,7 +432,7 @@ where (name, res) }) .collect::>(); - self.update(module_id, &resolutions); + self.update(module_id, &resolutions, vis); } Some(d) => { log::debug!("glob import {:?} from non-module/enum {:?}", import, d); @@ -434,21 +454,29 @@ where } } - self.update(module_id, &[(name, def.with_visibility(vis))]); + self.update(module_id, &[(name, def)], vis); } None => tested_by!(bogus_paths), } } } - fn update(&mut self, module_id: LocalModuleId, resolutions: &[(Name, PerNs)]) { - self.update_recursive(module_id, resolutions, 0) + fn update( + &mut self, + module_id: LocalModuleId, + resolutions: &[(Name, PerNs)], + vis: ResolvedVisibility, + ) { + self.update_recursive(module_id, resolutions, vis, 0) } fn update_recursive( &mut self, module_id: LocalModuleId, resolutions: &[(Name, PerNs)], + // All resolutions are imported with this visibility; the visibilies in + // the `PerNs` values are ignored and overwritten + vis: ResolvedVisibility, depth: usize, ) { if depth > 100 { @@ -458,7 +486,7 @@ where let scope = &mut self.def_map.modules[module_id].scope; let mut changed = false; for (name, res) in resolutions { - changed |= scope.push_res(name.clone(), *res); + changed |= scope.push_res(name.clone(), res.with_visibility(vis)); } if !changed { @@ -471,9 +499,13 @@ where .flat_map(|v| v.iter()) .cloned() .collect::>(); - for (glob_importing_module, _glob_import) in glob_imports { - // We pass the glob import so that the tracked import in those modules is that glob import - self.update_recursive(glob_importing_module, resolutions, depth + 1); + for (glob_importing_module, glob_import_vis) in glob_imports { + // we know all resolutions have the same visibility (`vis`), so we + // just need to check that once + if !vis.visible_from_def_map(&self.def_map, glob_importing_module) { + continue; + } + self.update_recursive(glob_importing_module, resolutions, glob_import_vis, depth + 1); } } @@ -715,7 +747,7 @@ where let def: ModuleDefId = module.into(); let vis = ResolvedVisibility::Public; // TODO handle module visibility self.def_collector.def_map.modules[self.module_id].scope.define_def(def); - self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))]); + self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))], vis); res } @@ -780,7 +812,7 @@ where .def_map .resolve_visibility(self.def_collector.db, self.module_id, vis) .unwrap_or(ResolvedVisibility::Public); - self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))]) + self.def_collector.update(self.module_id, &[(name, PerNs::from_def(def, vis))], vis) } fn collect_derives(&mut self, attrs: &Attrs, def: &raw::DefData) { diff --git a/crates/ra_hir_def/src/nameres/tests.rs b/crates/ra_hir_def/src/nameres/tests.rs index ff474b53b..78bcdc850 100644 --- a/crates/ra_hir_def/src/nameres/tests.rs +++ b/crates/ra_hir_def/src/nameres/tests.rs @@ -12,8 +12,8 @@ use test_utils::covers; use crate::{db::DefDatabase, nameres::*, test_db::TestDB, LocalModuleId}; -fn def_map(fixtute: &str) -> String { - let dm = compute_crate_def_map(fixtute); +fn def_map(fixture: &str) -> String { + let dm = compute_crate_def_map(fixture); render_crate_def_map(&dm) } @@ -32,7 +32,7 @@ fn render_crate_def_map(map: &CrateDefMap) -> String { *buf += path; *buf += "\n"; - let mut entries = map.modules[module].scope.collect_resolutions(); + let mut entries: Vec<_> = map.modules[module].scope.resolutions().collect(); entries.sort_by_key(|(name, _)| name.clone()); for (name, def) in entries { diff --git a/crates/ra_hir_def/src/nameres/tests/globs.rs b/crates/ra_hir_def/src/nameres/tests/globs.rs index 5e24cb94d..5f137d3a6 100644 --- a/crates/ra_hir_def/src/nameres/tests/globs.rs +++ b/crates/ra_hir_def/src/nameres/tests/globs.rs @@ -73,6 +73,84 @@ fn glob_2() { ); } +#[test] +fn glob_privacy_1() { + let map = def_map( + " + //- /lib.rs + mod foo; + use foo::*; + + //- /foo/mod.rs + pub mod bar; + pub use self::bar::*; + struct PrivateStructFoo; + + //- /foo/bar.rs + pub struct Baz; + struct PrivateStructBar; + pub use super::*; + ", + ); + assert_snapshot!(map, @r###" + crate + Baz: t v + bar: t + foo: t + + crate::foo + Baz: t v + PrivateStructFoo: t v + bar: t + + crate::foo::bar + Baz: t v + PrivateStructBar: t v + PrivateStructFoo: t v + bar: t + "### + ); +} + +#[test] +fn glob_privacy_2() { + let map = def_map( + " + //- /lib.rs + mod foo; + use foo::*; + use foo::bar::*; + + //- /foo/mod.rs + pub mod bar; + fn Foo() {}; + pub struct Foo {}; + + //- /foo/bar.rs + pub(super) struct PrivateBaz; + struct PrivateBar; + pub(crate) struct PubCrateStruct; + ", + ); + assert_snapshot!(map, @r###" + crate + Foo: t + PubCrateStruct: t v + bar: t + foo: t + + crate::foo + Foo: t v + bar: t + + crate::foo::bar + PrivateBar: t v + PrivateBaz: t v + PubCrateStruct: t v + "### + ); +} + #[test] fn glob_across_crates() { covers!(glob_across_crates); diff --git a/crates/ra_hir_def/src/nameres/tests/incremental.rs b/crates/ra_hir_def/src/nameres/tests/incremental.rs index ef2e9435c..faeb7aa4d 100644 --- a/crates/ra_hir_def/src/nameres/tests/incremental.rs +++ b/crates/ra_hir_def/src/nameres/tests/incremental.rs @@ -116,7 +116,7 @@ fn typing_inside_a_macro_should_not_invalidate_def_map() { let events = db.log_executed(|| { let crate_def_map = db.crate_def_map(krate); let (_, module_data) = crate_def_map.modules.iter().last().unwrap(); - assert_eq!(module_data.scope.collect_resolutions().len(), 1); + assert_eq!(module_data.scope.resolutions().collect::>().len(), 1); }); assert!(format!("{:?}", events).contains("crate_def_map"), "{:#?}", events) } @@ -126,7 +126,7 @@ fn typing_inside_a_macro_should_not_invalidate_def_map() { let events = db.log_executed(|| { let crate_def_map = db.crate_def_map(krate); let (_, module_data) = crate_def_map.modules.iter().last().unwrap(); - assert_eq!(module_data.scope.collect_resolutions().len(), 1); + assert_eq!(module_data.scope.resolutions().collect::>().len(), 1); }); assert!(!format!("{:?}", events).contains("crate_def_map"), "{:#?}", events) } diff --git a/crates/ra_hir_def/src/per_ns.rs b/crates/ra_hir_def/src/per_ns.rs index 16e61a6a5..7637d8a37 100644 --- a/crates/ra_hir_def/src/per_ns.rs +++ b/crates/ra_hir_def/src/per_ns.rs @@ -61,6 +61,14 @@ impl PerNs { self.macros.map(|it| it.0) } + pub fn filter_visibility(self, mut f: impl FnMut(ResolvedVisibility) -> bool) -> PerNs { + PerNs { + types: self.types.filter(|(_, v)| f(*v)), + values: self.values.filter(|(_, v)| f(*v)), + macros: self.macros.filter(|(_, v)| f(*v)), + } + } + pub fn with_visibility(self, vis: ResolvedVisibility) -> PerNs { PerNs { types: self.types.map(|(it, _)| (it, vis)), diff --git a/crates/ra_hir_def/src/visibility.rs b/crates/ra_hir_def/src/visibility.rs index 8cac52630..990b2975c 100644 --- a/crates/ra_hir_def/src/visibility.rs +++ b/crates/ra_hir_def/src/visibility.rs @@ -134,13 +134,25 @@ impl ResolvedVisibility { if from_module.krate != to_module.krate { return false; } - // from_module needs to be a descendant of to_module let def_map = db.crate_def_map(from_module.krate); + self.visible_from_def_map(&def_map, from_module.local_id) + } + + pub(crate) fn visible_from_def_map( + self, + def_map: &crate::nameres::CrateDefMap, + from_module: crate::LocalModuleId, + ) -> bool { + let to_module = match self { + ResolvedVisibility::Module(m) => m, + ResolvedVisibility::Public => return true, + }; + // from_module needs to be a descendant of to_module let mut ancestors = std::iter::successors(Some(from_module), |m| { - let parent_id = def_map[m.local_id].parent?; - Some(ModuleId { local_id: parent_id, ..*m }) + let parent_id = def_map[*m].parent?; + Some(parent_id) }); - ancestors.any(|m| m == to_module) + ancestors.any(|m| m == to_module.local_id) } } -- cgit v1.2.3