Warn when catching Errors - #26794
Conversation
| */ | ||
| override def checkConformsExpr(actual: Type, expected: Type, tree: Tree, notes: List[Note])(using Context): Type = | ||
| val saved = ccState.ignoreClassifiers | ||
| try |
There was a problem hiding this comment.
review without whitespace
|
|
||
| /** Use a class loader to load the plugin class. | ||
| */ | ||
| @nowarn("msg=Catching NoClassDefFoundError can lead to unexpected behavior") // backwards compat |
There was a problem hiding this comment.
I'm not sure about this one. loadClass throws ClassNotFoundException if it can't find the class. So we're catching errors that would happen if the class is found but its dependencies are not?
| catch | ||
| case err: AssertionError => | ||
| logger.log(Level.WARNING, err.getMessage()) | ||
| false |
There was a problem hiding this comment.
I don't see why assertion errors would be less severe than other errors (which use Level.SEVERE when logged just below)
9784d87 to
7250db7
Compare
7250db7 to
f547d48
Compare
SolalPirelli
added
the
needs-squashing
f547d48 to
8414e6a
Compare
|
@sjrd does this look like a reasonable thing to do overall? before I send this for a proper review |
|
That is way too strong to be a default, IMO. Not all Perhaps put this behind a linting flag. |
|
Test frameworks are special cases. I like the structure added for logging failed assertions. Everyone has a favored debug style, of course. |
Right, this is definitely a warning that expects users to silence it in specific "module boundary" cases where you do actually need to catch, say, AssertionError for a testing framework. Putting it behind a lint flag is appealing, though I wonder what % of users actually need to catch Errors for their software to make sense. I'd expect most libraries and apps never do, since they depend on a framework that does it for them. |
3 participants
Closes #25799
Final piece of the "stop catching stack overflow" puzzle: discourage everyone from catching errors that are likely to leave their program in a dubious state, or even to not be caught on Scala.js.
Review commit by commit
Have you relied on LLM-based tools in this contribution?
No
How was the solution tested?
New automated tests (including the issue's reproducer, if applicable)