Fork me on GitHub
#lsp
<
2022-01-28
>
jacob.maine00:01:10

@ericdallo When you have time, can I pick your brains about resolveCodeAction? I’m wondering why clojure-lsp needs it at all

jacob.maine00:01:35

Here’s the mental model I’m forming about code actions… Please correct anything I have wrong here.

jacob.maine00:01:16

textDocument/codeAction generates the options for the actions menu. It has to be very fast to keep the editor responsive. To facilitate this speed, the CodeAction instances which it returns come in three different flavors

jacob.maine00:01:46

First, if the edit is trivially easy to compute, so easy that it won’t slow down the calculation of which other code actions are available, the CodeAction can contain edits. In this case the client can just apply the edits directly, without having to consult the server again. We never use this mode.

jacob.maine00:01:38

Second, if the edit is too slow to compute, but the server can at least figure out what the command should be, it can return a CodeAction with a command. The client can execute this command as soon as the user selects the action. It still requires another round-trip to the server, but it follows the same code path as workspace/executeCommand. (Spoiler: I think we can and should be using this more often, and using the third option less or not at all.)

ericdallo00:01:01

yes, you are absolutely correct

ericdallo00:01:21

clojure-lsp use to use only 1, then I migrated to use 2 and some months ago to 3 😅

ericdallo00:01:18

I migrated from 2 to 3 because some spec issue that I can't remember but there was a issue about that, affecting especially calva

ericdallo00:01:40

I don't see any issues with 3 though

Lukas Domagala00:01:52

whats option 3?

ericdallo00:01:15

yeah, he didn't write 😂 but I'm supposing it's return the edits on the resolve, right?

jacob.maine00:01:22

Right so, finally the third option is, if both the edit and the command are too slow to compute, but the server can quickly gather just enough information to know that an action is possible, then it can return a CodeAction with just data. The client will have to call codeAction/resolve to get the full CodeAction. That action can contain, edits or a command, or both

jacob.maine00:01:41

Our codeAction/resolve always returns edits but if it contained a command the client would also execute it (after applying the edits, if any)

Lukas Domagala00:01:09

ah interesting. thats actually kind of a cool system

jacob.maine00:01:41

So, I think we could get rid of codeAction/resolve and instead, just returns CodeActions with commands. Then we wouldn’t have two code paths for figuring out edits, one via workspace/executeCommand and one via codeAction/resolve

ericdallo00:01:56

yes, we used to return the command and don't need at all the resolve, but there was a issue regarding that, let me find it

jacob.maine00:01:59

Oh cool. Yeah, I’d definitely be curious to see that

Lukas Domagala00:01:35

ah now I get it. you still want all 3 options, but always return commands from the third one so codeAction/resolve becomes simpler?

ericdallo00:01:20

I think the issue is we can't return commands from resolveCodeAction according to the SPEC

ericdallo00:01:29

vscode crashes it

ericdallo00:01:49

so the way would be returning commands without a resolveCodeAction at all, but:

jacob.maine00:01:04

@U02EMBDU2JU Not quite… I’m saying abandon the third option entirely. We can always (?) calculate commands fast enough to use the 2nd option. Both the 2nd and 3rd option involve an extra call from the client to the server to figure out what the actual edits should be. But in the second option, you’re following the executeCommand code path, which you have to support anyway. The third option, forces you to have a second path to calculate edits

ericdallo00:01:07

this would cause clojure-lsp proccess some code to check if we could apply that command

ericdallo00:01:11

let me double check

jacob.maine00:01:10

@ericdallo that issue seems related, but it’s saying that codeAction/resolve should behave differently (return edits not commands). But I’m saying we shouldn’t need codeAction/resolve at all. If textDocument/codeAction returned a command instead of data then the editors wouldn’t need to call codeAction/resolve they could just invoke the command via workspace/executeCommand . That’s assuming they all know how to do that though 🙂

jacob.maine00:01:38

To put it in pictures:

ericdallo00:01:54

Yeah, I took a look at the code and didn't see any issues just returning the commands :thinking_face:

ericdallo00:01:05

and don't have the resolve at all

jacob.maine00:01:11

Current

open menu -> :textDocument/codeAction
          <- [CodeAction w/ data]

choose action -> codeAction/resolve + data
              <- CodeAction w/edits
Proposed
open menu -> :textDocument/codeAction
          <- [CodeAction w/ command]

choose action -> textDocument/executeCommand
              <- WorkspaceEdit w/edits

ericdallo00:01:57

I suspect there is something I'm not remembering since it was months/year ago that I changed that 😅 but I agree we could try a PR and test on some clients (vscode + emacs should be enough)

jacob.maine01:01:20

That’s what I’m thinking too… Let me elaborate on why I’d like it to work differently…

ericdallo01:01:47

we would remove a lot of duplication indeed, that's already a good argument

🎯 1
Lukas Domagala01:01:58

I’m off to bed, but it’s a very interesting discussion 🙂

😀 1
jacob.maine01:01:26

