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 #170

Merged
merged 2 commits into from Mar 28, 2015
Merged

Debugging in Cider #170

merged 2 commits into from Mar 28, 2015

Conversation

Malabarba
Copy link
Member

This goes with clojure-emacs/cider#1019.

cider-debug

What is this?

An edebug-like debugger for cider. You can:

  1. step through the code,
  2. quit execution,
  3. and inject values.

How to use it

  1. Do the usualM-x cider-jackin, but make sure you're using clojure >= 1.6. For instance,

    (setq cider-lein-parameters "with-profile +1.6 repl :headless")
    
  2. Call M-x cider-debug-defun-at-point with point inside a top-level sexp.

For a regular sexp this will immediately start debugging. If the top-level sexp was a defn, it will start debugging when you run the function.

Once you're in the debugger you can hit:

  • n: next step
  • c: continue execution
  • i: inject value

TODO

  • keybind and menu-bar entry.

Possible Eventual Improvements

  • e key to evaluate an expression in the current scope.
  • q key to completely quit execution.
  • o key to “out” a depth level.
  • A couple of things regarding the debugger-message variable (see source comments).

@Malabarba
Copy link
Member Author

The tests are failing with [WARNING] No nREPL middleware descriptor in metadata of null, see clojure.tools.middleware/set-descriptor!. I'm guessing this means I need to add some specific configuration for the tests?

@bbatsov
Copy link
Member

bbatsov commented Mar 15, 2015

Make sure you're using clojure > 1.6. For instance,

(setq cider-lein-parameters "with-profile +1.6 repl :headless")

This is because of clj-debugger, right? Probably it can be updated to support 1.5 as well.

@Malabarba
Copy link
Member Author

This is because of clj-debugger, right? Probably it can be updated to support 1.5 as well.

Yes, I'll try to ping the developer now.

@bbatsov
Copy link
Member

bbatsov commented Mar 15, 2015

It's very good clj-debugger has no dependencies other than Clojure - our users are unlikely to run into dep conflicts when we enable it.

@fgiasson
Copy link

@Bruce-Connor To answer your question: "I'd like to know if the implementation is welcome before delving more into it", I think this is more than welcome :)

@expez
Copy link
Member

expez commented Mar 17, 2015

mfw

mfw

@Malabarba
Copy link
Member Author

Ok. I just pushed an update to both repos which:

  • Handles macros and special forms (worked on basically anything I tried).
  • Moves the code-walking to clojure.
  • Removes the need for an init command.
  • Adds a bunch of tests.

I'm going to play around with this for a few hours to see if I spot any problems. But if it all works as I expect I think this is pretty close to complete. There are many additional features that could be implemented (some of which are listed on first comment above), but what we have now is enough to stand on its own.

forms))))

