Support java.apply.workspaceEdit

[theli-ua]

Fixes #376
This follows exactly same pattern as organizeImports was done. This should preserve existing behaviour for vscode

[eclipse-ls-bot]

Can one of the admins verify this patch?

[rgrunber]

If this is still wanted, I'm open to just merging. It should be safe given that clients can choose for themselves whether to use the new delegate command for applying workspace edits, or whether it should be done client side.

[theli-ua]

Well its still needed in at least some editors, eg helix-editor, see helix-editor/helix#5421

[theli-ua]

So, yes, pretty much wanted

[rgrunber]

Looks fine overall. Just some minor cleanup that can hopefully simplify things.

[rgrunber]

Would it be ok to rename the command from java.apply.workspaceEdit to something like java.edit.workspaceEdit ?

There's one small thing I forgot. That extra line in the plugin.xml (aside from being required) also ensures the language client will register the command client-side. In the case of VS Code, this would cause a clash.

Because of that line, the server sends

[Trace - 15:19:27] Received request 'client/registerCapability - (22)'.
Params: {
    "registrations": [
        {
            "id": "4bf188aa-e88a-4eda-86a0-2c00e25b9762",
            "method": "workspace/executeCommand",
            "registerOptions": {
                "commands": [
                ...
                ...
                    "java.apply.workspaceEdit",
                ...
                ...
                ]
            }
        }
    ]
}

which ends up in https://github.com/microsoft/vscode-languageserver-node/blob/277e4564193c4ed258fe4d8d405d3379665bbab9/client/src/common/executeCommand.ts#L77-L83 .

Exception has occurred: Error: command 'java.apply.workspaceEdit' already exists
t i.registerCommand (/usr/share/code/resources/app/out/vs/workbench/api/node/extensionHostProcess.js:125:138275)
	at Object.registerCommand (/usr/share/code/resources/app/out/vs/workbench/api/node/extensionHostProcess.js:140:20532)
	at ExecuteCommandFeature.register (/home/rgrunber/git/vscode-java/dist/extension.js:112907:52)
	at TracingLanguageClient.doRegisterCapability (/home/rgrunber/git/vscode-java/dist/extension.js:109925:25)
	at TracingLanguageClient.handleRegistrationRequest (/home/rgrunber/git/vscode-java/dist/extension.js:109900:25)
	at /home/rgrunber/git/vscode-java/dist/extension.js:109372:108
	at handleRequest (/home/rgrunber/git/vscode-java/dist/extension.js:105538:41)
	at handleMessage (/home/rgrunber/git/vscode-java/dist/extension.js:105384:13)
	at processMessageQueue (/home/rgrunber/git/vscode-java/dist/extension.js:105407:17)
	at Immediate._onImmediate (/home/rgrunber/git/vscode-java/dist/extension.js:105379:13)
	at process.processImmediate (node:internal/timers:476:21)
	at process.callbackTrampoline (node:internal/async_hooks:130:17)

This can be seen on

eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java

Line 79 in 2412ed9

public static final String COMMAND_ID_APPLY_EDIT = "java.apply.workspaceEdit";

eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java

Line 279 in 2412ed9

command = new Command(name, COMMAND_ID_APPLY_EDIT, Collections.singletonList(edit));

However, I think we can just rename the command you're proposing and as long as clients know to use that, it should be ok. Let me know if you're fine with that.

[theli-ua]

and as long as clients know to use that

But it doesn't come from clients, it comes from jdt.ls itself o_O so clients really have nothing to do with that

