Make doc block finding fall back to interface for hover.

Summary:
When hovering/autocompleting a class member that doesn't have a doc block, we should fall back to the doc block on the interface that declares that member, if there is one.

This change will make hover fall back to interfaces.

Differential Revision: D7679170

fbshipit-source-id: 4b9a48a22ed588277bc1fc33d05363bd241528f0

hphp/hack/src/server/serverDocblockAt.ml

@@ -9,7 +9,107 @@
 
 open Hh_core
 
-let go env (filename, line, char) =
+let get_all_ancestors tcopt class_name =
+  let rec helper classes_to_check cinfos seen_classes =
+    match classes_to_check with
+    | [] -> cinfos
+    | class_name :: classes when SSet.mem class_name seen_classes ->
+      helper classes cinfos seen_classes
+    | class_name :: classes ->
+      begin match Typing_lazy_heap.get_class tcopt class_name with
+      | None ->
+        helper classes cinfos seen_classes
+      | Some class_info ->
+        let ancestors = SMap.keys class_info.Typing_defs.tc_ancestors in
+        helper
+          (ancestors @ classes)
+          (class_info :: cinfos)
+          (SSet.add class_name seen_classes)
+      end
+  in
+  helper [class_name] [] SSet.empty
+
+let get_docblock_for_member class_info member_name =
+  let open Option.Monad_infix in
+  SMap.find_opt member_name (class_info.Typing_defs.tc_methods)
+  >>= fun member ->
+  match Lazy.force member.Typing_defs.ce_type with
+  | _, Typing_defs.Tfun ft ->
+    let pos = ft.Typing_defs.ft_pos in
+    let filename = Pos.filename pos in
+    File_heap.get_contents filename
+    >>= begin fun contents ->
+      ServerSymbolDefinition.get_definition_cst_node_from_pos
+        SymbolDefinition.Method
+        (Full_fidelity_source_text.make filename contents)
+        pos
+      >>= Docblock_finder.get_docblock
+    end
+  | _ -> None
+
+let render_ancestor_docblocks docblocks =
+  let docblocks_to_ancestor =
+    docblocks
+    |> List.fold ~init:SMap.empty ~f:begin fun acc (class_name, docblock) ->
+      let existing_ancestors = match SMap.find_opt docblock acc with
+        | None -> []
+        | Some lst -> lst
+      in
+      SMap.add docblock (class_name :: existing_ancestors) acc
+    end
+  in
+  match SMap.elements docblocks_to_ancestor with
+  | [] -> None
+  | [docblock, _] -> Some docblock
+  | docblock_ancestors_pairs ->
+    docblock_ancestors_pairs
+    |> List.map ~f:begin fun (docblock, ancestors) ->
+      let ancestors_str =
+        String.concat ", " (List.map ~f:Utils.strip_ns ancestors)
+      in
+      Printf.sprintf "%s\n(from %s)" docblock ancestors_str
+    end
+    |> String.concat "\n\n---\n\n"
+    |> fun results -> Some results
+
+let fallback tcopt class_name member_name =
+  let rec all_interfaces_or_first_class_docblock seen_interfaces ancestors_to_check =
+    match ancestors_to_check with
+    | [] -> seen_interfaces
+    | ancestor :: ancestors ->
+      begin match get_docblock_for_member ancestor member_name with
+      | None ->
+        all_interfaces_or_first_class_docblock seen_interfaces ancestors
+      | Some docblock ->
+        match ancestor.Typing_defs.tc_kind with
+        | Ast.Cabstract
+        | Ast.Cnormal ->
+          [(ancestor.Typing_defs.tc_name, docblock)]
+        | _ ->
+          all_interfaces_or_first_class_docblock
+            ((ancestor.Typing_defs.tc_name, docblock) :: seen_interfaces)
+            ancestors
+      end
+  in
+  get_all_ancestors tcopt class_name
+  |> all_interfaces_or_first_class_docblock []
+  |> render_ancestor_docblocks
+
+let go_def tcopt def ~base_class_name ~file =
+  let open Option.Monad_infix in
+  (** Read as "or-else." *)
+  let (>>~) opt f = if Option.is_some opt then opt else f () in
+  def.SymbolDefinition.docblock
+  >>~ fun () ->
+  ServerSymbolDefinition.get_definition_cst_node file def
+  >>= Docblock_finder.get_docblock
+  >>~ fun () ->
+  match def.SymbolDefinition.kind, base_class_name with
+  | SymbolDefinition.Method, Some base_class_name ->
+    fallback tcopt base_class_name def.SymbolDefinition.name
+  | _ -> None
+
+let go_location env (filename, line, char) ~base_class_name =
   let open Option.Monad_infix in
   let ServerEnv.{ tcopt; _ } = env in
   let relative_path = Relative_path.create_detect_prefix filename in