(def specifier-map
"Map between specifiers and [matcher handler] functions."
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this isn't a multimethod dispatching on the keys in this map?

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason in particular. I'll look into that now, thanks.

@bbatsov
Copy link
Member

bbatsov commented Mar 18, 2015

I'm going to play around with this for a few hours to see if I spot any problems. But if it all works as I expect I think this is pretty close to complete. There are many additional features that could be implemented (some of which are listed on first comment above), but what we have now is enough to stand on its own.

I'm fine with releasing this with it's current features set, but have to take care of the outstanding debugger issues first:

  • Clojure 1.5 compatibility
  • drop clj-time

Both changes, should be fairly easy.

@bbatsov
Copy link
Member

bbatsov commented Mar 18, 2015

Btw, you should mention the new middleware in the README and we should figure out what's wrong with the travis build.

@Malabarba
Copy link
Member Author

drop clj-time

I had a brief look through debugger's source yesterday, and clj-time is used only 2 or 3 times. So it shouldn't be hard for someone to send a pull request.

Btw, you should mention the new middleware in the README

👍

we should figure out what's wrong with the travis build.

👍 :-(

@Malabarba
Copy link
Member Author

Added a menu-bar item and a snippet to the readme.

@vspinu
Copy link
Contributor

vspinu commented Mar 19, 2015

I cannot try this one. On jack-in I am getting:

Error loading cider.nrepl.middleware.debug: java.lang.RuntimeException: Unable to resolve symbol: some? in this context, compiling:(debugger/formatter.clj:49:1)
[WARNING] No nREPL middleware descriptor in metadata of null, see clojure.tools.middleware/set-descriptor!
Exception in thread "main" java.lang.NullPointerException, compiling:(nrepl.clj:38:3)
    at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3463)
    at clojure.lang.Compiler$DefExpr.eval(Compiler.java:408)
    at clojure.lang.Compiler.eval(Compiler.java:6624)
    at clojure.lang.Compiler.load(Compiler.java:7064)
    at clojure.lang.RT.loadResourceScript(RT.java:370)
    at clojure.lang.RT.loadResourceScript(RT.java:361)
    at clojure.lang.RT.load(RT.java:440)
    at clojure.lang.RT.load(RT.java:411)
    at clojure.core$load$fn__5018.invoke(core.clj:5530)
    at clojure.core$load.doInvoke(core.clj:5529)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5336)
    at clojure.core$load_lib$fn__4967.invoke(core.clj:5375)
    at clojure.core$load_lib.doInvoke(core.clj:5374)
    at clojure.lang.RestFn.applyTo(RestFn.java:142)
    at clojure.core$apply.invoke(core.clj:619)
    at clojure.core$load_libs.doInvoke(core.clj:5413)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:619)
    at clojure.core$require.doInvoke(core.clj:5496)
    at clojure.lang.RestFn.invoke(RestFn.java:421)
    at cider_nrepl.plugin$eval7241$loading__4910__auto____7242.invoke(plugin.clj:1)
    at cider_nrepl.plugin$eval7241.invoke(plugin.clj:1)
    at clojure.lang.Compiler.eval(Compiler.java:6619)
    at clojure.lang.Compiler.eval(Compiler.java:6608)
    at clojure.lang.Compiler.load(Compiler.java:7064)
    at clojure.lang.RT.loadResourceScript(RT.java:370)
    at clojure.lang.RT.loadResourceScript(RT.java:361)
    at clojure.lang.RT.load(RT.java:440)
    at clojure.lang.RT.load(RT.java:411)
    at clojure.core$load$fn__5018.invoke(core.clj:5530)
    at clojure.core$load.doInvoke(core.clj:5529)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5336)
    at clojure.core$load_lib$fn__4967.invoke(core.clj:5375)
    at clojure.core$load_lib.doInvoke(core.clj:5374)
    at clojure.lang.RestFn.applyTo(RestFn.java:142)
    at clojure.core$apply.invoke(core.clj:619)
    at clojure.core$load_libs.doInvoke(core.clj:5413)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:619)
    at clojure.core$require.doInvoke(core.clj:5496)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at user$eval7237.invoke(form-init6256550459440863591.clj:1)
    at clojure.lang.Compiler.eval(Compiler.java:6619)
    at clojure.lang.Compiler.eval(Compiler.java:6608)
    at clojure.lang.Compiler.eval(Compiler.java:6608)
    at clojure.lang.Compiler.load(Compiler.java:7064)
    at clojure.lang.Compiler.loadFile(Compiler.java:7020)
    at clojure.main$load_script.invoke(main.clj:294)
    at clojure.main$init_opt.invoke(main.clj:299)
    at clojure.main$initialize.invoke(main.clj:327)
    at clojure.main$null_opt.invoke(main.clj:362)
    at clojure.main$main.doInvoke(main.clj:440)
    at clojure.lang.RestFn.invoke(RestFn.java:421)
    at clojure.lang.Var.invoke(Var.java:419)
    at clojure.lang.AFn.applyToHelper(AFn.java:163)
    at clojure.lang.Var.applyTo(Var.java:532)
    at clojure.main.main(main.java:37)
