checker-framework icon indicating copy to clipboard operation
checker-framework copied to clipboard

On upgrade to 3.47.0 supplier seems to get inferred as nullable

Open agentgt opened this issue 1 year ago • 3 comments

The following code the supplier is inferred as @Nullable when it should be @NonNull.

	@SuppressWarnings("unchecked")
	@Override
	public <T> T putIfAbsent(Class<T> type, String name, Supplier<T> supplier) {
		var t = (T) services.computeIfAbsent(new ServiceKey(type, name), k -> Objects.requireNonNull(supplier.get()));
		return t;
	}

https://github.com/jstachio/rainbowgum/blob/c8bfceaef5e07c75c76b1d59f59ef5ba4288b8b6/core/src/main/java/io/jstach/rainbowgum/ServiceRegistry.java#L168

I will try to add an isolated reproducible example so that you don't need my entire project.

Reproducible is here:

https://github.com/agentgt/checker-issues/tree/main/checker-issue-6789

[ERROR] /Users/agent/projects/checker-issues/checker-issue-6789/src/main/java/io/jstach/checker/issue6689/ServiceRegistry.java:[165,107] error: [argument] incompatible argument for parameter obj of Objects.requireNonNull.
[ERROR]   found   : T extends @Initialized @Nullable Object
[ERROR]   required: T extends @Initialized @NonNull Object

agentgt avatar Sep 05 '24 13:09 agentgt

Here's a relatively small test case that reproduces the problem (automatically produced by Specimin):

package io.jstach.rainbowgum;

import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.function.BiConsumer;
import java.util.function.Supplier;

public sealed interface ServiceRegistry extends AutoCloseable permits DefaultServiceRegistry {

    public default <T> T putIfAbsent(Class<T> type, String name, Supplier<T> supplier) {
        throw new Error();
    }

    public default void close() {
        throw new Error();
    }
}

record ServiceKey(Class<?> type, String name) {

    ServiceKey {
        Objects.requireNonNull(type);
        Objects.requireNonNull(name);
    }
}

final class DefaultServiceRegistry implements ServiceRegistry {

    private final Map<ServiceKey, Object> services = null;

    public <T> void put(Class<T> type, String name, T service) {
        throw new Error();
    }

    @SuppressWarnings("unchecked")
    public <T> T findOrNull(Class<T> type, String name) {
        throw new Error();
    }

    @SuppressWarnings("unchecked")
    public <T> List<T> find(Class<T> type) {
        throw new Error();
    }

    @SuppressWarnings("unchecked")
    public <T> void forEach(Class<T> type, BiConsumer<String, T> consumer) {
        throw new Error();
    }

    @SuppressWarnings("unchecked")
    public <T> T putIfAbsent(Class<T> type, String name, Supplier<T> supplier) {
        var t = (T) services.computeIfAbsent(new ServiceKey(type, name), k -> Objects.requireNonNull(supplier.get()));
        return t;
    }

    public void onClose(AutoCloseable closeable) {
        throw new Error();
    }

    public void close() {
        throw new Error();
    }
}

Running the Nullness Checker from CF 3.47.0 on that code produces:

(base) ➜  rainbowgum-out $CHECKERFRAMEWORK/checker/bin/javac -processor nullness **/*.java
io/jstach/rainbowgum/ServiceRegistry.java:30: error: [assignment] incompatible types in assignment.
    private final Map<ServiceKey, Object> services = null;
                                                     ^
  found   : null (NullType)
  required: @Initialized @NonNull Map<@Initialized @NonNull ServiceKey, @Initialized @NonNull Object>
io/jstach/rainbowgum/ServiceRegistry.java:53: error: [argument] incompatible argument for parameter obj of Objects.requireNonNull.
        var t = (T) services.computeIfAbsent(new ServiceKey(type, name), k -> Objects.requireNonNull(supplier.get()));
                                                                                                                 ^
  found   : T extends @Initialized @Nullable Object
  required: T extends @Initialized @NonNull Object
2 errors

kelloggm avatar Sep 05 '24 13:09 kelloggm

Oh, I didn't see that the reporter had added a reproducer. @agentgt beat me to it by a few minutes :)

kelloggm avatar Sep 05 '24 13:09 kelloggm

@kelloggm That specimin tool looks cool! Starred it for future exploring. Thanks for the link to it!

agentgt avatar Sep 05 '24 13:09 agentgt

This is a true positive. putIfAbsent will throw a NPE if the provided supplier returns a null object. To fix the error, annotate putIfAbsent in ServiceRegistry like so:

public <T> T putIfAbsent(Class<T> type, String name, Supplier<@NonNull T> supplier) {

smillst avatar Mar 26 '25 17:03 smillst

@smillst

Sorry for the late followup. I think I get this. I need the extra annotation because Supplier is not my own class and is actually defined Supplier<T extends @Nullable Object> and not interface Supplier<T> in NullMarked (and thus would implicitly be Supplier<T extends @NonNull Object>).

The thing is I have seen other tools including Eclipse and IntelliJ (https://github.com/jspecify/jspecify/issues/762) report that:

// Assume we are in NullMarked
<T> T putIfAbsent(Class<T> type, String name, Supplier<@NonNull T> supplier)
                                                      //^^^ warning here as redundant

I don't think checker reports it as a warning. The issue is the declaration site of some method using a type seem to want to infer that Supplier<T> is Supplier<@NonNull T> in @NullMarked code when in fact the class it matters more how the class is declared. Is that correct?

agentgt avatar Sep 08 '25 14:09 agentgt