-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better handling of multiple exceptions for saferExceptions #13914
Better handling of multiple exceptions for saferExceptions #13914
Conversation
965793b
to
7dfad2c
Compare
* Generate a single accumulated CanThrow capability for multiple catch cases in a try * Allow parentheses around exceptions alternatives in throws clauses
7dfad2c
to
05114db
Compare
@odersky could you have a look at the PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this scenario?
def foo() throws E1 | E2 = ...
try
try foo()
catch case ex: E1 => ...
catch case ex: E2 => ...
That should compile. Does it?
if Feature.enabled(Feature.saferExceptions) then | ||
for | ||
CaseDef(pat, guard, _) <- cases | ||
tpe = pat.tpe.widen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that an =
in a for expression is horribly expensive, so it's actually better to do pat.tpe.widen
twice.
caughtExceptions match | ||
case Nil => expr | ||
case head :: tail => | ||
val capabilityProof = tail.foldLeft(head: Type)(OrType(_, _, true)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not:
if caughtExceptions.isEmpty then expr
else
val capabilityProof = caughtExceptions.reduce(OrType(_, _, true))
...
Catching multiple exceptions in nested catch
case _: Ex1 =>
case _: Ex2 => will work (which is already an improvement when compared to the current state) but catch
case _: Ex1 =>
catch
case _: Ex2 => will fail but this doesn't work without my changes either so there's no regression. Also the docs explicitly say that
try
try
foo()
catch
case _: Ex1 =>
catch
case _: Ex2 => gets desugared to try
erased given CanThrow[Ex2] = ???
try
erased given CanThrow[Ex1 | Ex2] = ???
foo()
catch
case _: Ex1 =>
catch
case _: Ex2 => instead of try
erased given CanThrow[Ex2] = ???
try
erased given CanThrow[Ex1] = ???
foo()
catch
case _: Ex1 =>
catch
case _: Ex2 => (1) would solve both the problem that (2) fixes and the corner case described in the docs when someone tries to call a method defined with a merged capability When I think about it now, (2) shouldn't be too difficult to implement but still this could be done in a separate PR. |
what if it is declared as |
A throws Ex1 | Ex2 desugars to A $throws Ex1 $throws Ex2 Which would effectively work like (using CanThrow[Ex1])(using CanThrow[Ex2]): A in signatures of methods, not like (using CanThrow[Ex1 | Ex2]): A |
Ah yes, that answers it. Thanks! So I think except for the other minor remarks, LGTM |
Reworks done. I also slightly improved the implicitNotFound message as I was often confused with the previous one. |
@odersky can we get this merged? |
Yes, LGTM. |
Fixes #13816