JSpecify: handle incorrect method parameter nullability for method reference

[akulk022]

We now report an error for the following test case:

class Test {
    interface A<T1 extends @Nullable Object> {
        String function(T1 o);
    }
    static String foo(Object o) {
        return o.toString();
    }
    static void testPositive() {
        // we now report an error here, as foo's parameter needs to be @Nullable
        A<@Nullable Object> p = Test::foo;
    }
    static void testNegative() {
        A<Object> p = Test::foo;
    }
}

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2d2b829) 86.84% compared to head (6092a56) 86.85%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #845      +/-   ##
============================================
+ Coverage     86.84%   86.85%   +0.01%     
- Complexity     1878     1879       +1     
============================================
  Files            74       74              
  Lines          6189     6194       +5     
  Branches       1201     1201              
============================================
+ Hits           5375     5380       +5     
  Misses          406      406              
  Partials        408      408              

Files	Coverage Δ
...rc/main/java/com/uber/nullaway/GenericsChecks.java	90.22% <100.00%> (ø)
...away/src/main/java/com/uber/nullaway/NullAway.java	89.73% <100.00%> (+0.04%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[msridhar]

Very excited that it looks like this works! In the spirit of small PRs, let's get this cleaned up and land it. We can do a separate PR for handling return types of method references.

[msridhar]

Cool that this works! But it's a bit unclean to pass in a MemberReferenceTree directly. Instead, how about we have a parameter @Nullable Type enclosingType, and at the caller you pass in the type of the MemberReferenceTree if the tree is non-null. Then in this method, if enclosingType == null, we set it to be getTypeForSymbol(enclosingSymbol, state).

[msridhar]

We will also need to update the Javadoc for this method to document the new parameter.

[msridhar]

Actually, I have a better idea. Just below this method (line 909), there is another overload of getGenericMethodParameterNullness() that takes a Type rather than a Symbol. How about we not modify this method, and instead at the call site just invoke the other method if we have a MemberReferenceTree? Right now we'll have to do a null check on the Type at the caller, but I am also open to making the parameter of the other method @Nullable and doing the check there.

[akulk022]

if we have a MemberReferenceTree!=null, it's type will also not be null right? if I understand correctly is this what you're suggesting?, the place where we call the method from NullAway.java (line 734) to be something like:

if (isOverriddenMethodAnnotated) {
      for (int i = 0; i < superParamSymbols.size(); i++) {
        overriddenMethodArgNullnessMap[i] =
            Nullness.paramHasNullableAnnotation(overriddenMethod, i, config)
                ? Nullness.NULLABLE
                : (config.isJSpecifyMode()
                    ? (memberReferenceTree != null)
                        ? GenericsChecks.getGenericMethodParameterNullness(
                            i,
                            overriddenMethod,
                            ASTHelpers.getType(memberReferenceTree),
                            state,
                            config)
                        : GenericsChecks.getGenericMethodParameterNullness(
                            i,
                            overriddenMethod,
                            overridingParamSymbols.get(i).owner.owner,
                            state,
                            config)
                    : Nullness.NONNULL);
      }
    }

This does make that line very complicated but then we don't have to change any method signatures. Maybe I misunderstood and what you're suggesting is even simpler

[msridhar]

Right now, the full error message we emit here is:

warning: [NullAway] parameter o of referenced method is @NonNull, but parameter in functional interface method com.uber.Test.A.function(T1) is @Nullable

It'd be nice if we could find a way to print the functional interface method type so it refers to the annotated type arguments rather than the T1 type variable.

[akulk022]

I agree, I'll try and find a way to do that

[msridhar]

After further investigation getting better error messages needs more thought. I'll file a follow-up issue but let's not hold up this PR on this.

[msridhar]

I have done some further cleanup here and I think this is ready to land.

[lazaroclapp]

LGTM!