Merge #879

879: Fixes to goto definition r=vipentti a=vipentti

Previously goto definition would fail when the cursor was over the name of the definition. Now we should properly resolve to a `NavigationTarget` when on top of the name of a definition.

In addition this adds `name_range` field to `FileSymbol`, this further fixes goto_definition and symbol based navigation by allowing the `NavigationTarget` to actually have a `focus_range`, meaning instead of focusing on the start of the `full_range`, we can have the cursor focus on the name.

e.g. goto definition
```rust
fn bar() {
    fn foo() { }
  
   foo<|>();
}
```

Previously this would put the cursor at the start of the FN_DEF:
```rust
fn bar() {
   <|>fn foo() { }
  
   foo();
}
```
Now when using the symbol based resolving, we'll have a proper focus range and instead put the cursor at the start of the name.

```rust
fn bar() {
   fn <|>foo() { }
  
   foo();
}
```

This fixes #877 but doesn't contain the refactoring of the return type for `goto_definition`

Co-authored-by: Ville Penttinen <[email protected]>

3 files changed, 139 insertions, 11 deletions

diff --git a/crates/ra_ide_api/src/goto_definition.rs b/crates/ra_ide_api/src/goto_definition.rs
index 96ed8c8e9..4dac96bfe 100644
--- a/crates/ra_ide_api/src/goto_definition.rs
+++ b/crates/ra_ide_api/src/goto_definition.rs
@@ -1,7 +1,8 @@
 use ra_db::{FileId, SourceDatabase};
 use ra_syntax::{
     AstNode, ast,
-    algo::find_node_at_offset,
+    algo::{find_node_at_offset, visit::{visitor, Visitor}},
+    SyntaxNode,
 };
 use test_utils::tested_by;
 use hir::Resolution;
@@ -114,7 +115,9 @@ fn name_definition(
     file_id: FileId,
     name: &ast::Name,
 ) -> Option<Vec<NavigationTarget>> {
-    if let Some(module) = name.syntax().parent().and_then(ast::Module::cast) {
+    let parent = name.syntax().parent()?;
+
+    if let Some(module) = ast::Module::cast(&parent) {
         if module.has_semi() {
             if let Some(child_module) =
                 hir::source_binder::module_from_declaration(db, file_id, module)
@@ -124,9 +127,29 @@ fn name_definition(
             }
         }
     }
+
+    if let Some(nav) = named_target(file_id, &parent) {
+        return Some(vec![nav]);
+    }
+
     None
 }
 
+fn named_target(file_id: FileId, node: &SyntaxNode) -> Option<NavigationTarget> {
+    visitor()
+        .visit(|node: &ast::StructDef| NavigationTarget::from_named(file_id, node))
+        .visit(|node: &ast::EnumDef| NavigationTarget::from_named(file_id, node))
+        .visit(|node: &ast::EnumVariant| NavigationTarget::from_named(file_id, node))
+        .visit(|node: &ast::FnDef| NavigationTarget::from_named(file_id, node))
+        .visit(|node: &ast::TypeDef| NavigationTarget::from_named(file_id, node))
+        .visit(|node: &ast::ConstDef| NavigationTarget::from_named(file_id, node))
+        .visit(|node: &ast::StaticDef| NavigationTarget::from_named(file_id, node))
+        .visit(|node: &ast::TraitDef| NavigationTarget::from_named(file_id, node))
+        .visit(|node: &ast::NamedFieldDef| NavigationTarget::from_named(file_id, node))
+        .visit(|node: &ast::Module| NavigationTarget::from_named(file_id, node))
+        .accept(node)
+}
+
 #[cfg(test)]
 mod tests {
     use test_utils::covers;
@@ -231,4 +254,98 @@ mod tests {
             "spam NAMED_FIELD_DEF FileId(1) [17; 26) [17; 21)",
         );
     }
+
+    #[test]
+    fn goto_definition_works_when_used_on_definition_name_itself() {
+        check_goto(
+            "
+            //- /lib.rs
+            struct Foo<|> { value: u32 }
+            ",
+            "Foo STRUCT_DEF FileId(1) [0; 25) [7; 10)",
+        );
+
+        check_goto(
+            r#"
+            //- /lib.rs
+            struct Foo {
+                field<|>: string,
+            }
+            "#,
+            "field NAMED_FIELD_DEF FileId(1) [17; 30) [17; 22)",
+        );
+
+        check_goto(
+            "
+            //- /lib.rs
+            fn foo_test<|>() {
+            }
+            ",
+            "foo_test FN_DEF FileId(1) [0; 17) [3; 11)",
+        );
+
+        check_goto(
+            "
+            //- /lib.rs
+            enum Foo<|> {
+                Variant,
+            }
+            ",
+            "Foo ENUM_DEF FileId(1) [0; 25) [5; 8)",
+        );
+
+        check_goto(
+            "
+            //- /lib.rs
+            enum Foo {
+                Variant1,
+                Variant2<|>,
+                Variant3,
+            }
+            ",
+            "Variant2 ENUM_VARIANT FileId(1) [29; 37) [29; 37)",
+        );
+
+        check_goto(
+            r#"
+            //- /lib.rs
+            static inner<|>: &str = "";
+            "#,
+            "inner STATIC_DEF FileId(1) [0; 24) [7; 12)",
+        );
+
+        check_goto(
+            r#"
+            //- /lib.rs
+            const inner<|>: &str = "";
+            "#,
+            "inner CONST_DEF FileId(1) [0; 23) [6; 11)",
+        );
+
+        check_goto(
+            r#"
+            //- /lib.rs
+            type Thing<|> = Option<()>;
+            "#,
+            "Thing TYPE_DEF FileId(1) [0; 24) [5; 10)",
+        );
+
+        check_goto(
+            r#"
+            //- /lib.rs
+            trait Foo<|> {
+            }
+            "#,
+            "Foo TRAIT_DEF FileId(1) [0; 13) [6; 9)",
+        );
+
+        check_goto(
+            r#"
+            //- /lib.rs
+            mod bar<|> {
+            }
+            "#,
+            "bar MODULE FileId(1) [0; 11) [4; 7)",
+        );
+    }
 }