@@ -21,5 +121,4 @@ let go env (filename, line, char) =
     in
     List.hd definitions
   end
-  >>= ServerSymbolDefinition.get_definition_cst_node (ServerCommandTypes.FileName filename)
-  >>= Docblock_finder.get_docblock
+  >>= go_def tcopt ~base_class_name ~file:(ServerCommandTypes.FileName filename)

hphp/hack/src/server/serverDocblockAt.mli

@@ -7,5 +7,17 @@
  *
  *)
 
+(** Returns the docblock for the given symbol or expression. *)
+val go_def :
+  TypecheckerOptions.t ->
+  Relative_path.t SymbolDefinition.t ->
+  base_class_name: string option ->
+  file: ServerCommandTypes.file_input ->
+  DocblockService.result
+
 (** Returns the docblock for the symbol or expression at the given location. *)
-val go : ServerEnv.env -> (string * int * int) -> DocblockService.result
+val go_location :
+  ServerEnv.env ->
+  (string * int * int) ->
+  base_class_name: string option ->
+  DocblockService.result

hphp/hack/src/server/serverHover.ml

@@ -63,13 +63,8 @@ let make_hover_info tcopt env_and_ty file (occurrence, def_opt) =
   let addendum = [
     (match def_opt with
       | Some def ->
-        begin match def.SymbolDefinition.docblock with
-        | Some s -> [s]
-        | None ->
-          ServerSymbolDefinition.get_definition_cst_node file def
-          |> (fun c -> Option.bind c Docblock_finder.get_docblock)
-          |> Option.to_list
-        end
+        ServerDocblockAt.go_def tcopt def ~base_class_name:(SymbolOccurrence.enclosing_class occurrence) ~file
+        |> Option.to_list
       | None -> []);
     (match occurrence, env_and_ty with
       | { type_ = Method _; _ }, _

hphp/hack/src/server/serverRpc.ml

@@ -39,7 +39,7 @@ let handle : type a. genv -> env -> is_stale:bool -> a t -> env * a =
     | IDE_HOVER (fn, line, char) ->
         env, ServerHover.go env (fn, line, char)
     | DOCBLOCK_AT (filename, line, char) ->
-        env, ServerDocblockAt.go env (filename, line, char)
+        env, ServerDocblockAt.go_location env (filename, line, char) ~base_class_name:None
     | AUTOCOMPLETE content ->
         let result = try
           let autocomplete_context = { AutocompleteTypes.

hphp/hack/src/server/serverSymbolDefinition.ml

@@ -164,26 +164,15 @@ let go tcopt ast result =
       get_local_var_def
         ast result.SymbolOccurrence.name result.SymbolOccurrence.pos
 
-let get_definition_cst_node fallback_fn definition =
-  let open SymbolDefinition in
+let get_definition_cst_node_from_pos kind source_text pos =
   let env = Full_fidelity_parser_env.default in
-  let source_text = if Pos.filename definition.pos = ServerIdeUtils.path
-    then
-      (* When the definition is in an IDE buffer with local changes, the filename
-         in the definition will be empty. *)
-      match fallback_fn with
-      | ServerCommandTypes.FileName filename ->
-        SourceText.from_file (Relative_path.create_detect_prefix filename)
-      | ServerCommandTypes.FileContent content ->
-        SourceText.make Relative_path.default content
-    else SourceText.from_file (Pos.filename definition.pos)
-  in
   let tree = SyntaxTree.make ~env source_text in
-  let (line, start, _) = Pos.info_pos definition.pos in
+  let (line, start, _) = Pos.info_pos pos in
   let offset = SourceText.position_to_offset source_text (line, start) in
   let parents = Syntax.parentage (SyntaxTree.root tree) offset in
+  let open SymbolDefinition in
   List.find parents ~f:begin fun syntax ->
-    match definition.kind, Syntax.kind syntax with
+    match kind, Syntax.kind syntax with
     | Function, SyntaxKind.FunctionDeclaration
     | Class, SyntaxKind.ClassishDeclaration
     | Method, SyntaxKind.MethodishDeclaration
@@ -198,3 +187,18 @@ let get_definition_cst_node fallback_fn definition =
     | Typedef, SyntaxKind.SimpleTypeSpecifier -> true
     | _ -> false
   end
+
+let get_definition_cst_node fallback_fn definition =
+  let open SymbolDefinition in
+  let source_text = if Pos.filename definition.pos = ServerIdeUtils.path
+    then
+      (* When the definition is in an IDE buffer with local changes, the filename
+         in the definition will be empty. *)
+      match fallback_fn with
+      | ServerCommandTypes.FileName filename ->
+        SourceText.from_file (Relative_path.create_detect_prefix filename)
+      | ServerCommandTypes.FileContent content ->
+        SourceText.make Relative_path.default content
+    else SourceText.from_file (Pos.filename definition.pos)
+  in
+  get_definition_cst_node_from_pos definition.kind source_text definition.pos

hphp/hack/src/server/serverSymbolDefinition.mli

@@ -13,6 +13,12 @@ val go :
   Relative_path.t SymbolOccurrence.t ->
   Relative_path.t SymbolDefinition.t option
 
+val get_definition_cst_node_from_pos :
+  SymbolDefinition.kind ->
+  Full_fidelity_source_text.t ->
+  'a Pos.pos ->
+  Full_fidelity_positioned_syntax.t option
+
 val get_definition_cst_node :
   ServerCommandTypes.file_input ->
   Relative_path.t SymbolDefinition.t ->

hphp/hack/src/utils/symbolOccurrence.ml

@@ -38,3 +38,11 @@ let kind_to_string = function
   | ClassConst _ -> "member_const"
   | Typeconst _ -> "typeconst"
   | GConst -> "global_const"
+
+let enclosing_class occurrence =
+  match occurrence.type_ with
+  | Method (c, _)
+  | Property (c, _)
+  | ClassConst (c, _)
+  | Typeconst (c, _) -> Some c
+  | _ -> None

hphp/hack/test/integration_ml/test_server_hover.ml

@@ -546,6 +546,137 @@ let bounded_generic_fun_cases = [
   }];
 ]
 
+let doc_block_fallback = "<?hh // strict
+function dbfb_func(DBFBClass2 $c, DBFBClass3 $d): void {
+  $c->doTheThing();
+//    ^3:7
+  $c->docBlockInClass();
+//    ^5:7
+  $c->identical();
+//    ^7:7
+  $c->slightlyDifferent();
+//    ^9:7
+  $c->noDocBlock();
+//    ^11:7
+  $d->docBlockInClass2();
+//    ^13:7
+}
+
+interface DBFBInterface1 {
+  /** DBFBInterface1. */
+  public function doTheThing(): void;
+
+  /** Identical. */
+  public function identical(): void;
+
+  /** Slightly different. */
+  public function slightlyDifferent(): void;
+
+  public function noDocBlock(): void;
+}
+
+interface DBFBInterface2 {
+  /** DBFBInterface2. */
+  public function doTheThing(): void;
+
+  /** DBFBInterface2. */
+  public function docBlockInClass(): void;
+
+  /** DBFBInterface2. */
+  public function docBlockInClass2(): void;
+
+  /** Identical. */
+  public function identical(): void;
+
+  /** Slightly different. */
+  public function slightlyDifferent(): void;
+
+  public function noDocBlock(): void;
+}
+
+interface DBFBInterface3 {
+  /** Slightly more different. */
+  public function slightlyDifferent(): void;
+}
+
+class DBFBClass1 implements DBFBInterface1 {
+  public function doTheThing(): void {}
+
+  /** DBFBClass1. */
+  public function docBlockInClass(): void {}
+
+  /** DBFBClass1. */
+  public function docBlockInClass2(): void {}
+
+  public function identical(): void {}
+
+  public function slightlyDifferent(): void {}
+
+  public function noDocBlock(): void {}
+}
+
+class DBFBClass2 extends DBFBClass1 implements DBFBInterface2, DBFBInterface3 {
+  public function docBlockInClass2(): void {}
+}
+
+class DBFBClass3 extends DBFBClass2 {
+  public function docBlockInClass2(): void {}
+}
+"
+
+let doc_block_fallback_cases = [
+  ("doc_block_fallback.php", 3, 7), [{
+    snippet = "public function doTheThing(): void";
+    addendum = [
+      "DBFBInterface2.\n(from DBFBInterface2)\n\n---\n\nDBFBInterface1.\n(from DBFBInterface1)";
+      "Full name: `DBFBClass1::doTheThing`";
+    ];
+    pos = pos_at (3, 7) (3, 16);
+  }];
+  ("doc_block_fallback.php", 5, 7), [{
+    snippet = "public function docBlockInClass(): void";
+    addendum = [
+      "DBFBClass1.";
+      "Full name: `DBFBClass1::docBlockInClass`";
+    ];
+    pos = pos_at (5, 7) (5, 21);
+  }];
+  ("doc_block_fallback.php", 7, 7), [{
+   snippet = "public function identical(): void";
+   addendum = [
+     "Identical.";
+     "Full name: `DBFBClass1::identical`";
+   ];
+   pos = pos_at (7, 7) (7, 15);
+  }];
+  ("doc_block_fallback.php", 9, 7), [{
+   snippet = "public function slightlyDifferent(): void";
+   addendum = [
+     "Slightly more different.\n(from DBFBInterface3)\n\n\
+        ---\n\n\
+        Slightly different.\n(from DBFBInterface1, DBFBInterface2)";
+     "Full name: `DBFBClass1::slightlyDifferent`";
+   ];
+   pos = pos_at (9, 7) (9, 23);
+  }];
+  ("doc_block_fallback.php", 11, 7), [{
+   snippet = "public function noDocBlock(): void";
+   addendum = ["Full name: `DBFBClass1::noDocBlock`"];
+   pos = pos_at (11, 7) (11, 16);
+  }];
+
+  (* When falling back, if any class ancestors have a doc block don't show any
+     doc blocks from interface ancestors. *)
+  ("doc_block_fallback.php", 13, 7), [{
+   snippet = "public function docBlockInClass2(): void";
+   addendum = [
+     "DBFBClass1.";
+     "Full name: `DBFBClass3::docBlockInClass2`";
+   ];
+   pos = pos_at (13, 7) (13, 22);
+  }];
+]
+
 let files = [
   "class_members.php", class_members;
   "classname_call.php", classname_call;
@@ -554,10 +685,12 @@ let files = [
   "docblock.php", docblock;
   "special_cases.php", special_cases;
   "bounded_generic_fun.php", bounded_generic_fun;
+  "doc_block_fallback.php", doc_block_fallback;
 ]
 
 let cases =
-  special_cases_cases
+  doc_block_fallback_cases
+  @ special_cases_cases
   @ docblock_cases
   @ class_members_cases
   @ classname_call_cases