Add extract code actions

[jfeser]

Adds two new code actions: Extract local and Extract function.

Extract local transforms:

let f x =  $x+1$ + 2

to

let f x = 
  let new_var = x + 1 in 
  new_var + 2

The new variable gets placed at the tightest enclosing binder of the extracted expression.

Extract function would transform the same code to:

let new_fun x = x + 1
let f x = new_fun x + 2

The new function will be inserted directly before the enclosing structure item. Any free variables in the expression will be passed as arguments.

Just wanted to get opinions on whether this is a useful feature, and whether these behaviors make sense.

TODO:

• Should extract local move the expression to the loosest binder rather than the tightest?
• Extract function doesn't handle free variables with nontrivial Longidents.
• Add tests.

[rgrinberg]

The feature looks useful to me.

Should extract local move the expression to the loosest binder rather than the tightest?

I would expect the tightest. But perhaps you could have two extract actions for the different behaviors.

Or perhaps we could have different refactorings for hoisting bindings up.

The new function will be inserted directly before the enclosing structure item.

I would expect it to stay close to the scope where it's being used personally.

[rgrinberg]

Btw, can you try splitting this code action into a discovery/resolve phase and evaluate if it helps performance a little. I'm getting a little worried about all these code actions clogging up the server.

[ulugbekna]

I am working on a code actions “framework” within olsp that should batch merlin work into a single job and facilitate implementing code action resolve easily, so I would say it’s fine if we don’t have resolve implemented in this PR.

[jfeser]

I have a branch (https://github.com/jfeser/ocaml-lsp/tree/tracing) with some extra tracing. From the traces I've looked at, code actions aren't a performance concern. How have you been evaluating performance/where have you seen performance problems?

[rgrinberg]

If you measured it and it's not a problem then let's leave it be.

I haven't measured anything but I've noticed that lsp gets stuck "thinking" sometimes and then fires off a bunch of responses. I do not know the source of the bottle neck though.

[coveralls]

Pull Request Test Coverage Report for Build 3872

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 99 of 117 (84.62%) changed or added relevant lines in 4 files are covered.
• 821 unchanged lines in 19 files lost coverage.
• Overall coverage increased (+0.05%) to 18.089%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
ocaml-lsp-server/src/document.ml	4	5	80.0%
ocaml-lsp-server/src/code_actions/action_mark_remove_unused.ml	0	5	0.0%
ocaml-lsp-server/src/code_actions/action_extract.ml	91	103	88.35%

Files with Coverage Reduction	New Missed Lines	%
lsp/src/cancel_request.ml	1	75.0%
lsp/src/position.ml	1	75.0%
ocaml-lsp-server/src/code_actions/diagnostic_util.ml	1	50.0%
ocaml-lsp-server/src/custom_requests/req_switch_impl_intf.ml	9	0.0%
ocaml-lsp-server/src/custom_requests/req_infer_intf.ml	13	0.0%
ocaml-lsp-server/src/document_text_command.ml	13	0.0%
lsp/src/array_view.ml	16	46.67%
ocaml-lsp-server/src/merlin_config_command.ml	19	0.0%
ocaml-lsp-server/src/custom_requests/req_typed_holes.ml	23	0.0%
ocaml-lsp-server/src/document_symbol.ml	29	0.0%

Totals
Change from base Build 3867:	0.05%
Covered Lines:	4257
Relevant Lines:	23533

---

💛 - Coveralls

[rgrinberg]

Is it possible the user can encounter this error?

1. If yes, then it should be a Jsonrpc error.
2. if not, it should it be a code error.

[jfeser]

This is part of the test harness. It gets raised when the test input doesn't have $ markers for the selection. Do you want me to use a different exception?

[rgrinberg]

Oh never mind. It's fine.

[3Rafal]

Unit tests for this PR are broken. CI fails on 4/7 checks

[rgrinberg]

@jfeser could you take a look?