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

cider-symbol-at-point does not return ns-qualified keyword to re-jump.el #2912

Closed
adityaathalye opened this issue Oct 18, 2020 · 9 comments
Closed
Labels

Comments

@adityaathalye
Copy link

adityaathalye commented Oct 18, 2020

I'm having trouble getting re-jump.el to work in context of a re-frame app. It might have to do with cider-symbol-at-point.

re-jump.el provides emacs navigation for re-frame projects. It helps jump to re-frame effect registrations, which are basically namespace-qualified keywords. Ref. its README for an example https://github.com/oliyh/re-jump.el

re-jump relies on cider-symbol-at-point to give it a fully-qualified keyword, which feeds into a regex-based search, to jump to definition.

And I think cider-find-keyword is broken for me, for the same reason.

Expected behavior

Using re-jump.el as of SHA 443ddfa33dd2ae593cc0a013d16fff21f2afd925

cider-symbol-at-point should return a fully namespace qualified keyword to re-jump.el when we call M-x re-frame-jump-to-reg or M->.

Actual behavior

cider-symbol-at-point returns an empty string to re-jump.el.

At one point I thought refactor-nrepl might be interfering, but I am able to reproduce the problem without refactor-nrepl too.

Steps to reproduce the problem

Setting up an exact replica of my project may be too much. I suspect this is a general problem that will reproduce within any clojurescript project, given the software versions documented below.

