add support for refactor-open (qualify and unqualify) code action #399

ulugbekna · 2021-04-03T14:42:14Z

Reservations I have:

I'm not sure code action names are easily understandable / friendly to newcomers

Blockers:

Code actions are shown only if the cursor is on open statement, but they can be shown even if the code action won't do anything, e.g.,

open Unix (* both qualify and unqualify code actions will be shown *)
let u = ()

(this is merged into ocaml/merlin#master, we need to rebase vendored merlin) refactor-open qualify qualifies with full paths, e.g., for open Import, it does Dune__exe.Import.foo....

To-do

add tests
add an entry to the README, so that users know about these code actions

rgrinberg · 2021-04-04T03:33:18Z

I don't mind the names. This is one of those things that's easy enough to learn by example.

Code actions are shown only if the cursor is on open statement, but they can be shown even if the code action won't do anything, e.g.,

If the code action won't do anything, the edit it computes should be empty. Can we filter it out then?

ulugbekna · 2021-04-06T05:16:48Z

If the code action won't do anything, the edit it computes should be empty. Can we filter it out then?

Unfortunately, merlin always returns edits, even if those edits when applied do not change the document at all.

(I could check whether edits do anything by applying them before returning the code action, but that seems expensive)

Other notes:

The code action is quite useful for stdlib and in-file modules refactor-open qualify, as is seen in the tests. However, merlin doesn't use short paths when qualifying modules. Example:

open Import
module Bin = Stdune.Bin

let _PATH =
  lazy
    (Bin.parse_path
       (Dune__exe.Import.Option.value ~default:"" (Dune__exe.Import.Unix_env.get Dune__exe.Import.Unix_env.initial "PATH")))

Meanwhile, do you happen to know if that's also the case with refactor-open in merlin's emacs mode?

I will try to fix this anyway.

Do you happen to know what's a good way to do several sequenced queries to merlin (vs ocamlmerlin single)? I need merlin to take into account ".merlin" and all source files (while single query only can take one)?

rgrinberg · 2021-04-10T19:25:59Z

Meanwhile, do you happen to know if that's also the case with refactor-open in merlin's emacs mode?

No idea. I don't use the emacs mode.

Do you happen to know what's a good way to do several sequenced queries to merlin (vs ocamlmerlin single)? I need merlin to take into account ".merlin" and all source files (while single query only can take one)?

Is this for tests or something? We don't have .merlin files anymore.

Let's see if @voodoos can help us out.

ulugbekna · 2021-04-10T20:35:47Z

A due update:

I created PRs to the merlin repo for short-paths and useless edits problems (refactor-open qualify uses short-paths merlin#1313 and refactor-open: don't return useless edits merlin#1314).

I would appreciate your take on whether we should merge this PR as is and wait for upstream merlin to merge fix-PRs, or merge those in ocaml-lsp merlin. Both are fine with me.

Do you happen to know what's a good way to do several sequenced queries to merlin (vs ocamlmerlin single)? I need merlin to take into account ".merlin" and all source files (while single query only can take one)?
Is this for tests or something? We don't have .merlin files anymore.

Yes, I needed to write some tests for merlin, but I figured this out.

rgrinberg · 2021-04-10T21:05:32Z

I would appreciate your take on whether we should merge this PR as is and wait for upstream merlin to merge fix-PRs, or merge those in ocaml-lsp merlin. Both are fine with me.

Do you think this feature is useful to users in its current form?

ulugbekna · 2021-04-13T14:58:23Z

Let's aim to merge this for the release after the next one. I hope by then we'll get a feel whether ocaml/merlin is willing to merge.

ulugbekna · 2021-07-23T13:46:12Z

@rgrinberg I think that this feature has improved quite a bit in merlin and should be useful to ocaml-lsp users as well :-)

I think this PR is ready as soon as we switch to using the latest merlin master (I think you'd prefer to do that yourself? otherwise, I can make a PR).

I included b975615 in this PR as well:

what it does? when we update to the latest merlin, we automatically get the fix for Wrong documentation on hover #344, so I included the changes entry about it and updated the according test
why here? we anyway usually rebase and merge, so including this here should be innocuous; let me know if you'd prefer it in a separate PR

I can squash all but b975615 for cleaner history; lmk if you'd like that.

Other feedback/proposals are also welcome.

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

rgrinberg

The PR looks good but we can't rely on the following special libraries in the tests: unix, str, dynlink.

rgrinberg · 2021-07-24T21:14:07Z

CHANGES.md

@@ -48,6 +48,12 @@

 - Change the formatting-on-save error notification to a warning notification (#472)

+- Bug fix: do not show "random" documentation on hover


Isn't this already in the merlin change list? We usually do not replicate merlin's change list here.

It is already in the merlin changes list, but since this bug has been reported several times in both ocaml-lsp & vscode-ocaml-platform, I thought it is useful for users to see it fixed.

I think that merlin is ocaml-lsp's backend, but the end user shouldn't be lurking in merlin Changes to see what's new in ocaml-lsp -- maybe we could at least add a link to their Changes inside the ocaml-lsp-vendored merlin source.

Also, the change entry in merlin is "locate: reset global state from all entry points (#1364)", which is cryptic, isn't it?

It is cryptic. But that can be mended on merlin's side. I'd prefer to see the links to the corresponding change list entries where the fix was done.

I think that merlin is ocaml-lsp's backend, but the end user shouldn't be lurking in merlin Changes to see what's new in ocaml-lsp -- maybe we could at least add a link to their Changes inside the ocaml-lsp-vendored merlin source.

We could have a policy of replicating only the relevant merlin changes in our repo. The problem is that I often lack context as to what is relevant to lsp, and it is extra work. We can bring it up with the rest of the team to see what they think.

* update the according test

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 force-pushed the refactor-open branch from ccd0c1b to 01e9c4b Compare April 5, 2021 12:59

ulugbekna force-pushed the refactor-open branch from 01e9c4b to b975615 Compare July 23, 2021 13:40

ulugbekna marked this pull request as ready for review July 23, 2021 13:40

ulugbekna linked an issue Jul 23, 2021 that may be closed by this pull request

Wrong documentation on hover #344

Closed

ulugbekna mentioned this pull request Jul 24, 2021

Fix: correctly rename var when it's punned #478

Merged

rgrinberg reviewed Jul 24, 2021

View reviewed changes

ocaml-lsp-server/test/e2e/__tests__/textDocument-codeAction.test.ts Outdated Show resolved Hide resolved

rgrinberg force-pushed the refactor-open branch from b975615 to 0db8009 Compare July 24, 2021 21:11

rgrinberg approved these changes Jul 24, 2021

View reviewed changes

rgrinberg approved these changes Jul 25, 2021

View reviewed changes

ulugbekna added 2 commits July 26, 2021 11:12

add support for refactor-open (qualify and unqualify) code action

a34dcbe

* add changes entry that we don't show random docs anymore

b2aafba

* update the according test

ulugbekna force-pushed the refactor-open branch from 6a49e22 to b2aafba Compare July 26, 2021 06:14

ulugbekna merged commit 07df575 into ocaml:master Jul 26, 2021

ulugbekna deleted the refactor-open branch July 26, 2021 06:30

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

add support for refactor-open (qualify and unqualify) code action #399

add support for refactor-open (qualify and unqualify) code action #399

ulugbekna commented Apr 3, 2021 •

edited

Loading

rgrinberg commented Apr 4, 2021

ulugbekna commented Apr 6, 2021 •

edited

Loading

rgrinberg commented Apr 10, 2021

ulugbekna commented Apr 10, 2021

rgrinberg commented Apr 10, 2021

ulugbekna commented Apr 13, 2021

ulugbekna commented Jul 23, 2021 •

edited

Loading

rgrinberg left a comment

rgrinberg Jul 24, 2021

ulugbekna Jul 25, 2021

ulugbekna Jul 25, 2021 •

edited

Loading

rgrinberg Jul 25, 2021

		@@ -48,6 +48,12 @@

		- Change the formatting-on-save error notification to a warning notification (#472)

		- Bug fix: do not show "random" documentation on hover

add support for refactor-open (qualify and unqualify) code action #399

add support for refactor-open (qualify and unqualify) code action #399

Conversation

ulugbekna commented Apr 3, 2021 • edited Loading

rgrinberg commented Apr 4, 2021

ulugbekna commented Apr 6, 2021 • edited Loading

rgrinberg commented Apr 10, 2021

ulugbekna commented Apr 10, 2021

rgrinberg commented Apr 10, 2021

ulugbekna commented Apr 13, 2021

ulugbekna commented Jul 23, 2021 • edited Loading

rgrinberg left a comment

Choose a reason for hiding this comment

rgrinberg Jul 24, 2021

Choose a reason for hiding this comment

ulugbekna Jul 25, 2021

Choose a reason for hiding this comment

ulugbekna Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

rgrinberg Jul 25, 2021

Choose a reason for hiding this comment

ulugbekna commented Apr 3, 2021 •

edited

Loading

ulugbekna commented Apr 6, 2021 •

edited

Loading

ulugbekna commented Jul 23, 2021 •

edited

Loading

ulugbekna Jul 25, 2021 •

edited

Loading