-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8347291: Exhaustive switch over a generic sealed abstract class #23286
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: master
Are you sure you want to change the base?
Conversation
👋 Welcome back cushon! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
FWIW: there was this attempt: |
I think this is worth considering - the code is already handling an |
Webrevs
|
@cushon This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Any thoughts on this? To me false positives in exhaustiveness checking seem a lot worse than false negatives, and the principled fix to inference to avoid the crash seems like it's going to be more challenging. I think in the short term there's a argument for making this change, so in the rare cases where this comes up the switches need an explicit But if this is too hacky, and there's a preference for leaving this alone until there's time to fix the inference crash, that's fine and I can drop the PR. |
IIRC, I was looking into this when this came up. I think when I was debugging, there seemed to be incorrectly sorted bounds sent to intersection type, which then broke the rest of the inference machinery. I've had this: But I think, at that time, the idea was to take a better look at the (pattern) inference as a whole. @mcimadamore - any insights? Thanks! |
@cushon This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
Thanks! My feeling here is still that this is complementary to inference fixes, as long as there's a If it's possible to ensure inference never throws an exception that needs to be caught that would also be good, and avoids this. |
@cushon This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
/touch |
@cushon The pull request is being re-evaluated and the inactivity timeout has been reset. |
@cushon This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
I still think this is an improvement over the status quo, I am also fine dropping this if there's a preference for sticking with the status quo until it's possible to fix the inference crash and avoid the |
This change avoids javac incorrectly reporting switches like the example in JDK-8347291 as exhaustive.
As Jan noted in the bug it seems like the inference behaviour here may not be correct. If it was able to infer a type without crashing that would also avoid the bug.
However with the current inference behaviour, it's safer to assume that if inference crashes the subtype may need to be handled by the switch, instead of assuming that it doesn't.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23286/head:pull/23286
$ git checkout pull/23286
Update a local copy of the PR:
$ git checkout pull/23286
$ git pull https://git.openjdk.org/jdk.git pull/23286/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23286
View PR using the GUI difftool:
$ git pr show -t 23286
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23286.diff
Using Webrev
Link to Webrev Comment