[eval] Refactor exception handling logic and overlay display

[alexander-yakushev]

Alright, this is a big one. I'll try to give the problem statement first.

The context

We want to show error messages when the user evaluates something wrong. There are two types of errors – runtime and compile-time.

• For runtime errors, we want to pop up a *cider-error* buffer.
• For compile-time errors, we want to omit the buffer but instead display a shortened error as a special error overlay within the code buffer.
• However, user might have disabled cider-error buffer completely. In this case, we want to show even runtime errors as overlays.
• We also want to underline compilation errors in the source and jump to them.
• Not only errors – compilation warnings have to be underlined too.

The problem (dragons ahead)

Compilation errors with source locations come to us via stderr output, so we have to catch them there and parse the messages. Remember, warnings are also there – and warnings don't produce the actual error. So we definitely have to look into stderr.

Nrepl error handling for eval op is a mess. When error happens during evalution, nrepl sends err responses (which correspond to stderr printouts), and then sends an eval-error response – which contains barely any actual exception description. So on the client we have to follow that up with analyze-last-exception request. Then cider-nrepl will respond with multiple responses of analyzed exception, one per exception cause.

Current CIDER implementation messed something up, so even two analyze-last-exception are sent. The first one is sent by the stderr handler to figure out if the error which happened is a compile-time error to know whether to render the overlay. This is incorrect because stderr output could happen not only because of an error (could be a warning or arbitrary user stderr printing). The second analyze... request is correctly sent inside eval-error handler. But that one only deals with popping up *cider-error* buffer.

The solution

It took me a real real while to untangle this mess. Here are the exact steps I've taken:

1. Move overlay handling into cider--handle-stacktrace-response. Now this function deals both with *cider-error* buffer and overlays. It accepts the analyzed exception, so it knows whether the error is a compile-time one.
2. Accumulate all causes from the response of analyze-last-exception first and then call the supplied callback.
3. Modify cider-default-err-op-handler, cider-default-err-handler, nrepl-err-handler to accept a BUFFER argument. This is necessary so that they can show an overlay in the correct buffer.
4. Remove code that used to check if an error is a "Clojure error" so that random stderr output does not appear in the overlay. This is no longer needed as we now trigger the overlay stuff only as a response to eval-error event where we know for sure this is an error (also, the message now comes from the analyzed exception and not from stderr).
5. Relieve various eval commands from sending a second analyze... request.

Possible complications

1. The change of nrepl-err-handler and relative functions' signature can break setups where users for some reason supply custom functions for this. I don't know if CIDER ever encouraged that – this stuff is quite low-level.
2. In order to keep the new signature clean and sane, I let go of various bounds-checking code and position-calculating code for where the overlay should land. I believe that @vemv had good reasons for this code, but I want to try trimming this and see what/who breaks. This can be brought back if needed, but it will certainly complicate the interfaces and the flow.

---

• You've updated the changelog (if adding/changing user-visible functionality)

[bbatsov]

For compile-time errors, we want to omit the buffer but instead display a shortened error as a special error overlay within the code buffer.

We also have to consider how things will look when the overlays are disabled (as they are configurable, and I also don't think they work in terminal emulators). I'm guessing the errors are just dumped to the minibuffer or the REPL in this case, but I haven't used this in a while and my memory on the subject is fuzzy.

[bbatsov]

Nrepl error handling for eval op is a mess. When error happens during evalution, nrepl sends err responses (which correspond to stderr printouts), and then sends an eval-error response – which contains barely any actual exception description. So on the client we have to follow that up with analyze-last-exception request. Then cider-nrepl will respond with multiple responses of analyzed exception, one per exception cause.

True that. I think that some error handling was the first thing I did on nrepl.el (back then) - some of the code there certainly predates the days of cider-nrepl. Much of older message handling in nrepl-client.el is still a mess to this day, as I never had the time and the motivation to untangle it.

The change of nrepl-err-handler and relative functions' signature can break setups where users for some reason supply custom functions for this. I don't know if CIDER ever encouraged that – this stuff is quite low-level.

I wouldn't worry about this too much. Some of the configuration options in nrepl-client.el are there only because I hoped to fully extract it at some point, so packages like Monroe and other tools willing to use nREPL wouldn't have to reimplement this layer. Another failed objective.

Your suggestions seem reasonable to me at a glance, but I'll need a bit of time to go over the code and remember how things were supposed to behave.

[alexander-yakushev]

Sounds good! Definitely a lot to unpack here. I'll be using the changes locally meanwhile.

[bbatsov]

This might also be a good opportunity to review and update the documentation here https://docs.cider.mx/cider/usage/dealing_with_errors.html to reflect the actual behavior of things. I think it's mostly accurate, but I wouldn't be surprised if something was never documented or has changed.

[yuhan0]

This looks great, thank you so much for tackling this 💯

I've actually gone down this exact path more than once of attempting to untangle the mess of error handlers and display logic, and might even have contributed in the past (sorry!) by hacking local workarounds onto the overlay-displaying functions.

I pulled these changes locally and nothing seems to be broken so far, will provide feedback if I encounter any issues :)

