Analyze stacktrace in region or at point

[r0man]

This PR adds functionality to Orchard to support analyze stacktrace in region or at point commands in Cider.

Problem

As a Clojure programmer I deal a lot with stacktraces. When evaluating code or dealing with the REPL, Cider has me covered here with its stacktrace inspector. Which is great!

But the stacktraces I'm looking at in the Cider stacktrace inspector are unfortunately only half of the stacktraces I am looking at in total. Many stacktraces I look at, I encounter in all sorts of Emacs buffers. Those stacktraces are printed in different formats like Java (.printStackTrace), Clojure (prn, #error tagged literal, clojure.stacktrace). Sometimes even buried within strings themselves or printed in a non-human friendly way. I would like to view those stacktraces pretty printed in the Cider inspector as well. With minimal effort, using Emacs commands like these:

• M-x cider-stacktrace-analyze-in-region Show the stacktrace in region in the Cider stacktrace inspector
• M-x cider-stacktrace-analyze-at-point Show the stacktrace at point in the Cider stacktrace inspector

Solution

Let the client select the stacktrace to analyze (via cider-stacktrace-analyze-in-region for example), let the middleware parse and analyze it, and show the analysis in the client's stacktrace inspector.

Approach

Add an optional "stacktrace" parameter to the Cider stacktrace middleware which the client can provide. If the "stacktrace" parameter is not blank, its value will be parsed and analyzed instead of the default *e session parameter. The middleware tries various strategies (different parsers for different formats, unwrapping an exception buried in a string, etc.) to produce a stacktrace analysis as a result.

Limitations

• At the moment there's only one parser implemented in the middleware. The parser for the #error tagged literal, which Clojure uses to print stacktraces with pr. The idea is to make this plug-able by using a multi method for example. There could be more parsers for stacktraces produced by other printers like the Java .printStackTrace printer, the clojure.stacktrace printer, etc.

Open questions

• What's the best approach to implement those parsers?
• Are the Clojure libraries out there that can parse already many formats?
• Can we depend on an external library for this if there is some benefit?
• Could we depend on Instaparse (supports Clojure since 1.5) in case we build our own parsers?
• I moved analyzer namespace from cider-nrepl, because I thought this is something that could live in orchard. Is this ok?

---

Before submitting a PR make sure the following things have been done:

• [x] The commits are consistent with our contribution guidelines
• [x] You've added tests to cover your change(s)
• [x] All tests are passing
• [x] The new code is not generating reflection warnings
• [ ] You've updated the changelog (if adding/changing user-visible functionality)

What do you think of this idea?

[vemv]

I moved analyzer namespace from cider-nrepl, because I thought this is something that could live in orchard. Is this ok?

I think so, my undertanding is that cider-nrepl should be 'hollowed out' when possible

[bbatsov]

I think so, my undertanding is that cider-nrepl should be 'hollowed out' when possible

Yeah, this was planned for a very long time, but I never got to doing it. At the end of the day cider-nrepl should should be only middleware and middleware-specific support/utility code.

[r0man]

Hi @bbatsov and @vemv,

I added support for parsing stacktraces that have been printed by:

• Aviso https://github.com/AvisoNovate/pretty - The parser is implemented through a Instaparse grammar.

• Clojure - the #error tagged literal. This is just a edn/read-string on a string, with garbage ignored at the begining.

• Java - The parser is implemented through a Instaparse grammar and parses stacktraces produced with .printStackTrace.

On the Cider frontend, I kept things very simple for now. I'm using think-at-point with sentence and paragraph to find the current stacktrace at point. This seems to work fine in the majority of cases.

However, there's sometimes an issue with Aviso stacktraces if point is inside a stacktrace. Sometimes thing-at-point only grabs part of an Aviso stacktrace, and the stacktrace that gets analyzed is not the full one. Something to improve on.

Another issue, would you be fine with adding Instaparse as a dependency once [1] is resolved?

WDYT?

[1] https://github.com/benedekfazekas/mranderson/pull/71

[vemv]

Kudos for the outstanding work!

Will review during the week

[bbatsov]

Another issue, would you be fine with adding Instaparse as a dependency once [1] is resolved?

I'll have to think about this. There has been so much pain with MrAnderson over the years, that I'm really not excited at all to add it here, but I can live with it as a regular runtime dep. I assume almost no regular application would depend on Instaparse anyways.

[r0man]

Yes, I can understand that point of view. I just felt the pain. :)

[bbatsov]

Seems this has to be rebased.

As for instaparse dep - I've checked the project and I saw it has no runtime deps, so I guess it's going to be mostly OK to depend on it directly in Orchard. We'll have to update our docs where we say we don't have any runtime deps, though. :-)

