Fixing Elm LSP: Only send codeAction/resolve for unresolved code actions

[krksgbr]

Hi everyone!

First of all thanks for building Zed! I've found out about it recently and I was impressed immediately. I'm really appreciate the speed, the aesthetics and the attention to detail and would really like for it to become my daily driver replacing neovim and cursor.

However I came across an issue described in #19834, about code actions failing to resolve when using the elm-language-server. This is quite a problem for me as I build stuff in Elm most of the time and I thought I'd look into this. I think I found an appropriate solution and would like to share my analysis.

TLDR
I noticed that even when a code action has an edit associated with it already, Zed still sends a codeAction/resolve request to the server to resolve the action. This request seems unnecessary, as the action already has its edit information. Applying the edit immediately instead of sending a resolve request would prevent the issue above from happening.

The long version
Indeed as suggested in the issue, elm ls does expect a data.uri field in the CodeAction, which is missing in the outgoing (Zed -> LS) message:

https://github.com/elm-tooling/elm-language-server/blob/372dd6ea798fadcf0a998eb3bdf7c69d090d7b12/src/common/providers/codeActionProvider.ts#L117

I'm not familiar with the details of LSP, so I tried to read the specs carefully, but to be honest it still seems quite fuzzy to me what is actually being mandated. I think the idea is that the server can attach metadata to this field so that later it would help with resolving a code action, and it's not supposed to change between message exchanges. According to the specs:

/**
	 * A data entry field that is preserved on a code action between
	 * a `textDocument/codeAction` and a `codeAction/resolve` request.
	 *
	 * @since 3.16.0
	 */
	data?: any;

and also:

To help servers to uniquely identify a code action in the resolve request, a code action literal can optionally carry a data property.

Zed's behaviour is consistent with this, as demonstrated by this message exchange:

// Send:
{
  "jsonrpc": "2.0",
  "id": 58,
  "method": "textDocument/codeAction",
  "params": {
    "textDocument": {
      "uri": "..."
    },
    "range": { ... },
    "context": {
      "diagnostics": [
        {
          "range": { ... },
          "severity": 3,
          "source": "Elm",
          "message": "Missing type annotation: `...`.",
          "data": {
            "uri": "...",
            "code": "missing_type_annotation"
          }
        }
      ],
      "only": [
        "",
        "quickfix",
        "refactor",
        "refactor.extract",
        "source"
      ]
    }
  }
}


// Receive:
{
  "jsonrpc": "2.0",
  "id": 58,
  "result": [
    {
      "title": "Add inferred annotation",
      "kind": "quickfix",
      "edit": { "changes": { ... } },
      "diagnostics": [
        {
          "range": { ... },
          "severity": 3,
          "source": "Elm",
          "message": "Missing type annotation: `...`.",
          "data": { "uri": "...", "code": "missing_type_annotation" }
        }
      ],
      "data": { "fixId": "add_type_annotation" },
      "isPreferred": true
    }
      ...
  ]
}

The diagnostic entry sent by elm-language-server only includes data.fixId , but no data.uri, so I don't think data.uri should be expected in the codeAction/resolve message.

However, the edits don't even need to be resolved, because it's already included in the response, and the code action could be applied without another round trip to the server.

It seems that Zed sends the codeAction/resolve request despite already having the necessary information to apply the code action (note the (action.lsp_action.command.is_none() || action.lsp_action.edit.is_none()) condition) :

zed/crates/project/src/lsp_store.rs

Lines 1722 to 1736 in b4d8b1b

	async fn try_resolve_code_action(
	lang_server: &LanguageServer,
	action: &mut CodeAction,
	) -> anyhow::Result<()> {
	if GetCodeActions::can_resolve_actions(&lang_server.capabilities())
	&& action.lsp_action.data.is_some()
	&& (action.lsp_action.command.is_none() || action.lsp_action.edit.is_none())
	{
	action.lsp_action = lang_server
	.request::<lsp::request::CodeActionResolveRequest>(action.lsp_action.clone())
	.await?;
	}
	anyhow::Ok(())
	}

Again, I'm not deeply familiar with LSP, but shouldn't this be:

diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs
index e73bef795b..11893af286 100644
--- a/crates/project/src/lsp_store.rs
+++ b/crates/project/src/lsp_store.rs
@@ -1725,7 +1725,8 @@ impl LocalLspStore {
     ) -> anyhow::Result<()> {
         if GetCodeActions::can_resolve_actions(&lang_server.capabilities())
             && action.lsp_action.data.is_some()
-            && (action.lsp_action.command.is_none() || action.lsp_action.edit.is_none())
+            && action.lsp_action.command.is_none()
+            && action.lsp_action.edit.is_none()
         {
             action.lsp_action = lang_server
                 .request::<lsp::request::CodeActionResolveRequest>(action.lsp_action.clone())

I'm not actually sure if it's the right place to check if there's an edit already that can be applied to the buffer, maybe it could be done even earlier to prevent a bunch of code from running unnecessarily. In any case, I rebuilt Zed with this change and now everything seems to work as expected.

I hope this is useful and thanks for reading!