Fixing the debugger "wrong namespace" issues. #433

vspinu · 2017-08-08T01:03:49Z

Fixing #420, clojure-emacs/cider#2039 and clojure-emacs/cider#2004

expez · 2017-08-08T05:15:50Z

src/cider/nrepl/middleware/debug.clj

@@ -189,7 +190,7 @@ this map (identified by a key), and will `dissoc` it afterwards."}
  "Like `read`, but reply is sent through `debugger-message`.
  type is sent in the message as :input-type."
  [extras type prompt]
-  (let [key (u/random-uuid-str)
+  (let [key   (u/random-uuid-str)


Think you forgot to turn off your alignment minor mode :)

It's not really a minor mode - just a toggle in clojure-mode. ;-)

What's wrong with this? You don't want aligned key-val pairs in cider projects?

Yes. I'm not fond of this style and avoid it everywhere.

expez · 2017-08-08T05:17:36Z

Think a note in the changelog for the next cider release would be nice too. I know a lot of people have been tripped up by this one.

Great stuff as usual, @vspinu! 👍

bbatsov · 2017-08-08T05:47:55Z

src/cider/nrepl/middleware/debug.clj

@@ -227,18 +237,19 @@ this map (identified by a key), and will `dissoc` it afterwards."}
        nil)
      (finally
        (try
-          (in-ns ns)
+          ;; Restore original ns in case it was changed by the evaled code.


evaled -> evaluated

Was saving few chars not to get into fill.

bbatsov · 2017-08-08T05:48:29Z

src/cider/nrepl/middleware/debug.clj

-                  :forms @*tmp-forms*}]
+  `(let [~'META__ {:msg ~(let [{:keys [code id file line column ns]} *msg*]
+                           (-> {:code code,
+                                ;; Passing namespace as :original-ns stalls. Bug?


stalls what?

bbatsov · 2017-08-08T05:50:11Z

src/cider/nrepl/middleware/debug.clj

@@ -583,12 +602,7 @@ this map (identified by a key), and will `dissoc` it afterwards."}
                 (h (maybe-debug msg)))
      "debug-instrumented-defs" (instrumented-defs-reply msg)
      "debug-input" (when-let [pro (@promises (:key msg))]
-                      (swap! promises dissoc  (:key msg))


How do those changes fit with the rest?

There is no rest, the only consumer of that promise is the read-debug itself.

bbatsov · 2017-08-08T05:51:14Z

src/cider/nrepl/middleware/debug.clj

+  (let [has-debug?  (atom false)
+        ;; Don't return nil in reader (https://dev.clojure.org/jira/browse/CLJ-1138)
+        fake-reader (fn [x] (reset! has-debug? true) x)]
+    (binding [*ns*           (find-ns (symbol (or ns "user")))


We don't really use alignment in bindings in cider-nrepl.

Let bindings as well?

bbatsov · 2017-08-08T05:52:33Z

src/cider/nrepl/middleware/debug.clj

-      (binding [*tmp-locals* locals]
+      (binding [*tmp-locals* (:locals extras)
+                ;; evaluate in the instrumentation ns
+                *ns*         (find-ns (symbol (:original-ns extras)))]


Same here. What will happen if :original-ns is nil here?

:original-ns can never be nil, it's always assigned in with-initial-debug-bindings.

bbatsov · 2017-08-08T05:53:34Z

src/cider/nrepl/middleware/debug.clj

-  [form locals]
+  If an exception is thrown, it is caught and sent to the client, and this
+  function returns nil. `extras` is a metadata map received from `break'."
+  [form extras]


I'd name extras something else - without reading its docstring it was hard for me to figure out what it is. break-state perhaps?

Good sugestion. That map contains both current breakpoint state and current debug session state, but I think break-state is informative of both.

Maybe debugger-state then with two maps inside of it?

bbatsov · 2017-08-08T05:53:49Z

src/cider/nrepl/middleware/debug.clj

          (catch IllegalStateException _))))))

 (defn- read-debug-eval-expression
  "Read and eval an expression from the client.
-  extras is a map to be added to the message, and prompt is added into
+  `extras` is a map received from `break`, and `prompt` is added into


bbatsov · 2017-08-08T05:56:45Z

Think a note in the changelog for the next cider release would be nice too. I know a lot of people have been tripped up by this one.

Yeah, once this is merged it should have a changelog entry for sure. Probably it will be the final bug-fix for 0.15.1. Seems no other major problems with 0.15 have been discovered in the weeks after its release.

Fixes clojure-emacs/cider#2039

... and eval during debugger session within that ns Fixes clojure-emacs/cider#2004

…tation ns

Untangle `inspect-then-read-command`, `stack-then-read-command`

- META__ --> STATE__ - extras --> dbg-state - read-debug --> read-debug-input - read-debug-eval-expression --> read-eval-expression

vspinu · 2017-08-08T22:08:11Z

Good to go AFAIC. Addressed comments, fixed tests and added some extras:

improved and disentangled error handling during debug eval commands (eval, inspect, inject, locals),
refactored naming conventions here and there

Malabarba · 2017-08-08T23:21:51Z

src/cider/nrepl/middleware/debug.clj

+                          {:status :eval-error
+                           :causes [(let [causes# (stacktrace/analyze-causes e# (:pprint-fn *msg*))]
+                                      (when (coll? causes#) (last causes#)))]})))
+                    ::error))]


It would be more robust (and just as easy) to use a generated symbol here.

Good point. Changed.

Malabarba · 2017-08-10T00:17:54Z

LGTM

bbatsov · 2017-08-10T05:49:52Z

👍

expez reviewed Aug 8, 2017

View reviewed changes

bbatsov reviewed Aug 8, 2017

View reviewed changes

Instrument forms in correct *ns*

75cc977

Fixes clojure-emacs/cider#2039

vspinu force-pushed the debug-ns branch from 1f26c1a to 70a27d4 Compare August 8, 2017 18:18

vspinu added 6 commits August 8, 2017 23:12

Track instrumentation ns within debugged forms

063e578

... and eval during debugger session within that ns Fixes clojure-emacs/cider#2004

[Fix clojure-emacs#420] Read debugger eval forms within the instrumen…

c13b80a

…tation ns

Improve error handling within debugger commands

132bb27

Untangle `inspect-then-read-command`, `stack-then-read-command`

Fix debug tests

848e103

Improve on naming conventions in debug.clj

5c48d50

- META__ --> STATE__ - extras --> dbg-state - read-debug --> read-debug-input - read-debug-eval-expression --> read-eval-expression

Use recur in read-debug-command

ee1307a

vspinu force-pushed the debug-ns branch from 70a27d4 to ee1307a Compare August 8, 2017 21:54

Malabarba reviewed Aug 8, 2017

View reviewed changes

Use gensym as error marker in try-if-let

e3bf4de

vspinu mentioned this pull request Aug 9, 2017

Document debugger fixes clojure-emacs/cider#2072

Merged

Malabarba approved these changes Aug 9, 2017

View reviewed changes

bbatsov merged commit b1f85bd into clojure-emacs:master Aug 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing the debugger "wrong namespace" issues. #433

Fixing the debugger "wrong namespace" issues. #433

vspinu commented Aug 8, 2017

expez Aug 8, 2017

bbatsov Aug 8, 2017

vspinu Aug 8, 2017

bbatsov Aug 8, 2017

expez commented Aug 8, 2017

bbatsov Aug 8, 2017

vspinu Aug 8, 2017

bbatsov Aug 8, 2017

bbatsov Aug 8, 2017

vspinu Aug 8, 2017

bbatsov Aug 8, 2017

bbatsov Aug 8, 2017

vspinu Aug 8, 2017

bbatsov Aug 8, 2017

bbatsov Aug 8, 2017

vspinu Aug 8, 2017

bbatsov Aug 8, 2017

vspinu Aug 8, 2017

bbatsov Aug 8, 2017

bbatsov Aug 8, 2017

bbatsov commented Aug 8, 2017

vspinu commented Aug 8, 2017

Malabarba Aug 8, 2017

vspinu Aug 9, 2017

Malabarba commented Aug 10, 2017

bbatsov commented Aug 10, 2017

Fixing the debugger "wrong namespace" issues. #433

Fixing the debugger "wrong namespace" issues. #433

Conversation

vspinu commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

expez commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Aug 8, 2017

vspinu commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Malabarba commented Aug 10, 2017

bbatsov commented Aug 10, 2017