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

[Maurizio Cimadamore]

On Wed, 5 Jul 2023 21:20:45 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> addressing some review comments related to PR [1]
>> 
>> [1] https://github.com/openjdk/valhalla/pull/872
>
> Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
> 
>   adding more regression tests

Looks good - I left some suggestions for improvements and consolidation.

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.

test/langtools/tools/javac/bang/BangTypesCompilationTests.java line 150:

> 148:                                         Supplier<? extends T> factory = nullFactory();
> 149:                                     }
> 150:                                     Supplier<? extends T!> nullFactory() { return () -> null; }

Nice!

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) ?

test/langtools/tools/javac/bang/BangTypesCompilationTests.java line 489:

> 487:                                 class Box<T> {}
> 488:                                 class Test {
> 489:                                     void m(Box<Shape!> lp) {

This test reminds me of something: capture conversion should probably be tweaked to take nullability into account. Consider this:


class Foo<X extends Bar!> { 
    X x() { ... }
}

Foo<? extends Object> foo = ...
Object! restricted = foo.x(); // this should be ok  (e.g. no warnings)


I'm basing this on the fact that capture conversion already tries to establish some common facts between declaration-site and use-site. This test is similar to when use-site says `<? extends Object>` but the decl site says `<X extends List>`, in which case `List` is used as the bound for the captured variable. In this case, it seems like the null restriction that is present at the declaration site should be carried over to the captured variable as well.  Of course this is all outside the context of this review, but I thought I'd leave a comment to note the issue.

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

PR Review: https://git.openjdk.org/valhalla/pull/880#pullrequestreview-1525928001
PR Review Comment: https://git.openjdk.org/valhalla/pull/880#discussion_r1260908537
PR Review Comment: https://git.openjdk.org/valhalla/pull/880#discussion_r1260910636
PR Review Comment: https://git.openjdk.org/valhalla/pull/880#discussion_r1260912605
PR Review Comment: https://git.openjdk.org/valhalla/pull/880#discussion_r1260920045