RFR: 8364991: Incorrect not-exhaustive error [v3]

[Jan Lahoda]

On Tue, 14 Oct 2025 00:17:38 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Adding tests with generic records.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 805:
> 
>> 803:             }
>> 804:             Set<PatternDescription> patterns = patternSet;
>> 805:             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:
https://github.com/openjdk/jdk/pull/27247/commits/51b7fc283e88ac5947cfebfce6609704c360e235

Thanks!

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1070:
> 
>> 1068:                                             if (!rpOne.nested[i].equals(rpOther.nested[i])) {
>> 1069:                                                 if (useHashes) {
>> 1070:                                                     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 (https://github.com/openjdk/jdk/pull/27247/commits/9138ef6bbba63787796bb964b29d273a3d6c76b9), and added some comments. Looks much better, thanks!

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1079:
> 
>> 1077:                                                 }
>> 1078:                                                 if (rpOne.nested[i] instanceof BindingPattern bpOne) {
>> 1079:                                                     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 (https://github.com/openjdk/jdk/pull/27247/commits/11ee4dfaa7ddef03ecdb410df5cf0f39857b12c9), let's see how that will work. Thanks!

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/27247#discussion_r2432572924
PR Review Comment: https://git.openjdk.org/jdk/pull/27247#discussion_r2432564345
PR Review Comment: https://git.openjdk.org/jdk/pull/27247#discussion_r2432566198