Improved crash handling #16593
Improved crash handling #16593
Conversation
|
I've signed the CLA, please recheck |
|
Some other notes:
To solve these issues, I propose some new rules:
|
|
I actually think that the current exception handling in the compiler is mostly fine. Essentially the rules are:
I like the idea of documenting expected exceptions with a I also like the idea to fine-tune what gets reported to the user when they note an exception. We should prefix that with a friendly invitation to submit a bug report. So I'd like to keep that part of the proposal. Truncating exception trace is not helpful IMO. If an exception occurs we want to see all the info that's available in the bug report. But more sweeping changes might cause major friction and loss of time for the people fixing bugs. There are many established processes that help us fixing issues quickly. I am skeptical that introducing more ceremony like |
Just to throw one more outsiders opinion in here. I think this is true for those working on the compiler on a regular basis. However, I've experience this first hand that when an assertion fails in the compiler, I literally have no idea what it means half the time. It's incredibly hard to make sense of the huge jumbled tree that you get without detailed knowledge. UX improvements in this area could really really improve this. I encourage you to look at this from the perspective of someone that doesn't have literally dozens of years working on compilers and dealing with trees.
This totally makes sense, we should probably for sure keep an easy way to get the entire output for reports.
True, but having a much nicer output when things blow up with a message that's easy to digest may encourage users to more often report these things, which could also help those working on fixing the bugs tackle them. |
I started working on this because, like @ckipp01, I had no idea what the cause might be. The crash report asks for a minimized example - when you don't even know how to get your code compiling again, that's a hard ask.
I agree that more information is generally better; however many stack traces (and sometimes the trees that get printed with them) are so large that it gets lost (as in scrolls off the top of my terminal). The bottom 100 lines of the stack trace are useless if you miss even a few rows at the top. A way around this would be to keep the truncated stack trace to display to the user (along with other helpful info) like tree position etc., then dump as much info as possible into a zip file which the user can review for more information or submit with the bug report. I think putting the sources in a 2nd zip file may be helpful, so users can choose to submit sources or not depending on the sensitivity
The amount of ceremony involved is minimal. This is important because of the way that Contexts work - if you throw, you blow away all the stack frames between where you threw and where you catch - along with whatever contexts exist within them. This is exacerbated because a lot of care has (rightly) been taken to ensure references to contexts don't survive. This approach is different to scala 2, where a lot more state survives when an exception is thrown. Implosion guarantees that the most up-to-date context information is caught. That can only be a good thing for the compiler team trying to fix bugs and provides a much better experience for people who don't have intimate knowledge of the compiler. |
|
I agree with Martin that we should never truncate stacktraces, they're precious information and they rarely get very big (unless we get a stack overflow, which we try to manage with |
|
|
||
| /** Extension methods for contexts where we want to keep the ctx.<methodName> syntax */ | ||
| object ContextOps: | ||
|
|
||
| case class Implosion(ex: Throwable) extends Exception(ex) |
There was a problem hiding this comment.
I think Implosion should be an Error, although that won't be enough to pass through the compiler cleanly since there's many places where we catch all Throwable or use the NonFatal extractor
There was a problem hiding this comment.
I think Error probably makes sense. Per previous comment I think the use of Throwable & the NonFatal extractor should be replaced with something specific to the compiler which won't catch ControlFlow exceptions, so this wouldn't be a problem
There was a problem hiding this comment.
Changed to ControlThrowable instead of Error to avoid being caught by existing NonFatal extractors
|
A "final" production compiler could dump to a file. The nice user message shows the path to the file to submit the bug report. That's much nicer than cut/paste from terminal. (Though I tire of hotspot crash files accumulating.) Any other development version dumps to stderr as usual, or also under Dotty needs Someone recently asked why Scala 2 doesn't do better on crashes, meaning exits not initiated on I like that "implode" suggests "controlled implosion", but it also sounds jokey to me. I think "abort" can be avoided. Maybe "exit" is sufficient to say we are exiting instead of completing normally. In a cinema, there is no distinction between an emergency exit and a normal one. If I am exiting and the movie hasn't finished, it must be an emergency. There is An alternative to log and throw is to throw and catch in main and then log. Someone added a |
|
very related: |
|
I still don't see the value of |
Perhaps the movie is just terrible :) I walked out of Man on Fire because it was terrible - from the ratings others disagreed, but I digress -
Thanks - I'll look into it.
I've suggested a custom |
It's not just about maintainers though, people have to actually use the compiler as well, and shouldn't be expected to deal with or have any understanding of compiler internals. Asking for a minimized reproduction is a big disincentive to even report crashes (see the http4s comment above). I honestly believe these compiler crashes are hurting the adoption of scala 3. I recently started a greenfields project in scala 3, very excited to use all these great new features and honestly, it's the single biggest productivity mistake I've made so far. I didn't want to start a brand new project in 2.13 because 2.x is something close to EOL, but after using in in anger for 3 months, my opinion that scala 3 isn't ready for serious use at this stage. That means there are currently no good options for a new scala project and very little incentive for existing projects to upgrade. Scala is in no man's land at the moment, and if these issues don't improve soon, it may seriously damage scala's usage. In any case, if I make the changes I mentioned above, you should have far more information to work with, not less, because I can stuff as much state as you like into a zip file, and can even get it to print full stack traces and whatever other information you'd care for if you run the compiler with some verbose option (I assume one exists but I don't generally want to know anything other than what errors I need to fix). |
|
If you want to stabilize Scala 3 there are better options than make existing compiler maintainers drop into a productivity hole because you change the infrastructure they are used to work with.
Both of these are hard, but both of these have definite positive impact. EDIT: I see now that you did file three issues that are quite actionable. I was using the wrong search term so I did not see them before. Thank you for that! Hopefully someone will have a look at them soon. I am still very sceptical about changing error output of the compiler. There's a lot of implicit knowledge going on that could be made useless, and we don't want that. |
I really don't see how this proposal will significantly change either your infrastructure or workflows for solving problems. You'll still have all the information you currently have plus significantly more to help you nail down problems. I'm planning on working on this some more tomorrow, perhaps once I push some more updates you'll feel more comfortable with it. So far everyone else who has commented seems generally supportive of this change.
As you note, in your edit, yes I have, but I've also had many crashes where I have no idea where to even start minimization, and these haven't been reported. Frequently I just don't have the time to even consider it. My new workflow for a crash is to commit in a new branch so I have a crash to come back to if I find the time, then just undo a bunch of changes or go back to my last commit and try a different approach.
You've been working on dotty for many years now and have intricate knowledge of the compiler, in addition to decades of experience working on compilers in general. I don't think you realise how opaque and generally unapproachable the dotty codebase is to an outsider. I've read all the docs I can find and asked a bunch of questions on discord (big thanks to everyone there and especially @SethTisue for their help), and I'm still just scratching the surface. I'm still a long way from being able to track down and fix a bug. I happen to have a burning desire to know how just about everything under the sun works, but most don't share that interest. If I'm struggling, I suspect many may not even get as far as I have. In any case, not everyone wants to become dotty contributor, and nor should they need to in order to use scala. There are many prolific OS contributors that could contribute here, but that would take away from their contributions elsewhere. Others are already pulling long hours at companies trying to deliver their own products.
This PR is (the start of) my attempt at making both of these options significantly easier for users.
One of those issues is from April. There's little incentive to spend time minimising and reporting errors if they don't get fixed. Someone that shall remain nameless unless they wish to identify themselves said to me fairly recently that "reporting compiler errors without a PR to fix may as well be sending them to /dev/null". This is obviously an exaggeration, but hopefully, you can see the grain of truth in the comment that might cause this person to say that.
I still don't think my proposed changes will fundamentally change anything here, but of course, I have no idea what this implicit knowledge is and no way of finding out. Let's revisit this once I've made some more changes, and perhaps you can give me some concrete examples of how the changes make things harder for maintainers and perhaps I can address some of those issues. I really want to see Scala 3 succeed, but for it to succeed, it needs to be easy for users (including when things go wrong) as well as maintainers. |
|
I really hope we can find some way forward here, as I strongly agree that the problem here for end users is real. I've seen it over and over again in various chat rooms, forums, and bug trackers that actual Scala 3 users are hitting compiler crashes and the compiler is giving them almost nothing useful to help them and encourage them to isolate, minimize, and report the problem so it can be fixed. http4s/http4s#6899 (comment) is a perfect example, and I'm sorry to report that it isn't an outlier — it's something I'm seeing happen over and over again out in the community. We should do our best to find some way to meet our users at least halfway on this. It will help them help us and everyone will benefit. |
|
by no means finished, but I've tried to address some feedback and incorporated a lot of @dwijnand's work from #16685 (thanks for your work on this). I haven't got to writing files yet so at this stage displayed stack traces are not truncated - I would like to do this once they're saved using
I've used I've also added a "black box" to the I'm getting a couple of compiler errors, and I can't see how they relate to anything I've done. If anybody has any suggestions on fixing this it'd be appreciated :) |
I think this will be a more controversial change (I would likely be against it), so I'd prefer to see it in a separate PR.
|
| class SuspendException extends Exception | ||
| class SuspendException extends ControlThrowable |
There was a problem hiding this comment.
This change came up recently and was rejected: #16616 (comment)
More generally it seems from https://contributors.scala-lang.org/t/nonfatal-and-controlthrowable-changed-in-stdlib/6049 that there is a desire to move away from / deprecate ControlThrowable.
Makes sense, I'll leave that out for now
Ah, thanks! I changed this and Since |
|
Just as a meta-comment here, perhaps some splitting into multiple PRs would help, so that any separable clear wins could get merged, as we continue to discuss anything that seems to require more consideration. (EDIT: Oh, I see I'm seconding Guillaume.) |
Got some initial runs going on a couple of issues last night and it's looking really nice thanks to @dwijnand's significantly better formatting, so yeah I think I could do some minor tidy up tonight then I'll ask Martin to take another look. I'll just revert the |
|
pretty :) |
robmwalsh
force-pushed
the
better-crash-handling
branch
from
2b7f900
to
0955899
Compare
|
#16207 is a great example, because it prints a huge, compiler-generated tree in the assertion message, as a compiler hacker's choice. As that message (in the exception) is returned by the compiler and gets printed (at least in sbt running - not sure about in zinc compile) perhaps we shouldn't enrich the message and print the enrichment and then throw with the original message. I think we should just throw an exception with an enriched message so it gets printed once, followed by the stacktrace (in the default handling of exception). |
robmwalsh
force-pushed
the
better-crash-handling
branch
from
0955899
to
cb548ff
Compare
I did consider this originally, but decided to use report.error instead so that diagnostics work in an IDE. Metals currently silently fails on a compiler crash which is rather frustrating, and one of the reasons I was getting into so much trouble with compiler crashes, because it wasn't obvious which change introduced the crash! Are you saying it prints the message twice using SBT with this implementation? It shouldn't be, because at the edges of the compiler the |
|
Looks like I have a few more errors to resolve :( that's a job for another day |
Your own comment shows it printed twice: #16593 (comment) |
Oh, right 🙃 It was a long day... Thanks for the pickup. |
|
a rebase should make the Scaladoc failure go away (we think, anyway) the "Context was thrown away, this crash report may not be accurate" failure(s?) seem like a genuine logic problem with the current diffs |
|
Thanks @SethTisue, I'll have some time over this weekend to take another look. |
robmwalsh
force-pushed
the
better-crash-handling
branch
from
cb548ff
to
5b5083d
Compare
|
@robmwalsh would you be ok with me landing some of the initial improvements I had my eye on in #16685, while this isn't moving? |
|
@dwijnand go for it. I've spent some time trying to figure out what's wrong with this and I'm a bit stumped. |
| * use: | ||
| * `ctx.implode(Exception("foo"))` | ||
| */ | ||
| inline def implode(inline cause: Throwable): Nothing = |
There was a problem hiding this comment.
| inline def implode(inline cause: Throwable): Nothing = | |
| def implode(inline cause: Throwable): Nothing = |
There was a problem hiding this comment.
We will not gain anything from inlining this definition. We might actually end up generating more code.
| /** | ||
| * Crash the compiler with a message in a controlled way | ||
| */ | ||
| inline def implode(inline msg: Any): Nothing = |
There was a problem hiding this comment.
| inline def implode(inline msg: Any): Nothing = | |
| def implode(inline msg: Any): Nothing = |
| val blackBox = new StringBuilder() | ||
|
|
||
| extension [A](a: A) | ||
| transparent inline def record: A = |
There was a problem hiding this comment.
| transparent inline def record: A = | |
| def record: a.type = |
|
|
||
| transparent inline def assert(inline assertion: Boolean, inline message: Any): Unit = | ||
| if !assertion then | ||
| summonFrom { |
There was a problem hiding this comment.
Indentation is missing here.
| transparent inline def assert(inline assertion: Boolean): Unit = | ||
| assert(assertion, null) | ||
|
|
||
| transparent inline def assert(inline assertion: Boolean, inline message: Any): Unit = |
There was a problem hiding this comment.
It would be better if this one is not called assert because it can be confused with the normal assert. There is no way to distinguish a call to this assert without a context with a call to Predef assert.
following on from discussion here https://discord.com/channels/632150470000902164/632628489719382036/1052965390382280714
adds an
implodemethod to dump state from the context and a betterassertwhich makes use of this for some hope at figuring out what's causing the crashes.assertis source compatible withPredef.assertso swapping can be as easy as importing it.cc @smarter
example (updated) compiler output for #16207 (somewhat mangled by markdown) :