unfortunately, not via code actions since the protocol is a little bit strict on that part, but it's possible via commands or custom commands and I think that would work for all clients, like: • code action call command without args: we infer some name (what we do today) • user manually call command (via Ctrl + p for example on calva): we ask for args

I’m very much an LSP-newbie, but the python LSP seems to be able to do it (see attached). Can’t code actions perform commands? That’s what it seems like clojure-lsp is doing already I think

That's interesting, are e you sure the python extension is using LSP? I will double check the LSP spec but last time I checked didn't work

It’s Pylance, which afaik uses LSP

could you get the LSP client logs between client and server and check what is being returned when you execute the code action?

pylance doesn’t have an option for logging those, is there another way to get them?

usually any LSP has that, maybe it's something on the vscode side?

Got it, here you go

Hum... it's applying the code action and calling thre rename LSP feature right after automatically 🤯 but not sure how since this is triggered by the client (editor)

Maybe it has to do with that didChange notification?

not really, https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_didChange

maybe we could check the server side code when it returns the apply of this command if there is anything mention about the rename request

Fwiw Pylance is both the LSP and the vscode extension, so I wouldn’t put it past them to be doing something that requires coordination between the two

ahh that's probably it

Does this mean this could be done solely in Calva?

yes, like when receive the specific X response of the command, call a rename, but I'd advise against this custom changes which could cause undesired behaviors of specific clients

I’m not sure I follow. Calva’s the client right? So it could listen for extracts, and then immediately trigger a rename

And sure that would couple calva and lsp together, but that’d be a tradeoff to weigh against the dx improvement

I think that change it's ok, but usually it's a good idea avoiding adding custom stuff to a specific client since that won't apply for emacs, vim, intellij, sublime and other editors, also, it's bad for clojure-lsp when each client behaves different, and since it's not in the spec, it's easy for some change happen on clojure-lsp happen that will break calva because it's doing differently

That makes sense. But it sounds like it can’t be done in Clojure-lsp, right?

Paging @U0ETXRFEW now that this is moving into calva-land

without custom methods yeah

Interesting. A feature request on Calva is welcome, @U01EB0V3H39. Let’s investigate it a bit and see what wiggle room we have.

Filed: https://github.com/BetterThanTomorrow/calva/issues/2388

I’ve started looking at a solution for this issue, but I want to run the approach by y’all since it would require coordinated changes with clojure-lsp. As best I can tell, the “magic” that makes Pylance’s auto-rename feature work is that when you trigger one of the appropriate actions, it returns this extra data in the response:

"data": {
  "newSymbolName": "new_var"
}

Thanks for thinking about this! ❤️ 🙏

An alternative approach could be if clojure-lsp, after an extract-ish fix, always places the cursor adjacent to the new-thing symbol. Then we could trigger the refactor-rename command and it would pick up the right symbol. I’m unsure how we can know that the extract is performed, but I recall reading about that somewhere in some vscode issue.

Can you elaborate more on that? The tricky thing is knowing where to put the cursor, there’s nothing in the lsp server response right now that says what the new symbol is. It just has the edits and ranges

Yeah, although we have new custom methods that are not in the LSP spec, I'd suggest avoiding changing the existing ones which could become complex, but maybe we could have pez's idea using the https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument. So, after specific code actions, clojure-lsp could request this to client so user can trigger rename quickly after running a code action, WDYT? needs some testing first

It’s an interesting idea, probably worth thinking through the pros/cons. Here’s my thoughts so far Follow code action with showDocument • ++ doesn’t require extending LSP spec • - client has to wait for a specific request after sending some code actions, could be flaky (what if the second request never arrives? What if another user action happens in between?) • -½ Takes extra work to restore the user’s cursor position after the second command arrives • - not backwards-compatible with existing LSP clients (in that it would cause new and unexpected behavior) (unless it’s behind a capability or something?) Extend code action response • -- requires extending LSP spec, which could become complex ◦ On the other hand , Pylance did it and probably so did many other LSPs if they also have this feature • + backwards-compatible with existing LSP clients (just adding extra keys, they don’t have to read them) • + completely client-driven, i.e. could be handled as a single promise chain (so while there are still concurrency issues, they’re more on the beaten path) • +½ fully decoupled from cursor position My 2c: from purely a software design perspective, the fact that these code actions don’t already return info about the new symbol is a design error. That said, now that it is the way it is, it comes down to the tradeoffs between the two approaches (or coming up with a third?) I would be interested to see more data points from other language plugins. Is everyone doing it like Pylance, and if so, are those additions basically an unofficial part of the spec at this point? I can run python, js, and ts, but when setting "(java|type)script.trace.server": "verbose", I can’t seem to find any logs (I’m guessing b/c it doesn’t actually use an LSP under the hood). Would y’all mind grabbing LSP traces from whatever other languages you have extensions set up for?

