Debugger tweaks

CHANGELOG.md

@@ -6,6 +6,9 @@
 
 * Bump `cljfmt` to 0.9.2.
 * Bump `puget` to 1.3.4.
+* [#769](https://github.com/clojure-emacs/cider-nrepl/issues/769): Introduce new `#break!` and `#dbg!` reader macros for breaking on exception
+* [#772](https://github.com/clojure-emacs/cider-nrepl/issues/772): Improve debugger heuristics on "interesting" breakpoints (Always break on forms tagged with #break, including vectors and maps)
+* The `wrap-debug` middleware now adds an optional `:caught-msg` key to the `eval` op response, containing the exception message when caught by the debugger. 
 
 ## 0.30.0 (2023-01-31)
 

doc/modules/ROOT/pages/hacking.adoc

@@ -1,12 +1,12 @@
 = Hacking on cider-nrepl
 
-Hacking on cider-nrepl requires nothing bit a bit of knowledge of Clojure and nREPL.
+Hacking on cider-nrepl requires nothing but a bit of knowledge of Clojure and nREPL.
 In this section we'll take a look at some practical tips to make you more productive
 while working on the project.
 
 == Makefile
 
-cider-nrepl has some pretty complicated Lein profiles, as it has to deal with multiple version of
+cider-nrepl has some pretty complicated Lein profiles, as it has to deal with multiple versions of
 Clojure and ClojureScript, plus dependency inlining with Mr. Anderson. That's why we've
 added a good old `Makefile` to save you the trouble of having to think about the profiles
 and just focus on the tasks at hand.

src/cider/nrepl/middleware/debug.clj

@@ -284,15 +284,18 @@ this map (identified by a key), and will `dissoc` it afterwards."}
          :rendered pr-str)))
 
 (defn- debug-stacktrace
-  "Create a dummy exception, send its stack."
-  []
+  "Send the stacktrace of `value` if it is an exception.
+  Otherwise, create a dummy exception to view the call stack at the current location."
+  [value]
   (debugger-send
    {:status :stack
-    :causes [{:class "StackTrace"
-              :message "Harmless user-requested stacktrace"
-              :stacktrace (-> (Exception. "Dummy")
-                              (stacktrace.analyzer/analyze (::print/print-fn *msg*))
-                              last :stacktrace)}]}))
+    :causes (if (instance? Throwable value)
+              (stacktrace.analyzer/analyze value (::print/print-fn *msg*))
+              [{:class      "StackTrace"
+                :message    "Harmless user-requested stacktrace"
+                :stacktrace (-> (Exception. "Dummy")
+                                (stacktrace.analyzer/analyze (::print/print-fn *msg*))
+                                last :stacktrace)}])}))
 
 (def debug-commands
   "An unsorted set of commands supported by the debugger."
@@ -365,7 +368,7 @@ this map (identified by a key), and will `dissoc` it afterwards."}
                         value)
         :here       (do (skip-breaks! :before coord (:code dbg-state) force?)
                         value)
-        :stacktrace (do (debug-stacktrace)
+        :stacktrace (do (debug-stacktrace value)
                         (recur value dbg-state))
         :trace      (do (skip-breaks! :trace)
                         value)
@@ -488,6 +491,13 @@ this map (identified by a key), and will `dissoc` it afterwards."}
      ~@body))
 
 (defmacro breakpoint-with-initial-debug-bindings
+  {:style/indent 1}
+  [form dbg-state original-form]
+  `(with-initial-debug-bindings
+     (breakpoint
+      ~form ~dbg-state ~original-form)))
+
+(defmacro breakpoint-if-interesting-with-initial-debug-bindings
   {:style/indent 1}
   [form dbg-state original-form]
   `(with-initial-debug-bindings
@@ -568,68 +578,68 @@ this map (identified by a key), and will `dissoc` it afterwards."}
       (and (seq? form)
            (irrelevant-return-value-forms (first form)))))
 
