Optional emptiness check in Rx `map` does not work with method reference

[shubhamugare]

Optional emptiness check in Rx map does not work with method reference:

private static boolean perhaps() { return Math.random() > 0.5; }",
 void foo(Observable<Optional<String>> observable) {
              observable
                  .filter(optional -> optional.isPresent() || perhaps())
                  // should throw an error here, since optional can be empty 
                  .map(Optional::get);
     }

[shubhamugare]

It is because the annotation for Optional::get call is Non-null we assume that the overriding is okay.

  if (overriddenMethodReturnsNonNull
        && Nullness.hasNullableAnnotation(overridingMethod)
        && getComputedNullness(memberReferenceTree).equals(Nullness.NULLABLE)) {
       //throw error
   }

In case of method invocation the handler decision overrides the the annotation decision and we decide to use the data flow results.

    if (!Nullness.hasNullableAnnotation(exprSymbol)) {
      exprMayBeNull = false;
    }
    exprMayBeNull = handler.onOverrideMayBeNullExpr(this, expr, state, exprMayBeNull);
    return exprMayBeNull ? nullnessFromDataflow(state, expr) : false;

So something can be changed in former to make this work, thoughts? @msridhar @lazaroclapp

[msridhar]

It is because the annotation for Optional::get call is Non-null we assume that the overriding is okay.

  if (overriddenMethodReturnsNonNull
        && Nullness.hasNullableAnnotation(overridingMethod)
        && getComputedNullness(memberReferenceTree).equals(Nullness.NULLABLE)) {
       //throw error
   }

So is the issue here that Nullness.hasNullableAnnotation(overridingMethod) returns false? Maybe we need to modify that conjunct to invoke the handler somehow?

[shubhamugare]

So is the issue here that Nullness.hasNullableAnnotation(overridingMethod) returns false?

Yes.

Maybe we need to modify that conjunct to invoke the handler somehow?

Yeah, that will be a little hacky solution again. The approach of making optionalFoo.get() as nullable for potentially empty Optional is itself hacky and causing these problems. Maybe in the future, we can just go for some cleaner solution with EMPTY_POSSIBLE enum for such Optionals. What do you think?

[msridhar]

Random idea. What if when Optional checking is enabled, we add a library model making Optional.get() return @Nullable, rather than trying to override the nullability in the Optional handler. I think that should fix things, yes? In this case, we already have logic in the Rx handler for updating a computed nullness map for method references that should work just fine:

https://github.com/uber/NullAway/blob/29dd90ca53172edf38768fb6205c94cbec3bf992/nullaway/src/main/java/com/uber/nullaway/handlers/RxNullabilityPropagator.java#L411-L414

[shubhamugare]

Random idea. What if when Optional checking is enabled, we add a library model making Optional.get() return @Nullable, rather than trying to override the nullability in the Optional handler. I think that should fix things, yes? In this case, we already have logic in the Rx handler for updating a computed nullness map for method references that should work just fine:

NullAway/nullaway/src/main/java/com/uber/nullaway/handlers/RxNullabilityPropagator.java

Lines 411 to 414 in 29dd90c

// We found our method, and it was non-null when called inside the filter, so we mark the // return of the // method reference as non-null here analysis.setComputedNullness(tree, Nullness.NONNULL);

hasNullableAnnotation doesn't depend on the library models. So should we make the conjuct to have library model value of annotation? Yeah, as you said. setComputedNullness will update the nullness in the case when it is non-null.