codefix: Fix code actions that return Command[] directly instead of CodeAction[] #3929
Conversation
|
Oh, there actually are tests. Mea culpa. Will look at this later then. |
|
I wrote LSP part for fun, I don't use any LSP myself (only tsserver which is not really LSP) so there might be missing parts. Do you have any sample response? |
autoload/ale/codefix.vim
Outdated
| @@ -201,6 +201,13 @@ function! ale#codefix#ApplyLSPCodeAction(data, item) abort | |||
| \ l:command.arguments, | |||
| \) | |||
|
|
|||
| let l:request_id = ale#lsp#Send(a:data.connection_id, l:message) | |||
| elseif type(a:item.command) == v:t_string | |||
There was a problem hiding this comment.
you must check has_key(a:item, 'command') as well for readability at least. I'm not sure what vimscript does if there is no item in dict a:item.
There was a problem hiding this comment.
Good catch, I'll fix this.
Oh, that was my situation as well with my first code actions PR a year ago, but now I started using https://github.com/python-lsp/python-lsp-server and https://github.com/haskell/haskell-language-server somewhat regularly and wanted to make them work. :-) I'm attaching a sample response from haskell-language-server. I think I've fixed the test and will push a fixup in a minute as well. {
"result": [
{
"title": "Disable \"typed-holes\" warnings",
"command": "859695:hls:fallbackCodeAction",
"arguments": [
{
"fallbackCommand": null,
"fallbackWorkspaceEdit": {
"changes": {
"file:///home/tomi/.xmonad-testing/xmonad-contrib/XMonad/Hooks/StatusBar/PP.hs": [
{
"range": {
"end": {
"character": 0,
"line": 3
},
"start": {
"character": 0,
"line": 3
}
},
"newText": "{-# OPTIONS_GHC -Wno-typed-holes #-}\n"
}
]
}
}
}
]
},
{
"title": "Wingman: Attempt to fill hole",
"command": "859695:hls:fallbackCodeAction",
"arguments": [
{
"fallbackCommand": {
"title": "Wingman: Attempt to fill hole",
"command": "859695:tactics:tacticsAutoCommand",
"arguments": [
{
"tp_range": {
"end": {
"character": 7,
"line": 265
},
"start": {
"character": 6,
"line": 265
}
},
"tp_var_name": "",
"tp_file": "file:///home/tomi/.xmonad-testing/xmonad-contrib/XMonad/Hooks/StatusBar/PP.hs"
}
]
},
"fallbackWorkspaceEdit": null
}
]
},
{
"title": "Wingman: Case split on a",
"command": "859695:hls:fallbackCodeAction",
"arguments": [
{
"fallbackCommand": {
"title": "Wingman: Case split on a",
"command": "859695:tactics:tacticsDestructCommand",
"arguments": [
{
"tp_range": {
"end": {
"character": 7,
"line": 265
},
"start": {
"character": 6,
"line": 265
}
},
"tp_var_name": "a",
"tp_file": "file:///home/tomi/.xmonad-testing/xmonad-contrib/XMonad/Hooks/StatusBar/PP.hs"
}
]
},
"fallbackWorkspaceEdit": null
}
]
},
{
"title": "Wingman: Split all function arguments",
"command": "859695:hls:fallbackCodeAction",
"arguments": [
{
"fallbackCommand": {
"title": "Wingman: Split all function arguments",
"command": "859695:tactics:tacticsDestructAllCommand",
"arguments": [
{
"tp_range": {
"end": {
"character": 7,
"line": 265
},
"start": {
"character": 6,
"line": 265
}
},
"tp_var_name": "",
"tp_file": "file:///home/tomi/.xmonad-testing/xmonad-contrib/XMonad/Hooks/StatusBar/PP.hs"
}
]
},
"fallbackWorkspaceEdit": null
}
]
},
{
"title": "Wingman: Refine hole",
"command": "859695:hls:fallbackCodeAction",
"arguments": [
{
"fallbackCommand": {
"title": "Wingman: Refine hole",
"command": "859695:tactics:tacticsRefineCommand",
"arguments": [
{
"tp_range": {
"end": {
"character": 7,
"line": 265
},
"start": {
"character": 6,
"line": 265
}
},
"tp_var_name": "",
"tp_file": "file:///home/tomi/.xmonad-testing/xmonad-contrib/XMonad/Hooks/StatusBar/PP.hs"
}
]
},
"fallbackWorkspaceEdit": null
}
]
},
{
"title": "Wingman: Use custom tactic block",
"command": "859695:hls:fallbackCodeAction",
"arguments": [
{
"fallbackCommand": {
"title": "Wingman: Use custom tactic block",
"command": "859695:tactics:tacticsBeginMetaprogramCommand",
"arguments": [
{
"tp_range": {
"end": {
"character": 7,
"line": 265
},
"start": {
"character": 6,
"line": 265
}
},
"tp_var_name": "",
"tp_file": "file:///home/tomi/.xmonad-testing/xmonad-contrib/XMonad/Hooks/StatusBar/PP.hs"
}
]
},
"fallbackWorkspaceEdit": null
}
]
}
],
"id": 6,
"jsonrpc": "2.0"
} |
|
That's awesome. I approve this PR (I don't have rights but it looks good to me). I hope that will help it pass review later. |
…odeAction[] According to https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#textDocument_codeAction, the response to textDocument/codeAction is: (Command | CodeAction)[] | null and the code only handled the case where it was a CodeAction that either specified an edit or a command, but didn't handle a direct Command. Note that the specification also says that both can be specified and then the edit is applied first, then the command. Furthermore, there seems to be some hacky code handling arguments directly, which I suspect is non-standard and only works with a specific LSP server that happens to pass the edits in the arguments unmodified.
liskin
force-pushed
the
code-actions-fix-hls
branch
from
60465ae
to
cbdeb83
Compare
|
Cool, thanks. I squashed the fixup and forcepushed. |
|
@liskin Now you will need to write a blog entry about that (or at least that would be appreciated, I think) 🍻 |
Is this a new policy for PRs here, a joke, or something else entirely? As I said, I'm quite time-constrained these days, and this was just one tiny fix that I made to improve my workflow somewhat. I mean, I could maybe write it, but I really have no idea why I'd be doing it and what the audience is. :-/ |
Joke 😉 I hope people will find out somehow what Ale can do. |
According to
https://microsoft.github.io/language-server-protocol/specifications/specification-3-17/#textDocument_codeAction,
the response to textDocument/codeAction is:
and the code only handled the case where it was a CodeAction that either
specified an edit or a command, but didn't handle a direct Command.
Note that the specification also says that both can be specified and
then the edit is applied first, then the command. Furthermore, there
seems to be some hacky code handling arguments directly, which I suspect
is non-standard and only works with a specific LSP server that happens
to pass the edits in the arguments unmodified.
There were no other tests for this part of ALE, and I haven't had time to add them yet. I'd like to ping @daliusd, who's the original author of this code, to check it and perhaps shed some light on the last paragraph of the commit message, i.e. why the code looks into arguments even though the LSP protocol spec doesn't look like editors are supposed to touch arguments directly. I am not opposed to writing tests, I'm a bit time-constrained these days however, so I'd rather put at least something out there now.