A bit of debugging inside cider-util.el/cider-symbol-at-point suggests the function is able to extract the keyword with (thing-at-point 'symbol). But, it appears to not macroexpand the keyword in a clojurescript nREPL.

ref: https://github.com/clojure-emacs/cider/blob/master/cider-util.el#L134

(when (string-match-p "^::.+" str)
  (setq str (or (ignore-errors (cider-sync-request:macroexpand "macroexpand-1" str)) "")))

I found macroexpanison returns nil. I observed this with println style debugging and with edebug step through.

The regex string-match appears to work, but then, cider-sync-request:macroexpand appears to return nil.

When stepping through, I noticed str seems to get bound to a fontified structure.

Result: #("::my-target-event-registration" 0 2 (help-echo cider--help-echo fontified t cider-locals #1=("slug" "_" "_") face clojure-keyword-face) 2 28 (help-echo cider--help-echo fontified t cider-locals #1# face clojure-keyword-face))

I tried to remove font lock, and re-tried step through. Now str gets set to "::my-target-event-registration", but still no dice... macroexpansion still returns nil.

        ;; remove font-locking
        (setq str (substring-no-properties str))
        ;; resolve ns-aliased keywords
        (when (string-match-p "^::.+" str)
          (setq str (or (ignore-errors (cider-sync-request:macroexpand "macroexpand-1" str)) "")))

This is the limit of my emacs-fu, I'm afraid... I'll need help to debug this further to find a fix.


Looking through commit history, I noticed the above keyword-expansion behaviour was added here (starting line 132 in cider-util.el): https://github.com/clojure-emacs/cider/pull/2826/files#diff-21ccb709cb3dcff7d2b47a250e64c363ca182b5a3155ab034ce6743fec58fa03R132


Note: I start the repl in my terminal and connect from Emacs with cider-connect-cljs. The exact REPL invocation is:

lein with-profile cljs update-in :dependencies conj '[cider/cider-nrepl "0.25.3"]' -- run -m shadow.cljs.devtools.cli --npm watch app

Environment & Version information

CIDER version information

;; CIDER 1.0.0snapshot (package: 20201018.957), nREPL 0.7.0
;; Clojure 1.10.1, Java 11.0.8

;; ClojureScript REPL type: shadow-select
;; ClojureScript REPL init form: (do (require '[shadow.cljs.devtools.api :as shadow]) (shadow/nrepl-select :app))

Middleware:

  • refactor-nrepl 2.5.0
  • cider/cider-nrepl 0.25.3

Lein/Boot version

  • Leiningen 2.9.4 on Java 11.0.8 OpenJDK 64-Bit Server VM
  • shadow-cljs 2.10.21 (invoked via yarn + npm)

Emacs version

GNU Emacs 26.3 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.13) of 2019-12-24

Operating system

Ubuntu 20.04.1 LTS

@bbatsov
Copy link
Member

bbatsov commented Oct 19, 2020

I'm assuming that the issue is that you need a resolved symbol and cider-symbol-at-point actually returns the symbol at point. There's cider-resolve-var which resolves a symbol within the context of a namespace. Keyword, however, are tricker as they don't have real definitions and what cider-find-keyword does is a search for the first occurance of a keyword.

A bit of debugging inside cider-util.el/cider-symbol-at-point suggests the function is able to extract the keyword with (thing-at-point 'symbol). But, it appears to not macroexpand the keyword in a clojurescript nREPL.

Might be that the problem is on the middleware side, indeed.

I think it'd be best to run those commands with Emacs's debugger and get a better picture of what's happening https://docs.cider.mx/cider/0.26/troubleshooting.html#debugging-cider-commands

@adityaathalye
Copy link
Author

Thanks for the response, @bbatsov !

I'm assuming that the issue is that you need a resolved symbol

Yes, that is correct.

and cider-symbol-at-point actually returns the symbol at point.

It doesn't appear to... that's what has stumped me.

I think it'd be best to run those commands with Emacs's debugger and get a better picture of what's happening

I observed this with both: println style debugging and with edebug step through. Maybe I am doing something wrong. I'll dig more and report back. Cheers!

@yuhan0
Copy link
Contributor

yuhan0 commented Oct 19, 2020

Hi, I introduced this change in #2826 and didn't consider how macroexpansion would work in Clojurescript (which I don't use on a regular basis)

In any case it's probably the wrong abstraction to use - can you try editing cider-symbol-at-point like this and see if it works?

        ;; resolve ns-aliased keywords
        (when (string-match-p "^::.+" str)
-          (setq str (or (ignore-errors (cider-sync-request:macroexpand "macroexpand-1" str)) "")))
+          (setq str (or (nrepl-dict-get (cider-sync-tooling-eval str (cider-current-ns))
+                                        "value")
+                        "")))

@adityaathalye
Copy link
Author

@yuhan0 thank you for your reply.

I tried with your modifications, and stepped through with edebug. Now cider-symbol-at-point tries to send the keyword (::events/fetch-success) to the REPL, but fails with the error below.

(dict "status"
      ("eval-error" "done" "state")
      "err" "Syntax error reading source at (REPL:1:1).\nInvalid token: ::events/fetch-success\n"
      "id" "16"
      "session" "2260a95c-3340-4730-8fcb-f4649aed015b"
      "time-stamp" "2020-10-19 21:49:47.7550296752020-10-19 21:49:47.7557421042020-10-19 21:49:47.7562461332020-10-19 21:49:47.758477047"
      "ex" "class clojure.lang.ExceptionInfo"
      "root-ex" "class java.lang.RuntimeException"
      "changed-namespaces" (dict)
      "repl-type" "clj")

I haven't investigated beyond this.

@adityaathalye
Copy link
Author

adityaathalye commented Nov 2, 2020

@yuhan0 Using cider-nrepl-sync-request:eval appears to do the right thing. However, I'm not familiar with CIDER internals enough to judge if this is indeed appropriate:

        ;; resolve ns-aliased keywords
        (when (string-match-p "^::.+" str)
-          (setq str (or (ignore-errors (cider-sync-request:macroexpand "macroexpand-1" str)) "")))
+          (setq str (or (nrepl-dict-get (cider-nrepl-sync-request:eval str (cider-current-connection) (cider-current-ns))
+                                        "value")
+                        "")))

For comparison, if I apply the diff in this comment: #2912 (comment), then I experience the error noted here: #2912 (comment)

        ;; resolve ns-aliased keywords
        (when (string-match-p "^::.+" str)
-          (setq str (or (ignore-errors (cider-sync-request:macroexpand "macroexpand-1" str)) "")))
+          (setq str (or (nrepl-dict-get (cider-sync-tooling-eval str (cider-current-ns))
+                                        "value")
+                        "")))

It looks like cider-sync-tooling-eval causes the REPL buffer to switch to the tooling REPL, which is a clj REPL. This causes the keyword resolution to fail with the error "Syntax error reading source at (REPL:1:1).\nInvalid token: ::events/fetch-success\n" (as seen in the previous comment).

I suppose this is because the clj "tooling" runtime does not know anything about the cljs namespace.

Also, the tooling eval function appears to disconnect the cljs namespaces from the cljs REPL. This further causes CIDER interactions like C-xC-e / C-c-C-z to break. To fix it, I have to navigate to the cljs REPL buffer and evaluate some scalar, to get the buffer name to update back to the correct cljs buffer.

@davidpham87
Copy link

Do we have any news about this? I find it would be great to have this one working.

@adityaathalye
Copy link
Author

The workaround in this comment worked for me, but I'm not sure if it's a proper fix.

@stale
Copy link

stale bot commented Apr 8, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the stale label Apr 8, 2021
@stale
Copy link

stale bot commented Jun 2, 2021

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.

@stale stale bot closed this as completed Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants