Merge #167

167: Attempt to extract useful comments from function signatures r=matklad a=kjeremy

I'm trying to extract useful function comments for signature info. This will also be useful for hover.  This is a WIP (and actually works pretty well!) but I don't think it's the right approach long term so some guidance would be appreciated so that we could also get comments for say types and variable instances etc.

Currently `test_fn_signature_with_simple_doc` fails due to a bug in `extend` but we probably shouldn't use this approach anyway. Maybe comments should be attached to nodes somehow? I'm also thinking that maybe the markdown bits should live in the language server.

Thoughts?

Co-authored-by: Jeremy A. Kolb <[email protected]>

6 files changed, 235 insertions, 5 deletions

diff --git a/crates/ra_analysis/src/descriptors/function/mod.rs b/crates/ra_analysis/src/descriptors/function/mod.rs
index bb68b0ce7..ae40f3e8f 100644
--- a/crates/ra_analysis/src/descriptors/function/mod.rs
+++ b/crates/ra_analysis/src/descriptors/function/mod.rs
@@ -1,8 +1,11 @@
 pub(super) mod imp;
 mod scope;
 
+use std::cmp::{min, max};
+
 use ra_syntax::{
-    ast::{self, AstNode, NameOwner}
+    ast::{self, AstNode, DocCommentsOwner, NameOwner},
+    TextRange, TextUnit
 };
 
 use crate::{
@@ -30,14 +33,17 @@ pub struct FnDescriptor {
     pub label: String,
     pub ret_type: Option<String>,
     pub params: Vec<String>,
+    pub doc: Option<String>
 }
 
 impl FnDescriptor {
     pub fn new(node: ast::FnDef) -> Option<Self> {
         let name = node.name()?.text().to_string();
 
+        let mut doc = None;
+
         // Strip the body out for the label.
-        let label: String = if let Some(body) = node.body() {
+        let mut label: String = if let Some(body) = node.body() {
             let body_range = body.syntax().range();
             let label: String = node
                 .syntax()
@@ -50,6 +56,36 @@ impl FnDescriptor {
             node.syntax().text().to_string()
         };
 
+        if let Some((comment_range, docs)) = FnDescriptor::extract_doc_comments(node) {
+            let comment_range = comment_range.checked_sub(node.syntax().range().start()).unwrap();
+            let start = comment_range.start().to_usize();
+            let end = comment_range.end().to_usize();
+
+            // Remove the comment from the label
+            label.replace_range(start..end, "");
+
+            // Massage markdown
+            let mut processed_lines = Vec::new();
+            let mut in_code_block = false;
+            for line in docs.lines() {
+                if line.starts_with("```") {
+                    in_code_block = !in_code_block;
+                }
+
+                let line = if in_code_block && line.starts_with("```") && !line.contains("rust") {
+                    "```rust".into()
+                } else {
+                    line.to_string()
+                };
+
+                processed_lines.push(line);
+            }
+
+            if !processed_lines.is_empty() {
+                doc = Some(processed_lines.join("\n"));
+            }
+        }
+
         let params = FnDescriptor::param_list(node);
         let ret_type = node.ret_type().map(|r| r.syntax().text().to_string());
 
@@ -57,10 +93,28 @@ impl FnDescriptor {
             name,
             ret_type,
             params,
-            label,
+            label: label.trim().to_owned(),
+            doc
         })
     }
 
+    fn extract_doc_comments(node: ast::FnDef) -> Option<(TextRange, String)> {
+        if node.doc_comments().count() == 0 {
+            return None;
+        }
+
+        let comment_text = node.doc_comment_text();
+
+        let (begin, end) = node.doc_comments()
+            .map(|comment| comment.syntax().range())
+            .map(|range| (range.start().to_usize(), range.end().to_usize()))
+            .fold((std::usize::MAX, std::usize::MIN), |acc, range| (min(acc.0, range.0), max(acc.1, range.1)));
+
+        let range = TextRange::from_to(TextUnit::from_usize(begin), TextUnit::from_usize(end));
+
+        Some((range, comment_text))
+    }
+
     fn param_list(node: ast::FnDef) -> Vec<String> {
         let mut res = vec![];
         if let Some(param_list) = node.param_list() {
diff --git a/crates/ra_analysis/tests/tests.rs b/crates/ra_analysis/tests/tests.rs
index 94e025677..9e2478d9e 100644
--- a/crates/ra_analysis/tests/tests.rs
+++ b/crates/ra_analysis/tests/tests.rs
@@ -195,6 +195,153 @@ fn bar() {
     assert_eq!(param, Some(1));
 }
 
+#[test]
+fn test_fn_signature_with_docs_simple() {
+    let (desc, param) = get_signature(
+        r#"
+// test
+fn foo(j: u32) -> u32 {
+    j
+}
+
+fn bar() {
+    let _ = foo(<|>);
+}
+"#,
+    );
+
+    assert_eq!(desc.name, "foo".to_string());
+    assert_eq!(desc.params, vec!["j".to_string()]);
+    assert_eq!(desc.ret_type, Some("-> u32".to_string()));
+    assert_eq!(param, Some(0));
+    assert_eq!(desc.label, "fn foo(j: u32) -> u32".to_string());
+    assert_eq!(desc.doc, Some("test".into()));
+}
+
+#[test]
+fn test_fn_signature_with_docs() {
+    let (desc, param) = get_signature(
+        r#"
+/// Adds one to the number given.
+///
+/// # Examples
+///
+/// ```
+/// let five = 5;
+///
+/// assert_eq!(6, my_crate::add_one(5));
+/// ```
+pub fn add_one(x: i32) -> i32 {
+    x + 1
+}
+
+pub fn do() {
+    add_one(<|>
+}"#,
+    );
+
+    assert_eq!(desc.name, "add_one".to_string());
+    assert_eq!(desc.params, vec!["x".to_string()]);
+    assert_eq!(desc.ret_type, Some("-> i32".to_string()));
+    assert_eq!(param, Some(0));
+    assert_eq!(desc.label, "pub fn add_one(x: i32) -> i32".to_string());
+    assert_eq!(desc.doc, Some(
+r#"Adds one to the number given.
+
+# Examples
+
+```rust
+let five = 5;
+
+assert_eq!(6, my_crate::add_one(5));
+```"#.into()));
+}
+
+#[test]
+fn test_fn_signature_with_docs_impl() {
+    let (desc, param) = get_signature(
+        r#"
+struct addr;
+impl addr {
+    /// Adds one to the number given.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// let five = 5;
+    ///
+    /// assert_eq!(6, my_crate::add_one(5));
+    /// ```
+    pub fn add_one(x: i32) -> i32 {
+        x + 1
+    }
+}
+
+pub fn do_it() {
+    addr {};
+    addr::add_one(<|>);
+}"#);
+
+    assert_eq!(desc.name, "add_one".to_string());
+    assert_eq!(desc.params, vec!["x".to_string()]);
+    assert_eq!(desc.ret_type, Some("-> i32".to_string()));
+    assert_eq!(param, Some(0));
+    assert_eq!(desc.label, "pub fn add_one(x: i32) -> i32".to_string());
+    assert_eq!(desc.doc, Some(
+r#"Adds one to the number given.
+
+# Examples
+
+```rust
+let five = 5;
+
+assert_eq!(6, my_crate::add_one(5));
+```"#.into()));
+}
+
+#[test]
+fn test_fn_signature_with_docs_from_actix() {
+    let (desc, param) = get_signature(
+        r#"
+pub trait WriteHandler<E>
+where
+    Self: Actor,
+    Self::Context: ActorContext,
+{
+    /// Method is called when writer emits error.
+    ///
+    /// If this method returns `ErrorAction::Continue` writer processing
+    /// continues otherwise stream processing stops.
+    fn error(&mut self, err: E, ctx: &mut Self::Context) -> Running {
+        Running::Stop
+    }
+
+    /// Method is called when writer finishes.
+    ///
+    /// By default this method stops actor's `Context`.
+    fn finished(&mut self, ctx: &mut Self::Context) {
+        ctx.stop()
+    }
+}
+
+pub fn foo() {
+    WriteHandler r;
+    r.finished(<|>);
+}
+
+"#);
+
+    assert_eq!(desc.name, "finished".to_string());
+    assert_eq!(desc.params, vec!["&mut self".to_string(), "ctx".to_string()]);
+    assert_eq!(desc.ret_type, None);
+    assert_eq!(param, Some(1));
+    assert_eq!(desc.doc, Some(
+r#"Method is called when writer finishes.
+
+By default this method stops actor's `Context`."#.into()));
+}
+
+
 fn get_all_refs(text: &str) -> Vec<(FileId, TextRange)> {
     let (analysis, position) = single_file_with_position(text);
     analysis.find_all_refs(position.file_id, position.offset).unwrap()
diff --git a/crates/ra_lsp_server/src/main_loop/handlers.rs b/crates/ra_lsp_server/src/main_loop/handlers.rs
index 4ac08e527..20cb5f772 100644
--- a/crates/ra_lsp_server/src/main_loop/handlers.rs
+++ b/crates/ra_lsp_server/src/main_loop/handlers.rs
@@ -5,7 +5,7 @@ use languageserver_types::{
     CodeActionResponse, Command, CompletionItem, CompletionItemKind, Diagnostic,
     DiagnosticSeverity, DocumentSymbol, FoldingRange, FoldingRangeKind, FoldingRangeParams,
     InsertTextFormat, Location, Position, SymbolInformation, TextDocumentIdentifier, TextEdit,
-    RenameParams, WorkspaceEdit, PrepareRenameResponse
+    RenameParams, WorkspaceEdit, PrepareRenameResponse, Documentation, MarkupContent, MarkupKind
 };
 use gen_lsp_server::ErrorCode;
 use ra_analysis::{FileId, FoldKind, Query, RunnableKind};
@@ -465,9 +465,18 @@ pub fn handle_signature_help(
             })
             .collect();
 
+        let documentation = if let Some(doc) = descriptor.doc {
+            Some(Documentation::MarkupContent(MarkupContent {
+                kind: MarkupKind::Markdown,
+                value: doc
+            }))
+        } else {
+            None
+        };
+
         let sig_info = SignatureInformation {
             label: descriptor.label,
-            documentation: None,
+            documentation,
             parameters: Some(parameters),
         };
 
diff --git a/crates/ra_syntax/src/ast/generated.rs b/crates/ra_syntax/src/ast/generated.rs
index d0cd060d3..b6a15dbdc 100644
--- a/crates/ra_syntax/src/ast/generated.rs
+++ b/crates/ra_syntax/src/ast/generated.rs
@@ -864,6 +864,7 @@ impl<'a> AstNode<'a> for FnDef<'a> {
 impl<'a> ast::NameOwner<'a> for FnDef<'a> {}
 impl<'a> ast::TypeParamsOwner<'a> for FnDef<'a> {}
 impl<'a> ast::AttrsOwner<'a> for FnDef<'a> {}
+impl<'a> ast::DocCommentsOwner<'a> for FnDef<'a> {}
 impl<'a> FnDef<'a> {
     pub fn param_list(self) -> Option<ParamList<'a>> {
         super::child_opt(self)
diff --git a/crates/ra_syntax/src/ast/mod.rs b/crates/ra_syntax/src/ast/mod.rs
index c033263a1..3aa11b9dd 100644
--- a/crates/ra_syntax/src/ast/mod.rs
+++ b/crates/ra_syntax/src/ast/mod.rs
@@ -65,6 +65,24 @@ pub trait AttrsOwner<'a>: AstNode<'a> {
     }
 }
 
+pub trait DocCommentsOwner<'a>: AstNode<'a> {
+    fn doc_comments(self) -> AstChildren<'a, Comment<'a>> { children(self) }
+
+    /// Returns the textual content of a doc comment block as a single string.
+    /// That is, strips leading `///` and joins lines
+    fn doc_comment_text(self) -> String {
+        self.doc_comments()
+            .map(|comment| {
+                let prefix = comment.prefix();
+                let trimmed = comment.text().as_str()
+                    .trim()
+                    .trim_start_matches(prefix)
+                    .trim_start();
+                trimmed.to_owned()
+            }).join("\n")
+    }
+}
+
 impl<'a> FnDef<'a> {
     pub fn has_atom_attr(&self, atom: &str) -> bool {
         self.attrs().filter_map(|x| x.as_atom()).any(|x| x == atom)
diff --git a/crates/ra_syntax/src/grammar.ron b/crates/ra_syntax/src/grammar.ron
index c1c215e0d..6951db010 100644
--- a/crates/ra_syntax/src/grammar.ron
+++ b/crates/ra_syntax/src/grammar.ron
@@ -251,6 +251,7 @@ Grammar(
                 "NameOwner",
                 "TypeParamsOwner",
                 "AttrsOwner",
+                "DocCommentsOwner"
             ],
             options: [ "ParamList", ["body", "Block"], "RetType" ],
         ),