Revamp exception handling, from nsc

[dwijnand]

No description provided.

[smarter]

It looks like this overlaps with @robmwalsh's work in #16593, I presume you've seen that PR?

[robmwalsh]

I just want it fixed. If @dwijnand wants to run with it I'm happy for him to do so.

At an initial glance, it looks a bit more invasive than my approach, but withdrawn, sorry <3. it's based on nsc so perhaps it will be more familiar to contributors that have worked on both which will address some of @odersky's concerns in this area.

It appears this approach will only work for assertions - this isn't the only type of exception thrown in the compiler and I think it's important to cover them all.

My initial approach (never committed it) was just having a custom assert but I changed it to the more general implosion so other types of errors could be handled.

[robmwalsh]

I'm also happy to continue with my approach if that's the preferred option. I have time tomorrow and next week, but obviously would prefer not to spend more time on it if @dwijnand's approach is preferred

[dwijnand]

It looks like this overlaps with @robmwalsh's work in #16593, I presume you've seen that PR?

Yes, my approach was based on that, its feedback and nsc's approach.

more invasive than my approach

More invasive how? My intentions was to be less invasive that your pull request, while still being effective in fixing the underlying issues (in particular, not creating new exceptions as well as trying to avoid them again later).

It appears this approach will only work for assertions - this isn't the only type of exception thrown in the compiler and I think it's important to cover them all.

I do cover them all.

[robmwalsh]

It's too late to capture the context here. Unless I'm mistaken, the tree, symbol, position, even source file are all going to be wrong at this point, because the context they existed in got blown away many stack frames ago. see my last para here #16593 (comment)

@smarter's comment in the next reply seems to back this up

[dwijnand]

Yeah, this case effectively handles when compiler.newRun fails. If it succeeds, then there's the handling in run.compile. I just applied the same strategy to these existing catch cases, to avoid it spamming, in particular I took out re-printing the exception message.

[dwijnand]

So, one of the issues I looked to understand and fix, as Guillaume mentioned, the repeated printing of the trees. But also the "display error" and enormous printing of the tree case class toString that Chris posted.

Turns out these were closely connected: the exception handling in ReTyper, (part of Erasure.Typer), was printing the original, pre-erasure tree, but Erasure runs the next, post-erasure phase:

  def run(using Context): Unit = {
    val unit = ctx.compilationUnit
    unit.tpdTree = eraser.typedExpr(unit.tpdTree)(using ctx.fresh.setTyper(eraser).setPhase(this.next))
  }

That forced the trees denotations to update and transform, and for post-erasure that means running TypeErasure on the types. But ImportType isn't a type that is expected to exist post-erasure, because Import trees are dropped, and pre-erasure trees contain them...

So that's why I printed the trees using the previous mega phase. But I also changed the display logic so it just prints the exception class name, much like it nicely does for CyclicReference - there's both a mode and a Y-setting that a hacker can enable to unguard that catch. I also gave that in-exhaustive match in TypeErasure some more detail.

[robmwalsh]

More invasive how? My intentions was to be less invasive that your pull request, while still being effective in fixing the underlying issues (in particular, not creating new exceptions as well as trying to avoid them again later).

Sorry that comment was unnecessary and didn't add anything to the discussion. What I should have said (and was thinking) was that this requires new contributors to always use a different name for assert, where mine was designed to be source compatible (just import the new assert, no other changes required). I'm also not a fan of adding mutable state to Run for something that only exists in case of a crash. This is better attached to the exception, though I guess that's a matter of coding style.

I've also changed Implosion to be a ControlThrowable, so the current NonFatal extractor handling still works, but I can't expect you to know that because I didn't push it, just mentioned it on discord 🙃

It appears this approach will only work for assertions - this isn't the only type of exception thrown in the compiler and I think it's important to cover them all.

I do cover them all.

I wasn't clear enough here - all exceptions are indeed covered in that they are caught, but only the custom assert is capturing the context and doing something with it. It won't work in that the trees/symbol/pos etc will be wrong if not captured before the exception is thrown

[robmwalsh]

So, one of the issues I looked to understand and fix, as Guillaume mentioned, the repeated printing of the trees. But also the "display error" and enormous printing of the tree case class toString that Chris reported.

This is important work and should be retained regardless of other error handling shenanigans.

[robmwalsh]

this requires new contributors to always use a different name for assert

Did I misread something or did you change the name of the assert method in your force push?

[robmwalsh]

It looks like this overlaps with @robmwalsh's work in #16593, I presume you've seen that PR?
Yes, my approach was based on that, its feedback and nsc's approach.

@dwijnand In future, if you want to work on the same issue as me, I'd really appreciate it if you reached out to me so we could work together on solving it together rather than coming up with a competing solution and not even referencing my work in the PR. probably wouldn't even have seen this if @smarter didn't tag me and link to my PR.

As an outsider to both nsc and dotty, I had to put a fair bit of time into understanding how the error handling in both nsc and dotty worked so I could come up with something to solve an issue for all scala 3 users. Having a "competing" pr appear out of the blue is just making a generally shitty experience worse, and isn't at all encouraging to me as new (prospective) contributor.

[michelou]

It looks like this overlaps with @robmwalsh's work in #16593, I presume you've seen that PR?
Yes, my approach was based on that, its feedback and nsc's approach.

