Implement LSP code actions

[helin24]

These are the things that code actions handle:

• Quick fixes:
  • Import missing library
  • Create missing declaration (e.g. a class that doesn’t exist)
  • Add required parameter (e.g. you are trying to call a method but don’t remember params)
  • Remove dead/unused code
• Quick assists and structural refactorings
  • Flutter widget wrap and remove
  • Convert getter to method, vice versa
  • Convert positional to named parameters
• Source / batch actions
  • Organize imports (note - this is different from optimize imports)
  • Sort members (e.g. methods in a class that are not alphabetized)
• Interactive code refactorings
  • Extract method
  • Extract local variable

There are some significant UI differences from legacy, and we should decide if we are okay going ahead with these:

• Source / batch actions are newly in the option+enter menu. Before they needed shortcuts or Code menu
• Extract method in the legacy implementation offered options of whether you want to replace all occurrences and whether you want to make the method a getter.
• Extract local variable would ask you whether you wanted to replace all occurrences
• (Similarly, inline would ask whether you wanted to delete the variable/method)
• Cmd + Opt + M will no longer trigger refactor

In general, edit.getRefactoring usage has declined pretty significantly (for more details, ask me for this data), so I am not so worried about removing this.

[gemini-code-assist]

Code Review

This pull request implements support for LSP Code Actions in the Dart plugin via an in-process bridge (DartBridgeLspServer) communicating with the Dart Analysis Server (DAS). It introduces a custom JSON converter (DartLspJsonConverter) with a specialized TypeAdapterFactory to handle the deserialization of Either<Command, CodeAction> lists, and integrates version checks to enable these features on sufficient SDK versions. However, two critical issues must be addressed: first, the bridge currently ignores the response of workspace/applyEdit requests, which will cause client commands to hang; second, the discriminator logic for distinguishing Command and CodeAction is fragile and will fail on valid code actions that only contain a title and a command.

[gemini-code-assist]

[MUST-FIX] The bridge forwards the workspace/applyEdit request to the LSP client via client.applyEdit(params), but it completely ignores the returned CompletableFuture and never sends the LSP response back to the Dart Analysis Server (DAS).

In LSP, workspace/applyEdit is a request from the server to the client. When DAS executes a command (e.g., via workspace/executeCommand), it sends workspace/applyEdit to the client and waits for the response before completing the workspace/executeCommand request. If the bridge never sends the response back to DAS, the workspace/executeCommand request will hang and eventually timeout on the client side, breaking the execution of code actions.

We should handle the completion of client.applyEdit(params) and send the LSP response back to DAS wrapped in an lsp.handle legacy request.

            } else if (method == "workspace/applyEdit") {
                val paramsObj = msgObj.get("params")
                val params = GSON.fromJson(paramsObj, ApplyWorkspaceEditParams::class.java)
                val id = msgObj.get("id")
                client.applyEdit(params).whenComplete { response, error ->
                    if (id != null) {
                        val legacyId = das.generateUniqueId()
                        if (legacyId != null) {
                            val lspResponse = JsonObject().apply {
                                addProperty("jsonrpc", JSONRPC_VERSION)
                                add("id", id)
                                if (error != null) {
                                    add("error", JsonObject().apply {
                                        addProperty("code", -32603)
                                        addProperty("message", error.message ?: "Internal error")
                                    })
                                } else {
                                    add("result", GSON.toJsonTree(response))
                                }
                            }
                            val legacyRequest = JsonObject().apply {
                                addProperty("id", legacyId)
                                addProperty("method", "lsp.handle")
                                add("params", JsonObject().apply {
                                    add("lspMessage", lspResponse)
                                })
                            }
                            try {
                                das.sendRequest(legacyId, legacyRequest)
                            } catch (e: Exception) {
                                logger.error("Failed to send response to DAS: $legacyRequest", e)
                            }
                        }
                    }
                }

[alexander-doroshko]

but it completely ignores the returned CompletableFuture and never sends the LSP response back to the Dart Analysis Server (DAS)

Valid comment. This function is called forwardNotificationToClient. Perhaps there should be something like forwardRequestToClient as well, which would care about handling the client's response.

By the way, functions handleDasResponse and setupDasResponseListener have misleading names as they are currently handling not only DAS responses but also server-initiated notifications and requests.

[gemini-code-assist]

[MUST-FIX] The current discriminator logic for distinguishing between Command and CodeAction relies on checking the absence of "kind", "edit", and "diagnostics". However, according to the LSP specification, all fields in CodeAction except title are optional. A CodeAction can contain only title and command (with no kind, edit, or diagnostics), which is very common for code actions that just delegate to a command.

If such a CodeAction is received, the current logic will incorrectly classify it as a Command (Left) because obj.has("command") is true and the other fields are absent. This will then fail to deserialize because the command field in Command is expected to be a string, whereas in CodeAction it is a nested Command object, throwing a JsonSyntaxException.

A much more robust way to distinguish them is to inspect the type of the command field itself:

• In a Command, the command field is always a JSON primitive (string).
• In a CodeAction, the command field is always a JSON object (the nested Command object).
• If the command field is absent, it must be a CodeAction (since command is optional in CodeAction but required in Command).

                        val commandElem = obj.get("command")
                        val isCommand = commandElem != null && commandElem.isJsonPrimitive

                        if (Command::class.java.isAssignableFrom(leftRaw) && CodeAction::class.java.isAssignableFrom(rightRaw)) {
                            if (isCommand) {
                                val leftVal = leftAdapter.fromJsonTree(element)
                                return Either.forLeft(leftVal)
                            }
                            val rightVal = rightAdapter.fromJsonTree(element)
                            return Either.forRight(rightVal)
                        }
                        if (CodeAction::class.java.isAssignableFrom(leftRaw) && Command::class.java.isAssignableFrom(rightRaw)) {
                            if (isCommand) {
                                val rightVal = rightAdapter.fromJsonTree(element)
                                return Either.forRight(rightVal)
                            }
                            val leftVal = leftAdapter.fromJsonTree(element)
                            return Either.forLeft(leftVal)
                        }

[helin24]

@alexander-doroshko Gemini generated this for me, and from follow up conversation, I gather this is because we are using this custom bridge instead of letting the LSP client communicate with DAS over stdin/out. But it also seems there must be some code within LSP client that would do this kind of transformation to JSON; is that possible to call from our code, or possible to expose somehow, so that we don't need to have this custom code in the bridge server?

[alexander-doroshko]

There is no such low-level JSON handling in the LSP Client code. It might be hidden somewhere in the LSP4J library, but we never had to care about it.

I was a bit surprised to see the code that Gemini generated in normalizeExecuteCommandParams and in DartLspJsonConverter. I didn't expect the need of such low-level JSON handling.

I agree with Gemini as a reviewer (a comment above) that the code generated by Gemini as a developer looks fragile.

I'd expect the LSP4J library to contain all the low-level stuff we may need, but I haven't tried to simplify or remove the normalizeExecuteCommandParams / DartLspJsonConverter code yet.

[helin24]

@alexander-doroshko same question as above for this JSON transformation class.

[alexander-doroshko]

Maybe logger.debug() or logger.trace() here and everywhere?

'Info' level is written to log files out-of-the-box, so all users will get megabytes of unneeded spam in logs.

'Debug' and 'Trace' levels are turned off by default, you can enable then only for yourself via Help -> Diagnostic Tools -> Debug Log Settings