RFR: 8310905: [lw5] addressing review comments on null restricted types [v4]

[Vicente Romero]

On Wed, 12 Jul 2023 09:40:11 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   adding more regression tests
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 1284:
> 
>> 1282:             NullabilityComparator reset(BiFunction<Type, Type, Boolean> differentNullability) {
>> 1283:                 this.differentNullability = differentNullability;
>> 1284:                 if (this.warner == null || this.warner == noWarnings) {
> 
> I'm not sure I follow what's happening here. We have a warner stack in Types already. But this class seems to somehow override whatever warner is in the stack (if a client called setWarner on the nullability comparator). Shouldn't we try to reuse the warnStack as a way to set warners, and then tweak nullability comparator to use whatever warner is on the stack? E.g. I'm suspicious about the set/clearWarner methods, as they seem to duplicate functionality we already have in other forms.

I did this because `Check::checkExtends` invokes `Types::isSubtype`, in a previous iteration I think you suggested not to add a Warner argument to `Types::isSubtype`, so this is an effort to circumvent that. I will give it another try

> test/langtools/tools/javac/bang/BangTypesCompilationTests.java line 359:
> 
>> 357:                                 Result.Warning,
>> 358:                                 "compiler.warn.unchecked.nullness.conversion",
>> 359:                                 2),  // this needs to be reviewed, the warning is printed twice, should be once only
> 
> Does this have to do with the fact that the subtyping is executed twice (e.g. once in overload, once in the "check" phase) ?

yep I think so

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/880#discussion_r1261614450
PR Review Comment: https://git.openjdk.org/valhalla/pull/880#discussion_r1261615802