The way the fix works is that it returns nil (wrapped in a CompletableFuture) from codeAction/resolve. And instead, in the background, it publishes edits and then publishes cursor positioning, which is what executeCommand does too. That fixes the cursor positioning, but it’s a really weird behavior for codeAction/resolve. Just by the name of codeAction/resolve, it should return a CodeAction, not nil with some background job running.

👍 1
ericdallo01:01:14

@U07M2C8TT good finding, feel free to start with a issue and then a optional PR changing that, we will probably find the issue if any during the development / testing on calva/emacs

jacob.maine01:01:14

for some reason it feels OK for that to be the behavior of executeCommand though. That may be wrong too, but if so, I don’t know how else you’re supposed to sequence edits with cursor positions.

ericdallo01:01:31

check especially complex commands like resolve macro or commands that ask for user input

jacob.maine01:01:11

OK, I will. That makes sense. I’ll pay special attention to the complex commands. I don’t use Calva, so I may need someone else to help with that testing. I won’t be surprised if this doesn’t work, but I think it’s worth trying. I also have the fix for #722. As I said, I’m not really thrilled with it, but on the other hand, I’d like the cursor positioning to work. Would you like me to submit it as a PR? Or hold off until I test this other path?

👍 1
jacob.maine01:01:04

Oh, the fix for #722 also includes an integration test for code actions! I’ll make sure that gets included

ericdallo01:01:53

I'd like to check the other path first, if you don't mind

jacob.maine03:01:40

👌 I’ll move the integration test over. That should help a bit, although the editors will be the final judges of whether everything works

Lukas Domagala13:01:58

most of Calvas lsp client is just https://github.com/microsoft/vscode-languageserver-node . There’s some middleware on top, but that’s mostly to replace LSP calls with nrepl calls once there is a connection.

jacob.maine20:01:12

@U02EMBDU2JU that bodes well for Calva support. In what scenarios are LSP calls replaced with nrepl calls? And why?

Lukas Domagala22:01:59

Mostly stuff like get references, completions, find tests. I think it's there because historically nrepl has been more accurate for such things. There's a ticket to make this optional so people can choose the tooling that works best for their use case.

ericdallo22:01:51

yeah, IMO some clojure-lsp features are better to rely like lens, references, definition, mostly things that makes sense to rely on static analysis

jacob.maine22:01:28

Spacemacs has the same issue—with both cider and clojure-lsp dueling for who gets to do what. But I agree @ericdallo, at this point clojure-lsp is as good or better than cider for many things I care about—especially references and go to definition. And it doesn’t need a connected repl. I still use many of the cider refactorings, partially because they’re in my muscle memory, and partially because it sometimes does a nicer job of formatting

☝️ 1
Lukas Domagala01:01:02

Yeah I’d like to get the option into Calva, just so I can try both side by side. I’m never actually using half of LSP because there’s always a running repl. But there are also things nrepl can do that are not in LSP, like “find tests” or “go to java source”. So giving a choice or potentially even merging some of them makes sense to me. Plus conceptually I like the “ask the run-time” lispy approach, just because 😉

ericdallo03:01:27

Yeah, I like the merge approach but hard to make it happen unfortunately

wcohen21:01:59

As a quick note, looks like lsp doesn’t build when graal bumps from 21 to 22.0: https://hydra.nixos.org/build/165609209/nixlog/1

ericdallo21:01:27

@U2B2YS5V0 I was aware of that, but we replaced datalevin with transit, next releases should build

wcohen21:01:47

Got it. Sorry for the bother!

ericdallo21:01:15

no problem! I should release soon next week and I'll open the PR on nixpkgs :)

wcohen21:01:13

Ugh, I don’t know how I missed this note! Thanks again

👍 1
Noah Bogart21:01:34

“thread-all” action bug: using thread-all-first at the | position in (|(:after enrich-interceptor) ctx) returns (-> ctx (:after enrich-interceptor)), which is incorrect

ericdallo22:01:07

that's curious, for me both via command or code action it returns:

(|(:after enrich-interceptor) ctx)
((-> enrich-interceptor
     :after) ctx)
which looks right to me, right?

ericdallo22:01:40

Same result via Thread last all

Noah Bogart22:01:00

Hm maybe vim is highlighting the wrong thing for me. Try performing it on the leftmost parentheses.

ericdallo22:01:05

yeah:

|((:after enrich-interceptor) ctx)
(-> ctx
    (:after enrich-interceptor))
seems like vim is sending wrong location

ericdallo22:01:25

you can confirm that with client <-> server json logs to see what vim is sending

👍 1
Noah Bogart22:01:31

Hm interesting. I'll poke around, see what's happening.

Noah Bogart22:01:59

Wait, that second one is incorrect

Noah Bogart22:01:20

To achieve the same result, it should wrap the :after call twice

ericdallo22:01:48

yeah, you are right

👍 1
Noah Bogart22:01:53

Otherwise, it will evaluate to (:after ctx enrich-interceptor)

Noah Bogart22:01:15

I can open a GitHub issue if you'd like

ericdallo23:01:46

yeah, this is kind of confusing 😅 , please open a issue with detailed information on how it should behave

ericdallo23:01:59

I'm kind of 🤯 testing it 😂