This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2022-01-28
Channels
- # announcements (4)
- # aws (2)
- # babashka (56)
- # beginners (43)
- # calva (70)
- # clj-kondo (25)
- # cljs-dev (29)
- # clojure (103)
- # clojure-dev (9)
- # clojure-europe (55)
- # clojure-gamedev (8)
- # clojure-nl (5)
- # clojure-norway (5)
- # clojure-uk (4)
- # clojured (1)
- # clojurescript (56)
- # copenhagen-clojurians (1)
- # core-async (2)
- # cursive (16)
- # datomic (5)
- # deps-new (18)
- # emacs (9)
- # events (3)
- # fulcro (45)
- # graphql (2)
- # gratitude (2)
- # kaocha (6)
- # lambdaisland (8)
- # lsp (72)
- # meander (41)
- # missionary (5)
- # nextjournal (52)
- # off-topic (2)
- # pathom (12)
- # pedestal (2)
- # practicalli (1)
- # re-frame (6)
- # reitit (5)
- # releases (1)
- # reveal (1)
- # specter (3)
- # sql (4)
- # tools-deps (22)
- # vim (8)
- # wasm (1)
- # xtdb (22)
@ericdallo When you have time, can I pick your brains about resolveCodeAction? I’m wondering why clojure-lsp needs it at all
Here’s the mental model I’m forming about code actions… Please correct anything I have wrong here.
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
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.
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.)
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
whats option 3?
yeah, he didn't write 😂 but I'm supposing it's return the edits on the resolve, right?
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
Our codeAction/resolve always returns edits
but if it contained a command
the client would also execute it (after applying the edits
, if any)
ah interesting. thats actually kind of a cool system
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
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
Oh cool. Yeah, I’d definitely be curious to see that
ah now I get it. you still want all 3 options, but always return commands from the third one so codeAction/resolve becomes simpler?
I think I found it: https://github.com/clojure-lsp/clojure-lsp/issues/655
I think the issue is we can't return commands from resolveCodeAction according to the SPEC
@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
this would cause clojure-lsp proccess some code to check if we could apply that command
@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 🙂
To put it in pictures:
Yeah, I took a look at the code and didn't see any issues just returning the commands :thinking_face:
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
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)
That’s what I’m thinking too… Let me elaborate on why I’d like it to work differently…
I have a fix for https://github.com/clojure-lsp/clojure-lsp/issues/722, but…
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.
@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
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.
check especially complex commands like resolve macro or commands that ask for user input
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?
Oh, the fix for #722 also includes an integration test for code actions! I’ll make sure that gets included
👌 I’ll move the integration test over. That should help a bit, although the editors will be the final judges of whether everything works
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.
@U02EMBDU2JU that bodes well for Calva support. In what scenarios are LSP calls replaced with nrepl calls? And why?
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.
yeah, IMO some clojure-lsp features are better to rely like lens, references, definition, mostly things that makes sense to rely on static analysis
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
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 😉
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
@U2B2YS5V0 I was aware of that, but we replaced datalevin with transit, next releases should build
“thread-all” action bug: using thread-all-first
at the | position in (|(:after enrich-interceptor) ctx)
returns (-> ctx (:after enrich-interceptor))
, which is incorrect
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?Hm maybe vim is highlighting the wrong thing for me. Try performing it on the leftmost parentheses.
yeah:
|((:after enrich-interceptor) ctx)
(-> ctx
(:after enrich-interceptor))
seems like vim is sending wrong locationyou can confirm that with client <-> server json logs to see what vim is sending
Hm interesting. I'll poke around, see what's happening.
Wait, that second one is incorrect
To achieve the same result, it should wrap the :after call twice
Otherwise, it will evaluate to (:after ctx enrich-interceptor)
I can open a GitHub issue if you'd like
yeah, this is kind of confusing 😅 , please open a issue with detailed information on how it should behave
Will do!