From 70d4829560b81e3f5dc8e624da702ed6d345c49c Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Wed, 24 Jun 2020 09:30:54 -0400 Subject: Order of glob imports should not affect import shadowing --- crates/ra_hir_def/src/nameres/collector.rs | 41 ++++++++++++++++++------------ 1 file changed, 25 insertions(+), 16 deletions(-) (limited to 'crates/ra_hir_def/src/nameres/collector.rs') diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index 94da700ad..665dd106c 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -380,26 +380,35 @@ impl DefCollector<'_> { while self.unresolved_imports.len() < n_previous_unresolved { n_previous_unresolved = self.unresolved_imports.len(); - let imports = std::mem::replace(&mut self.unresolved_imports, Vec::new()); - for mut directive in imports { + let mut imports = std::mem::replace(&mut self.unresolved_imports, Vec::new()); + for mut directive in &mut imports { directive.status = self.resolve_import(directive.module_id, &directive.import); + } - 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); + 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); + } } } - } + }; + + record(glob_imports); + record(non_glob_imports); } } -- cgit v1.2.3 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/nameres/collector.rs | 84 +++++++++++++++++------------- 1 file changed, 49 insertions(+), 35 deletions(-) (limited to 'crates/ra_hir_def/src/nameres/collector.rs') 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 } -- cgit v1.2.3 From de9e964e4ac21897bd48adbe37f379d74422919f Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Thu, 25 Jun 2020 12:42:12 -0400 Subject: Track import type outside of , use enum rather than bool to improve readability --- crates/ra_hir_def/src/nameres/collector.rs | 36 ++++++++++++++++++------------ 1 file changed, 22 insertions(+), 14 deletions(-) (limited to 'crates/ra_hir_def/src/nameres/collector.rs') diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index 93f58e2c7..f7b99e0be 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -20,6 +20,7 @@ use test_utils::mark; use crate::{ attr::Attrs, db::DefDatabase, + item_scope::ImportType, item_tree::{ self, FileItemTreeId, ItemTree, ItemTreeId, MacroCall, Mod, ModItem, ModKind, StructDefKind, }, @@ -80,6 +81,7 @@ pub(super) fn collect_defs(db: &dyn DefDatabase, mut def_map: CrateDefMap) -> Cr mod_dirs: FxHashMap::default(), cfg_options, proc_macros, + import_types: FxHashMap::default(), }; collector.collect(); collector.finish() @@ -186,6 +188,7 @@ struct DefCollector<'a> { mod_dirs: FxHashMap, cfg_options: &'a CfgOptions, proc_macros: Vec<(Name, ProcMacroExpander)>, + import_types: FxHashMap, } impl DefCollector<'_> { @@ -305,7 +308,7 @@ impl DefCollector<'_> { self.def_map.root, &[(name, PerNs::macros(macro_, Visibility::Public))], Visibility::Public, - false, + ImportType::Named, ); } } @@ -331,7 +334,7 @@ impl DefCollector<'_> { self.def_map.root, &[(name, PerNs::macros(macro_, Visibility::Public))], Visibility::Public, - false, + ImportType::Named, ); } @@ -478,7 +481,7 @@ impl DefCollector<'_> { .filter(|(_, res)| !res.is_none()) .collect::>(); - self.update(module_id, &items, vis, true); + self.update(module_id, &items, vis, ImportType::Glob); } else { // glob import from same crate => we do an initial // import, and then need to propagate any further @@ -500,7 +503,7 @@ impl DefCollector<'_> { .filter(|(_, res)| !res.is_none()) .collect::>(); - self.update(module_id, &items, vis, true); + self.update(module_id, &items, vis, ImportType::Glob); // 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) { @@ -530,7 +533,7 @@ impl DefCollector<'_> { (name, res) }) .collect::>(); - self.update(module_id, &resolutions, vis, true); + self.update(module_id, &resolutions, vis, ImportType::Glob); } Some(d) => { log::debug!("glob import {:?} from non-module/enum {:?}", import, d); @@ -556,7 +559,7 @@ impl DefCollector<'_> { } } - self.update(module_id, &[(name, def)], vis, false); + self.update(module_id, &[(name, def)], vis, ImportType::Named); } None => mark::hit!(bogus_paths), } @@ -568,9 +571,9 @@ impl DefCollector<'_> { module_id: LocalModuleId, resolutions: &[(Name, PerNs)], vis: Visibility, - is_from_glob: bool, + import_type: ImportType, ) { - self.update_recursive(module_id, resolutions, vis, is_from_glob, 0) + self.update_recursive(module_id, resolutions, vis, import_type, 0) } fn update_recursive( @@ -582,7 +585,7 @@ impl DefCollector<'_> { 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, + import_type: ImportType, depth: usize, ) { if depth > 100 { @@ -592,8 +595,12 @@ 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).from_glob(is_from_glob)); + changed |= scope.push_res( + &mut self.import_types, + name.clone(), + res.with_visibility(vis), + import_type, + ); } if !changed { @@ -616,7 +623,7 @@ impl DefCollector<'_> { glob_importing_module, resolutions, glob_import_vis, - true, + ImportType::Glob, depth + 1, ); } @@ -940,7 +947,7 @@ impl ModCollector<'_, '_> { self.module_id, &[(name.clone(), PerNs::from_def(id, vis, has_constructor))], vis, - false, + ImportType::Named, ) } } @@ -1047,7 +1054,7 @@ impl ModCollector<'_, '_> { self.module_id, &[(name, PerNs::from_def(def, vis, false))], vis, - false, + ImportType::Named, ); res } @@ -1177,6 +1184,7 @@ mod tests { mod_dirs: FxHashMap::default(), cfg_options: &CfgOptions::default(), proc_macros: Default::default(), + import_types: FxHashMap::default(), }; collector.collect(); collector.def_map -- cgit v1.2.3 From 76755ce176222128c354647941cea65ac30ff4bd Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Thu, 25 Jun 2020 21:34:39 -0400 Subject: Split glob import map to per-ns, switch ExprCollector to use a simpler push_res --- crates/ra_hir_def/src/nameres/collector.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'crates/ra_hir_def/src/nameres/collector.rs') diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index f7b99e0be..e74f67b3d 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -20,7 +20,7 @@ use test_utils::mark; use crate::{ attr::Attrs, db::DefDatabase, - item_scope::ImportType, + item_scope::{ImportType, PerNsGlobImports}, item_tree::{ self, FileItemTreeId, ItemTree, ItemTreeId, MacroCall, Mod, ModItem, ModKind, StructDefKind, }, @@ -81,7 +81,7 @@ pub(super) fn collect_defs(db: &dyn DefDatabase, mut def_map: CrateDefMap) -> Cr mod_dirs: FxHashMap::default(), cfg_options, proc_macros, - import_types: FxHashMap::default(), + from_glob_import: Default::default(), }; collector.collect(); collector.finish() @@ -188,7 +188,7 @@ struct DefCollector<'a> { mod_dirs: FxHashMap, cfg_options: &'a CfgOptions, proc_macros: Vec<(Name, ProcMacroExpander)>, - import_types: FxHashMap, + from_glob_import: PerNsGlobImports, } impl DefCollector<'_> { @@ -595,9 +595,9 @@ 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( - &mut self.import_types, - name.clone(), + changed |= scope.push_res_with_import( + &mut self.from_glob_import, + (module_id, name.clone()), res.with_visibility(vis), import_type, ); @@ -1184,7 +1184,7 @@ mod tests { mod_dirs: FxHashMap::default(), cfg_options: &CfgOptions::default(), proc_macros: Default::default(), - import_types: FxHashMap::default(), + from_glob_import: Default::default(), }; collector.collect(); collector.def_map -- cgit v1.2.3 From b700443e781b8b54f88177247ceefcc03155b440 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Fri, 26 Jun 2020 11:13:58 -0400 Subject: Remove comment that's no longer valid --- crates/ra_hir_def/src/nameres/collector.rs | 2 -- 1 file changed, 2 deletions(-) (limited to 'crates/ra_hir_def/src/nameres/collector.rs') diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index e74f67b3d..5a0eba437 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -583,8 +583,6 @@ 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 import_type: ImportType, depth: usize, ) { -- cgit v1.2.3