Anonymous Inner Classes - NullAway checks not working

[garg-ketki]

• Problem Statement NullAway checks not working for the below scenario with anonymous inner classes. Please find the code snippet for reference:

• Code Snippet:

  class Helper{
  @Nullable
  public static <T> codeThatCanReturnNull() {
  }
  }

  class Foo{
  new Bar(() -> codeThatCanReturnNull())
  }

  class Bar{
  private final Supplier<Abc> abcSupplier;
  Bar(Supplier<Abc> abcSupplier){
     this.abcSupplier = abcSupplier;
  }
  
  private void callSupplier() {
  final Abc abcProvider = abcSupplier.get();
  return abcProvider.method(); // can throw NullPointerException
  }
  }

  class Abc{
  public void method(){}
  }

  package com.google.common.base;
  import com.google.common.annotations.GwtCompatible;
  import com.google.errorprone.annotations.CanIgnoreReturnValue;
  @FunctionalInterface
  @GwtCompatible
  public interface Supplier<T> extends java.util.function.Supplier<T> {
  @CanIgnoreReturnValue
  T get();
  }

[msridhar]

I think the issue here is that the java.util.function and com.google.common.base packages are treated as unannotated by default. This means NullAway is maximally permissive with calls to methods in those packages; it assumes the return is @NonNull and all parameters are @Nullable. To properly handle these functional interfaces requires support for nullability annotations on generic types, but NullAway does not support that yet (see some discussion on #54).

As a workaround, you could choose to add com.google.common.base as an annotated package. But this will mean you will never be able to write a Supplier that can return null.

[imoverclocked]

Could you subclass Supplier and annotate the subclass' return value with @Nonnull?

eg:

interface NonNullSupplier<T> extends Supplier<T> {
    @Nonnull T get();
}

[lazaroclapp]

Could you subclass Supplier and annotate the subclass' return value with @Nonnull?

eg:

interface NonNullSupplier<T> extends Supplier<T> {
    @Nonnull T get();
}

Do you mean in addition to either making com.google.common.base an annotated package or making a custom library model for Supplier that makes get() return @Nullable T?

If so, first, you wouldn't need @NonNull, NullAway assumes first-party code is non-null by default.

Then the issue is that you would need to change the argument to being NonNullSupplier specifically, wherever you want to accept a non-null supplier. Otherwise, the functional interface for the lambda expression will be resolved to Supplier, not a subtype. That said, assuming you can do this, then, yes, that would be one way of having both @Nullable and non-null versions of Supplier.

[imoverclocked]

If we were to correctly assume that get() from java.util.function.Supplier is nullable then the NonNullSupplier interface makes sense.

If we incorrectly assume that get() is Nonnull then we can't create a sub-interface with an @Nullable T get(); because of interface nullability constraints.

ie: NullAway should have a built-in model for java.util.function.Supplier's get() to be @Nullable

This works under NullAway today and is a NullAway bug:

    private Supplier<Integer> getIntSupplier() {
        return () -> { return null; };
    }

This does not pass under NullAway today, and currently does the right thing:

    private NonNullSupplier<Integer> getIntSupplier() {
        return () -> { return null; }; // error: [NullAway] returning @Nullable expression from method with @NonNull return type
    }

[msridhar]

This works under NullAway today and is a NullAway bug:

    private Supplier<Integer> getIntSupplier() {
        return () -> { return null; };
    }

Yes, this is a known soundness hole, which happens anywhere we have code that is treated as unannotated.

This does not work under NullAway today, and is already correct:

    private NonNullSupplier<Integer> getIntSupplier() {
        return () -> { return null; }; // error: [NullAway] returning @Nullable expression from method with @NonNull return type
    }

This I don't understand. If you have a NonNullSupplier why would you want to allow an implementation to return null?

In any case, bottom line this comes down to support for generics which we don't have yet. I think the best options for now are to either treat java.util.function as an annotated package, in which case you cannot return null from any java.util.function.Supplier, or write your own implementations NonNullSupplier and NullableSupplier unrelated to java.util.function.Supplier. Neither solution is ideal, unfortunately; the first may be more practical.

[imoverclocked]

This I don't understand. If you have a NonNullSupplier why would you want to allow an implementation to return null?

You want NullAway to mark it as an error. That's why this isn't a bug today (ie: it's currently correct behavior.) I'll update my wording to be more clear.

---

I'm not sure what this has to do with generics? Do you mean we can't create a model around interfaces?

Something like:

    public ImmutableSet<MethodRef> nullableReturns() {
        return ImmutableSet.of(
                methodRef("java.util.function.Supplier", "get()")
        );
    }

[msridhar]

Sorry I should be clearer. The thing we need here to do this right is to be able to write type qualifiers on generic type parameters. Then you could declare types like Supplier<@NonNull Integer> or Supplier<@Nullable Integer> as appropriate for the particular supplier. You can see how Checker Framework handles this stuff here; it gets pretty involved. We haven't had time to support it yet.

And by the way, if you really require a more sound handling of nullability, you can give the Checker Framework a shot. The main drawback compared to NullAway is it runs slower. Also, since it does more careful checking, you will likely need to write more annotations to get your code to pass the checker.

[lazaroclapp]

@msridhar The ask is around making Supplier.get() default to returning @Nullable, rather than @NonNull, under the current NullAway type system. This is certainly sounder, given our current limitations, but at the risk of far more false positives in the cases where the client code meant what a more versatile type system would express as Supplier<@NonNull T>.

@imoverclocked : I am on the fence on requiring people to define a first-party NonNullSupplier class to get the behavior of Supplier<@NonNull T>, which is what your proposed addition to the default models would imply. Even if the tool as a whole might be sounder for it, that seems to me like making the common case harder.

I don't at all disagree it would be "more correct" to have that model, but NullAway does take the stance, in some other cases, of preferring what is practical to what is perfectly sound (e.g. our model for Android's findViewById(...) is purposefully "wrong" in returning non-null, since that method can return null, but only in cases where its arguments are not proper view ids, and most passed view ids are constants from R.view.x).

Let me try the model you propose in some of our larger codebases and get an idea how likely it's to cause problems for users to make Supplier.get() return @Nullable.

In the meanwhile, if you want NullAway to work that way right now, for your use case, one way to do so is to use a custom library models class and wire it with @AutoService as we do here. That's an easier alternative than just saying "migrate to the checker framework" (although, if you generally want something that will nearly always go the sounder route, at the possible expense of other concerns, that remains a good option!).

I imagine that's not the most satisfying answer to you, particularly if you see the missing model for Supplier as a bug, but without generics support, we do have to think a bit about what would be more practical to do here on the default models. Again, happy to look into that too, I might just need a few cycles to do the change, test internally and then make a decision based on how many fixes that requires for us and hope that translates well to other's codebases :) )

[msridhar]

The custom library model solution sounds like a good one! I'd only want to change the default handling of Supplier if there is evidence that many / most Supplier implementations can actually return null. I would be surprised if that were true. As @lazaroclapp notes, if most Suppliers cannot return null, changing the default treatment could lead to a lot of noise in NullAway results.

[imoverclocked]

We use NullAway as a gate in PR checks and I noticed someone adding a return null; to a Supplier<> lambda which is why I started looking into it. I certainly appreciate going for the most appropriate solution and I understand why adding this model requires careful thought. Thanks for the discussion and all of the great work done in this project!