Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump-to-definition doesn't work in Babashka #3456

Closed
daveliepmann opened this issue Sep 8, 2023 · 12 comments · Fixed by #3460
Closed

Jump-to-definition doesn't work in Babashka #3456

daveliepmann opened this issue Sep 8, 2023 · 12 comments · Fixed by #3460
Assignees

Comments

@daveliepmann
Copy link
Contributor

The CIDER for Babashka docs imply that vars defined by the running program should support jump-to-definition:

Built-in vars (e.g. clojure.core/map) don’t have definition location metadata. In practice this means you can’t navigate to their definitions in CIDER.

However, this appears not to be the case. Clojurians thread

Expected behavior

I expect to be able to define a var and then jump to its definition from any of its usage sites.

Actual behavior

Jump to definition does not work for any var, whether built-in or defined locally.

Steps to reproduce the problem

Screenshot 2023-09-08 at 09 32 31
  1. [terminal] bb nrepl-server
  2. [emacs] M-x cider-connect
(ns reproduction)
(def foo 1) ;; => #'reproduction/foo
foo ;; => 1
  1. with cursor on the latter foo, M-. --> instead of jumping to definition, minibuffer displays "Visit tags table..." prompt

Environment & Version information

CIDER version information

Include here the version string displayed when
CIDER's REPL is launched. Here's an example:

