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

[Vicente Romero]

On Fri, 14 Jul 2023 00:44:55 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Vicente Romero has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - addressing additional review comments
>>  - dont generate warnings for literals or constructor invocations
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 5407:
> 
>> 5405:     public boolean hasSameNullability(Type t, Type s) {
>> 5406:         // special case for literals, a literal is always != null
>> 5407:         boolean isLiteral = s != null && s.getMetadata(TypeMetadata.ConstantValue.class) != null;
> 
> I believe this should be addressed at the level of the `isNullUnspecified` predicates. E.g. if something is a constant type, then you know it's null-restricted (e.g. because we infer it so). Once that's dealt with, I believe these higher-level methods here can stay unchanged?

yes I did that experiment first but then I got to some unexpected issues. For example: `1L` now would be consider to have type `long!` which doesn't make too much sense, we could exclude primitives from the predicates I guess but there are still some other places where we can have surprises. I found this example:


import java.lang.invoke.*;
class Cell {
    final void reset() {
        /* we are testing that the compiler won't infer the arguments of
         * VarHandle::setVolatile as (Cell, String!)
         */
        VALUE.setVolatile(this, "");
    }
    final void reset(String identity) {
        /* if that were the case, see comment above, then this invocation would generate
         * a warning, VarHandle::setVolatile is a polymorphic signature method
         */
        VALUE.setVolatile(this, identity);
    }
                                                                
    private static final VarHandle VALUE;
    static {
        try {
            MethodHandles.Lookup l = MethodHandles.lookup();
            VALUE = l.findVarHandle(Cell.class, "value", long.class);
        } catch (ReflectiveOperationException e) {
            throw new ExceptionInInitializerError(e);
        }
    }
}

VarHandle::setVolatile is a polymorphic signature method. So from the first invocation the compiler would infer argument types: `(Cell, String!)` is this what we want? For the second invocation we find that we already have a method that matches but the invocation will generate a warning if compiled with `Xlint:null`, not sure we want this. This is why I preferred to stay with the declared type for literals and tread them as a special case. We can still do what you suggest and do something different when we infer the signature of polymorphic sig methods, like remove null restrictions from the inferred type.

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

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