diff --git a/crates/ra_ide_api/src/navigation_target.rs b/crates/ra_ide_api/src/navigation_target.rs
index fd001179a..ce5ae0363 100644
--- a/crates/ra_ide_api/src/navigation_target.rs
+++ b/crates/ra_ide_api/src/navigation_target.rs
@@ -67,7 +67,7 @@ impl NavigationTarget {
             name: symbol.name.clone(),
             kind: symbol.ptr.kind(),
             full_range: symbol.ptr.range(),
-            focus_range: None,
+            focus_range: symbol.name_range,
             container_name: symbol.container_name.clone(),
         }
     }
@@ -193,12 +193,13 @@ impl NavigationTarget {
             buf.push_str(&format!(" {:?}", focus_range))
         }
         if let Some(container_name) = self.container_name() {
-            buf.push_str(&format!(" {:?}", container_name))
+            buf.push_str(&format!(" {}", container_name))
         }
         buf
     }
 
-    fn from_named(file_id: FileId, node: &impl ast::NameOwner) -> NavigationTarget {
+    /// Allows `NavigationTarget` to be created from a `NameOwner`
+    pub(crate) fn from_named(file_id: FileId, node: &impl ast::NameOwner) -> NavigationTarget {
         let name = node.name().map(|it| it.text().clone()).unwrap_or_default();
         let focus_range = node.name().map(|it| it.syntax().range());
         NavigationTarget::from_syntax(file_id, name, focus_range, node.syntax())
diff --git a/crates/ra_ide_api/src/symbol_index.rs b/crates/ra_ide_api/src/symbol_index.rs
index afb10fa92..93bdf05d8 100644
--- a/crates/ra_ide_api/src/symbol_index.rs
+++ b/crates/ra_ide_api/src/symbol_index.rs
@@ -33,6 +33,7 @@ use ra_syntax::{
     SyntaxKind::{self, *},
     ast::{self, NameOwner},
     WalkEvent,
+    TextRange,
 };
 use ra_db::{
     SourceRootId, SourceDatabase,
@@ -70,7 +71,7 @@ fn file_symbols(db: &impl SymbolsDatabase, file_id: FileId) -> Arc<SymbolIndex>
         let node = find_covering_node(source_file.syntax(), text_range);
         let ptr = SyntaxNodePtr::new(node);
         // TODO: Should we get container name for macro symbols?
-        symbols.push(FileSymbol { file_id, name, ptr, container_name: None })
+        symbols.push(FileSymbol { file_id, name, ptr, name_range: None, container_name: None })
     }
 
     Arc::new(SymbolIndex::new(symbols))
@@ -207,6 +208,7 @@ pub(crate) struct FileSymbol {
     pub(crate) file_id: FileId,
     pub(crate) name: SmolStr,
     pub(crate) ptr: SyntaxNodePtr,
+    pub(crate) name_range: Option<TextRange>,
     pub(crate) container_name: Option<SmolStr>,
 }
 
@@ -236,12 +238,14 @@ fn source_file_to_file_symbols(source_file: &SourceFile, file_id: FileId) -> Vec
     symbols
 }
 
-fn to_symbol(node: &SyntaxNode) -> Option<(SmolStr, SyntaxNodePtr)> {
-    fn decl<N: NameOwner>(node: &N) -> Option<(SmolStr, SyntaxNodePtr)> {
-        let name = node.name()?.text().clone();
+fn to_symbol(node: &SyntaxNode) -> Option<(SmolStr, SyntaxNodePtr, TextRange)> {
+    fn decl<N: NameOwner>(node: &N) -> Option<(SmolStr, SyntaxNodePtr, TextRange)> {
+        let name = node.name()?;
+        let name_range = name.syntax().range();
+        let name = name.text().clone();
         let ptr = SyntaxNodePtr::new(node.syntax());
 
-        Some((name, ptr))
+        Some((name, ptr, name_range))
     }
     visitor()
         .visit(decl::<ast::FnDef>)
@@ -256,5 +260,11 @@ fn to_symbol(node: &SyntaxNode) -> Option<(SmolStr, SyntaxNodePtr)> {
 }
 
 fn to_file_symbol(node: &SyntaxNode, file_id: FileId) -> Option<FileSymbol> {
-    to_symbol(node).map(move |(name, ptr)| FileSymbol { name, ptr, file_id, container_name: None })
+    to_symbol(node).map(move |(name, ptr, name_range)| FileSymbol {
+        name,
+        ptr,
+        file_id,
+        name_range: Some(name_range),
+        container_name: None,
+    })
 }