Are you suggesting that upon receiving "java.apply.workspaceEdit" from server clients have to know to send back "java.edit.workspaceEdit" ? In which case this defeats the purpose of the change - to have protocol compliant clients to work without any jdtls-specific code :(

[theli-ua]

can we instead just remove that from plugin.xml ? That still works perfectly fine (which I just checked) for normal clients that would just send java.apply.workspaceEdit back to jdtls, at the same time it will avoid it trying to register that with a client explicitly?

[rgrunber]

can we instead just remove that from plugin.xml ? That still works perfectly fine (which I just checked) for normal clients that would just send java.apply.workspaceEdit back to jdtls, at the same time it will avoid it trying to register that with a client explicitly?

I was looking at that. However, when VS Code tries to run workspace/executeCommand with the command id argument being java.apply.workspaceEdit, it fails because it bases success of running the delegate command on its presence in the plugin.xml 😐

eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/WorkspaceExecuteCommandHandler.java

Lines 214 to 216 in 2412ed9

	if (candidates.isEmpty()) {
	throw new ResponseErrorException(new ResponseError(ResponseErrorCode.MethodNotFound, String.format("No delegateCommandHandler for %s", params.getCommand()), null));
	}

How about we add some extendedClientCapability (that only VS Code would need to set as true) and when present, we make an exception, and avoid adding java.apply.workspaceEdit to the list of commands to send back to the client for registration. It could be done at

eclipse.jdt.ls/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/WorkspaceExecuteCommandHandler.java

Lines 163 to 170 in 2412ed9

	public Set<String> getNonStaticCommands() {
	Collection<DelegateCommandHandlerDescriptor> handlers = getDelegateCommandHandlerDescriptors();
	Set<String> commands = new HashSet<>();
	for (DelegateCommandHandlerDescriptor handler : handlers) {
	commands.addAll(handler.getNonStaticCommands());
	}
	return commands;
	}

since VS Code supports the commands as non-static, so we would only have to do it in that case. It's not ideal, but I think that could work.

Let me know.

[theli-ua]

However, when VS Code tries to run workspace/executeCommand with the command id argument being java.apply.workspaceEdit

I thought the whole point was that VScode's jdt.ls plugin handles java.apply.workspaceEdit locally without ever sending it back to the server like a regular lsp client does, no?

I think I'll try to set up vscode to just cross-check this change to make sure w/e we come up with will work with both
fwiw I'm using helix editor to check a regular lsp client with no jdt-ls specific handling

[rgrunber]

I thought the whole point was that VScode's jdt.ls plugin handles java.apply.workspaceEdit locally without ever sending it back to the server like a regular lsp client does, no?

I was using java.apply.workspaceEdit as an example. I meant any command ID. In other words, placing the command in the plugin.xml is required for the command to be run using workspace/executeCommand.

[theli-ua]

In other words, placing the command in the plugin.xml is required for the command to be run using workspace/executeCommand.

Required by vscode? jdtls?

As I said I've removed it from plugin.xml, rebuilt jdtls and it worked just fine. And vscode plugin wouldn't even send it back as it process it itself (thus the conflict if we do put it there) and there is no need to register this command with it... so everyone is happy if its not there?

A different, non-jdtls specific client that simply implements the spec would simply echo the command back when user selects the codeAction, as per spec
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_codeAction

commands specified in a code actions should be handled by the server and not by the client

[mfussenegger]

Required by vscode? jdtls?

If it's not listed in plugin.xml it will not show up as supported command within the server capabilities in executeCommandProvider.commands. E.g. neovim uses that to see if a command is supported by a server, and if not, it tries client commands and if there is no client command either it will show a warning that it's missing. So without it, there's no guarantee that this change would work in standard compliant editors

---

What's the reason that eclipse.jdt.ls isn't sending a regular workspace edit to begin with?

[theli-ua]

How about we add some extendedClientCapability (that only VS Code would need to set as true)

I take it for this approach you wouldn't want to just check for client name?

Or alternatively - ignore java.apply.workspaceEdit in vscode-java when registering?

[theli-ua]

What's the reason that eclipse.jdt.ls isn't sending a regular workspace edit to begin with?

There was a PR that changed code action to use edit directly if supported by the client. It was reverted because on vscode-java apparently that would cause formatting differences from how internal custom command would do it ( #1278 )

[rgrunber]

What's the reason that eclipse.jdt.ls isn't sending a regular workspace edit to begin with?

I think we could investigate using a workspace edit directly since the VS Code client should be passing it's format settings to the server since #1657 . I guess the only other issue would be if there are clients that would take longer to migrate to the new (correct) approach. I just wanted to get this PR merged to give other clients some workaround.

[mfussenegger]

I guess the only other issue would be if there are clients that would take longer to migrate to the new (correct) approach

Could eclipse.jdt.ls check the workspace/applyEdit client capability, and if present use a regular workspace edit, and if not fallback to the (current) non-standard java java.apply.workspaceEdit command?

This should work for any standard compliant client that implements the capability, and also wouldn't break any existing client that works via java.apply.workspaceEdit

[theli-ua]

Could eclipse.jdt.ls check the workspace/applyEdit client capability, and if present use a regular workspace edit, and if not fallback to the (current) non-standard java java.apply.workspaceEdit command?

This is exactly what #1278 was doing. And we could just re-apply it again, as the only reason that it was reverted was that it didn't use vscode's client-side identation settings. But supposedly that should not be an issue anymore as @rgrunber points out

[theli-ua]

I've opened #2962 to return edit directly

[theli-ua]

Closing in favor of #2962