Cleaner "Defined in" section

Summary:
See test plan for a before/after picture.
This will allow to add a link to where the method is defined in a future diff.

Reviewed By: mheiber

Differential Revision: D69860623

fbshipit-source-id: 86d9b0eb30fe55cbd1b5c399478b25173c5d8bf7

hphp/hack/src/client/clientLsp.ml

@@ -2205,23 +2205,7 @@ let do_rageFB (state : state) : RageFB.result Lwt.t =
   Lwt.return [{ RageFB.title = None; data }]
 
 let do_hover_common (infos : HoverService.hover_info list) : Hover.result =
-  let contents =
-    infos
-    |> List.map ~f:(fun hoverInfo ->
-           (* Hack server uses None to indicate absence of a result. *)
-           (* We're also catching the non-result "" just in case...               *)
-           match hoverInfo with
-           | { HoverService.snippet = ""; _ } -> []
-           | { HoverService.snippet; addendum; _ } ->
-             let addendum =
-               if List.is_empty addendum then
-                 addendum
-               else
-                 MarkedString "---" :: addendum
-             in
-             MarkedCode ("hack", snippet) :: addendum)
-    |> List.concat
-  in
+  let contents = HoverService.as_marked_string_list infos in
   (* We pull the position from the SymbolOccurrence.t record, so I would be
      surprised if there were any different ones in here. Just take the first
      non-None one. *)

hphp/hack/src/client/ide_service/ide_hover.ml

@@ -406,19 +406,17 @@ let split_class_name (full_name : string) : string =
   | Some (class_name, _member) -> class_name
   | None -> full_name
 
-let fun_defined_in def_opt : string =
-  match def_opt with
-  | Some def ->
-    let abs_name = "\\" ^ SymbolDefinition.full_name def in
-    if SN.PseudoFunctions.is_pseudo_function abs_name then
-      ""
-    else (
-      match String.rsplit2 (Utils.strip_hh_lib_ns abs_name) ~on:'\\' with
-      | Some (namespace, _) when not (String.equal namespace "") ->
-        Printf.sprintf "// Defined in namespace %s\n" namespace
-      | _ -> ""
-    )
-  | _ -> ""
+let make_fun_defined_in_section def_opt : string option =
+  let open Option.Let_syntax in
+  let* def = def_opt in
+  let abs_name = "\\" ^ SymbolDefinition.full_name def in
+  if SN.PseudoFunctions.is_pseudo_function abs_name then
+    None
+  else
+    match String.rsplit2 (Utils.strip_hh_lib_ns abs_name) ~on:'\\' with
+    | Some (namespace, _) when not (String.equal namespace "") ->
+      Some (Printf.sprintf "Defined in namespace `%s`" namespace)
+    | _ -> None
 
 (** Return the decl type of a class member or function represented by a SymbolDefinition.t.
     If passed a class member, also return the type parameters of that class *)
@@ -482,6 +480,8 @@ let get_decl_ty
     | _ -> None)
   | _ -> None
 
