Improve generation of function template variable names

CHANGELOG.md

@@ -15,6 +15,7 @@
 #### :rocket: New Feature
 
 - Docstring template Code Action. https://github.com/rescript-lang/rescript-vscode/pull/764
+- Improve unlabelled argument names in completion function templates. https://github.com/rescript-lang/rescript-vscode/pull/754
 
 ## 1.16.0
 

analysis/src/CompletionBackEnd.ml

@@ -1342,26 +1342,13 @@ let rec completeTypedValue ~full ~prefix ~completionContext ~mode
       ]
     else []
   | Tfunction {env; typ; args} when prefix = "" && mode = Expression ->
-    let prettyPrintArgTyp ?currentIndex (argTyp : Types.type_expr) =
-      let indexText =
-        match currentIndex with
-        | None -> ""
-        | Some i -> string_of_int i
-      in
-      match argTyp |> TypeUtils.pathFromTypeExpr with
-      | None -> "v" ^ indexText
-      | Some p -> (
-        (* Pretty print a few common patterns. *)
-        match Path.head p |> Ident.name with
-        | "unit" -> "()"
-        | "ReactEvent" | "JsxEvent" -> "event"
-        | _ -> "v" ^ indexText)
-    in
     let mkFnArgs ~asSnippet =
       match args with
       | [(Nolabel, argTyp)] when TypeUtils.typeIsUnit argTyp -> "()"
       | [(Nolabel, argTyp)] ->
-        let varName = prettyPrintArgTyp argTyp in
+        let varName =
+          CompletionExpressions.prettyPrintFnTemplateArgName ~env ~full argTyp
+        in
         if asSnippet then "${1:" ^ varName ^ "}" else varName
       | _ ->
         let currentUnlabelledIndex = ref 0 in
@@ -1376,7 +1363,10 @@ let rec completeTypedValue ~full ~prefix ~completionContext ~mode
                    else (
                      currentUnlabelledIndex := !currentUnlabelledIndex + 1;
                      let num = !currentUnlabelledIndex in
-                     let varName = prettyPrintArgTyp typ ~currentIndex:num in
+                     let varName =
+                       CompletionExpressions.prettyPrintFnTemplateArgName
+                         ~currentIndex:num ~env ~full typ
+                     in
                      if asSnippet then
                        "${" ^ string_of_int num ^ ":" ^ varName ^ "}"
                      else varName))

analysis/src/CompletionExpressions.ml

@@ -216,3 +216,42 @@ and traverseExprTupleItems tupleItems ~nextExprPath ~resultFromFoundItemNum ~pos
            if pos >= Loc.start e.Parsetree.pexp_loc then posNum := index);
     if !posNum > -1 then Some ("", resultFromFoundItemNum !posNum) else None
   | v, _ -> v
+
+let prettyPrintFnTemplateArgName ?currentIndex ~env ~full
+    (argTyp : Types.type_expr) =
+  let indexText =
+    match currentIndex with
+    | None -> ""
+    | Some i -> string_of_int i
+  in
+  let defaultVarName = "v" ^ indexText in
+  let argTyp, suffix, _env =
+    TypeUtils.digToRelevantTemplateNameType ~env ~package:full.package argTyp
+  in
+  match argTyp |> TypeUtils.pathFromTypeExpr with
+  | None -> defaultVarName
+  | Some p -> (
+    let trailingElementsOfPath =
+      p |> Utils.expandPath |> List.rev |> Utils.lastElements
+    in
+    match trailingElementsOfPath with
+    | [] | ["t"] -> defaultVarName
+    | ["unit"] -> "()"
+    (* Special treatment for JsxEvent, since that's a common enough thing
+       used in event handlers. *)
+    | ["JsxEvent"; "synthetic"] -> "event"
+    | ["synthetic"] -> "event"
+    (* Ignore `t` types, and go for its module name instead. *)
+    | [someName; "t"] | [_; someName] | [someName] -> (
+      match someName with
+      | "string" | "int" | "float" | "array" | "option" | "bool" ->
+        defaultVarName
+      | someName when String.length someName < 30 ->
+        if someName = "synthetic" then
+          Printf.printf "synthetic! %s\n"
+            (trailingElementsOfPath |> SharedTypes.ident);
+        (* We cap how long the name can be, so we don't end up with super
+           long type names. *)
+        (someName |> Utils.lowercaseFirstChar) ^ suffix
+      | _ -> defaultVarName)
+    | _ -> defaultVarName)

analysis/src/ProcessCmt.ml

