fix(starlark_lsp): hover should give docs for functions with no docstrings

Summary:
I don't know why this was done, but it makes buck2 lsp have largely broken hover on most real code.

This area of the code needs tests. I will look into writing a test fixture when I triage more of these bugs.

Part of facebook/buck2#899

X-link: facebook/buck2#934

Reviewed By: JakobDegen

Differential Revision: D74042714

fbshipit-source-id: 45cb2462c1360a0b31058ec62ccb114b569a85eb

Author: lf-

starlark/src/docs.rs

@@ -87,7 +87,8 @@ pub struct DocFunction {
     /// they are present.
     pub docs: Option<DocString>,
     /// The parameters that this function takes. Docs for these parameters should generally be
-    /// extracted from the main docstring's details.
+    /// extracted from the main docstring's details, but may be extracted from the definition if the
+    /// docstring is not present.
     pub params: DocParams,
     /// Details about what this function returns.
     pub ret: DocReturn,

starlark_lsp/src/docs.rs

@@ -30,6 +30,7 @@ use starlark_syntax::syntax::ast::AstStmtP;
 use starlark_syntax::syntax::ast::DefP;
 use starlark_syntax::syntax::ast::ExprP;
 use starlark_syntax::syntax::ast::StmtP;
+use starlark_syntax::syntax::def::DefParamKind;
 use starlark_syntax::syntax::def::DefParams;
 
 /// Given the AST node for a `def` statement, return a `DocFunction` if the
@@ -38,38 +39,45 @@ pub(crate) fn get_doc_item_for_def<P: AstPayload>(
     def: &DefP<P>,
     codemap: &CodeMap,
 ) -> Option<DocFunction> {
-    if let Some(doc_string) = peek_docstring(&def.body) {
-        // TODO(nga): do not unwrap.
-        let def = DefParams::unpack(&def.params, codemap).unwrap();
+    let raw_docstring = peek_docstring(&def.body);
+    // TODO(nga): do not unwrap.
+    let def = DefParams::unpack(&def.params, codemap).unwrap();
 
-        let dp = |i: usize| -> DocParam {
-            let param = &def.params[i];
-            DocParam {
-                name: param.ident.ident.clone(),
-                docs: None,
-                typ: Ty::any(),
-                default_value: None,
+    let dp = |i: usize| -> DocParam {
+        let param = &def.params[i];
+
+        // Just get the default's expr directly out of the source file for now
+        // TODO(jadel): maybe this should be re-formatted somehow?
+        let default_value = match param.node.kind {
+            DefParamKind::Regular(_, Some(default)) => {
+                Some(codemap.source_span(default.span).to_owned())
             }
+            _ => None,
         };
 
-        let doc_params = DocParams {
-            pos_only: def.indices.pos_only().map(dp).collect(),
-            pos_or_named: def.indices.pos_or_named().map(dp).collect(),
-            args: def.indices.args.map(|a| a as usize).map(dp),
-            named_only: def.indices.named_only(def.params.len()).map(dp).collect(),
-            kwargs: def.indices.kwargs.map(|a| a as usize).map(dp),
-        };
-        let doc_function = DocFunction::from_docstring(
-            DocStringKind::Starlark,
-            doc_params,
-            // TODO: Figure out how to get a `Ty` from the `def.return_type`.
-            Ty::any(),
-            Some(doc_string),
-        );
-        Some(doc_function)
-    } else {
-        None
-    }
+        DocParam {
+            name: param.ident.ident.clone(),
+            docs: None,
+            typ: Ty::any(),
+            default_value,
+        }
+    };
+
+    let doc_params = DocParams {
+        pos_only: def.indices.pos_only().map(dp).collect(),
+        pos_or_named: def.indices.pos_or_named().map(dp).collect(),
+        args: def.indices.args.map(|a| a as usize).map(dp),
+        named_only: def.indices.named_only(def.params.len()).map(dp).collect(),
+        kwargs: def.indices.kwargs.map(|a| a as usize).map(dp),
+    };
+    let doc_function = DocFunction::from_docstring(
+        DocStringKind::Starlark,
+        doc_params,
+        // TODO: Figure out how to get a `Ty` from the `def.return_type`.
+        Ty::any(),
+        raw_docstring,
+    );
+    Some(doc_function)
 }
 
 pub(crate) fn get_doc_item_for_assign<P: AstPayload>(

starlark_lsp/src/symbols.rs

@@ -166,8 +166,15 @@ pub(crate) fn find_symbols_at_location<P: AstPayload>(
 mod tests {
     use std::collections::HashMap;
 
+    use starlark::docs::DocFunction;
+    use starlark::docs::DocItem;
+    use starlark::docs::DocMember;
+    use starlark::docs::DocParam;
+    use starlark::docs::DocParams;
+    use starlark::docs::DocReturn;
     use starlark::syntax::AstModule;
     use starlark::syntax::Dialect;
+    use starlark::typing::Ty;
     use starlark_syntax::codemap::ResolvedPos;
     use starlark_syntax::syntax::module::AstModuleFields;
 
@@ -181,7 +188,7 @@ mod tests {
             "t.star",
             r#"load("foo.star", "exported_a", renamed = "exported_b")
 
-def method(param):
+def method(param = True):
     pass
 
 my_var = True
@@ -191,12 +198,14 @@ my_var = True
         )
         .unwrap();
 
+        let foo = find_symbols_at_location(
+            ast_module.codemap(),
+            ast_module.statement(),
+            ResolvedPos { line: 6, column: 0 },
+        );
+
         assert_eq!(
-            find_symbols_at_location(
-                ast_module.codemap(),
-                ast_module.statement(),
-                ResolvedPos { line: 6, column: 0 },
-            ),
+            foo,
             HashMap::from([
                 (
                     "exported_a".to_owned(),
@@ -224,7 +233,25 @@ my_var = True
                         name: "method".to_owned(),
                         detail: None,
                         kind: SymbolKind::Method,
-                        doc: None,
+                        doc: Some(DocItem::Member(DocMember::Function(DocFunction {
+                            docs: None,
+                            params: DocParams {
+                                pos_only: vec![],
+                                pos_or_named: vec![DocParam {
+                                    name: String::from("param"),
+                                    docs: None,
+                                    typ: Ty::any(),
+                                    default_value: Some(String::from("True"))
+                                }],
+                                args: None,
+                                named_only: vec![],
+                                kwargs: None
+                            },
+                            ret: DocReturn {
+                                docs: None,
+                                typ: Ty::any()
+                            }
+                        }))),
                         param: None,
                     },
                 ),
@@ -291,7 +318,25 @@ my_var = True
                         name: "method".to_owned(),
                         detail: None,
                         kind: SymbolKind::Method,
-                        doc: None,
+                        doc: Some(DocItem::Member(DocMember::Function(DocFunction {
+                            docs: None,
+                            params: DocParams {
+                                pos_only: vec![],
+                                pos_or_named: vec![DocParam {
+                                    name: String::from("param"),
+                                    docs: None,
+                                    typ: Ty::any(),
+                                    default_value: None
+                                }],
+                                args: None,
+                                named_only: vec![],
+                                kwargs: None
+                            },
+                            ret: DocReturn {
+                                docs: None,
+                                typ: Ty::any()
+                            }
+                        }))),
                         param: None,
                     },
                 ),
@@ -302,7 +347,15 @@ my_var = True
                         detail: None,
                         kind: SymbolKind::Variable,
                         doc: None,
-                        param: None,
+                        param: Some((
+                            String::from("param"),
+                            DocParam {
+                                name: String::from("param"),
+                                docs: None,
+                                typ: Ty::any(),
+                                default_value: None
+                            }
+                        )),
                     }
                 ),
                 (

starlark_syntax/src/syntax/def.rs

@@ -49,9 +49,14 @@ pub enum DefParamKind<'a, P: AstPayload> {
     Kwargs,
 }
 
+/// One function parameter.
 pub struct DefParam<'a, P: AstPayload> {
+    /// Name of the parameter.
     pub ident: &'a AstAssignIdentP<P>,
+    /// Whether this is a regular parameter (with optional default) or a varargs construct (*args,
+    /// **kwargs).
     pub kind: DefParamKind<'a, P>,
+    /// Type of the parameter. This is None when a type is not specified.
     pub ty: Option<&'a AstTypeExprP<P>>,
 }