-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Dealias in isSimpleThrowable check #24744
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
base: main
Are you sure you want to change the base?
Conversation
|
@hamzaremmal Maybe you could review since you left a comment on the issue. Reading the doc for |
|
Hi @eejbyfeldt, the diff seems small but I want to double check things for a spec point of view before approving. I'll add it into my todos of next week. |
|
I think this PR has the correct behaviour, given we consider the I'm wondering, can we generate byte code for simple catch like |
|
I remember that try-catch doesn't support I have a local test about |
I'm curious, can I take a look of the test? |
|
@noti0na1 Sorry I have no idea why I wrote this. Probably just noodling. |
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.
Hi @eejbyfeldt, I've just taken a look at the spec and played a bit with some examples and this is indeed the right fix for this issue 👍🏻. Nevertheless, the test could be more robust and should highlight the fact that we should not match in the E1or2 case since Exception <: E1 and Exception <: E2 are false.
I will also suggest adding a test that shows when we throw E1 and E2 separately, we will have the right semantics.
| case e: E1or2 => assert(e.isInstanceOf[E1or2]) | ||
| case _ => () |
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.
This makes the test more robust.
| case e: E1or2 => assert(e.isInstanceOf[E1or2]) | |
| case _ => () | |
| case e: E1or2 => assert(false) | |
| case _ => () |
fixes: #24357