Construct as completion suggestions by ulugbekna · Pull Request #472 · ocaml/ocaml-lsp

ulugbekna · 2021-07-07T11:26:07Z

Implements support for merlin Construct.

The main reservation that I have so far is that we're not flexible enough about the depth parameter for Construct. How can we fix this?

From our discussion with voodoos: Local param for Construct isn't ready yet, and we should specify depth param later, when it's more efficient.

To discuss: Maybe it makes more sense to replace _s with snippets, eg calling construct in _ arg1 arg2 will result in (fun _ _ -> _) arg1 arg2 and the cursor will jump to the third underscore (typed hole), while the user likely wants to change the function arguments first.

Otherwise, I used the implementation myself. It seems quite good to me. Demo (the demo used Construct with param set to `Local to use local values, which we turned off now)

Screen.Recording.2021-07-06.at.23.13.40.mov

rgrinberg

Thanks for your efforts. I did an initial review and the PR should be good to go after some minor fixes.

ocaml-lsp-server/src/code_actions/action_construct.ml

ocaml-lsp-server/src/compl.ml

ocaml-lsp-server/test/e2e/__tests__/textDocument-completion.test.ts

ocaml-lsp-server/src/code_actions/action_construct.ml

ocaml-lsp-server/src/ocaml_lsp_server.ml

ocaml-lsp-server/src/code_actions/action_construct.ml

ocaml-lsp-server/src/compl.ml

rgrinberg · 2021-07-10T04:24:17Z

Maybe it makes more sense to replace _s with snippets, eg calling construct in _ arg1 arg2 will result in (fun _ _ -> _) arg1 arg2 and the cursor will jump to the third underscore (typed hole), while the user likely wants to change the function arguments first.

I would wait until it is possible to insert snippets with code actions before exploring this. It would be very confusing UI if the editor only offers snippets only via completions.

* we don't need to pair completion entries with a fixed range * use more self-explanatory names for variables * add comments to where it's not obvious why we're doing this * make newlines more consistent

(AFAIU, await in an async function is useless if we don't await that function itself)

…aram of request) does not have `data` field. We return the completion item itself if the `data` field is missing. We need this for the "merlin construct as completions" feature

…ode action `Construct an expression`

…ct code

* add comments to explain the code action

syntactically or in AST form

especially vscode-specific commands

* do not put two separate task in merlin thread; do requests in one

ulugbekna · 2021-07-13T13:17:59Z

I hope that trying to make two merlin dispatches at once (fcd625b) is worth the complexity. But I tried to make the code modular, readable, and maintainable.

rgrinberg

I hope that trying to make two merlin dispatches at once (fcd625b) is worth the complexity. But I tried to make the code modular, readable, and maintainable.

It's definitely worth it.

The new code looks much better. Some more minor tweaks and it should be good to go.

ocaml-lsp-server/src/client.ml

ocaml-lsp-server/src/compl.ml

ocaml-lsp-server/test/e2e/__tests__/textDocument-completion.test.ts

ocaml-lsp-server/src/ocaml_lsp_server.ml

in the completion suggestions

ocaml-lsp-server/src/compl.ml

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

rgrinberg

Pushed a few tweaks.

Looks good to go.

CHANGES: ## Features - Add a new code action `Add missing rec keyword`, which is available when adding a `rec` keyword can fix `Unbound value ...` error, e.g., ```ocaml let fact n = if n = 0 then 1 else n * fact (n - 1) (* ^^^^ Unbound value fact *) ``` Adding `rec` to the definition of `fact` will fix the problem. The new code action offers adding `rec`. - Jump to the first hole on calling `Destruct` code action (only with client VSCode OCaml Platform) (ocaml/ocaml-lsp#468) Example: when a user invokes `Destruct` code action on `Some 1`, this code is replaced by `match Some 1 with None -> _ | Some _ -> _`, where the 1st and 3rd underscores are "typed holes", a concept created by Merlin to be able to put "holes" in OCaml code. With this change, now for VSCode OCaml Platform users, on such invocation of `Destruct`, the cursor will jump to the first typed hole and select it, so that user can start editing right away. - Use ocamlformat to properly format type snippets. This feature requires the `ocamlformat-rpc` opam package to be installed. (ocaml/ocaml-lsp#386) - Add completion support for polymorphic variants, when it is possible to pin down the precise type. Examples (`<|>` stands for the cursor) when completion will work (ocaml/ocaml-lsp#473) Function application: ``` let foo (a: [`Alpha | `Beta]) = () foo `A<|> ``` Type explicitly shown: ``` let a : [`Alpha | `Beta] = `B<|> ``` Note: this is actually a bug fix, since we were ignoring the backtick when constructing the prefix for completion. - Parse merlin errors (best effort) into a more structured form. This allows reporting all locations as "related information" (ocaml/ocaml-lsp#475) - Add support for Merlin `Construct` command as completion suggestions, i.e., show complex expressions that could complete the typed hole. (ocaml/ocaml-lsp#472) - Add a code action `Construct an expression` that is shown when the cursor is at the end of the typed hole, i.e., `_|`, where `|` is the cursor. The code action simply triggers the client (currently only VS Code is supported) to show completion suggestions. (ocaml/ocaml-lsp#472) - Change the formatting-on-save error notification to a warning notification (ocaml/ocaml-lsp#472) - Code action to qualify ("put module name in identifiers") and unqualify ("remove module name from identifiers") module names in identifiers (ocaml/ocaml-lsp#399) Starting from: ```ocaml open Unix let times = Unix.times () let f x = x.Unix.tms_stime, x.Unix.tms_utime ``` Calling "remove module name from identifiers" with the cursor on the open statement will produce: ```ocaml open Unix let times = times () let f x = x.tms_stime, x.tms_utime ``` Calling "put module name in identifiers" will restore: ```ocaml open Unix let times = Unix.times () let f x = x.Unix.tms_stime, x.Unix.tms_utime ``` ## Fixes - Do not show "random" documentation on hover - fixed by [merlin#1364](ocaml/merlin#1364) - fixes duplicate: - [ocaml-lsp#344](ocaml/ocaml-lsp#344) - [vscode-ocaml-platform#111](ocamllabs/vscode-ocaml-platform#111) - Correctly rename a variable used as a named/optional argument (ocaml/ocaml-lsp#478) - When reporting an error at the beginning of the file, use the first line not the second (ocaml/ocaml-lsp#489)

CHANGES: ## Features - Add a new code action `Add missing rec keyword`, which is available when adding a `rec` keyword can fix `Unbound value ...` error, e.g., ```ocaml let fact n = if n = 0 then 1 else n * fact (n - 1) (* ^^^^ Unbound value fact *) ``` Adding `rec` to the definition of `fact` will fix the problem. The new code action offers adding `rec`. - Use ocamlformat to properly format type snippets. This feature requires the `ocamlformat-rpc` opam package to be installed. (ocaml/ocaml-lsp#386) - Add completion support for polymorphic variants, when it is possible to pin down the precise type. Examples (`<|>` stands for the cursor) when completion will work (ocaml/ocaml-lsp#473) Function application: ``` let foo (a: [`Alpha | `Beta]) = () foo `A<|> ``` Type explicitly shown: ``` let a : [`Alpha | `Beta] = `B<|> ``` Note: this is actually a bug fix, since we were ignoring the backtick when constructing the prefix for completion. - Parse merlin errors (best effort) into a more structured form. This allows reporting all locations as "related information" (ocaml/ocaml-lsp#475) - Add support for Merlin `Construct` command as completion suggestions, i.e., show complex expressions that could complete the typed hole. (ocaml/ocaml-lsp#472) - Add a code action `Construct an expression` that is shown when the cursor is at the end of the typed hole, i.e., `_|`, where `|` is the cursor. The code action simply triggers the client (currently only VS Code is supported) to show completion suggestions. (ocaml/ocaml-lsp#472) - Change the formatting-on-save error notification to a warning notification (ocaml/ocaml-lsp#472) - Code action to qualify ("put module name in identifiers") and unqualify ("remove module name from identifiers") module names in identifiers (ocaml/ocaml-lsp#399) Starting from: ```ocaml open Unix let times = Unix.times () let f x = x.Unix.tms_stime, x.Unix.tms_utime ``` Calling "remove module name from identifiers" with the cursor on the open statement will produce: ```ocaml open Unix let times = times () let f x = x.tms_stime, x.tms_utime ``` Calling "put module name in identifiers" will restore: ```ocaml open Unix let times = Unix.times () let f x = x.Unix.tms_stime, x.Unix.tms_utime ``` ## Fixes - Do not show "random" documentation on hover - fixed by [merlin#1364](ocaml/merlin#1364) - fixes duplicate: - [ocaml-lsp#344](ocaml/ocaml-lsp#344) - [vscode-ocaml-platform#111](ocamllabs/vscode-ocaml-platform#111) - Correctly rename a variable used as a named/optional argument (ocaml/ocaml-lsp#478) - When reporting an error at the beginning of the file, use the first line not the second (ocaml/ocaml-lsp#489)

ulugbekna mentioned this pull request Jul 7, 2021

Better Construct support ocamllabs/vscode-ocaml-platform#660

Merged

ulugbekna requested a review from rgrinberg July 7, 2021 13:16

ulugbekna force-pushed the construct-as-completion branch from 040446a to 11a3aeb Compare July 8, 2021 08:39

rgrinberg requested changes Jul 10, 2021

View reviewed changes

ulugbekna added 19 commits July 13, 2021 18:09

minimize work in merlin thread

4375dc4

simplify compl.ml:

0fe6109

* we don't need to pair completion entries with a fixed range * use more self-explanatory names for variables * add comments to where it's not obvious why we're doing this * make newlines more consistent

fix a bug: await the document to open

07790ed

(AFAIU, await in an async function is useless if we don't await that function itself)

cut down on boilerplate with jsonable interface

62bd1e2

completionItem/resolve: handle case when the completion item (the p…

d399adc

…aram of request) does not have `data` field. We return the completion item itself if the `data` field is missing. We need this for the "merlin construct as completions" feature

move all completion related modules together

2cf3d43

add support for construct in completion suggestions

63586c1

add Construct an expression code action

1d1ccdb

add changes entries for Construct as completion suggestions and a c…

81da42b

…ode action `Construct an expression`

client shouldn't notify if there is no hole to jump to in the constru…

15d52cb

…ct code

use Construct arguments suggested by @voodoos

5d1e985

remove useless binding for params.range

c395027

comment argument fields for ocaml.next-hole

ad4beaa

* minimize work in merlin thread

5e07f5b

* add comments to explain the code action

refactor preselecting the first item

8cf2a41

simplify code

cf68e81

factor out code that deals with typed holes

db1f76d

syntactically or in AST form

factor out code to handle client commands,

b2c0a5e

especially vscode-specific commands

* make code more modular

fcd625b

* do not put two separate task in merlin thread; do requests in one

ulugbekna force-pushed the construct-as-completion branch from 036c29e to fcd625b Compare July 13, 2021 13:15

rgrinberg requested changes Jul 13, 2021

View reviewed changes

ulugbekna added 3 commits July 14, 2021 12:53

make hole completion test less noisy

a0227d6

add an interface for client.ml

618618d

add a changes entry about changing formatting error to a warning

ea19194

ulugbekna added 3 commits July 14, 2021 19:54

make notify-if-no-hole mandatory

13277cc

let completion fail if completion by prefix fails

6b5e17f

show constructed expressions deparenthesized

7824b7d

in the completion suggestions

rgrinberg reviewed Jul 14, 2021

View reviewed changes

ocaml-lsp-server/src/compl.ml Outdated Show resolved Hide resolved

Simplify conditional with is_suffix/is_prefix

acbd501

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

rgrinberg approved these changes Jul 14, 2021

View reviewed changes

_

6f9a240

rgrinberg force-pushed the construct-as-completion branch from 600e4ec to 6f9a240 Compare July 14, 2021 18:07

ulugbekna merged commit a12fae4 into ocaml:master Jul 15, 2021

ulugbekna deleted the construct-as-completion branch July 15, 2021 06:47

rgrinberg mentioned this pull request Aug 19, 2021

[new release] ocaml-lsp-server, lsp and jsonrpc (1.8.0) ocaml/opam-repository#19286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Construct as completion suggestions#472

Construct as completion suggestions#472
ulugbekna merged 27 commits intoocaml:masterfrom
ulugbekna:construct-as-completion

ulugbekna commented Jul 7, 2021 •

edited

Loading

Uh oh!

rgrinberg left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgrinberg commented Jul 10, 2021

Uh oh!

ulugbekna commented Jul 13, 2021

Uh oh!

rgrinberg left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgrinberg left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ulugbekna commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgrinberg commented Jul 10, 2021

Uh oh!

ulugbekna commented Jul 13, 2021

Uh oh!

rgrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ulugbekna commented Jul 7, 2021 •

edited

Loading