construct: Do not produce redundant prefixes (#1618)

from bcc32/omit-module-prefixes

CHANGES.md

@@ -2,6 +2,9 @@ unreleased
 ==========
   + merlin binary
     - Handle concurrent server start (#1622)
+    - Omit module prefixes for constructors and record fields in the
+      `construct` command (#1618).  Prefixes are still produced when
+      warning 42 (disambiguated name) is active.
   + editor modes
     - emacs: call merlin-client-logger with "interrupted" if the
       merlin binary itself is interrupted, not just the parsing of the

src/analysis/construct.ml

@@ -65,6 +65,11 @@ module Util = struct
   let prefix env ~env_check path name =
     to_shortest_lid ~env ~env_check ~name path
 
+  let maybe_prefix env ~env_check path name =
+    match Warnings.is_active (Disambiguated_name "") with
+    | false -> Longident.Lident name
+    | true -> prefix env ~env_check path name
+
   let var_of_id id = Location.mknoloc @@ Ident.name id
 
   let type_to_string t =
@@ -81,6 +86,16 @@ module Util = struct
       Btype.backtrack snap;
       None
 
+  let typeable env exp type_expected =
+    let snap = Btype.snapshot () in
+    let typeable =
+      match Typecore.type_expect env exp (Typecore.mk_expected type_expected) with
+      | (_ : Typedtree.expression) -> true
+      | exception _ -> false
+    in
+    Btype.backtrack snap;
+    typeable
+
   let is_in_stdlib path =
     Path.head path |> Ident.name = "Stdlib"
 
@@ -332,7 +347,8 @@ module Gen = struct
         match Util.unifiable env type_expr ty_res with
         | Some snap ->
           let lid =
-            Util.prefix env ~env_check:Env.find_constructor_by_name
+            Util.maybe_prefix env
+              ~env_check:Env.find_constructor_by_name
               path cstr_descr.cstr_name
             |> Location.mknoloc
           in
@@ -353,10 +369,9 @@ module Gen = struct
 
                 We therefore check that constructed expressions
                 can be typed. *)
-              try
-                Typecore.type_expression env exp |> ignore;
-                Some exp
-              with _ -> None)
+              if Util.typeable env exp type_expr
+              then Some exp
+              else None)
         | None -> []
       in
       List.map constrs ~f:(make_constr env path type_expr)
@@ -403,7 +418,9 @@ module Gen = struct
         let _, arg, res = Ctype.instance_label true lbl in
         Ctype.unify env res typ ;
         let lid =
-          Util.prefix env ~env_check:Env.find_label_by_name path lbl_name
+          Util.maybe_prefix env
+            ~env_check:Env.find_label_by_name
+            path lbl_name
           |> Location.mknoloc
         in
         let exprs = exp_or_hole env arg in
@@ -536,21 +553,27 @@ let to_string_with_parentheses exp =
   in
   Format.asprintf f Pprintast.expression exp
 
-let node ?(depth = 1) ~keywords ~values_scope node =
-  match node with
-  | Browse_raw.Expression { exp_type; exp_env; _ } ->
-      let idents_table = Util.idents_table ~keywords in
-      Gen.expression ~idents_table values_scope ~depth exp_env exp_type
-      |> List.map ~f:to_string_with_parentheses
-  | Browse_raw.Module_expr
-      { mod_desc = Tmod_constraint _ ; mod_type; mod_env; _ }
-  | Browse_raw.Module_expr
-      {  mod_desc = Tmod_apply _; mod_type; mod_env; _ } ->
-      let m = Gen.module_ mod_env mod_type in
-      [ Format.asprintf "%a" Pprintast.module_expr m ]
-  | Browse_raw.Module_expr _
-  | Browse_raw.Module_binding _ ->
-      (* Constructible modules have an explicit constraint or are functor
-        applications. In other cases we do not know what to construct.  *)
-      raise No_constraint
-  | _ -> raise Not_a_hole
+let node ?(depth = 1) ~(config : Mconfig.t) ~keywords ~values_scope node =
+  Warnings.with_state config.ocaml.warnings
+    (fun () ->
+       match node with
+       | Browse_raw.Expression { exp_type; exp_env; _ } ->
+         let idents_table = Util.idents_table ~keywords in
+         Gen.expression ~idents_table values_scope ~depth exp_env exp_type
+         |> List.map ~f:to_string_with_parentheses
+       | Browse_raw.Module_expr
+           { mod_desc = Tmod_constraint _ ; mod_type; mod_env; _ }
+       | Browse_raw.Module_expr
+           {  mod_desc = Tmod_apply _; mod_type; mod_env; _ } ->
+         let m = Gen.module_ mod_env mod_type in
+         [ Format.asprintf "%a" Pprintast.module_expr m ]
+       | Browse_raw.Module_expr _
+       | Browse_raw.Module_binding _ ->
+         (* Constructible modules have an explicit constraint or are functor
+            applications. In other cases we do not know what to construct.
+
+            It is ok to raise here, since Warnings.with_state handles it. *)
+         raise No_constraint
+       | _ ->
+         (* As above, it is ok to raise here. *)
+         raise Not_a_hole)

