8364991: Incorrect not-exhaustive error #27247

lahodaj · 2025-09-12T07:53:09Z

Consider this code:

$ cat Test.java
package test;
public class Test {
    private int test1(Root r) {
        return switch (r) {
            case Root(R2(R1 _), R2(R1 _)) -> 0;
            case Root(R2(R1 _), R2(R2 _)) -> 0;
            case Root(R2(R2 _), R2(R1 _)) -> 0;
            case Root(R2(R2 _), R2 _) -> 0;
        };
    }
    sealed interface Base {}
    record R1() implements Base {}
    record R2(Base b1) implements Base {}
    record Root(R2 b2, R2 b3) {}
}

javac (JDK 25) will produce a compile-time error for this code:

$ javac test/Test.java
.../test/Test.java:4: error: the switch expression does not cover all possible input values
        return switch (r) {
               ^
1 error

This error is not correct according to the JLS. JLS defines a set of possible reductions of pattern sets, and if there exists a series of reductions from the pattern set into a pattern set that covers the selector type, the switch is exhaustive.

One such reduction is that if there's a sub-set of (record) patterns that only differ in one component ("the mismatching component"), we can replace them with a (set of) patterns where this component is reduced, and the other components are unmodified.

Such path exists here (every line shows a set of patterns that is being transformed):

Root(R2(R1 _), R2(R1 _)), Root(R2(R1 _), R2(R2 _)), Root(R2(R2 _), R2(R1 _)), Root(R2(R2 _), R2 _)
=> choosing the second component as the mismatching component, then we can reduce Root(R2(R1 _), R2(R1 _)), Root(R2(R1 _), R2(R2 _)) => Root(R2(R1 _), R2 _); as we can reduce R2(R1 _), R2(R2 _) to R2 _
Root(R2(R1 _), R2 _), Root(R2(R2 _), R2(R1 _)), Root(R2(R2 _), R2 _)
=> choosing the first component as the mismatching component, we can reduce Root(R2(R1 _), R2 _), Root(R2(R2 _), R2 _) => Root(R2 _, R2 _)
Root(R2 _, R2 _)
=>
Root _
=>
exhaustive

The problem here is that in the first step, javac chooses this path:

Root(R2(R1 _), R2(R1 _)), Root(R2(R1 _), R2(R2 _)), Root(R2(R2 _), R2(R1 _)), Root(R2(R2 _), R2 _)
=> reduce Root(R2(R1 _), R2(R1 _)),  Root(R2(R2 _), R2(R1 _)) => Root(R2 _, R2(R1 _))
Root(R2 _, R2(R1 _)), Root(R2(R1 _), R2(R2 _)), Root(R2(R2 _), R2 _)
=> dead end, as there are no two patterns that would have the same nested pattern in the same component

If javac would do full backtracking, it could go back, and choose the other path, and find out the switch is exhaustive. But, full naive backtracking is, I think, prohibitively too slow for even relatively small switches. The implementation approach javac is using so far is that it does not remove some of the reduced patterns from the set. So, it can use the pattern again, and hence basically pick a different reduction path. But, it fails here, and if we would keep the relevant patterns here in the set, the overall performance would be too bad.

So, this PR proposes that, when reducing a sub-set of patterns to another set of patterns, javac keeps a record that the new pattern(s) originate in specific original pattern(s), and if it needs to, it will look into this record when searching for possible reductions. javac does "fast reduction rounds" normally using hashes, but if it fails to find reductions using the fast approach, it switches to a (much) slower approach that uses plain subtyping instead of hashes. The new approach to search for reductions proposed herein is part of this slow round only.

So, basically, the new chain after this PR is roughly:

Root(R2(R1 _), R2(R1 _)), Root(R2(R1 _), R2(R2 _)), Root(R2(R2 _), R2(R1 _)), Root(R2(R2 _), R2 _)
=> reduce Root(R2(R1 _), R2(R1 _)),  Root(R2(R2 _), R2(R1 _)) => Root(R2 _, R2(R1 _))
Root(R2 _, R2(R1 _)), Root(R2(R1 _), R2(R2 _)), Root(R2(R2 _), R2 _)
=> javac does not find anything it can reduce in the first stage, and will look at the original patterns. It will find out that Root(R2 _, R2(R1 _)) originates in Root(R2(R1 _), R2(R1 _)),  Root(R2(R2 _), R2(R1 _)), and that it could reduce (second component mismatching): Root(R2(R1 _), R2(R1 _)), Root(R2(R1 _), R2(R2 _)) => Root(R2(R1 _), R2 _)
Root(R2 _, R2(R1 _)), Root(R2(R1 _), R2 _), Root(R2(R2 _), R2 _)
=> as before, choosing the first component as the mismatching component, we can reduce Root(R2(R1 _), R2 _), Root(R2(R2 _), R2 _) => Root(R2 _, R2 _)
Root(R2 _, R2(R1 _)), Root(R2 _, R2 _)
=>
Root _
=>
exhaustive

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-8364991: Incorrect not-exhaustive error (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27247/head:pull/27247
$ git checkout pull/27247

Update a local copy of the PR:
$ git checkout pull/27247
$ git pull https://git.openjdk.org/jdk.git pull/27247/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27247

View PR using the GUI difftool:
$ git pr show -t 27247

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27247.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-09-12T07:55:16Z

👋 Welcome back jlahoda! 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 · 2025-09-12T07:56:19Z

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

openjdk · 2025-09-12T07:57:27Z

@lahodaj 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.

mlbridge · 2025-09-12T08:02:26Z

Webrevs

liach · 2025-09-29T15:15:39Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

+
+                                                        if (nestedRPOne.equals(currentReplaced)) {
+                                                            foundMatchingReplaced = true;
+                                                            break;


Can we just use continue ACCEPT;, and remove the foundMatchingReplaced variable?

test/langtools/tools/javac/patterns/Exhaustiveness.java

+               public class Test {
+                   private int test(Root r) {
+                       return switch (r) {
+                           case Root(R1 _, _, _) -> 0;


vicente-romero-oracle · 2025-10-13T15:02:25Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

-                                                    !(rpOther.nested[i] instanceof BindingPattern bpOther) ||
-                                                    !types.isSubtype(types.erasure(bpOne.type), types.erasure(bpOther.type))) {
+                                                if (useHashes) {
+                                                    continue NEXT_PATTERN;


I guess the code would be more readable if these labels and jumps could be (re)factored out. Another improvement in this sense could be using explicit types instead of var

I've moved this check into a separate method (9138ef6), and added some comments. Looks much better, thanks!

vicente-romero-oracle · 2025-10-13T17:11:00Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

+                                                    continue NEXT_PATTERN;
+                                                }
+                                                if (rpOne.nested[i] instanceof BindingPattern bpOne) {
+                                                    if (!types.isSubtype(types.erasure(bpOne.type), types.erasure(bpOther.type))) {


it seems from the description that the subtyping test is in the critical path when no hashes are being used. Would it make sense to use a cache and probably reduce the number of times we need to do the full fledged subtyping?

I've added caching (11ee4df), let's see how that will work. Thanks!

vicente-romero-oracle · 2025-10-14T00:17:38Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

                }
            }
            Set<PatternDescription> patterns = patternSet;
+            Map<PatternDescription, Set<PatternDescription>> replaces = new IdentityHashMap<>();


why using IdentityHashMap here? won't this imply, potentially, having duplicated info in this map? I'm sure there is a good reason for this but not clear to me at first glance. It seems like we need to preserve like the position of the element being replaced.

Also if the implementation depends on this DS we should probably document it.

Yes, the identity map is intentional. It is entirely possible two equivalent types are produced from two different originating pattern sets, and when backtracking, we want to use the correct set. That is achieve by using the identity search. I've added a comment:
51b7fc2

Thanks!

thanks for adding the comment!

to be honest, the problem I have with IdentityHashMap is that it has a lot of misses, at least that's that I saw while debugging some examples. What I saw was that in order to make sure that a given PatternDescription was not in the map, several keys in the table were visited. This somehow can kill the benefit of using a map as it can degenerate in some cases. So I wonder if it could be possible to define a key that takes into consideration the position of the JCRecordPattern we got the record pattern from, in order to make them "unique". Of course this is in the performance side and could be done in a follow-up patch

vicente-romero-oracle · 2025-10-15T16:40:31Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

            alive = alive.or(resolveYields(tree, prevPendingExits));
        }

+        private final Map<Pair<Type, Type>, Boolean> isSubtypeCache = new HashMap<>();


I think that you want to compare a type using Types::isSameType instead of Type::equals method. There is one cache that does this: Infer::incorporationCache. Although it could be that identity comparison is enough for this application, dunno.

EDIT: But I can see that this current definition should be faster than doing Types::isSameType, yep scratch the above. I think it is better the way you defined it. In type inference we do the isSameType to derive more constraints which is not applicable / necessary here. Also if the Pair stores the erased types instead of the unerased types, then we have the equivalent to comparing two types using Types::isSameType as erased types tend to be unique

vicente-romero-oracle · 2025-10-15T16:50:52Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

                }
            }
            Set<PatternDescription> patterns = patternSet;
+            Map<PatternDescription, Set<PatternDescription>> replaces = new IdentityHashMap<>();


thanks for adding the comment!

vicente-romero-oracle · 2025-10-15T16:53:47Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

+         * - it was produced by a reduction from a record pattern that is equivalent to
+         *   the existing pattern
+         */
+        private boolean nestedComponentsEquivalent(RecordPattern existing,


yep looks better now, thanks!

vicente-romero-oracle · 2025-10-15T17:13:38Z

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java

                }
            }
            Set<PatternDescription> patterns = patternSet;
+            Map<PatternDescription, Set<PatternDescription>> replaces = new IdentityHashMap<>();


to be honest, the problem I have with IdentityHashMap is that it has a lot of misses, at least that's that I saw while debugging some examples. What I saw was that in order to make sure that a given PatternDescription was not in the map, several keys in the table were visited. This somehow can kill the benefit of using a map as it can degenerate in some cases. So I wonder if it could be possible to define a key that takes into consideration the position of the JCRecordPattern we got the record pattern from, in order to make them "unique". Of course this is in the performance side and could be done in a follow-up patch

lahodaj added 5 commits August 7, 2025 18:27

8364991: Incorrect not-exhaustive error

2cfbc71

Improving test debuggability.

e4f655d

Merge branch 'master' into JDK-8364991

74c72e6

Adjusting to spec.

8735a3d

Adding test.

bed37c4

openjdk bot added compiler compiler-dev@openjdk.org rfr Pull request is ready for review labels Sep 12, 2025

openjdk bot mentioned this pull request Sep 12, 2025

8367499: Refactor exhaustiveness computation from Flow into a separate class #27253

Open

3 tasks

liach reviewed Sep 29, 2025

View reviewed changes

Simplifying the code as suggested.

0ee6086

vicente-romero-oracle reviewed Oct 11, 2025

View reviewed changes

test/langtools/tools/javac/patterns/Exhaustiveness.java

public class Test {

private int test(Root r) {

return switch (r) {

case Root(R1 _, _, _) -> 0;

This comment was marked as resolved.

Sign in to view

Adding tests with generic records.

026dc4c

vicente-romero-oracle reviewed Oct 13, 2025

View reviewed changes

vicente-romero-oracle reviewed Oct 14, 2025

View reviewed changes

lahodaj added 3 commits October 15, 2025 12:23

Factoring out the 'substitutable' check, as suggested.

9138ef6

Adding explanation to the replaces map.

51b7fc2

Caching isSubtype, as suggested.

11ee4df

vicente-romero-oracle reviewed Oct 15, 2025

View reviewed changes

8364991: Incorrect not-exhaustive error #27247

Are you sure you want to change the base?

8364991: Incorrect not-exhaustive error #27247

Conversation

lahodaj commented Sep 12, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Sep 12, 2025

Uh oh!

openjdk bot commented Sep 12, 2025

Uh oh!

openjdk bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahodaj Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicente-romero-oracle Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

lahodaj commented Sep 12, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 12, 2025 •

edited

Loading

mlbridge bot commented Sep 12, 2025 •

edited

Loading

lahodaj Oct 15, 2025 •

edited

Loading

vicente-romero-oracle Oct 14, 2025 •

edited

Loading

vicente-romero-oracle Oct 15, 2025 •

edited

Loading