Merge #435

435: Refactor hover r=matklad a=matklad

Primaraly this moves `hover` to `ra_analysis`, so that we finally can write tests for it!

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

4 files changed, 204 insertions, 161 deletions

diff --git a/crates/ra_analysis/src/hover.rs b/crates/ra_analysis/src/hover.rs
new file mode 100644
index 000000000..766fa0547
--- /dev/null
+++ b/crates/ra_analysis/src/hover.rs
@@ -0,0 +1,176 @@
+use ra_db::{Cancelable, SyntaxDatabase};
+use ra_syntax::{
+    AstNode, SyntaxNode,
+    ast::{self, NameOwner},
+    algo::{find_covering_node, visit::{visitor, Visitor}},
+};
+
+use crate::{db::RootDatabase, RangeInfo, FilePosition, FileRange, NavigationTarget};
+
+pub(crate) fn hover(
+    db: &RootDatabase,
+    position: FilePosition,
+) -> Cancelable<Option<RangeInfo<String>>> {
+    let mut res = Vec::new();
+    let range = if let Some(rr) = db.approximately_resolve_symbol(position)? {
+        for nav in rr.resolves_to {
+            res.extend(doc_text_for(db, nav)?)
+        }
+        rr.reference_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 frange = FileRange {
+            file_id: position.file_id,
+            range: expr.syntax().range(),
+        };
+        res.extend(type_of(db, frange)?);
+        expr.syntax().range()
+    };
+    if res.is_empty() {
+        return Ok(None);
+    }
+    let res = RangeInfo::new(range, res.join("\n\n---\n"));
+    Ok(Some(res))
+}
+
+pub(crate) fn type_of(db: &RootDatabase, frange: FileRange) -> Cancelable<Option<String>> {
+    let file = db.source_file(frange.file_id);
+    let syntax = file.syntax();
+    let node = find_covering_node(syntax, frange.range);
+    let parent_fn = ctry!(node.ancestors().find_map(ast::FnDef::cast));
+    let function = ctry!(hir::source_binder::function_from_source(
+        db,
+        frange.file_id,
+        parent_fn
+    )?);
+    let infer = function.infer(db)?;
+    Ok(infer.type_of_node(node).map(|t| t.to_string()))
+}
+
+// FIXME: this should not really use navigation target. Rather, approximatelly
+// resovled symbol should return a `DefId`.
+fn doc_text_for(db: &RootDatabase, nav: NavigationTarget) -> Cancelable<Option<String>> {
+    let result = match (nav.description(db), nav.docs(db)) {
+        (Some(desc), Some(docs)) => Some("```rust\n".to_string() + &*desc + "\n```\n\n" + &*docs),
+        (Some(desc), None) => Some("```rust\n".to_string() + &*desc + "\n```"),
+        (None, Some(docs)) => Some(docs),
+        _ => None,
+    };
+
+    Ok(result)
+}
+
+impl NavigationTarget {
+    fn node(&self, db: &RootDatabase) -> Option<SyntaxNode> {
+        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)?
+            .owned();
+        Some(node)
+    }
+
+    fn docs(&self, db: &RootDatabase) -> Option<String> {
+        let node = self.node(db)?;
+        let node = node.borrowed();
+        fn doc_comments<'a, N: ast::DocCommentsOwner<'a>>(node: N) -> Option<String> {
+            let comments = node.doc_comment_text();
+            if comments.is_empty() {
+                None
+            } else {
+                Some(comments)
+            }
+        }
+
+        visitor()
+            .visit(doc_comments::<ast::FnDef>)
+            .visit(doc_comments::<ast::StructDef>)
+            .visit(doc_comments::<ast::EnumDef>)
+            .visit(doc_comments::<ast::TraitDef>)
+            .visit(doc_comments::<ast::Module>)
+            .visit(doc_comments::<ast::TypeDef>)
+            .visit(doc_comments::<ast::ConstDef>)
+            .visit(doc_comments::<ast::StaticDef>)
+            .accept(node)?
+    }
+
+    /// Get a description of this node.
+    ///
+    /// e.g. `struct Name`, `enum Name`, `fn Name`
+    fn description(&self, db: &RootDatabase) -> Option<String> {
+        // TODO: After type inference is done, add type information to improve the output
+        let node = self.node(db)?;
+        let node = node.borrowed();
+        // TODO: Refactor to be have less repetition
+        visitor()
+            .visit(|node: ast::FnDef| {
+                let mut string = "fn ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::StructDef| {
+                let mut string = "struct ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::EnumDef| {
+                let mut string = "enum ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::TraitDef| {
+                let mut string = "trait ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::Module| {
+                let mut string = "mod ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::TypeDef| {
+                let mut string = "type ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::ConstDef| {
+                let mut string = "const ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .visit(|node: ast::StaticDef| {
+                let mut string = "static ".to_string();
+                node.name()?.syntax().text().push_to(&mut string);
+                Some(string)
+            })
+            .accept(node)?
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use ra_syntax::TextRange;
+
+    use crate::mock_analysis::single_file_with_position;
+
+    #[test]
+    fn hover_shows_type_of_an_expression() {
+        let (analysis, position) = single_file_with_position(
+            "
+            pub fn foo() -> u32 { 1 }
+
+            fn main() {
+                let foo_test = foo()<|>;
+            }
+        ",
+        );
+        let hover = analysis.hover(position).unwrap().unwrap();
+        assert_eq!(hover.range, TextRange::from_to(95.into(), 100.into()));
+        assert_eq!(hover.info, "u32");
+    }
+}
diff --git a/crates/ra_analysis/src/imp.rs b/crates/ra_analysis/src/imp.rs
index eae73c2c4..e2871451c 100644
--- a/crates/ra_analysis/src/imp.rs
+++ b/crates/ra_analysis/src/imp.rs
@@ -8,11 +8,10 @@ use hir::{
 use ra_db::{FilesDatabase, SourceRoot, SourceRootId, SyntaxDatabase};
 use ra_editor::{self, find_node_at_offset, assists, LocalEdit, Severity};
 use ra_syntax::{
-    algo::{find_covering_node, visit::{visitor, Visitor}},
-    ast::{self, ArgListOwner, Expr, FnDef, NameOwner},
+    ast::{self, ArgListOwner, Expr, NameOwner},
     AstNode, SourceFileNode,
     SyntaxKind::*,
-    SyntaxNode, SyntaxNodeRef, TextRange, TextUnit,
+    SyntaxNodeRef, TextRange, TextUnit,
 };
 
 use crate::{
@@ -256,18 +255,6 @@ impl db::RootDatabase {
             Ok(Some((binding, descr)))
         }
     }
-    pub(crate) fn doc_text_for(&self, nav: NavigationTarget) -> Cancelable<Option<String>> {
-        let result = match (nav.description(self), nav.docs(self)) {
-            (Some(desc), Some(docs)) => {
-                Some("```rust\n".to_string() + &*desc + "\n```\n\n" + &*docs)
-            }
-            (Some(desc), None) => Some("```rust\n".to_string() + &*desc + "\n```"),
-            (None, Some(docs)) => Some(docs),
-            _ => None,
-        };
-
-        Ok(result)
-    }
 
     pub(crate) fn diagnostics(&self, file_id: FileId) -> Cancelable<Vec<Diagnostic>> {
         let syntax = self.source_file(file_id);
@@ -410,19 +397,6 @@ impl db::RootDatabase {
         Ok(None)
     }
 
-    pub(crate) fn type_of(&self, frange: FileRange) -> Cancelable<Option<String>> {
-        let file = self.source_file(frange.file_id);
-        let syntax = file.syntax();
-        let node = find_covering_node(syntax, frange.range);
-        let parent_fn = ctry!(node.ancestors().find_map(FnDef::cast));
-        let function = ctry!(source_binder::function_from_source(
-            self,
-            frange.file_id,
-            parent_fn
-        )?);
-        let infer = function.infer(self)?;
-        Ok(infer.type_of_node(node).map(|t| t.to_string()))
-    }
     pub(crate) fn rename(
         &self,
         position: FilePosition,
@@ -506,91 +480,3 @@ impl<'a> FnCallNode<'a> {
         }
     }
 }
-
-impl NavigationTarget {
-    fn node(&self, db: &db::RootDatabase) -> Option<SyntaxNode> {
-        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)?
-            .owned();
-        Some(node)
-    }
-
-    fn docs(&self, db: &db::RootDatabase) -> Option<String> {
-        let node = self.node(db)?;
-        let node = node.borrowed();
-        fn doc_comments<'a, N: ast::DocCommentsOwner<'a>>(node: N) -> Option<String> {
-            let comments = node.doc_comment_text();
-            if comments.is_empty() {
-                None
-            } else {
-                Some(comments)
-            }
-        }
-
-        visitor()
-            .visit(doc_comments::<ast::FnDef>)
-            .visit(doc_comments::<ast::StructDef>)
-            .visit(doc_comments::<ast::EnumDef>)
-            .visit(doc_comments::<ast::TraitDef>)
-            .visit(doc_comments::<ast::Module>)
-            .visit(doc_comments::<ast::TypeDef>)
-            .visit(doc_comments::<ast::ConstDef>)
-            .visit(doc_comments::<ast::StaticDef>)
-            .accept(node)?
-    }
-
-    /// Get a description of this node.
-    ///
-    /// e.g. `struct Name`, `enum Name`, `fn Name`
-    fn description(&self, db: &db::RootDatabase) -> Option<String> {
-        // TODO: After type inference is done, add type information to improve the output
-        let node = self.node(db)?;
-        let node = node.borrowed();
-        // TODO: Refactor to be have less repetition
-        visitor()
-            .visit(|node: ast::FnDef| {
-                let mut string = "fn ".to_string();
-                node.name()?.syntax().text().push_to(&mut string);
-                Some(string)
-            })
-            .visit(|node: ast::StructDef| {
-                let mut string = "struct ".to_string();
-                node.name()?.syntax().text().push_to(&mut string);
-                Some(string)
-            })
-            .visit(|node: ast::EnumDef| {
-                let mut string = "enum ".to_string();
-                node.name()?.syntax().text().push_to(&mut string);
-                Some(string)
-            })
-            .visit(|node: ast::TraitDef| {
-                let mut string = "trait ".to_string();
-                node.name()?.syntax().text().push_to(&mut string);
-                Some(string)
-            })
-            .visit(|node: ast::Module| {
-                let mut string = "mod ".to_string();
-                node.name()?.syntax().text().push_to(&mut string);
-                Some(string)
-            })
-            .visit(|node: ast::TypeDef| {
-                let mut string = "type ".to_string();
-                node.name()?.syntax().text().push_to(&mut string);
-                Some(string)
-            })
-            .visit(|node: ast::ConstDef| {
-                let mut string = "const ".to_string();
-                node.name()?.syntax().text().push_to(&mut string);
-                Some(string)
-            })
-            .visit(|node: ast::StaticDef| {
-                let mut string = "static ".to_string();
-                node.name()?.syntax().text().push_to(&mut string);
-                Some(string)
-            })
-            .accept(node)?
-    }
-}
diff --git a/crates/ra_analysis/src/lib.rs b/crates/ra_analysis/src/lib.rs
index 1e26a2889..b068119d2 100644
--- a/crates/ra_analysis/src/lib.rs
+++ b/crates/ra_analysis/src/lib.rs
@@ -21,6 +21,7 @@ mod runnables;
 
 mod extend_selection;
 mod syntax_highlighting;
