Skip to content

Commit 7fdcee7

Browse files
zthzth
authored
Improve generation of function template variable names (#754)
* handle @editor.templateVariableName on types * better logic for inferring template variable names * restructure result some * tweak logic some and break out functionality to own function * clearer example * remove attribute for renaming template variable name * changelog
1 parent 7482e1d commit 7fdcee7

10 files changed

+258
-22
lines changed

Diff for: ‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#### :rocket: New Feature
1616

1717
- Docstring template Code Action. https://github.com/rescript-lang/rescript-vscode/pull/764
18+
- Improve unlabelled argument names in completion function templates. https://github.com/rescript-lang/rescript-vscode/pull/754
1819
- Add `Some(fieldName)` case when completing in a pattern with an option on a record field. https://github.com/rescript-lang/rescript-vscode/pull/766
1920

2021
#### :bug: Bug Fix

Diff for: ‎analysis/src/CompletionBackEnd.ml

+7-17
Original file line numberDiff line numberDiff line change
@@ -1355,26 +1355,13 @@ let rec completeTypedValue ~full ~prefix ~completionContext ~mode
13551355
]
13561356
else []
13571357
| Tfunction {env; typ; args} when prefix = "" && mode = Expression ->
1358-
let prettyPrintArgTyp ?currentIndex (argTyp : Types.type_expr) =
1359-
let indexText =
1360-
match currentIndex with
1361-
| None -> ""
1362-
| Some i -> string_of_int i
1363-
in
1364-
match argTyp |> TypeUtils.pathFromTypeExpr with
1365-
| None -> "v" ^ indexText
1366-
| Some p -> (
1367-
(* Pretty print a few common patterns. *)
1368-
match Path.head p |> Ident.name with
1369-
| "unit" -> "()"
1370-
| "ReactEvent" | "JsxEvent" -> "event"
1371-
| _ -> "v" ^ indexText)
1372-
in
13731358
let mkFnArgs ~asSnippet =
13741359
match args with
13751360
| [(Nolabel, argTyp)] when TypeUtils.typeIsUnit argTyp -> "()"
13761361
| [(Nolabel, argTyp)] ->
1377-
let varName = prettyPrintArgTyp argTyp in
1362+
let varName =
1363+
CompletionExpressions.prettyPrintFnTemplateArgName ~env ~full argTyp
1364+
in
13781365
if asSnippet then "${1:" ^ varName ^ "}" else varName
13791366
| _ ->
13801367
let currentUnlabelledIndex = ref 0 in
@@ -1389,7 +1376,10 @@ let rec completeTypedValue ~full ~prefix ~completionContext ~mode
13891376
else (
13901377
currentUnlabelledIndex := !currentUnlabelledIndex + 1;
13911378
let num = !currentUnlabelledIndex in
1392-
let varName = prettyPrintArgTyp typ ~currentIndex:num in
1379+
let varName =
1380+
CompletionExpressions.prettyPrintFnTemplateArgName
1381+
~currentIndex:num ~env ~full typ
1382+
in
13931383
if asSnippet then
13941384
"${" ^ string_of_int num ^ ":" ^ varName ^ "}"
13951385
else varName))

Diff for: ‎analysis/src/CompletionExpressions.ml

+39
Original file line numberDiff line numberDiff line change
@@ -216,3 +216,42 @@ and traverseExprTupleItems tupleItems ~nextExprPath ~resultFromFoundItemNum ~pos
216216
if pos >= Loc.start e.Parsetree.pexp_loc then posNum := index);
217217
if !posNum > -1 then Some ("", resultFromFoundItemNum !posNum) else None
218218
| v, _ -> v
219+
220+
let prettyPrintFnTemplateArgName ?currentIndex ~env ~full
221+
(argTyp : Types.type_expr) =
222+
let indexText =
223+
match currentIndex with
224+
| None -> ""
225+
| Some i -> string_of_int i
226+
in
227+
let defaultVarName = "v" ^ indexText in
228+
let argTyp, suffix, _env =
229+
TypeUtils.digToRelevantTemplateNameType ~env ~package:full.package argTyp
230+
in
231+
match argTyp |> TypeUtils.pathFromTypeExpr with
232+
| None -> defaultVarName
233+
| Some p -> (
234+
let trailingElementsOfPath =
235+
p |> Utils.expandPath |> List.rev |> Utils.lastElements
236+
in
237+
match trailingElementsOfPath with
238+
| [] | ["t"] -> defaultVarName
239+
| ["unit"] -> "()"
240+
(* Special treatment for JsxEvent, since that's a common enough thing
241+
used in event handlers. *)
242+
| ["JsxEvent"; "synthetic"] -> "event"
243+
| ["synthetic"] -> "event"
244+
(* Ignore `t` types, and go for its module name instead. *)
245+
| [someName; "t"] | [_; someName] | [someName] -> (
246+
match someName with
247+
| "string" | "int" | "float" | "array" | "option" | "bool" ->
248+
defaultVarName
249+
| someName when String.length someName < 30 ->
250+
if someName = "synthetic" then
251+
Printf.printf "synthetic! %s\n"
252+
(trailingElementsOfPath |> SharedTypes.ident);
253+
(* We cap how long the name can be, so we don't end up with super
254+
long type names. *)
255+
(someName |> Utils.lowercaseFirstChar) ^ suffix
256+
| _ -> defaultVarName)
257+
| _ -> defaultVarName)

