Stop catching Throwable/IIOBE/NPE in the compiler, outside of stack overflow handling#25832
Conversation
| case ex: ClosedByInterruptException => | ||
| try Files.deleteIfExists(path) // don't leave a empty of half-written classfile around after an interrupt | ||
| catch { case _: Throwable => () } | ||
| catch { case _: java.io.IOException => () } |
There was a problem hiding this comment.
for the IO-related ones I followed the Javadoc, and in one case removed a catch because it was calling one of our own util methods that already catches and ignores internally
| case ex: CompilationUnit.SuspendException => throw ex | ||
| case ex: Throwable => | ||
| if !ex.isInstanceOf[TypeError] then ex.printStackTrace() | ||
| case ex: TypeError => |
There was a problem hiding this comment.
given that we were willing to print a stack trace for non-typeerrors, I think letting it bubble up to "compiler bug" is better
There was a problem hiding this comment.
TODO: double check that if something bad happens in the backend we get the usual compiler "plz file a bug" message (says Seb)
| catch { | ||
| case ex: Throwable => | ||
| handleRecursive("member names", i"of $this", ex) | ||
| case ex: Throwable => handleRecursive("member names", i"of $this", ex) |
There was a problem hiding this comment.
I changed the formatting for the handleRecursives where it was easy to do so they're easier to look at in search results, since I'll deal with those in the next step
lrytz
left a comment
There was a problem hiding this comment.
For the standard library this looks like a very substantial breaking change that I think we cannot "just" do?
e40c10d to
a9d11dc
Compare
3381d22 to
11742a6
Compare
There was a problem hiding this comment.
As discussed orally, I was wondering if NonFatal is really the right tool (it seems ad hoc, will need to be updated, is potentially slow with its 5 type tests, etc.) or if we could more radically only catch Exception instead of Throwable. I believe it was the original idea behind the separation between Error and Exception, but I don't know if it would be practical.
2c3a9b3 to
3a196e0
Compare
| Interpreter.suspendOnMissing(sym, origin, annot.tree) | ||
| // We're dealing with user-thrown things here, so we must use NonFatal to catch anything realistic, | ||
| // including `Error`s like `NotImplementedError` | ||
| case NonFatal(ex) => |
There was a problem hiding this comment.
the only remaining use of NonFatal, can't really get rid of it :/
There was a problem hiding this comment.
fun fact: ??? is an Error not an Exception so if someone uses it in a macro we have to be able to catch that... there is indeed a test that does so
| val instantiated = dealiased.instantiate(args) | ||
| if (followAlias) instantiated.normalized else instantiated | ||
| catch | ||
| case ex: Throwable => handleRecursive("try to instantiate", i"$dealiased[$args%, %]", ex) |
There was a problem hiding this comment.
this is the only non-mechanical change, I had to add a specific guard for what the catch case IndexOutOfBoundsException was trying to prevent
sjrd
left a comment
There was a problem hiding this comment.
Is the title still accurate? It suggests that we still catch IOOBE and NPE, but the PR content suggests that those are not caught anymore.
| case e: Exception => | ||
| println(s"Compile $path exception:") | ||
| e.printStackTrace() | ||
| throw e |
There was a problem hiding this comment.
I don't think we should drop exceptions on the floor during tests. I've already had to fix one such case in another PR where I was seeing crashes due to stack overflows while running tests yet the tests "passed"
72ad800 to
fb22933
Compare
…verflow handling (scala#25832) Part of scala#25799 ## How much have you relied on LLM-based tools in this contribution? Not at all ## How was the solution tested? Covered by existing tests (this is a refactoring)
Part of #25799
How much have you relied on LLM-based tools in this contribution?
Not at all
How was the solution tested?
Covered by existing tests (this is a refactoring)