[bbatsov]

One more thing, while you're at this you might want to create some doc about the stacktrace functionality (similar to what you did for the inspector). At the very least we should document the support for various types of stacktraces that you've added.

[bbatsov]

Really impressive work so far! Just a couple of small suggestions:

• add a couple of API usage examples in the docs
• mention in the docs how to create new parsers
• add :added metadata and top-level docstrings to all new namespaces

Let me know when the PR is ready for a final review.

[r0man]

Ok, will look into your suggestion. Will ping you when I am ready

[r0man]

An update about Mr Anderson. @benedekfazekas helped me a lot with it and I am optimistic we will get it working. One issue is already solved, and the other could easily be avoided be removing the dash from the namespace Cider uses for inlining dependencies (use cider.nrepl.inlined.deps instead of cider.nrepl.inlined-deps). I believe this is a safer setting anyway, for other inlined libraries as well. Avoids also a whole class of issues going forward.

About just copying the Instaparse sources. We have the versions of Mr Anderson and Instaparse pinned when we build the cider-nrepl JAR. So I am wondering if hand copying buys us much there? But I might be missing something. That said. I'm not against copying, I was just wondering. Let's see what @bbatsov thinks.

[vemv]

For the record, having cutted many refactor-nrepl releases over the last year or so I'm very confident about mranderson. Supressing its parallelism plus using other recent commits (we fixed this and that there) has made the experience a whole lot better than previously.

I'm only suggesting copying "to avoid debates" :)

[bbatsov]