+let make_hack_marked_code s = Lsp.MarkedCode ("hack", s)
+
 (** Make the 'instantiation' section of the hover card, something like:
 
 ---
@@ -500,45 +500,42 @@ let make_instantiation_section
     let Derive_type_instantiation.Instantiation.{ this; subst } = subst in
     let header = Lsp.MarkedString "Instantiation:" in
     let section =
-      Lsp.MarkedCode
-        ( "hack",
-          let print_tparam (tparam, ty) =
-            Printf.sprintf "  %s = %s;" tparam (Tast_env.print_ty env ty)
-          in
-          let printed_this =
-            Option.map this ~f:(fun ty -> print_tparam ("this", ty))
-          in
-          let printed_tparams =
-            SMap.elements subst |> List.rev_map ~f:print_tparam
-          in
-          let printed_tparams =
-            match printed_this with
-            | None -> printed_tparams
-            | Some printed_this -> printed_this :: printed_tparams
-          in
-          String.concat ~sep:"\n" printed_tparams )
+      let print_tparam (tparam, ty) =
+        Printf.sprintf "  %s = %s;" tparam (Tast_env.print_ty env ty)
+      in
+      let printed_this =
+        Option.map this ~f:(fun ty -> print_tparam ("this", ty))
+      in
+      let printed_tparams =
+        SMap.elements subst |> List.rev_map ~f:print_tparam
+      in
+      let printed_tparams =
+        match printed_this with
+        | None -> printed_tparams
+        | Some printed_this -> printed_this :: printed_tparams
+      in
+      String.concat ~sep:"\n" printed_tparams |> make_hack_marked_code
     in
     Some [header; section]
 
 (** Make the "Defined in" section of the hover card, which indicates where a member
     is defined *)
 let make_defined_in_section def_opt (tparams : Typing_defs.decl_tparam list) =
-  match def_opt with
-  | Some def ->
-    let tparams =
-      match tparams with
-      | [] -> ""
-      | _ ->
-        "<"
-        ^ (String.concat ~sep:", "
-          @@ List.map tparams ~f:(fun param -> snd param.Typing_defs.tp_name))
-        ^ ">"
-    in
-    Printf.sprintf
-      "// Defined in %s%s\n"
-      (split_class_name @@ SymbolDefinition.full_name def)
-      tparams
-  | None -> ""
+  let open Option.Let_syntax in
+  let+ def = def_opt in
+  let tparams =
+    match tparams with
+    | [] -> ""
+    | _ ->
+      "<"
+      ^ (String.concat ~sep:", "
+        @@ List.map tparams ~f:(fun param -> snd param.Typing_defs.tp_name))
+      ^ ">"
+  in
+  Printf.sprintf
+    "Defined in `%s%s`"
+    (split_class_name @@ SymbolDefinition.full_name def)
+    tparams
 
 (** Return the hover card section for the uninstantiated signature and the
     hover card section showing the instantiation.
@@ -595,12 +592,13 @@ let make_hover_info
   let improved_hover =
     (Provider_context.get_tcopt ctx).GlobalOptions.improved_hover
   in
-  let (snippet, instantiation_section) =
+  let (defined_in, snippet, instantiation_section) =
     match (type_, info_opt) with
-    | (_, None) -> (Utils.strip_hh_lib_ns name, None)
+    | (_, None) -> (None, Utils.strip_hh_lib_ns name, None)
     | (SymbolOccurrence.BestEffortArgument (recv, i), _) ->
       let param_name = nth_param ctx recv i in
-      ( Printf.sprintf "Parameter: %s" (Option.value ~default:"$_" param_name),
+      ( None,
+        Printf.sprintf "Parameter: %s" (Option.value ~default:"$_" param_name),
         None )
     | (SymbolOccurrence.Method _, Some info)
     | (SymbolOccurrence.ClassConst _, Some info)
@@ -612,10 +610,12 @@ let make_hover_info
             class_tparams )
         | _ -> ((print_locl_ty_with_identity info, None), [])
       in
-      ( make_defined_in_section def_opt class_tparams ^ snippet,
+      ( make_defined_in_section def_opt class_tparams,
+        snippet,
         instantiation_section )
     | (SymbolOccurrence.GConst, Some info) ->
-      ( (match make_hover_const_definition entry def_opt with
+      ( None,
+        (match make_hover_const_definition entry def_opt with
         | Some def_txt -> def_txt
         | None -> print_locl_ty_with_identity info),
         None )
@@ -627,13 +627,14 @@ let make_hover_info
         | _ ->
           (print_locl_ty_with_identity ~do_not_strip_dynamic:true info, None)
       in
-      (fun_defined_in def_opt ^ snippet, instantiation_section)
+      (make_fun_defined_in_section def_opt, snippet, instantiation_section)
     | ( SymbolOccurrence.(
           ( Class _ | Module | Typeconst _ | Attribute _ | EnumClassLabel _
           | Keyword _ | BuiltInType _ | LocalVar | TypeVar | XhpLiteralAttr _
           | HhFixme | HhIgnore | PureFunctionContext )),
         Some info ) ->
-      ( print_locl_ty_with_identity ~do_not_strip_dynamic:true info
+      ( None,
+        print_locl_ty_with_identity ~do_not_strip_dynamic:true info
         ^ under_dynamic_result,
         None )
   in
@@ -654,13 +655,19 @@ let make_hover_info
     | _ -> make_hover_doc_block ctx entry occurrence def_opt
   in
   let addendum = List.map addendum ~f:(fun s -> Lsp.MarkedString s) in
-  let addendum =
+  let main_section =
     match instantiation_section with
-    | None -> addendum
-    | Some instantiation_section ->
-      (match addendum with
-      | [] -> instantiation_section
-      | _ :: _ -> instantiation_section @ (Lsp.MarkedString "---" :: addendum))
+    | None -> []
+    | Some instantiation_section -> instantiation_section :: []
+  in
+  let main_section = [make_hack_marked_code snippet] :: main_section in
+  let main_section =
+    match defined_in with
+    | None -> main_section
+    | Some defined_in -> [Lsp.MarkedString defined_in] :: main_section
+  in
+  let snippet =
+    List.intersperse main_section ~sep:[Lsp.MarkedString "---"] |> List.concat
   in
   HoverService.{ snippet; addendum; pos = Some occurrence.SymbolOccurrence.pos }
 
@@ -771,7 +778,11 @@ let go_quarantined
     in
     [
       {
-        snippet = Tast_env.print_ty env ty ^ under_dynamic_result;
+        snippet =
+          [
+            make_hack_marked_code
+              (Tast_env.print_ty env ty ^ under_dynamic_result);
+          ];
         addendum;
         pos = None;
       };
@@ -798,8 +809,11 @@ let go_quarantined
         [
           {
             snippet =
-              Tast_env.print_ty (ServerInferType.get_env info) ty
-              ^ under_dynamic_result;
+              [
+                make_hack_marked_code
+                  (Tast_env.print_ty (ServerInferType.get_env info) ty
+                  ^ under_dynamic_result);
+              ];
             addendum = [];
             pos = None;
           };
@@ -811,7 +825,10 @@ let go_quarantined
       | Some param_name ->
         [
           {
-            snippet = Printf.sprintf "Parameter: %s" param_name;
+            snippet =
+              [
+                make_hack_marked_code (Printf.sprintf "Parameter: %s" param_name);
+              ];
             addendum = [];
             pos = None;
           };

hphp/hack/src/client_and_server/hoverService.ml

@@ -6,27 +6,42 @@
  * LICENSE file in the "hack" directory of this source tree.
  *
  *)
+open Hh_prelude
 
 type hover_info = {
-  snippet: string;
-      (** For fields and locals, this is the type. For method and function calls, it
-          is the signature, including any inferred types for generics.
-          This is also the only part of the hover info to get displayed as code
-          rather than Markdown. *)
+  snippet: Lsp.markedString list;
   addendum: Lsp.markedString list;
-      (** Additional information, such as doc string and declaration file. Displayed
-          as Markdown. *)
   pos: Pos.t option;  (** Position of this result. *)
 }
 [@@deriving eq]
 
 type result = hover_info list
 
+let info_as_marked_string_list { snippet; addendum; pos = _ } =
+  match snippet with
+  | [] ->
+    (* Hack server uses None to indicate absence of a result.
+       We're also catching the non-result [] just in case... *)
+    []
+  | _ :: _ ->
+    let addendum =
+      if List.is_empty addendum then
+        addendum
+      else
+        Lsp.MarkedString "---" :: addendum
+    in
+    snippet @ addendum
+
+let as_marked_string_list (l : result) =
+  List.map l ~f:info_as_marked_string_list
+  |> List.intersperse ~sep:[Lsp.MarkedString "---"]
+  |> List.concat
+
 let string_of_result { snippet; addendum; pos } =
   Printf.sprintf
-    "{ snippet = %S; addendum = [%s]; pos = %s }"
-    snippet
-    (String.concat "; " (List.map Lsp.show_markedString addendum))
+    "{ snippet = [%s]; addendum = [%s]; pos = %s }"
+    (String.concat ~sep:"; " (List.map ~f:Lsp.show_markedString snippet))
+    (String.concat ~sep:"; " (List.map ~f:Lsp.show_markedString addendum))
     (match pos with
     | None -> "None"
     | Some p -> Printf.sprintf "Some %S" (Pos.multiline_string_no_file p))

hphp/hack/src/hh_single_type_check.ml

@@ -1786,23 +1786,14 @@ let do_hover ~ctx ~filename oc (pos_given : File_content.Position.t option) =
   in
   let results = Ide_hover.go_quarantined ~ctx ~entry pos in
   let formatted_results =
-    List.map
-      ~f:(fun r ->
-        let open HoverService in
-        String.concat
-          ~sep:"\n"
-          (r.snippet
-          :: List.map
-               ~f:(function
-                 | Lsp.MarkedCode (_, s) -> s
-                 | Lsp.MarkedString s -> s)
-               r.addendum))
-      results
+    HoverService.as_marked_string_list results
+    |> List.map ~f:(fun (r : Lsp.markedString) ->
+           match r with
+           | Lsp.MarkedCode (_, s) -> s
+           | Lsp.MarkedString s -> s)
+    |> String.concat ~sep:"\n"
   in
-  Printf.fprintf
-    oc
-    "%s\n"
-    (String.concat ~sep:"\n-------------\n" formatted_results)
+  Printf.fprintf oc "%s\n" formatted_results
 
 let handle_mode
     mode

hphp/hack/test/hover/abstract_class.php.exp

@@ -1,4 +1,5 @@
 abstract
+---
 An `abstract` class can only contain `static` methods and `abstract` instance methods.
 
 `abstract` classes cannot be instantiated directly. You can only use `new` on child classes that aren't `abstract`.

hphp/hack/test/hover/abstract_method.php.exp

@@ -1,2 +1,3 @@
 abstract
+---
 An `abstract` method has a signature but no body. Child classes must provide an implementation.

hphp/hack/test/hover/ancestor_docblock.php.exp

@@ -1,5 +1,7 @@
-// Defined in C
+Defined in `C`
+---
 public function foo(): int
+---
 
 This is a docblock 2
 

hphp/hack/test/hover/ancestor_docblock2.php.exp

@@ -1,5 +1,7 @@
-// Defined in C
+Defined in `C`
+---
 public function foo(): int
+---
 
 This is a docblock 2
 

hphp/hack/test/hover/ancestor_docblock3.php.exp

@@ -1,5 +1,7 @@
-// Defined in C
+Defined in `C`
+---
 public function foo(): int
+---
 
 This is a docblock 2
 

hphp/hack/test/hover/ancestor_docblock4.php.exp

@@ -1,5 +1,7 @@
-// Defined in C
+Defined in `C`
+---
 public function foo(): int
+---
 
 This is a docblock 2
 

hphp/hack/test/hover/async_block.php.exp

@@ -1,4 +1,5 @@
 Awaitable<int>
+---
 An `async` block is syntactic sugar for an `async` lambda that is immediately called.
 
 ```

hphp/hack/test/hover/async_keyword.php.exp

@@ -1,4 +1,5 @@
 async
+---
 An `async` function can use `await` to get results from other `async` functions. You may still return plain values, e.g. `return 1;` is permitted in an `Awaitable<int>` function.
 
 This does not give you threads. Only one function is running at any point in time. Instead, the runtime may switch to another function at an `await` expression, and come back to this function later.