[alexander-yakushev]

@yuhan0 Thanks a lot! Dogfooding this PR before merging is a tremendous help.

[yuhan0]

This may also be an opportunity to merge the two variables cider-show-error-buffer and the (poorly-named) cider-clojure-compilation-error-phases? An alist mapping error-phase => display-method (choice 'popup 'overlay) could make sense and allow flexibility for extension in the future.

For compile-time errors, we want to omit the buffer but instead display a shortened error as a special error overlay within the code buffer.

Some additional nuance may be needed here – I might be misremembering things, but sometime in the past the *cider-error* stacktrace buffer was always generated in the background even if it was not shown to the user, and one could manually summon it with cider-selector.

Something in the recent-ish batch of changes to compilation error handling seems to have changed this behavior, increasing the friction of eg. inspecting macroexpansion-time errors. (Where most of the time an error overlay will suffice, but occasionally one needs a full stacktrace to track down the source of a failing macro.)

[alexander-yakushev]

This may also be an opportunity to merge the two variables cider-show-error-buffer and the (poorly-named) cider-clojure-compilation-error-phases? An alist mapping error-phase => display-method (choice 'popup 'overlay) could make sense and allow flexibility for extension in the future.

I have several reservations about this.

1. I really don't think that this flexibility in being able to choose which phases pop *cider-error* buffer and which don't is in much demand. I personally just nilled this variable to make all errors go into the buffer, and I assume that most people do that too (if they don't like the default). Micromanaging this behavior feels cumbersome.
2. Even with current variable names being suboptimal, I'm not a fan of breaking public customizations. And if we were to keep the old ones for backward compat, this would just cause more maintenance headache.

I might be misremembering things, but sometime in the past the cider-error stacktrace buffer was always generated in the background even if it was not shown to the user, and one could manually summon it with cider-selector.

It appears that way in the docs, I have to check in the code. I'm fine with that – I don't think that *cider-error* contents are particularly valuable between re-evaluations to be kept intact. User should assume this buffer is transient.

Something in the recent-ish batch of changes to compilation error handling seems to have changed this behavior, increasing the friction of eg. inspecting macroexpansion-time errors. (Where most of the time an error overlay will suffice, but occasionally one needs a full stacktrace to track down the source of a failing macro.)

In that case, I'll look into fixing this. It makes sense to always send the error to *cider-error*, just not pop it up for compilation errors.

[yuhan0]

Yeah, on further reflection I agree that a fully specified map would be too fine-grained to be meaningful, in practice I was thinking it would make it easier to configure a default fallthrough behavior (rather than having to understand the strange negative-enumeration semantics of the current var)
I also had the impression that cider-show-error-buffer was a simple boolean, the additional semantics of except-in-repl and only-in-repl further complicates things..

Thanks for pointing out that the docs explicitly specify a background-generated error buffer, I should've reported it sooner but assumed that I had been relying on undocumented behavior when it broke.

[yuhan0]

I'm encountering a duplicated message on inline compilation errors, don't think it was there before

[yuhan0]

Here's the issue, we probably want to use newlines to join the messages of nested exceptions, and do something about the redundant messages in the case of compile-time errors

#{1 1} ;; boom

(->> *e
  (clojure.datafy/datafy)
  (:via)
  (map :message))
;; => (nil
;;     "java.lang.IllegalArgumentException: Duplicate key: 1"
;;     "Duplicate key: 1")

[yuhan0]

Additional note - this redundancy only seems to happen for the :read-source phase, the other phases don't have this issue. Also the duplicated message doesn't occur when simply relaying the contents of stderr, see
#3338 (comment) for how things used to look

[alexander-yakushev]

Thank you for the link to this comment, this is a very helpful to have a list of all cases we need to handle.

Looks like we have a problem with spec errors. This is what I get right now:

Where the ideal output would be this:

So, this "spec explain" output only goes to stderr, not to messages.

Perhaps, it is something that we could handle on the orchard.stacktrace side (previously, Haystack).

[alexander-yakushev]

On the other hand, maybe it is OK. Sometimes these spec explanations get quite long, e.g.:

Putting all of that into the overlay is questionable.

[yuhan0]

I think it's better to err on the side of verbosity and let the user decide what is too long - after all the reverse is impossible. There's already a defcustom cider-inline-error-message-function which can eg. limit the maximum number of lines displayed.

[alexander-yakushev]

This complicates things. I'll see what I can do.

[alexander-yakushev]

The read-error duplication is still present after my changes. Honestly, I plan to leave it as is. It feels weird and flimsy to try to deduplicate just that, and I think it's not that big of a deal.

[bbatsov]

Some additional nuance may be needed here – I might be misremembering things, but sometime in the past the cider-error stacktrace buffer was always generated in the background even if it was not shown to the user, and one could manually summon it with cider-selector.

Yeah, I believe it was always generated, but not always displayed (depending on the user settings). Don't recall what was the reasoning for this back in the day. Likely some people were just annoyed to always see the buffer pop, but still wanted to have access to it. One common complaint I got (that ultimately lead to the Monroe fork) was that CIDER was relying too much on special buffers and some people found this annoying and preferred for things like errors to be displayed only in the REPL. (even if caused by some in-line evaluation)

It appears that way in the docs, I have to check in the code. I'm fine with that – I don't think that cider-error contents are particularly valuable between re-evaluations to be kept intact. User should assume this buffer is transient.

Yeah, I don't think it's important to keep the old behavior. I was just sharing some historical context.

[yuhan0]

Just to provide a concrete example:
With the default value of cider-clojure-compilation-error-phases

;; throw an error which pops a buffer
(/ 1 0) 

;; and then one that doesn't (phase :macro-syntax-check)
(cond 1)

User brings up the error buffer manually (C-c M-s, x)

Expected (old behavior, documented)

*cider-error* buffer shows the stacktrace of the macroexpansion error

2. Unhandled java.lang.IllegalArgumentException
   cond requires an even number of forms

1. Caused by clojure.lang.Compiler$CompilerException
   Error compiling /Users/yuhan/scratch/repro.clj at (5:1)
   #:clojure.error{:phase :macro-syntax-check,
                   :line 83,
                   :column 1,
                   :source "/Users/yuhan/scratch/repro.clj",
                   :symbol cond}

