Merge #634

634: rename def_id -> def r=matklad a=matklad



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

crates/ra_arena/src/map.rs

@@ -29,6 +29,10 @@ impl<ID: ArenaId, T> ArenaMap<ID, T> {
         self.v.get(Self::to_idx(id)).and_then(|it| it.as_ref())
     }
 
+    pub fn get_mut(&mut self, id: ID) -> Option<&mut T> {
+        self.v.get_mut(Self::to_idx(id)).and_then(|it| it.as_mut())
+    }
+
     pub fn values(&self) -> impl Iterator<Item = &T> {
         self.v.iter().filter_map(|o| o.as_ref())
     }

crates/ra_hir/src/code_model_api.rs

@@ -165,11 +165,11 @@ impl Module {
 
     /// Returns a `ModuleScope`: a set of items, visible in this module.
     pub fn scope(&self, db: &impl HirDatabase) -> ModuleScope {
-        self.scope_impl(db)
+        db.item_map(self.krate)[self.module_id].clone()
     }
 
     pub fn resolve_path(&self, db: &impl HirDatabase, path: &Path) -> PerNs<ModuleDef> {
-        self.resolve_path_impl(db, path)
+        db.item_map(self.krate).resolve_path(db, *self, path)
     }
 
     pub fn problems(&self, db: &impl HirDatabase) -> Vec<(TreeArc<SyntaxNode>, Problem)> {

crates/ra_hir/src/code_model_impl/module.rs

@@ -2,10 +2,10 @@ use ra_db::FileId;
 use ra_syntax::{ast, SyntaxNode, TreeArc};
 
 use crate::{
-    Module, ModuleSource, Problem, ModuleDef,
+    Module, ModuleSource, Problem,
-    Crate, Name, Path, PathKind, PerNs,
+    Crate, Name,
     module_tree::ModuleId,
-    nameres::{ModuleScope, lower::ImportId},
+    nameres::{lower::ImportId},
     db::HirDatabase,
 };
 
@@ -90,74 +90,6 @@ impl Module {
         Some(self.with_module_id(parent_id))
     }
 
-    /// Returns a `ModuleScope`: a set of items, visible in this module.
-    pub(crate) fn scope_impl(&self, db: &impl HirDatabase) -> ModuleScope {
-        let item_map = db.item_map(self.krate);
-        item_map.per_module[&self.module_id].clone()
-    }
-
-    pub(crate) fn resolve_path_impl(&self, db: &impl HirDatabase, path: &Path) -> PerNs<ModuleDef> {
-        let mut curr_per_ns: PerNs<ModuleDef> = PerNs::types(match path.kind {
-            PathKind::Crate => self.crate_root(db).into(),
-            PathKind::Self_ | PathKind::Plain => self.clone().into(),
-            PathKind::Super => {
-                if let Some(p) = self.parent(db) {
-                    p.into()
-                } else {
-                    return PerNs::none();
-                }
-            }
-            PathKind::Abs => {
-                // TODO: absolute use is not supported
-                return PerNs::none();
-            }
-        });
-
-        for segment in path.segments.iter() {
-            let curr = match curr_per_ns.as_ref().take_types() {
-                Some(r) => r,
-                None => {
-                    // we still have path segments left, but the path so far
-                    // didn't resolve in the types namespace => no resolution
-                    // (don't break here because curr_per_ns might contain
-                    // something in the value namespace, and it would be wrong
-                    // to return that)
-                    return PerNs::none();
-                }
-            };
-            // resolve segment in curr
-
-            curr_per_ns = match curr {
-                ModuleDef::Module(m) => {
-                    let scope = m.scope(db);
-                    match scope.get(&segment.name) {
-                        Some(r) => r.def_id.clone(),
-                        None => PerNs::none(),
-                    }
-                }
-                ModuleDef::Enum(e) => {
-                    // enum variant
-                    let matching_variant = e
-                        .variants(db)
-                        .into_iter()
-                        .find(|(n, _variant)| n == &segment.name);
-
-                    match matching_variant {
-                        Some((_n, variant)) => PerNs::both(variant.into(), (*e).into()),
-                        None => PerNs::none(),
-                    }
-                }
-                _ => {
-                    // could be an inherent method call in UFCS form
-                    // (`Struct::method`), or some other kind of associated
-                    // item... Which we currently don't handle (TODO)
-                    PerNs::none()
-                }
-            };
-        }
-        curr_per_ns
-    }
-
     pub(crate) fn problems_impl(
         &self,
         db: &impl HirDatabase,

crates/ra_hir/src/marks.rs

@@ -1,3 +1,4 @@
-use test_utils::mark;
+test_utils::marks!(
-
+    name_res_works_for_broken_modules
-mark!(name_res_works_for_broken_modules);
+    item_map_enum_importing
+);

crates/ra_hir/src/nameres.rs

@@ -19,6 +19,8 @@ pub(crate) mod lower;
 use std::sync::Arc;
 
 use ra_db::CrateId;
+use ra_arena::map::ArenaMap;
+use test_utils::tested_by;
 use rustc_hash::{FxHashMap, FxHashSet};
 
 use crate::{
@@ -27,16 +29,21 @@ use crate::{
     HirDatabase, Crate,
     Name,
     module_tree::{ModuleId, ModuleTree},
-//FIXME: deglobify
+    nameres::lower::{ImportId, LoweredModule, ImportData},
-    nameres::lower::*,
 };
 
 /// `ItemMap` is the result of name resolution. It contains, for each
 /// module, the set of visible items.
-// FIXME: currenty we compute item map per source-root. We should do it per crate instead.
 #[derive(Default, Debug, PartialEq, Eq)]
 pub struct ItemMap {
-    pub per_module: FxHashMap<ModuleId, ModuleScope>,
+    per_module: ArenaMap<ModuleId, ModuleScope>,
+}
+
+impl std::ops::Index<ModuleId> for ItemMap {
+    type Output = ModuleScope;
+    fn index(&self, id: ModuleId) -> &ModuleScope {
+        &self.per_module[id]
+    }
 }
 
 #[derive(Debug, Default, PartialEq, Eq, Clone)]
@@ -58,7 +65,7 @@ impl ModuleScope {
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Resolution {
     /// None for unresolved
-    pub def_id: PerNs<ModuleDef>,
+    pub def: PerNs<ModuleDef>,
     /// ident by which this is imported into local scope.
     pub import: Option<ImportId>,
 }
@@ -210,11 +217,11 @@ where
                 let krate = Crate::new(crate_id);
                 for dep in krate.dependencies(self.db) {
                     if let Some(module) = dep.krate.root_module(self.db) {
-                        let def_id = module.into();
+                        let def = module.into();
                         self.add_module_item(
                             &mut module_items,
                             dep.name.clone(),
-                            PerNs::types(def_id),
+                            PerNs::types(def),
                         );
                     }
                 }
@@ -226,7 +233,7 @@ where
                     module_items.items.insert(
                         segment.name.clone(),
                         Resolution {
-                            def_id: PerNs::none(),
+                            def: PerNs::none(),
                             import: Some(import_id),
                         },
                     );
@@ -234,11 +241,8 @@ where
             }
         }
         // Populate explicitly declared items, except modules
-        for (name, &def_id) in input.declarations.iter() {
+        for (name, &def) in input.declarations.iter() {
-            let resolution = Resolution {
+            let resolution = Resolution { def, import: None };
-                def_id,
-                import: None,
-            };
             module_items.items.insert(name.clone(), resolution);
         }
 
@@ -254,16 +258,8 @@ where
         self.result.per_module.insert(module_id, module_items);
     }
 
-    fn add_module_item(
+    fn add_module_item(&self, module_items: &mut ModuleScope, name: Name, def: PerNs<ModuleDef>) {
-        &self,
+        let resolution = Resolution { def, import: None };
-        module_items: &mut ModuleScope,
-        name: Name,
-        def_id: PerNs<ModuleDef>,
-    ) {
-        let resolution = Resolution {
-            def_id,
-            import: None,
-        };
         module_items.items.insert(name, resolution);
     }
 
@@ -273,7 +269,7 @@ where
                 // already done
                 continue;
             }
-            if self.resolve_import(module_id, import_id, import_data) {
+            if self.resolve_import(module_id, import_id, import_data) == ReachedFixedPoint::Yes {
                 log::debug!("import {:?} resolved (or definite error)", import_id);
                 self.processed_imports.insert((module_id, import_id));
             }
@@ -285,118 +281,148 @@ where
         module_id: ModuleId,
         import_id: ImportId,
         import: &ImportData,
-    ) -> bool {
+    ) -> ReachedFixedPoint {
         log::debug!("resolving import: {:?}", import);
         if import.is_glob {
-            return false;
+            return ReachedFixedPoint::Yes;
         };
-
+        let original_module = Module {
-        let mut curr: ModuleId = match import.path.kind {
+            krate: self.krate,
-            PathKind::Plain | PathKind::Self_ => module_id,
+            module_id,
-            PathKind::Super => {
-                match module_id.parent(&self.module_tree) {
-                    Some(it) => it,
-                    None => {
-                        // TODO: error
-                        log::debug!("super path in root module");
-                        return true; // this can't suddenly resolve if we just resolve some other imports
-                    }
-                }
-            }
-            PathKind::Crate => module_id.crate_root(&self.module_tree),
-            PathKind::Abs => {
-                // TODO: absolute use is not supported for now
-                return false;
-            }
         };
+        let (def, reached_fixedpoint) =
+            self.result
+                .resolve_path_fp(self.db, original_module, &import.path);
 
-        for (i, segment) in import.path.segments.iter().enumerate() {
+        if reached_fixedpoint == ReachedFixedPoint::Yes {
-            let is_last = i == import.path.segments.len() - 1;
+            let last_segment = import.path.segments.last().unwrap();
-
+            self.update(module_id, |items| {
-            let def_id = match self.result.per_module[&curr].items.get(&segment.name) {
+                let res = Resolution {
-                Some(res) if !res.def_id.is_none() => res.def_id,
+                    def,
-                _ => {
+                    import: Some(import_id),
-                    log::debug!("path segment {:?} not found", segment.name);
-                    return false;
-                }
-            };
-
-            if !is_last {
-                let type_def_id = if let Some(d) = def_id.take(Namespace::Types) {
-                    d
-                } else {
-                    log::debug!(
-                        "path segment {:?} resolved to value only, but is not last",
-                        segment.name
-                    );
-                    return false;
                 };
-                curr = match type_def_id {
+                items.items.insert(last_segment.name.clone(), res);
-                    ModuleDef::Module(module) => {
+            });
-                        if module.krate == self.krate {
+            log::debug!(
-                            module.module_id
+                "resolved import {:?} ({:?}) cross-source root to {:?}",
-                        } else {
+                last_segment.name,
-                            let path = Path {
+                import,
-                                segments: import.path.segments[i + 1..].iter().cloned().collect(),
+                def,
-                                kind: PathKind::Crate,
+            );
-                            };
-                            log::debug!("resolving {:?} in other source root", path);
-                            let def_id = module.resolve_path(self.db, &path);
-                            if !def_id.is_none() {
-                                let last_segment = path.segments.last().unwrap();
-                                self.update(module_id, |items| {
-                                    let res = Resolution {
-                                        def_id,
-                                        import: Some(import_id),
-                                    };
-                                    items.items.insert(last_segment.name.clone(), res);
-                                });
-                                log::debug!(
-                                    "resolved import {:?} ({:?}) cross-source root to {:?}",
-                                    last_segment.name,
-                                    import,
-                                    def_id,
-                                );
-                                return true;
-                            } else {
-                                log::debug!("rest of path did not resolve in other source root");
-                                return true;
-                            }
-                        }
-                    }
-                    _ => {
-                        log::debug!(
-                            "path segment {:?} resolved to non-module {:?}, but is not last",
-                            segment.name,
-                            type_def_id,
-                        );
-                        return true; // this resolved to a non-module, so the path won't ever resolve
-                    }
-                }
-            } else {
-                log::debug!(
-                    "resolved import {:?} ({:?}) within source root to {:?}",
-                    segment.name,
-                    import,
-                    def_id,
-                );
-                self.update(module_id, |items| {
-                    let res = Resolution {
-                        def_id,
-                        import: Some(import_id),
-                    };
-                    items.items.insert(segment.name.clone(), res);
-                })
-            }
         }
-        true
+        reached_fixedpoint
     }
 
     fn update(&mut self, module_id: ModuleId, f: impl FnOnce(&mut ModuleScope)) {
-        let module_items = self.result.per_module.get_mut(&module_id).unwrap();
+        let module_items = self.result.per_module.get_mut(module_id).unwrap();
         f(module_items)
     }
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum ReachedFixedPoint {
+    Yes,
+    No,
+}
+
+impl ItemMap {
+    pub(crate) fn resolve_path(
+        &self,
+        db: &impl HirDatabase,
+        original_module: Module,
+        path: &Path,
+    ) -> PerNs<ModuleDef> {
+        self.resolve_path_fp(db, original_module, path).0
+    }
+
+    // Returns Yes if we are sure that additions to `ItemMap` wouldn't change
+    // the result.
+    fn resolve_path_fp(
+        &self,
+        db: &impl HirDatabase,
+        original_module: Module,
+        path: &Path,
+    ) -> (PerNs<ModuleDef>, ReachedFixedPoint) {
+        let mut curr_per_ns: PerNs<ModuleDef> = PerNs::types(match path.kind {
+            PathKind::Crate => original_module.crate_root(db).into(),
+            PathKind::Self_ | PathKind::Plain => original_module.into(),
+            PathKind::Super => {
+                if let Some(p) = original_module.parent(db) {
+                    p.into()
+                } else {
+                    log::debug!("super path in root module");
+                    return (PerNs::none(), ReachedFixedPoint::Yes);
+                }
+            }
+            PathKind::Abs => {
+                // TODO: absolute use is not supported
+                return (PerNs::none(), ReachedFixedPoint::Yes);
+            }
+        });
+
+        for (i, segment) in path.segments.iter().enumerate() {
+            let curr = match curr_per_ns.as_ref().take_types() {
+                Some(r) => r,
+                None => {
+                    // we still have path segments left, but the path so far
+                    // didn't resolve in the types namespace => no resolution
+                    // (don't break here because curr_per_ns might contain
+                    // something in the value namespace, and it would be wrong
+                    // to return that)
+                    return (PerNs::none(), ReachedFixedPoint::No);
+                }
+            };
+            // resolve segment in curr
+
+            curr_per_ns = match curr {
+                ModuleDef::Module(module) => {
+                    if module.krate != original_module.krate {
+                        let path = Path {
+                            segments: path.segments[i..].iter().cloned().collect(),
+                            kind: PathKind::Crate,
+                        };
+                        log::debug!("resolving {:?} in other crate", path);
+                        let def = module.resolve_path(db, &path);
+                        return (def, ReachedFixedPoint::Yes);
+                    }
+
+                    match self[module.module_id].items.get(&segment.name) {
+                        Some(res) if !res.def.is_none() => res.def,
+                        _ => {
+                            log::debug!("path segment {:?} not found", segment.name);
+                            return (PerNs::none(), ReachedFixedPoint::No);
+                        }
+                    }
+                }
+                ModuleDef::Enum(e) => {
+                    // enum variant
+                    tested_by!(item_map_enum_importing);
+                    let matching_variant = e
+                        .variants(db)
+                        .into_iter()
+                        .find(|(n, _variant)| n == &segment.name);
+
+                    match matching_variant {
+                        Some((_n, variant)) => PerNs::both(variant.into(), (*e).into()),
+                        None => PerNs::none(),
+                    }
+                }
+                _ => {
+                    // could be an inherent method call in UFCS form
+                    // (`Struct::method`), or some other kind of associated
+                    // item... Which we currently don't handle (TODO)
+                    log::debug!(
+                        "path segment {:?} resolved to non-module {:?}, but is not last",
+                        segment.name,
+                        curr,
+                    );
+                    return (PerNs::none(), ReachedFixedPoint::Yes);
+                }
+            };
+        }
+        (curr_per_ns, ReachedFixedPoint::Yes)
+    }
+}
+
 #[cfg(test)]
 mod tests;

crates/ra_hir/src/nameres/tests.rs

@@ -20,7 +20,7 @@ fn item_map(fixture: &str) -> (Arc<ItemMap>, ModuleId) {
 }
 
 fn check_module_item_map(map: &ItemMap, module_id: ModuleId, expected: &str) {
-    let mut lines = map.per_module[&module_id]
+    let mut lines = map[module_id]
         .items
         .iter()
         .map(|(name, res)| format!("{}: {}", name, dump_resolution(res)))
@@ -37,8 +37,8 @@ fn check_module_item_map(map: &ItemMap, module_id: ModuleId, expected: &str) {
 
     fn dump_resolution(resolution: &Resolution) -> &'static str {
         match (
-            resolution.def_id.types.is_some(),
+            resolution.def.types.is_some(),
-            resolution.def_id.values.is_some(),
+            resolution.def.values.is_some(),
         ) {
             (true, true) => "t v",
             (true, false) => "t",
@@ -215,6 +215,27 @@ fn item_map_using_self() {
     );
 }
 
+#[test]
+fn item_map_enum_importing() {
+    covers!(item_map_enum_importing);
+    let (item_map, module_id) = item_map(
+        "
+        //- /lib.rs
+        enum E { V }
+        use self::E::V;
+        <|>
+        ",
+    );
+    check_module_item_map(
+        &item_map,
+        module_id,
+        "
+        E: t
+        V: t v
+        ",
+    );
+}
+
 #[test]
 fn item_map_across_crates() {
     let (mut db, sr) = MockDatabase::with_files(

crates/ra_ide_api/src/completion/completion_item.rs

@@ -209,10 +209,7 @@ impl Builder {
         ctx: &CompletionContext,
         resolution: &hir::Resolution,
     ) -> Builder {
-        let def = resolution
+        let def = resolution.def.take_types().or(resolution.def.take_values());
-            .def_id
-            .take_types()
-            .or(resolution.def_id.take_values());
         let def = match def {
             None => return self,
             Some(it) => it,

crates/ra_ide_api/src/marks.rs

@@ -1,3 +1 @@
-use test_utils::mark;
+test_utils::marks!(inserts_parens_for_function_calls);
-
-mark!(inserts_parens_for_function_calls);

crates/test_utils/src/marks.rs

@@ -46,11 +46,13 @@ macro_rules! covers {
 }
 
 #[macro_export]
-macro_rules! mark {
+macro_rules! marks {
-    ($ident:ident) => {
+    ($($ident:ident)*) => {
+        $(
         #[allow(bad_style)]
         pub(crate) static $ident: std::sync::atomic::AtomicUsize =
             std::sync::atomic::AtomicUsize::new(0);
+        )*
     };
 }