Caused by: java.lang.NullPointerException
    at clojure.core$comp$fn__4158.doInvoke(core.clj:2347)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.tools.nrepl.server$default_handler.doInvoke(server.clj:90)
    at clojure.lang.RestFn.applyTo(RestFn.java:137)
    at clojure.core$apply.invoke(core.clj:617)
    at clojure.lang.AFn.applyToHelper(AFn.java:163)
    at clojure.lang.RestFn.applyTo(RestFn.java:132)
    at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3458)
    ... 58 more


(defn wrap-debug [h]
(fn [{:keys [op force] :as msg}]
(if (= "init-debugger" (:op msg))
Copy link
Contributor

Choose a reason for hiding this comment

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

There is op local already :)

Copy link
Member Author

Choose a reason for hiding this comment

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

oops :)

@Malabarba
Copy link
Member Author

@vspinu Did you do the (setq cider-lein-parameters "with-profile +1.6 repl :headless")?

(if (and debugger-message (not force))
(transport/send (:transport msg)
(response-for msg :status :done))
(do (when debugger-message
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I think I understand now. You are using debugger-message simply as a storage for the most recent debug-init request (given that cider-debug-defun-at-point is calling cider--debug-init-connection).

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it stores the first request it receives, and immediately closes all subsequent ones. But it makes no difference in the end. We could store the most recent instead, and close the older one, and the final effect would be the same.

Regardless. I think I'll rewrite cider-debug-defun-at-point the way we discussed above, so that this whole thing will no longer be necessary.

@vspinu
Copy link
Contributor

vspinu commented Mar 19, 2015

@vspinu Did you do the (setq cider-lein-parameters "with-profile +1.6 repl :headless")?

I missed that part. thanks.

@bbatsov
Copy link
Member

bbatsov commented Mar 25, 2015

@Bruce-Connor So, we now need a new release of clj-debugger, right?

@Malabarba
Copy link
Member Author

@bbatsov Yes. I'm not familiar with how releasing works in the clojure/lein world (though, I noticed cider-nrepl snapshots builds seem to follow the repo)

@bbatsov
Copy link
Member

bbatsov commented Mar 25, 2015

I'm not familiar with how releasing works in the clojure/lein world (though, I noticed cider-nrepl snapshots builds seem to follow the repo)

They follow it, because I constantly update clojars. :-) It's not an automated process.

At any rate - 0.1.5 is out. You can use it to wrap this.

@Malabarba
Copy link
Member Author

I've bumped the dependency, but I'm at work so I haven't tried building the jar yet.
In any case, let's see how the tests go...

@bbatsov
Copy link
Member

bbatsov commented Mar 27, 2015

If linting is the only hold up here, let's just disable it and squash and merge the PR. It'd be nice to start collecting feedback from CIDER's users.

@cichli
Copy link
Member

cichli commented Mar 27, 2015

I get the same error when linting clj-debugger itself FWIW. It's likely either an issue with that or Eastwood - I'll try and get to the bottom of it and log an issue to one of the two.

@Malabarba
Copy link
Member Author

Linting is the only issue, I think.
But give me a few hours. I haven't been able to actually try out the last set of changes I made.
I'll test tonight and then give the OK.

@Malabarba
Copy link
Member Author

I'm playing around with it here, and it seems we can't merge just quite yet. There are two problems:

  1. To apease the linting tests, I changed the def inside wrap-debug to a set!. That, however, doesn't work (the *debugger-message* variable remains forever nil). Unless someone knows the right thing to do here I'll just change it back to def.
  2. Using the latest version of clj-debugger with this branch gives me a very weird issue. Whenever I try to debug something I get the following exception saying that the assertion in format-line-with-line-numbers failed (which is a function from debugger):
1. Unhandled java.lang.AssertionError
   Assert failed: (nil? break-line)

                 formatter.clj:   49  debugger.formatter/format-line-with-line-numbers

That assertion is indeed one of the things I changed to drop the 1.6 dependency. The strange thing is that the assertion is not the reported (nil? break-line). Instead, it is (not (nil? break-line)).
To make matters even stranger, as the nrepl is still running, if I open debug.clj (yes, the cider-nrepl file), and evaluate the source of the breakpoint function then everything works again! Even though this breakpoint function is exactly the same as the one I installed (with lein instal) moments ago!

@Malabarba
Copy link
Member Author

Actually, now that I'm looking into clojure assertions, I think that assertion is ill-defined, there should be square brackets around it.

The reason why reevaluating one of our function suddenly got rid of that is completely beyond me...

@Malabarba
Copy link
Member Author

Ok. Now you have my 👍 to merge.
Though it probably won't work until razum2um/clj-debugger#9 is merged. :-)

@bbatsov
Copy link
Member

bbatsov commented Mar 28, 2015

New version of clj-debugger is out. Fingers crossed. :-)

@Malabarba
Copy link
Member Author

🎉

@bbatsov
Copy link
Member

bbatsov commented Mar 28, 2015

Squash the commits in both PRs.

On Saturday, March 28, 2015, Artur Malabarba notifications@github.com
wrote:

[image: 🎉]


Reply to this email directly or view it on GitHub
#170 (comment)
.

Best Regards,
Bozhidar Batsov

http://www.batsov.com

@Malabarba
Copy link
Member Author

Doing that now. And by the way, can I bind cider-debug-defun-at-point to C-u C-M-x.
cider-eval-defun-at-point already uses the prefix argument to print the evaluation results,
but this key would make it consistent with how edebug is used and you can already print the results with C-u C-x e anyway.

@Malabarba
Copy link
Member Author

(Despite the lack of question marks, that was a question :-)

@Malabarba
Copy link
Member Author

Squashed

@Malabarba
Copy link
Member Author

And apparently I broke the tests... Looking into it...

@bbatsov
Copy link
Member

bbatsov commented Mar 28, 2015

:-)

@bbatsov
Copy link
Member

bbatsov commented Mar 28, 2015

Doing that now. And by the way, can I bind cider-debug-defun-at-point to C-u C-M-x.
cider-eval-defun-at-point already uses the prefix argument to print the evaluation results,
but this key would make it consistent with how edebug is used and you can already print the results with C-u C-x e anyway.

Ops, didn't see this - yeah, let's make it consistent with edebug.

The instrumenter will walk through the code and wrap in breakpoints
anything that looks non-trivial. Entry-point is the `instrument`
function. See doc and code comments on how to use it.
@Malabarba
Copy link
Member Author

Ok. The eastwook test will still fail because of that def thingy. But everything else should pass.

@bbatsov
Copy link
Member

bbatsov commented Mar 28, 2015

Ok. The eastwook test will still fail because of that def thingy. But everything else should pass.

Won't using alter-var-root work there? Or you can make its value an atom and update it? This code definitely looks strange.

@Malabarba
Copy link
Member Author

👍 Now using an atom.

(transport/send (:transport stored-message)
(response-for stored-message :status :done)))
;; TODO: Redefine this in the session binding map, see inspect.clj.
(swap! debugger-message (constantly msg)))))
Copy link
Member

Choose a reason for hiding this comment

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

In such cases you should use reset!.

@bbatsov
Copy link
Member

bbatsov commented Mar 28, 2015

And the linting failed again... Merging this PR has become an epic undertaking! :-)

This only works in combination with cider-debug.el. To use it, invoke
   M-x cider-debug-defun-at-point
@Malabarba
Copy link
Member Author

In such cases you should use reset!.

Done

And the linting failed again... Merging this PR has become an epic undertaking! :-)

Yeah. That's the failure we already had and couldn't figure out the reason. @cichli mentioned it might be from debugger or from a bug in eastwood.

bbatsov added a commit that referenced this pull request Mar 28, 2015
@bbatsov bbatsov merged commit edddc70 into clojure-emacs:master Mar 28, 2015
@bbatsov
Copy link
Member

bbatsov commented Mar 28, 2015

OK, I'll just disable the linting for the time being.

@cichli
Copy link
Member

cichli commented Mar 30, 2015

I've opened razum2um/clj-debugger/pull/10 for the lint issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants