uber / NullAway

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

Consider support of field nullability information in stream closures #570

Open lazaroclapp opened 2 years ago

lazaroclapp commented 2 years ago

The following code is very natural to write, with unexpected nullability implications:

import java.util.stream.Stream;
import javax.annotation.Nullable;

public class Test {
  @Nullable String prefix = null;
  public void test(Stream<String> suffixes) {
     if(prefix == null) {
         this.prefix = \"Printing: \";
     }
     suffixes.forEach(s -> System.out.println(prefix.concat(s))); // null deref here
  }
}

In the example above, the developer checks whether prefix is null (and sets it to a non-null value if so) before passing a lambda that dereferences it to a stream. However, because NullAway is strictly intra-procedural with respect to fields, it will not propagate the information about prefix being non-null within Test.test into the nullness store of the lambda being passed to Stream.forEach(...).

Handling this in the general case is not possible: when NullAway sees a lambda being passed around with a nullable field in its closure, it has no way to know when the lambda will be executed, and thus whether or not the state/value of that field will have changed between closure creation and the execution of the body of the lambda.

At the same time, the code above is very natural for developers to write and we do reason about inter-procedural nullability and lambda's in other cases involving streams (our usual .filter(...).map(...) handling, for example). I wonder if it might be worth it to special case some stream operations that we know execute eagerly and propagate field nullability information into them.

msridhar commented 2 years ago

Agreed that if it comes up often enough, probably the stream nullability handler is where to add reasoning about this case.