Merge #496

496: Include two element ranges into the nav. r=matklad a=matklad



Co-authored-by: Aleksey Kladov <[email protected]>

6 files changed, 253 insertions, 199 deletions

diff --git a/crates/ra_ide_api/src/goto_definition.rs b/crates/ra_ide_api/src/goto_definition.rs
index eaddd5083..8d2ff561a 100644
--- a/crates/ra_ide_api/src/goto_definition.rs
+++ b/crates/ra_ide_api/src/goto_definition.rs
@@ -1,22 +1,24 @@
 use ra_db::{FileId, Cancelable, SyntaxDatabase};
 use ra_syntax::{
-    TextRange, AstNode, ast, SyntaxKind::{NAME, MODULE},
+    AstNode, ast,
     algo::find_node_at_offset,
 };
 
-use crate::{FilePosition, NavigationTarget, db::RootDatabase};
+use crate::{FilePosition, NavigationTarget, db::RootDatabase, RangeInfo};
 
 pub(crate) fn goto_definition(
     db: &RootDatabase,
     position: FilePosition,
-) -> Cancelable<Option<Vec<NavigationTarget>>> {
+) -> Cancelable<Option<RangeInfo<Vec<NavigationTarget>>>> {
     let file = db.source_file(position.file_id);
     let syntax = file.syntax();
     if let Some(name_ref) = find_node_at_offset::<ast::NameRef>(syntax, position.offset) {
-        return Ok(Some(reference_definition(db, position.file_id, name_ref)?));
+        let navs = reference_definition(db, position.file_id, name_ref)?;
+        return Ok(Some(RangeInfo::new(name_ref.syntax().range(), navs)));
     }
     if let Some(name) = find_node_at_offset::<ast::Name>(syntax, position.offset) {
-        return name_definition(db, position.file_id, name);
+        let navs = ctry!(name_definition(db, position.file_id, name)?);
+        return Ok(Some(RangeInfo::new(name.syntax().range(), navs)));
     }
     Ok(None)
 }
@@ -32,13 +34,7 @@ pub(crate) fn reference_definition(
         let scope = fn_descr.scopes(db)?;
         // First try to resolve the symbol locally
         if let Some(entry) = scope.resolve_local_name(name_ref) {
-            let nav = NavigationTarget {
-                file_id,
-                name: entry.name().to_string().into(),
-                range: entry.ptr().range(),
-                kind: NAME,
-                ptr: None,
-            };
+            let nav = NavigationTarget::from_scope_entry(file_id, &entry);
             return Ok(vec![nav]);
         };
     }
@@ -79,18 +75,7 @@ fn name_definition(
             if let Some(child_module) =
                 hir::source_binder::module_from_declaration(db, file_id, module)?
             {
-                let (file_id, _) = child_module.definition_source(db)?;
-                let name = match child_module.name(db)? {
-                    Some(name) => name.to_string().into(),
-                    None => "".into(),
-                };
-                let nav = NavigationTarget {
-                    file_id,
-                    name,
-                    range: TextRange::offset_len(0.into(), 0.into()),
-                    kind: MODULE,
-                    ptr: None,
-                };
+                let nav = NavigationTarget::from_module(db, child_module)?;
                 return Ok(Some(vec![nav]));
             }
         }