+mod hover;
 
 use std::{fmt, sync::Arc};
 
@@ -260,6 +261,18 @@ impl NavigationTarget {
     }
 }
 
+#[derive(Debug)]
+pub struct RangeInfo<T> {
+    pub range: TextRange,
+    pub info: T,
+}
+
+impl<T> RangeInfo<T> {
+    fn new(range: TextRange, info: T) -> RangeInfo<T> {
+        RangeInfo { range, info }
+    }
+}
+
 /// Result of "goto def" query.
 #[derive(Debug)]
 pub struct ReferenceResolution {
@@ -390,9 +403,9 @@ impl Analysis {
     pub fn find_all_refs(&self, position: FilePosition) -> Cancelable<Vec<(FileId, TextRange)>> {
         self.db.find_all_refs(position)
     }
-    /// Returns documentation string for a given target.
-    pub fn doc_text_for(&self, nav: NavigationTarget) -> Cancelable<Option<String>> {
-        self.db.doc_text_for(nav)
+    /// Returns a short text descrbing element at position.
+    pub fn hover(&self, position: FilePosition) -> Cancelable<Option<RangeInfo<String>>> {
+        hover::hover(&*self.db, position)
     }
     /// Returns a `mod name;` declaration which created the current module.
     pub fn parent_module(&self, position: FilePosition) -> Cancelable<Vec<NavigationTarget>> {
@@ -437,7 +450,7 @@ impl Analysis {
     }
     /// Computes the type of the expression at the given position.
     pub fn type_of(&self, frange: FileRange) -> Cancelable<Option<String>> {
-        self.db.type_of(frange)
+        hover::type_of(&*self.db, frange)
     }
     /// Returns the edit required to rename reference at the position to the new
     /// name.
diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs
index 2fc4d3649..ffca3f51c 100644
--- a/crates/ra_lsp_server/src/main_loop/handlers.rs
+++ b/crates/ra_lsp_server/src/main_loop/handlers.rs
@@ -9,7 +9,7 @@ use languageserver_types::{
     Range, WorkspaceEdit, ParameterInformation, ParameterLabel, SignatureInformation, Hover,
     HoverContents, DocumentFormattingParams, DocumentHighlight,
 };
-use ra_analysis::{FileId, FoldKind, Query, RunnableKind, FileRange, FilePosition, Severity, NavigationTarget};
+use ra_analysis::{FileId, FoldKind, Query, RunnableKind, FileRange, FilePosition, Severity};
 use ra_syntax::{TextUnit, text_utils::intersect};
 use ra_text_edit::text_utils::contains_offset_nonstrict;
 use rustc_hash::FxHashMap;
@@ -509,36 +509,18 @@ pub fn handle_hover(
     world: ServerWorld,
     params: req::TextDocumentPositionParams,
 ) -> Result<Option<Hover>> {
-    // TODO: Cut down on number of allocations
     let position = params.try_conv_with(&world)?;
-    let line_index = world.analysis().file_line_index(position.file_id);
-    let rr = match world.analysis().approximately_resolve_symbol(position)? {
+    let info = match world.analysis().hover(position)? {
         None => return Ok(None),
-        Some(it) => it,
+        Some(info) => info,
     };
-    let mut result = Vec::new();
-    let file_id = params.text_document.try_conv_with(&world)?;
-    let file_range = FileRange {
-        file_id,
-        range: rr.reference_range,
+    let line_index = world.analysis.file_line_index(position.file_id);
+    let range = info.range.conv_with(&line_index);
+    let res = Hover {
+        contents: HoverContents::Scalar(MarkedString::String(info.info)),
+        range: Some(range),
     };
-    if let Some(type_name) = get_type(&world, file_range) {
-        result.push(type_name);
-    }
-    for nav in rr.resolves_to {
-        if let Some(docs) = get_doc_text(&world, nav) {
-            result.push(docs);
-        }
-    }
-
-    let range = rr.reference_range.conv_with(&line_index);
-    if result.len() > 0 {
-        return Ok(Some(Hover {
-            contents: HoverContents::Scalar(MarkedString::String(result.join("\n\n---\n"))),
-            range: Some(range),
-        }));
-    }
-    Ok(None)
+    Ok(Some(res))
 }
 
 /// Test doc comment
@@ -762,17 +744,3 @@ fn to_diagnostic_severity(severity: Severity) -> DiagnosticSeverity {
         WeakWarning => DiagnosticSeverity::Hint,
     }
 }
-
-fn get_type(world: &ServerWorld, file_range: FileRange) -> Option<String> {
-    match world.analysis().type_of(file_range) {
-        Ok(result) => result,
-        _ => None,
-    }
-}
-
-fn get_doc_text(world: &ServerWorld, nav: NavigationTarget) -> Option<String> {
-    match world.analysis().doc_text_for(nav) {
-        Ok(result) => result,
-        _ => None,
-    }
-}