Merge #463

463: Use name resolution for goto definition r=matklad a=flodiebold

This tries proper name resolution before falling back on the index.

@matklad There was currently no way of getting the location of a `DefId` from outside `ra_hir`. I added something, but it's probably not the best API, maybe you have a better idea?

Co-authored-by: Florian Diebold <flodiebold@gmail.com>

crates/ra_hir/src/code_model_api.rs

@@ -2,10 +2,10 @@ use std::sync::Arc;
 
 use relative_path::RelativePathBuf;
 use ra_db::{CrateId, Cancelable, FileId};
-use ra_syntax::{ast, TreePtr, SyntaxNode};
+use ra_syntax::{ast, TreePtr, SyntaxNode, AstNode};
 
 use crate::{
-    Name, DefId, Path, PerNs, ScopesWithSyntaxMapping, Ty,
+    Name, DefId, Path, PerNs, ScopesWithSyntaxMapping, Ty, HirFileId,
     type_ref::TypeRef,
     nameres::ModuleScope,
     db::HirDatabase,
@@ -181,6 +181,19 @@ impl Struct {
             .collect();
         Ok(res)
     }
+
+    pub fn source(
+        &self,
+        db: &impl HirDatabase,
+    ) -> Cancelable<(HirFileId, TreePtr<ast::StructDef>)> {
+        let (file_id, syntax) = self.def_id.source(db);
+        Ok((
+            file_id,
+            ast::StructDef::cast(&syntax)
+                .expect("struct def should point to StructDef node")
+                .to_owned(),
+        ))
+    }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -204,6 +217,16 @@ impl Enum {
     pub fn variants(&self, db: &impl HirDatabase) -> Cancelable<Vec<(Name, EnumVariant)>> {
         Ok(db.enum_data(self.def_id)?.variants.clone())
     }
+
+    pub fn source(&self, db: &impl HirDatabase) -> Cancelable<(HirFileId, TreePtr<ast::EnumDef>)> {
+        let (file_id, syntax) = self.def_id.source(db);
+        Ok((
+            file_id,
+            ast::EnumDef::cast(&syntax)
+                .expect("enum def should point to EnumDef node")
+                .to_owned(),
+        ))
+    }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -231,6 +254,19 @@ impl EnumVariant {
     pub fn variant_data(&self, db: &impl HirDatabase) -> Cancelable<Arc<VariantData>> {
         Ok(db.enum_variant_data(self.def_id)?.variant_data.clone())
     }
+
+    pub fn source(
+        &self,
+        db: &impl HirDatabase,
+    ) -> Cancelable<(HirFileId, TreePtr<ast::EnumVariant>)> {
+        let (file_id, syntax) = self.def_id.source(db);
+        Ok((
+            file_id,
+            ast::EnumVariant::cast(&syntax)
+                .expect("variant def should point to EnumVariant node")
+                .to_owned(),
+        ))
+    }
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, Hash)]
@@ -241,11 +277,16 @@ pub struct Function {
 /// The declared signature of a function.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct FnSignature {
+    pub(crate) name: Name,
     pub(crate) args: Vec<TypeRef>,
     pub(crate) ret_type: TypeRef,
 }
 
 impl FnSignature {
+    pub fn name(&self) -> &Name {
+        &self.name
+    }
+
     pub fn args(&self) -> &[TypeRef] {
         &self.args
     }
@@ -260,8 +301,8 @@ impl Function {
         self.def_id
     }
 
-    pub fn source(&self, db: &impl HirDatabase) -> TreePtr<ast::FnDef> {
-        self.source_impl(db)
+    pub fn source(&self, db: &impl HirDatabase) -> Cancelable<(HirFileId, TreePtr<ast::FnDef>)> {
+        Ok(self.source_impl(db))
     }
 
     pub fn body_syntax_mapping(&self, db: &impl HirDatabase) -> Cancelable<Arc<BodySyntaxMapping>> {

crates/ra_hir/src/code_model_impl/function.rs

@@ -5,11 +5,11 @@ use std::sync::Arc;
 use ra_db::Cancelable;
 use ra_syntax::{
     TreePtr,
-    ast::{self, AstNode},
+    ast::{self, AstNode, NameOwner},
 };
 
 use crate::{
-    DefId, DefKind, HirDatabase, Name, Function, FnSignature, Module,
+    DefId, DefKind, HirDatabase, Name, AsName, Function, FnSignature, Module, HirFileId,
     type_ref::{TypeRef, Mutability},
     expr::Body,
     impl_block::ImplBlock,
@@ -22,11 +22,14 @@ impl Function {
         Function { def_id }
     }
 
-    pub(crate) fn source_impl(&self, db: &impl HirDatabase) -> TreePtr<ast::FnDef> {
+    pub(crate) fn source_impl(&self, db: &impl HirDatabase) -> (HirFileId, TreePtr<ast::FnDef>) {
         let def_loc = self.def_id.loc(db);
         assert!(def_loc.kind == DefKind::Function);
         let syntax = db.file_item(def_loc.source_item_id);
-        ast::FnDef::cast(&syntax).unwrap().to_owned()
+        (
+            def_loc.source_item_id.file_id,
+            ast::FnDef::cast(&syntax).unwrap().to_owned(),
+        )
     }
 
     pub(crate) fn body(&self, db: &impl HirDatabase) -> Cancelable<Arc<Body>> {
@@ -46,7 +49,11 @@ impl Function {
 impl FnSignature {
     pub(crate) fn fn_signature_query(db: &impl HirDatabase, def_id: DefId) -> Arc<FnSignature> {
         let func = Function::new(def_id);
-        let node = func.source(db);
+        let node = func.source_impl(db).1; // TODO we're using source_impl here to avoid returning Cancelable... this is a bit hacky
+        let name = node
+            .name()
+            .map(|n| n.as_name())
+            .unwrap_or_else(Name::missing);
         let mut args = Vec::new();
         if let Some(param_list) = node.param_list() {
             if let Some(self_param) = param_list.self_param() {
@@ -76,7 +83,11 @@ impl FnSignature {
         } else {
             TypeRef::unit()
         };
-        let sig = FnSignature { args, ret_type };
+        let sig = FnSignature {
+            name,
+            args,
+            ret_type,
+        };
         Arc::new(sig)
     }
 }

crates/ra_hir/src/expr.rs

@@ -762,7 +762,7 @@ pub(crate) fn body_syntax_mapping(
     let def = def_id.resolve(db)?;
 
     let body_syntax_mapping = match def {
-        Def::Function(f) => collect_fn_body_syntax(&f.source(db)),
+        Def::Function(f) => collect_fn_body_syntax(&f.source(db)?.1),
         // TODO: consts, etc.
         _ => panic!("Trying to get body for item type without body"),
     };

crates/ra_hir/src/ids.rs

@@ -34,7 +34,7 @@ pub struct HirFileId(HirFileIdRepr);
 impl HirFileId {
     /// For macro-expansion files, returns the file original source file the
     /// expansionoriginated from.
-    pub(crate) fn original_file(self, db: &impl HirDatabase) -> FileId {
+    pub fn original_file(self, db: &impl HirDatabase) -> FileId {
         match self.0 {
             HirFileIdRepr::File(file_id) => file_id,
             HirFileIdRepr::Macro(macro_call_id) => {
@@ -179,6 +179,12 @@ impl DefId {
         Ok(res)
     }
 
+    pub(crate) fn source(self, db: &impl HirDatabase) -> (HirFileId, TreePtr<SyntaxNode>) {
+        let loc = self.loc(db);
+        let syntax = db.file_item(loc.source_item_id);
+        (loc.source_item_id.file_id, syntax)
+    }
+
     /// For a module, returns that module; for any other def, returns the containing module.
     pub fn module(self, db: &impl HirDatabase) -> Cancelable<Module> {
         let loc = self.loc(db);

crates/ra_ide_api/src/completion/complete_path.rs

@@ -15,11 +15,11 @@ pub(super) fn complete_path(acc: &mut Completions, ctx: &CompletionContext) -> C
     match def_id.resolve(ctx.db)? {
         hir::Def::Module(module) => {
             let module_scope = module.scope(ctx.db)?;
-            module_scope.entries().for_each(|(name, res)| {
+            for (name, res) in module_scope.entries() {
                 CompletionItem::new(CompletionKind::Reference, name.to_string())
                     .from_resolution(ctx, res)
-                    .add_to(acc)
-            });
+                    .add_to(acc);
+            }
         }
         hir::Def::Enum(e) => {
             e.variants(ctx.db)?

crates/ra_ide_api/src/goto_definition.rs

@@ -42,6 +42,24 @@ pub(crate) fn reference_definition(
             return Ok(vec![nav]);
         };
     }
+    // Then try module name resolution
+    if let Some(module) =
+        hir::source_binder::module_from_child_node(db, file_id, name_ref.syntax())?
+    {
+        if let Some(path) = name_ref
+            .syntax()
+            .ancestors()
+            .find_map(ast::Path::cast)
+            .and_then(hir::Path::from_ast)
+        {
+            let resolved = module.resolve_path(db, &path)?;
+            if let Some(def_id) = resolved.take_types().or(resolved.take_values()) {
+                if let Some(target) = NavigationTarget::from_def(db, def_id.resolve(db)?)? {
+                    return Ok(vec![target]);
+                }
+            }
+        }
+    }
     // If that fails try the index based approach.
     let navs = db
         .index_resolve(name_ref)?
@@ -104,6 +122,31 @@ mod tests {
         );
     }
 
+    #[test]
+    fn goto_definition_resolves_correct_name() {
+        let (analysis, pos) = analysis_and_position(
+            "
+            //- /lib.rs
+            use a::Foo;
+            mod a;
+            mod b;
+            enum E { X(Foo<|>) }
+            //- /a.rs
+            struct Foo;
+            //- /b.rs
+            struct Foo;
+            ",
+        );
+
+        let symbols = analysis.goto_definition(pos).unwrap().unwrap();
+        assert_eq_dbg(
+            r#"[NavigationTarget { file_id: FileId(2), name: "Foo",
+                                   kind: STRUCT_DEF, range: [0; 11),
+                                   ptr: Some(LocalSyntaxPtr { range: [0; 11), kind: STRUCT_DEF }) }]"#,
+            &symbols,
+        );
+    }
+
     #[test]
     fn goto_definition_works_for_module_declaration() {
         let (analysis, pos) = analysis_and_position(

crates/ra_ide_api/src/lib.rs

@@ -33,7 +33,8 @@ mod syntax_highlighting;
 
 use std::{fmt, sync::Arc};
 
-use ra_syntax::{SmolStr, SourceFile, TreePtr, SyntaxKind, TextRange, TextUnit};
+use hir::{Def, ModuleSource, Name};
+use ra_syntax::{SmolStr, SourceFile, TreePtr, SyntaxKind, SyntaxNode, TextRange, TextUnit, AstNode};
 use ra_text_edit::TextEdit;
 use ra_db::{SyntaxDatabase, FilesDatabase, LocalSyntaxPtr, BaseDatabase};
 use rayon::prelude::*;
@@ -268,6 +269,67 @@ impl NavigationTarget {
         }
     }
 
+    fn from_syntax(name: Option<Name>, file_id: FileId, node: &SyntaxNode) -> NavigationTarget {
+        NavigationTarget {
+            file_id,
+            name: name.map(|n| n.to_string().into()).unwrap_or("".into()),
+            kind: node.kind(),
+            range: node.range(),
+            ptr: Some(LocalSyntaxPtr::new(node)),
+        }
+    }
+    // TODO once Def::Item is gone, this should be able to always return a NavigationTarget
+    fn from_def(db: &db::RootDatabase, def: Def) -> Cancelable<Option<NavigationTarget>> {
+        Ok(match def {
+            Def::Struct(s) => {
+                let (file_id, node) = s.source(db)?;
+                Some(NavigationTarget::from_syntax(
+                    s.name(db)?,
+                    file_id.original_file(db),
+                    node.syntax(),
+                ))
+            }
+            Def::Enum(e) => {
+                let (file_id, node) = e.source(db)?;
+                Some(NavigationTarget::from_syntax(
+                    e.name(db)?,
+                    file_id.original_file(db),
+                    node.syntax(),
+                ))
+            }
+            Def::EnumVariant(ev) => {
+                let (file_id, node) = ev.source(db)?;
+                Some(NavigationTarget::from_syntax(
+                    ev.name(db)?,
+                    file_id.original_file(db),
+                    node.syntax(),
+                ))
+            }
+            Def::Function(f) => {
+                let (file_id, node) = f.source(db)?;
+                let name = f.signature(db).name().clone();
+                Some(NavigationTarget::from_syntax(
+                    Some(name),
+                    file_id.original_file(db),
+                    node.syntax(),
+                ))
+            }
+            Def::Module(m) => {
+                let (file_id, source) = m.definition_source(db)?;
+                let name = m.name(db)?;
+                match source {
+                    ModuleSource::SourceFile(node) => {
+                        Some(NavigationTarget::from_syntax(name, file_id, node.syntax()))
+                    }
+                    ModuleSource::Module(node) => {
+                        Some(NavigationTarget::from_syntax(name, file_id, node.syntax()))
+                    }
+                }
+            }
+            Def::Item => None,
+        })
+    }
+
     pub fn name(&self) -> &SmolStr {
         &self.name
     }