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

[vemv]

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.

That's why our CIs are also built against clojure-master, whenever possible.

As mentioned 1.9 support was dropped.

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

For the record, I do appreciate having different points of view, and leaving a trail of those that we can revisit later.

[bbatsov]

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.

@yuhan0 Feel free to open a separate ticket/discussion for this. Perhaps we can add some configuration option that implements the behavior you'd prefer. In general I agree with @vemv that it's unlikely we'd often run into problems like the one that caused the breakage you've reported and fixed recently.

[vemv]

Personally I wouldn't introduce a defcustom since that means explaining more stuff - I don't like the idea of overburdening users with intricacies.

Ideally things would just work - here, neither approach is perfect. Letting users choose which flavor of imperfection they prefer seems rather low-level.

I'd say, let's make it for @yuhan0 (and anyone watching this issue) easy to customize this 'unofficialy' via a advice-add. If some change is needed, we can add a big scary ;; comment to prevent deletions.

And assess the situation again 1y from now - quite often time is the best judge for these problems.

[yuhan0]

I'm not sure if I managed to communicate the root problem - My most recent PR only addressed a specific bug in the regex, while the babashka example in the issue description is still reproducible.

The current behaviour when cider-show-error-buffer = nil is still broken on all runtimes besides JVM Clojure 1.10+.

I've only tested it on Babashka, but it seems that we also have NBB, Scittle and now Basilisp, I think Clojurescript is a separate thing altogether (?) as I've never had functioning cider-error popups there.

Maybe a gif illustrates it better:

err-bb.mov

This was actually working fine before #3616, which introduced the regression by using cider-clojure-runtime-error-regexp and cider-clojure-compilation-error-regexp to judge if the error should be displayed to the user.

err-bb2.mov

At the very least it should dispatch on (cider-runtime) and only apply the condition when running on JVM clojure? Or call a private function which dispatches on runtime and returns the appropriate regex.

Either way it's clearly a bug, I guess I should have explained it better instead of jumping to the suggestion of inverting the logic + introducing defcustom.

[bbatsov]

@yuhan0 Thanks for the great summary of the problem! Now it's clear that we need at least a separate issue tracking this:

The current behaviour when cider-show-error-buffer = nil is still broken on all runtimes besides JVM Clojure 1.10+.

At the very least it should dispatch on (cider-runtime) and only apply the condition when running on JVM clojure? Or call a private function which dispatches on runtime and returns the appropriate regex.

Something this this seems like a reasonable approach to me.

[vemv]

Either way it's clearly a bug

It is, however let me remark that the Babashka integration is incomplete in many ways.

Its nrepl server implements the very idea of middleware in an incompatible way and only a small subset of cider-nrepl ops are actually implemented.

tbh if I could go back in time, I would make sure that we only provide support for things that are really compatible - it's not like users would be short of options (e.g. use inf-clojure).

---

Anyway, issue/PR welcome for At the very least it should dispatch on (cider-runtime) and only apply the condition when running on JVM clojure?, yes

[bbatsov]

Its nrepl server implements the very idea of middleware in an incompatible way and only a small subset of cider-nrepl ops are actually implemented.

Middleware is an implementation detail, not a part of the nREPL protocol. I have personally advised the author of Babashka against copying nREPL's own implementation as I've seen this being as a big part of the reason why more tools didn't adopt the nREPL protocol (they disliked the complexity that went with the middleware approach). Same with nREPL-CLR which lay dead/dormant for years, because it was hard for the maintainer to port 1:1 what we were doing in the Clojure nREPL. I keep repeating that for me it's very important that CIDER is highly usable without any cider-nrepl middleware/features, and that everything fundamental should be promoted to the protocol level (that's why I added lookup/info and completions/complete there a few years ago).

I also believe that we should not be dogmatic in our approach to providing support for Clojure-like languages - if someone wants to provide basic support for Clojure and they find the outcomes reasonable/useful, I'm not going to force them to do the (significant amount of) work necessary for full compatibility. Caveats should be documented (and ideally addressed over the course of time), but that's all from my perspective. I think that dealing with the error formats of the various runtimes shouldn't be particularly complex.

So, I'd prefer to narrow this conversation to the specific issue at hand and ignore the other differences.

[yuhan0]

Ahh, just stumbled on another case uncaught by the cider-clojure-* regexes:

(file-seq "does-not-exist") ;; <= cider-eval

Which produces:

Error printing return value (ClassCastException) at clojure.core/file-seq$fn (core.clj:4997).
class java.lang.String cannot be cast to class java.io.File (java.lang.String and java.io.File are in module java.base of loader 'bootstrap')

I'll have a closer look at the regex later and submit a PR along with the dispatch on (cider-runtime) fix.