Merge #437

437: refactor goto defenition r=matklad a=matklad



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

6 files changed, 138 insertions, 212 deletions

diff --git a/crates/ra_analysis/src/goto_defenition.rs b/crates/ra_analysis/src/goto_defenition.rs
index 607a25115..e37421f8d 100644
--- a/crates/ra_analysis/src/goto_defenition.rs
+++ b/crates/ra_analysis/src/goto_defenition.rs
@@ -1,73 +1,138 @@
-use ra_db::FileId;
-use ra_syntax::ast;
+use ra_db::{FileId, Cancelable, SyntaxDatabase};
+use ra_syntax::{TextRange, AstNode, ast, SyntaxKind::{NAME, MODULE}};
 
-use crate::db::RootDatabase;
+use ra_editor::find_node_at_offset;
 
-pub fn goto_defenition(db: &RootDatabase, position: FilePosition,
+use crate::{FilePosition, NavigationTarget, db::RootDatabase};
+
+pub(crate) fn goto_defenition(
+    db: &RootDatabase,
+    position: FilePosition,
 ) -> Cancelable<Option<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_defenition(db, position.file_id, name_ref)));
+        return Ok(Some(reference_defenition(db, position.file_id, name_ref)?));
     }
-    if let Some(name) = find_node_at_offset::<ast::Name>(syntax, position.offset)  {
-        return Ok(Some(name_defenition(db, position.file_idname)));
+    if let Some(name) = find_node_at_offset::<ast::Name>(syntax, position.offset) {
+        return name_defenition(db, position.file_id, name);
     }
     Ok(None)
 }
 
