elixir-tools / credo-language-server

LSP implementation for Credo.
MIT License
96 stars 11 forks source link

CredoLanguageServer.handle_request/2 not handling TextDocumentDocumentSymbol #8

Closed wkirschbaum closed 1 year ago

wkirschbaum commented 1 year ago

When hooking up to Emac's lsp program called eglot, the server crashes when trying to handle the following request:

%GenLSP.Requests.TextDocumentDocumentSymbol{
  params: %GenLSP.Structures.DocumentSymbolParams{
    partial_result_token: nil,
    work_done_token: nil,
    text_document: %GenLSP.Structures.TextDocumentIdentifier{
      uri: "file:///home/whk/src/elixir/za_id_number/lib/za_id_number/validator.ex"
    }
  },
  id: 3,
  jsonrpc: "2.0",
  method: "textDocument/documentSymbol"
}

The following change fixes it for me locally:

@@ -104,6 +104,10 @@ defmodule CredoLanguageServer do
     {:noreply, assign(lsp, exit_code: 0)}
   end

+  def handle_request(_, lsp) do
+    {:noreply, lsp}
+  end
+
   @impl true
   def handle_notification(%Initialized{}, lsp) do
     GenLSP.log(lsp, "[Credo] LSP Initialized!")
mhanberg commented 1 year ago

So the server is not advertising that it supports the document symbol capability, so the client should not be sending the request.

wkirschbaum commented 1 year ago

Sure, but when is it a good idea to trust a client to not crash the server?

On Thu, 20 Apr 2023, 13:06 Mitchell Hanberg, @.***> wrote:

So the server is not advertising that it supports the document symbol capability, so the client should not be sending the request.

— Reply to this email directly, view it on GitHub https://github.com/elixir-tools/credo-language-server/issues/8#issuecomment-1516137356, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAK5POYP4VZLHMCNHRAOLJDXCEKDVANCNFSM6AAAAAAXFETRTU . You are receiving this because you authored the thread.Message ID: @.***>

wkirschbaum commented 1 year ago

This is eglot in emacs, will have a look there too.

On Thu, 20 Apr 2023, 13:10 Wilhelm Kirschbaum, @.***> wrote:

Sure, but when is it a good idea to trust a client to not crash the server?

On Thu, 20 Apr 2023, 13:06 Mitchell Hanberg, @.***> wrote:

So the server is not advertising that it supports the document symbol capability, so the client should not be sending the request.

— Reply to this email directly, view it on GitHub https://github.com/elixir-tools/credo-language-server/issues/8#issuecomment-1516137356, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAK5POYP4VZLHMCNHRAOLJDXCEKDVANCNFSM6AAAAAAXFETRTU . You are receiving this because you authored the thread.Message ID: @.***>

mhanberg commented 1 year ago

https://github.com/mhanberg/.dotfiles/blob/20316645d2eac5d8c2435de86e2c3fe495acb091/config/nvim/lua/motch/autocmds.lua#L53

Example 👆

mhanberg commented 1 year ago

Sure, but when is it a good idea to trust a client to not crash the server?

Fair point, but I'm just saying the protocol specifies these capabilities just for this reason and most other LSP clients seem to handle it fine.

mhanberg commented 1 year ago

Let me take another look at the protocol to see what it says about how to handle a situation like this.

mhanberg commented 1 year ago

I think I know the change I need to make, I'll push that up shortly

wkirschbaum commented 1 year ago

I will submit a patch to emacs for this if i can spot the bug there, but thanks for having a look.

mhanberg commented 1 year ago

I am not sure about your config but could possibly make sure you don't have a plug-in that is the culprit.

wkirschbaum commented 1 year ago

This is core emacs with no strange config options. I have not seen this issue with other language servers I use.

mhanberg commented 1 year ago

@wkirschbaum would you mind trying out the branch on #15 and let me know if it works?

wkirschbaum commented 1 year ago

@mhanberg thanks. I am not getting the same error, but have timeouts after 30 seconds. This was observed before on larger projects, but happening on smaller ones on this branch as well. I will investigate a bit more and log an issue a bit later.

mhanberg commented 1 year ago

@wkirschbaum Would you be able to share with me a minimal emacs eglot config to get me started? Would probably help figure this out faster

wkirschbaum commented 1 year ago

For testing I use the following:

(require 'eglot)
(require 'heex-ts-mode)
(require 'elixir-ts-mode)

(setq eglot-server-programs `((elixir-ts-mode . ("~/src/elixir/credo-language-server/bin/credo-language-server"))))
;; or
(setq eglot-server-programs `((elixir-ts-mode . ("localhost" 9000))))

(add-hook 'elixir-ts-mode-hook 'eglot-ensure)
(add-hook 'heex-ts-mode-hook 'eglot-ensure)

you can use it with elixir-mode as well from melpa and would look like this:

(require 'eglot)

(setq eglot-server-programs `((elixir-ts-mode . ("localhost" 9000))))

(add-hook 'elixir-mode-hook 'eglot-ensure)

I am currently using only core emacs on the master branch, compiled with tree-sitter.

Unless you are comfortable with Emacs I dont want to burden you with debugging, I will get to it soonish :)

mhanberg commented 1 year ago

@wkirschbaum i'm calling the original issue fixed by #15.

Please open a new issue once you narrow down the new problem you're having with eglot.

Thanks again!