search

haskell / haskell-language-server

Official haskell ide support via language server (LSP). Successor of ghcide & haskell-ide-engine.
Apache License 2.0
2.65k stars 354 forks source link

Use custom `CodeActionKind`s to support client-side filtering #1458

Open michaelpj opened 3 years ago

michaelpj commented 3 years ago

The CodeActionKind enum is actually an open enumeration of strings. There's nothing stopping us from adding our own kinds.

The main advantage of this is if we want to do some client-side filtering this is a much nicer thing to use than, say, matching on the action title.

We might also want plugins to tell us about custom kinds they support so that we can return the set that we use in CodeActionOptions.

Real-world usecase: https://github.com/emacs-lsp/lsp-haskell/pull/112#discussion_r584291108

michaelpj commented 3 years ago

It also occurred to me that the "Apply all hints" action is rather odd: arguably this should be something the client could do generically for a particular code action kind (if we put hlints in quickfix.hlint or something). Even just "apply all quickfixes" should be something clients can do for us.

pepeiborra commented 3 years ago

VScode can apply all the code actions of the kinds that you select (in configuration), on save. It would be great if we enriched our code actions with kind metadata so that we could, for example, insert all the missing imports on save.

TOTBWF commented 3 years ago

@isovector for the wingman stuff, the following kinds would probably be useful

michaelpj commented 3 years ago

I would suggest nesting them somewhat, maybe something like refactor.wingman.refine or something. That way e.g. VSCode will show them in a "refactoring" menu, and you can filter down to just the wingman actions, and you can filter down to a specific type of wingman action.

isovector commented 3 years ago

Wow, this is a great improvement in wingman. I'd suggest we also add kinds to the import code actions, so that people can only see the imports for their favorite styles.

jneira commented 3 years ago

@michaelpj that is a really great idea, many thanks for the suggestion, will do for hlint asap

jneira commented 3 years ago

Could we made up such document in this pr, adding the code actions with kind informed and which have to be added? Taking as base the starting point in https://github.com/haskell/haskell-language-server/pull/1570#issuecomment-800129646 by @michaelpj

michaelpj commented 2 years ago

TODOS:

andys8 commented 2 years ago

@michaelpj I started looking over code action kinds and their usages. Here my current findings.

Code Action Kinds

image

Defined in https://hackage.haskell.org/package/lsp-types-1.5.0.0/docs/Language-LSP-Types.html

CodeActionQuickFix

Seems to be what is used the most. Used in:

hls-haddock-comments-plugin
hls-class-plugin
hls-qualify-imported-names-plugin
hls-explicit-imports-plugin
hls-pragmas-plugin
hls-hlint-plugin

CodeActionRefactor*

Used in hls-gadt-plugin, hls-retrie-plugin and hls-splice-plugin

CodeActionSource

Source code actions apply to the entire file.

With this explanation I think this could make sense for changes to pragmas

https://github.com/haskell/haskell-language-server/blob/344323be54c95bc7f3dc24908300132bba20a4e8/plugins/hls-pragmas-plugin/src/Ide/Plugin/Pragmas.hs#L73

or disabling an hlint rule for the whole file

https://github.com/haskell/haskell-language-server/blob/344323be54c95bc7f3dc24908300132bba20a4e8/plugins/hls-hlint-plugin/src/Ide/Plugin/Hlint.hs#L455

CodeActionSourceOrganizeImports

Could be interesting for "remove (all) unused imports" (ghcide)

https://github.com/alanz/ghcide/blob/c1678223bbe9ec73628888ef466e0e471258040c/src/Development/IDE/Plugin/CodeAction.hs#L329-L375

CodeActionUnknown

This is currently in use to express details, but if or how the given information is leveraged.

"quickfix.import.refine"
"quickfix.literals.style"
"refactor.wingman.<x>"

https://github.com/alanz/ghcide/blob/c1678223bbe9ec73628888ef466e0e471258040c/src/Development/IDE/Plugin/CodeAction.hs#L1932-L1938

michaelpj commented 2 years ago

Great, thanks!

With this explanation I think this could make sense for changes to pragmas

Honestly, the intended meaning of these kinds is very unclear :/ I wonder if we can look at what other people do to figure out a sensible rubric?

For pragmas, I think it makes sense to keep them as a quickfix since it's an action that appears in response to a diagnostic, i.e. it fixes a compilation error.

or disabling an hlint rule for the whole file

Maybe? Apply all hints might also fit!

This is currently in use to express details, but if or how the given information is leveraged.

I think that's the correct way to do it. The lsp-types model of CodeActionKind doesn't really expose their hierarchical nature, but my understanding is that they are supposed to be hierarchical, so having a "custom" code action kind of quickfix.X.Y is totally legitimate.

If anything, I think we should use this more, and that was what the ticket was originally about. If we can group the code actions we provide nicely using hierarchical kinds, then on the client side it's relatively easy for someone to have e.g. a keybinding to trigger a specific code action or subgroup of code actions (e.g. people have asked for this for the wingman code actions quite a few times).

andys8 commented 2 years ago

I think that's the correct way to do it. The lsp-types model of CodeActionKind doesn't really expose their hierarchical nature, but my understanding is that they are supposed to be hierarchical, so having a "custom" code action kind of quickfix.X.Y is totally legitimate.

Thanks, this is very interesting. Judging from the constructor name I assumed CodeActionUnknown is for .. unknown, non-matching or maybe legacy actions. But looking at the implementation it's actually more of a CodeActionCustom. Not sure about the details, but maybe it's possible (and worth) touching the types. It doesn't look like the term "Unknown" is used in the specification but rather defined in Haskell's lsp-types.

CodeActionSourceOrganizeImports

source.organizeImports is currently not used. There are code actions that modify / fix imports though. It assume that using this kind would lead prominently defined "organize import" shortcuts to work probably in several tools.

E.g. in Vim/Coc this is in the default configuration example in the README but doesn't work for HLS. Not sure if this is also the case in other editors.

image https://github.com/neoclide/coc.nvim

michaelpj commented 2 years ago

I'm revamping lsp-types substantially and it's going to be CodeActionKind_Custom or similar indeed :)

source.organizeImports is currently not used. There are code actions that modify / fix imports though. It assume that using this kind would lead prominently defined "organize import" shortcuts to work probably in several tools.

Well, I think it's not 100% clear what "organizing imports" would entail. As you say, we have several actions that do part of that, but tastes differ. I would expect "organize imports" to do some or all of:

Hard to say, really! At the moment we provide a bunch of such actions, and if you triggered them all with organizeImports then you might e.g. get all your imports made explicit even if you don't want that. Maybe we need a specific organize imports action that is configurable to do what the user wants? Not sure.