Merge #3260

3260: Refactor how builtins are resolved r=matklad a=flodiebold

This fixes autocompletion suggesting e.g. `self::usize`. (I thought we had a bug for that, but I didn't find it.)

Co-authored-by: Florian Diebold <[email protected]>

5 files changed, 55 insertions, 68 deletions

diff --git a/crates/ra_hir_def/src/item_scope.rs b/crates/ra_hir_def/src/item_scope.rs
index 6e958ca75..5e943b780 100644
--- a/crates/ra_hir_def/src/item_scope.rs
+++ b/crates/ra_hir_def/src/item_scope.rs
@@ -30,7 +30,7 @@ pub struct ItemScope {
     legacy_macros: FxHashMap<Name, MacroDefId>,
 }
 
-static BUILTIN_SCOPE: Lazy<FxHashMap<Name, PerNs>> = Lazy::new(|| {
+pub(crate) static BUILTIN_SCOPE: Lazy<FxHashMap<Name, PerNs>> = Lazy::new(|| {
     BuiltinType::ALL
         .iter()
         .map(|(name, ty)| (name.clone(), PerNs::types(ty.clone().into(), Visibility::Public)))
@@ -40,9 +40,9 @@ static BUILTIN_SCOPE: Lazy<FxHashMap<Name, PerNs>> = Lazy::new(|| {
 /// Shadow mode for builtin type which can be shadowed by module.
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub(crate) enum BuiltinShadowMode {
-    // Prefer Module
+    /// Prefer user-defined modules (or other types) over builtins.
     Module,
-    // Prefer Other Types
+    /// Prefer builtins over user-defined modules (but not other types).
     Other,
 }
 
@@ -51,7 +51,7 @@ pub(crate) enum BuiltinShadowMode {
 impl ItemScope {
     pub fn entries<'a>(&'a self) -> impl Iterator<Item = (&'a Name, PerNs)> + 'a {
         //FIXME: shadowing
-        self.visible.iter().chain(BUILTIN_SCOPE.iter()).map(|(n, def)| (n, *def))
+        self.visible.iter().map(|(n, def)| (n, *def))
     }
 
     pub fn entries_without_primitives<'a>(
@@ -79,29 +79,8 @@ impl ItemScope {
     }
 
     /// Get a name from current module scope, legacy macros are not included
-    pub(crate) fn get(&self, name: &Name, shadow: BuiltinShadowMode) -> PerNs {
-        match shadow {
-            BuiltinShadowMode::Module => self
-                .visible
-                .get(name)
-                .or_else(|| BUILTIN_SCOPE.get(name))
-                .copied()
-                .unwrap_or_else(PerNs::none),
-            BuiltinShadowMode::Other => {
-                let item = self.visible.get(name).copied();
-                if let Some(def) = item {
-                    if let Some(ModuleDefId::ModuleId(_)) = def.take_types() {
-                        return BUILTIN_SCOPE
-                            .get(name)
-                            .copied()
-                            .or(item)
-                            .unwrap_or_else(PerNs::none);
-                    }
-                }
-
-                item.or_else(|| BUILTIN_SCOPE.get(name).copied()).unwrap_or_else(PerNs::none)
-            }
-        }
+    pub(crate) fn get(&self, name: &Name) -> PerNs {
+        self.visible.get(name).copied().unwrap_or_else(PerNs::none)
     }
 
     pub(crate) fn name_of(&self, item: ItemInNs) -> Option<(&Name, Visibility)> {
diff --git a/crates/ra_hir_def/src/nameres/path_resolution.rs b/crates/ra_hir_def/src/nameres/path_resolution.rs
index fd6422d60..c058e70aa 100644
--- a/crates/ra_hir_def/src/nameres/path_resolution.rs
+++ b/crates/ra_hir_def/src/nameres/path_resolution.rs
@@ -18,6 +18,7 @@ use test_utils::tested_by;
 
 use crate::{
     db::DefDatabase,
+    item_scope::BUILTIN_SCOPE,
     nameres::{BuiltinShadowMode, CrateDefMap},
     path::{ModPath, PathKind},
     per_ns::PerNs,
@@ -103,15 +104,6 @@ impl CrateDefMap {
         path: &ModPath,
         shadow: BuiltinShadowMode,
     ) -> ResolvePathResult {
-        // if it is not the last segment, we prefer the module to the builtin
-        let prefer_module = |index| {
-            if index == path.segments.len() - 1 {
-                shadow
-            } else {
-                BuiltinShadowMode::Module
-            }
-        };
-
         let mut segments = path.segments.iter().enumerate();
         let mut curr_per_ns: PerNs = match path.kind {
             PathKind::DollarCrate(krate) => {
@@ -140,20 +132,29 @@ impl CrateDefMap {
                 if self.edition == Edition::Edition2015
                     && (path.kind == PathKind::Abs || mode == ResolveMode::Import) =>
             {
-                let (idx, segment) = match segments.next() {
+                let (_, segment) = match segments.next() {
                     Some((idx, segment)) => (idx, segment),
                     None => return ResolvePathResult::empty(ReachedFixedPoint::Yes),
                 };
                 log::debug!("resolving {:?} in crate root (+ extern prelude)", segment);
-                self.resolve_name_in_crate_root_or_extern_prelude(&segment, prefer_module(idx))
+                self.resolve_name_in_crate_root_or_extern_prelude(&segment)
             }
             PathKind::Plain => {
-                let (idx, segment) = match segments.next() {
+                let (_, segment) = match segments.next() {
                     Some((idx, segment)) => (idx, segment),
                     None => return ResolvePathResult::empty(ReachedFixedPoint::Yes),
                 };
+                // The first segment may be a builtin type. If the path has more
+                // than one segment, we first try resolving it as a module
+                // anyway.
+                // FIXME: If the next segment doesn't resolve in the module and
+                // BuiltinShadowMode wasn't Module, then we need to try
+                // resolving it as a builtin.
+                let prefer_module =
+                    if path.segments.len() == 1 { shadow } else { BuiltinShadowMode::Module };
+
                 log::debug!("resolving {:?} in module", segment);
-                self.resolve_name_in_module(db, original_module, &segment, prefer_module(idx))
+                self.resolve_name_in_module(db, original_module, &segment, prefer_module)
             }
             PathKind::Super(lvl) => {
                 let m = successors(Some(original_module), |m| self.modules[*m].parent)
@@ -216,7 +217,7 @@ impl CrateDefMap {
                     }
 
                     // Since it is a qualified path here, it should not contains legacy macros
-                    self[module.local_id].scope.get(&segment, prefer_module(i))
+                    self[module.local_id].scope.get(&segment)
                 }
                 ModuleDefId::AdtId(AdtId::EnumId(e)) => {
                     // enum variant
@@ -275,33 +276,35 @@ impl CrateDefMap {
             .scope
             .get_legacy_macro(name)
             .map_or_else(PerNs::none, |m| PerNs::macros(m, Visibility::Public));
-        let from_scope = self[module].scope.get(name, shadow);
+        let from_scope = self[module].scope.get(name);
+        let from_builtin = BUILTIN_SCOPE.get(name).copied().unwrap_or_else(PerNs::none);
+        let from_scope_or_builtin = match shadow {
+            BuiltinShadowMode::Module => from_scope.or(from_builtin),
+            BuiltinShadowMode::Other => {
+                if let Some(ModuleDefId::ModuleId(_)) = from_scope.take_types() {
+                    from_builtin.or(from_scope)
+                } else {
+                    from_scope.or(from_builtin)
+                }
+            }
+        };
         let from_extern_prelude = self
             .extern_prelude
             .get(name)
             .map_or(PerNs::none(), |&it| PerNs::types(it, Visibility::Public));
-        let from_prelude = self.resolve_in_prelude(db, name, shadow);
+        let from_prelude = self.resolve_in_prelude(db, name);
 
-        from_legacy_macro.or(from_scope).or(from_extern_prelude).or(from_prelude)
+        from_legacy_macro.or(from_scope_or_builtin).or(from_extern_prelude).or(from_prelude)
     }
 
-    fn resolve_name_in_crate_root_or_extern_prelude(
-        &self,
-        name: &Name,
-        shadow: BuiltinShadowMode,
-    ) -> PerNs {
-        let from_crate_root = self[self.root].scope.get(name, shadow);
+    fn resolve_name_in_crate_root_or_extern_prelude(&self, name: &Name) -> PerNs {
+        let from_crate_root = self[self.root].scope.get(name);
         let from_extern_prelude = self.resolve_name_in_extern_prelude(name);
 
         from_crate_root.or(from_extern_prelude)
     }
 
-    fn resolve_in_prelude(
-        &self,
-        db: &impl DefDatabase,
-        name: &Name,
-        shadow: BuiltinShadowMode,
-    ) -> PerNs {
+    fn resolve_in_prelude(&self, db: &impl DefDatabase, name: &Name) -> PerNs {
         if let Some(prelude) = self.prelude {
             let keep;
             let def_map = if prelude.krate == self.krate {
@@ -311,7 +314,7 @@ impl CrateDefMap {
                 keep = db.crate_def_map(prelude.krate);
                 &keep
             };
-            def_map[prelude.local_id].scope.get(name, shadow)
+            def_map[prelude.local_id].scope.get(name)
         } else {
             PerNs::none()
         }
diff --git a/crates/ra_hir_def/src/resolver.rs b/crates/ra_hir_def/src/resolver.rs
index 5365b80e2..9dd4fa555 100644
--- a/crates/ra_hir_def/src/resolver.rs
+++ b/crates/ra_hir_def/src/resolver.rs
@@ -15,7 +15,7 @@ use crate::{
     db::DefDatabase,
     expr::{ExprId, PatId},
     generics::GenericParams,
-    item_scope::BuiltinShadowMode,
+    item_scope::{BuiltinShadowMode, BUILTIN_SCOPE},
     nameres::CrateDefMap,
     path::{ModPath, PathKind},
     per_ns::PerNs,
@@ -193,7 +193,7 @@ impl Resolver {
                     return Some((res, idx));
                 }
                 Scope::LocalItemsScope(body) => {
-                    let def = body.item_scope.get(first_name, BuiltinShadowMode::Other);
+                    let def = body.item_scope.get(first_name);
                     if let Some(res) = to_type_ns(def) {
                         return Some((res, None));
                     }
@@ -335,8 +335,10 @@ impl Resolver {
                     };
                 }
                 Scope::LocalItemsScope(body) => {
-                    let def = body.item_scope.get(first_name, BuiltinShadowMode::Other);
-                    if let Some(res) = to_value_ns(def) {
+                    // we don't bother looking in the builtin scope here because there are no builtin values
+                    let def = to_value_ns(body.item_scope.get(first_name));
+
+                    if let Some(res) = def {
                         return Some(ResolveValueResult::ValueNs(res));
                     }
                 }
@@ -476,6 +478,9 @@ impl Scope {
                 m.crate_def_map.extern_prelude.iter().for_each(|(name, &def)| {
                     f(name.clone(), ScopeDef::PerNs(PerNs::types(def, Visibility::Public)));
                 });
+                BUILTIN_SCOPE.iter().for_each(|(name, &def)| {
+                    f(name.clone(), ScopeDef::PerNs(def));
+                });
                 if let Some(prelude) = m.crate_def_map.prelude {
                     let prelude_def_map = db.crate_def_map(prelude.krate);
                     prelude_def_map[prelude.local_id].scope.entries().for_each(|(name, def)| {
diff --git a/crates/ra_ide/src/completion/complete_path.rs b/crates/ra_ide/src/completion/complete_path.rs
index af24e9f48..2d7f09a6c 100644
--- a/crates/ra_ide/src/completion/complete_path.rs
+++ b/crates/ra_ide/src/completion/complete_path.rs
@@ -1,4 +1,4 @@
-//! FIXME: write short doc here
+//! Completion of paths, including when writing a single name.
 
 use hir::{Adt, PathResolution, ScopeDef};
 use ra_syntax::AstNode;
@@ -20,10 +20,6 @@ pub(super) fn complete_path(acc: &mut Completions, ctx: &CompletionContext) {
             let module_scope = module.scope(ctx.db);
             for (name, def) in module_scope {
                 if ctx.use_item_syntax.is_some() {
-                    if let hir::ScopeDef::ModuleDef(hir::ModuleDef::BuiltinType(..)) = def {
-                        tested_by!(dont_complete_primitive_in_use);
-                        continue;
-                    }
                     if let ScopeDef::Unknown = def {
                         if let Some(name_ref) = ctx.name_ref_syntax.as_ref() {
                             if name_ref.syntax().text() == name.to_string().as_str() {
@@ -125,12 +121,17 @@ mod tests {
 
     #[test]
     fn dont_complete_primitive_in_use() {
-        covers!(dont_complete_primitive_in_use);
         let completions = do_completion(r"use self::<|>;", CompletionKind::BuiltinType);
         assert!(completions.is_empty());
     }
 
     #[test]
+    fn dont_complete_primitive_in_module_scope() {
+        let completions = do_completion(r"fn foo() { self::<|> }", CompletionKind::BuiltinType);
+        assert!(completions.is_empty());
+    }
+
+    #[test]
     fn completes_primitives() {
         let completions =
             do_completion(r"fn main() { let _: <|> = 92; }", CompletionKind::BuiltinType);
diff --git a/crates/ra_ide/src/marks.rs b/crates/ra_ide/src/marks.rs
index 5bf4d2062..bcb67e373 100644
--- a/crates/ra_ide/src/marks.rs
+++ b/crates/ra_ide/src/marks.rs
@@ -10,6 +10,5 @@ test_utils::marks!(
     goto_def_for_field_init_shorthand
     call_info_bad_offset
     dont_complete_current_use
-    dont_complete_primitive_in_use
     test_resolve_parent_module_on_module_decl
 );