uber / NullAway

A tool to help eliminate NullPointerExceptions (NPEs) in your Java code with low build-time overhead
MIT License
3.64k stars 295 forks source link

Respect annotated AtomicReference's #681

Open lycarter opened 2 years ago

lycarter commented 2 years ago

In this example code, I've annotated my AtomicReference to show that it can never be null. However, NullAway still has a warning [NullAway] returning @Nullable expression from method with @NonNull return type. I believe this stems from https://github.com/uber/NullAway/blob/master/nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java#L618, which (based on my quick skim of the code) looks like it pre-annotates the AtomicReference::get method with @Nullable, even when that isn't necessarily true.

Could NullAway consider supporting this better, or is there a good workaround here that I might be missing?

import java.util.concurrent.atomic.AtomicReference;
import org.checkerframework.checker.nullness.qual.NonNull;

public final class MyClass {

    private final AtomicReference<@NonNull String> ref;

    MyClass() {
        ref = new AtomicReference<>("starting value");
    }

    public String getCurrentValue() {
        return ref.get(); // returning @Nullable expression from method with @NonNull return type
    }

    public void updateValue(@NonNull String newValue) {
        ref.set(newValue);
    }
}

Using nullaway 0.9.9 via https://github.com/palantir/gradle-baseline (cc @carterkozak @schlosna )

msridhar commented 2 years ago

Thanks for the report. This is related to #660 and generics support, which we are actively working on (/cc @NikitaAware). Our first milestone here would be an ability to opt in to this kind of checking, with limited to no support for inference of generic types. I believe we could handle an example like the one you give in that first milestone; we will aim for it. I'll comment back here when we have something worth testing.

As an aside, @NonNull is the default with NullAway, so once our milestone is working, the above example should type check without any explicit annotations. If you wanted to be able to store null into an AtomicReference, then you would need to declare the type of the field as AtomicReference<@Nullable String>.

lycarter commented 2 years ago

That all makes sense - thanks!