Code actions for marking & removing unused code

[jfeser]

Adds code actions for marking unused code (i.e. by prepending '_' to variable names) and for removing unused code. Currently it can remove unused types, modules, opens, constructors, and variables.

[rgrinberg]

Looks good. A few small tweaks and it should be good to go.

[ulugbekna]

The code actions don't work for me since the error messages have the form (after rebasing to the latest master):

I guess we could search for a substring but pessimistic about such solution's performance. Wdyt?

There are a couple more changes that I have in mind before merging the PR, which I'll add tomorrow hopefully

[jfeser]

This still needs some changes to pick up the right error messages.

How does ocamllsp decide which diagnostics to show? I have a test project with all warnings turned on, but I only see a subset of them in my editor. When I build with dune I see all of the warnings.

[ulugbekna]

I know that ocaml-lsp uses merlin to get the warnings, but I'm not sure how merlin decides which warnings to show -- it may either get configs from dune or have only support for some of the warnings

…

On Tue, 7 Sept 2021 at 18:33, Jack Feser ***@***.***> wrote: This still needs some changes to pick up the right error messages. How does ocamllsp decide which diagnostics to show? I have a test project with all warnings turned on, but I only see a subset of them in my editor. When I build with dune I see all of the warnings. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#502 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4YR62CWBYU3HMMIC6CJLLUAY5FVANCNFSM5DMGP5UQ> .

[jfeser]

My understanding is that merlin gets configs from dune (through dune ocaml-merlin?). So I'm not sure why it's not picking up my dune file config.

[jfeser]

It seems that merlin is in fact seeing my config. However, it doesn't support most of the "unused X" warnings.

Supported: 26, 27
Unsupported: 32-37, 60, 66

Not sure how hard it would be to get support for 32-37 into merlin.

[rgrinberg]

Can you upload the project where you're testing this stuff? AFAIK, merlin doesn't special case any warnings and just reads the warning config from dune.

[jfeser]

https://github.com/jfeser/warnings-test

Merlin doesn't special case warnings, but I think they only hook into certain parts of the compiler (ocaml/merlin#343). They also do some filtering (ocaml/merlin#419).

[rgrinberg]

In that case what you have is fine. Soon, some diagnostics will come directly from the dune build so your code actions will be picked up from there.

[jfeser]

That's good to hear.

@ulugbekna Have another look. The code actions should work now.

[ulugbekna]

Thanks, the code looks much better and works like a charm! :-)

Importantly, I think that these two code actions are activated in a very similar setting & should be combined to avoid duplicate work of filtering diagnostics.

Regarding testing, ideally, we would pass code to ocaml-lsp, get diagnostics for the code, and use those real diagnostics to test the code. Thus, we would catch if the diag messages for warnings 26 and 27 are changed.

I also left some suggestions inline.

[ulugbekna]

We should better handle patterns.

1. We should make sure that mark as unused for fields in a record have a different edit, ie { a; b = _ }. Currently, we propose an edit that results in an error.

We need to inspect the context in which we propose the code action. Ideally, we should do that in the same pattern match as enclosing_value_binding_range.

2. Optionally, removing unused record fields also seems plausible: we should check whether the pattern for the record is closed or not, based on that we can simply remove the field or remove the field and put ; _ after the last field. This doesn't have to happen in this PR though.

3. Optionally, we could also offer removing unused vars from patterns, eg (a, b, c) -> (a, b). More challenging than the rest of course.

---

Importantly, I think we don't need to handle structure_items in enclosing_value_binding_range, because the following will not trigger a diagnostic in merlin as of now:

(* foo.ml *) 
let a = 1 (* [a] is not used, but no warning *)

Only on build we get a warning 32 about unused val declaration.

---

I think there is also a dangling file action_remove_unused.ml or something?

[jfeser]

1. We should make sure that mark as unused for fields in a record have a different edit, ie { a; b = _ }. Currently, we
   propose an edit that results in an error.

Good catch. I think that should be fixed now.

1. Optionally, removing unused record fields also seems plausible: we should check whether the pattern for the record is closed or not, based on that we can simply remove the field or remove the field and put ; _ after the last field. This doesn't have to happen in this PR though.

Yep, this would be a nice addition.

2. Optionally, we could also offer removing unused vars from patterns, eg (a, b, c) -> (a, b). More challenging than the rest of course.

Would this be (a,b,c) -> (a,b,_)?

Importantly, I think we don't need to handle structure_items in enclosing_value_binding_range, because the following will not trigger a diagnostic in merlin as of now:
I think there is also a dangling file action_remove_unused.ml or something?

Yep, removed.

[rgrinberg]

merging to avoid making the author resolve more merge conflicts. if any other improvements are necessary, please continue them in separate PR's.