Merge #2624

2624: Separate module item from module scope r=matklad a=matklad

bors r+

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

crates/ra_hir/src/code_model.rs

@@ -221,7 +221,7 @@ impl Module {
 
     pub fn impl_blocks(self, db: &impl DefDatabase) -> Vec<ImplBlock> {
         let def_map = db.crate_def_map(self.id.krate);
-        def_map[self.id.local_id].impls.iter().copied().map(ImplBlock::from).collect()
+        def_map[self.id.local_id].scope.impls().map(ImplBlock::from).collect()
     }
 
     pub(crate) fn with_module_id(self, module_id: LocalModuleId) -> Module {

crates/ra_hir_def/src/body.rs

@@ -17,7 +17,8 @@ use rustc_hash::FxHashMap;
 use crate::{
     db::DefDatabase,
     expr::{Expr, ExprId, Pat, PatId},
-    nameres::{BuiltinShadowMode, CrateDefMap},
+    item_scope::BuiltinShadowMode,
+    nameres::CrateDefMap,
     path::{ModPath, Path},
     src::HasSource,
     DefWithBodyId, HasModule, Lookup, ModuleDefId, ModuleId,

crates/ra_hir_def/src/child_by_source.rs

@@ -80,9 +80,9 @@ impl ChildBySource for ModuleId {
 
         module_data.scope.declarations().for_each(|item| add_module_def(db, &mut res, item));
 
-        for &impl_ in module_data.impls.iter() {
+        for imp in module_data.scope.impls() {
-            let src = impl_.lookup(db).source(db);
+            let src = imp.lookup(db).source(db);
-            res[keys::IMPL].insert(src, impl_)
+            res[keys::IMPL].insert(src, imp)
         }
 
         res

crates/ra_hir_def/src/item_scope.rs

@@ -0,0 +1,165 @@
+//! Describes items defined or visible (ie, imported) in a certain scope.
+//! This is shared between modules and blocks.
+
+use hir_expand::name::Name;
+use once_cell::sync::Lazy;
+use rustc_hash::FxHashMap;
+
+use crate::{per_ns::PerNs, BuiltinType, ImplId, LocalImportId, MacroDefId, ModuleDefId, TraitId};
+
+#[derive(Debug, Default, PartialEq, Eq)]
+pub struct ItemScope {
+    items: FxHashMap<Name, Resolution>,
+    impls: Vec<ImplId>,
+    /// Macros visible in current module in legacy textual scope
+    ///
+    /// For macros invoked by an unqualified identifier like `bar!()`, `legacy_macros` will be searched in first.
+    /// If it yields no result, then it turns to module scoped `macros`.
+    /// It macros with name qualified with a path like `crate::foo::bar!()`, `legacy_macros` will be skipped,
+    /// and only normal scoped `macros` will be searched in.
+    ///
+    /// Note that this automatically inherit macros defined textually before the definition of module itself.
+    ///
+    /// Module scoped macros will be inserted into `items` instead of here.
+    // FIXME: Macro shadowing in one module is not properly handled. Non-item place macros will
+    // be all resolved to the last one defined if shadowing happens.
+    legacy_macros: FxHashMap<Name, MacroDefId>,
+}
+
+static BUILTIN_SCOPE: Lazy<FxHashMap<Name, Resolution>> = Lazy::new(|| {
+    BuiltinType::ALL
+        .iter()
+        .map(|(name, ty)| {
+            (name.clone(), Resolution { def: PerNs::types(ty.clone().into()), import: None })
+        })
+        .collect()
+});
+
+/// Shadow mode for builtin type which can be shadowed by module.
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+pub(crate) enum BuiltinShadowMode {
+    // Prefer Module
+    Module,
+    // Prefer Other Types
+    Other,
+}
+
+/// Legacy macros can only be accessed through special methods like `get_legacy_macros`.
+/// Other methods will only resolve values, types and module scoped macros only.
+impl ItemScope {
+    pub fn entries<'a>(&'a self) -> impl Iterator<Item = (&'a Name, &'a Resolution)> + 'a {
+        //FIXME: shadowing
+        self.items.iter().chain(BUILTIN_SCOPE.iter())
+    }
+
+    pub fn declarations(&self) -> impl Iterator<Item = ModuleDefId> + '_ {
+        self.entries()
+            .filter_map(|(_name, res)| if res.import.is_none() { Some(res.def) } else { None })
+            .flat_map(|per_ns| {
+                per_ns.take_types().into_iter().chain(per_ns.take_values().into_iter())
+            })
+    }
+
+    pub fn impls(&self) -> impl Iterator<Item = ImplId> + ExactSizeIterator + '_ {
+        self.impls.iter().copied()
+    }
+
+    /// Iterate over all module scoped macros
+    pub(crate) fn macros<'a>(&'a self) -> impl Iterator<Item = (&'a Name, MacroDefId)> + 'a {
+        self.items
+            .iter()
+            .filter_map(|(name, res)| res.def.take_macros().map(|macro_| (name, macro_)))
+    }
+
+    /// Iterate over all legacy textual scoped macros visible at the end of the module
+    pub(crate) fn legacy_macros<'a>(&'a self) -> impl Iterator<Item = (&'a Name, MacroDefId)> + 'a {
+        self.legacy_macros.iter().map(|(name, def)| (name, *def))
+    }
+
+    /// Get a name from current module scope, legacy macros are not included
+    pub(crate) fn get(&self, name: &Name, shadow: BuiltinShadowMode) -> Option<&Resolution> {
+        match shadow {
+            BuiltinShadowMode::Module => self.items.get(name).or_else(|| BUILTIN_SCOPE.get(name)),
+            BuiltinShadowMode::Other => {
+                let item = self.items.get(name);
+                if let Some(res) = item {
+                    if let Some(ModuleDefId::ModuleId(_)) = res.def.take_types() {
+                        return BUILTIN_SCOPE.get(name).or(item);
+                    }
+                }
+
+                item.or_else(|| BUILTIN_SCOPE.get(name))
+            }
+        }
+    }
+
+    pub(crate) fn traits<'a>(&'a self) -> impl Iterator<Item = TraitId> + 'a {
+        self.items.values().filter_map(|r| match r.def.take_types() {
+            Some(ModuleDefId::TraitId(t)) => Some(t),
+            _ => None,
+        })
+    }
+
+    pub(crate) fn get_legacy_macro(&self, name: &Name) -> Option<MacroDefId> {
+        self.legacy_macros.get(name).copied()
+    }
+
+    pub(crate) fn define_impl(&mut self, imp: ImplId) {
+        self.impls.push(imp)
+    }
+
+    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,
+        res: &Resolution,
+        import: Option<LocalImportId>,
+    ) -> bool {
+        let mut changed = false;
+        let existing = self.items.entry(name.clone()).or_default();
+
+        if existing.def.types.is_none() && res.def.types.is_some() {
+            existing.def.types = res.def.types;
+            existing.import = import.or(res.import);
+            changed = true;
+        }
+        if existing.def.values.is_none() && res.def.values.is_some() {
+            existing.def.values = res.def.values;
+            existing.import = import.or(res.import);
+            changed = true;
+        }
+        if existing.def.macros.is_none() && res.def.macros.is_some() {
+            existing.def.macros = res.def.macros;
+            existing.import = import.or(res.import);
+            changed = true;
+        }
+
+        if existing.def.is_none()
+            && res.def.is_none()
+            && existing.import.is_none()
+            && res.import.is_some()
+        {
+            existing.import = res.import;
+        }
+        changed
+    }
+
+    pub(crate) fn collect_resolutions(&self) -> Vec<(Name, Resolution)> {
+        self.items.iter().map(|(name, res)| (name.clone(), res.clone())).collect()
+    }
+
+    pub(crate) fn collect_legacy_macros(&self) -> FxHashMap<Name, MacroDefId> {
+        self.legacy_macros.clone()
+    }
+}
+
+#[derive(Debug, Clone, PartialEq, Eq, Default)]
+pub struct Resolution {
+    /// None for unresolved
+    pub def: PerNs,
+    /// ident by which this is imported into local scope.
+    pub import: Option<LocalImportId>,
+}

