Merge 0d922df into 2cf8011

uber · Oct 23, 2018 · 56067cf · 56067cf
2 parents 2cf8011 + 0d922df
commit 56067cf
Show file tree

Hide file tree

Showing 8 changed files with 137 additions and 8 deletions.
diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -678,15 +678,14 @@ private Description checkOverriding(
       Symbol.MethodSymbol overridingMethod,
       @Nullable MemberReferenceTree memberReferenceTree,
       VisitorState state) {
-    // 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.
+    final boolean isOverridenMethodUnannotated =
+        NullabilityUtil.isUnannotated(overriddenMethod, config);
     boolean overriddenMethodReturnsNonNull =
-        !(NullabilityUtil.isUnannotated(overriddenMethod, config)
-            || Nullness.hasNullableAnnotation(overriddenMethod));
+        ((isOverridenMethodUnannotated
+                && handler.onUnannotatedInvocationGetExplicitlyNonNullReturn(
+                    overriddenMethod, false))
+            || (!isOverridenMethodUnannotated
+                && !Nullness.hasNullableAnnotation(overriddenMethod)));
     // if the super method returns nonnull,
     // overriding method better not return nullable
     if (overriddenMethodReturnsNonNull

diff --git a/nullaway/src/main/java/com/uber/nullaway/Nullness.java b/nullaway/src/main/java/com/uber/nullaway/Nullness.java
@@ -157,6 +157,16 @@ public static boolean isNonNullAnnotation(String annotName) {
     return annotName.endsWith(".NonNull") || annotName.endsWith(".NotNull");
   }
 
+  /**
+   * Does the symbol have a {@code @NonNull} declaration or type-use annotation?
+   *
+   * <p>NOTE: this method does not work for checking all annotations of parameters of methods from
+   * class files. For that case, use {@link #paramHasNullableAnnotation(Symbol.MethodSymbol, int)}
+   */
+  public static boolean hasNonNullAnnotation(Symbol symbol) {
+    return hasNonNullAnnotation(NullabilityUtil.getAllAnnotations(symbol));
+  }
+
   /**
    * Does the symbol have a {@code @Nullable} declaration or type-use annotation?
    *

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/BaseNoOpHandler.java
@@ -109,6 +109,13 @@ public ImmutableSet<Integer> onUnannotatedInvocationGetExplicitlyNullablePositio
     return explicitlyNullablePositions;
   }
 
+  @Override
+  public boolean onUnannotatedInvocationGetExplicitlyNonNullReturn(
+      Symbol.MethodSymbol methodSymbol, boolean explicitlyNonNullReturn) {
+    // NoOp
+    return explicitlyNonNullReturn;
+  }
+
   @Override
   public ImmutableSet<Integer> onUnannotatedInvocationGetNonNullPositions(
       NullAway analysis,

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/CompositeHandler.java
@@ -127,6 +127,17 @@ public ImmutableSet<Integer> onUnannotatedInvocationGetExplicitlyNullablePositio
     return explicitlyNullablePositions;
   }
 
+  @Override
+  public boolean onUnannotatedInvocationGetExplicitlyNonNullReturn(
+      Symbol.MethodSymbol methodSymbol, boolean explicitlyNonNullReturn) {
+    for (Handler h : handlers) {
+      explicitlyNonNullReturn =
+          h.onUnannotatedInvocationGetExplicitlyNonNullReturn(
+              methodSymbol, explicitlyNonNullReturn);
+    }
+    return explicitlyNonNullReturn;
+  }
+
   @Override
   public ImmutableSet<Integer> onUnannotatedInvocationGetNonNullPositions(
       NullAway analysis,

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/Handler.java
@@ -150,6 +150,21 @@ ImmutableSet<Integer> onUnannotatedInvocationGetExplicitlyNullablePositions(
       Symbol.MethodSymbol methodSymbol,
       ImmutableSet<Integer> explicitlyNullablePositions);
 
+  /**
+   * Called when NullAway encounters an unannotated method and asks if return value is explicitly
+   * marked @Nullable
+   *
+   * <p>Note that this should be only used for return values EXPLICLTY marked as @NonNull (e.g. by
+   * library models) rather than those considered @Nullable by NullAway's default optimistic
+   * assumptions.
+   *
+   * @param methodSymbol The method symbol for the unannotated method in question.
+   * @param explicitlyNonNullReturn return nullability computed by upstream handlers.
+   * @return Updated return nullability computed by this handler.
+   */
+  boolean onUnannotatedInvocationGetExplicitlyNonNullReturn(
+      Symbol.MethodSymbol methodSymbol, boolean explicitlyNonNullReturn);
+
   /**
    * Called when NullAway encounters an unannotated method and asks for params default nullability
    *

diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/RestrictiveAnnotationHandler.java
@@ -78,6 +78,28 @@ public boolean onOverrideMayBeNullExpr(
     return exprMayBeNull;
   }
 
+  @Override
+  public ImmutableSet<Integer> onUnannotatedInvocationGetExplicitlyNullablePositions(
+      NullAway analysis,
+      VisitorState state,
+      Symbol.MethodSymbol methodSymbol,
+      ImmutableSet<Integer> explicitlyNullablePositions) {
+    HashSet<Integer> positions = new HashSet<Integer>();
+    positions.addAll(explicitlyNullablePositions);
+    for (int i = 0; i < methodSymbol.getParameters().size(); ++i) {
+      if (Nullness.paramHasNullableAnnotation(methodSymbol, i)) {
+        positions.add(i);
+      }
+    }
+    return ImmutableSet.copyOf(positions);
+  }
+
+  @Override
+  public boolean onUnannotatedInvocationGetExplicitlyNonNullReturn(
+      Symbol.MethodSymbol methodSymbol, boolean explicitlyNonNullReturn) {
+    return Nullness.hasNonNullAnnotation(methodSymbol) || explicitlyNonNullReturn;
+  }
+
   @Override
   public NullnessHint onDataflowVisitMethodInvocation(
       MethodInvocationNode node,

diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java
@@ -1043,6 +1043,47 @@ public void restrictivelyAnnotatedMethodsWorkWithNullnessFromDataflow2() {
         .doTest();
   }
 
+  @Test
+  public void OverridingRestrictivelyAnnotatedMethod() {
+    compilationHelper
+        .setArgs(
+            Arrays.asList(
+                "-d",
+                temporaryFolder.getRoot().getAbsolutePath(),
+                "-XepOpt:NullAway:AnnotatedPackages=com.uber",
+                "-XepOpt:NullAway:UnannotatedSubPackages=com.uber.lib.unannotated",
+                "-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
+        .addSourceLines(
+            "Test.java",
+            "package com.uber;",
+            "import com.uber.lib.unannotated.RestrictivelyAnnotatedClass;",
+            "import org.checkerframework.checker.nullness.qual.NonNull;",
+            "import javax.annotation.Nullable;",
+            "public class Test extends RestrictivelyAnnotatedClass {",
+            "   Test(){ super(new Object()); }",
+            "   @Override public void acceptsNonNull(@Nullable Object o) { }",
+            "   @Override public void acceptsNonNull2(Object o) { }",
+            "   @Override public void acceptsNullable2(@Nullable Object o) { }",
+            "   @Override public Object returnsNonNull() { return new Object(); }",
+            "   @Override public Object returnsNullable() { return new Object(); }",
+            "   @Override public @Nullable Object returnsNullable2() { return new Object();}",
+            "}")
+        .addSourceLines(
+            "Test2.java",
+            "package com.uber;",
+            "import com.uber.lib.unannotated.RestrictivelyAnnotatedClass;",
+            "import org.checkerframework.checker.nullness.qual.NonNull;",
+            "import javax.annotation.Nullable;",
+            "public class Test2 extends RestrictivelyAnnotatedClass {",
+            "   Test2(){ super(new Object()); }",
+            "   // BUG: Diagnostic contains: parameter o is @NonNull",
+            "   public void acceptsNullable(Object o) { }",
+            "   // BUG: Diagnostic contains: method returns @Nullable",
+            "   public @Nullable Object returnsNonNull2() { return new Object(); }",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void testCastToNonNull() {
     compilationHelper

diff --git a/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedClass.java b/test-java-lib/src/main/java/com/uber/lib/unannotated/RestrictivelyAnnotatedClass.java
@@ -25,4 +25,28 @@ public static void consumesObjectNonNull(@NonNull Object o) {}
   public static void consumesObjectNotNull(@NotNull Object o) {}
 
   public static void consumesObjectUnannotated(Object o) {}
+
+  public void acceptsNonNull(@NonNull Object o) {}
+
+  public void acceptsNonNull2(@NonNull Object o) {}
+
+  public void acceptsNullable(@Nullable Object o) {}
+
+  public void acceptsNullable2(@Nullable Object o) {}
+
+  public @NonNull Object returnsNonNull() {
+    return new Object();
+  }
+
+  public @NonNull Object returnsNonNull2() {
+    return new Object();
+  }
+
+  public @Nullable Object returnsNullable() {
+    return null;
+  }
+
+  public @Nullable Object returnsNullable2() {
+    return null;
+  }
 }