Even if MrAnderson was working flawlessly, I'd still be against using it in this case, as including the inlining step complicates the build process and inflates the size of the final artifact considerably (the inlining made cider-nrepl huge and forced us to further complicate stuff with deferred loading, etc). Given instaparse is a library that's fairly unlikely to conflict with an user project (it's not a common dep in Clojure apps) I'd rather just include it as a runtime dep and see if we run into some practical issues. Plus, to my knowledge it has a very stable API, which makes the dep resolution step relatively safe.

I'm very much against inlining the whole library manually, as this makes it hard to track upstream changes. Let's go with the simplest possible approach and consider the next steps only if we run into some actual problems.

[r0man]

I don't have strong feelings here, so I'm fine with trying it.

[vemv]

Giving this a shot just in case...

including the inlining step complicates the build process

It's a well-known step, would be a matter of copying it from refactor-nrepl's build for instance.

and inflates the size of the final artifact considerably

Would it really, or is the past biasing us here? mranderson would copy a handful of .clj namespaces - how much does that really weigh?

it's not a common dep in Clojure apps / I'd rather just include it as a runtime dep and see if we run into some practical issues.

Honestly (very honestly! not for the sake of argumentation) this is just authoring a bug and waiting for users to eventually see it. Not even a new kind of bug - it has been seen time and again in other projects as well, which is why they use mranderson (or dolly - like Eastwood does).

All in all, https://github.com/benedekfazekas/mranderson/tree/master/.circleci has cider-nrepl and refactor-nrepl clones as part of its CI build. That's about as reliable as things can get.

Good chance to get with the times here 😄 and make things better for everyone.

[bbatsov]

Orchard is already inlined in cider-nrepl and to my knowledge it has very little usage outside of it currently, so I really don't see any point in complicating it for the sake of fixing problems that might not even exist. I'm a big believer in simplicity and avoiding premature optimizations.

[bbatsov]

A couple of other points:

• for my general distaste for inlining - there are constantly problem even today. E.g. this is the most recent case of an inlined library breaking for no obvious reason https://github.com/clojure-emacs/cider-nrepl/commit/3de551c77da266185d6a161bf65d4ece81224027 Over the course of years I've wasted a lot of time tracing updates that broke with MrAnderson. It's extremely hard (impossible) to convince me at this point of the robustness of it
• I've started Orchard with the intention to be self-contained exactly to avoid having to deal with things like this. I'm a huge believer in self-contained libraries and reducing external deps in general

As I think that this stacktrace parsing is useful for most people I'm willing to make a compromise with the second point, but I'm adamantly against using MrAnderson with Orchard at this time.

[bbatsov]

I'm finally back home after 2 weeks on the road and I'd like us to wrap this up before my next batch of business travel kicks in. @r0man is there any remaining work to be done from your perspective?

[r0man]

@bbatsov I think this is mostly ready. Since I'm AWFK for the next 2 weeks I would not merge it now. Let's wait a bit.

Regarding the inlining of Instaparse. I got it working with the help of @benedekfazekas and I'm using it for a while now without any issue noticed. So I pushed this here [1]. I also think @vemv has a point that adding Instaparse does not add too much bloat. So since we got it working, why not play safe? I actually do remember having dealt with a version conflict on Instaparse. That was years ago, but if some old version is floating around on the classpath of some users, it might hit them.

This would be my 2cents on this topic. If you are strictly against this, I can revert the commit. WDTY?

[1] https://github.com/clojure-emacs/cider-nrepl/pull/758/commits/3864afe1a4813b955984ead18e04546cad238dbb

[vemv]

or my general distaste for inlining - there are constantly problem even today. E.g. this is the most recent case of an inlined library breaking for no obvious reason https://github.com/clojure-emacs/cider-nrepl/commit/3de551c77da266185d6a161bf65d4ece81224027

I tracked down the issue to https://github.com/greglook/clj-arrangement/pull/5 and also reported it as https://github.com/benedekfazekas/mranderson/issues/76

It's a quite extreme edge case - a workaround for an issue that no longer exists in the first place.

I think that very rarely things are perfect "by default". If we get a proactive stance e.g. report issues, we give mr-anderson a chance to improve. Else it's sort of a self-fulfilling prophecy 🌀

In less philosophical terms, just last week https://github.com/benedekfazekas/mranderson/commits/master got a few tasty news. Looks like there are more to come!

[bbatsov]

I think that very rarely things are perfect "by default". If we get a proactive stance e.g. report issues, we give mr-anderson a chance to improve. Else it's sort of a self-fulfilling prophecy 🌀

I don't expect things to be perfect, but I definitely expect that I'd be running into less issues than has been my experience with MrAnderson so far. Anyways, I'm happy to see that you and @lread are helping with the maintenance and I hope that things are going to improve there down the road.

In less philosophical terms, just last week https://github.com/benedekfazekas/mranderson/commits/master got a few tasty news. Looks like there are more to come!

Might be good to cut a new release soon. The last one is ancient.

[bbatsov]

@bbatsov I think this is mostly ready. Since I'm AWFK for the next 2 weeks I would not merge it now. Let's wait a bit.

No rush. I'll consider cutting a smaller Orchard release just for the Spec 2 updates.

Regarding the inlining of Instaparse. I got it working with the help of @benedekfazekas and I'm using it for a while now without any issue noticed. So I pushed this here [1]. I also think @vemv has a point that adding Instaparse does not add too much bloat. So since we got it working, why not play safe? I actually do remember having dealt with a version conflict on Instaparse. That was years ago, but if some old version is floating around on the classpath of some users, it might hit them.

So, the idea is to make this an optional dependency for Orchard and have cider-nrepl provide it, right? If so - that's perfectly fine by me. My main point was that I didn't want to add MrAnderson to Orchard's build chain and that if even if Instaparse is a runtime dep of Orchard it would still be transitively inlined in cider-nrepl, but in principle I like the idea to make this optional even more. I guess it will just be nice to have some minimal fallback for the default stacktrace format in case instaparse is not present, so the stacktrace functionality is not completely absent without it.

[vemv]

There's also the option of moving this PR's code to a new artifact e.g. cider/stacktrace-analyzer.

I see a few advantages:

• no optional dependencies - it's less confusing for consumers
• clearer policy around mranderson - it's something we either use or not (and not some sort of "depends")
• leaner codebase, build times, etc. Else orchard could end up being sort of monolithic.

In the end, this PR's code probably depends on Orchard utils, while the rest of Orchard does not depend on stacktrace analysis.

So this repo separation would reify that dependency relationship.

[bbatsov]

Yeah, I've been thinking about something along those lines myself. I can imagine the stacktrace analyzer having some value on its own, but this will mean a bit of extra work for us (one more library to maintain). Overall I do like the idea, as it seems like a good compromise.

[vemv]

Given we agree I'd be happy to contribute the initial setup (Makefile/CI/deployment pipeline)

If you'd like to proceed - any suggested name for the project?

[bbatsov]

That's the hardest question always! :D stacktrace-analyzer is kind of accurate, but super boring. I was thinking of something like "pimp my stacktrace", but that might be a bit too much. Let me think about this.

[vemv]

That's the hardest question always!

(@r0man input most welcome of course!)

[r0man]

Yeah, I'm thinking. :)

[r0man]

Ok, maybe here is one to make @bbatsov happy. :) What about stackbow? According to https://www.vice.com/en/article/7xp3xq/a-fine-cider-sommelier-judges-the-uks-worst-ciders strongbow is one of the worst Ciders in UK. Replace strong with stack -> stackbow.

[bbatsov]

It's a funny name for sure, but I'm leaning towards haystack and restack right now. If you don't have any objections let's go with one of them.