Merge be8f556 into f3be0a7

nullaway/src/main/java/com/uber/nullaway/LibraryModels.java

@@ -41,6 +41,15 @@ public interface LibraryModels {
    */
   ImmutableSetMultimap<MethodRef, Integer> failIfNullParameters();
 
+  /**
+   * @return map from the names of methods with @Nullable parameters to the indexes of the arguments
+   *     that are @Nullable.
+   *     <p>This is taken into account for override checks, requiring methods that override the
+   *     methods listed here to take @Nullable parameters on the same indexes. The main use for this
+   *     is to document which API callbacks can be passed null values.
+   */
+  ImmutableSetMultimap<MethodRef, Integer> nullableParameters();
+
   /**
    * @return map from the names of methods with @NonNull parameters to the indexes of the arguments
    *     that are @NonNull.

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

@@ -484,44 +484,57 @@ private Description checkParamOverriding(
     // for unbound member references, we need to adjust parameter indices by 1 when matching with
     // overridden method
     int startParam = unboundMemberRef ? 1 : 0;
-    for (int i = startParam; i < superParamSymbols.size(); i++) {
-      // we need to call paramHasNullableAnnotation here since overriddenMethod may be defined
-      // in a class file
-      if (Nullness.paramHasNullableAnnotation(overriddenMethod, i)) {
-        int methodParamInd = i - startParam;
-        VarSymbol paramSymbol = overridingParamSymbols.get(methodParamInd);
-        // in the case where we have a parameter of a lambda expression, we do
-        // *not* force the parameter to be annotated with @Nullable; instead we "inherit"
-        // nullability from the corresponding functional interface method.
-        // So, we report an error if the @Nullable annotation is missing *and*
-        // we don't have a lambda with implicitly typed parameters
-        boolean implicitlyTypedLambdaParam =
-            lambdaExpressionTree != null
-                && NullabilityUtil.lambdaParamIsImplicitlyTyped(
-                    lambdaExpressionTree.getParameters().get(methodParamInd));
-        if (!Nullness.hasNullableAnnotation(paramSymbol) && !implicitlyTypedLambdaParam) {
-          String message =
-              "parameter "
-                  + paramSymbol.name.toString()
-                  + (memberReferenceTree != null ? " of referenced method" : "")
-                  + " is @NonNull, but parameter in "
-                  + ((lambdaExpressionTree != null || memberReferenceTree != null)
-                      ? "functional interface "
-                      : "superclass ")
-                  + "method "
-                  + ASTHelpers.enclosingClass(overriddenMethod)
-                  + "."
-                  + overriddenMethod.toString()
-                  + " is @Nullable";
-          Tree errorTree;
-          if (memberReferenceTree != null) {
-            errorTree = memberReferenceTree;
-          } else {
-            errorTree = getTreesInstance(state).getTree(paramSymbol);
-          }
-          return createErrorDescription(
-              MessageTypes.WRONG_OVERRIDE_PARAM, errorTree, message, errorTree);
+    // Collect @Nullable params of overriden method
+    ImmutableSet<Integer> nullableParamsOfOverriden;
+    if (NullabilityUtil.isUnannotated(overriddenMethod, config)) {
+      nullableParamsOfOverriden =
+          handler.onUnannotatedInvocationGetExplicitlyNullablePositions(
+              this, state, overriddenMethod, ImmutableSet.of());
+    } else {
+      ImmutableSet.Builder<Integer> builder = ImmutableSet.builder();
+      for (int i = startParam; i < superParamSymbols.size(); i++) {
+        // we need to call paramHasNullableAnnotation here since overriddenMethod may be defined
+        // in a class file
+        if (Nullness.paramHasNullableAnnotation(overriddenMethod, i)) {
+          builder.add(i);
+        }
+      }
+      nullableParamsOfOverriden = builder.build();
+    }
+    for (int i : nullableParamsOfOverriden) {
+      int methodParamInd = i - startParam;
+      VarSymbol paramSymbol = overridingParamSymbols.get(methodParamInd);
+      // in the case where we have a parameter of a lambda expression, we do
+      // *not* force the parameter to be annotated with @Nullable; instead we "inherit"
+      // nullability from the corresponding functional interface method.
+      // So, we report an error if the @Nullable annotation is missing *and*
+      // we don't have a lambda with implicitly typed parameters
+      boolean implicitlyTypedLambdaParam =
+          lambdaExpressionTree != null
+              && NullabilityUtil.lambdaParamIsImplicitlyTyped(
+                  lambdaExpressionTree.getParameters().get(methodParamInd));
+      if (!Nullness.hasNullableAnnotation(paramSymbol) && !implicitlyTypedLambdaParam) {
+        String message =
+            "parameter "
+                + paramSymbol.name.toString()
+                + (memberReferenceTree != null ? " of referenced method" : "")
+                + " is @NonNull, but parameter in "
+                + ((lambdaExpressionTree != null || memberReferenceTree != null)
+                    ? "functional interface "
+                    : "superclass ")
+                + "method "
+                + ASTHelpers.enclosingClass(overriddenMethod)
+                + "."
+                + overriddenMethod.toString()
+                + " is @Nullable";
+        Tree errorTree;
+        if (memberReferenceTree != null) {
+          errorTree = memberReferenceTree;
+        } else {
+          errorTree = getTreesInstance(state).getTree(paramSymbol);
         }
+        return createErrorDescription(
+            MessageTypes.WRONG_OVERRIDE_PARAM, errorTree, message, errorTree);
       }
     }
     return Description.NO_MATCH;
@@ -611,12 +624,18 @@ private Description checkOverriding(
       Symbol.MethodSymbol overridingMethod,
       @Nullable MemberReferenceTree memberReferenceTree,
       VisitorState state) {
-    if (NullabilityUtil.isUnannotated(overriddenMethod, config)) {
-      return Description.NO_MATCH;
-    }
+    // We ignore unannotated methods for now, since we always assume they return @NonNull, but we
+    // don't actually know,
+    // so it might make sense to override them as @Nullable.
+    // TODO: Add an equivalent to Handler.onUnannotatedInvocationGetExplicitlyNullablePositions for
+    // @NonNull return
+    // values if needed.
+    boolean overriddenMethodReturnsNonNull =
+        !(NullabilityUtil.isUnannotated(overriddenMethod, config)
+            || Nullness.hasNullableAnnotation(overriddenMethod));
     // if the super method returns nonnull,
     // overriding method better not return nullable
-    if (!Nullness.hasNullableAnnotation(overriddenMethod)
+    if (overriddenMethodReturnsNonNull
         && Nullness.hasNullableAnnotation(overridingMethod)
         && getComputedNullness(memberReferenceTree).equals(Nullness.NULLABLE)) {
       String message;

nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java

@@ -100,13 +100,24 @@ public void onMatchReturn(NullAway analysis, ReturnTree tree, VisitorState state
     // NoOp
   }
 
+  @Override
+  public ImmutableSet<Integer> onUnannotatedInvocationGetExplicitlyNullablePositions(
+      NullAway analysis,
+      VisitorState state,
+      Symbol.MethodSymbol methodSymbol,
+      ImmutableSet<Integer> explicitlyNullablePositions) {
+    // NoOp
+    return explicitlyNullablePositions;
+  }
+
   @Override
   public ImmutableSet<Integer> onUnannotatedInvocationGetNonNullPositions(
       NullAway analysis,
       VisitorState state,
       Symbol.MethodSymbol methodSymbol,
       List<? extends ExpressionTree> actualParams,
       ImmutableSet<Integer> nonNullPositions) {
+    // NoOp
     return nonNullPositions;
   }
 

nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java

@@ -114,6 +114,20 @@ public void onMatchReturn(NullAway analysis, ReturnTree tree, VisitorState state
     }
   }
 
+  @Override
+  public ImmutableSet<Integer> onUnannotatedInvocationGetExplicitlyNullablePositions(
+      NullAway analysis,
+      VisitorState state,
+      Symbol.MethodSymbol methodSymbol,
+      ImmutableSet<Integer> explicitlyNullablePositions) {
+    for (Handler h : handlers) {
+      explicitlyNullablePositions =
+          h.onUnannotatedInvocationGetExplicitlyNullablePositions(
+              analysis, state, methodSymbol, explicitlyNullablePositions);
+    }
+    return explicitlyNullablePositions;
+  }
+
   @Override
   public ImmutableSet<Integer> onUnannotatedInvocationGetNonNullPositions(
       NullAway analysis,

nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java

@@ -126,6 +126,30 @@ void onMatchMethodReference(
    */
   void onMatchReturn(NullAway analysis, ReturnTree tree, VisitorState state);
 
+  /**
+   * Called when NullAway encounters an unannotated method and asks for params explicitly
+   * marked @Nullable
+   *
+   * <p>This is mostly used to force overriding methods to also use @Nullable, e.g. for callbacks
+   * that can get passed null values.
+   *
+   * <p>Note that this should be only used for parameters EXPLICLTY marked as @Nullable (e.g. by
+   * library models) rather than those considered @Nullable by NullAway's default optimistic
+   * assumptions.
+   *
+   * @param analysis A reference to the running NullAway analysis.
+   * @param state The current visitor state.
+   * @param methodSymbol The method symbol for the unannotated method in question.
+   * @param explicitlyNullablePositions Parameter nullability computed by upstream handlers (the
+   *     core analysis supplies the empty set to the first handler in the chain).
+   * @return Updated parameter nullability computed by this handler.
+   */
+  ImmutableSet<Integer> onUnannotatedInvocationGetExplicitlyNullablePositions(
+      NullAway analysis,
+      VisitorState state,
+      Symbol.MethodSymbol methodSymbol,
+      ImmutableSet<Integer> explicitlyNullablePositions);
+
   /**
    * Called when NullAway encounters an unannotated method and asks for params default nullability
    *

nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java

@@ -78,6 +78,20 @@ public ImmutableSet<Integer> onUnannotatedInvocationGetNonNullPositions(
         .immutableCopy();
   }
 
+  @Override
+  public ImmutableSet<Integer> onUnannotatedInvocationGetExplicitlyNullablePositions(
+      NullAway analysis,
+      VisitorState state,
+      Symbol.MethodSymbol methodSymbol,
+      ImmutableSet<Integer> explicitlyNullablePositions) {
+    return Sets.union(
+            explicitlyNullablePositions,
+            libraryModels
+                .nullableParameters()
+                .get(LibraryModels.MethodRef.fromSymbol(methodSymbol)))
+        .immutableCopy();
+  }
+
   @Override
   public boolean onOverrideMayBeNullExpr(
       NullAway analysis, ExpressionTree expr, VisitorState state, boolean exprMayBeNull) {
@@ -186,17 +200,26 @@ private static class DefaultLibraryModels implements LibraryModels {
                 1)
             .build();
 
+    private static final ImmutableSetMultimap<MethodRef, Integer> NULLABLE_PARAMETERS =
+        new ImmutableSetMultimap.Builder<MethodRef, Integer>()
+            .put(
+                methodRef(
+                    "android.view.GestureDetector.OnGestureListener",
+                    "onScroll(android.view.MotionEvent,android.view.MotionEvent,float,float)"),
+                0)
+            .build();
+
     private static final ImmutableSetMultimap<MethodRef, Integer> NON_NULL_PARAMETERS =
         new ImmutableSetMultimap.Builder<MethodRef, Integer>()
             .put(
                 methodRef(
                     "com.android.sdklib.build.ApkBuilder",
-                    "ApkBuilder(java.io.File,java.io.File,java.io.File,java.lang.String,java.io.PrintStream))"),
+                    "ApkBuilder(java.io.File,java.io.File,java.io.File,java.lang.String,java.io.PrintStream)"),
                 0)
             .put(
                 methodRef(
                     "com.android.sdklib.build.ApkBuilder",
-                    "ApkBuilder(java.io.File,java.io.File,java.io.File,java.lang.String,java.io.PrintStream))"),
+                    "ApkBuilder(java.io.File,java.io.File,java.io.File,java.lang.String,java.io.PrintStream)"),
                 1)
             .put(methodRef("com.google.common.collect.ImmutableList.Builder", "add(E)"), 0)
             .put(
@@ -330,6 +353,11 @@ public ImmutableSetMultimap<MethodRef, Integer> failIfNullParameters() {
       return FAIL_IF_NULL_PARAMETERS;
     }
 
+    @Override
+    public ImmutableSetMultimap<MethodRef, Integer> nullableParameters() {
+      return NULLABLE_PARAMETERS;
+    }
+
     @Override
     public ImmutableSetMultimap<MethodRef, Integer> nonNullParameters() {
       return NON_NULL_PARAMETERS;
@@ -355,6 +383,8 @@ private static class CombinedLibraryModels implements LibraryModels {
 
     private final ImmutableSetMultimap<MethodRef, Integer> failIfNullParameters;
 
+    private final ImmutableSetMultimap<MethodRef, Integer> nullableParameters;
+
     private final ImmutableSetMultimap<MethodRef, Integer> nonNullParameters;
 
     private final ImmutableSetMultimap<MethodRef, Integer> nullImpliesTrueParameters;
@@ -366,6 +396,8 @@ private static class CombinedLibraryModels implements LibraryModels {
     public CombinedLibraryModels(Iterable<LibraryModels> models) {
       ImmutableSetMultimap.Builder<MethodRef, Integer> failIfNullParametersBuilder =
           new ImmutableSetMultimap.Builder<>();
+      ImmutableSetMultimap.Builder<MethodRef, Integer> nullableParametersBuilder =
+          new ImmutableSetMultimap.Builder<>();
       ImmutableSetMultimap.Builder<MethodRef, Integer> nonNullParametersBuilder =
           new ImmutableSetMultimap.Builder<>();
       ImmutableSetMultimap.Builder<MethodRef, Integer> nullImpliesTrueParametersBuilder =
@@ -376,6 +408,9 @@ public CombinedLibraryModels(Iterable<LibraryModels> models) {
         for (Map.Entry<MethodRef, Integer> entry : libraryModels.failIfNullParameters().entries()) {
           failIfNullParametersBuilder.put(entry);
         }
+        for (Map.Entry<MethodRef, Integer> entry : libraryModels.nullableParameters().entries()) {
+          nullableParametersBuilder.put(entry);
+        }
         for (Map.Entry<MethodRef, Integer> entry : libraryModels.nonNullParameters().entries()) {
           nonNullParametersBuilder.put(entry);
         }
@@ -391,6 +426,7 @@ public CombinedLibraryModels(Iterable<LibraryModels> models) {
         }
       }
       failIfNullParameters = failIfNullParametersBuilder.build();
+      nullableParameters = nullableParametersBuilder.build();
       nonNullParameters = nonNullParametersBuilder.build();
       nullImpliesTrueParameters = nullImpliesTrueParametersBuilder.build();
       nullableReturns = nullableReturnsBuilder.build();
@@ -402,6 +438,11 @@ public ImmutableSetMultimap<MethodRef, Integer> failIfNullParameters() {
       return failIfNullParameters;
     }
 
+    @Override
+    public ImmutableSetMultimap<MethodRef, Integer> nullableParameters() {
+      return nullableParameters;
+    }
+
     @Override
     public ImmutableSetMultimap<MethodRef, Integer> nonNullParameters() {
       return nonNullParameters;

nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java

@@ -1143,6 +1143,48 @@ public void invokeNativeFromInitializer() {
             "  }",
             "}")
         .doTest();
-    ;
+  }
+
+  @Test
+  public void overrideFailsOnExplicitlyNullableLibraryModelParam() {
+    compilationHelper
+        .addSourceLines( // Dummy android.view.GestureDetector.OnGestureListener interface
+            "GestureDetector.java",
+            "package android.view;",
+            "public class GestureDetector {",
+            "  public static interface OnGestureListener {",
+            // Ignore other methods for this test, to make code shorter on both files:
+            "    boolean onScroll(MotionEvent me1, MotionEvent me2, float f1, float f2);",
+            "  }",
+            "}")
+        .addSourceLines( // Dummy android.view.MotionEvent class
+            "MotionEvent.java", "package android.view;", "public class MotionEvent { }")
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import android.view.GestureDetector;",
+            "import android.view.MotionEvent;",
+            "class Test implements GestureDetector.OnGestureListener {",
+            "  Test() {  }",
+            "  @Override",
+            "  // BUG: Diagnostic contains: parameter me1 is @NonNull",
+            "  public boolean onScroll(MotionEvent me1, MotionEvent me2, float f1, float f2) {",
+            "    return false; // NoOp",
+            "  }",
+            "}")
+        .addSourceLines(
+            "Test2.java",
+            "package com.uber;",
+            "import javax.annotation.Nullable;",
+            "import android.view.GestureDetector;",
+            "import android.view.MotionEvent;",
+            "class Test2 implements GestureDetector.OnGestureListener {",
+            "  Test2() {  }",
+            "  @Override",
+            "  public boolean onScroll(@Nullable MotionEvent me1, MotionEvent me2, float f1, float f2) {",
+            "    return false; // NoOp",
+            "  }",
+            "}")
+        .doTest();
   }
 }

sample-library-model/src/main/java/com/uber/modelexample/ExampleLibraryModels.java

@@ -30,6 +30,11 @@ public ImmutableSetMultimap<MethodRef, Integer> failIfNullParameters() {
     return ImmutableSetMultimap.of();
   }
 
+  @Override
+  public ImmutableSetMultimap<MethodRef, Integer> nullableParameters() {
+    return ImmutableSetMultimap.of();
+  }
+
   @Override
   public ImmutableSetMultimap<MethodRef, Integer> nonNullParameters() {
     return ImmutableSetMultimap.of();