@dwijnand In future, if you want to work on the same issue as me, I'd really appreciate it if you reached out to me so we could work together on solving it together rather than coming up with a competing solution and not even referencing my work in the PR. probably wouldn't even have seen this if @smarter didn't tag me and link to my PR.

As an outsider to both nsc and dotty, I had to put a fair bit of time into understanding how the error handling in both nsc and dotty worked so I could come up with something to solve an issue for all scala 3 users. Having a "competing" pr appear out of the blue is just making a generally shitty experience worse, and isn't at all encouraging to me as new (prospective) contributor.

@robmwalsh I fully support your last comment.

[dwijnand]

What I should have said (and was thinking) was that this requires new contributors to always use a different name for assert, where mine was designed to be source compatible (just import the new assert, no other changes required).

Same. I think my diff might be misleading you: it's not assertShort, I'm introducing an enriched assert that captures the context (if available).

I'm also not a fan of adding mutable state to Run for something that only exists in case of a crash. This is better attached to the exception, though I guess that's a matter of coding style.

I also have a strong aversion to mutable state, but there's a few reasons I considered it: 1) it's in line with how lots of things are done in the compiler, 2) it avoids creating a new exception and then trying to manage that, 3) it avoids splitting the stacktrace, with the "caused by" part printing at the end. But perhaps with ControlThrowable and perhaps redefining the cause/stacktrace we can go back to that solution?

It won't work in that the trees/symbol/pos etc will be wrong if not captured before the exception is thrown

True. But as I see it, as a Scala user you just want some idea of what file, what line, and perhaps what phase the compiler crashed. You might not get the most precise symbol or narrowed position, but you have something. I'm all for enriching the creation of the other errors, judicially. I just enriched the current catch-all handling as a starting point.

---

@dwijnand In future, if you want to work on the same issue as me, I'd really appreciate it if you reached out to me so we could work together on solving it together rather than coming up with a competing solution and not even referencing my work in the PR. probably wouldn't even have seen this if @smarter didn't tag me and link to my PR.

As an outsider to both nsc and dotty, I had to put a fair bit of time into understanding how the error handling in both nsc and dotty worked so I could come up with something to solve an issue for all scala 3 users. Having a "competing" pr appear out of the blue is just making a generally shitty experience worse, and isn't at all encouraging to me as new (prospective) contributor.

I felt like that pull request had stalled and I felt like it would be great if we tried to find some solution to the underlying problem that was acceptable. But rather than continuing to debate things in words I thought I would focus on the code. I was actually concerned your experience there had put you off, seeing as you said you were "planning on working on this some more tomorrow" and then nothing else.

I submitted this as a draft pull request to run CI on it and because it's easier to show Seth in a GitHub diff. Guillaume beat me by a few hours, because I would have linked the two and atted people probably this morning, once I got it green (which it isn't yet 🙄). I would rather take people's attention when I'm sure I have something of value to share and worthy of their time, which is why I didn't mention it before had for fear of overpromising.

I also just want this fixed. So I get the sense that you're willing to push on in your pull request, which I'm really happy to hear. I apologise to you, Rob, that I made your experience worse. 😞

Please feel free to use any ideas here, and feel free to reach out if you need any help.

[robmwalsh]

I also have a strong aversion to mutable state, but there's a few reasons I considered it: 1) it's in line with how lots of things are done in the compiler, 2) it avoids creating a new exception and then trying to manage that, 3) it avoids splitting the stacktrace, with the "caused by" part printing at the end. But perhaps with ControlThrowable and perhaps redefining the cause/stacktrace we can go back to that solution?

That seems reasonable. You're right, splitting the stack trace with "caused by" is undesirable. I'll fix that.

True. But as I see it, as a Scala user you just want some idea of what file, what line, and perhaps what phase the compiler crashed. You might not get the most precise symbol or narrowed position, but you have something. I'm all for enriching the creation of the other errors, judicially. I just enriched the current catch-all handling as a starting point.

Primarily I want to know the exact line and position. Including all the other stuff was more because I thought it would be useful for dotty maintainers (and that's what nsc does). I just raised another crash report here #16696
An exact position on that would have made the problem obvious and saved me significant time.

I was actually concerned your experience there had put you off, seeing as you said you were "planning on working on this some more tomorrow" and then nothing else.

I must say I did come close to giving up on it. I did end up starting on it the next day but then life got in the way so didn't get much done. Perhaps next time just ask :)

I apologise to you, Rob, that I made your experience worse.

Thanks, I do appreciate that.

Please feel free to use any ideas here, and feel free to reach out if you need any help.

And this too. I haven't looked at it in detail but I suspect I may need some help on the repeated tree print issue you looked at as part of this. Alternatively perhaps you could deal with that in a separate PR.

[dwijnand]

I haven't looked at it in detail but I suspect I may need some help on the repeated tree print issue you looked at as part of this.

You are already, in terms of the repetition in typerUnadapted by virtue of ctx.implode's avoidance of Implosion. The other cause for repetition, using #16207 as reference, is the print exception and rethrow that I took out of Driver.doCompile seeing as the assert in #16207 actually prints out a tree it built and failed on, embedded in the assertion message.