Skip to content

Improve signature help precision #675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 5, 2023
Merged

Improve signature help precision #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d60d21c
add failing tests
zth Jan 4, 2023
3a19857
improve precision in signature help by accounting for commas (meaning…
zth Jan 4, 2023
897443a
changelog
zth Jan 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#### :nail_care: Polish

- Prefer opened `Belt` modules in autocomplete when `-open Belt` is detected in `bsconfig`. https://github.com/rescript-lang/rescript-vscode/pull/673
- Improve precision in signature help. You now do not need to type anything into the argument for it to highlight. https://github.com/rescript-lang/rescript-vscode/pull/675

## v1.10.0

Expand Down
9 changes: 1 addition & 8 deletions analysis/src/CompletionFrontEnd.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
open SharedTypes

let rec skipWhite text i =
if i < 0 then 0
else
match text.[i] with
| ' ' | '\n' | '\r' | '\t' -> skipWhite text (i - 1)
| _ -> i

let extractCompletableArgValueInfo exp =
match exp.Parsetree.pexp_desc with
| Pexp_ident {txt = Lident txt} -> Some txt
Expand Down Expand Up @@ -302,7 +295,7 @@ let completePipeChain ~(lhs : Parsetree.expression) =
| _ -> None

let completionWithParser1 ~currentFile ~debug ~offset ~path ~posCursor ~text =
let offsetNoWhite = skipWhite text (offset - 1) in
let offsetNoWhite = Utils.skipWhite text (offset - 1) in
let posNoWhite =
let line, col = posCursor in
(line, max 0 col - offset + offsetNoWhite)
Expand Down
351 changes: 199 additions & 152 deletions analysis/src/SignatureHelp.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,159 +152,206 @@ let docsForLabel typeExpr ~file ~package ~supportsMarkdownLinks =
typeString :: typeDefinitions |> String.concat "\n"

let signatureHelp ~path ~pos ~currentFile ~debug =
let posBeforeCursor = Pos.posBeforeCursor pos in
Copy link
Collaborator Author

@zth zth Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely terrible display of the diff in GitHub. I've added two optional unwrappings which messes with the indentation, which messes up the diff. Haven't really changed that much.

I added comments highlighting the things I've actually changed/added.

let supportsMarkdownLinks = true in
let foundFunctionApplicationExpr = ref None in
let setFound r =
if !foundFunctionApplicationExpr = None then
foundFunctionApplicationExpr := Some r
in
let searchForArgWithCursor ~isPipeExpr ~args ~exp =
let extractedArgs = extractExpApplyArgs ~args in
let argAtCursor =
let unlabelledArgCount = ref (if isPipeExpr then 1 else 0) in
extractedArgs
|> List.find_map (fun arg ->
match arg.label with
| None ->
let currentUnlabelledArgCount = !unlabelledArgCount in
unlabelledArgCount := currentUnlabelledArgCount + 1;
(* An argument without a label is just the expression, so we can use that. *)
if arg.exp.pexp_loc |> Loc.hasPos ~pos:posBeforeCursor then
Some (Unlabelled currentUnlabelledArgCount)
else None
| Some {name; posStart; posEnd} -> (
(* Check for the label identifier itself having the cursor *)
match
pos |> CursorPosition.classifyPositions ~posStart ~posEnd
with
| HasCursor -> Some (Labelled name)
| NoCursor | EmptyLoc -> (
(* If we're not in the label, check the exp. Either the exp
exists and has the cursor. Or the exp is a parser recovery
node, in which case we assume that the parser recovery
indicates that the cursor was here. *)
match
( arg.exp.pexp_desc,
arg.exp.pexp_loc
|> CursorPosition.classifyLoc ~pos:posBeforeCursor )
with
| Pexp_extension ({txt = "rescript.exprhole"}, _), _
| _, HasCursor ->
Some (Labelled name)
| _ -> None)))
in
setFound (argAtCursor, exp, extractedArgs)
in
let expr (iterator : Ast_iterator.iterator) (expr : Parsetree.expression) =
(match expr with
(* Handle pipes, like someVar->someFunc(... *)
| {
pexp_desc =
Pexp_apply
( {pexp_desc = Pexp_ident {txt = Lident "|."}},
[
_;
( _,
{
pexp_desc =
Pexp_apply (({pexp_desc = Pexp_ident _} as exp), args);
pexp_loc;
} );
] );
}
when pexp_loc
|> CursorPosition.classifyLoc ~pos:posBeforeCursor
== HasCursor ->
searchForArgWithCursor ~isPipeExpr:true ~args ~exp
(* Look for applying idents, like someIdent(...) *)
| {
pexp_desc = Pexp_apply (({pexp_desc = Pexp_ident _} as exp), args);
pexp_loc;
}
when pexp_loc
|> CursorPosition.classifyLoc ~pos:posBeforeCursor
== HasCursor ->
searchForArgWithCursor ~isPipeExpr:false ~args ~exp
| _ -> ());
Ast_iterator.default_iterator.expr iterator expr
in
let iterator = {Ast_iterator.default_iterator with expr} in
let parser = Res_driver.parsingEngine.parseImplementation ~forPrinter:false in
let {Res_driver.parsetree = structure} = parser ~filename:currentFile in
iterator.structure iterator structure |> ignore;
match !foundFunctionApplicationExpr with
| Some (argAtCursor, exp, _extractedArgs) -> (
(* Not looking for the cursor position after this, but rather the target function expression's loc. *)
let pos = exp.pexp_loc |> Loc.end_ in
match findFunctionType ~currentFile ~debug ~path ~pos with
| Some (args, name, docstring, type_expr, package, _env, file) ->
if debug then
Printf.printf "argAtCursor: %s\n"
(match argAtCursor with
| None -> "none"
| Some (Labelled name) -> "~" ^ name
| Some (Unlabelled index) -> "unlabelled<" ^ string_of_int index ^ ">");

(* 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
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
offsets from the parser. *)
let textOpt = Files.readFile currentFile in
match textOpt with
| None | Some "" -> None
| Some text -> (
match Pos.positionToOffset text pos with
| None -> None
| Some offset -> (
let posBeforeCursor = Pos.posBeforeCursor pos in
let offsetNoWhite = Utils.skipWhite text (offset - 1) in
let firstCharBeforeCursorNoWhite =
if offsetNoWhite < String.length text && offsetNoWhite >= 0 then
Some text.[offsetNoWhite]
else None
Comment on lines +155 to +167
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this.

in
let supportsMarkdownLinks = true in
let foundFunctionApplicationExpr = ref None in
let setFound r =
if !foundFunctionApplicationExpr = None then
foundFunctionApplicationExpr := Some r
in
let searchForArgWithCursor ~isPipeExpr ~args ~exp =
let extractedArgs = extractExpApplyArgs ~args in
let argAtCursor =
let firstArgIndex = if isPipeExpr then 1 else 0 in
let unlabelledArgCount = ref firstArgIndex in
let lastUnlabelledArgBeforeCursor = ref firstArgIndex in
let argAtCursor_ =
extractedArgs
|> List.find_map (fun arg ->
match arg.label with
| None ->
let currentUnlabelledArgCount = !unlabelledArgCount in
unlabelledArgCount := currentUnlabelledArgCount + 1;
(* An argument without a label is just the expression, so we can use that. *)
if arg.exp.pexp_loc |> Loc.hasPos ~pos:posBeforeCursor then
Some (Unlabelled currentUnlabelledArgCount)
else (
(* If this unlabelled arg doesn't have the cursor, record
it as the last seen unlabelled arg before the
cursor.*)
if posBeforeCursor >= (arg.exp.pexp_loc |> Loc.start)
then
lastUnlabelledArgBeforeCursor :=
currentUnlabelledArgCount;
Comment on lines +192 to +198
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new.

None)
| Some {name; posStart; posEnd} -> (
(* Check for the label identifier itself having the cursor *)
match
pos |> CursorPosition.classifyPositions ~posStart ~posEnd
with
| HasCursor -> Some (Labelled name)
| NoCursor | EmptyLoc -> (
(* If we're not in the label, check the exp. Either the exp
exists and has the cursor. Or the exp is a parser recovery
node, in which case we assume that the parser recovery
indicates that the cursor was here. *)
match
( arg.exp.pexp_desc,
arg.exp.pexp_loc
|> CursorPosition.classifyLoc ~pos:posBeforeCursor )
with
| Pexp_extension ({txt = "rescript.exprhole"}, _), _
| _, HasCursor ->
Some (Labelled name)
| _ -> None)))
in

(* 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. *)
let label = "let " ^ name ^ ": " ^ Shared.typeToString type_expr in
let {Res_driver.parsetree = signature} =
Res_driver.parseInterfaceFromSource ~forPrinter:false
~displayFilename:"<missing-file>" ~source:label
match argAtCursor_ with
| None ->
Some
(Unlabelled
(!lastUnlabelledArgBeforeCursor
+
if firstCharBeforeCursorNoWhite = Some ',' then 1
(* If we found no argument with the cursor, we might still be
able to complete for an unlabelled argument, if the char
before the cursor is ',', like: `someFn(123, <com>)`
complete for argument 2, or: `someFn(123, <com>, true)`
complete for argument 2 as well. Adding 1 here accounts
for the comma telling us that the users intent is to fill
in the next argument. *)
else 0))
| v -> v
Comment on lines +222 to +237
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new.

in
setFound (argAtCursor, exp, extractedArgs)
in
let expr (iterator : Ast_iterator.iterator) (expr : Parsetree.expression)
=
(match expr with
(* Handle pipes, like someVar->someFunc(... *)
| {
pexp_desc =
Pexp_apply
( {pexp_desc = Pexp_ident {txt = Lident "|."}},
[
_;
( _,
{
pexp_desc =
Pexp_apply (({pexp_desc = Pexp_ident _} as exp), args);
pexp_loc;
} );
] );
}
when pexp_loc
|> CursorPosition.classifyLoc ~pos:posBeforeCursor
== HasCursor ->
searchForArgWithCursor ~isPipeExpr:true ~args ~exp
(* Look for applying idents, like someIdent(...) *)
| {
pexp_desc = Pexp_apply (({pexp_desc = Pexp_ident _} as exp), args);
pexp_loc;
}
when pexp_loc
|> CursorPosition.classifyLoc ~pos:posBeforeCursor
== HasCursor ->
searchForArgWithCursor ~isPipeExpr:false ~args ~exp
| _ -> ());
Ast_iterator.default_iterator.expr iterator expr
in
let iterator = {Ast_iterator.default_iterator with expr} in
let parser =
Res_driver.parsingEngine.parseImplementation ~forPrinter:false
in
let {Res_driver.parsetree = structure} = parser ~filename:currentFile in
iterator.structure iterator structure |> ignore;
match !foundFunctionApplicationExpr with
| Some (argAtCursor, exp, _extractedArgs) -> (
(* Not looking for the cursor position after this, but rather the target function expression's loc. *)
let pos = exp.pexp_loc |> Loc.end_ in
match findFunctionType ~currentFile ~debug ~path ~pos with
| Some (args, name, docstring, type_expr, package, _env, file) ->
if debug then
Printf.printf "argAtCursor: %s\n"
(match argAtCursor with
| None -> "none"
| Some (Labelled name) -> "~" ^ name
| Some (Unlabelled index) ->
"unlabelled<" ^ string_of_int index ^ ">");

let parameters = extractParameters ~signature ~label in
if debug then
Printf.printf "extracted params: \n%s\n"
(parameters
|> List.map (fun (_, start, end_) ->
String.sub label start (end_ - start))
|> list);
(* 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
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
offsets from the parser. *)

(* Figure out the active parameter *)
let activeParameter = findActiveParameter ~argAtCursor ~args in
Some
{
Protocol.signatures =
[
{
label;
parameters =
parameters
|> List.map (fun (argLabel, start, end_) ->
{
Protocol.label = (start, end_);
documentation =
(match
args
|> List.find_opt (fun (lbl, _) ->
lbl = argLabel)
with
| None ->
{Protocol.kind = "markdown"; value = "Nope"}
| Some (_, labelTypExpr) ->
{
Protocol.kind = "markdown";
value =
docsForLabel ~supportsMarkdownLinks ~file
~package labelTypExpr;
});
});
documentation =
(match List.nth_opt docstring 0 with
| None -> None
| Some docs -> Some {Protocol.kind = "markdown"; value = docs});
};
];
activeSignature = Some 0;
activeParameter =
(match activeParameter with
| None -> Some (-1)
| activeParameter -> activeParameter);
}
| _ -> None)
| _ -> None
(* 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. *)
let label = "let " ^ name ^ ": " ^ Shared.typeToString type_expr in
let {Res_driver.parsetree = signature} =
Res_driver.parseInterfaceFromSource ~forPrinter:false
~displayFilename:"<missing-file>" ~source:label
in

let parameters = extractParameters ~signature ~label in
if debug then
Printf.printf "extracted params: \n%s\n"
(parameters
|> List.map (fun (_, start, end_) ->
String.sub label start (end_ - start))
|> list);

(* Figure out the active parameter *)
let activeParameter = findActiveParameter ~argAtCursor ~args in
Some
{
Protocol.signatures =
[
{
label;
parameters =
parameters
|> List.map (fun (argLabel, start, end_) ->
{
Protocol.label = (start, end_);
documentation =
(match
args
|> List.find_opt (fun (lbl, _) ->
lbl = argLabel)
with
| None ->
{Protocol.kind = "markdown"; value = "Nope"}
| Some (_, labelTypExpr) ->
{
Protocol.kind = "markdown";
value =
docsForLabel ~supportsMarkdownLinks ~file
~package labelTypExpr;
});
});
documentation =
(match List.nth_opt docstring 0 with
| None -> None
| Some docs ->
Some {Protocol.kind = "markdown"; value = docs});
};
];
activeSignature = Some 0;
activeParameter =
(match activeParameter with
| None -> Some (-1)
| activeParameter -> activeParameter);
}
| _ -> None)
| _ -> None))
7 changes: 7 additions & 0 deletions analysis/src/Utils.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,10 @@ let identifyType type_desc =
| Tunivar _ -> "Tunivar"
| Tpoly _ -> "Tpoly"
| Tpackage _ -> "Tpackage"

let rec skipWhite text i =
if i < 0 then 0
else
match text.[i] with
| ' ' | '\n' | '\r' | '\t' -> skipWhite text (i - 1)
| _ -> i
Loading