CIDER 1.1.1 Can't Handle Errors Without Stack Traces

[timvisher]

Expected behavior

Be able to eval (< 1 nil) and get a reasonable stack trace.

Actual behavior

CIDER panics with *Messages*

error in process filter: cider-stacktrace-render-frame: Format specifier doesn’t match argument type
error in process filter: Format specifier doesn’t match argument type

and a Stacktrace buffer like

1. Unhandled java.lang.NullPointerException
   (No message)

Steps to reproduce the problem

1. Open a clojure file against a JVM without -XX:-OmitStackTraceInFastThrow passed.
2. Insert (< 1 nil)
3. Whack M-x cider-eval-last-sexp RET

This can be fixed by passing -XX:-OmitStackTraceInFastThrow to the underlying JVM via the appropriate channel.

For instance:

$ cat deps.edn
{:deps
 …
 :aliases {:dev {:jvm-opts ["-XX:-OmitStackTraceInFastThrow"]}}}

Followed by M-0 M-x cider-jack-in RET and appending:dev to the CL like

jack-in command: /usr/local/bin/clojure … -M:cider/nrepl:dev`

Environment & Version information

CIDER version information

;; CIDER 1.1.1 (Plovdiv), nREPL 0.8.3
;; Clojure 1.10.3, Java 16.0.1

clojure version

$ clojure --version
Clojure CLI version 1.10.3.855

Emacs version

GNU Emacs 27.2 (build 1, x86_64-apple-darwin18.7.0) of 2021-03-25

Operating system

$ sw_vers
ProductName:    Mac OS X
ProductVersion: 10.14.6
BuildVersion:   18G9216

[bbatsov]

I'm not sure what's the best behaviour in such scenarios, as some stack frames seem to be missing. I recall this problem has been around for ages, but we never decided on the best approach to tackle it. E.g.:

• adding the JVM opt automatically on jack-in
• displaying some more informative message that the stacktrace is looking offf
• try to somehow salvage the situation and display some stacktrace in CIDER

[timvisher]

Personally speaking I think adding that option by default seems like a great addition. I can't think of any interactive development situation where I wouldn't want it on, but maybe I'm not thinking of something common?

[timvisher]

I don't know nearly enough about the stack trace processing code but is it possible to somehow handle this case in the code that's emitting the

error in process filter: cider-stacktrace-render-frame: Format specifier doesn’t match argument type
error in process filter: Format specifier doesn’t match argument type

messages?

While I think that it makes sense to add the JVM option by default it's actually those messages that make it feel like a bug in CIDER to me. If I just got a stack trace without a trace but CIDER wasn't raising errors I think that might be slightly better?

[timvisher]

Also I guess I should've linked the discussion in #cider :) https://clojurians.slack.com/archives/C0617A8PQ/p1624726767180000

Alex Miller even says that he's personally fixed this issue in Clojure several times.

[yuhan0]

Edit: ignore this post!

Here's a commit that I made more than a year ago on my local branch of Cider that I think tackles the surface symptoms of this same issue (the stacktrace buffer's formatting errors).
In any case it was a hacky workaround, just pasting it here in case it helps anyone in their way to a better solution:

diff --git a/cider-stacktrace.el b/cider-stacktrace.el
index 73bf2636..012c14a6 100644
--- a/cider-stacktrace.el
+++ b/cider-stacktrace.el
@@ -796,15 +796,14 @@ cider-stacktrace-render-cause
   (with-current-buffer buffer
     (nrepl-dbind-response cause (class message data spec stacktrace)
       (let ((indent "   ")
-            (class-face 'cider-stacktrace-error-class-face)
-            (message-face 'cider-stacktrace-error-message-face))
+            (message-face 'cider-stacktrace-error-message-face)
+            (level-0-message (concat (propertize note 'font-lock-face 'font-lock-comment-face) " "
+                                     (propertize class 'font-lock-face 'cider-stacktrace-error-class-face))))
+        ;; (message (propertize level-0-message 'face 'cider-test-failure-face))
         (cider-propertize-region `(cause ,num)
           ;; Detail level 0: exception class
           (cider-propertize-region '(detail 0)
-            (insert (format "%d. " num)
-                    (propertize note 'font-lock-face 'font-lock-comment-face) " "
-                    (propertize class 'font-lock-face class-face)
-                    "\n"))
+            (insert (format "%d. " num) level-0-message "\n"))
           ;; Detail level 1: message + ex-data
           (cider-propertize-region '(detail 1)
             (if (equal class "clojure.lang.Compiler$CompilerException")
@@ -852,7 +851,8 @@ cider-stacktrace-initialize
               (goto-char (next-single-property-change (point) 'compile-error))
             (progn
               (while (cider-stacktrace-next-cause))
-              (goto-char (next-single-property-change (point) 'flags)))))))))
+              (goto-char (next-single-property-change (point) 'flags nil (point-max))))))))))
+
 
 (defun cider-stacktrace-render (buffer causes &optional error-types)
   "Emit into BUFFER useful stacktrace information for the CAUSES.

[bbatsov]

@yuhan0 Which part of the code was the problematic one? Was that the modified insert? I'm also curious how are the empty frames rendered with your changes. Perhaps you can share some screenshot?

[yuhan0]

I just got around to looking at this - turns out that change was for something else entirely! Sorry for any confusion 😓

The issue is in cider-stacktrace-render-frame - when this 'optimization' happens the argument frame is nil and further destructuring it into nils causes the formatting string error.

I agree with the discussion in #3023 that we should just check for this and point users to the relevant troubleshooting page, instead of setting any JVM option by default.

Here's what it might look like:

If that's ok I'll submit a PR :) But first the website should be updated to reflect the commit linked above 2f9bec5

[vemv]

A PR SGTM (FWIW!)

I'd make sure to have at least one CI job run without the sensitive flag (i.e. probably duplicating the entire matrix would be overkill)

[bbatsov]

@yuhan0 I've updated the docs site, so you can go ahead with the PR.

I'd make sure to have at least one CI job run without the sensitive flag (i.e. probably duplicating the entire matrix would be overkill)

With the proposed solution I don't think we need any changes to the CI setup. We can just have add some unit test that mocks the nil stack frame, although I'm not sure there are any stacktrace related tests on the Elisp side. I'll take a simple PR without tests gladly, given the circumstances.

[vemv]

My thinking is that creating a test for the < just covers one specific bug.

By running a whole CI job with an alt flag set up, we implicitly exercise a lot of code for that flag value.

[bbatsov]

My point was mostly that there are almost no integration tests in CIDER that check something against a real nREPL server. The tests typically stub the nREPL replies. That's why I didn't see why we'd run the tests with/without this JVM flag.

[timvisher]

This is what OSS is all about. I submit an issue and offer to do the work and then someone else beats me to it and makes my life better.

Thanks everyone! :-D

[vemv]

My point was mostly that there are almost no integration tests in CIDER that check something against a real nREPL server.

I just realised now that we're not in the cider-nrepl repo 🤦‍♂️ sorry for the noise.

[vemv]

I wonder though how things are handled cider-nrepl side. There might be gains to be made especially for clients other than cider.el.