;; CIDER 1.7.0 (Côte d'Azur), babashka.nrepl 0.0.6-SNAPSHOT
;; Babashka 1.3.181

Lein / Clojure CLI version

n/a

Emacs version

GNU Emacs 28.2 (build 1, aarch64-apple-darwin22.1.0, Carbon Version 169 AppKit 2299) of 2022-12-10

Operating system

macOS Ventura/13.5.1 (22G90)

JDK distribution

openjdk 17.0.2 2022-01-18
OpenJDK Runtime Environment (build 17.0.2+8-86)
OpenJDK 64-Bit Server VM (build 17.0.2+8-86, mixed mode, sharing)

@iarenaza
Copy link
Contributor

I'd say the problem lies in the cider--xref-backend function. That function is the one added by cider-mode to the xref-backend-functions hook. cider--xref-backend checks for the ns-path nREPL op, and if it is not available, disables CIDER's completion machinery.

But the ns-path op is not one of the base supported nREPL operations. It is instead an operation defined in the cider-nrepl extension.

Given that Babashka nREPL server only implements (an important subset) of the base supported nREPL operations, but no other extensions at all (unless you make it load additional middleware extensions), CIDER disables its own completion machinery, and xref falls back to the default completion method, which is based on etags.

@borkdude
Copy link

When hitting nREPL limitations with babashka, you can now use bb print-deps to print out a deps.edn map and just use the regular JVM tooling for development.

You can maybe even write some elisp to add -Sdeps $(bb print-deps) to the CIDER clojure-cli arguments automatically

@vemv
Copy link
Member

vemv commented Sep 11, 2023

I'd say the problem lies in the cider--xref-backend function.

Thanks for the observation!

By customizing cider-use-xref nil, one would avoid this code path. Worth a try.

@daveliepmann
Copy link
Contributor Author

By customizing cider-use-xref nil, one would avoid this code path. Worth a try.

no difference that i can tell

@vemv
Copy link
Member

vemv commented Sep 11, 2023

Got it.

Anyway, assuming that babashka doesn't implement cider-nrepl ops as analyzed by @iarenaza , then it is to be expected that most things won't work.

We have some fallbacks though, as reflected in this issue: #2611

Apparently, this is what is supported as of today:

  • completion
  • jump to definition
  • doc
  • eldoc
  • running tests

So in theory, jump to definition is supported.

Maybe at some point we had a regression / oversight that omitted these fallbacks.

@vemv
Copy link
Member

vemv commented Sep 11, 2023

After the fix, I could see jump-to-def working without cider-nrepl with:

  • M-x cider-find-var
  • M-x xref-find-definitions

That should be all needed for bb compat. I saw that bb already implements the relatively new lookup op.

Available in CIDER master, and as a MELPA snapshot within a couple hours.

@daveliepmann
Copy link
Contributor Author

@vemv I really appreciate your fast PR, but I think it broke the babashka repl entirely? I get this on cider-connect now that I'm on cider-20230911.1428:

[nREPL] Establishing direct connection to localhost:1667 ...
[nREPL] Direct connection to localhost:1667 established
error in process filter: cider-get-ns-name: Wrong number of arguments: clojure-find-ns, 1
error in process filter: Wrong number of arguments: clojure-find-ns, 1

I think JVM projects are broken, too. On opening .clj files I get

File mode specification error: (wrong-number-of-arguments clojure-find-ns 1)
cider-get-ns-name: Wrong number of arguments: clojure-find-ns, 1

and upon cider-jack-in and trying to eval the ns form:

[nREPL] Starting server via bash /Users/daveliepmann/.emacs.d/elpa/cider-20230911.1428/clojure.sh /opt/homebrew/bin/clojure -Sdeps \{\:deps\ \{nrepl/nrepl\ \{\:mvn/version\ \"1.0.0\"\}\ cider/cider-nrepl\ \{\:mvn/version\ \"0.37.0\"\}\ refactor-nrepl/refactor-nrepl\ \{\:mvn/version\ \"3.6.0\"\}\}\ \:aliases\ \{\:cider/nrepl\ \{\:main-opts\ \[\"-m\"\ \"nrepl.cmdline\"\ \"--middleware\"\ \"\[refactor-nrepl.middleware/wrap-refactor\,cider.nrepl/cider-middleware\]\"\]\}\}\} -M:cider/nrepl
Mark set [2 times]
[nREPL] server started on 49838
[nREPL] Establishing direct connection to localhost:49838 ...
[nREPL] Direct connection to localhost:49838 established
Reverting buffer ‘uwatenage<applied-science>’.
error in process filter: cider-get-ns-name: Wrong number of arguments: clojure-find-ns, 1
error in process filter: Wrong number of arguments: clojure-find-ns, 1
Error in ‘cider-repl--state-handler’: (wrong-number-of-arguments clojure-find-ns 1)
eldoc error: (wrong-number-of-arguments clojure-find-ns 1)
cider-get-ns-name: Wrong number of arguments: clojure-find-ns, 1

@iarenaza
Copy link
Contributor

iarenaza commented Sep 12, 2023

I think you need a quite recent version of clojure-mode[1]. There were some changes around clojure-find-ns recently, where the number of parameters changed.

[1] I see that vemv just tagged 5.17.0 a few hours ago, with some additional changes around clojure-find-ns.

@daveliepmann
Copy link
Contributor Author

You're absolutely right, @iarenaza, thank you.

At least on my machine, M-. on vars defined locally now jumps to the top of the buffer/ns form, not their definition. Could I perhaps still be missing something?

;; Connected to nREPL server - nrepl://localhost:1667
;; CIDER 1.8.0-snapshot, babashka.nrepl 0.0.6-SNAPSHOT
;; Babashka 1.3.181
;;     Docs: (doc function-name)
;;           (find-doc part-of-name)
;;   Source: (source function-name)
;;  Javadoc: (javadoc java-object-or-class)
;;     Exit: <C-c C-q>
;;  Results: Stored in vars *1, *2, *3, an exception in *e;
WARNING: Can't determine Clojure version.  The refactor-nrepl middleware requires clojure 1.8.0 (or newer)WARNING: clj-refactor and refactor-nrepl are out of sync.
Their versions are 3.6.0 and n/a, respectively.
You can mute this warning by changing cljr-suppress-middleware-warnings.
user> 

@vemv
Copy link
Member

vemv commented Sep 12, 2023

Please setq nrepl-log-messages t prior to starting the repl, repro the issue and then paste the *nrepl-messages <your repl>* in a gist.

(If I had to guess, there's a difference between the expected and actual format of bb's lookup response format)

@daveliepmann
Copy link
Contributor Author

Please setq nrepl-log-messages t prior to starting the repl, repro the issue and then paste the *nrepl-messages <your repl>* in a gist.

https://gist.github.com/daveliepmann/c4fc9e4ee0e10296e1144a031afad5ea

@vemv
Copy link
Member

vemv commented Sep 12, 2023

Thanks much!

As seen in the last request/respons cycle, your cider.el requested for op "info" about books, and bb replied with:

(<--
  id           "23"
  session      "eaddb3f9-9be9-4c7a-82c1-3229aada4ca2"
  time-stamp   "2023-09-12 10:29:21.766390000"
  arglists-str ""
  file         "/Users/daveliepmann/src/knowuro/knowuro-cljd/bb/import_books..."
  line         1
  name         "books"
  ns           "import-books"
  status       ("done")
)

It says the line is 1. There's nothing we can do about that - feel free to raise an issue in the adequate repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants