Inline error overlays not displaying

[yuhan0]

The changes introduced in #3616 to filter out stderr messages cause actual eval errors to be swallowed up when cider-show-error-buffer is set to nil.

Expected behavior

Error messages should always be displayed to the user.

Actual behavior

In certain situations, there is no indication is given of the eval command having successfully completed/ failed, which may be misinterpreted as ongoing evaluation or a REPL crash.

Steps to reproduce the problem

Elisp:

(setq cider-show-error-buffer nil)

JVM clojure

cider-eval the following and observe the lack of an error overlay (the only feedback being an error output in the possibly hidden REPL buffer)

(slurp "does-not-exist.txt")

Stderr output:

Execution error (FileNotFoundException) at java.io.FileInputStream/open0 (FileInputStream.java:-2).
does-not-exist.txt (No such file or directory)

Babashka

Jack into a bb repl and trigger any exception, observe that all error overlays are suppressed.

(ns repro)
(inc nil)

Stderr output:

java.lang.NullPointerException repro /Users/yuhan/.local/scripts/repro.bb:2:1

Additional context

The issue can be traced to this conditional

cider/cider-eval.el

Line 925 in a74dff9

(or (string-match-p cider-clojure-runtime-error-regexp err)

which only displays the error overlay if the stderr output matches an expected clojure-1.10 regexp, notice that the exception thrown from slurp doesn't match due to the 'line number' being negative:

Execution error (FileNotFoundException) at java.io.FileInputStream/open0 (FileInputStream.java:-2).
does-not-exist.txt (No such file or directory)

One straightforward fix would be to tweak the cider-clojure-1.10--location regexp to accept the - char

cider/cider-eval.el

Line 571 in a74dff9

(group-n 3 (one-or-more digit))

But this seems to be an especially brittle way of doing things - are we sure that the regex fully describes all valid error messages? I had no idea that interop calls could result in negative line numbers, maybe it's due to the absence of .java sources or specific to the JDK that I'm running, and I'm certain there are more edge cases that aren't covered by this regex.

Not to mention that it breaks compatibility with Clojure < 1.9 in an unnecessarily severe manner - A false negative here can be quite confusing since it manifests as zero feedback of any kind, which seems as though the REPL has crashed or is still busy processing.

I think that a more robust approach would be to default to showing all errors, and leaving it to a user-defined regex to weed out any false positives. This way the original issue with Timbre logs would be solved by something like

;; Filter anything that begins with an ISO date stamp
(setq cider-suppressed-error-regexp "^[[:digit:]]\\{4\\}-")

Environment & Version information

CIDER version information

;; CIDER 1.14.0-snapshot (package: 1.14.0-snapshot), nREPL 1.2.0-beta1
;; Clojure 1.12.0-alpha9, Java 21.0.2

Lein / Clojure CLI version

Clojure CLI version 1.11.3.1463

Emacs version

29.3

Operating system

NA

JDK distribution

openjdk version "21.0.2" 2024-01-16 LTS

[vemv]

Hi @yuhan0 ! Thanks for the report.

I'd start by the simplest fix, namely adding the -.

From memory, Clojure has a specific manner of conveying exceptions, no matter what the given exception is, so they all should match a regex.

Having these regexes comprehensive/up-to-date is also a good goal to have, since we use these for a variety of purposes.

I'm certain there are more edge cases that aren't covered by this regex

Examples welcome.

[yuhan0]

Ok, will push a fix in a bit, taking the liberty to refactor some of the rx forms as well

Examples welcome.

well.. I meant it as a prediction that more cases would crop up over time, as anyone who's played whack-a-mole with regexes is surely familiar with :/

[vemv]

Thanks!

well.. I meant it as a prediction that more cases would crop up over time, as anyone who's played whack-a-mole with regexes is surely familiar with

My understanding is that the regex approach has served well CIDER over the years. This particular regex is younger, so more prone to have issues.

We don't have a whole lot more options - last time I checked, the root of these is deep in the guts of clojure.repl and nrepl - fair bit of legacy stuff there.

[yuhan0]

Just realised that this applies across all runtimes like clojurescript / Babashka / nbb / etc., which may all have different error message formats - in those cases using the JVM Clojure regex as a interface filter is definitely an error.

(I've updated the issue description with a babashka repro)

[vemv]

Thanks! That doesn't change things IMO other than opening the need for expanding the regexes in the same way that the regexes already target a variety of Clojure versions.

As hinted, if the whole stack was re-done from scratch , error handling would probably look quite different (e.g. what Haystack does wouldn't be a cider-nrepl addition, but would live deeper in the stack), but that's not what we have at the moment.

[bbatsov]

Not to mention that it breaks compatibility with Clojure < 1.9 in an unnecessarily severe manner - A false negative here can be quite confusing since it manifests as zero feedback of any kind, which seems as though the REPL has crashed or is still busy processing.

I'll just add here that we officially dropped support for Clojure 1.8 and 1.9 for that matter. But I assume you're proposing that we tried to match the old regexp if the new one failed or something along those lines, right?

[bbatsov]

Just realised that this applies across all runtimes like clojurescript / Babashka / nbb / etc., which may all have different error message formats - in those cases using the JVM Clojure regex as a interface filter is definitely an error.

Fundamentally, the problem is that none of those existed when CIDER was created, which obviously influenced my thinking back then. As @vemv said - we'd probably do things very differently if we were starting out today and had to target many different runtimes from day 1. Life was a lot easier when I had to worry only about JVM Clojure. :-)

[yuhan0]

But I assume you're proposing that we tried to match the old regexp if the new one failed or something along those lines, right?

I'm suggesting that we invert the logic that was recently introduced in #3616 to decide whether a error message should be displayed to the user. I have no issues with how Cider parses error messages, only with the faulty assumption that

regex matches   => execution-related error     => display as overlay
does not match  => irrelevant logging output   => hide from user silently

The problem is that false negatives have (in my opinion) a far worse impact to the user experience, compared to the annoyance of having error logs appear as easily dismissable overlays.
It seems like a less brittle approach (vs. attempting to encompass all possible error formats) would be to default to 'always show error', and let the user explicitly configure a negative-regex for suppressing anything that they find annoying.

[vemv]

Well, I disagree in what is brittle. It seems to me extremely unlikely that users will write an Ekisp regex to improve the UX.

There are N, surely hundreds of bespoke errors out there in the wild.

Contrariwise there's a finite and predictable set of Clojure releases.

Babashka/etc would entail some work, but very finite in nature and testable.

(Babashka integration is incomplete in many ways, anyway)

[vemv]

I'll close the issue as it's mostly solved by your PR (thanks)!

Further discussion is welcome.

If you'd feel more comfortable with a different behavior, let us know if we can do anything to facilitate that (I wouldn't offer a defcustom, however we can decouple things into smaller defuns so that you can advice-add very precisely just one thin function)

[yuhan0]

Hmm, do you think it's relevant to take into account the severity of a false-negative vs a false-positive as I mentioned above? Maybe I'm biased from never having encountered the original issue reported in #3587, but the experience of having cider-eval fail to give any feedback was quite unpleasant and hard to pin down, vs. what I imagine is a relatively rare and easy-to-diagnose/google situation of "how to stop Cider error logs from showing up in the minibuffer"

[vemv]

CIDER generally is always developed by a number of users like you and me that can have strong expectations about the expected behavior and diagnose when those aren't met.

You say it was hard to pin down, however you did, and being very honest that's a win on my book because there's a very small number of people willing to go into the guts of our Elisp 😄 very plausibly that knowledge can pay off on some other occasion.

For the long term, we can have nice Clojure<->Elisp tests run in CI. That's already the case for an unrelated feature, namely:

https://github.com/clojure-emacs/cider/blob/6646778bccedcbb1f96bdf36b0a08bd208a4c25a/dev/generate_html_fragments.clj

Would be happy to keep building up on that pattern.

[yuhan0]

Contrariwise there's a finite and predictable set of Clojure releases.
Babashka/etc would entail some work, but very finite in nature and testable.

Yeah, I guess we could make ad-hoc patches to the regex to support <1.9 and the error formats of various other runtimes, but eg. when future versions make a small change to the wording or introduce some new class of errors, surely users would rather lose some affordances like auto-jump-to-error (graceful degradation) than have basic parts of the interface break completely and find out that they have to upgrade.

Ultimately it just seems like greater maintenance burden and a subpar broken-by-default experience for the cases that one hasn't put in the work to support yet (or decided to drop support for) :/

Just pointing this out, feel free to decide on the tradeoffs of course