Modified xref-backend-references to use buffer locations instead of filenames

[jeeger]

The filenames returned by cider-sync-request:fn-refs were not in the format expected by uxref,
probably because they are relative to the project root and not relative to the current buffer (which
is what xref needs, from my understanding). I've implemented a somewhat dirty fix by using identical
code to xref-backend-definitions, which uses cider--find-buffer-for-filename. This function
seems to work around the issue, with the drawback of potentially opening a large amount of buffers.

The cleaner way to fix this bug would be computing absolute file names, but I am unsure what the filenames returned by cider-sync-request:fn-refs are relative to.

Fixes #3112.

[bbatsov]

The cleaner way to fix this bug would be computing absolute file names, but I am unsure what the filenames returned by cider-sync-request:fn-refs are relative to.

They are relative to clojure-project-dir (from clojure-mode).

[jeeger]

Am I correct in thinking that I should convert filenames using that information instead of the proposed fix in the PR?
Update: And I think that would cause cider to have a dependency on clojure-mode, which it currently doesn't have?

[vemv]

Am I correct in thinking that I should convert filenames using that information instead of the proposed fix in the PR?

Yes, most likely it's better to fix cider-sync-request:fn-refs

The relevant files are:

https://github.com/clojure-emacs/cider-nrepl/blob/a53ab41c7ae56fda9f4a7b5916c5bd1f8fa60d95/src/cider/nrepl/middleware/xref.clj

https://github.com/clojure-emacs/orchard/blob/5e003ffe55baf4e3a844c416bab8217cd220e048/src/orchard/xref.clj

Let us know if you're up for a PR there, else I could eye it this week/end

[jeeger]

I'm looking at contributing to cider-nrepl, but I'm having problems loading a locally installed version of cider-nrepl into leiningen. Is there something special besides PROJECT_VERSION=… make install I need to do?

[vemv]

Not that I recall.

If the version you are make installing matches with the version of cider-nrepl you normally use, that should be it

If you hack on Orchard, make install it, ensuring the version is the expected one, then make install cider-nrepl again (since it needs to "inline" that library with mranderson)

Hope it helps, else let us know of any specific problem

[iarenaza]

Am I correct in thinking that I should convert filenames using that information instead of the proposed fix in the PR? Update: And I think that would cause cider to have a dependency on clojure-mode, which it currently doesn't have?

I would rather use the cider--find-buffer-for-filename way. That function takes care of translating the file paths as seen by the REPL process, to the local paths seen by Emacs. These two might be different (are usually different) if you are using Tramp or if you are running your REPL process in another machine or inside a container (or if you are using a couple of other special cases also handled by cider-find-file). cider--find-buffer-for-filename takes care of all of those details, by calling cider-find-file.

[bbatsov]

Fine by me.

[vemv]

I guess it's fine, I'd just keep an eye on cider--find-buffer-for-filename not creating buffers that would pollute the user's set of open buffers.

Also, if there's truly an issue cider-nrepl side, fixing it there would fix it for non-Emacs clients.

[bbatsov]

Yep. Probably we should eventually have the middleware return absolute paths, but I think it's preferable short-term to go with any fix instead of leaving this functionality broken. I've been super busy in the past few weeks, otherwise I would have tackled this myself.

[bbatsov]

I'd add some comment here explaining the need for this and some todo to eventually use xref-make-file-location once the nREPL api has been modified.

[bbatsov]

@jeeger Don't forget to add a changelog entry for this bugfix.

[vemv]

Mmm, I thought @jeeger would look into cider-nrepl/Orchard.

Are we sure that cider--find-buffer-for-file isn't going to create those buffers? That issue could easily be more annoying than the current issue.

If we absolutely want cider-nrepl fixed, I can prioritize it for the first half of this week. I was simply waiting for @jeeger input in the meantime.

[bbatsov]

@vemv Did you see @iarenaza's concerns?

[vemv]

Probably both things can be taken care of, nrepl side (assuming there's an issue there) and cider.el side.

cider--find-buffer-for-file takes care of all of those details, by calling cider-find-file.

Ideally we could decouple cider-find-file's logic (finding the right thing) from the side-effect (producing a buffer).

If it was proven too difficult, maybe we could close the buffers that weren't open already prior to the computation?

Worth pointing out, Emacs sessions can last weeks, if buffers piled up to e.g. 1000, it wouldn't surprise me that that cascaded into all sorts of issues. Which is to say, better to tread carefully.

[bbatsov]

I have to admit I still use grep instead of xref, so for me that's not a big issue. 😆

[jeeger]

I was looking into orchard/cider-nrepl, but I didn't get anywhere last week — turns out that the :file meta property is either already absolute (when something is evaluated in the REPL) or relative to the classpath (when loaded from somewhere else). Since references can also be inside JAR files, I got the feeling that reusing the already existing logic was the most expedient way forward.

[vemv]

I was looking into orchard/cider-nrepl, but I didn't get anywhere last week — turns out that the :file meta property is either already absolute (when something is evaluated in the REPL) or relative to the classpath (when loaded from somewhere else). Since references can also be inside JAR files, I got the feeling that reusing the already existing logic was the most expedient way forward.

That's useful info, thanks!

So as suggested in #3349 (comment) I think hacking cider-find-file would be the way to go.

Either:

• split it into two, if possible
  • one part of the core logic, another for the buffer opening
  • xref-backend-references would use the logic part

...or:

• copy it, and make its buffers transient
  • e.g. any buffer that wasn't open already will be closed before returning the filename.
  • I'm assuming that these filenames are always useful, even if they're not necessarily associated to an existing buffer.
    • This may or may not be the case for 'funny' use cases like TRAMP, Docker, etc.

Does this sound like a workable plan?

Thanks - V

[vemv]

I'll work based on the design outlined at #3349 (comment) soon, if there are no objections.

[bbatsov]

No objections from me.

[vemv]

new PR soon

Thanks everyone for the valuable input!

[vemv]

The final fix took some inspiration from here, with nuances, and also some changes at cider-nrepl level.

Things should now be pretty for all things xref:

• references
• deps
• jump to definition

Feel free to try them out in CIDER master.

Thanks again!