When jacking in to shadow, the cider-repl-type is prematurely being reported as cljs while it is not (yet)

[ikappaki]

Hi,

It appears that the latest logic in #3282 is prematurely reporting that a shadow nREPL session, created with cider-jack-in-cljs, has switched to cljs while it is still clj, which is wrong.

E.g. calling (cider-repls 'cljs nil) just a little while after a cider-jack-in-cljs for a node repl, returns a cljs cider REPL even though the shadow repl has not yet received the commands from the client to switch to cljs.

This misbehavior is caught by the cljs jack in to-shadow integration test.

The test does the following to test that jacking in to a shadow cljs REPL works (high level description):

1. Sets up an empty shadow-cljs project
2. Jacks in to it
3. Waits until a cljs REPL is available, by periodically calling (cider-repls 'cljs nil) until it returns a cider cljs repl.
4. Evals some clojurescript command on the REPL to check that is working. In this case the command is (print :cljs? (some? *clojurescript-version*))

The expectation here is that the cljs REPL will print out the clojurescript version, but the REPL returns the following error (requires the improved integration anlalytics #3285 to see it)

"Syntax error compiling at (*cider-repl tmp/NiSeGW:localhost:44593(pending-cljs)*:1:15).\nUnable to resolve symbol: *clojurescript-version* in this context\n\"

This is because the REPL evaluated the code in Clojure but not in ClojureScript as expected. The issue is with step 3 above that is misreported that there is a cider cljs repl available, while there is none (yet) available.

This used to work as expected before #3282. Looking at the code, I've traced the error to be caused by the new connection capabilities as follows (high level description):

1. When jacking in to cljs, the cider-repl-type on the client buffer is set to pending-cljs.
2. When the client is conneted to the nREPL server, the new capabilities logic kicks in, and because the cider-repl-type is set to pending-cljs, the connection capability is set to cljs:

(defun cider--set-connection-capabilities (&optional conn-buffer)
  "Set `cider-connection-capabilities' for CONN-BUFFER during repl init.
See `cider-connection-capabilities'."
  (with-current-buffer (or conn-buffer (current-buffer))
    (setf cider-connection-capabilities
          (append
           (pcase (cider-runtime)
             ('clojure '(clojure jvm-compilation-errors))
             ('babashka '(babashka jvm-compilation-errors))
             (_ '()))
           (when
               (or
                (member cider-repl-type '(cljs pending-cljs))
                ;; This check is currently basically for nbb.
                ;; See `cider-sync-tooling-eval', but it is defined on a higher layer
                (nrepl-dict-get
                 (nrepl-sync-request:eval "cljs.core/demunge" (current-buffer) (cider-current-ns) 'tooling)
                 "value"))
             '(cljs))))))

3. The cider--match-repl-type was before only checking the buffers cider-repl-type local varabile to confirm whether the type is cljs, but now it also looks at the connection capabilities above:

(defun cider--match-repl-type (type buffer)
  "Return non-nil if TYPE matches BUFFER's REPL type."
  (let ((buffer-repl-type (cider-repl-type buffer)))
    (cond ((null buffer-repl-type) nil)
          ((or (null type) (eq type 'multi) (eq type 'any)) t)
          ((listp type) (member buffer-repl-type type))
          (t
           (or (string= type buffer-repl-type)
                ;; --- new logic, also check for connection capabilities ---
                 (let ((capabilities
                      (buffer-local-value 'cider-connection-capabilities buffer)))
                 (cond ((listp type)
                        (cl-some (lambda (it) (member it capabilities)) type))
                       (t (member type capabilities)))))))))

5. The cider-repl-type will start its life as pending-cljs, and in the case of shadow node repl connection will eventually move to clj and then cljs by the repl server.
6. In the old way the cider--match-repl-type would only return true after repl-type was reported by the server to be cljs. The shadow nREPL server will first report its repl-type as clj and after a while, after the ClojureScript code is compiled and brought up by shadow, it will then report cljs. Thus it will take a while before cider--match-repl-type returns true, and only if the clojurescript code has been succesfully compiled.
7. But with the new way the cider--match-repl-type will also look at the connection capabilities, the capabilities are immediately set to cljs (even though the cider-repl type is still pending-cljs), and the function will return true even though the REPL has not yet switched to cljs. This has a chain effect for (cider-repls 'cljs nil) to return the REPL even though it is not cljs, which is wrong.

To cocnclude it appears to me the new capabilities logic have broken the shadow cljs jack in functionality by immediately reporting the shadow REPL as being cljs while it is not. I think the logic has to be revisited.

Thanks,

cc @benjamin-asdf

[benjamin-asdf]

I see thanks for the in depth analysis. Yea indeed the repl type currently also signals "pending" or not. So we should preserve saying "pending" for pending-cljs.

(defun cider--match-repl-type (type buffer)
  "Return non-nil if TYPE matches BUFFER's REPL type."
  (let ((buffer-repl-type (cider-repl-type buffer)))
    (cond ((null buffer-repl-type) nil)
          ((or (null type) (eq type 'multi) (eq type 'any)) t)
          ((listp type) (member buffer-repl-type type))
          ;; If the repl is still pending we do not want to match cljs, unless we
          ;; specifically ask to match pendings-cljs
          ((string= buffer-repl-type 'pending-cljs)
           (string= type buffer-repl-type))
          (t
           (or (string= type buffer-repl-type)
               (let ((capabilities
                      (buffer-local-value 'cider-connection-capabilities buffer)))
                 (cond ((listp type)
                        (cl-some (lambda (it) (member it capabilities)) type))
                       (t (member type capabilities)))))))))

Here is one fix for this. I'll try to double check with running the tests.

[benjamin-asdf]

Turns out what I posted does not fix it, because during initialization the pending repl also becomes a clj repl. I think the best way would be to split the idea of "pending-cljs" into a speparate identity.

[benjamin-asdf]

#3291 turned out to be relatively straight forward