CIDER macroexpand: handle errors more gracefully #3557
Conversation
| @@ -643,7 +643,9 @@ others." | |||
| "Emit toggle buttons for each of the ERROR-TYPES leading this stacktrace BUFFER." | |||
| (with-current-buffer buffer | |||
| (when error-types | |||
| (insert " This is an unexpected CIDER middleware error.\n Please submit a bug report via `") | |||
There was a problem hiding this comment.
I softened this wording because we cannot be really sure if a given err/ex tuple was really caused by a bug.
| @@ -643,7 +643,9 @@ others." | |||
| "Emit toggle buttons for each of the ERROR-TYPES leading this stacktrace BUFFER." | |||
| (with-current-buffer buffer | |||
| (when error-types | |||
| (insert " This is an unexpected CIDER middleware error.\n Please submit a bug report via `") | |||
| (insert " This is a CIDER middleware error. | |||
| It may be a due to a bug, or perhaps simply to bad user input. | |||
There was a problem hiding this comment.
Hmm, the reasoning back in the day was that if bad input breaks the middleware it is a middleware bug, as we didn't anticipate some input. I think the new message is kind of confusing. I might be able to dig up the original discussion around this.
There was a problem hiding this comment.
I'd be open to such an insight.
But it seems fair to me to return a generic err / stacktrace on bad input - we could certainly "hide it under the rug" by returning a domain-specific result (e.g. :bad-macroexpansion-input), but that would only make debugging harder IMO.
There was a problem hiding this comment.
I think the work in this direction started here clojure-emacs/cider-nrepl#313
There was a problem hiding this comment.
Note that the macroexpansion middleware (like most in cider-nrepl) uses with-safe-transport nowadays.
(I added clojure-emacs/cider-nrepl@4b8a78b yesterday for extra coverage)
In a way, with-safe-transport does add a try/catch. Stacktraces can still pop up, but one can prevent that from happening by setting cider-show-error-buffer to 'only-in-repl, for example.
I'm not sure of what a good alternative would be. For Compliment (per clojure-emacs/cider-nrepl#313 ) or macroexpansion, do we really want to do nothing on errors and pretend they didn't happen?
That makes things harder to diagnose and debug.
Our middleware is not supposed to have bugs, so these will always be exceptional cases.
IMO, good clients simply handle the red paths, as this PR shows. Most times we already do.
There was a problem hiding this comment.
Yeah, I'm not arguing with that - I'm just saying that if I were an user the proposed message would be confusing to me, as it's hard to draw a line between a bug and bad user input. We can't expect the users to know what's unexpected input from some middleware without flagging this explicitly with some specialized error response.
There was a problem hiding this comment.
as it's hard to draw a line between a bug and bad user input
Indeed. Sometimes (and in this case, specifically) we cannot either. Macroexpansion implies running arbitrary Clojure code, so we can't quite be sure if that code is at fault, of our code.
Either way, all I've done is softened the wording from:
- this is a cider-nrepl bug
to:
- this may be a cider-nrepl bug, or may be bad user input.
So we'd be changing something easily misleading to something ambiguous. Ambiguous is better than misleading, at least in this context.
Ultimately, many Clojure users are savvy enough to have the intuition to see that macroexpansion failed because of the provided input. e.g. a usual debugging measure is to retry in vanilla clj.
cider-macroexpansion.el
Outdated
| (cider-nrepl-send-sync-request)))) | ||
| (nrepl-dbind-response result (expansion err) | ||
| (if err | ||
| (user-error "Macroexpansion failed. Please check *cider-error*") |
There was a problem hiding this comment.
See/check cider-error for more details
cider-macroexpansion.el
Outdated
| (nconc (when cider-macroexpansion-print-metadata | ||
| '("print-meta" "true"))) | ||
| (cider-nrepl-send-sync-request)))) | ||
| (nrepl-dbind-response result (expansion err) |
There was a problem hiding this comment.
There's no error marker (e.g. status) on failed macroexpansion, just err output?
|
Applied feedback. Feel free to reword the discussed message afterwards! |
Bad macroexpansions ultimately caused by bad user input would (correctly) cause the nrepl middleware to return an
err.However that red path wasn't handled.
Cheers - V