> unless it’s behind a capability or something? there is a capability for that, so we would trigger that only if that is true > What if another user action happens in between?) clojure-lsp is usually pretty fast, invoke a show document request after a sucessful command would be pretty fast AFAICS > Pylance did it and probably so did many other LSPs if they also have this feature I only know Pylance doing that, the others are probably not LSP, would be nice to know more about it.

There’s already a capability, or you’re saying you’d add a new one?

there is already https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument a window.showDocument client capability

Oh yeah of course, I was talking about something a little different. Without this change, when a user triggers the action, their cursor is put in some location afterwards (it appears that the location in Calva right now is somewhat arbitrary, but clients could have different behaviors). After this change, all clients get hit with a showDocument right after they finish the action and the user’s cursor gets moved to the new symbol no matter what. If the client doesn’t implement this auto-rename feature, then the cursor is just left there, which might or might not be desirable and definitely might be unexpected. In any case, it’s a un-opt-outable behavior change. I was suggesting that “follow extract actions with showDocument” could be a feature flag.

>> What if another user action happens in between?) > clojure-lsp is usually pretty fast, invoke a show document request after a sucessful command would be pretty fast AFAICS Do you think there’s a risk of races client-side? The client might do stuff async in response to the code action response, the completion of the request doesn’t necessarily mean they’re ready for the showDocument

> which might or might not be desirable and definitely might be unexpected. I I doubt it will be a problem, the auto-rename will not happen anyway via this way, we will just move the cursor to the right place, the user would need to trigger rename anyway

Or calva could automatically trigger it, but sure

> Do you think there’s a risk of races client-side? The way clojure-lsp respond to refactor commands, is to respond to the command request with changes, so it's not async, and it can send a show document sync request right after

Right, I’m talking about races client-side. In the command response callback the client could do something async, which might cause a race with the showDocument request

A lot of vscode APIs are async, so that’s not unheard of

I understand, I don't think it's a problem but that's why I mentioned it'd need some testing to confirm it's ok

curious if Java and Flutter have this kind of feature

Do you have those set up? I don’t

yeah, I have, let me quickly check

It seems flutter has a pylance similar feature via the code action, let me debug the LSP logs

https://github.com/Dart-Code/Dart-Code/blob/37ae5ace42c372c1c65ae7797d892cac21128ee2/src/extension/analysis/analyzer_lsp.ts#L292-L293, same custom client logic 😔

It looks like they’re popping up their own box instead of using the rename action though? And sending that directly from the LSP?