Diff for: ‎analysis/src/ProcessCmt.ml

+2
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ let rec forTypeSignatureItem ~(env : SharedTypes.Env.t) ~(exported : Exported.t)
6565
~item:
6666
{
6767
Type.decl;
68+
attributes = type_attributes;
6869
name = name.txt;
6970
kind =
7071
(match type_kind with
@@ -172,6 +173,7 @@ let forTypeDeclaration ~env ~(exported : Exported.t)
172173
~item:
173174
{
174175
Type.decl = typ_type;
176+
attributes = typ_attributes;
175177
name = name.txt;
176178
kind =
177179
(match typ_kind with

Diff for: ‎analysis/src/SharedTypes.ml

+6-1
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,12 @@ module Type = struct
5959
| Record of field list
6060
| Variant of Constructor.t list
6161

62-
type t = {kind: kind; decl: Types.type_declaration; name: string}
62+
type t = {
63+
kind: kind;
64+
decl: Types.type_declaration;
65+
name: string;
66+
attributes: Parsetree.attributes;
67+
}
6368
end
6469

6570
module Exported = struct

Diff for: ‎analysis/src/TypeUtils.ml

+16
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,22 @@ let pathFromTypeExpr (t : Types.type_expr) =
202202
Some path
203203
| _ -> None
204204

205+
let rec digToRelevantTemplateNameType ~env ~package ?(suffix = "")
206+
(t : Types.type_expr) =
207+
match t.desc with
208+
| Tlink t1 | Tsubst t1 | Tpoly (t1, []) ->
209+
digToRelevantTemplateNameType ~suffix ~env ~package t1
210+
| Tconstr (Path.Pident {name = "option"}, [t1], _) ->
211+
digToRelevantTemplateNameType ~suffix ~env ~package t1
212+
| Tconstr (Path.Pident {name = "array"}, [t1], _) ->
213+
digToRelevantTemplateNameType ~suffix:"s" ~env ~package t1
214+
| Tconstr (path, _, _) -> (
215+
match References.digConstructor ~env ~package path with
216+
| Some (env, {item = {decl = {type_manifest = Some typ}}}) ->
217+
digToRelevantTemplateNameType ~suffix ~env ~package typ
218+
| _ -> (t, suffix, env))
219+
| _ -> (t, suffix, env)
220+
205221
let rec resolveTypeForPipeCompletion ~env ~package ~lhsLoc ~full
206222
(t : Types.type_expr) =
207223
let builtin =

Diff for: ‎analysis/src/Utils.ml

+9
Original file line numberDiff line numberDiff line change
@@ -204,3 +204,12 @@ module Option = struct
204204
| None -> None
205205
| Some v -> f v
206206
end
207+
208+
let rec lastElements list =
209+
match list with
210+
| ([_; _] | [_] | []) as res -> res
211+
| _ :: tl -> lastElements tl
212+
213+
let lowercaseFirstChar s =
214+
if String.length s = 0 then s
215+
else String.mapi (fun i c -> if i = 0 then Char.lowercase_ascii c else c) s

Diff for: ‎analysis/tests/src/CompletionExpressions.res

+54
Original file line numberDiff line numberDiff line change
@@ -196,3 +196,57 @@ ignore(fff)
196196

197197
// fff.someOpt
198198
// ^com
199+
200+
type someTyp = {test: bool}
201+
202+
let takesCb = cb => {
203+
cb({test: true})
204+
}
205+
206+
// takesCb()
207+
// ^com
208+
209+
module Environment = {
210+
type t = {hello: bool}
211+
}
212+
213+
let takesCb2 = cb => {
214+
cb({Environment.hello: true})
215+
}
216+
217+
// takesCb2()
218+
// ^com
219+
220+
type apiCallResult = {hi: bool}
221+
222+
let takesCb3 = cb => {
223+
cb({hi: true})
224+
}
225+
226+
// takesCb3()
227+
// ^com
228+
229+
let takesCb4 = cb => {
230+
cb(Some({hi: true}))
231+
}
232+
233+
// takesCb4()
234+
// ^com
235+
236+
let takesCb5 = cb => {
237+
cb([Some({hi: true})])
238+
}
239+
240+
// takesCb5()
241+
// ^com
242+
243+
module RecordSourceSelectorProxy = {
244+
type t
245+
}
246+
247+
@val
248+
external commitLocalUpdate: (~updater: RecordSourceSelectorProxy.t => unit) => unit =
249+
"commitLocalUpdate"
250+
251+
// commitLocalUpdate(~updater=)
252+
// ^com

Diff for: ‎analysis/tests/src/expected/CompletionExpressions.res.txt

+122-2
Original file line numberDiff line numberDiff line change
@@ -848,13 +848,13 @@ ContextPath CArgument Value[fnTakingCallback]($3)
848848
ContextPath Value[fnTakingCallback]
849849
Path fnTakingCallback
850850
[{
851-
"label": "(~on, ~off=?, v1) => {}",
851+
"label": "(~on, ~off=?, variant) => {}",
852852
"kind": 12,
853853
"tags": [],
854854
"detail": "(~on: bool, ~off: bool=?, variant) => int",
855855
"documentation": null,
856856
"sortText": "A",
857-
"insertText": "(~on, ~off=?, ${1:v1}) => {$0}",
857+
"insertText": "(~on, ~off=?, ${1:variant}) => {$0}",
858858
"insertTextFormat": 2
859859
}]
860860

@@ -954,3 +954,123 @@ Path fff
954954
"documentation": null
955955
}]
956956

957+
Complete src/CompletionExpressions.res 205:11
958+
posCursor:[205:11] posNoWhite:[205:10] Found expr:[205:3->205:12]
959+
Pexp_apply ...[205:3->205:10] (...[205:11->205:12])
960+
Completable: Cexpression CArgument Value[takesCb]($0)
961+
Package opens Pervasives.JsxModules.place holder
962+
Resolved opens 1 pervasives
963+
ContextPath CArgument Value[takesCb]($0)
964+
ContextPath Value[takesCb]
965+
Path takesCb
966+
[{
967+
"label": "someTyp => {}",
968+
"kind": 12,
969+
"tags": [],
970+
"detail": "someTyp => 'a",
971+
"documentation": null,
972+
"sortText": "A",
973+
"insertText": "${1:someTyp} => {$0}",
974+
"insertTextFormat": 2
975+
}]
976+
977+
Complete src/CompletionExpressions.res 216:12
978+
posCursor:[216:12] posNoWhite:[216:11] Found expr:[216:3->216:13]
979+
Pexp_apply ...[216:3->216:11] (...[216:12->216:13])
980+
Completable: Cexpression CArgument Value[takesCb2]($0)
981+
Package opens Pervasives.JsxModules.place holder
982+
Resolved opens 1 pervasives
983+
ContextPath CArgument Value[takesCb2]($0)
984+
ContextPath Value[takesCb2]
985+
Path takesCb2
986+
[{
987+
"label": "environment => {}",
988+
"kind": 12,
989+
"tags": [],
990+
"detail": "Environment.t => 'a",
991+
"documentation": null,
992+
"sortText": "A",
993+
"insertText": "${1:environment} => {$0}",
994+
"insertTextFormat": 2
995+
}]
996+
997+
Complete src/CompletionExpressions.res 225:12
998+
posCursor:[225:12] posNoWhite:[225:11] Found expr:[225:3->225:13]
999+
Pexp_apply ...[225:3->225:11] (...[225:12->225:13])
1000+
Completable: Cexpression CArgument Value[takesCb3]($0)
1001+
Package opens Pervasives.JsxModules.place holder
1002+
Resolved opens 1 pervasives
1003+
ContextPath CArgument Value[takesCb3]($0)
1004+
ContextPath Value[takesCb3]
1005+
Path takesCb3
1006+
[{
1007+
"label": "apiCallResult => {}",
1008+
"kind": 12,
1009+
"tags": [],
1010+
"detail": "apiCallResult => 'a",
1011+
"documentation": null,
1012+
"sortText": "A",
1013+
"insertText": "${1:apiCallResult} => {$0}",
1014+
"insertTextFormat": 2
1015+
}]
1016+
1017+
Complete src/CompletionExpressions.res 232:12
1018+
posCursor:[232:12] posNoWhite:[232:11] Found expr:[232:3->232:13]
1019+
Pexp_apply ...[232:3->232:11] (...[232:12->232:13])
1020+
Completable: Cexpression CArgument Value[takesCb4]($0)
1021+
Package opens Pervasives.JsxModules.place holder
1022+
Resolved opens 1 pervasives
1023+
ContextPath CArgument Value[takesCb4]($0)
1024+
ContextPath Value[takesCb4]
1025+
Path takesCb4
1026+
[{
1027+
"label": "apiCallResult => {}",
1028+
"kind": 12,
1029+
"tags": [],
1030+
"detail": "option<apiCallResult> => 'a",
1031+
"documentation": null,
1032+
"sortText": "A",
1033+
"insertText": "${1:apiCallResult} => {$0}",
1034+
"insertTextFormat": 2
1035+
}]
1036+
1037+
Complete src/CompletionExpressions.res 239:12
1038+
posCursor:[239:12] posNoWhite:[239:11] Found expr:[239:3->239:13]
1039+
Pexp_apply ...[239:3->239:11] (...[239:12->239:13])
1040+
Completable: Cexpression CArgument Value[takesCb5]($0)
1041+
Package opens Pervasives.JsxModules.place holder
1042+
Resolved opens 1 pervasives
1043+
ContextPath CArgument Value[takesCb5]($0)
1044+
ContextPath Value[takesCb5]
1045+
Path takesCb5
1046+
[{
1047+
"label": "apiCallResults => {}",
1048+
"kind": 12,
1049+
"tags": [],
1050+
"detail": "array<option<apiCallResult>> => 'a",
1051+
"documentation": null,
1052+
"sortText": "A",
1053+
"insertText": "${1:apiCallResults} => {$0}",
1054+
"insertTextFormat": 2
1055+
}]
1056+
1057+
Complete src/CompletionExpressions.res 250:30
1058+
posCursor:[250:30] posNoWhite:[250:29] Found expr:[250:3->250:31]
1059+
Pexp_apply ...[250:3->250:20] (~updater250:22->250:29=...__ghost__[0:-1->0:-1])
1060+
Completable: Cexpression CArgument Value[commitLocalUpdate](~updater)
1061+
Package opens Pervasives.JsxModules.place holder
1062+
Resolved opens 1 pervasives
1063+
ContextPath CArgument Value[commitLocalUpdate](~updater)
1064+
ContextPath Value[commitLocalUpdate]
1065+
Path commitLocalUpdate
1066+
[{
1067+
"label": "recordSourceSelectorProxy => {}",
1068+
"kind": 12,
1069+
"tags": [],
1070+
"detail": "RecordSourceSelectorProxy.t => unit",
1071+
"documentation": null,
1072+
"sortText": "A",
1073+
"insertText": "${1:recordSourceSelectorProxy} => {$0}",
1074+
"insertTextFormat": 2
1075+
}]
1076+

Diff for: ‎analysis/tests/src/expected/CompletionJsxProps.res.txt

+2-2
Original file line numberDiff line numberDiff line change
@@ -185,13 +185,13 @@ ContextPath CJsxPropValue [div] onMouseEnter
185185
Path ReactDOM.domProps
186186
Path Pervasives.JsxDOM.domProps
187187
[{
188-
"label": "v => {}",
188+
"label": "event => {}",
189189
"kind": 12,
190190
"tags": [],
191191
"detail": "JsxEventC.Mouse.t => unit",
192192
"documentation": null,
193193
"sortText": "A",
194-
"insertText": "{${1:v} => {$0}}",
194+
"insertText": "{${1:event} => {$0}}",
195195
"insertTextFormat": 2
196196
}]
197197

0 commit comments

Comments
 (0)