Apply review suggestions

Author: flodiebold

crates/ra_assists/src/ast_transform.rs

@@ -60,6 +60,8 @@ impl<'a, DB: HirDatabase> SubstituteTypeParams<'a, DB> {
             previous: Box::new(NullTransformer),
         };
 
+        // FIXME: It would probably be nicer if we could get this via HIR (i.e. get the
+        // trait ref, and then go from the types in the substs back to the syntax)
         fn get_syntactic_substs(impl_block: ast::ImplBlock) -> Option<Vec<ast::TypeRef>> {
             let target_trait = impl_block.target_trait()?;
             let path_type = match target_trait {
@@ -131,12 +133,12 @@ impl<'a, DB: HirDatabase> QualifyPaths<'a, DB> {
         let resolution = analyzer.resolve_path(self.db, &p)?;
         match resolution {
             PathResolution::Def(def) => {
-                let found_path = from.find_path(self.db, def)?;
+                let found_path = from.find_use_path(self.db, def)?;
                 let args = p
                     .segment()
                     .and_then(|s| s.type_arg_list())
                     .map(|arg_list| apply(self, node.with_value(arg_list)));
-                Some(make::path_with_type_arg_list(found_path.to_ast(), args).syntax().clone())
+                Some(make::path_with_type_arg_list(path_to_ast(found_path), args).syntax().clone())
             }
             PathResolution::Local(_)
             | PathResolution::TypeParam(_)
@@ -171,8 +173,7 @@ impl<'a, DB: HirDatabase> AstTransform<'a> for QualifyPaths<'a, DB> {
     }
 }
 
-// FIXME: It would probably be nicer if we could get this via HIR (i.e. get the
-// trait ref, and then go from the types in the substs back to the syntax)
-// FIXME: This should be a general utility (not even just for assists)
-
-// FIXME: This should be a general utility (not even just for assists)
+fn path_to_ast(path: hir::ModPath) -> ast::Path {
+    let parse = ast::SourceFile::parse(&path.to_string());
+    parse.tree().syntax().descendants().find_map(ast::Path::cast).unwrap()
+}

crates/ra_hir/src/code_model.rs

@@ -228,7 +228,9 @@ impl Module {
         Module::new(self.krate(), module_id)
     }
 
-    pub fn find_path(
+    /// Finds a path that can be used to refer to the given item from within
+    /// this module, if possible.
+    pub fn find_use_path(
         self,
         db: &impl DefDatabase,
         item: ModuleDef,

crates/ra_hir/src/source_binder.rs

@@ -206,7 +206,7 @@ impl SourceAnalyzer {
     }
 
     pub fn module(&self) -> Option<crate::code_model::Module> {
-        Some(crate::code_model::Module { id: self.resolver.module_id()? })
+        Some(crate::code_model::Module { id: self.resolver.module()? })
     }
 
     fn expr_id(&self, expr: &ast::Expr) -> Option<ExprId> {

crates/ra_hir_def/src/db.rs

@@ -107,13 +107,6 @@ pub trait DefDatabase: InternDatabase + AstDatabase {
     // Remove this query completely, in favor of `Attrs::docs` method
     #[salsa::invoke(Documentation::documentation_query)]
     fn documentation(&self, def: AttrDefId) -> Option<Documentation>;
-
-    #[salsa::invoke(crate::find_path::importable_locations_in_crate_query)]
-    fn importable_locations_in_crate(
-        &self,
-        item: crate::item_scope::ItemInNs,
-        krate: CrateId,
-    ) -> Arc<[(ModuleId, hir_expand::name::Name, crate::visibility::Visibility)]>;
 }
 
 fn crate_def_map(db: &impl DefDatabase, krate: CrateId) -> Arc<CrateDefMap> {

crates/ra_hir_def/src/find_path.rs

@@ -34,7 +34,7 @@ fn find_path_inner(
     // - if the item is already in scope, return the name under which it is
     let def_map = db.crate_def_map(from.krate);
     let from_scope: &crate::item_scope::ItemScope = &def_map.modules[from.local_id].scope;
-    if let Some((name, _)) = from_scope.reverse_get(item) {
+    if let Some((name, _)) = from_scope.name_of(item) {
         return Some(ModPath::from_simple_segments(PathKind::Plain, vec![name.clone()]));
     }
 
@@ -77,7 +77,7 @@ fn find_path_inner(
         let prelude_def_map = db.crate_def_map(prelude_module.krate);
         let prelude_scope: &crate::item_scope::ItemScope =
             &prelude_def_map.modules[prelude_module.local_id].scope;
-        if let Some((name, vis)) = prelude_scope.reverse_get(item) {
+        if let Some((name, vis)) = prelude_scope.name_of(item) {
             if vis.is_visible_from(db, from) {
                 return Some(ModPath::from_simple_segments(PathKind::Plain, vec![name.clone()]));
             }
@@ -146,7 +146,7 @@ fn find_importable_locations(
         .chain(crate_graph.dependencies(from.krate).map(|dep| dep.crate_id))
     {
         result.extend(
-            db.importable_locations_in_crate(item, krate)
+            importable_locations_in_crate(db, item, krate)
                 .iter()
                 .filter(|(_, _, vis)| vis.is_visible_from(db, from))
                 .map(|(m, n, _)| (*m, n.clone())),
@@ -160,17 +160,16 @@ fn find_importable_locations(
 /// non-private `use`s.
 ///
 /// Note that the crate doesn't need to be the one in which the item is defined;
-/// it might be re-exported in other crates. We cache this as a query since we
-/// need to walk the whole def map for it.
-pub(crate) fn importable_locations_in_crate_query(
+/// it might be re-exported in other crates.
+fn importable_locations_in_crate(
     db: &impl DefDatabase,
     item: ItemInNs,
     krate: CrateId,
-) -> std::sync::Arc<[(ModuleId, Name, Visibility)]> {
+) -> Vec<(ModuleId, Name, Visibility)> {
     let def_map = db.crate_def_map(krate);
     let mut result = Vec::new();
     for (local_id, data) in def_map.modules.iter() {
-        if let Some((name, vis)) = data.scope.reverse_get(item) {
+        if let Some((name, vis)) = data.scope.name_of(item) {
             let is_private = if let Visibility::Module(private_to) = vis {
                 private_to.local_id == local_id
             } else {
@@ -192,7 +191,7 @@ pub(crate) fn importable_locations_in_crate_query(
             result.push((ModuleId { krate, local_id }, name.clone(), vis));
         }
     }
-    result.into()
+    result
 }
 
 #[cfg(test)]

crates/ra_hir_def/src/item_scope.rs

@@ -104,7 +104,7 @@ impl ItemScope {
         }
     }
 
-    pub(crate) fn reverse_get(&self, item: ItemInNs) -> Option<(&Name, Visibility)> {
+    pub(crate) fn name_of(&self, item: ItemInNs) -> Option<(&Name, Visibility)> {
         for (name, per_ns) in &self.visible {
             if let Some(vis) = item.match_with(*per_ns) {
                 return Some((name, vis));
@@ -207,8 +207,7 @@ impl ItemInNs {
 
     pub fn as_module_def_id(self) -> Option<ModuleDefId> {
         match self {
-            ItemInNs::Types(t) => Some(t),
-            ItemInNs::Values(v) => Some(v),
+            ItemInNs::Types(id) | ItemInNs::Values(id) => Some(id),
             ItemInNs::Macros(_) => None,
         }
     }

crates/ra_hir_def/src/path.rs

@@ -1,7 +1,11 @@
 //! A desugared representation of paths like `crate::foo` or `<Type as Trait>::bar`.
 mod lower;
 
-use std::{fmt::Display, iter, sync::Arc};
+use std::{
+    fmt::{self, Display},
+    iter,
+    sync::Arc,
+};
 
 use hir_expand::{
     hygiene::Hygiene,
@@ -78,12 +82,6 @@ impl ModPath {
         }
         self.segments.first()
     }
-
-    pub fn to_ast(&self) -> ast::Path {
-        use ast::AstNode;
-        let parse = ast::SourceFile::parse(&self.to_string());
-        parse.tree().syntax().descendants().find_map(ast::Path::cast).unwrap()
-    }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -255,7 +253,7 @@ impl From<Name> for ModPath {
 }
 
 impl Display for ModPath {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let mut first_segment = true;
         let mut add_segment = |s| {
             if !first_segment {

crates/ra_hir_def/src/resolver.rs

@@ -128,7 +128,7 @@ impl Resolver {
         path: &ModPath,
         shadow: BuiltinShadowMode,
     ) -> PerNs {
-        let (item_map, module) = match self.module() {
+        let (item_map, module) = match self.module_scope() {
             Some(it) => it,
             None => return PerNs::none(),
         };
@@ -239,7 +239,7 @@ impl Resolver {
     ) -> Option<Visibility> {
         match visibility {
             RawVisibility::Module(_) => {
-                let (item_map, module) = match self.module() {
+                let (item_map, module) = match self.module_scope() {
                     Some(it) => it,
                     None => return None,
                 };
@@ -379,7 +379,7 @@ impl Resolver {
         db: &impl DefDatabase,
         path: &ModPath,
     ) -> Option<MacroDefId> {
-        let (item_map, module) = self.module()?;
+        let (item_map, module) = self.module_scope()?;
         item_map.resolve_path(db, module, &path, BuiltinShadowMode::Other).0.take_macros()
     }
 
@@ -403,21 +403,21 @@ impl Resolver {
         traits
     }
 
-    fn module(&self) -> Option<(&CrateDefMap, LocalModuleId)> {
+    fn module_scope(&self) -> Option<(&CrateDefMap, LocalModuleId)> {
         self.scopes.iter().rev().find_map(|scope| match scope {
             Scope::ModuleScope(m) => Some((&*m.crate_def_map, m.module_id)),
 
             _ => None,
         })
     }
 
-    pub fn module_id(&self) -> Option<ModuleId> {
-        let (def_map, local_id) = self.module()?;
+    pub fn module(&self) -> Option<ModuleId> {
+        let (def_map, local_id) = self.module_scope()?;
         Some(ModuleId { krate: def_map.krate, local_id })
     }
 
     pub fn krate(&self) -> Option<CrateId> {
-        self.module().map(|t| t.0.krate)
+        self.module_scope().map(|t| t.0.krate)
     }
 
     pub fn where_predicates_in_scope<'a>(

crates/ra_hir_expand/src/name.rs

@@ -37,10 +37,6 @@ impl Name {
         Name(Repr::TupleField(idx))
     }
 
-    pub fn for_crate_dependency(dep: &ra_db::Dependency) -> Name {
-        Name::new_text(dep.name.clone())
-    }
-
     /// Shortcut to create inline plain text name
     const fn new_inline_ascii(text: &[u8]) -> Name {
         Name::new_text(SmolStr::new_inline_from_ascii(text.len(), text))