@@ -100,31 +85,32 @@ fn name_definition(
 
 #[cfg(test)]
 mod tests {
-    use test_utils::assert_eq_dbg;
     use crate::mock_analysis::analysis_and_position;
 
+    fn check_goto(fixuture: &str, expected: &str) {
+        let (analysis, pos) = analysis_and_position(fixuture);
+
+        let mut navs = analysis.goto_definition(pos).unwrap().unwrap().info;
+        assert_eq!(navs.len(), 1);
+        let nav = navs.pop().unwrap();
+        nav.assert_match(expected);
+    }
+
     #[test]
     fn goto_definition_works_in_items() {
-        let (analysis, pos) = analysis_and_position(
+        check_goto(
             "
             //- /lib.rs
             struct Foo;
             enum E { X(Foo<|>) }
             ",
-        );
-
-        let symbols = analysis.goto_definition(pos).unwrap().unwrap();
-        assert_eq_dbg(
-            r#"[NavigationTarget { file_id: FileId(1), name: "Foo",
-                                   kind: STRUCT_DEF, range: [0; 11),
-                                   ptr: Some(LocalSyntaxPtr { range: [0; 11), kind: STRUCT_DEF }) }]"#,
-            &symbols,
+            "Foo STRUCT_DEF FileId(1) [0; 11) [7; 10)",
         );
     }
 
     #[test]
     fn goto_definition_resolves_correct_name() {
-        let (analysis, pos) = analysis_and_position(
+        check_goto(
             "
             //- /lib.rs
             use a::Foo;
@@ -136,47 +122,30 @@ mod tests {
             //- /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,
+            "Foo STRUCT_DEF FileId(2) [0; 11) [7; 10)",
         );
     }
 
     #[test]
     fn goto_definition_works_for_module_declaration() {
-        let (analysis, pos) = analysis_and_position(
+        check_goto(
             "
             //- /lib.rs
             mod <|>foo;
             //- /foo.rs
             // empty
-        ",
-        );
-
-        let symbols = analysis.goto_definition(pos).unwrap().unwrap();
-        assert_eq_dbg(
-            r#"[NavigationTarget { file_id: FileId(2), name: "foo", kind: MODULE, range: [0; 0), ptr: None }]"#,
-            &symbols,
+            ",
+            "foo SOURCE_FILE FileId(2) [0; 10)",
         );
 
-        let (analysis, pos) = analysis_and_position(
+        check_goto(
             "
             //- /lib.rs
             mod <|>foo;
             //- /foo/mod.rs
             // empty
-        ",
-        );
-
-        let symbols = analysis.goto_definition(pos).unwrap().unwrap();
-        assert_eq_dbg(
-            r#"[NavigationTarget { file_id: FileId(2), name: "foo", kind: MODULE, range: [0; 0), ptr: None }]"#,
-            &symbols,
+            ",
+            "foo SOURCE_FILE FileId(2) [0; 10)",
         );
     }
 }
diff --git a/crates/ra_ide_api/src/hover.rs b/crates/ra_ide_api/src/hover.rs
index 41309e756..f544ffa6d 100644
--- a/crates/ra_ide_api/src/hover.rs
+++ b/crates/ra_ide_api/src/hover.rs
@@ -88,11 +88,11 @@ fn doc_text_for(db: &RootDatabase, nav: NavigationTarget) -> Cancelable<Option<S
 
 impl NavigationTarget {
     fn node(&self, db: &RootDatabase) -> Option<TreePtr<SyntaxNode>> {
-        let source_file = db.source_file(self.file_id);
+        let source_file = db.source_file(self.file_id());
         let source_file = source_file.syntax();
         let node = source_file
             .descendants()
-            .find(|node| node.kind() == self.kind && node.range() == self.range)?
+            .find(|node| node.kind() == self.kind() && node.range() == self.full_range())?
             .to_owned();
         Some(node)
     }
diff --git a/crates/ra_ide_api/src/imp.rs b/crates/ra_ide_api/src/imp.rs
index 7c60ab7d6..ba4aa0fd5 100644
--- a/crates/ra_ide_api/src/imp.rs
+++ b/crates/ra_ide_api/src/imp.rs
@@ -11,12 +11,11 @@ use ra_syntax::{
     TextRange, AstNode, SourceFile,
     ast::{self, NameOwner},
     algo::find_node_at_offset,
-    SyntaxKind::*,
 };
 
 use crate::{
     AnalysisChange,
-    Cancelable, NavigationTarget,
+    Cancelable,
     CrateId, db, Diagnostic, FileId, FilePosition, FileRange, FileSystemEdit,
     Query, RootChange, SourceChange, SourceFileEdit,
     symbol_index::{LibrarySymbolsQuery, FileSymbol},
@@ -99,29 +98,6 @@ impl db::RootDatabase {
 }
 
 impl db::RootDatabase {
-    /// This returns `Vec` because a module may be included from several places. We
-    /// don't handle this case yet though, so the Vec has length at most one.
-    pub(crate) fn parent_module(
-        &self,
-        position: FilePosition,
-    ) -> Cancelable<Vec<NavigationTarget>> {
-        let module = match source_binder::module_from_position(self, position)? {
-            None => return Ok(Vec::new()),
-            Some(it) => it,
-        };
-        let (file_id, ast_module) = match module.declaration_source(self)? {
-            None => return Ok(Vec::new()),
-            Some(it) => it,
-        };
-        let name = ast_module.name().unwrap();
-        Ok(vec![NavigationTarget {
-            file_id,
-            name: name.text().clone(),
-            range: name.syntax().range(),
-            kind: MODULE,
-            ptr: None,
-        }])
-    }
     /// Returns `Vec` for the same reason as `parent_module`
     pub(crate) fn crate_for(&self, file_id: FileId) -> Cancelable<Vec<CrateId>> {
         let module = match source_binder::module_from_file_id(self, file_id)? {
diff --git a/crates/ra_ide_api/src/lib.rs b/crates/ra_ide_api/src/lib.rs
index 65d21d899..6155d903a 100644
--- a/crates/ra_ide_api/src/lib.rs
+++ b/crates/ra_ide_api/src/lib.rs
@@ -18,25 +18,26 @@ macro_rules! ctry {
     };
 }
 
-mod completion;
 mod db;
-mod goto_definition;
 mod imp;
 pub mod mock_analysis;
-mod runnables;
 mod symbol_index;
+mod navigation_target;
 
+mod completion;
+mod runnables;
+mod goto_definition;
 mod extend_selection;
 mod hover;
 mod call_info;
 mod syntax_highlighting;
+mod parent_module;
 
 use std::{fmt, sync::Arc};
 
-use hir::{Def, ModuleSource, Name};
-use ra_syntax::{SmolStr, SourceFile, TreePtr, SyntaxKind, SyntaxNode, TextRange, TextUnit, AstNode};
+use ra_syntax::{SourceFile, TreePtr, TextRange, TextUnit};
 use ra_text_edit::TextEdit;
-use ra_db::{SyntaxDatabase, FilesDatabase, LocalSyntaxPtr, BaseDatabase};
+use ra_db::{SyntaxDatabase, FilesDatabase, BaseDatabase};
 use rayon::prelude::*;
 use relative_path::RelativePathBuf;
 use rustc_hash::FxHashMap;
@@ -50,6 +51,7 @@ use crate::{
 pub use crate::{
     completion::{CompletionItem, CompletionItemKind, InsertText},
     runnables::{Runnable, RunnableKind},
+    navigation_target::NavigationTarget,
 };
 pub use ra_ide_api_light::{
     Fold, FoldKind, HighlightedRange, Severity, StructureNode,
@@ -243,110 +245,6 @@ impl Query {
     }
 }
 
-/// `NavigationTarget` represents and element in the editor's UI whihc you can
-/// click on to navigate to a particular piece of code.
-///
-/// Typically, a `NavigationTarget` corresponds to some element in the source
-/// code, like a function or a struct, but this is not strictly required.
-#[derive(Debug, Clone)]
-pub struct NavigationTarget {
-    file_id: FileId,
-    name: SmolStr,
-    kind: SyntaxKind,
-    range: TextRange,
-    // Should be DefId ideally
-    ptr: Option<LocalSyntaxPtr>,
-}
-
-impl NavigationTarget {
-    fn from_symbol(symbol: FileSymbol) -> NavigationTarget {
-        NavigationTarget {
-            file_id: symbol.file_id,
-            name: symbol.name.clone(),
-            kind: symbol.ptr.kind(),
-            range: symbol.ptr.range(),
-            ptr: Some(symbol.ptr.clone()),
-        }
-    }
-
-    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
-    }
-
-    pub fn kind(&self) -> SyntaxKind {
-        self.kind
-    }
-
-    pub fn file_id(&self) -> FileId {
-        self.file_id
-    }
-
-    pub fn range(&self) -> TextRange {
-        self.range
-    }
-}
-
 #[derive(Debug)]
 pub struct RangeInfo<T> {
     pub range: TextRange,
@@ -354,7 +252,7 @@ pub struct RangeInfo<T> {
 }
 
 impl<T> RangeInfo<T> {
-    fn new(range: TextRange, info: T) -> RangeInfo<T> {
+    pub fn new(range: TextRange, info: T) -> RangeInfo<T> {
         RangeInfo { range, info }
     }
 }
@@ -494,7 +392,7 @@ impl Analysis {
     pub fn goto_definition(
         &self,
         position: FilePosition,
-    ) -> Cancelable<Option<Vec<NavigationTarget>>> {
+    ) -> Cancelable<Option<RangeInfo<Vec<NavigationTarget>>>> {
         self.db
             .catch_canceled(|db| goto_definition::goto_definition(db, position))?
     }
@@ -517,7 +415,7 @@ impl Analysis {
 
     /// Returns a `mod name;` declaration which created the current module.
     pub fn parent_module(&self, position: FilePosition) -> Cancelable<Vec<NavigationTarget>> {
-        self.with_db(|db| db.parent_module(position))?
+        self.with_db(|db| parent_module::parent_module(db, position))?
     }
 
     /// Returns crates this file belongs too.
diff --git a/crates/ra_ide_api/src/navigation_target.rs b/crates/ra_ide_api/src/navigation_target.rs
new file mode 100644
index 000000000..b955bbe42
--- /dev/null
+++ b/crates/ra_ide_api/src/navigation_target.rs
@@ -0,0 +1,159 @@
+use ra_db::{FileId, Cancelable};
+use ra_syntax::{
+    SyntaxNode, AstNode, SmolStr, TextRange, ast,
+    SyntaxKind::{self, NAME},
+};
+use hir::{Def, ModuleSource};
+
+use crate::{FileSymbol, db::RootDatabase};
+
+/// `NavigationTarget` represents and element in the editor's UI which you can
+/// click on to navigate to a particular piece of code.
+///
+/// Typically, a `NavigationTarget` corresponds to some element in the source
+/// code, like a function or a struct, but this is not strictly required.
+#[derive(Debug, Clone)]
+pub struct NavigationTarget {
+    file_id: FileId,
+    name: SmolStr,
+    kind: SyntaxKind,
+    full_range: TextRange,
+    focus_range: Option<TextRange>,
+}
+
+impl NavigationTarget {
+    pub fn name(&self) -> &SmolStr {
+        &self.name
+    }
+
+    pub fn kind(&self) -> SyntaxKind {
+        self.kind
+    }
+
+    pub fn file_id(&self) -> FileId {
+        self.file_id
+    }
+
+    pub fn full_range(&self) -> TextRange {
+        self.full_range
+    }
+
+    /// A "most interesting" range withing the `range_full`.
+    ///
+    /// Typically, `range` is the whole syntax node, including doc comments, and
+    /// `focus_range` is the range of the identifier.
+    pub fn focus_range(&self) -> Option<TextRange> {
+        self.focus_range
+    }
+
+    pub(crate) fn from_symbol(symbol: FileSymbol) -> NavigationTarget {
+        NavigationTarget {
+            file_id: symbol.file_id,
+            name: symbol.name.clone(),
+            kind: symbol.ptr.kind(),
+            full_range: symbol.ptr.range(),
+            focus_range: None,
+        }
+    }
+
+    pub(crate) fn from_scope_entry(
+        file_id: FileId,
+        entry: &hir::ScopeEntryWithSyntax,
+    ) -> NavigationTarget {
+        NavigationTarget {
+            file_id,
+            name: entry.name().to_string().into(),
+            full_range: entry.ptr().range(),
+            focus_range: None,
+            kind: NAME,
+        }
+    }
+
+    pub(crate) fn from_module(
+        db: &RootDatabase,
+        module: hir::Module,
+    ) -> Cancelable<NavigationTarget> {
+        let (file_id, source) = module.definition_source(db)?;
+        let name = module
+            .name(db)?
+            .map(|it| it.to_string().into())
+            .unwrap_or_default();
+        let res = match source {
+            ModuleSource::SourceFile(node) => {
+                NavigationTarget::from_syntax(file_id, name, None, node.syntax())
+            }
+            ModuleSource::Module(node) => {
+                NavigationTarget::from_syntax(file_id, name, None, node.syntax())
+            }
+        };
+        Ok(res)
+    }
+
+    // TODO once Def::Item is gone, this should be able to always return a NavigationTarget
+    pub(crate) fn from_def(db: &RootDatabase, def: Def) -> Cancelable<Option<NavigationTarget>> {
+        let res = match def {
+            Def::Struct(s) => {
+                let (file_id, node) = s.source(db)?;
+                NavigationTarget::from_named(file_id.original_file(db), &*node)
+            }
+            Def::Enum(e) => {
+                let (file_id, node) = e.source(db)?;
+                NavigationTarget::from_named(file_id.original_file(db), &*node)
+            }
+            Def::EnumVariant(ev) => {
+                let (file_id, node) = ev.source(db)?;
+                NavigationTarget::from_named(file_id.original_file(db), &*node)
+            }
+            Def::Function(f) => {
+                let (file_id, node) = f.source(db)?;
+                NavigationTarget::from_named(file_id.original_file(db), &*node)
+            }
+            Def::Module(m) => NavigationTarget::from_module(db, m)?,
+            Def::Item => return Ok(None),
+        };
+        Ok(Some(res))
+    }
+
+    #[cfg(test)]
+    pub(crate) fn assert_match(&self, expected: &str) {
+        let actual = self.debug_render();
+        test_utils::assert_eq_text!(expected.trim(), actual.trim(),);
+    }
+
+    #[cfg(test)]
+    pub(crate) fn debug_render(&self) -> String {
+        let mut buf = format!(
+            "{} {:?} {:?} {:?}",
+            self.name(),
+            self.kind(),
+            self.file_id(),
+            self.full_range()
+        );
+        if let Some(focus_range) = self.focus_range() {
+            buf.push_str(&format!(" {:?}", focus_range))
+        }
+        buf
+    }
+
+    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())
+    }
+
+    fn from_syntax(
+        file_id: FileId,
+        name: SmolStr,
+        focus_range: Option<TextRange>,
+        node: &SyntaxNode,
+    ) -> NavigationTarget {
+        NavigationTarget {
+            file_id,
+            name,
+            kind: node.kind(),
+            full_range: node.range(),
+            focus_range,
+            // ptr: Some(LocalSyntaxPtr::new(node)),
+        }
+    }
+}
diff --git a/crates/ra_ide_api/src/parent_module.rs b/crates/ra_ide_api/src/parent_module.rs
new file mode 100644
index 000000000..d345839a3
--- /dev/null
+++ b/crates/ra_ide_api/src/parent_module.rs
@@ -0,0 +1,52 @@
+use ra_db::{Cancelable, FilePosition};
+
+use crate::{NavigationTarget, db::RootDatabase};
+
+/// This returns `Vec` because a module may be included from several places. We
+/// don't handle this case yet though, so the Vec has length at most one.
+pub(crate) fn parent_module(
+    db: &RootDatabase,
+    position: FilePosition,
+) -> Cancelable<Vec<NavigationTarget>> {
+    let module = match hir::source_binder::module_from_position(db, position)? {
+        None => return Ok(Vec::new()),
+        Some(it) => it,
+    };
+    let nav = NavigationTarget::from_module(db, module)?;
+    Ok(vec![nav])
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::mock_analysis::analysis_and_position;
+
+    #[test]
+    fn test_resolve_parent_module() {
+        let (analysis, pos) = analysis_and_position(
+            "
+            //- /lib.rs
+            mod foo;
+            //- /foo.rs
+            <|>// empty
+            ",
+        );
+        let nav = analysis.parent_module(pos).unwrap().pop().unwrap();
+        nav.assert_match("foo SOURCE_FILE FileId(2) [0; 10)");
+    }
+
+    #[test]
+    fn test_resolve_parent_module_for_inline() {
+        let (analysis, pos) = analysis_and_position(
+            "
+            //- /lib.rs
+            mod foo {
+                mod bar {
+                    mod baz { <|> }
+                }
+            }
+            ",
+        );
+        let nav = analysis.parent_module(pos).unwrap().pop().unwrap();
+        nav.assert_match("baz MODULE FileId(1) [32; 44)");
+    }
+}