crates/ra_hir_def/src/lang_item.rs

@@ -81,7 +81,7 @@ impl LangItems {
         // Look for impl targets
         let def_map = db.crate_def_map(module.krate);
         let module_data = &def_map[module.local_id];
-        for &impl_block in module_data.impls.iter() {
+        for impl_block in module_data.scope.impls() {
             self.collect_lang_item(db, impl_block, LangItemTarget::ImplBlockId)
         }
 

crates/ra_hir_def/src/lib.rs

@@ -15,6 +15,7 @@ pub mod type_ref;
 pub mod builtin_type;
 pub mod diagnostics;
 pub mod per_ns;
+pub mod item_scope;
 
 pub mod dyn_map;
 pub mod keys;

crates/ra_hir_def/src/nameres.rs

@@ -57,8 +57,7 @@ mod tests;
 
 use std::sync::Arc;
 
-use hir_expand::{diagnostics::DiagnosticSink, name::Name, InFile, MacroDefId};
+use hir_expand::{diagnostics::DiagnosticSink, name::Name, InFile};
-use once_cell::sync::Lazy;
 use ra_arena::Arena;
 use ra_db::{CrateId, Edition, FileId, FilePosition};
 use ra_prof::profile;
@@ -69,12 +68,12 @@ use ra_syntax::{
 use rustc_hash::FxHashMap;
 
 use crate::{
-    builtin_type::BuiltinType,
     db::DefDatabase,
+    item_scope::{BuiltinShadowMode, ItemScope},
     nameres::{diagnostics::DefDiagnostic, path_resolution::ResolveMode},
     path::ModPath,
     per_ns::PerNs,
-    AstId, ImplId, LocalImportId, LocalModuleId, ModuleDefId, ModuleId, TraitId,
+    AstId, LocalModuleId, ModuleDefId, ModuleId,
 };
 
 /// Contains all top-level defs from a macro-expanded crate
@@ -166,113 +165,10 @@ impl ModuleOrigin {
 pub struct ModuleData {
     pub parent: Option<LocalModuleId>,
     pub children: FxHashMap<Name, LocalModuleId>,
-    pub scope: ModuleScope,
+    pub scope: ItemScope,
 
     /// Where does this module come from?
     pub origin: ModuleOrigin,
-
-    pub impls: Vec<ImplId>,
-}
-
-#[derive(Debug, Default, PartialEq, Eq)]
-pub struct ModuleScope {
-    items: FxHashMap<Name, Resolution>,
-    /// Macros visable in current module in legacy textual scope
-    ///
-    /// For macros invoked by an unquatified identifier like `bar!()`, `legacy_macros` will be searched in first.
-    /// If it yields no result, then it turns to module scoped `macros`.
-    /// It macros with name quatified with a path like `crate::foo::bar!()`, `legacy_macros` will be skipped,
-    /// and only normal scoped `macros` will be searched in.
-    ///
-    /// Note that this automatically inherit macros defined textually before the definition of module itself.
-    ///
-    /// Module scoped macros will be inserted into `items` instead of here.
-    // FIXME: Macro shadowing in one module is not properly handled. Non-item place macros will
-    // be all resolved to the last one defined if shadowing happens.
-    legacy_macros: FxHashMap<Name, MacroDefId>,
-}
-
-static BUILTIN_SCOPE: Lazy<FxHashMap<Name, Resolution>> = Lazy::new(|| {
-    BuiltinType::ALL
-        .iter()
-        .map(|(name, ty)| {
-            (name.clone(), Resolution { def: PerNs::types(ty.clone().into()), import: None })
-        })
-        .collect()
-});
-
-/// Shadow mode for builtin type which can be shadowed by module.
-#[derive(Debug, Copy, Clone, PartialEq, Eq)]
-pub enum BuiltinShadowMode {
-    // Prefer Module
-    Module,
-    // Prefer Other Types
-    Other,
-}
-
-/// Legacy macros can only be accessed through special methods like `get_legacy_macros`.
-/// Other methods will only resolve values, types and module scoped macros only.
-impl ModuleScope {
-    pub fn entries<'a>(&'a self) -> impl Iterator<Item = (&'a Name, &'a Resolution)> + 'a {
-        //FIXME: shadowing
-        self.items.iter().chain(BUILTIN_SCOPE.iter())
-    }
-
-    pub fn declarations(&self) -> impl Iterator<Item = ModuleDefId> + '_ {
-        self.entries()
-            .filter_map(|(_name, res)| if res.import.is_none() { Some(res.def) } else { None })
-            .flat_map(|per_ns| {
-                per_ns.take_types().into_iter().chain(per_ns.take_values().into_iter())
-            })
-    }
-
-    /// Iterate over all module scoped macros
-    pub fn macros<'a>(&'a self) -> impl Iterator<Item = (&'a Name, MacroDefId)> + 'a {
-        self.items
-            .iter()
-            .filter_map(|(name, res)| res.def.take_macros().map(|macro_| (name, macro_)))
-    }
-
-    /// Iterate over all legacy textual scoped macros visable at the end of the module
-    pub fn legacy_macros<'a>(&'a self) -> impl Iterator<Item = (&'a Name, MacroDefId)> + 'a {
-        self.legacy_macros.iter().map(|(name, def)| (name, *def))
-    }
-
-    /// Get a name from current module scope, legacy macros are not included
-    pub fn get(&self, name: &Name, shadow: BuiltinShadowMode) -> Option<&Resolution> {
-        match shadow {
-            BuiltinShadowMode::Module => self.items.get(name).or_else(|| BUILTIN_SCOPE.get(name)),
-            BuiltinShadowMode::Other => {
-                let item = self.items.get(name);
-                if let Some(res) = item {
-                    if let Some(ModuleDefId::ModuleId(_)) = res.def.take_types() {
-                        return BUILTIN_SCOPE.get(name).or(item);
-                    }
-                }
-
-                item.or_else(|| BUILTIN_SCOPE.get(name))
-            }
-        }
-    }
-
-    pub fn traits<'a>(&'a self) -> impl Iterator<Item = TraitId> + 'a {
-        self.items.values().filter_map(|r| match r.def.take_types() {
-            Some(ModuleDefId::TraitId(t)) => Some(t),
-            _ => None,
-        })
-    }
-
-    fn get_legacy_macro(&self, name: &Name) -> Option<MacroDefId> {
-        self.legacy_macros.get(name).copied()
-    }
-}
-
-#[derive(Debug, Clone, PartialEq, Eq, Default)]
-pub struct Resolution {
-    /// None for unresolved
-    pub def: PerNs,
-    /// ident by which this is imported into local scope.
-    pub import: Option<LocalImportId>,
 }
 
 impl CrateDefMap {

crates/ra_hir_def/src/nameres/collector.rs

@@ -18,9 +18,10 @@ use test_utils::tested_by;
 use crate::{
     attr::Attrs,
     db::DefDatabase,
+    item_scope::Resolution,
     nameres::{
         diagnostics::DefDiagnostic, mod_resolution::ModDir, path_resolution::ReachedFixedPoint,
-        raw, BuiltinShadowMode, CrateDefMap, ModuleData, ModuleOrigin, Resolution, ResolveMode,
+        raw, BuiltinShadowMode, CrateDefMap, ModuleData, ModuleOrigin, ResolveMode,
     },
     path::{ModPath, PathKind},
     per_ns::PerNs,
@@ -225,14 +226,14 @@ where
 
     /// Define a legacy textual scoped macro in module
     ///
-    /// We use a map `legacy_macros` to store all legacy textual scoped macros visable per module.
+    /// We use a map `legacy_macros` to store all legacy textual scoped macros visible per module.
     /// It will clone all macros from parent legacy scope, whose definition is prior to
     /// the definition of current module.
-    /// And also, `macro_use` on a module will import all legacy macros visable inside to
+    /// And also, `macro_use` on a module will import all legacy macros visible inside to
     /// current legacy scope, with possible shadowing.
-    fn define_legacy_macro(&mut self, module_id: LocalModuleId, name: Name, macro_: MacroDefId) {
+    fn define_legacy_macro(&mut self, module_id: LocalModuleId, name: Name, mac: MacroDefId) {
         // Always shadowing
-        self.def_map.modules[module_id].scope.legacy_macros.insert(name, macro_);
+        self.def_map.modules[module_id].scope.define_legacy_macro(name, mac);
     }
 
     /// Import macros from `#[macro_use] extern crate`.
@@ -371,11 +372,7 @@ where
                         let scope = &item_map[m.local_id].scope;
 
                         // Module scoped macros is included
-                        let items = scope
+                        let items = scope.collect_resolutions();
-                            .items
-                            .iter()
-                            .map(|(name, res)| (name.clone(), res.clone()))
-                            .collect::<Vec<_>>();
 
                         self.update(module_id, Some(import_id), &items);
                     } else {
@@ -385,11 +382,7 @@ where
                         let scope = &self.def_map[m.local_id].scope;
 
                         // Module scoped macros is included
-                        let items = scope
+                        let items = scope.collect_resolutions();
-                            .items
-                            .iter()
-                            .map(|(name, res)| (name.clone(), res.clone()))
-                            .collect::<Vec<_>>();
 
                         self.update(module_id, Some(import_id), &items);
                         // record the glob import in case we add further items
@@ -466,34 +459,10 @@ where
             // prevent stack overflows (but this shouldn't be possible)
             panic!("infinite recursion in glob imports!");
         }
-        let module_items = &mut self.def_map.modules[module_id].scope;
+        let scope = &mut self.def_map.modules[module_id].scope;
         let mut changed = false;
         for (name, res) in resolutions {
-            let existing = module_items.items.entry(name.clone()).or_default();
+            changed |= scope.push_res(name.clone(), res, import);
-
-            if existing.def.types.is_none() && res.def.types.is_some() {
-                existing.def.types = res.def.types;
-                existing.import = import.or(res.import);
-                changed = true;
-            }
-            if existing.def.values.is_none() && res.def.values.is_some() {
-                existing.def.values = res.def.values;
-                existing.import = import.or(res.import);
-                changed = true;
-            }
-            if existing.def.macros.is_none() && res.def.macros.is_some() {
-                existing.def.macros = res.def.macros;
-                existing.import = import.or(res.import);
-                changed = true;
-            }
-
-            if existing.def.is_none()
-                && res.def.is_none()
-                && existing.import.is_none()
-                && res.import.is_some()
-            {
-                existing.import = res.import;
-            }
         }
 
         if !changed {
@@ -666,7 +635,9 @@ where
                         let impl_id =
                             ImplLoc { container, ast_id: AstId::new(self.file_id, ast_id) }
                                 .intern(self.def_collector.db);
-                        self.def_collector.def_map.modules[self.module_id].impls.push(impl_id)
+                        self.def_collector.def_map.modules[self.module_id]
+                            .scope
+                            .define_impl(impl_id)
                     }
                 }
             }
