Skip to content

Commit c3db3bc

Browse files
zthzth
committed
improve precision in signature help by accounting for commas (meaning intent to fill in the next argument)
1 parent 4def439 commit c3db3bc

File tree

4 files changed

+218
-171
lines changed

4 files changed

+218
-171
lines changed

‎analysis/src/CompletionFrontEnd.ml

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,5 @@
11
open SharedTypes
22

3-
let rec skipWhite text i =
4-
if i < 0 then 0
5-
else
6-
match text.[i] with
7-
| ' ' | '\n' | '\r' | '\t' -> skipWhite text (i - 1)
8-
| _ -> i
9-
103
let extractCompletableArgValueInfo exp =
114
match exp.Parsetree.pexp_desc with
125
| Pexp_ident {txt = Lident txt} -> Some txt
@@ -302,7 +295,7 @@ let completePipeChain ~(lhs : Parsetree.expression) =
302295
| _ -> None
303296

304297
let completionWithParser1 ~currentFile ~debug ~offset ~path ~posCursor ~text =
305-
let offsetNoWhite = skipWhite text (offset - 1) in
298+
let offsetNoWhite = Utils.skipWhite text (offset - 1) in
306299
let posNoWhite =
307300
let line, col = posCursor in
308301
(line, max 0 col - offset + offsetNoWhite)

‎analysis/src/SignatureHelp.ml

Lines changed: 199 additions & 152 deletions
Original file line numberDiff line numberDiff line change
@@ -152,159 +152,206 @@ let docsForLabel typeExpr ~file ~package ~supportsMarkdownLinks =
152152
typeString :: typeDefinitions |> String.concat "\n"
153153