-fn reference_defenition(db: &RootDatabase, file_id: FileId, name_ref: ast::NameRef) -> Cancelable<Vec<Nav>> {
-    if let Some(name_ref) = find_node_at_offset::<ast::NameRef>(syntax, position.offset) {
-        let mut rr = ReferenceResolution::new(name_ref.syntax().range());
-        if let Some(fn_descr) =
-            source_binder::function_from_child_node(self, position.file_id, name_ref.syntax())?
-        {
-            let scope = fn_descr.scopes(self);
-            // First try to resolve the symbol locally
-            if let Some(entry) = scope.resolve_local_name(name_ref) {
-                rr.resolves_to.push(NavigationTarget {
-                    file_id: position.file_id,
-                    name: entry.name().to_string().into(),
-                    range: entry.ptr().range(),
-                    kind: NAME,
-                    ptr: None,
-                });
-                return Ok(Some(rr));
+pub(crate) fn reference_defenition(
+    db: &RootDatabase,
+    file_id: FileId,
+    name_ref: ast::NameRef,
+) -> Cancelable<Vec<NavigationTarget>> {
+    if let Some(fn_descr) =
+        hir::source_binder::function_from_child_node(db, file_id, name_ref.syntax())?
+    {
+        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,
             };
-        }
-        // If that fails try the index based approach.
-        rr.resolves_to.extend(
-            self.index_resolve(name_ref)?
-                .into_iter()
-                .map(NavigationTarget::from_symbol),
-        );
-        return Ok(Some(rr));
+            return Ok(vec![nav]);
+        };
     }
-        if let Some(name) = find_node_at_offset::<ast::Name>(syntax, position.offset) {
-            let mut rr = ReferenceResolution::new(name.syntax().range());
-            if let Some(module) = name.syntax().parent().and_then(ast::Module::cast) {
-                if module.has_semi() {
-                    if let Some(child_module) =
-                        source_binder::module_from_declaration(self, position.file_id, module)?
-                    {
-                        let file_id = child_module.file_id();
-                        let name = match child_module.name() {
-                            Some(name) => name.to_string().into(),
-                            None => "".into(),
-                        };
-                        let symbol = NavigationTarget {
-                            file_id,
-                            name,
-                            range: TextRange::offset_len(0.into(), 0.into()),
-                            kind: MODULE,
-                            ptr: None,
-                        };
-                        rr.resolves_to.push(symbol);
-                        return Ok(Some(rr));
-                    }
-                }
+    // If that fails try the index based approach.
+    let navs = db
+        .index_resolve(name_ref)?
+        .into_iter()
+        .map(NavigationTarget::from_symbol)
+        .collect();
+    Ok(navs)
+}
+
+fn name_defenition(
+    db: &RootDatabase,
+    file_id: FileId,
+    name: ast::Name,
+) -> Cancelable<Option<Vec<NavigationTarget>>> {
+    if let Some(module) = name.syntax().parent().and_then(ast::Module::cast) {
+        if module.has_semi() {
+            if let Some(child_module) =
+                hir::source_binder::module_from_declaration(db, file_id, module)?
+            {
+                let file_id = child_module.file_id();
+                let name = match child_module.name() {
+                    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,
+                };
+                return Ok(Some(vec![nav]));
             }
         }
-        Ok(None)
+    }
+    Ok(None)
+}
+
+#[cfg(test)]
+mod tests {
+    use test_utils::assert_eq_dbg;
+    use crate::mock_analysis::analysis_and_position;
 
+    #[test]
+    fn goto_defenition_works_in_items() {
+        let (analysis, pos) = analysis_and_position(
+            "
+            //- /lib.rs
+            struct Foo;
+            enum E { X(Foo<|>) }
+            ",
+        );
+
+        let symbols = analysis.goto_defenition(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,
+        );
+    }
+
+    #[test]
+    fn goto_defenition_works_for_module_declaration() {
+        let (analysis, pos) = analysis_and_position(
+            "
+            //- /lib.rs
+            mod <|>foo;
+            //- /foo.rs
+            // empty
+        ",
+        );
+
+        let symbols = analysis.goto_defenition(pos).unwrap().unwrap();
+        assert_eq_dbg(
+            r#"[NavigationTarget { file_id: FileId(2), name: "foo", kind: MODULE, range: [0; 0), ptr: None }]"#,
+            &symbols,
+        );
+
+        let (analysis, pos) = analysis_and_position(
+            "
+            //- /lib.rs
+            mod <|>foo;
+            //- /foo/mod.rs
+            // empty
+        ",
+        );
+
+        let symbols = analysis.goto_defenition(pos).unwrap().unwrap();
+        assert_eq_dbg(
+            r#"[NavigationTarget { file_id: FileId(2), name: "foo", kind: MODULE, range: [0; 0), ptr: None }]"#,
+            &symbols,
+        );
+    }
 }
diff --git a/crates/ra_analysis/src/hover.rs b/crates/ra_analysis/src/hover.rs
index 766fa0547..758de376e 100644
--- a/crates/ra_analysis/src/hover.rs
+++ b/crates/ra_analysis/src/hover.rs
@@ -1,4 +1,5 @@
 use ra_db::{Cancelable, SyntaxDatabase};
+use ra_editor::find_node_at_offset;
 use ra_syntax::{
     AstNode, SyntaxNode,
     ast::{self, NameOwner},
@@ -11,18 +12,18 @@ pub(crate) fn hover(
     db: &RootDatabase,
     position: FilePosition,
 ) -> Cancelable<Option<RangeInfo<String>>> {
+    let file = db.source_file(position.file_id);
     let mut res = Vec::new();
-    let range = if let Some(rr) = db.approximately_resolve_symbol(position)? {
-        for nav in rr.resolves_to {
+    let range = if let Some(name_ref) =
+        find_node_at_offset::<ast::NameRef>(file.syntax(), position.offset)
+    {
+        let navs = crate::goto_defenition::reference_defenition(db, position.file_id, name_ref)?;
+        for nav in navs {
             res.extend(doc_text_for(db, nav)?)
         }
-        rr.reference_range
+        name_ref.syntax().range()
     } else {
-        let file = db.source_file(position.file_id);
-        let expr: ast::Expr = ctry!(ra_editor::find_node_at_offset(
-            file.syntax(),
-            position.offset
-        ));
+        let expr: ast::Expr = ctry!(find_node_at_offset(file.syntax(), position.offset));
         let frange = FileRange {
             file_id: position.file_id,
             range: expr.syntax().range(),
diff --git a/crates/ra_analysis/src/imp.rs b/crates/ra_analysis/src/imp.rs
index e2871451c..6ab3c5476 100644
--- a/crates/ra_analysis/src/imp.rs
+++ b/crates/ra_analysis/src/imp.rs
@@ -18,7 +18,7 @@ use crate::{
     AnalysisChange,
     Cancelable, NavigationTarget,
     CrateId, db, Diagnostic, FileId, FilePosition, FileRange, FileSystemEdit,
-    Query, ReferenceResolution, RootChange, SourceChange, SourceFileEdit,
+    Query, RootChange, SourceChange, SourceFileEdit,
     symbol_index::{LibrarySymbolsQuery, FileSymbol},
 };
 
@@ -139,66 +139,6 @@ impl db::RootDatabase {
     pub(crate) fn crate_root(&self, crate_id: CrateId) -> FileId {
         self.crate_graph().crate_root(crate_id)
     }
-    pub(crate) fn approximately_resolve_symbol(
-        &self,
-        position: FilePosition,
-    ) -> Cancelable<Option<ReferenceResolution>> {
-        let file = self.source_file(position.file_id);
-        let syntax = file.syntax();
-        if let Some(name_ref) = find_node_at_offset::<ast::NameRef>(syntax, position.offset) {
-            let mut rr = ReferenceResolution::new(name_ref.syntax().range());
-            if let Some(fn_descr) =
-                source_binder::function_from_child_node(self, position.file_id, name_ref.syntax())?
-            {
-                let scope = fn_descr.scopes(self);
-                // First try to resolve the symbol locally
-                if let Some(entry) = scope.resolve_local_name(name_ref) {
-                    rr.resolves_to.push(NavigationTarget {
-                        file_id: position.file_id,
-                        name: entry.name().to_string().into(),
-                        range: entry.ptr().range(),
-                        kind: NAME,
-                        ptr: None,
-                    });
-                    return Ok(Some(rr));
-                };
-            }
-            // If that fails try the index based approach.
-            rr.resolves_to.extend(
-                self.index_resolve(name_ref)?
-                    .into_iter()
-                    .map(NavigationTarget::from_symbol),
-            );
-            return Ok(Some(rr));
-        }
-        if let Some(name) = find_node_at_offset::<ast::Name>(syntax, position.offset) {
-            let mut rr = ReferenceResolution::new(name.syntax().range());
-            if let Some(module) = name.syntax().parent().and_then(ast::Module::cast) {
-                if module.has_semi() {
-                    if let Some(child_module) =
-                        source_binder::module_from_declaration(self, position.file_id, module)?
-                    {
-                        let file_id = child_module.file_id();
-                        let name = match child_module.name() {
-                            Some(name) => name.to_string().into(),
-                            None => "".into(),
-                        };
-                        let symbol = NavigationTarget {
-                            file_id,
-                            name,
-                            range: TextRange::offset_len(0.into(), 0.into()),
-                            kind: MODULE,
-                            ptr: None,
-                        };
-                        rr.resolves_to.push(symbol);
-                        return Ok(Some(rr));
-                    }
-                }
-            }
-        }
-        Ok(None)
-    }
-
     pub(crate) fn find_all_refs(
         &self,
         position: FilePosition,
@@ -416,7 +356,7 @@ impl db::RootDatabase {
             .collect::<Vec<_>>();
         Ok(res)
     }
-    fn index_resolve(&self, name_ref: ast::NameRef) -> Cancelable<Vec<FileSymbol>> {
+    pub(crate) fn index_resolve(&self, name_ref: ast::NameRef) -> Cancelable<Vec<FileSymbol>> {
         let name = name_ref.text();
         let mut query = Query::new(name.to_string());
         query.exact();
diff --git a/crates/ra_analysis/src/lib.rs b/crates/ra_analysis/src/lib.rs
index b068119d2..4d895b004 100644
--- a/crates/ra_analysis/src/lib.rs
+++ b/crates/ra_analysis/src/lib.rs
@@ -15,6 +15,7 @@ macro_rules! ctry {
 mod db;
 mod imp;
 mod completion;
+mod goto_defenition;
 mod symbol_index;
 pub mod mock_analysis;
 mod runnables;
@@ -273,26 +274,6 @@ impl<T> RangeInfo<T> {
     }
 }
 
-/// Result of "goto def" query.
-#[derive(Debug)]
-pub struct ReferenceResolution {
-    /// The range of the reference itself. Client does not know what constitutes
-    /// a reference, it handles us only the offset. It's helpful to tell the
-    /// client where the reference was.
-    pub reference_range: TextRange,
-    /// What this reference resolves to.
-    pub resolves_to: Vec<NavigationTarget>,
-}
-
-impl ReferenceResolution {
-    fn new(reference_range: TextRange) -> ReferenceResolution {
-        ReferenceResolution {
-            reference_range,
-            resolves_to: Vec::new(),
-        }
-    }
-}
-
 /// `AnalysisHost` stores the current state of the world.
 #[derive(Debug, Default)]
 pub struct AnalysisHost {
@@ -392,12 +373,11 @@ impl Analysis {
             .collect();
         Ok(res)
     }
-    /// Resolves reference to definition, but does not gurantee correctness.
-    pub fn approximately_resolve_symbol(
+    pub fn goto_defenition(
         &self,
         position: FilePosition,
-    ) -> Cancelable<Option<ReferenceResolution>> {
-        self.db.approximately_resolve_symbol(position)
+    ) -> Cancelable<Option<Vec<NavigationTarget>>> {
+        goto_defenition::goto_defenition(&*self.db, position)
     }
     /// Finds all usages of the reference at point.
     pub fn find_all_refs(&self, position: FilePosition) -> Cancelable<Vec<(FileId, TextRange)>> {
diff --git a/crates/ra_analysis/tests/test/main.rs b/crates/ra_analysis/tests/test/main.rs
index beeae1e19..e15035304 100644
--- a/crates/ra_analysis/tests/test/main.rs
+++ b/crates/ra_analysis/tests/test/main.rs
@@ -15,65 +15,6 @@ fn get_signature(text: &str) -> (FnSignatureInfo, Option<usize>) {
 }
 
 #[test]
-fn approximate_resolve_works_in_items() {
-    let (analysis, pos) = analysis_and_position(
-        "
-        //- /lib.rs
-        struct Foo;
-        enum E { X(Foo<|>) }
-    ",
-    );
-
-    let symbols = analysis.approximately_resolve_symbol(pos).unwrap().unwrap();
-    assert_eq_dbg(
-        r#"ReferenceResolution {
-            reference_range: [23; 26),
-            resolves_to: [NavigationTarget { file_id: FileId(1), name: "Foo", kind: STRUCT_DEF, range: [0; 11), ptr: Some(LocalSyntaxPtr { range: [0; 11), kind: STRUCT_DEF }) }]
-        }"#,
-        &symbols,
-    );
-}
-
-#[test]
-fn test_resolve_module() {
-    let (analysis, pos) = analysis_and_position(
-        "
-        //- /lib.rs
-        mod <|>foo;
-        //- /foo.rs
-        // empty
-    ",
-    );
-
-    let symbols = analysis.approximately_resolve_symbol(pos).unwrap().unwrap();
-    assert_eq_dbg(
-        r#"ReferenceResolution {
-            reference_range: [4; 7),
-            resolves_to: [NavigationTarget { file_id: FileId(2), name: "foo", kind: MODULE, range: [0; 0), ptr: None }]
-        }"#,
-        &symbols,
-    );
-
-    let (analysis, pos) = analysis_and_position(
-        "
-        //- /lib.rs
-        mod <|>foo;
-        //- /foo/mod.rs
-        // empty
-    ",
-    );
-
-    let symbols = analysis.approximately_resolve_symbol(pos).unwrap().unwrap();
-    assert_eq_dbg(
-        r#"ReferenceResolution {
-            reference_range: [4; 7),
-            resolves_to: [NavigationTarget { file_id: FileId(2), name: "foo", kind: MODULE, range: [0; 0), ptr: None }]
-        }"#,
-        &symbols,
-    );
-}
-
-#[test]
 fn test_unresolved_module_diagnostic() {
     let (analysis, file_id) = single_file("mod foo;");
     let diagnostics = analysis.diagnostics(file_id).unwrap();
diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs
index ffca3f51c..1baed73ad 100644
--- a/crates/ra_lsp_server/src/main_loop/handlers.rs
+++ b/crates/ra_lsp_server/src/main_loop/handlers.rs
@@ -207,12 +207,11 @@ pub fn handle_goto_definition(
     params: req::TextDocumentPositionParams,
 ) -> Result<Option<req::GotoDefinitionResponse>> {
     let position = params.try_conv_with(&world)?;
-    let rr = match world.analysis().approximately_resolve_symbol(position)? {
+    let navs = match world.analysis().goto_defenition(position)? {
         None => return Ok(None),
         Some(it) => it,
     };
-    let res = rr
-        .resolves_to
+    let res = navs
         .into_iter()
         .map(|nav| nav.try_conv_with(&world))
         .collect::<Result<Vec<_>>>()?;