From 0b657ddbfe9754afce9811c70a4e61e4ea9efeaf Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Wed, 24 Jun 2020 21:42:44 -0400 Subject: Revert resolution of all glob imports first, replace with tracking of glob imports and shadowing when more specific --- crates/ra_hir_def/src/item_scope.rs | 20 ++++--- crates/ra_hir_def/src/nameres/collector.rs | 84 ++++++++++++++++------------ crates/ra_hir_def/src/nameres/tests/globs.rs | 44 +++++++++++++++ crates/ra_hir_def/src/per_ns.rs | 20 +++++-- 4 files changed, 119 insertions(+), 49 deletions(-) (limited to 'crates/ra_hir_def') diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index c81b966c3..0184b6af9 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -128,15 +128,19 @@ impl ItemScope { let existing = self.visible.entry(name).or_default(); macro_rules! check_changed { - ($changed:ident, $existing:expr, $def:expr) => { - match ($existing, $def) { + ($changed:ident, ($existing:ident/$def:ident).$field:ident) => { + match ($existing.$field, $def.$field) { (None, Some(_)) => { - $existing = $def; + $existing.from_glob = $def.from_glob; + $existing.$field = $def.$field; $changed = true; } - (Some(e), Some(d)) if e.0 != d.0 => { + // Only update if the new def came from a specific import and the existing + // import came from a glob import. + (Some(_), Some(_)) if $existing.from_glob && !$def.from_glob => { mark::hit!(import_shadowed); - $existing = $def; + $existing.from_glob = $def.from_glob; + $existing.$field = $def.$field; $changed = true; } _ => {} @@ -144,9 +148,9 @@ impl ItemScope { }; } - check_changed!(changed, existing.types, def.types); - check_changed!(changed, existing.values, def.values); - check_changed!(changed, existing.macros, def.macros); + check_changed!(changed, (existing / def).types); + check_changed!(changed, (existing / def).values); + check_changed!(changed, (existing / def).macros); changed } diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index 665dd106c..93f58e2c7 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -305,6 +305,7 @@ impl DefCollector<'_> { self.def_map.root, &[(name, PerNs::macros(macro_, Visibility::Public))], Visibility::Public, + false, ); } } @@ -330,6 +331,7 @@ impl DefCollector<'_> { self.def_map.root, &[(name, PerNs::macros(macro_, Visibility::Public))], Visibility::Public, + false, ); } @@ -380,35 +382,25 @@ impl DefCollector<'_> { while self.unresolved_imports.len() < n_previous_unresolved { n_previous_unresolved = self.unresolved_imports.len(); - let mut imports = std::mem::replace(&mut self.unresolved_imports, Vec::new()); - for mut directive in &mut imports { + let imports = std::mem::replace(&mut self.unresolved_imports, Vec::new()); + for mut directive in imports { directive.status = self.resolve_import(directive.module_id, &directive.import); - } - - let (glob_imports, non_glob_imports): (Vec<_>, Vec<_>) = - imports.into_iter().partition(|directive| directive.import.is_glob); - let mut record = |imports: Vec| { - for directive in imports { - match directive.status { - PartialResolvedImport::Indeterminate(_) => { - self.record_resolved_import(&directive); - // FIXME: For avoid performance regression, - // we consider an imported resolved if it is indeterminate (i.e not all namespace resolved) - self.resolved_imports.push(directive) - } - PartialResolvedImport::Resolved(_) => { - self.record_resolved_import(&directive); - self.resolved_imports.push(directive) - } - PartialResolvedImport::Unresolved => { - self.unresolved_imports.push(directive); - } + match directive.status { + PartialResolvedImport::Indeterminate(_) => { + self.record_resolved_import(&directive); + // FIXME: For avoid performance regression, + // we consider an imported resolved if it is indeterminate (i.e not all namespace resolved) + self.resolved_imports.push(directive) + } + PartialResolvedImport::Resolved(_) => { + self.record_resolved_import(&directive); + self.resolved_imports.push(directive) + } + PartialResolvedImport::Unresolved => { + self.unresolved_imports.push(directive); } } - }; - - record(glob_imports); - record(non_glob_imports); + } } } @@ -486,7 +478,7 @@ impl DefCollector<'_> { .filter(|(_, res)| !res.is_none()) .collect::>(); - self.update(module_id, &items, vis); + self.update(module_id, &items, vis, true); } else { // glob import from same crate => we do an initial // import, and then need to propagate any further @@ -508,7 +500,7 @@ impl DefCollector<'_> { .filter(|(_, res)| !res.is_none()) .collect::>(); - self.update(module_id, &items, vis); + self.update(module_id, &items, vis, true); // 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(|(mid, _)| *mid == module_id) { @@ -538,7 +530,7 @@ impl DefCollector<'_> { (name, res) }) .collect::>(); - self.update(module_id, &resolutions, vis); + self.update(module_id, &resolutions, vis, true); } Some(d) => { log::debug!("glob import {:?} from non-module/enum {:?}", import, d); @@ -564,15 +556,21 @@ impl DefCollector<'_> { } } - self.update(module_id, &[(name, def)], vis); + self.update(module_id, &[(name, def)], vis, false); } None => mark::hit!(bogus_paths), } } } - fn update(&mut self, module_id: LocalModuleId, resolutions: &[(Name, PerNs)], vis: Visibility) { - self.update_recursive(module_id, resolutions, vis, 0) + fn update( + &mut self, + module_id: LocalModuleId, + resolutions: &[(Name, PerNs)], + vis: Visibility, + is_from_glob: bool, + ) { + self.update_recursive(module_id, resolutions, vis, is_from_glob, 0) } fn update_recursive( @@ -582,6 +580,9 @@ impl DefCollector<'_> { // All resolutions are imported with this visibility; the visibilies in // the `PerNs` values are ignored and overwritten vis: Visibility, + // All resolutions are imported with this glob status; the glob status + // in the `PerNs` values are ignored and overwritten + is_from_glob: bool, depth: usize, ) { if depth > 100 { @@ -591,7 +592,8 @@ impl DefCollector<'_> { 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.with_visibility(vis)); + changed |= + scope.push_res(name.clone(), res.with_visibility(vis).from_glob(is_from_glob)); } if !changed { @@ -610,7 +612,13 @@ impl DefCollector<'_> { if !vis.is_visible_from_def_map(&self.def_map, glob_importing_module) { continue; } - self.update_recursive(glob_importing_module, resolutions, glob_import_vis, depth + 1); + self.update_recursive( + glob_importing_module, + resolutions, + glob_import_vis, + true, + depth + 1, + ); } } @@ -932,6 +940,7 @@ impl ModCollector<'_, '_> { self.module_id, &[(name.clone(), PerNs::from_def(id, vis, has_constructor))], vis, + false, ) } } @@ -1034,7 +1043,12 @@ impl ModCollector<'_, '_> { let module = ModuleId { krate: self.def_collector.def_map.krate, local_id: res }; let def: ModuleDefId = module.into(); 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, false))], vis); + self.def_collector.update( + self.module_id, + &[(name, PerNs::from_def(def, vis, false))], + vis, + false, + ); res } diff --git a/crates/ra_hir_def/src/nameres/tests/globs.rs b/crates/ra_hir_def/src/nameres/tests/globs.rs index d5a02137c..7f3d7509c 100644 --- a/crates/ra_hir_def/src/nameres/tests/globs.rs +++ b/crates/ra_hir_def/src/nameres/tests/globs.rs @@ -322,3 +322,47 @@ fn glob_shadowed_def_reversed() { "### ); } + +#[test] +fn glob_shadowed_def_dependencies() { + let map = def_map( + r###" + //- /lib.rs + mod a { pub mod foo { pub struct X; } } + mod b { pub use super::a::foo; } + mod c { pub mod foo { pub struct Y; } } + mod d { + use super::c::foo; + use super::b::*; + use foo::Y; + } + "###, + ); + assert_snapshot!(map, @r###" + ⋮crate + ⋮a: t + ⋮b: t + ⋮c: t + ⋮d: t + ⋮ + ⋮crate::d + ⋮Y: t v + ⋮foo: t + ⋮ + ⋮crate::c + ⋮foo: t + ⋮ + ⋮crate::c::foo + ⋮Y: t v + ⋮ + ⋮crate::b + ⋮foo: t + ⋮ + ⋮crate::a + ⋮foo: t + ⋮ + ⋮crate::a::foo + ⋮X: t v + "### + ); +} diff --git a/crates/ra_hir_def/src/per_ns.rs b/crates/ra_hir_def/src/per_ns.rs index 74665c588..e5cbca71d 100644 --- a/crates/ra_hir_def/src/per_ns.rs +++ b/crates/ra_hir_def/src/per_ns.rs @@ -9,6 +9,7 @@ use crate::{item_scope::ItemInNs, visibility::Visibility, ModuleDefId}; #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct PerNs { + pub from_glob: bool, pub types: Option<(ModuleDefId, Visibility)>, pub values: Option<(ModuleDefId, Visibility)>, pub macros: Option<(MacroDefId, Visibility)>, @@ -16,29 +17,29 @@ pub struct PerNs { impl Default for PerNs { fn default() -> Self { - PerNs { types: None, values: None, macros: None } + PerNs { from_glob: false, types: None, values: None, macros: None } } } impl PerNs { pub fn none() -> PerNs { - PerNs { types: None, values: None, macros: None } + PerNs { from_glob: false, types: None, values: None, macros: None } } pub fn values(t: ModuleDefId, v: Visibility) -> PerNs { - PerNs { types: None, values: Some((t, v)), macros: None } + PerNs { from_glob: false, types: None, values: Some((t, v)), macros: None } } pub fn types(t: ModuleDefId, v: Visibility) -> PerNs { - PerNs { types: Some((t, v)), values: None, macros: None } + PerNs { from_glob: false, types: Some((t, v)), values: None, macros: None } } pub fn both(types: ModuleDefId, values: ModuleDefId, v: Visibility) -> PerNs { - PerNs { types: Some((types, v)), values: Some((values, v)), macros: None } + PerNs { from_glob: false, types: Some((types, v)), values: Some((values, v)), macros: None } } pub fn macros(macro_: MacroDefId, v: Visibility) -> PerNs { - PerNs { types: None, values: None, macros: Some((macro_, v)) } + PerNs { from_glob: false, types: None, values: None, macros: Some((macro_, v)) } } pub fn is_none(&self) -> bool { @@ -63,6 +64,7 @@ impl PerNs { pub fn filter_visibility(self, mut f: impl FnMut(Visibility) -> bool) -> PerNs { PerNs { + from_glob: self.from_glob, types: self.types.filter(|(_, v)| f(*v)), values: self.values.filter(|(_, v)| f(*v)), macros: self.macros.filter(|(_, v)| f(*v)), @@ -71,6 +73,7 @@ impl PerNs { pub fn with_visibility(self, vis: Visibility) -> PerNs { PerNs { + from_glob: self.from_glob, types: self.types.map(|(it, _)| (it, vis)), values: self.values.map(|(it, _)| (it, vis)), macros: self.macros.map(|(it, _)| (it, vis)), @@ -79,6 +82,7 @@ impl PerNs { pub fn or(self, other: PerNs) -> PerNs { PerNs { + from_glob: self.from_glob, types: self.types.or(other.types), values: self.values.or(other.values), macros: self.macros.or(other.macros), @@ -92,4 +96,8 @@ impl PerNs { .chain(self.values.map(|it| ItemInNs::Values(it.0)).into_iter()) .chain(self.macros.map(|it| ItemInNs::Macros(it.0)).into_iter()) } + + pub fn from_glob(self, from_glob: bool) -> PerNs { + PerNs { from_glob, ..self } + } } -- cgit v1.2.3