Refactor plugin: Prefer code action

[andys8]

_isPreferred can influence the client side order of code actions. The idea is that an unused import is likely to be removed and less likely the warning will be disabled. Therefore actions to remove a single or all redundant imports should be preferred, so that the client can prioritize them higher.

Followup of #3018

Demo

Here is an example in Vim/CoC. There is an import that isn't necessary. The list of code starts with "Disable unused-imports warnings", which is also the option that would be defaulted to. Instead removing the redundant import should be preferred and will influence the order (depends on the language server client).

[georgefst]

When implementing #1235, I did think that the disable-warning actions should probably always be at the bottom of the list, but I couldn't work out how to do that. This looks good!

[andys8]

@georgefst Cool, maybe we could (in a different PR) also set these code actions to isPreferred = Just False to explicitly mark them as not the preferred (default / go to) solution. But looks like there would be a bit of refactoring necessary in order to pass the information along.

https://github.com/haskell/haskell-language-server/pull/1235/files#diff-bf3e1eeb4b7bfd43c7bd24d10063ea6e23c0714d61ab5f5c62e7323ebf1282aaR231

[pepeiborra]

Just to confirm, does this work in Vim/Coc? And have you tested with VSCode?

[andys8]

Honestly, I don't know how all clients behave and I haven't tested in other editors, but I can tell what I know. That being said, the actual effect will likely depend on the client. But these changes add information of what the most reasonable choice of actions is. I think it would also make sense to mark all "disable something" actions as "not preferred", but often seems to need refactoring.

Previous Change

I've tested a similar change in #3018, but this one not again. There are screenshots demonstrating the change in behavior with Vim and CoC.

This Change

Tested this change by building it locally. Seems to show "Remove all redundant imports" before "Disable warning". Not sure why the "Remove import" is at the end, maybe they are created in different places.

Language Server Protocol

LSP docs say:

 * A quick fix should be marked preferred if it properly addresses the
 * underlying error. A refactoring should be marked preferred if it is the
 * most reasonable choice of actions to take.
 *
 * @since 3.15.0

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/

CoC Implementation

Here is the sorting implementation of coc (vim client). It seems to take the property into account.

https://github.com/neoclide/coc.nvim/blob/59fa3972e4395adf1852717011d57fbfa5c169b5/src/handler/codeActions.ts#L64-L70

[andys8]

Rebased ✔️

[michaelpj]

I think it would also make sense to mark all "disable something" actions as "not preferred", but often seems to need refactoring.

I actually don't know if clients treat "no isPreferred field" differently from "isPreferred set to false". The LSP spec of course doesn't tell us anything like usual 🙄

This LGTM, we've used this in the past for hlint and I think it's the right thing to do.

[andys8]

@michaelpj I don't know either. In theory isPreferred?: boolean allows 3 states in TypeScript because of its optionality: true, false, and undefined. But for example the CoC implementation only checks for truthiness with !b.isPreferred. Therefore for CoC's current implementation Just False and Nothing in Haskell should result in the same behavior. But I don't know about other clients and if that's intended or random implementation.

It's a good point though. Adding isPreferred: Just False would be meaningful information, but we could end up not changing actual client behavior.