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

Debugging in Cider #1019

Merged
merged 2 commits into from
Mar 28, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@

### New features

* [#1019](https://github.com/clojure-emacs/cider/pull/1019): New file, cider-debug.el.
Provides a new command, `cider-debug-defun-at-point`, bound to <kbd>C-u C-M-x</kbd>.
Interactively debug top-level clojure forms.

* New defcustom, `cider-auto-select-test-report-buffer` (boolean).
Controls whether the test report buffer is selected after running a test. Defaults to true.
* Trigger Grimoire doc lookup from doc buffers by pressing <kbd>g</kbd> (in Emacs) and <kbd>G</kbd> (in browser).
Expand Down Expand Up @@ -36,6 +40,8 @@

### Changes

* [#1019](https://github.com/clojure-emacs/cider/pull/1019):
<kbd>C-u C-M-x</kbd> no longer does `eval-defun`+print-result. Instead it debugs the form at point.
* [#854](https://github.com/clojure-emacs/cider/pull/854) Error navigation now
favors line information reported by the stacktrace, being more detailed than
the info reported by `info` middleware.
Expand Down
16 changes: 15 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,8 @@ Keyboard shortcut | Description
<kbd>C-c M-p</kbd> | Load the form preceding point in the REPL buffer.
<kbd>C-c C-p</kbd> | Evaluate the form preceding point and pretty-print the result in a popup buffer.
<kbd>C-c C-f</kbd> | Evaluate the top level form under point and pretty-print the result in a popup buffer.
<kbd>C-M-x</kbd> <kbd>C-c C-c</kbd> | Evaluate the top level form under point and display the result in the echo area. If invoked with a prefix argument, insert the result into the current buffer.
<kbd>C-M-x</kbd> <kbd>C-c C-c</kbd> | Evaluate the top level form under point and display the result in the echo area.
<kbd>C-u C-M-x</kbd> <kbd>C-u C-c C-c</kbd> | Debug the top level form under point and walk through its evaluation
<kbd>C-c C-r</kbd> | Evaluate the region and display the result in the echo area.
<kbd>C-c C-b</kbd> | Interrupt any pending evaluations.
<kbd>C-c C-m</kbd> | Invoke `macroexpand-1` on the form at point and display the result in a macroexpansion buffer. If invoked with a prefix argument, `macroexpand` is used instead of `macroexpand-1`.
Expand Down Expand Up @@ -844,6 +845,19 @@ Keyboard shortcut | Description
<kbd>d</kbd> | toggle display of duplicate frames
<kbd>a</kbd> | toggle display of all frames

### cider-debug
<!-- Technically this is not a mode (yet), but let's not burden the user with that knowledge. -->

cider-debug (invoked with <kbd>C-u C-M-x</kbd>) tries to be consistent with
Edebug. So it makes available the following bindings while stepping through
code.

Keyboard shortcut | Description
--------------------------------|-------------------------------
<kbd>n</kbd> | Next step
<kbd>c</kbd> | Continue without stopping
<kbd>i</kbd> | Inject a value into running code

### Managing multiple sessions

You can connect to multiple nREPL servers using <kbd>M-x
Expand Down
174 changes: 174 additions & 0 deletions cider-debug.el
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
;;; cider-debug.el --- CIDER interaction with clj-debugger -*- lexical-binding: t; -*-

;; Copyright (C) 2015 Artur Malabarba

;; Author: Artur Malabarba <bruce.connor.am@gmail.com>

;; This program is free software; you can redistribute it and/or modify
;; it under the terms of the GNU General Public License as published by
;; the Free Software Foundation, either version 3 of the License, or
;; (at your option) any later version.

;; This program is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
;; GNU General Public License for more details.

;; You should have received a copy of the GNU General Public License
;; along with this program. If not, see <http://www.gnu.org/licenses/>.

;;; Commentary:

;; Instrument code with `cider-debug-defun-at-point', and when the code is
;; executed cider-debug will kick in. See this function's doc for more
;; information.

;;; Code:

(require 'nrepl-client)
(require 'cider-interaction)

(defvar cider--current-debug-value nil
"Last value received from the debugger.
Is printed by `cider--debug-read-command' while stepping through
code.")

(defconst cider--instrument-format
(concat "(cider.nrepl.middleware.debug/instrument-and-eval"
;; filename and point are passed in a map. Eventually, this should be
;; part of the message (which the nrepl sees as a map anyway).
" {:filename %S :point %S} '%s)")
"Format to instrument an expression given a file and a coordinate.")


;;; Implementation
(defun cider--debug-init-connection ()
"Initialize a connection with clj-debugger."
(nrepl-send-request
'("op" "init-debugger")
(let ((connection-buffer (nrepl-current-connection-buffer)))
(lambda (response)
(nrepl-dbind-response response (value coor filename point status id)
(if (not (member "done" status))
(cider--handle-debug value coor filename point connection-buffer)
(puthash id (gethash id nrepl-pending-requests)
nrepl-completed-requests)
(remhash id nrepl-pending-requests)))))))

(defun cider--forward-sexp (n)
"Move forward N logical sexps.
This will skip over sexps that don't represent objects, such as ^{}."
(while (> n 0)
;; Non-logical sexps.
(while (progn (forward-sexp 1)
(forward-sexp -1)
(looking-at-p "\\^"))
(forward-sexp 1))
;; The actual sexp
(forward-sexp 1)
(setq n (1- n))))

(defun cider--handle-debug (value coordinates file point connection-buffer)
"Handle debugging notification.
VALUE is saved in `cider--current-debug-value' to be printed
while waiting for user input.
COORDINATES, FILE and POINT are used to place point at the instrumented sexp.
CONNECTION-BUFFER is the nrepl buffer."
;; Be ready to prompt the user when debugger.core/break is
;; triggers a need-input request.
(nrepl-push-input-handler #'cider--need-debug-input connection-buffer)

;; Navigate to the instrumented sexp, wherever we might be.
(find-file file)
;; Position of the sexp.
(goto-char point)
(condition-case nil
;; Make sure it is a list.
(let ((coordinates (append coordinates nil)))
;; Navigate through sexps inside the sexp.
(while coordinates
(down-list)
(cider--forward-sexp (pop coordinates)))
;; Place point at the end of instrumented sexp.
(cider--forward-sexp 1))
;; Avoid throwing actual errors, since this happens on every breakpoint.
(error (message "Can't find instrumented sexp, did you edit the source?")))
;; Prepare to notify the user.
(setq cider--current-debug-value value))

(defun cider--debug-read-command ()
"Receive input from the user representing a command to do."
(let ((cider-interactive-eval-result-prefix
"(n)ext (c)ontinue (i)nject => "))
(cider--display-interactive-eval-result
cider--current-debug-value))
(let ((input
(cl-case (read-char)
;; These keys were chosen to match edebug rather than clj-debugger.
(?n "(c)")
(?c "(q)")
;; Inject
(?i (condition-case nil
(concat (read-from-minibuffer "Expression to inject (non-nil): ")
"\n(c)")
(quit nil))))))
(if (and input (not (string= "" input)))
(progn (setq cider--current-debug-value nil)
input)
(cider--debug-read-command))))

(defun cider--need-debug-input (buffer)
"Handle an need-input request from BUFFER."
(with-current-buffer buffer
(nrepl-request:stdin
;; For now we immediately try to read-char. Ideally, this will
;; be done in a minor-mode (like edebug does) so that the user
;; isn't blocked from doing anything else.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and also bound to some rarely used but easelly accessible combos (like C-Q and C-C) to avoidinterference with the editing. The only thing I dislike about edebug is the read-only lock.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it read-only is a precaution we might need as well. The way it's currently designed, if the user edits the source file inside or before the instrumented sexp, then the debugger will start to misbehave. It will probably place point in the wrong places and might end up throwing an error while trying to navigate the code (due to calling forward-sexp in an invalid location).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the user edits the source file inside or before the instrumented sexp, then the debugger will start to misbehave.

The debugging can be aborted if that happens, similarly to what edebug does when the source has been modified.

Alternatively we can check the region for modifications, and if there have been such since the last instrumentation, re-instrument the definition in the background. Not sure if that's possible at debugger.core level.

(concat (cider--debug-read-command) "\n")
(cider-stdin-handler buffer))))


;;; User commands
;;;###autoload
(defun cider-debug-defun-at-point ()
"Instrument the top-level expression at point.
If it is a defn, dispatch the instrumented definition. Otherwise,
immediately evaluate the instrumented expression.

While debugged code is being evaluated, the user is taken through the
source code and displayed the value of various expressions. At each step,
the following keys are available:
n: Next step
c: Continue without stopping
i: Inject a value at this point"
(interactive)
(cider--debug-init-connection)
(let* ((expression (cider-defun-at-point))
(eval-buffer (current-buffer))
(position (cider-defun-at-point-start-pos))
(prefix
(if (string-match "\\`(defn-? " expression)
"Instrumented => " "=> "))
(instrumented (format cider--instrument-format
(buffer-file-name)
position
expression)))
;; Once the code has been instrumented, it can be sent as a
;; regular evaluation. Any debug messages will be received by the
;; callback specified in `cider--debug-init-connection'.
(cider-interactive-source-tracking-eval
instrumented position
(nrepl-make-response-handler (current-buffer)
(lambda (_buffer value)
(let ((cider-interactive-eval-result-prefix prefix))
(cider--display-interactive-eval-result value)))
;; Below is the default for `cider-interactive-source-tracking-eval'.
(lambda (_buffer out)
(cider-emit-interactive-eval-output out))
(lambda (_buffer err)
(cider-emit-interactive-eval-err-output err)
(cider-handle-compilation-errors err eval-buffer))
'()))))

(provide 'cider-debug)
;;; cider-debug.el ends here
14 changes: 9 additions & 5 deletions cider-interaction.el
Original file line number Diff line number Diff line change
Expand Up @@ -1558,12 +1558,16 @@ Print its value into the current buffer."

(defun cider-eval-defun-at-point (&optional prefix)
"Evaluate the current toplevel form, and print result in the minibuffer.
With a PREFIX argument, print the result in the current buffer."
With a PREFIX argument, debug the form instead by invoking
`cider-debug-defun-at-point'."
(interactive "P")
(cider-interactive-source-tracking-eval
(cider-defun-at-point)
(cider-defun-at-point-start-pos)
(when prefix (cider-eval-print-handler))))
(if prefix
(progn (require 'cider-debug)
(cider-debug-defun-at-point))
(cider-interactive-source-tracking-eval
(cider-defun-at-point)
(cider-defun-at-point-start-pos)
(when prefix (cider-eval-print-handler)))))

(defun cider-pprint-eval-defun-at-point ()
"Evaluate the top-level form at point and pprint its value in a popup buffer."
Expand Down
1 change: 1 addition & 0 deletions cider-mode.el
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ entirely."
["Show test report" cider-test-show-report]
"--"
["Inspect" cider-inspect]
["Debug top-level form" cider-debug-defun-at-point]
"--"
["Set ns" cider-repl-set-ns]
["Switch to REPL" cider-switch-to-repl-buffer]
Expand Down
1 change: 1 addition & 0 deletions cider.el
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
(require 'cider-repl)
(require 'cider-mode)
(require 'cider-util)
(require 'cider-debug)
(require 'tramp-sh)

(defvar cider-version "0.9.0-snapshot"
Expand Down
24 changes: 23 additions & 1 deletion nrepl-client.el
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,23 @@ for functionality like pretty-printing won't clobber the values of *1, *2, etc."
(defvar nrepl-err-handler 'cider-default-err-handler
"Evaluation error handler.")

(defvar-local nrepl--input-handler-queue (make-queue)
"A queue of handlers (functions) for incoming \"need-input\" messages.
Functions are passed the connection buffer as the only argument and should
send a string to stdin on that connection. See `cider-need-input' for an
example.

This variable is designed as a queue, so new elements should be added to
the bottom using `nrepl-push-input-handler', and they are removed from the
top when used. Whenever the variable is nil, `cider-need-input' is used.")

(defun nrepl-push-input-handler (function buffer)
"Add FUNCTION to input handlers queue on BUFFER's connection.
FUNCTION is added to the bottom of `nrepl--input-handler-queue'."
(with-current-buffer buffer
(with-current-buffer (nrepl-current-connection-buffer)
(queue-enqueue nrepl--input-handler-queue function))))

(defun nrepl-make-response-handler (buffer value-handler stdout-handler
stderr-handler done-handler
&optional eval-error-handler)
Expand Down Expand Up @@ -859,7 +876,12 @@ server responses."
(when (member "namespace-not-found" status)
(message "Namespace not found."))
(when (member "need-input" status)
(cider-need-input buffer))
(let ((handler
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused, cannot debug need-input events be handled directly in cider--handle-debug? This input handler queue doesn't look like a very robust thing to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There a two things at play here.

  1. The input handler queue is not absolutely necessary. We can just use a boolean variable and hardcode cider--debug-need-input in there. I just need a way to decide which function gets to handle a "need-input" message depending on whether we are debugging or not.

  2. The other factor at play here, which is also why the above is necessary, is that our breakpoints actually consist of two messages. The first is a message defined by us (well, by cider-nrepl) that does two things. It tells us where to find the breakpoint in code, and tells us that there's a debugger.core/break incoming. The second message is a "need-input" sent by the call to debugger.core/break.

    This would be more robust if it were done in one message, but that would involve us redefining a close immitation of debugger.core/break, because this macro doesn't provide us all we need at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To elaborate a bit more on this. I have just started looking at the code so this is a bit uneducated.

Wouldn't it be nicer if all the mechanics of the cider--handle-debug, cider--need-debug-input and the call handler in cider-debug-defun-at-point be comprised into one handler to say debug op?

Currently you seem to keep one global debugger-message which seems to imply that only one function can be debugged at a time. Correct me if I am wrong but the scenarios like "in the middle of the debugging of A you decided to debug B" will not work.

So wouldn't it be more reliable to have a debug middleware with an instrument op accepting instrument-id. Then have quit, step and continue ops that would also keep track of the original instrument-id. That is, embed each instrumentation and all the action associated with it in its own "bubble". On the emacs side there will be one handler for everything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other factor at play here, which is also why the above is necessary, is that our breakpoints actually consist of two messages. The first is a message defined by us (well, by cider-nrepl) that does two things. It tells us where to find the breakpoint in code, and tells us that there's a debugger.core/break incoming. The second message is a "need-input" sent by the call to debugger.core/break.

Sorry I haven't seen this when writing my previous post.

Now I see. So you are quite restrained by the debugger.core/break then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be nicer if all the mechanics of the cider--handle-debug, cider--need-debug-input and the call handler in cider-debug-defun-at-point be comprised into one handler to say debug op?

  • cider-debug-defun-at-point uses the "eval" op simply because that was easier for me when I was first writting this. It can be made to use a "debug" op which would take care of both instrumenting and eval'ing. As a nice side-effect, both the cider--init-debugger function and the debugger-message variable over at cider-nrepl would become unnecessary.
  • Regarding cider--need-debug-input, yes, it would be nice for its mechanics to be integrated into this same op. Again, however, that lies on us rewriting debugger.core/break to do some arbitrary stuff instead of just spawning a repl which triggers a regular "need-input" message. I'm not saying I can do that, though, because by looking at their code I'm not sure their break would work at all without spawning a repl.

Currently you seem to keep one global debugger-message which seems to imply that only one function can be debugged at a time. Correct me if I am wrong but the scenarios like "in the middle of the debugging of A you decided to debug B" will not work.

Depends on what you mean.

  • You can instrument two different functions (which presumably call each other). Then, when you call one of them the debugger will work seamlessly as they call each other and you'll be able to debug both.
  • While you're actually in the middle of debugging (because our input handler immediately calls read-char) you can't really do anything other than continue debugging. So no, you can't suddenly decide to instrument another function. This doesn't have anything to do with the fact we use a single message. It's a limitation on how we ask for input at the emacs level. Later on, there should many ways to get around this. I think the easiest here is to drop into a recursive-edit and activate a relevant minor-mode instead of calling read-char.

So wouldn't it be more reliable to have a debug middleware with an instrument op accepting instrument-id. Then have quit, step and continue ops that would also keep track of the original instrument-id.

  • The instrument-id part can (and probably will) be done as a consequence of item 1. above. But as it is right now, you can already have multiple instrumented functions simultaneously. The fact that it's all hapenning in a single message doesn't cause any problems.
  • Having quit, step and continue ops would require us do item 2. above and them some. debugger.core/break doesn't even have a real "quit execution" command. The (quit) function it provides is equivalent to edebug's "continue" (which is why I've bound it to c).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see. So you are quite restrained by the debugger.core/break then.

Yes. :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cider-debug-defun-at-point uses the "eval" op simply because that was easier for me when I was first writting this.

Actually cider was using this trick quite a lot till fairly recently. Now eval op shortcut is somewhat thrown upon. It's a very good start nevertheless.

Regarding cider--need-debug-input, yes, it would be nice for its mechanics to be integrated into this same op. Again, however, that lies on us rewriting debugger.core/break

Yeh. I think I have now a fairly complete picture of what's going on. It's not a big deal anyways. The only thing is that this elisp input-queue thingy is a bit of an over-engineering IMO. I think a simple check if repl is in a debug-mode should be indeed sufficient.

(with-current-buffer buffer
(with-current-buffer (nrepl-current-connection-buffer)
(or (queue-dequeue nrepl--input-handler-queue)
#'cider-need-input)))))
(funcall handler buffer)))
(when (member "done" status)
(puthash id (gethash id nrepl-pending-requests) nrepl-completed-requests)
(remhash id nrepl-pending-requests)
Expand Down