RFR: 8343306: javac is failing to determine if a class and a sealed interface are disjoint [v4]

[Jan Lahoda]

On Thu, 31 Oct 2024 19:31:09 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> For code like:
>> 
>> 
>> class Test {
>>     sealed interface I permits C1 {}
>>     non-sealed class C1 implements I {}
>>     class C2 extends C1 {}
>>     class C3 {}
>>     I m(int s, C3 c3) {
>>         I i = (I)c3;
>>     }
>> }
>> 
>> javac is failing to issue an error and accepts this code. The spec is clear stating that code like this should be rejected. See:
>> 
>> 5.1.6.1 Allowed Narrowing Reference Conversion:
>> 
>> • A class named C is disjoint from an interface named I if (i) it is not the case that
>>   C <: I , and (ii) one of the following cases applies:
>>   – C is freely extensible (§8.1.1.2), and I is sealed , and C is disjoint from all of
>>   the permitted direct subclasses and subinterfaces of I .
>> 
>> and just below it continues:
>> • A class named C is disjoint from another class named D if (i) it is not the case
>> that C <: D , and (ii) it is not the case that D <: C .
>> 
>> so here we have the `C3` is a freely extensible class and interface `I` is sealed and `C3` is disjoint from `C1` which is is the permitted subclass of interface `I`
>> 
>> This PR should sync javac with the spec
>> 
>> TIA
>
> Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
> 
>   addressing review comments

Looks good to me. A few comments for consideration inline - these are up to you.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 1689:

> 1687:                     return false;
> 1688: 
> 1689:                 if (ts.isInterface() != ss.isInterface()) { // case I: one is a class and the other one is an interface

Only for consideration: the specification does not have 3 cases, but 4, when the second asks the "areDisjoint" question with swapped arguments. The code here is equivalent to that, but might be easier to just re-do what the spec is doing, having handle the case where `ts` is a class, `ss` is an interface, and then have something along the lines:

} else if (ts.isInterface() && !ss.isInterface()) {
     return areDisjoint(ss, ts);
}

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 1697:

> 1695:                         } else if (csym.isSealed()) {
> 1696:                             return areDisjoint(isym, csym.getPermittedSubclasses());
> 1697:                         } else if (!csym.isSealed() && isym.isSealed()) {

Nit: the `!csym.isSealed()` seems superfluous here. We know `csym.isSealed()` returns `false` (otherwise we would not get here). Hence `!csym.isSealed()` is always `true`.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 1701:

> 1699:                         }
> 1700:                     }
> 1701:                 } else if (!ts.isInterface() &&              // case II: both are classes

Nit: it is not necessary to check both `ts` and `ss` (although it is not wrong). If one of them is a class, both are. For your consideration.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 1713:

> 1711:                     }
> 1712:                 }
> 1713:                 // at this point we haven't been able to prove that the classes or interfaces are disjoint so we bail out

Nit: I would drop the "so we bail out". javac is not giving up - it just checked what JLS says should be checked, and can't prove the types are disjoint.

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

Marked as reviewed by jlahoda (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21794#pullrequestreview-2410098024
PR Review Comment: https://git.openjdk.org/jdk/pull/21794#discussion_r1825835099
PR Review Comment: https://git.openjdk.org/jdk/pull/21794#discussion_r1825836054
PR Review Comment: https://git.openjdk.org/jdk/pull/21794#discussion_r1825837557
PR Review Comment: https://git.openjdk.org/jdk/pull/21794#discussion_r1825841400