154154
let signatureHelp ~path ~pos ~currentFile ~debug =
155-
let posBeforeCursor = Pos.posBeforeCursor pos in
156-
let supportsMarkdownLinks = true in
157-
let foundFunctionApplicationExpr = ref None in
158-
let setFound r =
159-
if !foundFunctionApplicationExpr = None then
160-
foundFunctionApplicationExpr := Some r
161-
in
162-
let searchForArgWithCursor ~isPipeExpr ~args ~exp =
163-
let extractedArgs = extractExpApplyArgs ~args in
164-
let argAtCursor =
165-
let unlabelledArgCount = ref (if isPipeExpr then 1 else 0) in
166-
extractedArgs
167-
|> List.find_map (fun arg ->
168-
match arg.label with
169-
| None ->
170-
let currentUnlabelledArgCount = !unlabelledArgCount in
171-
unlabelledArgCount := currentUnlabelledArgCount + 1;
172-
(* An argument without a label is just the expression, so we can use that. *)
173-
if arg.exp.pexp_loc |> Loc.hasPos ~pos:posBeforeCursor then
174-
Some (Unlabelled currentUnlabelledArgCount)
175-
else None
176-
| Some {name; posStart; posEnd} -> (
177-
(* Check for the label identifier itself having the cursor *)
178-
match
179-
pos |> CursorPosition.classifyPositions ~posStart ~posEnd
180-
with
181-
| HasCursor -> Some (Labelled name)
182-
| NoCursor | EmptyLoc -> (
183-
(* If we're not in the label, check the exp. Either the exp
184-
exists and has the cursor. Or the exp is a parser recovery
185-
node, in which case we assume that the parser recovery
186-
indicates that the cursor was here. *)
187-
match
188-
( arg.exp.pexp_desc,
189-
arg.exp.pexp_loc
190-
|> CursorPosition.classifyLoc ~pos:posBeforeCursor )
191-
with
192-
| Pexp_extension ({txt = "rescript.exprhole"}, _), _
193-
| _, HasCursor ->
194-
Some (Labelled name)
195-
| _ -> None)))
196-
in
197-
setFound (argAtCursor, exp, extractedArgs)
198-
in
199-
let expr (iterator : Ast_iterator.iterator) (expr : Parsetree.expression) =
200-
(match expr with
201-
(* Handle pipes, like someVar->someFunc(... *)
202-
| {
203-
pexp_desc =
204-
Pexp_apply
205-
( {pexp_desc = Pexp_ident {txt = Lident "|."}},
206-
[
207-
_;
208-
( _,
209-
{
210-
pexp_desc =
211-
Pexp_apply (({pexp_desc = Pexp_ident _} as exp), args);
212-
pexp_loc;
213-
} );
214-
] );
215-
}
216-
when pexp_loc
217-
|> CursorPosition.classifyLoc ~pos:posBeforeCursor
218-
== HasCursor ->
219-
searchForArgWithCursor ~isPipeExpr:true ~args ~exp
220-
(* Look for applying idents, like someIdent(...) *)
221-
| {
222-
pexp_desc = Pexp_apply (({pexp_desc = Pexp_ident _} as exp), args);
223-
pexp_loc;
224-
}
225-
when pexp_loc
226-
|> CursorPosition.classifyLoc ~pos:posBeforeCursor
227-
== HasCursor ->
228-
searchForArgWithCursor ~isPipeExpr:false ~args ~exp
229-
| _ -> ());
230-
Ast_iterator.default_iterator.expr iterator expr
231-
in
232-
let iterator = {Ast_iterator.default_iterator with expr} in
233-
let parser = Res_driver.parsingEngine.parseImplementation ~forPrinter:false in
234-
let {Res_driver.parsetree = structure} = parser ~filename:currentFile in
235-
iterator.structure iterator structure |> ignore;
236-
match !foundFunctionApplicationExpr with
237-
| Some (argAtCursor, exp, _extractedArgs) -> (
238-
(* Not looking for the cursor position after this, but rather the target function expression's loc. *)
239-
let pos = exp.pexp_loc |> Loc.end_ in
240-
match findFunctionType ~currentFile ~debug ~path ~pos with
241-
| Some (args, name, docstring, type_expr, package, _env, file) ->
242-
if debug then
243-
Printf.printf "argAtCursor: %s\n"
244-
(match argAtCursor with
245-
| None -> "none"
246-
| Some (Labelled name) -> "~" ^ name
247-
| Some (Unlabelled index) -> "unlabelled<" ^ string_of_int index ^ ">");
248-
249-
(* The LS protocol wants us to send both the full type signature (label) that the end user sees as the signature help, and all parameters in that label
250-
in the form of a list of start/end character offsets. We leverage the parser to figure the offsets out by parsing the label, and extract the
251-
offsets from the parser. *)
155+
let textOpt = Files.readFile currentFile in
156+
match textOpt with
157+
| None | Some "" -> None
158+
| Some text -> (
159+
match Pos.positionToOffset text pos with
160+
| None -> None
161+
| Some offset -> (
162+
let posBeforeCursor = Pos.posBeforeCursor pos in
163+
let offsetNoWhite = Utils.skipWhite text (offset - 1) in
164+
let firstCharBeforeCursorNoWhite =
165+
if offsetNoWhite < String.length text && offsetNoWhite >= 0 then
166+
Some text.[offsetNoWhite]
167+
else None
168+
in
169+
let supportsMarkdownLinks = true in
170+
let foundFunctionApplicationExpr = ref None in
171+
let setFound r =
172+
if !foundFunctionApplicationExpr = None then
173+
foundFunctionApplicationExpr := Some r
174+
in
175+
let searchForArgWithCursor ~isPipeExpr ~args ~exp =
176+
let extractedArgs = extractExpApplyArgs ~args in
177+
let argAtCursor =
178+
let firstArgIndex = if isPipeExpr then 1 else 0 in
179+
let unlabelledArgCount = ref firstArgIndex in
180+
let lastUnlabelledArgBeforeCursor = ref firstArgIndex in
181+
let argAtCursor_ =
182+
extractedArgs
183+
|> List.find_map (fun arg ->
184+
match arg.label with
185+
| None ->
186+
let currentUnlabelledArgCount = !unlabelledArgCount in
187+
unlabelledArgCount := currentUnlabelledArgCount + 1;
188+
(* An argument without a label is just the expression, so we can use that. *)
189+
if arg.exp.pexp_loc |> Loc.hasPos ~pos:posBeforeCursor then
190+
Some (Unlabelled currentUnlabelledArgCount)
191+
else (
192+
(* If this unlabelled arg doesn't have the cursor, record
193+
it as the last seen unlabelled arg before the
194+
cursor.*)
195+
if posBeforeCursor >= (arg.exp.pexp_loc |> Loc.start)
196+
then
197+
lastUnlabelledArgBeforeCursor :=
198+
currentUnlabelledArgCount;
199+
None)
200+
| Some {name; posStart; posEnd} -> (
201+
(* Check for the label identifier itself having the cursor *)
202+
match
203+
pos |> CursorPosition.classifyPositions ~posStart ~posEnd
204+
with
205+
| HasCursor -> Some (Labelled name)
206+
| NoCursor | EmptyLoc -> (
207+
(* If we're not in the label, check the exp. Either the exp
208+
exists and has the cursor. Or the exp is a parser recovery
209+
node, in which case we assume that the parser recovery
210+
indicates that the cursor was here. *)
211+
match
212+
( arg.exp.pexp_desc,
213+
arg.exp.pexp_loc
214+
|> CursorPosition.classifyLoc ~pos:posBeforeCursor )
215+
with
216+
| Pexp_extension ({txt = "rescript.exprhole"}, _), _
217+
| _, HasCursor ->
218+
Some (Labelled name)
219+
| _ -> None)))
220+
in
252221

253-
(* Put together a label here that both makes sense to show to the end user in the signature help, but also can be passed to the parser. *)
254-
let label = "let " ^ name ^ ": " ^ Shared.typeToString type_expr in
255-
let {Res_driver.parsetree = signature} =
256-
Res_driver.parseInterfaceFromSource ~forPrinter:false
257-
~displayFilename:"<missing-file>" ~source:label
222+
match argAtCursor_ with
223+
| None ->
224+
Some
225+
(Unlabelled
226+
(!lastUnlabelledArgBeforeCursor
227+
+
228+
if firstCharBeforeCursorNoWhite = Some ',' then 1
229+
(* If we found no argument with the cursor, we might still be
230+
able to complete for an unlabelled argument, if the char
231+
before the cursor is ',', like: `someFn(123, <com>)`
232+
complete for argument 2, or: `someFn(123, <com>, true)`
233+
complete for argument 2 as well. Adding 1 here accounts
234+
for the comma telling us that the users intent is to fill
235+
in the next argument. *)
236+
else 0))
237+
| v -> v
238+
in
239+
setFound (argAtCursor, exp, extractedArgs)
240+
in
241+
let expr (iterator : Ast_iterator.iterator) (expr : Parsetree.expression)
242+
=
243+
(match expr with
244+
(* Handle pipes, like someVar->someFunc(... *)
245+
| {
246+
pexp_desc =
247+
Pexp_apply
248+
( {pexp_desc = Pexp_ident {txt = Lident "|."}},
249+
[
250+
_;
251+
( _,
252+
{
253+
pexp_desc =
254+
Pexp_apply (({pexp_desc = Pexp_ident _} as exp), args);
255+
pexp_loc;
256+
} );
257+
] );
258+
}
259+
when pexp_loc
260+
|> CursorPosition.classifyLoc ~pos:posBeforeCursor
261+
== HasCursor ->
262+
searchForArgWithCursor ~isPipeExpr:true ~args ~exp
263+
(* Look for applying idents, like someIdent(...) *)
264+
| {
265+
pexp_desc = Pexp_apply (({pexp_desc = Pexp_ident _} as exp), args);
266+
pexp_loc;
267+
}
268+
when pexp_loc
269+
|> CursorPosition.classifyLoc ~pos:posBeforeCursor
270+
== HasCursor ->
271+
searchForArgWithCursor ~isPipeExpr:false ~args ~exp
272+
| _ -> ());
273+
Ast_iterator.default_iterator.expr iterator expr
274+
in
275+
let iterator = {Ast_iterator.default_iterator with expr} in
276+
let parser =
277+
Res_driver.parsingEngine.parseImplementation ~forPrinter:false
258278
in
279+
let {Res_driver.parsetree = structure} = parser ~filename:currentFile in
280+
iterator.structure iterator structure |> ignore;
281+
match !foundFunctionApplicationExpr with
282+
| Some (argAtCursor, exp, _extractedArgs) -> (
283+
(* Not looking for the cursor position after this, but rather the target function expression's loc. *)
284+
let pos = exp.pexp_loc |> Loc.end_ in
285+
match findFunctionType ~currentFile ~debug ~path ~pos with
286+
| Some (args, name, docstring, type_expr, package, _env, file) ->
287+
if debug then
288+
Printf.printf "argAtCursor: %s\n"
289+
(match argAtCursor with
290+
| None -> "none"
291+
| Some (Labelled name) -> "~" ^ name
292+
| Some (Unlabelled index) ->
293+
"unlabelled<" ^ string_of_int index ^ ">");
259294

260-
let parameters = extractParameters ~signature ~label in
261-
if debug then
262-
Printf.printf "extracted params: \n%s\n"
263-
(parameters
264-
|> List.map (fun (_, start, end_) ->
265-
String.sub label start (end_ - start))
266-
|> list);
295+
(* The LS protocol wants us to send both the full type signature (label) that the end user sees as the signature help, and all parameters in that label
296+
in the form of a list of start/end character offsets. We leverage the parser to figure the offsets out by parsing the label, and extract the
297+
offsets from the parser. *)
267298

268-
(* Figure out the active parameter *)
269-
let activeParameter = findActiveParameter ~argAtCursor ~args in
270-
Some
271-
{
272-
Protocol.signatures =
273-
[
274-
{
275-
label;
276-
parameters =
277-
parameters
278-
|> List.map (fun (argLabel, start, end_) ->
279-
{
280-
Protocol.label = (start, end_);
281-
documentation =
282-
(match
283-
args
284-
|> List.find_opt (fun (lbl, _) ->
285-
lbl = argLabel)
286-
with
287-
| None ->
288-
{Protocol.kind = "markdown"; value = "Nope"}
289-
| Some (_, labelTypExpr) ->
290-
{
291-
Protocol.kind = "markdown";
292-
value =
293-
docsForLabel ~supportsMarkdownLinks ~file
294-
~package labelTypExpr;
295-
});
296-
});
297-
documentation =
298-
(match List.nth_opt docstring 0 with
299-
| None -> None
300-
| Some docs -> Some {Protocol.kind = "markdown"; value = docs});
301-
};
302-
];
303-
activeSignature = Some 0;
304-
activeParameter =
305-
(match activeParameter with
306-
| None -> Some (-1)
307-
| activeParameter -> activeParameter);
308-
}
309-
| _ -> None)
310-
| _ -> None
299+
(* Put together a label here that both makes sense to show to the end user in the signature help, but also can be passed to the parser. *)
300+
let label = "let " ^ name ^ ": " ^ Shared.typeToString type_expr in
301+
let {Res_driver.parsetree = signature} =
302+
Res_driver.parseInterfaceFromSource ~forPrinter:false
303+
~displayFilename:"<missing-file>" ~source:label
304+
in
305+
306+
let parameters = extractParameters ~signature ~label in
307+
if debug then
308+
Printf.printf "extracted params: \n%s\n"
309+
(parameters
310+
|> List.map (fun (_, start, end_) ->
311+
String.sub label start (end_ - start))
312+
|> list);
313+
314+
(* Figure out the active parameter *)
315+
let activeParameter = findActiveParameter ~argAtCursor ~args in
316+
Some
317+
{
318+
Protocol.signatures =
319+
[
320+
{
321+
label;
322+
parameters =
323+
parameters
324+
|> List.map (fun (argLabel, start, end_) ->
325+
{
326+
Protocol.label = (start, end_);
327+
documentation =
328+
(match
329+
args
330+
|> List.find_opt (fun (lbl, _) ->
331+
lbl = argLabel)
332+
with
333+
| None ->
334+
{Protocol.kind = "markdown"; value = "Nope"}
335+
| Some (_, labelTypExpr) ->
336+
{
337+
Protocol.kind = "markdown";
338+
value =
339+
docsForLabel ~supportsMarkdownLinks ~file
340+
~package labelTypExpr;
341+
});
342+
});
343+
documentation =
344+
(match List.nth_opt docstring 0 with
345+
| None -> None
346+
| Some docs ->
347+
Some {Protocol.kind = "markdown"; value = docs});
348+
};
349+
];
350+
activeSignature = Some 0;
351+
activeParameter =
352+
(match activeParameter with
353+
| None -> Some (-1)
354+
| activeParameter -> activeParameter);
355+
}
356+
| _ -> None)
357+
| _ -> None))

‎analysis/src/Utils.ml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,3 +152,10 @@ let identifyType type_desc =
152152
| Tunivar _ -> "Tunivar"
153153
| Tpoly _ -> "Tpoly"
154154
| Tpackage _ -> "Tpackage"
155+
156+
let rec skipWhite text i =
157+
if i < 0 then 0
158+
else
159+
match text.[i] with
160+
| ' ' | '\n' | '\r' | '\t' -> skipWhite text (i - 1)
161+
| _ -> i

0 commit comments

Comments
 (0)