Debugger tweaks

[yuhan0]

Continuing the discussion from #769, and marking this as a draft PR as I'm unsure about some of these changes, in particular the hack with the STATE__ anaphor to get a :caught-msg key into the response.

Re. naming, should I change the tags to #break! and #dbg! as previously suggested, or try and implement as metadata keys? Note that currently it doesn't seem possible to combine :break/when functionality with the new on-exception debugging.

There are some changes here related to overriding various heuristics on what is "interesting", commit c5f3084 in particular makes it so you can actually tag a vector / map with #break, something I've always been annoyed by:

(into {}
  (for [x (range 5)]
    #break
    [(char (+ 97 x)) (inc x)]))

Also I haven't figured out how to write tests for these changes yet- the current deftests for debugger functionality are quite high-level and don't cover what I would consider basic data-transformation tests like (is (= (tag <expr>) <tagged-expr>)).
These are tricky to introspect since they involve a lot of invisible metadata which are added and then stripped out within the main instrument-tagged-code procedure, so it might involve some refactoring in order to test the intermediate states. Hoping to demystify the internals a bit for future contributors in the process, while I have some grasp on their workings.

---

• The commits are consistent with our contribution guidelines
• You've added tests to cover your change(s)
• All tests are passing
• The new code is not generating reflection warnings
• You've updated the README (if adding/changing middleware)

[yuhan0]

Ping on how to proceed re. the above questions, I'm free to refactor and clean things up for review over the weekend :)

[bbatsov]

Re. naming, should I change the tags to #break! and #dbg! as previously suggested, or try and implement as metadata keys?

I'd go with #break! and #dbg!.

There are some changes here related to overriding various heuristics on what is "interesting", commit c5f3084 in particular makes it so you can actually tag a vector / map with #break, something I've always been annoyed by.

I like your changes.

Also I haven't figured out how to write tests for these changes yet- the current deftests for debugger functionality are quite high-level and don't cover what I would consider basic data-transformation tests like (is (= (tag ) )).
These are tricky to introspect since they involve a lot of invisible metadata which are added and then stripped out within the main instrument-tagged-code procedure, so it might involve some refactoring in order to test the intermediate states. Hoping to demystify the internals a bit for future contributors in the process, while I have some grasp on their workings.

Admittedly the debugger tests are not great. I'd be down with whatever degree of effort you're willing to put into this. I'd also appreciate if you document any insights you get about the workings of the debugger on the current modest documentation page. I've long wanted to make it easier for people to contribute to the debugger and I feel most people are quite intimidated by it. The lack of docs is not helping. :-)

[bbatsov]

@yuhan0 Anything else you need from me?

[yuhan0]

Sorry for such a late response, some other matters got in the way of OSS related work for the past few weeks. And I may have went too far down the rabbit hole of debuggers the other time, ended up poking into Flowstorm and Emacs' own built-in Edebug. 👀

Re. docs, I found the current explanations in the form of code comments to be actually pretty good, at least for getting a handle on the overall code structure. The main difficulty I had with understanding was due to the existence of various mutable global state atoms and confusing naming of terms (the words "break(point)" / "message" / "state" being overloaded in different contexts) .

Looking at the git logs some of it appeared to be accumulated complexity with features and patches added on over time - if we did want to make things easier to contribute, it would probably require some careful refactoring, making sure not to break any of its current behaviour (lack of low-level tests doesn't help).

For now I'll just restrict the scope of this PR and go forward with the renaming of the new exception readers and a few other minor changes :)

[yuhan0]

In particular I was held up trying to restructure the breakpoint macros and simplify things such that:
#break! (expr) could just be syntax sugar for
#break ^{:break/on-ex Throwable} (expr), logically extending the existing metadata syntax for ^:break/when.

Which would then allow for more extensions like ^:break/skip to tag some uninteresting forms within a #dbg, or ^:break/skip-till-here to ignore all breakpoints before it - something I would personally find very useful in a big instrumented function, having always to mash the "n" key to get past various forms making sure not to overshoot the one I'm currently interested in .

If this sounds desirable I can open another issue for further discussion - there were some subtle considerations like how multiple keys would compose or act when nested etc.

[yuhan0]

Added tests and a changelog entry, also fixed some local test failures unrelated to the PR.

[bbatsov]

If this sounds desirable I can open another issue for further discussion - there were some subtle considerations like how multiple keys would compose or act when nested etc.

Yeah, let's open a separate discussion for this.

Great work with this PR overall! 🙇 I'll cut a new CIDER release right after we wrap up the matching Elisp PR as well.

[yuhan0]

Aagh, somehow I didn't pay attention during a merge conflict and this exn->break! renaming got lost in the process... @bbatsov do you mind fixing it up on your side?

[bbatsov]

I'll fix it.

[bbatsov]

Done.