8347291: Exhaustive switch over a generic sealed abstract class

[cushon]

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]

👋 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]

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

[openjdk]

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

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

[cushon]

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]

Webrevs

• 01: Full - Incremental (db7305a8)
• 00: Full (d7029901)

[bridgekeeper]

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

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]

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]

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

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]

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

/touch

[openjdk]

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

[bridgekeeper]

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

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