From 70d4829560b81e3f5dc8e624da702ed6d345c49c Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Wed, 24 Jun 2020 09:30:54 -0400 Subject: [PATCH 1/6] Order of glob imports should not affect import shadowing --- crates/ra_hir_def/src/nameres/collector.rs | 41 ++++++++++------- crates/ra_hir_def/src/nameres/tests/globs.rs | 46 ++++++++++++++++++++ 2 files changed, 71 insertions(+), 16 deletions(-) diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index 94da700ad37..665dd106c79 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -380,26 +380,35 @@ fn resolve_imports(&mut self) { 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); } } diff --git a/crates/ra_hir_def/src/nameres/tests/globs.rs b/crates/ra_hir_def/src/nameres/tests/globs.rs index 2f440975a97..d5a02137c26 100644 --- a/crates/ra_hir_def/src/nameres/tests/globs.rs +++ b/crates/ra_hir_def/src/nameres/tests/globs.rs @@ -276,3 +276,49 @@ pub mod baz { "### ); } + +#[test] +fn glob_shadowed_def_reversed() { + let map = def_map( + r###" + //- /lib.rs + mod foo; + mod bar; + + use bar::baz; + use foo::*; + + use baz::Bar; + + //- /foo.rs + pub mod baz { + pub struct Foo; + } + + //- /bar.rs + pub mod baz { + pub struct Bar; + } + "###, + ); + assert_snapshot!(map, @r###" + ⋮crate + ⋮Bar: t v + ⋮bar: t + ⋮baz: t + ⋮foo: t + ⋮ + ⋮crate::bar + ⋮baz: t + ⋮ + ⋮crate::bar::baz + ⋮Bar: t v + ⋮ + ⋮crate::foo + ⋮baz: t + ⋮ + ⋮crate::foo::baz + ⋮Foo: t v + "### + ); +} From 0b657ddbfe9754afce9811c70a4e61e4ea9efeaf Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Wed, 24 Jun 2020 21:42:44 -0400 Subject: [PATCH 2/6] 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(-) diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index c81b966c398..0184b6af9e5 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -128,15 +128,19 @@ pub(crate) fn push_res(&mut self, name: Name, def: PerNs) -> bool { 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 @@ macro_rules! check_changed { }; } - 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 665dd106c79..93f58e2c761 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -305,6 +305,7 @@ fn define_macro( self.def_map.root, &[(name, PerNs::macros(macro_, Visibility::Public))], Visibility::Public, + false, ); } } @@ -330,6 +331,7 @@ fn define_proc_macro(&mut self, name: Name, macro_: MacroDefId) { self.def_map.root, &[(name, PerNs::macros(macro_, Visibility::Public))], Visibility::Public, + false, ); } @@ -380,35 +382,25 @@ fn resolve_imports(&mut self) { 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 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) { .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 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) { .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 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) { (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 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) { } } - 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 @@ fn update_recursive( // 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 @@ fn update_recursive( 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 @@ fn update_recursive( 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 @@ fn collect(&mut self, items: &[ModItem]) { self.module_id, &[(name.clone(), PerNs::from_def(id, vis, has_constructor))], vis, + false, ) } } @@ -1034,7 +1043,12 @@ fn push_child_module( 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 d5a02137c26..7f3d7509c94 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 @@ pub mod baz { "### ); } + +#[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 74665c58851..e5cbca71d61 100644 --- a/crates/ra_hir_def/src/per_ns.rs +++ b/crates/ra_hir_def/src/per_ns.rs @@ -9,6 +9,7 @@ #[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 @@ pub fn take_macros(self) -> Option { 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 @@ pub fn filter_visibility(self, mut f: impl FnMut(Visibility) -> bool) -> 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 @@ pub fn with_visibility(self, vis: Visibility) -> 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 @@ pub fn iter_items(self) -> impl Iterator { .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 } + } } From de9e964e4ac21897bd48adbe37f379d74422919f Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Thu, 25 Jun 2020 12:42:12 -0400 Subject: [PATCH 3/6] Track import type outside of , use enum rather than bool to improve readability --- crates/ra_hir_def/src/body/lower.rs | 6 ++- crates/ra_hir_def/src/item_scope.rs | 51 +++++++++++++++++----- crates/ra_hir_def/src/nameres/collector.rs | 36 +++++++++------ crates/ra_hir_def/src/per_ns.rs | 20 +++------ 4 files changed, 73 insertions(+), 40 deletions(-) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index 3ced648e56f..d749c828da0 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -26,7 +26,7 @@ dummy_expr_id, ArithOp, Array, BinaryOp, BindingAnnotation, CmpOp, Expr, ExprId, Literal, LogicOp, MatchArm, Ordering, Pat, PatId, RecordFieldPat, RecordLitField, Statement, }, - item_scope::BuiltinShadowMode, + item_scope::{BuiltinShadowMode, ImportType}, item_tree::{FileItemTreeId, ItemTree, ItemTreeNode}, path::{GenericArgs, Path}, type_ref::{Mutability, Rawness, TypeRef}, @@ -81,6 +81,7 @@ pub(super) fn lower( map }, expander, + import_types: FxHashMap::default(), } .collect(params, body) } @@ -93,6 +94,7 @@ struct ExprCollector<'a> { source_map: BodySourceMap, item_trees: FxHashMap>, + import_types: FxHashMap, } impl ExprCollector<'_> { @@ -711,8 +713,10 @@ fn collect_block_items(&mut self, block: &ast::BlockExpr) { _ => true, }; self.body.item_scope.push_res( + &mut self.import_types, name.as_name(), crate::per_ns::PerNs::from_def(def, vis, has_constructor), + ImportType::Named, ); } } diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index 0184b6af9e5..511c08a8d78 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -12,6 +12,28 @@ Lookup, MacroDefId, ModuleDefId, TraitId, }; +#[derive(Copy, Clone)] +pub(crate) enum ImportType { + Glob, + Named, +} + +impl ImportType { + fn is_glob(&self) -> bool { + match self { + ImportType::Glob => true, + ImportType::Named => false, + } + } + + fn is_named(&self) -> bool { + match self { + ImportType::Glob => false, + ImportType::Named => true, + } + } +} + #[derive(Debug, Default, PartialEq, Eq)] pub struct ItemScope { visible: FxHashMap, @@ -123,23 +145,30 @@ pub(crate) fn define_legacy_macro(&mut self, name: Name, mac: MacroDefId) { self.legacy_macros.insert(name, mac); } - pub(crate) fn push_res(&mut self, name: Name, def: PerNs) -> bool { + pub(crate) fn push_res( + &mut self, + existing_import_map: &mut FxHashMap, + name: Name, + def: PerNs, + def_import_type: ImportType, + ) -> bool { let mut changed = false; - let existing = self.visible.entry(name).or_default(); + let existing = self.visible.entry(name.clone()).or_default(); + let existing_import_type = existing_import_map.entry(name).or_insert(def_import_type); macro_rules! check_changed { - ($changed:ident, ($existing:ident/$def:ident).$field:ident) => { + ($changed:ident, ($existing:ident/$def:ident).$field:ident, $existing_import_type:ident, $def_import_type:ident) => { match ($existing.$field, $def.$field) { (None, Some(_)) => { - $existing.from_glob = $def.from_glob; + *existing_import_type = $def_import_type; $existing.$field = $def.$field; $changed = true; } - // 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 => { + (Some(_), Some(_)) + if $existing_import_type.is_glob() && $def_import_type.is_named() => + { mark::hit!(import_shadowed); - $existing.from_glob = $def.from_glob; + *$existing_import_type = $def_import_type; $existing.$field = $def.$field; $changed = true; } @@ -148,9 +177,9 @@ macro_rules! check_changed { }; } - check_changed!(changed, (existing / def).types); - check_changed!(changed, (existing / def).values); - check_changed!(changed, (existing / def).macros); + check_changed!(changed, (existing / def).types, existing_import_type, def_import_type); + check_changed!(changed, (existing / def).values, existing_import_type, def_import_type); + check_changed!(changed, (existing / def).macros, existing_import_type, def_import_type); changed } diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index 93f58e2c761..f7b99e0bef7 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -20,6 +20,7 @@ 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 @@ fn define_macro( self.def_map.root, &[(name, PerNs::macros(macro_, Visibility::Public))], Visibility::Public, - false, + ImportType::Named, ); } } @@ -331,7 +334,7 @@ fn define_proc_macro(&mut self, name: Name, macro_: MacroDefId) { self.def_map.root, &[(name, PerNs::macros(macro_, Visibility::Public))], Visibility::Public, - false, + ImportType::Named, ); } @@ -478,7 +481,7 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) { .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 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) { .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 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) { (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 @@ fn record_resolved_import(&mut self, directive: &ImportDirective) { } } - 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 @@ fn update( 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 @@ fn update_recursive( 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 @@ fn update_recursive( 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 @@ fn update_recursive( glob_importing_module, resolutions, glob_import_vis, - true, + ImportType::Glob, depth + 1, ); } @@ -940,7 +947,7 @@ fn collect(&mut self, items: &[ModItem]) { self.module_id, &[(name.clone(), PerNs::from_def(id, vis, has_constructor))], vis, - false, + ImportType::Named, ) } } @@ -1047,7 +1054,7 @@ fn push_child_module( self.module_id, &[(name, PerNs::from_def(def, vis, false))], vis, - false, + ImportType::Named, ); res } @@ -1177,6 +1184,7 @@ fn do_collect_defs(db: &dyn DefDatabase, def_map: CrateDefMap) -> CrateDefMap { mod_dirs: FxHashMap::default(), cfg_options: &CfgOptions::default(), proc_macros: Default::default(), + import_types: FxHashMap::default(), }; collector.collect(); collector.def_map diff --git a/crates/ra_hir_def/src/per_ns.rs b/crates/ra_hir_def/src/per_ns.rs index e5cbca71d61..74665c58851 100644 --- a/crates/ra_hir_def/src/per_ns.rs +++ b/crates/ra_hir_def/src/per_ns.rs @@ -9,7 +9,6 @@ #[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)>, @@ -17,29 +16,29 @@ pub struct PerNs { impl Default for PerNs { fn default() -> Self { - PerNs { from_glob: false, types: None, values: None, macros: None } + PerNs { types: None, values: None, macros: None } } } impl PerNs { pub fn none() -> PerNs { - PerNs { from_glob: false, types: None, values: None, macros: None } + PerNs { types: None, values: None, macros: None } } pub fn values(t: ModuleDefId, v: Visibility) -> PerNs { - PerNs { from_glob: false, types: None, values: Some((t, v)), macros: None } + PerNs { types: None, values: Some((t, v)), macros: None } } pub fn types(t: ModuleDefId, v: Visibility) -> PerNs { - PerNs { from_glob: false, types: Some((t, v)), values: None, macros: None } + PerNs { types: Some((t, v)), values: None, macros: None } } pub fn both(types: ModuleDefId, values: ModuleDefId, v: Visibility) -> PerNs { - PerNs { from_glob: false, types: Some((types, v)), values: Some((values, v)), macros: None } + PerNs { types: Some((types, v)), values: Some((values, v)), macros: None } } pub fn macros(macro_: MacroDefId, v: Visibility) -> PerNs { - PerNs { from_glob: false, types: None, values: None, macros: Some((macro_, v)) } + PerNs { types: None, values: None, macros: Some((macro_, v)) } } pub fn is_none(&self) -> bool { @@ -64,7 +63,6 @@ pub fn take_macros(self) -> Option { 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)), @@ -73,7 +71,6 @@ pub fn filter_visibility(self, mut f: impl FnMut(Visibility) -> bool) -> 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)), @@ -82,7 +79,6 @@ pub fn with_visibility(self, vis: Visibility) -> 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), @@ -96,8 +92,4 @@ pub fn iter_items(self) -> impl Iterator { .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 } - } } From 76755ce176222128c354647941cea65ac30ff4bd Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Thu, 25 Jun 2020 21:34:39 -0400 Subject: [PATCH 4/6] Split glob import map to per-ns, switch ExprCollector to use a simpler push_res --- crates/ra_hir_def/src/body/lower.rs | 6 +- crates/ra_hir_def/src/item_scope.rs | 77 ++++++++++++++++------ crates/ra_hir_def/src/nameres/collector.rs | 14 ++-- crates/ra_hir_ty/src/tests/simple.rs | 46 +++++++++++++ 4 files changed, 110 insertions(+), 33 deletions(-) diff --git a/crates/ra_hir_def/src/body/lower.rs b/crates/ra_hir_def/src/body/lower.rs index d749c828da0..3ced648e56f 100644 --- a/crates/ra_hir_def/src/body/lower.rs +++ b/crates/ra_hir_def/src/body/lower.rs @@ -26,7 +26,7 @@ dummy_expr_id, ArithOp, Array, BinaryOp, BindingAnnotation, CmpOp, Expr, ExprId, Literal, LogicOp, MatchArm, Ordering, Pat, PatId, RecordFieldPat, RecordLitField, Statement, }, - item_scope::{BuiltinShadowMode, ImportType}, + item_scope::BuiltinShadowMode, item_tree::{FileItemTreeId, ItemTree, ItemTreeNode}, path::{GenericArgs, Path}, type_ref::{Mutability, Rawness, TypeRef}, @@ -81,7 +81,6 @@ pub(super) fn lower( map }, expander, - import_types: FxHashMap::default(), } .collect(params, body) } @@ -94,7 +93,6 @@ struct ExprCollector<'a> { source_map: BodySourceMap, item_trees: FxHashMap>, - import_types: FxHashMap, } impl ExprCollector<'_> { @@ -713,10 +711,8 @@ fn collect_block_items(&mut self, block: &ast::BlockExpr) { _ => true, }; self.body.item_scope.push_res( - &mut self.import_types, name.as_name(), crate::per_ns::PerNs::from_def(def, vis, has_constructor), - ImportType::Named, ); } } diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index 511c08a8d78..d0923df6d8a 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -4,12 +4,12 @@ use hir_expand::name::Name; use once_cell::sync::Lazy; use ra_db::CrateId; -use rustc_hash::FxHashMap; +use rustc_hash::{FxHashMap, FxHashSet}; use test_utils::mark; use crate::{ db::DefDatabase, per_ns::PerNs, visibility::Visibility, AdtId, BuiltinType, HasModule, ImplId, - Lookup, MacroDefId, ModuleDefId, TraitId, + LocalModuleId, Lookup, MacroDefId, ModuleDefId, TraitId, }; #[derive(Copy, Clone)] @@ -19,13 +19,6 @@ pub(crate) enum ImportType { } impl ImportType { - fn is_glob(&self) -> bool { - match self { - ImportType::Glob => true, - ImportType::Named => false, - } - } - fn is_named(&self) -> bool { match self { ImportType::Glob => false, @@ -34,6 +27,13 @@ fn is_named(&self) -> bool { } } +#[derive(Debug, Default)] +pub struct PerNsGlobImports { + types: FxHashSet<(LocalModuleId, Name)>, + values: FxHashSet<(LocalModuleId, Name)>, + macros: FxHashSet<(LocalModuleId, Name)>, +} + #[derive(Debug, Default, PartialEq, Eq)] pub struct ItemScope { visible: FxHashMap, @@ -145,30 +145,65 @@ pub(crate) fn define_legacy_macro(&mut self, name: Name, mac: MacroDefId) { self.legacy_macros.insert(name, mac); } - pub(crate) fn push_res( + pub(crate) fn push_res(&mut self, name: Name, def: PerNs) -> bool { + let mut changed = false; + let existing = self.visible.entry(name).or_default(); + + if existing.types.is_none() && def.types.is_some() { + existing.types = def.types; + changed = true; + } + + if existing.values.is_none() && def.values.is_some() { + existing.values = def.values; + changed = true; + } + + if existing.macros.is_none() && def.macros.is_some() { + existing.macros = def.macros; + changed = true; + } + + changed + } + + pub(crate) fn push_res_with_import( &mut self, - existing_import_map: &mut FxHashMap, - name: Name, + glob_imports: &mut PerNsGlobImports, + lookup: (LocalModuleId, Name), def: PerNs, def_import_type: ImportType, ) -> bool { let mut changed = false; - let existing = self.visible.entry(name.clone()).or_default(); - let existing_import_type = existing_import_map.entry(name).or_insert(def_import_type); + let existing = self.visible.entry(lookup.1.clone()).or_default(); macro_rules! check_changed { - ($changed:ident, ($existing:ident/$def:ident).$field:ident, $existing_import_type:ident, $def_import_type:ident) => { + ( + $changed:ident, + ( $existing:ident / $def:ident ) . $field:ident, + $glob_imports:ident [ $lookup:ident ], + $def_import_type:ident + ) => { match ($existing.$field, $def.$field) { (None, Some(_)) => { - *existing_import_type = $def_import_type; + match $def_import_type { + ImportType::Glob => { + $glob_imports.$field.insert($lookup.clone()); + } + ImportType::Named => { + $glob_imports.$field.remove(&$lookup); + } + } + $existing.$field = $def.$field; $changed = true; } (Some(_), Some(_)) - if $existing_import_type.is_glob() && $def_import_type.is_named() => + if $glob_imports.$field.contains(&$lookup) + && $def_import_type.is_named() => { mark::hit!(import_shadowed); - *$existing_import_type = $def_import_type; + $glob_imports.$field.remove(&$lookup); $existing.$field = $def.$field; $changed = true; } @@ -177,9 +212,9 @@ macro_rules! check_changed { }; } - check_changed!(changed, (existing / def).types, existing_import_type, def_import_type); - check_changed!(changed, (existing / def).values, existing_import_type, def_import_type); - check_changed!(changed, (existing / def).macros, existing_import_type, def_import_type); + check_changed!(changed, (existing / def).types, glob_imports[lookup], def_import_type); + check_changed!(changed, (existing / def).values, glob_imports[lookup], def_import_type); + check_changed!(changed, (existing / def).macros, glob_imports[lookup], def_import_type); changed } diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index f7b99e0bef7..e74f67b3d9f 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -20,7 +20,7 @@ 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 @@ fn update_recursive( 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 @@ fn do_collect_defs(db: &dyn DefDatabase, def_map: CrateDefMap) -> CrateDefMap { 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 diff --git a/crates/ra_hir_ty/src/tests/simple.rs b/crates/ra_hir_ty/src/tests/simple.rs index d7ef9add667..7d8197f8b02 100644 --- a/crates/ra_hir_ty/src/tests/simple.rs +++ b/crates/ra_hir_ty/src/tests/simple.rs @@ -1739,6 +1739,52 @@ fn main() { assert_eq!(t, "u32"); } +// This test is actually testing the shadowing behavior within ra_hir_def. It +// lives here because the testing infrastructure in ra_hir_def isn't currently +// capable of asserting the necessary conditions. +#[test] +fn should_be_shadowing_imports() { + let t = type_at( + r#" +mod a { + pub fn foo() -> i8 {0} + pub struct foo { a: i8 } +} +mod b { pub fn foo () -> u8 {0} } +mod c { pub struct foo { a: u8 } } +mod d { + pub use super::a::*; + pub use super::c::foo; + pub use super::b::foo; +} + +fn main() { + d::foo()<|>; +}"#, + ); + assert_eq!(t, "u8"); + + let t = type_at( + r#" +mod a { + pub fn foo() -> i8 {0} + pub struct foo { a: i8 } +} +mod b { pub fn foo () -> u8 {0} } +mod c { pub struct foo { a: u8 } } +mod d { + pub use super::a::*; + pub use super::c::foo; + pub use super::b::foo; +} + +fn main() { + d::foo{a:0<|>}; +}"#, + ); + assert_eq!(t, "u8"); +} + #[test] fn closure_return() { assert_snapshot!( From b700443e781b8b54f88177247ceefcc03155b440 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Fri, 26 Jun 2020 11:13:58 -0400 Subject: [PATCH 5/6] Remove comment that's no longer valid --- crates/ra_hir_def/src/nameres/collector.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/ra_hir_def/src/nameres/collector.rs b/crates/ra_hir_def/src/nameres/collector.rs index e74f67b3d9f..5a0eba43713 100644 --- a/crates/ra_hir_def/src/nameres/collector.rs +++ b/crates/ra_hir_def/src/nameres/collector.rs @@ -583,8 +583,6 @@ fn update_recursive( // 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, ) { From 1f5d30ff1662eb94839bd1cf2e0cb57cc6fac4e4 Mon Sep 17 00:00:00 2001 From: Paul Daniel Faria Date: Fri, 26 Jun 2020 22:51:13 -0400 Subject: [PATCH 6/6] Replace simple is_named with matches macro --- crates/ra_hir_def/src/item_scope.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs index d0923df6d8a..4d446c7073b 100644 --- a/crates/ra_hir_def/src/item_scope.rs +++ b/crates/ra_hir_def/src/item_scope.rs @@ -18,15 +18,6 @@ pub(crate) enum ImportType { Named, } -impl ImportType { - fn is_named(&self) -> bool { - match self { - ImportType::Glob => false, - ImportType::Named => true, - } - } -} - #[derive(Debug, Default)] pub struct PerNsGlobImports { types: FxHashSet<(LocalModuleId, Name)>, @@ -200,7 +191,7 @@ macro_rules! check_changed { } (Some(_), Some(_)) if $glob_imports.$field.contains(&$lookup) - && $def_import_type.is_named() => + && matches!($def_import_type, ImportType::Named) => { mark::hit!(import_shadowed); $glob_imports.$field.remove(&$lookup);