diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java index f9914c0763..f01c524846 100644 --- 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 index 7d5222fbc0..e907f824e0 100644 --- 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? + * + *

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 index e3345a3734..c76f2f47c4 100644 --- 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 onUnannotatedInvocationGetExplicitlyNullablePositio return explicitlyNullablePositions; } + @Override + public boolean onUnannotatedInvocationGetExplicitlyNonNullReturn( + Symbol.MethodSymbol methodSymbol, boolean explicitlyNonNullReturn) { + // NoOp + return explicitlyNonNullReturn; + } + @Override public ImmutableSet 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 index 8276597ef4..91ce21d406 100644 --- 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 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 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 index 365d4319c6..e8c09dcaa2 100644 --- 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 onUnannotatedInvocationGetExplicitlyNullablePositions( Symbol.MethodSymbol methodSymbol, ImmutableSet explicitlyNullablePositions); + /** + * Called when NullAway encounters an unannotated method and asks if return value is explicitly + * marked @Nullable + * + *

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 index 4593c27a79..f148895d27 100644 --- 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 onUnannotatedInvocationGetExplicitlyNullablePositions( + NullAway analysis, + VisitorState state, + Symbol.MethodSymbol methodSymbol, + ImmutableSet explicitlyNullablePositions) { + HashSet positions = new HashSet(); + 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 index 13f67c8754..4a0be5a1d5 100644 --- 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 index 5d34a7f5c7..f455db3d6e 100644 --- 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; + } }