+(defmacro breakpoint
+  "Wrap form in a breakpoint unconditionally."
+  [form {:keys [coor] :as dbg-state} original-form]
+  (let [condition (:break/when (meta form))]
+    (if condition
+      ;; If there is a condition and it is falsy, we need to skip
+      ;; the current level (:deeper than parent coor), but only
+      ;; once. Next time, we need to test the condition again.
+      `(let [old-breaks# @*skip-breaks*]
+         (when-not ~condition
+           (skip-breaks! :deeper ~(vec (butlast coor)) (:code (:msg ~'STATE__)) false))
+         (try
+           (expand-break ~form ~dbg-state ~original-form)
+           ;; in case :continue-all was requested in a deeper level
+           ;; we don't want go back to the old-breaks
+           (finally (when (not= :all (:mode @*skip-breaks*))
+                      (reset! *skip-breaks* old-breaks#)))))
+      `(expand-break ~form ~dbg-state ~original-form))))
+
 (defmacro breakpoint-if-interesting
   "Wrap form in a breakpoint if it looks interesting.
   Uninteresting forms are symbols that resolve to `clojure.core`
   (taking locals into account), and sexps whose head is present in
   `irrelevant-return-value-forms`. Used as :breakfunction in `tag-form`."
-  [form {:keys [coor] :as dbg-state} original-form]
+  [form dbg-state original-form]
   (if (uninteresting-form? &env form)
     form
-    (let [condition (:break/when (meta form))]
-      (if condition
-        ;; If there is a condition and it is falsy, we need to skip
-        ;; the current level (:deeper than parent coor), but only
-        ;; once. Next time, we need to test the condition again.
-        `(let [old-breaks# @*skip-breaks*]
-           (when-not ~condition
-             (skip-breaks! :deeper ~(vec (butlast coor)) (:code (:msg ~'STATE__)) false))
-           (try
-             (expand-break ~form ~dbg-state ~original-form)
-             ;; in case :continue-all was requested in a deeper level
-             ;; we don't want go back to the old-breaks
-             (finally (when (not= :all (:mode @*skip-breaks*))
-                        (reset! *skip-breaks* old-breaks#)))))
-        `(expand-break ~form ~dbg-state ~original-form)))))
+    `(breakpoint ~form ~dbg-state ~original-form)))
 
 (defmacro breakpoint-if-exception
   "Wrap form in a try-catch that has breakpoint on exception.
-  Ignores uninteresting forms.
-  Uninteresting forms are symbols that resolve to `clojure.core`
-  (taking locals into account), and sexps whose head is present in
-  `irrelevant-return-value-forms`. Used as :breakfunction in `tag-form`."
+  Used as :breakfunction in `tag-form`."
   [form dbg-state original-form]
-  (if (uninteresting-form? &env form)
-    form
-    `(try ~form
-          (catch Throwable ex#
-            (let [exn-message# (.getMessage ex#)
-                  break-result# (expand-break exn-message# ~dbg-state ~original-form)]
-              (if  (= exn-message# break-result#)
-                ;; if they continued then rethrow the exception
-                (throw ex#)
-                ;; otherwise return the value they injected
-                break-result#))))))
+  `(try ~form
+        (catch Throwable ex#
+          (let [~'STATE__ (assoc-in ~'STATE__ [:msg :caught-msg] (.getMessage ex#))
+                break-result#  (expand-break ex# ~dbg-state ~original-form)]
+            (if (= ex# break-result#)
+              ;; if they continued then rethrow the exception
+              (throw ex#)
+              ;; otherwise return the value they injected
+              break-result#)))))
 
 ;;; ## Data readers
 ;;
 ;; Set in `src/data_readers.clj`.
 (defn breakpoint-reader
   "#break reader. Mark `form` for breakpointing."
   [form]
-  (ins/tag-form form #'breakpoint-with-initial-debug-bindings))
+  (ins/tag-form form #'breakpoint-with-initial-debug-bindings true))
 
 (defn debug-reader
   "#dbg reader. Mark all forms in `form` for breakpointing.
   `form` itself is also marked."
   [form]
   (ins/tag-form (ins/tag-form-recursively form #'breakpoint-if-interesting)
-                #'breakpoint-with-initial-debug-bindings))
+                #'breakpoint-if-interesting-with-initial-debug-bindings))
 
 (defn break-on-exception-reader
   "#exn reader. Wrap `form` in try-catch and break only on exception"
   [form]
-  (ins/tag-form form #'breakpoint-if-exception-with-initial-debug-bindings))
+  (ins/tag-form form #'breakpoint-if-exception-with-initial-debug-bindings true))
 
 (defn debug-on-exception-reader
   "#dbgexn reader. Mark all forms in `form` for breakpointing on exception.
@@ -655,6 +665,10 @@ this map (identified by a key), and will `dissoc` it afterwards."}
                 (eval form1)))
           (throw e))))))
 
+(def ^:dynamic *debug-data-readers*
+  "Reader macros like #dbg which cause code to be instrumented when present."
+  '#{dbg exn dbgexn break light})
+
 ;;; ## Middleware
 (defn- maybe-debug
   "Return msg, prepared for debugging if code contains debugging macros."
@@ -666,8 +680,8 @@ this map (identified by a key), and will `dissoc` it afterwards."}
         fake-reader (fn [x] (reset! has-debug? true) x)]
     (binding [*ns* (find-ns (symbol (or ns "user")))
               *data-readers* (->> (repeat fake-reader)
-                                  (interleave '[dbg exn dbgexn break light])
-                                  (apply assoc *data-readers*))]
+                                  (zipmap *debug-data-readers*)
+                                  (merge *data-readers*))]
       (try
         ;; new-line in REPL always throws; skip for debug convenience
         (when (> (count code) 3)

src/cider/nrepl/middleware/util/instrument.clj

@@ -225,9 +225,11 @@
     ;; Other coll types are safe, so we go inside them and only
     ;; instrument what's interesting.
     ;; Do we also need to check for seq?
-    coll? (doall (instrument-coll form))
+    coll? (cond-> (doall (instrument-coll form))
+            (::do-break (meta form)) (with-break))
     ;; Other things are uninteresting, literals or unreadable objects.
-    form))
+    (cond-> form
+      (::do-break (meta form)) (with-break))))
 
 ;;;; ## Pre-instrumentation
 ;;;
@@ -294,9 +296,14 @@
   "Tag form to be instrumented with breakfunction.
   This sets the ::breakfunction metadata of form, which can then be
   used by `instrument-tagged-code`. See this function for the meaning
-  of breakfunction."
-  [form breakfunction]
-  (m/merge-meta form {::breakfunction breakfunction}))
+  of breakfunction.
+  When `do-break?` is true it tells the instrumenter to wrap the form
+  with a breakpoint regardless of other heuristics."
+  ([form breakfunction]
+   (tag-form form breakfunction false))
+  ([form breakfunction do-break?]
+   (m/merge-meta form {::breakfunction breakfunction}
+     (when do-break? {::do-break true}))))
 
 (defn tag-form-recursively
   "Like `tag-form` but also tag all forms inside the given form."

src/data_readers.clj

@@ -1,5 +1,5 @@
 {dbg cider.nrepl.middleware.debug/debug-reader
  break cider.nrepl.middleware.debug/breakpoint-reader
- exn cider.nrepl.middleware.debug/break-on-exception-reader
- dbgexn cider.nrepl.middleware.debug/debug-on-exception-reader
+ break! cider.nrepl.middleware.debug/break-on-exception-reader
+ dbg! cider.nrepl.middleware.debug/debug-on-exception-reader
  light cider.nrepl.middleware.enlighten/light-reader}

test/clj/cider/nrepl/middleware/content_type_test.clj

@@ -44,14 +44,15 @@
                         [:body :content-type :content-transfer-encoding :status]))))
 
   (testing "java.io.File"
-    (let [f (java.io.File/createTempFile "foo" ".txt")]
+    (let [f (java.io.File/createTempFile "foo" ".txt")
+          path (.getCanonicalPath f)]
       (is (= {:body ""
               :content-type
               ["message/external-body"
-               {:URL (str "file:" f) :access-type "URL"}]
+               {:URL (str "file:" path) :access-type "URL"}]
               :status #{"done"}}
              (-> {:op "eval"
-                  :code (str "(java.io.File. " (pr-str (str f)) ")")
+                  :code (str "(java.io.File. " (pr-str path) ")")
                   :content-type "true"}
                  session/message
                  (select-keys [:body :content-type :content-transfer-encoding :status]))))))

test/clj/cider/nrepl/middleware/debug_integration_test.clj

@@ -136,18 +136,59 @@
         (f)))))
 
 (deftest debug-expression-test
+  (--> :eval "(ns user.test.debug)")
+  (<-- {:ns "user.test.debug"})
+  (<-- {:status ["done"]})
+
   (testing "normal eval (no debugging)"
     (--> :eval "(+ 2 3)")
     (<-- {:value "5"})
     (<-- {:status ["done"]}))
 
-  (testing "#break reader, no breakpoints"
-    ;; This code has only literals and core functions, so it should
-    ;; not break, but should just return the value
+  (testing "Top level breakpoints do not trigger"
+    ;; Since triggering a breakpoint to inspect the toplevel value is no different
+    ;; from returning the value.
     (--> :eval "#break (+ 2 3)")
     (<-- {:value "5"})
     (<-- {:status ["done"]}))
 
+  (testing "#break on return value of call"
+    (--> :eval "(do #break (+ 2 3) :ok)")
+    (<-- {:debug-value "5"})
+    (--> :next)
+    (<-- {:value ":ok"})
+    (<-- {:status ["done"]}))
+
+  (testing "#dbg reader on uninteresting forms"
+    ;; The return value of a def form is uninteresting, as well as the sub forms
+    ;; which are just core vars and literals.
+    ;; #dbg should not break on any of them and just return the value.
+    (--> :eval "(do #dbg (def foo [2 + {:skip \"me\"}]) foo)")
+    (<-- {:value "[2 #function[clojure.core/+] {:skip \"me\"}]"})
+    (<-- {:status ["done"]}))
+
+  (testing "#break reader on uninteresting forms"
+    ;; #break signifies that the user explicitly wants to set a breakpoint there,
+    ;; so ignore any of #dbg reader's heuristics on what an interesting form is.
+    (--> :eval "(let [a 1] #break [2 3], #break {:a a}, #break + #break (def foo 4) foo) ")
+    (<-- {:debug-value "[2 3]"})
+    (--> :next)
+    (<-- {:debug-value "{:a 1}"})
+    (--> :next)
+    (<-- {:debug-value "#function[clojure.core/+]"})
+    (--> :next)
+    (<-- {:debug-value "#'user.test.debug/foo"})
+    (--> :next)
+    (<-- {:value "4"})
+    (<-- {:status ["done"]}))
+
+  (testing "#break reader on literals"
+    ;; The instrumenter cannot tag literals which don't support metadata,
+    ;; so #break does not work on them.
+    (--> :eval "(do #break :kwd, #break 123, #break \"string\" :ok)")
+    (<-- {:value ":ok"})
+    (<-- {:status ["done"]}))
+
   (testing "#dbg reader, with breaks"
     (--> :eval
          "#dbg

test/clj/cider/nrepl/middleware/info_test.clj

@@ -310,8 +310,8 @@
 
     (testing "java interop method with multiple classes"
       (let [response (session/message {:op "eldoc" :sym ".length" :ns "cider.nrepl.middleware.info-test"})]
-        (is (= (:class response)
-               ["java.lang.String" "java.lang.StringBuffer" "java.lang.CharSequence" "java.lang.StringBuilder"])
+        (is (every? (set (:class response)) ;; is a superset of:
+                    ["java.lang.String" "java.lang.StringBuffer" "java.lang.CharSequence" "java.lang.StringBuilder"])
             (pr-str response))
         (is (= (:member response) "length"))
         (is (not (contains? response :ns)))