oantolin / embark

Emacs Mini-Buffer Actions Rooted in Keymaps
GNU General Public License v3.0
936 stars 56 forks source link

`xref-find-references` does not work with `eglot` + `clojure-lsp` #480

Closed Andre0991 closed 2 years ago

Andre0991 commented 2 years ago

Hi.

I'm getting an error when using xref-find-references thru embark:

Debugger entered--Lisp error: (wrong-type-argument xref-item nil)
  xref-item-location(nil)
  xref-pop-to-location(nil nil)
  consult-xref(#f(compiled-function () #<bytecode -0x14919ef836108010>) ((window . #<window 550 on trace.clj>) (display-action) (auto-jump)))
  xref--show-xrefs(#f(compiled-function () #<bytecode -0x14919ef836108010>) nil)
  xref--find-xrefs("LSP identifier at point." references "LSP identifier at point." nil)
  xref-find-references("LSP identifier at point.")
  funcall-interactively(xref-find-references "LSP identifier at point.")
  command-execute(xref-find-references)
  #f(compiled-function () #<bytecode -0x1f3062335ff2f8bc>)()
  embark--quit-and-run(#f(compiled-function () #<bytecode -0x1f3062335ff2f8bc>))
  embark--act(xref-find-references (:orig-type identifier :orig-target #("trace-indent" 0 12 (face font-lock-function-name-face fontified t)) :bounds (2767 . 2779) :type identifier :target #("trace-indent" 0 12 (face font-lock-function-name-face fontified t))) t)
  embark-act(nil)
  funcall-interactively(embark-act nil)
  command-execute(embark-act)

image

I don't know if this depends on eglot or any particular LSP server, so I'll describe all steps for reproducing it.

Steps to reproduce

  1. Download clojure-lsp:
wget https://github.com/clojure-lsp/clojure-lsp/releases/download/2022.02.23-12.12.12/clojure-lsp
  1. Clone this Clojure repo:
git clone https://github.com/clojure/tools.trace.git
  1. Start emacs and go to tools.trace/src/main/clojure/clojure/tools/trace.clj. Run eglot. Choose clojure-lsp when eglot asks for the server (indicate the full path or add clojure-lsp to PATH before running eglot.

  2. Go to line 86, use embark-act with the point on trace-indent and use r.

I'm not using emacs -Q. My config is not minimal but it's not too big either: https://github.com/Andre0991/dotfiles/blob/master/emacs.el#L1 Let me know if you prefer that I reproduce this with less config.

oantolin commented 2 years ago

I think it would be a large amount of work for me to attempt to reproduce this bug since I don't use any of the following: eglot, lsp-mode, clojure or even a JVM. What is clojure-lsp written in? (My guess would be clojure...)

oantolin commented 2 years ago

I guess the first thing to check is whether xref-find-references works when called directly, not through embark-act.

oantolin commented 2 years ago

Wait a second: does the stacktrace say xref-find-references is called with the string "LSP identifier at point."? That definitely sounds wrong, I wonder where that string even came from...

Andre0991 commented 2 years ago

I think it would be a large amount of work for me to attempt to reproduce this bug since I don't use any of the following: eglot, lsp-mode, clojure or even a JVM. What is clojure-lsp written in? (My guess would be clojure...)

Yeah, I understand. I don't know what is essential for reproducing this bug, though – I guess clojure-lsp could be just any other LSP server, but I'm not sure and Clojure is the only I use anyway.

In any case, you are right, clojure-lsp is in Clojure, but I use the native binary generated with GraalVM (https://clojure-lsp.io/installation/).

I guess the first thing to check is whether xref-find-references works when called directly, not through embark-act.

It does, yeap. I forgot to mention that.

Andre0991 commented 2 years ago

Wait a second: does the stacktrace say xref-find-references is called with the string "LSP identifier at point."? That definitely sounds wrong, I wonder where that string even came from...

Indeed, this looks wrong. Let me know what can I do for adding more info to this issue – maybe debugging embark--act, since it originates the xref command?

Andre0991 commented 2 years ago

The "LSP identifier at point." string comes from this:

https://github.com/joaotavora/eglot/blob/fd9a5646d1b49ef5968713005d83131dd75a52ad/eglot.el#L2341

I actually stumbled upon this when investigating other issues. I don't know the context of it, I'd need to investigate it later.

(cl-defun eglot--lsp-xref-helper (method &key extra-params capability )
  "Helper for `eglot-find-declaration' & friends."
  (let ((eglot--lsp-xref-refs (eglot--lsp-xrefs-for-method
                               method
                               :extra-params extra-params
                               :capability capability)))
    (if eglot--lsp-xref-refs
        (xref-find-references "LSP identifier at point.")
      (eglot--message "%s returned no references" method))))
zikajk commented 2 years ago

I confirm this behaviour - identical to what @Andre0991 describes.

oantolin commented 2 years ago

I managed to install two LSP servers:

So it looks like debugging this may actually require clojure-lsp.

hmelman commented 2 years ago

I can reproduce this on Emacs 28 (mac port) using eglot and pyright. On a symbol where M-? (xref-find-references) works, using embark-act r fails with "xref-item-location: Wrong type argument: xref-item, nil"

oantolin commented 2 years ago

Well, this was a little frustrating! I decided to install a JVM, Clojure, eglot and clojure-lsp and reproduce this bug. All of that was much, much easier to install than I expected. I did have to increase eglot's timeout from 30 seconds to a full minute, because man, is clojure-lsp ever slow to start!

But then after all that success, I had no luck reproducing the problem: both M-? and <embark-act> r worked just fine and give the same result. I'm on Emacs 29, xref 1.4.1 and eglot 1.8. Maybe I need to downgrade some things to be able to reproduce the problem.

@hmelman is on Emacs 28, what about you @Andre0991, and you @zikajk? Probably the version of xref is also relevant. Maybe even eglot too, but I suspect xref at this point.

oantolin commented 2 years ago

By the way, this is my first time running eglot or any LSP server and I can see why people won't shut up about this! It finally makes programming in other languages half as smooth as Common Lisp or Emacs Lisp. 😄 (The other half that is still missing is evaluating expression right in the source bufffer.)

hmelman commented 2 years ago

I just started using lsp too, just on python so far and probably for the foreseeable future. I liked elgot a bit better than lsp mode but I haven't used either extensively yet.

Andre0991 commented 2 years ago

Hello.

There are my versions:

GNU Emacs 28.1 (build 2, x86_64-apple-darwin19.6.0, NS appkit-1894.60 Version 10.15.7 (Build 19H2)) of 2022-04-07 xref: Version: 1.3.0 eglot:

;; Version: 1.8
;; Package-Version: 20220404.1005

I di have to increase eglot's timeout from 30 seconds to a full minute, because man, is clojure-lsp ever slow to start!

Are you using the native binary clojure-lsp? It's way faster than the non-native version. Another possibility is that it was slower in the first run, because it has to calculate the classpath and do other things. Subsequent runs should be faster.

oantolin commented 2 years ago

I used whatever clojure-lsp I got by following your instructions:

wget https://github.com/clojure-lsp/clojure-lsp/releases/download/2022.02.23-12.12.12/clojure-lsp

I don't mind it taking more than 30 seconds to start, I was just surprised.

Andre0991 commented 2 years ago

This is the binary, yes. So I suppose it was slow because it was calculating stuff about the project for the first time.

oantolin commented 2 years ago

Could you try installing xref 1.4.1 from GNU ELPA, @Andre0991?

oantolin commented 2 years ago

I just found out pyright is written in TypeScript! I mean, why not, but it still feels weird.

Andre0991 commented 2 years ago

Could you try installing xref 1.4.1 from GNU ELPA, @Andre0991?

Sure. How do I do that?

I tried to add xref to package-selected-packages. Then, I used package-refresh-contents and (package-install-selected-packages). But I got "All your packages are already installed".

This is my config: https://github.com/Andre0991/dotfiles/blob/master/emacs.el#L1

Andre0991 commented 2 years ago

Well, I ended up using the interface provided by list-packages for installing the latest version (1.4.1). Not sure for to do it with elisp.

Anyways, was able to reproduce the error with that version as well:

Debugger entered--Lisp error: (wrong-type-argument xref-item nil)
  xref-item-location(nil)
  xref-pop-to-location(nil nil)
  consult-xref(#f(compiled-function () #<bytecode 0x1b2295daecf40c4c>) ((window . #<window 3 on test_helpers.clj>) (display-action) (auto-jump)))
  xref--show-xrefs(#f(compiled-function () #<bytecode 0x1b2295daecf40c4c>) nil)
  xref--find-xrefs("LSP identifier at point." references "LSP identifier at point." nil)
  xref-find-references("LSP identifier at point.")
  funcall-interactively(xref-find-references "LSP identifier at point.")
  command-execute(xref-find-references)
  #f(compiled-function () #<bytecode 0x132c2814f5aa856c>)()
  embark--quit-and-run(#f(compiled-function () #<bytecode 0x132c2814f5aa856c>))
  embark--act(xref-find-references (:orig-type identifier :orig-target #("test-infos" 0 10 (face font-lock-function-name-face fontified t)) :bounds (2790 . 2800) :type identifier :target #("test-infos" 0 10 (face font-lock-function-name-face fontified t))) t)
  embark-act(nil)
  funcall-interactively(embark-act nil)
  command-execute(embark-act)
oantolin commented 2 years ago

Glad you figured out a way. I was about to say that maybe (package-install-selected-packages) doesn't update installed packages, but only installs those that are missing. In that case M-x package-reinstall RET xref should work.

It's too bad the error persists. I'll try to get my hands on an older Emacs and test there in the next few days.

Andre0991 commented 2 years ago

No worries. Thanks!

oantolin commented 2 years ago

I CAN REPRODUCE IT!

I had the default setting of

(setq xref-show-xrefs-function #'xref--show-xref-buffer)

and that worked just fine, but when I replaced it with consult-xref I got the error!

And now the error is obvious in retrospect: Embark was injecting the variable name at the consult-xref prompt and of course, that's not a full xref completion candidate.

You can fix this by using:

(push 'embark--ignore-target
      (alist-get #'xref-find-references embark-target-injection-hooks))

I will add that to Embark as well.

oantolin commented 2 years ago

OK, done. I'm glad that (1) in the end it was a trivial bug and (2) you got me to try an LSP server!

Andre0991 commented 2 years ago

Amazing! Thanks a lot. 💯

oantolin commented 2 years ago

I'm curious about something, @Andre0991: it seems to me that clojure-lsp has a subset of the functionality of cider; do you not use cider? Do you use both and only keep clojure-lsp for the "find references" functionality (which I don't recall seeing in cider, but I used cider a long time ago)?

Andre0991 commented 2 years ago

it seems to me that clojure-lsp has a subset of the functionality of clojure-lsp; do you not use cider?

You are right, there is some overlap. For example, both cider and LSP have a notion of "Go to definition". Similarly, there's a function for find all references, for they work differently: LSP information is based on static analysis, while cider only consider code that was evaluated already (so, in practice, for getting reliable 'find reference' with cider, one would need to eval the whole project).

However, there's functionality that is only present with static analysis, and other features depend on interaction with a REPL, so LSP does not cover it. So, in general, people use both cider and LSP.

On my part, I do not use cider. I use inf-clojure instead, which is simpler and dumber: https://github.com/clojure-emacs/inf-clojure

Using inf-clojure alone would be hard, but inf-clojure for a REPL + LSP for static analysis is perfect for my needs.

Andre0991 commented 2 years ago

Do you use both and only keep clojure-lsp for the "find references" functionality (which I don't recall seeing in cider, but I used cider a long time ago)?

And just for complementing: I use just LSP for that. eglot registers an xref function for this. I'm not sure how the xref machinery works, but as fas as I understand, the eglot client adds its function that uses LSP when invoking xref-find-refrences.