Skip to content

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cushon
Copy link
Contributor

@cushon cushon commented Jan 24, 2025

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

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8347291: Exhaustive switch over a generic sealed abstract class (Bug - P3)

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 24, 2025

👋 Welcome back cushon! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Jan 24, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk
Copy link

openjdk bot commented Jan 24, 2025

@cushon The following label will be automatically applied to this pull request:

  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@lahodaj
Copy link
Contributor

lahodaj commented Jan 24, 2025

FWIW: there was this attempt:
lahodaj@108e5af
but it requires more discussion on what exactly should the inference do.

@cushon cushon marked this pull request as ready for review March 12, 2025 22:13
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 12, 2025
@cushon
Copy link
Contributor Author

cushon commented Mar 12, 2025

I think this is worth considering - the code is already handling an InferenceException and working around it in a way that leads to a MatchException at runtime. Until it's possible to improve the underlying inference behaviour to not fail, it seems safer to treat types where inference fails conservatively.

@mlbridge
Copy link

mlbridge bot commented Mar 12, 2025

Webrevs

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 10, 2025

@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!

@cushon
Copy link
Contributor Author

cushon commented Apr 10, 2025

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 default: instead of getting an unexpected MatchException.

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.

@lahodaj
Copy link
Contributor

lahodaj commented Apr 10, 2025

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:
lahodaj@108e5af

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!

@bridgekeeper
Copy link

bridgekeeper bot commented May 8, 2025

@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 or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@cushon
Copy link
Contributor Author

cushon commented May 8, 2025

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: lahodaj@108e5af

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!

Thanks! My feeling here is still that this is complementary to inference fixes, as long as there's a } catch (Infer.InferenceException ex) { I think it's worth considering treating that case conservatively, i.e. not assuming a switch is exhaustive if inference throws an exception.

If it's possible to ensure inference never throws an exception that needs to be caught that would also be good, and avoids this.

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 5, 2025

@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 or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@cushon
Copy link
Contributor Author

cushon commented Jun 5, 2025

/touch

@openjdk
Copy link

openjdk bot commented Jun 5, 2025

@cushon The pull request is being re-evaluated and the inactivity timeout has been reset.

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 4, 2025

@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 or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@cushon
Copy link
Contributor Author

cushon commented Jul 7, 2025

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 InferenceException entirely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler [email protected] rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

2 participants