@@ -65,6 +65,7 @@ let rec forTypeSignatureItem ~(env : SharedTypes.Env.t) ~(exported : Exported.t)
         ~item:
           {
             Type.decl;
+            attributes = type_attributes;
             name = name.txt;
             kind =
               (match type_kind with
@@ -172,6 +173,7 @@ let forTypeDeclaration ~env ~(exported : Exported.t)
       ~item:
         {
           Type.decl = typ_type;
+          attributes = typ_attributes;
           name = name.txt;
           kind =
             (match typ_kind with

analysis/src/SharedTypes.ml

@@ -59,7 +59,12 @@ module Type = struct
     | Record of field list
     | Variant of Constructor.t list
 
-  type t = {kind: kind; decl: Types.type_declaration; name: string}
+  type t = {
+    kind: kind;
+    decl: Types.type_declaration;
+    name: string;
+    attributes: Parsetree.attributes;
+  }
 end
 
 module Exported = struct

analysis/src/TypeUtils.ml

@@ -202,6 +202,22 @@ let pathFromTypeExpr (t : Types.type_expr) =
     Some path
   | _ -> None
 
+let rec digToRelevantTemplateNameType ~env ~package ?(suffix = "")
+    (t : Types.type_expr) =
+  match t.desc with
+  | Tlink t1 | Tsubst t1 | Tpoly (t1, []) ->
+    digToRelevantTemplateNameType ~suffix ~env ~package t1
+  | Tconstr (Path.Pident {name = "option"}, [t1], _) ->
+    digToRelevantTemplateNameType ~suffix ~env ~package t1
+  | Tconstr (Path.Pident {name = "array"}, [t1], _) ->
+    digToRelevantTemplateNameType ~suffix:"s" ~env ~package t1
+  | Tconstr (path, _, _) -> (
+    match References.digConstructor ~env ~package path with
+    | Some (env, {item = {decl = {type_manifest = Some typ}}}) ->
+      digToRelevantTemplateNameType ~suffix ~env ~package typ
+    | _ -> (t, suffix, env))
+  | _ -> (t, suffix, env)
+
 let rec resolveTypeForPipeCompletion ~env ~package ~lhsLoc ~full
     (t : Types.type_expr) =
   let builtin =

analysis/src/Utils.ml

@@ -204,3 +204,12 @@ module Option = struct
     | None -> None
     | Some v -> f v
 end
+
+let rec lastElements list =
+  match list with
+  | ([_; _] | [_] | []) as res -> res
+  | _ :: tl -> lastElements tl
+
+let lowercaseFirstChar s =
+  if String.length s = 0 then s
+  else String.mapi (fun i c -> if i = 0 then Char.lowercase_ascii c else c) s

analysis/tests/src/CompletionExpressions.res

@@ -196,3 +196,57 @@ ignore(fff)
 
 // fff.someOpt
 //            ^com
+
+type someTyp = {test: bool}
+
+let takesCb = cb => {
+  cb({test: true})
+}
+
+// takesCb()
+//         ^com
+
+module Environment = {
+  type t = {hello: bool}
+}
+
+let takesCb2 = cb => {
+  cb({Environment.hello: true})
+}
+
+// takesCb2()
+//          ^com
+
+type apiCallResult = {hi: bool}
+
+let takesCb3 = cb => {
+  cb({hi: true})
+}
+
+// takesCb3()
+//          ^com
+
+let takesCb4 = cb => {
+  cb(Some({hi: true}))
+}
+
+// takesCb4()
+//          ^com
+
+let takesCb5 = cb => {
+  cb([Some({hi: true})])
+}
+
+// takesCb5()
+//          ^com
+
+module RecordSourceSelectorProxy = {
+  type t
+}
+
+@val
+external commitLocalUpdate: (~updater: RecordSourceSelectorProxy.t => unit) => unit =
+  "commitLocalUpdate"
+
+// commitLocalUpdate(~updater=)
+//                            ^com

analysis/tests/src/expected/CompletionExpressions.res.txt

@@ -845,13 +845,13 @@ ContextPath CArgument Value[fnTakingCallback]($3)
 ContextPath Value[fnTakingCallback]
 Path fnTakingCallback
 [{
-    "label": "(~on, ~off=?, v1) => {}",
+    "label": "(~on, ~off=?, variant) => {}",
     "kind": 12,
     "tags": [],
     "detail": "(~on: bool, ~off: bool=?, variant) => int",
     "documentation": null,
     "sortText": "A",
-    "insertText": "(~on, ~off=?, ${1:v1}) => {$0}",
+    "insertText": "(~on, ~off=?, ${1:variant}) => {$0}",
     "insertTextFormat": 2
   }]
 
@@ -951,3 +951,123 @@ Path fff
     "documentation": null
   }]
 
+Complete src/CompletionExpressions.res 205:11
+posCursor:[205:11] posNoWhite:[205:10] Found expr:[205:3->205:12]
+Pexp_apply ...[205:3->205:10] (...[205:11->205:12])
+Completable: Cexpression CArgument Value[takesCb]($0)
+Package opens Pervasives.JsxModules.place holder
+Resolved opens 1 pervasives
+ContextPath CArgument Value[takesCb]($0)
+ContextPath Value[takesCb]
+Path takesCb
+[{
+    "label": "someTyp => {}",
+    "kind": 12,
+    "tags": [],
+    "detail": "someTyp => 'a",
+    "documentation": null,
+    "sortText": "A",
+    "insertText": "${1:someTyp} => {$0}",
+    "insertTextFormat": 2
+  }]
+
+Complete src/CompletionExpressions.res 216:12
+posCursor:[216:12] posNoWhite:[216:11] Found expr:[216:3->216:13]
+Pexp_apply ...[216:3->216:11] (...[216:12->216:13])
+Completable: Cexpression CArgument Value[takesCb2]($0)
+Package opens Pervasives.JsxModules.place holder
+Resolved opens 1 pervasives
+ContextPath CArgument Value[takesCb2]($0)
+ContextPath Value[takesCb2]
+Path takesCb2
+[{
+    "label": "environment => {}",
+    "kind": 12,
+    "tags": [],
+    "detail": "Environment.t => 'a",
+    "documentation": null,
+    "sortText": "A",
+    "insertText": "${1:environment} => {$0}",
+    "insertTextFormat": 2
+  }]
+
+Complete src/CompletionExpressions.res 225:12
+posCursor:[225:12] posNoWhite:[225:11] Found expr:[225:3->225:13]
+Pexp_apply ...[225:3->225:11] (...[225:12->225:13])
+Completable: Cexpression CArgument Value[takesCb3]($0)
+Package opens Pervasives.JsxModules.place holder
+Resolved opens 1 pervasives
+ContextPath CArgument Value[takesCb3]($0)
+ContextPath Value[takesCb3]
+Path takesCb3
+[{
+    "label": "apiCallResult => {}",
+    "kind": 12,
+    "tags": [],
+    "detail": "apiCallResult => 'a",
+    "documentation": null,
+    "sortText": "A",
+    "insertText": "${1:apiCallResult} => {$0}",
+    "insertTextFormat": 2
+  }]
+
+Complete src/CompletionExpressions.res 232:12
+posCursor:[232:12] posNoWhite:[232:11] Found expr:[232:3->232:13]
+Pexp_apply ...[232:3->232:11] (...[232:12->232:13])
+Completable: Cexpression CArgument Value[takesCb4]($0)
+Package opens Pervasives.JsxModules.place holder
+Resolved opens 1 pervasives
+ContextPath CArgument Value[takesCb4]($0)
+ContextPath Value[takesCb4]
+Path takesCb4
+[{
+    "label": "apiCallResult => {}",
+    "kind": 12,
+    "tags": [],
+    "detail": "option<apiCallResult> => 'a",
+    "documentation": null,
+    "sortText": "A",
+    "insertText": "${1:apiCallResult} => {$0}",
+    "insertTextFormat": 2
+  }]
+
+Complete src/CompletionExpressions.res 239:12
+posCursor:[239:12] posNoWhite:[239:11] Found expr:[239:3->239:13]
+Pexp_apply ...[239:3->239:11] (...[239:12->239:13])
+Completable: Cexpression CArgument Value[takesCb5]($0)
+Package opens Pervasives.JsxModules.place holder
+Resolved opens 1 pervasives
+ContextPath CArgument Value[takesCb5]($0)
+ContextPath Value[takesCb5]
+Path takesCb5
+[{
+    "label": "apiCallResults => {}",
+    "kind": 12,
+    "tags": [],
+    "detail": "array<option<apiCallResult>> => 'a",
+    "documentation": null,
+    "sortText": "A",
+    "insertText": "${1:apiCallResults} => {$0}",
+    "insertTextFormat": 2
+  }]
+
+Complete src/CompletionExpressions.res 250:30
+posCursor:[250:30] posNoWhite:[250:29] Found expr:[250:3->250:31]
+Pexp_apply ...[250:3->250:20] (~updater250:22->250:29=...__ghost__[0:-1->0:-1])
+Completable: Cexpression CArgument Value[commitLocalUpdate](~updater)
+Package opens Pervasives.JsxModules.place holder
+Resolved opens 1 pervasives
+ContextPath CArgument Value[commitLocalUpdate](~updater)
+ContextPath Value[commitLocalUpdate]
+Path commitLocalUpdate
+[{
+    "label": "recordSourceSelectorProxy => {}",
+    "kind": 12,
+    "tags": [],
+    "detail": "RecordSourceSelectorProxy.t => unit",
+    "documentation": null,
+    "sortText": "A",
+    "insertText": "${1:recordSourceSelectorProxy} => {$0}",
+    "insertTextFormat": 2
+  }]
+

analysis/tests/src/expected/CompletionJsxProps.res.txt

@@ -185,13 +185,13 @@ ContextPath CJsxPropValue [div] onMouseEnter
 Path ReactDOM.domProps
 Path Pervasives.JsxDOM.domProps
 [{
-    "label": "v => {}",
+    "label": "event => {}",
     "kind": 12,
     "tags": [],
     "detail": "JsxEventC.Mouse.t => unit",
     "documentation": null,
     "sortText": "A",
-    "insertText": "{${1:v} => {$0}}",
+    "insertText": "{${1:event} => {$0}}",
     "insertTextFormat": 2
   }]