We could register a handler for client.onRequest workspace/applyEdit and examine the edits. If we see that it looks like something that introduces a new symbol, we can place the cursor on it and trigger rename. For something introducing a let, the check may be if there are two edits, if one of them is /(let \[.*new-binding\s/, and the other simply new-binding. Place the cursor at the start of new-binding. Trigger rename. Of course, instead of a regex, we can use Calva’s structural editor to be more precise about the nature of the edit. This would require zero changes in clojure-lsp.

How would you prevent that from triggering when someone pastes code in?

or expands a snippet

That wouldn’t trigger the lsp client.

Well, maybe the clojure-lsp has something with snippets? idk.

yeah, although it won't follow the spec it's probably a ok way to fix the issue. Not sure I understood the problem with someone pasting code

Probably a misunderstanding on my part. I was thinking that if Calva is just sitting around waiting for an applyEdit that looks like it could be a refactor, then it could get false positives

@UKFSJSM38 did you see what Java does? More data points for the fire 😆

Something like that, @U01EB0V3H39. Though, I’m suggesting Calva sitting around waiting for an applyEdit from clojure-lsp.

Or, suggesting… I’m asking us to entertain the idea. 😃 It would be a hack that could potentially false trigger.

POC.

// client.registerFeature(new ExecuteCommandDisablement());
  client.onRequest('workspace/applyEdit', async (params) => {
    const edit: vscode.WorkspaceEdit = await client.protocol2CodeConverter.asWorkspaceEdit(
      params.edit
    );
    return vscode.workspace.applyEdit(edit).then((value) => {
      const range = getNewBindingRange(edit);
      if (range) {
        if (vscode.window.activeTextEditor) {
          void vscode.window.activeTextEditor.revealRange(range);
          vscode.window.activeTextEditor.selection = new vscode.Selection(range.start, range.end);
          void vscode.commands.executeCommand('editor.action.rename');
        }
      }
      return { applied: value };
    });
  });

  void client.start().catch((err) => {
    console.error('Client failed to start', err);
  });

  return {
    id: params.id,
    uri: params.uri,
    client,
    get status() {
      return utils.lspClientStateToStatus(client.state);
    },
  };
};

function getNewBindingRange(edit: vscode.WorkspaceEdit): vscode.Range | null {
  for (const [uri, textEdits] of edit.entries()) {
    for (const textEdit of textEdits) {
      if (textEdit.newText.match(/\bnew-binding\s.*\bnew-binding\b/s)) {
        const index = textEdit.newText.lastIndexOf('new-binding');
        if (index !== -1) {
          const textLines = textEdit.newText.split('\n');
          const lines = textLines.length;
          for (let line = lines - 1; line >= 0; line--) {
            const character = textLines[line].indexOf('new-binding');
            if (character !== -1) {
              const start = textEdit.range.start.translate(
                line,
                character - textEdit.range.start.character
              );
              return new vscode.Range(start, start);
            }
          }
        }
      }
    }
  }
  return null;
}

I should of course have used the structural editor instead, which would let us generalize this to work with extract function too.

1.5 more ideas: • Calva could prompt for the new name before sending the code action, and then pass it to the LSP. I’m not sure if there’s a place to stuff the new name in the protocol, but of course clojure-lsp looking for an extra key and defaulting the value if it’s not present is a backwards-compatible change. • Calva could look for the new binding in the LSP response rather than generally listening to applyEdit. That would remove the risk of false positives, the only remaining source of risk would be in the code for identifying the new binding

good impl pez!

> Calva could prompt for the new name before sending the code action I think it’s good to stick to the refactor-rename command. For example, they are right now running an experiment with CoPilot suggesting names, and things like that won’t happen if we roll our own. > Calva could look for the new binding in the LSP response rather than generally listening to applyEdit. I think it is the only way to listen to the response. I could be wrong, but I think the applyEdit is the response. @UKFSJSM38 may be able to correct me.

As best I can tell, the response to a particular command is in the response body. For example, if you add a .then(x => console.log(x)) https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L57-L71, you see the edit object

So you don’t have to listen for applyEdit generally, you could stick a callback resp -> Promise in specific commands, and it would get called only when those commands are used

The refactor commands already prompt for a binding name, though. It’s the quick-fixes we’re dealing with here. They’re negotiated between the lsp client and server a bit out-of-band. So far I have only found applyEdit to tell me anything about that.

You don’t think those go through that function? In my tests they seemed to

or are you saying there’s no command object to attach the callback to?

Yes, Calva never sends any command for these actions. Afaik.

Interesting, I locally added logging to that region and saw it get hit when I used a quick fix

You may be right about that. I mean if you see it get hit, you’re probably at the right place.

I’ll try modifying the command objects in that file and seeing if they’re making it through or if new ones are coming from the lsp

I tried it now too, and indeed this is the right place! We get both the refactoring command id that is about to be executed and the edits once it done. No guessing needed. Probably some of the code I whipped up there can be used to place the cursor and and trigger the rename. Will you take it from here, @U01EB0V3H39? I can do it if you’re short on time. And it can also wait for you to have time.

I’m happy either way, I probably won’t get to it until next week. 2 thoughts on the poc: Some extract commands return multiple ranges, we’ll want to make sure that’s handled. I’m not sure how I feel about keying it on the “new-binding” name: • that name is different based on which command is used • what if the user already has a binding of that name? Odd but you wouldn’t want it to break • That name isn’t really part of an interface or anything, it’s out-of-band information and therefore fragile. It would be nice if the lsp could tell calva which names to look for.

Pylance does the last bullet with that extra data key in the response, but it seems like y’all don’t love that approach. Are there other ways the LSP could tell calva which names to look for?

We won’t need to hardcode it on new-binding, we get that as an argument to the command.

how would that work without extending the protocol?

Dunno, but I see it here:

Oh, I think I understand! The binding name is an arg to the command, and it’s not in the LSP spec because none of the command args are

so that’s actually great!

Yes, I think we have everything we need to implement this as the default behaviour without worrying for false positives or anything funny.

The binding names are probably specified in clojure-lsp, but they’re round-tripping through calva(?)

I’m surprised there even was a hook for it, but it certainly seems like it is a round-trip. Calva does not configure it anywhere.

yeah, I think the only real edge case is looking for the binding name already existing in the edit range (I don’t know what we’d want to do in that case 🤷)

what are you thinking for identifying which commands to do this on? A set of command ids? I’m assuming that quick fix actions don’t use the command objects defined elsewhere in that file

Yeah, a hardcoded set of commands.

I think there are three. introduce-let, move-to-let, extract-function. But I could have missed others.

also extract-to-def and promote-fn

I’m kind of excited about giving this PR a try, mind if I take a first shot at it? Unless you’re in a hurry to get it in

I’d love if you take a shot at it.

I took a look at this today and ran into an issue: we need to be able to distinguish between commands triggered through quick actions and commands through other means so we don’t trigger the rename for non-quick-action commands. I have some thoughts on that, but first, while I was investigating how that might be done, I made an interesting discovery: The caller of sendCommandRequest when triggered through a quick action is https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L115! The only place that fn is called https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L244, where it maps over clojureLspCommands, and that means that the list of quick refactors available doesn’t appear to come solely from clojure-lsp. This opens up some more design options: rather than looking at the command in sendCommandRequest and trying to reconstruct the command, we could take an approach similar to the userspace refactor commands and add https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L207. Then we also don’t need a list of special commands that need extra work done when they complete floating around. The ideas I have for moving forward fall in two broad categories • Same behavior for userspace and quick action commands ◦ Currently registerUserspaceLspCommand looks at extraParamFn and registerInternalLspCommand does not. They could simply be made to do the same thing ▪︎ I tried this out, and the UI prompt pops up fine, but the refactor LSP request fails with an internal error ◦ Have both types of commands fire off a rename symbol command after the extract refactor succeeds • Different behavior for userspace and quick action commands ◦ Pass a little extra flag of some kind in registerInternal… and/or registerUserspace… that sendCommandRequest can use to decide whether to do a rename symbol afterwards ▪︎ This seems like the hackiest and least-nice option to me ◦ Add an extra fn property to the relevant command objects similar to extraParamFn that runs after the command completes. That fn is tacked onto the promise chain in the handler. Thoughts?

One other interesting discovery: both promote-fn and extract-to-def also introduce new names but don’t have an extraParamFn, so I tried to trigger them via the command palette to see what would happen. Interestingly, https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L232, they don’t appear in the command palette at all! So the only way to trigger them is via quick actions?

Hey y’all, just wanted to check back in on this. I think my favored approach would be to have the quick actions do the same thing the palette commands already do and prompt for the new name, but I could use some help figuring out where that internal LSP error is coming from.

I definitely want the quick actions to invoke the rename command. So that we can benefit from whatever VS Code has for that feature.

So do you want to change the behavior of the existing non-quick-action commands to match that? Or do you want them to have different behavior? My instincts go against the latter

Also, I’m curious what additional benefits you see coming out of the rename command, vs having the LSP return code with the new names already in it (as it does right now for normal commands)

I saw something somewhere about CoPilot suggesting the name.

Huh ok, that’s fair

We can take care of the command versions later.

I guess there’s also an argument to be made for just doing what all the other language extensions are doing if there’s not strong reasons to deviate

I think it might be easier to do them all in one go, since they both come from the same data

The crux with the commands is that they are part of both clojure-lsp’s and Calva’s API. So not all that easy to change.

Let me ask a question to see if I understand: if someone sends a command from a non-command-palette source right now, would you expect it to pop up the new name prompt?

If they don’t provide an argument, yes.

Does it change the API if, when they don’t provide an argument, calva follows up with a rename instead?

Maybe not. Haven’t thought that through.

Guess I don’t see a reason to mix the two tasks. We know everything we need to know to do the quick-fix. The commands hold unknowns.

Another clarifying question: I see the userspace and internal handlers in that file, can you fill in the gaps as to when each is called? • Command palette: userspace • Quick action: internal • Command bound to keyboard shortcut: ??? • Command triggered via API: ??? • Other sources?

> Guess I don’t see a reason to mix the two tasks Because adding a separate codepath to distinguish between the two is extra work, not having to distinguish between them is easier and adds less complexity

Do the ??? denote the gaps? I’m not sure I understand the question.

yep, ??? is the gaps

Just trying to build up my mental model of what the difference between the userspace and internal handlers is

When Calva registers something as a command, it automatically gets part of the API. Keyboard shortcuts being one example of API usage. Other extensions or Joyride scripts being other exemples.

Got it, and does that go through the userspace or internal handlers?

Not sure what you mean with that?

are they registered via registerUserspaceLspCommand or registerInternalLspCommand?

the former prompt for a name if a name arg isn’t present, the latter do not

Oh. I don’t know. Not very familiar with these parts of the codebase.

Do you know if keyboard shortcuts and API commands go through the same codepath? Thinking about how I could test this

If I disregard the lsp client aspect of this… yes these two sources enter Calva at the same entrypoint for regular commands.

I think I see what you mean with the two code paths now. Just tried both quick-fix and command and they enter the same path where we are planning to intercept.

I think we can get away with not prompting and sending a known string if the name is missing.

Right, what I was saying before is that if you want to have different behavior for commands and quick fixes, then in that code path you have to have a way to tell them apart, which makes this trickier. Since it seemed desirable to have them have the same behavior anyways, it seemed smart to just do that instead. Here’s what I’m thinking at a high level: To get a rename after a refactor, the rename has to be triggered from the promise chain in sendCommand, https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L60https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L60https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L60https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L60`catch`https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L60. For palette commands, we need to get an extra arg from somewhere. For quick fixes, the arg is provided by the LSP. Here’s how I’d propose doing that: • Add a postSuccess (placeholder name) callback property to https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L164`clojureLspCommands`. The callbacks take the command args and the command response, and trigger a rename if the name arg isn’t present or is equal to defaultName (see below) • Add a defaultName string property to all of the same commands • https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L106`registerUserspaceLspCommand‎`https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L106, pull defaultName off the command and stick it on the end of the args before calling sendCommand • https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L114`registerInternalLspCommand`https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L114, replace the last arg with defaultName • Add a .then that calls the postSuccess callback in sendCommand https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L60`.sendRequest`https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L60`.catch` The results would be that all commands from all sources would trigger a rename after one of the relevant refactors if a name arg was not provided

It matters whether keyboard shortcuts go through the userspace or internal handlers b/c if they go through the internal ones we don’t want to disregard the provided name. I’m trying to test but the calva build process keeps dying on npm run compile

ok, got that fixed. Keyboard shortcuts go through the userspace handler, which seems to indicate that only quick fixes go through the internal handler. Does that sound correct?

Yeah, sounds correct. And your plan sounds good too.

Making good progress on this PR, one more interesting discovery: this change cannot break backwards compatibility with user configs or joyride scripts because the userspace command handler takes no arguments, so there was previously no way for users to specify the new name. All those commands go through the userspace handler, and https://github.com/BetterThanTomorrow/calva/blob/486e5c8073373d389ed2978b72b77e7707525c2b/src/lsp/commands/lsp-commands.ts#L96`getLSPCommandParams()`, which only have the doc uri, line, and column

Awesome! Now I can get back on stable. As far as I can tell, the new version is not yet on Homebrew. How long does it usually take for new versions to show up on Homebrew?

$ brew info clojure-lsp
==> clojure-lsp: stable 20231030T162541 (bottled), HEAD

That's not the official tap and has performance issues, check the docs https://clojure-lsp.io/installation/#homebrew-macos-and-linux

Got it. Followed the official install instructions, got the latest version.

$ clojure-lsp --version
clojure-lsp 2024.01.30-16.09.56-nightly
clj-kondo 2023.12.15