src/analysis/construct.mli

@@ -5,6 +5,7 @@ type values_scope = Null | Local
 
 val node
   : ?depth : int
+  -> config : Mconfig.t
   -> keywords : string list
   -> values_scope : values_scope
   -> Browse_raw.node

src/frontend/query_commands.ml

@@ -623,6 +623,7 @@ let dispatch pipeline (type a) : a Query_protocol.t -> a =
       | Some `None | None -> Construct.Null
       | Some `Local -> Construct.Local
     in
+    let config = Mpipeline.final_config pipeline in
     let keywords = Mpipeline.reader_lexer_keywords pipeline in
     let typer = Mpipeline.typer_result pipeline in
     let typedtree = Mtyper.get_typedtree typer in
@@ -633,11 +634,11 @@ let dispatch pipeline (type a) : a Query_protocol.t -> a =
     | (_, (Browse_raw.Module_expr { mod_desc = Tmod_hole; _ } as node_for_loc))
       :: (_, node) :: _parents ->
         let loc = Mbrowse.node_loc node_for_loc in
-        (loc, Construct.node ~keywords ?depth ~values_scope node)
+        (loc, Construct.node ~config ~keywords ?depth ~values_scope node)
     | (_,  (Browse_raw.Expression { exp_desc = Texp_hole; _ } as node))
       :: _parents ->
       let loc = Mbrowse.node_loc node in
-      (loc, Construct.node ~keywords ?depth ~values_scope node)
+      (loc, Construct.node ~config ~keywords ?depth ~values_scope node)
     | _ :: _ -> raise Construct.Not_a_hole
     | [] -> raise No_nodes
     end

tests/test-dirs/construct/c-prefix.t

@@ -25,8 +25,8 @@ Test 1.1 :
       }
     },
     [
-      "(Prefix.A _)",
-      "Prefix.B"
+      "(A _)",
+      "B"
     ]
   ]
 
@@ -76,14 +76,14 @@ Test 1.3 :
   $ $MERLIN single construct -position 5:20 -filename c13.ml <c13.ml |
   >  jq ".value[1]"
   [
-    "(Prefix.A _)",
-    "Prefix.B"
+    "(A _)",
+    "B"
   ]
 
   $ $MERLIN single construct -position 6:20 -filename c13.ml <c13.ml |
   >  jq ".value[1]"
   [
-    "{ Prefix.a = _ }"
+    "{ a = _ }"
   ]
 
   $ $MERLIN single construct -position 8:13 -filename c13.ml <c13.ml |
@@ -99,3 +99,30 @@ Test 1.3 :
     "{ a = _ }"
   ]
 
+With warning 42 (disambiguated name) active, prefixes are added:
+
+  $ $MERLIN single construct -position 5:20 -filename c13.ml <c13.ml -w +disambiguated-name |
+  >  jq ".value[1]"
+  [
+    "(Prefix.A _)",
+    "Prefix.B"
+  ]
+
+  $ $MERLIN single construct -position 6:20 -filename c13.ml <c13.ml -w +disambiguated-name |
+  >  jq ".value[1]"
+  [
+    "{ Prefix.a = _ }"
+  ]
+
+  $ $MERLIN single construct -position 8:13 -filename c13.ml <c13.ml -w +disambiguated-name |
+  >  jq ".value[1]"
+  [
+    "(A _)",
+    "B"
+  ]
+
+  $ $MERLIN single construct -position 9:13 -filename c13.ml <c13.ml -w +disambiguated-name |
+  >  jq ".value[1]"
+  [
+    "{ a = _ }"
+  ]