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

Conditional breakpoints #303

Merged

Conversation

@grammati
Copy link
Contributor

grammati commented Feb 26, 2016

Not entirely ready, but I would like to get some more eyes on this and see whether others agree with the approach.

The feature is conditional breakpoints - that is, breakpoints that will only enter the debugger when an expression is truthy.

Conditions are attached to forms as :break/when metadata. Example:

(for [i (range 3)]
  #dbg ^{:break/when (odd? i)
  (inc i))

Evaluating this, you will stop in the debugger only once, when i is 1.

However, the primary use case (at least for me) is likely to be instrumenting a function with a conditional breakpoint. I have implemented this in cider as C-u C-u C-M-x, which prompts for an expression. This can be an expression of the function arguments. The form is then sent to cider-nrepl with #dbg ^{:break/when (the expression)} prefixed.

Making this work required a slightly hairy special-case that moves :break/when metadata down from the defn form and onto the forms inside the body of the macroexpanded function.

To get some confidence that I have the slightest idea what I'm doing, I wrote a bunch of new tests, in a new test namespace. These interact with cider-nrepl only by sending and receiving nrepl messages, just as a client (i.e. emacs) would.

Thanks everyone, please let me know what you think.

@Malabarba
Copy link
Member

Malabarba commented Feb 26, 2016

Wow. First of all, thanks a ton! :-)
This is a feature that's been requested several times already, so it's very welcome.
Also, thanks so much for writing so many tests! :-)

I'll make some comments directly in the code, but overall I like how you've implemented it.

@@ -139,10 +143,10 @@
;;; `wrap-debug` receives an initial message from the client, stores
;;; it in `debugger-message`, and `breakpoint` answers it when asking
;;; for input.
(def debugger-message

This comment has been minimized.

@Malabarba

Malabarba Feb 26, 2016 Member

I'm fine with this change, but I'd rather leave it to a separate commit (unless it has some relation to this feature).

project.clj Outdated
:eastwood {:config-files ["eastwood.clj"]}}})
:eastwood {:config-files ["eastwood.clj"]}}

:debug {:jvm-opts ["-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=5009"]}})

This comment has been minimized.

@Malabarba

Malabarba Feb 26, 2016 Member

Could you explain what this line is for?

This comment has been minimized.

@grammati

grammati Feb 26, 2016 Author Contributor

Ah, yes, unrelated to this PR. I put that in most of my projects so that I can attach Intellij to the running process and debug with Cursive. I can remove itl

This comment has been minimized.

@Malabarba

Malabarba Feb 26, 2016 Member

Yes please. :-)

This comment has been minimized.

@bbatsov

bbatsov Feb 26, 2016 Member

Debug cider in cider. :-)

@grammati
Copy link
Contributor Author

grammati commented Feb 26, 2016

I pushed a PR for a rudimentary version of the front-end portion, here: clojure-emacs/cider#1591

;; 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.
(binding [*skip-breaks* (if skip?#

This comment has been minimized.

@Malabarba

Malabarba Feb 26, 2016 Member

Do we need :deeper? Isn't that the same as skipping everything inside this binding?

This comment has been minimized.

@grammati

grammati Feb 26, 2016 Author Contributor

My thinking at the time was that if the "skipped" form calls another function which is instrumented, we do want to stop in the debugger, and that using :all would prevent that. I can add some test cases to verify.

This comment has been minimized.

@Malabarba

Malabarba Feb 26, 2016 Member

Good point!
And yes, a test case would be nice.

(assoc *extras* :debug-value)
(read-debug-command val#)))))

(defmacro maybe-breakpoint

This comment has been minimized.

@Malabarba

Malabarba Feb 26, 2016 Member

This macro always introduces a breakpoint. It might be skipped during execution, but it's always there.

I prefer to call this macro breakpoint, and change the name of the macro above to something like breakpoint-implementation.

@@ -246,6 +248,36 @@
;; Don't use `postwalk` because it destroys previous metadata.
(walk-indexed #(tag-form %2 breakfunction) form))

(defn transfer-break-conditions

This comment has been minimized.

@Malabarba

Malabarba Feb 26, 2016 Member

This part is a little tricky. Can we leave it out for now?

Firstly, it shouldn't be here. Doing something like this is the job of debug.clj, not instrument.clj.

Second, I realise you need this so that your Emacs command will work, but I still want to discuss what's the best interface for this feature.

@grammati grammati force-pushed the grammati:conditional-break-squashed branch from e9e00a3 to 4a7ab0e Feb 28, 2016
@grammati
Copy link
Contributor Author

grammati commented Feb 28, 2016

@Malabarba, I pushed a commit that addresses all your comments, I think.

I pared back the functionality to be just the basic conditional break. I removed the special-casing of defns that moved the condition down into the body of the fn. I can re-introduce that in another PR, where we can discuss the best interface.

I also added a few more tests, including one (unrelated to conditional breakpoints) that I think should pass but does not. Can you have a look at call-instrumented-fn-when-stepping-out? Perhaps my expectations of how the debugger should behave are just wrong.

Thanks!

@Malabarba
Copy link
Member

Malabarba commented Feb 29, 2016

Ok, thanks! I'll have a look at the tests when I get a chance. 👍

Also some new, high-level tests for the debugger.
@grammati grammati force-pushed the grammati:conditional-break-squashed branch from 4a7ab0e to df91bb7 Mar 3, 2016
@grammati
Copy link
Contributor Author

grammati commented Mar 3, 2016

I rebased this onto master and pushed again

@Malabarba
Copy link
Member

Malabarba commented Mar 3, 2016

Ok. Looks like your tests are good. The only 2 failures are the cljs tests which are also failing on master. I've also tested your code locally and it seems to work well.
I'm going to give this a final read and merge. 👍

Malabarba added a commit that referenced this pull request Mar 3, 2016
Conditional breakpoints
@Malabarba Malabarba merged commit 97a0113 into clojure-emacs:master Mar 3, 2016
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@Malabarba
Copy link
Member

Malabarba commented Mar 3, 2016

Ok. This should now be on clojar.
Thanks again for the feature and specially for the tests. :-)

@grammati grammati deleted the grammati:conditional-break-squashed branch Apr 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.