@@ -740,7 +711,9 @@ where
         let res = modules.alloc(ModuleData::default());
         modules[res].parent = Some(self.module_id);
         modules[res].origin = ModuleOrigin::not_sure_file(definition, declaration);
-        modules[res].scope.legacy_macros = modules[self.module_id].scope.legacy_macros.clone();
+        for (name, mac) in modules[self.module_id].scope.collect_legacy_macros() {
+            modules[res].scope.define_legacy_macro(name, mac)
+        }
         modules[self.module_id].children.insert(name.clone(), res);
         let resolution = Resolution {
             def: PerNs::types(
@@ -904,7 +877,7 @@ where
     }
 
     fn import_all_legacy_macros(&mut self, module_id: LocalModuleId) {
-        let macros = self.def_collector.def_map[module_id].scope.legacy_macros.clone();
+        let macros = self.def_collector.def_map[module_id].scope.collect_legacy_macros();
         for (name, macro_) in macros {
             self.def_collector.define_legacy_macro(self.module_id, name.clone(), macro_);
         }

crates/ra_hir_def/src/nameres/tests.rs

@@ -32,27 +32,22 @@ fn render_crate_def_map(map: &CrateDefMap) -> String {
         *buf += path;
         *buf += "\n";
 
-        let mut entries = map.modules[module]
+        let mut entries = map.modules[module].scope.collect_resolutions();
-            .scope
+        entries.sort_by_key(|(name, _)| name.clone());
-            .items
-            .iter()
-            .map(|(name, res)| (name, res.def))
-            .collect::<Vec<_>>();
-        entries.sort_by_key(|(name, _)| *name);
 
         for (name, res) in entries {
             *buf += &format!("{}:", name);
 
-            if res.types.is_some() {
+            if res.def.types.is_some() {
                 *buf += " t";
             }
-            if res.values.is_some() {
+            if res.def.values.is_some() {
                 *buf += " v";
             }
-            if res.macros.is_some() {
+            if res.def.macros.is_some() {
                 *buf += " m";
             }
-            if res.is_none() {
+            if res.def.is_none() {
                 *buf += " _";
             }
 
@@ -587,6 +582,6 @@ mod b {
     ⋮T: v
     ⋮
     ⋮crate::a
-    ⋮T: t v    
+    ⋮T: t v
 "###);
 }

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.items.len(), 1);
+            assert_eq!(module_data.scope.collect_resolutions().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.items.len(), 1);
+            assert_eq!(module_data.scope.collect_resolutions().len(), 1);
         });
         assert!(!format!("{:?}", events).contains("crate_def_map"), "{:#?}", events)
     }

crates/ra_hir_def/src/nameres/tests/macros.rs

@@ -610,7 +610,7 @@ fn expand_derive() {
         struct Foo;
         ",
     );
-    assert_eq!(map.modules[map.root].impls.len(), 1);
+    assert_eq!(map.modules[map.root].scope.impls().len(), 1);
 }
 
 #[test]
@@ -622,5 +622,5 @@ fn expand_multiple_derive() {
         struct Foo;
         ",
     );
-    assert_eq!(map.modules[map.root].impls.len(), 2);
+    assert_eq!(map.modules[map.root].scope.impls().len(), 2);
 }

crates/ra_hir_def/src/resolver.rs

@@ -14,7 +14,8 @@ use crate::{
     db::DefDatabase,
     expr::{ExprId, PatId},
     generics::GenericParams,
-    nameres::{BuiltinShadowMode, CrateDefMap},
+    item_scope::BuiltinShadowMode,
+    nameres::CrateDefMap,
     path::{ModPath, PathKind},
     per_ns::PerNs,
     AdtId, AssocContainerId, ConstId, ContainerId, DefWithBodyId, EnumId, EnumVariantId,

crates/ra_hir_ty/src/method_resolution.rs

@@ -58,7 +58,7 @@ impl CrateImplBlocks {
 
         let crate_def_map = db.crate_def_map(krate);
         for (_module_id, module_data) in crate_def_map.modules.iter() {
-            for &impl_id in module_data.impls.iter() {
+            for impl_id in module_data.scope.impls() {
                 match db.impl_trait(impl_id) {
                     Some(tr) => {
                         res.impls_by_trait.entry(tr.trait_).or_default().push(impl_id);

crates/ra_hir_ty/src/test_db.rs

@@ -98,7 +98,7 @@ impl TestDB {
                     }
                 }
 
-                for &impl_id in crate_def_map[module_id].impls.iter() {
+                for impl_id in crate_def_map[module_id].scope.impls() {
                     let impl_data = self.impl_data(impl_id);
                     for item in impl_data.items.iter() {
                         if let AssocItemId::FunctionId(f) = item {

crates/ra_hir_ty/src/tests.rs

@@ -182,7 +182,7 @@ fn visit_module(
             _ => (),
         }
     }
-    for &impl_id in crate_def_map[module_id].impls.iter() {
+    for impl_id in crate_def_map[module_id].scope.impls() {
         let impl_data = db.impl_data(impl_id);
         for &item in impl_data.items.iter() {
             match item {