Actual (recent regression)

*cider-error* buffer shows the unrelated error from before

1. Unhandled java.lang.ArithmeticException
   Divide by zero

              Numbers.java:  190  clojure.lang.Numbers/divide
                      REPL:    8  repro/eval142360

[alexander-yakushev]

@bbatsov

We also have to consider how things will look when the overlays are disabled (as they are configurable, and I also don't think they work in terminal emulators). I'm guessing the errors are just dumped to the minibuffer or the REPL in this case, but I haven't used this in a while and my memory on the subject is fuzzy.

Yes, I double-checked, "using overlays" really means "using either overlays or minibuffer or both, depending on cider-use-overlays customizable". I will update the code accordingly to make this more prominent.

[alexander-yakushev]

Update: I've addressed all comments, I think.

Triage message is now displayed inline:

And also in the error buffer:

Compilation errors are now always rendered in *cider-error* but the buffer doesn't pop up.

@yuhan0 Could you please check it? Note that this PR now also updates cider-nrepl version, you need the latest for everything to work correctly.

[alexander-yakushev]

@bbatsov I've looked through the doc on errors and it is fine. There is a section on inspecting printed stacktraces that I should remove, I'll do that in a separate PR together with deleting corresponding functions.

[bbatsov]

Overall the PR looks in a good shape to me.

[alexander-yakushev]

@bbatsov Another update – I fixed the issue that the overlay was now displayed when sending code to REPL buffer, e.g.:

Can you please take a look at the latest commit and tell if this approach is the correct one, or it can be done better?

[bbatsov]

I think I usually named those source-buffer in other parts of the code.

[bbatsov]

Those derived-mode-p checks will also work for the cider-scratch buffers. Checking the major mode by itself is not enough to determine if a buffer is visiting a file. (buffer-file-name is a useful thing to check in such cases)

[alexander-yakushev]

But it is helpful in a cider-scratch buffer though 🤔.

[alexander-yakushev]

It's probably more that I want to restrict the overlay from appearing in REPL buffer and in ancilary buffers.

[bbatsov]

I just mentioned this, because of your comment above about visiting a source file.

[bbatsov]

Well, the alternative is to check if a buffer is not derived from comint-mode or special-mode. I'm guessing that comint-mode might be deriving from special-mode as well.

[alexander-yakushev]

Neither comint-mode nor special-mode returns t for the REPL buffer. Guess I'll leave the current check as is, and say in the comment what we want to achieve.

[bbatsov]

My bad! I forgot cider-repl-mode doesn't inherit from comint-mode. This is for historical reasons (as SLIME's REPL was also standalone and early on we modeled almost everything after SLIME). At some point I considered changing it, but I never had much incentive to do this. But overall it doesn't matter much which approach do you chose.

[yuhan0]

In order to keep the new signature clean and sane, I let go of various bounds-checking code and position-calculating code for where the overlay should land.

I have cider-result-overlay-position set to at-point, previously the error overlays were appearing in the same location as the result overlays, but now it behaves differently

I don't think this is necessarily a bad thing – the error isn't 'associated' with the entire expression, so there's less rationale for having it appear in the same location as the evaluated result.

[alexander-yakushev]

Thanks for checking! Yeah, with bounds it looked slightly cleaner, but I think the simplicity of the current solution beats it. Maybe somebody in the future comes to this again and tries to unify it once more... and it will be easier if we simplify things now.

[alexander-yakushev]

Let's go!