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

Sideeffect checking cider--xref-backend #3170

Closed
ericdallo opened this issue Mar 24, 2022 · 4 comments · Fixed by #3171
Closed

Sideeffect checking cider--xref-backend #3170

ericdallo opened this issue Mar 24, 2022 · 4 comments · Fixed by #3171

Comments

@ericdallo
Copy link
Contributor

When doom runs (xref-find-backend) to find current xref backend, cider gives a error: No linked CIDER sessions. xref backend functions should not have side-effects like that, this is making doom's code not be able to check for other backends like lsp making users need to jack in repl to use doom's lsp find-definition for example even if not using cider find definition.

Expected behavior

(cider--xref-backend) not throw any side effects like checking if repl is available.

Actual behavior

(cider--xref-backend) checks if repl is available, throwing a error message if not.

Steps to reproduce the problem

M-x (cider--xref-backend) with a repl not enabled in a project.

Environment & Version information

CIDER version information

;; CIDER 1.3.0 (Ukraine), nREPL 0.9.0
;; Clojure 1.11.0, Java 11.0.12

Emacs version

28.0.91

Operating system

NixOS

@vemv
Copy link
Member

vemv commented Mar 24, 2022

Looks like the ensure argument is being passed to cider-current-repl when calling this:

cider/cider-find.el

Lines 233 to 238 in 1e27eba

(defun cider--xref-backend ()
"Used for xref integration."
;; Check if `cider-nrepl` middleware is loaded. Allows fallback to other xref
;; backends, if cider-nrepl is not loaded.
(when (cider-nrepl-op-supported-p "ns-path")
'cider))

@ericdallo
Copy link
Contributor Author

Yes, here exactly:

(nrepl-op-supported-p op (or connection (cider-current-repl nil 'ensure))))

@vemv
Copy link
Member

vemv commented Mar 24, 2022

PR welcome adding extra arguments and/or wrapped functions here and there, wdyt?

It would be particularly appreciated from that the implicit QAing (I don't use xref for instance, so can't QA this atm)

@ericdallo
Copy link
Contributor Author

Proposal here @vemv

ericdallo added a commit to ericdallo/cider that referenced this issue Mar 24, 2022
hlissner added a commit to doomemacs/doomemacs that referenced this issue Mar 30, 2022
To ensure lsp/eglot settings have precedence over local servers (e.g.
cider and lookup handlers).

Ref: clojure-emacs/cider#3170
Sneaksolid pushed a commit to Sneaksolid/doom-emacs that referenced this issue Apr 13, 2022
To ensure lsp/eglot settings have precedence over local servers (e.g.
cider and lookup handlers).

Ref: clojure-emacs/cider#3170
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.

2 participants